Fix: Use proper scopes in refresh token handler #698

silverspace · 2022-08-31T23:34:28Z

Fixes #696

The token refresh handler currently has two issues related to scopes:

It does not read or honor the optional scope form parameter
It defaults to the current client scopes, rather than the originally granted scopes. One issue related to this is that if the client scopes are narrowed, then the existing refresh tokens may fail to refresh, even if the user originally consented to those scopes.

This PR fixes the OAuth2 token refresh handler to:

Read and use the optional 'scope' form parameter, if present.
Otherwise default to requesting the originally granted scopes.

The token refresh handler endpoint should be completely agnostic of:

The originally requested scopes
The client scopes (both current and past client scopes)

Checklist

I have read the contributing guidelines and signed the CLA.
I have referenced an issue containing the design document if my change introduces a new feature.
I have read the security policy.
I confirm that this pull request does not address a security vulnerability.
If this pull request addresses a security vulnerability,
I confirm that I got green light (please contact [email protected]) from the maintainers to push the changes.
I have added tests that prove my fix is effective or that my feature works.
I have added necessary documentation within the code base (if appropriate).

Further comments

Fixing the OAuth2 token refresh handler to: - Read and use the optional 'scope' form parameter, if present. - Otherwise default to requesting the originally granted scopes. This endpoint should be completely agnostic of: - The originally **requested** scopes - The **client scopes** (both current and past client scopes) Fixes ory#696

Updating our OAuth2 token refresh handler tests to completely ignore the **Client Scopes** and **Originally Requested Scopes**. Instead, the originally granted scopes should be the only scopes validated against. Also adding some tests to validate the optional 'scope' parameter, as outlined in https://www.rfc-editor.org/rfc/rfc6749#section-6 Note that this implementation returns an ErrInvalidScope if the 'scope' form parameter is defined but empty.

aeneasr

Thank you very much for the detailed issue and PR! I think this is working though as intended for a reason, please check my comment. Thanks!

aeneasr · 2022-09-01T08:16:22Z

handler/oauth2/flow_refresh.go

 request.SetRequestedAudience(originalRequest.GetRequestedAudience())

- for _, scope := range originalRequest.GetGrantedScopes() {
- if !c.Config.GetScopeStrategy(ctx)(request.GetClient().GetScopes(), scope) {


For context: This design decision was originally made to cover the use case where a client could gain access to scopes that are not in its allowed list. This can happen when a client is updated. I'd say this behavior is correct. The client should not receive scopes which are not in its allowed list.

This check was merely moved here from what I can tell: https://github.com/ory/fosite/pull/698/files#diff-e733adcae5e1fe83d4192fc112318cfd1d77b5ad76b6d71d383ae705ea29e3f2R111-R112

From looking purely at the code change what happens is if the request to the token endpoint is a refresh and the form contains a scopes key it uses that as the source of the intended scopes, if it doesn't it uses the original request as a source of the intended scopes, then performs the original check to ensure the client is allowed to issue those scopes.

I can see another issue with this PR though. I'll make a review.

It is strange that the conformity test failed though...

For context: This design decision was originally made to cover the use case where a client could gain access to scopes that are not in its allowed list. This can happen when a client is updated. I'd say this behavior is correct. The client should not receive scopes which are not in its allowed list.

@aeneasr Please let me know if James' response makes sense, or if you still have issues you'd like to chat through with this implementation.

I see the issue, we should probably still check the intended scopes against the client to ensure it absolutely can issue them (even though it theoretically did before). But lets see what Aeneasr says.

Hi @aeneasr , the above does NOT follow RFC6749. This is precisely the bug that I am encountering in Fosite, and the reason I filed this PR to fix it. The implementation according to the RFC should be:

Authorization Grant is done with scope a, b, c - client is granted scopes a, b, c

Refresh grant is executed without ?scope parameter, thus scope a, b, c is granted

Refresh grant is executed again with ?scope=a,b, and scope a,b is granted

Client is updated to only allow scope a, b

Refresh grant is executed without ?scope parameter, scope a, b, c is granted

Refresh grant is executed with ?scope=a,b,c parameter, refresh ~~fails~~ succeeds

Refresh grant is executed with ?scope=a,b parameter, and scope a,b is granted

Refreshing an access token should be completely agnostic to the currently configured client scopes. Note that the entire RFC makes no mention of the term client scope.

The other issue that this PR attempts to resolve is that steps (3) and (7) are broken in Fosite, since Fosite does not read the ?scope=a,b parameter.

I respectfully disagree. #698 (comment) is not what I trust the correct sequence to be, I think it reduces security, and it's also not how I interpret the RFC. What could work to convince me otherwise is to get the conformance tests (which test OIDC conformity ...) to pass since they are failing on this PR (potentially due to the changes made), and to get someone from the OAuth2 IETF authors or maintainers to agree with your interpretation.

Hi @aeneasr I can understand your reservations, and desire to get sign-off from the OAuth2 IETF authors. How about we focus on the more clear-cut issue of Fosite not honoring the ?scope=a,b parameter for now? That would at least unblock my particular use case.

Yes, Fosite should definitely respect that parameter! :)

handler/oauth2/flow_refresh.go

silverspace · 2022-09-02T16:19:39Z

Perhaps we can put aside the "client scope" discussion for a moment, and simply agree that the Fosite library DOES have a bug whereby the scope form parameter is completely ignored. The refresh token handler does not read the scope form parameter in the current implementation, so it is currently impossible to request a more narrowly scoped access token.

james-d-elliott · 2022-09-03T01:31:52Z

Perhaps we can put aside the "client scope" discussion for a moment, and simply agree that the Fosite library DOES have a bug whereby the scope form parameter is completely ignored. The refresh token handler does not read the scope form parameter in the current implementation, so it is currently impossible to request a more narrowly scoped access token.

Yeah I agree with focusing on the one aspect, it's more efficient. Especially with focusing on something I think is less ambiguous/open to interpretation/not explicitly stated in the spec, is a good idea.

silverspace · 2022-09-08T03:47:44Z

@aeneasr please let me know if you have time to discuss #698 (comment)

aeneasr · 2022-11-01T13:06:28Z

Given the discussion and reasoning above I will close this PR. Any ideas for future work outlined in this PR we‘ll happily accept though :)

silverspace requested a review from aeneasr as a code owner August 31, 2022 23:34

silverspace changed the title ~~Use proper scopes in refresh token handler~~ Fix: Use proper scopes in refresh token handler Aug 31, 2022

silverspace added 2 commits August 31, 2022 16:50

silverspace force-pushed the greg/custom_refresh_token_scope_fix branch from cfcbd40 to d9ecd46 Compare August 31, 2022 23:50

aeneasr requested changes Sep 1, 2022

View reviewed changes

james-d-elliott suggested changes Sep 1, 2022

View reviewed changes

handler/oauth2/flow_refresh.go Show resolved Hide resolved

silverspace requested a review from aeneasr September 2, 2022 06:15

aeneasr mentioned this pull request Oct 25, 2022

Token endpoint doesn't accept "scope" query parameter ory/hydra#3311

Open

6 tasks

aeneasr closed this Nov 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Use proper scopes in refresh token handler #698

Fix: Use proper scopes in refresh token handler #698

silverspace commented Aug 31, 2022

aeneasr left a comment

aeneasr Sep 1, 2022

james-d-elliott Sep 1, 2022

james-d-elliott Sep 1, 2022

silverspace Sep 1, 2022 •

edited

Loading

james-d-elliott Sep 1, 2022 •

edited

Loading

silverspace Sep 8, 2022 •

edited

Loading

silverspace Sep 8, 2022

aeneasr Sep 9, 2022

silverspace Sep 9, 2022

aeneasr Sep 11, 2022

silverspace commented Sep 2, 2022

james-d-elliott commented Sep 3, 2022 •

edited

Loading

silverspace commented Sep 8, 2022

aeneasr commented Nov 1, 2022

Fix: Use proper scopes in refresh token handler #698

Fix: Use proper scopes in refresh token handler #698

Conversation

silverspace commented Aug 31, 2022

Related Issue or Design Document

Checklist

Further comments

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr Sep 1, 2022

Choose a reason for hiding this comment

james-d-elliott Sep 1, 2022

Choose a reason for hiding this comment

james-d-elliott Sep 1, 2022

Choose a reason for hiding this comment

silverspace Sep 1, 2022 • edited Loading

Choose a reason for hiding this comment

james-d-elliott Sep 1, 2022 • edited Loading

Choose a reason for hiding this comment

silverspace Sep 8, 2022 • edited Loading

Choose a reason for hiding this comment

silverspace Sep 8, 2022

Choose a reason for hiding this comment

aeneasr Sep 9, 2022

Choose a reason for hiding this comment

silverspace Sep 9, 2022

Choose a reason for hiding this comment

aeneasr Sep 11, 2022

Choose a reason for hiding this comment

silverspace commented Sep 2, 2022

james-d-elliott commented Sep 3, 2022 • edited Loading

silverspace commented Sep 8, 2022

aeneasr commented Nov 1, 2022

silverspace Sep 1, 2022 •

edited

Loading

james-d-elliott Sep 1, 2022 •

edited

Loading

silverspace Sep 8, 2022 •

edited

Loading

james-d-elliott commented Sep 3, 2022 •

edited

Loading