[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}

components/signin/core/browser/account_reconcilor.cc

@@ -305,14 +305,11 @@ void AccountReconcilor::StartReconcile() {
 
   // Reset state for validating oauth2 tokens.
   add_to_cookie_.clear();
-  bool is_primary_account_valid = false;
-  chrome_accounts_ = LoadValidAccountsFromTokenService(
-      &primary_account_, &is_primary_account_valid);
-  if (!is_primary_account_valid &&
+  primary_account_ = signin_manager_->GetAuthenticatedAccountId();
+  if (token_service_->GetDelegate()->RefreshTokenHasError(primary_account_) &&
       delegate_->ShouldAbortReconcileIfPrimaryHasError()) {
     VLOG(1) << "AccountReconcilor::StartReconcile: primary has error, abort.";
     primary_account_.clear();
-    chrome_accounts_.clear();
     return;
   }
 
@@ -351,7 +348,16 @@ void AccountReconcilor::OnGaiaAccountsInCookieUpdated(
         << "Ignoring " << accounts.size() - verified_gaia_accounts.size()
         << " unverified account(s).";
 
-    FinishReconcile(std::move(verified_gaia_accounts));
+    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));
   } else {
     if (is_reconcile_started_)
       error_during_last_reconcile_ = true;
@@ -359,23 +365,15 @@ void AccountReconcilor::OnGaiaAccountsInCookieUpdated(
   }
 }
 
-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> AccountReconcilor::LoadValidAccountsFromTokenService()
+    const {
   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, or if the primary account is in error, then don't reconcile any
-  // accounts.
+  // empty 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();
@@ -387,8 +385,7 @@ std::vector<std::string> AccountReconcilor::LoadValidAccountsFromTokenService(
                         chrome_accounts.end());
 
   VLOG(1) << "AccountReconcilor::ValidateAccountsFromTokenService: "
-          << "Chrome " << chrome_accounts.size() << " accounts, "
-          << "Primary is '" << *out_primary_account << "'";
+          << "Chrome " << chrome_accounts.size() << " accounts";
 
   return chrome_accounts;
 }
@@ -401,15 +398,17 @@ 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);
@@ -420,9 +419,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;
     }
   }
@@ -443,13 +442,13 @@ void AccountReconcilor::FinishReconcile(
     // Gaia cookie has been cleared or was already empty.
     DCHECK((first_account_mismatch && rebuild_cookie) ||
            (number_gaia_accounts == 0));
-    RevokeAllSecondaryTokens();
+    RevokeAllSecondaryTokens(chrome_accounts);
   } 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]);
     }
   }
 
@@ -478,7 +477,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();
@@ -520,8 +519,9 @@ void AccountReconcilor::ScheduleStartReconcileIfChromeAccountsChanged() {
   }
 }
 
-void AccountReconcilor::RevokeAllSecondaryTokens() {
-  for (const std::string& account : chrome_accounts_) {
+void AccountReconcilor::RevokeAllSecondaryTokens(
+    const std::vector<std::string>& chrome_accounts) {
+  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,6 +125,9 @@ 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);
@@ -180,18 +183,17 @@ class AccountReconcilor : public KeyedService,
   // Used during periodic reconciliation.
   void StartReconcile();
   // |gaia_accounts| are the accounts in the Gaia cookie.
-  void FinishReconcile(std::vector<gaia::ListedAccount>&& gaia_accounts);
+  void FinishReconcile(const std::vector<std::string>& chrome_accounts,
+                       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();
+  // Revokes tokens for all accounts in chrome_accounts but primary_account_.
+  void RevokeAllSecondaryTokens(
+      const std::vector<std::string>& chrome_accounts);
 
   // Returns the list of valid accounts from the TokenService.
-  // 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;
+  std::vector<std::string> LoadValidAccountsFromTokenService() const;
 
   // Note internally that this |account_id| is added to the cookie jar.
   bool MarkAccountAsAddedToCookie(const std::string& account_id);
@@ -262,7 +264,6 @@ 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,4 +26,9 @@ 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,6 +41,11 @@ 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,10 +956,9 @@ TEST_F(AccountReconcilorTest, DiceNoMigrationAfterReconcile) {
   // Enable Dice migration.
   SetAccountConsistency(signin::AccountConsistencyMethod::kDiceMigration);
 
-  // Add a token in Chrome but do not sign in.
+  // Add a token in Chrome.
   const std::string account_id =
-      PickAccountIdForAccount("12345", "user@gmail.com");
-  token_service()->UpdateCredentials(account_id, "refresh_token");
+      ConnectProfileToAccount("12345", "user@gmail.com");
   cookie_manager_service()->SetListAccountsResponseNoAccounts();
   AccountReconcilor* reconcilor = GetMockReconcilor();
   signin::DiceAccountReconcilorDelegate* dice_delegate =
@@ -987,6 +986,81 @@ 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,6 +155,15 @@ 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,6 +40,8 @@ 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,3 +58,28 @@ 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>()));
+  }
+}