[Sync] Add an option in setting page to Enter Custom Sync Url #25484

jagadeshjai · 2024-09-07T06:12:01Z

Added an option in settings page to enter custom sync url which is specific to each profile.
Also handle the GPO for the same.
Rename brave_components_sync_driver_[sources/deps] -> brave_components_sync_service_[sources/deps]

Resolves brave/brave-browser#12314

Resolves brave/brave-browser#41280

UI:

brave_custom_sync_input.webm

After configuring policy

Two different Profiles

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

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, for instance:

Test Plan:

jagadeshjai · 2024-09-09T17:45:12Z

Hey @aguscruiz ! Could you confirm the design for this UI?

Kindly refer SC in Overview.

jagadeshjai · 2024-09-15T18:36:14Z

chromium_src/chrome/browser/sync/sync_service_factory.cc

 #include "brave/components/sync/service/brave_sync_service_impl.h"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/sync/device_info_sync_service_factory.h"
+#include "components/sync/base/command_line_switches.h"
+
+// Below includes are just to prevent redefining of


These included seems nasty,
@goodov Do we have any known alternatives to avoid this, other than direct patching?

yeah, this is too messy.

I think we should stop using syncer::kSyncServiceURL for setting the URL via the command line, but instead do this:

override syncer::internal::kSyncServerUrl and syncer::internal::kSyncDevServerUrl to be BUILDFLAG(BRAVE_SYNC_ENDPOINT).

modify BraveGetSyncServiceURL to also use the pref value if syncer::kSyncServiceURL flag is not passed (right now it's used only if the pref is managed).

remove syncer::kSyncServiceURL setting in //brave/app/brave_main_delegate.cc.

support AdjustSyncServiceUrlForAndroid in BraveGetSyncServiceURL.

cc @AlexeyBarabash ^ does this sound correct? aren't there any hidden shenanigans on iOS/Android if we go this way?

Sorry, @jagadeshjai @goodov I have conmpletly missed this PR and the question.

all above sounds correct and I see the code is now much cleaner.

No hidden shenanigans on Android.

iOS should also be ok. Just in case lets ask @Brandon-T, does iOS have any code to setup custom sync server url?

I just have tested the PR on Android and Desktop - works fine.

chromium_src/components/sync/base/DEPS

chromium_src/chrome/browser/sync/sync_service_factory.cc

browser/resources/settings/brave_sync_page/sync_url_input.ts

jagadeshjai · 2024-10-19T05:46:11Z

@jagadeshjai

======= Failed test run #1 ==========

Fixed 👍

jagadeshjai · 2024-10-02T17:45:17Z

chromium_src/components/sync/service/sync_service_impl.cc

+// on Desktop OSes. However, we should also handle it on Android in the future.
+#if !BUILDFLAG(IS_ANDROID)
+  if (prefs) {
+    std::string value = prefs->GetString(brave_sync::kCustomSyncServiceUrl);
    if (!value.empty()) {
      GURL custom_sync_url(value);
      // Provided URL must be HTTPS.
      if (custom_sync_url.is_valid() &&
          custom_sync_url.SchemeIs(url::kHttpsScheme)) {


@AlexeyBarabash

I have doubt - Do we really need to check for the https scheme here? I feel that most users likely won’t serve their custom server over https, especially when it's within a personal/home network.

CC: @bsclifton

I will add that I use a server over http. (And for remote access away from home, I use a VPN, so it's http within a Wireguard tunnel).

Sorry for not responding earlier @jagadeshjai - thanks for raising

It was a requirement to have HTTPS by our security team. I haven't tested - but it should still be possible to use sync with a self signed cert. Also Let's Encrypt is pretty great now too

Does the sync server have E2E encryption of the data that only the client can decrypt?

My understanding is that it does and that this is independent of the connection protocol (HTTP/HTTPS) so even if someone did something dumb and opened an http sync server to the internet, the only thing that might be visible is metadata but the actual data is encrypted.

If the security team is convinced something must be done, I would suggest a compromise of a warning, a sub-setting, or a flag.

Of course, if nothing is changed, then yeah, it'll force users to setup a reverse proxy (I'm not aware of a built-in way to use HTTPS with the docker container for the sync server as there is a lack of documentation for self-hosting the sync server because basic setup is basically just run the docker compose).

I hope this will be considered by the Brave teams. I appreciate all of the work done by Brave to make such a great browser!

Brave uses client side encryption/description - it's impossible for us to see what is in a blob being stored (a bookmark, password, etc). We can see the type and that's it. Chrome has this too - but it's something you need to enable and provide a pass phrase. Hosting the https://github.com/brave/go-sync should be enough

jagadeshjai · 2024-11-03T03:52:05Z

@goodov Could you please review this PR?

bsclifton

Rebased and added some quick changes for strings (and fixed a few CI issues).
String reviewers ++

CI now running with #25691

bsclifton · 2024-11-04T20:37:26Z

OK pinged a few folks - I need to share the UI with design / product to make sure it's good. I can follow up about any concerns once that happens. Thanks for your patience

bsclifton · 2024-11-06T20:16:07Z

Just a few screenshots to share - I know the original post has a video too

Here is what brave://settings/braveSync looks like (basically clicking Sync on left menu of brave://settings) for a new user:

Here's what it looks like for folks that have a sync group setup:

I think we should disable editing Custom Sync URL if the sync group is established. The reason for this would be: person would have created the data on the existing backend (by default, Brave). If they edit the URL, the browser code likely won't handle the switch to the new backend gracefully. The person would need to delete the sync group first before changing the URL.

If the person didn't change the Sync URL and the group is created, we can probably hide this setting.

And here's what it looks like if you have group policy setup that overrides BraveSyncUrl

This case looks good - but I think it needs a tooltip over the special group policy icon - so it could say This setting is managed by your organization (or similar) when you put the mouse over the icons for the buildings (next to the textbox)

relaunch button when toogle/url change.

…ndling custom sync url. Also show Relauch button and Managed icon when required. Conflicts: browser/ui/webui/brave_settings_ui.cc

…ervice via command line before bulding its service in browser context.

…DevServerUrl using chromium_src overrides instead of passing it via command_line

…in Android to |BraveGetSyncServiceURL|.

…s_sync_service_[sources/deps]. And Remove some unused includes.

…and line

jagadeshjai changed the title ~~[Sync] Add an option in setting page to Enable/Enter Custom Sync Url.~~ [Sync] [WIP] Add an option in setting page to Enable/Enter Custom Sync Url. Sep 8, 2024

jagadeshjai force-pushed the feature__add_option_custom_sync_url branch from 8d7d7ff to f26c2e6 Compare September 8, 2024 19:18

jagadeshjai force-pushed the feature__add_option_custom_sync_url branch 2 times, most recently from e2da192 to 4fc9c3e Compare September 9, 2024 18:09

NicholasFlamy mentioned this pull request Sep 10, 2024

Group policy/settings: configuration for --sync-url= argument brave/brave-browser#20431

Closed

jagadeshjai force-pushed the feature__add_option_custom_sync_url branch 5 times, most recently from 7ddc422 to 8cc6efd Compare September 15, 2024 16:14

jagadeshjai changed the title ~~[Sync] [WIP] Add an option in setting page to Enable/Enter Custom Sync Url.~~ [Sync] [WIP] Add an option in setting page to Enter Custom Sync Url Sep 15, 2024

jagadeshjai force-pushed the feature__add_option_custom_sync_url branch 2 times, most recently from e0844cf to 6034292 Compare September 15, 2024 18:29

jagadeshjai commented Sep 15, 2024

View reviewed changes

jagadeshjai force-pushed the feature__add_option_custom_sync_url branch from 6034292 to c42c7ba Compare September 15, 2024 18:38

jagadeshjai marked this pull request as ready for review September 15, 2024 18:38

jagadeshjai requested review from a team as code owners September 15, 2024 18:38

jagadeshjai force-pushed the feature__add_option_custom_sync_url branch 5 times, most recently from 2c9a8ce to 0d046ba Compare September 22, 2024 04:30

goodov reviewed Sep 23, 2024

View reviewed changes

chromium_src/components/sync/base/DEPS Outdated Show resolved Hide resolved

chromium_src/chrome/browser/sync/sync_service_factory.cc Outdated Show resolved Hide resolved

browser/resources/settings/brave_sync_page/sync_url_input.ts Show resolved Hide resolved

brave-builds mentioned this pull request Sep 23, 2024

CI run for contributor PR #25484 #25691

Draft

jagadeshjai force-pushed the feature__add_option_custom_sync_url branch from 0d046ba to bf28c14 Compare September 25, 2024 03:04

jagadeshjai requested a review from a team as a code owner September 25, 2024 03:04

jagadeshjai force-pushed the feature__add_option_custom_sync_url branch from ae64392 to a8b34bc Compare October 19, 2024 05:45

jagadeshjai commented Oct 19, 2024

View reviewed changes

jagadeshjai force-pushed the feature__add_option_custom_sync_url branch from a8b34bc to c5623ab Compare November 3, 2024 03:50

bsclifton force-pushed the feature__add_option_custom_sync_url branch 2 times, most recently from 83b5438 to 9d2de48 Compare November 4, 2024 07:24

bsclifton approved these changes Nov 4, 2024

View reviewed changes

jagadeshjai force-pushed the feature__add_option_custom_sync_url branch from 9d2de48 to 3b1d897 Compare November 10, 2024 15:13

jagadeshjai requested review from a team and iefremov as code owners November 10, 2024 15:13

jagadeshjai and others added 9 commits November 10, 2024 20:50

[Sync] Add an option in setting page to Enable/Enter Custom Sync Url.

e745e8f

[Sync] Add a spearate input element for custom sync url and show

d519652

relaunch button when toogle/url change.

[Sync] Remove Enable/Disable switch and just use input element for ha…

7191a06

…ndling custom sync url. Also show Relauch button and Managed icon when required. Conflicts: browser/ui/webui/brave_settings_ui.cc

[Sync] Pass the custom sync url from the profile preference to sync s…

0d60129

…ervice via command line before bulding its service in browser context.

[Sync] Set the |BRAVE_SYNC_ENDPOINT| directly in kSyncServerUrl/kSync…

67df87e

…DevServerUrl using chromium_src overrides instead of passing it via command_line

[Android][Sync] Move the logic for changing the sync url for staging …

32e5aa1

…in Android to |BraveGetSyncServiceURL|.

Rename brave_components_sync_driver_[sources/deps] -> brave_component…

5287b2a

…s_sync_service_[sources/deps]. And Remove some unused includes.

[Sync] Remove test case for checking BRAVE_SYNC_ENDPOINT set via comm…

49b377b

…and line

FIXUP: small edits for CI and string changes

4c3ef66

jagadeshjai force-pushed the feature__add_option_custom_sync_url branch from 3b1d897 to 4c3ef66 Compare November 10, 2024 15:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Sync] Add an option in setting page to Enter Custom Sync Url #25484

[Sync] Add an option in setting page to Enter Custom Sync Url #25484

jagadeshjai commented Sep 7, 2024 •

edited

Loading

jagadeshjai commented Sep 9, 2024

jagadeshjai Sep 15, 2024

goodov Sep 16, 2024 •

edited

Loading

goodov Sep 16, 2024

AlexeyBarabash Sep 27, 2024

jagadeshjai commented Oct 19, 2024

jagadeshjai Oct 2, 2024

NicholasFlamy Oct 20, 2024

bsclifton Oct 21, 2024

NicholasFlamy Oct 21, 2024

bsclifton Oct 21, 2024

jagadeshjai commented Nov 3, 2024

bsclifton left a comment •

edited

Loading

bsclifton commented Nov 4, 2024

bsclifton commented Nov 6, 2024 •

edited

Loading

[Sync] Add an option in setting page to Enter Custom Sync Url #25484

Are you sure you want to change the base?

[Sync] Add an option in setting page to Enter Custom Sync Url #25484

Conversation

jagadeshjai commented Sep 7, 2024 • edited Loading

After configuring policy

Two different Profiles

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

jagadeshjai commented Sep 9, 2024

Choose a reason for hiding this comment

goodov Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jagadeshjai commented Oct 19, 2024

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

jagadeshjai commented Nov 3, 2024

bsclifton left a comment • edited Loading

Choose a reason for hiding this comment

bsclifton commented Nov 4, 2024

bsclifton commented Nov 6, 2024 • edited Loading

jagadeshjai commented Sep 7, 2024 •

edited

Loading

goodov Sep 16, 2024 •

edited

Loading

bsclifton left a comment •

edited

Loading

bsclifton commented Nov 6, 2024 •

edited

Loading