From afbe0b232b834f99b79fb9a00fdb9b313d79ec86 Mon Sep 17 00:00:00 2001 From: dzcr <1137729123@qq.com> Date: Fri, 30 Sep 2022 11:13:43 +0800 Subject: [PATCH] Adjust based on review. --- .../main/asciidoc/spring-cloud-openfeign.adoc | 4 +- .../openfeign/FeignAutoConfiguration.java | 13 ++-- .../OAuth2AccessTokenInterceptor.java | 45 ++++------- .../FeignAutoConfigurationTests.java | 23 +++--- .../OAuth2AccessTokenInterceptorTests.java | 75 +++++++------------ 5 files changed, 59 insertions(+), 101 deletions(-) diff --git a/docs/src/main/asciidoc/spring-cloud-openfeign.adoc b/docs/src/main/asciidoc/spring-cloud-openfeign.adoc index 3dd8e88e..2c893da4 100644 --- a/docs/src/main/asciidoc/spring-cloud-openfeign.adoc +++ b/docs/src/main/asciidoc/spring-cloud-openfeign.adoc @@ -813,9 +813,9 @@ Sometimes, when load balancing is enabled for Feign clients, you may want to use feign.oauth2.load-balanced=true When the flag is set to true, and the oauth2 client context resource details are present, a bean of class `OAuth2AccessTokenInterceptor` is created. Before each request, the interceptor resolves the required access token and includes it as a header. -`OAuth2AccessTokenInterceptor` uses the `AuthorizedClientServiceOAuth2AuthorizedClientManager` to get `OAuth2AuthorizedClient` that holds an `OAuth2AccessToken`. If the user has specified an OAuth2 `clientId` using the `spring.cloud.openfeign.oauth2.clientId` property, it will be used to retrieve the token. If the token is not retrieved or the `clientId` has not been specified, the `serviceId` retrieved from the `url` host segment will be used. +`OAuth2AccessTokenInterceptor` uses the `AuthorizedClientServiceOAuth2AuthorizedClientManager` to get `OAuth2AuthorizedClient` that holds an `OAuth2AccessToken`. If the user has specified an OAuth2 `clientRegistrationId` using the `spring.cloud.openfeign.oauth2.clientRegistrationId` property, it will be used to retrieve the token. If the token is not retrieved or the `clientRegistrationId` has not been specified, the `serviceId` retrieved from the `url` host segment will be used. -TIP:: Using the `serviceId` as OAuth2 client id is convenient for load-balanced Feign clients. For non-load-balanced ones, the property-based `clientId` is a suitable approach. +TIP:: Using the `serviceId` as OAuth2 client registrationId is convenient for load-balanced Feign clients. For non-load-balanced ones, the property-based `clientRegistrationId` is a suitable approach. === Transform the load-balanced HTTP request diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignAutoConfiguration.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignAutoConfiguration.java index a182c0d9..2bec6315 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignAutoConfiguration.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignAutoConfiguration.java @@ -75,6 +75,7 @@ import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.Import; import org.springframework.data.domain.Page; import org.springframework.data.domain.Sort; +import org.springframework.security.oauth2.client.OAuth2AuthorizedClientManager; import org.springframework.security.oauth2.client.OAuth2AuthorizedClientService; import org.springframework.security.oauth2.client.OAuth2ClientContext; import org.springframework.security.oauth2.client.registration.ClientRegistrationRepository; @@ -380,13 +381,13 @@ public class FeignAutoConfiguration { } @Bean - @ConditionalOnBean({OAuth2AuthorizedClientService.class, ClientRegistrationRepository.class}) + @ConditionalOnBean({ OAuth2AuthorizedClientService.class, ClientRegistrationRepository.class }) public OAuth2AccessTokenInterceptor defaultOAuth2AccessTokenInterceptor( - @Value("${spring.cloud.openfeign.oauth2.clientId:}") String clientId, - OAuth2AuthorizedClientService oAuth2AuthorizedClientService, - ClientRegistrationRepository clientRegistrationRepository) { - return new OAuth2AccessTokenInterceptor(clientId, oAuth2AuthorizedClientService, - clientRegistrationRepository); + @Value("${spring.cloud.openfeign.oauth2.clientRegistrationId:}") String clientRegistrationId, + OAuth2AuthorizedClientService oAuth2AuthorizedClientService, + ClientRegistrationRepository clientRegistrationRepository) { + return new OAuth2AccessTokenInterceptor(clientRegistrationId, oAuth2AuthorizedClientService, + clientRegistrationRepository); } } diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/security/OAuth2AccessTokenInterceptor.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/security/OAuth2AccessTokenInterceptor.java index b421d0ae..dacd8b30 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/security/OAuth2AccessTokenInterceptor.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/security/OAuth2AccessTokenInterceptor.java @@ -17,7 +17,6 @@ package org.springframework.cloud.openfeign.security; import java.net.URI; -import java.time.Instant; import java.util.Optional; import feign.RequestInterceptor; @@ -69,9 +68,7 @@ public class OAuth2AccessTokenInterceptor implements RequestInterceptor { private final String header; - private final String clientId; - - private final OAuth2AuthorizedClientService oAuth2AuthorizedClientService; + private final String clientRegistrationId; private OAuth2AuthorizedClientManager authorizedClientManager; @@ -87,20 +84,20 @@ public class OAuth2AccessTokenInterceptor implements RequestInterceptor { this(null, oAuth2AuthorizedClientService, clientRegistrationRepository); } - public OAuth2AccessTokenInterceptor(String clientId, OAuth2AuthorizedClientService oAuth2AuthorizedClientService, + public OAuth2AccessTokenInterceptor(String clientRegistrationId, + OAuth2AuthorizedClientService oAuth2AuthorizedClientService, ClientRegistrationRepository clientRegistrationRepository) { - this(BEARER, AUTHORIZATION, clientId, oAuth2AuthorizedClientService, clientRegistrationRepository); + this(BEARER, AUTHORIZATION, clientRegistrationId, oAuth2AuthorizedClientService, clientRegistrationRepository); } - public OAuth2AccessTokenInterceptor(String tokenType, String header, String clientId, + public OAuth2AccessTokenInterceptor(String tokenType, String header, String clientRegistrationId, OAuth2AuthorizedClientService oAuth2AuthorizedClientService, ClientRegistrationRepository clientRegistrationRepository) { this.tokenType = tokenType; this.header = header; - this.clientId = clientId; - this.oAuth2AuthorizedClientService = oAuth2AuthorizedClientService; + this.clientRegistrationId = clientRegistrationId; this.authorizedClientManager = new AuthorizedClientServiceOAuth2AuthorizedClientManager( - clientRegistrationRepository, this.oAuth2AuthorizedClientService); + clientRegistrationRepository, oAuth2AuthorizedClientService); } @Override @@ -113,8 +110,8 @@ public class OAuth2AccessTokenInterceptor implements RequestInterceptor { public OAuth2AccessToken getToken(RequestTemplate template) { // If specified, try to use them to get token. - if (StringUtils.hasText(clientId)) { - OAuth2AccessToken token = getToken(clientId); + if (StringUtils.hasText(clientRegistrationId)) { + OAuth2AccessToken token = getToken(clientRegistrationId); if (token != null) { return token; } @@ -129,8 +126,8 @@ public class OAuth2AccessTokenInterceptor implements RequestInterceptor { throw new IllegalStateException("OAuth2 token has not been successfully acquired."); } - protected OAuth2AccessToken getToken(String clientId) { - if (!StringUtils.hasText(clientId)) { + protected OAuth2AccessToken getToken(String clientRegistrationId) { + if (!StringUtils.hasText(clientRegistrationId)) { return null; } @@ -139,26 +136,10 @@ public class OAuth2AccessTokenInterceptor implements RequestInterceptor { principal = ANONYMOUS_AUTHENTICATION; } - // Already exist - OAuth2AuthorizedClient oAuth2AuthorizedClient = oAuth2AuthorizedClientService.loadAuthorizedClient(clientId, - principal.getName()); - if (oAuth2AuthorizedClient != null) { - OAuth2AccessToken accessToken = oAuth2AuthorizedClient.getAccessToken(); - if (accessToken != null && notExpired(accessToken)) { - return accessToken; - } - } - - OAuth2AuthorizeRequest authorizeRequest = OAuth2AuthorizeRequest.withClientRegistrationId(clientId) + OAuth2AuthorizeRequest authorizeRequest = OAuth2AuthorizeRequest.withClientRegistrationId(clientRegistrationId) .principal(principal).build(); OAuth2AuthorizedClient authorizedClient = authorizedClientManager.authorize(authorizeRequest); - return Optional.ofNullable(authorizedClient).map(OAuth2AuthorizedClient::getAccessToken) - .filter(this::notExpired).orElse(null); - } - - protected boolean notExpired(OAuth2AccessToken token) { - return Optional.ofNullable(token).map(OAuth2AccessToken::getExpiresAt) - .map(expire -> expire.isAfter(Instant.now())).orElse(false); + return Optional.ofNullable(authorizedClient).map(OAuth2AuthorizedClient::getAccessToken).orElse(null); } private static String getServiceId(RequestTemplate template) { diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignAutoConfigurationTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignAutoConfigurationTests.java index a8f1c919..b43fba31 100644 --- a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignAutoConfigurationTests.java +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignAutoConfigurationTests.java @@ -139,13 +139,13 @@ class FeignAutoConfigurationTests { @Test void shouldInstantiateFeignOAuth2FeignRequestInterceptor() { runner.withPropertyValues("spring.cloud.openfeign.oauth2.enabled=true", - "spring.cloud.openfeign.oauth2.clientId=feign-client") - .withBean(OAuth2AuthorizedClientService.class, () -> mock(OAuth2AuthorizedClientService.class)) - .withBean(ClientRegistrationRepository.class, () -> mock(ClientRegistrationRepository.class)) - .run(ctx -> { - assertOauth2AccessTokenInterceptorExists(ctx); - assertThatOauth2AccessTokenInterceptorHasSpecifiedIdsPropertyWithValue(ctx, "feign-client"); - }); + "spring.cloud.openfeign.oauth2.clientRegistrationId=feign-client") + .withBean(OAuth2AuthorizedClientService.class, () -> mock(OAuth2AuthorizedClientService.class)) + .withBean(ClientRegistrationRepository.class, () -> mock(ClientRegistrationRepository.class)) + .run(ctx -> { + assertOauth2AccessTokenInterceptorExists(ctx); + assertThatOauth2AccessTokenInterceptorHasSpecifiedIdsPropertyWithValue(ctx, "feign-client"); + }); } private void assertOauth2FeignRequestInterceptorExists(ConfigurableApplicationContext ctx) { @@ -177,20 +177,19 @@ class FeignAutoConfigurationTests { } private void assertThatOauth2AccessTokenInterceptorHasSpecifiedIdsPropertyWithValue( - ConfigurableApplicationContext ctx, String expectedValue) { + ConfigurableApplicationContext ctx, String expectedValue) { final OAuth2AccessTokenInterceptor bean = ctx.getBean(OAuth2AccessTokenInterceptor.class); - assertThat(bean).hasFieldOrPropertyWithValue("clientId", expectedValue); + assertThat(bean).hasFieldOrPropertyWithValue("clientRegistrationId", expectedValue); } private void assertOnlyOneTargeterPresent(ConfigurableApplicationContext ctx, Class beanClass) { - assertThat(ctx.getBeansOfType(Targeter.class)).hasSize(1) - .hasValueSatisfying(new Condition<>( + assertThat(ctx.getBeansOfType(Targeter.class)).hasSize(1).hasValueSatisfying(new Condition<>( beanClass::isInstance, String.format("Targeter should be an instance of %s", beanClass))); } private void assertThatFeignCircuitBreakerTargeterHasGroupEnabledPropertyWithValue( - ConfigurableApplicationContext ctx, boolean expectedValue) { + ConfigurableApplicationContext ctx, boolean expectedValue) { final FeignCircuitBreakerTargeter bean = ctx.getBean(FeignCircuitBreakerTargeter.class); assertThat(bean).hasFieldOrPropertyWithValue("circuitBreakerGroupEnabled", expectedValue); } diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/security/OAuth2AccessTokenInterceptorTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/security/OAuth2AccessTokenInterceptorTests.java index 9a825e57..acedec4e 100644 --- a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/security/OAuth2AccessTokenInterceptorTests.java +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/security/OAuth2AccessTokenInterceptorTests.java @@ -32,14 +32,11 @@ import org.springframework.security.oauth2.client.registration.ClientRegistratio import org.springframework.security.oauth2.client.registration.ClientRegistrationRepository; import org.springframework.security.oauth2.core.AuthorizationGrantType; import org.springframework.security.oauth2.core.OAuth2AccessToken; -import org.springframework.util.AlternativeJdkIdGenerator; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.argThat; -import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -52,6 +49,9 @@ import static org.mockito.Mockito.when; */ class OAuth2AccessTokenInterceptorTests { + private final ClientRegistrationRepository mockClientRegistrationRepository = mock( + ClientRegistrationRepository.class); + private final OAuth2AuthorizedClientService mockOAuth2AuthorizedClientService = mock( OAuth2AuthorizedClientService.class); @@ -62,7 +62,7 @@ class OAuth2AccessTokenInterceptorTests { private RequestTemplate requestTemplate; - private static final String DEFAULT_CLIENT_ID = "feign-client"; + private static final String DEFAULT_CLIENT_REGISTRATION_ID = "feign-client"; @BeforeEach void setUp() { @@ -74,9 +74,8 @@ class OAuth2AccessTokenInterceptorTests { @Test void shouldThrowExceptionWhenNoTokenAcquired() { - when(mockOAuth2AuthorizedClientService.loadAuthorizedClient(anyString(), anyString())).thenReturn(null); oAuth2AccessTokenInterceptor = new OAuth2AccessTokenInterceptor(mockOAuth2AuthorizedClientService, - mock(ClientRegistrationRepository.class)); + mockClientRegistrationRepository); when(mockOAuth2AuthorizedClientManager.authorize(any())).thenReturn(null); oAuth2AccessTokenInterceptor.setAuthorizedClientManager(mockOAuth2AuthorizedClientManager); @@ -87,9 +86,20 @@ class OAuth2AccessTokenInterceptorTests { @Test void shouldAcquireValidToken() { - when(mockOAuth2AuthorizedClientService.loadAuthorizedClient(anyString(), anyString())).thenReturn(null); oAuth2AccessTokenInterceptor = new OAuth2AccessTokenInterceptor(mockOAuth2AuthorizedClientService, - mock(ClientRegistrationRepository.class)); + mockClientRegistrationRepository); + when(mockOAuth2AuthorizedClientManager.authorize(any())).thenReturn(validTokenOAuth2AuthorizedClient()); + oAuth2AccessTokenInterceptor.setAuthorizedClientManager(mockOAuth2AuthorizedClientManager); + + oAuth2AccessTokenInterceptor.apply(requestTemplate); + + assertThat(requestTemplate.headers().get("Authorization")).contains("Bearer Valid Token"); + } + + @Test + void shouldAcquireValidTokenFromServiceId() { + oAuth2AccessTokenInterceptor = new OAuth2AccessTokenInterceptor(mockOAuth2AuthorizedClientService, + mockClientRegistrationRepository); when(mockOAuth2AuthorizedClientManager.authorize( argThat((OAuth2AuthorizeRequest request) -> ("test").equals(request.getClientRegistrationId())))) .thenReturn(validTokenOAuth2AuthorizedClient()); @@ -100,37 +110,14 @@ class OAuth2AccessTokenInterceptorTests { assertThat(requestTemplate.headers().get("Authorization")).contains("Bearer Valid Token"); } - @Test - void shouldThrowExceptionWhenExpiredTokenAcquired() { - when(mockOAuth2AuthorizedClientService.loadAuthorizedClient(anyString(), anyString())).thenReturn(null); - oAuth2AccessTokenInterceptor = new OAuth2AccessTokenInterceptor(mockOAuth2AuthorizedClientService, - mock(ClientRegistrationRepository.class)); - when(mockOAuth2AuthorizedClientManager.authorize(any())).thenReturn(expiredTokenOAuth2AuthorizedClient()); - oAuth2AccessTokenInterceptor.setAuthorizedClientManager(mockOAuth2AuthorizedClientManager); - - assertThatExceptionOfType(IllegalStateException.class) - .isThrownBy(() -> oAuth2AccessTokenInterceptor.apply(requestTemplate)) - .withMessage("OAuth2 token has not been successfully acquired."); - } - - @Test - void shouldAcquireTokenFromAuthorizedClient() { - when(mockOAuth2AuthorizedClientService.loadAuthorizedClient(eq("test"), anyString())) - .thenReturn(validTokenOAuth2AuthorizedClient()); - oAuth2AccessTokenInterceptor = new OAuth2AccessTokenInterceptor(mockOAuth2AuthorizedClientService, - mock(ClientRegistrationRepository.class)); - - oAuth2AccessTokenInterceptor.apply(requestTemplate); - - assertThat(requestTemplate.headers().get("Authorization")).contains("Bearer Valid Token"); - } - @Test void shouldAcquireValidTokenFromSpecifiedClientId() { - when(mockOAuth2AuthorizedClientService.loadAuthorizedClient(eq("testId"), anyString())) - .thenReturn(validTokenOAuth2AuthorizedClient()); - oAuth2AccessTokenInterceptor = new OAuth2AccessTokenInterceptor("testId", mockOAuth2AuthorizedClientService, - mock(ClientRegistrationRepository.class)); + oAuth2AccessTokenInterceptor = new OAuth2AccessTokenInterceptor(DEFAULT_CLIENT_REGISTRATION_ID, + mockOAuth2AuthorizedClientService, mockClientRegistrationRepository); + when(mockOAuth2AuthorizedClientManager + .authorize(argThat((OAuth2AuthorizeRequest request) -> (DEFAULT_CLIENT_REGISTRATION_ID) + .equals(request.getClientRegistrationId())))).thenReturn(validTokenOAuth2AuthorizedClient()); + oAuth2AccessTokenInterceptor.setAuthorizedClientManager(mockOAuth2AuthorizedClientManager); oAuth2AccessTokenInterceptor.apply(requestTemplate); @@ -142,23 +129,13 @@ class OAuth2AccessTokenInterceptorTests { Instant.now().plusSeconds(60L)); } - private OAuth2AccessToken expiredToken() { - return new OAuth2AccessToken(OAuth2AccessToken.TokenType.BEARER, "Expired Token", - Instant.now().minusSeconds(61L), Instant.now().minusSeconds(60L)); - } - private OAuth2AuthorizedClient validTokenOAuth2AuthorizedClient() { return new OAuth2AuthorizedClient(defaultClientRegistration(), "anonymousUser", validToken()); } - private OAuth2AuthorizedClient expiredTokenOAuth2AuthorizedClient() { - return new OAuth2AuthorizedClient(defaultClientRegistration(), "anonymousUser", expiredToken()); - } - private ClientRegistration defaultClientRegistration() { - return ClientRegistration.withRegistrationId(new AlternativeJdkIdGenerator().generateId().toString()) - .clientId(DEFAULT_CLIENT_ID).tokenUri("mock token uri") - .authorizationGrantType(AuthorizationGrantType.CLIENT_CREDENTIALS).build(); + return ClientRegistration.withRegistrationId(DEFAULT_CLIENT_REGISTRATION_ID).clientId("clientId") + .tokenUri("mock token uri").authorizationGrantType(AuthorizationGrantType.CLIENT_CREDENTIALS).build(); } }