From 9353b93fd60ba7cd3e197946ec92b65ea55693e4 Mon Sep 17 00:00:00 2001 From: Shivan Kaul Sahib Date: Fri, 17 Jan 2025 01:34:20 -0800 Subject: [PATCH 1/2] Enable passwords syncing by default --- browser/about_flags.cc | 9 ++++ components/brave_sync/features.cc | 3 ++ components/brave_sync/features.h | 1 + components/sync/service/BUILD.gn | 1 + .../sync/service/brave_sync_service_impl.cc | 8 +++- .../brave_sync_service_impl_unittest.cc | 45 ++++++++++++++++++- 6 files changed, 64 insertions(+), 3 deletions(-) diff --git a/browser/about_flags.cc b/browser/about_flags.cc index ec5befc05f5a..3e46c5dbe7ec 100644 --- a/browser/about_flags.cc +++ b/browser/about_flags.cc @@ -24,6 +24,7 @@ #include "brave/components/brave_rewards/common/buildflags/buildflags.h" #include "brave/components/brave_rewards/common/features.h" #include "brave/components/brave_shields/core/common/features.h" +#include "brave/components/brave_sync/features.h" #include "brave/components/brave_vpn/common/buildflags/buildflags.h" #include "brave/components/brave_wallet/common/features.h" #include "brave/components/de_amp/common/features.h" @@ -1003,6 +1004,14 @@ "corners, padding, and a drop shadow", \ kOsWin | kOsLinux | kOsMac, \ FEATURE_VALUE_TYPE(features::kBraveWebViewRoundedCorners), \ + }, \ + { \ + "brave-sync-default-passwords", \ + "Enable password syncing by default", \ + "Turn on password syncing when Sync is enabled.", \ + kOsAll, \ + FEATURE_VALUE_TYPE( \ + brave_sync::features::kBraveSyncDefaultPasswords), \ }) \ BRAVE_NATIVE_WALLET_FEATURE_ENTRIES \ BRAVE_NEWS_FEATURE_ENTRIES \ diff --git a/components/brave_sync/features.cc b/components/brave_sync/features.cc index 9d6418bc869d..bc10078d4fff 100644 --- a/components/brave_sync/features.cc +++ b/components/brave_sync/features.cc @@ -10,5 +10,8 @@ namespace brave_sync::features { BASE_FEATURE(kBraveSync, "BraveSync", base::FEATURE_ENABLED_BY_DEFAULT); +BASE_FEATURE(kBraveSyncDefaultPasswords, + "BraveSyncDefaultPasswords", + base::FEATURE_DISABLED_BY_DEFAULT); } // namespace brave_sync::features diff --git a/components/brave_sync/features.h b/components/brave_sync/features.h index 11db015866df..22509bb496d8 100644 --- a/components/brave_sync/features.h +++ b/components/brave_sync/features.h @@ -12,6 +12,7 @@ namespace brave_sync { namespace features { BASE_DECLARE_FEATURE(kBraveSync); +BASE_DECLARE_FEATURE(kBraveSyncDefaultPasswords); } // namespace features } // namespace brave_sync diff --git a/components/sync/service/BUILD.gn b/components/sync/service/BUILD.gn index ac9021fc8393..a82a4d4e79d4 100644 --- a/components/sync/service/BUILD.gn +++ b/components/sync/service/BUILD.gn @@ -19,6 +19,7 @@ source_set("unit_tests") { deps = [ "//base/test:test_support", "//brave/components/brave_sync:crypto", + "//brave/components/brave_sync:features", "//brave/components/brave_sync:network_time_helper", "//brave/components/brave_sync:p3a", "//brave/components/brave_sync:prefs", diff --git a/components/sync/service/brave_sync_service_impl.cc b/components/sync/service/brave_sync_service_impl.cc index 82fb6ff371cf..8b9ec28f7a9e 100644 --- a/components/sync/service/brave_sync_service_impl.cc +++ b/components/sync/service/brave_sync_service_impl.cc @@ -14,6 +14,7 @@ #include "base/strings/string_util.h" #include "brave/components/brave_sync/brave_sync_p3a.h" #include "brave/components/brave_sync/crypto/crypto.h" +#include "brave/components/brave_sync/features.h" #include "brave/components/sync/service/brave_sync_auth_manager.h" #include "brave/components/sync/service/sync_service_impl_delegate.h" #include "build/build_config.h" @@ -42,7 +43,6 @@ BraveSyncServiceImpl::BraveSyncServiceImpl( brave_sync::Prefs::GetSeedPath(), base::BindRepeating(&BraveSyncServiceImpl::OnBraveSyncPrefsChanged, base::Unretained(this))); - bool failed_to_decrypt = false; GetBraveSyncAuthManager()->DeriveSigningKeys( brave_sync_prefs_.GetSeed(&failed_to_decrypt)); @@ -165,7 +165,7 @@ void BraveSyncServiceImpl::OnBraveSyncPrefsChanged(const std::string& path) { if (!seed.empty()) { GetBraveSyncAuthManager()->DeriveSigningKeys(seed); - // Default enabled types: Bookmarks + // Default enabled types: Bookmarks, Passwords // Related Chromium change: 33441a0f3f9a591693157f2fd16852ce072e6f9d // We need to acquire setup handle before change selected types. @@ -175,6 +175,10 @@ void BraveSyncServiceImpl::OnBraveSyncPrefsChanged(const std::string& path) { syncer::UserSelectableTypeSet selected_types; selected_types.Put(UserSelectableType::kBookmarks); + if (base::FeatureList::IsEnabled( + brave_sync::features::kBraveSyncDefaultPasswords)) { + selected_types.Put(UserSelectableType::kPasswords); + } GetUserSettings()->SetSelectedTypes(false, selected_types); brave_sync_prefs_.ClearLeaveChainDetails(); diff --git a/components/sync/service/brave_sync_service_impl_unittest.cc b/components/sync/service/brave_sync_service_impl_unittest.cc index a2b29de761de..43a9605a71be 100644 --- a/components/sync/service/brave_sync_service_impl_unittest.cc +++ b/components/sync/service/brave_sync_service_impl_unittest.cc @@ -12,8 +12,10 @@ #include "base/memory/raw_ptr.h" #include "base/test/gtest_util.h" #include "base/test/metrics/histogram_tester.h" +#include "base/test/scoped_feature_list.h" #include "base/test/task_environment.h" #include "brave/components/brave_sync/brave_sync_p3a.h" +#include "brave/components/brave_sync/features.h" #include "brave/components/history/core/browser/sync/brave_history_data_type_controller.h" #include "brave/components/history/core/browser/sync/brave_history_delete_directives_data_type_controller.h" #include "brave/components/sync/service/sync_service_impl_delegate.h" @@ -23,6 +25,7 @@ #include "components/os_crypt/sync/os_crypt_mocker.h" #include "components/prefs/pref_change_registrar.h" #include "components/signin/public/identity_manager/accounts_in_cookie_jar_info.h" +#include "components/sync/base/data_type.h" #include "components/sync/engine/nigori/key_derivation_params.h" #include "components/sync/engine/nigori/nigori.h" #include "components/sync/model/type_entities_count.h" @@ -55,7 +58,7 @@ constexpr char kValidSyncCode[] = "exotic ill sure question trial squirrel glove celery " "awkward push jelly logic broccoli almost grocery drift"; -// Taken from anonimous namespace from sync_service_crypto_unittest.cc +// Taken from anonymous namespace from sync_service_crypto_unittest.cc sync_pb::EncryptedData MakeEncryptedData( const std::string& passphrase, const KeyDerivationParams& derivation_params) { @@ -118,6 +121,8 @@ class BraveSyncServiceImplTest : public testing::Test { std::unique_ptr sync_client = sync_service_impl_bundle_.CreateSyncClientMock(); + ON_CALL(*sync_client, IsPasswordSyncAllowed).WillByDefault(Return(true)); + auto sync_service_delegate(std::make_unique()); sync_service_delegate_ = sync_service_delegate.get(); @@ -712,6 +717,44 @@ TEST_F(BraveSyncServiceImplTest, NoLeaveDetailsWhenInitializeIOS) { EXPECT_FALSE(brave_sync_prefs()->GetLeaveChainDetails().empty()); } +class BraveSyncServiceImpl_EnableDefaultPasswordSyncingTest + : public BraveSyncServiceImplTest { + public: + BraveSyncServiceImpl_EnableDefaultPasswordSyncingTest() { + scoped_feature_list_.InitAndEnableFeature( + brave_sync::features::kBraveSyncDefaultPasswords); + } + + protected: + base::test::ScopedFeatureList scoped_feature_list_; +}; + +TEST_F(BraveSyncServiceImpl_EnableDefaultPasswordSyncingTest, + BookmarksAndPasswordsAfterSetup) { + OSCryptMocker::SetUp(); + CreateSyncService(DataTypeSet({BOOKMARKS, PASSWORDS})); + EXPECT_FALSE(engine()); + brave_sync_service_impl()->SetSyncCode(kValidSyncCode); + task_environment_.RunUntilIdle(); + + auto sync_blocker = brave_sync_service_impl()->GetSetupInProgressHandle(); + brave_sync_service_impl() + ->GetUserSettings() + ->SetInitialSyncFeatureSetupComplete( + syncer::SyncFirstSetupCompleteSource::ADVANCED_FLOW_CONFIRM); + EXPECT_TRUE(engine()); + + EXPECT_FALSE( + brave_sync_service_impl()->GetUserSettings()->IsSyncEverythingEnabled()); + auto selected_types = + brave_sync_service_impl()->GetUserSettings()->GetSelectedTypes(); + EXPECT_EQ(selected_types.size(), 2u); + EXPECT_TRUE(selected_types.Has(UserSelectableType::kBookmarks)); + EXPECT_TRUE(selected_types.Has(UserSelectableType::kPasswords)); + + OSCryptMocker::TearDown(); +} + namespace { enum class GACookiesMethodType { kOnAccountsCookieDeletedByUserAction, From bfe2094ccb8a7172c9015c8c69ccaa9743932dda Mon Sep 17 00:00:00 2001 From: Shivan Kaul Sahib Date: Fri, 17 Jan 2025 01:42:15 -0800 Subject: [PATCH 2/2] Fix RunUntilIdle warning --- .../sync/service/brave_sync_service_impl_unittest.cc | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/components/sync/service/brave_sync_service_impl_unittest.cc b/components/sync/service/brave_sync_service_impl_unittest.cc index 43a9605a71be..75a83c475c63 100644 --- a/components/sync/service/brave_sync_service_impl_unittest.cc +++ b/components/sync/service/brave_sync_service_impl_unittest.cc @@ -101,7 +101,9 @@ class SyncServiceObserverMock : public SyncServiceObserver { class BraveSyncServiceImplTest : public testing::Test { public: BraveSyncServiceImplTest() - : brave_sync_prefs_(sync_service_impl_bundle_.pref_service()), + : task_environment_( + base::test::SingleThreadTaskEnvironment::TimeSource::MOCK_TIME), + brave_sync_prefs_(sync_service_impl_bundle_.pref_service()), sync_prefs_(sync_service_impl_bundle_.pref_service()) { sync_service_impl_bundle_.identity_test_env() ->SetAutomaticIssueOfAccessTokens(true); @@ -735,8 +737,8 @@ TEST_F(BraveSyncServiceImpl_EnableDefaultPasswordSyncingTest, CreateSyncService(DataTypeSet({BOOKMARKS, PASSWORDS})); EXPECT_FALSE(engine()); brave_sync_service_impl()->SetSyncCode(kValidSyncCode); - task_environment_.RunUntilIdle(); - + task_environment_.FastForwardUntilNoTasksRemain(); + auto sync_blocker = brave_sync_service_impl()->GetSetupInProgressHandle(); brave_sync_service_impl() ->GetUserSettings()