clean

Author: Andrea Tomassilli

src/azure-cli/azure/cli/command_modules/role/custom.py

@@ -538,9 +538,15 @@ def _search_role_assignments(assignments_client, definitions_client,
             ra.scope.lower() == scope.lower()
         )]
 
+        # Filter by role. Compare role definition IDs instead of full resource IDs because
+        # the same role can have different ID formats depending on scope (e.g., tenant-scoped
+        # /providers/Microsoft.Authorization/roleDefinitions/{guid} vs subscription-scoped
+        # /subscriptions/{sub}/providers/Microsoft.Authorization/roleDefinitions/{guid}).
         if role:
-            role_id = _resolve_role_id(role, scope, definitions_client)
-            assignments = [ra for ra in assignments if ra.role_definition_id and ra.role_definition_id.endswith(role_id)]
+            resource_id = _resolve_role_id(role, scope, definitions_client)
+            role_id = _get_role_definition_id(resource_id)
+            assignments = [ra for ra in assignments
+                           if _get_role_definition_id(ra.role_definition_id) == role_id]
 
         # filter the assignee if "include_groups" is not provided because service side
         # does not accept filter "principalId eq and atScope()"
@@ -550,6 +556,11 @@ def _search_role_assignments(assignments_client, definitions_client,
     return assignments
 
 
+def _get_role_definition_id(resource_id):
+    """Extract the role definition ID from a role definition resource ID."""
+    return resource_id.split('/')[-1] if resource_id else None
+
+
 def _build_role_scope(resource_group_name, scope, subscription_id):
     subscription_scope = '/subscriptions/' + subscription_id
     if scope:
@@ -567,10 +578,12 @@ def _build_role_scope(resource_group_name, scope, subscription_id):
 
 
 def _resolve_role_id(role, scope, definitions_client):
-    """Resolve a role to its full role definition resource ID from
-      - role definition resource ID (returned as-is)
-      - role definition GUID
-      - role name (e.g. 'Reader')
+    """Resolve a role to its full role definition resource ID.
+
+    Accepts:
+        - role definition resource ID (returned as-is)
+        - role definition GUID
+        - role name (e.g. 'Reader')
     """
     if re.match(r'(/subscriptions/.+)?/providers/Microsoft.Authorization/roleDefinitions/', role, re.I):
         return role

src/azure-cli/azure/cli/command_modules/role/tests/latest/test_role_custom.py

@@ -5,11 +5,31 @@
 import pytest
 from unittest import mock
 
-from azure.cli.command_modules.role.custom import _resolve_role_id, _search_role_assignments
+from azure.cli.command_modules.role.custom import (
+    _resolve_role_id, _search_role_assignments, _get_role_definition_id
+)
 
 # pylint: disable=line-too-long
 
 
+class TestGetRoleDefinitionId:
+    """Tests for _get_role_definition_id helper function."""
+
+    @pytest.mark.parametrize("resource_id,expected", [
+        # Tenant-scoped format
+        ('/providers/Microsoft.Authorization/roleDefinitions/acdd72a7-3385-48ef-bd42-f606fba81ae7',
+         'acdd72a7-3385-48ef-bd42-f606fba81ae7'),
+        # Subscription-scoped format
+        ('/subscriptions/sub1/providers/Microsoft.Authorization/roleDefinitions/b24988ac-6180-42a0-ab88-20f7382dd24c',
+         'b24988ac-6180-42a0-ab88-20f7382dd24c'),
+        # None returns None
+        (None, None),
+    ])
+    def test_extracts_guid_from_role_definition_id(self, resource_id, expected):
+        """Extracts the GUID from various role definition ID formats."""
+        assert _get_role_definition_id(resource_id) == expected
+
+
 class TestResolveRoleId:
     """Tests for _resolve_role_id function."""
 
@@ -80,7 +100,7 @@ def _create_assignment(scope, role_definition_id, principal_id='principal-1'):
     @pytest.mark.parametrize("scope,role_def_format", [
         # Root scope with tenant-format role definition ID
         ('/', '/providers/Microsoft.Authorization/roleDefinitions/{guid}'),
-        # Management group scope with tenant-format role definition ID  
+        # Management group scope with tenant-format role definition ID
         ('/providers/Microsoft.Management/managementGroups/my-mg',
          '/providers/Microsoft.Authorization/roleDefinitions/{guid}'),
         # Subscription scope with subscription-format role definition ID
@@ -123,104 +143,6 @@ def test_different_role_guid_does_not_match(self, mock_clients):
 
         assert len(result) == 0
 
-    def test_scope_comparison_is_case_insensitive(self, mock_clients):
-        """Scope matching is case insensitive."""
-        assignments_client, definitions_client = mock_clients
-        role_def_id = '/providers/Microsoft.Authorization/roleDefinitions/acdd72a7-3385-48ef-bd42-f606fba81ae7'
-
-        assignments_client.list_for_scope.return_value = [
-            self._create_assignment('/Subscriptions/SUB1/ResourceGroups/RG1', role_def_id),
-        ]
-
-        result = _search_role_assignments(
-            assignments_client, definitions_client,
-            scope='/subscriptions/sub1/resourcegroups/rg1',
-            assignee_object_id=None, role=None,
-            include_inherited=False, include_groups=False
-        )
-
-        assert len(result) == 1
-
-    def test_include_inherited_returns_parent_scope_assignments(self, mock_clients):
-        """include_inherited=True returns assignments at and above the scope."""
-        assignments_client, definitions_client = mock_clients
-        role_def_id = '/providers/Microsoft.Authorization/roleDefinitions/acdd72a7-3385-48ef-bd42-f606fba81ae7'
-
-        assignments_client.list_for_scope.return_value = [
-            self._create_assignment('/', role_def_id, 'principal-1'),
-            self._create_assignment('/subscriptions/sub1', role_def_id, 'principal-2'),
-        ]
-
-        result = _search_role_assignments(
-            assignments_client, definitions_client,
-            scope='/subscriptions/sub1',
-            assignee_object_id=None, role=None,
-            include_inherited=True, include_groups=False
-        )
-
-        assert len(result) == 2
-
-    def test_include_inherited_false_filters_parent_scope(self, mock_clients):
-        """include_inherited=False filters out assignments at parent scopes."""
-        assignments_client, definitions_client = mock_clients
-        role_def_id = '/providers/Microsoft.Authorization/roleDefinitions/acdd72a7-3385-48ef-bd42-f606fba81ae7'
-
-        assignments_client.list_for_scope.return_value = [
-            self._create_assignment('/', role_def_id, 'principal-1'),
-            self._create_assignment('/subscriptions/sub1', role_def_id, 'principal-2'),
-        ]
-
-        result = _search_role_assignments(
-            assignments_client, definitions_client,
-            scope='/subscriptions/sub1',
-            assignee_object_id=None, role=None,
-            include_inherited=False, include_groups=False
-        )
-
-        assert len(result) == 1
-        assert result[0].principal_id == 'principal-2'
-
-    def test_assignee_filter(self, mock_clients):
-        """assignee_object_id filters assignments by principal."""
-        assignments_client, definitions_client = mock_clients
-        role_def_id = '/providers/Microsoft.Authorization/roleDefinitions/acdd72a7-3385-48ef-bd42-f606fba81ae7'
-
-        assignments_client.list_for_scope.return_value = [
-            self._create_assignment('/subscriptions/sub1', role_def_id, 'principal-1'),
-            self._create_assignment('/subscriptions/sub1', role_def_id, 'principal-2'),
-        ]
-
-        result = _search_role_assignments(
-            assignments_client, definitions_client,
-            scope='/subscriptions/sub1',
-            assignee_object_id='principal-1', role=None,
-            include_inherited=False, include_groups=False
-        )
-
-        assert len(result) == 1
-        assert result[0].principal_id == 'principal-1'
-
-    def test_no_scope_uses_subscription_api(self, mock_clients):
-        """When scope is None, list_for_subscription is called and all assignments returned."""
-        assignments_client, definitions_client = mock_clients
-        role_def_id = '/providers/Microsoft.Authorization/roleDefinitions/acdd72a7-3385-48ef-bd42-f606fba81ae7'
-
-        assignments_client.list_for_subscription.return_value = [
-            self._create_assignment('/subscriptions/sub1', role_def_id, 'principal-1'),
-            self._create_assignment('/subscriptions/sub1/resourceGroups/rg1', role_def_id, 'principal-2'),
-        ]
-
-        result = _search_role_assignments(
-            assignments_client, definitions_client,
-            scope=None,
-            assignee_object_id=None, role=None,
-            include_inherited=False, include_groups=False
-        )
-
-        assert len(result) == 2
-        assignments_client.list_for_subscription.assert_called_once()
-        assignments_client.list_for_scope.assert_not_called()
-
     def test_none_role_definition_id_is_skipped(self, mock_clients):
         """Assignments with None role_definition_id are skipped when filtering by role."""
         assignments_client, definitions_client = mock_clients
@@ -238,4 +160,4 @@ def test_none_role_definition_id_is_skipped(self, mock_clients):
         )
 
         assert len(result) == 1
-        assert result[0].role_definition_id is not None
+        assert result[0].role_definition_id is not None