[Role] Fix --role filter failing at non-subscription scopes#32534
[Role] Fix --role filter failing at non-subscription scopes#32534
--role filter failing at non-subscription scopes#32534Conversation
️✔️AzureCLI-FullTest
|
️✔️AzureCLI-BreakingChangeTest
|
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
|
The git hooks are available for azure-cli and azure-cli-extensions repos. They could help you run required checks before creating the PR. Please sync the latest code with latest dev branch (for azure-cli) or main branch (for azure-cli-extensions). pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>
|
microsoft-github-policy-service
bot
added
customer-reported
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where az role assignment list --role <guid> and az role assignment delete --role <guid> returned empty results when querying at non-subscription scopes (tenant root, management groups, service groups). The root cause was a mismatch between tenant-format role definition IDs returned by the API and subscription-format IDs generated by the resolver.
Key changes:
- Modified
_resolve_role_idto return tenant-format role definition IDs for GUIDs - Changed role filtering logic from strict equality to
endswith()comparison - Migrated tests from unittest to pytest and added comprehensive test coverage for multiple scope scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/azure-cli/azure/cli/command_modules/role/custom.py |
Updated _resolve_role_id to return tenant-format IDs for GUIDs; changed role filtering to use endswith() comparison; added function docstring |
src/azure-cli/azure/cli/command_modules/role/tests/latest/test_role_custom.py |
Converted from unittest to pytest; added comprehensive test coverage for role ID resolution and role assignment filtering across different scopes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/azure-cli/azure/cli/command_modules/role/tests/latest/test_role_custom.py
Outdated
Show resolved
Hide resolved
src/azure-cli/azure/cli/command_modules/role/tests/latest/test_role_custom.py
Show resolved
Hide resolved
atomassi
force-pushed
the
atomassilli/32533
branch
2 times, most recently
from
36c5ba3 to
fb88971
Compare
atomassi
force-pushed
the
atomassilli/32533
branch
from
fb88971 to
969b830
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
| ) | ||
|
|
||
| assert len(result) == 1 | ||
| assert result[0].role_definition_id is not None No newline at end of file |
| """ | ||
| if resource_id: | ||
| try: | ||
| return uuid.UUID(resource_id.rsplit('/', 1)[-1]) |
There was a problem hiding this comment.
We usually use azure.mgmt.core.tools.parse_resource_id to parse resource ID, but it seems it cannot parse a tenant-level resource:
from azure.mgmt.core.tools import parse_resource_id
print(parse_resource_id('/providers/Microsoft.Authorization/roleDefinitions/e89f72ce-a487-4671-bffa-5eaf57e59f58'))
print(parse_resource_id('/subscriptions/7e30e593-3cc2-47b7-8339-7d5eb15f8142/providers/Microsoft.Authorization/roleDefinitions/e89f72ce-a487-4671-bffa-5eaf57e59f58'))Output:
{'name': '/providers/Microsoft.Authorization/roleDefinitions/e89f72ce-a487-4671-bffa-5eaf57e59f58'}
{'subscription': '7e30e593-3cc2-47b7-8339-7d5eb15f8142', 'namespace': 'Microsoft.Authorization', 'type': 'roleDefinitions', 'name': 'e89f72ce-a487-4671-bffa-5eaf57e59f58', 'children': '', 'resource_parent': '', 'resource_namespace': 'Microsoft.Authorization', 'resource_type': 'roleDefinitions', 'resource_name': 'e89f72ce-a487-4671-bffa-5eaf57e59f58'}
| """ | ||
| # Check if it's a role definition resource ID: /providers/Microsoft.Authorization/roleDefinitions/<guid> | ||
| # optionally prefixed with /subscriptions/... The last segment must be a valid GUID. | ||
| if (re.match(r'(/subscriptions/[^/]+)?/providers/Microsoft.Authorization/roleDefinitions/[^/]+$', role, re.I) and |
There was a problem hiding this comment.
is_valid_resource_id cannot parse tenant-level resource either:
from azure.mgmt.core.tools import is_valid_resource_id
print(is_valid_resource_id('/providers/Microsoft.Authorization/roleDefinitions/e89f72ce-a487-4671-bffa-5eaf57e59f58'))
print(is_valid_resource_id('/subscriptions/7e30e593-3cc2-47b7-8339-7d5eb15f8142/providers/Microsoft.Authorization/roleDefinitions/e89f72ce-a487-4671-bffa-5eaf57e59f58'))Output:
False
TrueThere was a problem hiding this comment.
looks like only used for subscription scoped resources in this module
azure-cli/src/azure-cli/azure/cli/command_modules/role/custom.py
Lines 559 to 561 in 63606b2
also it does not work for management groups and service groups, e.g.,
> python -c "from azure.mgmt.core.tools import is_valid_resource_id; rid='/providers/Microsoft.Management/managementGroups/mgmt1/providers/Microsoft.Authorization/roleAssignments/c4cb02d0-af03-4afc-a6ea-1a5af4e84b9b'; print(is_valid_resource_id(rid))"
False
|
I can confirm that the same role definition such as |
| """ | ||
| if resource_id: | ||
| try: | ||
| return uuid.UUID(resource_id.rsplit('/', 1)[-1]) |
There was a problem hiding this comment.
There is no need to build a uuid.UUID object. String comparison is sufficient.
There was a problem hiding this comment.
this is to
- check if valid
- deal with different guid formats (e.g,
c4cb02d0-af03-4afc-a6ea-1a5af4e84b9bvsc4cb02d0af034afca6ea-1a5af4e84b9b) and case differences
| mock_client = mock.Mock() | ||
| mock_client._config.subscription_id = '123' | ||
| test_role_id = 'b24988ac-6180-42a0-ab88-20f738123456' | ||
| @pytest.mark.parametrize("resource_id,expected", [ |
There was a problem hiding this comment.
We currently don't use pytest features in order to keep max compatibility with unittest.
| return role | ||
|
|
||
| if is_guid(role): | ||
| return f"/providers/Microsoft.Authorization/roleDefinitions/{role}" |
There was a problem hiding this comment.
I assume this is only a dummy resource ID because the /providers/Microsoft.Authorization/roleDefinitions/ prefix will be stripped by _get_role_definition_id later. Perhaps we can make _resolve_role_id return the role name (GUID)?
There was a problem hiding this comment.
this is because this method is also used by the create role assignment path and it needs a full resource ID (not just a guid).
Agree, this can just return the guid and the create can build the resource path internally
azure-cli/src/azure-cli/azure/cli/command_modules/role/custom.py
Lines 208 to 219 in 63606b2
Closes #32533
Related command
az role assignment listaz role assignment deleteDescription
Fix
az role assignment list --role <guid>andaz role assignment delete --role <guid>returning empty results when querying at non-subscription scopes://providers/Microsoft.Management/managementGroups/{mg-name}/providers/Microsoft.Management/serviceGroups/{service-group-name}Root Cause:
Role definition IDs can have two formats:
/providers/Microsoft.Authorization/roleDefinitions/{guid}/subscriptions/{sub-id}/providers/Microsoft.Authorization/roleDefinitions/{guid}When listing role assignments at tenant/management group/service group scope, the returned
roleDefinitionIduses the tenant format, while the previous code resolved--role <guid>to the subscription format. The strict equality comparison then failed to match assignments.azure-cli/src/azure-cli/azure/cli/command_modules/role/custom.py
Lines 575 to 577 in 63606b2
Then,
_search_role_assignmentsperformed a strict equality comparison:azure-cli/src/azure-cli/azure/cli/command_modules/role/custom.py
Lines 541 to 543 in 63606b2
Example of the mismatch:
roleDefinitionId(from API at root/managed group/service group scope)/providers/Microsoft.Authorization/roleDefinitions/acdd72a7-...role_id(from_resolve_role_id)/subscriptions/xxx/providers/Microsoft.Authorization/roleDefinitions/acdd72a7-...Changes:
_resolve_role_idnow returns tenant-format role definition IDs (/providers/Microsoft.Authorization/roleDefinitions/{guid}) when given a GUID, ensuring consistency across scopes. Also improved regex validation to reject malformed paths._get_role_definition_idhelper function that extracts the role definition GUID from a full resource ID as auuid.UUIDobject for case-insensitive comparison (handling both tenant-format and subscription-format IDs). ReturnsNonefor invalid inputs._search_role_assignmentsnow compares extracted GUIDs (as UUID objects) instead of full resource IDs, so that assignments match regardless of whether they use tenant-format or subscription-format role definition IDs. Includes early return if the role ID couldn't be parsed to a valid GUID.Testing Guide
History Notes
[Role]
az role assignment list/delete: Fix--rolefilter returning empty results at tenant, management group, and service group scopesThis checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.