-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Authentication
in the security context is not updated during the refresh token flow
#15509
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
Comments
OidcClientInitiated(Server)LogoutSuccessHandler
might build a redirection URI to the authorization server end_session_endpoint
with an expired ID tokenOidcClientInitiated(Server)LogoutSuccessHandler
might use an expired ID token
@sjohnr I think I progressed in my understanding of the root cause for this issue and re-wrote the description accordingly. |
OidcClientInitiated(Server)LogoutSuccessHandler
might use an expired ID tokenOidcClientInitiated(Server)LogoutSuccessHandler
might use an expired or outdated ID token
OidcClientInitiated(Server)LogoutSuccessHandler
might use an expired or outdated ID tokenAuthentication
in the security context is not updated during the refresh token flow
Just wanted to confirm, Spring Authorization Server does refresh the |
I encountered the same issue when using the Spring OAuth2 client with the Spring Authorization Server. During the refresh token process, if the As a result, when the client side later initiates an RP-Initiated Logout, the value of the |
This issue does not break solely the RP-Initiated Logout. It might also break the access control in all applications configured with So, this issue seems rather critical. @sjohnr is it planned to solve it in a specific sprint? |
Thanks for being patient, and sorry I was unable to look at this closely sooner. I'm doing some thinking about this issue this week. I think the challenges with this issue are as follows:
With all of that as context, I think that assuming we can somehow solve (1) above, a solution for this issue could be to publish an event from the
Does anyone have additional thoughts on this? |
One additional thought I have is that we could publish the event with information directly from the |
A re-authentication triggered by the A new access-token can mean new authorities. Whatever the source for these authorities is ( |
Thanks @ch4mpy.
By "regular" OAuth2 do you mean the original OAuth2 Login with Twitter or Facebook social login use case? |
I mean OAuth2 without OpenID, when there is never an ID token and the OAuth2 client with Reading the OpenID spec again, the "every time the refresh token flow is used" might not be strictly required. Something like "if an ID token is part of the response or the |
Ok. I think we can keep it in mind, and see if it's possible to handle for OAuth2 Login without OIDC. However, I think focusing on updating the |
Thanks for the additional details here @sjohnr as I was the one that proposes #16253 I have also been following this issue. Let's assume we tackle that issue separately. For the other points, perhaps an option would be to tackle this using the Currently the |
Thanks @filiphr, appreciate your perspective on this.
It is certainly an option to solve #16253 first with changes to the domain model, and then use a success handler configured with the
Also, I don't consider adding a This would be extremely easy to opt into and in fact could potentially be handled by the framework in many cases without requiring manual configuration by the user, since we could detect the |
Hey folks, sorry for the delay, it has taken some time to make progress on this issue (which I suspected would be the case). I have updated the PR #16589 based on work by @yhao3 with changes to fully capture this idea. There are two pairs of events and listeners: The first listener ( The second listener ( The reason there are two events is that it is nice to keep separate the logic of creating an updated @yhao3 @ch4mpy @filiphr @mlitcher If you have a chance, please review #16589 with my updates and provide feedback. Please feel free to also test this out on the branch, or if you prefer you can just provide feedback and wait until it gets merged and test in a milestone release. |
Publishing a However, from what I understand from the PR, it will work only when the That said, I only use as authorization servers some OpenID Providers fully complying with OIDC, and I always request the |
Hi @ch4mpy, thanks for the feedback.
This is true currently. While this issue is general and includes both OAuth2 and OpenID Connect forms of login, the solution is currently aimed at just OIDC, which has a clear use case and associated problem (the The issue I see there is that we only have one piece of information telling us that an "OAuth2 Login is being refreshed", and that's the For that reason, I feel the first stab at this should focus on OIDC. It may be possible and worthwhile to follow up with an enhancement to focus on the non-OIDC case but I'm not fully convinced yet of the need. The ground work would be laid for this to be added in the framework later or just as a custom event listener. That's also the reason that I've kept the 2nd event listener hidden, so we could expand it to possibly respond to refreshing other user types, such as |
As I understand In this context, the ID token is no more than one of the possible sources for knowing who the user is. The As granted authorities usually drive what a user can access, it should probably be derived from the access token (which is not the case with the default authorities mapper, but that's another story). So, the So, if the application with
@sjohnr yes, I think it should remain open. Even if it was my motivation in the 1st place, what I report is not only about the need to update the ID token, it is the need to update all of the |
@ch4mpy do you know if this was fixed? Im having the same issue when using SCG RP initiated logout even after upgrading to 6.5.0 |
@toob2, I know 2 scenarios for which it is not fixed:
Sample scenario for parallel requests:
As stated by @sjohnr, Spring Security is not designed to handle parallel requests from the same frontend. In this case, each request may be running a refresh token flow of its own, resulting in potentially as many new ID tokens as parallel requests, with at least two consequences:
So, when having frontends sending parallel requests (which is a very frequent use-case for me), we should override the This failure is difficult to spot because it occurs only when there are concurrent requests from the same frontend when the tokens should be refreshed, and when the ID token saved in the session isn't the last emitted, which makes this bug flickery and hard to reproduce. |
@ch4mpy thanks, I'm using the reactive version, will wait for the fix in #17188 |
Well, if using Java 21 or above (with virtual threads), you should probably switch to the servlet version. Anyhow, unless the Spring Security team changes its mind about parallel requests within the same session, you might face the issue I describe above, even after gh-17188 is fixed. |
For the case you described for parallel requests, do you have any PoC in MVC? I have this exact issue |
@joaquinjsb I don't have a public reproducer, which, as I wrote above, is not that easy to make. We'd need:
This is quite some work and is mostly useless, considering that the Spring Security team stated that it is an expected behavior (when using Spring Security, we should serialize requests). So, for now, the point is more about gathering enough community feedback that parallel requests should be a supported use case, rather than putting reproducers together or even submitting PRs. |
got it, I guess I'll try to implement a way to synchronize the RefreshToken(Reactive)OAuth2AuthorizedClientProvider, any hints on it? Thanks |
Uh oh!
There was an error while loading. Please reload this page.
Describe the bug
Apparently, neither the ID token nor the
userinfo
are updated during the refresh token flow in Spring clients withoauth2Login
. This has at least two consequences:OidcUser
orOAuth2User
) might contain outdated dataOidcClientInitiated(Server)LogoutSuccessHandler
might build a redirection URI to the authorization serverend_session_endpoint
with an expired or outdated ID token, in which case the second part of the logout fails and the user session on the OpenID Provider might not be ended.Additional context about ID tokens
ID tokens are JWTs and, as with all JWTs, they expire.
ID tokens can be refreshed.
During RP-Initiated Logout, OpenID Providers SHOULD (not MUST) accept ID tokens even when the exp time has passed. This means that some OPs might not accept expired tokens and that clients would better do their best to send valid tokens.
Also, aside from expiration considerations, most OPs refreshing ID tokens accept only the last token they issued for a client. So when a new ID token is returned as part of a refresh token flow, the client should use this last issued ID token to build the RP-Initiated Logout location URI.
Last, I know no constraint in the specs about the different tokens relative lifespans. As a consequence, it seems possible that an ID token expires before the access one. And as far as I understood the source, the
RefreshToken(Reactive)OAuth2AuthorizedClientProvider
refreshes tokens only if the access token expired which leaves room for expired ID token in the security context.To Reproduce
Using an authorization server refreshing ID token as part of the refresh token flow (Keyckloak is one, and, according to the Stackoverflow question linked below, Spring Authorization Server seems to be another):
oauth2Client
configured withoauth2Login
and RP-Initated Logout(Reactive)OAuth2AuthorizedClientManager
to retrieve the access token - like Spring Cloud GatewayTokenRelay=
filter does - is enough.POST
request to the/logout
endpoint)Expected behavior
clockSkew
)OidcUser
) and if the refresh token response contains an ID token, then the ID token in the security context should be updatedend_session_endpoint
should be built with the last issued ID tokenThe
OidcClientInitiated(Server)LogoutSuccessHandler
currently uses the(Reactive)ClientRegistrationRepository
which doesn't trigger the refresh token flow in case of expired tokens. Shouldn't it use the(Reactive)OAuth2AuthorizedClientManager
instead?Some StackOverflow questions related to this issue:
The text was updated successfully, but these errors were encountered: