[identity] Correctly handle expired tokens in IdentityTokenCache

This CL fixes a bug in IdentityTokenCache::GetToken function that
incorrectly handles expired tokens. The code contained a DCHECK that is
triggered all the time when the access token cache returns an expired
token.

This CL modifies IdentityTokenCache::GetToken to iterate over several
possible match candidates discarding all expired tokens.

Bug: 1127187, 1100535
Change-Id: I30390b76debb740a5bf43df89383d35695dcc6c0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2404851
Commit-Queue: Alex Ilin <alexilin@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#806214}

chrome/browser/extensions/api/identity/identity_token_cache.cc

@@ -146,8 +146,8 @@ IdentityTokenCache::~IdentityTokenCache() = default;
 
 void IdentityTokenCache::SetToken(const ExtensionTokenKey& key,
                                   const IdentityTokenCacheValue& token_data) {
-  DCHECK_NE(IdentityTokenCacheValue::CACHE_STATUS_NOTFOUND,
-            token_data.status());
+  if (token_data.status() == IdentityTokenCacheValue::CACHE_STATUS_NOTFOUND)
+    return;
 
   if (token_data.status() != IdentityTokenCacheValue::CACHE_STATUS_TOKEN) {
     const IdentityTokenCacheValue& cached_value = GetToken(key);
@@ -199,6 +199,7 @@ void IdentityTokenCache::EraseAllTokens() {
 
 const IdentityTokenCacheValue& IdentityTokenCache::GetToken(
     const ExtensionTokenKey& key) {
+  EraseStaleTokens();
   AccessTokensKey access_tokens_key(key);
   auto find_tokens_it = access_tokens_cache_.find(access_tokens_key);
   if (find_tokens_it != access_tokens_cache_.end()) {
@@ -211,8 +212,10 @@ const IdentityTokenCacheValue& IdentityTokenCache::GetToken(
         });
 
     if (matched_token_it != cached_tokens.end()) {
-      DCHECK_EQ(IdentityTokenCacheValue::CACHE_STATUS_TOKEN,
-                matched_token_it->status());
+      IdentityTokenCacheValue::CacheValueStatus status =
+          matched_token_it->status();
+      DCHECK(status == IdentityTokenCacheValue::CACHE_STATUS_TOKEN ||
+             status == IdentityTokenCacheValue::CACHE_STATUS_NOTFOUND);
       return *matched_token_it;
     }
   }
@@ -229,4 +232,25 @@ IdentityTokenCache::access_tokens_cache() {
   return access_tokens_cache_;
 }
 
+void IdentityTokenCache::EraseStaleTokens() {
+  // Expired tokens have CACHE_STATUS_NOTFOUND status.
+  for (auto it = access_tokens_cache_.begin();
+       it != access_tokens_cache_.end();) {
+    auto& cached_tokens = it->second;
+    base::EraseIf(cached_tokens, [](const IdentityTokenCacheValue& value) {
+      return value.status() == IdentityTokenCacheValue::CACHE_STATUS_NOTFOUND;
+    });
+
+    if (cached_tokens.empty())
+      it = access_tokens_cache_.erase(it);
+    else
+      ++it;
+  }
+
+  base::EraseIf(intermediate_value_cache_, [](const auto& key_value_pair) {
+    const IdentityTokenCacheValue& value = key_value_pair.second;
+    return value.status() == IdentityTokenCacheValue::CACHE_STATUS_NOTFOUND;
+  });
+}
+
 }  // namespace extensions

chrome/browser/extensions/api/identity/identity_token_cache.h

@@ -112,6 +112,8 @@ class IdentityTokenCache {
   const AccessTokensCache& access_tokens_cache();
 
  private:
+  void EraseStaleTokens();
+
   AccessTokensCache access_tokens_cache_;
   std::map<ExtensionTokenKey, IdentityTokenCacheValue>
       intermediate_value_cache_;

chrome/browser/extensions/api/identity/identity_token_cache_unittest.cc

@@ -7,6 +7,7 @@
 #include <set>
 #include <string>
 
+#include "base/test/task_environment.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace extensions {
@@ -22,10 +23,17 @@ class IdentityTokenCacheTest : public testing::Test {
   void SetAccessToken(const std::string& ext_id,
                       const std::string& token_string,
                       const std::set<std::string>& scopes) {
-    ExtensionTokenKey key(ext_id, CoreAccountInfo(), scopes);
-    IdentityTokenCacheValue token = IdentityTokenCacheValue::CreateToken(
-        token_string, scopes, base::TimeDelta::FromSeconds(3600));
-    cache_.SetToken(key, token);
+    SetAccessTokenInternal(ext_id, token_string, scopes,
+                           base::TimeDelta::FromSeconds(3600));
+  }
+
+  void SetExpiredAccessToken(const std::string& ext_id,
+                             const std::string& token_string,
+                             const std::set<std::string>& scopes) {
+    // Token must not be expired at the insertion moment.
+    SetAccessTokenInternal(ext_id, token_string, scopes,
+                           base::TimeDelta::FromMilliseconds(1));
+    task_environment_.FastForwardBy(base::TimeDelta::FromMilliseconds(2));
   }
 
   void SetRemoteConsentApprovedToken(const std::string& ext_id,
@@ -46,6 +54,19 @@ class IdentityTokenCacheTest : public testing::Test {
   IdentityTokenCache& cache() { return cache_; }
 
  private:
+  void SetAccessTokenInternal(const std::string& ext_id,
+                              const std::string& token_string,
+                              const std::set<std::string>& scopes,
+                              base::TimeDelta time_to_live) {
+    ExtensionTokenKey key(ext_id, CoreAccountInfo(), scopes);
+    IdentityTokenCacheValue token = IdentityTokenCacheValue::CreateToken(
+        token_string, scopes, time_to_live);
+    cache_.SetToken(key, token);
+  }
+
+  base::test::TaskEnvironment task_environment_{
+      base::test::TaskEnvironment::TimeSource::MOCK_TIME};
+
   IdentityTokenCache cache_;
 };
 
@@ -61,6 +82,19 @@ TEST_F(IdentityTokenCacheTest, AccessTokenCacheHit) {
   EXPECT_EQ(scopes, cached_token.granted_scopes());
 }
 
+// The cache should return NOTFOUND status when a token expires.
+// Regression test for https://crbug.com/1127187.
+TEST_F(IdentityTokenCacheTest, ExpiredAccessTokenCacheHit) {
+  std::string token_string = "token";
+  std::set<std::string> scopes = {"foo", "bar"};
+  SetExpiredAccessToken(kDefaultExtensionId, token_string, scopes);
+
+  const IdentityTokenCacheValue& cached_token =
+      GetToken(kDefaultExtensionId, scopes);
+  EXPECT_EQ(IdentityTokenCacheValue::CACHE_STATUS_NOTFOUND,
+            cached_token.status());
+}
+
 TEST_F(IdentityTokenCacheTest, IntermediateValueCacheHit) {
   std::string consent_result = "result";
   std::set<std::string> scopes = {"foo", "bar"};
@@ -87,6 +121,21 @@ TEST_F(IdentityTokenCacheTest, CacheHitPriority) {
   EXPECT_EQ(scopes, cached_token.granted_scopes());
 }
 
+TEST_F(IdentityTokenCacheTest, CacheHitAfterExpired) {
+  std::string token_string = "token";
+  std::set<std::string> scopes = {"foo", "bar"};
+  SetExpiredAccessToken(kDefaultExtensionId, token_string, scopes);
+
+  std::string consent_result = "result";
+  SetRemoteConsentApprovedToken(kDefaultExtensionId, consent_result, scopes);
+
+  const IdentityTokenCacheValue& cached_token =
+      GetToken(kDefaultExtensionId, scopes);
+  EXPECT_EQ(IdentityTokenCacheValue::CACHE_STATUS_REMOTE_CONSENT_APPROVED,
+            cached_token.status());
+  EXPECT_EQ(consent_result, cached_token.consent_result());
+}
+
 TEST_F(IdentityTokenCacheTest, AccessTokenCacheMiss) {
   std::string ext_1 = "ext_1";
   std::set<std::string> scopes_1 = {"foo", "bar"};
@@ -259,4 +308,46 @@ TEST_F(IdentityTokenCacheTest, SubsetMatchCacheHitPriority) {
   EXPECT_EQ(scopes_small, cached_token.granted_scopes());
 }
 
+TEST_F(IdentityTokenCacheTest, SubsetMatchCacheHitPriorityOneExpired) {
+  std::set<std::string> scopes_smallest = {"foo"};
+  std::string consent_result = "result";
+  SetRemoteConsentApprovedToken(kDefaultExtensionId, consent_result,
+                                scopes_smallest);
+
+  std::set<std::string> scopes_small = {"foo", "bar"};
+  std::string token_string_small = "token_small";
+  SetExpiredAccessToken(kDefaultExtensionId, token_string_small, scopes_small);
+
+  std::set<std::string> scopes_large = {"foo", "bar", "foobar"};
+  std::string token_string_large = "token_large";
+  SetAccessToken(kDefaultExtensionId, token_string_large, scopes_large);
+
+  const IdentityTokenCacheValue& cached_token =
+      GetToken(kDefaultExtensionId, scopes_small);
+  EXPECT_EQ(IdentityTokenCacheValue::CACHE_STATUS_TOKEN, cached_token.status());
+  EXPECT_EQ(token_string_large, cached_token.token());
+  EXPECT_EQ(scopes_large, cached_token.granted_scopes());
+}
+
+TEST_F(IdentityTokenCacheTest, SubsetMatchCacheHitPriorityTwoExpired) {
+  std::set<std::string> scopes_smallest = {"foo"};
+  std::string consent_result = "result";
+  SetRemoteConsentApprovedToken(kDefaultExtensionId, consent_result,
+                                scopes_smallest);
+
+  std::set<std::string> scopes_small = {"foo", "bar"};
+  std::string token_string_small = "token_small";
+  SetExpiredAccessToken(kDefaultExtensionId, token_string_small, scopes_small);
+
+  std::set<std::string> scopes_large = {"foo", "bar", "foobar"};
+  std::string token_string_large = "token_large";
+  SetExpiredAccessToken(kDefaultExtensionId, token_string_large, scopes_large);
+
+  const IdentityTokenCacheValue& cached_token =
+      GetToken(kDefaultExtensionId, std::set<std::string>({"foo"}));
+  EXPECT_EQ(IdentityTokenCacheValue::CACHE_STATUS_REMOTE_CONSENT_APPROVED,
+            cached_token.status());
+  EXPECT_EQ(consent_result, cached_token.consent_result());
+}
+
 }  // namespace extensions