-
Notifications
You must be signed in to change notification settings - Fork 75
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
Add tokens and scopes. #208
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
A couple of points:
(a) this is going to be backwards incompatible ... which isn't great and
(b) it would be useful to somewhat get inspiration on terminology / requests / responses / etc from the OIDC token endpoint
It would be interesting to find a formulation in which (b) is 100% backwards compatible to the OIDC endpoint, because then we could re-use their existing deployments. I don't think it is a mandatory / hard requirement, but if everything fits into that model, it wouldn't be bad to reuse the existing deployment.
Also, would be useful to anticipate the kinds of security affordances that were added to that endpoint so that we don't run into the same walls.
There is also the authorization endpoint which I don't think I quite understand the distinction between that and the token endpoint, but worth pointing out.
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.
Backwards incompatible with what? FedCM or OIDC?
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 authorization endpoint in OAuth https://www.rfc-editor.org/rfc/rfc6749.html#section-3.1 is where the client sends the user for interactive authentication and approval of the requested access.
The token endpoint in OAuth https://www.rfc-editor.org/rfc/rfc6749.html#section-3.2 is used directly by the client to get tokens - typically by presenting an authorization code obtained from the authorization endpoint or a refresh token.
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.
Which is to say that, while I don't know that backwards compatible is even possible here, it would be nice if different terms were used for things which are different than established things in these established protocols.
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 authorization endpoint is used for cases where the user needs to interact with the IDP (in a browser window). If I understand your scoping correctly (https://github.com/fedidcg/FedCM/blob/main/explainer.md), you want to exclude such interactive requests currently (under "out of scope", "Sign into the IDP"). There are two main cases where the user might need to interact with the IDP: 1) initial sign-in to the IDP 2) additional authentication required (e.g. TFA). As a concrete example of additional authentication required, Microsoft requires any sign-in (e.g. password) to sync the browser history in Microsoft Edge. However, to access the OneDrive personal vault, the sign-in must include TFA. I think excluding interactive sign-in (and thus the authorization endpoint) is reasonable - you may want to add "additional authentication required" as an out of scope case in the explainer as well.
Connecting to OAuth's token endpoint. Microsoft defines an artifact called the primary refresh token. It's similar to an OAuth refresh token, but useable for all clients, not just one clients, because it is owned by the IDP. Here, you've effectively done the same thing, but with the IDP cookies.
Consider if you start from an OAuth refresh token request that would be valid for a public client under RFC 6749 Section 6:
You move the refresh_token from form data to cookie, and treat the cookie as if it is valid for all clients, not just one client. That gets you to something like:
If you drop grant_type and switch from /token to /tokens.php, and add the security header, you have a FedCM request:
So, to the extent that you want to make this more OAuth-y (which I tend to think is a good thing), I think the only change you would really need to make is to add a grant_type parameter and to ensure you carefully follow OAuth semantics (e.g. returning unknown response parameters, not interpreting request scope parameter, etc.).