From 07b8aa0b1f0a6bd75aa2fe6cf85b9c02e6c0de18 Mon Sep 17 00:00:00 2001 From: Joe Grandja Date: Wed, 27 Nov 2019 12:11:14 -0500 Subject: [PATCH] DefaultReactiveOAuth2AuthorizedClientManager requires non-null serverWebExchange Issue gh-7544 --- ...ReactiveOAuth2AuthorizedClientManager.java | 5 +- ...iveOAuth2AuthorizedClientManagerTests.java | 47 +++++++------------ 2 files changed, 21 insertions(+), 31 deletions(-) diff --git a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/DefaultReactiveOAuth2AuthorizedClientManager.java b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/DefaultReactiveOAuth2AuthorizedClientManager.java index 5e64d7ba7e..852e910e6f 100644 --- a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/DefaultReactiveOAuth2AuthorizedClientManager.java +++ b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/DefaultReactiveOAuth2AuthorizedClientManager.java @@ -99,15 +99,16 @@ public final class DefaultReactiveOAuth2AuthorizedClientManager implements React private Mono loadAuthorizedClient(String clientRegistrationId, Authentication principal, ServerWebExchange serverWebExchange) { return Mono.justOrEmpty(serverWebExchange) .switchIfEmpty(Mono.defer(() -> currentServerWebExchange())) + .switchIfEmpty(Mono.error(() -> new IllegalArgumentException("serverWebExchange cannot be null"))) .flatMap(exchange -> this.authorizedClientRepository.loadAuthorizedClient(clientRegistrationId, principal, exchange)); } private Mono saveAuthorizedClient(OAuth2AuthorizedClient authorizedClient, Authentication principal, ServerWebExchange serverWebExchange) { return Mono.justOrEmpty(serverWebExchange) .switchIfEmpty(Mono.defer(() -> currentServerWebExchange())) + .switchIfEmpty(Mono.error(() -> new IllegalArgumentException("serverWebExchange cannot be null"))) .flatMap(exchange -> this.authorizedClientRepository.saveAuthorizedClient(authorizedClient, principal, exchange) - .thenReturn(authorizedClient)) - .defaultIfEmpty(authorizedClient); + .thenReturn(authorizedClient)); } private static Mono currentServerWebExchange() { diff --git a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/DefaultReactiveOAuth2AuthorizedClientManagerTests.java b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/DefaultReactiveOAuth2AuthorizedClientManagerTests.java index 2dd4e3ed6a..1e1bcbb3bc 100644 --- a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/DefaultReactiveOAuth2AuthorizedClientManagerTests.java +++ b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/DefaultReactiveOAuth2AuthorizedClientManagerTests.java @@ -65,6 +65,7 @@ public class DefaultReactiveOAuth2AuthorizedClientManagerTests { private MockServerWebExchange serverWebExchange; private Context context; private ArgumentCaptor authorizationContextCaptor; + private PublisherProbe loadAuthorizedClientProbe; private PublisherProbe saveAuthorizedClientProbe; @SuppressWarnings("unchecked") @@ -74,8 +75,9 @@ public class DefaultReactiveOAuth2AuthorizedClientManagerTests { when(this.clientRegistrationRepository.findByRegistrationId( anyString())).thenReturn(Mono.empty()); this.authorizedClientRepository = mock(ServerOAuth2AuthorizedClientRepository.class); + this.loadAuthorizedClientProbe = PublisherProbe.empty(); when(this.authorizedClientRepository.loadAuthorizedClient( - anyString(), any(Authentication.class), any(ServerWebExchange.class))).thenReturn(Mono.empty()); + anyString(), any(Authentication.class), any(ServerWebExchange.class))).thenReturn(this.loadAuthorizedClientProbe.mono()); this.saveAuthorizedClientProbe = PublisherProbe.empty(); when(this.authorizedClientRepository.saveAuthorizedClient( any(OAuth2AuthorizedClient.class), any(Authentication.class), any(ServerWebExchange.class))).thenReturn(this.saveAuthorizedClientProbe.mono()); @@ -131,6 +133,16 @@ public class DefaultReactiveOAuth2AuthorizedClientManagerTests { .hasMessage("authorizeRequest cannot be null"); } + @Test + public void authorizeWhenExchangeIsNullThenThrowIllegalArgumentException() { + OAuth2AuthorizeRequest authorizeRequest = OAuth2AuthorizeRequest.withClientRegistrationId(this.clientRegistration.getRegistrationId()) + .principal(this.principal) + .build(); + assertThatThrownBy(() -> this.authorizedClientManager.authorize(authorizeRequest).block()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("serverWebExchange cannot be null"); + } + @Test public void authorizeWhenClientRegistrationNotFoundThenThrowIllegalArgumentException() { OAuth2AuthorizeRequest authorizeRequest = OAuth2AuthorizeRequest.withClientRegistrationId("invalid-registration-id") @@ -162,7 +174,8 @@ public class DefaultReactiveOAuth2AuthorizedClientManagerTests { assertThat(authorizationContext.getPrincipal()).isEqualTo(this.principal); assertThat(authorizedClient).isNull(); - verify(this.authorizedClientRepository, never()).saveAuthorizedClient(any(), any(), any()); + this.loadAuthorizedClientProbe.assertWasSubscribed(); + this.saveAuthorizedClientProbe.assertWasNotSubscribed(); } @SuppressWarnings("unchecked") @@ -193,38 +206,14 @@ public class DefaultReactiveOAuth2AuthorizedClientManagerTests { this.saveAuthorizedClientProbe.assertWasSubscribed(); } - @Test - public void authorizeWhenNotAuthorizedAndSupportedProviderAndExchangeUnavailableThenAuthorizedButNotSaved() { - when(this.clientRegistrationRepository.findByRegistrationId( - eq(this.clientRegistration.getRegistrationId()))).thenReturn(Mono.just(this.clientRegistration)); - - when(this.authorizedClientProvider.authorize( - any(OAuth2AuthorizationContext.class))).thenReturn(Mono.just(this.authorizedClient)); - - OAuth2AuthorizeRequest authorizeRequest = OAuth2AuthorizeRequest.withClientRegistrationId(this.clientRegistration.getRegistrationId()) - .principal(this.principal) - .build(); - OAuth2AuthorizedClient authorizedClient = this.authorizedClientManager.authorize(authorizeRequest).block(); - - verify(this.authorizedClientProvider).authorize(this.authorizationContextCaptor.capture()); - verify(this.contextAttributesMapper).apply(eq(authorizeRequest)); - - OAuth2AuthorizationContext authorizationContext = this.authorizationContextCaptor.getValue(); - assertThat(authorizationContext.getClientRegistration()).isEqualTo(this.clientRegistration); - assertThat(authorizationContext.getAuthorizedClient()).isNull(); - assertThat(authorizationContext.getPrincipal()).isEqualTo(this.principal); - - assertThat(authorizedClient).isSameAs(this.authorizedClient); - verify(this.authorizedClientRepository, never()).saveAuthorizedClient(any(), any(), any()); - } - @SuppressWarnings("unchecked") @Test public void authorizeWhenAuthorizedAndSupportedProviderThenReauthorized() { when(this.clientRegistrationRepository.findByRegistrationId( eq(this.clientRegistration.getRegistrationId()))).thenReturn(Mono.just(this.clientRegistration)); + this.loadAuthorizedClientProbe = PublisherProbe.of(Mono.just(this.authorizedClient)); when(this.authorizedClientRepository.loadAuthorizedClient( - eq(this.clientRegistration.getRegistrationId()), eq(this.principal), eq(this.serverWebExchange))).thenReturn(Mono.just(this.authorizedClient)); + eq(this.clientRegistration.getRegistrationId()), eq(this.principal), eq(this.serverWebExchange))).thenReturn(this.loadAuthorizedClientProbe.mono()); OAuth2AuthorizedClient reauthorizedClient = new OAuth2AuthorizedClient( this.clientRegistration, this.principal.getName(), @@ -313,7 +302,7 @@ public class DefaultReactiveOAuth2AuthorizedClientManagerTests { assertThat(authorizationContext.getPrincipal()).isEqualTo(this.principal); assertThat(authorizedClient).isSameAs(this.authorizedClient); - verify(this.authorizedClientRepository, never()).saveAuthorizedClient(any(), any(), any()); + this.saveAuthorizedClientProbe.assertWasNotSubscribed(); } @SuppressWarnings("unchecked")