Sync v2 #5294

darkdh · 2020-04-20T18:52:48Z

Resolves brave/brave-browser#9989
Resolves brave/brave-browser#9241

This PR

Removes all sync v1 codes
Sync V2 UI
2.1. Setup flow concept inherited from v1 will be in brave:///settings/braveSync as subpage
2.2. Brings back about:sync-internals
2.3. brave://sync will be redirect to brave:///settings/braveSync
2.4. Client encryption is enforced in UI using bip39 seed.
Sync V2 Engine
3.1. Seed is encrypted using OSCrypt when stored in prefs
3.2. ProfileSyncService monitor seed changes to decide reset sync chain or tell SyncAuthManger to derive signing key
3.3. BraveSyncAuthManager will have a dummy SyncAccountInfo and create access tokenbase64(timestamp_hex|signed_timestamp_hex|public_key_hex), timestamp is from network time, if network time is not available, local time will be used. Each server response will update the network time by header Sane-Time-Millis
Migration
4.1. Strip off bookmark meta info used by v1 in BookmarkModel
4.2 Stop engine to have a clean engine state in ProfileSyncService
Tests
5.1. upstream sync_auth_manager_unittest.cc
5.2 brave_sync_auth_manager_unittest.cc
Sync v2 will only be available on Nightly channel and development build

Submitter Checklist:

Test Plan:

Note that Sync v1 and v2 are incompatible so DONOT try to connect v1 and v2 devices, it won't work

Upgrade from v1

Migration

Have v1 enabled profile and sync some bookmarks
Upgrade to v2, we should be able to create sync chain
Check profile/Default/Bookmarks and each bookmark will no longer have sync v1 meta info
All the bookmarks are still there

Feature flag

Disable brave://flags/#brave-sync in v1
Upgrade to v2 and go to brave:///settings/braveSync
We should see sync entry
Go to brave://flags and there will only be brave://flags/#brave-sync-v2 and brave://flags/#brave-sync is gone

Infobar

#5804 (comment)

v2 test

Single device

Enable sync v2 using fresh profile (brave:///settings/braveSync)
Open about:sync-internals
2.1. Make sure Passphrase Type is PassphraseType::kCustomPassphrase
2.2. Encrypted Types include every types we support
2.3. Has Token is true
Check profile/Default/Preferences
3.1. seed in brave_sync_v2 should not be your sync code (it is encrypted)
3.2. v1_meta_info_cleared is true
3.3. v1_migrated is true
Type brave://sync/ in URL bar should take you to brave:///settings/braveSync
Leave sync chain
Open about:sync-internals, sync engine should stop

Multiple devices

Create sync chain on A and make device B joined
Check sync engines are running in both about:sync-internals
Add a bookmark on A and toggle Always show bookmarks (Type Bookmarks & Preferences)
device B should see the same thing in about 1 minute

Offline sync chain creation

Turn off network
Create sync chain
Open about:sync-internals
Turn network back on
It should be able to connect to server in few retry

Client clock out of date

Turn client clock to be at least 24 hours ahead of behind
Create sync chain
Open about:sync-internals
It should be able to connect to server in few retry

Feature flag

Enable sync v2
Open about:sync-internals and make sure sync engine is running
Go to brave://flags/#brave-sync-v2 and disable it
After relaunch about:sync-internals should show Transport State is Sync service does not exist
There should be no sync entry in brave:///settings/
Go to brave://flags/#brave-sync-v2 and enable it
After relaunch about:sync-internals will show sync engine is running

UI test

Test bad sync code
Test empty sync code
Test enable/disable different data types
- Note that change will be saved only when you leave sync page
Close sync page when in the middle of sync setup

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

patches/chrome-browser-resources-settings-people_page-sync_browser_proxy.js.patch

bridiver · 2020-06-08T20:25:55Z

patches/chrome-browser-ui-webui-settings-people_handler.cc.patch

+
+ bool IsSyncSubpage(const GURL& current_url) {
+  BRAVE_IS_SYNC_SUBPAGE
+   return current_url == chrome::GetSettingsUrl(chrome::kSyncSetupSubPage);


GetSettingsUrl looks like something we could easily override

fixed in b2fdf8c

bridiver · 2020-06-08T20:36:34Z

patches/components-sync-driver-profile_sync_service.h.patch


+ BRAVE_PROFILE_SYNC_SERVICE_H_


see https://github.com/brave/brave-browser/wiki/Patching-Chromium#making-methods-virtual for adding a method without patching

fixed b2fdf8c

bridiver · 2020-06-08T20:37:08Z

patches/components-sync-driver-sync_auth_manager.h.patch

+   void ResetRequestAccessTokenBackoffForTest();
+
+  private:
+  BRAVE_SYNC_AUTH_MANAGER_H_


I believe the technique in https://github.com/brave/brave-browser/wiki/Patching-Chromium#making-methods-virtual can also be used here

fixed b2fdf8c

bridiver · 2020-06-08T20:37:24Z

patches/components-sync-driver-BUILD.gn.patch

+   defines = [ "SYNC_USER_AGENT_PRODUCT=$sync_user_agent_product" ]
+
+   configs += [ "//build/config/compiler:wexit_time_destructors" ]
+


don't add extra newlines in patches

fixed in b2fdf8c

components/brave_sync/network_time_helper.cc

components/sync/driver/brave_sync_auth_manager.cc

bridiver · 2020-06-09T00:10:10Z

browser/resources/settings/brave_sync_page/brave_sync_page.js

+        const syncCode = await this.braveBrowserProxy_.getSyncCode()
+        syncPrefs.setNewPassphrase = false;
+        syncPrefs.passphrase = syncCode;
+        console.debug('sync set encryption', syncPrefs)


won't this leak the passphrase into the console (and potentially the log)?

cc @petemill

fixed in 247a37c

bridiver · 2020-06-09T00:12:06Z

browser/resources/settings/brave_sync_page/brave_sync_code_dialog.js

+        return
+      }
+      const data = await this.syncBrowserProxy_.getQRCode(this.syncCode)
+      console.log('qr code', data)


I don't think we should be outputting this kind of data on the console (and potentially logging to disk)

fixed in 247a37c

bridiver · 2020-06-09T00:14:25Z

browser/ui/webui/settings/brave_sync_handler.cc

+
+  std::vector<uint8_t> seed;
+  if (!brave_sync::crypto::PassphraseToBytes32(sync_code->GetString(), &seed)) {
+    LOG(ERROR) << "invalid sync code:" << sync_code->GetString();


this is only logging invalid codes, but a user might just be using the wrong sync chain or something so we probably should log this

yea i think this could be sensitive info and shouldn't be logged

fixed in 247a37c

bridiver · 2020-06-09T00:14:36Z

browser/ui/webui/settings/brave_sync_handler.cc

+
+  std::vector<uint8_t> seed;
+  if (!brave_sync::crypto::PassphraseToBytes32(sync_code->GetString(), &seed)) {
+    LOG(ERROR) << "invalid sync code:" << sync_code->GetString();


same as above

fixed in 247a37c

bridiver · 2020-06-09T00:15:09Z

components/brave_sync/network_time_helper.cc

+  base::Time time;
+  if (network_time_tracker_->GetNetworkTime(&time, nullptr) !=
+      network_time::NetworkTimeTracker::NETWORK_TIME_AVAILABLE) {
+    LOG(WARNING) << "Network time not available, using local time";


this seems like it should be a vlog

fixed in b2fdf8c

bridiver · 2020-06-09T00:15:44Z

browser/ui/webui/settings/brave_sync_handler.cc

+  CHECK(args->Get(1, &sync_code));
+
+  if (sync_code->GetString().empty()) {
+    LOG(ERROR) << "No sync code parameter provided!";


seems like all of these should be VLOG

This one I think should stay as error since our code doesn't supply the wrong number of parameters so if it errors it will be a one off dev error when making a change

darkdh · 2020-06-09T19:54:51Z

@petemill there are some html checks failed in sonar cloud
https://github.com/brave/brave-core/pull/5294/checks?check_run_id=755297176

diracdeltas · 2020-06-09T21:27:36Z

app/brave_generated_resources.grd

+        Sync Settings
+      </message>
+      <message name="IDS_SETTINGS_BRAVE_SYNC_SETTINGS_SUBTITLE" desc="Brave Sync settings subtitle">
+        Manage what information you would like to sync between devices. These settings only effect this device.


affect not effect

fixed in 4f4fa65

diracdeltas · 2020-06-09T21:27:58Z

app/brave_generated_resources.grd

+        To start, you will need Brave installed on all the devices you plan to sync. Then, securely link them together with a sync code.
+      </message>
+      <message name="IDS_SETTINGS_BRAVE_SYNC_MANAGE_ACTION_LABEL" desc="Brave Sync manage action label">
+        Manage your sync'd devices


i think synced instead of sync'd looks more correct?

fixed in 4f4fa65

FYI @bradleyrichter sync'd -> synced

app/brave_main_delegate.cc

diracdeltas · 2020-06-10T21:15:02Z

components/sync/driver/brave_sync_auth_manager.cc

+  base::Base64Encode(access_token, &encoded_access_token);
+  DCHECK(!encoded_access_token.empty());
+
+  VLOG(1) << "access_token= " << encoded_access_token;


did you mean to keep this VLOG? (access token is sensitive info)

fixed in 5e3cc37

app/brave_main_delegate.cc

bridiver · 2020-06-10T23:38:48Z

app/brave_main_delegate.cc

@@ -158,7 +172,7 @@ bool BraveMainDelegate::BasicStartupComplete(int* exit_code) {

  // Brave's sync protocol does not use the sync service url
  command_line.AppendSwitchASCII(switches::kSyncServiceURL,
-                                 "https://no-thanks.invalid");
+                                 kBraveSyncServiceURL);


we should check to make sure the sync url is empty before setting it, otherwise you can't override

void BraveCommandLineHelper::AppendSwitch(const char* switch_key) { if (!command_line_.HasSwitch(switch_key)) command_line_.AppendSwitch(switch_key); }

our BraveCommandLineHelper already checks it

and add BRAVE_SERVICES_KEY header for sync request Also remove access token VLOG

…ith how to add BraveServiceKeys in http_bridge.cc

…me cases

The infobar displays a notice that sync v2 must be setup in order to continue syncing. In this infobar, the user can - perform an action to begin sync v2 setup, or; - dismiss the infobar.

[sync-v2 base] Sync: Show an infobar at startup for sync v1 users

darkdh · 2020-06-11T19:47:43Z

@petemill mentioned we can ignore the only error from SonarCloud
https://sonarcloud.io/organizations/brave/rules?languages=css&open=css%3AS4670
which will be disabled soon because it doesn't play nicely with Web Components

darkdh force-pushed the sync-v2 branch 3 times, most recently from 668915c to 3b5392f Compare April 21, 2020 22:30

NejcZdovc mentioned this pull request May 2, 2020

New Sync v2 UI wiring brave/brave-browser#9241

Closed

7 tasks

darkdh force-pushed the sync-v2 branch 2 times, most recently from de74c2a to 4fb0a66 Compare May 6, 2020 00:02

darkdh self-assigned this May 22, 2020

darkdh marked this pull request as ready for review May 22, 2020 23:19

darkdh requested a review from bridiver as a code owner May 22, 2020 23:19

darkdh changed the title ~~[WIP] Sync v2~~ Sync v2 May 22, 2020

darkdh force-pushed the sync-v2 branch 2 times, most recently from 5697bb2 to 02023d0 Compare June 2, 2020 00:38

bridiver reviewed Jun 2, 2020

View reviewed changes

patches/chrome-browser-resources-settings-people_page-sync_browser_proxy.js.patch Outdated Show resolved Hide resolved

darkdh added CI/skip Do not run CI builds (except noplatform) and removed CI/skip Do not run CI builds (except noplatform) labels Jun 4, 2020

darkdh force-pushed the sync-v2 branch 2 times, most recently from 4f791a2 to 08eb047 Compare June 7, 2020 22:22

bridiver reviewed Jun 8, 2020

View reviewed changes

darkdh force-pushed the sync-v2 branch from bd0bc25 to 57f8662 Compare June 8, 2020 20:40

bridiver reviewed Jun 8, 2020

View reviewed changes

components/brave_sync/network_time_helper.cc Show resolved Hide resolved

diracdeltas reviewed Jun 8, 2020

View reviewed changes

components/sync/driver/brave_sync_auth_manager.cc Outdated Show resolved Hide resolved

bridiver reviewed Jun 9, 2020

View reviewed changes

diracdeltas reviewed Jun 9, 2020

View reviewed changes

app/brave_main_delegate.cc Show resolved Hide resolved

darkdh mentioned this pull request Jun 9, 2020

Whitelist sync v2 for network audit brave/brave-browser#10192

Closed

Use differnt feature flag for sync v2 and l10n string cleanup

4f4fa65

diracdeltas reviewed Jun 10, 2020

View reviewed changes

app/brave_main_delegate.cc Show resolved Hide resolved

bridiver reviewed Jun 10, 2020

View reviewed changes

darkdh requested a review from iefremov as a code owner June 11, 2020 03:03

darkdh force-pushed the sync-v2 branch from 58fcc9b to 7514a4e Compare June 11, 2020 03:52

diracdeltas approved these changes Jun 11, 2020

View reviewed changes

darkdh and others added 5 commits June 11, 2020 01:35

Generalize BRAVE_SERVICES_KEY defines and brave_service_key declare_args

df0a12d

and add BRAVE_SERVICES_KEY header for sync request Also remove access token VLOG

Overrides set Authorization header in SyncStoppedReporter to comply w…

be4f1e4

…ith how to add BraveServiceKeys in http_bridge.cc

Settings WebUI: Accessibility improvements for sync html

d51a3f3

Pinning certificate for sync v2 endpoints

51b4d3c

SetNetworkTimeTracker might be called later than GetNetworkTime in so…

b5f41a3

…me cases

darkdh force-pushed the sync-v2 branch from 876a69f to b5f41a3 Compare June 11, 2020 08:36

petemill and others added 3 commits June 11, 2020 11:10

Sync: Show an infobar at startup for sync v1 users

1467ef7

The infobar displays a notice that sync v2 must be setup in order to continue syncing. In this infobar, the user can - perform an action to begin sync v2 setup, or; - dismiss the infobar.

Merge pull request #5804 from brave/sync-v2-infobar

9ce2485

[sync-v2 base] Sync: Show an infobar at startup for sync v1 users

Update CODEOWNERS

103c336

Fix android build by including buildflags header for all platforms

a181816

mihaiplesa merged commit 2c8b566 into master Jun 11, 2020

mihaiplesa deleted the sync-v2 branch June 11, 2020 23:02

darkdh added this to the 1.12.x - Nightly milestone Jun 12, 2020

darkdh mentioned this pull request Jun 18, 2020

Implement "Resync" option brave/brave-browser#6674

Closed

ckujau mentioned this pull request Jul 2, 2020

Sync Open tabs across devices brave/brave-browser#4412

Closed

btlechowski mentioned this pull request Jul 22, 2020

Sync Test Plan brave/qa-resources#172

Closed

74 tasks

btlechowski mentioned this pull request Aug 6, 2020

Sync v2 brave/brave-browser#9989

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync v2 #5294

Sync v2 #5294

darkdh commented Apr 20, 2020 •

edited by kjozwiak

Loading

bridiver Jun 8, 2020

darkdh Jun 9, 2020

bridiver Jun 8, 2020

darkdh Jun 9, 2020

bridiver Jun 8, 2020

darkdh Jun 9, 2020

bridiver Jun 8, 2020

darkdh Jun 9, 2020

bridiver Jun 9, 2020

darkdh Jun 9, 2020

darkdh Jun 9, 2020

bridiver Jun 9, 2020

darkdh Jun 9, 2020

bridiver Jun 9, 2020

diracdeltas Jun 9, 2020

darkdh Jun 9, 2020

bridiver Jun 9, 2020

darkdh Jun 9, 2020

bridiver Jun 9, 2020

darkdh Jun 9, 2020

bridiver Jun 9, 2020

petemill Jun 9, 2020

darkdh commented Jun 9, 2020

diracdeltas Jun 9, 2020

darkdh Jun 10, 2020

diracdeltas Jun 9, 2020

darkdh Jun 10, 2020

petemill Jun 10, 2020

diracdeltas Jun 10, 2020

darkdh Jun 11, 2020

bridiver Jun 10, 2020

darkdh Jun 10, 2020

darkdh commented Jun 11, 2020

Sync v2 #5294

Sync v2 #5294

Conversation

darkdh commented Apr 20, 2020 • edited by kjozwiak Loading

Submitter Checklist:

Test Plan:

Upgrade from v1

Migration

Feature flag

Infobar

v2 test

Single device

Multiple devices

Offline sync chain creation

Client clock out of date

Feature flag

UI test

Reviewer Checklist:

After-merge Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkdh commented Jun 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkdh commented Jun 11, 2020

darkdh commented Apr 20, 2020 •

edited by kjozwiak

Loading