Skip to content

[RBAC] BREAKING CHANGE: fix issue #11883 az role assignment create scope default to subscription if empty #11977

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

Closed
arrownj wants to merge 6 commits into from
Closed

[RBAC] BREAKING CHANGE: fix issue #11883 az role assignment create scope default to subscription if empty #11977

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3faa696
[Role] BREAKING CHANGE: fix issue #11883 az role assignment create sc…
arrownj Jan 22, 2020
0e905dc
[Role] BREAKING CHANGE: fix issue #11883 az role assignment create sc…
arrownj Jan 29, 2020
5fa7cf9
Merge branch 'fix/role_assignment' of github.com:arrownj/azure-cli in…
arrownj Jan 29, 2020
e4ac08b
[Role] BREAKING CHANGE: fix issue #11883 az role assignment create sc…
arrownj Jan 22, 2020
f6ced7f
Merge branch 'fix/role_assignment' of github.com:arrownj/azure-cli in…
arrownj Jan 29, 2020
06b67c0
update HISTORY.rst
arrownj Jan 29, 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
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 @@ -3,6 +3,10 @@
Release History
===============

**RBAC**

* [BREAKING CHANGE] Fix issue #11883 az role assignment create scope default to subscription if empty
Copy link
Member

@jiasli jiasli Jan 29, 2020

Choose a reason for hiding this comment

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

Please follow the history convention like:

Fix #11883: az role assignment create: scope defaults to subscription if not specified


**ACR**

* [BREAKING CHANGE] `az acr delete` will prompt
Expand Down
12 changes: 12 additions & 0 deletions src/azure-cli/azure/cli/command_modules/role/_validators.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,18 @@ def process_assignment_namespace(cmd, namespace): # pylint: disable=unused-argu
if namespace.scope and resource_group and getattr(resource_group, 'is_default', None):
namespace.resource_group_name = None # drop configured defaults

if not namespace.scope and not namespace.resource_group_name:
raise CLIError('usage error: please specify at least one of "--scope" and "--resource-group".')


def process_list_assignment_namespace(cmd, namespace): # pylint: disable=unused-argument
resource_group = namespace.resource_group_name
if namespace.scope and resource_group and getattr(resource_group, 'is_default', None):
namespace.resource_group_name = None # drop configured defaults
Comment on lines +99 to +101
Copy link
Member

@jiasli jiasli Jan 29, 2020

Choose a reason for hiding this comment

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

This is a duplicate of L90-L92. I'd suggested to extract it to a function like drop_default_group.


if not namespace.show_all and not namespace.scope and not namespace.resource_group_name:
raise CLIError('usage error: please specify at least one of "--all", "--scope" and "--resource-group".')
Copy link
Member

@jiasli jiasli Jan 29, 2020

Choose a reason for hiding this comment

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

We usually use single quotes in error messages.

Copy link
Contributor Author

@arrownj arrownj Jan 29, 2020

Choose a reason for hiding this comment

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

I searched similar error message in our code base, seems we usually use nothing in such case. The error message will be at least one of --scope and --resource-group in the new PR.

Comment on lines +103 to +104
Copy link
Member

@jiasli jiasli Jan 29, 2020

Choose a reason for hiding this comment

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

Why the behavior of az role assignment list is also changed? There doesn't seem to be any security concern in this command.

Also, it is also a breaking change to force one of --all, --scope and -resource-group, which needs to go to the history.



def process_msi_namespace(cmd, namespace):
get_default_location_from_resource_group(cmd, namespace)
Expand Down
5 changes: 3 additions & 2 deletions src/azure-cli/azure/cli/command_modules/role/commands.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@
from ._client_factory import (_auth_client_factory, _graph_client_factory,
_msi_user_identities_operations, _msi_operations_operations)

from ._validators import process_msi_namespace, process_assignment_namespace, validate_change_password
from ._validators import (process_msi_namespace, process_assignment_namespace, process_list_assignment_namespace,
validate_change_password)


def transform_definition_list(result):
Expand Down Expand Up @@ -131,7 +132,7 @@ def load_command_table(self, _):

with self.command_group('role assignment') as g:
g.custom_command('delete', 'delete_role_assignments', validator=process_assignment_namespace)
g.custom_command('list', 'list_role_assignments', validator=process_assignment_namespace, table_transformer=transform_assignment_list)
g.custom_command('list', 'list_role_assignments', validator=process_list_assignment_namespace, table_transformer=transform_assignment_list)
g.custom_command('create', 'create_role_assignment', validator=process_assignment_namespace)
g.custom_command('list-changelogs', 'list_role_assignment_change_logs')

Expand Down
8 changes: 6 additions & 2 deletions src/azure-cli/azure/cli/command_modules/role/tests/latest/test_graph.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,8 +216,10 @@ class CreateForRbacScenarioTest(ScenarioTest):

@AllowLargeResponse()
def test_revoke_sp_for_rbac(self):
subscription_id = self.get_subscription_id()
self.kwargs['display_name'] = self.create_random_name(prefix='cli-graph', length=14)
self.kwargs['name'] = 'http://' + self.kwargs['display_name']
self.kwargs['sub'] = subscription_id

with mock.patch('azure.cli.command_modules.role.custom._gen_guid', side_effect=self.create_guid):
self.cmd('ad sp create-for-rbac -n {display_name}')
Expand All @@ -226,7 +228,7 @@ def test_revoke_sp_for_rbac(self):

self.cmd('ad app list --identifier-uri {name}')

result = self.cmd('role assignment list --assignee {name}').get_output_in_json()
result = self.cmd('role assignment list --assignee {name} --scope /subscriptions/{sub}').get_output_in_json()
object_id = result[0]['principalId']

self.cmd('ad sp delete --id {name}')
Expand All @@ -241,16 +243,18 @@ def test_revoke_sp_for_rbac(self):

@AllowLargeResponse()
def test_create_for_rbac_idempotent(self):
subscription_id = self.get_subscription_id()
self.kwargs['display_name'] = self.create_random_name(prefix='sp_', length=14)
self.kwargs['name'] = 'http://' + self.kwargs['display_name']
self.kwargs['sub'] = subscription_id

with mock.patch('azure.cli.command_modules.role.custom._gen_guid', side_effect=self.create_guid):
try:
self.cmd('ad sp create-for-rbac -n {display_name}')
self.cmd('ad sp create-for-rbac -n {display_name}')
result = self.cmd('ad sp list --spn {name}').get_output_in_json()
self.assertEqual(1, len(result))
result = self.cmd('role assignment list --assignee {name}').get_output_in_json()
result = self.cmd('role assignment list --assignee {name} --scope /subscriptions/{sub}').get_output_in_json()
self.assertEqual(1, len(result))
finally:
self.cmd('ad sp delete --id {name}')
Expand Down
30 changes: 23 additions & 7 deletions src/azure-cli/azure/cli/command_modules/role/tests/latest/test_role.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import mock
import unittest

from knack.util import CLIError
from azure_devtools.scenario_tests import AllowLargeResponse, record_only
from azure.cli.core.profiles import ResourceType, get_sdk
from azure.cli.testsdk import ScenarioTest, LiveScenarioTest, ResourceGroupPreparer, KeyVaultPreparer
Expand Down Expand Up @@ -66,7 +67,7 @@ def test_create_for_rbac_with_secret_with_assignment(self, resource_group):
checks=self.check("length([])", 1))
self.cmd('role assignment delete --assignee {sp} -g {rg}',
checks=self.is_empty())
self.cmd('role assignment delete --assignee {sp}',
self.cmd('role assignment delete --assignee {sp} --scope {scope}',
checks=self.is_empty())
finally:
self.cmd('ad app delete --id {sp}')
Expand Down Expand Up @@ -242,6 +243,8 @@ def test_role_assignment_e2e(self, resource_group):
if self.run_under_service_principal():
return # this test delete users which are beyond a SP's capacity, so quit...

subscription_id = self.get_subscription_id()
self.kwargs['sub'] = subscription_id
with mock.patch('azure.cli.command_modules.role.custom._gen_guid', side_effect=self.create_guid):
user = self.create_random_name('testuser', 15)
self.kwargs.update({
Expand Down Expand Up @@ -289,15 +292,27 @@ def test_role_assignment_e2e(self, resource_group):
checks=self.is_empty())

# test role assignment on subscription level
self.cmd('role assignment create --assignee {upn} --role reader')
self.cmd('role assignment list --assignee {upn} --role reader',
self.cmd('role assignment create --assignee {upn} --role reader --scope /subscriptions/{sub}')
self.cmd('role assignment list --assignee {upn} --role reader --scope /subscriptions/{sub}',
checks=self.check("length([])", 1))
self.cmd('role assignment list --assignee {upn}',
self.cmd('role assignment list --assignee {upn} --scope /subscriptions/{sub}',
checks=self.check("length([])", 1))
self.cmd('role assignment delete --assignee {upn} --role reader')
self.cmd('role assignment delete --assignee {upn} --role reader --scope /subscriptions/{sub}')

# test role assignment usage error
with self.assertRaisesRegexp(CLIError,
'usage error: please specify at least one of "--all", "--scope" and "--resource-group".'):
self.cmd('role assignment list')

with self.assertRaisesRegexp(CLIError,
'usage error: please specify at least one of "--scope" and "--resource-group".'):
self.cmd('role assignment create --assignee {upn} --role reader')
self.cmd('role assignment delete --assignee {upn} --role reader')
finally:
self.cmd('ad user delete --upn-or-object-id {upn}')



@ResourceGroupPreparer(name_prefix='cli_role_assign')
@AllowLargeResponse()
def test_role_assignment_create_using_principal_type(self, resource_group):
Expand Down Expand Up @@ -452,8 +467,9 @@ class RoleAssignmentListScenarioTest(ScenarioTest):
@ResourceGroupPreparer(name_prefix='cli_test_assignments_for_coadmins')
@AllowLargeResponse()
def test_assignments_for_co_admins(self, resource_group):

result = self.cmd('role assignment list --include-classic-administrator').get_output_in_json()
subscription_id = self.get_subscription_id()
self.kwargs['sub'] = subscription_id
result = self.cmd('role assignment list --include-classic-administrator --scope /subscriptions/{sub}').get_output_in_json()
self.assertTrue([x for x in result if x['roleDefinitionName'] in ['CoAdministrator', 'AccountAdministrator']])
self.cmd('role assignment list -g {}'.format(resource_group), checks=[
self.check("length([])", 0)
Expand Down