Revert "[signin] Dice migration: Clear LST tokens when gaia cookies are cleared"

This reverts commit cc9fb229.

Reason for revert: This seems to have broken ios_chrome_settings_egtests (iPad Air 2 iOS 10.0) failing on chromium.mac/ios-simulator-full-configs

The AccountCollectionsTestCase/testMDMError test is failing.

See https://uberchromegw.corp.google.com/i/chromium.mac/builders/ios-simulator-full-configs.

Original change's description:
> [signin] Dice migration: Clear LST tokens when gaia cookies are cleared
> 
> This CL adds ShouldReconcileAccount() function on the
> DiceAccountReconcilor delegate, that is used to revoke the tokens when
> cookies are empty.
> 
> As a cleanup, the chrome_account_ member is changed to a local variable,
> to reduce the amount of state in the AccountReconcilor, making it
> hopefully more readable.
> 
> Bug: 788315
> Change-Id: Id4348a6ae2e524de721d39c84cb4e150adb892b2
> Reviewed-on: https://chromium-review.googlesource.com/789032
> Commit-Queue: David Roger <droger@chromium.org>
> Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#519697}

TBR=droger@chromium.org,msarda@chromium.org

Change-Id: If4f5e30f574365d1cf7d58e4ca1c41889fa1b0bd
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 788315
Reviewed-on: https://chromium-review.googlesource.com/794250Reviewed-by: Scott Little <sclittle@chromium.org>
Commit-Queue: Scott Little <sclittle@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519730}

components/signin/core/browser/account_reconcilor.cc

@@ -305,11 +305,14 @@ void AccountReconcilor::StartReconcile() {
 
   // Reset state for validating oauth2 tokens.
   add_to_cookie_.clear();
-  primary_account_ = signin_manager_->GetAuthenticatedAccountId();
-  if (token_service_->GetDelegate()->RefreshTokenHasError(primary_account_) &&
+  bool is_primary_account_valid = false;
+  chrome_accounts_ = LoadValidAccountsFromTokenService(
+      &primary_account_, &is_primary_account_valid);
+  if (!is_primary_account_valid &&
       delegate_->ShouldAbortReconcileIfPrimaryHasError()) {
     VLOG(1) << "AccountReconcilor::StartReconcile: primary has error, abort.";
     primary_account_.clear();
+    chrome_accounts_.clear();
     return;
   }
 
@@ -348,16 +351,7 @@ void AccountReconcilor::OnGaiaAccountsInCookieUpdated(
         << "Ignoring " << accounts.size() - verified_gaia_accounts.size()
         << " unverified account(s).";
 
-    if (delegate_->ShouldRevokeAllSecondaryTokensBeforeReconcile(
-            verified_gaia_accounts)) {
-      for (const std::string& account : token_service_->GetAccounts()) {
-        if (account != primary_account_)
-          token_service_->RevokeCredentials(account);
-      }
-    }
-
-    FinishReconcile(LoadValidAccountsFromTokenService(),
-                    std::move(verified_gaia_accounts));
+    FinishReconcile(std::move(verified_gaia_accounts));
   } else {
     if (is_reconcile_started_)
       error_during_last_reconcile_ = true;
@@ -365,15 +359,23 @@ void AccountReconcilor::OnGaiaAccountsInCookieUpdated(
   }
 }
 
-std::vector<std::string> AccountReconcilor::LoadValidAccountsFromTokenService()
-    const {
+std::vector<std::string> AccountReconcilor::LoadValidAccountsFromTokenService(
+    std::string* out_primary_account,
+    bool* out_is_primary_account_valid) const {
+  DCHECK(out_primary_account);
+  DCHECK(out_is_primary_account_valid);
+  *out_primary_account = signin_manager_->GetAuthenticatedAccountId();
   std::vector<std::string> chrome_accounts = token_service_->GetAccounts();
+  *out_is_primary_account_valid = true;
 
   // Remove any accounts that have an error.  There is no point in trying to
   // reconcile them, since it won't work anyway.  If the list ends up being
-  // empty then don't reconcile any accounts.
+  // empty, or if the primary account is in error, then don't reconcile any
+  // accounts.
   for (auto i = chrome_accounts.begin(); i != chrome_accounts.end(); ++i) {
     if (token_service_->GetDelegate()->RefreshTokenHasError(*i)) {
+      if (*out_primary_account == *i)
+        *out_is_primary_account_valid = false;
       VLOG(1) << "AccountReconcilor::ValidateAccountsFromTokenService: " << *i
               << " has error, won't reconcile";
       i->clear();
@@ -385,7 +387,8 @@ std::vector<std::string> AccountReconcilor::LoadValidAccountsFromTokenService()
                         chrome_accounts.end());
 
   VLOG(1) << "AccountReconcilor::ValidateAccountsFromTokenService: "
-          << "Chrome " << chrome_accounts.size() << " accounts";
+          << "Chrome " << chrome_accounts.size() << " accounts, "
+          << "Primary is '" << *out_primary_account << "'";
 
   return chrome_accounts;
 }
@@ -398,17 +401,15 @@ void AccountReconcilor::OnReceivedManageAccountsResponse(
 }
 
 void AccountReconcilor::FinishReconcile(
-    const std::vector<std::string>& chrome_accounts,
     std::vector<gaia::ListedAccount>&& gaia_accounts) {
   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|.
+      chrome_accounts_, gaia_accounts, primary_account_, first_execution_);
+  // |first_account| must be in |chrome_accounts_|.
   DCHECK(first_account.empty() ||
-         (std::find(chrome_accounts.begin(), chrome_accounts.end(),
-                    first_account) != chrome_accounts.end()));
+         (std::find(chrome_accounts_.begin(), chrome_accounts_.end(),
+                    first_account) != chrome_accounts_.end()));
   size_t number_gaia_accounts = gaia_accounts.size();
   bool first_account_mismatch =
       (number_gaia_accounts > 0) && (first_account != gaia_accounts[0].id);
@@ -419,9 +420,9 @@ void AccountReconcilor::FinishReconcile(
   int removed_from_cookie = 0;
   for (size_t i = 0; i < number_gaia_accounts; ++i) {
     if (gaia_accounts[i].valid &&
-        chrome_accounts.end() == std::find(chrome_accounts.begin(),
-                                           chrome_accounts.end(),
-                                           gaia_accounts[i].id)) {
+        chrome_accounts_.end() == std::find(chrome_accounts_.begin(),
+                                            chrome_accounts_.end(),
+                                            gaia_accounts[i].id)) {
       ++removed_from_cookie;
     }
   }
@@ -442,13 +443,13 @@ void AccountReconcilor::FinishReconcile(
     // Gaia cookie has been cleared or was already empty.
     DCHECK((first_account_mismatch && rebuild_cookie) ||
            (number_gaia_accounts == 0));
-    RevokeAllSecondaryTokens(chrome_accounts);
+    RevokeAllSecondaryTokens();
   } else {
     // Create a list of accounts that need to be added to the Gaia cookie.
     add_to_cookie_.push_back(first_account);
-    for (size_t i = 0; i < chrome_accounts.size(); ++i) {
-      if (chrome_accounts[i] != first_account)
-        add_to_cookie_.push_back(chrome_accounts[i]);
+    for (size_t i = 0; i < chrome_accounts_.size(); ++i) {
+      if (chrome_accounts_[i] != first_account)
+        add_to_cookie_.push_back(chrome_accounts_[i]);
     }
   }
 
@@ -477,7 +478,7 @@ void AccountReconcilor::FinishReconcile(
   }
 
   signin_metrics::LogSigninAccountReconciliation(
-      chrome_accounts.size(), added_to_cookie, removed_from_cookie,
+      chrome_accounts_.size(), added_to_cookie, removed_from_cookie,
       !first_account_mismatch, first_execution_, number_gaia_accounts);
   first_execution_ = false;
   CalculateIfReconcileIsDone();
@@ -519,9 +520,8 @@ void AccountReconcilor::ScheduleStartReconcileIfChromeAccountsChanged() {
   }
 }
 
-void AccountReconcilor::RevokeAllSecondaryTokens(
-    const std::vector<std::string>& chrome_accounts) {
-  for (const std::string& account : chrome_accounts) {
+void AccountReconcilor::RevokeAllSecondaryTokens() {
+  for (const std::string& account : chrome_accounts_) {
     if (account != primary_account_) {
       reconcile_is_noop_ = false;
       if (delegate_->IsAccountConsistencyEnforced()) {

components/signin/core/browser/account_reconcilor.h

@@ -125,9 +125,6 @@ class AccountReconcilor : public KeyedService,
                            DiceNoMigrationAfterReconcile);
   FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest,
                            DiceReconcileReuseGaiaFirstAccount);
-  FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest,
-                           MigrationClearSecondaryTokens);
-  FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest, MigrationClearAllTokens);
   FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest, TokensNotLoaded);
   FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest,
                            StartReconcileCookiesDisabled);
@@ -183,17 +180,18 @@ class AccountReconcilor : public KeyedService,
   // Used during periodic reconciliation.
   void StartReconcile();
   // |gaia_accounts| are the accounts in the Gaia cookie.
-  void FinishReconcile(const std::vector<std::string>& chrome_accounts,
-                       std::vector<gaia::ListedAccount>&& gaia_accounts);
+  void FinishReconcile(std::vector<gaia::ListedAccount>&& gaia_accounts);
   void AbortReconcile();
   void CalculateIfReconcileIsDone();
   void ScheduleStartReconcileIfChromeAccountsChanged();
-  // Revokes tokens for all accounts in chrome_accounts but primary_account_.
-  void RevokeAllSecondaryTokens(
-      const std::vector<std::string>& chrome_accounts);
+  // Revokes tokens for all accounts in chrome_accounts_ but primary_account_.
+  void RevokeAllSecondaryTokens();
 
   // Returns the list of valid accounts from the TokenService.
-  std::vector<std::string> LoadValidAccountsFromTokenService() const;
+  // The primary account is returned in |out_primary_account| if any.
+  std::vector<std::string> LoadValidAccountsFromTokenService(
+      std::string* out_primary_account,
+      bool* out_is_primary_account_valid) const;
 
   // Note internally that this |account_id| is added to the cookie jar.
   bool MarkAccountAsAddedToCookie(const std::string& account_id);
@@ -264,6 +262,7 @@ class AccountReconcilor : public KeyedService,
   // Used during reconcile action.
   // These members are used to validate the tokens in OAuth2TokenService.
   std::string primary_account_;
+  std::vector<std::string> chrome_accounts_;
   std::vector<std::string> add_to_cookie_;
   bool chrome_accounts_changed_;
 

components/signin/core/browser/account_reconcilor_delegate.cc

@@ -26,9 +26,4 @@ std::string AccountReconcilorDelegate::GetFirstGaiaAccountForReconcile(
   return std::string();
 }
 
-bool AccountReconcilorDelegate::ShouldRevokeAllSecondaryTokensBeforeReconcile(
-    const std::vector<gaia::ListedAccount>& gaia_accounts) {
-  return false;
-}
-
 }  // namespace signin

components/signin/core/browser/account_reconcilor_delegate.h

@@ -41,11 +41,6 @@ class AccountReconcilorDelegate {
       const std::string& primary_account,
       bool first_execution) const;
 
-  // Returns true if all secondary accounts should be cleared at the beginning
-  // of the reconcile.
-  virtual bool ShouldRevokeAllSecondaryTokensBeforeReconcile(
-      const std::vector<gaia::ListedAccount>& gaia_accounts);
-
   // Called when reconcile is finished.
   virtual void OnReconcileFinished(const std::string& first_account,
                                    bool reconcile_is_noop) {}

components/signin/core/browser/account_reconcilor_unittest.cc

@@ -956,9 +956,10 @@ TEST_F(AccountReconcilorTest, DiceNoMigrationAfterReconcile) {
   // Enable Dice migration.
   SetAccountConsistency(signin::AccountConsistencyMethod::kDiceMigration);
 
-  // Add a token in Chrome.
+  // Add a token in Chrome but do not sign in.
   const std::string account_id =
-      ConnectProfileToAccount("12345", "user@gmail.com");
+      PickAccountIdForAccount("12345", "user@gmail.com");
+  token_service()->UpdateCredentials(account_id, "refresh_token");
   cookie_manager_service()->SetListAccountsResponseNoAccounts();
   AccountReconcilor* reconcilor = GetMockReconcilor();
   signin::DiceAccountReconcilorDelegate* dice_delegate =
@@ -986,81 +987,6 @@ TEST_F(AccountReconcilorTest, DiceNoMigrationAfterReconcile) {
   EXPECT_FALSE(dice_delegate->IsAccountConsistencyEnforced());
 }
 
-// Tests that secondary refresh tokens are cleared when cookie is empty during
-// Dice migration.
-TEST_F(AccountReconcilorTest, MigrationClearSecondaryTokens) {
-  // Enable Dice migration.
-  SetAccountConsistency(signin::AccountConsistencyMethod::kDiceMigration);
-
-  // Add a tokens in Chrome, signin to Sync, but no Gaia cookies.
-  const std::string account_id_1 =
-      ConnectProfileToAccount("12345", "user@gmail.com");
-  const std::string account_id_2 =
-      PickAccountIdForAccount("67890", "other@gmail.com");
-  token_service()->UpdateCredentials(account_id_2, "refresh_token");
-  cookie_manager_service()->SetListAccountsResponseNoAccounts();
-  ASSERT_TRUE(token_service()->RefreshTokenIsAvailable(account_id_1));
-  ASSERT_TRUE(token_service()->RefreshTokenIsAvailable(account_id_2));
-
-  // Reconcile should revoke the secondary account.
-  AccountReconcilor* reconcilor = GetMockReconcilor();
-  EXPECT_CALL(*GetMockReconcilor(), PerformMergeAction(account_id_1));
-  signin::DiceAccountReconcilorDelegate* dice_delegate =
-      static_cast<signin::DiceAccountReconcilorDelegate*>(
-          reconcilor->delegate_.get());
-  reconcilor->StartReconcile();
-  ASSERT_TRUE(reconcilor->is_reconcile_started_);
-  base::RunLoop().RunUntilIdle();
-  SimulateAddAccountToCookieCompleted(reconcilor, account_id_1,
-                                      GoogleServiceAuthError::AuthErrorNone());
-  ASSERT_FALSE(reconcilor->is_reconcile_started_);
-  ASSERT_EQ(signin_metrics::ACCOUNT_RECONCILOR_OK, reconcilor->GetState());
-
-  EXPECT_TRUE(token_service()->RefreshTokenIsAvailable(account_id_1));
-  EXPECT_FALSE(token_service()->RefreshTokenIsAvailable(account_id_2));
-
-  // Profile was not migrated.
-  EXPECT_FALSE(
-      dice_delegate->IsReadyForDiceMigration(false /* is_new_profile */));
-}
-
-// Tests that all refresh tokens are cleared when cookie is empty during
-// Dice migration, if Sync is not enabled.
-TEST_F(AccountReconcilorTest, MigrationClearAllTokens) {
-  // Enable Dice migration.
-  SetAccountConsistency(signin::AccountConsistencyMethod::kDiceMigration);
-
-  // Add a tokens in Chrome but no Gaia cookies.
-  const std::string account_id_1 =
-      PickAccountIdForAccount("12345", "user@gmail.com");
-  const std::string account_id_2 =
-      PickAccountIdForAccount("67890", "other@gmail.com");
-  token_service()->UpdateCredentials(account_id_1, "refresh_token");
-  token_service()->UpdateCredentials(account_id_2, "refresh_token");
-  cookie_manager_service()->SetListAccountsResponseNoAccounts();
-  ASSERT_TRUE(token_service()->RefreshTokenIsAvailable(account_id_1));
-  ASSERT_TRUE(token_service()->RefreshTokenIsAvailable(account_id_2));
-
-  // Reconcile should revoke the secondary account.
-  AccountReconcilor* reconcilor = GetMockReconcilor();
-  signin::DiceAccountReconcilorDelegate* dice_delegate =
-      static_cast<signin::DiceAccountReconcilorDelegate*>(
-          reconcilor->delegate_.get());
-  reconcilor->StartReconcile();
-  ASSERT_TRUE(reconcilor->is_reconcile_started_);
-  base::RunLoop().RunUntilIdle();
-  ASSERT_FALSE(reconcilor->is_reconcile_started_);
-  ASSERT_EQ(signin_metrics::ACCOUNT_RECONCILOR_OK, reconcilor->GetState());
-
-  EXPECT_FALSE(token_service()->RefreshTokenIsAvailable(account_id_1));
-  EXPECT_FALSE(token_service()->RefreshTokenIsAvailable(account_id_2));
-
-  // Profile is now ready for migration on next startup.
-  base::RunLoop().RunUntilIdle();
-  EXPECT_TRUE(
-      dice_delegate->IsReadyForDiceMigration(false /* is_new_profile */));
-}
-
 #endif  // BUILDFLAG(ENABLE_DICE_SUPPORT)
 
 // Tests that reconcile cannot start before the tokens are loaded, and is

components/signin/core/browser/dice_account_reconcilor_delegate.cc

@@ -155,15 +155,6 @@ std::string DiceAccountReconcilorDelegate::GetFirstGaiaAccountForReconcile(
   return std::string();
 }
 
-bool DiceAccountReconcilorDelegate::
-    ShouldRevokeAllSecondaryTokensBeforeReconcile(
-        const std::vector<gaia::ListedAccount>& gaia_accounts) {
-  // During the Dice migration step, before Dice is actually enabled, chrome
-  // tokens must be cleared when the cookies are cleared.
-  return signin::IsDicePrepareMigrationEnabled() &&
-         !signin::IsDiceEnabledForProfile(user_prefs_) && gaia_accounts.empty();
-}
-
 void DiceAccountReconcilorDelegate::OnReconcileFinished(
     const std::string& first_account,
     bool reconcile_is_noop) {

components/signin/core/browser/dice_account_reconcilor_delegate.h

@@ -40,8 +40,6 @@ class DiceAccountReconcilorDelegate : public AccountReconcilorDelegate {
       const std::vector<gaia::ListedAccount>& gaia_accounts,
       const std::string& primary_account,
       bool first_execution) const override;
-  bool ShouldRevokeAllSecondaryTokensBeforeReconcile(
-      const std::vector<gaia::ListedAccount>& gaia_accounts) override;
   void OnReconcileFinished(const std::string& first_account,
                            bool reconcile_is_noop) override;
 

components/signin/core/browser/dice_account_reconcilor_delegate_unittest.cc

@@ -58,28 +58,3 @@ TEST(DiceAccountReconcilorDelegateTest, NewProfile) {
   signin::DiceAccountReconcilorDelegate delegate(&pref_service, true);
   EXPECT_TRUE(signin::IsDiceEnabledForProfile(&pref_service));
 }
-
-TEST(DiceAccountReconcilorDelegateTest, RevokeTokens) {
-  sync_preferences::TestingPrefServiceSyncable pref_service;
-  signin::DiceAccountReconcilorDelegate::RegisterProfilePrefs(
-      pref_service.registry());
-  signin::RegisterAccountConsistencyProfilePrefs(pref_service.registry());
-  signin::DiceAccountReconcilorDelegate delegate(&pref_service, false);
-  {
-    // Dice is enabled, don't revoke.
-    signin::ScopedAccountConsistencyDice scoped_dice;
-    EXPECT_FALSE(delegate.ShouldRevokeAllSecondaryTokensBeforeReconcile(
-        std::vector<gaia::ListedAccount>()));
-  }
-  {
-    signin::ScopedAccountConsistencyDiceMigration scoped_dice_migration;
-    // Gaia accounts are not empty, don't revoke.
-    gaia::ListedAccount gaia_account;
-    gaia_account.id = "other";
-    EXPECT_FALSE(delegate.ShouldRevokeAllSecondaryTokensBeforeReconcile(
-        std::vector<gaia::ListedAccount>{gaia_account}));
-    // Revoke.
-    EXPECT_TRUE(delegate.ShouldRevokeAllSecondaryTokensBeforeReconcile(
-        std::vector<gaia::ListedAccount>()));
-  }
-}