Unable to request narrower scopes in OAuth2 Refresh Token Exchange

[silverspace]

Preflight checklist

• I could not find a solution in the existing issues, docs, nor discussions.
• I agree to follow this project's Code of Conduct.
• I have read and am following this repository's Contribution Guidelines.
• This issue affects my Ory Cloud project.
• I have joined the Ory Community Slack.
• I am signed up to the Ory Security Patch Newsletter.

Describe the bug

Hello!

I recently discovered that the Refresh Token Grant Handler does not currently honor the (optional) user requested scope parameter to request a subset of the originally granted scopes.

For reference, here is the code in question:

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

The issues that I see with the current implementation are:

1. The current implementation does not read the scope form param.
2. The implementation currently compares the originally granted scopes against the current client scopes, but this endpoint should be completely agnostic of the client scopes. I think it should only depend on the originally granted scopes (and optionally the scope form param). The result is that the refresh token exchange will fail if the client scopes are themselves narrowed after a refresh token is created.

This is the implementation that I would propose, given my understanding of https://www.rfc-editor.org/rfc/rfc6749#section-6

diff --git a/handler/oauth2/flow_refresh.go b/handler/oauth2/flow_refresh.go
index 0e3be4d..642e774 100644
--- a/handler/oauth2/flow_refresh.go
+++ b/handler/oauth2/flow_refresh.go
@@ -96,11 +96,15 @@ func (c *RefreshTokenGrantHandler) HandleTokenEndpointRequest(ctx context.Contex
        }
 
        request.SetSession(originalRequest.GetSession().Clone())
-       request.SetRequestedScopes(originalRequest.GetRequestedScopes())
+       if request.GetRequestForm().Has("scope") {
+               request.SetRequestedScopes(strings.Split(request.GetRequestForm().Get("scope"), " "))
+       } else {
+               request.SetRequestedScopes(originalRequest.GetGrantedScopes())
+       }
        request.SetRequestedAudience(originalRequest.GetRequestedAudience())
 
-       for _, scope := range originalRequest.GetGrantedScopes() {
-               if !c.Config.GetScopeStrategy(ctx)(request.GetClient().GetScopes(), scope) {
+       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)

Some pertinent notes from my reading of https://www.rfc-editor.org/rfc/rfc6749#section-6:

scope
         OPTIONAL.  The scope of the access request as described by
         [Section 3.3](https://www.rfc-editor.org/rfc/rfc6749#section-3.3).  The requested scope MUST NOT include any scope
         not originally granted by the resource owner, and if omitted is
         treated as equal to the scope originally granted by the
         resource owner.

• This optional scope parameter should allow the client to request a narrower set of scopes than were originally granted by the resource owner.
• The refresh token grant handler should be completely agnostic of the client scopes. It should only depend on the originally granted scopes, and optionally the scope form param.
• I would expect this narrower set of scopes to apply only to this specific access token, and not to future refresh token requests. (This is referenced later in the RFC with "If a new refresh token is issued, the refresh token scope MUST be identical to that of the refresh token included by the client in the request.")

Reproducing the bug

The current unit tests reproduce this issue. For example:

=== RUN   TestRefreshFlow_HandleTokenEndpointRequest/strategy=hmac/case=should_pass
    flow_refresh_test.go:194: 
        	Error Trace:	flow_refresh_test.go:194
        	            				flow_refresh_test.go:356
        	Error:      	Not equal: 
        	            	expected: fosite.Arguments{"foo", "bar", "offline"}
        	            	actual  : fosite.Arguments{"foo", "offline"}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,4 +1,3 @@
        	            	-(fosite.Arguments) (len=3) {
        	            	+(fosite.Arguments) (len=2) {
        	            	  (string) (len=3) "foo",
        	            	- (string) (len=3) "bar",
        	            	  (string) (len=7) "offline"
        	Test:       	TestRefreshFlow_HandleTokenEndpointRequest/strategy=hmac/case=should_pass
        --- FAIL: TestRefreshFlow_HandleTokenEndpointRequest/strategy=hmac/case=should_pass (0.00s)


Expected :fosite.Arguments{"foo", "bar", "offline"}
Actual   :fosite.Arguments{"foo", "offline"}

Relevant log output

No response

Relevant configuration

No response

Version

master

On which operating system are you observing this issue?

No response

In which environment are you deploying?

No response

Additional Context

No response

[james-d-elliott]

Your reading of the RFC seems correct to me. I can't speak officially on behalf of ory but generally this is something that provided there is a linked RFC, and you can produce a test that fails before your fix and passes after it will be accepted if you're interested in producing a PR. If not I'm happy to do so and credit you.

[silverspace]

Sure, I'd be happy to throw together a PR with this fix. Thanks!

[aeneasr]

As discussed in the PR, the scope parameter should be respected in the refresh flow as well as the OAuth2 client scope :)

[silverspace]

Just to clarify the above: Fosite does currently have a bug where it does not respect the scope form field in the refresh flow.

[james-d-elliott]

I've managed to wrangle together some actual proof the issue exists. The tests are very primitive but demonstrate the scope field is being ignored. I used the higher level functions to ensure one of the auxiliary handlers was not handling this case specifically as I'm not 100% certain of the interaction of some of the handlers (many are utilized in loops).

https://gist.github.com/james-d-elliott/1475671e805c9b4e2167c6c1b61f05f2

[james-d-elliott]

References:

• https://openid.net/specs/openid-connect-core-1_0.html#RefreshingAccessToken
• https://www.rfc-editor.org/rfc/rfc6749#section-6

[james-d-elliott]

The following patch resolves said issue but breaks TestRefreshFlow_HandleTokenEndpointRequest (I'll look into why but it's probably due to my use of checking the requested scopes instead of granted scopes):

Index: handler/oauth2/flow_refresh.go
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/handler/oauth2/flow_refresh.go b/handler/oauth2/flow_refresh.go
--- a/handler/oauth2/flow_refresh.go	(revision 2d32d9ebe7d97181c9193292f7004133ae097f40)
+++ b/handler/oauth2/flow_refresh.go	(date 1667363251433)
@@ -96,13 +96,25 @@
 	}
 
 	request.SetSession(originalRequest.GetSession().Clone())
-	request.SetRequestedScopes(originalRequest.GetRequestedScopes())
+
+	switch scope := request.GetRequestForm().Get("scope"); scope {
+	case "":
+		request.SetRequestedScopes(originalRequest.GetGrantedScopes())
+	default:
+		request.SetRequestedScopes(fosite.RemoveEmpty(strings.Split(scope, " ")))
+	}
+
 	request.SetRequestedAudience(originalRequest.GetRequestedAudience())
 
-	for _, scope := range originalRequest.GetGrantedScopes() {
+	for _, scope := range request.GetRequestedScopes() {
+		if !originalRequest.GetGrantedScopes().Has(scope) {
+			return errorsx.WithStack(fosite.ErrInvalidScope.WithHintf("The requested scope '%s' is not allowed as it was not originally granted during the access request.", scope))
+		}
+
 		if !c.Config.GetScopeStrategy(ctx)(request.GetClient().GetScopes(), scope) {
 			return errorsx.WithStack(fosite.ErrInvalidScope.WithHintf("The OAuth 2.0 Client is not allowed to request scope '%s'.", scope))
 		}
+
 		request.GrantScope(scope)
 	}

[james-d-elliott]

So I think the original specific issue is solved by the changes. There is one particular point regarding the spec and what the text if omitted is treated as equal to the scope originally granted by the resource owner exactly means. The particular question is what originally granted means. Does it mean the first access response that granted the refresh token? Or does it mean the scope granted of the (potentially) narrower token?

[aeneasr]

The particular question is what originally granted means. Does it mean the first access response that granted the refresh token? Or does it mean the scope granted of the (potentially) narrower token?

The resource owner is not involved during token refresh and is not granting scopes there. He/she only grants scopes on the consent UI screen. Therefore, this has to imply the original request!

[aeneasr]

The particular question is what originally granted means. Does it mean the first access response that granted the refresh token? Or does it mean the scope granted of the (potentially) narrower token?

The resource owner is not involved during token refresh and is not granting scopes there. He/she only grants scopes on the consent UI screen. Therefore, this has to imply the original request!