auth_token cached token handling

auth_token handles tokens that are from its cache, uncached PKI
(compressed and ASN, "offline validation") and UUID/hashed tokens
("online" validation).

For all of these cases, the auth_token middleware was doing

1) expiration check
2) confirm token bind
3) store in cache

In some cases, some of these steps aren't necessary.

When getting the token from the cache
1) no expiration check is needed because the expiration time is
   stored as cache data and the token would be rejected during
   _cache_get.
2) Storing in the cache is unnecessary because the token is
   already in the cache.

When doing online validation, it's not necessary to do the
expiration check because the identity server would have rejected
the token if it was expired.

Closes-Bug: #1289075
Change-Id: I6afa98504215521538434f1f8a2d97585ce35de5

2 files changed, 33 insertions, 17 deletions

diff --git a/keystonemiddleware/auth_token.py b/keystonemiddleware/auth_token.py
index ce60402..a3a5a23 100644
--- a/keystonemiddleware/auth_token.py
+++ b/keystonemiddleware/auth_token.py
@@ -366,7 +366,7 @@ def _token_is_v3(token_info):
     return ('token' in token_info)
 
 
-def _confirm_token_not_expired(data):
+def _get_token_expiration(data):
     if not data:
         raise InvalidUserToken('Token authorization failed')
     if _token_is_v2(data):
@@ -377,6 +377,11 @@ def _confirm_token_not_expired(data):
         raise InvalidUserToken('Token authorization failed')
     expires = timeutils.parse_isotime(timestamp)
     expires = timeutils.normalize_time(expires)
+    return expires
+
+
+def _confirm_token_not_expired(data):
+    expires = _get_token_expiration(data)
     utcnow = timeutils.utcnow()
     if utcnow >= expires:
         raise InvalidUserToken('Token authorization failed')
@@ -660,6 +665,11 @@ class AuthProtocol(object):
             token_ids, cached = self._token_cache.get(user_token)
             token_id = token_ids[0]
             if cached:
+                # Token was retrieved from the cache. In this case, there's no
+                # need to check that the token is expired because the cache
+                # fetch fails for an expired token. Also, there's no need to
+                # put the token in the cache because it's already in the cache.
+
                 data = cached
 
                 if self._check_revocations_for_cached:
@@ -673,17 +683,26 @@ class AuthProtocol(object):
                                 'Token is marked as having been revoked')
                             raise InvalidUserToken(
                                 'Token authorization failed')
-            elif cms.is_pkiz(user_token):
-                verified = self._verify_pkiz_token(user_token, token_ids)
-                data = jsonutils.loads(verified)
-            elif cms.is_asn1_token(user_token):
-                verified = self._verify_signed_token(user_token, token_ids)
-                data = jsonutils.loads(verified)
+                self._confirm_token_bind(data, env)
             else:
-                data = self._identity_server.verify_token(user_token, retry)
-            expires = _confirm_token_not_expired(data)
-            self._confirm_token_bind(data, env)
-            self._token_cache.store(token_id, data, expires)
+                # Token wasn't cached. In this case, the token needs to be
+                # checked that it's not expired, and also put in the cache.
+                if cms.is_pkiz(user_token):
+                    verified = self._verify_pkiz_token(user_token, token_ids)
+                    data = jsonutils.loads(verified)
+                    expires = _confirm_token_not_expired(data)
+                elif cms.is_asn1_token(user_token):
+                    verified = self._verify_signed_token(user_token, token_ids)
+                    data = jsonutils.loads(verified)
+                    expires = _confirm_token_not_expired(data)
+                else:
+                    data = self._identity_server.verify_token(user_token,
+                                                              retry)
+                    # No need to confirm token expiration here since
+                    # verify_token fails for expired tokens.
+                    expires = _get_token_expiration(data)
+                self._confirm_token_bind(data, env)
+                self._token_cache.store(token_id, data, expires)
             return data
         except NetworkError:
             self._LOG.debug('Token validation failure.', exc_info=True)
diff --git a/keystonemiddleware/tests/test_auth_token_middleware.py b/keystonemiddleware/tests/test_auth_token_middleware.py
index 0e4689a..97cb2e7 100644
--- a/keystonemiddleware/tests/test_auth_token_middleware.py
+++ b/keystonemiddleware/tests/test_auth_token_middleware.py
@@ -1213,9 +1213,7 @@ class CommonAuthTokenMiddlewareTest(object):
                                       success=False)
 
     def test_caching_token_on_verify(self):
-        # When the token is cached, it's cached again when it's verified.
-        # NOTE(blk-u): This behavior is incorrect and inefficient, see
-        # bug 1289075.
+        # When the token is cached it isn't cached again when it's verified.
 
         # The token cache has to be initialized with our cache instance.
         self.middleware._token_cache._env_cache_name = 'cache'
@@ -1240,9 +1238,8 @@ class CommonAuthTokenMiddlewareTest(object):
         self.middleware(req.environ, self.start_fake_response)
         self.assertEqual(200, self.response_status)
 
-        # FIXME(blk-u): This should be 1 since the token shouldn't be cached
-        # again.
-        self.assertThat(2, matchers.Equals(cache.set.call_count))
+        # Assert that the token wasn't cached again.
+        self.assertThat(1, matchers.Equals(cache.set.call_count))
 
 
 class V2CertDownloadMiddlewareTest(BaseAuthTokenMiddlewareTest,