Skip to content

Commit adb5d13

Browse files
authored
Fix instance discovery bug 5546 (#5549)
* Fix for #5546 and a test for #5545 * Fix * err * type * ex type
1 parent 4c04a58 commit adb5d13

File tree

4 files changed

+148
-52
lines changed

4 files changed

+148
-52
lines changed

src/client/Microsoft.Identity.Client/Instance/Discovery/InstanceDiscoveryManager.cs

Lines changed: 15 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
using System;
55
using System.Collections.Generic;
66
using System.Linq;
7+
using System.Net.Http;
78
using System.Threading.Tasks;
89
using Microsoft.Identity.Client.Core;
910
using Microsoft.Identity.Client.Http;
@@ -196,28 +197,26 @@ private async Task<InstanceDiscoveryMetadataEntry> FetchNetworkMetadataOrFallbac
196197
{
197198
return await _networkMetadataProvider.GetMetadataAsync(authorityUri, requestContext).ConfigureAwait(false);
198199
}
199-
catch (MsalServiceException ex)
200+
catch (MsalServiceException ex) when (ex.ErrorCode == MsalError.InvalidInstance)
200201
{
201202
if (!requestContext.ServiceBundle.Config.Authority.AuthorityInfo.ValidateAuthority)
202203
{
203-
requestContext.Logger.Info("[Instance Discovery] Skipping Instance discovery as validate authority is set to false. ");
204+
requestContext.Logger.Info($"[Instance Discovery] Skipping Instance discovery as validate authority is set to false. {ex}");
204205
return CreateEntryForSingleAuthority(authorityUri);
205206
}
206207

207-
// Validate Authority exception
208-
if (ex.ErrorCode == MsalError.InvalidInstance)
209-
{
210-
requestContext.Logger.Error("[Instance Discovery] Instance discovery failed - invalid instance!");
211-
throw;
212-
}
213-
214-
string message =
215-
"[Instance Discovery] Instance Discovery failed. Potential cause: no network connection or discovery endpoint is busy. See exception below. MSAL will continue without network instance metadata. ";
216-
217-
requestContext.Logger.WarningPii(message + " Authority: " + authorityUri, message);
218-
requestContext.Logger.WarningPii(ex);
219-
220-
return _knownMetadataProvider.GetMetadata(authorityUri.Host, Enumerable.Empty<string>(), requestContext.Logger);
208+
requestContext.Logger.Error($"[Instance Discovery] Instance discovery failed - invalid instance! ");
209+
throw;
210+
}
211+
catch (Exception e)
212+
{
213+
requestContext.Logger.Warning(
214+
$"[Instance Discovery] Instance Discovery failed. MSAL will continue without network instance metadata. \n\r" +
215+
$" Exception: {e} ");
216+
217+
return
218+
_knownMetadataProvider.GetMetadata(authorityUri.Host, Enumerable.Empty<string>(), requestContext.Logger)
219+
?? CreateEntryForSingleAuthority(authorityUri);
221220
}
222221
}
223222

tests/Microsoft.Identity.Test.Common/Core/Mocks/MockHttpManager.cs

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,7 @@ internal sealed class MockHttpManager : IHttpManager,
3131
public MockHttpManager(
3232
bool disableInternalRetries = false,
3333
string testName = null,
34-
Func<MockHttpMessageHandler> messageHandlerFunc = null,
35-
Func<HttpClient> validateServerCertificateCallback = null,
34+
Func<MockHttpMessageHandler> messageHandlerFunc = null,
3635
bool invokeNonMtlsHttpManagerFactory = false)
3736
{
3837
_httpManager = invokeNonMtlsHttpManagerFactory
@@ -160,7 +159,6 @@ protected HttpClient GetHttpClientInternal(X509Certificate2 mtlsBindingCert)
160159
{
161160
messageHandler.ClientCertificates.Add(mtlsBindingCert);
162161
}
163-
164162
var httpClient = new HttpClient(messageHandler)
165163
{
166164
MaxResponseContentBufferSize = HttpClientConfig.MaxResponseContentBufferSizeInBytes

tests/Microsoft.Identity.Test.Unit/PublicApiTests/ConfidentialClientApplicationTests.cs

Lines changed: 1 addition & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -151,39 +151,7 @@ public async Task ConfidentialClientUsingSecretNoCacheProvidedTestAsync()
151151
appCacheAccess.AssertAccessCounts(1, 1);
152152
userCacheAccess.AssertAccessCounts(0, 0);
153153
}
154-
}
155-
156-
[TestMethod]
157-
public async Task ConfidentialClientUsingSecretNoInstanceDiscoveryTestAsync()
158-
{
159-
using (var httpManager = new MockHttpManager())
160-
{
161-
162-
var app = ConfidentialClientApplicationBuilder.Create(TestConstants.ClientId)
163-
.WithAuthority(new Uri(ClientApplicationBase.DefaultAuthority), true)
164-
.WithRedirectUri(TestConstants.RedirectUri)
165-
.WithClientSecret(TestConstants.ClientSecret)
166-
.WithHttpManager(httpManager)
167-
.WithInstanceDiscovery(false)
168-
.BuildConcrete();
169-
170-
var appCacheAccess = app.AppTokenCache.RecordAccess();
171-
var userCacheAccess = app.UserTokenCache.RecordAccess();
172-
173-
httpManager.AddMockHandlerSuccessfulClientCredentialTokenResponseMessage();
174-
175-
var result = await app.AcquireTokenForClient(TestConstants.s_scope.ToArray()).ExecuteAsync(CancellationToken.None).ConfigureAwait(false);
176-
Assert.IsNotNull(result);
177-
Assert.IsNotNull("header.payload.signature", result.AccessToken);
178-
Assert.AreEqual(TestConstants.s_scope.AsSingleString(), result.Scopes.AsSingleString());
179-
180-
Assert.IsNotNull(app.UserTokenCache);
181-
Assert.IsNotNull(app.AppTokenCache);
182-
183-
appCacheAccess.AssertAccessCounts(1, 1);
184-
userCacheAccess.AssertAccessCounts(0, 0);
185-
}
186-
}
154+
}
187155

188156
[TestMethod]
189157
[TestCategory(TestCategories.Regression)]
Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
// Copyright (c) Microsoft Corporation. All rights reserved.
2+
// Licensed under the MIT License.
3+
4+
using System;
5+
using System.Collections.Generic;
6+
using System.Globalization;
7+
using System.IdentityModel.Tokens.Jwt;
8+
using System.Linq;
9+
using System.Net.Http;
10+
using System.Security.Cryptography.X509Certificates;
11+
using System.Threading;
12+
using System.Threading.Tasks;
13+
using Microsoft.Identity.Client;
14+
using Microsoft.Identity.Client.RP;
15+
using Microsoft.Identity.Client.Cache;
16+
using Microsoft.Identity.Client.Internal;
17+
using Microsoft.Identity.Client.Internal.ClientCredential;
18+
using Microsoft.Identity.Client.OAuth2;
19+
using Microsoft.Identity.Client.PlatformsCommon.Shared;
20+
using Microsoft.Identity.Client.Utils;
21+
using Microsoft.Identity.Test.Common;
22+
using Microsoft.Identity.Test.Common.Core.Helpers;
23+
using Microsoft.Identity.Test.Common.Core.Mocks;
24+
using Microsoft.VisualStudio.TestTools.UnitTesting;
25+
using NSubstitute;
26+
using Microsoft.Identity.Client.Extensibility;
27+
using System.Net;
28+
using System.Security.Cryptography;
29+
using System.Text;
30+
31+
namespace Microsoft.Identity.Test.Unit.PublicApiTests
32+
{
33+
[TestClass]
34+
public class InstanceDiscoveryTests : TestBase
35+
{
36+
[TestMethod]
37+
public async Task ConfidentialClientUsingSecretNoInstanceDiscoveryTestAsync()
38+
{
39+
using (var httpManager = new MockHttpManager())
40+
{
41+
42+
var app = ConfidentialClientApplicationBuilder.Create(TestConstants.ClientId)
43+
.WithAuthority(new Uri(ClientApplicationBase.DefaultAuthority), true)
44+
.WithRedirectUri(TestConstants.RedirectUri)
45+
.WithClientSecret(TestConstants.ClientSecret)
46+
.WithHttpManager(httpManager)
47+
.WithInstanceDiscovery(false)
48+
.BuildConcrete();
49+
50+
var appCacheAccess = app.AppTokenCache.RecordAccess();
51+
var userCacheAccess = app.UserTokenCache.RecordAccess();
52+
53+
httpManager.AddMockHandlerSuccessfulClientCredentialTokenResponseMessage();
54+
55+
var result = await app.AcquireTokenForClient(TestConstants.s_scope.ToArray()).ExecuteAsync(CancellationToken.None).ConfigureAwait(false);
56+
Assert.IsNotNull(result);
57+
Assert.IsNotNull("header.payload.signature", result.AccessToken);
58+
Assert.AreEqual(TestConstants.s_scope.AsSingleString(), result.Scopes.AsSingleString());
59+
60+
Assert.IsNotNull(app.UserTokenCache);
61+
Assert.IsNotNull(app.AppTokenCache);
62+
63+
appCacheAccess.AssertAccessCounts(1, 1);
64+
userCacheAccess.AssertAccessCounts(0, 0);
65+
}
66+
}
67+
68+
[TestMethod]
69+
[WorkItem(5545)] // https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/issues/5545
70+
public async Task NoInstanceDiscovery_AirgappedCould_TestAsync()
71+
{
72+
using (var httpManager = new MockHttpManager(disableInternalRetries: true))
73+
using (new EnvVariableContext())
74+
{
75+
Environment.SetEnvironmentVariable("REGION_NAME", TestConstants.Region);
76+
77+
// Instance discovery explicitly disabled
78+
var app = ConfidentialClientApplicationBuilder.Create(TestConstants.ClientId)
79+
.WithAuthority(TestConstants.AuthorityNotKnownTenanted)
80+
.WithClientSecret(TestConstants.ClientSecret)
81+
.WithHttpManager(httpManager)
82+
.WithAzureRegion(ConfidentialClientApplication.AttemptRegionDiscovery)
83+
.WithInstanceDiscovery(false)
84+
.Build();
85+
86+
// Direct token request (no instance discovery mock!)
87+
httpManager.AddMockHandlerSuccessfulClientCredentialTokenResponseMessage();
88+
89+
// Act
90+
var result = await app.AcquireTokenForClient(TestConstants.s_scope.ToArray())
91+
.ExecuteAsync(CancellationToken.None)
92+
.ConfigureAwait(false);
93+
94+
// Assert happens when httpManager disposes and checks for unconsumed handlers
95+
}
96+
}
97+
98+
[TestMethod]
99+
[WorkItem(5546)] // https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/issues/5546
100+
public async Task HttpErrorsInDiscoveryShouldBeIgnored_AirgappedCould_TestAsync()
101+
{
102+
using (var httpManager = new MockHttpManager(disableInternalRetries: true))
103+
using (new EnvVariableContext())
104+
{
105+
Environment.SetEnvironmentVariable("REGION_NAME", TestConstants.Region);
106+
107+
// Instance discovery explicitly disabled
108+
var app = ConfidentialClientApplicationBuilder.Create(TestConstants.ClientId)
109+
.WithAuthority(TestConstants.AuthorityNotKnownTenanted)
110+
.WithClientSecret(TestConstants.ClientSecret)
111+
.WithHttpManager(httpManager)
112+
.WithAzureRegion(ConfidentialClientApplication.AttemptRegionDiscovery)
113+
.Build();
114+
115+
httpManager.AddMockHandler(
116+
new MockHttpMessageHandler()
117+
{
118+
ExceptionToThrow = new HttpRequestException("Simulated network error")
119+
});
120+
httpManager.AddMockHandlerSuccessfulClientCredentialTokenResponseMessage();
121+
122+
// Act
123+
var result = await app.AcquireTokenForClient(TestConstants.s_scope.ToArray())
124+
.ExecuteAsync(CancellationToken.None)
125+
.ConfigureAwait(false);
126+
127+
// Assert happens when httpManager disposes and checks for unconsumed handlers
128+
}
129+
}
130+
}
131+
}

0 commit comments

Comments
 (0)