Browse Source

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 c2d8830662)
(cherry picked from commit 4d413f1eba)
changes/26/704726/1
Colleen Murphy 1 month ago
parent
commit
af470fd639
5 changed files with 51 additions and 7 deletions
  1. +5
    -3
      keystone/assignment/core.py
  2. +4
    -0
      keystone/tests/unit/protection/v3/test_assignment.py
  3. +4
    -0
      keystone/tests/unit/test_v3.py
  4. +31
    -4
      keystone/tests/unit/test_v3_assignment.py
  5. +7
    -0
      releasenotes/notes/bug-1858012-584267ada7e33f2c.yaml

+ 5
- 3
keystone/assignment/core.py View File

@@ -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(

+ 4
- 0
keystone/tests/unit/protection/v3/test_assignment.py View File

@@ -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',

+ 4
- 0
keystone/tests/unit/test_v3.py View File

@@ -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']}}


+ 31
- 4
keystone/tests/unit/test_v3_assignment.py View File

@@ -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....


+ 7
- 0
releasenotes/notes/bug-1858012-584267ada7e33f2c.yaml View File

@@ -0,0 +1,7 @@
---
fixes:
- |
[`bug 1858012 <https://bugs.launchpad.net/keystone/+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.

Loading…
Cancel
Save