From 8e99069c95510cb070cc1f1b60cc8d3876e95944 Mon Sep 17 00:00:00 2001 From: Yuxin Bai Date: Thu, 11 Mar 2021 15:33:06 -0500 Subject: [PATCH] Only recreate client if the existing client has different authorities Previously we recreate client before each test. When multiple tests are running concurrently, if a token is issued for test A but the client was deleted during test B, then the token used for test A would be an 'invalid token' and fail the test. Another error scenario is when test A and test B both trigger client creation at the same time, one of them would error out due to `client already exists`. This fix should avoid most of the risk in terms of client creation --- .../CloudFoundryAcceptanceTest.java | 7 ++---- .../cf/CloudFoundryClientConfiguration.java | 4 ++++ .../acceptance/fixtures/uaa/UaaService.java | 24 ++++++++++++++++--- 3 files changed, 27 insertions(+), 8 deletions(-) diff --git a/spring-cloud-app-broker-acceptance-tests/src/test/java/org/springframework/cloud/appbroker/acceptance/CloudFoundryAcceptanceTest.java b/spring-cloud-app-broker-acceptance-tests/src/test/java/org/springframework/cloud/appbroker/acceptance/CloudFoundryAcceptanceTest.java index a219c5c..34a8917 100644 --- a/spring-cloud-app-broker-acceptance-tests/src/test/java/org/springframework/cloud/appbroker/acceptance/CloudFoundryAcceptanceTest.java +++ b/spring-cloud-app-broker-acceptance-tests/src/test/java/org/springframework/cloud/appbroker/acceptance/CloudFoundryAcceptanceTest.java @@ -143,8 +143,8 @@ abstract class CloudFoundryAcceptanceTest { @BeforeEach void setUp(TestInfo testInfo, BrokerProperties brokerProperties) { List appBrokerProperties = getAppBrokerProperties(brokerProperties); - blockingSubscribe(initializeBroker(appBrokerProperties)); blockingSubscribe(initializeUser()); + blockingSubscribe(initializeBroker(appBrokerProperties)); } void setUpForBrokerUpdate(BrokerProperties brokerProperties) { @@ -219,10 +219,7 @@ abstract class CloudFoundryAcceptanceTest { .flatMap(orgId -> cloudFoundryService .getOrCreateSpace(userCloudFoundryService.getOrgName(), userCloudFoundryService.getSpaceName()) .map(SpaceSummary::getId) - .flatMap(spaceId -> uaaService.createClient( - USER_CLIENT_ID, - USER_CLIENT_SECRET, - USER_CLIENT_AUTHORITIES) + .flatMap(spaceId -> uaaService.createClient(USER_CLIENT_ID, USER_CLIENT_SECRET, USER_CLIENT_AUTHORITIES) .then(cloudFoundryService .associateClientWithOrgAndSpace(USER_CLIENT_ID, orgId, spaceId)))); } diff --git a/spring-cloud-app-broker-acceptance-tests/src/test/java/org/springframework/cloud/appbroker/acceptance/fixtures/cf/CloudFoundryClientConfiguration.java b/spring-cloud-app-broker-acceptance-tests/src/test/java/org/springframework/cloud/appbroker/acceptance/fixtures/cf/CloudFoundryClientConfiguration.java index c519ac1..4f85c9a 100644 --- a/spring-cloud-app-broker-acceptance-tests/src/test/java/org/springframework/cloud/appbroker/acceptance/fixtures/cf/CloudFoundryClientConfiguration.java +++ b/spring-cloud-app-broker-acceptance-tests/src/test/java/org/springframework/cloud/appbroker/acceptance/fixtures/cf/CloudFoundryClientConfiguration.java @@ -44,6 +44,8 @@ public class CloudFoundryClientConfiguration { /** * The broker client secret + * Please note that acceptance tests setup would not recreate the client if client id or authorities doesn't change. + * Manual environment clean up is needed on existing test environments if secret changes are necessary. */ public static final String APP_BROKER_CLIENT_SECRET = "app-broker-client-secret"; @@ -61,6 +63,8 @@ public class CloudFoundryClientConfiguration { /** * The user client secret + * Please note that acceptance tests setup would not recreate the client if client id or authorities doesn't change. + * Manual environment clean up is needed on existing test environments if secret changes are necessary. */ public static final String USER_CLIENT_SECRET = "app-broker-user-client-secret"; diff --git a/spring-cloud-app-broker-acceptance-tests/src/test/java/org/springframework/cloud/appbroker/acceptance/fixtures/uaa/UaaService.java b/spring-cloud-app-broker-acceptance-tests/src/test/java/org/springframework/cloud/appbroker/acceptance/fixtures/uaa/UaaService.java index c91b550..10cc41d 100644 --- a/spring-cloud-app-broker-acceptance-tests/src/test/java/org/springframework/cloud/appbroker/acceptance/fixtures/uaa/UaaService.java +++ b/spring-cloud-app-broker-acceptance-tests/src/test/java/org/springframework/cloud/appbroker/acceptance/fixtures/uaa/UaaService.java @@ -16,6 +16,8 @@ package org.springframework.cloud.appbroker.acceptance.fixtures.uaa; +import java.util.Arrays; + import org.cloudfoundry.uaa.UaaClient; import org.cloudfoundry.uaa.clients.CreateClientRequest; import org.cloudfoundry.uaa.clients.DeleteClientRequest; @@ -49,11 +51,22 @@ public class UaaService { } public Mono createClient(String clientId, String clientSecret, String... authorities) { + final String clientNotFound = "CLIENT_NOT_FOUND"; return getUaaClient(clientId) + .defaultIfEmpty(GetClientResponse.builder() + .clientId(clientNotFound) + .authorities(clientNotFound) + .build()) + .filter(response -> authoritiesChanged(response, authorities)) + .delayUntil(response -> { + if (!clientNotFound.equals(response.getClientId())) { + return uaaClient.clients() + .delete(DeleteClientRequest.builder().clientId(clientId).build()) + .doOnError(error -> LOG.error("Error deleting client: " + clientId + " with error: " + error)); + } + return Mono.empty(); + }) .flatMap(response -> uaaClient.clients() - .delete(DeleteClientRequest.builder().clientId(clientId).build()) - .doOnError(error -> LOG.error("Error deleting client: " + clientId + " with error: " + error))) - .then(uaaClient.clients() .create(CreateClientRequest .builder() .clientId(clientId) @@ -65,4 +78,9 @@ public class UaaService { .then(); } + private boolean authoritiesChanged(GetClientResponse response, String... authorities) { + return !response.getAuthorities().containsAll(Arrays.asList(authorities)) || + response.getAuthorities().size() != authorities.length; + } + }