From af470fd6394af9758a277f05744dd4544bac09e5 Mon Sep 17 00:00:00 2001 From: Colleen Murphy Date: Tue, 31 Dec 2019 16:22:34 -0800 Subject: [PATCH] Fix role_assignments role.id filter Without this patch, if there are multiple role assignments on the system and they are not all the same role, querying for role assignments with /v3/role_assignments?role.id={role_id} may leak some role assignments that don't match the role_id, making the returned results incorrect. This patch fixes the issue by using a list comprehension instead of a for loop over a list that was being modified within the loop. Change-Id: Icfce3b14abb55c6fef3de1b314cee22fc8b1d08c Closes-bug: #1858012 (cherry picked from commit c2d88306621f890a857acd6831ea8bf073f55537) (cherry picked from commit 4d413f1eba2d1e6b16ecd57fa27de528dd0f67cb) --- keystone/assignment/core.py | 8 +++-- .../unit/protection/v3/test_assignment.py | 4 +++ keystone/tests/unit/test_v3.py | 4 +++ keystone/tests/unit/test_v3_assignment.py | 35 ++++++++++++++++--- .../notes/bug-1858012-584267ada7e33f2c.yaml | 7 ++++ 5 files changed, 51 insertions(+), 7 deletions(-) create mode 100644 releasenotes/notes/bug-1858012-584267ada7e33f2c.yaml diff --git a/keystone/assignment/core.py b/keystone/assignment/core.py index 8db1a7bc4c..44a4e33db3 100644 --- a/keystone/assignment/core.py +++ b/keystone/assignment/core.py @@ -926,9 +926,11 @@ class Manager(manager.Manager): a['system'] = {'all': True} system_assignments.append(a) - for i, assignment in enumerate(system_assignments): - if role_id and role_id != assignment['role_id']: - system_assignments.pop(i) + if role_id: + system_assignments = [ + sa for sa in system_assignments + if role_id == sa['role_id'] + ] assignments = [] for assignment in itertools.chain( diff --git a/keystone/tests/unit/protection/v3/test_assignment.py b/keystone/tests/unit/protection/v3/test_assignment.py index a5bc13d44c..b9074fc7b1 100644 --- a/keystone/tests/unit/protection/v3/test_assignment.py +++ b/keystone/tests/unit/protection/v3/test_assignment.py @@ -384,6 +384,8 @@ class _SystemUserTests(object): def test_user_can_filter_role_assignments_by_role(self): assignments = self._setup_test_role_assignments() + self.expected = [ra for ra in self.expected + if ra['role_id'] == assignments['role_id']] self.expected.append({ 'user_id': assignments['user_id'], 'project_id': assignments['project_id'], @@ -483,6 +485,8 @@ class _SystemUserTests(object): def test_user_can_filter_role_assignments_by_system_and_role(self): assignments = self._setup_test_role_assignments() + self.expected = [ra for ra in self.expected + if ra['role_id'] == assignments['role_id']] self.expected.append({ 'user_id': assignments['user_id'], 'system': 'all', diff --git a/keystone/tests/unit/test_v3.py b/keystone/tests/unit/test_v3.py index c782c9c27f..79656a3a24 100644 --- a/keystone/tests/unit/test_v3.py +++ b/keystone/tests/unit/test_v3.py @@ -1401,6 +1401,8 @@ class AssignmentTestMixin(object): """ if attribs.get('domain_id'): link = '/domains/' + attribs['domain_id'] + elif attribs.get('system'): + link = '/system' else: link = '/projects/' + attribs['project_id'] @@ -1428,6 +1430,8 @@ class AssignmentTestMixin(object): if attribs.get('domain_id'): entity['scope'] = {'domain': {'id': attribs['domain_id']}} + elif attribs.get('system'): + entity['scope'] = {'system': {'all': True}} else: entity['scope'] = {'project': {'id': attribs['project_id']}} diff --git a/keystone/tests/unit/test_v3_assignment.py b/keystone/tests/unit/test_v3_assignment.py index 3bc15af6ed..260bb5146a 100644 --- a/keystone/tests/unit/test_v3_assignment.py +++ b/keystone/tests/unit/test_v3_assignment.py @@ -920,7 +920,7 @@ class AssignmentTestCase(test_v3.RestfulTestCase, self.role2 = unit.new_role_ref() PROVIDERS.role_api.create_role(self.role2['id'], self.role2) - # Now add one of each of the four types of assignment + # Now add one of each of the six types of assignment gd_entity = self.build_role_assignment_entity( domain_id=self.domain_id, group_id=group1['id'], @@ -944,6 +944,22 @@ class AssignmentTestCase(test_v3.RestfulTestCase, role_id=self.role2['id']) self.put(up_entity['links']['assignment']) + gs_entity = self.build_role_assignment_entity( + system='all', + group_id=group1['id'], + role_id=self.role1['id']) + self.put(gs_entity['links']['assignment']) + us_entity = self.build_role_assignment_entity( + system='all', + user_id=user1['id'], + role_id=self.role2['id']) + self.put(us_entity['links']['assignment']) + us2_entity = self.build_role_assignment_entity( + system='all', + user_id=user2['id'], + role_id=self.role2['id']) + self.put(us2_entity['links']['assignment']) + # Now list by various filters to make sure we get back the right ones collection_url = ('/role_assignments?scope.project.id=%s' % @@ -970,7 +986,7 @@ class AssignmentTestCase(test_v3.RestfulTestCase, r = self.get(collection_url, expected_status=http_client.OK) self.head(collection_url, expected_status=http_client.OK) self.assertValidRoleAssignmentListResponse(r, - expected_length=2, + expected_length=3, resource_url=collection_url) self.assertRoleAssignmentInListResponse(r, up_entity) self.assertRoleAssignmentInListResponse(r, ud_entity) @@ -979,7 +995,7 @@ class AssignmentTestCase(test_v3.RestfulTestCase, r = self.get(collection_url, expected_status=http_client.OK) self.head(collection_url, expected_status=http_client.OK) self.assertValidRoleAssignmentListResponse(r, - expected_length=2, + expected_length=3, resource_url=collection_url) self.assertRoleAssignmentInListResponse(r, gd_entity) self.assertRoleAssignmentInListResponse(r, gp_entity) @@ -988,10 +1004,21 @@ class AssignmentTestCase(test_v3.RestfulTestCase, r = self.get(collection_url, expected_status=http_client.OK) self.head(collection_url, expected_status=http_client.OK) self.assertValidRoleAssignmentListResponse(r, - expected_length=2, + expected_length=3, resource_url=collection_url) self.assertRoleAssignmentInListResponse(r, gd_entity) self.assertRoleAssignmentInListResponse(r, gp_entity) + self.assertRoleAssignmentInListResponse(r, gs_entity) + + collection_url = '/role_assignments?role.id=%s' % self.role2['id'] + r = self.get(collection_url, expected_status=http_client.OK) + self.head(collection_url, expected_status=http_client.OK) + self.assertValidRoleAssignmentListResponse(r, + expected_length=4, + resource_url=collection_url) + self.assertRoleAssignmentInListResponse(r, ud_entity) + self.assertRoleAssignmentInListResponse(r, up_entity) + self.assertRoleAssignmentInListResponse(r, us_entity) # Let's try combining two filers together.... diff --git a/releasenotes/notes/bug-1858012-584267ada7e33f2c.yaml b/releasenotes/notes/bug-1858012-584267ada7e33f2c.yaml new file mode 100644 index 0000000000..175dd6c1aa --- /dev/null +++ b/releasenotes/notes/bug-1858012-584267ada7e33f2c.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + [`bug 1858012 `_] + Fixes a bug in the /v3/role_assignments filtering where the `role.id` query + parameter didn't properly filter role assignments by role in cases where + there were multiple system role assignments.