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) {