Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[core] maintain the logical subscription name on log-in with msi #7744

Merged
merged 4 commits into from
Nov 12, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/azure-cli-core/HISTORY.rst
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ Release History
===============
2.0.51
++++++
* Minor fixes
* msi login: do not reuse subscription name for identity info

2.0.50
++++++
Expand Down
19 changes: 13 additions & 6 deletions src/azure-cli-core/azure/cli/core/_profile.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@

_SYSTEM_ASSIGNED_IDENTITY = 'systemAssignedIdentity'
_USER_ASSIGNED_IDENTITY = 'userAssignedIdentity'
_ASSIGNED_IDENTITY_INFO = 'assignedIdentityInfo'


def load_subscriptions(cli_ctx, all_clouds=False, refresh=False):
Expand Down Expand Up @@ -229,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):
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({
Expand All @@ -246,6 +248,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):
Expand Down Expand Up @@ -305,9 +309,9 @@ 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)

# 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)
Expand Down Expand Up @@ -469,9 +473,12 @@ 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)
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 = subscription_name.split('-', 1)
if not msi_info:
msi_info = account[_SUBSCRIPTION_NAME] # fall back to old persisting way
williexu marked this conversation as resolved.
Show resolved Hide resolved
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
Expand Down
10 changes: 6 additions & 4 deletions src/azure-cli-core/azure/cli/core/tests/test_profile.py
Original file line number Diff line number Diff line change
Expand Up @@ -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']['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')

Expand Down Expand Up @@ -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']['assignedIdentityInfo'], 'MSIClient-{}'.format(test_client_id))
self.assertEqual(s['id'], self.id1.split('/')[-1])
self.assertEqual(s['tenantId'], '54826b22-38d6-4fb2-bad9-b7b93a3e9c5a')

Expand Down Expand Up @@ -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']['assignedIdentityInfo'], 'MSIObject-{}'.format(test_object_id))

@mock.patch('requests.get', autospec=True)
@mock.patch('azure.cli.core.profiles._shared.get_client_class', autospec=True)
Expand Down Expand Up @@ -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']['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)
Expand Down