DefaultReactiveOAuth2AuthorizedClientManager never calls UnAuthenticatedServerOAuth2AuthorizedClientRepository

[frzme]

Summary

When using UnAuthenticatedServerOAuth2AuthorizedClientRepository together with the DefaultReactiveOAuth2AuthorizedClientManager authorized clients are not correctly loaded/saved.

This seems to relate to #7468 (@jgrandja )

It seems that when using the UnAuthenticatedServerOAuth2AuthorizedClientRepository it is (and can) never be called from DefaultReactiveOAuth2AuthorizedClientManager as it tries to flatMap a Mono<ServerWebExchange (in the method loadAuthorizedClient). In situations there UnAuthenticatedServerOAuth2AuthorizedClientRepository can/should be used the WebExchange well be null/empty the Mono will be empty and therefore the code in flatMap will not execute.
If ServerWebExchange would be present UnAuthenticatedServerOAuth2AuthorizedClientRepository would throw an Exception there therefore needs to be a way for the ClientManager to call the repository without a WebExchange.

Version

Spring Security 5.2.0 via Spring Boot 2.2.0

[jgrandja]

@frzme This issue may be related to #7546. Can you confirm?

[frzme]

It seems related but I believe it to be not the same. If the Mono returned from currentServerWebExchange() inside

spring-security/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/DefaultReactiveOAuth2AuthorizedClientManager.java

Line 107 in 6ad328f

.switchIfEmpty(Mono.defer(() -> currentServerWebExchange()))

is empty the whole map function on

spring-security/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/DefaultReactiveOAuth2AuthorizedClientManager.java

Lines 108 to 111 in 6ad328f

	.map(exchange -> {
	this.authorizedClientRepository.saveAuthorizedClient(authorizedClient, principal, exchange);
	return authorizedClient;
	})

will not run

It might make sense to .map(exchange -> Optional::of).defaultIfEmpty(Optional.empty()) and then optionalExchange.get() to statisty the wish of UnAuthenticatedServerOAuth2AuthorizedClientRepository to receive a null exchange.

[frzme]

For further context: I'm using the OAuth2 support in the WebClient outside the context of a WebFlux application and requests I'm doing are not related to the SecurityContext associated with a web user. This also seems to be the scope/usecase for the UnAuthenticatedServerOAuth2AuthorizedClientRepository

[jgrandja]

@frzme DefaultReactiveOAuth2AuthorizedClientManager is designed to work within a request context ServerWebExchange. The same applies for DefaultOAuth2AuthorizedClientManager, as it should also be used within a HttpServletRequest context.

I see that you need to use it outside of a request context. We do have a new implementation introduced in 5.2.0, AuthorizedClientServiceOAuth2AuthorizedClientManager, that is intended to be used outside of a HttpServletRequest context. However, we don't have an equivalent reactive implementation. It would be fairly trivial to implement though.

Does this help?

[frzme]

I believe that would help! I have no issues using a different ClientManager, it however was unexpected to me during migration to 5.2.0 that that would be necessary. Will you be providing such a reactive ReactiveAuthorizedClientServiceOAuth2AuthorizedClientManager and if so will it happen in the scope of 5.2.x - having UnAuthenticatedServerOAuth2AuthorizedClientRepository not working anymore in 5.2.x would be an regression?

[jgrandja]

@frzme

having UnAuthenticatedServerOAuth2AuthorizedClientRepository not working anymore in 5.2.x would be an regression

No it wouldn't be. As mentioned, DefaultReactiveOAuth2AuthorizedClientManager is designed to work within a request context ServerWebExchange. So configuring it with UnAuthenticatedServerOAuth2AuthorizedClientRepository would be a misconfiguration since it does not support ServerWebExchange.

I have no issues using a different ClientManager, it however was unexpected to me during migration to 5.2.0 that that would be necessary

With the introduction of ReactiveOAuth2AuthorizedClientManager and OAuth2AuthorizedClientManager in 5.2.0, the intent is to provide implementation(s) of the AuthorizedClientManager to meet different requirements. Even though UnAuthenticatedServerOAuth2AuthorizedClientRepository is meant to work outside of a request context, the caller should also support this as well. It's much more flexible to implement this at the main controller component, which is the AuthorizedClientManager. Also note, that there is no equivalent Servlet implementation of UnAuthenticatedServerOAuth2AuthorizedClientRepository. This is the purpose of AuthorizedClientServiceOAuth2AuthorizedClientManager.

[jgrandja]

@frzme I just logged #7569. Would you be interested in submitting a PR for this?

[jgrandja]

@frzme I'm going to close this issue and associated PR since DefaultReactiveOAuth2AuthorizedClientManager is designed to work within a request context and therefore works-as-designed. Keep track of #7569 as this is the ReactiveOAuth2AuthorizedClientManager implementation you will need to use to operate outside of a request context.

[frzme]

Thank you and sorry for being unresponsive. I think it is rather unfortunate that this exact behavior which worked in the previous version of spring security (and there were also examples doing just this) is now now "broken-as-designed". It also fails transparently - for users it looks like it might be working, it's just re-requesting oauth tokens with every request, which makes it rather hard to spot this.

I'm however fine with a natively supported alternate configuration. I see that someone has already provided one in the linked Issue, I'm happy about that and hope it can become part of spring-security 5.2.2.

[jgrandja]

@frzme I'm re-opening and will address this in 5.2.x. My initial analysis was incorrect. Existing applications using UnAuthenticatedServerOAuth2AuthorizedClientRepository and upgrading to 5.2 should still work. I'll get back to this shortly.

[jgrandja]

@frzme The fix #7684 has been applied in 5.2.x. This should resolve your issue. Let me know how it goes. Thanks again for reporting this.

[jgrandja]

@frzme FYI, we just merged the new AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager and backported to 5.2.x #7717