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.

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

MSC4026: Allow /versions to optionally accept authentication #4026

Merged
merged 4 commits into from
Sep 5, 2023

Conversation

H-Shay
Copy link
Contributor

@H-Shay H-Shay commented Jun 6, 2023

A proposal for _matrix/client/versions to 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

@H-Shay H-Shay marked this pull request as draft June 6, 2023 22:18
@H-Shay H-Shay changed the title MSCXXX: Allow /versions to optionally accept authentication MSC4026: Allow /versions to optionally accept authentication Jun 6, 2023
@H-Shay H-Shay marked this pull request as ready for review June 6, 2023 22:35
@turt2live turt2live added proposal A matrix spec change proposal client-server Client-Server API kind:maintenance MSC which clarifies/updates existing spec needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels Jun 6, 2023
@H-Shay H-Shay self-assigned this Jun 8, 2023
Comment on lines +23 to +24
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.
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.

@ara4n
Copy link
Member

ara4n commented Jul 28, 2023

lgtm!

@erikjohnston
Copy link
Member

Since nobody thinks this is a bad idea......

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Aug 17, 2023

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 mscbot added proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. disposition-merge labels Aug 17, 2023
@turt2live turt2live removed the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Aug 21, 2023
## 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).
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.

proposals/4026-optional-authed-versions.md Show resolved Hide resolved
H-Shay and others added 2 commits August 23, 2023 14:01
Co-authored-by: Hubert Chathi <hubert@uhoreg.ca>
Co-authored-by: Hubert Chathi <hubert@uhoreg.ca>
@richvdh richvdh self-requested a review August 30, 2023 17:25
@mscbot
Copy link
Collaborator

mscbot commented Aug 31, 2023

🔔 This is now entering its final comment period, as per the review above. 🔔

@mscbot mscbot added final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. and removed proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels Aug 31, 2023
Comment on lines +22 to +23
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.
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.

@benkuly
Copy link
Contributor

benkuly commented Sep 1, 2023

This would break server discovery, would't it?

From the current spec:

Clients SHOULD validate that the URL points to a valid homeserver before accepting it by connecting to the /_matrix/client/versions endpoint, ensuring that it does not return an error, and parsing and validating that the data conforms with the expected response format. If any step in the validation fails, then FAIL_ERROR.

@anoadragon453
Copy link
Member

@benkuly This MSC proposes optional authentication for this endpoint. Clients can still request /_matrix/client/versions without authentication to complete a server discovery check.

Please use a pull request comment when commenting on MSCs, to allow for collapsible threads of discussion.

@mscbot
Copy link
Collaborator

mscbot commented Sep 5, 2023

The final comment period, with a disposition to merge, as per the review above, is now complete.

@mscbot mscbot added finished-final-comment-period and removed disposition-merge final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. labels Sep 5, 2023
@richvdh richvdh merged commit 294b051 into main Sep 5, 2023
1 check passed
@clokep clokep deleted the shay/opt_auth_versions branch September 5, 2023 13:45
@turt2live turt2live added spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec and removed finished-final-comment-period labels Sep 12, 2023
dbkr added a commit to matrix-org/matrix-js-sdk that referenced this pull request Dec 18, 2023
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.
github-merge-queue bot pushed a commit to matrix-org/matrix-js-sdk that referenced this pull request Dec 19, 2023
* 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
@turt2live
Copy link
Member

Spec PR: matrix-org/matrix-spec#1728

@turt2live turt2live added spec-pr-in-review A proposal which has been PR'd against the spec and is in review and removed spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec labels Feb 23, 2024
@uhoreg uhoreg added merged A proposal whose PR has merged into the spec! and removed spec-pr-in-review A proposal which has been PR'd against the spec and is in review labels Mar 7, 2024
@uhoreg
Copy link
Member

uhoreg commented Mar 7, 2024

Merged! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API kind:maintenance MSC which clarifies/updates existing spec merged A proposal whose PR has merged into the spec! proposal A matrix spec change proposal
Projects
Status: Done to some definition
Status: Done for now
Development

Successfully merging this pull request may close these issues.