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

Jump to bottom

MSC4026: Allow /versions to optionally accept authentication #4026

Merged
merged 4 commits into from
Sep 5, 2023
Merged

MSC4026: Allow /versions to optionally accept authentication #4026

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7fb0734
add proposal to optionally auth `/versions`
H-Shay Jun 6, 2023
fbda588
update file name with actual number
H-Shay Jun 6, 2023
9e998a8
Update proposals/4026-optional-authed-versions.md
H-Shay Aug 23, 2023
8373784
Update proposals/4026-optional-authed-versions.md
H-Shay Aug 23, 2023
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
39 changes: 39 additions & 0 deletions proposals/4026-optional-authed-versions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# MSC4026: Allow /versions to optionally accept authentication

## Introduction
H-Shay marked this conversation as resolved.
Show resolved Hide resolved

Synapse is implementing the ability to turn some unstable features on per-user. Once this is
implemented, certain experimental features will be available to be enabled per-user via the [Admin API](https://matrix-org.github.io/synapse/latest/usage/administration/admin_api/index.html).
Copy link
Member

Choose a reason for hiding this comment

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

While I don't object, in principle, to making /versions optionally accept authentication, I'm a bit concerned about the proposed usage. If the usage is only for experimental features, how would clients detect support for stable features? I'd expect that the answer to that would be through the /capabilities endpoint which seems kind of strange to use two completely different endpoints to detect whether a client can do something. That means that when the client switches between unstable and stable implementations (or wants to support both), then it need to call two different endpoints.

I'd be more in favour of relaxing the restriction on /capabilities and distinguishing between server support for a feature and whether a given client is allowed to make use of the feature: /capabilities should not be used to indicate server support for an unstable feature (which should still be indicated using /versions), but may be used to indicate whether a client can use it.

Copy link
Member

Choose a reason for hiding this comment

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

Using /capabilities for the awkward accepted-but-not-merged state might be appropriate (though clients are already calling /versions to determine feature support), but for truly stable (ie: merged) features I strongly prefer we don't have flags at all. Feature support is ultimately determined by the spec version.

If the concern is the awkward accepted-but-not-merged state, I feel that spec releases happen often enough these days where we can stop saying things are stable upon FCP acceptance. In practice, both clients and servers are taking more than 1 release cycle to get any given release implemented anyways, so it does not feel like it's materially helping to have stable features so early in the process.

Copy link
Member

Choose a reason for hiding this comment

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

In practice, both clients and servers are taking more than 1 release cycle to get any given release implemented anyways, so it does not feel like it's materially helping to have stable features so early in the process.

Isn't this why it is important to have stable features before the spec version though? Since clients/servers aren't keeping up with the spec versions, but want to be able to use new features quickly.

Copy link
Member

Choose a reason for hiding this comment

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

They don't appear to be using those new features earlier than release, or at least they can't if the support is 1 release cycle behind.

Copy link
Member

Choose a reason for hiding this comment

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

but for truly stable (ie: merged) features I strongly prefer we don't have flags at all. Feature support is ultimately determined by the spec version.

But the point of this MSC is for selectively enabling/disabling a feature for certain users, not about whether the server has any support for a feature, and it's not about stable-but-not-merged features.

If a feature is being selectively enabled for certain users while it is unstable, my assumption is that it will continue to offer that feature only to certain users after it is stable/merged. So clients will need some way of determining whether they have access to the feature. If they just look at /versions, they can see if the server supports the given spec version, and know whether the server supports the feature, but they'd need to find out whether they have access, presumably by asking /capabilities. And if clients will be using /capabilities in the stable version, then it seems strange to tell them to use something else in the unstable version.

Copy link
Member

Choose a reason for hiding this comment

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

yea, for that particular case an unstable and stable capability pairing would indeed be better.

Copy link
Member

Choose a reason for hiding this comment

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

If a feature is being selectively enabled for certain users while it is unstable, my assumption is that it will continue to offer that feature only to certain users after it is stable/merged.

That was not the motivation behind this MSC TBH. The reason we want to selectively enable unstable features for certain users is so that we can test these unstable features with real accounts without risking other users finding it and relying on those features (i.e. we want to be able to enable stuff on matrix.org for certain users without risking the unstable features becoming "defacto stable").

Copy link
Member

Choose a reason for hiding this comment

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

Ah, OK. That makes sense. Thanks for the clarification.

Can this be clarified in the MSC as well? Maybe something like:

Suggested change
implemented, certain experimental features will be available to be enabled per-user via the [Admin API](https://matrix-org.github.io/synapse/latest/usage/administration/admin_api/index.html).
implemented, certain experimental features will be available to be enabled per-user via the [Admin API](https://matrix-org.github.io/synapse/latest/usage/administration/admin_api/index.html).
The intention is to allow certain users to test the experimental feature without making it available to
all users before it is stable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I have updated to reflect the suggestion.

The intention is to allow certain users to test the experimental feature without making it available to
all users before it is stable.
This is in addition to the current ability to toggle on/off those features system-wide in the configuration.

However, this poses a problem when considering how to advertise that those features are enabled to clients.
Traditionally, to determine what unstable features were available from a server clients checked the [`/_matrix/client/versions`](https://spec.matrix.org/v1.7/client-server-api/#get_matrixclientversions)
endpoint, which in turn checked the Synapse configuration to determine what experimental features were enabled. With the
changes being implemented this is no longer possible, as some experimental features may be enabled per-user. As the
`/_matrix/client/versions` endpoint does not require authentication there is no way to know which experimental features
are enabled - there is no access token that we can extract user info from to determine which unstable features are
currently enabled (as they may only be enabled for some users) - and thus there is no way to correctly communicate to
clients which experimental features are enabled.

## Proposal

The proposal to remedy this is to make `/_matrix/client/versions` optionally accept authentication, and ask clients
to use the authenticated version when determining which experimental features are enabled.
Comment on lines +22 to +23
Copy link
Member

Choose a reason for hiding this comment

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

For clarity, nobody should walk away from this thinking that the correct solution is to not advertise any unstable features to unauthenticated users. Unstable features that affect clients before they are logged in, and older clients that will not know to authenticate to this endpoint, are still valid.


## Potential issues

This does raise the question of what the non-authenticated version of `/_matrix/client/versions` should return with
regard to unstable features if the proposal is accepted.
Comment on lines +27 to +28
Copy link
Contributor

Choose a reason for hiding this comment

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

If the feature is only enabled for certain users, it seems like the unauthenticated version should just indicate no support for the given feature.


## Alternatives

An alternative to the proposal would be to move advertising the unstable features to the [`/_matrix/client/v3/capabilities`](https://spec.matrix.org/v1.7/client-server-api/#get_matrixclientv3capabilities)
endpoint, which does require authentication. However, the spec is clear that `/_matrix/client/v3/capabilities` "should
not be used to advertise unstable or experimental features - this is better done by the `/versions` endpoint." Thus,
this seems like a less desirable option than the proposed solution.

## Security considerations

None that I am currently aware of.
Loading