EMSI: clean up usage of older term and support better role assignment creation #5273
Conversation
|
View a preview at https://prompt.ws/r/Azure/azure-cli/5273 |
|
//cc: @skwan |
| if large_resp_body: | ||
| large_resp_body._max_response_body = size # pylint: disable=protected-access | ||
|
|
||
| def get_guid_gen_patch(self, guids, test_seam='azure.cli.command_modules.role.custom._gen_guid'): |
There was a problem hiding this comment.
This is to ensure we will have fixed guid (used in the url) recorded into the .yaml files for playback later
|
|
||
| _CUSTOM_RULE = 'CustomRole' | ||
|
|
||
| # pylint: disable=too-many-lines |
There was a problem hiding this comment.
nit: this is the indication that this file should be split into smaller ones.
There was a problem hiding this comment.
From my experience working with modules that did split their custom commands into multiple files, it simply added complexity.
| def create_role_assignment(cmd, role, assignee=None, assignee_object_id=None, resource_group_name=None, scope=None): | ||
| if bool(assignee) == bool(assignee_object_id): | ||
| raise CLIError('usage error: --assignee STRING | --assignee-object-id GUID') | ||
| resolve_assignee = not assignee_object_id |
There was a problem hiding this comment.
nit: Can this assignment just be rolled into the parameter assignment on line 131?
| if principal_dics.get(i['properties']['principalId']): | ||
| i['properties']['principalName'] = principal_dics[i['properties']['principalId']] | ||
| except (CloudError, GraphErrorException): | ||
| pass # failure on resolving principal due to graph permission should not block the whole thing |
There was a problem hiding this comment.
Let me log an info. It is an expected exception when login as a SP w/p graph permission, so warning might be too much.
| c.argument('show_all', options_list=['--all'], action='store_true', help='show all assignments under the current subscription') | ||
| c.argument('include_inherited', action='store_true', help='include assignments applied on parent scopes') | ||
| c.argument('assignee', help='represent a user, group, or service principal. supported format: object id, user sign-in name, or service principal name') | ||
| c.argument('assignee_object_id', help="assignee's graph object id, such as the 'principal id' from a managed service identity. Use this instead of '--assignee' to by-pass graph permission issues") |
There was a problem hiding this comment.
nit: by-pass should be "bypass".
| return _create_role_assignment(cmd.cli_ctx, role, assignee, resource_group_name, scope) | ||
| def create_role_assignment(cmd, role, assignee=None, assignee_object_id=None, resource_group_name=None, scope=None): | ||
| if bool(assignee) == bool(assignee_object_id): | ||
| raise CLIError('usage error: --assignee STRING | --assignee-object-id GUID') |
There was a problem hiding this comment.
Is there a reason we couldn't simply accept a string or GUID for assignee? This seems like an Xplat anti-pattern.
There was a problem hiding this comment.
The problem is GUID can be used as assignee names, so w/o graph query, we would not know. For people login with service principal, they need to have the option to bypass the graph query.
I have proposed that on graph permission error cli can fallback to object id as long as it is a guid, but it is possible we could assign by mistake when service principal 2's object id happens to be the service principal 1's name.
| class RoleScenarioTest(ScenarioTest): | ||
|
|
||
| def enable_large_payload(self, size=8192): | ||
| from azure_devtools.scenario_tests import LargeResponseBodyProcessor |
There was a problem hiding this comment.
We should consider moving this into the test base. It is used in many places to work around what I would consider a bug in devtools.
5 participants
2 parts:
explicit identity, the right term should beuser assigned identity--assignee-object-idto bypass the graph query. This is needed when login as a service principal which by default has no graph permissions.Command Guidelines
(see Authoring Command Modules)