diff --git a/keystonemiddleware/auth_token.py b/keystonemiddleware/auth_token.py index a9625e79..15d3df7a 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 bf64995c..55e79d05 100644 --- a/keystonemiddleware/tests/test_auth_token_middleware.py +++ b/keystonemiddleware/tests/test_auth_token_middleware.py @@ -1225,9 +1225,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' @@ -1252,9 +1250,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,