Skip to content

Commit

Permalink
fix(oauth2): requested scope ignored in refresh flow
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@james-d-elliott @aeneasr
james-d-elliott authored and aeneasr committed Nov 3, 2022
1 parent 2a93db5 commit cda9c24
Show file tree
Hide file tree
Showing 2 changed files with 119 additions and 9 deletions.
31 changes: 28 additions & 3 deletions handler/oauth2/flow_refresh.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import (
"time"

"github.com/ory/x/errorsx"

"github.com/pkg/errors"

"github.com/ory/fosite"
Expand Down Expand Up @@ -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)
}

Expand Down
97 changes: 91 additions & 6 deletions handler/oauth2/flow_refresh_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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))
Expand Down

0 comments on commit cda9c24

Please sign in to comment.