{Core} Support Azure Stack and ADFS with MSAL

src/azure-cli-core/azure/cli/core/_profile.py

@@ -147,10 +147,7 @@ def login(self,
         if not scopes:
             scopes = self._arm_scope
 
-        # For ADFS, auth_tenant is 'adfs'
-        # https://github.com/Azure/azure-sdk-for-python/blob/661cd524e88f480c14220ed1f86de06aaff9a977/sdk/identity/azure-identity/CHANGELOG.md#L19
-        authority, auth_tenant = _detect_adfs_authority(self.cli_ctx.cloud.endpoints.active_directory, tenant)
-        identity = Identity(authority=authority, tenant_id=auth_tenant, client_id=client_id)
+        identity = Identity(self._authority, tenant_id=tenant, client_id=client_id)
 
         user_identity = None
         if interactive:
@@ -589,7 +586,7 @@ def _create_credential(self, account, tenant_id=None, client_id=None):
         user_type = account[_USER_ENTITY][_USER_TYPE]
         username_or_sp_id = account[_USER_ENTITY][_USER_NAME]
         tenant_id = tenant_id if tenant_id else account[_TENANT_ID]
-        identity = Identity(client_id=client_id, authority=self._authority, tenant_id=tenant_id)
+        identity = Identity(self._authority, tenant_id=tenant_id, client_id=client_id)
 
         # User
         if user_type == _USER:
@@ -720,7 +717,7 @@ def find_using_common_tenant(self, username, credential=None):
 
             logger.info("Finding subscriptions under tenant %s", t.tenant_id_name)
 
-            identity = Identity(self.authority, tenant_id)
+            identity = Identity(self.authority, tenant_id=tenant_id)
 
             specific_tenant_credential = identity.get_user_credential(username)
 

src/azure-cli-core/azure/cli/core/auth/identity.py

@@ -13,6 +13,7 @@
 
 from .msal_authentication import UserCredential, ServicePrincipalCredential
 from .util import check_result
+from .persistence import load_persisted_token_cache, file_extensions
 
 # Service principal entry properties
 from .msal_authentication import _CLIENT_ID, _TENANT, _CLIENT_SECRET, _CERTIFICATE, _CLIENT_ASSERTION,\
@@ -38,19 +39,22 @@ class Identity:  # pylint: disable=too-many-instance-attributes
     # https://github.com/AzureAD/microsoft-authentication-library-for-python/pull/407
     http_cache = None
 
-    def __init__(self, authority=None, tenant_id=None, client_id=None):
+    def __init__(self, authority, tenant_id=None, client_id=None):
         """
-
-        :param authority: AAD endpoint, like https://login.microsoftonline.com/
-        :param tenant_id: Tenant GUID, like 00000000-0000-0000-0000-000000000000
+        :param authority: Authentication authority endpoint. For example,
+            - AAD: https://login.microsoftonline.com
+            - ADFS: https://adfs.redmond.azurestack.corp.microsoft.com/adfs
+        :param tenant_id: Tenant GUID, like 00000000-0000-0000-0000-000000000000. If unspecified, default to
+            'organizations'.
         :param client_id: Client ID of the CLI application.
         """
         self.authority = authority
-        self.tenant_id = tenant_id or "organizations"
-        # Build the authority in MSAL style, like https://login.microsoftonline.com/your_tenant
-        self.msal_authority = "{}/{}".format(self.authority, self.tenant_id)
+        self.tenant_id = tenant_id
         self.client_id = client_id or AZURE_CLI_CLIENT_ID
 
+        # Build the authority in MSAL style
+        self._msal_authority, self._is_adfs = _get_authority_url(authority, tenant_id)
+
         config_dir = get_config_dir()
         self._token_cache_file = os.path.join(config_dir, "msal_token_cache")
         self._secret_file = os.path.join(config_dir, "service_principal_entries")
@@ -65,13 +69,12 @@ def __init__(self, authority=None, tenant_id=None, client_id=None):
         # Store for Service principal credential persistence
         self._msal_secret_store = ServicePrincipalStore(self._secret_file, self.token_encryption)
         self._msal_app_kwargs = {
-            "authority": self.msal_authority,
+            "authority": self._msal_authority,
             "token_cache": self._load_msal_cache()
             # "http_cache": Identity.http_cache
         }
 
     def _load_msal_cache(self):
-        from .persistence import load_persisted_token_cache
         # Store for user token persistence
         cache = load_persisted_token_cache(self._token_cache_file, self.token_encryption)
         return cache
@@ -108,15 +111,17 @@ def msal_app(self):
     def login_with_auth_code(self, scopes=None, **kwargs):
         # Emit a warning to inform that a browser is opened.
         # Only show the path part of the URL and hide the query string.
-        logger.warning("The default web browser has been opened at %s/oauth2/v2.0/authorize. "
-                       "Please continue the login in the web browser. "
+        logger.warning("The default web browser has been opened at %s. Please continue the login in the web browser. "
                        "If no web browser is available or if the web browser fails to open, use device code flow "
-                       "with `az login --use-device-code`.", self.msal_authority)
+                       "with `az login --use-device-code`.", self._msal_authority)
 
         success_template, error_template = _read_response_templates()
 
+        # For AAD, use port 0 to let the system choose arbitrary unused ephemeral port to avoid port collision
+        # on port 8400 from the old design. However, ADFS only allows port 8400.
         result = self.msal_app.acquire_token_interactive(
-            scopes, prompt='select_account', success_template=success_template, error_template=error_template, **kwargs)
+            scopes, prompt='select_account', port=8400 if self._is_adfs else None,
+            success_template=success_template, error_template=error_template, **kwargs)
         return check_result(result)
 
     def login_with_device_code(self, scopes=None, **kwargs):
@@ -159,10 +164,8 @@ def logout_user(self, user):
             self.msal_app.remove_account(account)
 
     def logout_all_users(self):
-        try:
-            os.remove(self._token_cache_file)
-        except FileNotFoundError:
-            pass
+        for e in file_extensions.values():
+            _try_remove(self._token_cache_file + e)
 
     def logout_service_principal(self, sp):
         # remove service principal secrets
@@ -300,10 +303,8 @@ def remove_entry(self, sp_id):
             self._save_persistence()
 
     def remove_all_entries(self):
-        try:
-            os.remove(self._secret_file)
-        except FileNotFoundError:
-            pass
+        for e in file_extensions.values():
+            _try_remove(self._secret_file + e)
 
     def _save_persistence(self):
         self._secret_store.save(self._entries)
@@ -323,3 +324,30 @@ def _read_response_templates():
         error_template = f.read()
 
     return success_template, error_template
+
+
+def _get_authority_url(authority_endpoint, tenant):
+    """Convert authority endpoint (active_directory) to MSAL authority:
+        - AAD: https://login.microsoftonline.com/your_tenant
+        - ADFS: https://adfs.redmond.azurestack.corp.microsoft.com/adfs
+    For ADFS, tenant is discarded.
+    """
+
+    # Some Azure Stack (bellevue)'s metadata returns
+    #   "loginEndpoint": "https://login.microsoftonline.com/"
+    # Normalize it by removing the trailing /, so that authority_url won't become
+    # "https://login.microsoftonline.com//tenant_id".
+    authority_endpoint = authority_endpoint.rstrip('/').lower()
+    is_adfs = authority_endpoint.endswith('adfs')
+    if is_adfs:
+        authority_url = authority_endpoint
+    else:
+        authority_url = '{}/{}'.format(authority_endpoint, tenant or "organizations")
+    return authority_url, is_adfs
+
+
+def _try_remove(path):
+    try:
+        os.remove(path)
+    except FileNotFoundError:
+        pass

src/azure-cli-core/azure/cli/core/auth/persistence.py

@@ -18,6 +18,9 @@
 
 logger = get_logger(__name__)
 
+# Files extensions for encrypted and plaintext persistence
+file_extensions = {True: '.bin', False: '.json'}
+
 
 def load_persisted_token_cache(location, encrypt):
     persistence = build_persistence(location, encrypt)
@@ -31,8 +34,8 @@ def load_secret_store(location, encrypt):
 
 def build_persistence(location, encrypt):
     """Build a suitable persistence instance based your current OS"""
+    location += file_extensions[encrypt]
     if encrypt:
-        location += '.bin'
         if sys.platform.startswith('win'):
             return FilePersistenceWithDataProtection(location)
         if sys.platform.startswith('darwin'):
@@ -44,7 +47,6 @@ def build_persistence(location, encrypt):
                 attributes={"my_attr1": "foo", "my_attr2": "bar"}
             )
     else:
-        location += '.json'
         return FilePersistence(location)
 
 

src/azure-cli-core/azure/cli/core/auth/tests/test_identity.py

@@ -7,15 +7,16 @@
 import unittest
 from unittest import mock
 
-from azure.cli.core.auth.identity import Identity, ServicePrincipalAuth, ServicePrincipalStore
+from azure.cli.core.auth.identity import (Identity, ServicePrincipalAuth, ServicePrincipalStore,
+                                          _get_authority_url)
 from knack.util import CLIError
 
 
 class TestIdentity(unittest.TestCase):
 
     def test_login_with_service_principal_certificate_cert_err(self):
         import os
-        identity = Identity()
+        identity = Identity('https://login.microsoftonline.com')
         current_dir = os.path.dirname(os.path.realpath(__file__))
         test_cert_file = os.path.join(current_dir, 'err_sp_cert.pem')
 
@@ -139,6 +140,35 @@ def test_remove_entry(self, load_secret_store_mock):
         assert store._content == []
 
 
+class TestUtils(unittest.TestCase):
+
+    def test_get_authority_url(self):
+        # AAD
+        # Default tenant
+        self.assertEqual(_get_authority_url('https://login.microsoftonline.com', None),
+                         ('https://login.microsoftonline.com/organizations', False))
+        # Trailing slash is stripped
+        self.assertEqual(_get_authority_url('https://login.microsoftonline.com/', None),
+                         ('https://login.microsoftonline.com/organizations', False))
+        # Custom tenant
+        self.assertEqual(_get_authority_url('https://login.microsoftonline.com',
+                                            '54826b22-38d6-4fb2-bad9-b7b93a3e9c5a'),
+                         ('https://login.microsoftonline.com/54826b22-38d6-4fb2-bad9-b7b93a3e9c5a', False))
+
+        # ADFS
+        # Default tenant
+        adfs_expected = ('https://adfs.redmond.azurestack.corp.microsoft.com/adfs', True)
+        self.assertEqual(_get_authority_url('https://adfs.redmond.azurestack.corp.microsoft.com/adfs', None),
+                         adfs_expected)
+        # Trailing slash is stripped
+        self.assertEqual(_get_authority_url('https://adfs.redmond.azurestack.corp.microsoft.com/adfs/', None),
+                         adfs_expected)
+        # Tenant ID is discarded
+        self.assertEqual(_get_authority_url('https://adfs.redmond.azurestack.corp.microsoft.com/adfs',
+                                            '601d729d-0000-0000-0000-000000000000'),
+                         adfs_expected)
+
+
 class MemoryStore:
 
     def __init__(self):

src/azure-cli-core/azure/cli/core/auth/util.py

@@ -123,11 +123,11 @@ def check_result(result, **kwargs):
 
     # For user authentication
     if 'id_token_claims' in result:
-        idt = result['id_token_claims']
+        id_token = result['id_token_claims']
         return {
             # AAD returns "preferred_username", ADFS returns "upn"
-            'username': idt.get("preferred_username") or idt["upn"],
-            'tenant_id': idt['tid']
+            'username': id_token.get("preferred_username") or id_token["upn"],
+            'tenant_id': id_token['tid']
         }
 
     return None

src/azure-cli-core/azure/cli/core/tests/test_profile.py

@@ -16,8 +16,7 @@
 
 from azure.core.credentials import AccessToken
 
-from azure.cli.core._profile import (Profile, SubscriptionFinder,
-                                     _detect_adfs_authority, _attach_token_tenant,
+from azure.cli.core._profile import (Profile, SubscriptionFinder, _attach_token_tenant,
                                      _transform_subscription_for_multiapi)
 
 from azure.mgmt.resource.subscriptions.models import \
@@ -1417,29 +1416,6 @@ def __init__(self, tenant_id, display_name="DISPLAY_NAME"):
 
 
 class TestUtils(unittest.TestCase):
-    def test_detect_adfs_authority(self):
-        # Public cloud
-        # Default tenant
-        self.assertEqual(_detect_adfs_authority('https://login.microsoftonline.com', None),
-                         ('https://login.microsoftonline.com', None))
-        # Trailing slash is stripped
-        self.assertEqual(_detect_adfs_authority('https://login.microsoftonline.com/', None),
-                         ('https://login.microsoftonline.com', None))
-        # Custom tenant
-        self.assertEqual(_detect_adfs_authority('https://login.microsoftonline.com', '601d729d-0000-0000-0000-000000000000'),
-                         ('https://login.microsoftonline.com', '601d729d-0000-0000-0000-000000000000'))
-
-        # ADFS
-        # Default tenant
-        self.assertEqual(_detect_adfs_authority('https://adfs.redmond.azurestack.corp.microsoft.com/adfs', None),
-                         ('https://adfs.redmond.azurestack.corp.microsoft.com', 'adfs'))
-        # Trailing slash is stripped
-        self.assertEqual(_detect_adfs_authority('https://adfs.redmond.azurestack.corp.microsoft.com/adfs/', None),
-                         ('https://adfs.redmond.azurestack.corp.microsoft.com', 'adfs'))
-        # Tenant ID is discarded
-        self.assertEqual(_detect_adfs_authority('https://adfs.redmond.azurestack.corp.microsoft.com/adfs', '601d729d-0000-0000-0000-000000000000'),
-                         ('https://adfs.redmond.azurestack.corp.microsoft.com', 'adfs'))
-
     def test_attach_token_tenant(self):
         from azure.mgmt.resource.subscriptions.v2016_06_01.models import Subscription \
             as Subscription_v2016_06_01