Merge "Group role revocation invalidates all user tokens" into stable/kilo

5 files changed, 119 insertions, 29 deletions

diff --git a/keystone/assignment/core.py b/keystone/assignment/core.py
index 901cbfad1..481209635 100644
--- a/keystone/assignment/core.py
+++ b/keystone/assignment/core.py
@@ -423,6 +423,11 @@ class Manager(manager.Manager):
     def _emit_invalidate_user_token_persistence(self, user_id):
         self.identity_api.emit_invalidate_user_token_persistence(user_id)
 
+    def _emit_invalidate_grant_token_persistence(self, user_id, project_id):
+        self.identity_api.emit_invalidate_grant_token_persistence(
+            {'user_id': user_id, 'project_id': project_id}
+        )
+
     @notifications.role_assignment('created')
     def create_grant(self, role_id, user_id=None, group_id=None,
                      domain_id=None, project_id=None,
@@ -460,6 +465,10 @@ class Manager(manager.Manager):
         return self.role_api.list_roles_from_ids(grant_ids)
 
     @notifications.role_assignment('deleted')
+    def _emit_revoke_user_grant(self, role_id, user_id, domain_id, project_id,
+                                inherited_to_projects, context):
+        self._emit_invalidate_grant_token_persistence(user_id, project_id)
+
     def delete_grant(self, role_id, user_id=None, group_id=None,
                      domain_id=None, project_id=None,
                      inherited_to_projects=False, context=None):
@@ -468,17 +477,29 @@ class Manager(manager.Manager):
                                             role_id=role_id,
                                             domain_id=domain_id,
                                             project_id=project_id)
+            self._emit_revoke_user_grant(
+                role_id, user_id, domain_id, project_id,
+                inherited_to_projects, context)
         else:
             try:
-                # NOTE(morganfainberg): The user ids are the important part
-                # for invalidating tokens below, so extract them here.
-                for user in self.identity_api.list_users_in_group(group_id):
-                    if user['id'] != user_id:
-                        self._emit_invalidate_user_token_persistence(
-                            user['id'])
-                        self.revoke_api.revoke_by_grant(
-                            user_id=user['id'], role_id=role_id,
-                            domain_id=domain_id, project_id=project_id)
+                # Group may contain a lot of users so revocation will be
+                # by role & domain/project
+                if domain_id is None:
+                    self.revoke_api.revoke_by_project_role_assignment(
+                        project_id, role_id
+                    )
+                else:
+                    self.revoke_api.revoke_by_domain_role_assignment(
+                        domain_id, role_id
+                    )
+                if CONF.token.revoke_by_id:
+                    # NOTE(morganfainberg): The user ids are the important part
+                    # for invalidating tokens below, so extract them here.
+                    for user in self.identity_api.list_users_in_group(
+                            group_id):
+                        self._emit_revoke_user_grant(
+                            role_id, user['id'], domain_id, project_id,
+                            inherited_to_projects, context)
             except exception.GroupNotFound:
                 LOG.debug('Group %s not found, no tokens to invalidate.',
                           group_id)
@@ -495,8 +516,6 @@ class Manager(manager.Manager):
             self.resource_api.get_project(project_id)
         self.driver.delete_grant(role_id, user_id, group_id, domain_id,
                                  project_id, inherited_to_projects)
-        if user_id is not None:
-            self._emit_invalidate_user_token_persistence(user_id)
 
     def delete_tokens_for_role_assignments(self, role_id):
         assignments = self.list_role_assignments_for_role(role_id=role_id)
diff --git a/keystone/contrib/revoke/core.py b/keystone/contrib/revoke/core.py
index c7335690b..3100f588a 100644
--- a/keystone/contrib/revoke/core.py
+++ b/keystone/contrib/revoke/core.py
@@ -109,11 +109,12 @@ class Manager(manager.Manager):
         self.revoke(
             model.RevokeEvent(access_token_id=payload['resource_info']))
 
-    def _group_callback(self, service, resource_type, operation, payload):
-        user_ids = (u['id'] for u in self.identity_api.list_users_in_group(
-            payload['resource_info']))
-        for uid in user_ids:
-            self.revoke(model.RevokeEvent(user_id=uid))
+    def _role_assignment_callback(self, service, resource_type, operation,
+                                  payload):
+        info = payload['resource_info']
+        self.revoke_by_grant(role_id=info['role_id'], user_id=info['user_id'],
+                             domain_id=info.get('domain_id'),
+                             project_id=info.get('project_id'))
 
     def _register_listeners(self):
         callbacks = {
@@ -124,6 +125,7 @@ class Manager(manager.Manager):
                 ['role', self._role_callback],
                 ['user', self._user_callback],
                 ['project', self._project_callback],
+                ['role_assignment', self._role_assignment_callback]
             ],
             notifications.ACTIONS.disabled: [
                 ['user', self._user_callback],
diff --git a/keystone/identity/core.py b/keystone/identity/core.py
index 60ccf75a8..7195cf894 100644
--- a/keystone/identity/core.py
+++ b/keystone/identity/core.py
@@ -968,6 +968,19 @@ class Manager(manager.Manager):
         """
         pass
 
+    @notifications.internal(
+        notifications.INVALIDATE_USER_PROJECT_TOKEN_PERSISTENCE)
+    def emit_invalidate_grant_token_persistence(self, user_project):
+        """Emit a notification to the callback system to revoke grant tokens.
+
+        This method and associated callback listener removes the need for
+        making a direct call to another manager to delete and revoke tokens.
+
+        :param user_project: {'user_id': user_id, 'project_id': project_id}
+        :type user_project: dict
+        """
+        pass
+
     @manager.response_truncated
     @domains_configured
     @exception_translated('user')
diff --git a/keystone/tests/unit/common/test_notifications.py b/keystone/tests/unit/common/test_notifications.py
index 1b4040951..ce1696f5d 100644
--- a/keystone/tests/unit/common/test_notifications.py
+++ b/keystone/tests/unit/common/test_notifications.py
@@ -819,10 +819,10 @@ class CadfNotificationsWrapperTestCase(test_v3.RestfulTestCase):
             self.assertEqual(project, event.project)
         if domain:
             self.assertEqual(domain, event.domain)
-        if user:
-            self.assertEqual(user, event.user)
         if group:
             self.assertEqual(group, event.group)
+        elif user:
+            self.assertEqual(user, event.user)
         self.assertEqual(role_id, event.role)
         self.assertEqual(inherit, event.inherited_to_projects)
 
@@ -869,7 +869,7 @@ class CadfNotificationsWrapperTestCase(test_v3.RestfulTestCase):
         event_type = '%s.%s.%s' % (notifications.SERVICE,
                                    self.ROLE_ASSIGNMENT, DELETED_OPERATION)
         self._assert_last_note(action, self.user_id, event_type)
-        self._assert_event(role, project, domain, user, group)
+        self._assert_event(role, project, domain, user, None)
 
     def test_user_project_grant(self):
         url = ('/projects/%s/users/%s/roles/%s' %
@@ -881,10 +881,12 @@ class CadfNotificationsWrapperTestCase(test_v3.RestfulTestCase):
     def test_group_domain_grant(self):
         group_ref = self.new_group_ref(domain_id=self.domain_id)
         group = self.identity_api.create_group(group_ref)
+        self.identity_api.add_user_to_group(self.user_id, group['id'])
         url = ('/domains/%s/groups/%s/roles/%s' %
                (self.domain_id, group['id'], self.role_id))
         self._test_role_assignment(url, self.role_id,
                                    domain=self.domain_id,
+                                   user=self.user_id,
                                    group=group['id'])
 
     def test_add_role_to_user_and_project(self):
diff --git a/keystone/tests/unit/test_v3_auth.py b/keystone/tests/unit/test_v3_auth.py
index faa15b6ab..a76df7195 100644
--- a/keystone/tests/unit/test_v3_auth.py
+++ b/keystone/tests/unit/test_v3_auth.py
@@ -33,7 +33,6 @@ from keystone.tests import unit as tests
 from keystone.tests.unit import ksfixtures
 from keystone.tests.unit import test_v3
 
-
 CONF = cfg.CONF
 
 
@@ -1109,7 +1108,7 @@ class TestTokenRevokeById(test_v3.RestfulTestCase):
         - Delete the grant group1 has on ProjectA
         - Check tokens for user1 & user2 are no longer valid,
           since user1 and user2 are members of group1
-        - Check token for user3 is still valid
+        - Check token for user3 is invalid too
 
         """
         auth_data = self.build_authentication_request(
@@ -1152,10 +1151,11 @@ class TestTokenRevokeById(test_v3.RestfulTestCase):
         self.head('/auth/tokens',
                   headers={'X-Subject-Token': token2},
                   expected_status=404)
-        # But user3's token should still be valid
+        # But user3's token should be invalid too as revocation is done for
+        # scope role & project
         self.head('/auth/tokens',
                   headers={'X-Subject-Token': token3},
-                  expected_status=200)
+                  expected_status=404)
 
     def test_domain_group_role_assignment_maintains_token(self):
         """Test domain-group role assignment maintains existing token.
@@ -1242,6 +1242,14 @@ class TestTokenRevokeById(test_v3.RestfulTestCase):
 
     def test_removing_role_assignment_does_not_affect_other_users(self):
         """Revoking a role from one user should not affect other users."""
+
+        # This group grant is not needed for the test
+        self.delete(
+            '/projects/%(project_id)s/groups/%(group_id)s/roles/%(role_id)s' %
+            {'project_id': self.projectA['id'],
+             'group_id': self.group1['id'],
+             'role_id': self.role1['id']})
+
         user1_token = self.get_requested_token(
             self.build_authentication_request(
                 user_id=self.user1['id'],
@@ -1260,12 +1268,6 @@ class TestTokenRevokeById(test_v3.RestfulTestCase):
                 'project_id': self.projectA['id'],
                 'user_id': self.user1['id'],
                 'role_id': self.role1['id']})
-        self.delete(
-            '/projects/%(project_id)s/groups/%(group_id)s/roles/%(role_id)s' %
-            {'project_id': self.projectA['id'],
-             'group_id': self.group1['id'],
-             'role_id': self.role1['id']})
-
         # authorization for the first user should now fail
         self.head('/auth/tokens',
                   headers={'X-Subject-Token': user1_token},
@@ -1424,6 +1426,58 @@ class TestTokenRevokeById(test_v3.RestfulTestCase):
                   expected_status=200)
 
 
+class TestTokenRevokeByAssignment(TestTokenRevokeById):
+
+    def config_overrides(self):
+        super(TestTokenRevokeById, self).config_overrides()
+        self.config_fixture.config(
+            group='revoke',
+            driver='keystone.contrib.revoke.backends.kvs.Revoke')
+        self.config_fixture.config(
+            group='token',
+            provider='keystone.token.providers.uuid.Provider',
+            revoke_by_id=True)
+
+    def test_removing_role_assignment_keeps_other_project_token_groups(self):
+        """Test assignment isolation.
+
+        Revoking a group role from one project should not invalidate all group
+        users' tokens
+        """
+        self.assignment_api.create_grant(self.role1['id'],
+                                         group_id=self.group1['id'],
+                                         project_id=self.projectB['id'])
+
+        project_token = self.get_requested_token(
+            self.build_authentication_request(
+                user_id=self.user1['id'],
+                password=self.user1['password'],
+                project_id=self.projectB['id']))
+
+        other_project_token = self.get_requested_token(
+            self.build_authentication_request(
+                user_id=self.user1['id'],
+                password=self.user1['password'],
+                project_id=self.projectA['id']))
+
+        self.assignment_api.delete_grant(self.role1['id'],
+                                         group_id=self.group1['id'],
+                                         project_id=self.projectB['id'])
+
+        # authorization for the projectA should still succeed
+        self.head('/auth/tokens',
+                  headers={'X-Subject-Token': other_project_token},
+                  expected_status=200)
+        # while token for the projectB should not
+        self.head('/auth/tokens',
+                  headers={'X-Subject-Token': project_token},
+                  expected_status=404)
+        revoked_tokens = [
+            t['id'] for t in self.token_provider_api.list_revoked_tokens()]
+        # token is in token revocation list
+        self.assertIn(project_token, revoked_tokens)
+
+
 class TestTokenRevokeApi(TestTokenRevokeById):
     EXTENSION_NAME = 'revoke'
     EXTENSION_TO_ADD = 'revoke_extension'