-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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.
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
Allow custom OIDC client filters to force a new token acquisition #38480
Allow custom OIDC client filters to force a new token acquisition #38480
Conversation
0be84de
to
c6a894c
Compare
@lordvlad FYI, if this flag is set then if the refresh token is available then it should be used, otherwise some providers may return the current token if it is still valid, so I dropped your second boolean check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 48 should read
prepareUni((!forceNewTokens && tokens.getRefreshToken() != null && !tokens.isRefreshTokenExpired())
Otherwise it will get new tokens using the refresh token, while the intent was to also discard refresh tokens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry @sberyozkin just now saw your reasoning for dropping the second check.
Unfortunately, in our case that would then require another workaround because getting the token via refresh token will fail.
Background: the resource server and identity provider pin the access an refresh token to a fixed IP. Because we have a few layers between our app and the idp and resource servers, the applications IP address is not stable.
We need a way to force a fresh set of tokens from scratch (not from refresh tokens) in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lordvlad Sure, lets keep this flag for the original purpose, and we can make this process more customizable going forward if necessary
Thanks @gastaldi, let me restore the extra boolean check originally added by @lordvlad, which I decided to remove at the last moment with the 2nd push. My idea was I thought generally correct, as refreshing the tokens can typically recycle both the access and the current refresh token but it will fail in the @lordvlad's case. So I'll restore that extra check to truly mean it is about a new access token grant request - and we'll tune it, if necessary, with the refresh grant token grant, if someone will have a concrete requirement. Sorry about the noise. |
…kens, discarding all access or refresh tokens
c6a894c
to
fc68cde
Compare
✔️ The latest workflow run for the pull request has completed successfully. It should be safe to merge provided you have a look at the other checks in the summary. You can consult the Develocity build scans. |
This simple PR is based on the @lordvlad's input and initial implementation. It adds an option for custom filters to force a new token acquisition, even if the current access token is still valid, when the access token propagation fails even if the access token has not yet expired.
I have only added a new method overload to avoid (a rather unlikely) breaking of some client code using the TokenHelpers directly and a log message to inform that the current tokens will be discarded if the custom filter has requested it.
I've looked at the
integration-tests/oidc-client-reactive
test which checks the file logs to confirm that the tokens have been refreshed and I thought it would be quite tricky to try to update it with another log check to verify that the second token refresh happened because the custom filter may have requested it, with having to order tests, and deal with some likely flakiness, not sure it is necessary given the simplicity of this update.@lordvlad, can you check this PR please ?
@gastaldi can you please review, I'll try to figure out how to test it if you prefer, would not be a problem, @lordvlad has a workaround, so the PR is not urgent to merge