Merge branch 'dev' into wam

rayluo · rayluo · commit 9794dcf099e7 · 2022-09-28T10:58:44.000-07:00
diff --git a/msal/application.py b/msal/application.py
@@ -13,7 +13,7 @@
 
 from .oauth2cli import Client, JwtAssertionCreator
 from .oauth2cli.oidc import decode_part
-from .authority import Authority
+from .authority import Authority, WORLD_WIDE
 from .mex import send_request as mex_send_request
 from .wstrust_request import send_request as wst_send_request
 from .wstrust_response import *
@@ -150,7 +150,6 @@ def obtain_token_by_username_password(self, username, password, **kwargs):
 
 
 class ClientApplication(object):
-
     ACQUIRE_TOKEN_SILENT_ID = "84"
     ACQUIRE_TOKEN_BY_REFRESH_TOKEN = "85"
     ACQUIRE_TOKEN_BY_USERNAME_PASSWORD_ID = "301"
@@ -178,6 +177,7 @@ def __init__(
                 # when we would eventually want to add this feature to PCA in future.
             exclude_scopes=None,
             http_cache=None,
+            instance_discovery=None,
             allow_broker=None,
             ):
         """Create an instance of application.
@@ -415,6 +415,34 @@ def __init__(
 
             New in version 1.16.0.
 
+        :param boolean instance_discovery:
+            Historically, MSAL would connect to a central endpoint located at
+            ``https://login.microsoftonline.com`` to acquire some metadata,
+            especially when using an unfamiliar authority.
+            This behavior is known as Instance Discovery.
+
+            This parameter defaults to None, which enables the Instance Discovery.
+
+            If you know some authorities which you allow MSAL to operate with as-is,
+            without involving any Instance Discovery, the recommended pattern is::
+
+                known_authorities = frozenset([  # Treat your known authorities as const
+                    "https://contoso.com/adfs", "https://login.azs/foo"])
+                ...
+                authority = "https://contoso.com/adfs"  # Assuming your app will use this
+                app1 = PublicClientApplication(
+                    "client_id",
+                    authority=authority,
+                    # Conditionally disable Instance Discovery for known authorities
+                    instance_discovery=authority not in known_authorities,
+                    )
+
+            If you do not know some authorities beforehand,
+            yet still want MSAL to accept any authority that you will provide,
+            you can use a ``False`` to unconditionally disable Instance Discovery.
+
+            New in version 1.19.0.
+
         :param boolean allow_broker:
             Brokers provide Single-Sign-On, device identification,
             and application identification verification.
@@ -448,6 +476,7 @@ def __init__(
         self.client_credential = client_credential
         self.client_claims = client_claims
         self._client_capabilities = client_capabilities
+        self._instance_discovery = instance_discovery
 
         if exclude_scopes and not isinstance(exclude_scopes, list):
             raise ValueError(
@@ -487,18 +516,24 @@ def __init__(
 
         # Here the self.authority will not be the same type as authority in input
         try:
+            authority_to_use = authority or "https://{}/common/".format(WORLD_WIDE)
             self.authority = Authority(
-                authority or "https://login.microsoftonline.com/common/",
-                self.http_client, validate_authority=validate_authority)
+                authority_to_use,
+                self.http_client,
+                validate_authority=validate_authority,
+                instance_discovery=self._instance_discovery,
+                )
         except ValueError:  # Those are explicit authority validation errors
             raise
         except Exception:  # The rest are typically connection errors
             if validate_authority and azure_region:
                 # Since caller opts in to use region, here we tolerate connection
                 # errors happened during authority validation at non-region endpoint
                 self.authority = Authority(
-                    authority or "https://login.microsoftonline.com/common/",
-                    self.http_client, validate_authority=False)
+                    authority_to_use,
+                    self.http_client,
+                    instance_discovery=False,
+                    )
             else:
                 raise
         is_confidential_app = bool(
@@ -584,10 +619,11 @@ def _get_regional_authority(self, central_authority):
                     "sts.windows.net",
                     )
                 else "{}.{}".format(region_to_use, central_authority.instance))
-            return Authority(
+            return Authority(  # The central_authority has already been validated
                 "https://{}/{}".format(regional_host, central_authority.tenant),
                 self.http_client,
-                validate_authority=False)  # The central_authority has already been validated
+                instance_discovery=False,
+                )
         return None
 
     def _build_client(self, client_credential, authority, skip_regional_client=False):
@@ -839,7 +875,8 @@ def get_authorization_request_url(
         # Multi-tenant app can use new authority on demand
         the_authority = Authority(
             authority,
-            self.http_client
+            self.http_client,
+            instance_discovery=self._instance_discovery,
             ) if authority else self.authority
 
         client = _ClientWithCcsRoutingInfo(
@@ -1062,14 +1099,23 @@ def _find_msal_accounts(self, environment):
             }
         return list(grouped_accounts.values())
 
+    def _get_instance_metadata(self):  # This exists so it can be mocked in unit test
+        resp = self.http_client.get(
+            "https://login.microsoftonline.com/common/discovery/instance?api-version=1.1&authorization_endpoint=https://login.microsoftonline.com/common/oauth2/authorize",  # TBD: We may extend this to use self._instance_discovery endpoint
+            headers={'Accept': 'application/json'})
+        resp.raise_for_status()
+        return json.loads(resp.text)['metadata']
+
     def _get_authority_aliases(self, instance):
+        if self._instance_discovery is False:
+            return []
+        if self.authority._is_known_to_developer:
+            # Then it is an ADFS/B2C/known_authority_hosts situation
+            # which may not reach the central endpoint, so we skip it.
+            return []
         if not self.authority_groups:
-            resp = self.http_client.get(
-                "https://login.microsoftonline.com/common/discovery/instance?api-version=1.1&authorization_endpoint=https://login.microsoftonline.com/common/oauth2/authorize",
-                headers={'Accept': 'application/json'})
-            resp.raise_for_status()
             self.authority_groups = [
-                set(group['aliases']) for group in json.loads(resp.text)['metadata']]
+                set(group['aliases']) for group in self._get_instance_metadata()]
         for group in self.authority_groups:
             if instance in group:
                 return [alias for alias in group if alias != instance]
@@ -1223,6 +1269,7 @@ def acquire_token_silent_with_error(
         # the_authority = Authority(
         #     authority,
         #     self.http_client,
+        #     instance_discovery=self._instance_discovery,
         #     ) if authority else self.authority
         result = self._acquire_token_silent_from_cache_and_possibly_refresh_it(
             scopes, account, self.authority, force_refresh=force_refresh,
@@ -1244,7 +1291,8 @@ def acquire_token_silent_with_error(
             the_authority = Authority(
                 "https://" + alias + "/" + self.authority.tenant,
                 self.http_client,
-                validate_authority=False)
+                instance_discovery=False,
+                )
             result = self._acquire_token_silent_from_cache_and_possibly_refresh_it(
                 scopes, account, the_authority, force_refresh=force_refresh,
                 claims_challenge=claims_challenge,
@@ -1539,10 +1587,9 @@ def acquire_token_by_username_password(
                 scopes,  # Decorated scopes won't work due to offline_access
                 MSALRuntime_Username=username,
                 MSALRuntime_Password=password,
-                validateAuthority="no"
-                    if self.authority._validate_authority is False
-                    or self.authority.is_adfs or self.authority._is_b2c
-                    else None,
+                validateAuthority="no" if (
+                    self.authority._is_known_to_developer
+                    or self._instance_discovery is False) else None,
                 claims=claims,
                 )
             return self._process_broker_response(response, scopes, kwargs.get("data", {}))
@@ -1807,10 +1854,9 @@ def _acquire_token_interactive_via_broker(
             logger.debug(kwargs["welcome_template"])  # Experimental
         authority = "https://{}/{}".format(
             self.authority.instance, self.authority.tenant)
-        validate_authority = (
-            "no" if self.authority._validate_authority is False
-                or self.authority.is_adfs or self.authority._is_b2c
-            else None)
+        validate_authority = "no" if (
+            self.authority._is_known_to_developer
+            or self._instance_discovery is False) else None
         # Calls different broker methods to mimic the OIDC behaviors
         if login_hint and prompt != "select_account":  # OIDC prompts when the user did not sign in
             accounts = self.get_accounts(username=login_hint)
diff --git a/msal/authority.py b/msal/authority.py
@@ -58,7 +58,11 @@ def http_client(self):  # Obsolete. We will remove this eventually
             "authority.http_client might be removed in MSAL Python 1.21+", DeprecationWarning)
         return self._http_client
 
-    def __init__(self, authority_url, http_client, validate_authority=True):
+    def __init__(
+            self, authority_url, http_client,
+            validate_authority=True,
+            instance_discovery=None,
+            ):
         """Creates an authority instance, and also validates it.
 
         :param validate_authority:
@@ -67,20 +71,34 @@ def __init__(self, authority_url, http_client, validate_authority=True):
             This parameter only controls whether an instance discovery will be
             performed.
         """
+        # :param instance_discovery:
+        #    By default, the known-to-Microsoft validation will use an
+        #    instance discovery endpoint located at ``login.microsoftonline.com``.
+        #    You can customize the endpoint by providing a url as a string.
+        #    Or you can turn this behavior off by passing in a False here.
         self._http_client = http_client
         if isinstance(authority_url, AuthorityBuilder):
             authority_url = str(authority_url)
         authority, self.instance, tenant = canonicalize(authority_url)
+        self.is_adfs = tenant.lower() == 'adfs'
         parts = authority.path.split('/')
-        self._is_b2c = any(self.instance.endswith("." + d) for d in WELL_KNOWN_B2C_HOSTS) or (
-            len(parts) == 3 and parts[2].lower().startswith("b2c_"))
-        self._validate_authority = True if validate_authority is None else bool(validate_authority)
-        if (tenant != "adfs" and (not self._is_b2c) and self._validate_authority
-                and self.instance not in WELL_KNOWN_AUTHORITY_HOSTS):
-            payload = instance_discovery(
+        self._is_b2c = any(
+            self.instance.endswith("." + d) for d in WELL_KNOWN_B2C_HOSTS
+            ) or (len(parts) == 3 and parts[2].lower().startswith("b2c_"))
+        self._is_known_to_developer = self.is_adfs or self._is_b2c or not validate_authority
+        is_known_to_microsoft = self.instance in WELL_KNOWN_AUTHORITY_HOSTS
+        instance_discovery_endpoint = 'https://{}/common/discovery/instance'.format(  # Note: This URL seemingly returns V1 endpoint only
+            WORLD_WIDE  # Historically using WORLD_WIDE. Could use self.instance too
+                # See https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/blob/4.0.0/src/Microsoft.Identity.Client/Instance/AadInstanceDiscovery.cs#L101-L103
+                # and https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/blob/4.0.0/src/Microsoft.Identity.Client/Instance/AadAuthority.cs#L19-L33
+            ) if instance_discovery in (None, True) else instance_discovery
+        if instance_discovery_endpoint and not (
+                is_known_to_microsoft or self._is_known_to_developer):
+            payload = _instance_discovery(
                 "https://{}{}/oauth2/v2.0/authorize".format(
                     self.instance, authority.path),
-                self._http_client)
+                self._http_client,
+                instance_discovery_endpoint)
             if payload.get("error") == "invalid_instance":
                 raise ValueError(
                     "invalid_instance: "
@@ -114,7 +132,6 @@ def __init__(self, authority_url, http_client, validate_authority=True):
         self.token_endpoint = openid_config['token_endpoint']
         self.device_authorization_endpoint = openid_config.get('device_authorization_endpoint')
         _, _, self.tenant = canonicalize(self.token_endpoint)  # Usually a GUID
-        self.is_adfs = self.tenant.lower() == 'adfs'
 
     def user_realm_discovery(self, username, correlation_id=None, response=None):
         # It will typically return a dict containing "ver", "account_type",
@@ -146,13 +163,9 @@ def canonicalize(authority_url):
             % authority_url)
     return authority, authority.hostname, parts[1]
 
-def instance_discovery(url, http_client, **kwargs):
-    resp = http_client.get(  # Note: This URL seemingly returns V1 endpoint only
-        'https://{}/common/discovery/instance'.format(
-            WORLD_WIDE  # Historically using WORLD_WIDE. Could use self.instance too
-                # See https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/blob/4.0.0/src/Microsoft.Identity.Client/Instance/AadInstanceDiscovery.cs#L101-L103
-                # and https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/blob/4.0.0/src/Microsoft.Identity.Client/Instance/AadAuthority.cs#L19-L33
-            ),
+def _instance_discovery(url, http_client, instance_discovery_endpoint, **kwargs):
+    resp = http_client.get(
+        instance_discovery_endpoint,
         params={'authorization_endpoint': url, 'api-version': '1.0'},
         **kwargs)
     return json.loads(resp.text)
diff --git a/setup.py b/setup.py
@@ -76,7 +76,7 @@
         'requests>=2.0.0,<3',
         'PyJWT[crypto]>=1.0.0,<3',  # MSAL does not use jwt.decode(), therefore is insusceptible to CVE-2022-29217 so no need to bump to PyJWT 2.4+
 
-        'cryptography>=0.6,<40',
+        'cryptography>=0.6,<41',
             # load_pem_private_key() is available since 0.6
             # https://github.com/pyca/cryptography/blob/master/CHANGELOG.rst#06---2014-09-29
             #
diff --git a/tests/test_authority.py b/tests/test_authority.py
@@ -1,5 +1,10 @@
 import os
+try:
+    from unittest.mock import patch
+except:
+    from mock import patch
 
+import msal
 from msal.authority import *
 from tests import unittest
 from tests.http_client import MinimalHttpClient
@@ -123,3 +128,64 @@ class MockResponse(object):
         finally:  # MUST NOT let the previous test changes affect other test cases
             Authority._domains_without_user_realm_discovery = set([])
 
+
+@patch("msal.authority.tenant_discovery", return_value={
+    "authorization_endpoint": "https://contoso.com/placeholder",
+    "token_endpoint": "https://contoso.com/placeholder",
+    })
+@patch("msal.authority._instance_discovery")
+@patch.object(msal.ClientApplication, "_get_instance_metadata", return_value=[])
+class TestMsalBehaviorsWithoutAndWithInstanceDiscoveryBoolean(unittest.TestCase):
+    """Test cases use ClientApplication, which is a base class of both PCA and CCA"""
+
+    def test_by_default_a_known_to_microsoft_authority_should_skip_validation_but_still_use_instance_metadata(
+            self, instance_metadata, known_to_microsoft_validation, _):
+        app = msal.ClientApplication("id", authority="https://login.microsoftonline.com/common")
+        known_to_microsoft_validation.assert_not_called()
+        app.get_accounts()  # This could make an instance metadata call for authority aliases
+        instance_metadata.assert_called_once_with()
+
+    def test_validate_authority_boolean_should_skip_validation_and_instance_metadata(
+            self, instance_metadata, known_to_microsoft_validation, _):
+        """Pending deprecation, but kept for backward compatibility, for now"""
+        app = msal.ClientApplication(
+            "id", authority="https://contoso.com/common", validate_authority=False)
+        known_to_microsoft_validation.assert_not_called()
+        app.get_accounts()  # This could make an instance metadata call for authority aliases
+        instance_metadata.assert_not_called()
+
+    def test_by_default_adfs_should_skip_validation_and_instance_metadata(
+            self, instance_metadata, known_to_microsoft_validation, _):
+        """Not strictly required, but when/if we already supported it, we better keep it"""
+        app = msal.ClientApplication("id", authority="https://contoso.com/adfs")
+        known_to_microsoft_validation.assert_not_called()
+        app.get_accounts()  # This could make an instance metadata call for authority aliases
+        instance_metadata.assert_not_called()
+
+    def test_by_default_b2c_should_skip_validation_and_instance_metadata(
+            self, instance_metadata, known_to_microsoft_validation, _):
+        """Not strictly required, but when/if we already supported it, we better keep it"""
+        app = msal.ClientApplication(
+            "id", authority="https://login.b2clogin.com/contoso/b2c_policy")
+        known_to_microsoft_validation.assert_not_called()
+        app.get_accounts()  # This could make an instance metadata call for authority aliases
+        instance_metadata.assert_not_called()
+
+    def test_turning_off_instance_discovery_should_work_for_all_kinds_of_clouds(
+            self, instance_metadata, known_to_microsoft_validation, _):
+        for authority in [
+                "https://login.microsoftonline.com/common",  # Known to Microsoft
+                "https://contoso.com/adfs",  # ADFS
+                "https://login.b2clogin.com/contoso/b2c_policy",  # B2C
+                "https://private.cloud/foo",  # Private Cloud
+                ]:
+            self._test_turning_off_instance_discovery_should_skip_authority_validation_and_instance_metadata(
+                authority, instance_metadata, known_to_microsoft_validation)
+
+    def _test_turning_off_instance_discovery_should_skip_authority_validation_and_instance_metadata(
+            self, authority, instance_metadata, known_to_microsoft_validation):
+        app = msal.ClientApplication("id", authority=authority, instance_discovery=False)
+        known_to_microsoft_validation.assert_not_called()
+        app.get_accounts()  # This could make an instance metadata call for authority aliases
+        instance_metadata.assert_not_called()
+
diff --git a/tests/test_e2e.py b/tests/test_e2e.py

Original file line number	Diff line number	Diff line change
`@@ -76,7 +76,7 @@`
`76`	`76`	`'requests>=2.0.0,<3',`
`77`	`77`	`'PyJWT[crypto]>=1.0.0,<3', # MSAL does not use jwt.decode(), therefore is insusceptible to CVE-2022-29217 so no need to bump to PyJWT 2.4+`
`78`	`78`
`79`		`- 'cryptography>=0.6,<40',`
	`79`	`+ 'cryptography>=0.6,<41',`
`80`	`80`	`# load_pem_private_key() is available since 0.6`
`81`	`81`	`# https://github.com/pyca/cryptography/blob/master/CHANGELOG.rst#06---2014-09-29`
`82`	`82`	`#`