Skip to content

[Role] Fix #11594: az role assignment create: Only show supported values for --assignee-principal-type#16056

Merged
jiasli merged 5 commits intoAzure:devfrom
jiasli:assignee-principal-type
Jan 5, 2021
Merged

[Role] Fix #11594: az role assignment create: Only show supported values for --assignee-principal-type#16056
jiasli merged 5 commits intoAzure:devfrom
jiasli:assignee-principal-type

Conversation

@jiasli
Copy link
Copy Markdown
Member

@jiasli jiasli commented Nov 26, 2020

Description
Fix #11594: --assignee-principal-type documents wrong list of expected values

The REST spec of RoleAssignmentPropertiesWithScope.principalType and RoleAssignmentProperties.principalType contains invalid values which are used only internally by the service. (Azure/azure-rest-api-specs#11830)

Until the REST spec is fixed, manually override PrincipalType from SDK so that only allowed values are shown:

  • User
  • Group
  • ServicePrincipal

Testing Guide
See the added tests under test_role_assignment_create_using_principal_type.

See email: BadRequestError: The PrincipalType property 'MSI' is not valid. It must be 'User', 'Group' or 'ServicePrincipal'

@yonzhan
Copy link
Copy Markdown
Collaborator

yonzhan commented Nov 26, 2020

Role

@yonzhan yonzhan added this to the S180 milestone Nov 26, 2020
@yonzhan yonzhan requested a review from evelyn-ys November 26, 2020 09:15
from azure.cli.core.commands.parameters import get_enum_type, get_three_state_flag, get_location_type, tags_type
from azure.cli.core.commands.validators import validate_file_or_dict
from azure.cli.core.profiles import ResourceType
# from azure.cli.core.profiles import ResourceType
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can remove it directly.

Copy link
Copy Markdown
Member Author

@jiasli jiasli Dec 1, 2020

Choose a reason for hiding this comment

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

We need it back after the swagger is fixed, so keeping it here for future usage.

group = "Group"
service_principal = "ServicePrincipal"

c.argument('assignee_principal_type', min_api='2018-09-01-preview', arg_type=get_enum_type(PrincipalType),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I remember that there are several other allowed values and there may be some users who are using them. Are you sure all these values are invalid so that we can remove them directly ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I am sure. According to the error message from service:

The PrincipalType property 'MSI' is not valid. It must be 'User', 'Group' or 'ServicePrincipal'.

> az role assignment create --scope /subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590/resourceGroups/rg1111 --role "Reader" --assignee-principal-type MSI --assignee-object-id 2fdcd421-c744-440d-b253-3daf68c697eb --debug

msrest.http_logger : Request URL: 'https://management.azure.com/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590/resourceGroups/rg1111/providers/Microsoft.Authorization/roleAssignments/0a5f880e-f02f-4a3e-8799-0a09fd95cf29?api-version=2020-04-01-preview'
msrest.http_logger : Request method: 'PUT'
msrest.http_logger : Request body:
msrest.http_logger : {"properties": {"roleDefinitionId": "/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590/providers/Microsoft.Authorization/roleDefinitions/acdd72a7-3385-48ef-bd42-f606fba81ae7", "principalId": "2fdcd421-c744-440d-b253-3daf68c697eb", "principalType": "MSI"}}

msrest.http_logger : Response status: 400
msrest.http_logger : Response content:
msrest.http_logger : {"error":{"code":"InvalidPrincipalType","message":"The PrincipalType property 'MSI' is not valid. It must be 'User', 'Group' or 'ServicePrincipal'."}}

See email: BadRequestError: The PrincipalType property 'MSI' is not valid. It must be 'User', 'Group' or 'ServicePrincipal'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

arg_type=get_enum_type(['User', 'Group', 'ServicePrincipal']) for short

reference: registering enums

Copy link
Copy Markdown
Member Author

@jiasli jiasli Dec 2, 2020

Choose a reason for hiding this comment

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

I prefer to leave it as it is now, as I am only patching PrincipalType to make as less code change as possible (for the same reason as #16056 (comment)). PrincipalType will be fetched from Python SDK again once the REST spec is fixed.

@jiasli jiasli requested a review from yonzhan December 3, 2020 02:35
Copy link
Copy Markdown
Collaborator

@yonzhan yonzhan left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

--assignee-principal-type documents wrong list of expected values

4 participants