From 19b6252f22fe5cdbef3cc38888f2034b9ed43682 Mon Sep 17 00:00:00 2001 From: yugangw-msft Date: Thu, 1 Nov 2018 17:22:14 -0700 Subject: [PATCH 1/4] keep subscription name --- src/azure-cli-core/azure/cli/core/_profile.py | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/azure-cli-core/azure/cli/core/_profile.py b/src/azure-cli-core/azure/cli/core/_profile.py index ba0f0aa4eff..1bf953250c7 100644 --- a/src/azure-cli-core/azure/cli/core/_profile.py +++ b/src/azure-cli-core/azure/cli/core/_profile.py @@ -67,6 +67,7 @@ _SYSTEM_ASSIGNED_IDENTITY = 'systemAssignedIdentity' _USER_ASSIGNED_IDENTITY = 'userAssignedIdentity' +_ASSIGNED_IDENTITY_INFO = 'assignedIdentityId' def load_subscriptions(cli_ctx, all_clouds=False, refresh=False): @@ -229,7 +230,7 @@ def find_subscriptions_on_login(self, # use deepcopy as we don't want to persist these changes to file. return deepcopy(consolidated) - def _normalize_properties(self, user, subscriptions, is_service_principal, cert_sn_issuer_auth=None): + def _normalize_properties(self, user, subscriptions, is_service_principal, cert_sn_issuer_auth=None, user_assigned_identity_id=None): consolidated = [] for s in subscriptions: consolidated.append({ @@ -246,6 +247,8 @@ def _normalize_properties(self, user, subscriptions, is_service_principal, cert_ }) if cert_sn_issuer_auth: consolidated[-1][_USER_ENTITY][_SERVICE_PRINCIPAL_CERT_SN_ISSUER_AUTH] = True + if user_assigned_identity_id: + consolidated[-1][_USER_ENTITY][_ASSIGNED_IDENTITY_INFO] = user_assigned_identity_id return consolidated def _build_tenant_level_accounts(self, tenants): @@ -305,9 +308,10 @@ def find_subscriptions_in_vm_with_msi(self, identity_id=None): base_name = ('{}-{}'.format(identity_type, identity_id) if identity_id else identity_type) user = _USER_ASSIGNED_IDENTITY if identity_id else _SYSTEM_ASSIGNED_IDENTITY - consolidated = self._normalize_properties(user, subscriptions, is_service_principal=True) - for s in consolidated: - s[_SUBSCRIPTION_NAME] = base_name + consolidated = self._normalize_properties(user, subscriptions, is_service_principal=True, + user_assigned_identity_id=base_name) + # for s in consolidated: + # s[_SUBSCRIPTION_NAME] = base_name # key-off subscription name to allow accounts with same id(but under different identities) self._set_subscriptions(consolidated, secondary_key_name=_SUBSCRIPTION_NAME) return deepcopy(consolidated) @@ -469,9 +473,10 @@ def get_access_token_for_resource(self, username, tenant, resource): @staticmethod def _try_parse_msi_account_name(account): - subscription_name, user = account[_SUBSCRIPTION_NAME], account[_USER_ENTITY].get(_USER_NAME) + emsi_id, user = account[_USER_ENTITY].get(_ASSIGNED_IDENTITY_INFO), account[_USER_ENTITY].get(_USER_NAME) + if user in [_SYSTEM_ASSIGNED_IDENTITY, _USER_ASSIGNED_IDENTITY]: - parts = subscription_name.split('-', 1) + parts = emsi_id.split('-', 1) if parts[0] in MsiAccountTypes.valid_msi_account_types(): return parts[0], (None if len(parts) <= 1 else parts[1]) return None, None From 2fa9cd493dd38013f70ca97906460cbe9a20bad0 Mon Sep 17 00:00:00 2001 From: Yugang Wang Date: Thu, 1 Nov 2018 20:55:35 -0700 Subject: [PATCH 2/4] update history --- src/azure-cli-core/HISTORY.rst | 2 +- src/azure-cli-core/azure/cli/core/_profile.py | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/azure-cli-core/HISTORY.rst b/src/azure-cli-core/HISTORY.rst index b4e5ee2d138..a9e6585f61e 100644 --- a/src/azure-cli-core/HISTORY.rst +++ b/src/azure-cli-core/HISTORY.rst @@ -4,7 +4,7 @@ Release History =============== 2.0.51 ++++++ -* Minor fixes +* msi login: do not reuse subscription name for identity info 2.0.50 ++++++ diff --git a/src/azure-cli-core/azure/cli/core/_profile.py b/src/azure-cli-core/azure/cli/core/_profile.py index 1bf953250c7..c5b04847109 100644 --- a/src/azure-cli-core/azure/cli/core/_profile.py +++ b/src/azure-cli-core/azure/cli/core/_profile.py @@ -310,8 +310,7 @@ def find_subscriptions_in_vm_with_msi(self, identity_id=None): consolidated = self._normalize_properties(user, subscriptions, is_service_principal=True, user_assigned_identity_id=base_name) - # for s in consolidated: - # s[_SUBSCRIPTION_NAME] = base_name + # key-off subscription name to allow accounts with same id(but under different identities) self._set_subscriptions(consolidated, secondary_key_name=_SUBSCRIPTION_NAME) return deepcopy(consolidated) @@ -473,10 +472,12 @@ def get_access_token_for_resource(self, username, tenant, resource): @staticmethod def _try_parse_msi_account_name(account): - emsi_id, user = account[_USER_ENTITY].get(_ASSIGNED_IDENTITY_INFO), account[_USER_ENTITY].get(_USER_NAME) + msi_info, user = account[_USER_ENTITY].get(_ASSIGNED_IDENTITY_INFO), account[_USER_ENTITY].get(_USER_NAME) if user in [_SYSTEM_ASSIGNED_IDENTITY, _USER_ASSIGNED_IDENTITY]: - parts = emsi_id.split('-', 1) + if not msi_info: + msi_info = account[_SUBSCRIPTION_NAME] # fall back to old persisting way + parts = msi_info.split('-', 1) if parts[0] in MsiAccountTypes.valid_msi_account_types(): return parts[0], (None if len(parts) <= 1 else parts[1]) return None, None From 0e1bd12ae3610522444a2b670e0f5767fb088f03 Mon Sep 17 00:00:00 2001 From: Yugang Wang Date: Thu, 1 Nov 2018 22:04:45 -0700 Subject: [PATCH 3/4] adjust test --- src/azure-cli-core/azure/cli/core/_profile.py | 3 ++- .../azure/cli/core/tests/test_profile.py | 10 ++++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/azure-cli-core/azure/cli/core/_profile.py b/src/azure-cli-core/azure/cli/core/_profile.py index c5b04847109..5b46135b691 100644 --- a/src/azure-cli-core/azure/cli/core/_profile.py +++ b/src/azure-cli-core/azure/cli/core/_profile.py @@ -230,7 +230,8 @@ def find_subscriptions_on_login(self, # use deepcopy as we don't want to persist these changes to file. return deepcopy(consolidated) - def _normalize_properties(self, user, subscriptions, is_service_principal, cert_sn_issuer_auth=None, user_assigned_identity_id=None): + def _normalize_properties(self, user, subscriptions, is_service_principal, cert_sn_issuer_auth=None, + user_assigned_identity_id=None): consolidated = [] for s in subscriptions: consolidated.append({ diff --git a/src/azure-cli-core/azure/cli/core/tests/test_profile.py b/src/azure-cli-core/azure/cli/core/tests/test_profile.py index fd7ffdc505a..9e5217a92bd 100644 --- a/src/azure-cli-core/azure/cli/core/tests/test_profile.py +++ b/src/azure-cli-core/azure/cli/core/tests/test_profile.py @@ -867,7 +867,8 @@ def __init__(self, *args, **kwargs): s = subscriptions[0] self.assertEqual(s['user']['name'], 'systemAssignedIdentity') self.assertEqual(s['user']['type'], 'servicePrincipal') - self.assertEqual(s['name'], 'MSI') + self.assertEqual(s['user']['assignedIdentityId'], 'MSI') + self.assertEqual(s['name'], self.display_name1) self.assertEqual(s['id'], self.id1.split('/')[-1]) self.assertEqual(s['tenantId'], '54826b22-38d6-4fb2-bad9-b7b93a3e9c5a') @@ -904,7 +905,8 @@ def __init__(self, *args, **kwargs): s = subscriptions[0] self.assertEqual(s['user']['name'], 'userAssignedIdentity') self.assertEqual(s['user']['type'], 'servicePrincipal') - self.assertEqual(s['name'], 'MSIClient-{}'.format(test_client_id)) + self.assertEqual(s['name'], self.display_name1) + self.assertEqual(s['user']['assignedIdentityId'], 'MSIClient-{}'.format(test_client_id)) self.assertEqual(s['id'], self.id1.split('/')[-1]) self.assertEqual(s['tenantId'], '54826b22-38d6-4fb2-bad9-b7b93a3e9c5a') @@ -954,7 +956,7 @@ def set_token(self): subscriptions = profile.find_subscriptions_in_vm_with_msi(identity_id=test_object_id) # assert - self.assertEqual(subscriptions[0]['name'], 'MSIObject-{}'.format(test_object_id)) + self.assertEqual(subscriptions[0]['user']['assignedIdentityId'], 'MSIObject-{}'.format(test_object_id)) @mock.patch('requests.get', autospec=True) @mock.patch('azure.cli.core.profiles._shared.get_client_class', autospec=True) @@ -987,7 +989,7 @@ def __init__(self, *args, **kwargs): subscriptions = profile.find_subscriptions_in_vm_with_msi(identity_id=test_res_id) # assert - self.assertEqual(subscriptions[0]['name'], 'MSIResource-{}'.format(test_res_id)) + self.assertEqual(subscriptions[0]['user']['assignedIdentityId'], 'MSIResource-{}'.format(test_res_id)) @mock.patch('adal.AuthenticationContext.acquire_token_with_username_password', autospec=True) @mock.patch('adal.AuthenticationContext.acquire_token', autospec=True) From ce9202496c8255cbb6e577ccd0d94097a62f3f34 Mon Sep 17 00:00:00 2001 From: Yugang Wang Date: Thu, 1 Nov 2018 22:10:38 -0700 Subject: [PATCH 4/4] update field name --- src/azure-cli-core/azure/cli/core/_profile.py | 2 +- src/azure-cli-core/azure/cli/core/tests/test_profile.py | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/azure-cli-core/azure/cli/core/_profile.py b/src/azure-cli-core/azure/cli/core/_profile.py index 5b46135b691..8f517992f24 100644 --- a/src/azure-cli-core/azure/cli/core/_profile.py +++ b/src/azure-cli-core/azure/cli/core/_profile.py @@ -67,7 +67,7 @@ _SYSTEM_ASSIGNED_IDENTITY = 'systemAssignedIdentity' _USER_ASSIGNED_IDENTITY = 'userAssignedIdentity' -_ASSIGNED_IDENTITY_INFO = 'assignedIdentityId' +_ASSIGNED_IDENTITY_INFO = 'assignedIdentityInfo' def load_subscriptions(cli_ctx, all_clouds=False, refresh=False): diff --git a/src/azure-cli-core/azure/cli/core/tests/test_profile.py b/src/azure-cli-core/azure/cli/core/tests/test_profile.py index 9e5217a92bd..f17d99c4212 100644 --- a/src/azure-cli-core/azure/cli/core/tests/test_profile.py +++ b/src/azure-cli-core/azure/cli/core/tests/test_profile.py @@ -867,7 +867,7 @@ def __init__(self, *args, **kwargs): s = subscriptions[0] self.assertEqual(s['user']['name'], 'systemAssignedIdentity') self.assertEqual(s['user']['type'], 'servicePrincipal') - self.assertEqual(s['user']['assignedIdentityId'], 'MSI') + self.assertEqual(s['user']['assignedIdentityInfo'], 'MSI') self.assertEqual(s['name'], self.display_name1) self.assertEqual(s['id'], self.id1.split('/')[-1]) self.assertEqual(s['tenantId'], '54826b22-38d6-4fb2-bad9-b7b93a3e9c5a') @@ -906,7 +906,7 @@ def __init__(self, *args, **kwargs): self.assertEqual(s['user']['name'], 'userAssignedIdentity') self.assertEqual(s['user']['type'], 'servicePrincipal') self.assertEqual(s['name'], self.display_name1) - self.assertEqual(s['user']['assignedIdentityId'], 'MSIClient-{}'.format(test_client_id)) + self.assertEqual(s['user']['assignedIdentityInfo'], 'MSIClient-{}'.format(test_client_id)) self.assertEqual(s['id'], self.id1.split('/')[-1]) self.assertEqual(s['tenantId'], '54826b22-38d6-4fb2-bad9-b7b93a3e9c5a') @@ -956,7 +956,7 @@ def set_token(self): subscriptions = profile.find_subscriptions_in_vm_with_msi(identity_id=test_object_id) # assert - self.assertEqual(subscriptions[0]['user']['assignedIdentityId'], 'MSIObject-{}'.format(test_object_id)) + self.assertEqual(subscriptions[0]['user']['assignedIdentityInfo'], 'MSIObject-{}'.format(test_object_id)) @mock.patch('requests.get', autospec=True) @mock.patch('azure.cli.core.profiles._shared.get_client_class', autospec=True) @@ -989,7 +989,7 @@ def __init__(self, *args, **kwargs): subscriptions = profile.find_subscriptions_in_vm_with_msi(identity_id=test_res_id) # assert - self.assertEqual(subscriptions[0]['user']['assignedIdentityId'], 'MSIResource-{}'.format(test_res_id)) + self.assertEqual(subscriptions[0]['user']['assignedIdentityInfo'], 'MSIResource-{}'.format(test_res_id)) @mock.patch('adal.AuthenticationContext.acquire_token_with_username_password', autospec=True) @mock.patch('adal.AuthenticationContext.acquire_token', autospec=True)