Skip to content

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

Open
evgeniycheban wants to merge 1 commit intospring-projects:mainfrom
evgeniycheban:gh-17188
Open

Ensure ID Token is updated after refresh token (Reactive)#17246
evgeniycheban wants to merge 1 commit intospring-projects:mainfrom
evgeniycheban:gh-17188

Conversation

@evgeniycheban
Copy link
Contributor

@evgeniycheban evgeniycheban commented Jun 14, 2025

Closes gh-17188

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 14, 2025
@jgrandja jgrandja self-assigned this Jun 16, 2025
@jgrandja jgrandja added type: enhancement A general enhancement in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 16, 2025
@jgrandja jgrandja added this to the 7.0.x milestone Jun 16, 2025
@evgeniycheban evgeniycheban marked this pull request as draft June 17, 2025 23:40
@jgrandja jgrandja modified the milestones: 7.0.x, 7.1.0-M1 Nov 12, 2025
@jgrandja
Copy link
Contributor

jgrandja commented Nov 12, 2025

Thanks for your patience @evgeniycheban and apologies that I could not get this into 7.0. We had quite a workload for this round of majors.

I have scheduled this for 7.1.0-M1 and will review this soon. In the meantime, when you have a moment can you please rebase. Thank you.

@evgeniycheban evgeniycheban force-pushed the gh-17188 branch 2 times, most recently from be68a71 to ac99b3b Compare November 18, 2025 22:00
@evgeniycheban
Copy link
Contributor Author

evgeniycheban commented Nov 18, 2025

Hi @jgrandja, I have rebased my branch and also updated license headers to meet new pattern, let me know your feedback once you review it, thanks.

@jgrandja
Copy link
Contributor

jgrandja commented Dec 11, 2025

@evgeniycheban I took a look at the PR and noticed that the implementation is different compared to the Servlet implementation.

RefreshTokenReactiveOAuth2AuthorizationSuccessHandler combines the logic in OidcAuthorizedClientRefreshedEventListener and OidcUserRefreshedEventListener and does not make use of Spring Framework's ApplicationEventPublisher and ApplicationListener.

The preference is to keep the Servlet and Reactive implementations as close as possible. Is there a reason you did not make use of ApplicationEventPublisher and ApplicationListener ?

Just a heads up that I'm on PTO starting tomorrow and returning Jan 5 so I'll reply when I return and we can work through this.

@evgeniycheban
Copy link
Contributor Author

evgeniycheban commented Jan 3, 2026

Hi @jgrandja, thank you for reviewing it. The reason I did not use the ApplicationEventPublisher is because in the current implementation there is no support for reactive application events, I can recall that there was a draft PR raised by @marcusdacoregio but it didn't get merged.

@jgrandja
Copy link
Contributor

jgrandja commented Jan 9, 2026

@evgeniycheban Got it. I'll do a detailed review next week and we can work towards merging this.

@jgrandja jgrandja modified the milestones: 7.1.0-M1, 7.1.x, 7.1.0-M2 Jan 19, 2026
jgrandja
jgrandja requested changes Jan 22, 2026
View reviewed changes
Copy link
Contributor

@jgrandja jgrandja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your patience @evgeniycheban.

Overall, this looks good and I believe we are fairly close to merging.

Please see my review comments for further updates.

@evgeniycheban evgeniycheban force-pushed the gh-17188 branch 3 times, most recently from 8874f50 to 1ea9456 Compare February 2, 2026 20:07
@evgeniycheban evgeniycheban marked this pull request as ready for review February 2, 2026 22:41
@evgeniycheban evgeniycheban force-pushed the gh-17188 branch 5 times, most recently from 037e6fb to 38f0a6d Compare February 5, 2026 20:15
jgrandja
jgrandja requested changes Feb 6, 2026
View reviewed changes
Copy link
Contributor

@jgrandja jgrandja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the updates @evgeniycheban. I think we'll be ready to merge after you address some minor feedback.

Also, please add an integration test in ServerOAuth2AuthorizedClientExchangeFilterFunctionITests to ensure RefreshTokenReactiveOAuth2AuthorizationSuccessHandler is fully tested.

...gframework/security/oauth2/client/RefreshTokenReactiveOAuth2AuthorizationSuccessHandler.java Outdated
* @author Evgeniy Cheban
* @since 7.1
*/
public final class RefreshTokenReactiveOAuth2AuthorizationSuccessHandler
Copy link
Contributor

@jgrandja jgrandja Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename to RefreshOidcUserReactiveOAuth2AuthorizationSuccessHandler

...gframework/security/oauth2/client/RefreshTokenReactiveOAuth2AuthorizationSuccessHandler.java Outdated
}

@Override
public Mono<Void> onAuthorizationSuccess(OAuth2AuthorizedClient authorizedClient, Authentication principal,
Copy link
Contributor

@jgrandja jgrandja Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move onAuthorizationSuccess above the setters as it should be right after the members

...gframework/security/oauth2/client/RefreshTokenReactiveOAuth2AuthorizationSuccessHandler.java Outdated
}

private Mono<OAuth2AccessTokenResponse> accessTokenResponse(Map<String, Object> attributes) {
if (attributes.get(OAuth2AccessTokenResponse.class.getName()) instanceof OAuth2AccessTokenResponse response) {
Copy link
Contributor

@jgrandja jgrandja Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move these 2 checks so they are evaluated first, preserving the same order as in OidcAuthorizedClientRefreshedEventListener

...springframework/security/oauth2/client/web/DefaultReactiveOAuth2AuthorizedClientManager.java Outdated

private Function<OAuth2AuthorizeRequest, Mono<Map<String, Object>>> contextAttributesMapper = new DefaultContextAttributesMapper();

private ReactiveOAuth2AuthorizationSuccessHandler refreshTokenSuccessHandler = new RefreshTokenReactiveOAuth2AuthorizationSuccessHandler();
Copy link
Contributor

@jgrandja jgrandja Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this as authorizationSuccessHandler already defaults to the combined (save/refresh) behaviour and applications can override using setAuthorizationSuccessHandler().

@evgeniycheban
Copy link
Contributor Author

evgeniycheban commented Feb 7, 2026

Hi @jgrandja, thank you for the review, after we moved refreshTokenSuccessHandler to DefaultReactiveOAuth2AuthorizedClientManager I noticed that we no longer can access OAuth2AccessTokenResponse which is needed to get id_token from additionalParameters, would it be an option to add a String idToken field to OAuth2AuthorizedClient or even Map<String, Object> additionalParameters to store an additional context for the client?

@evgeniycheban
Ensure ID Token is updated after refresh token (Reactive)
849950f 
Closes spring-projectsgh-17188

Signed-off-by: Evgeniy Cheban <mister.cheban@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jgrandja jgrandja jgrandja requested changes

Assignees

@jgrandja jgrandja

Labels

in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement

Projects

None yet

Milestone

7.1.0-M2

Development

Successfully merging this pull request may close these issues.

Ensure ID Token is updated after refresh token (Reactive)

3 participants

@evgeniycheban @jgrandja @spring-projects-issues