Skip to content

Commit 2b8b61d

Browse files
vivshankarvivshankar
authored
Merge pull request #5 from vivshankar/refresh-token-narrow-scopes
fix: requested scope ignored in refresh flow
2 parents 85bbaeb + 019bd2e commit 2b8b61d

File tree

6 files changed

+487
-15
lines changed

6 files changed

+487
-15
lines changed

‎access_request.go‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,3 +23,22 @@ func NewAccessRequest(session Session) *AccessRequest {
2323
func (a *AccessRequest) GetGrantTypes() Arguments {
2424
return a.GrantTypes
2525
}
26+
27+
func (a *AccessRequest) SetGrantedScopes(scopes Arguments) {
28+
a.GrantedScope = scopes
29+
}
30+
31+
func (a *AccessRequest) SanitizeRestoreRefreshTokenOriginalRequester(requester Requester) Requester {
32+
r := a.Sanitize(nil).(*Request)
33+
34+
ar := &AccessRequest{
35+
Request: *r,
36+
}
37+
38+
ar.SetID(requester.GetID())
39+
40+
ar.SetRequestedScopes(requester.GetRequestedScopes())
41+
ar.SetGrantedScopes(requester.GetGrantedScopes())
42+
43+
return ar
44+
}

‎handler/oauth2/flow_refresh.go‎

Lines changed: 54 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,11 @@ type RefreshTokenGrantHandler struct {
3030
fosite.AudienceStrategyProvider
3131
fosite.RefreshTokenScopesProvider
3232
}
33+
34+
// IgnoreRequestedScopeNotInOriginalGrant determines the action to take when the requested scopes in the refresh
35+
// flow were not originally granted. If false which is the default the handler will automatically return an error.
36+
// If true the handler will filter out / ignore the scopes which were not originally granted.
37+
IgnoreRequestedScopeNotInOriginalGrant bool
3338
}
3439

3540
// HandleTokenEndpointRequest implements https://tools.ietf.org/html/rfc6749#section-6
@@ -79,13 +84,43 @@ func (c *RefreshTokenGrantHandler) HandleTokenEndpointRequest(ctx context.Contex
7984

8085
request.SetID(originalRequest.GetID())
8186
request.SetSession(originalRequest.GetSession().Clone())
82-
request.SetRequestedScopes(originalRequest.GetRequestedScopes())
87+
/*
88+
There are two key points in the following spec section this addresses:
89+
1. If omitted the scope param should be treated as the same as the scope originally granted by the resource owner.
90+
2. The REQUESTED scope MUST NOT include any scope not originally granted.
91+
scope
92+
OPTIONAL. The scope of the access request as described by Section 3.3. The requested scope MUST NOT
93+
include any scope not originally granted by the resource owner, and if omitted is treated as equal to
94+
the scope originally granted by the resource owner.
95+
See https://www.rfc-editor.org/rfc/rfc6749#section-6
96+
*/
97+
98+
// Addresses point 1 of the text in RFC6749 Section 6.
99+
if len(request.GetRequestedScopes()) == 0 {
100+
request.SetRequestedScopes(originalRequest.GetGrantedScopes())
101+
}
102+
83103
request.SetRequestedAudience(originalRequest.GetRequestedAudience())
84104

85-
for _, scope := range originalRequest.GetGrantedScopes() {
86-
if !c.Config.GetScopeStrategy(ctx)(request.GetClient().GetScopes(), scope) {
105+
strategy := c.Config.GetScopeStrategy(ctx)
106+
originalScopes := originalRequest.GetGrantedScopes()
107+
108+
for _, scope := range request.GetRequestedScopes() {
109+
if !strategy(originalScopes, scope) {
110+
if c.IgnoreRequestedScopeNotInOriginalGrant {
111+
// Skips addressing point 2 of the text in RFC6749 Section 6 and instead just prevents the scope
112+
// requested from being granted.
113+
continue
114+
} else {
115+
// Addresses point 2 of the text in RFC6749 Section 6.
116+
return errorsx.WithStack(fosite.ErrInvalidScope.WithHintf("The requested scope '%s' was not originally granted by the resource owner.", scope))
117+
}
118+
}
119+
120+
if !strategy(request.GetClient().GetScopes(), scope) {
87121
return errorsx.WithStack(fosite.ErrInvalidScope.WithHintf("The OAuth 2.0 Client is not allowed to request scope '%s'.", scope))
88122
}
123+
89124
request.GrantScope(scope)
90125
}
91126

@@ -134,26 +169,36 @@ func (c *RefreshTokenGrantHandler) PopulateTokenEndpointResponse(ctx context.Con
134169
err = c.handleRefreshTokenEndpointStorageError(ctx, err)
135170
}()
136171

137-
ts, err := c.TokenRevocationStorage.GetRefreshTokenSession(ctx, signature, nil)
172+
originalRequest, err := c.TokenRevocationStorage.GetRefreshTokenSession(ctx, signature, nil)
138173
if err != nil {
139174
return err
140-
} else if err := c.TokenRevocationStorage.RevokeAccessToken(ctx, ts.GetID()); err != nil {
175+
} else if err := c.TokenRevocationStorage.RevokeAccessToken(ctx, originalRequest.GetID()); err != nil {
141176
return err
142177
}
143178

144-
if err := c.TokenRevocationStorage.RevokeRefreshTokenMaybeGracePeriod(ctx, ts.GetID(), signature); err != nil {
179+
if err := c.TokenRevocationStorage.RevokeRefreshTokenMaybeGracePeriod(ctx, originalRequest.GetID(), signature); err != nil {
145180
return err
146181
}
147182

148183
storeReq := requester.Sanitize([]string{})
149-
storeReq.SetID(ts.GetID())
184+
storeReq.SetID(originalRequest.GetID())
150185

151186
if err = c.TokenRevocationStorage.CreateAccessTokenSession(ctx, accessSignature, storeReq); err != nil {
152187
return err
153188
}
154189

155-
if err = c.TokenRevocationStorage.CreateRefreshTokenSession(ctx, refreshSignature, storeReq); err != nil {
156-
return err
190+
if rtRequest, ok := requester.(fosite.RefreshTokenAccessRequester); ok {
191+
rtStoreReq := rtRequest.SanitizeRestoreRefreshTokenOriginalRequester(originalRequest)
192+
193+
rtStoreReq.SetSession(requester.GetSession().Clone())
194+
195+
if err = c.TokenRevocationStorage.CreateRefreshTokenSession(ctx, refreshSignature, rtStoreReq); err != nil {
196+
return err
197+
}
198+
} else {
199+
if err = c.TokenRevocationStorage.CreateRefreshTokenSession(ctx, refreshSignature, storeReq); err != nil {
200+
return err
201+
}
157202
}
158203

159204
responder.SetAccessToken(accessToken)

‎handler/oauth2/flow_refresh_test.go‎

Lines changed: 94 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -173,12 +173,103 @@ func TestRefreshFlow_HandleTokenEndpointRequest(t *testing.T) {
173173
assert.NotEqual(t, sess, areq.Session)
174174
assert.NotEqual(t, time.Now().UTC().Add(-time.Hour).Round(time.Hour), areq.RequestedAt)
175175
assert.Equal(t, fosite.Arguments{"foo", "offline"}, areq.GrantedScope)
176-
assert.Equal(t, fosite.Arguments{"foo", "bar", "offline"}, areq.RequestedScope)
176+
assert.Equal(t, fosite.Arguments{"foo", "offline"}, areq.RequestedScope)
177177
assert.NotEqual(t, url.Values{"foo": []string{"bar"}}, areq.Form)
178178
assert.Equal(t, time.Now().Add(time.Hour).UTC().Round(time.Second), areq.GetSession().GetExpiresAt(fosite.AccessToken))
179179
assert.Equal(t, time.Now().Add(time.Hour).UTC().Round(time.Second), areq.GetSession().GetExpiresAt(fosite.RefreshToken))
180180
},
181181
},
182+
{
183+
description: "should pass with scope in form",
184+
setup: func(config *fosite.Config) {
185+
areq.GrantTypes = fosite.Arguments{"refresh_token"}
186+
areq.Client = &fosite.DefaultClient{
187+
ID: "foo",
188+
GrantTypes: fosite.Arguments{"refresh_token"},
189+
Scopes: []string{"foo", "bar", "baz", "offline"},
190+
}
191+
192+
token, sig, err := strategy.GenerateRefreshToken(nil, nil)
193+
require.NoError(t, err)
194+
195+
areq.Form.Add("refresh_token", token)
196+
areq.Form.Add("scope", "foo bar baz offline")
197+
err = store.CreateRefreshTokenSession(nil, sig, &fosite.Request{
198+
Client: areq.Client,
199+
GrantedScope: fosite.Arguments{"foo", "bar", "baz", "offline"},
200+
RequestedScope: fosite.Arguments{"foo", "bar", "baz", "offline"},
201+
Session: sess,
202+
Form: url.Values{"foo": []string{"bar"}},
203+
RequestedAt: time.Now().UTC().Add(-time.Hour).Round(time.Hour),
204+
})
205+
require.NoError(t, err)
206+
},
207+
expect: func(t *testing.T) {
208+
assert.Equal(t, fosite.Arguments{"foo", "bar", "baz", "offline"}, areq.GrantedScope)
209+
assert.Equal(t, fosite.Arguments{"foo", "bar", "baz", "offline"}, areq.RequestedScope)
210+
},
211+
},
212+
{
213+
description: "should pass with scope in form and should narrow scopes",
214+
setup: func(config *fosite.Config) {
215+
areq.GrantTypes = fosite.Arguments{"refresh_token"}
216+
areq.Client = &fosite.DefaultClient{
217+
ID: "foo",
218+
GrantTypes: fosite.Arguments{"refresh_token"},
219+
Scopes: []string{"foo", "bar", "baz", "offline"},
220+
}
221+
222+
token, sig, err := strategy.GenerateRefreshToken(nil, nil)
223+
require.NoError(t, err)
224+
225+
areq.Form.Add("refresh_token", token)
226+
areq.Form.Add("scope", "foo bar offline")
227+
areq.SetRequestedScopes(fosite.Arguments{"foo", "bar", "offline"})
228+
229+
err = store.CreateRefreshTokenSession(nil, sig, &fosite.Request{
230+
Client: areq.Client,
231+
GrantedScope: fosite.Arguments{"foo", "bar", "baz", "offline"},
232+
RequestedScope: fosite.Arguments{"foo", "bar", "baz", "offline"},
233+
Session: sess,
234+
Form: url.Values{"foo": []string{"bar"}},
235+
RequestedAt: time.Now().UTC().Add(-time.Hour).Round(time.Hour),
236+
})
237+
require.NoError(t, err)
238+
},
239+
expect: func(t *testing.T) {
240+
assert.Equal(t, fosite.Arguments{"foo", "bar", "offline"}, areq.GrantedScope)
241+
assert.Equal(t, fosite.Arguments{"foo", "bar", "offline"}, areq.RequestedScope)
242+
},
243+
},
244+
{
245+
description: "should fail with broadened scopes even if the client can request it",
246+
setup: func(config *fosite.Config) {
247+
areq.GrantTypes = fosite.Arguments{"refresh_token"}
248+
areq.Client = &fosite.DefaultClient{
249+
ID: "foo",
250+
GrantTypes: fosite.Arguments{"refresh_token"},
251+
Scopes: []string{"foo", "bar", "baz", "offline"},
252+
}
253+
254+
token, sig, err := strategy.GenerateRefreshToken(nil, nil)
255+
require.NoError(t, err)
256+
257+
areq.Form.Add("refresh_token", token)
258+
areq.Form.Add("scope", "foo bar offline")
259+
areq.SetRequestedScopes(fosite.Arguments{"foo", "bar", "offline"})
260+
261+
err = store.CreateRefreshTokenSession(nil, sig, &fosite.Request{
262+
Client: areq.Client,
263+
GrantedScope: fosite.Arguments{"foo", "baz", "offline"},
264+
RequestedScope: fosite.Arguments{"foo", "baz", "offline"},
265+
Session: sess,
266+
Form: url.Values{"foo": []string{"bar"}},
267+
RequestedAt: time.Now().UTC().Add(-time.Hour).Round(time.Hour),
268+
})
269+
require.NoError(t, err)
270+
},
271+
expectErr: fosite.ErrInvalidScope,
272+
},
182273
{
183274
description: "should pass with custom client lifespans",
184275
setup: func(config *fosite.Config) {
@@ -211,7 +302,7 @@ func TestRefreshFlow_HandleTokenEndpointRequest(t *testing.T) {
211302
assert.NotEqual(t, sess, areq.Session)
212303
assert.NotEqual(t, time.Now().UTC().Add(-time.Hour).Round(time.Hour), areq.RequestedAt)
213304
assert.Equal(t, fosite.Arguments{"foo", "offline"}, areq.GrantedScope)
214-
assert.Equal(t, fosite.Arguments{"foo", "bar", "offline"}, areq.RequestedScope)
305+
assert.Equal(t, fosite.Arguments{"foo", "offline"}, areq.RequestedScope)
215306
assert.NotEqual(t, url.Values{"foo": []string{"bar"}}, areq.Form)
216307
internal.RequireEqualTime(t, time.Now().Add(*internal.TestLifespans.RefreshTokenGrantAccessTokenLifespan).UTC(), areq.GetSession().GetExpiresAt(fosite.AccessToken), time.Minute)
217308
internal.RequireEqualTime(t, time.Now().Add(*internal.TestLifespans.RefreshTokenGrantRefreshTokenLifespan).UTC(), areq.GetSession().GetExpiresAt(fosite.RefreshToken), time.Minute)
@@ -272,7 +363,7 @@ func TestRefreshFlow_HandleTokenEndpointRequest(t *testing.T) {
272363
assert.NotEqual(t, sess, areq.Session)
273364
assert.NotEqual(t, time.Now().UTC().Add(-time.Hour).Round(time.Hour), areq.RequestedAt)
274365
assert.Equal(t, fosite.Arguments{"foo"}, areq.GrantedScope)
275-
assert.Equal(t, fosite.Arguments{"foo", "bar"}, areq.RequestedScope)
366+
assert.Equal(t, fosite.Arguments{"foo"}, areq.RequestedScope)
276367
assert.NotEqual(t, url.Values{"foo": []string{"bar"}}, areq.Form)
277368
assert.Equal(t, time.Now().Add(time.Hour).UTC().Round(time.Second), areq.GetSession().GetExpiresAt(fosite.AccessToken))
278369
assert.Equal(t, time.Now().Add(time.Hour).UTC().Round(time.Second), areq.GetSession().GetExpiresAt(fosite.RefreshToken))

‎integration/helper_endpoints_test.go‎

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -77,8 +77,22 @@ func authEndpointHandler(t *testing.T, oauth2 fosite.OAuth2Provider, session fos
7777
return
7878
}
7979

80-
if ar.GetRequestedScopes().Has("fosite") {
81-
ar.GrantScope("fosite")
80+
if ar.GetClient().GetID() == "grant-all-requested-scopes-client" {
81+
for _, scope := range ar.GetRequestedScopes() {
82+
ar.GrantScope(scope)
83+
}
84+
} else {
85+
if ar.GetRequestedScopes().Has("fosite") {
86+
ar.GrantScope("fosite")
87+
}
88+
89+
if ar.GetRequestedScopes().Has("offline") {
90+
ar.GrantScope("offline")
91+
}
92+
93+
if ar.GetRequestedScopes().Has("openid") {
94+
ar.GrantScope("openid")
95+
}
8296
}
8397

8498
if ar.GetRequestedScopes().Has("offline") {
@@ -146,7 +160,7 @@ func tokenEndpointHandler(t *testing.T, provider fosite.OAuth2Provider) func(rw
146160

147161
response, err := provider.NewAccessResponse(ctx, accessRequest)
148162
if err != nil {
149-
t.Logf("Access request failed because: %+v", err)
163+
t.Logf("Access request failed because: %+v", fosite.ErrorToRFC6749Error(err).WithExposeDebug(true).GetDescription())
150164
t.Logf("Request: %+v", accessRequest)
151165
provider.WriteAccessError(req.Context(), rw, accessRequest, err)
152166
return

0 commit comments

Comments
 (0)