change

Author: Andrea Tomassilli

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

@@ -540,7 +540,7 @@ def _search_role_assignments(assignments_client, definitions_client,
 
         if role:
             role_id = _resolve_role_id(role, scope, definitions_client)
-            assignments = [ra for ra in assignments if ra.role_definition_id == role_id]
+            assignments = [ra for ra in assignments if ra.role_definition_id.endswith(role_id)]
 
         # filter the assignee if "include_groups" is not provided because service side
         # does not accept filter "principalId eq and atScope()"
@@ -567,24 +567,25 @@ def _build_role_scope(resource_group_name, scope, subscription_id):
 
 
 def _resolve_role_id(role, scope, definitions_client):
-    role_id = None
-    if re.match(r'/subscriptions/.+/providers/Microsoft.Authorization/roleDefinitions/',
-                role, re.I):
-        role_id = role
-    else:
-        if is_guid(role):
-            role_id = '/subscriptions/{}/providers/Microsoft.Authorization/roleDefinitions/{}'.format(
-                definitions_client._config.subscription_id, role)
-        if not role_id:  # retrieve role id
-            role_defs = list(definitions_client.list(scope, "roleName eq '{}'".format(role)))
-            if not role_defs:
-                raise CLIError("Role '{}' doesn't exist.".format(role))
-            if len(role_defs) > 1:
-                ids = [r.id for r in role_defs]
-                err = "More than one role matches the given name '{}'. Please pick a value from '{}'"
-                raise CLIError(err.format(role, ids))
-            role_id = role_defs[0].id
-    return role_id
+    """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')
+    """
+    if re.match(r'(/subscriptions/.+)?/providers/Microsoft.Authorization/roleDefinitions/', role, re.I):
+        return role
+
+    if is_guid(role):
+        return f"/providers/Microsoft.Authorization/roleDefinitions/{role}"
+
+    role_defs = list(definitions_client.list(scope, "roleName eq '{}'".format(role)))
+    if not role_defs:
+        raise CLIError("Role '{}' doesn't exist.".format(role))
+    if len(role_defs) > 1:
+        ids = [r.id for r in role_defs]
+        err = "More than one role matches the given name '{}'. Please pick a value from '{}'"
+        raise CLIError(err.format(role, ids))
+    return role_defs[0].id
 
 
 def create_application(cmd, client, display_name, identifier_uris=None,

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

@@ -2,27 +2,160 @@
 # Copyright (c) Microsoft Corporation. All rights reserved.
 # Licensed under the MIT License. See License.txt in the project root for license information.
 # --------------------------------------------------------------------------------------------
-import unittest
+import pytest
 from unittest import mock
 
-from azure.cli.command_modules.role.custom import _resolve_role_id
+from azure.cli.command_modules.role.custom import _resolve_role_id, _search_role_assignments
 
 # pylint: disable=line-too-long
 
 
-class TestRoleCustomCommands(unittest.TestCase):
+class TestResolveRoleId:
+    """Tests for _resolve_role_id function."""
 
-    def test_resolve_role_id(self, ):
-        mock_client = mock.Mock()
-        mock_client._config.subscription_id = '123'
-        test_role_id = 'b24988ac-6180-42a0-ab88-20f738123456'
+    @pytest.fixture
+    def mock_client(self):
+        client = mock.Mock()
+        client._config.subscription_id = '00000000-0000-0000-0000-000000000000'
+        return client
 
-        # action(using a logical name)
-        result = _resolve_role_id(test_role_id, 'foobar', mock_client)
+    @pytest.mark.parametrize("role_input,expected_output", [
+        # GUID returns tenant format
+        ('b24988ac-6180-42a0-ab88-20f738123456',
+         '/providers/Microsoft.Authorization/roleDefinitions/b24988ac-6180-42a0-ab88-20f738123456'),
+        # Subscription-scoped ID returned as-is
+        ('/subscriptions/sub1/providers/Microsoft.Authorization/roleDefinitions/5370bbf4-6b73-4417-969b-8f2e6e123456',
+         '/subscriptions/sub1/providers/Microsoft.Authorization/roleDefinitions/5370bbf4-6b73-4417-969b-8f2e6e123456'),
+        # Tenant-scoped ID returned as-is
+        ('/providers/Microsoft.Authorization/roleDefinitions/5370bbf4-6b73-4417-969b-8f2e6e123456',
+         '/providers/Microsoft.Authorization/roleDefinitions/5370bbf4-6b73-4417-969b-8f2e6e123456'),
+    ])
+    def test_resolve_role_id_formats(self, mock_client, role_input, expected_output):
+        """Role IDs (GUID, subscription-scoped, tenant-scoped) are resolved correctly."""
+        result = _resolve_role_id(role_input, '/subscriptions/sub1', mock_client)
+        assert result == expected_output
 
-        # assert
-        self.assertEqual('/subscriptions/123/providers/Microsoft.Authorization/roleDefinitions/{}'.format(test_role_id), result)
+    def test_role_name_queries_api(self, mock_client):
+        """Role name triggers API lookup and returns the role definition ID from API."""
+        mock_role_def = mock.Mock()
+        mock_role_def.id = '/subscriptions/123/providers/Microsoft.Authorization/roleDefinitions/acdd72a7'
+        mock_client.list.return_value = [mock_role_def]
 
-        # action (using a full id)
-        test_full_id = '/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272123456/providers/microsoft.authorization/roleDefinitions/5370bbf4-6b73-4417-969b-8f2e6e123456'
-        self.assertEqual(test_full_id, _resolve_role_id(test_full_id, 'foobar', mock_client))
+        result = _resolve_role_id('Reader', '/subscriptions/123', mock_client)
+
+        assert result == mock_role_def.id
+        mock_client.list.assert_called_once_with('/subscriptions/123', "roleName eq 'Reader'")
+
+    @pytest.mark.parametrize("api_response,error_contains", [
+        ([], "doesn't exist"),  # Not found
+        ([mock.Mock(id='id1'), mock.Mock(id='id2')], "More than one role"),  # Multiple matches
+    ])
+    def test_role_name_error_cases(self, mock_client, api_response, error_contains):
+        """Role name lookup raises CLIError for not found or multiple matches."""
+        from knack.util import CLIError
+        mock_client.list.return_value = api_response
+
+        with pytest.raises(CLIError, match=error_contains):
+            _resolve_role_id('SomeRole', '/subscriptions/123', mock_client)
+
+
+class TestSearchRoleAssignments:
+    """Tests for _search_role_assignments function, focusing on role filtering."""
+
+    @pytest.fixture
+    def mock_clients(self):
+        assignments_client = mock.Mock()
+        definitions_client = mock.Mock()
+        definitions_client._config.subscription_id = '00000000-0000-0000-0000-000000000000'
+        return assignments_client, definitions_client
+
+    @staticmethod
+    def _create_assignment(scope, role_definition_id, principal_id='principal-1'):
+        assignment = mock.Mock()
+        assignment.scope = scope
+        assignment.role_definition_id = role_definition_id
+        assignment.principal_id = principal_id
+        return assignment
+
+    @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  
+        ('/providers/Microsoft.Management/managementGroups/my-mg',
+         '/providers/Microsoft.Authorization/roleDefinitions/{guid}'),
+        # Subscription scope with subscription-format role definition ID
+        ('/subscriptions/00000000-0000-0000-0000-000000000000',
+         '/subscriptions/00000000-0000-0000-0000-000000000000/providers/Microsoft.Authorization/roleDefinitions/{guid}'),
+    ])
+    def test_guid_filter_matches_across_scopes(self, mock_clients, scope, role_def_format):
+        """GUID filter matches assignments at various scopes with different roleDefinitionId formats."""
+        assignments_client, definitions_client = mock_clients
+        role_guid = 'acdd72a7-3385-48ef-bd42-f606fba81ae7'
+        role_def_id = role_def_format.format(guid=role_guid)
+
+        assignments_client.list_for_scope.return_value = [
+            self._create_assignment(scope, role_def_id),
+        ]
+
+        result = _search_role_assignments(
+            assignments_client, definitions_client,
+            scope=scope, assignee_object_id=None, role=role_guid,
+            include_inherited=False, include_groups=False
+        )
+
+        assert len(result) == 1
+
+    def test_different_role_guid_does_not_match(self, mock_clients):
+        """Assignments with different role GUIDs are filtered out."""
+        assignments_client, definitions_client = mock_clients
+        filter_guid = 'acdd72a7-3385-48ef-bd42-f606fba81ae7'
+        other_guid = 'b24988ac-6180-42a0-ab88-20f7382dd24c'
+
+        assignments_client.list_for_scope.return_value = [
+            self._create_assignment('/', f'/providers/Microsoft.Authorization/roleDefinitions/{other_guid}'),
+        ]
+
+        result = _search_role_assignments(
+            assignments_client, definitions_client,
+            scope='/', assignee_object_id=None, role=filter_guid,
+            include_inherited=False, include_groups=False
+        )
+
+        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