spring-projects / spring-security

Spring Security
http://spring.io/projects/spring-security
Apache License 2.0
8.8k stars 5.9k forks source link

`Authentication` in the security context is not updated during the refresh token flow #15509

Open ch4mpy opened 3 months ago

ch4mpy commented 3 months ago

Describe the bug Apparently, neither the ID token nor the userinfo are updated during the refresh token flow in Spring clients with oauth2Login. This has at least two consequences:

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):

Expected behavior

The 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:

ch4mpy commented 2 months ago

@sjohnr I think I progressed in my understanding of the root cause for this issue and re-wrote the description accordingly.