Ensure ID Token is updated after refresh token #16589
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
Signed-off-by: Hao <[email protected]>
|
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
| @@ -232,7 +232,7 @@ public boolean supports(Class<?> authentication) { | |||
| return OAuth2LoginAuthenticationToken.class.isAssignableFrom(authentication); | |||
| } | |||
|
|
|||
| private OidcIdToken createOidcToken(ClientRegistration clientRegistration, | |||
There was a problem hiding this comment.
We will want to do something else here instead of reuse this method directly. We don't want to couple the event handler with the existing AuthenticationProvider. For now, I think it's perfectly fine to duplicate this code in the listener and see how it looks.
| private final OidcAuthorizationCodeAuthenticationProvider oidcAuthorizationCodeAuthenticationProvider; | ||
|
|
||
| public RefreshOidcIdTokenHandler( | ||
| OidcAuthorizationCodeAuthenticationProvider oidcAuthorizationCodeAuthenticationProvider) { | ||
| this.oidcAuthorizationCodeAuthenticationProvider = oidcAuthorizationCodeAuthenticationProvider; | ||
| } |
There was a problem hiding this comment.
See above comment. We don't want to couple the listener with the AuthenticationProvider. Can you please adjust this to directly include the necessary code from the createOidcToken() method?
| OAuth2AccessTokenResponse accessTokenResponse = event.getAccessTokenResponse(); | ||
| OidcIdToken refreshedOidcToken = this.oidcAuthorizationCodeAuthenticationProvider | ||
| .createOidcToken(authorizedClient.getClientRegistration(), accessTokenResponse); | ||
| Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); |
There was a problem hiding this comment.
Can you please adjust this to use a private SecurityContextHolderStrategy securityContextHolderStrategy field instead? It can be initialized with SecurityContextHolder.getContextHolderStrategy() and also include a setter for a custom strategy.
| SecurityContextHolder.getContext() | ||
| .setAuthentication(new OAuth2AuthenticationToken(oidcUser, oidcUser.getAuthorities(), | ||
| oauth2AuthenticationToken.getAuthorizedClientRegistrationId())); |
There was a problem hiding this comment.
Can you please adjust this to create a new empty SecurityContext via this.securityContextHolderStrategy.createEmptyContext() and then set the new context via this.securityContextHolderStrategy.setContext()?
Signed-off-by: Hao <[email protected]>
|
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. |
Signed-off-by: Hao <[email protected]>
|
Hi @sjohnr, Thanks for your response! I’ve made some additional adjustments to |
Related issue: gh-15509