From df721d05bfa4c69f8540d6051912d2430ed06213 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9CRichard?= Date: Fri, 18 Nov 2016 18:25:07 +0000 Subject: [PATCH] Don't invalidate all user tokens of roleless group As discussed in [1], deleting a group invalidates all user tokens which can flood the revocation event table if the deleted group contained thousands of users in the group. This happens regardless of whether the group had any role assignment or not. This patch makes it so that only groups that had role assignments to a project or domain can then invalidate user tokens, otherwise there is no need to revoke each user token because the group was not assigned any form of authorization to begin with. [1]: https://bugs.launchpad.net/keystone/+bug/1268751 Related-Bug: #1268751 Change-Id: I22ad364cb4737df3ed086f78310f75f3099ab4c1 --- keystone/identity/core.py | 9 ++++-- keystone/tests/unit/test_revoke.py | 49 ++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/keystone/identity/core.py b/keystone/identity/core.py index ee5abba3b4..377a8ce226 100644 --- a/keystone/identity/core.py +++ b/keystone/identity/core.py @@ -1114,6 +1114,7 @@ class Manager(manager.Manager): def delete_group(self, group_id, initiator=None): domain_id, driver, entity_id = ( self._get_domain_driver_and_entity_id(group_id)) + roles = self.assignment_api.list_role_assignments(group_id=group_id) user_ids = (u['id'] for u in self.list_users_in_group(group_id)) driver.delete_group(entity_id) self.get_group.invalidate(self, group_id) @@ -1122,8 +1123,12 @@ class Manager(manager.Manager): notifications.Audit.deleted(self._GROUP, group_id, initiator) - for uid in user_ids: - self.emit_invalidate_user_token_persistence(uid) + # If the group has been created and has users but has no role + # assignment for the group then we do not need to revoke all the users + # tokens and can just delete the group. + if roles: + for uid in user_ids: + self.emit_invalidate_user_token_persistence(uid) # Invalidate user role assignments cache region, as it may be caching # role assignments expanded from the specified group to its users diff --git a/keystone/tests/unit/test_revoke.py b/keystone/tests/unit/test_revoke.py index 26a256a69d..87d79bf0e2 100644 --- a/keystone/tests/unit/test_revoke.py +++ b/keystone/tests/unit/test_revoke.py @@ -17,6 +17,7 @@ import uuid import mock from oslo_utils import timeutils from six.moves import range +from testtools import matchers from keystone.common import utils import keystone.conf @@ -518,6 +519,54 @@ class RevokeTests(object): self.revoke_api.check_token, token_values) + def test_delete_group_without_role_does_not_revoke_users(self): + revocation_backend = sql.Revoke() + domain = unit.new_domain_ref() + self.resource_api.create_domain(domain['id'], domain) + # Create two groups. Group1 will be used to test deleting a group, + # without role assignments and users in the group, doesn't create + # revoked events. Group2 will show that deleting a group with role + # assignment and users in the group does create revoked events + group1 = unit.new_group_ref(domain_id=domain['id']) + group1 = self.identity_api.create_group(group1) + group2 = unit.new_group_ref(domain_id=domain['id']) + group2 = self.identity_api.create_group(group2) + role = unit.new_role_ref() + self.role_api.create_role(role['id'], role) + user1 = unit.new_user_ref(domain_id=domain['id']) + user1 = self.identity_api.create_user(user1) + user2 = unit.new_user_ref(domain_id=domain['id']) + user2 = self.identity_api.create_user(user2) + + # Add two users to the group, verify they are added, delete group, and + # check that the revocaiton events have not been created + self.identity_api.add_user_to_group(user_id=user1['id'], + group_id=group1['id']) + self.identity_api.add_user_to_group(user_id=user2['id'], + group_id=group1['id']) + self.assertEqual( + 2, len(self.identity_api.list_users_in_group(group1['id']))) + self.identity_api.delete_group(group1['id']) + self.assertEqual(0, len(revocation_backend.list_events())) + + # Assign a role to the group, add two users to the group, verify that + # the role has been assigned to the group, verify the users have been + # added to the group, delete the group, check that the revocation + # events have been created + self.assignment_api.create_grant(group_id=group2['id'], + domain_id=domain['id'], + role_id=role['id']) + grants = self.assignment_api.list_role_assignments(role_id=role['id']) + self.assertThat(grants, matchers.HasLength(1)) + self.identity_api.add_user_to_group(user_id=user1['id'], + group_id=group2['id']) + self.identity_api.add_user_to_group(user_id=user2['id'], + group_id=group2['id']) + self.assertEqual( + 2, len(self.identity_api.list_users_in_group(group2['id']))) + self.identity_api.delete_group(group2['id']) + self.assertEqual(2, len(revocation_backend.list_events())) + class UUIDSqlRevokeTests(test_backend_sql.SqlTests, RevokeTests): def config_overrides(self):