Skip to content

Commit ad80cd5

Browse files
AlexanderSherAlexanderSher
authored
Fix Azure#14974: [BUG] DefaultAzureCredential improperly catches AuthenticationFailedException (Azure#15057)
* Fix Azure#14974: [BUG] DefaultAzureCredential improperly catches AuthenticationFailedExcpetion * Check for cancellationToken * - Update Changelog - Add workaround for test playback * Addressed PR comments
1 parent 4c331c5 commit ad80cd5

25 files changed

+214
-90
lines changed

‎sdk/identity/Azure.Identity/CHANGELOG.md‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,25 @@
11
# Release History
2-
## 1.3.0-preview.1 (Unreleased)
2+
## 1.3.0-beta.1 (2020-09-11)
33

44
### New Features
55
- Restoring Application Authentication APIs from 1.2.0-preview.6
6+
- Added support for App Service Managed Identity API version `2019-08-01` ([#13687](https://github.com/Azure/azure-sdk-for-net/issues/13687))
67
- Added `IncludeX5CClaimHeader` to `ClientCertificateCredentialOptions` to enable subject name / issuer authentication with the `ClientCertificateCredential`.
78
- Added `RedirectUri` to `InteractiveBrowserCredentialOptions` to enable authentication with user specified application with a custom redirect url.
89
- Added `IdentityModelFactory` to enable constructing models from the Azure.Identity library for mocking.
10+
- Unify exception handling between `DefaultAzureCredential` and `ChainedTokenCredential` ([#14408](https://github.com/Azure/azure-sdk-for-net/issues/14408))
911

1012
### Fixes and improvements
13+
- Updated `MsalPublicClient` and `MsalConfidentialClient` to respect `CancellationToken` during initialization ([#13201](https://github.com/Azure/azure-sdk-for-net/issues/13201))
14+
- Fixed `VisualStudioCodeCredential` crashes on macOS (Issue [#14362](https://github.com/Azure/azure-sdk-for-net/issues/14362))
1115
- Fixed issue with non GUID Client Ids (Issue [#14585](https://github.com/Azure/azure-sdk-for-net/issues/14585))
1216
- Update `VisualStudioCredential` and `VisualStudioCodeCredential` to throw `CredentialUnavailableException` for ADFS tenant (Issue [#14639](https://github.com/Azure/azure-sdk-for-net/issues/14639))
1317

18+
## 1.2.3 (2020-09-11)
19+
20+
### Fixes and improvements
21+
- Fixed issue with `DefaultAzureCredential` incorrectly catching `AuthenticationFailedException` (Issue [#14974](https://github.com/Azure/azure-sdk-for-net/issues/14974))
22+
- Fixed issue with `DefaultAzureCredential` throwing exceptions during concurrent calls (Issue [#15013](https://github.com/Azure/azure-sdk-for-net/issues/15013))
1423

1524
## 1.2.2 (2020-08-20)
1625

‎sdk/identity/Azure.Identity/src/AuthenticationFailedException.cs‎

Lines changed: 0 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -31,28 +31,5 @@ public AuthenticationFailedException(string message, Exception innerException)
3131
: base(message, innerException)
3232
{
3333
}
34-
35-
internal static AuthenticationFailedException CreateAggregateException(string message, IList<Exception> exceptions)
36-
{
37-
// Build the credential unavailable message, this code is only reachable if all credentials throw AuthenticationFailedException
38-
StringBuilder errorMsg = new StringBuilder(message);
39-
40-
bool allCredentialUnavailableException = true;
41-
foreach (var exception in exceptions)
42-
{
43-
allCredentialUnavailableException &= exception is CredentialUnavailableException;
44-
errorMsg.Append(Environment.NewLine).Append("- ").Append(exception.Message);
45-
}
46-
47-
var innerException = exceptions.Count == 1
48-
? exceptions[0]
49-
: new AggregateException("Multiple exceptions were encountered while attempting to authenticate.", exceptions);
50-
51-
// If all credentials have thrown CredentialUnavailableException, throw CredentialUnavailableException,
52-
// otherwise throw AuthenticationFailedException
53-
return allCredentialUnavailableException
54-
? new CredentialUnavailableException(errorMsg.ToString(), innerException)
55-
: new AuthenticationFailedException(errorMsg.ToString(), innerException);
56-
}
5734
}
5835
}

‎sdk/identity/Azure.Identity/src/Azure.Identity.csproj‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
<PropertyGroup>
33
<Description>This is the implementation of the Azure SDK Client Library for Azure Identity</Description>
44
<AssemblyTitle>Microsoft Azure.Identity Component</AssemblyTitle>
5-
<Version>1.3.0-preview.1</Version>
5+
<Version>1.3.0-beta.1</Version>
66
<ApiCompatVersion>1.2.2</ApiCompatVersion>
77
<PackageTags>Microsoft Azure Identity;$(PackageCommonTags)</PackageTags>
88
<TargetFrameworks>$(RequiredTargetFrameworks)</TargetFrameworks>

‎sdk/identity/Azure.Identity/src/ChainedTokenCredential.cs‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ public class ChainedTokenCredential : TokenCredential
1818
{
1919
private const string AggregateAllUnavailableErrorMessage = "The ChainedTokenCredential failed to retrieve a token from the included credentials.";
2020

21-
private const string AggregateCredentialFailedErrorMessage = "The ChainedTokenCredential failed due to an unhandled exception: ";
21+
private const string AuthenticationFailedErrorMessage = "The ChainedTokenCredential failed due to an unhandled exception: ";
2222

2323
private readonly TokenCredential[] _sources;
2424

@@ -77,7 +77,7 @@ private async ValueTask<AccessToken> GetTokenImplAsync(bool async, TokenRequestC
7777
var groupScopeHandler = new ScopeGroupHandler(default);
7878
try
7979
{
80-
List<Exception> exceptions = new List<Exception>();
80+
List<CredentialUnavailableException> exceptions = new List<CredentialUnavailableException>();
8181
foreach (TokenCredential source in _sources)
8282
{
8383
try
@@ -88,18 +88,17 @@ private async ValueTask<AccessToken> GetTokenImplAsync(bool async, TokenRequestC
8888
groupScopeHandler.Dispose(default, default);
8989
return token;
9090
}
91-
catch (AuthenticationFailedException e)
91+
catch (CredentialUnavailableException e)
9292
{
9393
exceptions.Add(e);
9494
}
95-
catch (Exception e) when (!(e is OperationCanceledException))
95+
catch (Exception e) when (!cancellationToken.IsCancellationRequested)
9696
{
97-
exceptions.Add(e);
98-
throw AuthenticationFailedException.CreateAggregateException(AggregateCredentialFailedErrorMessage + e.Message, exceptions);
97+
throw new AuthenticationFailedException(AuthenticationFailedErrorMessage + e.Message, e);
9998
}
10099
}
101100

102-
throw AuthenticationFailedException.CreateAggregateException(AggregateAllUnavailableErrorMessage, exceptions);
101+
throw CredentialUnavailableException.CreateAggregateException(AggregateAllUnavailableErrorMessage, exceptions);
103102
}
104103
catch (Exception exception)
105104
{

‎sdk/identity/Azure.Identity/src/CredentialUnavailableException.cs‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22
// Licensed under the MIT License.
33

44
using System;
5+
using System.Collections.Generic;
6+
using System.Text;
57
using Azure.Core;
68

79
namespace Azure.Identity
@@ -31,5 +33,24 @@ public CredentialUnavailableException(string message, Exception innerException)
3133
: base(message, innerException)
3234
{
3335
}
36+
37+
internal static CredentialUnavailableException CreateAggregateException(string message, IList<CredentialUnavailableException> exceptions)
38+
{
39+
if (exceptions.Count == 1)
40+
{
41+
return exceptions[0];
42+
}
43+
44+
// Build the credential unavailable message, this code is only reachable if all credentials throw AuthenticationFailedException
45+
StringBuilder errorMsg = new StringBuilder(message);
46+
47+
foreach (var exception in exceptions)
48+
{
49+
errorMsg.Append(Environment.NewLine).Append("- ").Append(exception.Message);
50+
}
51+
52+
var innerException = new AggregateException("Multiple exceptions were encountered while attempting to authenticate.", exceptions);
53+
return new CredentialUnavailableException(errorMsg.ToString(), innerException);
54+
}
3455
}
3556
}

‎sdk/identity/Azure.Identity/src/DefaultAzureCredential.cs‎

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,7 @@ private static async ValueTask<AccessToken> GetTokenFromCredentialAsync(TokenCre
143143

144144
private static async ValueTask<(AccessToken, TokenCredential)> GetTokenFromSourcesAsync(TokenCredential[] sources, TokenRequestContext requestContext, bool async, CancellationToken cancellationToken)
145145
{
146-
List<Exception> exceptions = new List<Exception>();
146+
List<CredentialUnavailableException> exceptions = new List<CredentialUnavailableException>();
147147

148148
for (var i = 0; i < sources.Length && sources[i] != null; i++)
149149
{
@@ -155,18 +155,13 @@ private static async ValueTask<AccessToken> GetTokenFromCredentialAsync(TokenCre
155155

156156
return (token, sources[i]);
157157
}
158-
catch (AuthenticationFailedException e)
158+
catch (CredentialUnavailableException e)
159159
{
160160
exceptions.Add(e);
161161
}
162-
catch (Exception e) when (!(e is OperationCanceledException))
163-
{
164-
exceptions.Add(e);
165-
throw AuthenticationFailedException.CreateAggregateException(UnhandledExceptionMessage + e.Message, exceptions);
166-
}
167162
}
168163

169-
throw AuthenticationFailedException.CreateAggregateException(DefaultExceptionMessage, exceptions);
164+
throw CredentialUnavailableException.CreateAggregateException(DefaultExceptionMessage, exceptions);
170165
}
171166

172167
private static TokenCredential[] GetDefaultAzureCredentialChain(DefaultAzureCredentialFactory factory, DefaultAzureCredentialOptions options)

‎sdk/identity/Azure.Identity/src/VisualStudioCodeCredential.cs‎

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -69,12 +69,7 @@ private async ValueTask<AccessToken> GetTokenImplAsync(TokenRequestContext reque
6969
}
7070

7171
var cloudInstance = GetAzureCloudInstance(environmentName);
72-
var storedCredentials = _vscAdapter.GetCredentials(CredentialsSection, environmentName);
73-
74-
if (!IsRefreshTokenString(storedCredentials))
75-
{
76-
throw new CredentialUnavailableException("Need to re-authenticate user in VSCode Azure Account.");
77-
}
72+
string storedCredentials = GetStoredCredentials(environmentName);
7873

7974
var result = await _client.AcquireTokenByRefreshToken(requestContext.Scopes, storedCredentials, cloudInstance, tenant, async, cancellationToken).ConfigureAwait(false);
8075
return scope.Succeeded(new AccessToken(result.AccessToken, result.ExpiresOn));
@@ -89,6 +84,24 @@ private async ValueTask<AccessToken> GetTokenImplAsync(TokenRequestContext reque
8984
}
9085
}
9186

87+
private string GetStoredCredentials(string environmentName)
88+
{
89+
try
90+
{
91+
var storedCredentials = _vscAdapter.GetCredentials(CredentialsSection, environmentName);
92+
if (!IsRefreshTokenString(storedCredentials))
93+
{
94+
throw new CredentialUnavailableException("Need to re-authenticate user in VSCode Azure Account.");
95+
}
96+
97+
return storedCredentials;
98+
}
99+
catch (InvalidOperationException ex)
100+
{
101+
throw new CredentialUnavailableException("Stored credentials not found. Need to authenticate user in VSCode Azure Account.", ex);
102+
}
103+
}
104+
92105
private static bool IsRefreshTokenString(string str)
93106
{
94107
for (var index = 0; index < str.Length; index++)

‎sdk/identity/Azure.Identity/src/VisualStudioCredential.cs‎

Lines changed: 26 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -125,9 +125,9 @@ private async Task<AccessToken> RunProcessesAsync(List<ProcessStartInfo> process
125125
{
126126
exceptions.Add(new CredentialUnavailableException($"Process \"{processStartInfo.FileName}\" has non-json output: {output}.", exception));
127127
}
128-
catch (Exception exception)
128+
catch (Exception exception) when (!(exception is OperationCanceledException))
129129
{
130-
exceptions.Add(exception);
130+
exceptions.Add(new CredentialUnavailableException($"Process \"{processStartInfo.FileName}\" has failed with unexpected error: {exception.Message}.", exception));
131131
}
132132
}
133133

@@ -192,24 +192,35 @@ private VisualStudioTokenProvider[] GetTokenProviders(string tokenProviderPath)
192192
{
193193
var content = GetTokenProviderContent(tokenProviderPath);
194194

195-
using JsonDocument document = JsonDocument.Parse(content);
195+
try
196+
{
197+
using JsonDocument document = JsonDocument.Parse(content);
196198

197-
JsonElement providersElement = document.RootElement.GetProperty("TokenProviders");
199+
JsonElement providersElement = document.RootElement.GetProperty("TokenProviders");
198200

199-
var providers = new VisualStudioTokenProvider[providersElement.GetArrayLength()];
200-
for (int i = 0; i < providers.Length; i++)
201-
{
202-
JsonElement providerElement = providersElement[i];
201+
var providers = new VisualStudioTokenProvider[providersElement.GetArrayLength()];
202+
for (int i = 0; i < providers.Length; i++)
203+
{
204+
JsonElement providerElement = providersElement[i];
203205

204-
var path = providerElement.GetProperty("Path").GetString();
205-
var preference = providerElement.GetProperty("Preference").GetInt32();
206-
var arguments = GetStringArrayPropertyValue(providerElement, "Arguments");
206+
var path = providerElement.GetProperty("Path").GetString();
207+
var preference = providerElement.GetProperty("Preference").GetInt32();
208+
var arguments = GetStringArrayPropertyValue(providerElement, "Arguments");
207209

208-
providers[i] = new VisualStudioTokenProvider(path, arguments, preference);
209-
}
210+
providers[i] = new VisualStudioTokenProvider(path, arguments, preference);
211+
}
210212

211-
Array.Sort(providers);
212-
return providers;
213+
Array.Sort(providers);
214+
return providers;
215+
}
216+
catch (JsonException exception)
217+
{
218+
throw new CredentialUnavailableException($"File found at \"{tokenProviderPath}\" isn't a valid JSON file", exception);
219+
}
220+
catch (Exception exception)
221+
{
222+
throw new CredentialUnavailableException($"JSON file found at \"{tokenProviderPath}\" has invalid schema.", exception);
223+
}
213224
}
214225

215226
private string GetTokenProviderContent(string tokenProviderPath)

‎sdk/identity/Azure.Identity/tests/AzureCliCredentialTests.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ public void AuthenticateWithCliCredential_InvalidJsonOutput([Values("", "{}", "{
4747
{
4848
var testProcess = new TestProcess { Output = jsonContent };
4949
AzureCliCredential credential = InstrumentClient(new AzureCliCredential(CredentialPipeline.GetInstance(null), new TestProcessService(testProcess)));
50-
Assert.CatchAsync<AuthenticationFailedException>(async () => await credential.GetTokenAsync(new TokenRequestContext(MockScopes.Default)));
50+
Assert.ThrowsAsync<AuthenticationFailedException>(async () => await credential.GetTokenAsync(new TokenRequestContext(MockScopes.Default)));
5151
}
5252

5353
[Test]

‎sdk/identity/Azure.Identity/tests/ChainedTokenCredentialLiveTests.cs‎

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ public async Task ChainedTokenCredential_UseVisualStudioCredential()
5757
}
5858

5959
[Test]
60-
[RunOnlyOnPlatforms(Windows = true, OSX = true, ContainerNames = new[] { "ubuntu_netcore2_keyring" })]
60+
[RunOnlyOnPlatforms(Windows = true, OSX = true, ContainerNames = new[] { "ubuntu_netcore_keyring" })]
6161
public async Task ChainedTokenCredential_UseVisualStudioCodeCredential()
6262
{
6363
var cloudName = Guid.NewGuid().ToString();
@@ -89,7 +89,7 @@ public async Task ChainedTokenCredential_UseVisualStudioCodeCredential()
8989
}
9090

9191
[Test]
92-
[RunOnlyOnPlatforms(Windows = true, OSX = true, ContainerNames = new[] { "ubuntu_netcore2_keyring" })]
92+
[RunOnlyOnPlatforms(Windows = true, OSX = true, ContainerNames = new[] { "ubuntu_netcore_keyring" })]
9393
public async Task ChainedTokenCredential_UseVisualStudioCodeCredential_ParallelCalls()
9494
{
9595
var cloudName = Guid.NewGuid().ToString();
@@ -209,7 +209,35 @@ public void ChainedTokenCredential_AllCredentialsHaveFailed_CredentialUnavailabl
209209
}
210210

211211
[Test]
212-
public void ChainedTokenCredential_AllCredentialsHaveFailed_AuthenticationFailedException()
212+
[NonParallelizable]
213+
public void ChainedTokenCredential_AllCredentialsHaveFailed_FirstAuthenticationFailedException()
214+
{
215+
using var endpoint = new TestEnvVar("MSI_ENDPOINT", "abc");
216+
217+
var vscAdapter = new TestVscAdapter(ExpectedServiceName, "Azure", null);
218+
var fileSystem = new TestFileSystemService();
219+
var processService = new TestProcessService(new TestProcess {Error = "Error"});
220+
221+
var miCredential = new ManagedIdentityCredential(EnvironmentVariables.ClientId);
222+
var vsCredential = new VisualStudioCredential(default, default, fileSystem, processService);
223+
var vscCredential = new VisualStudioCodeCredential(new VisualStudioCodeCredentialOptions { TenantId = TestEnvironment.TestTenantId }, default, default, fileSystem, vscAdapter);
224+
var azureCliCredential = new AzureCliCredential(CredentialPipeline.GetInstance(null), processService);
225+
226+
var credential = InstrumentClient(new ChainedTokenCredential(miCredential, vsCredential, vscCredential, azureCliCredential));
227+
228+
List<ClientDiagnosticListener.ProducedDiagnosticScope> scopes;
229+
using (ClientDiagnosticListener diagnosticListener = new ClientDiagnosticListener(s => s.StartsWith("Azure.Identity")))
230+
{
231+
Assert.CatchAsync<AuthenticationFailedException>(async () => await credential.GetTokenAsync(new TokenRequestContext(new[] {"https://vault.azure.net/.default"}), CancellationToken.None));
232+
scopes = diagnosticListener.Scopes;
233+
}
234+
235+
Assert.AreEqual(1, scopes.Count);
236+
Assert.AreEqual($"{nameof(ManagedIdentityCredential)}.{nameof(ManagedIdentityCredential.GetToken)}", scopes[0].Name);
237+
}
238+
239+
[Test]
240+
public void ChainedTokenCredential_AllCredentialsHaveFailed_LastAuthenticationFailedException()
213241
{
214242
var vscAdapter = new TestVscAdapter(ExpectedServiceName, "Azure", null);
215243
var fileSystem = new TestFileSystemService();

0 commit comments

Comments
 (0)