Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MSC2264: Add an unstable feature flag to MSC2140 for clients to detect support #2264

Merged
merged 2 commits into from
Sep 4, 2019
Merged

MSC2264: Add an unstable feature flag to MSC2140 for clients to detect support #2264

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
16bb3bd
Add an unstable feature flag to MSC2140 for clients to detect support
turt2live Aug 29, 2019
f8780e2
add note about edit
turt2live Aug 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion proposals/2140-terms-of-service-2.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ This endpoint does *not* require authentication.

#### `POST $prefix/terms`:
Requests to this endpoint have a single key, `user_accepts` whose value is
a list of URLs (given by the `url` field in the GET response) of documents that
a list of URLs (given by the `url` field in the GET response) of documents that
the user has agreed to:

```json
Expand Down Expand Up @@ -277,6 +277,16 @@ Clients may add IS bindings for 3PIDs that already exist on the user's
Homeserver account by using the `POST /_matrix/client/r0/account/3pid`
to re-add the 3PID.

### Unstable feature flag for transition

In order to allow client implementations to determine if the homeserver they are developed
against supports `id_access_token`, an unstable feature flag of `m.id_access_token`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure you've thought about this and I have no real desire to get involved with the bikeshed-painting, but it might be useful to record the rationale for choosing m.id_access_token rather than (say) m.authenticated_is_api or m.is_api_v2?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've thought about it no more than copying the flag out of a synapse PR. I don't really know the rationale for picking the flag we did, but I also don't want this to be a bikeshed problem. The path of least resistance was very much abused here.

I'd definitely be in favour of m.identity_api_v2 or similar to avoid is.

Copy link
Member

@richvdh richvdh Aug 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure you've thought about this

"you" = "someone on the privacy project

is to be added to `/versions`. When the flag is `false` or not present, clients must assume
that the homeserver does not support being given `id_access_token` and may receive an error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aiui it's not really the fact that the HS supports being given the token (it would just ignore it if not) but more that it supports passing the token on to the IS?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Synapse makes angry noises when you supply id_access_token when it doesn't want one. But yes, that's another way to say it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does it do that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it probably shouldn't, but it's not exactly in the wrong here. Matrix doesn't say that a client can send unknown fields to endpoints, but it also doesn't say you can't. Synapse in this case I'd count as compliant, albeit annoying.

I can add some words about the flag being about supporting the field though, if wanted.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the field is in the body of a json post? I think it's fairly key to the extensibility of the protocol that servers don't complain about spurious fields. Anyway, this is OT here, I guess: I'm not really looking for a change if what it says is accurate currently.

for doing so. Clients are expected to use the supported specification versions the homeserver
advertises instead of the feature flag's presence once this proposal is included in a release
of the specification.

## Tradeoffs

The Identity Service API previously did not require authentication, and OpenID
Expand Down