Fix instance discovery for multi-cloud https://github.com/AzureAD/mic… (#1049)

bgavrilMS · web-flow · commit 01c977b2499b · 2019-04-08T19:06:12.000+01:00
* Fix instance discovery for multi-cloud #1048 * Add more pre-validated hosts
diff --git a/src/Microsoft.Identity.Client/Instance/AadAuthority.cs b/src/Microsoft.Identity.Client/Instance/AadAuthority.cs
@@ -31,31 +31,31 @@
 using System.Linq;
 using System.Threading.Tasks;
 using Microsoft.Identity.Client.Core;
+using Microsoft.Identity.Client.Utils;
 
 namespace Microsoft.Identity.Client.Instance
 {
     internal class AadAuthority : Authority
     {
-        internal static readonly HashSet<string> TrustedHostList = new HashSet<string>()
+        public const string DefaultTrustedHost = "login.microsoftonline.com";
+        public const string AADCanonicalAuthorityTemplate = "https://{0}/{1}/";
+
+        private static readonly HashSet<string> s_trustedHostList = new HashSet<string>()
         {
-            "login.windows.net", // Microsoft Azure Worldwide - Used in validation scenarios where host is not this list
-            "login.chinacloudapi.cn", // Microsoft Azure China
+            "login.partner.microsoftonline.cn", // Microsoft Azure China
+            "login.chinacloudapi.cn",
             "login.microsoftonline.de", // Microsoft Azure Blackforest
             "login-us.microsoftonline.com", // Microsoft Azure US Government - Legacy
             "login.microsoftonline.us", // Microsoft Azure US Government
-            "login.microsoftonline.com", // Microsoft Azure Worldwide
-            "login.cloudgovapi.us" // Microsoft Azure US Government
+             DefaultTrustedHost, // Microsoft Azure Worldwide
+            "login.windows.net"            
         };
 
         internal static bool IsInTrustedHostList(string host)
         {
-            return !string.IsNullOrEmpty(
-                TrustedHostList.FirstOrDefault(a => string.Compare(host, a, StringComparison.OrdinalIgnoreCase) == 0));
+            return s_trustedHostList.ContainsOrdinalIgnoreCase(host);
         }
 
-        public const string DefaultTrustedHost = "login.microsoftonline.com";
-        public const string AADCanonicalAuthorityTemplate = "https://{0}/{1}/";
-
         internal AadAuthority(
             IServiceBundle serviceBundle,
             AuthorityInfo authorityInfo) : base(serviceBundle, authorityInfo)
diff --git a/src/Microsoft.Identity.Client/Instance/AadInstanceDiscovery.cs b/src/Microsoft.Identity.Client/Instance/AadInstanceDiscovery.cs
@@ -133,7 +133,6 @@ private void CacheInstanceDiscoveryMetadata(string host, InstanceDiscoveryRespon
         {
             foreach (var entry in instanceDiscoveryResponse.Metadata ?? Enumerable.Empty<InstanceDiscoveryMetadataEntry>())
             {
-                entry.TenantDiscoveryEndpoint = instanceDiscoveryResponse.TenantDiscoveryEndpoint;
                 foreach (string aliasedAuthority in entry.Aliases ?? Enumerable.Empty<string>())
                 {
                     TryAddValue(aliasedAuthority, entry);
@@ -146,9 +145,8 @@ private void CacheInstanceDiscoveryMetadata(string host, InstanceDiscoveryRespon
                 {
                     PreferredNetwork = host,
                     PreferredCache = host,
-                    TenantDiscoveryEndpoint = instanceDiscoveryResponse.TenantDiscoveryEndpoint,
                     Aliases = null
                 });
         }
     }
-}
+}
diff --git a/src/Microsoft.Identity.Client/Instance/AadOpenIdConfigurationEndpointManager.cs b/src/Microsoft.Identity.Client/Instance/AadOpenIdConfigurationEndpointManager.cs
@@ -41,19 +41,17 @@ public AadOpenIdConfigurationEndpointManager(IServiceBundle serviceBundle)
         }
 
         /// <inheritdoc />
-        public async Task<string> GetOpenIdConfigurationEndpointAsync(
+        public async Task<string> ValidateAuthorityAndGetOpenIdDiscoveryEndpointAsync(
             AuthorityInfo authorityInfo,
             string userPrincipalName,
             RequestContext requestContext)
         {
             var authorityUri = new Uri(authorityInfo.CanonicalAuthority);
             if (authorityInfo.ValidateAuthority && !AadAuthority.IsInTrustedHostList(authorityUri.Host))
             {
-                var discoveryResponse = await _serviceBundle.AadInstanceDiscovery.GetMetadataEntryAsync(
+               await _serviceBundle.AadInstanceDiscovery.GetMetadataEntryAsync(
                                             authorityUri,
                                             requestContext).ConfigureAwait(false);
-
-                return discoveryResponse.TenantDiscoveryEndpoint;
             }
 
             return authorityInfo.CanonicalAuthority + Constants.OpenIdConfigurationEndpoint;
diff --git a/src/Microsoft.Identity.Client/Instance/AdfsOpenIdConfigurationEndpointManager.cs b/src/Microsoft.Identity.Client/Instance/AdfsOpenIdConfigurationEndpointManager.cs
@@ -45,7 +45,7 @@ public AdfsOpenIdConfigurationEndpointManager(IServiceBundle serviceBundle)
             _serviceBundle = serviceBundle;
         }
 
-        public async Task<string> GetOpenIdConfigurationEndpointAsync(
+        public async Task<string> ValidateAuthorityAndGetOpenIdDiscoveryEndpointAsync(
             AuthorityInfo authorityInfo,
             string userPrincipalName,
             RequestContext requestContext)
diff --git a/src/Microsoft.Identity.Client/Instance/AuthorityEndpointResolutionManager.cs b/src/Microsoft.Identity.Client/Instance/AuthorityEndpointResolutionManager.cs
@@ -81,7 +81,7 @@ public async Task<AuthorityEndpoints> ResolveEndpointsAsync(
 
             var endpointManager = OpenIdConfigurationEndpointManagerFactory.Create(authorityInfo, _serviceBundle);
 
-            string openIdConfigurationEndpoint = await endpointManager.GetOpenIdConfigurationEndpointAsync(
+            string openIdConfigurationEndpoint = await endpointManager.ValidateAuthorityAndGetOpenIdDiscoveryEndpointAsync(
                                                      authorityInfo,
                                                      userPrincipalName,
                                                      requestContext).ConfigureAwait(false);
diff --git a/src/Microsoft.Identity.Client/Instance/B2COpenIdConfigurationEndpointManager.cs b/src/Microsoft.Identity.Client/Instance/B2COpenIdConfigurationEndpointManager.cs
@@ -35,7 +35,7 @@ namespace Microsoft.Identity.Client.Instance
     internal class B2COpenIdConfigurationEndpointManager : IOpenIdConfigurationEndpointManager
     {
         /// <inheritdoc />
-        public Task<string> GetOpenIdConfigurationEndpointAsync(
+        public Task<string> ValidateAuthorityAndGetOpenIdDiscoveryEndpointAsync(
             AuthorityInfo authorityInfo,
             string userPrincipalName,
             RequestContext requestContext)
diff --git a/src/Microsoft.Identity.Client/Instance/IOpenIdConfigurationEndpointManager.cs b/src/Microsoft.Identity.Client/Instance/IOpenIdConfigurationEndpointManager.cs
@@ -32,7 +32,11 @@ namespace Microsoft.Identity.Client.Instance
 {
     internal interface IOpenIdConfigurationEndpointManager
     {
-        Task<string> GetOpenIdConfigurationEndpointAsync(
+        /// <summary>
+        /// Validates the authority if required and returns the OpenId discovery endpoint
+        /// for the given tenant. This is specific to each authority type.
+        /// </summary>
+        Task<string> ValidateAuthorityAndGetOpenIdDiscoveryEndpointAsync(
             AuthorityInfo authorityInfo,
             string userPrincipalName,
             RequestContext requestContext);
diff --git a/src/Microsoft.Identity.Client/Instance/InstanceDiscoveryMetadataEntry.cs b/src/Microsoft.Identity.Client/Instance/InstanceDiscoveryMetadataEntry.cs
@@ -36,10 +36,7 @@ internal sealed class InstanceDiscoveryMetadataEntry
         public string PreferredNetwork { get; set; }
 
         [DataMember(Name = "preferred_cache")]
-        public string PreferredCache { get; set; }
-
-        [DataMember(Name = "tenant_discovery_endpoint", IsRequired = false)]
-        public string TenantDiscoveryEndpoint { get; set; }
+        public string PreferredCache { get; set; }      
 
         [DataMember(Name = "aliases")]
         public string[] Aliases { get; set; }
diff --git a/tests/Microsoft.Identity.Test.Unit.net45/AuthorityAliasesTests.cs b/tests/Microsoft.Identity.Test.Unit.net45/AuthorityAliasesTests.cs
@@ -57,7 +57,7 @@ public void TestInitialize()
 #if !NET_CORE
         [TestMethod]
         [Description("Test authority migration")]
-        public async Task AuthorityMigration_IntegrationTestAsync()
+        public async Task AuthorityMigrationTestAsync()
         {
             // make sure that for all network calls "preferred_cache" environment is used
             // (it is taken from metadata in instance discovery response),
diff --git a/tests/Microsoft.Identity.Test.Unit.net45/CoreTests/InstanceTests/AadAuthorityTests.cs b/tests/Microsoft.Identity.Test.Unit.net45/CoreTests/InstanceTests/AadAuthorityTests.cs
@@ -47,7 +47,13 @@ namespace Microsoft.Identity.Test.Unit.CoreTests.InstanceTests
     [DeploymentItem("Resources\\OpenidConfiguration.json")]
     [DeploymentItem("Resources\\OpenidConfiguration-MissingFields.json")]
     public class AadAuthorityTests
-    {
+    {
+        [TestInitialize]
+        public void Init()
+        {
+            TestCommon.ResetStateAndInitMsal();
+        }
+
         [TestMethod]
         [TestCategory("AadAuthorityTests")]
         public void SuccessfulValidationTest()
@@ -77,7 +83,7 @@ public void SuccessfulValidationTest()
                     new MockHttpMessageHandler
                     {
                         ExpectedMethod = HttpMethod.Get,
-                        ExpectedUrl = "https://login.microsoftonline.in/mytenant.com/.well-known/openid-configuration",
+                        ExpectedUrl = "https://login.microsoftonline.in/mytenant.com/v2.0/.well-known/openid-configuration",
                         ResponseMessage = MockHelpers.CreateSuccessResponseMessage(
                            File.ReadAllText(ResourceHelper.GetTestResourceRelativePath("OpenidConfiguration.json")))
                     });
@@ -290,23 +296,5 @@ public void CanonicalAuthorityInitTest()
             authority = Authority.CreateAuthority(serviceBundle, UriCustomPort);
             Assert.AreEqual(UriCustomPortTailSlash, authority.AuthorityInfo.CanonicalAuthority);
         }
-
-        /*
-        [TestMethod]
-        [TestCategory("AadAuthorityTests")]
-        public void DeprecatedAuthorityTest()
-        {
-            const string uriNoPort = "https://login.windows.net/mytenant.com";
-            const string uriCustomPort = "https://login.windows.net:444/mytenant.com/";
-            var authority = Authority.CreateAuthority(uriNoPort, false);
-            Assert.AreEqual("https://login.microsoftonline.com/mytenant.com/", authority.CanonicalAuthority);
-            authority = Authority.CreateAuthority(uriCustomPort, false);
-            Assert.AreEqual("https://login.microsoftonline.com:444/mytenant.com/", authority.CanonicalAuthority);
-            authority = Authority.CreateAuthority("https://login.windows.net/tfp/tenant/policy", false);
-            Assert.AreEqual("https://login.microsoftonline.com/tfp/tenant/policy/", authority.CanonicalAuthority);
-            authority = Authority.CreateAuthority("https://login.windows.net:444/tfp/tenant/policy", false);
-            Assert.AreEqual("https://login.microsoftonline.com:444/tfp/tenant/policy/", authority.CanonicalAuthority);
-        }
-        */
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -133,7 +133,6 @@ private void CacheInstanceDiscoveryMetadata(string host, InstanceDiscoveryRespon`
`133`	`133`	`{`
`134`	`134`	`foreach (var entry in instanceDiscoveryResponse.Metadata ?? Enumerable.Empty<InstanceDiscoveryMetadataEntry>())`
`135`	`135`	`{`
`136`		`- entry.TenantDiscoveryEndpoint = instanceDiscoveryResponse.TenantDiscoveryEndpoint;`
`137`	`136`	`foreach (string aliasedAuthority in entry.Aliases ?? Enumerable.Empty<string>())`
`138`	`137`	`{`
`139`	`138`	`TryAddValue(aliasedAuthority, entry);`
`@@ -146,9 +145,8 @@ private void CacheInstanceDiscoveryMetadata(string host, InstanceDiscoveryRespon`
`146`	`145`	`{`
`147`	`146`	`PreferredNetwork = host,`
`148`	`147`	`PreferredCache = host,`
`149`		`- TenantDiscoveryEndpoint = instanceDiscoveryResponse.TenantDiscoveryEndpoint,`
`150`	`148`	`Aliases = null`
`151`	`149`	`});`
`152`	`150`	`}`
`153`	`151`	`}`
`154`		`-}`
	`152`	`+}`
Original file line number	Diff line number	Diff line change
`@@ -41,19 +41,17 @@ public AadOpenIdConfigurationEndpointManager(IServiceBundle serviceBundle)`
`41`	`41`	`}`
`42`	`42`
`43`	`43`	`/// <inheritdoc />`
`44`		`- public async Task<string> GetOpenIdConfigurationEndpointAsync(`
	`44`	`+ public async Task<string> ValidateAuthorityAndGetOpenIdDiscoveryEndpointAsync(`
`45`	`45`	`AuthorityInfo authorityInfo,`
`46`	`46`	`string userPrincipalName,`
`47`	`47`	`RequestContext requestContext)`
`48`	`48`	`{`
`49`	`49`	`var authorityUri = new Uri(authorityInfo.CanonicalAuthority);`
`50`	`50`	`if (authorityInfo.ValidateAuthority && !AadAuthority.IsInTrustedHostList(authorityUri.Host))`
`51`	`51`	`{`
`52`		`- var discoveryResponse = await _serviceBundle.AadInstanceDiscovery.GetMetadataEntryAsync(`
	`52`	`+ await _serviceBundle.AadInstanceDiscovery.GetMetadataEntryAsync(`
`53`	`53`	`authorityUri,`
`54`	`54`	`requestContext).ConfigureAwait(false);`
`55`		`-`
`56`		`- return discoveryResponse.TenantDiscoveryEndpoint;`
`57`	`55`	`}`
`58`	`56`
`59`	`57`	`return authorityInfo.CanonicalAuthority + Constants.OpenIdConfigurationEndpoint;`
Original file line number	Diff line number	Diff line change
`@@ -45,7 +45,7 @@ public AdfsOpenIdConfigurationEndpointManager(IServiceBundle serviceBundle)`
`45`	`45`	`_serviceBundle = serviceBundle;`
`46`	`46`	`}`
`47`	`47`
`48`		`- public async Task<string> GetOpenIdConfigurationEndpointAsync(`
	`48`	`+ public async Task<string> ValidateAuthorityAndGetOpenIdDiscoveryEndpointAsync(`
`49`	`49`	`AuthorityInfo authorityInfo,`
`50`	`50`	`string userPrincipalName,`
`51`	`51`	`RequestContext requestContext)`
Original file line number	Diff line number	Diff line change
`@@ -35,7 +35,7 @@ namespace Microsoft.Identity.Client.Instance`
`35`	`35`	`internal class B2COpenIdConfigurationEndpointManager : IOpenIdConfigurationEndpointManager`
`36`	`36`	`{`
`37`	`37`	`/// <inheritdoc />`
`38`		`- public Task<string> GetOpenIdConfigurationEndpointAsync(`
	`38`	`+ public Task<string> ValidateAuthorityAndGetOpenIdDiscoveryEndpointAsync(`
`39`	`39`	`AuthorityInfo authorityInfo,`
`40`	`40`	`string userPrincipalName,`
`41`	`41`	`RequestContext requestContext)`
Original file line number	Diff line number	Diff line change
`@@ -32,7 +32,11 @@ namespace Microsoft.Identity.Client.Instance`
`32`	`32`	`{`
`33`	`33`	`internal interface IOpenIdConfigurationEndpointManager`
`34`	`34`	`{`
`35`		`- Task<string> GetOpenIdConfigurationEndpointAsync(`
	`35`	`+ /// <summary>`
	`36`	`+ /// Validates the authority if required and returns the OpenId discovery endpoint`
	`37`	`+ /// for the given tenant. This is specific to each authority type.`
	`38`	`+ /// </summary>`
	`39`	`+ Task<string> ValidateAuthorityAndGetOpenIdDiscoveryEndpointAsync(`
`36`	`40`	`AuthorityInfo authorityInfo,`
`37`	`41`	`string userPrincipalName,`
`38`	`42`	`RequestContext requestContext);`
Original file line number	Diff line number	Diff line change
`@@ -57,7 +57,7 @@ public void TestInitialize()`
`57`	`57`	`#if !NET_CORE`
`58`	`58`	`[TestMethod]`
`59`	`59`	`[Description("Test authority migration")]`
`60`		`- public async Task AuthorityMigration_IntegrationTestAsync()`
	`60`	`+ public async Task AuthorityMigrationTestAsync()`
`61`	`61`	`{`
`62`	`62`	`// make sure that for all network calls "preferred_cache" environment is used`
`63`	`63`	`// (it is taken from metadata in instance discovery response),`