Ensure ID Token is updated after refresh token (Reactive)

[evgeniycheban]

Closes gh-17188

[jgrandja]

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]

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]

@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]

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]

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

[jgrandja]

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.

[jgrandja]

ReactiveOAuth2AuthorizationSuccessHandler is not designed to be used with a ReactiveOAuth2AuthorizedClientProvider and instead is meant to be used with a ReactiveOAuth2AuthorizedClientManager.

Can you re-work this and set RefreshTokenReactiveOAuth2AuthorizationSuccessHandler in DefaultReactiveOAuth2AuthorizedClientManager ?

[evgeniycheban]

Done

[jgrandja]

Update to 7.1

[evgeniycheban]

Done

[jgrandja]

OidcAuthorizedClientRefreshedEventListener performs a couple of validation checks that are missing here. Specifically, checks OAuth2AccessTokenResponse to ensure openid scope is associated with the access token and an id_token is returned. These pre-checks should be in place as we only want to proceed if this is an OIDC refresh token flow.

[evgeniycheban]

Done.

[jgrandja]

Instead of adding this logic, can we not update the SecurityContext in the reactor context in RefreshTokenReactiveOAuth2AuthorizationSuccessHandler right after this.serverSecurityContextRepository.save(exchange, securityContext) to avoid this serverSecurityContextRepository.load()?

[evgeniycheban]

The tricky part here is that we can't propagate the refreshed SecurityContext to the upstream in a reactor context, it is propagated always one-way from upstream to downstream, so that's why I used saving the refreshed SecurityContext in ServerSecurityContextRepository to be able later to continue the reactive chain with the refreshed SecurityContext and propagate it using contextWrite.

I also considered using a custom CoreSubscriber similar to how it's done in SecurityReactorContextConfiguration, but I'm not sure if it would be the right approach here.
What do you think?

[jgrandja]

it is propagated always one-way from upstream to downstream

I'm not sure I understand this?

The tricky part here is that we can't propagate the refreshed SecurityContext to the upstream in a reactor context

Please see the following issue to see if it helps?

#16665

[evgeniycheban]

I will have a look at context-propagation library, thank you.

[evgeniycheban]

I get the point you provided here @jgrandja, however, in order to propagate the refreshed SecurityContext to the next ExchangeFilterFunction in ServerOAuth2AuthorizedClientExchangeFilterFunction:

return next.exchange(request)
	.transform((responseMono) -> this.clientResponseHandler.handleResponse(request, responseMono));

updating SecurityContext in the reactor context in RefreshTokenReactiveOAuth2AuthorizationSuccessHandler has no effect, so we need to write the new context to the reactive chain before calling next.exchange(request):

return effectiveAuthentication(request).flatMap((authentication) -> next.exchange(request)
	.transform((responseMono) -> this.clientResponseHandler.handleResponse(request, responseMono))
	.contextWrite(ReactiveSecurityContextHolder.withAuthentication(authentication)));

At this point we load the SecurityContext to get the most recent version to be passed to the downstream.

Another option could be moving the logic from RefreshTokenReactiveOAuth2AuthorizationSuccessHandler to RefreshTokenReactiveOAuth2AuthorizedClientProvider and enriching an OAuth2AuthorizedClient with the new Authentication object when the token is refreshed and then propagate it to the downstream to the next ExchangeFilterFunction in ServerOAuth2AuthorizedClientExchangeFilterFunction.

@jgrandja, I'm also available for a short call if you want to discuss this in more details, just let me know, thanks.

[evgeniycheban]

@jgrandja I can as well commit the change that would demonstrate the failing test if we do .contextWrite in RefreshTokenReactiveOAuth2AuthorizationSuccessHandler instead of ServerOAuth2AuthorizedClientExchangeFilterFunction, please let me know.

[jgrandja]

Please move the public methods above the private methods.

[evgeniycheban]

Done.