Removing group role assignments results in overly broad revocation events

When a role on a group scoped to project/domain is revoked, it persists
revocation event in revoke_event table which is invalidating all tokens
created with same role in project/domain. Since token validations are happening
by populating role assignments at validation time, the need for persistence of
revocation events is no longer needed.

Change-Id: I112d5d4684f739d320606cea651e0a108f18d245
Closes-Bug: #1662514
(cherry picked from commit 2cb842cd645cbfcad4ccd62200340ce4616a8aa7)

4 files changed, 56 insertions, 43 deletions

diff --git a/keystone/assignment/core.py b/keystone/assignment/core.py
index 9d3641a69..eccc22d0e 100644
--- a/keystone/assignment/core.py
+++ b/keystone/assignment/core.py
@@ -349,25 +349,17 @@ class Manager(manager.Manager):
                      domain_id=None, project_id=None,
                      inherited_to_projects=False, context=None):
         if group_id is None:
-            self.revoke_api.revoke_by_grant(user_id=user_id,
-                                            role_id=role_id,
-                                            domain_id=domain_id,
-                                            project_id=project_id)
+            # check if role exists on the user before revoke
+            self.check_grant_role_id(role_id, user_id, None, domain_id,
+                                     project_id, inherited_to_projects)
             self._emit_revoke_user_grant(
                 role_id, user_id, domain_id, project_id,
                 inherited_to_projects, context)
         else:
             try:
-                # 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
-                    )
+                # check if role exists on the group before revoke
+                self.check_grant_role_id(role_id, None, group_id, domain_id,
+                                         project_id, inherited_to_projects)
                 if CONF.token.revoke_by_id:
                     # NOTE(morganfainberg): The user ids are the important part
                     # for invalidating tokens below, so extract them here.
@@ -379,7 +371,6 @@ class Manager(manager.Manager):
             except exception.GroupNotFound:
                 LOG.debug('Group %s not found, no tokens to invalidate.',
                           group_id)
-
         # TODO(henry-nash): While having the call to get_role here mimics the
         # previous behavior (when it was buried inside the driver delete call),
         # this seems an odd place to have this check, given what we have
diff --git a/keystone/tests/unit/assignment/test_backends.py b/keystone/tests/unit/assignment/test_backends.py
index e3b34fa26..c19902eee 100644
--- a/keystone/tests/unit/assignment/test_backends.py
+++ b/keystone/tests/unit/assignment/test_backends.py
@@ -1239,6 +1239,10 @@ class AssignmentTests(AssignmentTestHelperMixin):
             self.assertRaises(exception.RoleNotFound, f,
                               role_id=uuid.uuid4().hex, **kwargs)
 
+        def assert_role_assignment_not_found_exception(f, **kwargs):
+            self.assertRaises(exception.RoleAssignmentNotFound, f,
+                              role_id=uuid.uuid4().hex, **kwargs)
+
         user = unit.new_user_ref(domain_id=CONF.identity.default_domain_id)
         user_resp = self.identity_api.create_user(user)
         group = unit.new_group_ref(domain_id=CONF.identity.default_domain_id)
@@ -1248,8 +1252,7 @@ class AssignmentTests(AssignmentTestHelperMixin):
         project_resp = self.resource_api.create_project(project['id'], project)
 
         for manager_call in [self.assignment_api.create_grant,
-                             self.assignment_api.get_grant,
-                             self.assignment_api.delete_grant]:
+                             self.assignment_api.get_grant]:
             assert_role_not_found_exception(
                 manager_call,
                 user_id=user_resp['id'], project_id=project_resp['id'])
@@ -1265,6 +1268,21 @@ class AssignmentTests(AssignmentTestHelperMixin):
                 group_id=group_resp['id'],
                 domain_id=CONF.identity.default_domain_id)
 
+        assert_role_assignment_not_found_exception(
+            self.assignment_api.delete_grant,
+            user_id=user_resp['id'], project_id=project_resp['id'])
+        assert_role_assignment_not_found_exception(
+            self.assignment_api.delete_grant,
+            group_id=group_resp['id'], project_id=project_resp['id'])
+        assert_role_assignment_not_found_exception(
+            self.assignment_api.delete_grant,
+            user_id=user_resp['id'],
+            domain_id=CONF.identity.default_domain_id)
+        assert_role_assignment_not_found_exception(
+            self.assignment_api.delete_grant,
+            group_id=group_resp['id'],
+            domain_id=CONF.identity.default_domain_id)
+
     def test_multi_role_grant_by_user_group_on_project_domain(self):
         role_list = []
         for _ in range(10):
diff --git a/keystone/tests/unit/test_v3_assignment.py b/keystone/tests/unit/test_v3_assignment.py
index c7edcd773..f94fe6397 100644
--- a/keystone/tests/unit/test_v3_assignment.py
+++ b/keystone/tests/unit/test_v3_assignment.py
@@ -329,14 +329,12 @@ class AssignmentTestCase(test_v3.RestfulTestCase,
         self.head(member_url, expected_status=http_client.NOT_FOUND)
 
     def test_token_revoked_once_group_role_grant_revoked(self):
-        """Test token is revoked when group role grant is revoked.
+        """Test token invalid when direct & indirect role on user is revoked.
 
         When a role granted to a group is revoked for a given scope,
-        all tokens related to this scope and belonging to one of the members
-        of this group should be revoked.
+        and user direct role is revoked, then tokens created
+        by user will be invalid.
 
-        The revocation should be independently to the presence
-        of the revoke API.
         """
         time = datetime.datetime.utcnow()
         with freezegun.freeze_time(time) as frozen_datetime:
@@ -367,12 +365,15 @@ class AssignmentTestCase(test_v3.RestfulTestCase,
             self.assignment_api.delete_grant(role_id=self.role['id'],
                                              project_id=self.project['id'],
                                              group_id=self.group['id'])
+            # revokes the direct role form user on project
+            self.assignment_api.delete_grant(role_id=self.role['id'],
+                                             project_id=self.project['id'],
+                                             user_id=self.user['id'])
 
             frozen_datetime.tick(delta=datetime.timedelta(seconds=1))
             # validates the same token again; it should not longer be valid.
-            self.head('/auth/tokens',
-                      headers={'x-subject-token': token},
-                      expected_status=http_client.NOT_FOUND)
+            self.head('/auth/tokens', token=token,
+                      expected_status=http_client.UNAUTHORIZED)
 
     @unit.skip_if_cache_disabled('assignment')
     def test_delete_grant_from_user_and_project_invalidate_cache(self):
diff --git a/keystone/tests/unit/test_v3_auth.py b/keystone/tests/unit/test_v3_auth.py
index c17cda822..0cd449229 100644
--- a/keystone/tests/unit/test_v3_auth.py
+++ b/keystone/tests/unit/test_v3_auth.py
@@ -3033,15 +3033,15 @@ class TestTokenRevokeById(test_v3.RestfulTestCase):
 
         Test Plan:
 
-        - Get a token for user1, scoped to ProjectA
-        - Delete the grant user1 has on ProjectA
+        - Get a token for user, scoped to Project
+        - Delete the grant user has on Project
         - Check token is no longer valid
 
         """
         auth_data = self.build_authentication_request(
-            user_id=self.user1['id'],
-            password=self.user1['password'],
-            project_id=self.projectA['id'])
+            user_id=self.user['id'],
+            password=self.user['password'],
+            project_id=self.project['id'])
         token = self.get_requested_token(auth_data)
         # Confirm token is valid
         self.head('/auth/tokens',
@@ -3051,13 +3051,12 @@ class TestTokenRevokeById(test_v3.RestfulTestCase):
         grant_url = (
             '/projects/%(project_id)s/users/%(user_id)s/'
             'roles/%(role_id)s' % {
-                'project_id': self.projectA['id'],
-                'user_id': self.user1['id'],
-                'role_id': self.role1['id']})
+                'project_id': self.project['id'],
+                'user_id': self.user['id'],
+                'role_id': self.role['id']})
         self.delete(grant_url)
-        self.head('/auth/tokens',
-                  headers={'X-Subject-Token': token},
-                  expected_status=http_client.NOT_FOUND)
+        self.head('/auth/tokens', token=token,
+                  expected_status=http_client.UNAUTHORIZED)
 
     def role_data_fixtures(self):
         self.projectC = unit.new_project_ref(domain_id=self.domainA['id'])
@@ -3311,17 +3310,21 @@ class TestTokenRevokeById(test_v3.RestfulTestCase):
                 'group_id': self.group1['id'],
                 'role_id': self.role1['id']})
         self.delete(grant_url)
-        self.head('/auth/tokens',
-                  headers={'X-Subject-Token': token1},
-                  expected_status=http_client.NOT_FOUND)
-        self.head('/auth/tokens',
-                  headers={'X-Subject-Token': token2},
-                  expected_status=http_client.NOT_FOUND)
+        self.assignment_api.delete_grant(role_id=self.role1['id'],
+                                         project_id=self.projectA['id'],
+                                         user_id=self.user1['id'])
+        self.assignment_api.delete_grant(role_id=self.role1['id'],
+                                         project_id=self.projectA['id'],
+                                         user_id=self.user2['id'])
+        self.head('/auth/tokens', token=token1,
+                  expected_status=http_client.UNAUTHORIZED)
+        self.head('/auth/tokens', token=token2,
+                  expected_status=http_client.UNAUTHORIZED)
         # 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=http_client.NOT_FOUND)
+                  expected_status=http_client.OK)
 
     def test_domain_group_role_assignment_maintains_token(self):
         """Test domain-group role assignment maintains existing token.