[Role] Fix --role filter failing at non-subscription scopes

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

@@ -538,9 +538,16 @@ 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 resource ID formats depending on scope
         if role:
-            role_id = _resolve_role_id(role, scope, definitions_client)
-            assignments = [ra for ra in assignments if ra.role_definition_id == role_id]
+            resource_id = _resolve_role_id(role, scope, definitions_client)
+            # If the role ID couldn't be parsed to a valid GUID, no assignments can match
+            if not (role_id := _get_role_definition_id(resource_id)):
+                return []
+
+            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 +557,19 @@ def _search_role_assignments(assignments_client, definitions_client,
     return assignments
 
 
+def _get_role_definition_id(resource_id):
+    """Extract the role definition GUID from a role definition resource ID.
+
+    Returns the GUID as a UUID object for case-insensitive comparison, or None if invalid.
+    """
+    if resource_id:
+        try:
+            return uuid.UUID(resource_id.rsplit('/', 1)[-1])
+        except ValueError:
+            pass
+    return None
+
+
 def _build_role_scope(resource_group_name, scope, subscription_id):
     subscription_scope = '/subscriptions/' + subscription_id
     if scope:
@@ -567,24 +587,30 @@ 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.
+
+    Accepts:
+        - role definition resource ID (returned as-is)
+        - role definition GUID
+        - role name (e.g. 'Reader')
+    """
+    # 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
+            is_guid(role.rsplit('/', 1)[-1])):
+        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,179 @@
 # 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 uuid
+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, _get_role_definition_id
+)
 
 # pylint: disable=line-too-long
 
 
-class TestRoleCustomCommands(unittest.TestCase):
+class TestGetRoleDefinitionId:
+    """Tests for _get_role_definition_id helper 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.mark.parametrize("resource_id,expected", [
+        # Tenant-scoped format
+        ('/providers/Microsoft.Authorization/roleDefinitions/acdd72a7-3385-48ef-bd42-f606fba81ae7',
+         uuid.UUID('acdd72a7-3385-48ef-bd42-f606fba81ae7')),
+        # Subscription-scoped format
+        ('/subscriptions/sub1/providers/Microsoft.Authorization/roleDefinitions/b24988ac-6180-42a0-ab88-20f7382dd24c',
+         uuid.UUID('b24988ac-6180-42a0-ab88-20f7382dd24c')),
+        # None returns None
+        (None, None),
+        # Empty string returns None (not a valid GUID)
+        ('', None),
+        # Invalid GUID in last segment returns None
+        ('/providers/Microsoft.Authorization/roleDefinitions/not-a-guid', None),
+        # Malformed path with extra segments still extracts last segment if valid GUID
+        ('/some/path/acdd72a7-3385-48ef-bd42-f606fba81ae7',
+         uuid.UUID('acdd72a7-3385-48ef-bd42-f606fba81ae7')),
+    ])
+    def test_extracts_guid_from_role_definition_id(self, resource_id, expected):
+        """Extracts the GUID from various role definition ID formats."""
+        result = _get_role_definition_id(resource_id)
+        assert result == expected
 
-        # action(using a logical name)
-        result = _resolve_role_id(test_role_id, 'foobar', mock_client)
+    def test_returns_uuid_for_case_insensitive_comparison(self):
+        """Returns UUID objects that compare equal regardless of case."""
+        lower = _get_role_definition_id('/providers/Microsoft.Authorization/roleDefinitions/acdd72a7-3385-48ef-bd42-f606fba81ae7')
+        upper = _get_role_definition_id('/providers/Microsoft.Authorization/roleDefinitions/ACDD72A7-3385-48EF-BD42-F606FBA81AE7')
+        assert lower == upper
+        assert isinstance(lower, uuid.UUID)
+        assert isinstance(upper, uuid.UUID)
 
-        # assert
-        self.assertEqual('/subscriptions/123/providers/Microsoft.Authorization/roleDefinitions/{}'.format(test_role_id), result)
 
-        # 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))
+class TestResolveRoleId:
+    """Tests for _resolve_role_id function."""
+
+    @pytest.fixture
+    def mock_client(self):
+        client = mock.Mock()
+        client._config.subscription_id = '00000000-0000-0000-0000-000000000000'
+        return 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
+
+    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]
+
+        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_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
+        role_guid = 'acdd72a7-3385-48ef-bd42-f606fba81ae7'
+
+        assignment_with_none = self._create_assignment('/', None)
+        assignment_with_role = self._create_assignment(
+            '/', f'/providers/Microsoft.Authorization/roleDefinitions/{role_guid}')
+        assignments_client.list_for_scope.return_value = [assignment_with_none, assignment_with_role]
+
+        result = _search_role_assignments(
+            assignments_client, definitions_client,
+            scope='/', assignee_object_id=None, role=role_guid,
+            include_inherited=False, include_groups=False
+        )
+
+        assert len(result) == 1
+        assert result[0].role_definition_id is not None