Unable to request narrower scopes in OAuth2 Refresh Token Exchange #696

silverspace · 2022-08-25T05:51:00Z

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:

The current implementation does not read the scope form param.
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

The text was updated successfully, but these errors were encountered:

james-d-elliott · 2022-08-25T08:28:28Z

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 · 2022-08-29T23:41:55Z

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

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

aeneasr · 2022-11-01T13:07:58Z

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

silverspace · 2022-11-01T17:12:40Z

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 · 2022-11-02T02:36:27Z

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 · 2022-11-02T02:59:19Z

References:

james-d-elliott · 2022-11-02T04:30:26Z

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 · 2022-11-04T01:25:51Z

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 · 2022-11-04T06:39:31Z

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 · 2022-11-04T06:39:34Z

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!

silverspace added the bug Something is not working. label Aug 25, 2022

silverspace mentioned this issue Aug 31, 2022

Fix: Use proper scopes in refresh token handler #698

Closed

6 tasks

james-d-elliott linked a pull request Nov 2, 2022 that will close this issue

fix: requested scope ignored in refresh flow #718

Open

6 tasks

james-d-elliott mentioned this issue Jan 2, 2023

fix: fosite refresh scope narrowing e2e ory/hydra#3403

Draft

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unable to request narrower scopes in OAuth2 Refresh Token Exchange #696

Unable to request narrower scopes in OAuth2 Refresh Token Exchange #696

silverspace commented Aug 25, 2022 •

edited

james-d-elliott commented Aug 25, 2022

silverspace commented Aug 29, 2022

aeneasr commented Nov 1, 2022

silverspace commented Nov 1, 2022

james-d-elliott commented Nov 2, 2022 •

edited

james-d-elliott commented Nov 2, 2022

james-d-elliott commented Nov 2, 2022 •

edited

james-d-elliott commented Nov 4, 2022 •

edited

aeneasr commented Nov 4, 2022

aeneasr commented Nov 4, 2022

Unable to request narrower scopes in OAuth2 Refresh Token Exchange #696

Unable to request narrower scopes in OAuth2 Refresh Token Exchange #696

Comments

silverspace commented Aug 25, 2022 • edited

Preflight checklist

Describe the bug

Reproducing the bug

Relevant log output

Relevant configuration

Version

On which operating system are you observing this issue?

In which environment are you deploying?

Additional Context

james-d-elliott commented Aug 25, 2022

silverspace commented Aug 29, 2022

aeneasr commented Nov 1, 2022

silverspace commented Nov 1, 2022

james-d-elliott commented Nov 2, 2022 • edited

james-d-elliott commented Nov 2, 2022

james-d-elliott commented Nov 2, 2022 • edited

james-d-elliott commented Nov 4, 2022 • edited

aeneasr commented Nov 4, 2022

aeneasr commented Nov 4, 2022

silverspace commented Aug 25, 2022 •

edited

james-d-elliott commented Nov 2, 2022 •

edited

james-d-elliott commented Nov 2, 2022 •

edited

james-d-elliott commented Nov 4, 2022 •

edited