Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
MSC2964: Usage of OAuth 2.0 authorization code grant and refresh token grant #2964
Changes from 29 commits
2af8b49
5962618
38c97a5
e12ee77
20d865d
261e3b0
38bb557
45d510b
d114f82
8fc3ea1
029e1e5
0802d8f
20ee4a3
6e387d8
4a2ed74
40048da
f0e319a
55215c1
21fee1c
2c0625d
24e0290
d145fd2
acfa845
fa506ff
378348e
c859c0b
c1c8312
05748a2
1034122
4830d47
f84428f
c57be5e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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:
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.
These values would be URL encoded
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.
I kept this part simplified with no URL-encoding and extra spaces. It might be worth saying that, and give the actual url-encoded, with no space added URL in addition
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.
The values are already listed above in their non-url-encoded format. The oauth specs also include line breaks in their examples but url-encode the values
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.
URL encoding
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.
Same here, this is done for readability