Skip to content

{Core} az login: Add error handling when finding subscriptions #15776

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

Merged
evelyn-ys merged 3 commits into from
Nov 11, 2020
Merged

{Core} az login: Add error handling when finding subscriptions #15776

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bc51668
Add error handling when finding subscriptions
evelyn-ys Nov 10, 2020
029d501
exception handler
evelyn-ys Nov 11, 2020
91579bb
pylint
evelyn-ys Nov 11, 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
38 changes: 30 additions & 8 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 @@ -24,7 +24,6 @@
is_windows, is_wsl
from azure.cli.core.cloud import get_active_cloud, set_cloud_subscription


logger = get_logger(__name__)

# Names below are used by azure-xplat-cli to persist account information into
Expand Down Expand Up @@ -825,7 +824,11 @@ def create_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)
if client_type is None:
raise CLIInternalError("Unable to get '{}' in profile '{}'"
.format(ResourceType.MGMT_RESOURCE_SUBSCRIPTIONS, cli_ctx.cloud.profile))
Comment on lines 828 to +831
Copy link
Member Author

@evelyn-ys evelyn-ys Nov 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

For some weird cases(CX upgrade or degrade MGMT_RESOURCE_SUBSCRIPTIONS themselves, I guess.), get_client_class will fail and return None. If not verified, line 829 will throw TypeError: _NoneType_ object is not callable.
@jiasli Any idea on the real reason?

Copy link
Contributor

@houk-ms houk-ms Nov 5, 2020

Choose a reason for hiding this comment

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

I am ok with it, currently.

But we may need go deeper to see what are the exact reasons that cause the errrors and provide the corresponding suggestions. Because our goal is to resolve all the CLIInternalErrors

Copy link
Member

@jiasli jiasli Nov 10, 2020

Choose a reason for hiding this comment

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

This is because get_client_class returned None. In such case azure-mgmt-resource SDK is somehow missing.

get_client_class should actually raise an Exception instead of returning None. This is a design flaw and need further discussion.

api_version = get_api_version(cli_ctx, ResourceType.MGMT_RESOURCE_SUBSCRIPTIONS)
client = client_type(credentials, api_version=api_version,
base_url=self.cli_ctx.cloud.endpoints.resource_manager)
Expand All @@ -837,10 +840,13 @@ def create_arm_client_factory(credentials):

def find_from_user_account(self, username, password, tenant, resource):
context = self._create_auth_context(tenant)
if password:
token_entry = context.acquire_token_with_username_password(resource, username, password, _CLIENT_ID)
else: # when refresh account, we will leverage local cached tokens
token_entry = context.acquire_token(resource, username, _CLIENT_ID)
try:
if password:
token_entry = context.acquire_token_with_username_password(resource, username, password, _CLIENT_ID)
else: # when refresh account, we will leverage local cached tokens
token_entry = context.acquire_token(resource, username, _CLIENT_ID)
except Exception as err: # pylint: disable=broad-except
_login_exception_handler(err)

if not token_entry:
return []
Expand All @@ -861,8 +867,11 @@ def find_through_authorization_code_flow(self, tenant, resource, authority_url):

# exchange the code for the token
context = self._create_auth_context(tenant)
token_entry = context.acquire_token_with_authorization_code(results['code'], results['reply_url'],
resource, _CLIENT_ID, None)
try:
token_entry = context.acquire_token_with_authorization_code(results['code'], results['reply_url'],
resource, _CLIENT_ID, None)
except Exception as err: # pylint: disable=broad-except
_login_exception_handler(err)
self.user_id = token_entry[_TOKEN_ENTRY_USER_ID]
logger.warning("You have logged in. Now let us find all the subscriptions to which you have access...")
if tenant is None:
Expand All @@ -873,7 +882,10 @@ def find_through_authorization_code_flow(self, tenant, resource, authority_url):

def find_through_interactive_flow(self, tenant, resource):
context = self._create_auth_context(tenant)
code = context.acquire_user_code(resource, _CLIENT_ID)
try:
code = context.acquire_user_code(resource, _CLIENT_ID)
except Exception as err: # pylint: disable=broad-except
_login_exception_handler(err)
logger.warning(code['message'])
token_entry = context.acquire_token_with_device_code(resource, code, _CLIENT_ID)
self.user_id = token_entry[_TOKEN_ENTRY_USER_ID]
Expand Down Expand Up @@ -1331,3 +1343,13 @@ def _get_authorization_code(resource, authority_url):
if results.get('no_browser'):
raise RuntimeError()
return results


def _login_exception_handler(ex):
from requests.exceptions import InvalidURL
if isinstance(ex, InvalidURL):
import traceback
from azure.cli.core.azclierror import UnknownError
logger.debug('Invalid url when acquiring token\n%s', traceback.format_exc())
raise UnknownError(error_msg='Invalid url when acquiring token',
recommendation='Please make sure the cloud is registered with valid url')
10 changes: 7 additions & 3 deletions src/azure-cli-core/azure/cli/core/adal_authentication.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,9 +119,13 @@ def set_token(self):
except requests.exceptions.HTTPError as err:
logger.debug('throw requests.exceptions.HTTPError when doing MSIAuthentication: \n%s',
traceback.format_exc())
raise AzureResponseError('Failed to connect to MSI. Please make sure MSI is configured correctly.\n'
'Get Token request returned http error: {}, reason: {}'
.format(err.response.status, err.response.reason))
try:
raise AzureResponseError('Failed to connect to MSI. Please make sure MSI is configured correctly.\n'
'Get Token request returned http error: {}, reason: {}'
.format(err.response.status, err.response.reason))
except AttributeError:
raise AzureResponseError('Failed to connect to MSI. Please make sure MSI is configured correctly.\n'
'Get Token request returned: {}'.format(err.response))
except TimeoutError as err:
logger.debug('throw TimeoutError when doing MSIAuthentication: \n%s',
traceback.format_exc())
Expand Down
35 changes: 35 additions & 0 deletions 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 @@ -316,6 +316,15 @@ def test_subscription_finder_constructor(self, get_api_mock):
result = finder._arm_client_factory(mock.MagicMock())
self.assertEqual(result.config.base_url, 'http://foo_arm')

@mock.patch('azure.cli.core.profiles._shared.get_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
finder = SubscriptionFinder(cli, None, None, arm_client_factory=None)
from azure.cli.core.azclierror import CLIInternalError
with self.assertRaisesRegexp(CLIInternalError, 'Unable to get'):
finder._arm_client_factory(mock.MagicMock())

@mock.patch('adal.AuthenticationContext', autospec=True)
def test_get_auth_info_for_logged_in_service_principal(self, mock_auth_context):
cli = DummyCli()
Expand Down Expand Up @@ -1037,6 +1046,32 @@ def test_find_subscriptions_thru_username_non_password(self, mock_auth_context):
# assert
self.assertEqual([], subs)

@mock.patch('adal.AuthenticationContext', autospec=True)
@mock.patch('azure.cli.core._profile._get_authorization_code', autospec=True)
def test_find_subscriptions_with_invalid_authority_url(self, _get_authorization_code_mock, mock_auth_context):
from requests.exceptions import InvalidURL
from azure.cli.core.azclierror import UnknownError

def mock_acquire(*args, **kwargs):
raise InvalidURL(request='http://some.unknown.endpoints')

cli = DummyCli()
mock_auth_context.acquire_token_with_username_password.side_effect = mock_acquire
mock_auth_context.acquire_token_with_authorization_code.side_effect = mock_acquire
mock_auth_context.acquire_user_code.side_effect = mock_acquire
_get_authorization_code_mock.return_value = {
'code': 'code1',
'reply_url': 'http://localhost:8888'
}

finder = SubscriptionFinder(cli, lambda _, _1, _2: mock_auth_context, None, lambda _: None)
with self.assertRaisesRegexp(UnknownError, 'Invalid url when acquiring token'):
finder.find_from_user_account(self.user1, 'bar', None, 'http://goo-resource')
with self.assertRaisesRegexp(UnknownError, 'Invalid url when acquiring token'):
finder.find_through_authorization_code_flow(None, 'https://management.core.windows.net/', 'https:/some_aad_point/common')
with self.assertRaisesRegexp(UnknownError, 'Invalid url when acquiring token'):
finder.find_through_interactive_flow(None, 'https://management.core.windows.net/')

@mock.patch('azure.cli.core.adal_authentication.MSIAuthenticationWrapper', autospec=True)
@mock.patch('azure.cli.core.profiles._shared.get_client_class', autospec=True)
@mock.patch('azure.cli.core._profile._get_cloud_console_token_endpoint', autospec=True)
Expand Down