Remove DICE-specific logic from the PrimaryAccountMutator.

Move logic to clear an unconsented primary account from the identity manager to the signin manager. This ensures that this state is handled uniformly and that an account in a permanent auth error is not kept as the unconsented primary account. Bug: None Change-Id: I3f11aef7ffd526919cfeb5870739dc51ba7d3209 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3581995 Reviewed-by: Monica Basta <[email protected]> Commit-Queue: Mihai Sardarescu <[email protected]> Cr-Commit-Position: refs/heads/main@{#996128}
hadissulejmani · Apr 26, 2022 · 3f5c943 · 3f5c943
1 parent 0806a35
commit 3f5c943
Show file tree

Hide file tree

Showing 7 changed files with 80 additions and 50 deletions.
diff --git a/chrome/browser/signin/dice_browsertest.cc b/chrome/browser/signin/dice_browsertest.cc
@@ -851,6 +851,34 @@ IN_PROC_BROWSER_TEST_F(DiceBrowserTest, SignoutAllAccounts) {
   WaitForReconcilorUnblockedCount(1);
 }
 
+// Checks that the Dice signout flow works and deletes all tokens.
+IN_PROC_BROWSER_TEST_F(DiceBrowserTest, RevokeSyncAccountInAuthErrorState) {
+  // Start from a signed-in state.
+  SetupSignedInAccounts(signin::ConsentLevel::kSync);
+
+  // Signout from main account.
+  SignOutWithDice(kMainAccount);
+
+  // Check that the user is in error state.
+  ASSERT_TRUE(
+      GetIdentityManager()->HasPrimaryAccount(signin::ConsentLevel::kSync));
+  ASSERT_TRUE(
+      GetIdentityManager()->HasAccountWithRefreshToken(GetMainAccountID()));
+  ASSERT_TRUE(
+      GetIdentityManager()->HasAccountWithRefreshTokenInPersistentErrorState(
+          GetMainAccountID()));
+
+  // Revoking the sync consent should clear the primary account as it is in
+  // an permanent auth error state.
+  RevokeSyncConsent(GetIdentityManager());
+
+  // Updating the primary is done asynchronously. Wait for the update to happen.
+  WaitForPrimaryAccount(GetIdentityManager(), signin::ConsentLevel::kSignin,
+                        CoreAccountId());
+  EXPECT_FALSE(
+      GetIdentityManager()->HasPrimaryAccount(signin::ConsentLevel::kSignin));
+}
+
 // Checks that Dice request header is not set from request from WebUI.
 // See https://crbug.com/428396
 #if BUILDFLAG(IS_WIN)

diff --git a/chrome/browser/signin/signin_manager.cc b/chrome/browser/signin/signin_manager.cc
@@ -157,14 +157,8 @@ CoreAccountInfo SigninManager::ComputeUnconsentedPrimaryAccountInfo() const {
     AccountInfo account_info =
         identity_manager_->FindExtendedAccountInfoByAccountId(
             cookie_accounts[0].id);
-
-    // Verify the first account in cookies has a refresh token that is valid.
-    bool error_state =
-        account_info.IsEmpty() ||
-        identity_manager_->HasAccountWithRefreshTokenInPersistentErrorState(
-            account_info.account_id);
-
-    return error_state ? CoreAccountInfo() : account_info;
+    return IsValidUnconsentedPrimaryAccount(account_info) ? account_info
+                                                          : CoreAccountInfo();
   }
 
   if (!identity_manager_->HasPrimaryAccount(signin::ConsentLevel::kSignin))
@@ -173,27 +167,24 @@ CoreAccountInfo SigninManager::ComputeUnconsentedPrimaryAccountInfo() const {
   // If cookies or tokens are not loaded, it is not possible to fully compute
   // the unconsented primary account. However, if the current unconsented
   // primary account is no longer valid, it has to be removed.
-  CoreAccountId current_account =
-      identity_manager_->GetPrimaryAccountId(signin::ConsentLevel::kSignin);
-
-  if (!identity_manager_->HasAccountWithRefreshToken(current_account)) {
-    // Tokens are loaded, but the current UPA doesn't have a refresh token.
-    // Clear the current UPA.
-    return CoreAccountInfo();
-  }
+  CoreAccountInfo current_primary_account =
+      identity_manager_->GetPrimaryAccountInfo(signin::ConsentLevel::kSignin);
+  return IsValidUnconsentedPrimaryAccount(current_primary_account)
+             ? current_primary_account
+             : CoreAccountInfo();
+#endif  // BUILDFLAG(IS_CHROMEOS_LACROS)
+}
 
-  if (cookie_info.accounts_are_fresh) {
-    if (cookie_accounts.empty() || cookie_accounts[0].id != current_account) {
-      // The current UPA is not the first in fresh cookies. It needs to be
-      // cleared.
-      return CoreAccountInfo();
-    }
-  }
+bool SigninManager::IsValidUnconsentedPrimaryAccount(
+    const CoreAccountInfo& account) const {
+  DCHECK(identity_manager_->AreRefreshTokensLoaded());
+  if (account.IsEmpty())
+    return false;
 
-  // No indication that the current UPA is invalid, return current UPA.
-  return identity_manager_->GetPrimaryAccountInfo(
-      signin::ConsentLevel::kSignin);
-#endif  // BUILDFLAG(IS_CHROMEOS_LACROS)
+  const CoreAccountId& account_id = account.account_id;
+  return identity_manager_->HasAccountWithRefreshToken(account_id) &&
+         !identity_manager_->HasAccountWithRefreshTokenInPersistentErrorState(
+             account_id);
 }
 
 void SigninManager::Shutdown() {

diff --git a/chrome/browser/signin/signin_manager.h b/chrome/browser/signin/signin_manager.h
@@ -78,6 +78,10 @@ class SigninManager : public KeyedService,
   // valid UPA.
   CoreAccountInfo ComputeUnconsentedPrimaryAccountInfo() const;
 
+  // Checks wheter |account| is a valid account that can be used as an
+  // unconsented primary account.
+  bool IsValidUnconsentedPrimaryAccount(const CoreAccountInfo& account) const;
+
   // KeyedService implementation.
   void Shutdown() override;
 

diff --git a/components/signin/internal/identity_manager/primary_account_mutator_impl.cc b/components/signin/internal/identity_manager/primary_account_mutator_impl.cc
@@ -96,21 +96,7 @@ bool PrimaryAccountMutatorImpl::CanTransitionFromSyncToSigninConsentLevel()
     const {
   switch (account_consistency_) {
     case AccountConsistencyMethod::kDice:
-      // If DICE is enabled, then adding and removing accounts is handled from
-      // the Google web services. This means that the user needs to be signed
-      // in to the their Google account on the web in order to be able to sign
-      // out of that accounts. As in most cases, the Google auth cookies are
-      // are derived from the refresh token, which means that the user is signed
-      // out of their Google account on the web when the primary account is in
-      // an auth error. It is therefore important to clear all accounts when
-      // the user revokes their sync consent for a primary account that is in
-      // an auth error as otherwise the user will not be able to remove it from
-      // Chrome.
-      //
-      // TODO(msarda): The logic in this function is platform specific and we
-      // should consider moving it to |SigninManager|.
-      return !token_service_->RefreshTokenHasError(
-          primary_account_manager_->GetPrimaryAccountId(ConsentLevel::kSync));
+      return true;
     case AccountConsistencyMethod::kMirror:
 #if BUILDFLAG(IS_CHROMEOS_LACROS)
       return true;

diff --git a/components/signin/public/identity_manager/identity_test_utils.cc b/components/signin/public/identity_manager/identity_test_utils.cc
@@ -273,6 +273,25 @@ void ClearPrimaryAccount(IdentityManager* identity_manager) {
 #endif
 }
 
+void WaitForPrimaryAccount(IdentityManager* identity_manager,
+                           ConsentLevel consent_level,
+                           const CoreAccountId& account_id) {
+  if (identity_manager->GetPrimaryAccountId(consent_level) == account_id)
+    return;
+
+  base::RunLoop run_loop;
+  TestIdentityManagerObserver primary_account_observer(identity_manager);
+  primary_account_observer.SetOnPrimaryAccountChangedCallback(base::BindOnce(
+      [](IdentityManager* identity_manager, ConsentLevel consent_level,
+         const CoreAccountId& account_id, base::RunLoop* run_loop,
+         PrimaryAccountChangeEvent event) {
+        if (identity_manager->GetPrimaryAccountId(consent_level) == account_id)
+          run_loop->Quit();
+      },
+      identity_manager, consent_level, account_id, &run_loop));
+  run_loop.Run();
+}
+
 AccountInfo MakeAccountAvailable(IdentityManager* identity_manager,
                                  const std::string& email) {
   AccountTrackerService* account_tracker_service =

diff --git a/components/signin/public/identity_manager/identity_test_utils.h b/components/signin/public/identity_manager/identity_test_utils.h
@@ -96,6 +96,16 @@ void RevokeSyncConsent(IdentityManager* identity_manager);
 // NOTE: See disclaimer at top of file re: direct usage.
 void ClearPrimaryAccount(IdentityManager* identity_manager);
 
+// Waits until the primary account id at consent_level to be equal to
+// |account_id|.
+//
+// Note: Passing an empty |account_id| will make this function wait until
+// the primary account id is cleared at the |consent_level| (calling
+// identity_manager->HasPrimaryAccount(consent_level) will return false)
+void WaitForPrimaryAccount(IdentityManager* identity_manager,
+                           ConsentLevel consent_level,
+                           const CoreAccountId& account_id);
+
 // Makes an account available for the given email address, generating a GAIA ID
 // and refresh token that correspond uniquely to that email address. Blocks
 // until the account is available. Returns the AccountInfo of the

diff --git a/components/signin/public/identity_manager/primary_account_mutator_unittest.cc b/components/signin/public/identity_manager/primary_account_mutator_unittest.cc
@@ -518,14 +518,6 @@ TEST_F(PrimaryAccountMutatorTest, RevokeSyncConsent_DiceConsistency) {
                            RemoveAccountExpectation::kKeepAll);
 }
 
-// Test that revoking the sync consent when DICE account consistency is
-// enabled clears the primary account if it uin auth error state.
-TEST_F(PrimaryAccountMutatorTest, RevokeSyncConsent_DiceConsistency_AuthError) {
-  RunRevokeSyncConsentTest(signin::AccountConsistencyMethod::kDice,
-                           RemoveAccountExpectation::kRemoveAll,
-                           AuthExpectation::kAuthError);
-}
-
 #else  //! BUILDFLAG(IS_CHROMEOS_ASH)
 
 TEST_F(PrimaryAccountMutatorTest, CROS_ASH_RevokeSyncConsent) {