Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 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
37 changes: 34 additions & 3 deletions src/azure-cli-core/azure/cli/core/_profile.py
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ def find_subscriptions_on_login(self, # pylint: disable=too-many-arguments
password,
is_service_principal,
tenant,
allow_no_subscriptions=False,
subscription_finder=None):
from azure.cli.core._debug import allow_debug_adal_connection
allow_debug_adal_connection()
Expand All @@ -128,18 +129,26 @@ def find_subscriptions_on_login(self, # pylint: disable=too-many-arguments
subscriptions = subscription_finder.find_from_user_account(
username, password, tenant, self._ad_resource_uri)

if not subscriptions:
raise CLIError('No subscriptions found for this account.')
if not allow_no_subscriptions and not subscriptions:
raise CLIError("No subscriptions were found for '{}'. If this is expected, use "
"'--allow-no-subscriptions' to have a tenant level account".format(
username))
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need an explicit flag for this? If there are subscriptions but --allow-no-subscriptions is specified, what happens? If the behavior isn't changed, then why wouldn't we just issue a warning and proceed with the --allow-no-subcriptions logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a bit subtle.
--allow-no-subscriptions can be used even if you have subscriptions. The story is, login will visit every tenants you are member of. Let us say you have [t1,t2], and there is a subscription s1 in t1, but not any in the t2. W/o this flag, the login will only build an account for t1\s1, which makes sense because almost all commands need to have subscriptions. But for a few commands which doesn't need subscriptions, say those ad ones, users have opened the issue saying they like to run them on t2.
I don't like to build up the tenant level accounts by default, because more likely people don't want it. That is particularly true for RBAC scenarios, that it is common people by mistake forgot to create the role assignment, causing subscription access is not built up. I am open to incoming user voices though, and can switch if needed.
Also, let me know if you have suggestions for a better arg name.


if is_service_principal:
self._creds_cache.save_service_principal_cred(sp_auth.get_entry_to_persist(username,
tenant))

if self._creds_cache.adal_token_cache.has_state_changed:
self._creds_cache.persist_cached_creds()

if allow_no_subscriptions:
t_list = [s.tenant_id for s in subscriptions]
bare_tenants = [t for t in subscription_finder.tenants if t not in t_list]
subscriptions = Profile._build_tenant_level_accounts(bare_tenants)

consolidated = Profile._normalize_properties(subscription_finder.user_id,
subscriptions,
is_service_principal)

self._set_subscriptions(consolidated)
# use deepcopy as we don't want to persist these changes to file.
return deepcopy(consolidated)
Expand All @@ -162,6 +171,24 @@ def _normalize_properties(user, subscriptions, is_service_principal):
})
return consolidated

@staticmethod
def _build_tenant_level_accounts(tenants):
from azure.cli.core.profiles import get_sdk, ResourceType
SubscriptionType = get_sdk(ResourceType.MGMT_RESOURCE_SUBSCRIPTIONS,
'Subscription', mod='models')
StateType = get_sdk(ResourceType.MGMT_RESOURCE_SUBSCRIPTIONS,
'SubscriptionState', mod='models')
result = []
for t in tenants:
s = SubscriptionType()
s.id = '/subscriptions/' + t
s.subscription = t
s.tenant_id = t
s.display_name = 'N/A(tenant level account)'
s.state = StateType.enabled
result.append(s)
return result

def _set_subscriptions(self, new_subscriptions):
existing_ones = self.load_cached_subscriptions(all_clouds=True)
active_one = next((x for x in existing_ones if x.get(_IS_DEFAULT_SUBSCRIPTION)), None)
Expand Down Expand Up @@ -345,6 +372,7 @@ def create_arm_client_factory(config):
config, base_url=CLOUD.endpoints.resource_manager))

self._arm_client_factory = create_arm_client_factory
self.tenants = []

def find_from_user_account(self, username, password, tenant, resource):
context = self._create_auth_context(tenant or _COMMON_TENANT)
Expand Down Expand Up @@ -379,6 +407,7 @@ def find_from_service_principal_id(self, client_id, sp_auth, tenant, resource):
token_entry = sp_auth.acquire_token(context, resource, client_id)
self.user_id = client_id
result = self._find_using_specific_tenant(tenant, token_entry[_ACCESS_TOKEN])
self.tenants = [tenant]
return result

def _create_auth_context(self, tenant, use_token_cache=True):
Expand Down Expand Up @@ -410,6 +439,7 @@ def _find_using_common_tenant(self, access_token, resource):
temp_credentials[_ACCESS_TOKEN])
all_subscriptions.extend(subscriptions)

self.tenants = tenants
return all_subscriptions

def _find_using_specific_tenant(self, tenant, access_token):
Expand All @@ -422,6 +452,7 @@ def _find_using_specific_tenant(self, tenant, access_token):
for s in subscriptions:
setattr(s, 'tenant_id', tenant)
all_subscriptions.append(s)
self.tenants = [tenant]
return all_subscriptions


Expand Down
30 changes: 30 additions & 0 deletions src/azure-cli-core/tests/test_profile.py
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,7 @@ def test_get_expanded_subscription_info_for_logged_in_service_principal(self,
'my-secret',
True,
self.tenant_id,
False,
finder)
# action
extended_info = profile.get_expanded_subscription_info()
Expand All @@ -253,6 +254,35 @@ def test_get_expanded_subscription_info_for_logged_in_service_principal(self,
self.assertEqual('https://login.microsoftonline.com',
extended_info['endpoints'].active_directory)

@mock.patch('adal.AuthenticationContext', autospec=True)
def test_create_account_without_subscriptions(self, mock_auth_context):
mock_auth_context.acquire_token_with_client_credentials.return_value = self.token_entry1
mock_arm_client = mock.MagicMock()
mock_arm_client.subscriptions.list.return_value = []
finder = SubscriptionFinder(lambda _, _2: mock_auth_context,
None,
lambda _: mock_arm_client)

storage_mock = {'subscriptions': []}
profile = Profile(storage_mock)
profile._management_resource_uri = 'https://management.core.windows.net/'

# action
result = profile.find_subscriptions_on_login(False,
'1234',
'my-secret',
True,
self.tenant_id,
allow_no_subscriptions=True,
subscription_finder=finder)

# assert
self.assertTrue(1, len(result))
self.assertEqual(result[0]['id'], self.tenant_id)
self.assertEqual(result[0]['state'], 'Enabled')
self.assertEqual(result[0]['tenantId'], self.tenant_id)
self.assertEqual(result[0]['name'], 'N/A(tenant level account)')

@mock.patch('azure.cli.core._profile._load_tokens_from_file', autospec=True)
def test_get_current_account_user(self, mock_read_cred_file):
# setup
Expand Down
3 changes: 3 additions & 0 deletions src/command_modules/azure-cli-profile/HISTORY.rst
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

Release History
===============
2.0.4 (unreleased)
++++++++++++++++++
* Support login when there are no subscriptions found (#2560)

2.0.3 (2017-04-17)
++++++++++++++++++
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ def get_subscription_id_list(prefix, **kwargs): # pylint: disable=unused-argume
register_cli_argument('login', 'service_principal', action='store_true', help='The credential representing a service principal.')
register_cli_argument('login', 'username', options_list=('--username', '-u'), help='Organization id or service principal')
register_cli_argument('login', 'tenant', options_list=('--tenant', '-t'), help='The AAD tenant, must provide when using service principals.')
register_cli_argument('login', 'allow_no_subscriptions', action='store_true', help='Support login for tenants without subscriptions. This is uncommon')

register_cli_argument('logout', 'username', help='account user, if missing, logout the current active account')

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,8 @@ def account_clear():
profile.logout_all()


def login(username=None, password=None, service_principal=None, tenant=None):
def login(username=None, password=None, service_principal=None, tenant=None,
allow_no_subscriptions=False):
"""Log in to access Azure subscriptions"""
from adal.adal_error import AdalError
import requests
Expand All @@ -76,7 +77,8 @@ def login(username=None, password=None, service_principal=None, tenant=None):
username,
password,
service_principal,
tenant)
tenant,
allow_no_subscriptions=allow_no_subscriptions)
except AdalError as err:
# try polish unfriendly server errors
if username:
Expand Down