From 3f5c9436da2666300ee2029e214544d18191a4f2 Mon Sep 17 00:00:00 2001 From: Mihai Sardarescu Date: Tue, 26 Apr 2022 13:21:05 +0000 Subject: [PATCH] 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 Commit-Queue: Mihai Sardarescu Cr-Commit-Position: refs/heads/main@{#996128} --- chrome/browser/signin/dice_browsertest.cc | 28 ++++++++++++ chrome/browser/signin/signin_manager.cc | 45 ++++++++----------- chrome/browser/signin/signin_manager.h | 4 ++ .../primary_account_mutator_impl.cc | 16 +------ .../identity_manager/identity_test_utils.cc | 19 ++++++++ .../identity_manager/identity_test_utils.h | 10 +++++ .../primary_account_mutator_unittest.cc | 8 ---- 7 files changed, 80 insertions(+), 50 deletions(-) diff --git a/chrome/browser/signin/dice_browsertest.cc b/chrome/browser/signin/dice_browsertest.cc index 1cbf136b98c63a..3b9820dc3acfc8 100644 --- 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 index 9117547d0b5885..4c9bf9b486fdf7 100644 --- 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 index fc24ce1cac7d2e..b4aed5329e326c 100644 --- 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 index ef7e043fa6aaba..40b3b6d9648430 100644 --- 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 index a583e60bbe1601..509a0376d67b0e 100644 --- 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 index 499bb6ed89037b..a2ab617f8e263f 100644 --- 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 index aa0962935e2ef0..25c7303fec4ed2 100644 --- 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) {