From cda9c24ce1a981f60d6c2e99b7fceec1654c807d Mon Sep 17 00:00:00 2001 From: James Elliott Date: Wed, 2 Nov 2022 20:15:51 +1100 Subject: [PATCH] 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. --- handler/oauth2/flow_refresh.go | 31 ++++++++- handler/oauth2/flow_refresh_test.go | 97 +++++++++++++++++++++++++++-- 2 files changed, 119 insertions(+), 9 deletions(-) diff --git a/handler/oauth2/flow_refresh.go b/handler/oauth2/flow_refresh.go index 78a63bdec..6625826e9 100644 --- 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 index 490c4e9c5..e3298994a 100644 --- 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))