-
Notifications
You must be signed in to change notification settings - Fork 388
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
MSC2964: Usage of OAuth 2.0 authorization code grant and refresh token grant #2964
base: main
Are you sure you want to change the base?
Conversation
Related issue: matrix-org/matrix-spec#636 Related MSCs: #2965, #2966, #2967 In this MSC, there are a few areas that still need work. First, it outlines different profiles of clients. One important client type that is not yet covered by it are CLI tools. Second, there are the parts that we enforce. Third there is device handling in general. MSC2967 talks a bit about this, but there will definitely be some changes to the device API. |
Co-authored-by: Dominik Henneke <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nitpicky things things I noticed while reading through the updated version and suggested fixes
Co-authored-by: Tonkku <[email protected]>
e25fd17
to
c57be5e
Compare
|
||
#### Authorization request | ||
|
||
It then constructs the authorization request URL using the `authorization_endpoint` value, with the following query parameters: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OAuth also has Pushed Authorization Requests which attempt to alleviate some of the issues of query parameters (see e.g. the introduction of RFC9126). I wonder if it'd be worth to consider these from the outset here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point, although because most Matrix clients will be public clients, we wouldn't really gain from the cryptographic integrity or confidentiality advantages PAR gives us. It would help us in two other ways though:
- validating the authorisation request earlier, to possibly show a user-friendly error within the client
- large authorisation requests, if we start having fine-grained authorisations
This is why for now, I'd prefer leaving them out of the proposal for the sake of simplicity, and introduce them later (probably alongside RAR) when we actually need it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense and sounds fair to me. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(it would be good to have this alternative documented in the MSC)
MSCs proposed for Final Comment Period (FCP) should meet the requirements outlined in the checklist prior to being accepted into the spec. This checklist is a bit long, but aims to reduce the number of follow-on MSCs after a feature lands. SCT members: please check off things you check for, and raise a concern against FCP if the checklist is incomplete. If an item doesn't apply, prefer to check it rather than remove it. Unchecking items is encouraged where applicable. Checklist:
|
@mscbot fcp merge |
Team member @mscbot has proposed to merge this. The next step is review by the rest of the tagged people: Concerns:
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 concern FCP checklist incomplete |
|
||
### Prerequisites | ||
|
||
This proposal requires the client to know the following authorization server metadata about the homeserver: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we also need code_challenge_methods_supported
for the PKCE methods?
|
||
Homeservers and clients must: | ||
|
||
- support PKCE as per [RFC7636] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it seems that using the S256
method is required below, maybe it should be mentioned here?
Rendered
Related: matrix-org/matrix-spec#636
Status:
Part of the following proposal:
Implementations:
Homeservers
Clients
In OIDC-native mode:
SCT stuff:
checklist
FCP tickyboxes