Core: Allow disabling token exchange as refresh #13809
Conversation
|
|
||
| return response; | ||
| } else { | ||
| return fetchToken( |
There was a problem hiding this comment.
do we need to check whether we have a credential here? I think it's possible that we started just with a token and so we'd end up with null credential. We might also want to add a test for this in TestRESTCatalog, since we already have some other tests where e.g. we either start with a credential or with a token only
There was a problem hiding this comment.
I'll add a check, but that's really a configuration issue since if you only have a token, then your only option is exchange flow. Disabling exchange and not having a client credential would also mean that you need to disable refresh (you won't be able to get a new token regardless).
nastra
changed the title
| @ValueSource(strings = {"v1/oauth/tokens", "https://auth-server.com/token"}) | ||
| public void testCatalogExpiredTokenCredentialRefreshWithExchangeDisabled(String oauth2ServerUri) { | ||
| // expires at epoch second = 1 | ||
| String token = ".eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyLCJleHAiOjF9."; |
There was a problem hiding this comment.
@nastra gitleaks doesn't like the JWTs that we have in the tests (we probably shouldn't have JWTs that look valid even if expired). Omitting the header and signature appears to render it harmless.
There was a problem hiding this comment.
yeah I noticed that too and and things still work as expected, the removing the header + signature should be fine, since we really only care about the expiration. Do you want to update the other tokens in this test class as well?
danielcweeks
force-pushed
the
exchange-enabled
branch
from
b5ad11d to
04c44c0
Compare
nastra
left a comment
nastra
left a comment
There was a problem hiding this comment.
LGTM, thanks @danielcweeks
|
Hi, would it be possible to include this in 1.10? |
|
@adutra since it's already on main it will automatically be included in 1.10 |
4 participants
We've had a number of issues reported that relate to the default refresh path for tokens created through the client credentials flow. The first is that we assume that token exchange is the default refresh path and the second is that some IDPs have different behaviors around token exchange. Some disallow refreshing and fail while others support exchange without refreshing the token expiration and others fully support this approach which provides a viable refresh path. There's a lot of flexibility within spec for the RFCs governing this.
Given that many IDPs do not support this exchange and the expected path to get a new token when you have client credentials is to just use the client credentials flow, we need a way to allow that behavior.
In the future, we want to better support refresh tokens and possibly remove the exchange behavior, but regardless the recommended path for new token creation with client credentials is to use the client credential flow like proposed in this PR.