From 7db902da0f7ad90b013f69e005957e22909ac13e Mon Sep 17 00:00:00 2001 From: Alex Ilin Date: Fri, 15 Oct 2021 09:37:24 +0000 Subject: [PATCH] [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 Commit-Queue: Alex Ilin Cr-Commit-Position: refs/heads/main@{#931942} --- .../account_manager/account_profile_mapper.cc | 2 + .../account_profile_mapper_unittest.cc | 105 +++++++++--------- .../profile_account_manager_unittest.cc | 17 ++- chrome/browser/profiles/profile_manager.cc | 4 +- chrome/test/base/testing_profile_manager.cc | 13 +++ chrome/test/base/testing_profile_manager.h | 9 ++ 6 files changed, 90 insertions(+), 60 deletions(-) diff --git a/chrome/browser/lacros/account_manager/account_profile_mapper.cc b/chrome/browser/lacros/account_manager/account_profile_mapper.cc index 661319b4a81489..8476e1342ec029 100644 --- 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 index ae1f775306ee00..6965079da6dad9 100644 --- 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 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 GetMapperNonInitialized( + AccountProfileMapper* CreateMapperNonInitialized( const AccountMapping& accounts) { - SetAccountsInStorage(accounts); - ExpectAccountsInStorage(accounts); ExpectFacadeGetAccountsCalled(); - std::unique_ptr mapper = + testing_profile_manager_.SetAccountProfileMapper( std::make_unique(mock_facade(), - attributes_storage()); - return mapper; + attributes_storage())); + SetAccountsInStorage(accounts); + ExpectAccountsInStorage(accounts); + return testing_profile_manager_.profile_manager() + ->GetAccountProfileMapper(); } - std::unique_ptr GetMapper( - const AccountMapping& accounts) { - std::unique_ptr 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 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&)> 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 mapper = - GetMapper({{main_path(), {"A"}}, {other_path, {"B", "C"}}}); + AccountProfileMapper* mapper = + CreateMapper({{main_path(), {"A"}}, {other_path, {"B", "C"}}}); base::MockRepeatingCallback&)> 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 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 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 mapper = - GetMapper({{main_path(), {"A"}}, {other_path, {"B"}}}); + AccountProfileMapper* mapper = + CreateMapper({{main_path(), {"A"}}, {other_path, {"B"}}}); base::MockRepeatingCallback 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 mapper = - GetMapperNonInitialized({{main_path(), {"A", "B"}}}); + AccountProfileMapper* mapper = + CreateMapperNonInitialized({{main_path(), {"A", "B"}}}); base::MockOnceCallback error_callback; base::MockOnceCallback&)> 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 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 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 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 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 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 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 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 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 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 mapper = - GetMapper({{main_path(), {"A"}}, {other_path, {"B"}}}); + AccountProfileMapper* mapper = + CreateMapper({{main_path(), {"A"}}, {other_path, {"B"}}}); MockAccountProfileMapperObserver mock_observer; base::ScopedObservation 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 mapper = - GetMapper({{main_path(), {"A"}}}); + AccountProfileMapper* mapper = CreateMapper({{main_path(), {"A"}}}); MockAccountProfileMapperObserver mock_observer; base::ScopedObservation 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 index 41b9a15c65ce9d..f1caf4ad2f3b0f 100644 --- 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,12 +22,17 @@ 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(&mock_facade_, storage()); + testing_profile_manager_.SetAccountProfileMapper( + std::make_unique(&mock_facade_, storage())); } - AccountProfileMapper* mapper() { return mapper_.get(); } + AccountProfileMapper* mapper() { + return testing_profile_manager_.profile_manager() + ->GetAccountProfileMapper(); + } private: ProfileAttributesStorage* storage() { @@ -33,10 +40,10 @@ class ProfileAccountManagerTest : public testing::Test { ->GetProfileAttributesStorage(); } + base::test::ScopedFeatureList scoped_feature_list_; content::BrowserTaskEnvironment task_environment_; - TestingProfileManager testing_profile_manager_; account_manager::MockAccountManagerFacade mock_facade_; - std::unique_ptr 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 index 7e70c2a0836ab2..c115e07edf460f 100644 --- 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( GetAccountManagerFacade(/*profile_path=*/std::string()), &GetProfileAttributesStorage()); diff --git a/chrome/test/base/testing_profile_manager.cc b/chrome/test/base/testing_profile_manager.cc index de4379ca5f5984..2a1466f5e03712 100644 --- 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 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_; diff --git a/chrome/test/base/testing_profile_manager.h b/chrome/test/base/testing_profile_manager.h index b032d7516f419e..6e8a3e85bab6ce 100644 --- a/chrome/test/base/testing_profile_manager.h +++ b/chrome/test/base/testing_profile_manager.h @@ -13,6 +13,7 @@ #include "base/files/file_path.h" #include "base/scoped_multi_source_observation.h" #include "base/test/scoped_path_override.h" +#include "build/chromeos_buildflags.h" #include "chrome/browser/profiles/profile_observer.h" #include "chrome/test/base/scoped_testing_local_state.h" #include "chrome/test/base/testing_profile.h" @@ -26,6 +27,10 @@ namespace sync_preferences { class PrefServiceSyncable; } +#if BUILDFLAG(IS_CHROMEOS_LACROS) +class AccountProfileMapper; +#endif + // The TestingProfileManager is a TestingProfile factory for a multi-profile // environment. It will bring up a full ProfileManager and attach it to the // TestingBrowserProcess set up in your test. @@ -112,6 +117,10 @@ class TestingProfileManager : public ProfileObserver { // Sets the last used profile; also sets the active time to now. void UpdateLastUser(Profile* last_active); +#if BUILDFLAG(IS_CHROMEOS_LACROS) + void SetAccountProfileMapper(std::unique_ptr mapper); +#endif + // Helper accessors. const base::FilePath& profiles_dir(); ProfileManager* profile_manager();