fix(oauth2): fix a bug that refresh_token could be shared across instances #11342
Conversation
liverpool8056
force-pushed
the
fix/check_scopes_when_refresh_token_oauth2
branch
2 times, most recently
from
c649429
to
08c44c1
Compare
There was a problem hiding this comment.
It overall LGTM. I have one question:
If two OAuth2 plugin has common scopes but the token_expiration is different.
admin_api.oauth2_plugins:insert({
route = { id = route18.id },
config = {
scopes = { "common", "foo" },
global_credentials = true,
token_expiration = 10
}
})
admin_api.oauth2_plugins:insert({
route = { id = route19.id },
config = {
scopes = { "common", "bar" },
global_credentials = true,
token_expiration = 20
}
})
A shorter TTL access_token(with scope common)'s refresh_token can be considered valid to another refresh endpoint to get a longer TTL access_token as it passes the scope validation. Is this expected?
liverpool8056
force-pushed
the
fix/check_scopes_when_refresh_token_oauth2
branch
from
f0d6d9c
to
855e650
Compare
| error = "invalid_scope", | ||
| error_description = "scope mismatch" | ||
| }, res) | ||
| end) |
There was a problem hiding this comment.
Please add a test case that verifies the subsetting functionality.
liverpool8056
force-pushed
the
fix/check_scopes_when_refresh_token_oauth2
branch
from
7bc45ac
to
c7bea72
Compare
liverpool8056
force-pushed
the
fix/check_scopes_when_refresh_token_oauth2
branch
from
1cfcd6f
to
1dced32
Compare
There was a problem hiding this comment.
The test file does not conform to the while spacing rules that we follow across the code base. Please adjust the white space of the new tests to conform.
| method = "POST", | ||
| path = "/oauth2/token", | ||
| body = { | ||
| refresh_token = token.refresh_token, |
There was a problem hiding this comment.
It seems that this test reuses a token generated by a previous test. Please make each test self contained. The outcome of one test should not influence other tests, as that makes it hard to run and debug tests in isolation. Also, it is not clear from looking at the test how the token has been generated, making it difficult to understand what is being tested.
liverpool8056
force-pushed
the
fix/check_scopes_when_refresh_token_oauth2
branch
from
f79b23a
to
4e3d389
Compare
services when `global_credentials` is set as `true`. With `global_credential=true`, `access_token` can be shared across services, as well as `refresh_token` currently. This means that a `refresh_token` belonging to a service can be used to refresh tokens belonging to another service, which is consider as a bug. In this PR, the scope is taken into account as a new creteria of the request validation. Scopes associated with a token provided in the request will be compared with those configured in the Oauth2 instance hit by that request. FTI-5173
Co-authored-by: Hans Hübner <[email protected]>
Co-authored-by: Hans Hübner <[email protected]>
Co-authored-by: Hans Hübner <[email protected]>
liverpool8056
force-pushed
the
fix/check_scopes_when_refresh_token_oauth2
branch
from
4e3d389
to
34715ad
Compare
Summary
With
global_credential=true,access_tokencan be shared across services, as well asrefresh_tokencurrently. This means that arefresh_tokenbelonging to a service can be used to refresh tokens belonging to another service, which is consider as a bug. In this PR, the scope is taken into account as a new creteria of the request validation. Scopes associated with a token provided in the request will be compared with those configured in the Oauth2 instance hit by that request.Checklist
Full changelog
Issue reference
FTI-5173