[signin] Reconcilor handles invalidated cookies

Invalidated cookies were not handled correctly. This CL adds proper
support for them.

TBR=treib

Bug: 854799
Change-Id: Iad3ee729172a7e44d737b25fb665f0009d78de84
Reviewed-on: https://chromium-review.googlesource.com/1110218
Commit-Queue: David Roger <droger@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570825}

chrome/browser/supervised_user/child_accounts/child_account_service_unittest.cc

@@ -64,10 +64,10 @@ TEST_F(ChildAccountServiceTest, GetGoogleAuthState) {
 
   // A valid, signed-in account means authenticated.
   gaia_cookie_manager_service_->SetListAccountsResponseOneAccountWithParams(
-      "me@example.com", "abcdef",
-      /* is_email_valid = */ true,
-      /* is_signed_out = */ false,
-      /* verified = */ true);
+      {"me@example.com", "abcdef",
+       /* valid = */ true,
+       /* is_signed_out = */ false,
+       /* verified = */ true});
   gaia_cookie_manager_service_->TriggerListAccounts("ChildAccountServiceTest");
   content::RunAllTasksUntilIdle();
   EXPECT_EQ(ChildAccountService::AuthState::AUTHENTICATED,
@@ -75,10 +75,10 @@ TEST_F(ChildAccountServiceTest, GetGoogleAuthState) {
 
   // An invalid (but signed-in) account means not authenticated.
   gaia_cookie_manager_service_->SetListAccountsResponseOneAccountWithParams(
-      "me@example.com", "abcdef",
-      /* is_email_valid = */ false,
-      /* is_signed_out = */ false,
-      /* verified = */ true);
+      {"me@example.com", "abcdef",
+       /* valid = */ false,
+       /* is_signed_out = */ false,
+       /* verified = */ true});
   gaia_cookie_manager_service_->TriggerListAccounts("ChildAccountServiceTest");
   content::RunAllTasksUntilIdle();
   EXPECT_EQ(ChildAccountService::AuthState::NOT_AUTHENTICATED,
@@ -86,10 +86,10 @@ TEST_F(ChildAccountServiceTest, GetGoogleAuthState) {
 
   // A valid but not signed-in account means not authenticated.
   gaia_cookie_manager_service_->SetListAccountsResponseOneAccountWithParams(
-      "me@example.com", "abcdef",
-      /* is_email_valid = */ true,
-      /* is_signed_out = */ true,
-      /* verified = */ true);
+      {"me@example.com", "abcdef",
+       /* valid = */ true,
+       /* is_signed_out = */ true,
+       /* verified = */ true});
   gaia_cookie_manager_service_->TriggerListAccounts("ChildAccountServiceTest");
   content::RunAllTasksUntilIdle();
   EXPECT_EQ(ChildAccountService::AuthState::NOT_AUTHENTICATED,

components/signin/core/browser/account_reconcilor.cc

@@ -466,15 +466,7 @@ void AccountReconcilor::FinishReconcile(
   VLOG(1) << "AccountReconcilor::FinishReconcile";
   DCHECK(add_to_cookie_.empty());
 
-  std::string first_account = delegate_->GetFirstGaiaAccountForReconcile(
-      chrome_accounts, gaia_accounts, primary_account, first_execution_);
-  // |first_account| must be in |chrome_accounts|.
-  DCHECK(first_account.empty() ||
-         base::ContainsValue(chrome_accounts, first_account));
   size_t number_gaia_accounts = gaia_accounts.size();
-  bool first_account_mismatch =
-      (number_gaia_accounts > 0) && (first_account != gaia_accounts[0].id);
-
   // If there are any accounts in the gaia cookie but not in chrome, then
   // those accounts need to be removed from the cookie.  This means we need
   // to blow the cookie away.
@@ -486,6 +478,12 @@ void AccountReconcilor::FinishReconcile(
     }
   }
 
+  std::string first_account = delegate_->GetFirstGaiaAccountForReconcile(
+      chrome_accounts, gaia_accounts, primary_account, first_execution_,
+      removed_from_cookie > 0);
+  bool first_account_mismatch =
+      (number_gaia_accounts > 0) && (first_account != gaia_accounts[0].id);
+
   bool rebuild_cookie = first_account_mismatch || (removed_from_cookie > 0);
   std::vector<gaia::ListedAccount> original_gaia_accounts = gaia_accounts;
   if (rebuild_cookie) {
@@ -499,13 +497,18 @@ void AccountReconcilor::FinishReconcile(
 
   if (first_account.empty()) {
     DCHECK(!delegate_->ShouldAbortReconcileIfPrimaryHasError());
-    // Gaia cookie has been cleared or was already empty.
-    DCHECK((first_account_mismatch && rebuild_cookie) ||
-           (number_gaia_accounts == 0));
     RevokeAllSecondaryTokens(primary_account, chrome_accounts);
   } else {
     // Create a list of accounts that need to be added to the Gaia cookie.
-    add_to_cookie_.push_back(first_account);
+    if (base::ContainsValue(chrome_accounts, first_account)) {
+      add_to_cookie_.push_back(first_account);
+    } else {
+      // If the first account is not empty and not in chrome_accounts, it is
+      // impossible to rebuild it. It must be already the current default
+      // account, and no logout can happen.
+      DCHECK_EQ(gaia_accounts[0].gaia_id, first_account);
+      DCHECK(!rebuild_cookie);
+    }
     for (size_t i = 0; i < chrome_accounts.size(); ++i) {
       if (chrome_accounts[i] != first_account)
         add_to_cookie_.push_back(chrome_accounts[i]);

components/signin/core/browser/account_reconcilor.h

@@ -159,7 +159,7 @@ class AccountReconcilor : public KeyedService,
   FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest, StartReconcileBadPrimary);
   FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest, StartReconcileOnlyOnce);
   FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest, Lock);
-  FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest,
+  FRIEND_TEST_ALL_PREFIXES(AccountReconcilorMethodParamTest,
                            StartReconcileWithSessionInfoExpiredDefault);
   FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest,
                            AddAccountToCookieCompletedWithBogusAccount);

components/signin/core/browser/account_reconcilor_delegate.cc

@@ -31,7 +31,8 @@ std::string AccountReconcilorDelegate::GetFirstGaiaAccountForReconcile(
     const std::vector<std::string>& chrome_accounts,
     const std::vector<gaia::ListedAccount>& gaia_accounts,
     const std::string& primary_account,
-    bool first_execution) const {
+    bool first_execution,
+    bool will_logout) const {
   return std::string();
 }
 

components/signin/core/browser/account_reconcilor_delegate.h

@@ -51,11 +51,15 @@ class AccountReconcilorDelegate {
   // Returns the first account to add in the Gaia cookie.
   // If this returns an empty string, the user must be logged out of all
   // accounts.
+  // |first_execution| is true for the first reconciliation after startup.
+  // |will_logout| is true if the reconcilor will perform a logout no matter
+  // what is returned by this function.
   virtual std::string GetFirstGaiaAccountForReconcile(
       const std::vector<std::string>& chrome_accounts,
       const std::vector<gaia::ListedAccount>& gaia_accounts,
       const std::string& primary_account,
-      bool first_execution) const;
+      bool first_execution,
+      bool will_logout) const;
 
   // Returns whether secondary accounts should be cleared at the beginning of
   // the reconcile.

components/signin/core/browser/dice_account_reconcilor_delegate.cc

@@ -49,16 +49,14 @@ std::string DiceAccountReconcilorDelegate::GetFirstGaiaAccountForReconcile(
     const std::vector<std::string>& chrome_accounts,
     const std::vector<gaia::ListedAccount>& gaia_accounts,
     const std::string& primary_account,
-    bool first_execution) const {
-  if (chrome_accounts.empty())
-    return std::string();  // No Chrome account, log out.
-
-  bool valid_primary_account =
+    bool first_execution,
+    bool will_logout) const {
+  bool primary_account_has_token =
       !primary_account.empty() &&
       base::ContainsValue(chrome_accounts, primary_account);
 
   if (gaia_accounts.empty()) {
-    if (valid_primary_account)
+    if (primary_account_has_token)
       return primary_account;
 
     // Try the last known account. This happens when the cookies are cleared
@@ -67,36 +65,44 @@ std::string DiceAccountReconcilorDelegate::GetFirstGaiaAccountForReconcile(
       return last_known_first_account_;
 
     // As a last resort, use the first Chrome account.
-    return chrome_accounts[0];
+    return chrome_accounts.empty() ? std::string() : chrome_accounts[0];
   }
 
   const std::string& first_gaia_account = gaia_accounts[0].id;
-  bool first_gaia_account_is_valid =
-      gaia_accounts[0].valid &&
+  bool first_gaia_account_has_token =
       base::ContainsValue(chrome_accounts, first_gaia_account);
 
-  if (!first_gaia_account_is_valid && (primary_account == first_gaia_account)) {
-    // The primary account is also the first Gaia account, and is invalid.
+  if (!first_gaia_account_has_token &&
+      (primary_account == first_gaia_account) && gaia_accounts[0].valid) {
+    // The primary account is also the first Gaia account, and has no token.
     // Logout everything.
     return std::string();
   }
 
+  // If the primary Chrome account and the default Gaia account are both in
+  // error, then the first gaia account can be kept, to avoid logging the user
+  // out of their other accounts.
+  // It's only possible when the reconcilor will not perform a logout, because
+  // that account cannot be rebuilt.
+  if (!primary_account_has_token && !gaia_accounts[0].valid && !will_logout)
+    return first_gaia_account;
+
   if (first_execution) {
     // On first execution, try the primary account, and then the first Gaia
     // account.
-    if (valid_primary_account)
+    if (primary_account_has_token)
       return primary_account;
-    if (first_gaia_account_is_valid)
+    if (first_gaia_account_has_token)
       return first_gaia_account;
     // As a last resort, use the first Chrome account.
-    return chrome_accounts[0];
+    return chrome_accounts.empty() ? std::string() : chrome_accounts[0];
   }
 
   // While Chrome is running, try the first Gaia account, and then the
   // primary account.
-  if (first_gaia_account_is_valid)
+  if (first_gaia_account_has_token)
     return first_gaia_account;
-  if (valid_primary_account)
+  if (primary_account_has_token)
     return primary_account;
 
   // Changing the first Gaia account while Chrome is running would be

components/signin/core/browser/dice_account_reconcilor_delegate.h

@@ -30,7 +30,8 @@ class DiceAccountReconcilorDelegate : public AccountReconcilorDelegate {
       const std::vector<std::string>& chrome_accounts,
       const std::vector<gaia::ListedAccount>& gaia_accounts,
       const std::string& primary_account,
-      bool first_execution) const override;
+      bool first_execution,
+      bool will_logout) const override;
   RevokeTokenOption ShouldRevokeSecondaryTokensBeforeReconcile(
       const std::vector<gaia::ListedAccount>& gaia_accounts) override;
   void OnReconcileFinished(const std::string& first_account,

components/signin/core/browser/fake_gaia_cookie_manager_service.cc

@@ -4,6 +4,7 @@
 
 #include "components/signin/core/browser/fake_gaia_cookie_manager_service.h"
 
+#include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "components/signin/core/browser/profile_oauth2_token_service.h"
 #include "google_apis/gaia/gaia_constants.h"
@@ -37,78 +38,58 @@ void FakeGaiaCookieManagerService::SetListAccountsResponseWebLoginRequired() {
       "Info=WebLoginRequired", net::HTTP_OK, net::URLRequestStatus::SUCCESS);
 }
 
-void FakeGaiaCookieManagerService::SetListAccountsResponseNoAccounts() {
+void FakeGaiaCookieManagerService::SetListAccountsResponseWithParams(
+    const std::vector<CookieParams>& params) {
   DCHECK(url_fetcher_factory_);
+
+  std::vector<std::string> response_body;
+  for (const auto& param : params) {
+    std::string response_part = base::StringPrintf(
+        "[\"b\", 0, \"n\", \"%s\", \"p\", 0, 0, 0, 0, %d, \"%s\"",
+        param.email.c_str(), param.valid ? 1 : 0, param.gaia_id.c_str());
+    if (param.signed_out || !param.verified) {
+      response_part +=
+          base::StringPrintf(", null, null, null, %d, %d",
+                             param.signed_out ? 1 : 0, param.verified ? 1 : 0);
+    }
+    response_part += "]";
+    response_body.push_back(response_part);
+  }
+
   url_fetcher_factory_->SetFakeResponse(
       GaiaUrls::GetInstance()->ListAccountsURLWithSource(
           GaiaConstants::kChromeSource),
-      "[\"f\", []]", net::HTTP_OK, net::URLRequestStatus::SUCCESS);
+      std::string("[\"f\", [") + base::JoinString(response_body, ", ") + "]]",
+      net::HTTP_OK, net::URLRequestStatus::SUCCESS);
+}
+
+void FakeGaiaCookieManagerService::SetListAccountsResponseNoAccounts() {
+  SetListAccountsResponseWithParams({});
 }
 
 void FakeGaiaCookieManagerService::SetListAccountsResponseOneAccount(
-    const char* email,
-    const char* gaia_id) {
-  DCHECK(url_fetcher_factory_);
-  url_fetcher_factory_->SetFakeResponse(
-      GaiaUrls::GetInstance()->ListAccountsURLWithSource(
-          GaiaConstants::kChromeSource),
-      base::StringPrintf(
-          "[\"f\", [[\"b\", 0, \"n\", \"%s\", \"p\", 0, 0, 0, 0, 1, \"%s\"]]]",
-          email, gaia_id),
-      net::HTTP_OK, net::URLRequestStatus::SUCCESS);
+    const std::string& email,
+    const std::string& gaia_id) {
+  CookieParams params = {email, gaia_id, true /* valid */,
+                         false /* signed_out */, true /* verified */};
+  SetListAccountsResponseWithParams({params});
 }
 
 void FakeGaiaCookieManagerService::SetListAccountsResponseOneAccountWithParams(
-    const char* email,
-    const char* gaia_id,
-    bool is_email_valid,
-    bool signed_out,
-    bool verified) {
-  DCHECK(url_fetcher_factory_);
-  url_fetcher_factory_->SetFakeResponse(
-      GaiaUrls::GetInstance()->ListAccountsURLWithSource(
-          GaiaConstants::kChromeSource),
-      base::StringPrintf(
-          "[\"f\", [[\"b\", 0, \"n\", \"%s\", \"p\", 0, 0, 0, 0, %d, \"%s\", "
-          "null, null, null, %d, %d]]]",
-          email, is_email_valid ? 1 : 0, gaia_id, signed_out ? 1 : 0,
-          verified ? 1 : 0),
-      net::HTTP_OK, net::URLRequestStatus::SUCCESS);
+    const CookieParams& params) {
+  SetListAccountsResponseWithParams({params});
 }
 
 void FakeGaiaCookieManagerService::SetListAccountsResponseTwoAccounts(
-    const char* email1,
-    const char* gaia_id1,
-    const char* email2,
-    const char* gaia_id2) {
-  DCHECK(url_fetcher_factory_);
-  url_fetcher_factory_->SetFakeResponse(
-      GaiaUrls::GetInstance()->ListAccountsURLWithSource(
-          GaiaConstants::kChromeSource),
-      base::StringPrintf(
-          "[\"f\", [[\"b\", 0, \"n\", \"%s\", \"p\", 0, 0, 0, 0, 1, \"%s\"], "
-          "[\"b\", 0, \"n\", \"%s\", \"p\", 0, 0, 0, 0, 1, \"%s\"]]]",
-          email1, gaia_id1, email2, gaia_id2),
-      net::HTTP_OK, net::URLRequestStatus::SUCCESS);
-}
-
-void FakeGaiaCookieManagerService::SetListAccountsResponseTwoAccountsWithExpiry(
-    const char* email1,
-    const char* gaia_id1,
-    bool account1_expired,
-    const char* email2,
-    const char* gaia_id2,
-    bool account2_expired) {
-  DCHECK(url_fetcher_factory_);
-  url_fetcher_factory_->SetFakeResponse(
-      GaiaUrls::GetInstance()->ListAccountsURLWithSource(
-          GaiaConstants::kChromeSource),
-      base::StringPrintf(
-          "[\"f\", [[\"b\", 0, \"n\", \"%s\", \"p\", 0, 0, 0, 0, %d, \"%s\"], "
-          "[\"b\", 0, \"n\", \"%s\", \"p\", 0, 0, 0, 0, %d, \"%s\"]]]",
-          email1, account1_expired ? 0 : 1, gaia_id1, email2,
-          account2_expired ? 0 : 1, gaia_id2),
-      net::HTTP_OK, net::URLRequestStatus::SUCCESS);
+    const std::string& email1,
+    const std::string& gaia_id1,
+    const std::string& email2,
+    const std::string& gaia_id2) {
+  SetListAccountsResponseWithParams(
+      {{email1, gaia_id1, true /* valid */, false /* signed_out */,
+        true /* verified */},
+       {email2, gaia_id2, true /* valid */, false /* signed_out */,
+        true /* verified */}});
 }
 
 std::string FakeGaiaCookieManagerService::GetSourceForRequest(

components/signin/core/browser/fake_gaia_cookie_manager_service.h

@@ -13,6 +13,15 @@
 
 class FakeGaiaCookieManagerService : public GaiaCookieManagerService {
  public:
+  // Parameters for the fake ListAccounts response.
+  struct CookieParams {
+    std::string email;
+    std::string gaia_id;
+    bool valid;
+    bool signed_out;
+    bool verified;
+  };
+
   FakeGaiaCookieManagerService(OAuth2TokenService* token_service,
                                const std::string& source,
                                SigninClient* client);
@@ -21,24 +30,18 @@ class FakeGaiaCookieManagerService : public GaiaCookieManagerService {
 
   void SetListAccountsResponseHttpNotFound();
   void SetListAccountsResponseWebLoginRequired();
+  void SetListAccountsResponseWithParams(
+      const std::vector<CookieParams>& params);
+
+  // Helper methods, equivalent to calling SetListAccountsResponseWithParams().
   void SetListAccountsResponseNoAccounts();
-  void SetListAccountsResponseOneAccount(const char* email,
-                                         const char* gaia_id);
-  void SetListAccountsResponseOneAccountWithParams(const char* account,
-                                                   const char* gaia_id,
-                                                   bool is_email_valid,
-                                                   bool is_signed_out,
-                                                   bool verified);
-  void SetListAccountsResponseTwoAccounts(const char* email1,
-                                          const char* gaia_id1,
-                                          const char* email2,
-                                          const char* gaia_id2);
-  void SetListAccountsResponseTwoAccountsWithExpiry(const char* email1,
-                                                    const char* gaia_id1,
-                                                    bool account1_expired,
-                                                    const char* email2,
-                                                    const char* gaia_id2,
-                                                    bool account2_expired);
+  void SetListAccountsResponseOneAccount(const std::string& email,
+                                         const std::string& gaia_id);
+  void SetListAccountsResponseOneAccountWithParams(const CookieParams& params);
+  void SetListAccountsResponseTwoAccounts(const std::string& email1,
+                                          const std::string& gaia_id1,
+                                          const std::string& email2,
+                                          const std::string& gaia_id2);
 
  private:
   std::string GetSourceForRequest(

components/signin/core/browser/mirror_account_reconcilor_delegate.cc

@@ -41,7 +41,8 @@ std::string MirrorAccountReconcilorDelegate::GetFirstGaiaAccountForReconcile(
     const std::vector<std::string>& chrome_accounts,
     const std::vector<gaia::ListedAccount>& gaia_accounts,
     const std::string& primary_account,
-    bool first_execution) const {
+    bool first_execution,
+    bool will_logout) const {
   // Mirror only uses the primary account, and it is never empty.
   DCHECK(!primary_account.empty());
   DCHECK(base::ContainsValue(chrome_accounts, primary_account));

components/signin/core/browser/mirror_account_reconcilor_delegate.h

@@ -28,7 +28,8 @@ class MirrorAccountReconcilorDelegate : public AccountReconcilorDelegate,
       const std::vector<std::string>& chrome_accounts,
       const std::vector<gaia::ListedAccount>& gaia_accounts,
       const std::string& primary_account,
-      bool first_execution) const override;
+      bool first_execution,
+      bool will_logout) const override;
 
   // SigninManagerBase::Observer:
   void GoogleSigninSucceeded(const std::string& account_id,