[BugFix-AppConfig] Single authentication support only in client builder (Azure#34038)

mssfang · web-flow · commit 008216f088f9 · 2023-03-27T11:06:19.000-07:00
diff --git a/sdk/appconfiguration/azure-data-appconfiguration/CHANGELOG.md b/sdk/appconfiguration/azure-data-appconfiguration/CHANGELOG.md
@@ -7,7 +7,10 @@
 ### Breaking Changes
 
 ### Bugs Fixed
-
+- Fixed the bug that multiple authentications coexist per builder. App Configuration client builder should only 
+  support single authentication per builder instance. 
+- Moved the validation of authentication to client builder's build method. 
+  
 ### Other Changes
 
 ## 1.4.3 (2023-03-16)
diff --git a/sdk/appconfiguration/azure-data-appconfiguration/src/main/java/com/azure/data/appconfiguration/ConfigurationClientBuilder.java b/sdk/appconfiguration/azure-data-appconfiguration/src/main/java/com/azure/data/appconfiguration/ConfigurationClientBuilder.java
@@ -132,11 +132,12 @@ public final class ConfigurationClientBuilder implements
             .set("Accept", "application/vnd.microsoft.azconfig.kv+json"));
     }
 
-    private final ClientLogger logger = new ClientLogger(ConfigurationClientBuilder.class);
+    private static final ClientLogger LOGGER = new ClientLogger(ConfigurationClientBuilder.class);
     private final List<HttpPipelinePolicy> perCallPolicies = new ArrayList<>();
     private final List<HttpPipelinePolicy> perRetryPolicies = new ArrayList<>();
 
     private ClientOptions clientOptions;
+    private String connectionString;
     private ConfigurationClientCredentials credential;
     private TokenCredential tokenCredential;
 
@@ -202,30 +203,66 @@ public ConfigurationAsyncClient buildAsyncClient() {
      * Builds an instance of ConfigurationClientImpl with the provided parameters.
      *
      * @return an instance of ConfigurationClientImpl.
+     * @throws NullPointerException If {@code connectionString} is null.
+     * @throws IllegalArgumentException If {@code connectionString} is an empty string, the {@code connectionString}
+     * secret is invalid, or the HMAC-SHA256 MAC algorithm cannot be instantiated.
+     * @throws IllegalArgumentException if {@code tokenCredential} is not null. App Configuration builder support single
+     * authentication per builder instance.
      */
     private AzureAppConfigurationImpl buildInnerClient(SyncTokenPolicy syncTokenPolicy) {
+        String endpointLocal = null;
+        ConfigurationClientCredentials credentialsLocal = null;
+        TokenCredential tokenCredentialLocal = null;
+        // validate the authentication setup
+        if (tokenCredential == null && connectionString == null) {
+            throw LOGGER.logExceptionAsError(new NullPointerException("'tokenCredential' and 'connectionString' "
+                + "both can not be null. Set one authentication before creating client."));
+        } else if (tokenCredential != null && connectionString != null) {
+            throw LOGGER.logExceptionAsError(new IllegalArgumentException("Multiple forms of authentication found. "
+                + "TokenCredential should be null if using connection string, vice versa."));
+        } else if (tokenCredential == null) {
+            if (connectionString.isEmpty()) {
+                throw LOGGER.logExceptionAsError(
+                    new IllegalArgumentException("'connectionString' cannot be an empty string."));
+            }
+            try {
+                credentialsLocal = new ConfigurationClientCredentials(connectionString);
+                endpointLocal = credentialsLocal.getBaseUri();
+            } catch (InvalidKeyException err) {
+                throw LOGGER.logExceptionAsError(new IllegalArgumentException("The secret contained within the"
+                    + " connection string is invalid and cannot instantiate the HMAC-SHA256 algorithm.", err));
+            } catch (NoSuchAlgorithmException err) {
+                throw LOGGER.logExceptionAsError(
+                    new IllegalArgumentException("HMAC-SHA256 MAC algorithm cannot be instantiated.", err));
+            }
+        } else {
+            tokenCredentialLocal = this.tokenCredential;
+        }
+
         // Service version
         ConfigurationServiceVersion serviceVersion = (version != null)
             ? version
             : ConfigurationServiceVersion.getLatest();
         // Don't share the default auto-created pipeline between App Configuration client instances.
         return new AzureAppConfigurationImplBuilder()
-                   .pipeline(pipeline == null ? createHttpPipeline(syncTokenPolicy) : pipeline)
+                   .pipeline(pipeline == null ? createDefaultHttpPipeline(
+                       syncTokenPolicy, credentialsLocal, tokenCredentialLocal) : pipeline)
                    .apiVersion(serviceVersion.getVersion())
-                   .endpoint(endpoint)
+                   .endpoint(endpointLocal)
                    .buildClient();
     }
 
-    private HttpPipeline createHttpPipeline(SyncTokenPolicy syncTokenPolicy) {
+    private HttpPipeline createDefaultHttpPipeline(SyncTokenPolicy syncTokenPolicy,
+        ConfigurationClientCredentials credentials, TokenCredential tokenCredential) {
         // Global Env configuration store
         Configuration buildConfiguration = (configuration == null)
             ? Configuration.getGlobalConfiguration()
             : configuration;
 
         // Endpoint
         String buildEndpoint = endpoint;
-        if (tokenCredential == null) {
-            buildEndpoint = getBuildEndpoint();
+        if (credentials != null) {
+            buildEndpoint = credentials.getBaseUri();
         }
         // endpoint cannot be null, which is required in request authentication
         Objects.requireNonNull(buildEndpoint, "'Endpoint' is required and can not be null.");
@@ -249,13 +286,13 @@ private HttpPipeline createHttpPipeline(SyncTokenPolicy syncTokenPolicy) {
         if (tokenCredential != null) {
             // User token based policy
             policies.add(
-                new BearerTokenAuthenticationPolicy(tokenCredential, String.format("%s/.default", buildEndpoint)));
-        } else if (credential != null) {
-            // Use credential based policy
-            policies.add(new ConfigurationCredentialsPolicy(credential));
+                new BearerTokenAuthenticationPolicy(tokenCredential, String.format("%s/.default", endpoint)));
+        } else if (credentials != null) {
+            // Use credentialS based policy
+            policies.add(new ConfigurationCredentialsPolicy(credentials));
         } else {
-            // Throw exception that credential and tokenCredential cannot be null
-            throw logger.logExceptionAsError(
+            // Throw exception that credentials and tokenCredential cannot be null
+            throw LOGGER.logExceptionAsError(
                 new IllegalArgumentException("Missing credential information while building a client."));
         }
         policies.add(syncTokenPolicy);
@@ -297,7 +334,7 @@ public ConfigurationClientBuilder endpoint(String endpoint) {
         try {
             new URL(endpoint);
         } catch (MalformedURLException ex) {
-            throw logger.logExceptionAsWarning(new IllegalArgumentException("'endpoint' must be a valid URL", ex));
+            throw LOGGER.logExceptionAsWarning(new IllegalArgumentException("'endpoint' must be a valid URL", ex));
         }
         this.endpoint = endpoint;
         return this;
@@ -334,31 +371,10 @@ public ConfigurationClientBuilder clientOptions(ClientOptions clientOptions) {
      * @param connectionString Connection string in the format "endpoint={endpoint_value};id={id_value};
      * secret={secret_value}"
      * @return The updated ConfigurationClientBuilder object.
-     * @throws NullPointerException If {@code connectionString} is null.
-     * @throws IllegalArgumentException If {@code connectionString} is an empty string, the {@code connectionString}
-     * secret is invalid, or the HMAC-SHA256 MAC algorithm cannot be instantiated.
      */
     @Override
     public ConfigurationClientBuilder connectionString(String connectionString) {
-        Objects.requireNonNull(connectionString, "'connectionString' cannot be null.");
-
-        if (connectionString.isEmpty()) {
-            throw logger.logExceptionAsError(
-                new IllegalArgumentException("'connectionString' cannot be an empty string."));
-        }
-
-        try {
-            this.credential = new ConfigurationClientCredentials(connectionString);
-        } catch (InvalidKeyException err) {
-            throw logger.logExceptionAsError(new IllegalArgumentException(
-                "The secret contained within the connection string is invalid and cannot instantiate the HMAC-SHA256"
-                    + " algorithm.", err));
-        } catch (NoSuchAlgorithmException err) {
-            throw logger.logExceptionAsError(
-                new IllegalArgumentException("HMAC-SHA256 MAC algorithm cannot be instantiated.", err));
-        }
-
-        this.endpoint = credential.getBaseUri();
+        this.connectionString = connectionString;
         return this;
     }
 
@@ -369,12 +385,9 @@ public ConfigurationClientBuilder connectionString(String connectionString) {
      *
      * @param tokenCredential {@link TokenCredential} used to authorize requests sent to the service.
      * @return The updated ConfigurationClientBuilder object.
-     * @throws NullPointerException If {@code credential} is null.
      */
     @Override
     public ConfigurationClientBuilder credential(TokenCredential tokenCredential) {
-        // token credential can not be null value
-        Objects.requireNonNull(tokenCredential);
         this.tokenCredential = tokenCredential;
         return this;
     }
@@ -443,7 +456,7 @@ public ConfigurationClientBuilder addPolicy(HttpPipelinePolicy policy) {
     @Override
     public ConfigurationClientBuilder httpClient(HttpClient client) {
         if (this.httpClient != null && client == null) {
-            logger.info("HttpClient is being set to 'null' when it was previously configured.");
+            LOGGER.info("HttpClient is being set to 'null' when it was previously configured.");
         }
 
         this.httpClient = client;
@@ -468,7 +481,7 @@ public ConfigurationClientBuilder httpClient(HttpClient client) {
     @Override
     public ConfigurationClientBuilder pipeline(HttpPipeline pipeline) {
         if (this.pipeline != null && pipeline == null) {
-            logger.info("HttpPipeline is being set to 'null' when it was previously configured.");
+            LOGGER.info("HttpPipeline is being set to 'null' when it was previously configured.");
         }
 
         this.pipeline = pipeline;
@@ -543,15 +556,5 @@ public ConfigurationClientBuilder serviceVersion(ConfigurationServiceVersion ver
         this.version = version;
         return this;
     }
-
-    private String getBuildEndpoint() {
-        if (endpoint != null) {
-            return endpoint;
-        } else if (credential != null) {
-            return credential.getBaseUri();
-        } else {
-            return null;
-        }
-    }
 }
 
diff --git a/sdk/appconfiguration/azure-data-appconfiguration/src/test/java/com/azure/data/appconfiguration/ConfigurationClientBuilderTest.java b/sdk/appconfiguration/azure-data-appconfiguration/src/test/java/com/azure/data/appconfiguration/ConfigurationClientBuilderTest.java
@@ -3,6 +3,8 @@
 
 package com.azure.data.appconfiguration;
 
+import com.azure.core.credential.AccessToken;
+import com.azure.core.credential.TokenCredential;
 import com.azure.core.exception.HttpResponseException;
 import com.azure.core.http.HttpClient;
 import com.azure.core.http.policy.ExponentialBackoffOptions;
@@ -28,6 +30,7 @@
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.time.Duration;
+import java.time.OffsetDateTime;
 import java.util.Collections;
 import java.util.Locale;
 import java.util.Objects;
@@ -83,6 +86,26 @@ public void emptyConnectionString() {
         });
     }
 
+    @Test
+    @DoNotRecord
+    public void nullCredentials() {
+        assertThrows(NullPointerException.class, () -> {
+            final ConfigurationClientBuilder builder = new ConfigurationClientBuilder();
+            builder.buildClient();
+        });
+    }
+
+    @Test
+    @DoNotRecord
+    public void multipleCredentialsExist() {
+        assertThrows(IllegalArgumentException.class, () -> {
+            final ConfigurationClientBuilder builder = new ConfigurationClientBuilder();
+            TokenCredential credentials = request -> Mono.just(new AccessToken("this_is_a_token", OffsetDateTime.MAX));
+            builder.connectionString(FAKE_CONNECTION_STRING)
+                .credential(credentials).buildClient();
+        });
+    }
+
     @Test
     @DoNotRecord
     public void missingSecretKey() {
