Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: requested scope ignored in refresh flow #718

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

fix: requested scope ignored in refresh flow #718

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
37a00cf
fix(oauth2): requested scope ignored in refresh flow
james-d-elliott Nov 2, 2022
5d4da29
fix: retain refresh_token original scopes
james-d-elliott Nov 15, 2022
c5c6056
test: add integration test
james-d-elliott Nov 21, 2022
deb3be3
refactor: remove unnecessary receiver
james-d-elliott Dec 12, 2022
6db02e8
refactor: simplify logic
james-d-elliott Jul 19, 2023
c472cd8
feat: filtering mode
james-d-elliott Jul 20, 2023
9a0bc79
fix: exact scope only during refresh original comparison
james-d-elliott Sep 4, 2023
45ede8b
Merge branch 'master' into fix-refresh-scope-narrowing
james-d-elliott Feb 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions access_request.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,22 @@ func NewAccessRequest(session Session) *AccessRequest {
func (a *AccessRequest) GetGrantTypes() Arguments {
return a.GrantTypes
}

func (a *AccessRequest) SetGrantedScopes(scopes Arguments) {
a.GrantedScope = scopes
}

func (a *AccessRequest) SanitizeRestoreRefreshTokenOriginalRequester(requester Requester) Requester {
r := a.Sanitize(nil).(*Request)

ar := &AccessRequest{
Request: *r,
}

ar.SetID(requester.GetID())

ar.SetRequestedScopes(requester.GetRequestedScopes())
ar.SetGrantedScopes(requester.GetGrantedScopes())

return ar
}
72 changes: 61 additions & 11 deletions handler/oauth2/flow_refresh.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
"time"

"github.com/ory/x/errorsx"

"github.com/pkg/errors"

"github.com/ory/fosite"
Expand All @@ -30,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
Comment on lines +33 to +36
Copy link
Contributor Author

@james-d-elliott james-d-elliott Jul 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note when looking at this. I have a per-client implementation of this utilizing an interface which passes the same tests. Maybe this is more desirable?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this not made through a new config interface?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me it looks like currently all config is done through config interface, even if it is not meant for hot reloading?

}

// HandleTokenEndpointRequest implements https://tools.ietf.org/html/rfc6749#section-6
Expand Down Expand Up @@ -69,7 +73,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
Expand All @@ -79,13 +82,50 @@ func (c *RefreshTokenGrantHandler) HandleTokenEndpointRequest(ctx context.Contex

request.SetID(originalRequest.GetID())
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
*/

// Addresses point 1 of the text in RFC6749 Section 6.
if len(request.GetRequestedScopes()) == 0 {
request.SetRequestedScopes(originalRequest.GetGrantedScopes())
}

request.SetRequestedAudience(originalRequest.GetRequestedAudience())

for _, scope := range originalRequest.GetGrantedScopes() {
if !c.Config.GetScopeStrategy(ctx)(request.GetClient().GetScopes(), scope) {
strategy := c.Config.GetScopeStrategy(ctx)
originalScopes := originalRequest.GetGrantedScopes()

for _, scope := range request.GetRequestedScopes() {
// 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.
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))
}
Comment on lines +115 to +122
Copy link
Contributor Author

@james-d-elliott james-d-elliott Jul 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note when looking at this. I have a per-client implementation of this utilizing an interface which passes the same tests. Maybe this is more desirable?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be the use case of having this per-client?

Copy link
Contributor Author

@james-d-elliott james-d-elliott Feb 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only way I'll personally use it is per-client where the default is to enforce the spec and you make an exception on a per-client basis. I'm happy enough to maintain my own entire implementation however.

}

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))
}

request.GrantScope(scope)
}

Expand Down Expand Up @@ -134,26 +174,36 @@ 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 err = c.TokenRevocationStorage.CreateRefreshTokenSession(ctx, refreshSignature, storeReq); err != nil {
return err
if rtRequest, ok := requester.(fosite.RefreshTokenAccessRequester); ok {
rtStoreReq := rtRequest.SanitizeRestoreRefreshTokenOriginalRequester(originalRequest)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this change about? Why it is necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the scopes of the current request are for the access token, not the refresh token. The scopes of the original granted refresh token must be restored to make it possible to meet the strict requirements of the spec:

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I mean, my question is why you have two cases here. It looks like you added RefreshTokenAccessRequester for backwards compatibility? But it feels to me that if you do not implement RefreshTokenAccessRequester then you get wrong behavior? Not sure if we should keep the old behavior at all?

Also, I am not sure that an extra method is already needed here? Sanitize returns a clone of requester to begin with? And you then call just:

ar.SetID(requester.GetID())
ar.SetRequestedScopes(requester.GetRequestedScopes())
ar.SetGrantedScopes(requester.GetGrantedScopes())

So why not just call originalRequest.Sanitize(nil) and then those three calls above, here, at this point? Without a method?

Probably I am missing something, I just checked very quickly this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An AccessRequester doesn't have SetGranted* receivers as part of the implementation. So a new receiver will be needed regardless. The question is just about what solution is going to lead to the least harmful outcome.

Adding those receivers to the AccessRequester interface seems like a poor choice as it isn't used now but could potentially lead to implementers making harmful decisions in this stage of the relevant flows in my opinion.

As far as the other I brought it up with aeneasr I believe so really up to an ory preference (he may have actually asked for it but I don't recall specifics). I personally don't have one myself in either direction.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has GrantScope? Isn't this enough? You loop over all granted scopes and call GrantScope?

Copy link
Contributor

@mitar mitar Feb 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the same being done currently in the code:

	request.SetID(originalRequest.GetID())
	request.SetSession(originalRequest.GetSession().Clone())
	request.SetRequestedScopes(originalRequest.GetRequestedScopes())
	request.SetRequestedAudience(originalRequest.GetRequestedAudience())

	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)
	}

	...

	for _, audience := range originalRequest.GetGrantedAudience() {
		request.GrantAudience(audience)
	}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this was a previous comment here: #718 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So to seems there is already some code to clone a requester. Maybe that should be just moved out into utility function and reused here for refresh token as well?

(And again sorry if I am missing something.)


rtStoreReq.SetSession(requester.GetSession().Clone())

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

Expand Down Expand Up @@ -177,13 +175,104 @@ 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))
assert.EqualValues(t, areq.Form.Get("or_request_id"), areq.GetID(), "Requester ID should be replaced based on the refresh token session")
},
},
{
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")
areq.SetRequestedScopes(fosite.Arguments{"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")
areq.SetRequestedScopes(fosite.Arguments{"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 @@ -216,7 +305,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 @@ -277,7 +366,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
24 changes: 15 additions & 9 deletions integration/helper_endpoints_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,16 +73,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() {
Expand Down Expand Up @@ -130,7 +136,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
Expand Down
Loading
Loading