Rename SignOutAndRemoveAllAccounts() and SignOutAndKeepAllAccounts()

This CL also renames method SignOutAndRemoveAllAccounts() and
SignOutAndKeepAllAccounts() to ClearPrimaryAccount() and
RevokeSyncConsent() to better match the PrimaryAccountMutator APIs.

Bug: 1155505

Change-Id: I395af84c98f32383f072eb3662d4ebde7e545534
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2580338
Commit-Queue: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#836110}

chrome/browser/profiles/gaia_info_update_service_unittest.cc

@@ -58,17 +58,22 @@ AccountInfo GetValidAccountInfo(std::string email,
   return account_info;
 }
 
-#if !defined(ANDROID)
+#if BUILDFLAG(ENABLE_DICE_SUPPORT)
 const char kChromiumOrgDomain[] = "chromium.org";
-#endif  // !defined(ANDROID)
+#endif  // BUILDFLAG(ENABLE_DICE_SUPPORT)
 
 #endif  // !BUILDFLAG(IS_CHROMEOS_ASH)
 
-class GAIAInfoUpdateServiceTest : public testing::Test {
+class GAIAInfoUpdateServiceTestBase : public testing::Test {
  protected:
-  GAIAInfoUpdateServiceTest()
-      : testing_profile_manager_(TestingBrowserProcess::GetGlobal()) {}
-  ~GAIAInfoUpdateServiceTest() override = default;
+  explicit GAIAInfoUpdateServiceTestBase(
+      signin::AccountConsistencyMethod account_consistency)
+      : testing_profile_manager_(TestingBrowserProcess::GetGlobal()),
+        identity_test_env_(/*test_url_loader_factory=*/nullptr,
+                           /*pref_service=*/nullptr,
+                           account_consistency,
+                           /*test_signin_client=*/nullptr) {}
+  ~GAIAInfoUpdateServiceTestBase() override = default;
 
   void SetUp() override {
     testing::Test::SetUp();
@@ -122,20 +127,29 @@ class GAIAInfoUpdateServiceTest : public testing::Test {
   signin::IdentityTestEnvironment identity_test_env_;
   std::unique_ptr<GAIAInfoUpdateService> service_;
 
+ private:
+  DISALLOW_COPY_AND_ASSIGN(GAIAInfoUpdateServiceTestBase);
+};
+
+class GAIAInfoUpdateServiceTest : public GAIAInfoUpdateServiceTestBase {
+ protected:
+  GAIAInfoUpdateServiceTest()
+      : GAIAInfoUpdateServiceTestBase(
+            signin::AccountConsistencyMethod::kDisabled) {}
+  ~GAIAInfoUpdateServiceTest() override = default;
+
  private:
   DISALLOW_COPY_AND_ASSIGN(GAIAInfoUpdateServiceTest);
 };
+
 }  // namespace
 
-TEST_F(GAIAInfoUpdateServiceTest, ShouldUseGAIAProfileInfo) {
 #if BUILDFLAG(IS_CHROMEOS_ASH)
-  // This feature should never be enabled on ChromeOS.
+// This feature should never be enabled on ChromeOS.
+TEST_F(GAIAInfoUpdateServiceTest, ShouldUseGAIAProfileInfo) {
   EXPECT_FALSE(GAIAInfoUpdateService::ShouldUseGAIAProfileInfo(profile()));
-#endif
 }
-
-#if !BUILDFLAG(IS_CHROMEOS_ASH)
-
+#else  // BUILDFLAG(IS_CHROMEOS_ASH)
 TEST_F(GAIAInfoUpdateServiceTest, SyncOnSyncOff) {
   AccountInfo info =
       identity_test_env()->MakeAccountAvailable("pat@example.com");
@@ -175,8 +189,21 @@ TEST_F(GAIAInfoUpdateServiceTest, SyncOnSyncOff) {
                   .empty());
 }
 
-#if !defined(ANDROID)
-TEST_F(GAIAInfoUpdateServiceTest, SyncOnSyncOffKeepAllAccounts) {
+#if BUILDFLAG(ENABLE_DICE_SUPPORT)
+namespace {
+class GAIAInfoUpdateServiceDiceTest : public GAIAInfoUpdateServiceTestBase {
+ protected:
+  GAIAInfoUpdateServiceDiceTest()
+      : GAIAInfoUpdateServiceTestBase(signin::AccountConsistencyMethod::kDice) {
+  }
+  ~GAIAInfoUpdateServiceDiceTest() override = default;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(GAIAInfoUpdateServiceDiceTest);
+};
+}  // namespace
+
+TEST_F(GAIAInfoUpdateServiceDiceTest, RevokeSyncConsent) {
   AccountInfo info =
       identity_test_env()->MakeAccountAvailable("pat@example.com");
   base::RunLoop().RunUntilIdle();
@@ -193,7 +220,7 @@ TEST_F(GAIAInfoUpdateServiceTest, SyncOnSyncOffKeepAllAccounts) {
   signin::SimulateAccountImageFetch(identity_test_env()->identity_manager(),
                                     info.account_id, "GAIA_IMAGE_URL_WITH_SIZE",
                                     gaia_picture);
-  // Turn off sync but stay logged in.
+  // Revoke sync consent (stay signed in with the primary account).
   identity_test_env()->RevokeSyncConsent();
   ASSERT_TRUE(identity_test_env()->identity_manager()->HasPrimaryAccount(
       signin::ConsentLevel::kNotRequired));
@@ -370,7 +397,7 @@ TEST_F(GAIAInfoUpdateServiceTest, MultiLoginAndLogOut) {
   tester.ExpectTotalCount("Profile.AllAccounts.Categories",
                           /*expected_count=*/2);
 }
-#endif  // !defined(ANDROID)
+#endif  // !BUILDFLAG(ENABLE_DICE_SUPPORT)
 
 TEST_F(GAIAInfoUpdateServiceTest, ClearGaiaInfoOnStartup) {
   // Simulate a state where the profile entry has GAIA related information
@@ -397,4 +424,4 @@ TEST_F(GAIAInfoUpdateServiceTest, ClearGaiaInfoOnStartup) {
   EXPECT_TRUE(entry->GetHostedDomain().empty());
 }
 
-#endif  // !BUILDFLAG(IS_CHROMEOS_ASH)
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH)

chrome/browser/signin/signin_manager_unittest.cc

@@ -30,7 +30,11 @@ class IdentityManagerObserver : public IdentityManager::Observer {
 
 class SigninManagerTest : public testing::Test {
  public:
-  SigninManagerTest() = default;
+  SigninManagerTest()
+      : identity_test_env_(/*test_url_loader_factory=*/nullptr,
+                           /*pref_service=*/nullptr,
+                           signin::AccountConsistencyMethod::kDice,
+                           /*test_signin_client=*/nullptr) {}
 
   void SetUp() override {
     testing::Test::SetUp();

components/signin/internal/identity_manager/primary_account_manager.cc

@@ -270,7 +270,7 @@ void PrimaryAccountManager::RemoveObserver(Observer* observer) {
 }
 
 #if !BUILDFLAG(IS_CHROMEOS_ASH)
-void PrimaryAccountManager::SignOutAndRemoveAllAccounts(
+void PrimaryAccountManager::ClearPrimaryAccount(
     signin_metrics::ProfileSignout signout_source_metric,
     signin_metrics::SignoutDelete signout_delete_metric) {
   StartSignOut(signout_source_metric, signout_delete_metric,
@@ -279,7 +279,7 @@ void PrimaryAccountManager::SignOutAndRemoveAllAccounts(
 
 #endif  // !BUILDFLAG(IS_CHROMEOS_ASH)
 
-void PrimaryAccountManager::SignOutAndKeepAllAccounts(
+void PrimaryAccountManager::RevokeSyncConsent(
     signin_metrics::ProfileSignout signout_source_metric,
     signin_metrics::SignoutDelete signout_delete_metric) {
   StartSignOut(signout_source_metric, signout_delete_metric,

components/signin/internal/identity_manager/primary_account_manager.h

@@ -118,18 +118,16 @@ class PrimaryAccountManager : public ProfileOAuth2TokenServiceObserver {
   // Signs a user out, removing the preference, erasing all keys
   // associated with the authenticated user, and canceling all auth in progress.
   // It removes all accounts from Chrome by revoking all refresh tokens.
-  void SignOutAndRemoveAllAccounts(
-      signin_metrics::ProfileSignout signout_source_metric,
-      signin_metrics::SignoutDelete signout_delete_metric);
+  void ClearPrimaryAccount(signin_metrics::ProfileSignout signout_source_metric,
+                           signin_metrics::SignoutDelete signout_delete_metric);
 
 #endif  // !BUILDFLAG(IS_CHROMEOS_ASH)
 
   // Signs a user out, removing the preference, erasing all keys
   // associated with the authenticated user, and canceling all auth in progress.
   // Does not remove the accounts from the token service.
-  void SignOutAndKeepAllAccounts(
-      signin_metrics::ProfileSignout signout_source_metric,
-      signin_metrics::SignoutDelete signout_delete_metric);
+  void RevokeSyncConsent(signin_metrics::ProfileSignout signout_source_metric,
+                         signin_metrics::SignoutDelete signout_delete_metric);
 
   // Adds and removes observers.
   void AddObserver(Observer* observer);

components/signin/internal/identity_manager/primary_account_manager_unittest.cc

@@ -152,9 +152,8 @@ TEST_F(PrimaryAccountManagerTest, SignOut) {
   CoreAccountId main_account_id =
       AddToAccountTracker("account_id", "user@gmail.com");
   manager_->SignIn("user@gmail.com");
-  manager_->SignOutAndRemoveAllAccounts(
-      signin_metrics::SIGNOUT_TEST,
-      signin_metrics::SignoutDelete::IGNORE_METRIC);
+  manager_->ClearPrimaryAccount(signin_metrics::SIGNOUT_TEST,
+                                signin_metrics::SignoutDelete::IGNORE_METRIC);
   EXPECT_FALSE(manager_->HasPrimaryAccount(ConsentLevel::kSync));
   EXPECT_TRUE(manager_->GetAuthenticatedAccountInfo().email.empty());
   EXPECT_TRUE(manager_->GetAuthenticatedAccountId().empty());
@@ -180,9 +179,8 @@ TEST_F(PrimaryAccountManagerTest, SignOutRevoke) {
   EXPECT_TRUE(manager_->HasPrimaryAccount(ConsentLevel::kSync));
   EXPECT_EQ(main_account_id, manager_->GetAuthenticatedAccountId());
 
-  manager_->SignOutAndRemoveAllAccounts(
-      signin_metrics::SIGNOUT_TEST,
-      signin_metrics::SignoutDelete::IGNORE_METRIC);
+  manager_->ClearPrimaryAccount(signin_metrics::SIGNOUT_TEST,
+                                signin_metrics::SignoutDelete::IGNORE_METRIC);
 
   // Tokens are revoked.
   EXPECT_FALSE(manager_->HasPrimaryAccount(ConsentLevel::kSync));
@@ -198,14 +196,12 @@ TEST_F(PrimaryAccountManagerTest, SignOutWhileProhibited) {
   AddToAccountTracker("gaia_id", "user@gmail.com");
   manager_->SignIn("user@gmail.com");
   signin_client()->set_is_signout_allowed(false);
-  manager_->SignOutAndRemoveAllAccounts(
-      signin_metrics::SIGNOUT_TEST,
-      signin_metrics::SignoutDelete::IGNORE_METRIC);
+  manager_->ClearPrimaryAccount(signin_metrics::SIGNOUT_TEST,
+                                signin_metrics::SignoutDelete::IGNORE_METRIC);
   EXPECT_TRUE(manager_->HasPrimaryAccount(ConsentLevel::kSync));
   signin_client()->set_is_signout_allowed(true);
-  manager_->SignOutAndRemoveAllAccounts(
-      signin_metrics::SIGNOUT_TEST,
-      signin_metrics::SignoutDelete::IGNORE_METRIC);
+  manager_->ClearPrimaryAccount(signin_metrics::SIGNOUT_TEST,
+                                signin_metrics::SignoutDelete::IGNORE_METRIC);
   EXPECT_FALSE(manager_->HasPrimaryAccount(ConsentLevel::kSync));
 }
 
@@ -221,9 +217,8 @@ TEST_F(PrimaryAccountManagerTest, UnconsentedSignOutWhileProhibited) {
   EXPECT_TRUE(manager_->HasPrimaryAccount(ConsentLevel::kNotRequired));
   EXPECT_FALSE(manager_->HasPrimaryAccount(ConsentLevel::kSync));
   signin_client()->set_is_signout_allowed(false);
-  manager_->SignOutAndRemoveAllAccounts(
-      signin_metrics::SIGNOUT_TEST,
-      signin_metrics::SignoutDelete::IGNORE_METRIC);
+  manager_->ClearPrimaryAccount(signin_metrics::SIGNOUT_TEST,
+                                signin_metrics::SignoutDelete::IGNORE_METRIC);
   EXPECT_FALSE(manager_->HasPrimaryAccount(ConsentLevel::kNotRequired));
 }
 
@@ -456,15 +451,14 @@ TEST_F(PrimaryAccountManagerTest, SetUnconsentedPrimaryAccountInfo) {
   EXPECT_EQ(CoreAccountInfo(), manager_->GetAuthenticatedAccountInfo());
 }
 
-TEST_F(PrimaryAccountManagerTest, SignOutAndKeepAllAccounts) {
+TEST_F(PrimaryAccountManagerTest, RevokeSyncConsent) {
   CreatePrimaryAccountManager();
   CoreAccountId account_id = AddToAccountTracker("gaia_id", "user@gmail.com");
   manager_->SignIn("user@gmail.com");
   EXPECT_TRUE(manager_->HasPrimaryAccount(ConsentLevel::kSync));
 
-  manager_->SignOutAndKeepAllAccounts(
-      signin_metrics::ProfileSignout::SIGNOUT_TEST,
-      signin_metrics::SignoutDelete::IGNORE_METRIC);
+  manager_->RevokeSyncConsent(signin_metrics::ProfileSignout::SIGNOUT_TEST,
+                              signin_metrics::SignoutDelete::IGNORE_METRIC);
   EXPECT_FALSE(manager_->HasPrimaryAccount(ConsentLevel::kSync));
   EXPECT_TRUE(manager_->HasPrimaryAccount(ConsentLevel::kNotRequired));
   EXPECT_EQ(account_id,
@@ -472,15 +466,14 @@ TEST_F(PrimaryAccountManagerTest, SignOutAndKeepAllAccounts) {
 }
 
 #if !BUILDFLAG(IS_CHROMEOS_ASH)
-TEST_F(PrimaryAccountManagerTest, SignOutAndRemoveAllAccounts) {
+TEST_F(PrimaryAccountManagerTest, ClearPrimaryAccount) {
   CreatePrimaryAccountManager();
   CoreAccountId account_id = AddToAccountTracker("gaia_id", "user@gmail.com");
   manager_->SignIn("user@gmail.com");
   EXPECT_TRUE(manager_->HasPrimaryAccount(ConsentLevel::kSync));
 
-  manager_->SignOutAndRemoveAllAccounts(
-      signin_metrics::ProfileSignout::SIGNOUT_TEST,
-      signin_metrics::SignoutDelete::IGNORE_METRIC);
+  manager_->ClearPrimaryAccount(signin_metrics::ProfileSignout::SIGNOUT_TEST,
+                                signin_metrics::SignoutDelete::IGNORE_METRIC);
   EXPECT_FALSE(manager_->HasPrimaryAccount(ConsentLevel::kSync));
   EXPECT_FALSE(manager_->HasPrimaryAccount(ConsentLevel::kNotRequired));
 }

components/signin/internal/identity_manager/primary_account_mutator_impl.cc

@@ -119,8 +119,7 @@ void PrimaryAccountMutatorImpl::RevokeSyncConsent(
     return;
   }
 #endif
-  primary_account_manager_->SignOutAndKeepAllAccounts(source_metric,
-                                                      delete_metric);
+  primary_account_manager_->RevokeSyncConsent(source_metric, delete_metric);
 }
 
 #if !BUILDFLAG(IS_CHROMEOS_ASH)
@@ -130,8 +129,7 @@ bool PrimaryAccountMutatorImpl::ClearPrimaryAccount(
   if (!primary_account_manager_->HasPrimaryAccount(ConsentLevel::kNotRequired))
     return false;
 
-  primary_account_manager_->SignOutAndRemoveAllAccounts(source_metric,
-                                                        delete_metric);
+  primary_account_manager_->ClearPrimaryAccount(source_metric, delete_metric);
   return true;
 }
 #endif

components/signin/internal/identity_manager/primary_account_policy_manager_impl.cc

@@ -56,12 +56,14 @@ void PrimaryAccountPolicyManagerImpl::InitializePolicy(
     //
     // On desktop, when PrimaryAccountManager is initializing, the profile was
     // not yet marked with sign out allowed. Therefore sign out is not allowed
-    // and all calls to SignOut methods are no-op.
+    // and all calls to RevokeSyncConsent() and ClearPrimaryAccount() methods
+    // are no-op.
     //
-    // TODO(msarda): SignOut methods do not guarantee that sign out can actually
-    // be done (this depends on whether sign out is allowed). Add a check here
-    // on desktop to make it clear that SignOut does not do anything.
-    primary_account_manager->SignOutAndKeepAllAccounts(
+    // TODO(msarda): RevokeSyncConsent() method do not guarantee that the sync
+    // consent can really be revoked (this depends on whether sign out is
+    // allowed). Add a check here on desktop to make it clear that
+    // RevokeSyncConsent() does not do anything.
+    primary_account_manager->RevokeSyncConsent(
         signin_metrics::SIGNIN_PREF_CHANGED_DURING_SIGNIN,
         signin_metrics::SignoutDelete::IGNORE_METRIC);
   }
@@ -74,7 +76,7 @@ void PrimaryAccountPolicyManagerImpl::OnGoogleServicesUsernamePatternChanged(
           primary_account_manager->GetAuthenticatedAccountInfo().email)) {
     // Signed in user is invalid according to the current policy so sign
     // the user out.
-    primary_account_manager->SignOutAndRemoveAllAccounts(
+    primary_account_manager->ClearPrimaryAccount(
         signin_metrics::GOOGLE_SERVICE_NAME_PATTERN_CHANGED,
         signin_metrics::SignoutDelete::IGNORE_METRIC);
   }
@@ -89,7 +91,7 @@ void PrimaryAccountPolicyManagerImpl::OnSigninAllowedPrefChanged(
   if (!IsSigninAllowed() &&
       primary_account_manager->HasPrimaryAccount(signin::ConsentLevel::kSync)) {
     VLOG(0) << "IsSigninAllowed() set to false, signing out the user";
-    primary_account_manager->SignOutAndRemoveAllAccounts(
+    primary_account_manager->ClearPrimaryAccount(
         signin_metrics::SIGNOUT_PREF_CHANGED,
         signin_metrics::SignoutDelete::IGNORE_METRIC);
   }

components/signin/public/identity_manager/identity_test_utils.cc

@@ -18,6 +18,7 @@
 #include "components/signin/public/base/list_accounts_test_utils.h"
 #include "components/signin/public/identity_manager/consent_level.h"
 #include "components/signin/public/identity_manager/identity_manager.h"
+#include "components/signin/public/identity_manager/primary_account_mutator.h"
 #include "components/signin/public/identity_manager/test_identity_manager_observer.h"
 #include "google_apis/gaia/gaia_auth_util.h"
 #include "google_apis/gaia/gaia_constants.h"
@@ -194,10 +195,11 @@ void RevokeSyncConsent(IdentityManager* identity_manager) {
   if (!identity_manager->HasPrimaryAccount(ConsentLevel::kSync))
     return;
 
+  DCHECK(identity_manager->GetPrimaryAccountMutator());
   base::RunLoop run_loop;
   TestIdentityManagerObserver signout_observer(identity_manager);
   signout_observer.SetOnPrimaryAccountClearedCallback(run_loop.QuitClosure());
-  identity_manager->GetPrimaryAccountManager()->SignOutAndKeepAllAccounts(
+  identity_manager->GetPrimaryAccountMutator()->RevokeSyncConsent(
       signin_metrics::SIGNOUT_TEST,
       signin_metrics::SignoutDelete::IGNORE_METRIC);
   run_loop.Run();
@@ -213,12 +215,13 @@ void ClearPrimaryAccount(IdentityManager* identity_manager) {
   if (!identity_manager->HasPrimaryAccount(ConsentLevel::kNotRequired))
     return;
 
+  DCHECK(identity_manager->GetPrimaryAccountMutator());
   bool wait_for_primary_acount_cleared_notification =
       identity_manager->HasPrimaryAccount(ConsentLevel::kSync);
   base::RunLoop run_loop;
   TestIdentityManagerObserver signout_observer(identity_manager);
   signout_observer.SetOnPrimaryAccountClearedCallback(run_loop.QuitClosure());
-  identity_manager->GetPrimaryAccountManager()->SignOutAndRemoveAllAccounts(
+  identity_manager->GetPrimaryAccountMutator()->ClearPrimaryAccount(
       signin_metrics::SIGNOUT_TEST,
       signin_metrics::SignoutDelete::IGNORE_METRIC);