Fix #14974: DefaultAzureCredential improperly catches AuthenticationF…

…ailedException (#15021) * Fix #14974: DefaultAzureCredential improperly catches AuthenticationFailedException * Throw on AuthenticationFailedException
Azure · Sep 9, 2020 · 57b3ebb · 57b3ebb
1 parent 023988d
commit 57b3ebb
Show file tree

Hide file tree

Showing 11 changed files with 104 additions and 28 deletions.
diff --git a/sdk/identity/Azure.Identity/src/DefaultAzureCredential.cs b/sdk/identity/Azure.Identity/src/DefaultAzureCredential.cs
@@ -155,7 +155,7 @@ private static async ValueTask<AccessToken> GetTokenFromCredentialAsync(TokenCre
 
                     return (token, sources[i]);
                 }
-                catch (AuthenticationFailedException e)
+                catch (CredentialUnavailableException e)
                 {
                     exceptions.Add(e);
                 }

diff --git a/sdk/identity/Azure.Identity/src/VisualStudioCodeCredential.cs b/sdk/identity/Azure.Identity/src/VisualStudioCodeCredential.cs
@@ -64,12 +64,7 @@ private async ValueTask<AccessToken> GetTokenImplAsync(TokenRequestContext reque
                 GetUserSettings(out var tenant, out var environmentName);
 
                 var cloudInstance = GetAzureCloudInstance(environmentName);
-                var storedCredentials = _vscAdapter.GetCredentials(CredentialsSection, environmentName);
-
-                if (!IsRefreshTokenString(storedCredentials))
-                {
-                    throw new CredentialUnavailableException("Need to re-authenticate user in VSCode Azure Account.");
-                }
+                string storedCredentials = GetStoredCredentials(environmentName);
 
                 var result = await _client.AcquireTokenByRefreshToken(requestContext.Scopes, storedCredentials, cloudInstance, tenant, async, cancellationToken).ConfigureAwait(false);
                 return scope.Succeeded(new AccessToken(result.AccessToken, result.ExpiresOn));
@@ -84,6 +79,24 @@ private async ValueTask<AccessToken> GetTokenImplAsync(TokenRequestContext reque
             }
         }
 
+        private string GetStoredCredentials(string environmentName)
+        {
+            try
+            {
+                var storedCredentials = _vscAdapter.GetCredentials(CredentialsSection, environmentName);
+                if (!IsRefreshTokenString(storedCredentials))
+                {
+                    throw new CredentialUnavailableException("Need to re-authenticate user in VSCode Azure Account.");
+                }
+
+                return storedCredentials;
+            }
+            catch (InvalidOperationException ex)
+            {
+                throw new CredentialUnavailableException("Stored credentials not found. Need to authenticate user in VSCode Azure Account.", ex);
+            }
+        }
+
         private static bool IsRefreshTokenString(string str)
         {
             for (var index = 0; index < str.Length; index++)

diff --git a/sdk/identity/Azure.Identity/src/VisualStudioCredential.cs b/sdk/identity/Azure.Identity/src/VisualStudioCredential.cs
@@ -64,7 +64,7 @@ private async ValueTask<AccessToken> GetTokenImplAsync(TokenRequestContext reque
 
             try
             {
-                var tokenProviderPath = GetTokenProviderPath();
+                string tokenProviderPath = GetTokenProviderPath();
                 var tokenProviders = GetTokenProviders(tokenProviderPath);
 
                 var resource = ScopeUtilities.ScopesToResource(requestContext.Scopes);
@@ -120,9 +120,9 @@ private async Task<AccessToken> RunProcessesAsync(List<ProcessStartInfo> process
                 {
                     exceptions.Add(new CredentialUnavailableException($"Process \"{processStartInfo.FileName}\" has non-json output: {output}.", exception));
                 }
-                catch (Exception exception)
+                catch (Exception exception) when (!(exception is OperationCanceledException))
                 {
-                    exceptions.Add(exception);
+                    exceptions.Add(new CredentialUnavailableException($"Process \"{processStartInfo.FileName}\" has failed with unexpected error: {exception.Message}.", exception));
                 }
             }
 
@@ -187,24 +187,35 @@ private VisualStudioTokenProvider[] GetTokenProviders(string tokenProviderPath)
         {
             var content = GetTokenProviderContent(tokenProviderPath);
 
-            using JsonDocument document = JsonDocument.Parse(content);
+            try
+            {
+                using JsonDocument document = JsonDocument.Parse(content);
 
-            JsonElement providersElement = document.RootElement.GetProperty("TokenProviders");
+                JsonElement providersElement = document.RootElement.GetProperty("TokenProviders");
 
-            var providers = new VisualStudioTokenProvider[providersElement.GetArrayLength()];
-            for (int i = 0; i < providers.Length; i++)
-            {
-                JsonElement providerElement = providersElement[i];
+                var providers = new VisualStudioTokenProvider[providersElement.GetArrayLength()];
+                for (int i = 0; i < providers.Length; i++)
+                {
+                    JsonElement providerElement = providersElement[i];
 
-                var path = providerElement.GetProperty("Path").GetString();
-                var preference = providerElement.GetProperty("Preference").GetInt32();
-                var arguments = GetStringArrayPropertyValue(providerElement, "Arguments");
+                    var path = providerElement.GetProperty("Path").GetString();
+                    var preference = providerElement.GetProperty("Preference").GetInt32();
+                    var arguments = GetStringArrayPropertyValue(providerElement, "Arguments");
 
-                providers[i] = new VisualStudioTokenProvider(path, arguments, preference);
-            }
+                    providers[i] = new VisualStudioTokenProvider(path, arguments, preference);
+                }
 
-            Array.Sort(providers);
-            return providers;
+                Array.Sort(providers);
+                return providers;
+            }
+            catch (JsonException exception)
+            {
+                throw new CredentialUnavailableException($"File found at \"{tokenProviderPath}\" isn't a valid JSON file", exception);
+            }
+            catch (Exception exception)
+            {
+                throw new CredentialUnavailableException($"JSON file found at \"{tokenProviderPath}\" has invalid schema.", exception);
+            }
         }
 
         private string GetTokenProviderContent(string tokenProviderPath)

diff --git a/sdk/identity/Azure.Identity/tests/AzureCliCredentialTests.cs b/sdk/identity/Azure.Identity/tests/AzureCliCredentialTests.cs
@@ -47,7 +47,7 @@ public void AuthenticateWithCliCredential_InvalidJsonOutput([Values("", "{}", "{
         {
             var testProcess = new TestProcess { Output = jsonContent };
             AzureCliCredential credential = InstrumentClient(new AzureCliCredential(CredentialPipeline.GetInstance(null), new TestProcessService(testProcess)));
-            Assert.CatchAsync<AuthenticationFailedException>(async () => await credential.GetTokenAsync(new TokenRequestContext(MockScopes.Default)));
+            Assert.ThrowsAsync<AuthenticationFailedException>(async () => await credential.GetTokenAsync(new TokenRequestContext(MockScopes.Default)));
         }
 
         [Test]

diff --git a/sdk/identity/Azure.Identity/tests/DefaultAzureCredentialLiveTests.cs b/sdk/identity/Azure.Identity/tests/DefaultAzureCredentialLiveTests.cs
@@ -237,7 +237,37 @@ public void DefaultAzureCredential_AllCredentialsHaveFailed_CredentialUnavailabl
         }
 
         [Test]
-        public void DefaultAzureCredential_AllCredentialsHaveFailed_AuthenticationFailedException()
+        [NonParallelizable]
+        public void DefaultAzureCredential_AllCredentialsHaveFailed_FirstAuthenticationFailedException()
+        {
+            using var endpoint = new TestEnvVar("MSI_ENDPOINT", "abc");
+
+            var options = Recording.InstrumentClientOptions(new DefaultAzureCredentialOptions
+            {
+                ExcludeEnvironmentCredential = true,
+                ExcludeInteractiveBrowserCredential = true,
+                ExcludeSharedTokenCacheCredential = true,
+            });
+
+            var vscAdapter = new TestVscAdapter(ExpectedServiceName, "Azure", null);
+            var factory = new TestDefaultAzureCredentialFactory(options, new TestFileSystemService(), new TestProcessService(new TestProcess { Error = "Error" }), vscAdapter);
+            var credential = InstrumentClient(new DefaultAzureCredential(factory, options));
+
+            List<ClientDiagnosticListener.ProducedDiagnosticScope> scopes;
+
+            using (ClientDiagnosticListener diagnosticListener = new ClientDiagnosticListener(s => s.StartsWith("Azure.Identity")))
+            {
+                Assert.CatchAsync<AuthenticationFailedException>(async () => await credential.GetTokenAsync(new TokenRequestContext(new[] {"https://vault.azure.net/.default"}), CancellationToken.None));
+                scopes = diagnosticListener.Scopes;
+            }
+
+            Assert.AreEqual(2, scopes.Count);
+            Assert.AreEqual($"{nameof(DefaultAzureCredential)}.{nameof(DefaultAzureCredential.GetToken)}", scopes[0].Name);
+            Assert.AreEqual($"{nameof(ManagedIdentityCredential)}.{nameof(ManagedIdentityCredential.GetToken)}", scopes[1].Name);
+        }
+
+        [Test]
+        public void DefaultAzureCredential_AllCredentialsHaveFailed_LastAuthenticationFailedException()
         {
             var options = Recording.InstrumentClientOptions(new DefaultAzureCredentialOptions
             {

diff --git a/...s/DefaultAzureCredential_AllCredentialsHaveFailed_FirstAuthenticationFailedException.json b/...s/DefaultAzureCredential_AllCredentialsHaveFailed_FirstAuthenticationFailedException.json
diff --git a/...aultAzureCredential_AllCredentialsHaveFailed_FirstAuthenticationFailedExceptionAsync.json b/...aultAzureCredential_AllCredentialsHaveFailed_FirstAuthenticationFailedExceptionAsync.json
diff --git a/...Failed_AuthenticationFailedException.json → ...ed_LastAuthenticationFailedException.json b/...Failed_AuthenticationFailedException.json → ...ed_LastAuthenticationFailedException.json
diff --git a/...d_AuthenticationFailedExceptionAsync.json → ...stAuthenticationFailedExceptionAsync.json b/...d_AuthenticationFailedExceptionAsync.json → ...stAuthenticationFailedExceptionAsync.json
diff --git a/sdk/identity/Azure.Identity/tests/VisualStudioCodeCredentialLiveTests.cs b/sdk/identity/Azure.Identity/tests/VisualStudioCodeCredentialLiveTests.cs
@@ -107,7 +107,7 @@ public void AuthenticateWithVscCredential_NoRefreshToken()
             var options = Recording.InstrumentClientOptions(new VisualStudioCodeCredentialOptions { TenantId = tenantId });
             VisualStudioCodeCredential credential = InstrumentClient(new VisualStudioCodeCredential(options, default, default, fileSystem, vscAdapter));
 
-            Assert.CatchAsync<AuthenticationFailedException>(async () => await credential.GetTokenAsync(new TokenRequestContext(new[] {"https://vault.azure.net/.default"}), CancellationToken.None));
+            Assert.ThrowsAsync<CredentialUnavailableException>(async () => await credential.GetTokenAsync(new TokenRequestContext(new[] {"https://vault.azure.net/.default"}), CancellationToken.None));
         }
 
         [Test]

diff --git a/sdk/identity/Azure.Identity/tests/VisualStudioCredentialTests.cs b/sdk/identity/Azure.Identity/tests/VisualStudioCredentialTests.cs
@@ -141,12 +141,22 @@ public void AuthenticateWithVsCredential_NoDirectoryFound()
 
         [Test]
         public void AuthenticateWithVsCredential_BrokenJsonFileFound()
+        {
+            var (_, _, processOutput) = CredentialTestHelpers.CreateTokenForVisualStudio();
+            var testProcess = new TestProcess { Output = processOutput };
+            var fileSystem = new TestFileSystemService { ReadAllHandler = p => "{\"Some\": " };
+            var credential = InstrumentClient(new VisualStudioCredential(default, default, fileSystem, new TestProcessService(testProcess)));
+            Assert.ThrowsAsync<CredentialUnavailableException>(async () => await credential.GetTokenAsync(new TokenRequestContext(new[]{"https://vault.azure.net/"}), CancellationToken.None));
+        }
+
+        [Test]
+        public void AuthenticateWithVsCredential_IncorrectJsonFileFound()
         {
             var (_, _, processOutput) = CredentialTestHelpers.CreateTokenForVisualStudio();
             var testProcess = new TestProcess { Output = processOutput };
             var fileSystem = new TestFileSystemService { ReadAllHandler = p => "{\"Some\": false}" };
             var credential = InstrumentClient(new VisualStudioCredential(default, default, fileSystem, new TestProcessService(testProcess)));
-            Assert.ThrowsAsync<AuthenticationFailedException>(async () => await credential.GetTokenAsync(new TokenRequestContext(new[]{"https://vault.azure.net/"}), CancellationToken.None));
+            Assert.ThrowsAsync<CredentialUnavailableException>(async () => await credential.GetTokenAsync(new TokenRequestContext(new[]{"https://vault.azure.net/"}), CancellationToken.None));
         }
 
         [Test]
@@ -155,7 +165,7 @@ public void AuthenticateWithVsCredential_ProcessFailed()
             var testProcess = new TestProcess { Error = "Some error" };
             var fileSystem = CredentialTestHelpers.CreateFileSystemForVisualStudio();
             var credential = InstrumentClient(new VisualStudioCredential(default, default, fileSystem, new TestProcessService(testProcess)));
-            Assert.ThrowsAsync<AuthenticationFailedException>(async () => await credential.GetTokenAsync(new TokenRequestContext(new[]{"https://vault.azure.net/"}), CancellationToken.None));
+            Assert.ThrowsAsync<CredentialUnavailableException>(async () => await credential.GetTokenAsync(new TokenRequestContext(new[]{"https://vault.azure.net/"}), CancellationToken.None));
         }
 
         [Test]