-
Notifications
You must be signed in to change notification settings - Fork 145
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
Access token response is not compliant with OAuth 2.0 (RFC 6749) #660
Comments
@isegura-eos-eng Yes, you are right it should't take much to add it to the struct. @livio-a this feels like a Deja Vu. Wasn't there already an issue, fix or discussion somewhere? |
Found it: zitadel/zitadel#6609 At the time someone volunteered for a fix, but went silent. @isegura-eos-eng do you want to send a PR for this? |
@muhlemmer I could try. I've looked at the code a little bit, already. There are four places where this response must be returned, assuming this is the only struct used for returning the Access Token response. I think this can be very easy or the opposite. Depending on whether the If it's the first case, then is pretty straight-forward, as we can just get the actual scopes from there and into the response. As I explained in the issue, we don't really need to know if the scopes granted differ from the ones requested, although is not very elegant, this would comply with the spec (maybe enough for now). The elegant way to do it is compare whether the scopes are different form the ones requested, as it's written by the RFC. If it's the second case, which I suspect it is. Then we need to get the information there. And looking at the code, I don't see a clear way of accessing this information. Does anyone have an idea how to make this change compatible with the API if this is the case? |
@isegura-eos-eng validation of scope is done early in the Lines 268 to 286 in 5ae555e
The token endpoint has no access to the originally requested scope. For this fix I would just always return scope, which is not forbidden by the standard and fulfills the |
Then it's pretty straight forward. I've never done a PR in this project, any tips @muhlemmer ? And another question: To build the scopes I have to do: |
Contribution guidelines are here: https://github.com/zitadel/oidc/blob/main/CONTRIBUTING.md
You can use the
We are always welcoming other improvements! But please open a separate PR. It is good to change custom functions to newer standard functions, but take care we have a strict semver policy that does not allow us to remove exported functions under Thanks in advance! |
Preflight Checklist
Describe your problem
In the RFC 6749 document describing the Access Token
scope
request parameter it indicates the following:I've implemented both
authorization_code
andclient_credentials
in my program with theop.Storage
interface. However, when I supply thescope
parameter in the token endpoint the response does not include the required parameter.I think I don't need to supply my code because it's easy to see that
oidc.AccessTokenResponse
does not include thescope
parameter:oidc/pkg/oidc/token.go
Lines 232 to 239 in 97d7b28
Describe your ideal solution
Be compliant with OAuth 2.0 in this case. Whenever the
scope
parameter is sent to the token endpoint, check whether the returned scope matches the request. Or return the scope always if that is easier, which complies also with the rule.Version
3.30.0
Environment
Self-hosted
Additional Context
I'm not sure if I this filed this issue correctly. In the OpenID spec, in the token endpoint section indicates to follow "Section 3.2" of RFC 6749. Clearly, 3.3 is not mentioned. Should 3.3 be included?
Aside of this, I think is very helpful to obtain the actual scope given in the access token response. I may have missed if there is other way to achieve this, so please forgive me and educate me if this is the case.
Edit:
oidc.AccessTokenResponse
permalink to show all lines of the struct.The text was updated successfully, but these errors were encountered: