fix(oauth2): requested scope ignored in refresh flow

This addresses an issue where the clients requested scope is ignored during the refresh flow preventing a refresh token from granting an access token with a more narrow scope. It is critical to note that it is completely ignored, and previous behaviour only issued the same scope as originally granted. An access request which requests a broader scope is equally ignored and it has been thoroughly tested to ensure this cannot occur in HEAD. See https://www.rfc-editor.org/rfc/rfc6749#section-6 for more information.
ory · Nov 2, 2022 · f35f7d0 · f35f7d0
1 parent 603ceb4
commit f35f7d0
Show file tree

Hide file tree

Showing 2 changed files with 119 additions and 9 deletions.
diff --git a/handler/oauth2/flow_refresh.go b/handler/oauth2/flow_refresh.go
@@ -28,7 +28,6 @@ import (
 	"time"
 
 	"github.com/ory/x/errorsx"
-
 	"github.com/pkg/errors"
 
 	"github.com/ory/fosite"
@@ -96,13 +95,39 @@ func (c *RefreshTokenGrantHandler) HandleTokenEndpointRequest(ctx context.Contex
 	}
 
 	request.SetSession(originalRequest.GetSession().Clone())
-	request.SetRequestedScopes(originalRequest.GetRequestedScopes())
+
+	/*
+			There are two key points in the following spec section this addresses:
+				1. If omitted the scope param should be treated as the same as the scope originally granted by the resource owner.
+				2. The REQUESTED scope MUST NOT include any scope not originally granted.
+
+			scope
+					OPTIONAL.  The scope of the access request as described by 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.
+
+			See https://www.rfc-editor.org/rfc/rfc6749#section-6
+	*/
+	switch scope := request.GetRequestForm().Get("scope"); scope {
+	case "":
+		// Addresses point 1 of the text in RFC6749 Section 6.
+		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() {
+		// Addresses point 2 of the text in RFC6749 Section 6.
+		if !originalRequest.GetGrantedScopes().Has(scope) {
+			return errorsx.WithStack(fosite.ErrInvalidScope.WithHintf("The requested scope '%s' was not originally granted by the resource owner.", 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)
 	}
 

diff --git a/handler/oauth2/flow_refresh_test.go b/handler/oauth2/flow_refresh_test.go
@@ -29,14 +29,12 @@ import (
 	"time"
 
 	"github.com/golang/mock/gomock"
-
-	"github.com/ory/fosite/internal"
-
 	"github.com/pkg/errors"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"github.com/ory/fosite"
+	"github.com/ory/fosite/internal"
 	"github.com/ory/fosite/storage"
 )
 
@@ -191,12 +189,99 @@ func TestRefreshFlow_HandleTokenEndpointRequest(t *testing.T) {
 						assert.NotEqual(t, sess, areq.Session)
 						assert.NotEqual(t, time.Now().UTC().Add(-time.Hour).Round(time.Hour), areq.RequestedAt)
 						assert.Equal(t, fosite.Arguments{"foo", "offline"}, areq.GrantedScope)
-						assert.Equal(t, fosite.Arguments{"foo", "bar", "offline"}, areq.RequestedScope)
+						assert.Equal(t, fosite.Arguments{"foo", "offline"}, areq.RequestedScope)
 						assert.NotEqual(t, url.Values{"foo": []string{"bar"}}, areq.Form)
 						assert.Equal(t, time.Now().Add(time.Hour).UTC().Round(time.Second), areq.GetSession().GetExpiresAt(fosite.AccessToken))
 						assert.Equal(t, time.Now().Add(time.Hour).UTC().Round(time.Second), areq.GetSession().GetExpiresAt(fosite.RefreshToken))
 					},
 				},
+				{
+					description: "should pass with scope in form",
+					setup: func(config *fosite.Config) {
+						areq.GrantTypes = fosite.Arguments{"refresh_token"}
+						areq.Client = &fosite.DefaultClient{
+							ID:         "foo",
+							GrantTypes: fosite.Arguments{"refresh_token"},
+							Scopes:     []string{"foo", "bar", "baz", "offline"},
+						}
+
+						token, sig, err := strategy.GenerateRefreshToken(nil, nil)
+						require.NoError(t, err)
+
+						areq.Form.Add("refresh_token", token)
+						areq.Form.Add("scope", "foo bar baz offline")
+						err = store.CreateRefreshTokenSession(nil, sig, &fosite.Request{
+							Client:         areq.Client,
+							GrantedScope:   fosite.Arguments{"foo", "bar", "baz", "offline"},
+							RequestedScope: fosite.Arguments{"foo", "bar", "baz", "offline"},
+							Session:        sess,
+							Form:           url.Values{"foo": []string{"bar"}},
+							RequestedAt:    time.Now().UTC().Add(-time.Hour).Round(time.Hour),
+						})
+						require.NoError(t, err)
+					},
+					expect: func(t *testing.T) {
+						assert.Equal(t, fosite.Arguments{"foo", "bar", "baz", "offline"}, areq.GrantedScope)
+						assert.Equal(t, fosite.Arguments{"foo", "bar", "baz", "offline"}, areq.RequestedScope)
+					},
+				},
+				{
+					description: "should pass with scope in form and should narrow scopes",
+					setup: func(config *fosite.Config) {
+						areq.GrantTypes = fosite.Arguments{"refresh_token"}
+						areq.Client = &fosite.DefaultClient{
+							ID:         "foo",
+							GrantTypes: fosite.Arguments{"refresh_token"},
+							Scopes:     []string{"foo", "bar", "baz", "offline"},
+						}
+
+						token, sig, err := strategy.GenerateRefreshToken(nil, nil)
+						require.NoError(t, err)
+
+						areq.Form.Add("refresh_token", token)
+						areq.Form.Add("scope", "foo bar offline")
+						err = store.CreateRefreshTokenSession(nil, sig, &fosite.Request{
+							Client:         areq.Client,
+							GrantedScope:   fosite.Arguments{"foo", "bar", "baz", "offline"},
+							RequestedScope: fosite.Arguments{"foo", "bar", "baz", "offline"},
+							Session:        sess,
+							Form:           url.Values{"foo": []string{"bar"}},
+							RequestedAt:    time.Now().UTC().Add(-time.Hour).Round(time.Hour),
+						})
+						require.NoError(t, err)
+					},
+					expect: func(t *testing.T) {
+						assert.Equal(t, fosite.Arguments{"foo", "bar", "offline"}, areq.GrantedScope)
+						assert.Equal(t, fosite.Arguments{"foo", "bar", "offline"}, areq.RequestedScope)
+					},
+				},
+				{
+					description: "should fail with broadened scopes even if the client can request it",
+					setup: func(config *fosite.Config) {
+						areq.GrantTypes = fosite.Arguments{"refresh_token"}
+						areq.Client = &fosite.DefaultClient{
+							ID:         "foo",
+							GrantTypes: fosite.Arguments{"refresh_token"},
+							Scopes:     []string{"foo", "bar", "baz", "offline"},
+						}
+
+						token, sig, err := strategy.GenerateRefreshToken(nil, nil)
+						require.NoError(t, err)
+
+						areq.Form.Add("refresh_token", token)
+						areq.Form.Add("scope", "foo bar offline")
+						err = store.CreateRefreshTokenSession(nil, sig, &fosite.Request{
+							Client:         areq.Client,
+							GrantedScope:   fosite.Arguments{"foo", "baz", "offline"},
+							RequestedScope: fosite.Arguments{"foo", "baz", "offline"},
+							Session:        sess,
+							Form:           url.Values{"foo": []string{"bar"}},
+							RequestedAt:    time.Now().UTC().Add(-time.Hour).Round(time.Hour),
+						})
+						require.NoError(t, err)
+					},
+					expectErr: fosite.ErrInvalidScope,
+				},
 				{
 					description: "should pass with custom client lifespans",
 					setup: func(config *fosite.Config) {
@@ -229,7 +314,7 @@ func TestRefreshFlow_HandleTokenEndpointRequest(t *testing.T) {
 						assert.NotEqual(t, sess, areq.Session)
 						assert.NotEqual(t, time.Now().UTC().Add(-time.Hour).Round(time.Hour), areq.RequestedAt)
 						assert.Equal(t, fosite.Arguments{"foo", "offline"}, areq.GrantedScope)
-						assert.Equal(t, fosite.Arguments{"foo", "bar", "offline"}, areq.RequestedScope)
+						assert.Equal(t, fosite.Arguments{"foo", "offline"}, areq.RequestedScope)
 						assert.NotEqual(t, url.Values{"foo": []string{"bar"}}, areq.Form)
 						internal.RequireEqualTime(t, time.Now().Add(*internal.TestLifespans.RefreshTokenGrantAccessTokenLifespan).UTC(), areq.GetSession().GetExpiresAt(fosite.AccessToken), time.Minute)
 						internal.RequireEqualTime(t, time.Now().Add(*internal.TestLifespans.RefreshTokenGrantRefreshTokenLifespan).UTC(), areq.GetSession().GetExpiresAt(fosite.RefreshToken), time.Minute)
@@ -290,7 +375,7 @@ func TestRefreshFlow_HandleTokenEndpointRequest(t *testing.T) {
 						assert.NotEqual(t, sess, areq.Session)
 						assert.NotEqual(t, time.Now().UTC().Add(-time.Hour).Round(time.Hour), areq.RequestedAt)
 						assert.Equal(t, fosite.Arguments{"foo"}, areq.GrantedScope)
-						assert.Equal(t, fosite.Arguments{"foo", "bar"}, areq.RequestedScope)
+						assert.Equal(t, fosite.Arguments{"foo"}, areq.RequestedScope)
 						assert.NotEqual(t, url.Values{"foo": []string{"bar"}}, areq.Form)
 						assert.Equal(t, time.Now().Add(time.Hour).UTC().Round(time.Second), areq.GetSession().GetExpiresAt(fosite.AccessToken))
 						assert.Equal(t, time.Now().Add(time.Hour).UTC().Round(time.Second), areq.GetSession().GetExpiresAt(fosite.RefreshToken))