-
Notifications
You must be signed in to change notification settings - Fork 42
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 a new guard clause to the oauth2 client credentials flow. #1050
Conversation
73a5c0a
to
64d63ae
Compare
if scope := " ".join(self.scopes): | ||
data["scope"] = scope |
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 would be an actual change, but what is the issue we are solving 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.
Some Idps doesn't require a scope parameter to issue a token. And since it's not required, maybe sending it as an empty value could cause an issue with the IdP itself.
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.
"Maybe" is a bit vague. Can you find a place in rcf6749 being specific about this?
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.
https://datatracker.ietf.org/doc/html/rfc6749#section-4.4.2
scope
OPTIONAL. The scope of the access request as described by
[Section 3.3](https://datatracker.ietf.org/doc/html/rfc6749#section-3.
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 tells me it's optional. But is it allowed to send an empty string? If yes, does it convey a different semantic?
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.
As we stated, if the scope is to be used, it must not be empty. https://datatracker.ietf.org/doc/html/rfc6749#section-3.3
Head branch was pushed to by a user without write access
According to the section 3.3 of RFC 6749 OAuth2 Access Token Scope, this is the notation that defines how the scope should be send to the IdentityProvider: `scope = scope-token *( SP scope-token )` Basically it should be omitted if it's empty.
Closes #1049