Make methods in AccountsMutatorImpl NOTREACHED on Chrome OS

On Chrome OS we should not change accounts using AccountsMutatorImpl.
All methods except UpdateAccountInfo should be NOTREACHED.

Bug: 1068240
Change-Id: Ib25550b82e8b81d7b58c0995a6809e148c4e3b33
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2317351
Commit-Queue: Anastasiia N <anastasiian@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Kush Sinha <sinhak@chromium.org>
Cr-Commit-Position: refs/heads/master@{#792415}

chrome/browser/chromeos/arc/auth/arc_auth_service_browsertest.cc

@@ -836,9 +836,8 @@ IN_PROC_BROWSER_TEST_P(ArcAuthServiceTest, AccountRemovalsArePropagated) {
   // be sent.
   EnableRemovalOfExtendedAccountInfo();
 
-  identity_manager->GetAccountsMutator()->RemoveAccount(
-      maybe_account_info.value().account_id,
-      signin_metrics::SourceForRefreshTokenOperation::kUnknown);
+  signin::RemoveRefreshTokenForAccount(identity_manager,
+                                       maybe_account_info.value().account_id);
   base::RunLoop().RunUntilIdle();
 
   EXPECT_EQ(1, auth_instance().num_account_removed_calls());

chrome/browser/safe_browsing/advanced_protection_status_manager_unittest.cc

@@ -329,9 +329,7 @@ TEST_F(AdvancedProtectionStatusManagerTest, AccountRemoval) {
   // This call is necessary to ensure that the account removal is fully
   // processed in this testing context.
   identity_test_env_.EnableRemovalOfExtendedAccountInfo();
-  identity_test_env_.identity_manager()->GetAccountsMutator()->RemoveAccount(
-      account_id,
-      signin_metrics::SourceForRefreshTokenOperation::kUserMenu_RemoveAccount);
+  identity_test_env_.RemoveRefreshTokenForAccount(account_id);
   EXPECT_FALSE(aps_manager.IsUnderAdvancedProtection());
   EXPECT_TRUE(
       pref_service_.HasPrefPath(prefs::kAdvancedProtectionLastRefreshInUs));

components/signin/internal/identity_manager/accounts_mutator_impl.cc

@@ -42,6 +42,9 @@ CoreAccountId AccountsMutatorImpl::AddOrUpdateAccount(
     const std::string& refresh_token,
     bool is_under_advanced_protection,
     signin_metrics::SourceForRefreshTokenOperation source) {
+#if defined(OS_CHROMEOS)
+  NOTREACHED();
+#endif
   CoreAccountId account_id =
       account_tracker_service_->SeedAccountInfo(gaia_id, email);
   account_tracker_service_->SetIsAdvancedProtectionAccount(
@@ -75,16 +78,25 @@ void AccountsMutatorImpl::UpdateAccountInfo(
 void AccountsMutatorImpl::RemoveAccount(
     const CoreAccountId& account_id,
     signin_metrics::SourceForRefreshTokenOperation source) {
+#if defined(OS_CHROMEOS)
+  NOTREACHED();
+#endif
   token_service_->RevokeCredentials(account_id, source);
 }
 
 void AccountsMutatorImpl::RemoveAllAccounts(
     signin_metrics::SourceForRefreshTokenOperation source) {
+#if defined(OS_CHROMEOS)
+  NOTREACHED();
+#endif
   token_service_->RevokeAllCredentials(source);
 }
 
 void AccountsMutatorImpl::InvalidateRefreshTokenForPrimaryAccount(
     signin_metrics::SourceForRefreshTokenOperation source) {
+#if defined(OS_CHROMEOS)
+  NOTREACHED();
+#endif
   DCHECK(primary_account_manager_->IsAuthenticated());
   CoreAccountInfo primary_account_info =
       primary_account_manager_->GetAuthenticatedAccountInfo();

components/signin/public/identity_manager/accounts_mutator_unittest.cc

@@ -20,12 +20,14 @@
 
 namespace {
 
+const char kTestEmail[] = "test_user@test.com";
+#if !defined(OS_CHROMEOS)
 const char kTestGaiaId[] = "gaia-id-test_user-test.com";
 const char kTestGaiaId2[] = "gaia-id-test_user-2-test.com";
-const char kTestEmail[] = "test_user@test.com";
 const char kTestEmail2[] = "test_user@test-2.com";
 const char kRefreshToken[] = "refresh_token";
 const char kRefreshToken2[] = "refresh_token_2";
+#endif
 
 // Class that observes diagnostics updates from signin::IdentityManager.
 class TestIdentityManagerDiagnosticsObserver
@@ -99,6 +101,8 @@ class AccountsMutatorTest : public testing::Test {
     return identity_test_env_.identity_manager_observer();
   }
 
+  IdentityTestEnvironment* identity_test_env() { return &identity_test_env_; }
+
   TestIdentityManagerDiagnosticsObserver*
   identity_manager_diagnostics_observer() {
     return &identity_manager_diagnostics_observer_;
@@ -122,6 +126,93 @@ TEST_F(AccountsMutatorTest, Basic) {
   // Should not crash.
 }
 
+// Test that the information of an existing account for a given ID gets updated.
+TEST_F(AccountsMutatorTest, UpdateAccountInfo) {
+  // Abort the test if the current platform does not support accounts mutation.
+  if (!accounts_mutator())
+    return;
+
+  // First of all add the account to the account tracker service.
+  base::RunLoop run_loop;
+  identity_manager_observer()->SetOnRefreshTokenUpdatedCallback(
+      run_loop.QuitClosure());
+
+  CoreAccountId account_id =
+      identity_test_env()->MakePrimaryAccountAvailable(kTestEmail).account_id;
+  run_loop.Run();
+
+  EXPECT_EQ(identity_manager()->GetAccountsWithRefreshTokens().size(), 1U);
+
+  AccountInfo original_account_info =
+      identity_manager()
+          ->FindExtendedAccountInfoForAccountWithRefreshTokenByAccountId(
+              account_id)
+          .value();
+  EXPECT_EQ(original_account_info.account_id, account_id);
+  EXPECT_EQ(original_account_info.email, kTestEmail);
+  EXPECT_FALSE(original_account_info.is_child_account);
+  EXPECT_FALSE(original_account_info.is_under_advanced_protection);
+
+  accounts_mutator()->UpdateAccountInfo(
+      account_id,
+      /*is_child_account=*/true,
+      /*is_under_advanced_protection=*/base::nullopt);
+  AccountInfo updated_account_info_1 =
+      identity_manager()
+          ->FindExtendedAccountInfoForAccountWithRefreshTokenByAccountId(
+              account_id)
+          .value();
+
+  // Only |is_child_account| changed so far, everything else remains the same.
+  EXPECT_EQ(identity_manager()->GetAccountsWithRefreshTokens().size(), 1U);
+  EXPECT_EQ(updated_account_info_1.account_id,
+            original_account_info.account_id);
+  EXPECT_EQ(updated_account_info_1.email, original_account_info.email);
+  EXPECT_NE(updated_account_info_1.is_child_account,
+            original_account_info.is_child_account);
+  EXPECT_EQ(updated_account_info_1.is_under_advanced_protection,
+            original_account_info.is_under_advanced_protection);
+
+  accounts_mutator()->UpdateAccountInfo(account_id,
+                                        /*is_child_account=*/base::nullopt,
+                                        /*is_under_advanced_protection=*/true);
+  AccountInfo updated_account_info_2 =
+      identity_manager()
+          ->FindExtendedAccountInfoForAccountWithRefreshTokenByAccountId(
+              account_id)
+          .value();
+
+  // |is_under_advanced_protection| has changed now, but |is_child_account|
+  // remains the same since we previously set it to |true| in the previous step.
+  EXPECT_NE(updated_account_info_2.is_under_advanced_protection,
+            original_account_info.is_under_advanced_protection);
+  EXPECT_EQ(updated_account_info_2.is_child_account,
+            updated_account_info_1.is_child_account);
+
+  // Last, reset |is_child_account| and |is_under_advanced_protection| together
+  // to its initial |false| value, which is no longer the case.
+  EXPECT_TRUE(updated_account_info_2.is_child_account);
+  EXPECT_TRUE(updated_account_info_2.is_under_advanced_protection);
+
+  accounts_mutator()->UpdateAccountInfo(account_id,
+                                        /*is_child_account=*/false,
+                                        /*is_under_advanced_protection=*/false);
+  AccountInfo reset_account_info =
+      identity_manager()
+          ->FindExtendedAccountInfoForAccountWithRefreshTokenByAccountId(
+              account_id)
+          .value();
+
+  // Everything is back to its original state now.
+  EXPECT_EQ(reset_account_info.is_child_account,
+            original_account_info.is_child_account);
+  EXPECT_EQ(reset_account_info.is_under_advanced_protection,
+            original_account_info.is_under_advanced_protection);
+  EXPECT_FALSE(reset_account_info.is_child_account);
+  EXPECT_FALSE(reset_account_info.is_under_advanced_protection);
+}
+
+#if !defined(OS_CHROMEOS)
 // Test that a new account gets added to the AccountTrackerService when calling
 // AddOrUpdateAccount() and that a new refresh token becomes available for the
 // passed account_id when adding an account for the first time.
@@ -234,94 +325,6 @@ TEST_F(AccountsMutatorTest, AddOrUpdateAccount_UpdateExistingAccount) {
             updated_account_info.is_under_advanced_protection);
 }
 
-// Test that the information of an existing account for a given ID gets updated.
-TEST_F(AccountsMutatorTest, UpdateAccountInfo) {
-  // Abort the test if the current platform does not support accounts mutation.
-  if (!accounts_mutator())
-    return;
-
-  // First of all add the account to the account tracker service.
-  base::RunLoop run_loop;
-  identity_manager_observer()->SetOnRefreshTokenUpdatedCallback(
-      run_loop.QuitClosure());
-
-  CoreAccountId account_id = accounts_mutator()->AddOrUpdateAccount(
-      kTestGaiaId, kTestEmail, kRefreshToken,
-      /*is_under_advanced_protection=*/false,
-      signin_metrics::SourceForRefreshTokenOperation::kUnknown);
-  run_loop.Run();
-
-  EXPECT_EQ(identity_manager()->GetAccountsWithRefreshTokens().size(), 1U);
-
-  AccountInfo original_account_info =
-      identity_manager()
-          ->FindExtendedAccountInfoForAccountWithRefreshTokenByAccountId(
-              account_id)
-          .value();
-  EXPECT_EQ(original_account_info.account_id, account_id);
-  EXPECT_EQ(original_account_info.email, kTestEmail);
-  EXPECT_FALSE(original_account_info.is_child_account);
-  EXPECT_FALSE(original_account_info.is_under_advanced_protection);
-
-  accounts_mutator()->UpdateAccountInfo(
-      account_id,
-      /*is_child_account=*/true,
-      /*is_under_advanced_protection=*/base::nullopt);
-  AccountInfo updated_account_info_1 =
-      identity_manager()
-          ->FindExtendedAccountInfoForAccountWithRefreshTokenByAccountId(
-              account_id)
-          .value();
-
-  // Only |is_child_account| changed so far, everything else remains the same.
-  EXPECT_EQ(identity_manager()->GetAccountsWithRefreshTokens().size(), 1U);
-  EXPECT_EQ(updated_account_info_1.account_id,
-            original_account_info.account_id);
-  EXPECT_EQ(updated_account_info_1.email, original_account_info.email);
-  EXPECT_NE(updated_account_info_1.is_child_account,
-            original_account_info.is_child_account);
-  EXPECT_EQ(updated_account_info_1.is_under_advanced_protection,
-            original_account_info.is_under_advanced_protection);
-
-  accounts_mutator()->UpdateAccountInfo(account_id,
-                                        /*is_child_account=*/base::nullopt,
-                                        /*is_under_advanced_protection=*/true);
-  AccountInfo updated_account_info_2 =
-      identity_manager()
-          ->FindExtendedAccountInfoForAccountWithRefreshTokenByAccountId(
-              account_id)
-          .value();
-
-  // |is_under_advanced_protection| has changed now, but |is_child_account|
-  // remains the same since we previously set it to |true| in the previous step.
-  EXPECT_NE(updated_account_info_2.is_under_advanced_protection,
-            original_account_info.is_under_advanced_protection);
-  EXPECT_EQ(updated_account_info_2.is_child_account,
-            updated_account_info_1.is_child_account);
-
-  // Last, reset |is_child_account| and |is_under_advanced_protection| together
-  // to its initial |false| value, which is no longer the case.
-  EXPECT_TRUE(updated_account_info_2.is_child_account);
-  EXPECT_TRUE(updated_account_info_2.is_under_advanced_protection);
-
-  accounts_mutator()->UpdateAccountInfo(account_id,
-                                        /*is_child_account=*/false,
-                                        /*is_under_advanced_protection=*/false);
-  AccountInfo reset_account_info =
-      identity_manager()
-          ->FindExtendedAccountInfoForAccountWithRefreshTokenByAccountId(
-              account_id)
-          .value();
-
-  // Everything is back to its original state now.
-  EXPECT_EQ(reset_account_info.is_child_account,
-            original_account_info.is_child_account);
-  EXPECT_EQ(reset_account_info.is_under_advanced_protection,
-            original_account_info.is_under_advanced_protection);
-  EXPECT_FALSE(reset_account_info.is_child_account);
-  EXPECT_FALSE(reset_account_info.is_under_advanced_protection);
-}
-
 TEST_F(AccountsMutatorTest,
        InvalidateRefreshTokenForPrimaryAccount_WithPrimaryAccount) {
   // Abort the test if the current platform does not support accounts mutation.
@@ -567,50 +570,6 @@ TEST_F(AccountsMutatorTest, RemoveAllAccounts) {
   EXPECT_EQ(identity_manager()->GetAccountsWithRefreshTokens().size(), 0U);
 }
 
-#if BUILDFLAG(ENABLE_DICE_SUPPORT)
-TEST_F(AccountsMutatorTest, MoveAccount) {
-  // All platforms that support DICE also support account mutation.
-  DCHECK(accounts_mutator());
-
-  AccountInfo account_info =
-      MakeAccountAvailable(identity_manager(), kTestEmail);
-  EXPECT_TRUE(
-      identity_manager()->HasAccountWithRefreshToken(account_info.account_id));
-  EXPECT_FALSE(
-      identity_manager()->HasAccountWithRefreshTokenInPersistentErrorState(
-          account_info.account_id));
-  EXPECT_EQ(1U, identity_manager()->GetAccountsWithRefreshTokens().size());
-
-  IdentityTestEnvironment other_identity_test_env;
-  auto* other_accounts_mutator =
-      other_identity_test_env.identity_manager()->GetAccountsMutator();
-
-  std::string device_id_1 = GetOrCreateScopedDeviceId(pref_service());
-  EXPECT_FALSE(device_id_1.empty());
-
-  accounts_mutator()->MoveAccount(other_accounts_mutator,
-                                  account_info.account_id);
-  EXPECT_EQ(0U, identity_manager()->GetAccountsWithRefreshTokens().size());
-
-  std::string device_id_2 = GetOrCreateScopedDeviceId(pref_service());
-  EXPECT_FALSE(device_id_2.empty());
-  // |device_id_1| and |device_id_2| should be different as the divice ID is
-  // recreated in MoveAccount().
-  EXPECT_NE(device_id_1, device_id_2);
-
-  auto other_accounts_with_refresh_token =
-      other_identity_test_env.identity_manager()
-          ->GetAccountsWithRefreshTokens();
-  EXPECT_EQ(1U, other_accounts_with_refresh_token.size());
-  EXPECT_TRUE(
-      other_identity_test_env.identity_manager()->HasAccountWithRefreshToken(
-          other_accounts_with_refresh_token[0].account_id));
-  EXPECT_FALSE(other_identity_test_env.identity_manager()
-                   ->HasAccountWithRefreshTokenInPersistentErrorState(
-                       other_accounts_with_refresh_token[0].account_id));
-}
-#endif  // BUILDFLAG(ENABLE_DICE_SUPPORT)
-
 TEST_F(AccountsMutatorTest, UpdateAccessTokenFromSource) {
   // Abort the test if the current platform does not support accounts mutation.
   if (!accounts_mutator())
@@ -658,5 +617,50 @@ TEST_F(AccountsMutatorTest, RemoveRefreshTokenFromSource) {
   EXPECT_EQ("Settings::Signout",
             identity_manager_diagnostics_observer()->token_remover_source());
 }
+#endif  // !defined(OS_CHROMEOS)
+
+#if BUILDFLAG(ENABLE_DICE_SUPPORT)
+TEST_F(AccountsMutatorTest, MoveAccount) {
+  // All platforms that support DICE also support account mutation.
+  DCHECK(accounts_mutator());
+
+  AccountInfo account_info =
+      MakeAccountAvailable(identity_manager(), kTestEmail);
+  EXPECT_TRUE(
+      identity_manager()->HasAccountWithRefreshToken(account_info.account_id));
+  EXPECT_FALSE(
+      identity_manager()->HasAccountWithRefreshTokenInPersistentErrorState(
+          account_info.account_id));
+  EXPECT_EQ(1U, identity_manager()->GetAccountsWithRefreshTokens().size());
+
+  IdentityTestEnvironment other_identity_test_env;
+  auto* other_accounts_mutator =
+      other_identity_test_env.identity_manager()->GetAccountsMutator();
+
+  std::string device_id_1 = GetOrCreateScopedDeviceId(pref_service());
+  EXPECT_FALSE(device_id_1.empty());
+
+  accounts_mutator()->MoveAccount(other_accounts_mutator,
+                                  account_info.account_id);
+  EXPECT_EQ(0U, identity_manager()->GetAccountsWithRefreshTokens().size());
+
+  std::string device_id_2 = GetOrCreateScopedDeviceId(pref_service());
+  EXPECT_FALSE(device_id_2.empty());
+  // |device_id_1| and |device_id_2| should be different as the device ID is
+  // recreated in MoveAccount().
+  EXPECT_NE(device_id_1, device_id_2);
+
+  auto other_accounts_with_refresh_token =
+      other_identity_test_env.identity_manager()
+          ->GetAccountsWithRefreshTokens();
+  EXPECT_EQ(1U, other_accounts_with_refresh_token.size());
+  EXPECT_TRUE(
+      other_identity_test_env.identity_manager()->HasAccountWithRefreshToken(
+          other_accounts_with_refresh_token[0].account_id));
+  EXPECT_FALSE(other_identity_test_env.identity_manager()
+                   ->HasAccountWithRefreshTokenInPersistentErrorState(
+                       other_accounts_with_refresh_token[0].account_id));
+}
+#endif  // BUILDFLAG(ENABLE_DICE_SUPPORT)
 
 }  // namespace signin