Always return current ClientRegistration in loadAuthorizedClient

This changes `InMemoryOAuth2AuthorizedClientService.loadAuthorizedClient`
(and its reactive counterpart) to always return `OAuth2AuthorizedClient`
instances containing the current `ClientRegistration` as obtained from
the `ClientRegistrationRepository`.

Before this change, the first `ClientRegistration` instance was cached,
with the effect that any changes made in the `ClientRegistrationRepository`
(such as a new client secret) would not have taken effect.

Closes gh-15511

Author: kzander91

oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/InMemoryOAuth2AuthorizedClientService.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2019 the original author or authors.
+ * Copyright 2002-2024 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -80,7 +80,13 @@ public <T extends OAuth2AuthorizedClient> T loadAuthorizedClient(String clientRe
 		if (registration == null) {
 			return null;
 		}
-		return (T) this.authorizedClients.get(new OAuth2AuthorizedClientId(clientRegistrationId, principalName));
+		OAuth2AuthorizedClient cachedAuthorizedClient = this.authorizedClients
+				.get(new OAuth2AuthorizedClientId(clientRegistrationId, principalName));
+		if (cachedAuthorizedClient == null) {
+			return null;
+		}
+		return (T) new OAuth2AuthorizedClient(registration, cachedAuthorizedClient.getPrincipalName(),
+				cachedAuthorizedClient.getAccessToken(), cachedAuthorizedClient.getRefreshToken());
 	}
 
 	@Override

oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/InMemoryReactiveOAuth2AuthorizedClientService.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2019 the original author or authors.
+ * Copyright 2002-2024 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -32,11 +32,11 @@
  *
  * @author Rob Winch
  * @author Vedran Pavic
- * @since 5.1
  * @see OAuth2AuthorizedClientService
  * @see OAuth2AuthorizedClient
  * @see ClientRegistration
  * @see Authentication
+ * @since 5.1
  */
 public final class InMemoryReactiveOAuth2AuthorizedClientService implements ReactiveOAuth2AuthorizedClientService {
 
@@ -47,6 +47,7 @@ public final class InMemoryReactiveOAuth2AuthorizedClientService implements Reac
 	/**
 	 * Constructs an {@code InMemoryReactiveOAuth2AuthorizedClientService} using the
 	 * provided parameters.
+	 *
 	 * @param clientRegistrationRepository the repository of client registrations
 	 */
 	public InMemoryReactiveOAuth2AuthorizedClientService(
@@ -62,8 +63,19 @@ public <T extends OAuth2AuthorizedClient> Mono<T> loadAuthorizedClient(String cl
 		Assert.hasText(clientRegistrationId, "clientRegistrationId cannot be empty");
 		Assert.hasText(principalName, "principalName cannot be empty");
 		return (Mono<T>) this.clientRegistrationRepository.findByRegistrationId(clientRegistrationId)
-			.map((clientRegistration) -> new OAuth2AuthorizedClientId(clientRegistrationId, principalName))
-			.flatMap((identifier) -> Mono.justOrEmpty(this.authorizedClients.get(identifier)));
+				.mapNotNull((clientRegistration) -> {
+					OAuth2AuthorizedClientId id = new OAuth2AuthorizedClientId(clientRegistrationId, principalName);
+					OAuth2AuthorizedClient cachedAuthorizedClient = this.authorizedClients.get(id);
+					if (cachedAuthorizedClient == null) {
+						return null;
+					}
+					// @formatter:off
+					return new OAuth2AuthorizedClient(clientRegistration,
+							cachedAuthorizedClient.getPrincipalName(),
+							cachedAuthorizedClient.getAccessToken(),
+							cachedAuthorizedClient.getRefreshToken());
+					// @formatter:on
+				});
 	}
 
 	@Override

oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/InMemoryOAuth2AuthorizedClientServiceTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2019 the original author or authors.
+ * Copyright 2002-2024 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -33,7 +33,7 @@
 import static org.assertj.core.api.Assertions.assertThatObject;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.BDDMockito.given;
-import static org.mockito.Mockito.mock;
+import static org.mockito.BDDMockito.mock;
 
 /**
  * Tests for {@link InMemoryOAuth2AuthorizedClientService}.
@@ -43,23 +43,23 @@
  */
 public class InMemoryOAuth2AuthorizedClientServiceTests {
 
-	private String principalName1 = "principal-1";
+	private final String principalName1 = "principal-1";
 
-	private String principalName2 = "principal-2";
+	private final String principalName2 = "principal-2";
 
-	private ClientRegistration registration1 = TestClientRegistrations.clientRegistration().build();
+	private final ClientRegistration registration1 = TestClientRegistrations.clientRegistration().build();
 
-	private ClientRegistration registration2 = TestClientRegistrations.clientRegistration2().build();
+	private final ClientRegistration registration2 = TestClientRegistrations.clientRegistration2().build();
 
-	private ClientRegistration registration3 = TestClientRegistrations.clientRegistration()
-		.clientId("client-3")
-		.registrationId("registration-3")
-		.build();
+	private final ClientRegistration registration3 = TestClientRegistrations.clientRegistration()
+			.clientId("client-3")
+			.registrationId("registration-3")
+			.build();
 
-	private ClientRegistrationRepository clientRegistrationRepository = new InMemoryClientRegistrationRepository(
+	private final ClientRegistrationRepository clientRegistrationRepository = new InMemoryClientRegistrationRepository(
 			this.registration1, this.registration2, this.registration3);
 
-	private InMemoryOAuth2AuthorizedClientService authorizedClientService = new InMemoryOAuth2AuthorizedClientService(
+	private final InMemoryOAuth2AuthorizedClientService authorizedClientService = new InMemoryOAuth2AuthorizedClientService(
 			this.clientRegistrationRepository);
 
 	@Test
@@ -79,9 +79,11 @@ public void constructorWhenAuthorizedClientsIsNullThenThrowIllegalArgumentExcept
 	@Test
 	public void constructorWhenAuthorizedClientsProvidedThenUseProvidedAuthorizedClients() {
 		String registrationId = this.registration3.getRegistrationId();
+		OAuth2AuthorizedClient authorizedClient = new OAuth2AuthorizedClient(this.registration3, this.principalName1,
+				mock(OAuth2AccessToken.class));
 		Map<OAuth2AuthorizedClientId, OAuth2AuthorizedClient> authorizedClients = Collections.singletonMap(
 				new OAuth2AuthorizedClientId(this.registration3.getRegistrationId(), this.principalName1),
-				mock(OAuth2AuthorizedClient.class));
+				authorizedClient);
 		ClientRegistrationRepository clientRegistrationRepository = mock(ClientRegistrationRepository.class);
 		given(clientRegistrationRepository.findByRegistrationId(eq(registrationId))).willReturn(this.registration3);
 		InMemoryOAuth2AuthorizedClientService authorizedClientService = new InMemoryOAuth2AuthorizedClientService(
@@ -92,7 +94,7 @@ public void constructorWhenAuthorizedClientsProvidedThenUseProvidedAuthorizedCli
 	@Test
 	public void loadAuthorizedClientWhenClientRegistrationIdIsNullThenThrowIllegalArgumentException() {
 		assertThatIllegalArgumentException()
-			.isThrownBy(() -> this.authorizedClientService.loadAuthorizedClient(null, this.principalName1));
+				.isThrownBy(() -> this.authorizedClientService.loadAuthorizedClient(null, this.principalName1));
 	}
 
 	@Test
@@ -104,14 +106,14 @@ public void loadAuthorizedClientWhenPrincipalNameIsNullThenThrowIllegalArgumentE
 	@Test
 	public void loadAuthorizedClientWhenClientRegistrationNotFoundThenReturnNull() {
 		OAuth2AuthorizedClient authorizedClient = this.authorizedClientService
-			.loadAuthorizedClient("registration-not-found", this.principalName1);
+				.loadAuthorizedClient("registration-not-found", this.principalName1);
 		assertThat(authorizedClient).isNull();
 	}
 
 	@Test
 	public void loadAuthorizedClientWhenClientRegistrationFoundButNotAssociatedToPrincipalThenReturnNull() {
 		OAuth2AuthorizedClient authorizedClient = this.authorizedClientService
-			.loadAuthorizedClient(this.registration1.getRegistrationId(), "principal-not-found");
+				.loadAuthorizedClient(this.registration1.getRegistrationId(), "principal-not-found");
 		assertThat(authorizedClient).isNull();
 	}
 
@@ -123,14 +125,42 @@ public void loadAuthorizedClientWhenClientRegistrationFoundAndAssociatedToPrinci
 				mock(OAuth2AccessToken.class));
 		this.authorizedClientService.saveAuthorizedClient(authorizedClient, authentication);
 		OAuth2AuthorizedClient loadedAuthorizedClient = this.authorizedClientService
-			.loadAuthorizedClient(this.registration1.getRegistrationId(), this.principalName1);
-		assertThat(loadedAuthorizedClient).isEqualTo(authorizedClient);
+				.loadAuthorizedClient(this.registration1.getRegistrationId(), this.principalName1);
+		assertAuthorizedClientEquals(authorizedClient, loadedAuthorizedClient);
+	}
+
+	@Test
+	public void loadAuthorizedClientWhenClientRegistrationIsUpdatedThenReturnAuthorizedClientWithUpdatedClientRegistration() {
+		ClientRegistration updatedRegistration = ClientRegistration.withClientRegistration(this.registration1)
+				.clientSecret("updated secret")
+				.build();
+		ClientRegistrationRepository repository = mock(ClientRegistrationRepository.class);
+		given(repository.findByRegistrationId(this.registration1.getRegistrationId())).willReturn(this.registration1,
+				updatedRegistration);
+
+		Authentication authentication = mock(Authentication.class);
+		given(authentication.getName()).willReturn(this.principalName1);
+
+		InMemoryOAuth2AuthorizedClientService service = new InMemoryOAuth2AuthorizedClientService(repository);
+
+		OAuth2AuthorizedClient authorizedClient = new OAuth2AuthorizedClient(this.registration1, this.principalName1,
+				mock(OAuth2AccessToken.class));
+		service.saveAuthorizedClient(authorizedClient, authentication);
+
+		OAuth2AuthorizedClient authorizedClientWithUpdatedRegistration = new OAuth2AuthorizedClient(updatedRegistration,
+				this.principalName1, mock(OAuth2AccessToken.class));
+		OAuth2AuthorizedClient firstLoadedClient = service.loadAuthorizedClient(this.registration1.getRegistrationId(),
+				this.principalName1);
+		OAuth2AuthorizedClient secondLoadedClient = service.loadAuthorizedClient(this.registration1.getRegistrationId(),
+				this.principalName1);
+		assertAuthorizedClientEquals(authorizedClient, firstLoadedClient);
+		assertAuthorizedClientEquals(authorizedClientWithUpdatedRegistration, secondLoadedClient);
 	}
 
 	@Test
 	public void saveAuthorizedClientWhenAuthorizedClientIsNullThenThrowIllegalArgumentException() {
 		assertThatIllegalArgumentException()
-			.isThrownBy(() -> this.authorizedClientService.saveAuthorizedClient(null, mock(Authentication.class)));
+				.isThrownBy(() -> this.authorizedClientService.saveAuthorizedClient(null, mock(Authentication.class)));
 	}
 
 	@Test
@@ -147,20 +177,20 @@ public void saveAuthorizedClientWhenSavedThenCanLoad() {
 				mock(OAuth2AccessToken.class));
 		this.authorizedClientService.saveAuthorizedClient(authorizedClient, authentication);
 		OAuth2AuthorizedClient loadedAuthorizedClient = this.authorizedClientService
-			.loadAuthorizedClient(this.registration3.getRegistrationId(), this.principalName2);
-		assertThat(loadedAuthorizedClient).isEqualTo(authorizedClient);
+				.loadAuthorizedClient(this.registration3.getRegistrationId(), this.principalName2);
+		assertAuthorizedClientEquals(authorizedClient, loadedAuthorizedClient);
 	}
 
 	@Test
 	public void removeAuthorizedClientWhenClientRegistrationIdIsNullThenThrowIllegalArgumentException() {
 		assertThatIllegalArgumentException()
-			.isThrownBy(() -> this.authorizedClientService.removeAuthorizedClient(null, this.principalName2));
+				.isThrownBy(() -> this.authorizedClientService.removeAuthorizedClient(null, this.principalName2));
 	}
 
 	@Test
 	public void removeAuthorizedClientWhenPrincipalNameIsNullThenThrowIllegalArgumentException() {
 		assertThatIllegalArgumentException().isThrownBy(() -> this.authorizedClientService
-			.removeAuthorizedClient(this.registration3.getRegistrationId(), null));
+				.removeAuthorizedClient(this.registration3.getRegistrationId(), null));
 	}
 
 	@Test
@@ -171,13 +201,38 @@ public void removeAuthorizedClientWhenSavedThenRemoved() {
 				mock(OAuth2AccessToken.class));
 		this.authorizedClientService.saveAuthorizedClient(authorizedClient, authentication);
 		OAuth2AuthorizedClient loadedAuthorizedClient = this.authorizedClientService
-			.loadAuthorizedClient(this.registration2.getRegistrationId(), this.principalName2);
+				.loadAuthorizedClient(this.registration2.getRegistrationId(), this.principalName2);
 		assertThat(loadedAuthorizedClient).isNotNull();
 		this.authorizedClientService.removeAuthorizedClient(this.registration2.getRegistrationId(),
 				this.principalName2);
 		loadedAuthorizedClient = this.authorizedClientService
-			.loadAuthorizedClient(this.registration2.getRegistrationId(), this.principalName2);
+				.loadAuthorizedClient(this.registration2.getRegistrationId(), this.principalName2);
 		assertThat(loadedAuthorizedClient).isNull();
 	}
 
+	private static void assertAuthorizedClientEquals(OAuth2AuthorizedClient expected, OAuth2AuthorizedClient actual) {
+		assertThat(actual).isNotNull();
+		assertThat(actual.getClientRegistration().getRegistrationId())
+				.isEqualTo(expected.getClientRegistration().getRegistrationId());
+		assertThat(actual.getClientRegistration().getClientName())
+				.isEqualTo(expected.getClientRegistration().getClientName());
+		assertThat(actual.getClientRegistration().getRedirectUri())
+				.isEqualTo(expected.getClientRegistration().getRedirectUri());
+		assertThat(actual.getClientRegistration().getAuthorizationGrantType())
+				.isEqualTo(expected.getClientRegistration().getAuthorizationGrantType());
+		assertThat(actual.getClientRegistration().getClientAuthenticationMethod())
+				.isEqualTo(expected.getClientRegistration().getClientAuthenticationMethod());
+		assertThat(actual.getClientRegistration().getClientId())
+				.isEqualTo(expected.getClientRegistration().getClientId());
+		assertThat(actual.getClientRegistration().getClientSecret())
+				.isEqualTo(expected.getClientRegistration().getClientSecret());
+		assertThat(actual.getPrincipalName()).isEqualTo(expected.getPrincipalName());
+		assertThat(actual.getAccessToken().getTokenType()).isEqualTo(expected.getAccessToken().getTokenType());
+		assertThat(actual.getAccessToken().getTokenValue()).isEqualTo(expected.getAccessToken().getTokenValue());
+		assertThat(actual.getAccessToken().getIssuedAt()).isEqualTo(expected.getAccessToken().getIssuedAt());
+		assertThat(actual.getAccessToken().getExpiresAt()).isEqualTo(expected.getAccessToken().getExpiresAt());
+		assertThat(actual.getAccessToken().getScopes()).isEqualTo(expected.getAccessToken().getScopes());
+		assertThat(actual.getRefreshToken()).isEqualTo(expected.getRefreshToken());
+	}
+
 }