Skip to content
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.

Sign up for GitHub

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

Fix: Use proper scopes in refresh token handler #698

Closed
wants to merge 2 commits into from
Closed

Fix: Use proper scopes in refresh token handler #698

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0f25684
oauth2 flow_refresh: Use granted scope parameters in token refresh
silverspace Aug 31, 2022
d9ecd46
oauth2 flow_refresh_test: Add unit tests for optional 'scope' param
silverspace Aug 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions handler/oauth2/flow_refresh.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,11 +96,20 @@ func (c *RefreshTokenGrantHandler) HandleTokenEndpointRequest(ctx context.Contex
}

request.SetSession(originalRequest.GetSession().Clone())
request.SetRequestedScopes(originalRequest.GetRequestedScopes())
request.SetRequestedAudience(originalRequest.GetRequestedAudience())

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@silverspace silverspace Sep 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@silverspace silverspace Sep 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Authorization Grant is done with scope a, b, c - client is granted scopes a, b, c
  2. Refresh grant is executed without ?scope parameter, thus scope a, b, c is granted
  3. Refresh grant is executed again with ?scope=a,b, and scope a,b is granted
  4. Client is updated to only allow scope a, b
  5. Refresh grant is executed without ?scope parameter, scope a, b, c is granted
  6. Refresh grant is executed with ?scope=a,b,c parameter, refresh fails succeeds
  7. 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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, Fosite should definitely respect that parameter! :)

if _, ok := request.GetRequestForm()["scope"]; ok {
requestedScopes := fosite.RemoveEmpty(strings.Split(request.GetRequestForm().Get("scope"), " "))
silverspace marked this conversation as resolved.
Show resolved Hide resolved
if len(requestedScopes) == 0 {
return errorsx.WithStack(fosite.ErrInvalidScope.WithHintf("The requested scope parameter is empty"))
}
request.SetRequestedScopes(requestedScopes)
} else {
request.SetRequestedScopes(originalRequest.GetGrantedScopes())
}

for _, scope := range request.GetRequestedScopes() {
if !c.Config.GetScopeStrategy(ctx)(originalRequest.GetGrantedScopes(), scope) {
return errorsx.WithStack(fosite.ErrInvalidScope.WithHintf("The OAuth 2.0 Client is not allowed to request scope '%s'.", scope))
}
request.GrantScope(scope)
Expand Down
Loading