Skip to content

Commit cda9c24

Browse files
james-d-elliottaeneasr
james-d-elliott
authored and
aeneasr
committed
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.
1 parent 2a93db5 commit cda9c24

File tree

2 files changed

+119
-9
lines changed

2 files changed

+119
-9
lines changed

‎handler/oauth2/flow_refresh.go‎

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,6 @@ import (
2828
"time"
2929

3030
"github.com/ory/x/errorsx"
31-
3231
"github.com/pkg/errors"
3332

3433
"github.com/ory/fosite"
@@ -96,13 +95,39 @@ func (c *RefreshTokenGrantHandler) HandleTokenEndpointRequest(ctx context.Contex
9695
}
9796

9897
request.SetSession(originalRequest.GetSession().Clone())
99-
request.SetRequestedScopes(originalRequest.GetRequestedScopes())
98+
99+
/*
100+
There are two key points in the following spec section this addresses:
101+
1. If omitted the scope param should be treated as the same as the scope originally granted by the resource owner.
102+
2. The REQUESTED scope MUST NOT include any scope not originally granted.
103+
104+
scope
105+
OPTIONAL. The scope of the access request as described by Section 3.3. The requested scope MUST NOT
106+
include any scope not originally granted by the resource owner, and if omitted is treated as equal to
107+
the scope originally granted by the resource owner.
108+
109+
See https://www.rfc-editor.org/rfc/rfc6749#section-6
110+
*/
111+
switch scope := request.GetRequestForm().Get("scope"); scope {
112+
case "":
113+
// Addresses point 1 of the text in RFC6749 Section 6.
114+
request.SetRequestedScopes(originalRequest.GetGrantedScopes())
115+
default:
116+
request.SetRequestedScopes(fosite.RemoveEmpty(strings.Split(scope, " ")))
117+
}
118+
100119
request.SetRequestedAudience(originalRequest.GetRequestedAudience())
101120

102-
for _, scope := range originalRequest.GetGrantedScopes() {
121+
for _, scope := range request.GetRequestedScopes() {
122+
// Addresses point 2 of the text in RFC6749 Section 6.
123+
if !originalRequest.GetGrantedScopes().Has(scope) {
124+
return errorsx.WithStack(fosite.ErrInvalidScope.WithHintf("The requested scope '%s' was not originally granted by the resource owner.", scope))
125+
}
126+
103127
if !c.Config.GetScopeStrategy(ctx)(request.GetClient().GetScopes(), scope) {
104128
return errorsx.WithStack(fosite.ErrInvalidScope.WithHintf("The OAuth 2.0 Client is not allowed to request scope '%s'.", scope))
105129
}
130+
106131
request.GrantScope(scope)
107132
}
108133

‎handler/oauth2/flow_refresh_test.go‎

Lines changed: 91 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -29,14 +29,12 @@ import (
2929
"time"
3030

3131
"github.com/golang/mock/gomock"
32-
33-
"github.com/ory/fosite/internal"
34-
3532
"github.com/pkg/errors"
3633
"github.com/stretchr/testify/assert"
3734
"github.com/stretchr/testify/require"
3835

3936
"github.com/ory/fosite"
37+
"github.com/ory/fosite/internal"
4038
"github.com/ory/fosite/storage"
4139
)
4240

@@ -191,12 +189,99 @@ func TestRefreshFlow_HandleTokenEndpointRequest(t *testing.T) {
191189
assert.NotEqual(t, sess, areq.Session)
192190
assert.NotEqual(t, time.Now().UTC().Add(-time.Hour).Round(time.Hour), areq.RequestedAt)
193191
assert.Equal(t, fosite.Arguments{"foo", "offline"}, areq.GrantedScope)
194-
assert.Equal(t, fosite.Arguments{"foo", "bar", "offline"}, areq.RequestedScope)
192+
assert.Equal(t, fosite.Arguments{"foo", "offline"}, areq.RequestedScope)
195193
assert.NotEqual(t, url.Values{"foo": []string{"bar"}}, areq.Form)
196194
assert.Equal(t, time.Now().Add(time.Hour).UTC().Round(time.Second), areq.GetSession().GetExpiresAt(fosite.AccessToken))
197195
assert.Equal(t, time.Now().Add(time.Hour).UTC().Round(time.Second), areq.GetSession().GetExpiresAt(fosite.RefreshToken))
198196
},
199197
},
198+
{
199+
description: "should pass with scope in form",
200+
setup: func(config *fosite.Config) {
201+
areq.GrantTypes = fosite.Arguments{"refresh_token"}
202+
areq.Client = &fosite.DefaultClient{
203+
ID: "foo",
204+
GrantTypes: fosite.Arguments{"refresh_token"},
205+
Scopes: []string{"foo", "bar", "baz", "offline"},
206+
}
207+
208+
token, sig, err := strategy.GenerateRefreshToken(nil, nil)
209+
require.NoError(t, err)
210+
211+
areq.Form.Add("refresh_token", token)
212+
areq.Form.Add("scope", "foo bar baz offline")
213+
err = store.CreateRefreshTokenSession(nil, sig, &fosite.Request{
214+
Client: areq.Client,
215+
GrantedScope: fosite.Arguments{"foo", "bar", "baz", "offline"},
216+
RequestedScope: fosite.Arguments{"foo", "bar", "baz", "offline"},
217+
Session: sess,
218+
Form: url.Values{"foo": []string{"bar"}},
219+
RequestedAt: time.Now().UTC().Add(-time.Hour).Round(time.Hour),
220+
})
221+
require.NoError(t, err)
222+
},
223+
expect: func(t *testing.T) {
224+
assert.Equal(t, fosite.Arguments{"foo", "bar", "baz", "offline"}, areq.GrantedScope)
225+
assert.Equal(t, fosite.Arguments{"foo", "bar", "baz", "offline"}, areq.RequestedScope)
226+
},
227+
},
228+
{
229+
description: "should pass with scope in form and should narrow scopes",
230+
setup: func(config *fosite.Config) {
231+
areq.GrantTypes = fosite.Arguments{"refresh_token"}
232+
areq.Client = &fosite.DefaultClient{
233+
ID: "foo",
234+
GrantTypes: fosite.Arguments{"refresh_token"},
235+
Scopes: []string{"foo", "bar", "baz", "offline"},
236+
}
237+
238+
token, sig, err := strategy.GenerateRefreshToken(nil, nil)
239+
require.NoError(t, err)
240+
241+
areq.Form.Add("refresh_token", token)
242+
areq.Form.Add("scope", "foo bar offline")
243+
err = store.CreateRefreshTokenSession(nil, sig, &fosite.Request{
244+
Client: areq.Client,
245+
GrantedScope: fosite.Arguments{"foo", "bar", "baz", "offline"},
246+
RequestedScope: fosite.Arguments{"foo", "bar", "baz", "offline"},
247+
Session: sess,
248+
Form: url.Values{"foo": []string{"bar"}},
249+
RequestedAt: time.Now().UTC().Add(-time.Hour).Round(time.Hour),
250+
})
251+
require.NoError(t, err)
252+
},
253+
expect: func(t *testing.T) {
254+
assert.Equal(t, fosite.Arguments{"foo", "bar", "offline"}, areq.GrantedScope)
255+
assert.Equal(t, fosite.Arguments{"foo", "bar", "offline"}, areq.RequestedScope)
256+
},
257+
},
258+
{
259+
description: "should fail with broadened scopes even if the client can request it",
260+
setup: func(config *fosite.Config) {
261+
areq.GrantTypes = fosite.Arguments{"refresh_token"}
262+
areq.Client = &fosite.DefaultClient{
263+
ID: "foo",
264+
GrantTypes: fosite.Arguments{"refresh_token"},
265+
Scopes: []string{"foo", "bar", "baz", "offline"},
266+
}
267+
268+
token, sig, err := strategy.GenerateRefreshToken(nil, nil)
269+
require.NoError(t, err)
270+
271+
areq.Form.Add("refresh_token", token)
272+
areq.Form.Add("scope", "foo bar offline")
273+
err = store.CreateRefreshTokenSession(nil, sig, &fosite.Request{
274+
Client: areq.Client,
275+
GrantedScope: fosite.Arguments{"foo", "baz", "offline"},
276+
RequestedScope: fosite.Arguments{"foo", "baz", "offline"},
277+
Session: sess,
278+
Form: url.Values{"foo": []string{"bar"}},
279+
RequestedAt: time.Now().UTC().Add(-time.Hour).Round(time.Hour),
280+
})
281+
require.NoError(t, err)
282+
},
283+
expectErr: fosite.ErrInvalidScope,
284+
},
200285
{
201286
description: "should pass with custom client lifespans",
202287
setup: func(config *fosite.Config) {
@@ -229,7 +314,7 @@ func TestRefreshFlow_HandleTokenEndpointRequest(t *testing.T) {
229314
assert.NotEqual(t, sess, areq.Session)
230315
assert.NotEqual(t, time.Now().UTC().Add(-time.Hour).Round(time.Hour), areq.RequestedAt)
231316
assert.Equal(t, fosite.Arguments{"foo", "offline"}, areq.GrantedScope)
232-
assert.Equal(t, fosite.Arguments{"foo", "bar", "offline"}, areq.RequestedScope)
317+
assert.Equal(t, fosite.Arguments{"foo", "offline"}, areq.RequestedScope)
233318
assert.NotEqual(t, url.Values{"foo": []string{"bar"}}, areq.Form)
234319
internal.RequireEqualTime(t, time.Now().Add(*internal.TestLifespans.RefreshTokenGrantAccessTokenLifespan).UTC(), areq.GetSession().GetExpiresAt(fosite.AccessToken), time.Minute)
235320
internal.RequireEqualTime(t, time.Now().Add(*internal.TestLifespans.RefreshTokenGrantRefreshTokenLifespan).UTC(), areq.GetSession().GetExpiresAt(fosite.RefreshToken), time.Minute)
@@ -290,7 +375,7 @@ func TestRefreshFlow_HandleTokenEndpointRequest(t *testing.T) {
290375
assert.NotEqual(t, sess, areq.Session)
291376
assert.NotEqual(t, time.Now().UTC().Add(-time.Hour).Round(time.Hour), areq.RequestedAt)
292377
assert.Equal(t, fosite.Arguments{"foo"}, areq.GrantedScope)
293-
assert.Equal(t, fosite.Arguments{"foo", "bar"}, areq.RequestedScope)
378+
assert.Equal(t, fosite.Arguments{"foo"}, areq.RequestedScope)
294379
assert.NotEqual(t, url.Values{"foo": []string{"bar"}}, areq.Form)
295380
assert.Equal(t, time.Now().Add(time.Hour).UTC().Round(time.Second), areq.GetSession().GetExpiresAt(fosite.AccessToken))
296381
assert.Equal(t, time.Now().Add(time.Hour).UTC().Round(time.Second), areq.GetSession().GetExpiresAt(fosite.RefreshToken))

0 commit comments

Comments
 (0)