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.

Sign up for GitHub

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][Profile] Add tenant parameter to get_raw_token #11798

Merged
merged 14 commits into from
Jan 30, 2020
Merged

[Core][Profile] Add tenant parameter to get_raw_token #11798

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
fbfe691
add tenant to get_raw_token
jiasli Jan 8, 2020
7722eb7
use assertRaisesRegexp to pass py27 CI
jiasli Jan 9, 2020
cd790da
fix test
jiasli Jan 9, 2020
20ff1d4
Merge branch 'dev' into tenant-token
jiasli Jan 15, 2020
0a61ed4
auto-switch tenant for sp
jiasli Jan 17, 2020
4243726
fix linter
jiasli Jan 17, 2020
9f52ccd
add examples
jiasli Jan 17, 2020
15e8062
fix linter
jiasli Jan 17, 2020
16d2ae8
add test for tenant not matching scenario
jiasli Jan 20, 2020
bc4dff2
for tenant token do not add subscription
jiasli Jan 20, 2020
e96810e
Merge branch 'dev' into tenant-token
jiasli Jan 20, 2020
ed68f10
Merge branch 'dev' into tenant-token
jiasli Jan 20, 2020
b18bb11
Merge branch 'dev' into tenant-token
jiasli Jan 30, 2020
19509df
Merge branch 'dev' into tenant-token
jiasli Jan 30, 2020
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: 2 additions & 0 deletions src/azure-cli-core/HISTORY.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
Release History
===============

* `get_raw_token`: Add `tenant` parameter to acquire token for the tenant directly, needless to specify a subscription

2.0.80
++++++
* No changes
Expand Down
31 changes: 21 additions & 10 deletions src/azure-cli-core/azure/cli/core/_profile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -587,31 +587,42 @@ def get_refresh_token(self, resource=None,
sp_secret = self._creds_cache.retrieve_secret_of_service_principal(username_or_sp_id)
return username_or_sp_id, sp_secret, None, str(account[_TENANT_ID])

def get_raw_token(self, resource=None, subscription=None):
def get_raw_token(self, resource=None, subscription=None, tenant=None):
if subscription and tenant:
raise CLIError("Please specify only one of subscription and tenant, not both")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this error message pattern seems not align with existing pattern like "subscription shouldn't e specified when tenant is specified"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This expression is used by many other modules. You may Find All with "not both" to see other usage.

account = self.get_subscription(subscription)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if subscription is not specified, account is empty? then can we get the user_type below?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If subscription is not specified, it retrieves the default subscription which is the one that is currently selected by az account set.

user_type = account[_USER_ENTITY][_USER_TYPE]
username_or_sp_id = account[_USER_ENTITY][_USER_NAME]
resource = resource or self.cli_ctx.cloud.endpoints.active_directory_resource_id

identity_type, identity_id = Profile._try_parse_msi_account_name(account)
if identity_type:
# MSI
if tenant:
raise CLIError("Tenant shouldn't be specified for MSI account")
msi_creds = MsiAccountTypes.msi_auth_factory(identity_type, identity_id, resource)
msi_creds.set_token()
token_entry = msi_creds.token
creds = (token_entry['token_type'], token_entry['access_token'], token_entry)
elif in_cloud_console() and account[_USER_ENTITY].get(_CLOUD_SHELL_ID):
# Cloud Shell
if tenant:
raise CLIError("Tenant shouldn't be specified for Cloud Shell account")
creds = self._get_token_from_cloud_shell(resource)

elif user_type == _USER:
creds = self._creds_cache.retrieve_token_for_user(username_or_sp_id,
account[_TENANT_ID], resource)
else:
creds = self._creds_cache.retrieve_token_for_service_principal(username_or_sp_id,
resource,
account[_TENANT_ID])
tenant_dest = tenant if tenant else account[_TENANT_ID]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when subscription is specified, it is checked if exist in cached file in function "get_subscription", is there anyway we can have a pre-check before send retrieve token request?

Copy link
Member Author

@jiasli jiasli Jan 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a user account we don't need to check if the tenant is in az account list, as the refreshToken in accessTokens.json is the same for all tenants. user : refresh token is a 1:1 mapping. The refresh token can be used to retrieve a token in ANY tenant, even if the user is not a member of that tenant. The token doesn't has any permission in that tenant.

A service principal is a binding between an app and a tenant, in other words, a projection of an app in a tenant.

Home Tenant Tenant1 Tenant 2 ...
App sp_home sp_1 sp_2

Please also see Application and service principal objects in Azure Active Directory

In accessTokens.json, the entry for a service principal account is:

{
    "servicePrincipalId": "6827...",
    "servicePrincipalTenant": "5482...",
    "accessToken": "43b2..."
}

servicePrincipalId is usually the appId returned by az ad sp create-for-rbac.

accessToken is actually the credential(password or certificate) of the service principal or app. If the credential is at app level, it is the same across tenants; if the credential is at service principal level, it is different across tenants. So service principal : access token(credential) is a 1:+ mapping.

Even if for app credential, service principal : access token(credential) is a 1:1 mapping, we can't tell whether the credential is valid in other tenants until the AAD server returns error. I prefer not to do the bet. So to retrieve the credential, we need to use both servicePrincipalId and servicePrincipalTenant, which is searched at

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

Lines 940 to 941 in fbfe691

matched = [x for x in self._service_principal_creds if sp_id == x[_SERVICE_PRINCIPAL_ID] and
tenant == x[_SERVICE_PRINCIPAL_TENANT]]

Therefore, for a Service Principal to work across tenants, we need to run az login twice for each tenant.

if user_type == _USER:
# User
creds = self._creds_cache.retrieve_token_for_user(username_or_sp_id,
tenant_dest, resource)
else:
# Service Principal
creds = self._creds_cache.retrieve_token_for_service_principal(username_or_sp_id,
resource,
tenant_dest)
return (creds,
str(account[_SUBSCRIPTION_ID]),
str(account[_TENANT_ID]))
str('N/A' if tenant else account[_SUBSCRIPTION_ID]),
str(tenant if tenant else account[_TENANT_ID]))

def refresh_accounts(self, subscription_finder=None):
subscriptions = self.load_cached_subscriptions()
Expand Down
66 changes: 65 additions & 1 deletion src/azure-cli-core/azure/cli/core/tests/test_profile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -716,6 +716,17 @@ def test_get_raw_token(self, mock_get_token, mock_read_cred_file):
self.assertEqual(sub, '1')
self.assertEqual(tenant, self.tenant_id)

# Test get_raw_token with tenant
creds, sub, tenant = profile.get_raw_token(resource='https://foo', tenant=self.tenant_id)

self.assertEqual(creds[0], self.token_entry1['tokenType'])
Copy link
Member

@jsntcy jsntcy Jan 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also add tests for MSI and cloud shell? (check exception) #Closed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. It is already in this file.

self.assertEqual(creds[1], self.raw_token1)
self.assertEqual(creds[2]['expiresOn'], self.token_entry1['expiresOn'])
mock_get_token.assert_called_with(mock.ANY, self.user1, self.tenant_id, 'https://foo')
self.assertEqual(mock_get_token.call_count, 2)
self.assertEqual(sub, 'N/A')
self.assertEqual(tenant, self.tenant_id)

@mock.patch('azure.cli.core._profile._load_tokens_from_file', autospec=True)
@mock.patch('azure.cli.core._profile.CredsCache.retrieve_token_for_service_principal', autospec=True)
def test_get_raw_token_for_sp(self, mock_get_token, mock_read_cred_file):
Expand Down Expand Up @@ -744,6 +755,17 @@ def test_get_raw_token_for_sp(self, mock_get_token, mock_read_cred_file):
self.assertEqual(sub, '1')
self.assertEqual(tenant, self.tenant_id)

# Test get_raw_token with tenant
creds, sub, tenant = profile.get_raw_token(resource='https://foo', tenant=self.tenant_id)

self.assertEqual(creds[0], self.token_entry1['tokenType'])
self.assertEqual(creds[1], self.raw_token1)
self.assertEqual(creds[2]['expiresOn'], self.token_entry1['expiresOn'])
mock_get_token.assert_called_with(mock.ANY, 'sp1', 'https://foo', self.tenant_id)
self.assertEqual(mock_get_token.call_count, 2)
self.assertEqual(sub, 'N/A')
self.assertEqual(tenant, self.tenant_id)

@mock.patch('azure.cli.core._profile._load_tokens_from_file', autospec=True)
@mock.patch('msrestazure.azure_active_directory.MSIAuthentication', autospec=True)
def test_get_raw_token_msi_system_assigned(self, mock_msi_auth, mock_read_cred_file):
Expand All @@ -765,12 +787,54 @@ def test_get_raw_token_msi_system_assigned(self, mock_msi_auth, mock_read_cred_f
mock_msi_auth.side_effect = MSRestAzureAuthStub

# action
cred, subscription_id, _ = profile.get_raw_token(resource='http://test_resource')
cred, subscription_id, tenant_id = profile.get_raw_token(resource='http://test_resource')

# assert
self.assertEqual(subscription_id, test_subscription_id)
self.assertEqual(cred[0], 'Bearer')
self.assertEqual(cred[1], TestProfile.test_msi_access_token)
self.assertEqual(subscription_id, test_subscription_id)
self.assertEqual(tenant_id, test_tenant_id)

# verify tenant shouldn't be specified for MSI account
with self.assertRaisesRegexp(CLIError, "MSI"):
cred, subscription_id, _ = profile.get_raw_token(resource='http://test_resource', tenant=self.tenant_id)

@mock.patch('azure.cli.core._profile.in_cloud_console', autospec=True)
@mock.patch('azure.cli.core._profile._load_tokens_from_file', autospec=True)
@mock.patch('msrestazure.azure_active_directory.MSIAuthentication', autospec=True)
def test_get_raw_token_in_cloud_console(self, mock_msi_auth, mock_read_cred_file, mock_in_cloud_console):
mock_read_cred_file.return_value = []
mock_in_cloud_console.return_value = True

# setup an existing msi subscription
profile = Profile(cli_ctx=DummyCli(), storage={'subscriptions': None}, use_global_creds_cache=False,
async_persist=False)
test_subscription_id = '12345678-1bf0-4dda-aec3-cb9272f09590'
test_tenant_id = '12345678-38d6-4fb2-bad9-b7b93a3e1234'
msi_subscription = SubscriptionStub('/subscriptions/' + test_subscription_id,
self.display_name1, self.state1, test_tenant_id)
consolidated = profile._normalize_properties(self.user1,
[msi_subscription],
True)
consolidated[0]['user']['cloudShellID'] = True
profile._set_subscriptions(consolidated)

mock_msi_auth.side_effect = MSRestAzureAuthStub

# action
cred, subscription_id, tenant_id = profile.get_raw_token(resource='http://test_resource')

# assert
self.assertEqual(subscription_id, test_subscription_id)
self.assertEqual(cred[0], 'Bearer')
self.assertEqual(cred[1], TestProfile.test_msi_access_token)
self.assertEqual(subscription_id, test_subscription_id)
self.assertEqual(tenant_id, test_tenant_id)

# verify tenant shouldn't be specified for Cloud Shell account
with self.assertRaisesRegexp(CLIError, 'Cloud Shell'):
cred, subscription_id, _ = profile.get_raw_token(resource='http://test_resource', tenant=self.tenant_id)

@mock.patch('azure.cli.core._profile._load_tokens_from_file', autospec=True)
@mock.patch('azure.cli.core._profile.CredsCache.retrieve_token_for_user', autospec=True)
Expand Down
4 changes: 4 additions & 0 deletions src/azure-cli/HISTORY.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ Release History

* Fix #6371: Support filename and environment variable completion in Bash

**Profile**

* `az account get-access-token`: Add `--tenant` parameter to acquire token for the tenant directly, needless to specify a subscription

2.0.80
++++++

Expand Down
1 change: 1 addition & 0 deletions src/azure-cli/azure/cli/command_modules/profile/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ def load_arguments(self, command):

with self.argument_context('account get-access-token') as c:
c.argument('resource_type', get_enum_type(cloud_resource_types), options_list=['--resource-type'], arg_group='', help='Type of well-known resource.')
c.argument('tenant', options_list=['--tenant', '-t'], is_preview=True, help='Tenant ID for which the token is acquired. Only available for user and service principal account, not for MSI or Cloud Shell account')


COMMAND_LOADER_CLS = ProfileCommandsLoader
8 changes: 4 additions & 4 deletions src/azure-cli/azure/cli/command_modules/profile/custom.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,20 +59,20 @@ def show_subscription(cmd, subscription=None, show_auth_for_sdk=None):
print(json.dumps(profile.get_sp_auth_info(subscription), indent=2))


def get_access_token(cmd, subscription=None, resource=None, resource_type=None):
'''
def get_access_token(cmd, subscription=None, resource=None, resource_type=None, tenant=None):
"""
get AAD token to access to a specified resource
:param resource: Azure resource endpoints. Default to Azure Resource Manager
:param resource-type: Name of Azure resource endpoints. Can be used instead of resource.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please also add tenent in comment

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc string is overridden by

azure-cli/src/azure-cli/azure/cli/command_modules/profile/__init__.py

Line 78 in fbfe691

c.argument('tenant', options_list=['--tenant', '-t'], is_preview=True, help='Tenant ID for which the token is acquired. Only available for user and service principal account, not for MSI or Cloud Shell account')

I'd prefer to omit here to eliminate duplication.

Use 'az cloud show' command for other Azure resources
'''
"""
if resource is None and resource_type is not None:
endpoints_attr_name = cloud_resource_type_mappings[resource_type]
resource = getattr(cmd.cli_ctx.cloud.endpoints, endpoints_attr_name)
else:
resource = (resource or cmd.cli_ctx.cloud.endpoints.active_directory_resource_id)
profile = Profile(cli_ctx=cmd.cli_ctx)
creds, subscription, tenant = profile.get_raw_token(subscription=subscription, resource=resource)
creds, subscription, tenant = profile.get_raw_token(subscription=subscription, resource=resource, tenant=tenant)
return {
'tokenType': creds[0],
'accessToken': creds[1],
Expand Down
13 changes: 9 additions & 4 deletions src/azure-cli/azure/cli/command_modules/profile/tests/latest/test_profile_custom.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,18 +30,18 @@ def test_list_only_enabled_one(self, logger_mock, load_subscription_mock):
logger_mock.warning.assert_called_once_with(mock.ANY)

@mock.patch('azure.cli.core._profile.Profile.get_raw_token', autospec=True)
def test_get_row_token(self, get_raw_token_mcok):
def test_get_raw_token(self, get_raw_token_mock):
cmd = mock.MagicMock()
cmd.cli_ctx = DummyCli()

# arrange
get_raw_token_mcok.return_value = (['bearer', 'token123', {'expiresOn': '2100-01-01'}], 'sub123', 'tenant123')
get_raw_token_mock.return_value = (['bearer', 'token123', {'expiresOn': '2100-01-01'}], 'sub123', 'tenant123')

# action
result = get_access_token(cmd)

# assert
get_raw_token_mcok.assert_called_with(mock.ANY, 'https://management.core.windows.net/', None)
get_raw_token_mock.assert_called_with(mock.ANY, 'https://management.core.windows.net/', None, None)
expected_result = {
'tokenType': 'bearer',
'accessToken': 'token123',
Expand All @@ -53,7 +53,12 @@ def test_get_row_token(self, get_raw_token_mcok):

# assert it takes customized resource, subscription
get_access_token(cmd, subscription='foosub', resource='foores')
get_raw_token_mcok.assert_called_with(mock.ANY, 'foores', 'foosub')
get_raw_token_mock.assert_called_with(mock.ANY, 'foores', 'foosub', None)

# test get token with tenant
tenant_id = '00000000-0000-0000-0000-000000000000'
get_access_token(cmd, tenant=tenant_id)
get_raw_token_mock.assert_called_with(mock.ANY, 'https://management.core.windows.net/', None, tenant_id)

@mock.patch('azure.cli.command_modules.profile.custom.Profile', autospec=True)
def test_get_login(self, profile_mock):
Expand Down