From 37a00cffd070db248bf387c2555f5ebd662b92f5 Mon Sep 17 00:00:00 2001 From: James Elliott Date: Wed, 2 Nov 2022 20:15:51 +1100 Subject: [PATCH 1/7] 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 02c50493f..d9a5e8a01 100644 --- a/handler/oauth2/flow_refresh.go +++ b/handler/oauth2/flow_refresh.go @@ -10,7 +10,6 @@ import ( "time" "github.com/ory/x/errorsx" - "github.com/pkg/errors" "github.com/ory/fosite" @@ -78,13 +77,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 bed4ba581..893b59778 100644 --- a/handler/oauth2/flow_refresh_test.go +++ b/handler/oauth2/flow_refresh_test.go @@ -11,14 +11,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" ) @@ -173,12 +171,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) { @@ -211,7 +296,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) @@ -272,7 +357,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)) From 5d4da299997b9bca98eb3488a168306ee35233a6 Mon Sep 17 00:00:00 2001 From: James Elliott Date: Wed, 16 Nov 2022 10:17:13 +1100 Subject: [PATCH 2/7] fix: retain refresh_token original scopes --- access_request.go | 31 ++++++++++++++++++++++++++++++ handler/oauth2/flow_refresh.go | 35 +++++++++++++++++++++++++++++----- oauth2.go | 30 +++++++++++++++++++++++++---- 3 files changed, 87 insertions(+), 9 deletions(-) diff --git a/access_request.go b/access_request.go index c03e74a6d..5e24970f2 100644 --- a/access_request.go +++ b/access_request.go @@ -7,6 +7,9 @@ type AccessRequest struct { GrantTypes Arguments `json:"grantTypes" gorethink:"grantTypes"` HandledGrantType Arguments `json:"handledGrantType" gorethink:"handledGrantType"` + RefreshTokenRequestedScope Arguments + RefreshTokenGrantedScope Arguments + Request } @@ -23,3 +26,31 @@ func NewAccessRequest(session Session) *AccessRequest { func (a *AccessRequest) GetGrantTypes() Arguments { return a.GrantTypes } + +func (a *AccessRequest) GetRefreshTokenRequestedScopes() (scopes Arguments) { + if a.RefreshTokenRequestedScope == nil { + return a.RequestedScope + } + + return a.RefreshTokenRequestedScope +} + +func (a *AccessRequest) SetRefreshTokenRequestedScopes(scopes Arguments) { + a.RefreshTokenRequestedScope = scopes +} + +func (a *AccessRequest) GetRefreshTokenGrantedScopes() (scopes Arguments) { + if a.RefreshTokenGrantedScope == nil { + return a.GrantedScope + } + + return a.RefreshTokenGrantedScope +} + +func (a *AccessRequest) SetRefreshTokenGrantedScopes(scopes Arguments) { + a.RefreshTokenGrantedScope = scopes +} + +func (a *AccessRequest) SetGrantedScopes(scopes Arguments) { + a.GrantedScope = scopes +} diff --git a/handler/oauth2/flow_refresh.go b/handler/oauth2/flow_refresh.go index d9a5e8a01..be4d52429 100644 --- a/handler/oauth2/flow_refresh.go +++ b/handler/oauth2/flow_refresh.go @@ -68,7 +68,6 @@ func (c *RefreshTokenGrantHandler) HandleTokenEndpointRequest(ctx context.Contex scopeNames := strings.Join(c.Config.GetRefreshTokenScopes(ctx), " or ") hint := fmt.Sprintf("The OAuth 2.0 Client was not granted scope %s and may thus not perform the 'refresh_token' authorization grant.", scopeNames) return errorsx.WithStack(fosite.ErrScopeNotGranted.WithHint(hint)) - } // The authorization server MUST ... and ensure that the refresh token was issued to the authenticated client @@ -98,15 +97,25 @@ func (c *RefreshTokenGrantHandler) HandleTokenEndpointRequest(ctx context.Contex request.SetRequestedScopes(fosite.RemoveEmpty(strings.Split(scope, " "))) } + // 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. + if rtRequest, ok := request.(fosite.RefreshTokenAccessRequester); ok { + rtRequest.SetRefreshTokenRequestedScopes(originalRequest.GetRequestedScopes()) + rtRequest.SetRefreshTokenGrantedScopes(originalRequest.GetGrantedScopes()) + } + request.SetRequestedAudience(originalRequest.GetRequestedAudience()) + strategy := c.Config.GetScopeStrategy(ctx) + originalScopes := originalRequest.GetGrantedScopes() + for _, scope := range request.GetRequestedScopes() { // Addresses point 2 of the text in RFC6749 Section 6. - if !originalRequest.GetGrantedScopes().Has(scope) { + if !strategy(originalScopes, 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) { + if !strategy(request.GetClient().GetScopes(), scope) { return errorsx.WithStack(fosite.ErrInvalidScope.WithHintf("The OAuth 2.0 Client is not allowed to request scope '%s'.", scope)) } @@ -176,8 +185,24 @@ func (c *RefreshTokenGrantHandler) PopulateTokenEndpointResponse(ctx context.Con return err } - if err = c.TokenRevocationStorage.CreateRefreshTokenSession(ctx, refreshSignature, storeReq); err != nil { - return err + if rtRequest, ok := requester.(fosite.RefreshTokenAccessRequester); ok { + r := requester.Sanitize([]string{}).(*fosite.Request) + r.SetID(ts.GetID()) + + rtStoreReq := &fosite.AccessRequest{ + Request: *r, + } + + rtStoreReq.SetRequestedScopes(rtRequest.GetRefreshTokenRequestedScopes()) + rtStoreReq.SetGrantedScopes(rtRequest.GetRefreshTokenGrantedScopes()) + + if err = c.TokenRevocationStorage.CreateRefreshTokenSession(ctx, refreshSignature, rtStoreReq); err != nil { + return err + } + } else { + if err = c.TokenRevocationStorage.CreateRefreshTokenSession(ctx, refreshSignature, storeReq); err != nil { + return err + } } responder.SetAccessToken(accessToken) diff --git a/oauth2.go b/oauth2.go index c25abf65a..57d60ccfa 100644 --- a/oauth2.go +++ b/oauth2.go @@ -176,7 +176,7 @@ type IntrospectionResponder interface { // IsActive returns true if the introspected token is active and false otherwise. IsActive() bool - // AccessRequester returns nil when IsActive() is false and the original access request object otherwise. + // GetAccessRequester returns nil when IsActive() is false and the original access request object otherwise. GetAccessRequester() AccessRequester // GetTokenUse optionally returns the type of the token that was introspected. This could be "access_token", "refresh_token", @@ -217,7 +217,7 @@ type Requester interface { // AppendRequestedScope appends a scope to the request. AppendRequestedScope(scope string) - // GetGrantScopes returns all granted scopes. + // GetGrantedScopes returns all granted scopes. GetGrantedScopes() (grantedScopes Arguments) // GetGrantedAudience returns all granted audiences. @@ -245,9 +245,31 @@ type Requester interface { Sanitize(allowedParameters []string) Requester } +// RefreshTokenAccessRequester is an extended AccessRequester implementation that allows preserving +// the original Requester. +type RefreshTokenAccessRequester interface { + // GetRefreshTokenRequestedScopes returns the request's scopes specifically for the refresh token. + GetRefreshTokenRequestedScopes() (scopes Arguments) + + // SetRefreshTokenRequestedScopes sets the request's scopes specifically for the refresh token. + SetRefreshTokenRequestedScopes(scopes Arguments) + + // GetRefreshTokenGrantedScopes returns all granted scopes specifically for the refresh token. + GetRefreshTokenGrantedScopes() (scopes Arguments) + + // SetRefreshTokenGrantedScopes sets all granted scopes specifically for the refresh token. + SetRefreshTokenGrantedScopes(scopes Arguments) + + // SetGrantedScopes sets all granted scopes. This is specifically used in the refresh flow to restore the originally + // granted scopes to the session. + SetGrantedScopes(scopes Arguments) + + AccessRequester +} + // AccessRequester is a token endpoint's request context. type AccessRequester interface { - // GetGrantType returns the requests grant type. + // GetGrantTypes returns the requests grant type. GetGrantTypes() (grantTypes Arguments) Requester @@ -305,7 +327,7 @@ type AccessResponder interface { // SetTokenType set's the responses mandatory token type SetTokenType(tokenType string) - // SetAccessToken returns the responses access token. + // GetAccessToken returns the responses access token. GetAccessToken() (token string) // GetTokenType returns the responses token type. From c5c6056e46bbef87db5c0d42709269ab4de159e4 Mon Sep 17 00:00:00 2001 From: James Elliott Date: Mon, 21 Nov 2022 15:18:08 +1100 Subject: [PATCH 3/7] test: add integration test --- integration/helper_endpoints_test.go | 24 ++++--- integration/refresh_token_grant_test.go | 89 +++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 9 deletions(-) diff --git a/integration/helper_endpoints_test.go b/integration/helper_endpoints_test.go index 76e4b5201..dfb491fae 100644 --- a/integration/helper_endpoints_test.go +++ b/integration/helper_endpoints_test.go @@ -77,16 +77,22 @@ func authEndpointHandler(t *testing.T, oauth2 fosite.OAuth2Provider, session fos return } - if ar.GetRequestedScopes().Has("fosite") { - ar.GrantScope("fosite") - } + if ar.GetClient().GetID() == "grant-all-requested-scopes-client" { + for _, scope := range ar.GetRequestedScopes() { + ar.GrantScope(scope) + } + } else { + if ar.GetRequestedScopes().Has("fosite") { + ar.GrantScope("fosite") + } - if ar.GetRequestedScopes().Has("offline") { - ar.GrantScope("offline") - } + if ar.GetRequestedScopes().Has("offline") { + ar.GrantScope("offline") + } - if ar.GetRequestedScopes().Has("openid") { - ar.GrantScope("openid") + if ar.GetRequestedScopes().Has("openid") { + ar.GrantScope("openid") + } } for _, a := range ar.GetRequestedAudience() { @@ -134,7 +140,7 @@ func tokenEndpointHandler(t *testing.T, provider fosite.OAuth2Provider) func(rw accessRequest, err := provider.NewAccessRequest(ctx, req, &oauth2.JWTSession{}) if err != nil { - t.Logf("Access request failed because: %+v", err) + t.Logf("Access request failed because: %+v", fosite.ErrorToRFC6749Error(err).WithExposeDebug(true).GetDescription()) t.Logf("Request: %+v", accessRequest) provider.WriteAccessError(req.Context(), rw, accessRequest, err) return diff --git a/integration/refresh_token_grant_test.go b/integration/refresh_token_grant_test.go index 89f73db39..7e3693f39 100644 --- a/integration/refresh_token_grant_test.go +++ b/integration/refresh_token_grant_test.go @@ -265,3 +265,92 @@ func TestRefreshTokenFlow(t *testing.T) { }) } } + +func TestRefreshTokenFlowScopeNarrowing(t *testing.T) { + session := &defaultSession{ + DefaultSession: &openid.DefaultSession{ + Claims: &jwt.IDTokenClaims{ + Subject: "peter", + }, + Headers: &jwt.Headers{}, + Subject: "peter", + Username: "peteru", + }, + } + fc := new(fosite.Config) + fc.RefreshTokenLifespan = -1 + fc.GlobalSecret = []byte("some-secret-thats-random-some-secret-thats-random-") + f := compose.ComposeAllEnabled(fc, fositeStore, gen.MustRSAKey()) + ts := mockServer(t, f, session) + defer ts.Close() + + fc.ScopeStrategy = fosite.ExactScopeStrategy + + oauthClient := newOAuth2Client(ts) + oauthClient.Scopes = []string{"openid", "offline", "offline_access", "foo", "bar"} + oauthClient.ClientID = "grant-all-requested-scopes-client" + + state := "1234567890" + + testRefreshingClient := &fosite.DefaultClient{ + ID: "grant-all-requested-scopes-client", + Secret: []byte(`$2a$10$IxMdI6d.LIRZPpSfEwNoeu4rY3FhDREsxFJXikcgdRRAStxUlsuEO`), // = "foobar" + RedirectURIs: []string{ts.URL + "/callback"}, + ResponseTypes: []string{"code"}, + GrantTypes: []string{"implicit", "refresh_token", "authorization_code", "password", "client_credentials"}, + Scopes: []string{"openid", "offline_access", "offline", "foo", "bar", "baz"}, + Audience: []string{"https://www.ory.sh/api"}, + } + + fositeStore.Clients["grant-all-requested-scopes-client"] = testRefreshingClient + + resp, err := http.Get(oauthClient.AuthCodeURL(state)) + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode) + + token, err := oauthClient.Exchange(oauth2.NoContext, resp.Request.URL.Query().Get("code"), oauth2.SetAuthURLParam("client_id", oauthClient.ClientID)) + require.NoError(t, err) + require.NotEmpty(t, token.AccessToken) + require.NotEmpty(t, token.RefreshToken) + + assert.Equal(t, "openid offline offline_access foo bar", token.Extra("scope")) + + token1Refresh, err := doRefresh(oauthClient, token, nil) + require.NoError(t, err) + require.NotEmpty(t, token1Refresh.AccessToken) + require.NotEmpty(t, token1Refresh.RefreshToken) + + assert.Equal(t, "openid offline offline_access foo bar", token1Refresh.Extra("scope")) + + token2Refresh, err := doRefresh(oauthClient, token1Refresh, []string{"openid", "offline_access", "foo"}) + require.NoError(t, err) + require.NotEmpty(t, token2Refresh.AccessToken) + require.NotEmpty(t, token2Refresh.RefreshToken) + + assert.Equal(t, "openid offline_access foo", token2Refresh.Extra("scope")) + + token3Refresh, err := doRefresh(oauthClient, token2Refresh, []string{"openid", "offline", "offline_access", "foo", "bar"}) + require.NoError(t, err) + require.NotEmpty(t, token3Refresh.AccessToken) + require.NotEmpty(t, token3Refresh.RefreshToken) + + assert.Equal(t, "openid offline offline_access foo bar", token3Refresh.Extra("scope")) + + token4Refresh, err := doRefresh(oauthClient, token3Refresh, []string{"openid", "offline", "offline_access", "foo", "bar", "baz"}) + require.Error(t, err) + require.Nil(t, token4Refresh) + require.Contains(t, err.Error(), "The requested scope is invalid, unknown, or malformed. The requested scope 'baz' was not originally granted by the resource owner.") +} + +func doRefresh(client *oauth2.Config, t *oauth2.Token, scopes []string) (token *oauth2.Token, err error) { + opts := []oauth2.AuthCodeOption{ + oauth2.SetAuthURLParam("refresh_token", t.RefreshToken), + oauth2.SetAuthURLParam("grant_type", "refresh_token"), + } + + if len(scopes) != 0 { + opts = append(opts, oauth2.SetAuthURLParam("scope", strings.Join(scopes, " ")), oauth2.SetAuthURLParam("client_id", client.ClientID)) + } + + return client.Exchange(oauth2.NoContext, "", opts...) +} From deb3be373779290d8a02eafc89b24c1cd964f358 Mon Sep 17 00:00:00 2001 From: James Elliott Date: Tue, 13 Dec 2022 09:48:59 +1100 Subject: [PATCH 4/7] refactor: remove unnecessary receiver --- access_request.go | 4 ---- handler/oauth2/flow_refresh.go | 12 ++++-------- oauth2.go | 4 ---- 3 files changed, 4 insertions(+), 16 deletions(-) diff --git a/access_request.go b/access_request.go index 5e24970f2..6fc21680a 100644 --- a/access_request.go +++ b/access_request.go @@ -50,7 +50,3 @@ func (a *AccessRequest) GetRefreshTokenGrantedScopes() (scopes Arguments) { func (a *AccessRequest) SetRefreshTokenGrantedScopes(scopes Arguments) { a.RefreshTokenGrantedScope = scopes } - -func (a *AccessRequest) SetGrantedScopes(scopes Arguments) { - a.GrantedScope = scopes -} diff --git a/handler/oauth2/flow_refresh.go b/handler/oauth2/flow_refresh.go index be4d52429..c520863bc 100644 --- a/handler/oauth2/flow_refresh.go +++ b/handler/oauth2/flow_refresh.go @@ -186,15 +186,11 @@ func (c *RefreshTokenGrantHandler) PopulateTokenEndpointResponse(ctx context.Con } if rtRequest, ok := requester.(fosite.RefreshTokenAccessRequester); ok { - r := requester.Sanitize([]string{}).(*fosite.Request) - r.SetID(ts.GetID()) + rtStoreReq := requester.Sanitize([]string{}).(*fosite.Request) + rtStoreReq.SetID(ts.GetID()) - rtStoreReq := &fosite.AccessRequest{ - Request: *r, - } - - rtStoreReq.SetRequestedScopes(rtRequest.GetRefreshTokenRequestedScopes()) - rtStoreReq.SetGrantedScopes(rtRequest.GetRefreshTokenGrantedScopes()) + rtStoreReq.RequestedScope = rtRequest.GetRefreshTokenRequestedScopes() + rtStoreReq.GrantedScope = rtRequest.GetRefreshTokenGrantedScopes() if err = c.TokenRevocationStorage.CreateRefreshTokenSession(ctx, refreshSignature, rtStoreReq); err != nil { return err diff --git a/oauth2.go b/oauth2.go index 57d60ccfa..65b6227d0 100644 --- a/oauth2.go +++ b/oauth2.go @@ -260,10 +260,6 @@ type RefreshTokenAccessRequester interface { // SetRefreshTokenGrantedScopes sets all granted scopes specifically for the refresh token. SetRefreshTokenGrantedScopes(scopes Arguments) - // SetGrantedScopes sets all granted scopes. This is specifically used in the refresh flow to restore the originally - // granted scopes to the session. - SetGrantedScopes(scopes Arguments) - AccessRequester } From 6db02e88edbb6e1a3427ac1e4dd6e7980b3b7cdd Mon Sep 17 00:00:00 2001 From: James Elliott Date: Wed, 19 Jul 2023 21:04:33 +1000 Subject: [PATCH 5/7] refactor: simplify logic --- access_request.go | 30 ++-- handler/oauth2/flow_refresh.go | 21 +-- integration/refresh_token_grant_test.go | 174 ++++++++++++++++++------ oauth2.go | 14 +- 4 files changed, 154 insertions(+), 85 deletions(-) diff --git a/access_request.go b/access_request.go index 6fc21680a..4f1583757 100644 --- a/access_request.go +++ b/access_request.go @@ -7,9 +7,6 @@ type AccessRequest struct { GrantTypes Arguments `json:"grantTypes" gorethink:"grantTypes"` HandledGrantType Arguments `json:"handledGrantType" gorethink:"handledGrantType"` - RefreshTokenRequestedScope Arguments - RefreshTokenGrantedScope Arguments - Request } @@ -27,26 +24,21 @@ func (a *AccessRequest) GetGrantTypes() Arguments { return a.GrantTypes } -func (a *AccessRequest) GetRefreshTokenRequestedScopes() (scopes Arguments) { - if a.RefreshTokenRequestedScope == nil { - return a.RequestedScope - } - - return a.RefreshTokenRequestedScope +func (a *AccessRequest) SetGrantedScopes(scopes Arguments) { + a.GrantedScope = scopes } -func (a *AccessRequest) SetRefreshTokenRequestedScopes(scopes Arguments) { - a.RefreshTokenRequestedScope = scopes -} +func (a *AccessRequest) SanitizeRestoreRefreshTokenOriginalRequester(requester Requester) Requester { + r := a.Sanitize(nil).(*Request) -func (a *AccessRequest) GetRefreshTokenGrantedScopes() (scopes Arguments) { - if a.RefreshTokenGrantedScope == nil { - return a.GrantedScope + ar := &AccessRequest{ + Request: *r, } - return a.RefreshTokenGrantedScope -} + ar.SetID(requester.GetID()) + + ar.SetRequestedScopes(requester.GetRequestedScopes()) + ar.SetGrantedScopes(requester.GetGrantedScopes()) -func (a *AccessRequest) SetRefreshTokenGrantedScopes(scopes Arguments) { - a.RefreshTokenGrantedScope = scopes + return ar } diff --git a/handler/oauth2/flow_refresh.go b/handler/oauth2/flow_refresh.go index c520863bc..659c4ea34 100644 --- a/handler/oauth2/flow_refresh.go +++ b/handler/oauth2/flow_refresh.go @@ -97,13 +97,6 @@ func (c *RefreshTokenGrantHandler) HandleTokenEndpointRequest(ctx context.Contex request.SetRequestedScopes(fosite.RemoveEmpty(strings.Split(scope, " "))) } - // 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. - if rtRequest, ok := request.(fosite.RefreshTokenAccessRequester); ok { - rtRequest.SetRefreshTokenRequestedScopes(originalRequest.GetRequestedScopes()) - rtRequest.SetRefreshTokenGrantedScopes(originalRequest.GetGrantedScopes()) - } - request.SetRequestedAudience(originalRequest.GetRequestedAudience()) strategy := c.Config.GetScopeStrategy(ctx) @@ -167,30 +160,28 @@ func (c *RefreshTokenGrantHandler) PopulateTokenEndpointResponse(ctx context.Con err = c.handleRefreshTokenEndpointStorageError(ctx, err) }() - ts, err := c.TokenRevocationStorage.GetRefreshTokenSession(ctx, signature, nil) + originalRequest, err := c.TokenRevocationStorage.GetRefreshTokenSession(ctx, signature, nil) if err != nil { return err - } else if err := c.TokenRevocationStorage.RevokeAccessToken(ctx, ts.GetID()); err != nil { + } else if err := c.TokenRevocationStorage.RevokeAccessToken(ctx, originalRequest.GetID()); err != nil { return err } - if err := c.TokenRevocationStorage.RevokeRefreshTokenMaybeGracePeriod(ctx, ts.GetID(), signature); err != nil { + if err := c.TokenRevocationStorage.RevokeRefreshTokenMaybeGracePeriod(ctx, originalRequest.GetID(), signature); err != nil { return err } storeReq := requester.Sanitize([]string{}) - storeReq.SetID(ts.GetID()) + storeReq.SetID(originalRequest.GetID()) if err = c.TokenRevocationStorage.CreateAccessTokenSession(ctx, accessSignature, storeReq); err != nil { return err } if rtRequest, ok := requester.(fosite.RefreshTokenAccessRequester); ok { - rtStoreReq := requester.Sanitize([]string{}).(*fosite.Request) - rtStoreReq.SetID(ts.GetID()) + rtStoreReq := rtRequest.SanitizeRestoreRefreshTokenOriginalRequester(originalRequest) - rtStoreReq.RequestedScope = rtRequest.GetRefreshTokenRequestedScopes() - rtStoreReq.GrantedScope = rtRequest.GetRefreshTokenGrantedScopes() + rtStoreReq.SetSession(requester.GetSession().Clone()) if err = c.TokenRevocationStorage.CreateRefreshTokenSession(ctx, refreshSignature, rtStoreReq); err != nil { return err diff --git a/integration/refresh_token_grant_test.go b/integration/refresh_token_grant_test.go index 7e3693f39..c91c8be61 100644 --- a/integration/refresh_token_grant_test.go +++ b/integration/refresh_token_grant_test.go @@ -4,6 +4,7 @@ package integration_test import ( + "context" "encoding/json" "net/http" "net/http/httptest" @@ -12,8 +13,6 @@ import ( "testing" "time" - "github.com/ory/fosite/internal/gen" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/oauth2" @@ -21,6 +20,7 @@ import ( "github.com/ory/fosite" "github.com/ory/fosite/compose" "github.com/ory/fosite/handler/openid" + "github.com/ory/fosite/internal/gen" "github.com/ory/fosite/token/jwt" ) @@ -266,7 +266,9 @@ func TestRefreshTokenFlow(t *testing.T) { } } -func TestRefreshTokenFlowScopeNarrowing(t *testing.T) { +func TestRefreshTokenFlowScopeParameter(t *testing.T) { + ctx := context.Background() + session := &defaultSession{ DefaultSession: &openid.DefaultSession{ Claims: &jwt.IDTokenClaims{ @@ -278,7 +280,6 @@ func TestRefreshTokenFlowScopeNarrowing(t *testing.T) { }, } fc := new(fosite.Config) - fc.RefreshTokenLifespan = -1 fc.GlobalSecret = []byte("some-secret-thats-random-some-secret-thats-random-") f := compose.ComposeAllEnabled(fc, fositeStore, gen.MustRSAKey()) ts := mockServer(t, f, session) @@ -286,9 +287,9 @@ func TestRefreshTokenFlowScopeNarrowing(t *testing.T) { fc.ScopeStrategy = fosite.ExactScopeStrategy - oauthClient := newOAuth2Client(ts) - oauthClient.Scopes = []string{"openid", "offline", "offline_access", "foo", "bar"} - oauthClient.ClientID = "grant-all-requested-scopes-client" + client := newOAuth2Client(ts) + client.Scopes = []string{"openid", "offline", "offline_access", "foo", "bar"} + client.ClientID = "grant-all-requested-scopes-client" state := "1234567890" @@ -304,53 +305,146 @@ func TestRefreshTokenFlowScopeNarrowing(t *testing.T) { fositeStore.Clients["grant-all-requested-scopes-client"] = testRefreshingClient - resp, err := http.Get(oauthClient.AuthCodeURL(state)) + s := compose.NewOAuth2HMACStrategy(fc) + + originalScopes := fosite.Arguments{"openid", "offline", "offline_access", "foo", "bar"} + + testCases := []struct { + name string + scopes fosite.Arguments + expected fosite.Arguments + err string + }{ + { + "ShouldGrantOriginalScopesWhenOmitted", + nil, + originalScopes, + "", + }, + { + "ShouldNarrowScopesWhenIncluded", + fosite.Arguments{"openid", "offline_access", "foo"}, + fosite.Arguments{"openid", "offline_access", "foo"}, + "", + }, + { + "ShouldGrantOriginalScopesWhenOmittedAfterNarrowing", + nil, + originalScopes, + "", + }, + { + "ShouldGrantOriginalScopesExplicitlyRequested", + originalScopes, + originalScopes, + "", + }, + { + "ShouldErrorWhenBroadeningScopesAllowedByClientButNotOriginallyGranted", + fosite.Arguments{"openid", "offline", "offline_access", "foo", "bar", "baz"}, + nil, + "The requested scope is invalid, unknown, or malformed. The requested scope 'baz' was not originally granted by the resource owner.", + }, + } + + type step struct { + OAuth2 *oauth2.Token + SessionAT, SessionRT fosite.Requester + } + + entries := make([]step, len(testCases)+1) + + resp, err := http.Get(client.AuthCodeURL(state)) require.NoError(t, err) require.Equal(t, http.StatusOK, resp.StatusCode) - token, err := oauthClient.Exchange(oauth2.NoContext, resp.Request.URL.Query().Get("code"), oauth2.SetAuthURLParam("client_id", oauthClient.ClientID)) + entries[0].OAuth2, err = client.Exchange(ctx, resp.Request.URL.Query().Get("code"), oauth2.SetAuthURLParam("client_id", client.ClientID)) + require.NoError(t, err) - require.NotEmpty(t, token.AccessToken) - require.NotEmpty(t, token.RefreshToken) + require.NotEmpty(t, entries[0].OAuth2.AccessToken) + require.NotEmpty(t, entries[0].OAuth2.RefreshToken) - assert.Equal(t, "openid offline offline_access foo bar", token.Extra("scope")) + assert.Equal(t, strings.Join(originalScopes, " "), entries[0].OAuth2.Extra("scope")) - token1Refresh, err := doRefresh(oauthClient, token, nil) + entries[0].SessionAT, err = fositeStore.GetAccessTokenSession(ctx, s.AccessTokenSignature(ctx, entries[0].OAuth2.AccessToken), nil) require.NoError(t, err) - require.NotEmpty(t, token1Refresh.AccessToken) - require.NotEmpty(t, token1Refresh.RefreshToken) - - assert.Equal(t, "openid offline offline_access foo bar", token1Refresh.Extra("scope")) - token2Refresh, err := doRefresh(oauthClient, token1Refresh, []string{"openid", "offline_access", "foo"}) + entries[0].SessionRT, err = fositeStore.GetRefreshTokenSession(ctx, s.RefreshTokenSignature(ctx, entries[0].OAuth2.RefreshToken), nil) require.NoError(t, err) - require.NotEmpty(t, token2Refresh.AccessToken) - require.NotEmpty(t, token2Refresh.RefreshToken) - assert.Equal(t, "openid offline_access foo", token2Refresh.Extra("scope")) + assert.ElementsMatch(t, entries[0].SessionAT.GetRequestedScopes(), originalScopes) + assert.ElementsMatch(t, entries[0].SessionRT.GetRequestedScopes(), originalScopes) + assert.ElementsMatch(t, entries[0].SessionAT.GetGrantedScopes(), originalScopes) + assert.ElementsMatch(t, entries[0].SessionRT.GetGrantedScopes(), originalScopes) + assert.Equal(t, strings.Join(originalScopes, " "), entries[0].OAuth2.Extra("scope")) - token3Refresh, err := doRefresh(oauthClient, token2Refresh, []string{"openid", "offline", "offline_access", "foo", "bar"}) - require.NoError(t, err) - require.NotEmpty(t, token3Refresh.AccessToken) - require.NotEmpty(t, token3Refresh.RefreshToken) + for i, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + time.Sleep(time.Second) - assert.Equal(t, "openid offline offline_access foo bar", token3Refresh.Extra("scope")) + idx := i + 1 - token4Refresh, err := doRefresh(oauthClient, token3Refresh, []string{"openid", "offline", "offline_access", "foo", "bar", "baz"}) - require.Error(t, err) - require.Nil(t, token4Refresh) - require.Contains(t, err.Error(), "The requested scope is invalid, unknown, or malformed. The requested scope 'baz' was not originally granted by the resource owner.") -} + opts := []oauth2.AuthCodeOption{ + oauth2.SetAuthURLParam("refresh_token", entries[i].OAuth2.RefreshToken), + oauth2.SetAuthURLParam("grant_type", "refresh_token"), + } -func doRefresh(client *oauth2.Config, t *oauth2.Token, scopes []string) (token *oauth2.Token, err error) { - opts := []oauth2.AuthCodeOption{ - oauth2.SetAuthURLParam("refresh_token", t.RefreshToken), - oauth2.SetAuthURLParam("grant_type", "refresh_token"), - } + if len(tc.scopes) != 0 { + opts = append(opts, oauth2.SetAuthURLParam("scope", strings.Join(tc.scopes, " ")), oauth2.SetAuthURLParam("client_id", client.ClientID)) + } - if len(scopes) != 0 { - opts = append(opts, oauth2.SetAuthURLParam("scope", strings.Join(scopes, " ")), oauth2.SetAuthURLParam("client_id", client.ClientID)) - } + entries[idx].OAuth2, err = client.Exchange(ctx, "", opts...) + if len(tc.err) != 0 { + require.Error(t, err) + require.Nil(t, entries[idx].OAuth2) + require.Contains(t, err.Error(), tc.err) + + return + } - return client.Exchange(oauth2.NoContext, "", opts...) + require.NoError(t, err) + require.NotEmpty(t, entries[idx].OAuth2.AccessToken) + require.NotEmpty(t, entries[idx].OAuth2.RefreshToken) + + entries[idx].SessionAT, err = fositeStore.GetAccessTokenSession(ctx, s.AccessTokenSignature(ctx, entries[idx].OAuth2.AccessToken), nil) + require.NoError(t, err) + + entries[idx].SessionRT, err = fositeStore.GetRefreshTokenSession(ctx, s.RefreshTokenSignature(ctx, entries[idx].OAuth2.RefreshToken), nil) + require.NoError(t, err) + + if len(tc.scopes) != 0 { + assert.ElementsMatch(t, entries[idx].SessionAT.GetRequestedScopes(), tc.scopes) + assert.Equal(t, strings.Join(tc.expected, " "), entries[idx].OAuth2.Extra("scope")) + } else { + assert.ElementsMatch(t, entries[idx].SessionAT.GetRequestedScopes(), originalScopes) + assert.Equal(t, strings.Join(originalScopes, " "), entries[idx].OAuth2.Extra("scope")) + } + assert.ElementsMatch(t, entries[idx].SessionAT.GetGrantedScopes(), tc.expected) + assert.ElementsMatch(t, entries[idx].SessionRT.GetRequestedScopes(), originalScopes) + assert.ElementsMatch(t, entries[idx].SessionRT.GetGrantedScopes(), originalScopes) + + var ( + j int + entry step + ) + + assert.Equal(t, entries[idx].SessionAT.GetID(), entries[idx].SessionRT.GetID()) + + for j, entry = range entries { + if j == idx { + break + } + + assert.Equal(t, entries[idx].SessionAT.GetID(), entry.SessionAT.GetID()) + assert.Equal(t, entries[idx].SessionAT.GetID(), entry.SessionRT.GetID()) + assert.Equal(t, entries[idx].SessionRT.GetID(), entry.SessionAT.GetID()) + assert.Equal(t, entries[idx].SessionRT.GetID(), entry.SessionRT.GetID()) + + assert.Greater(t, entries[idx].SessionAT.GetSession().GetExpiresAt(fosite.AccessToken).Unix(), entry.SessionAT.GetSession().GetExpiresAt(fosite.AccessToken).Unix()) + assert.Greater(t, entries[idx].SessionRT.GetSession().GetExpiresAt(fosite.RefreshToken).Unix(), entry.SessionRT.GetSession().GetExpiresAt(fosite.RefreshToken).Unix()) + assert.Greater(t, entries[idx].SessionAT.GetRequestedAt().Unix(), entry.SessionAT.GetRequestedAt().Unix()) + assert.Greater(t, entries[idx].SessionRT.GetRequestedAt().Unix(), entry.SessionRT.GetRequestedAt().Unix()) + } + }) + } } diff --git a/oauth2.go b/oauth2.go index 65b6227d0..fcd0164d1 100644 --- a/oauth2.go +++ b/oauth2.go @@ -248,17 +248,9 @@ type Requester interface { // RefreshTokenAccessRequester is an extended AccessRequester implementation that allows preserving // the original Requester. type RefreshTokenAccessRequester interface { - // GetRefreshTokenRequestedScopes returns the request's scopes specifically for the refresh token. - GetRefreshTokenRequestedScopes() (scopes Arguments) - - // SetRefreshTokenRequestedScopes sets the request's scopes specifically for the refresh token. - SetRefreshTokenRequestedScopes(scopes Arguments) - - // GetRefreshTokenGrantedScopes returns all granted scopes specifically for the refresh token. - GetRefreshTokenGrantedScopes() (scopes Arguments) - - // SetRefreshTokenGrantedScopes sets all granted scopes specifically for the refresh token. - SetRefreshTokenGrantedScopes(scopes Arguments) + // SanitizeRestoreRefreshTokenOriginalRequester returns a sanitized copy of this Requester and mutates the relevant + // values from the provided Requester which is the original refresh token session Requester. + SanitizeRestoreRefreshTokenOriginalRequester(requester Requester) Requester AccessRequester } From c472cd8c5ba789a1d648842aed0ef38f44c0b364 Mon Sep 17 00:00:00 2001 From: James Elliott Date: Fri, 21 Jul 2023 09:28:46 +1000 Subject: [PATCH 6/7] feat: filtering mode --- handler/oauth2/flow_refresh.go | 23 +- handler/oauth2/flow_refresh_test.go | 4 + integration/refresh_token_grant_test.go | 395 +++++++++++++++--------- 3 files changed, 272 insertions(+), 150 deletions(-) diff --git a/handler/oauth2/flow_refresh.go b/handler/oauth2/flow_refresh.go index 659c4ea34..dd9c9f831 100644 --- a/handler/oauth2/flow_refresh.go +++ b/handler/oauth2/flow_refresh.go @@ -29,6 +29,11 @@ type RefreshTokenGrantHandler struct { fosite.AudienceStrategyProvider fosite.RefreshTokenScopesProvider } + + // IgnoreRequestedScopeNotInOriginalGrant determines the action to take when the requested scopes in the refresh + // flow were not originally granted. If false which is the default the handler will automatically return an error. + // If true the handler will filter out / ignore the scopes which were not originally granted. + IgnoreRequestedScopeNotInOriginalGrant bool } // HandleTokenEndpointRequest implements https://tools.ietf.org/html/rfc6749#section-6 @@ -89,12 +94,10 @@ func (c *RefreshTokenGrantHandler) HandleTokenEndpointRequest(ctx context.Contex 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. + + // Addresses point 1 of the text in RFC6749 Section 6. + if len(request.GetRequestedScopes()) == 0 { request.SetRequestedScopes(originalRequest.GetGrantedScopes()) - default: - request.SetRequestedScopes(fosite.RemoveEmpty(strings.Split(scope, " "))) } request.SetRequestedAudience(originalRequest.GetRequestedAudience()) @@ -103,9 +106,15 @@ func (c *RefreshTokenGrantHandler) HandleTokenEndpointRequest(ctx context.Contex originalScopes := originalRequest.GetGrantedScopes() for _, scope := range request.GetRequestedScopes() { - // Addresses point 2 of the text in RFC6749 Section 6. if !strategy(originalScopes, scope) { - return errorsx.WithStack(fosite.ErrInvalidScope.WithHintf("The requested scope '%s' was not originally granted by the resource owner.", scope)) + if c.IgnoreRequestedScopeNotInOriginalGrant { + // Skips addressing point 2 of the text in RFC6749 Section 6 and instead just prevents the scope + // requested from being granted. + continue + } else { + // Addresses point 2 of the text in RFC6749 Section 6. + return errorsx.WithStack(fosite.ErrInvalidScope.WithHintf("The requested scope '%s' was not originally granted by the resource owner.", scope)) + } } if !strategy(request.GetClient().GetScopes(), scope) { diff --git a/handler/oauth2/flow_refresh_test.go b/handler/oauth2/flow_refresh_test.go index 893b59778..0683eb709 100644 --- a/handler/oauth2/flow_refresh_test.go +++ b/handler/oauth2/flow_refresh_test.go @@ -222,6 +222,8 @@ func TestRefreshFlow_HandleTokenEndpointRequest(t *testing.T) { areq.Form.Add("refresh_token", token) areq.Form.Add("scope", "foo bar offline") + areq.SetRequestedScopes(fosite.Arguments{"foo", "bar", "offline"}) + err = store.CreateRefreshTokenSession(nil, sig, &fosite.Request{ Client: areq.Client, GrantedScope: fosite.Arguments{"foo", "bar", "baz", "offline"}, @@ -252,6 +254,8 @@ func TestRefreshFlow_HandleTokenEndpointRequest(t *testing.T) { areq.Form.Add("refresh_token", token) areq.Form.Add("scope", "foo bar offline") + areq.SetRequestedScopes(fosite.Arguments{"foo", "bar", "offline"}) + err = store.CreateRefreshTokenSession(nil, sig, &fosite.Request{ Client: areq.Client, GrantedScope: fosite.Arguments{"foo", "baz", "offline"}, diff --git a/integration/refresh_token_grant_test.go b/integration/refresh_token_grant_test.go index c91c8be61..94257897b 100644 --- a/integration/refresh_token_grant_test.go +++ b/integration/refresh_token_grant_test.go @@ -19,6 +19,7 @@ import ( "github.com/ory/fosite" "github.com/ory/fosite/compose" + hoauth2 "github.com/ory/fosite/handler/oauth2" "github.com/ory/fosite/handler/openid" "github.com/ory/fosite/internal/gen" "github.com/ory/fosite/token/jwt" @@ -267,84 +268,11 @@ func TestRefreshTokenFlow(t *testing.T) { } func TestRefreshTokenFlowScopeParameter(t *testing.T) { - ctx := context.Background() - - session := &defaultSession{ - DefaultSession: &openid.DefaultSession{ - Claims: &jwt.IDTokenClaims{ - Subject: "peter", - }, - Headers: &jwt.Headers{}, - Subject: "peter", - Username: "peteru", - }, - } - fc := new(fosite.Config) - fc.GlobalSecret = []byte("some-secret-thats-random-some-secret-thats-random-") - f := compose.ComposeAllEnabled(fc, fositeStore, gen.MustRSAKey()) - ts := mockServer(t, f, session) - defer ts.Close() - - fc.ScopeStrategy = fosite.ExactScopeStrategy - - client := newOAuth2Client(ts) - client.Scopes = []string{"openid", "offline", "offline_access", "foo", "bar"} - client.ClientID = "grant-all-requested-scopes-client" - - state := "1234567890" - - testRefreshingClient := &fosite.DefaultClient{ - ID: "grant-all-requested-scopes-client", - Secret: []byte(`$2a$10$IxMdI6d.LIRZPpSfEwNoeu4rY3FhDREsxFJXikcgdRRAStxUlsuEO`), // = "foobar" - RedirectURIs: []string{ts.URL + "/callback"}, - ResponseTypes: []string{"code"}, - GrantTypes: []string{"implicit", "refresh_token", "authorization_code", "password", "client_credentials"}, - Scopes: []string{"openid", "offline_access", "offline", "foo", "bar", "baz"}, - Audience: []string{"https://www.ory.sh/api"}, - } - - fositeStore.Clients["grant-all-requested-scopes-client"] = testRefreshingClient - - s := compose.NewOAuth2HMACStrategy(fc) - - originalScopes := fosite.Arguments{"openid", "offline", "offline_access", "foo", "bar"} - - testCases := []struct { + type testCase struct { name string scopes fosite.Arguments expected fosite.Arguments err string - }{ - { - "ShouldGrantOriginalScopesWhenOmitted", - nil, - originalScopes, - "", - }, - { - "ShouldNarrowScopesWhenIncluded", - fosite.Arguments{"openid", "offline_access", "foo"}, - fosite.Arguments{"openid", "offline_access", "foo"}, - "", - }, - { - "ShouldGrantOriginalScopesWhenOmittedAfterNarrowing", - nil, - originalScopes, - "", - }, - { - "ShouldGrantOriginalScopesExplicitlyRequested", - originalScopes, - originalScopes, - "", - }, - { - "ShouldErrorWhenBroadeningScopesAllowedByClientButNotOriginallyGranted", - fosite.Arguments{"openid", "offline", "offline_access", "foo", "bar", "baz"}, - nil, - "The requested scope is invalid, unknown, or malformed. The requested scope 'baz' was not originally granted by the resource owner.", - }, } type step struct { @@ -352,98 +280,279 @@ func TestRefreshTokenFlowScopeParameter(t *testing.T) { SessionAT, SessionRT fosite.Requester } - entries := make([]step, len(testCases)+1) - - resp, err := http.Get(client.AuthCodeURL(state)) - require.NoError(t, err) - require.Equal(t, http.StatusOK, resp.StatusCode) + originalScopes := fosite.Arguments{"openid", "offline", "offline_access", "foo", "bar"} - entries[0].OAuth2, err = client.Exchange(ctx, resp.Request.URL.Query().Get("code"), oauth2.SetAuthURLParam("client_id", client.ClientID)) + scenarios := []struct { + name string + ignore bool + checkTime bool + testCases []testCase + }{ + { + "ShouldPassRFC", + false, + true, + []testCase{ + { + "ShouldGrantOriginalScopesWhenOmitted", + nil, + originalScopes, + "", + }, + { + "ShouldNarrowScopesWhenIncluded", + fosite.Arguments{"openid", "offline_access", "foo"}, + fosite.Arguments{"openid", "offline_access", "foo"}, + "", + }, + { + "ShouldGrantOriginalScopesWhenOmittedAfterNarrowing", + nil, + originalScopes, + "", + }, + { + "ShouldGrantOriginalScopesExplicitlyRequested", + originalScopes, + originalScopes, + "", + }, + { + "ShouldErrorWhenBroadeningScopesAllowedByClientButNotOriginallyGranted", + fosite.Arguments{"openid", "offline", "offline_access", "foo", "bar", "baz"}, + nil, + "The requested scope is invalid, unknown, or malformed. The requested scope 'baz' was not originally granted by the resource owner.", + }, + }, + }, + { + "ShouldPassIgnoreFilter", + true, + false, + []testCase{ + { + "ShouldGrantOriginalScopesWhenOmitted", + nil, + originalScopes, + "", + }, + { + "ShouldNarrowScopesWhenIncluded", + fosite.Arguments{"openid", "offline_access", "foo"}, + fosite.Arguments{"openid", "offline_access", "foo"}, + "", + }, + { + "ShouldGrantOriginalScopesWhenOmittedAfterNarrowing", + nil, + originalScopes, + "", + }, + { + "ShouldGrantOriginalScopesExplicitlyRequested", + originalScopes, + originalScopes, + "", + }, + { + "ShouldErrorWhenBroadeningScopesAllowedByClientButNotOriginallyGranted", + fosite.Arguments{"openid", "offline", "offline_access", "foo", "bar", "baz"}, + fosite.Arguments{"openid", "offline", "offline_access", "foo", "bar"}, + "", + }, + }, + }, + } - require.NoError(t, err) - require.NotEmpty(t, entries[0].OAuth2.AccessToken) - require.NotEmpty(t, entries[0].OAuth2.RefreshToken) + state := "1234567890" - assert.Equal(t, strings.Join(originalScopes, " "), entries[0].OAuth2.Extra("scope")) + for _, scenario := range scenarios { + t.Run(scenario.name, func(t *testing.T) { + ctx := context.Background() + + session := &defaultSession{ + DefaultSession: &openid.DefaultSession{ + Claims: &jwt.IDTokenClaims{ + Subject: "peter", + }, + Headers: &jwt.Headers{}, + Subject: "peter", + Username: "peteru", + }, + } - entries[0].SessionAT, err = fositeStore.GetAccessTokenSession(ctx, s.AccessTokenSignature(ctx, entries[0].OAuth2.AccessToken), nil) - require.NoError(t, err) + fc := new(fosite.Config) + fc.GlobalSecret = []byte("some-secret-thats-random-some-secret-thats-random-") + fc.ScopeStrategy = fosite.ExactScopeStrategy - entries[0].SessionRT, err = fositeStore.GetRefreshTokenSession(ctx, s.RefreshTokenSignature(ctx, entries[0].OAuth2.RefreshToken), nil) - require.NoError(t, err) + s := compose.NewOAuth2HMACStrategy(fc) - assert.ElementsMatch(t, entries[0].SessionAT.GetRequestedScopes(), originalScopes) - assert.ElementsMatch(t, entries[0].SessionRT.GetRequestedScopes(), originalScopes) - assert.ElementsMatch(t, entries[0].SessionAT.GetGrantedScopes(), originalScopes) - assert.ElementsMatch(t, entries[0].SessionRT.GetGrantedScopes(), originalScopes) - assert.Equal(t, strings.Join(originalScopes, " "), entries[0].OAuth2.Extra("scope")) + var f fosite.OAuth2Provider - for i, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - time.Sleep(time.Second) + if scenario.ignore { + keyGetter := func(context.Context) (interface{}, error) { + return gen.MustRSAKey(), nil + } - idx := i + 1 + // OAuth2RefreshTokenGrantFactory creates an OAuth2 refresh grant handler and registers + // an access token, refresh token and authorize code validator.nmj + factoryRefresh := func(config fosite.Configurator, storage interface{}, strategy interface{}) interface{} { + return &hoauth2.RefreshTokenGrantHandler{ + AccessTokenStrategy: strategy.(hoauth2.AccessTokenStrategy), + RefreshTokenStrategy: strategy.(hoauth2.RefreshTokenStrategy), + TokenRevocationStorage: storage.(hoauth2.TokenRevocationStorage), + Config: config, + IgnoreRequestedScopeNotInOriginalGrant: true, + } + } - opts := []oauth2.AuthCodeOption{ - oauth2.SetAuthURLParam("refresh_token", entries[i].OAuth2.RefreshToken), - oauth2.SetAuthURLParam("grant_type", "refresh_token"), + f = compose.Compose( + fc, + fositeStore, + &compose.CommonStrategy{ + CoreStrategy: compose.NewOAuth2HMACStrategy(fc), + OpenIDConnectTokenStrategy: compose.NewOpenIDConnectStrategy(keyGetter, fc), + Signer: &jwt.DefaultSigner{GetPrivateKey: keyGetter}, + }, + compose.OAuth2AuthorizeExplicitFactory, + compose.OAuth2AuthorizeImplicitFactory, + compose.OAuth2ClientCredentialsGrantFactory, + factoryRefresh, + compose.OAuth2ResourceOwnerPasswordCredentialsFactory, + compose.RFC7523AssertionGrantFactory, + + compose.OpenIDConnectExplicitFactory, + compose.OpenIDConnectImplicitFactory, + compose.OpenIDConnectHybridFactory, + compose.OpenIDConnectRefreshFactory, + + compose.OAuth2TokenIntrospectionFactory, + compose.OAuth2TokenRevocationFactory, + + compose.OAuth2PKCEFactory, + compose.PushedAuthorizeHandlerFactory, + ) + } else { + f = compose.ComposeAllEnabled(fc, fositeStore, gen.MustRSAKey()) } - if len(tc.scopes) != 0 { - opts = append(opts, oauth2.SetAuthURLParam("scope", strings.Join(tc.scopes, " ")), oauth2.SetAuthURLParam("client_id", client.ClientID)) + ts := mockServer(t, f, session) + defer ts.Close() + + client := newOAuth2Client(ts) + client.Scopes = []string{"openid", "offline", "offline_access", "foo", "bar"} + client.ClientID = "grant-all-requested-scopes-client" + + testRefreshingClient := &fosite.DefaultClient{ + ID: "grant-all-requested-scopes-client", + Secret: []byte(`$2a$10$IxMdI6d.LIRZPpSfEwNoeu4rY3FhDREsxFJXikcgdRRAStxUlsuEO`), // = "foobar" + RedirectURIs: []string{ts.URL + "/callback"}, + ResponseTypes: []string{"code"}, + GrantTypes: []string{"implicit", "refresh_token", "authorization_code", "password", "client_credentials"}, + Scopes: []string{"openid", "offline_access", "offline", "foo", "bar", "baz"}, + Audience: []string{"https://www.ory.sh/api"}, } - entries[idx].OAuth2, err = client.Exchange(ctx, "", opts...) - if len(tc.err) != 0 { - require.Error(t, err) - require.Nil(t, entries[idx].OAuth2) - require.Contains(t, err.Error(), tc.err) + fositeStore.Clients["grant-all-requested-scopes-client"] = testRefreshingClient - return - } + entries := make([]step, len(scenario.testCases)+1) + resp, err := http.Get(client.AuthCodeURL(state)) require.NoError(t, err) - require.NotEmpty(t, entries[idx].OAuth2.AccessToken) - require.NotEmpty(t, entries[idx].OAuth2.RefreshToken) + require.Equal(t, http.StatusOK, resp.StatusCode) - entries[idx].SessionAT, err = fositeStore.GetAccessTokenSession(ctx, s.AccessTokenSignature(ctx, entries[idx].OAuth2.AccessToken), nil) - require.NoError(t, err) + entries[0].OAuth2, err = client.Exchange(ctx, resp.Request.URL.Query().Get("code"), oauth2.SetAuthURLParam("client_id", client.ClientID)) - entries[idx].SessionRT, err = fositeStore.GetRefreshTokenSession(ctx, s.RefreshTokenSignature(ctx, entries[idx].OAuth2.RefreshToken), nil) require.NoError(t, err) + require.NotEmpty(t, entries[0].OAuth2.AccessToken) + require.NotEmpty(t, entries[0].OAuth2.RefreshToken) - if len(tc.scopes) != 0 { - assert.ElementsMatch(t, entries[idx].SessionAT.GetRequestedScopes(), tc.scopes) - assert.Equal(t, strings.Join(tc.expected, " "), entries[idx].OAuth2.Extra("scope")) - } else { - assert.ElementsMatch(t, entries[idx].SessionAT.GetRequestedScopes(), originalScopes) - assert.Equal(t, strings.Join(originalScopes, " "), entries[idx].OAuth2.Extra("scope")) - } - assert.ElementsMatch(t, entries[idx].SessionAT.GetGrantedScopes(), tc.expected) - assert.ElementsMatch(t, entries[idx].SessionRT.GetRequestedScopes(), originalScopes) - assert.ElementsMatch(t, entries[idx].SessionRT.GetGrantedScopes(), originalScopes) - - var ( - j int - entry step - ) + assert.Equal(t, strings.Join(originalScopes, " "), entries[0].OAuth2.Extra("scope")) - assert.Equal(t, entries[idx].SessionAT.GetID(), entries[idx].SessionRT.GetID()) - - for j, entry = range entries { - if j == idx { - break - } + entries[0].SessionAT, err = fositeStore.GetAccessTokenSession(ctx, s.AccessTokenSignature(ctx, entries[0].OAuth2.AccessToken), nil) + require.NoError(t, err) - assert.Equal(t, entries[idx].SessionAT.GetID(), entry.SessionAT.GetID()) - assert.Equal(t, entries[idx].SessionAT.GetID(), entry.SessionRT.GetID()) - assert.Equal(t, entries[idx].SessionRT.GetID(), entry.SessionAT.GetID()) - assert.Equal(t, entries[idx].SessionRT.GetID(), entry.SessionRT.GetID()) + entries[0].SessionRT, err = fositeStore.GetRefreshTokenSession(ctx, s.RefreshTokenSignature(ctx, entries[0].OAuth2.RefreshToken), nil) + require.NoError(t, err) - assert.Greater(t, entries[idx].SessionAT.GetSession().GetExpiresAt(fosite.AccessToken).Unix(), entry.SessionAT.GetSession().GetExpiresAt(fosite.AccessToken).Unix()) - assert.Greater(t, entries[idx].SessionRT.GetSession().GetExpiresAt(fosite.RefreshToken).Unix(), entry.SessionRT.GetSession().GetExpiresAt(fosite.RefreshToken).Unix()) - assert.Greater(t, entries[idx].SessionAT.GetRequestedAt().Unix(), entry.SessionAT.GetRequestedAt().Unix()) - assert.Greater(t, entries[idx].SessionRT.GetRequestedAt().Unix(), entry.SessionRT.GetRequestedAt().Unix()) + assert.ElementsMatch(t, entries[0].SessionAT.GetRequestedScopes(), originalScopes) + assert.ElementsMatch(t, entries[0].SessionRT.GetRequestedScopes(), originalScopes) + assert.ElementsMatch(t, entries[0].SessionAT.GetGrantedScopes(), originalScopes) + assert.ElementsMatch(t, entries[0].SessionRT.GetGrantedScopes(), originalScopes) + assert.Equal(t, strings.Join(originalScopes, " "), entries[0].OAuth2.Extra("scope")) + + for i, tc := range scenario.testCases { + t.Run(tc.name, func(t *testing.T) { + if scenario.checkTime { + time.Sleep(time.Second) + } + + idx := i + 1 + + opts := []oauth2.AuthCodeOption{ + oauth2.SetAuthURLParam("refresh_token", entries[i].OAuth2.RefreshToken), + oauth2.SetAuthURLParam("grant_type", "refresh_token"), + } + + if len(tc.scopes) != 0 { + opts = append(opts, oauth2.SetAuthURLParam("scope", strings.Join(tc.scopes, " ")), oauth2.SetAuthURLParam("client_id", client.ClientID)) + } + + entries[idx].OAuth2, err = client.Exchange(ctx, "", opts...) + if len(tc.err) != 0 { + require.Error(t, err) + require.Nil(t, entries[idx].OAuth2) + require.Contains(t, err.Error(), tc.err) + + return + } + + require.NoError(t, err) + require.NotEmpty(t, entries[idx].OAuth2.AccessToken) + require.NotEmpty(t, entries[idx].OAuth2.RefreshToken) + + entries[idx].SessionAT, err = fositeStore.GetAccessTokenSession(ctx, s.AccessTokenSignature(ctx, entries[idx].OAuth2.AccessToken), nil) + require.NoError(t, err) + + entries[idx].SessionRT, err = fositeStore.GetRefreshTokenSession(ctx, s.RefreshTokenSignature(ctx, entries[idx].OAuth2.RefreshToken), nil) + require.NoError(t, err) + + if len(tc.scopes) != 0 { + assert.ElementsMatch(t, entries[idx].SessionAT.GetRequestedScopes(), tc.scopes) + assert.Equal(t, strings.Join(tc.expected, " "), entries[idx].OAuth2.Extra("scope")) + } else { + assert.ElementsMatch(t, entries[idx].SessionAT.GetRequestedScopes(), originalScopes) + assert.Equal(t, strings.Join(originalScopes, " "), entries[idx].OAuth2.Extra("scope")) + } + assert.ElementsMatch(t, entries[idx].SessionAT.GetGrantedScopes(), tc.expected) + assert.ElementsMatch(t, entries[idx].SessionRT.GetRequestedScopes(), originalScopes) + assert.ElementsMatch(t, entries[idx].SessionRT.GetGrantedScopes(), originalScopes) + + var ( + j int + entry step + ) + + assert.Equal(t, entries[idx].SessionAT.GetID(), entries[idx].SessionRT.GetID()) + + for j, entry = range entries { + if j == idx { + break + } + + assert.Equal(t, entries[idx].SessionAT.GetID(), entry.SessionAT.GetID()) + assert.Equal(t, entries[idx].SessionAT.GetID(), entry.SessionRT.GetID()) + assert.Equal(t, entries[idx].SessionRT.GetID(), entry.SessionAT.GetID()) + assert.Equal(t, entries[idx].SessionRT.GetID(), entry.SessionRT.GetID()) + + if scenario.checkTime { + assert.Greater(t, entries[idx].SessionAT.GetSession().GetExpiresAt(fosite.AccessToken).Unix(), entry.SessionAT.GetSession().GetExpiresAt(fosite.AccessToken).Unix()) + assert.Greater(t, entries[idx].SessionRT.GetSession().GetExpiresAt(fosite.RefreshToken).Unix(), entry.SessionRT.GetSession().GetExpiresAt(fosite.RefreshToken).Unix()) + assert.Greater(t, entries[idx].SessionAT.GetRequestedAt().Unix(), entry.SessionAT.GetRequestedAt().Unix()) + assert.Greater(t, entries[idx].SessionRT.GetRequestedAt().Unix(), entry.SessionRT.GetRequestedAt().Unix()) + } + } + }) } }) } From 9a0bc79b8fb453b96b0a69184b1e2145d019adf3 Mon Sep 17 00:00:00 2001 From: James Elliott Date: Mon, 4 Sep 2023 18:48:21 +1000 Subject: [PATCH 7/7] fix: exact scope only during refresh original comparison --- handler/oauth2/flow_refresh.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/handler/oauth2/flow_refresh.go b/handler/oauth2/flow_refresh.go index dd9c9f831..1cf3684cf 100644 --- a/handler/oauth2/flow_refresh.go +++ b/handler/oauth2/flow_refresh.go @@ -106,7 +106,11 @@ func (c *RefreshTokenGrantHandler) HandleTokenEndpointRequest(ctx context.Contex originalScopes := originalRequest.GetGrantedScopes() for _, scope := range request.GetRequestedScopes() { - if !strategy(originalScopes, scope) { + // Utilizing the fosite.ScopeStrategy from the configuration here could be a mistake in some scenarios. + // The client could under certain circumstances be able to escape their originally granted scopes with carefully + // crafted requests and/or a custom scope strategy has not been implemented with this specific scenario in mind. + // This should always be an exact comparison for these reasons. + if !originalScopes.Has(scope) { if c.IgnoreRequestedScopeNotInOriginalGrant { // Skips addressing point 2 of the text in RFC6749 Section 6 and instead just prevents the scope // requested from being granted.