Skip to content

Commit 79de40f

Browse files
trwalketrwalke
trwalke
and
trwalke
authored
Updating exceptions for 404 to include authority (#5095)
* Updating exceptions for 404 to includ authority * Refactoring * Adding regional test * Clean up * Adding token endpoint * Adding additional test case * Test fix --------- Co-authored-by: trwalke <[email protected]>
1 parent e6dd08c commit 79de40f

File tree

6 files changed

+116
-10
lines changed

6 files changed

+116
-10
lines changed

‎src/client/Microsoft.Identity.Client/MsalServiceExceptionFactory.cs‎

Lines changed: 32 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33

44
using System;
55
using System.Collections.Generic;
6+
using System.Net;
7+
using System.Text;
68
using Microsoft.Identity.Client.Http;
79
using Microsoft.Identity.Client.Internal;
810
using Microsoft.Identity.Client.Internal.Broker;
@@ -23,7 +25,8 @@ internal static MsalServiceException FromHttpResponse(
2325
string errorCode,
2426
string errorMessage,
2527
HttpResponse httpResponse,
26-
Exception innerException = null)
28+
Exception innerException = null,
29+
RequestContext context = null)
2730
{
2831
MsalServiceException ex = null;
2932
OAuth2ResponseBase oAuth2Response = JsonHelper.TryToDeserializeFromJson<OAuth2ResponseBase>(httpResponse?.Body);
@@ -39,7 +42,7 @@ internal static MsalServiceException FromHttpResponse(
3942
else
4043
{
4144
errorMessageToUse = errorMessage;
42-
}
45+
}
4346

4447
if (oAuth2Response.Claims == null)
4548
{
@@ -61,7 +64,7 @@ internal static MsalServiceException FromHttpResponse(
6164
innerException);
6265
}
6366

64-
ex ??= new MsalServiceException(errorCode, errorMessage, innerException);
67+
ex ??= new MsalServiceException(errorCode, GetErrorMessage(errorMessage, httpResponse, context), innerException);
6568

6669
SetHttpExceptionData(ex, httpResponse);
6770

@@ -73,6 +76,32 @@ internal static MsalServiceException FromHttpResponse(
7376
return ex;
7477
}
7578

79+
private static string GetErrorMessage(string errorMessage, HttpResponse httpResponse, RequestContext context)
80+
{
81+
// Using StringBuilder for more efficient string concatenation
82+
var sb = new StringBuilder(errorMessage);
83+
84+
if (httpResponse.StatusCode == HttpStatusCode.NotFound && context != null)
85+
{
86+
sb.Append("\nAuthority used: ")
87+
.Append(context.ServiceBundle.Config.Authority?.AuthorityInfo?.CanonicalAuthority?.AbsoluteUri?.Split('?')[0]);
88+
89+
if (context.ApiEvent != null && !context.ApiEvent.TokenEndpoint.IsNullOrEmpty())
90+
{
91+
sb.Append("\nToken Endpoint: ")
92+
.Append(context.ApiEvent?.TokenEndpoint);
93+
}
94+
95+
if (!context.ServiceBundle.Config.AzureRegion.IsNullOrEmpty())
96+
{
97+
sb.Append("\nRegion Used: ")
98+
.Append(context.ServiceBundle.Config.AzureRegion);
99+
}
100+
}
101+
102+
return sb.ToString();
103+
}
104+
76105
private static bool IsThrottled(OAuth2ResponseBase oAuth2Response)
77106
{
78107
return oAuth2Response.ErrorDescription != null &&

‎src/client/Microsoft.Identity.Client/OAuth2/OAuth2Client.cs‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -256,24 +256,28 @@ private static void ThrowServerException(HttpResponse response, RequestContext r
256256
exceptionToThrow = MsalServiceExceptionFactory.FromHttpResponse(
257257
MsalError.NonParsableOAuthError,
258258
MsalErrorMessage.NonParsableOAuthError,
259-
response);
259+
response,
260+
null,
261+
requestContext);
260262
}
261263
catch (Exception ex)
262264
{
263-
264265
exceptionToThrow = MsalServiceExceptionFactory.FromHttpResponse(
265266
MsalError.UnknownError,
266267
response.Body,
267268
response,
268-
ex);
269+
ex,
270+
requestContext);
269271
}
270272

271273
exceptionToThrow ??= MsalServiceExceptionFactory.FromHttpResponse(
272274
response.StatusCode == HttpStatusCode.NotFound
273275
? MsalError.HttpStatusNotFound
274276
: MsalError.HttpStatusCodeNotOk,
275277
httpErrorCodeMessage,
276-
response);
278+
response,
279+
null,
280+
requestContext);
277281

278282
if (shouldLogAsError)
279283
{

‎tests/Microsoft.Identity.Test.Common/TestConstants.cs‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -230,7 +230,6 @@ public static IDictionary<string, string> ExtraQueryParameters
230230
}
231231
}
232232

233-
234233
public const string MsalCCAKeyVaultUri = "https://buildautomation.vault.azure.net/secrets/AzureADIdentityDivisionTestAgentSecret/";
235234
public const string MsalCCAKeyVaultSecretName = "MSIDLAB4-IDLABS-APP-AzureADMyOrg-CC";
236235
public const string MsalOBOKeyVaultUri = "https://buildautomation.vault.azure.net/secrets/IdentityDivisionDotNetOBOServiceSecret/";

‎tests/Microsoft.Identity.Test.Integration.netcore/HeadlessTests/ClientCredentialsTests.NetFwk.cs‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,6 @@ public async Task RefreshOnIsEnabled(bool useRegional)
8787
result.AuthenticationResultMetadata.RegionDetails.RegionOutcome);
8888
}
8989

90-
9190
[DataTestMethod]
9291
[DataRow(Cloud.Public, TargetFrameworks.NetFx | TargetFrameworks.NetCore)]
9392
[DataRow(Cloud.Adfs, TargetFrameworks.NetFx | TargetFrameworks.NetCore)]

‎tests/Microsoft.Identity.Test.Unit/ExceptionTests/MsalExceptionTests.cs‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -509,6 +509,82 @@ await app.AcquireTokenForClient(TestConstants.s_scope)
509509
}
510510
}
511511

512+
[TestMethod]
513+
public async Task AuthorityInNotFoundExceptions()
514+
{
515+
using (var harness = CreateTestHarness())
516+
{
517+
harness.HttpManager.AddMockHandler(MockHelpers.CreateInstanceDiscoveryMockHandler(TestConstants.AuthorityCommonTenant + TestConstants.DiscoveryEndPoint));
518+
harness.HttpManager.AddMockHandlerContentNotFound(HttpMethod.Post);
519+
520+
ConfidentialClientApplication app = null;
521+
var certificate = new X509Certificate2(
522+
ResourceHelper.GetTestResourceRelativePath("RSATestCertDotNet.pfx"));
523+
524+
app = ConfidentialClientApplicationBuilder
525+
.Create(TestConstants.ClientId)
526+
.WithAuthority(new Uri(ClientApplicationBase.DefaultAuthority), true)
527+
.WithRedirectUri(TestConstants.RedirectUri)
528+
.WithHttpManager(harness.HttpManager)
529+
.WithCertificate(certificate)
530+
.BuildConcrete();
531+
532+
var ex = await Assert.ThrowsExceptionAsync<MsalServiceException>(async () =>
533+
{
534+
await app.AcquireTokenForClient(TestConstants.s_scope)
535+
.ExecuteAsync(CancellationToken.None)
536+
.ConfigureAwait(false);
537+
}).ConfigureAwait(false);
538+
539+
Assert.IsTrue(ex.Message.Contains("Authority used: https://login.microsoftonline.com/common/"));
540+
Assert.IsTrue(ex.Message.Contains("Token Endpoint: https://login.microsoftonline.com/common/oauth2/v2.0/token"));
541+
542+
//Validate that the authority is not appended with extra query parameters
543+
//Validate that the region is also captured
544+
Environment.SetEnvironmentVariable("REGION_NAME", TestConstants.Region);
545+
546+
harness.HttpManager.AddMockHandler(MockHelpers.CreateInstanceDiscoveryMockHandler(TestConstants.AuthorityCommonTenant + TestConstants.DiscoveryEndPoint));
547+
harness.HttpManager.AddMockHandlerContentNotFound(HttpMethod.Post);
548+
549+
app = ConfidentialClientApplicationBuilder
550+
.Create(TestConstants.ClientId)
551+
.WithAuthority(new Uri(TestConstants.AuthorityNotKnownTenanted + "extra=qp"), true)
552+
.WithAzureRegion(TestConstants.Region)
553+
.WithRedirectUri(TestConstants.RedirectUri)
554+
.WithHttpManager(harness.HttpManager)
555+
.WithCertificate(certificate)
556+
.BuildConcrete();
557+
558+
ex = await AssertException.TaskThrowsAsync<MsalServiceException>(async () =>
559+
{
560+
await app.AcquireTokenForClient(TestConstants.s_scope)
561+
.ExecuteAsync(CancellationToken.None)
562+
.ConfigureAwait(false);
563+
}).ConfigureAwait(false);
564+
565+
Assert.IsTrue(ex.Message.Contains("Authority used: https://sts.access.edu/my-utid/"));
566+
Assert.IsTrue(ex.Message.Contains("Token Endpoint: https://centralus.sts.access.edu/my-utid/oauth2/v2.0/token"));
567+
Assert.IsTrue(ex.Message.Contains($"Region Used: {TestConstants.Region}"));
568+
569+
//harness.HttpManager.AddMockHandler(MockHelpers.CreateInstanceDiscoveryMockHandler(TestConstants.AuthorityCommonTenant + TestConstants.DiscoveryEndPoint));
570+
harness.HttpManager.AddRequestTimeoutResponseMessageMockHandler(HttpMethod.Post);
571+
harness.HttpManager.AddRequestTimeoutResponseMessageMockHandler(HttpMethod.Post);
572+
573+
//Ensure non 404 error codes do not trigger message
574+
ex = await AssertException.TaskThrowsAsync<MsalServiceException>(async () =>
575+
{
576+
await app.AcquireTokenForClient(TestConstants.s_scope)
577+
.WithForceRefresh(true)
578+
.ExecuteAsync(CancellationToken.None)
579+
.ConfigureAwait(false);
580+
}).ConfigureAwait(false);
581+
582+
Assert.IsFalse(ex.Message.Contains("Authority used:"));
583+
Assert.IsFalse(ex.Message.Contains("Token Endpoint:"));
584+
Assert.IsFalse(ex.Message.Contains($"Region Used:"));
585+
}
586+
}
587+
512588
private void AssertPropertyHasPublicGetAndSet(Type t, string propertyName)
513589
{
514590
var prop = t.GetProperty(propertyName);

‎tests/Microsoft.Identity.Test.Unit/RequestsTests/IntegratedWindowsAuthUsernamePasswordTests.cs‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -415,7 +415,6 @@ public async Task AcquireTokenByIntegratedWindowsAuthInvalidClientTestAsync()
415415
}
416416
}
417417

418-
419418
[TestMethod]
420419
[DeploymentItem(@"Resources\TestMex.xml")]
421420
[DeploymentItem(@"Resources\WsTrustResponse.xml")]

0 commit comments

Comments
 (0)