is_revoked check all viable subtrees

An indentation error coupled with an early return lead to cases
where a subtree containing a pertinent revocation event was not
searched, and a revoked token would report as unrevoked.

Closes-bug: #1294292
Change-Id: If56ae64d8a30b461563ee8be002544117fb14215

keystone/contrib/revoke/model.py

@@ -171,32 +171,56 @@ class RevokeTree(object):
            'consumer_id', 'access_token_id'
 
         """
+        # Alternative names to be checked in token for every field in
+        # revoke tree.
         alternatives = {
             'user_id': ['user_id', 'trustor_id', 'trustee_id'],
-            'domain_id': ['identity_domain_id', 'assignment_domain_id']}
-        subnode = [self.revoke_map]
+            'domain_id': ['identity_domain_id', 'assignment_domain_id'],
+        }
+        # Contains current forest (collection of trees) to be checked.
+        partial_matches = [self.revoke_map]
+        # We iterate over every layer of our revoke tree (except the last one).
         for name in _NAMES:
+            # bundle is the set of partial matches for the next level down
+            # the tree
             bundle = []
             wildcard = '%s=*' % (name,)
-            for tree in subnode:
+            # For every tree in current forest.
+            for tree in partial_matches:
+                # If there is wildcard node on current level we take it.
                 bundle.append(tree.get(wildcard))
                 if name == 'role_id':
+                    # Roles are very special since a token has a list of them.
+                    # If the revocation event matches any one of them,
+                    # revoke the token.
                     for role_id in token_data.get('roles', []):
                         bundle.append(tree.get('role_id=%s' % role_id))
                 else:
+                    # For other fields we try to get any branch that concur
+                    # with any alternative field in the token.
                     for alt_name in alternatives.get(name, [name]):
                         bundle.append(
                             tree.get('%s=%s' % (name, token_data[alt_name])))
-                bundle = filter(None, bundle)
-                if not bundle:
-                    return False
-                subnode = bundle
-            else:
-                for leaf in subnode:
-                    issued_before = leaf.get('issued_before')
-                    if issued_before is not None:
-                        if issued_before > token_data['issued_at']:
-                            return True
+            # tree.get returns `None` if there is no match, so `bundle.append`
+            # adds a 'None' entry. This call remoes the `None` entries.
+            partial_matches = [x for x in bundle if x is not None]
+            if not partial_matches:
+                # If we end up with no branches to follow means that the token
+                # is definitely not in the revoke tree and all further
+                # iterations will be for nothing.
+                return False
+
+        # The last (leaf) level is checked in a special way because we verify
+        # issued_at field differently.
+        for leaf in partial_matches:
+            try:
+                if leaf['issued_before'] > token_data['issued_at']:
+                    return True
+            except KeyError:
+                pass
+        # If we made it out of the loop then no element in revocation tree
+        # corresponds to our token and it is good.
+        return False
 
 
 def build_token_values_v2(access, default_domain_id):

keystone/tests/test_revoke.py

@@ -395,6 +395,18 @@ class RevokeTreeTests(tests.TestCase):
                               project_id=self.project_ids[0])
         self._assertTokenRevoked(token_to_revoke)
 
+    def test_by_project_and_user_and_role(self):
+        user_id1 = _new_id()
+        user_id2 = _new_id()
+        project_id = _new_id()
+        self.events.append(self._revoke_by_user(user_id1))
+        self.events.append(
+            self._revoke_by_user_and_project(user_id2, project_id))
+        token_data = _sample_blank_token()
+        token_data['user_id'] = user_id2
+        token_data['project_id'] = project_id
+        self._assertTokenRevoked(token_data)
+
     def _assertEmpty(self, collection):
         return self.assertEqual(0, len(collection), "collection not empty")