Fix Mirror cookie validity mismatch

In certain cases, Gaia's /ListAccounts APIs claims accounts to be in a
valid state when they are actually invalid. This causes issues with
account reconciliation because |AccountReconcilor| uses /ListAccounts as
the source of truth and is unable to detect this state.
In this state, certain Google properties like Gmail for example send a
Mirror re-authentication header for the offending account. However, even
after the user re-authenticates the account, account reconciliation ends
up being a no-op because the reconcilor thinks that the user's cookies
are valid and there is nothing to reconcile.

Fix this by storing some flags in |CookieReminter| which are set when
we receive Mirror re-authentication headers from Gaia, and are cleared
when the next reconciliation cycle begins.

This is a temporary fix until we can submit https://crrev.com/c/1871591.

Bug: 1012649
Change-Id: I04fd971cdc788094f038ee808b4a38003ef48e22
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1883652Reviewed-by: David Roger <droger@chromium.org>
Commit-Queue: Anastasiia Nikolaienko <anastasiian@chromium.org>
Cr-Commit-Position: refs/heads/master@{#710870}

chrome/browser/signin/chrome_signin_helper.cc

@@ -197,7 +197,8 @@ void ProcessMirrorHeader(
   //    have been forced through an online in-browser sign-in for sensitive
   //    webpages, thereby decreasing their session validity. After their session
   //    expires, they will receive a "Mirror" re-authentication request for all
-  //    Google web properties.
+  //    Google web properties. Another case when this can be triggered is
+  //    https://crbug.com/1012649.
   // 3. Displaying the Account Manager for managing accounts.
 
   // 1. Going incognito.
@@ -217,6 +218,26 @@ void ProcessMirrorHeader(
       return;
     }
 
+    // The account's cookie is invalid but the cookie has not been removed by
+    // |AccountReconcilor|. Ideally, this should not happen. At this point,
+    // |AccountReconcilor| cannot detect this state because its source of truth
+    // (/ListAccounts) is giving us false positives (claiming an invalid account
+    // to be valid). We need to store that this account's cookie is actually
+    // invalid, so that if/when this account is re-authenticated, we can force a
+    // reconciliation for this account instead of treating it as a no-op.
+    // See https://crbug.com/1012649 for details.
+
+    signin::IdentityManager* const identity_manager =
+        IdentityManagerFactory::GetForProfile(profile);
+    base::Optional<AccountInfo> maybe_account_info =
+        identity_manager
+            ->FindExtendedAccountInfoForAccountWithRefreshTokenByEmailAddress(
+                manage_accounts_params.email);
+    if (maybe_account_info.has_value()) {
+      account_reconcilor->ForceCookieRemintingOnNextTokenUpdate(
+          maybe_account_info.value());
+    }
+
     // Display a re-authentication dialog.
     chromeos::InlineLoginHandlerDialogChromeOS::Show(
         manage_accounts_params.email);

components/signin/core/browser/BUILD.gn

@@ -23,6 +23,8 @@ static_library("browser") {
     "chrome_connected_header_helper.h",
     "consistency_cookie_manager_base.cc",
     "consistency_cookie_manager_base.h",
+    "cookie_reminter.cc",
+    "cookie_reminter.h",
     "cookie_settings_util.cc",
     "cookie_settings_util.h",
     "dice_account_reconcilor_delegate.cc",

components/signin/core/browser/account_reconcilor.cc

@@ -22,6 +22,7 @@
 #include "build/build_config.h"
 #include "components/signin/core/browser/account_reconcilor_delegate.h"
 #include "components/signin/core/browser/consistency_cookie_manager_base.h"
+#include "components/signin/core/browser/cookie_reminter.h"
 #include "components/signin/public/base/account_consistency_method.h"
 #include "components/signin/public/base/signin_client.h"
 #include "components/signin/public/base/signin_metrics.h"
@@ -339,6 +340,7 @@ void AccountReconcilor::RegisterWithIdentityManager() {
   if (registered_with_identity_manager_)
     return;
 
+  cookie_reminter_ = std::make_unique<CookieReminter>(identity_manager_);
   identity_manager_->AddObserver(this);
   registered_with_identity_manager_ = true;
 }
@@ -348,6 +350,7 @@ void AccountReconcilor::UnregisterWithIdentityManager() {
   if (!registered_with_identity_manager_)
     return;
 
+  cookie_reminter_.reset();
   identity_manager_->RemoveObserver(this);
   registered_with_identity_manager_ = false;
 }
@@ -361,6 +364,11 @@ AccountReconcilor::GetScopedSyncDataDeletion() {
   return base::WrapUnique(new ScopedSyncedDataDeletion(this));
 }
 
+void AccountReconcilor::ForceCookieRemintingOnNextTokenUpdate(
+    const CoreAccountInfo& account_info) {
+  cookie_reminter_->ForceCookieRemintingOnNextTokenUpdate(account_info);
+}
+
 void AccountReconcilor::AddObserver(Observer* observer) {
   observer_list_.AddObserver(observer);
 }
@@ -535,6 +543,13 @@ void AccountReconcilor::FinishReconcileWithMultiloginEndpoint(
   DCHECK(!set_accounts_in_progress_);
   DCHECK_EQ(AccountReconcilorState::ACCOUNT_RECONCILOR_RUNNING, state_);
 
+#if defined(OS_CHROMEOS)
+  // Cookie may need to be reminted on Chrome OS. See https://crbug.com/1012649
+  // for details.
+  if (cookie_reminter_->RemintCookieIfRequired())
+    gaia_accounts.clear();
+#endif
+
   bool primary_has_error =
       identity_manager_->HasAccountWithRefreshTokenInPersistentErrorState(
           primary_account);
@@ -764,6 +779,14 @@ void AccountReconcilor::FinishReconcile(
       (number_gaia_accounts > 0) && (first_account != gaia_accounts[0].id);
 
   bool rebuild_cookie = first_account_mismatch || (removed_from_cookie > 0);
+
+#if defined(OS_CHROMEOS)
+  // Cookie may need to be reminted on Chrome OS. See https://crbug.com/1012649
+  // for details.
+  if (cookie_reminter_->RemintCookieIfRequired())
+    rebuild_cookie = true;
+#endif
+
   std::vector<gaia::ListedAccount> original_gaia_accounts = gaia_accounts;
   if (rebuild_cookie) {
     VLOG(1) << "AccountReconcilor::FinishReconcile: rebuild cookie";

components/signin/core/browser/account_reconcilor.h

@@ -34,6 +34,7 @@ class ConsistencyCookieManagerBase;
 enum class SetAccountsInCookieResult;
 }
 
+class CookieReminter;
 class SigninClient;
 
 class AccountReconcilor : public KeyedService,
@@ -128,6 +129,11 @@ class AccountReconcilor : public KeyedService,
   // from being invalidated during the deletion.
   std::unique_ptr<ScopedSyncedDataDeletion> GetScopedSyncDataDeletion();
 
+  // Forces a cookie reminting if/when the refresh token for |account_info| is
+  // updated.
+  void ForceCookieRemintingOnNextTokenUpdate(
+      const CoreAccountInfo& account_info);
+
  private:
   friend class AccountReconcilorTest;
   friend class DiceBrowserTest;
@@ -358,6 +364,7 @@ class AccountReconcilor : public KeyedService,
   std::vector<CoreAccountId> add_to_cookie_;  // Progress of AddAccount calls.
   bool set_accounts_in_progress_;             // Progress of SetAccounts calls.
   bool chrome_accounts_changed_;
+  std::unique_ptr<CookieReminter> cookie_reminter_;
 
   // Used for the Lock.
   // StartReconcile() is blocked while this is > 0.

components/signin/core/browser/cookie_reminter.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/signin/core/browser/cookie_reminter.h"
+#include "components/signin/public/identity_manager/accounts_cookie_mutator.h"
+
+namespace {
+
+bool DoesAccountRequireCookieReminting(
+    const std::vector<CoreAccountInfo>& accounts_requiring_cookie_remint,
+    const CoreAccountInfo& account_info) {
+  for (const CoreAccountInfo& account_requiring_remint :
+       accounts_requiring_cookie_remint) {
+    if (account_info.gaia == account_requiring_remint.gaia) {
+      return true;
+    }
+  }
+
+  return false;
+}
+
+}  // namespace
+
+CookieReminter::CookieReminter(signin::IdentityManager* identity_manager)
+    : identity_manager_(identity_manager) {
+  identity_manager_->AddObserver(this);
+}
+
+CookieReminter::~CookieReminter() {
+  identity_manager_->RemoveObserver(this);
+}
+
+void CookieReminter::ForceCookieRemintingOnNextTokenUpdate(
+    const CoreAccountInfo& account_info) {
+  if (DoesAccountRequireCookieReminting(accounts_requiring_cookie_remint_,
+                                        account_info)) {
+    return;
+  }
+
+  accounts_requiring_cookie_remint_.emplace_back(account_info);
+}
+
+bool CookieReminter::RemintCookieIfRequired() {
+  if (!is_forced_cookie_reminting_required_)
+    return false;
+
+  identity_manager_->GetAccountsCookieMutator()->LogOutAllAccounts(
+      gaia::GaiaSource::kChromeOS);
+  accounts_requiring_cookie_remint_.clear();
+  is_forced_cookie_reminting_required_ = false;
+  return true;
+}
+
+void CookieReminter::OnRefreshTokenUpdatedForAccount(
+    const CoreAccountInfo& account_info) {
+  if (DoesAccountRequireCookieReminting(accounts_requiring_cookie_remint_,
+                                        account_info)) {
+    // Cookies are going to be reminted for all accounts.
+    accounts_requiring_cookie_remint_.clear();
+    is_forced_cookie_reminting_required_ = true;
+  }
+}

components/signin/core/browser/cookie_reminter.h

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+#ifndef COMPONENTS_SIGNIN_CORE_BROWSER_COOKIE_REMINTER_H_
+#define COMPONENTS_SIGNIN_CORE_BROWSER_COOKIE_REMINTER_H_
+
+#include <vector>
+
+#include "components/signin/public/identity_manager/identity_manager.h"
+
+// Stores accounts with invalid cookies, which cannot be detected by
+// /ListAccounts, so we can force cookie reminting when account is
+// reauthenticated.
+//
+// Subscribed to |OnRefreshTokenUpdatedForAccount| of IdentityManager, calls
+// |AccountsCookieMutator::LogOutAllAccounts| after refresh token update of
+// any of the accounts that have been added to
+// |ForceCookieRemintingOnNextTokenUpdate|.
+class CookieReminter : public signin::IdentityManager::Observer {
+ public:
+  explicit CookieReminter(signin::IdentityManager* identity_manager);
+  ~CookieReminter() override;
+
+  // Forces a cookie reminting if/when the refresh token for |account_info| is
+  // updated.
+  void ForceCookieRemintingOnNextTokenUpdate(
+      const CoreAccountInfo& account_info);
+
+  // If there are accounts that require cookie reminting, calls
+  // |AccountsCookieMutator::LogOutAllAccounts| and returns true. Otherwise
+  // returns false.
+  bool RemintCookieIfRequired();
+
+  // Overridden from signin::IdentityManager::Observer.
+  void OnRefreshTokenUpdatedForAccount(
+      const CoreAccountInfo& account_info) override;
+
+ private:
+  bool is_forced_cookie_reminting_required_ = false;
+  signin::IdentityManager* identity_manager_;
+  std::vector<CoreAccountInfo> accounts_requiring_cookie_remint_;
+};
+
+#endif  // COMPONENTS_SIGNIN_CORE_BROWSER_COOKIE_REMINTER_H_