Ensure ID Token is updated after refresh token #16589
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
|
Hi @sjohnr, Could you please review this PR? I encountered this issue during development and took the opportunity to resolve it. Thanks in advance for your time and feedback! |
sjohnr
added
type: enhancement
.../security/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProvider.java
Outdated
Show resolved
Hide resolved
...rg/springframework/security/oauth2/client/oidc/authentication/RefreshOidcIdTokenHandler.java
Outdated
Show resolved
Hide resolved
...rg/springframework/security/oauth2/client/oidc/authentication/RefreshOidcIdTokenHandler.java
Outdated
Show resolved
Hide resolved
...rg/springframework/security/oauth2/client/oidc/authentication/RefreshOidcIdTokenHandler.java
Outdated
Show resolved
Hide resolved
|
Hi @sjohnr, Thanks for your review and suggestions! I’ve updated the implementation based on your feedback. |
|
Thanks @yhao3! I'll take another look at this hopefully early next week. |
|
Hi @sjohnr, Thanks for your response! I’ve made some additional adjustments to |
|
Hi @yhao3, thanks for your updates. Sorry for the delay, it took longer than I expected to work through the updates I felt were needed to get closer to merging this. I will post an update on the original issue for folks to give feedback. |
|
Hi @sjohnr, Thanks for the update! No worries at all, I really appreciate the time and effort you’re putting into this. I’ll keep an eye on the original issue for further feedback. |
...k/security/oauth2/client/oidc/authentication/OidcAuthorizedClientRefreshedEventListener.java
Show resolved
Hide resolved
| * @see OAuth2AuthorizedClientRefreshedEvent | ||
| * @see OidcUserRefreshedEvent | ||
| */ | ||
| public final class OidcAuthorizedClientRefreshedEventListener |
There was a problem hiding this comment.
As it listens for OAuth2AuthorizedClientRefreshedEvent shouldn't this listener be an OAuth2AuthorizedClientRefreshedEventListener?
| /** | ||
| * An {@link ApplicationListener} that listens for events of type | ||
| * {@link OAuth2AuthorizedClientRefreshedEvent} and publishes an event of type | ||
| * {@link OidcUserRefreshedEvent} in order to refresh an {@link OidcUser}. |
There was a problem hiding this comment.
Shouldn't the listener publish OAuth2UserRefreshedEvent if the principal is an OAuth2User but not an OidcUser?
| } | ||
|
|
||
| // The current principal must be an OidcUser | ||
| if (!(authenticationToken.getPrincipal() instanceof OidcUser existingOidcUser)) { |
There was a problem hiding this comment.
Testing for OAuth2User would allow to process OAuth2 refresh token flows without OIDC
|
|
||
| private static final String INVALID_NONCE_ERROR_CODE = "invalid_nonce"; | ||
|
|
||
| private OAuth2UserService<OidcUserRequest, OidcUser> userService = new OidcUserService(); |
There was a problem hiding this comment.
Typing with OidcUserRequest and OidcUser might be too narrow: the OAuth2User should be refreshed even if the provider is not configured with OIDC (calling the userinfo endpoint instead of parsing the ID token again)
...urity/oauth2/client/oidc/authentication/OidcAuthorizedClientRefreshedEventListenerTests.java
Outdated
Show resolved
Hide resolved
Related issue: gh-15509