From 7a4d18c713f248fc466e9ab182e0f27432b01da5 Mon Sep 17 00:00:00 2001 From: AlexeyBarabash Date: Thu, 14 Nov 2019 00:02:16 +0200 Subject: [PATCH 1/5] Redo Brave sync prefs Allow one device to be in chain. Remove unnecessary states. --- .../brave_profile_sync_service_impl.cc | 153 +++++------------- .../brave_profile_sync_service_impl.h | 29 ++-- 2 files changed, 53 insertions(+), 129 deletions(-) diff --git a/components/brave_sync/brave_profile_sync_service_impl.cc b/components/brave_sync/brave_profile_sync_service_impl.cc index fa58e21fc406..77699ed39c82 100644 --- a/components/brave_sync/brave_profile_sync_service_impl.cc +++ b/components/brave_sync/brave_profile_sync_service_impl.cc @@ -30,7 +30,6 @@ #include "components/bookmarks/browser/bookmark_model.h" #include "components/signin/public/identity_manager/account_info.h" #include "components/sync/engine_impl/syncer.h" -#include "content/public/browser/browser_thread.h" #include "net/base/network_interfaces.h" #include "ui/base/models/tree_node_iterator.h" @@ -78,22 +77,21 @@ std::string GetDeviceName() { return hostname; } -RecordsListPtr CreateDeviceCreationRecordExtension( - const std::string& deviceName, - const std::string& objectId, - const SyncRecord::Action& action, - const std::string& deviceId) { +RecordsListPtr CreateDeviceRecord(const std::string& device_name, + const std::string& object_id, + const SyncRecord::Action& action, + const std::string& device_id) { RecordsListPtr records = std::make_unique(); SyncRecordPtr record = std::make_unique(); record->action = action; - record->deviceId = deviceId; - record->objectId = objectId; + record->deviceId = device_id; + record->objectId = object_id; record->objectData = SyncObjectData_DEVICE; // "device" std::unique_ptr device = std::make_unique(); - device->name = deviceName; + device->name = device_name; record->SetDevice(std::move(device)); records->emplace_back(std::move(record)); @@ -167,11 +165,10 @@ BraveProfileSyncServiceImpl::BraveProfileSyncServiceImpl(Profile* profile, InitParams init_params) : BraveProfileSyncService(std::move(init_params)), brave_sync_client_(BraveSyncClient::Create(this, profile)) { - brave_sync_words_ = std::string(); brave_sync_prefs_ = std::make_unique(sync_client_->GetPrefService()); - // Moniter syncs prefs required in GetSettingsAndDevices + // Monitor syncs prefs required in GetSettingsAndDevices brave_pref_change_registrar_.Init(sync_client_->GetPrefService()); brave_pref_change_registrar_.Add( prefs::kSyncEnabled, @@ -200,30 +197,18 @@ BraveProfileSyncServiceImpl::BraveProfileSyncServiceImpl(Profile* profile, model_ = BookmarkModelFactory::GetForBrowserContext(profile); - if (!brave_sync_prefs_->GetSeed().empty() && - !brave_sync_prefs_->GetThisDeviceName().empty()) { - brave_sync_configured_ = true; - } network_connection_tracker_->AddNetworkConnectionObserver(this); RecordSyncStateP3A(); } void BraveProfileSyncServiceImpl::OnNudgeSyncCycle(RecordsListPtr records) { - if (!IsBraveSyncEnabled()) + if (!brave_sync_prefs_->GetSyncEnabled()) return; for (auto& record : *records) { record->deviceId = brave_sync_prefs_->GetThisDeviceId(); } if (!records->empty()) { - if (((!brave_sync::tools::IsTimeEmpty(chain_created_time_) && - (base::Time::Now() - chain_created_time_).InSeconds() < 30u) || - brave_sync_prefs_->GetSyncDevices()->size() < 2)) { - // Store records for now - pending_send_records_.push_back(std::move(records)); - return; - } - SendAndPurgePendingRecords(); SendSyncRecords(jslib_const::SyncRecordType_BOOKMARKS, std::move(records)); } } @@ -241,17 +226,22 @@ void BraveProfileSyncServiceImpl::OnSetupSyncHaveCode( return; } + Uint8Array seed; + if (!crypto::PassphraseToBytes32(sync_words, &seed)) { + OnSyncSetupError("ERR_SYNC_WRONG_WORDS"); + return; + } + if (brave_sync_initializing_) { NotifyLogMessage("currently initializing"); return; } - if (IsBraveSyncConfigured()) { + if (!brave_sync_prefs_->GetSeed().empty()) { NotifyLogMessage("already configured"); return; } - ForceCompleteReset(); DCHECK(!brave_sync_prefs_->GetSyncEnabled()); if (device_name.empty()) @@ -259,9 +249,8 @@ void BraveProfileSyncServiceImpl::OnSetupSyncHaveCode( else brave_sync_prefs_->SetThisDeviceName(device_name); brave_sync_initializing_ = true; - brave_sync_prefs_->SetSyncEnabled(true); - brave_sync_words_ = sync_words; + seed_ = seed; } void BraveProfileSyncServiceImpl::OnSetupSyncNewToSync( @@ -273,18 +262,13 @@ void BraveProfileSyncServiceImpl::OnSetupSyncNewToSync( return; } - if (IsBraveSyncConfigured()) { + if (!brave_sync_prefs_->GetSeed().empty()) { NotifyLogMessage("already configured"); return; } - ForceCompleteReset(); DCHECK(!brave_sync_prefs_->GetSyncEnabled()); - // If the previous attempt was connect to sync chain - // and failed to receive save-init-data - brave_sync_words_.clear(); - if (device_name.empty()) brave_sync_prefs_->SetThisDeviceName(GetDeviceName()); else @@ -305,6 +289,7 @@ void BraveProfileSyncServiceImpl::OnDeleteDevice(const std::string& device_id) { const std::string object_id = device->object_id_; SendDeviceSyncRecord(SyncRecord::Action::A_DELETE, device_name, device_id, object_id); + FetchDevices(); } } @@ -321,7 +306,6 @@ void BraveProfileSyncServiceImpl::OnResetSync() { // we can reset it by ResetSyncInternal() const std::string device_id = brave_sync_prefs_->GetThisDeviceId(); OnDeleteDevice(device_id); - reseting_ = true; } } @@ -394,12 +378,8 @@ void BraveProfileSyncServiceImpl::OnGetInitData( DCHECK_CURRENTLY_ON(content::BrowserThread::UI); Uint8Array seed; - if (!brave_sync_words_.empty()) { - VLOG(1) << "[Brave Sync] Init from sync words"; - if (!crypto::PassphraseToBytes32(brave_sync_words_, &seed)) { - OnSyncSetupError("ERR_SYNC_WRONG_WORDS"); - return; - } + if (!seed_.empty()) { + seed = seed_; } else if (!brave_sync_prefs_->GetSeed().empty()) { seed = Uint8ArrayFromString(brave_sync_prefs_->GetSeed()); VLOG(1) << "[Brave Sync] Init from prefs"; @@ -432,7 +412,7 @@ void BraveProfileSyncServiceImpl::OnGetInitData( void BraveProfileSyncServiceImpl::OnSaveInitData(const Uint8Array& seed, const Uint8Array& device_id) { DCHECK_CURRENTLY_ON(content::BrowserThread::UI); - DCHECK(!brave_sync_initialized_); + DCHECK(!brave_sync_ready_); // If we are here and brave_sync_initializing_ is false, we have came // not from OnSetupSyncNewToSync or OnSetupSyncHaveCode. // One case is we put wrong code words and then restarted before cleared @@ -444,27 +424,18 @@ void BraveProfileSyncServiceImpl::OnSaveInitData(const Uint8Array& seed, std::string prev_seed_str = brave_sync_prefs_->GetPrevSeed(); - brave_sync_words_.clear(); + seed_.clear(); DCHECK(!seed_str.empty()); if (prev_seed_str == seed_str) { // reconnecting to previous sync chain brave_sync_prefs_->SetPrevSeed(std::string()); } else if (!prev_seed_str.empty()) { // connect/create to new sync chain - // bookmark_change_processor_->Reset(true); brave_sync_prefs_->SetPrevSeed(std::string()); - } else { - // This is not required, because when there is no previous seed, bookmarks - // should not have a metadata. However, this is done by intention, to be - // a remedy for cases when sync had been reset and prev_seed_str had been - // cleared when it shouldn't (brave-browser#3188). - // bookmark_change_processor_->Reset(true); } brave_sync_prefs_->SetSeed(seed_str); brave_sync_prefs_->SetThisDeviceId(device_id_str); - brave_sync_configured_ = true; - brave_sync_initializing_ = false; } @@ -480,8 +451,8 @@ void BraveProfileSyncServiceImpl::OnSyncReady() { return; } - DCHECK(false == brave_sync_initialized_); - brave_sync_initialized_ = true; + DCHECK(false == brave_sync_ready_); + brave_sync_ready_ = true; // For launching from legacy sync profile and also brand new profile if (brave_sync_prefs_->GetMigratedBookmarksVersion() < 2) @@ -676,27 +647,16 @@ void BraveProfileSyncServiceImpl::NotifyHaveSyncWords( void BraveProfileSyncServiceImpl::ResetSyncInternal() { SignalWaitableEvent(); brave_sync_prefs_->SetPrevSeed(brave_sync_prefs_->GetSeed()); - brave_sync_prefs_->Clear(); - brave_sync_configured_ = false; - brave_sync_initialized_ = false; + brave_sync_ready_ = false; - brave_sync_prefs_->SetSyncEnabled(false); ProfileSyncService::GetUserSettings()->SetSyncRequested(false); // brave sync doesn't support pause sync so treating every new sync chain as // first time setup syncer::SyncPrefs sync_prefs(sync_client_->GetPrefService()); sync_prefs.SetLastSyncedTime(base::Time()); - - reseting_ = false; -} - -void BraveProfileSyncServiceImpl::ForceCompleteReset() { - if (reseting_) { - ResetSyncInternal(); - } } void BraveProfileSyncServiceImpl::SetPermanentNodesOrder( @@ -881,7 +841,7 @@ void BraveProfileSyncServiceImpl::SendDeviceSyncRecord( const std::string& device_id, const std::string& object_id) { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); - RecordsListPtr records = CreateDeviceCreationRecordExtension( + RecordsListPtr records = CreateDeviceRecord( device_name, object_id, static_cast(action), device_id); SendSyncRecords(SyncRecordType_PREFERENCES, std::move(records)); @@ -891,10 +851,8 @@ void BraveProfileSyncServiceImpl::OnResolvedPreferences( const RecordsList& records) { const std::string this_device_id = brave_sync_prefs_->GetThisDeviceId(); bool this_device_deleted = false; - bool contains_only_one_device = false; auto sync_devices = brave_sync_prefs_->GetSyncDevices(); - auto old_devices_size = sync_devices->size(); for (const auto& record : records) { DCHECK(record->has_device() || record->has_sitesetting()); if (record->has_device()) { @@ -907,30 +865,13 @@ void BraveProfileSyncServiceImpl::OnResolvedPreferences( this_device_deleted || (record->deviceId == this_device_id && record->action == SyncRecord::Action::A_DELETE && actually_merged); - contains_only_one_device = - sync_devices->size() < 2 && - record->action == SyncRecord::Action::A_DELETE && actually_merged; } } // for each device brave_sync_prefs_->SetSyncDevices(*sync_devices); - if (old_devices_size < 2 && sync_devices->size() >= 2) { - // Save chain creation time to send bookmarks 30 sec after - chain_created_time_ = base::Time::Now(); - } - if (!tools::IsTimeEmpty(chain_created_time_) && - (base::Time::Now() - chain_created_time_).InSeconds() > 30u) { - SendAndPurgePendingRecords(); - } - if (this_device_deleted) { ResetSyncInternal(); - } else if (contains_only_one_device) { - // We see amount of devices had been decreased to 1 and it is not this - // device had been deleted. So call OnResetSync which will send DELETE - // record for this device - OnResetSync(); } } @@ -956,31 +897,19 @@ bool BraveProfileSyncServiceImpl::IsBraveSyncEnabled() const { return brave_sync_prefs_->GetSyncEnabled(); } -bool BraveProfileSyncServiceImpl::IsBraveSyncInitialized() const { - return brave_sync_initialized_; -} - -bool BraveProfileSyncServiceImpl::IsBraveSyncConfigured() const { - return brave_sync_configured_ && - // When there is 0 or 1 device, it means chain is not completely - // created, so we should give a chance to make force reset in - // |OnSetupSyncHaveCode| or in |OnSetupSyncNewToSync| - (brave_sync_prefs_->GetSyncDevices()->size() >= 2); -} void BraveProfileSyncServiceImpl::FetchDevices() { DCHECK(sync_client_); brave_sync_prefs_->SetLastFetchTime(base::Time::Now()); - // When the chain is not fully created or our device preferences queue is - // not available, it may happened we will not find required device sync - // record when we will be switched to use SQS instead of S3 in sync lib. + + // When we create the device, we also create a set of SQS queues + // For other devices' S3 put lambda hook, some amount of time is required + // to see our queues in the result of listQueues // Default Chromium fetch interval is 60 sec. // So during 70 sec after the chain creation forcing use of S3 // by set start_at_time to 0. - base::Time start_at_time; - if (brave_sync_prefs_->GetSyncDevices()->size() <= 1 || - (!tools::IsTimeEmpty(chain_created_time_) && - (base::Time::Now() - chain_created_time_).InSeconds() < 70u)) { + if (!tools::IsTimeEmpty(this_device_created_time_) && + (base::Time::Now() - this_device_created_time_).InSeconds() < 70u) { start_at_time = base::Time(); } else { start_at_time = brave_sync_prefs_->GetLatestDeviceRecordTime(); @@ -991,15 +920,17 @@ void BraveProfileSyncServiceImpl::FetchDevices() { } void BraveProfileSyncServiceImpl::OnPollSyncCycle(GetRecordsCallback cb, base::WaitableEvent* wevent) { - if (!IsBraveSyncEnabled()) + if (!brave_sync_prefs_->GetSyncEnabled()) return; - if (IsTimeEmpty(brave_sync_prefs_->GetLastFetchTime())) + if (IsTimeEmpty(brave_sync_prefs_->GetLastFetchTime())) { SendCreateDevice(); + this_device_created_time_ = base::Time::Now(); + } FetchDevices(); - if (!brave_sync_initialized_) { + if (!brave_sync_ready_) { wevent->Signal(); return; } @@ -1027,14 +958,6 @@ BraveSyncService* BraveProfileSyncServiceImpl::GetSyncService() const { const_cast(this)); } -void BraveProfileSyncServiceImpl::SendAndPurgePendingRecords() { - for (auto& records_to_send : pending_send_records_) { - SendSyncRecords(jslib_const::SyncRecordType_BOOKMARKS, - std::move(records_to_send)); - } - pending_send_records_.clear(); -} - void BraveProfileSyncServiceImpl::SendSyncRecords( const std::string& category_name, RecordsListPtr records) { diff --git a/components/brave_sync/brave_profile_sync_service_impl.h b/components/brave_sync/brave_profile_sync_service_impl.h index fac3bae918fb..99e3b370b646 100644 --- a/components/brave_sync/brave_profile_sync_service_impl.h +++ b/components/brave_sync/brave_profile_sync_service_impl.h @@ -41,6 +41,10 @@ FORWARD_DECLARE_TEST(BraveSyncServiceTest, FORWARD_DECLARE_TEST(BraveSyncServiceTest, ExponentialResend); FORWARD_DECLARE_TEST(BraveSyncServiceTest, GetDevicesWithFetchSyncRecords); FORWARD_DECLARE_TEST(BraveSyncServiceTest, SendCompact); +FORWARD_DECLARE_TEST(BraveSyncServiceTest, SetSyncEnabled); +FORWARD_DECLARE_TEST(BraveSyncServiceTest, SetSyncDisabled); +FORWARD_DECLARE_TEST(BraveSyncServiceTest, IsSyncReadyOnNewProfile); +FORWARD_DECLARE_TEST(BraveSyncServiceTest, SetThisDeviceCreatedTime); class BraveSyncServiceTest; @@ -116,8 +120,6 @@ class BraveProfileSyncServiceImpl #endif bool IsBraveSyncEnabled() const override; - bool IsBraveSyncInitialized() const; - bool IsBraveSyncConfigured() const; syncer::ModelTypeSet GetPreferredDataTypes() const override; @@ -152,6 +154,11 @@ class BraveProfileSyncServiceImpl FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, GetDevicesWithFetchSyncRecords); FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, SendCompact); + FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, SetSyncEnabled); + FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, SetSyncDisabled); + FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, IsSyncReadyOnNewProfile); + FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, SetThisDeviceCreatedTime); + friend class ::BraveSyncServiceTest; void SignalWaitableEvent(); @@ -172,8 +179,6 @@ class BraveProfileSyncServiceImpl void NotifyHaveSyncWords(const std::string& sync_words); void ResetSyncInternal(); - void ForceCompleteReset(); - bool GetResettingForTest() const { return reseting_; } void SetPermanentNodesOrder(const std::string& base_order); @@ -190,8 +195,6 @@ class BraveProfileSyncServiceImpl std::unique_ptr PrepareResolvedPreferences( const RecordsList& records); - void SendAndPurgePendingRecords(); - void SendSyncRecords(const std::string& category_name, RecordsListPtr records); void ResendSyncRecords(const std::string& category_name); @@ -208,19 +211,17 @@ class BraveProfileSyncServiceImpl } std::unique_ptr brave_sync_prefs_; - // True when is in active sync chain - bool brave_sync_configured_ = false; // True if we have received SyncReady from JS lib - bool brave_sync_initialized_ = false; + // This is used only to prevent out of sequence invocation of OnSaveInitData + // and prevent double invocation of OnSyncReady + bool brave_sync_ready_ = false; // Prevent two sequential calls OnSetupSyncHaveCode or OnSetupSyncNewToSync // while being initializing bool brave_sync_initializing_ = false; - bool reseting_ = false; - - std::string brave_sync_words_; + Uint8Array seed_; brave_sync::GetRecordsCallback get_record_cb_; base::WaitableEvent* wevent_ = nullptr; @@ -232,9 +233,9 @@ class BraveProfileSyncServiceImpl std::unique_ptr brave_sync_client_; - base::Time chain_created_time_; - std::vector pending_send_records_; std::unique_ptr pending_received_records_; + // Time when current device sent CREATE device record + base::Time this_device_created_time_; // Used to ensure that certain operations are performed on the sequence that // this object was created on. From a3708ba372686cb1b99a26f39df2b9d48bef6920 Mon Sep 17 00:00:00 2001 From: AlexeyBarabash Date: Thu, 14 Nov 2019 20:06:54 +0200 Subject: [PATCH 2/5] Workaround for empty passphrase - will be reverted after UI PR --- components/brave_sync/brave_profile_sync_service_impl.cc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/components/brave_sync/brave_profile_sync_service_impl.cc b/components/brave_sync/brave_profile_sync_service_impl.cc index 77699ed39c82..14ff140a4fce 100644 --- a/components/brave_sync/brave_profile_sync_service_impl.cc +++ b/components/brave_sync/brave_profile_sync_service_impl.cc @@ -320,7 +320,11 @@ void BraveProfileSyncServiceImpl::GetSettingsAndDevices( void BraveProfileSyncServiceImpl::GetSyncWords() { DCHECK_CURRENTLY_ON(content::BrowserThread::UI); Uint8Array seed = Uint8ArrayFromString(brave_sync_prefs_->GetSeed()); - NotifyHaveSyncWords(crypto::PassphraseFromBytes32(seed)); + if (!seed.empty()) { + NotifyHaveSyncWords(crypto::PassphraseFromBytes32(seed)); + } else { + NotifyHaveSyncWords("TEST DEBUG"); + } } std::string BraveProfileSyncServiceImpl::GetSeed() { From d4b5b8bfd5e14dc6b49da44b1af4396440244de2 Mon Sep 17 00:00:00 2001 From: AlexeyBarabash Date: Thu, 14 Nov 2019 00:04:01 +0200 Subject: [PATCH 3/5] Updated testcases for redone Brave sync prefs --- .../brave_sync/brave_sync_service_unittest.cc | 225 +++++++++--------- 1 file changed, 119 insertions(+), 106 deletions(-) diff --git a/components/brave_sync/brave_sync_service_unittest.cc b/components/brave_sync/brave_sync_service_unittest.cc index 54ba98b5149d..4401bad51923 100644 --- a/components/brave_sync/brave_sync_service_unittest.cc +++ b/components/brave_sync/brave_sync_service_unittest.cc @@ -229,7 +229,6 @@ class BraveSyncServiceTest : public testing::Test { // When this is set, class should not install any other MessageLoops, like // base::test::ScopedTaskEnvironment content::TestBrowserThreadBundle thread_bundle_; - std::unique_ptr profile_; BraveProfileSyncServiceImpl* sync_service_; MockBraveSyncClient* sync_client_; @@ -248,8 +247,7 @@ TEST_F(BraveSyncServiceTest, SetSyncEnabled) { sync_service()->OnSetSyncEnabled(true); EXPECT_TRUE( profile()->GetPrefs()->GetBoolean(brave_sync::prefs::kSyncEnabled)); - EXPECT_FALSE(sync_service()->IsBraveSyncInitialized()); - EXPECT_FALSE(sync_service()->IsBraveSyncConfigured()); + EXPECT_FALSE(sync_service()->brave_sync_ready_); } TEST_F(BraveSyncServiceTest, SetSyncDisabled) { @@ -264,32 +262,54 @@ TEST_F(BraveSyncServiceTest, SetSyncDisabled) { sync_service()->OnSetSyncEnabled(false); EXPECT_FALSE( profile()->GetPrefs()->GetBoolean(brave_sync::prefs::kSyncEnabled)); - EXPECT_FALSE(sync_service()->IsBraveSyncInitialized()); - EXPECT_FALSE(sync_service()->IsBraveSyncConfigured()); + EXPECT_FALSE(sync_service()->brave_sync_ready_); } -TEST_F(BraveSyncServiceTest, IsSyncConfiguredOnNewProfile) { - EXPECT_FALSE(sync_service()->IsBraveSyncConfigured()); +TEST_F(BraveSyncServiceTest, IsSyncReadyOnNewProfile) { + EXPECT_FALSE(sync_service()->brave_sync_ready_); } -TEST_F(BraveSyncServiceTest, IsSyncInitializedOnNewProfile) { - EXPECT_FALSE(sync_service()->IsBraveSyncInitialized()); -} +const char kTestWords1[] = + "absurd avoid scissors anxiety gather lottery category door " + "army half long cage bachelor another expect people blade " + "school educate curtain scrub monitor lady beyond"; TEST_F(BraveSyncServiceTest, OnSetupSyncHaveCode) { EXPECT_CALL(*sync_client(), OnSyncEnabledChanged); // Expecting sync state changed twice: for enabled state and for device name EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(2); - sync_service()->OnSetupSyncHaveCode("word1 word2 word3", "test_device"); + sync_service()->OnSetupSyncHaveCode(kTestWords1, "test_device"); EXPECT_TRUE( profile()->GetPrefs()->GetBoolean(brave_sync::prefs::kSyncEnabled)); } +TEST_F(BraveSyncServiceTest, OnSetupSyncHaveCodeIncorrectCode) { + EXPECT_CALL(*sync_client(), OnSyncEnabledChanged).Times(0); + EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(0); + EXPECT_CALL(*observer(), + OnSyncSetupError(sync_service(), "ERR_SYNC_WRONG_WORDS")) + .Times(1); + sync_service()->OnSetupSyncHaveCode("wrong code", "test_device"); + EXPECT_FALSE( + profile()->GetPrefs()->GetBoolean(brave_sync::prefs::kSyncEnabled)); +} + +TEST_F(BraveSyncServiceTest, OnSetupSyncHaveCodeEmptyCode) { + EXPECT_CALL(*sync_client(), OnSyncEnabledChanged).Times(0); + EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(0); + EXPECT_CALL(*observer(), + OnSyncSetupError(sync_service(), "ERR_SYNC_WRONG_WORDS")) + .Times(1); + sync_service()->OnSetupSyncHaveCode("", "test_device"); + EXPECT_FALSE( + profile()->GetPrefs()->GetBoolean(brave_sync::prefs::kSyncEnabled)); +} + TEST_F(BraveSyncServiceTest, OnSetupSyncHaveCodeEmptyDeviceName) { EXPECT_CALL(*sync_client(), OnSyncEnabledChanged); // Expecting sync state changed twice: for enabled state and for device name EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(2); - sync_service()->OnSetupSyncHaveCode("word1 word2 word3", ""); + sync_service()->OnSetupSyncHaveCode(kTestWords1, ""); EXPECT_TRUE( profile()->GetPrefs()->GetBoolean(brave_sync::prefs::kSyncEnabled)); EXPECT_EQ( @@ -408,6 +428,7 @@ TEST_F(BraveSyncServiceTest, OnDeleteDevice) { ContainsDeviceRecord(SyncRecord::Action::A_DELETE, "device3"))) .Times(1); + EXPECT_CALL(*sync_client(), SendFetchSyncRecords(_, _, _)); sync_service()->OnDeleteDevice("3"); RecordsList resolved_records; @@ -446,6 +467,7 @@ TEST_F(BraveSyncServiceTest, OnDeleteDeviceWhenOneDevice) { EXPECT_TRUE(DevicesContains(devices.get(), "2", "device2")); EXPECT_CALL(*sync_client(), SendSyncRecords).Times(1); + EXPECT_CALL(*sync_client(), SendFetchSyncRecords(_, _, _)); sync_service()->OnDeleteDevice("2"); RecordsList resolved_records; @@ -455,34 +477,24 @@ TEST_F(BraveSyncServiceTest, OnDeleteDeviceWhenOneDevice) { // Expecting to be called one time to set the new devices list EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(1); - EXPECT_CALL(*sync_client(), SendSyncRecords).Times(1); + EXPECT_CALL(*sync_client(), SendSyncRecords).Times(0); + + // Still enabled + EXPECT_CALL(*sync_client(), OnSyncEnabledChanged).Times(0); sync_service()->OnResolvedPreferences(resolved_records); auto devices_semi_final = brave_sync_prefs()->GetSyncDevices(); EXPECT_FALSE(DevicesContains(devices_semi_final.get(), "2", "device2")); EXPECT_TRUE(DevicesContains(devices_semi_final.get(), "1", "device1")); - - // Enabled->Disabled - EXPECT_CALL(*sync_client(), OnSyncEnabledChanged).Times(1); - - // Emulate sending DELETE for this device - RecordsList resolved_records2; - auto resolved_record2 = SyncRecord::Clone(*records.at(0)); - resolved_record2->action = SyncRecord::Action::A_DELETE; - resolved_records2.push_back(std::move(resolved_record2)); - EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(3); - - sync_service()->OnResolvedPreferences(resolved_records2); - - auto devices_final = brave_sync_prefs()->GetSyncDevices(); - EXPECT_FALSE(DevicesContains(devices_final.get(), "1", "device1")); - EXPECT_FALSE(DevicesContains(devices_final.get(), "2", "device2")); - EXPECT_FALSE(sync_service()->IsBraveSyncConfigured()); } -TEST_F(BraveSyncServiceTest, OnDeleteDeviceWhenSelfDeleted) { +TEST_F(BraveSyncServiceTest, + OnDeleteDeviceWhenSelfDeleted) { brave_sync_prefs()->SetThisDeviceId("1"); + EXPECT_CALL(*sync_client(), OnSyncEnabledChanged).Times(1); + EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(1); + brave_sync_prefs()->SetSyncEnabled(true); RecordsList records; records.push_back( SimpleDeviceRecord(SyncRecord::Action::A_CREATE, "1", "device1")); @@ -501,11 +513,11 @@ TEST_F(BraveSyncServiceTest, OnDeleteDeviceWhenSelfDeleted) { ContainsDeviceRecord(SyncRecord::Action::A_DELETE, "device1"))) .Times(1); + EXPECT_CALL(*sync_client(), SendFetchSyncRecords(_, _, _)); sync_service()->OnDeleteDevice("1"); // Enabled->Disabled EXPECT_CALL(*sync_client(), OnSyncEnabledChanged).Times(1); - RecordsList resolved_records; auto resolved_record = SyncRecord::Clone(*records.at(0)); resolved_record->action = SyncRecord::Action::A_DELETE; @@ -514,11 +526,11 @@ TEST_F(BraveSyncServiceTest, OnDeleteDeviceWhenSelfDeleted) { EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(3); sync_service()->OnResolvedPreferences(resolved_records); + EXPECT_FALSE(brave_sync_prefs()->GetSyncEnabled()); + auto devices_final = brave_sync_prefs()->GetSyncDevices(); EXPECT_FALSE(DevicesContains(devices_final.get(), "1", "device1")); EXPECT_FALSE(DevicesContains(devices_final.get(), "2", "device2")); - - EXPECT_FALSE(sync_service()->IsBraveSyncConfigured()); } TEST_F(BraveSyncServiceTest, OnResetSync) { @@ -549,11 +561,13 @@ TEST_F(BraveSyncServiceTest, OnResetSync) { "this_device"))) .Times(1); + EXPECT_CALL(*sync_client(), SendFetchSyncRecords); sync_service()->OnResetSync(); RecordsList resolved_records; auto resolved_record = SyncRecord::Clone(*records.at(0)); resolved_record->action = SyncRecord::Action::A_DELETE; resolved_records.push_back(std::move(resolved_record)); + sync_service()->OnResolvedPreferences(resolved_records); auto devices_final = brave_sync_prefs()->GetSyncDevices(); @@ -598,8 +612,7 @@ TEST_F(BraveSyncServiceTest, OnResetSync) { profile()->GetPrefs()->GetString(brave_sync::prefs::kSyncApiVersion), "0"); - EXPECT_FALSE(sync_service()->IsBraveSyncInitialized()); - EXPECT_FALSE(sync_service()->IsBraveSyncConfigured()); + EXPECT_FALSE(sync_service()->brave_sync_ready_); EXPECT_FALSE(sync_prefs()->IsSyncRequested()); EXPECT_FALSE( @@ -680,25 +693,27 @@ TEST_F(BraveSyncServiceTest, OnBraveSyncPrefsChanged) { sync_service()->OnBraveSyncPrefsChanged(brave_sync::prefs::kSyncEnabled); } -TEST_F(BraveSyncServiceTest, StartSyncNonDeviceRecords) { - EXPECT_FALSE(sync_service()->IsBraveSyncInitialized()); - sync_service()->OnSetSyncEnabled(true); - profile()->GetPrefs()->SetString(brave_sync::prefs::kSyncBookmarksBaseOrder, - "1.1."); - profile()->GetPrefs()->SetBoolean(brave_sync::prefs::kSyncBookmarksEnabled, - true); - EXPECT_FALSE( - profile()->GetPrefs()->GetBoolean(syncer::prefs::kSyncBookmarks)); +void OnGetRecordsStub(std::unique_ptr records) { +} + +TEST_F(BraveSyncServiceTest, SetThisDeviceCreatedTime) { + EXPECT_TRUE(brave_sync::tools::IsTimeEmpty( + sync_service()->this_device_created_time_)); + + EXPECT_CALL(*sync_client(), OnSyncEnabledChanged).Times(1); + EXPECT_CALL(*observer(), OnSyncStateChanged); + brave_sync_prefs()->SetSyncEnabled(true); brave_sync_prefs()->SetThisDeviceId("1"); - RecordsList records; - records.push_back( - SimpleDeviceRecord(SyncRecord::Action::A_CREATE, "1", "device1")); - records.push_back( - SimpleDeviceRecord(SyncRecord::Action::A_CREATE, "2", "device2")); - EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(1); - sync_service()->OnResolvedPreferences(records); - EXPECT_TRUE( - !brave_sync::tools::IsTimeEmpty(sync_service()->chain_created_time_)); + + brave_sync::GetRecordsCallback on_get_records = + base::BindOnce(&OnGetRecordsStub); + base::WaitableEvent we; + EXPECT_CALL(*sync_client(), SendSyncRecords("PREFERENCES", _)); + EXPECT_CALL(*sync_client(), SendFetchSyncRecords); + sync_service()->OnPollSyncCycle(std::move(on_get_records), &we); + + EXPECT_FALSE(brave_sync::tools::IsTimeEmpty( + sync_service()->this_device_created_time_)); } TEST_F(BraveSyncServiceTest, OnSyncReadyNewToSync) { @@ -737,6 +752,8 @@ TEST_F(BraveSyncServiceTest, GetDisableReasons) { EXPECT_EQ(sync_service()->GetDisableReasons(), syncer::SyncService::DISABLE_REASON_ENTERPRISE_POLICY | syncer::SyncService::DISABLE_REASON_USER_CHOICE); + EXPECT_CALL(*sync_client(), OnSyncEnabledChanged).Times(1); + EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(1); sync_service()->OnSetSyncEnabled(true); EXPECT_EQ(sync_service()->GetDisableReasons(), syncer::SyncService::DISABLE_REASON_ENTERPRISE_POLICY | @@ -744,17 +761,18 @@ TEST_F(BraveSyncServiceTest, GetDisableReasons) { brave_sync_prefs()->SetMigratedBookmarksVersion(2); EXPECT_EQ(sync_service()->GetDisableReasons(), syncer::SyncService::DISABLE_REASON_NONE); + EXPECT_CALL(*sync_client(), OnSyncEnabledChanged).Times(1); + EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(1); sync_service()->OnSetSyncEnabled(false); EXPECT_TRUE(sync_service()->HasDisableReason( syncer::SyncService::DISABLE_REASON_ENTERPRISE_POLICY)); } TEST_F(BraveSyncServiceTest, OnSetupSyncHaveCode_Reset_SetupAgain) { - EXPECT_FALSE(sync_service()->GetResettingForTest()); EXPECT_CALL(*sync_client(), OnSyncEnabledChanged).Times(1); // Expecting sync state changed twice: for enabled state and for device name EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(2); - sync_service()->OnSetupSyncHaveCode("word1 word2 word3", "test_device"); + sync_service()->OnSetupSyncHaveCode(kTestWords1, "test_device"); EXPECT_TRUE( profile()->GetPrefs()->GetBoolean(brave_sync::prefs::kSyncEnabled)); @@ -778,37 +796,34 @@ TEST_F(BraveSyncServiceTest, OnSetupSyncHaveCode_Reset_SetupAgain) { ContainsDeviceRecord(SyncRecord::Action::A_DELETE, "this_device"))) .Times(1); + EXPECT_CALL(*sync_client(), SendFetchSyncRecords); sync_service()->OnResetSync(); - // Here we have marked service as in resetting state + // Actual kSyncEnabled will go to false after receiving confirmation of // this_device DELETE EXPECT_TRUE( profile()->GetPrefs()->GetBoolean(brave_sync::prefs::kSyncEnabled)); - EXPECT_TRUE(sync_service()->GetResettingForTest()); - - // OnSyncStateChanged actually is invoked 9 times: - // ForceCompleteReset - // ResetSyncInternal - // brave_sync::Prefs::Clear() - // device_name - // enabled => false - // bookmarks_enabled - // site_settings_enabled - // history_enabled - // device_list - // enabled => false - // OnSetupSyncHaveCode() - // device_name - // enabled => true - - // OnSyncEnabledChanged is actually invoked 3 times: - // see preference enabled in abobe list - EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())) - .Times(AtLeast(1)); - EXPECT_CALL(*sync_client(), OnSyncEnabledChanged).Times(AtLeast(1)); - sync_service()->OnSetupSyncHaveCode("word1 word2 word5", "test_device"); - EXPECT_FALSE(sync_service()->GetResettingForTest()); + // Emulating resolved with DELETE + records.at(0)->action = SyncRecord::Action::A_DELETE; + EXPECT_CALL(*sync_client(), OnSyncEnabledChanged).Times(1); + // Sync state is changed 4 times: + // 1) we do save updated devices list + // 2,3,4) we do ResetSyncInternal() which clears this device name, + // sync enabled and devices list + EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(4); + sync_service()->OnResolvedPreferences(records); + + EXPECT_FALSE( + profile()->GetPrefs()->GetBoolean(brave_sync::prefs::kSyncEnabled)); + + EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(2); + EXPECT_CALL(*sync_client(), OnSyncEnabledChanged).Times(1); + sync_service()->OnSetupSyncHaveCode( + "anxiety path library anxiety gather lottery category door army half " + "long cage bachelor another expect people blade school " + "educate curtain scrub monitor lady arctic", + "test_device"); EXPECT_TRUE( profile()->GetPrefs()->GetBoolean(brave_sync::prefs::kSyncEnabled)); @@ -908,15 +923,28 @@ TEST_F(BraveSyncServiceTest, ExponentialResend) { TEST_F(BraveSyncServiceTest, GetDevicesWithFetchSyncRecords) { using brave_sync::jslib_const::kPreferences; + EXPECT_CALL(*sync_client(), OnSyncEnabledChanged).Times(1); + EXPECT_CALL(*observer(), OnSyncStateChanged); + brave_sync_prefs()->SetSyncEnabled(true); + brave_sync_prefs()->SetThisDeviceId("1"); + // Expecting SendFetchSyncRecords will be invoked + // after sync_service()->OnPollSyncCycle EXPECT_EQ(brave_sync_prefs()->GetLastFetchTime(), base::Time()); EXPECT_EQ(brave_sync_prefs()->GetLatestDeviceRecordTime(), base::Time()); - EXPECT_CALL(*sync_client(), SendFetchSyncRecords(_, base::Time(), _)) - .Times(1); - sync_service()->FetchDevices(); + + using brave_sync::tools::IsTimeEmpty; + EXPECT_TRUE(IsTimeEmpty(sync_service()->this_device_created_time_)); + + brave_sync::GetRecordsCallback on_get_records = + base::BindOnce(&OnGetRecordsStub); + base::WaitableEvent we; + EXPECT_CALL(*sync_client(), SendSyncRecords("PREFERENCES", _)); + EXPECT_CALL(*sync_client(), SendFetchSyncRecords(_, base::Time(), _)); + sync_service()->OnPollSyncCycle(std::move(on_get_records), &we); + EXPECT_FALSE(IsTimeEmpty(sync_service()->this_device_created_time_)); // Emulate we received this device - brave_sync_prefs()->SetThisDeviceId("1"); auto records = std::make_unique(); records->push_back( SimpleDeviceRecord(SyncRecord::Action::A_CREATE, "1", "device1")); @@ -935,27 +963,7 @@ TEST_F(BraveSyncServiceTest, GetDevicesWithFetchSyncRecords) { EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(1); sync_service()->OnResolvedSyncRecords(kPreferences, std::move(records)); - // When there is only on device in chain, expect fetching sync records with - // start_at==0 - ASSERT_EQ(brave_sync_prefs()->GetSyncDevices()->size(), 1u); - EXPECT_CALL(*sync_client(), SendFetchSyncRecords(_, base::Time(), _)) - .Times(1); - sync_service()->FetchDevices(); - - // If number of devices becomes 2 or more we should next 70 sec use s3 - // with epmty start_at_time and then switch to sqs with non-empty - // start_at_time - records = std::make_unique(); - records->push_back( - SimpleDeviceRecord(SyncRecord::Action::A_CREATE, "2", "device2")); - EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(1); - sync_service()->OnResolvedSyncRecords(kPreferences, std::move(records)); - - // Chain has been created but 70 sec not yet passed - ASSERT_EQ(brave_sync_prefs()->GetSyncDevices()->size(), 2u); - EXPECT_CALL(*sync_client(), SendFetchSyncRecords(_, base::Time(), _)) - .Times(1); - sync_service()->FetchDevices(); + EXPECT_FALSE(IsTimeEmpty(sync_service()->this_device_created_time_)); // Less than 70 seconds have passed, we should fetch with empty start_at { @@ -979,20 +987,25 @@ TEST_F(BraveSyncServiceTest, SendCompact) { using brave_sync::jslib_const::kBookmarks; EXPECT_EQ(brave_sync_prefs()->GetLastCompactTimeBookmarks(), base::Time()); EXPECT_CALL(*sync_client(), SendCompact(kBookmarks)).Times(1); + EXPECT_CALL(*sync_client(), SendFetchSyncRecords).Times(1); sync_service()->FetchSyncRecords(true, false, true, 1000); // timestamp is not writtend until we get CompactedSyncCategory EXPECT_CALL(*sync_client(), SendCompact(kBookmarks)).Times(1); + EXPECT_CALL(*sync_client(), SendFetchSyncRecords).Times(1); sync_service()->FetchSyncRecords(true, false, true, 1000); sync_service()->OnCompactComplete(kBookmarks); EXPECT_CALL(*sync_client(), SendCompact(kBookmarks)).Times(0); + EXPECT_CALL(*sync_client(), SendFetchSyncRecords).Times(1); sync_service()->FetchSyncRecords(true, false, true, 1000); { auto time_override = OverrideForTimeDelta(base::TimeDelta::FromDays( sync_service()->GetCompactPeriodInDaysForTests())); EXPECT_CALL(*sync_client(), SendCompact(kBookmarks)).Times(1); + EXPECT_CALL(*sync_client(), SendFetchSyncRecords).Times(1); sync_service()->FetchSyncRecords(true, false, true, 1000); sync_service()->OnCompactComplete(kBookmarks); } EXPECT_CALL(*sync_client(), SendCompact(kBookmarks)).Times(0); + EXPECT_CALL(*sync_client(), SendFetchSyncRecords).Times(1); sync_service()->FetchSyncRecords(true, false, true, 1000); } From 61d4ba593ba468a23d8fa4019ae767dd9c8e484f Mon Sep 17 00:00:00 2001 From: AlexeyBarabash Date: Wed, 20 Nov 2019 13:53:10 +0200 Subject: [PATCH 4/5] Remove obsolete pref of sync prev seed --- .../chrome/browser/prefs/browser_prefs.cc | 5 ++++- components/brave_sync/BUILD.gn | 7 +++---- .../brave_profile_sync_service_impl.cc | 21 ++++++------------- components/brave_sync/brave_sync_prefs.cc | 16 +++++++------- components/brave_sync/brave_sync_prefs.h | 5 +++-- .../brave_sync/brave_sync_service_unittest.cc | 15 +++++++------ 6 files changed, 33 insertions(+), 36 deletions(-) diff --git a/chromium_src/chrome/browser/prefs/browser_prefs.cc b/chromium_src/chrome/browser/prefs/browser_prefs.cc index 6f02bd691c22..4d50675e29fb 100644 --- a/chromium_src/chrome/browser/prefs/browser_prefs.cc +++ b/chromium_src/chrome/browser/prefs/browser_prefs.cc @@ -3,8 +3,9 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this file, * You can obtain one at http://mozilla.org/MPL/2.0/. */ -#include "brave/browser/brave_profile_prefs.h" #include "brave/browser/brave_local_state_prefs.h" +#include "brave/browser/brave_profile_prefs.h" +#include "brave/components/brave_sync/brave_sync_prefs.h" #include "third_party/widevine/cdm/buildflags.h" #if BUILDFLAG(ENABLE_WIDEVINE) @@ -23,5 +24,7 @@ void MigrateObsoleteProfilePrefs(Profile* profile) { // Added 11/2019. MigrateWidevinePrefs(profile); #endif + // Added 11/2019. + MigrateBraveSyncPrefs(profile); } diff --git a/components/brave_sync/BUILD.gn b/components/brave_sync/BUILD.gn index e87dcdf5f59d..361c5f05199c 100644 --- a/components/brave_sync/BUILD.gn +++ b/components/brave_sync/BUILD.gn @@ -35,8 +35,8 @@ if (enable_brave_sync) { "//components/prefs", "//components/prefs", "//components/signin/public/identity_manager", - "//components/sync/driver:driver", "//components/sync:rest_of_sync", + "//components/sync/driver:driver", "//content/public/browser", "//extensions/browser", "//net", @@ -72,6 +72,7 @@ source_set("prefs") { deps = [ "//components/prefs", + "//content/public/browser", ] } @@ -100,9 +101,7 @@ source_set("crypto") { ] if (is_android) { - deps += [ - "//third_party/android_sdk:cpu_features", - ] + deps += [ "//third_party/android_sdk:cpu_features" ] } } diff --git a/components/brave_sync/brave_profile_sync_service_impl.cc b/components/brave_sync/brave_profile_sync_service_impl.cc index 14ff140a4fce..9cf46edeb690 100644 --- a/components/brave_sync/brave_profile_sync_service_impl.cc +++ b/components/brave_sync/brave_profile_sync_service_impl.cc @@ -78,9 +78,9 @@ std::string GetDeviceName() { } RecordsListPtr CreateDeviceRecord(const std::string& device_name, - const std::string& object_id, - const SyncRecord::Action& action, - const std::string& device_id) { + const std::string& object_id, + const SyncRecord::Action& action, + const std::string& device_id) { RecordsListPtr records = std::make_unique(); SyncRecordPtr record = std::make_unique(); @@ -426,17 +426,9 @@ void BraveProfileSyncServiceImpl::OnSaveInitData(const Uint8Array& seed, std::string seed_str = StrFromUint8Array(seed); std::string device_id_str = StrFromUint8Array(device_id); - std::string prev_seed_str = brave_sync_prefs_->GetPrevSeed(); - seed_.clear(); DCHECK(!seed_str.empty()); - if (prev_seed_str == seed_str) { // reconnecting to previous sync chain - brave_sync_prefs_->SetPrevSeed(std::string()); - } else if (!prev_seed_str.empty()) { // connect/create to new sync chain - brave_sync_prefs_->SetPrevSeed(std::string()); - } - brave_sync_prefs_->SetSeed(seed_str); brave_sync_prefs_->SetThisDeviceId(device_id_str); @@ -650,7 +642,6 @@ void BraveProfileSyncServiceImpl::NotifyHaveSyncWords( void BraveProfileSyncServiceImpl::ResetSyncInternal() { SignalWaitableEvent(); - brave_sync_prefs_->SetPrevSeed(brave_sync_prefs_->GetSeed()); brave_sync_prefs_->Clear(); brave_sync_ready_ = false; @@ -845,9 +836,9 @@ void BraveProfileSyncServiceImpl::SendDeviceSyncRecord( const std::string& device_id, const std::string& object_id) { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); - RecordsListPtr records = CreateDeviceRecord( - device_name, object_id, static_cast(action), - device_id); + RecordsListPtr records = + CreateDeviceRecord(device_name, object_id, + static_cast(action), device_id); SendSyncRecords(SyncRecordType_PREFERENCES, std::move(records)); } diff --git a/components/brave_sync/brave_sync_prefs.cc b/components/brave_sync/brave_sync_prefs.cc index f4d29fcf34d8..a58258fb6a8c 100644 --- a/components/brave_sync/brave_sync_prefs.cc +++ b/components/brave_sync/brave_sync_prefs.cc @@ -9,9 +9,17 @@ #include "brave/components/brave_sync/settings.h" #include "brave/components/brave_sync/sync_devices.h" +#include "chrome/browser/profiles/profile.h" #include "components/pref_registry/pref_registry_syncable.h" #include "components/prefs/pref_service.h" #include "components/prefs/scoped_user_pref_update.h" +#include "content/public/browser/browser_thread.h" + +void MigrateBraveSyncPrefs(Profile* profile) { + DCHECK_CURRENTLY_ON(content::BrowserThread::UI); + PrefService* prefs = profile->GetPrefs(); + prefs->ClearPref(brave_sync::prefs::kSyncPrevSeed); +} namespace brave_sync { namespace prefs { @@ -75,14 +83,6 @@ void Prefs::SetSeed(const std::string& seed) { pref_service_->SetString(kSyncSeed, seed); } -std::string Prefs::GetPrevSeed() const { - return pref_service_->GetString(kSyncPrevSeed); -} - -void Prefs::SetPrevSeed(const std::string& seed) { - pref_service_->SetString(kSyncPrevSeed, seed); -} - std::string Prefs::GetThisDeviceId() const { return pref_service_->GetString(kSyncDeviceId); } diff --git a/components/brave_sync/brave_sync_prefs.h b/components/brave_sync/brave_sync_prefs.h index f70e65f153a8..018359ed3e93 100644 --- a/components/brave_sync/brave_sync_prefs.h +++ b/components/brave_sync/brave_sync_prefs.h @@ -24,6 +24,8 @@ namespace user_prefs { class PrefRegistrySyncable; } +void MigrateBraveSyncPrefs(Profile* profile); + namespace brave_sync { class Settings; @@ -37,6 +39,7 @@ extern const char kSyncDeviceId[]; // like "145,58,125,111,85,164,236,38,204,67,40,31,182,114,14,152,242,..." extern const char kSyncSeed[]; // For storing previous seed after reset. It won't be cleared by Clear() +// Now is deprecated. extern const char kSyncPrevSeed[]; // String of current device namefor sync extern const char kSyncDeviceName[]; @@ -78,8 +81,6 @@ class Prefs { std::string GetSeed() const; void SetSeed(const std::string& seed); - std::string GetPrevSeed() const; - void SetPrevSeed(const std::string& seed); std::string GetThisDeviceId() const; void SetThisDeviceId(const std::string& device_id); std::string GetThisDeviceName() const; diff --git a/components/brave_sync/brave_sync_service_unittest.cc b/components/brave_sync/brave_sync_service_unittest.cc index 4401bad51923..ac91c045b052 100644 --- a/components/brave_sync/brave_sync_service_unittest.cc +++ b/components/brave_sync/brave_sync_service_unittest.cc @@ -398,11 +398,9 @@ TEST_F(BraveSyncServiceTest, GetSeed) { // Service gets seed from client via BraveSyncServiceImpl::OnSaveInitData const auto binary_seed = brave_sync::Uint8Array(16, 77); - EXPECT_TRUE(brave_sync_prefs()->GetPrevSeed().empty()); sync_service()->OnSaveInitData(binary_seed, {0}); std::string expected_seed = brave_sync::StrFromUint8Array(binary_seed); EXPECT_EQ(sync_service()->GetSeed(), expected_seed); - EXPECT_TRUE(brave_sync_prefs()->GetPrevSeed().empty()); } TEST_F(BraveSyncServiceTest, OnDeleteDevice) { @@ -489,8 +487,7 @@ TEST_F(BraveSyncServiceTest, OnDeleteDeviceWhenOneDevice) { EXPECT_TRUE(DevicesContains(devices_semi_final.get(), "1", "device1")); } -TEST_F(BraveSyncServiceTest, - OnDeleteDeviceWhenSelfDeleted) { +TEST_F(BraveSyncServiceTest, OnDeleteDeviceWhenSelfDeleted) { brave_sync_prefs()->SetThisDeviceId("1"); EXPECT_CALL(*sync_client(), OnSyncEnabledChanged).Times(1); EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(1); @@ -693,8 +690,7 @@ TEST_F(BraveSyncServiceTest, OnBraveSyncPrefsChanged) { sync_service()->OnBraveSyncPrefsChanged(brave_sync::prefs::kSyncEnabled); } -void OnGetRecordsStub(std::unique_ptr records) { -} +void OnGetRecordsStub(std::unique_ptr records) {} TEST_F(BraveSyncServiceTest, SetThisDeviceCreatedTime) { EXPECT_TRUE(brave_sync::tools::IsTimeEmpty( @@ -1009,3 +1005,10 @@ TEST_F(BraveSyncServiceTest, SendCompact) { EXPECT_CALL(*sync_client(), SendFetchSyncRecords).Times(1); sync_service()->FetchSyncRecords(true, false, true, 1000); } + +TEST_F(BraveSyncServiceTest, MigratePrevSeed) { + profile()->GetPrefs()->SetString(brave_sync::prefs::kSyncPrevSeed, "1,2,3"); + MigrateBraveSyncPrefs(profile()); + EXPECT_EQ(profile()->GetPrefs()->GetString(brave_sync::prefs::kSyncPrevSeed), + ""); +} From 47c29b663c87ed5b6f8d016f359503fd0f2fac3d Mon Sep 17 00:00:00 2001 From: AlexeyBarabash Date: Thu, 14 Nov 2019 00:04:42 +0200 Subject: [PATCH 5/5] Changes in sync UI to allow test of service - will be reverted after UI PR --- components/brave_sync/ui/components/app.tsx | 2 +- .../brave_sync/ui/components/modals/enterSyncCode.tsx | 7 +++---- .../brave_sync/ui/components/modals/removeDevice.tsx | 7 +------ components/brave_sync/ui/components/modals/scanCode.tsx | 9 ++++----- .../brave_sync/ui/components/modals/viewSyncCode.tsx | 9 ++++----- 5 files changed, 13 insertions(+), 21 deletions(-) diff --git a/components/brave_sync/ui/components/app.tsx b/components/brave_sync/ui/components/app.tsx index fa1cccd8ed96..b8e664ec0f1e 100644 --- a/components/brave_sync/ui/components/app.tsx +++ b/components/brave_sync/ui/components/app.tsx @@ -39,7 +39,7 @@ export class SyncPage extends React.PureComponent { return (
{ - syncData.isSyncConfigured && syncData.devices.length > 1 + syncData.isSyncConfigured ? : } diff --git a/components/brave_sync/ui/components/modals/enterSyncCode.tsx b/components/brave_sync/ui/components/modals/enterSyncCode.tsx index 86c7e6702d51..31dff5b8fe9d 100644 --- a/components/brave_sync/ui/components/modals/enterSyncCode.tsx +++ b/components/brave_sync/ui/components/modals/enterSyncCode.tsx @@ -53,8 +53,7 @@ export default class EnterSyncCodeModal extends React.PureComponent 1 + this.props.syncData.isSyncConfigured ) ) { this.setState({ willCreateNewSyncChainFromCode: false }) @@ -87,8 +86,8 @@ export default class EnterSyncCodeModal extends React.PureComponent { - const { error, thisDeviceName } = this.props.syncData - if (thisDeviceName !== '' || error) { + const { error } = this.props.syncData + if (error) { // Important for the STR return } const { passphrase } = this.state diff --git a/components/brave_sync/ui/components/modals/removeDevice.tsx b/components/brave_sync/ui/components/modals/removeDevice.tsx index f9312f522c64..46bc699b5423 100644 --- a/components/brave_sync/ui/components/modals/removeDevice.tsx +++ b/components/brave_sync/ui/components/modals/removeDevice.tsx @@ -59,12 +59,7 @@ export default class RemoveMainDeviceModal extends React.PureComponent { - const { syncData, deviceName, deviceId } = this.props - // if there aren't enough devices, reset sync - if (syncData.devices.length < 2) { - this.props.actions.onSyncReset() - return - } + const { deviceName, deviceId } = this.props this.props.actions.onRemoveDevice(Number(deviceId), deviceName) this.setState({ willRemoveDevice: true }) } diff --git a/components/brave_sync/ui/components/modals/scanCode.tsx b/components/brave_sync/ui/components/modals/scanCode.tsx index dd2928e16ee2..3adcb477d7db 100644 --- a/components/brave_sync/ui/components/modals/scanCode.tsx +++ b/components/brave_sync/ui/components/modals/scanCode.tsx @@ -52,7 +52,6 @@ export default class ScanCodeModal extends React.PureComponent { componentDidUpdate (prevProps: Readonly) { if ( - this.props.syncData.devices.length > 1 && prevProps.syncData.devices.length !== this.props.syncData.devices.length ) { @@ -78,10 +77,10 @@ export default class ScanCodeModal extends React.PureComponent { } onDismissModal = () => { - const { devices, isSyncConfigured } = this.props.syncData + const { isSyncConfigured } = this.props.syncData // if user is still trying to build a sync chain, // open the confirmation modal. otherwise close it - isSyncConfigured && devices.length < 2 + isSyncConfigured ? this.setState({ willCancelScanCode: true }) : this.dismissAllModals() } @@ -91,12 +90,12 @@ export default class ScanCodeModal extends React.PureComponent { } onConfirmDismissModal = () => { - const { devices, isSyncConfigured } = this.props.syncData + const { isSyncConfigured } = this.props.syncData // sync is enabled when at least 2 devices are in the chain. // this modal works both with sync enabled and disabled states. // in case user opens it in the enabled content screen, // check there are 2 devices in chain before reset - if (isSyncConfigured && devices.length < 2) { + if (isSyncConfigured) { this.props.actions.onSyncReset() this.dismissAllModals() } diff --git a/components/brave_sync/ui/components/modals/viewSyncCode.tsx b/components/brave_sync/ui/components/modals/viewSyncCode.tsx index ade60cf2f11d..4f46a9b66e7d 100644 --- a/components/brave_sync/ui/components/modals/viewSyncCode.tsx +++ b/components/brave_sync/ui/components/modals/viewSyncCode.tsx @@ -48,7 +48,6 @@ export default class ViewSyncCodeModal extends React.PureComponent componentDidUpdate (prevProps: Readonly) { if ( - this.props.syncData.devices.length > 1 && prevProps.syncData.devices.length !== this.props.syncData.devices.length ) { @@ -74,10 +73,10 @@ export default class ViewSyncCodeModal extends React.PureComponent } onDismissModal = () => { - const { devices, isSyncConfigured } = this.props.syncData + const { isSyncConfigured } = this.props.syncData // if user is still trying to build a sync chain, // open the confirmation modal. otherwise close it - isSyncConfigured && devices.length < 2 + isSyncConfigured ? this.setState({ willCancelViewCode: true }) : this.dismissAllModals() } @@ -87,12 +86,12 @@ export default class ViewSyncCodeModal extends React.PureComponent } onConfirmDismissModal = () => { - const { devices, isSyncConfigured } = this.props.syncData + const { isSyncConfigured } = this.props.syncData // sync is enabled when at least 2 devices are in the chain. // this modal works both with sync enabled and disabled states. // in case user opens it in the enabled content screen, // check there are 2 devices in chain before reset - if (isSyncConfigured && devices.length < 2) { + if (isSyncConfigured) { this.props.actions.onSyncReset() this.dismissAllModals() }