MSC4026: Allow /versions to optionally accept authentication
#4026
Conversation
H-Shay
changed the title
/versions to optionally accept authentication/versions to optionally accept authentication
turt2live
added
proposal
| 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. |
There was a problem hiding this comment.
If the feature is only enabled for certain users, it seems like the unauthenticated version should just indicate no support for the given feature.
|
lgtm! |
|
Since nobody thinks this is a bad idea...... @mscbot fcp merge |
|
Team member @erikjohnston has proposed to merge this. The next step is review by the rest of the tagged people: Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for information about what commands tagged team members can give me. |
mscbot
added
proposed-final-comment-period
turt2live
removed
the
needs-implementation
| ## Introduction | ||
|
|
||
| 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). |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
yea, for that particular case an unstable and stable capability pairing would indeed be better.
There was a problem hiding this comment.
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").
There was a problem hiding this comment.
Ah, OK. That makes sense. Thanks for the clarification.
Can this be clarified in the MSC as well? Maybe something like:
| 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. |
There was a problem hiding this comment.
Sure, I have updated to reflect the suggestion.
Co-authored-by: Hubert Chathi <hubert@uhoreg.ca>
Co-authored-by: Hubert Chathi <hubert@uhoreg.ca>
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
mscbot
added
final-comment-period
| 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. |
There was a problem hiding this comment.
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.
|
This would break server discovery, would't it? From the current spec:
|
|
@benkuly This MSC proposes optional authentication for this endpoint. Clients can still request Please use a pull request comment when commenting on MSCs, to allow for collapsible threads of discussion. |
|
The final comment period, with a disposition to merge, as per the review above, is now complete. |
mscbot
added
finished-final-comment-period
and removed
disposition-merge
final-comment-period
turt2live
added
spec-pr-missing
turt2live
added this to Needs idea feedback / initial review
in Spec Core Team Backlog
via automation
turt2live
moved this from Needs idea feedback / initial review
to Requires spec writing
in Spec Core Team Backlog
Implements [MSC4026](matrix-org/matrix-spec-proposals#4026). I believe this probably is as simple as this: it will mean that the versions response can obviously change after logging in, but since the client is constructed again with an access token, this should just work (?) A remaining question is whether this needs to be optional. Opening the PR to prompt the discussion. Apps might not expect it, but it's just the same auth that we're sending to other endpoints on the same server.
* Send authenticated /versions request Implements [MSC4026](matrix-org/matrix-spec-proposals#4026). I believe this probably is as simple as this: it will mean that the versions response can obviously change after logging in, but since the client is constructed again with an access token, this should just work (?) A remaining question is whether this needs to be optional. Opening the PR to prompt the discussion. Apps might not expect it, but it's just the same auth that we're sending to other endpoints on the same server. * Fix tests * Clear /versions cache on access token set
|
Spec PR: matrix-org/matrix-spec#1728 |
turt2live
added
spec-pr-in-review
turt2live
moved this from Requires spec writing
to Requires spec PR review
in Spec Core Team Backlog
uhoreg
added
merged
|
Merged! 🎉 |
turt2live
moved this from Requires spec PR review
to Done to some definition
in Spec Core Team Backlog
A proposal for
_matrix/client/versionsto optionally accept authentication in order to facilitate advertising to clients experimental features which are enabled per-user.Rendered
Related: matrix-org/matrix-spec#1532
FCP tickyboxes