Skip to content
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
5 changes: 3 additions & 2 deletions .flake8
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,12 @@ ignore =
per-file-ignores =
# module level import not at top of file, elevate timer to measure import time
src/azure-cli/azure/cli/__main__.py:E402
exclude =
exclude =
azure_bdist_wheel.py
build
tools
scripts
doc
build_scripts
*/grammar/
*/grammar/
vendored_sdks
59 changes: 42 additions & 17 deletions src/azure-cli-core/azure/cli/core/_profile.py
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@

_AZ_LOGIN_MESSAGE = "Please run 'az login' to setup account."

_USE_VENDORED_SUBSCRIPTION_SDK = True
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to change this to False in the future ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably not. azure-cli-core uses only very limited functionalities from this SDK, so we can always use the fixed version.



def load_subscriptions(cli_ctx, all_clouds=False, refresh=False):
profile = Profile(cli_ctx=cli_ctx)
Expand Down Expand Up @@ -258,7 +260,7 @@ def _normalize_properties(self, user, subscriptions, is_service_principal, cert_
subscription_dict = {
_SUBSCRIPTION_ID: s.id.rpartition('/')[2],
_SUBSCRIPTION_NAME: display_name,
_STATE: s.state.value,
_STATE: s.state,
_USER_ENTITY: {
_USER_NAME: user,
_USER_TYPE: _SERVICE_PRINCIPAL if is_service_principal else _USER
Expand Down Expand Up @@ -303,11 +305,17 @@ def _build_tenant_level_accounts(self, tenants):
return result

def _new_account(self):
from azure.cli.core.profiles import ResourceType, get_sdk
SubscriptionType, StateType = get_sdk(self.cli_ctx, ResourceType.MGMT_RESOURCE_SUBSCRIPTIONS, 'Subscription',
'SubscriptionState', mod='models')
"""Build an empty Subscription which will be used as a tenant account.
API version doesn't matter as only specified attributes are preserved by _normalize_properties."""
if _USE_VENDORED_SUBSCRIPTION_SDK:
from azure.cli.core.vendored_sdks.subscriptions.models import Subscription
SubscriptionType = Subscription
else:
from azure.cli.core.profiles import ResourceType, get_sdk
SubscriptionType = get_sdk(self.cli_ctx, ResourceType.MGMT_RESOURCE_SUBSCRIPTIONS,
'Subscription', mod='models')
s = SubscriptionType()
s.state = StateType.enabled
s.state = 'Enabled'
Copy link
Member Author

@jiasli jiasli Nov 16, 2020

Choose a reason for hiding this comment

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

Track 2 uses str instead of Enum SubscriptionState as the value when deserializing Subscription objects, so we do the same.

Track 1:

https://github.com/Azure/azure-sdk-for-python/blob/azure-mgmt-resource_10.2.0/sdk/resources/azure-mgmt-resource/azure/mgmt/resource/subscriptions/v2019_11_01/models/_models_py3.py#L390

'state': {'key': 'state', 'type': 'SubscriptionState'},

Track 2:

https://github.com/Azure/azure-sdk-for-python/blob/azure-mgmt-resource_15.0.0/sdk/resources/azure-mgmt-resource/azure/mgmt/resource/subscriptions/v2019_11_01/models/_models_py3.py#L453

'state': {'key': 'state', 'type': 'str'},

See email: A breaking change in Enum property's deserialization

return s

def find_subscriptions_in_vm_with_msi(self, identity_id=None, allow_no_subscriptions=None):
Expand Down Expand Up @@ -449,8 +457,7 @@ def _match_account(account, subscription_id, secondary_key_name, secondary_key_v

@staticmethod
def _pick_working_subscription(subscriptions):
from azure.mgmt.resource.subscriptions.models import SubscriptionState
s = next((x for x in subscriptions if x.get(_STATE) == SubscriptionState.enabled.value), None)
s = next((x for x in subscriptions if x.get(_STATE) == 'Enabled'), None)
Copy link
Member Author

Choose a reason for hiding this comment

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

return s or subscriptions[0]

def is_tenant_level_account(self):
Expand Down Expand Up @@ -821,18 +828,19 @@ def __init__(self, cli_ctx, auth_context_factory, adal_token_cache, arm_client_f
def create_arm_client_factory(credentials):
if arm_client_factory:
return arm_client_factory(credentials)
from azure.cli.core.profiles._shared import get_client_class
from azure.cli.core.profiles import ResourceType, get_api_version
from azure.cli.core.commands.client_factory import configure_common_settings
from azure.cli.core.azclierror import CLIInternalError
client_type = get_client_class(ResourceType.MGMT_RESOURCE_SUBSCRIPTIONS)
from azure.cli.core.commands.client_factory import _prepare_client_kwargs_track2

client_type = self._get_subscription_client_class()
if client_type is None:
from azure.cli.core.azclierror import CLIInternalError
raise CLIInternalError("Unable to get '{}' in profile '{}'"
.format(ResourceType.MGMT_RESOURCE_SUBSCRIPTIONS, cli_ctx.cloud.profile))
api_version = get_api_version(cli_ctx, ResourceType.MGMT_RESOURCE_SUBSCRIPTIONS)
client_kwargs = _prepare_client_kwargs_track2(cli_ctx)
# We don't need to change credential_scopes as 'scopes' is ignored by BasicTokenCredential anyway
client = client_type(credentials, api_version=api_version,
base_url=self.cli_ctx.cloud.endpoints.resource_manager)
Comment on lines 839 to -834
Copy link
Contributor

Choose a reason for hiding this comment

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

What api-version of Subscription is used by azure-cli, the corresponding version of vendor SDK is required for azure-cli-core, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Only 2019-11-01 and 2016-06-01 are used and they are included under vendored_sdks.

ResourceType.MGMT_RESOURCE_SUBSCRIPTIONS: '2019-11-01',

ResourceType.MGMT_RESOURCE_SUBSCRIPTIONS: '2016-06-01',

Copy link
Contributor

@zhoxing-ms zhoxing-ms Nov 17, 2020

Choose a reason for hiding this comment

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

Yes, I know it~ I mean if a new api-version of Subscription is added to azure-cli in the feture, does the vendor SDK in azure-cli-core also need to change to this new api-version, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

MGMT_RESOURCE_SUBSCRIPTIONS is now only used by azure-cli-core. We can use it for now, assuming the corresponding api-version is vendored correctly.

configure_common_settings(cli_ctx, client)
base_url=self.cli_ctx.cloud.endpoints.resource_manager, **client_kwargs)
return client

self._arm_client_factory = create_arm_client_factory
Expand Down Expand Up @@ -916,16 +924,17 @@ def _create_auth_context(self, tenant, use_token_cache=True):

def _find_using_common_tenant(self, access_token, resource):
import adal
from msrest.authentication import BasicTokenAuthentication
from azure.cli.core.adal_authentication import BasicTokenCredential

all_subscriptions = []
empty_tenants = []
mfa_tenants = []
token_credential = BasicTokenAuthentication({'access_token': access_token})
token_credential = BasicTokenCredential(access_token)
client = self._arm_client_factory(token_credential)
tenants = client.tenants.list()
for t in tenants:
tenant_id = t.tenant_id
logger.debug("Finding subscriptions under tenant %s", tenant_id)
# display_name is available since /tenants?api-version=2018-06-01,
# not available in /tenants?api-version=2016-06-01
if not hasattr(t, 'display_name'):
Expand All @@ -934,6 +943,7 @@ def _find_using_common_tenant(self, access_token, resource):
t.display_name = t.additional_properties.get('displayName')
temp_context = self._create_auth_context(tenant_id)
try:
logger.debug("Acquiring a token with tenant=%s, resource=%s", tenant_id, resource)
temp_credentials = temp_context.acquire_token(resource, self.user_id, _CLIENT_ID)
except adal.AdalError as ex:
# because user creds went through the 'common' tenant, the error here must be
Expand Down Expand Up @@ -990,9 +1000,9 @@ def _find_using_common_tenant(self, access_token, resource):
return all_subscriptions

def _find_using_specific_tenant(self, tenant, access_token):
from msrest.authentication import BasicTokenAuthentication
from azure.cli.core.adal_authentication import BasicTokenCredential

token_credential = BasicTokenAuthentication({'access_token': access_token})
token_credential = BasicTokenCredential(access_token)
client = self._arm_client_factory(token_credential)
subscriptions = client.subscriptions.list()
all_subscriptions = []
Expand All @@ -1005,6 +1015,21 @@ def _find_using_specific_tenant(self, tenant, access_token):
self.tenants.append(tenant)
return all_subscriptions

def _get_subscription_client_class(self): # pylint: disable=no-self-use
"""Get the subscription client class. It can come from either the vendored SDK or public SDK, depending
on the design of architecture.
"""
if _USE_VENDORED_SUBSCRIPTION_SDK:
# Use vendered subscription SDK to decouple from `resource` command module
from azure.cli.core.vendored_sdks.subscriptions import SubscriptionClient
client_type = SubscriptionClient
else:
# Use the public SDK
from azure.cli.core.profiles import ResourceType
from azure.cli.core.profiles._shared import get_client_class
client_type = get_client_class(ResourceType.MGMT_RESOURCE_SUBSCRIPTIONS)
return client_type


class CredsCache:
'''Caches AAD tokena and service principal secrets, and persistence will
Expand Down
13 changes: 13 additions & 0 deletions src/azure-cli-core/azure/cli/core/adal_authentication.py
Original file line number Diff line number Diff line change
Expand Up @@ -171,3 +171,16 @@ def _try_scopes_to_resource(scopes):

# Exactly only one scope is provided
return scopes_to_resource(scopes)


class BasicTokenCredential:
# pylint:disable=too-few-public-methods
"""A Track 2 implementation of msrest.authentication.BasicTokenAuthentication.
This credential shouldn't be used by any command module, expect azure-cli-core.
"""
def __init__(self, access_token):
self.access_token = access_token

def get_token(self, *scopes, **kwargs): # pylint:disable=unused-argument
# Because get_token can't refresh the access token, always mark the token as unexpired
return AccessToken(self.access_token, int(time.time() + 3600))
Copy link
Member

@jsntcy jsntcy Nov 18, 2020

Choose a reason for hiding this comment

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

Can we use a meaningful constant instead of a magic number (3600) so that reviewers can understand it easier?@[email protected]

Copy link
Member Author

Choose a reason for hiding this comment

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

As self.access_token is a fixed value, get_token cannot refresh the token, thus defeating the purpose of returning a valid expires_on. Using time.time() + 3600 will force SDK to use the provided token, regardless of whether the token is expired or not.

Also, Subscriptions - List and Tenants - List APIs are not long-running operations, so the time of expires_on will not be reached anyway.

23 changes: 14 additions & 9 deletions src/azure-cli-core/azure/cli/core/tests/test_profile.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,16 @@
from copy import deepcopy

from adal import AdalError
from azure.mgmt.resource.subscriptions.models import \
(SubscriptionState, Subscription, SubscriptionPolicies, SpendingLimit, ManagedByTenant)

from azure.cli.core._profile import (Profile, CredsCache, SubscriptionFinder,
ServicePrincipalAuth, _AUTH_CTX_FACTORY)
ServicePrincipalAuth, _AUTH_CTX_FACTORY, _USE_VENDORED_SUBSCRIPTION_SDK)
if _USE_VENDORED_SUBSCRIPTION_SDK:
from azure.cli.core.vendored_sdks.subscriptions.models import \
(SubscriptionState, Subscription, SubscriptionPolicies, SpendingLimit, ManagedByTenant)
else:
from azure.mgmt.resource.subscriptions.models import \
(SubscriptionState, Subscription, SubscriptionPolicies, SpendingLimit, ManagedByTenant)

from azure.cli.core.mock import DummyCli

from knack.util import CLIError
Expand Down Expand Up @@ -314,9 +319,9 @@ def test_subscription_finder_constructor(self, get_api_mock):
cli.cloud.endpoints.resource_manager = 'http://foo_arm'
finder = SubscriptionFinder(cli, None, None, arm_client_factory=None)
result = finder._arm_client_factory(mock.MagicMock())
self.assertEqual(result.config.base_url, 'http://foo_arm')
self.assertEqual(result._client._base_url, 'http://foo_arm')

@mock.patch('azure.cli.core.profiles._shared.get_client_class', autospec=True)
@mock.patch('azure.cli.core._profile.SubscriptionFinder._get_subscription_client_class', autospec=True)
def test_subscription_finder_fail_on_arm_client_factory(self, get_client_class_mock):
cli = DummyCli()
get_client_class_mock.return_value = None
Expand Down Expand Up @@ -1107,7 +1112,7 @@ def find_from_raw_token(self, tenant, token):
self.assertEqual(s['id'], self.id1.split('/')[-1])

@mock.patch('requests.get', autospec=True)
@mock.patch('azure.cli.core.profiles._shared.get_client_class', autospec=True)
@mock.patch('azure.cli.core._profile.SubscriptionFinder._get_subscription_client_class', autospec=True)
def test_find_subscriptions_in_vm_with_msi_system_assigned(self, mock_get_client_class, mock_get):

class ClientStub:
Expand Down Expand Up @@ -1145,7 +1150,7 @@ def __init__(self, *args, **kwargs):
self.assertEqual(s['tenantId'], '54826b22-38d6-4fb2-bad9-b7b93a3e9c5a')

@mock.patch('requests.get', autospec=True)
@mock.patch('azure.cli.core.profiles._shared.get_client_class', autospec=True)
@mock.patch('azure.cli.core._profile.SubscriptionFinder._get_subscription_client_class', autospec=True)
def test_find_subscriptions_in_vm_with_msi_no_subscriptions(self, mock_get_client_class, mock_get):

class ClientStub:
Expand Down Expand Up @@ -1183,7 +1188,7 @@ def __init__(self, *args, **kwargs):
self.assertEqual(s['tenantId'], self.test_msi_tenant)

@mock.patch('requests.get', autospec=True)
@mock.patch('azure.cli.core.profiles._shared.get_client_class', autospec=True)
@mock.patch('azure.cli.core._profile.SubscriptionFinder._get_subscription_client_class', autospec=True)
def test_find_subscriptions_in_vm_with_msi_user_assigned_with_client_id(self, mock_get_client_class, mock_get):

class ClientStub:
Expand Down Expand Up @@ -1269,7 +1274,7 @@ def set_token(self):
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)
@mock.patch('azure.cli.core._profile.SubscriptionFinder._get_subscription_client_class', autospec=True)
def test_find_subscriptions_in_vm_with_msi_user_assigned_with_res_id(self, mock_get_client_class, mock_get):

class ClientStub:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,17 @@
from copy import deepcopy

from adal import AdalError
from azure.mgmt.resource.subscriptions.v2016_06_01.models import \
(SubscriptionState, Subscription, SubscriptionPolicies, SpendingLimit)

from azure.cli.core._profile import (Profile, CredsCache, SubscriptionFinder,
ServicePrincipalAuth, _AUTH_CTX_FACTORY)
ServicePrincipalAuth, _AUTH_CTX_FACTORY, _USE_VENDORED_SUBSCRIPTION_SDK)

if _USE_VENDORED_SUBSCRIPTION_SDK:
from azure.cli.core.vendored_sdks.subscriptions.v2016_06_01.models import \
(SubscriptionState, Subscription, SubscriptionPolicies, SpendingLimit)
else:
from azure.mgmt.resource.subscriptions.v2016_06_01.models import \
(SubscriptionState, Subscription, SubscriptionPolicies, SpendingLimit)

from azure.cli.core.mock import DummyCli

from knack.util import CLIError
Expand Down Expand Up @@ -294,7 +300,7 @@ def test_subscription_finder_constructor(self, get_api_mock):
cli.cloud.endpoints.resource_manager = 'http://foo_arm'
finder = SubscriptionFinder(cli, None, None, arm_client_factory=None)
result = finder._arm_client_factory(mock.MagicMock())
self.assertEqual(result.config.base_url, 'http://foo_arm')
self.assertEqual(result._client._base_url, 'http://foo_arm')

@mock.patch('adal.AuthenticationContext', autospec=True)
def test_get_auth_info_for_logged_in_service_principal(self, mock_auth_context):
Expand Down Expand Up @@ -934,7 +940,7 @@ def find_from_raw_token(self, tenant, token):
self.assertEqual(s['id'], self.id1.split('/')[-1])

@mock.patch('requests.get', autospec=True)
@mock.patch('azure.cli.core.profiles._shared.get_client_class', autospec=True)
@mock.patch('azure.cli.core._profile.SubscriptionFinder._get_subscription_client_class', autospec=True)
def test_find_subscriptions_in_vm_with_msi_system_assigned(self, mock_get_client_class, mock_get):

class ClientStub:
Expand Down Expand Up @@ -972,7 +978,7 @@ def __init__(self, *args, **kwargs):
self.assertEqual(s['tenantId'], '54826b22-38d6-4fb2-bad9-b7b93a3e9c5a')

@mock.patch('requests.get', autospec=True)
@mock.patch('azure.cli.core.profiles._shared.get_client_class', autospec=True)
@mock.patch('azure.cli.core._profile.SubscriptionFinder._get_subscription_client_class', autospec=True)
def test_find_subscriptions_in_vm_with_msi_no_subscriptions(self, mock_get_client_class, mock_get):

class ClientStub:
Expand Down Expand Up @@ -1010,7 +1016,7 @@ def __init__(self, *args, **kwargs):
self.assertEqual(s['tenantId'], self.test_msi_tenant)

@mock.patch('requests.get', autospec=True)
@mock.patch('azure.cli.core.profiles._shared.get_client_class', autospec=True)
@mock.patch('azure.cli.core._profile.SubscriptionFinder._get_subscription_client_class', autospec=True)
def test_find_subscriptions_in_vm_with_msi_user_assigned_with_client_id(self, mock_get_client_class, mock_get):

class ClientStub:
Expand Down Expand Up @@ -1096,7 +1102,7 @@ def set_token(self):
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)
@mock.patch('azure.cli.core._profile.SubscriptionFinder._get_subscription_client_class', autospec=True)
def test_find_subscriptions_in_vm_with_msi_user_assigned_with_res_id(self, mock_get_client_class, mock_get):

class ClientStub:
Expand Down
7 changes: 7 additions & 0 deletions src/azure-cli-core/azure/cli/core/vendored_sdks/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# coding=utf-8
# --------------------------------------------------------------------------
# Copyright (c) Microsoft Corporation. All rights reserved.
# Licensed under the MIT License. See License.txt in the project root for license information.
# Code generated by Microsoft (R) AutoRest Code Generator.
# Changes may cause incorrect behavior and will be lost if the code is regenerated.
# --------------------------------------------------------------------------
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# coding=utf-8
# --------------------------------------------------------------------------
# Copyright (c) Microsoft Corporation. All rights reserved.
# Licensed under the MIT License. See License.txt in the project root for license information.
# Code generated by Microsoft (R) AutoRest Code Generator.
# Changes may cause incorrect behavior and will be lost if the code is regenerated.
# --------------------------------------------------------------------------

from ._subscription_client import SubscriptionClient
__all__ = ['SubscriptionClient']

try:
from ._patch import patch_sdk # type: ignore
patch_sdk()
except ImportError:
pass
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
# coding=utf-8
# --------------------------------------------------------------------------
# Copyright (c) Microsoft Corporation. All rights reserved.
# Licensed under the MIT License. See License.txt in the project root for
# license information.
#
# Code generated by Microsoft (R) AutoRest Code Generator.
# Changes may cause incorrect behavior and will be lost if the code is
# regenerated.
# --------------------------------------------------------------------------
from typing import Any

from azure.core.configuration import Configuration
from azure.core.pipeline import policies
from azure.mgmt.core.policies import ARMHttpLoggingPolicy

from ._version import VERSION


class SubscriptionClientConfiguration(Configuration):
"""Configuration for SubscriptionClient.

Note that all parameters used to create this instance are saved as instance
attributes.

:param credential: Credential needed for the client to connect to Azure.
:type credential: ~azure.core.credentials.TokenCredential
"""

def __init__(
self,
credential, # type: "TokenCredential"
**kwargs # type: Any
):
# type: (...) -> None
if credential is None:
raise ValueError("Parameter 'credential' must not be None.")
super(SubscriptionClientConfiguration, self).__init__(**kwargs)

self.credential = credential
self.credential_scopes = kwargs.pop('credential_scopes', ['https://management.azure.com/.default'])
kwargs.setdefault('sdk_moniker', 'azure-mgmt-resource/{}'.format(VERSION))
self._configure(**kwargs)

def _configure(
self,
**kwargs # type: Any
):
# type: (...) -> None
self.user_agent_policy = kwargs.get('user_agent_policy') or policies.UserAgentPolicy(**kwargs)
self.headers_policy = kwargs.get('headers_policy') or policies.HeadersPolicy(**kwargs)
self.proxy_policy = kwargs.get('proxy_policy') or policies.ProxyPolicy(**kwargs)
self.logging_policy = kwargs.get('logging_policy') or policies.NetworkTraceLoggingPolicy(**kwargs)
self.http_logging_policy = kwargs.get('http_logging_policy') or ARMHttpLoggingPolicy(**kwargs)
self.retry_policy = kwargs.get('retry_policy') or policies.RetryPolicy(**kwargs)
self.custom_hook_policy = kwargs.get('custom_hook_policy') or policies.CustomHookPolicy(**kwargs)
self.redirect_policy = kwargs.get('redirect_policy') or policies.RedirectPolicy(**kwargs)
self.authentication_policy = kwargs.get('authentication_policy')
if self.credential and not self.authentication_policy:
self.authentication_policy = policies.BearerTokenCredentialPolicy(self.credential, *self.credential_scopes, **kwargs)
Loading