From 2fdf89554f75c46e20e4b0ec4c373037da2cfe53 Mon Sep 17 00:00:00 2001 From: prashkre Date: Thu, 25 May 2017 21:41:55 +0530 Subject: [PATCH] Handle group NotFound in effective assignment list When keystone is using an external identity backend such as LDAP for storing users and groups, but storing role assignments in the local db, and a group that has role assignments is deleted out-of-band, its assignments will still exist in the keystone database. If, after this, a user attempts to list effective role assignments, keystone will try to lookup the group and fail with NotFound. This catches the NotFound exception of the list_users_in_group call and returns an empty user list so that the effective assignments list does not fail. Closes-Bug: 1693510 Change-Id: Ie5f69b150d59287bd0bc68f1ce9eecfeab04c91a (cherry picked from commit d09c337619fed8664272848abb3a1351dd5e4c85) --- keystone/assignment/core.py | 13 +++- .../tests/unit/assignment/test_backends.py | 62 +++++++++++++++++-- keystone/tests/unit/default_fixtures.py | 5 ++ 3 files changed, 72 insertions(+), 8 deletions(-) diff --git a/keystone/assignment/core.py b/keystone/assignment/core.py index a47e0f2999..29bb3db56a 100644 --- a/keystone/assignment/core.py +++ b/keystone/assignment/core.py @@ -469,9 +469,18 @@ class Manager(manager.Manager): if user_id: return [create_group_assignment(ref, user_id=user_id)] + # Note(prashkre): Try to get the users in a group, + # if a group wasn't found in the backend, users are set + # as empty list. + try: + users = self.identity_api.list_users_in_group(ref['group_id']) + except exception.GroupNotFound: + LOG.warning('Group %(group)s was not found but still has role ' + 'assignments.', {'group': ref['group_id']}) + users = [] + return [create_group_assignment(ref, user_id=m['id']) - for m in self.identity_api.list_users_in_group( - ref['group_id'])] + for m in users] def expand_inherited_assignment(ref, user_id, project_id, subtree_ids, expand_groups): diff --git a/keystone/tests/unit/assignment/test_backends.py b/keystone/tests/unit/assignment/test_backends.py index fbcc248547..c585ed1935 100644 --- a/keystone/tests/unit/assignment/test_backends.py +++ b/keystone/tests/unit/assignment/test_backends.py @@ -19,6 +19,7 @@ from testtools import matchers import keystone.conf from keystone import exception from keystone.tests import unit +from keystone.tests.unit import default_fixtures CONF = keystone.conf.CONF @@ -605,24 +606,73 @@ class AssignmentTests(AssignmentTestHelperMixin): def _group_not_found(value): raise exception.GroupNotFound(group_id=value) - # Note(knikolla): Patch get_group to return GroupNotFound - # this simulates the case of a group being deleted - # directly in the backend and still having lingering role - # assignments. + # Setup + # 1) Remove any pre-existing assignments so we control what's there + for a in self.assignment_api.list_role_assignments(): + self.assignment_api.delete_grant(**a) + # 2) create a group and 2 users in that group + domain_id = CONF.identity.default_domain_id + group = self.identity_api.create_group( + unit.new_group_ref(domain_id=domain_id)) + user1 = self.identity_api.create_user( + unit.new_user_ref(domain_id=domain_id)) + user2 = self.identity_api.create_user( + unit.new_user_ref(domain_id=domain_id)) + self.identity_api.add_user_to_group(user1['id'], group['id']) + self.identity_api.add_user_to_group(user2['id'], group['id']) + # 3) create a role assignment for the group + self.assignment_api.create_grant( + group_id=group['id'], + domain_id=domain_id, + role_id=default_fixtures.MEMBER_ROLE_ID) + + num_assignments = len(self.assignment_api.list_role_assignments()) + self.assertEqual(1, num_assignments) + + # Patch get_group to return GroupNotFound, allowing us to confirm + # that the exception is handled properly when include_names processing + # attempts to lookup a group that has been deleted in the backend with mock.patch.object(self.identity_api, 'get_group', _group_not_found): assignment_list = self.assignment_api.list_role_assignments( include_names=True ) - self.assertNotEqual([], assignment_list) + self.assertEqual(num_assignments, len(assignment_list)) for assignment in assignment_list: + includes_group_assignments = False if 'group_name' in assignment: - # Note(knikolla): In the case of a not found group we + includes_group_assignments = True + # Note(knikolla): In the case of a not-found group we # populate the values with empty strings. self.assertEqual('', assignment['group_name']) self.assertEqual('', assignment['group_domain_id']) self.assertEqual('', assignment['group_domain_name']) + self.assertTrue(includes_group_assignments) + + num_effective = len(self.assignment_api.list_role_assignments( + effective=True)) + self.assertGreater(num_effective, len(assignment_list)) + + # Patch list_users_in_group to return GroupNotFound allowing us to + # confirm that the exception is handled properly when effective + # processing attempts to lookup users for a group that has been deleted + # in the backend + with mock.patch.object(self.identity_api, 'list_users_in_group', + _group_not_found): + assignment_list = self.assignment_api.list_role_assignments( + effective=True + ) + + self.assertGreater(num_effective, len(assignment_list)) + + # cleanup + self.assignment_api.delete_grant( + group_id=group['id'], + domain_id=domain_id, + role_id=default_fixtures.MEMBER_ROLE_ID) + # TODO(edmondsw) should cleanup users/groups as well, but that raises + # LDAP read-only issues def test_add_duplicate_role_grant(self): roles_ref = self.assignment_api.get_roles_for_user_and_project( diff --git a/keystone/tests/unit/default_fixtures.py b/keystone/tests/unit/default_fixtures.py index 1fd48f028e..784b1dd90d 100644 --- a/keystone/tests/unit/default_fixtures.py +++ b/keystone/tests/unit/default_fixtures.py @@ -20,6 +20,7 @@ BAR_TENANT_ID = uuid.uuid4().hex BAZ_TENANT_ID = uuid.uuid4().hex MTU_TENANT_ID = uuid.uuid4().hex SERVICE_TENANT_ID = uuid.uuid4().hex +MEMBER_ROLE_ID = uuid.uuid4().hex DEFAULT_DOMAIN_ID = 'default' TENANTS = [ @@ -140,6 +141,10 @@ ROLES = [ 'id': 'service', 'name': 'Service', 'domain_id': None, + }, { + 'id': MEMBER_ROLE_ID, + 'name': 'member', + 'domain_id': None, } ]