Ensure ID Token is updated after refresh token #16589

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.

Sign up for GitHub

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

Jump to bottom

Merged

sjohnr merged 2 commits into spring-projects:main from yhao3:gh-15509

Mar 18, 2025

Contributor

yhao3 commented Feb 13, 2025

Related issue: gh-15509

spring-projects-issues added the status: waiting-for-triage label

yhao3 force-pushed the gh-15509 branch from c9d9848 to 8f7b48e Compare

February 13, 2025 16:19

Contributor Author

yhao3 commented Feb 13, 2025

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 self-assigned this

sjohnr added type: enhancement in: oauth2 labels

sjohnr suggested changes

View reviewed changes

Contributor

sjohnr left a comment

Thanks for the PR @yhao3. I have performed an initial review and noted a few issues below for you to consider.

.../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

Contributor Author

yhao3 commented Feb 14, 2025

Thanks for your review and suggestions! I’ve updated the implementation based on your feedback.

Contributor

sjohnr commented Feb 14, 2025

Thanks @yhao3! I'll take another look at this hopefully early next week.

Contributor Author

yhao3 commented Feb 15, 2025

Thanks for your response! I’ve made some additional adjustments to RefreshOidcIdTokenHandler and added tests. Looking forward to your feedback when you have a chance to review.

sjohnr force-pushed the gh-15509 branch from ef9896d to 06a4338 Compare

March 4, 2025 23:49

sjohnr added a commit to yhao3/spring-security that referenced this pull request


          Polish spring-projectsgh-16589

06a4338

Contributor

sjohnr commented Mar 5, 2025

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.

sjohnr mentioned this pull request

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

Open

Contributor Author

yhao3 commented Mar 5, 2025

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.

filiphr reviewed

View reviewed changes

Contributor

filiphr left a comment

This looks like a good solution to solve #15509. I've only added one comment, not sure if it is relevant or not in this scenario

...k/security/oauth2/client/oidc/authentication/OidcAuthorizedClientRefreshedEventListener.java Show resolved Hide resolved

ch4mpy reviewed

View reviewed changes

...k/security/oauth2/client/oidc/authentication/OidcAuthorizedClientRefreshedEventListener.java

+               * @see OAuth2AuthorizedClientRefreshedEvent
+               * @see OidcUserRefreshedEvent
+               */
+              public final class OidcAuthorizedClientRefreshedEventListener

Contributor

ch4mpy Mar 10, 2025

As it listens for OAuth2AuthorizedClientRefreshedEvent shouldn't this listener be an OAuth2AuthorizedClientRefreshedEventListener?

ch4mpy reviewed

View reviewed changes

...k/security/oauth2/client/oidc/authentication/OidcAuthorizedClientRefreshedEventListener.java

+              /**
+               * 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}.

Contributor

ch4mpy Mar 10, 2025

Shouldn't the listener publish OAuth2UserRefreshedEvent if the principal is an OAuth2User but not an OidcUser?

ch4mpy reviewed

View reviewed changes

...k/security/oauth2/client/oidc/authentication/OidcAuthorizedClientRefreshedEventListener.java

+              		}
+              		// The current principal must be an OidcUser
+              		if (!(authenticationToken.getPrincipal() instanceof OidcUser existingOidcUser)) {

Contributor

ch4mpy Mar 10, 2025

Testing for OAuth2User would allow to process OAuth2 refresh token flows without OIDC

ch4mpy reviewed

View reviewed changes

...k/security/oauth2/client/oidc/authentication/OidcAuthorizedClientRefreshedEventListener.java


		private static final String INVALID_NONCE_ERROR_CODE = "invalid_nonce";

		private OAuth2UserService<OidcUserRequest, OidcUser> userService = new OidcUserService();

Contributor

ch4mpy Mar 10, 2025

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)

yhao3 commented

View reviewed changes

...urity/oauth2/client/oidc/authentication/OidcAuthorizedClientRefreshedEventListenerTests.java Outdated Show resolved Hide resolved

sjohnr mentioned this pull request

Support refreshing OIDC ID Token #16253

Open

yhao3 and others added 2 commits

March 18, 2025 17:45


          Ensure ID Token is updated after refresh token

b9cc6e1

Signed-off-by: Hao <kyrieeeee2@gmail.com>


          Polish spring-projectsgh-16589

8ab4ddd

sjohnr force-pushed the gh-15509 branch from 06a4338 to 8ab4ddd Compare

March 18, 2025 22:54

sjohnr approved these changes

View reviewed changes

sjohnr enabled auto-merge (rebase)

March 18, 2025 22:54

sjohnr added this to the 6.5.0-RC1 milestone

sjohnr merged commit 5bb5d0f into spring-projects:main

6 checks passed

jgrandja removed the status: waiting-for-triage label

sjohnr added a commit that referenced this pull request


          Polish gh-16589

5f98ce5

sjohnr added a commit that referenced this pull request


          Add additional validation when refreshing ID tokens

860f130

Issue gh-16589

toob2 commented May 25, 2025 •

edited

Loading

@jgrandja was this fixed in 6.5.0? I'm still having this same issue after upgrading to 6.5.0

Contributor

jgrandja commented May 26, 2025

@toob2 Yes, this is in 6.5.0

toob2 commented May 26, 2025 •

edited

Loading

@toob2 Yes, this is in 6.5.0

@jgrandja this is however still broken, can this ticket be re-opened?

VithouJavaMaestro commented May 27, 2025 •

edited

Loading

Does this work with RefreshTokenReactiveOAuth2AuthorizedClientProvider? I only see a published event in the RefreshTokenOAuth2AuthorizedClientProvider class.

Contributor

jgrandja commented May 30, 2025

this is however still broken, can this ticket be re-opened?

I've confirmed that this fix in 6.5.0 does work.

In order for this solution to work, the RefreshTokenOAuth2AuthorizedClientProvider requires a setApplicationEventPublisher(ApplicationEventPublisher). Can you double check to ensure an ApplicationEventPublisher is set?

If you are using the default OAuth2AuthorizedClientManager @Bean then the ApplicationEventPublisher will automatically be set. However, if you are supplying your own OAuth2AuthorizedClientManager @Bean, then this might be the issue because you need to explicitly configure RefreshTokenOAuth2AuthorizedClientProvider.setApplicationEventPublisher(ApplicationEventPublisher).

jgrandja mentioned this pull request

Ensure ID Token is updated after refresh token (Reactive) #17188

Open

Contributor

jgrandja commented May 30, 2025

@VithouJavaMaestro

Does this work with RefreshTokenReactiveOAuth2AuthorizedClientProvider?

No, it hasn't been implemented yet. I've added gh-17188 to track it.

toob2 commented Jun 2, 2025

@toob2

this is however still broken, can this ticket be re-opened?

I've confirmed that this fix in 6.5.0 does work.

In order for this solution to work, the RefreshTokenOAuth2AuthorizedClientProvider requires a setApplicationEventPublisher(ApplicationEventPublisher). Can you double check to ensure an ApplicationEventPublisher is set?

If you are using the default OAuth2AuthorizedClientManager @Bean then the ApplicationEventPublisher will automatically be set. However, if you are supplying your own OAuth2AuthorizedClientManager @Bean, then this might be the issue because you need to explicitly configure RefreshTokenOAuth2AuthorizedClientProvider.setApplicationEventPublisher(ApplicationEventPublisher).

@jgrandja sorry I didnt mention I'm using reactive SCG, will wait for the fix, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

in: oauth2 type: enhancement