[Lacros] Do not create AccountProfileMapper if the feature is off

AccountProfileMapper should only be created if the kMultiProfileAccountConsistency feature is enabled. Otherwise, it might have negative side-effects like deleting user profiles. To add a DCHECK(kMultiProfileAccountConsistency) to AccountProfileMapper, this CL also enables kMultiProfileAccountConsistency in all related unit tests. This appears to be a non-trivial task because ProfileManager will now create its own instance of AccountProfileMapper that doesn't use an AccountManagerFacade mock. This CL adds a TestingProfileManager::SetAccountProfileMapper() methods to inject a fake instance for testing. Bug: 1226045 Change-Id: I69f74aed3f4bd9bdb107912d5e92872d53691a07 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3222809 Reviewed-by: David Roger <[email protected]> Commit-Queue: Alex Ilin <[email protected]> Cr-Commit-Position: refs/heads/main@{#931942}
blueboxd · Oct 15, 2021 · 7db902d · 7db902d
1 parent c09437a
commit 7db902d
Show file tree

Hide file tree

Showing 6 changed files with 90 additions and 60 deletions.
diff --git a/chrome/browser/lacros/account_manager/account_profile_mapper.cc b/chrome/browser/lacros/account_manager/account_profile_mapper.cc
@@ -17,6 +17,7 @@
 #include "chrome/browser/profiles/profile_attributes_entry.h"
 #include "chrome/browser/profiles/profile_attributes_storage.h"
 #include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/signin/signin_features.h"
 #include "components/account_manager_core/account.h"
 #include "google_apis/gaia/oauth2_access_token_fetcher.h"
 #include "google_apis/gaia/oauth2_access_token_fetcher_immediate_error.h"
@@ -41,6 +42,7 @@ AccountProfileMapper::AccountProfileMapper(
     ProfileAttributesStorage* storage)
     : account_manager_facade_(facade), profile_attributes_storage_(storage) {
   DCHECK(profile_attributes_storage_);
+  DCHECK(base::FeatureList::IsEnabled(kMultiProfileAccountConsistency));
   account_manager_facade_observation_.Observe(account_manager_facade_);
   account_manager_facade_->GetAccounts(
       base::BindOnce(&AccountProfileMapper::OnGetAccountsCompleted,

diff --git a/chrome/browser/lacros/account_manager/account_profile_mapper_unittest.cc b/chrome/browser/lacros/account_manager/account_profile_mapper_unittest.cc
@@ -13,11 +13,13 @@
 #include "base/files/file_path.h"
 #include "base/scoped_observation.h"
 #include "base/test/mock_callback.h"
+#include "base/test/scoped_feature_list.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_attributes_entry.h"
 #include "chrome/browser/profiles/profile_attributes_init_params.h"
 #include "chrome/browser/profiles/profile_attributes_storage.h"
 #include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/signin/signin_features.h"
 #include "chrome/test/base/profile_waiter.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_profile_manager.h"
@@ -142,7 +144,8 @@ std::vector<Account> AccountsFromGaiaIDs(
 class AccountProfileMapperTest : public testing::Test {
  public:
   AccountProfileMapperTest()
-      : testing_profile_manager_(TestingBrowserProcess::GetGlobal()) {
+      : scoped_feature_list_(kMultiProfileAccountConsistency),
+        testing_profile_manager_(TestingBrowserProcess::GetGlobal()) {
     CHECK(testing_profile_manager_.SetUp());
     main_path_ = GetProfilePath("Default");
     ON_CALL(mock_facade_, GetPersistentErrorForAccount)
@@ -207,21 +210,20 @@ class AccountProfileMapperTest : public testing::Test {
     ExpectAccountsInStorage(expected_accounts_in_storage);
   }
 
-  std::unique_ptr<AccountProfileMapper> GetMapperNonInitialized(
+  AccountProfileMapper* CreateMapperNonInitialized(
       const AccountMapping& accounts) {
-    SetAccountsInStorage(accounts);
-    ExpectAccountsInStorage(accounts);
     ExpectFacadeGetAccountsCalled();
-    std::unique_ptr<AccountProfileMapper> mapper =
+    testing_profile_manager_.SetAccountProfileMapper(
         std::make_unique<AccountProfileMapper>(mock_facade(),
-                                               attributes_storage());
-    return mapper;
+                                               attributes_storage()));
+    SetAccountsInStorage(accounts);
+    ExpectAccountsInStorage(accounts);
+    return testing_profile_manager_.profile_manager()
+        ->GetAccountProfileMapper();
   }
 
-  std::unique_ptr<AccountProfileMapper> GetMapper(
-      const AccountMapping& accounts) {
-    std::unique_ptr<AccountProfileMapper> mapper =
-        GetMapperNonInitialized(accounts);
+  AccountProfileMapper* CreateMapper(const AccountMapping& accounts) {
+    AccountProfileMapper* mapper = CreateMapperNonInitialized(accounts);
     // Initialize the mapper by completing the `GetAccounts()` call on the
     // facade.
     std::vector<std::string> accounts_in_facade;
@@ -363,12 +365,13 @@ class AccountProfileMapperTest : public testing::Test {
   }
 
  private:
+  base::test::ScopedFeatureList scoped_feature_list_;
   content::BrowserTaskEnvironment task_environment_;
-  TestingProfileManager testing_profile_manager_;
-  base::FilePath main_path_;
   account_manager::MockAccountManagerFacade mock_facade_;
   base::OnceCallback<void(const std::vector<Account>&)>
       facade_get_accounts_completion_;
+  TestingProfileManager testing_profile_manager_;
+  base::FilePath main_path_;
 };
 
 // Test basic functionality for `GetAccounts()`:
@@ -377,8 +380,8 @@ class AccountProfileMapperTest : public testing::Test {
 // - does not trigger a call to GetAccounts() on the facade.
 TEST_F(AccountProfileMapperTest, GetAccounts) {
   base::FilePath other_path = GetProfilePath("Other");
-  std::unique_ptr<AccountProfileMapper> mapper =
-      GetMapper({{main_path(), {"A"}}, {other_path, {"B", "C"}}});
+  AccountProfileMapper* mapper =
+      CreateMapper({{main_path(), {"A"}}, {other_path, {"B", "C"}}});
 
   base::MockRepeatingCallback<void(const std::vector<Account>&)> mock_callback;
 
@@ -406,9 +409,8 @@ TEST_F(AccountProfileMapperTest, GetAccounts) {
 // Tests that accounts are added by default to the main profile when there is
 // only one profile.
 TEST_F(AccountProfileMapperTest, UpdateSingleProfile) {
-  std::unique_ptr<AccountProfileMapper> mapper =
-      GetMapper({{main_path(), {"A", "B"}}});
-  TestMapperUpdateGaia(mapper.get(),
+  AccountProfileMapper* mapper = CreateMapper({{main_path(), {"A", "B"}}});
+  TestMapperUpdateGaia(mapper,
                        /*accounts_in_facade=*/{"A", "C"},
                        /*expected_accounts_upserted=*/{{main_path(), {"C"}}},
                        /*expected_accounts_removed=*/{{main_path(), {"B"}}},
@@ -419,10 +421,10 @@ TEST_F(AccountProfileMapperTest, UpdateSingleProfile) {
 // Tests that new accounts are left unassigned when there are multiple profiles.
 TEST_F(AccountProfileMapperTest, UpdateMulltiProfile) {
   base::FilePath other_path = GetProfilePath("Other");
-  std::unique_ptr<AccountProfileMapper> mapper =
-      GetMapper({{main_path(), {"A"}}, {other_path, {"B", "C"}}});
+  AccountProfileMapper* mapper =
+      CreateMapper({{main_path(), {"A"}}, {other_path, {"B", "C"}}});
   TestMapperUpdateGaia(
-      mapper.get(),
+      mapper,
       /*accounts_in_facade=*/{"A", "B", "D"},
       /*expected_accounts_upserted=*/{{base::FilePath(), {"D"}}},
       /*expected_accounts_removed=*/{{other_path, {"C"}}},
@@ -434,8 +436,8 @@ TEST_F(AccountProfileMapperTest, UpdateMulltiProfile) {
 // account is not in this profile.
 TEST_F(AccountProfileMapperTest, GetPersistentErrorForAccount) {
   base::FilePath other_path = GetProfilePath("Other");
-  std::unique_ptr<AccountProfileMapper> mapper =
-      GetMapper({{main_path(), {"A"}}, {other_path, {"B"}}});
+  AccountProfileMapper* mapper =
+      CreateMapper({{main_path(), {"A"}}, {other_path, {"B"}}});
   base::MockRepeatingCallback<void(const GoogleServiceAuthError&)>
       mock_callback;
 
@@ -456,8 +458,8 @@ TEST_F(AccountProfileMapperTest, GetPersistentErrorForAccount) {
 
 // Tests that consumer callbacks are delayed until initialization completes.
 TEST_F(AccountProfileMapperTest, WaitForInitialization) {
-  std::unique_ptr<AccountProfileMapper> mapper =
-      GetMapperNonInitialized({{main_path(), {"A", "B"}}});
+  AccountProfileMapper* mapper =
+      CreateMapperNonInitialized({{main_path(), {"A", "B"}}});
   base::MockOnceCallback<void(const GoogleServiceAuthError&)> error_callback;
   base::MockOnceCallback<void(const std::vector<Account>&)> accounts_callback;
   // Call the mapper before initialization, callback not invoked.
@@ -480,15 +482,15 @@ TEST_F(AccountProfileMapperTest, WaitForInitialization) {
 }
 
 TEST_F(AccountProfileMapperTest, NoObserversAtInitialization) {
-  std::unique_ptr<AccountProfileMapper> mapper =
-      GetMapperNonInitialized({{main_path(), {"A"}}});
+  AccountProfileMapper* mapper =
+      CreateMapperNonInitialized({{main_path(), {"A"}}});
   // Change the storage, so that observers would normally trigger.
   SetAccountsInStorage({{main_path(), {"A", "B"}}});
 
   MockAccountProfileMapperObserver mock_observer;
   base::ScopedObservation<AccountProfileMapper, AccountProfileMapper::Observer>
       observation{&mock_observer};
-  observation.Observe(mapper.get());
+  observation.Observe(mapper);
   EXPECT_CALL(mock_observer, OnAccountUpserted(testing::_, testing::_))
       .Times(0);
   EXPECT_CALL(mock_observer, OnAccountRemoved(testing::_, testing::_)).Times(0);
@@ -502,17 +504,15 @@ TEST_F(AccountProfileMapperTest, NoObserversAtInitialization) {
 }
 
 TEST_F(AccountProfileMapperTest, NonGaia) {
-  std::unique_ptr<AccountProfileMapper> mapper =
-      GetMapper({{main_path(), {"A"}}});
+  AccountProfileMapper* mapper = CreateMapper({{main_path(), {"A"}}});
   // Addition of non-Gaia account is ignored.
-  TestMapperUpdate(mapper.get(),
-                   {AccountFromGaiaID("A"), NonGaiaAccountFromID("B")},
+  TestMapperUpdate(mapper, {AccountFromGaiaID("A"), NonGaiaAccountFromID("B")},
                    /*expected_accounts_upserted=*/{},
                    /*expected_accounts_removed=*/{},
                    /*expected_accounts_in_storage=*/
                    {{main_path(), {"A"}}});
   // Removal is ignored as well.
-  TestMapperUpdate(mapper.get(), {AccountFromGaiaID("A")},
+  TestMapperUpdate(mapper, {AccountFromGaiaID("A")},
                    /*expected_accounts_upserted=*/{},
                    /*expected_accounts_removed=*/{},
                    /*expected_accounts_in_storage=*/
@@ -525,10 +525,10 @@ TEST_F(AccountProfileMapperTest, NonGaia) {
 // since it's an only remaining profile.
 TEST_F(AccountProfileMapperTest, RemovePrimaryAccountFromSecondaryProfile) {
   base::FilePath other_path = GetProfilePath("Other");
-  std::unique_ptr<AccountProfileMapper> mapper =
-      GetMapper({{main_path(), {"A"}}, {other_path, {"B", "C"}}});
+  AccountProfileMapper* mapper =
+      CreateMapper({{main_path(), {"A"}}, {other_path, {"B", "C"}}});
   SetPrimaryAccountForProfile(other_path, "B");
-  TestMapperUpdateGaia(mapper.get(),
+  TestMapperUpdateGaia(mapper,
                        /*accounts_in_facade=*/{"A", "C"},
                        /*expected_accounts_upserted=*/{{main_path(), {"C"}}},
                        /*expected_accounts_removed=*/{{other_path, {"B"}}},
@@ -546,11 +546,11 @@ TEST_F(AccountProfileMapperTest,
        RemovePrimaryAccountFromSecondaryProfile_MultipleProfiles) {
   base::FilePath second_path = GetProfilePath("Second");
   base::FilePath third_path = GetProfilePath("Third");
-  std::unique_ptr<AccountProfileMapper> mapper = GetMapper(
+  AccountProfileMapper* mapper = CreateMapper(
       {{main_path(), {"A"}}, {second_path, {"B", "C"}}, {third_path, {"D"}}});
   SetPrimaryAccountForProfile(second_path, "B");
   TestMapperUpdateGaia(
-      mapper.get(),
+      mapper,
       /*accounts_in_facade=*/{"A", "C", "D"},
       /*expected_accounts_upserted=*/{{base::FilePath(), {"C"}}},
       /*expected_accounts_removed=*/{{second_path, {"B"}}},
@@ -567,8 +567,7 @@ TEST_F(AccountProfileMapperTest,
 TEST_F(AccountProfileMapperTest,
        RemovePrimaryAccountFromSecondaryProfile_AtInitialization) {
   base::FilePath other_path = GetProfilePath("Other");
-  std::unique_ptr<AccountProfileMapper> mapper =
-      GetMapperNonInitialized({{main_path(), {"A"}}, {other_path, {"B", "C"}}});
+  CreateMapperNonInitialized({{main_path(), {"A"}}, {other_path, {"B", "C"}}});
   SetPrimaryAccountForProfile(other_path, "B");
   CompleteFacadeGetAccountsGaia({"A", "C"});
   ExpectAccountsInStorage({{main_path(), {"A", "C"}}});
@@ -580,10 +579,10 @@ TEST_F(AccountProfileMapperTest,
 // account is removed from the system.
 TEST_F(AccountProfileMapperTest, RemoveSecondaryAccountFromSecondaryProfile) {
   base::FilePath other_path = GetProfilePath("Other");
-  std::unique_ptr<AccountProfileMapper> mapper =
-      GetMapper({{main_path(), {"A"}}, {other_path, {"B", "C"}}});
+  AccountProfileMapper* mapper =
+      CreateMapper({{main_path(), {"A"}}, {other_path, {"B", "C"}}});
   SetPrimaryAccountForProfile(other_path, "B");
-  TestMapperUpdateGaia(mapper.get(),
+  TestMapperUpdateGaia(mapper,
                        /*accounts_in_facade=*/{"A", "B"},
                        /*expected_accounts_upserted=*/{},
                        /*expected_accounts_removed=*/{{other_path, {"C"}}},
@@ -594,10 +593,9 @@ TEST_F(AccountProfileMapperTest, RemoveSecondaryAccountFromSecondaryProfile) {
 // Tests that the primary profile doesn't get deleted even after its primary
 // account is removed from the system.
 TEST_F(AccountProfileMapperTest, RemovePrimaryAccountFromPrimaryProfile) {
-  std::unique_ptr<AccountProfileMapper> mapper =
-      GetMapper({{main_path(), {"A", "B"}}});
+  AccountProfileMapper* mapper = CreateMapper({{main_path(), {"A", "B"}}});
   SetPrimaryAccountForProfile(main_path(), "A");
-  TestMapperUpdateGaia(mapper.get(),
+  TestMapperUpdateGaia(mapper,
                        /*accounts_in_facade=*/{"B"},
                        /*expected_accounts_upserted=*/{},
                        /*expected_accounts_removed=*/{{main_path(), {"A"}}},
@@ -606,8 +604,8 @@ TEST_F(AccountProfileMapperTest, RemovePrimaryAccountFromPrimaryProfile) {
 }
 
 TEST_F(AccountProfileMapperTest, ShowAddAccountDialogBeforeInit) {
-  std::unique_ptr<AccountProfileMapper> mapper =
-      GetMapperNonInitialized({{main_path(), {"A"}}});
+  AccountProfileMapper* mapper =
+      CreateMapperNonInitialized({{main_path(), {"A"}}});
   // The facade is not called before initialization.
   EXPECT_CALL(*mock_facade(), ShowAddAccountDialog(testing::_, testing::_))
       .Times(0);
@@ -626,13 +624,13 @@ TEST_F(AccountProfileMapperTest, ShowAddAccountDialogBeforeInit) {
 
 TEST_F(AccountProfileMapperTest, ShowAddAccountDialog) {
   base::FilePath other_path = GetProfilePath("Other");
-  std::unique_ptr<AccountProfileMapper> mapper =
-      GetMapper({{main_path(), {"A"}}, {other_path, {"B"}}});
+  AccountProfileMapper* mapper =
+      CreateMapper({{main_path(), {"A"}}, {other_path, {"B"}}});
 
   MockAccountProfileMapperObserver mock_observer;
   base::ScopedObservation<AccountProfileMapper, AccountProfileMapper::Observer>
       observation{&mock_observer};
-  observation.Observe(mapper.get());
+  observation.Observe(mapper);
 
   MockAddAccountCallback account_added_callback;
   Account account_c = AccountFromGaiaID("C");
@@ -703,12 +701,11 @@ TEST_F(AccountProfileMapperTest, ShowAddAccountDialog) {
 }
 
 TEST_F(AccountProfileMapperTest, ShowAddAccountDialogNewProfile) {
-  std::unique_ptr<AccountProfileMapper> mapper =
-      GetMapper({{main_path(), {"A"}}});
+  AccountProfileMapper* mapper = CreateMapper({{main_path(), {"A"}}});
   MockAccountProfileMapperObserver mock_observer;
   base::ScopedObservation<AccountProfileMapper, AccountProfileMapper::Observer>
       observation{&mock_observer};
-  observation.Observe(mapper.get());
+  observation.Observe(mapper);
   MockAddAccountCallback account_added_callback;
 
   // Set expectations: a new profile should be created for the new account.

diff --git a/chrome/browser/lacros/account_manager/profile_account_manager_unittest.cc b/chrome/browser/lacros/account_manager/profile_account_manager_unittest.cc
@@ -7,7 +7,9 @@
 #include "base/check.h"
 #include "base/files/file_path.h"
 #include "base/scoped_observation.h"
+#include "base/test/scoped_feature_list.h"
 #include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/signin/signin_features.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_profile_manager.h"
 #include "components/account_manager_core/account.h"
@@ -20,23 +22,28 @@
 class ProfileAccountManagerTest : public testing::Test {
  public:
   ProfileAccountManagerTest()
-      : testing_profile_manager_(TestingBrowserProcess::GetGlobal()) {
+      : scoped_feature_list_(kMultiProfileAccountConsistency),
+        testing_profile_manager_(TestingBrowserProcess::GetGlobal()) {
     CHECK(testing_profile_manager_.SetUp());
-    mapper_ = std::make_unique<AccountProfileMapper>(&mock_facade_, storage());
+    testing_profile_manager_.SetAccountProfileMapper(
+        std::make_unique<AccountProfileMapper>(&mock_facade_, storage()));
   }
 
-  AccountProfileMapper* mapper() { return mapper_.get(); }
+  AccountProfileMapper* mapper() {
+    return testing_profile_manager_.profile_manager()
+        ->GetAccountProfileMapper();
+  }
 
  private:
   ProfileAttributesStorage* storage() {
     return &testing_profile_manager_.profile_manager()
                 ->GetProfileAttributesStorage();
   }
 
+  base::test::ScopedFeatureList scoped_feature_list_;
   content::BrowserTaskEnvironment task_environment_;
-  TestingProfileManager testing_profile_manager_;
   account_manager::MockAccountManagerFacade mock_facade_;
-  std::unique_ptr<AccountProfileMapper> mapper_;
+  TestingProfileManager testing_profile_manager_;
 };
 
 TEST_F(ProfileAccountManagerTest, Observer) {

diff --git a/chrome/browser/profiles/profile_manager.cc b/chrome/browser/profiles/profile_manager.cc
@@ -70,6 +70,7 @@
 #include "chrome/browser/profiles/profiles_state.h"
 #include "chrome/browser/signin/account_reconcilor_factory.h"
 #include "chrome/browser/signin/identity_manager_factory.h"
+#include "chrome/browser/signin/signin_features.h"
 #include "chrome/browser/signin/signin_util.h"
 #include "chrome/browser/sync/sync_service_factory.h"
 #include "chrome/browser/ui/startup/startup_browser_creator.h"
@@ -1019,7 +1020,8 @@ ProfileShortcutManager* ProfileManager::profile_shortcut_manager() {
 
 #if BUILDFLAG(IS_CHROMEOS_LACROS)
 AccountProfileMapper* ProfileManager::GetAccountProfileMapper() {
-  if (!account_profile_mapper_) {
+  if (!account_profile_mapper_ &&
+      base::FeatureList::IsEnabled(kMultiProfileAccountConsistency)) {
     account_profile_mapper_ = std::make_unique<AccountProfileMapper>(
         GetAccountManagerFacade(/*profile_path=*/std::string()),
         &GetProfileAttributesStorage());

diff --git a/chrome/test/base/testing_profile_manager.cc b/chrome/test/base/testing_profile_manager.cc
@@ -29,6 +29,10 @@
 #include "chrome/browser/ash/profiles/profile_helper.h"
 #endif
 
+#if BUILDFLAG(IS_CHROMEOS_LACROS)
+#include "chrome/browser/lacros/account_manager/account_profile_mapper.h"
+#endif
+
 const char kGuestProfileName[] = "Guest";
 const char kSystemProfileName[] = "System";
 
@@ -248,6 +252,15 @@ void TestingProfileManager::UpdateLastUser(Profile* last_active) {
 #endif
 }
 
+#if BUILDFLAG(IS_CHROMEOS_LACROS)
+void TestingProfileManager::SetAccountProfileMapper(
+    std::unique_ptr<AccountProfileMapper> mapper) {
+  DCHECK(!profile_manager_->account_profile_mapper_)
+      << "AccountProfileMapper must be set before the first usage";
+  profile_manager_->account_profile_mapper_ = std::move(mapper);
+}
+#endif
+
 const base::FilePath& TestingProfileManager::profiles_dir() {
   DCHECK(called_set_up_);
   return profiles_path_;