[identity] Use correct account in the remote consent flow

At the end of the remote consent flow, the browser makes the second
issueToken request. This request must use a login token of an account
that the user has chosen in the consent UI. Gaia returns an id of this
account in a consent result.

This CL updates IdentityGetAuthTokenFunction to use the id returned in a
consent result instead of the one that was originally chosen by Chrome.

This CL also moves IdentityTokenCacheValue to use static factory methods
instead of multiple constructor overloads. This makes obvious what type
of a cache value is created instead of relying on argument types.

Bug: 1026237
Change-Id: I59d5eefa7bbca91b59bb3d8cd69deca600580150
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2095302
Commit-Queue: Alex Ilin <alexilin@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#748742}

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

@@ -41,28 +41,55 @@
 
 namespace extensions {
 
-IdentityTokenCacheValue::IdentityTokenCacheValue()
-    : status_(CACHE_STATUS_NOTFOUND) {}
-
+IdentityTokenCacheValue::IdentityTokenCacheValue() = default;
 IdentityTokenCacheValue::IdentityTokenCacheValue(
-    const IssueAdviceInfo& issue_advice)
-    : status_(CACHE_STATUS_ADVICE), issue_advice_(issue_advice) {
-  expiration_time_ =
+    const IdentityTokenCacheValue& other) = default;
+IdentityTokenCacheValue::~IdentityTokenCacheValue() = default;
+
+// static
+IdentityTokenCacheValue IdentityTokenCacheValue::CreateIssueAdvice(
+    const IssueAdviceInfo& issue_advice) {
+  IdentityTokenCacheValue cache_value;
+  cache_value.status_ = CACHE_STATUS_ADVICE;
+  cache_value.issue_advice_ = issue_advice;
+  cache_value.expiration_time_ =
       base::Time::Now() + base::TimeDelta::FromSeconds(
                               identity_constants::kCachedIssueAdviceTTLSeconds);
+  return cache_value;
 }
 
-IdentityTokenCacheValue::IdentityTokenCacheValue(
-    const RemoteConsentResolutionData& resolution_data)
-    : status_(CACHE_STATUS_REMOTE_CONSENT), resolution_data_(resolution_data) {
-  expiration_time_ =
+// static
+IdentityTokenCacheValue IdentityTokenCacheValue::CreateRemoteConsent(
+    const RemoteConsentResolutionData& resolution_data) {
+  IdentityTokenCacheValue cache_value;
+  cache_value.status_ = CACHE_STATUS_REMOTE_CONSENT;
+  cache_value.resolution_data_ = resolution_data;
+  cache_value.expiration_time_ =
       base::Time::Now() + base::TimeDelta::FromSeconds(
                               identity_constants::kCachedIssueAdviceTTLSeconds);
+  return cache_value;
 }
 
-IdentityTokenCacheValue::IdentityTokenCacheValue(const std::string& token,
-                                                 base::TimeDelta time_to_live)
-    : status_(CACHE_STATUS_TOKEN), token_(token) {
+// static
+IdentityTokenCacheValue IdentityTokenCacheValue::CreateRemoteConsentApproved(
+    const std::string& consent_result) {
+  IdentityTokenCacheValue cache_value;
+  cache_value.status_ = CACHE_STATUS_REMOTE_CONSENT_APPROVED;
+  cache_value.consent_result_ = consent_result;
+  cache_value.expiration_time_ =
+      base::Time::Now() + base::TimeDelta::FromSeconds(
+                              identity_constants::kCachedIssueAdviceTTLSeconds);
+  return cache_value;
+}
+
+// static
+IdentityTokenCacheValue IdentityTokenCacheValue::CreateToken(
+    const std::string& token,
+    base::TimeDelta time_to_live) {
+  IdentityTokenCacheValue cache_value;
+  cache_value.status_ = CACHE_STATUS_TOKEN;
+  cache_value.token_ = token;
+
   // Remove 20 minutes from the ttl so cached tokens will have some time
   // to live any time they are returned.
   time_to_live -= base::TimeDelta::FromMinutes(20);
@@ -71,14 +98,10 @@ IdentityTokenCacheValue::IdentityTokenCacheValue(const std::string& token,
   if (time_to_live < zero_delta)
     time_to_live = zero_delta;
 
-  expiration_time_ = base::Time::Now() + time_to_live;
+  cache_value.expiration_time_ = base::Time::Now() + time_to_live;
+  return cache_value;
 }
 
-IdentityTokenCacheValue::IdentityTokenCacheValue(
-    const IdentityTokenCacheValue& other) = default;
-
-IdentityTokenCacheValue::~IdentityTokenCacheValue() {}
-
 IdentityTokenCacheValue::CacheValueStatus IdentityTokenCacheValue::status()
     const {
   if (is_expired())
@@ -96,6 +119,10 @@ const RemoteConsentResolutionData& IdentityTokenCacheValue::resolution_data()
   return resolution_data_;
 }
 
+const std::string& IdentityTokenCacheValue::consent_result() const {
+  return consent_result_;
+}
+
 const std::string& IdentityTokenCacheValue::token() const { return token_; }
 
 bool IdentityTokenCacheValue::is_expired() const {

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

@@ -44,35 +44,42 @@ namespace extensions {
 class IdentityTokenCacheValue {
  public:
   IdentityTokenCacheValue();
-  explicit IdentityTokenCacheValue(const IssueAdviceInfo& issue_advice);
-  explicit IdentityTokenCacheValue(
-      const RemoteConsentResolutionData& resolution_data);
-  IdentityTokenCacheValue(const std::string& token,
-                          base::TimeDelta time_to_live);
   IdentityTokenCacheValue(const IdentityTokenCacheValue& other);
   ~IdentityTokenCacheValue();
 
+  static IdentityTokenCacheValue CreateIssueAdvice(
+      const IssueAdviceInfo& issue_advice);
+  static IdentityTokenCacheValue CreateRemoteConsent(
+      const RemoteConsentResolutionData& resolution_data);
+  static IdentityTokenCacheValue CreateRemoteConsentApproved(
+      const std::string& consent_result);
+  static IdentityTokenCacheValue CreateToken(const std::string& token,
+                                             base::TimeDelta time_to_live);
+
   // Order of these entries is used to determine whether or not new
-  // entries supercede older ones in SetCachedToken.
+  // entries supersede older ones in SetCachedToken.
   enum CacheValueStatus {
     CACHE_STATUS_NOTFOUND,
     CACHE_STATUS_ADVICE,
     CACHE_STATUS_REMOTE_CONSENT,
+    CACHE_STATUS_REMOTE_CONSENT_APPROVED,
     CACHE_STATUS_TOKEN
   };
 
   CacheValueStatus status() const;
   const IssueAdviceInfo& issue_advice() const;
   const RemoteConsentResolutionData& resolution_data() const;
+  const std::string& consent_result() const;
   const std::string& token() const;
   const base::Time& expiration_time() const;
 
  private:
   bool is_expired() const;
 
-  CacheValueStatus status_;
+  CacheValueStatus status_ = CACHE_STATUS_NOTFOUND;
   IssueAdviceInfo issue_advice_;
   RemoteConsentResolutionData resolution_data_;
+  std::string consent_result_;
   std::string token_;
   base::Time expiration_time_;
 };

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

@@ -1455,8 +1455,8 @@ IN_PROC_BROWSER_TEST_F(GetAuthTokenFunctionTest, NonInteractiveCacheHit) {
   func->set_extension(extension.get());
 
   // pre-populate the cache with a token
-  IdentityTokenCacheValue token(kAccessToken,
-                                base::TimeDelta::FromSeconds(3600));
+  IdentityTokenCacheValue token = IdentityTokenCacheValue::CreateToken(
+      kAccessToken, base::TimeDelta::FromSeconds(3600));
   SetCachedToken(token);
 
   // Get a token. Should not require a GAIA request.
@@ -1491,8 +1491,8 @@ IN_PROC_BROWSER_TEST_F(GetAuthTokenFunctionTest,
   func->set_extension(extension.get());
 
   // pre-populate the cache with a token
-  IdentityTokenCacheValue token(kAccessToken,
-                                base::TimeDelta::FromSeconds(3600));
+  IdentityTokenCacheValue token = IdentityTokenCacheValue::CreateToken(
+      kAccessToken, base::TimeDelta::FromSeconds(3600));
   SetCachedTokenForAccount(account_info.account_id, token);
 
   if (id_api()->AreExtensionsRestrictedToPrimaryAccount()) {
@@ -1522,7 +1522,8 @@ IN_PROC_BROWSER_TEST_F(GetAuthTokenFunctionTest,
 
   // pre-populate the cache with advice
   IssueAdviceInfo info;
-  IdentityTokenCacheValue token(info);
+  IdentityTokenCacheValue token =
+      IdentityTokenCacheValue::CreateIssueAdvice(info);
   SetCachedToken(token);
 
   // Should return an error without a GAIA request.
@@ -1553,8 +1554,8 @@ IN_PROC_BROWSER_TEST_F(GetAuthTokenFunctionTest, InteractiveCacheHit) {
   RunFunctionAsync(func.get(), "[{\"interactive\": true}]");
 
   // Populate the cache with a token while the request is blocked.
-  IdentityTokenCacheValue token(kAccessToken,
-                                base::TimeDelta::FromSeconds(3600));
+  IdentityTokenCacheValue token = IdentityTokenCacheValue::CreateToken(
+      kAccessToken, base::TimeDelta::FromSeconds(3600));
   SetCachedToken(token);
 
   // When we wake up the request, it returns the cached token without
@@ -1579,8 +1580,8 @@ IN_PROC_BROWSER_TEST_F(GetAuthTokenFunctionTest, LoginInvalidatesTokenCache) {
   func->set_extension(extension.get());
 
   // pre-populate the cache with a token
-  IdentityTokenCacheValue token(kAccessToken,
-                                base::TimeDelta::FromSeconds(3600));
+  IdentityTokenCacheValue token = IdentityTokenCacheValue::CreateToken(
+      kAccessToken, base::TimeDelta::FromSeconds(3600));
   SetCachedToken(token);
 
   // Because the user is not signed in, the token will be removed,
@@ -2123,7 +2124,8 @@ IN_PROC_BROWSER_TEST_F(RemoveCachedAuthTokenFunctionTest, NotFound) {
 
 IN_PROC_BROWSER_TEST_F(RemoveCachedAuthTokenFunctionTest, Advice) {
   IssueAdviceInfo info;
-  IdentityTokenCacheValue advice(info);
+  IdentityTokenCacheValue advice =
+      IdentityTokenCacheValue::CreateIssueAdvice(info);
   SetCachedToken(advice);
   EXPECT_TRUE(InvalidateDefaultToken());
   EXPECT_EQ(IdentityTokenCacheValue::CACHE_STATUS_ADVICE,
@@ -2131,8 +2133,8 @@ IN_PROC_BROWSER_TEST_F(RemoveCachedAuthTokenFunctionTest, Advice) {
 }
 
 IN_PROC_BROWSER_TEST_F(RemoveCachedAuthTokenFunctionTest, NonMatchingToken) {
-  IdentityTokenCacheValue token("non_matching_token",
-                                base::TimeDelta::FromSeconds(3600));
+  IdentityTokenCacheValue token = IdentityTokenCacheValue::CreateToken(
+      "non_matching_token", base::TimeDelta::FromSeconds(3600));
   SetCachedToken(token);
   EXPECT_TRUE(InvalidateDefaultToken());
   EXPECT_EQ(IdentityTokenCacheValue::CACHE_STATUS_TOKEN,
@@ -2141,8 +2143,8 @@ IN_PROC_BROWSER_TEST_F(RemoveCachedAuthTokenFunctionTest, NonMatchingToken) {
 }
 
 IN_PROC_BROWSER_TEST_F(RemoveCachedAuthTokenFunctionTest, MatchingToken) {
-  IdentityTokenCacheValue token(kAccessToken,
-                                base::TimeDelta::FromSeconds(3600));
+  IdentityTokenCacheValue token = IdentityTokenCacheValue::CreateToken(
+      kAccessToken, base::TimeDelta::FromSeconds(3600));
   SetCachedToken(token);
   EXPECT_EQ(IdentityTokenCacheValue::CACHE_STATUS_TOKEN,
             GetCachedToken().status());

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

@@ -383,6 +383,8 @@ void IdentityGetAuthTokenFunction::StartMintTokenFlow(
       CompleteFunctionWithError(identity_constants::kNoGrant);
       return;
     }
+    // TODO(https://crbug.com/1026237): figure out whether this can be ignored
+    // when running the remote consent approval.
     if (!id_api->mint_queue()->empty(
             IdentityMintRequestQueue::MINT_TYPE_INTERACTIVE, token_key_)) {
       // Another call is going through a consent UI.
@@ -390,6 +392,7 @@ void IdentityGetAuthTokenFunction::StartMintTokenFlow(
       return;
     }
   }
+
   id_api->mint_queue()->RequestStart(type, token_key_, this);
 }
 
@@ -469,6 +472,14 @@ void IdentityGetAuthTokenFunction::StartMintToken(
         resolution_data_ = cache_entry.resolution_data();
         StartMintTokenFlow(IdentityMintRequestQueue::MINT_TYPE_INTERACTIVE);
         break;
+
+      case IdentityTokenCacheValue::CACHE_STATUS_REMOTE_CONSENT_APPROVED:
+        consent_result_ = cache_entry.consent_result();
+        should_prompt_for_scopes_ = false;
+        should_prompt_for_signin_ = false;
+        gaia_mint_token_mode_ = OAuth2MintTokenFlow::MODE_MINT_TOKEN_NO_FORCE;
+        StartTokenKeyAccountAccessTokenRequest();
+        break;
     }
   } else {
     DCHECK(type == IdentityMintRequestQueue::MINT_TYPE_INTERACTIVE);
@@ -485,6 +496,13 @@ void IdentityGetAuthTokenFunction::StartMintToken(
       case IdentityTokenCacheValue::CACHE_STATUS_REMOTE_CONSENT:
         ShowRemoteConsentDialog(resolution_data_);
         break;
+      case IdentityTokenCacheValue::CACHE_STATUS_REMOTE_CONSENT_APPROVED:
+        consent_result_ = cache_entry.consent_result();
+        should_prompt_for_scopes_ = false;
+        should_prompt_for_signin_ = false;
+        gaia_mint_token_mode_ = OAuth2MintTokenFlow::MODE_MINT_TOKEN_NO_FORCE;
+        StartTokenKeyAccountAccessTokenRequest();
+        break;
     }
   }
 }
@@ -494,8 +512,8 @@ void IdentityGetAuthTokenFunction::OnMintTokenSuccess(
     int time_to_live) {
   TRACE_EVENT_NESTABLE_ASYNC_INSTANT0("identity", "OnMintTokenSuccess", this);
 
-  IdentityTokenCacheValue token(access_token,
-                                base::TimeDelta::FromSeconds(time_to_live));
+  IdentityTokenCacheValue token = IdentityTokenCacheValue::CreateToken(
+      access_token, base::TimeDelta::FromSeconds(time_to_live));
   IdentityAPI::GetFactoryInstance()
       ->Get(GetProfile())
       ->SetCachedToken(token_key_, token);
@@ -538,7 +556,8 @@ void IdentityGetAuthTokenFunction::OnIssueAdviceSuccess(
 
   IdentityAPI::GetFactoryInstance()
       ->Get(GetProfile())
-      ->SetCachedToken(token_key_, IdentityTokenCacheValue(issue_advice));
+      ->SetCachedToken(
+          token_key_, IdentityTokenCacheValue::CreateIssueAdvice(issue_advice));
   CompleteMintTokenFlow();
 
   should_prompt_for_signin_ = false;
@@ -563,7 +582,8 @@ void IdentityGetAuthTokenFunction::OnRemoteConsentSuccess(
 
   IdentityAPI::GetFactoryInstance()
       ->Get(GetProfile())
-      ->SetCachedToken(token_key_, IdentityTokenCacheValue(resolution_data));
+      ->SetCachedToken(token_key_, IdentityTokenCacheValue::CreateRemoteConsent(
+                                       resolution_data));
   should_prompt_for_signin_ = false;
   resolution_data_ = resolution_data;
   CompleteMintTokenFlow();
@@ -666,7 +686,7 @@ void IdentityGetAuthTokenFunction::OnGaiaFlowCompleted(
   TRACE_EVENT_NESTABLE_ASYNC_INSTANT0("identity", "OnGaiaFlowCompleted", this);
   int time_to_live;
   if (!expiration.empty() && base::StringToInt(expiration, &time_to_live)) {
-    IdentityTokenCacheValue token_value(
+    IdentityTokenCacheValue token_value = IdentityTokenCacheValue::CreateToken(
         access_token, base::TimeDelta::FromSeconds(time_to_live));
     IdentityAPI::GetFactoryInstance()
         ->Get(GetProfile())
@@ -712,12 +732,29 @@ void IdentityGetAuthTokenFunction::OnGaiaRemoteConsentFlowApproved(
     const std::string& gaia_id) {
   // TODO(crbug.com/1026237): Reuse the same gaia id for this extension the next
   // time.
+  TRACE_EVENT_NESTABLE_ASYNC_INSTANT1(
+      "identity", "OnGaiaRemoteConsentFlowApproved", this, "gaia_id", gaia_id);
   DCHECK(!consent_result.empty());
+
+  CompleteMintTokenFlow();
+  base::Optional<AccountInfo> account =
+      IdentityManagerFactory::GetForProfile(GetProfile())
+          ->FindExtendedAccountInfoForAccountWithRefreshTokenByGaiaId(gaia_id);
+  if (!account) {
+    CompleteFunctionWithError(identity_constants::kUserNotSignedIn);
+    return;
+  }
+
+  token_key_.account_id = account->account_id;
   consent_result_ = consent_result;
   should_prompt_for_scopes_ = false;
   should_prompt_for_signin_ = false;
-  gaia_mint_token_mode_ = OAuth2MintTokenFlow::MODE_MINT_TOKEN_NO_FORCE;
-  StartTokenKeyAccountAccessTokenRequest();
+  IdentityAPI::GetFactoryInstance()
+      ->Get(GetProfile())
+      ->SetCachedToken(
+          token_key_,
+          IdentityTokenCacheValue::CreateRemoteConsentApproved(consent_result));
+  StartMintTokenFlow(IdentityMintRequestQueue::MINT_TYPE_NONINTERACTIVE);
 }
 
 void IdentityGetAuthTokenFunction::OnGetAccessTokenComplete(

chrome/browser/ui/webui/identity_internals_ui.cc

@@ -186,8 +186,10 @@ std::string IdentityInternalsUIMessageHandler::GetStatus(
   switch (token_cache_value.status()) {
     case extensions::IdentityTokenCacheValue::CACHE_STATUS_ADVICE:
     case extensions::IdentityTokenCacheValue::CACHE_STATUS_REMOTE_CONSENT:
-      // Fallthrough to NOT FOUND case, as ADVICE and REMOTE_CONSENT are short
-      // lived.
+    case extensions::IdentityTokenCacheValue::
+        CACHE_STATUS_REMOTE_CONSENT_APPROVED:
+      // Fallthrough to NOT FOUND case, as ADVICE, REMOTE_CONSENT and
+      // REMOTE_CONSENT_APPROVED are short lived.
     case extensions::IdentityTokenCacheValue::CACHE_STATUS_NOTFOUND:
       return "Not Found";
     case extensions::IdentityTokenCacheValue::CACHE_STATUS_TOKEN:

chrome/browser/ui/webui/identity_internals_ui_browsertest.cc

@@ -48,8 +48,8 @@ void IdentityInternalsUIBrowserTest::AddTokenToCache(
     const std::vector<std::string>& scopes,
     int time_to_live) {
   extensions::IdentityTokenCacheValue token_cache_value =
-      extensions::IdentityTokenCacheValue(token_id,
-          base::TimeDelta::FromSeconds(time_to_live));
+      extensions::IdentityTokenCacheValue::CreateToken(
+          token_id, base::TimeDelta::FromSeconds(time_to_live));
   extensions::ExtensionTokenKey key(
       extension_id, CoreAccountId("account_id"),
       std::set<std::string>(scopes.begin(), scopes.end()));