Add Brave VPN WireGuard setting to brave://settings/system #19531

spylogsster · 2023-08-02T10:00:30Z

Resolves brave/brave-browser#32002

Added Brave VPN section brave://settings/system
Added option to enable/disable Brave VPN Wireguard protocol
Disabling the option if Brave VPN connected.
Checking if the wireguard service was not registered and register it with elevated request.
Removed Wireguard feature flag.

Videos:

Register the service:

register.wg.service.mp4

Disable the option if VPN connected:

disable.if.connected.mp4

Enable/disable wireguard protocol:

2023-08-02_13h02_21.mp4

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 lint, 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:

Test Plan:

Go to brave://settings/system and enable/disable Use WireGuard for Brave VPN toggler.
The protocol used by Brave VPN should meet the option value.
If the service was not registered (try to remove it using sc delete [service name] from admin cmd). The browser should ask admin permissions to register the service if the option enabled.
if Brave VPN connected, the option should be reset to initial value and disabled while the vpn connected.
if Status Tray icon launched it should be stopped when Wireguard protocol is turned off.

brave-builds · 2023-08-02T11:15:59Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2023-08-02T12:00:29Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2023-08-02T15:11:09Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2023-08-03T03:13:30Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2023-08-03T09:11:04Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2023-08-03T10:08:44Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2023-08-03T11:30:33Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2023-08-03T12:00:27Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2023-08-04T05:23:14Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2023-08-04T07:55:31Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2023-08-09T18:04:46Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2023-08-10T05:07:29Z

A Storybook has been deployed to preview UI for the latest push

bsclifton

Lots of super small details here - checked them all out (both the code and trying the PR). Nicely done w/ the webui handler and moving the feature check code into utils methods. Everything LGTM and test plan works great 😄👍

sangwoo108

LGTM with a few minor nits 👍

sangwoo108 · 2023-08-10T13:07:02Z

browser/about_flags.cc

  EXPAND_FEATURE_ENTRIES({                                                   \
      kBraveVPNWireguardFeatureInternalName,                                 \
      "Enable experimental WireGuard Brave VPN service",                     \
-      "Experimental WireGuard VPN support. Not implemented yet",             \
+      "Experimental WireGuard VPN support. Deprecated.",                     \
      kOsWin,                                                                \
      FEATURE_VALUE_TYPE(brave_vpn::features::kBraveVPNUseWireguardService), \
  })


nit: Could we remove this entirely? As the feature value would be migrated into pref, I think we can hide this flag from the brave://flags

we have already tried, looks like no becuse we need to know the actual value to migrate

That's intersting. I thought leaving features.cc as is and removing only this part would work. 🤔

browser/ui/webui/settings/brave_vpn/brave_vpn_handler.h

sangwoo108 · 2023-08-10T13:13:24Z

browser/ui/webui/settings/brave_vpn/brave_vpn_handler.cc

+  if (service) {
+    Observe(service);
+  }


I'm wondering if we should consider it as a failure when |service| doesn't exist. If we should, I think CHECK(service) could make more sense.

sangwoo108 · 2023-08-10T13:16:43Z

components/brave_vpn/common/brave_vpn_utils.h

@@ -36,7 +36,10 @@ bool HasValidSubscriberCredential(PrefService* local_prefs);
 std::string GetSubscriberCredential(PrefService* local_prefs);
 bool HasValidSkusCredential(PrefService* local_prefs);
 std::string GetSkusCredential(PrefService* local_prefs);
-
+bool IsBraveVPNWireguardEnabled(PrefService* local_state);


Should we move IsBraveVPNWireguardEnabled into the IS_WIN guard? It seems we're not using this when it's not.

We will be adding WireGuard support on other platforms - hopefully soon 🙂 (macOS and Linux)

sangwoo108 · 2023-08-10T13:18:39Z

browser/resources/settings/brave_system_page/brave_vpn_page.html

+    <settings-toggle-button id="toggleWireguardButton"
+        pref="{{prefs.brave.brave_vpn.wireguard_enabled}}"
+        label="$i18n{useWireguardLabel}"
+        sub-label="[[toggleWireguardSubLabel_]]"
+        disabled="[[braveVpnConnected_]]"
+        on-change="onChange_">


Should we render this only when it's os win?

this seems to be causing a problem on macOS

sangwoo108 · 2023-08-10T13:27:00Z

components/brave_vpn/common/win/utils.cc

+  if (!status.has_value()) {
+    return false;
+  }
+  return status.value() == SERVICE_RUNNING;


Out of curiosity, why is SERVICE_START_PENDING is not considered as 'running' state unlike IsBraveVPNWireguardTunnelServiceRunning() does. Is it just because how each backend works is different?

yep, IsBraveVPNWireguardTunnelServiceRunning reflects the status of the tunnel service in runtime to show necessary icons, but in IsWindowsServiceRunning mostly used to ensure we have vpn in working state which is not correct for SERVICE_START_PENDING event.

Co-authored-by: Sangwoo Ko <sko@brave.com>

brave-builds · 2023-08-10T16:57:30Z

A Storybook has been deployed to preview UI for the latest push

spylogsster self-assigned this Aug 2, 2023

github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Aug 2, 2023

spylogsster changed the title ~~Brave 32002~~ Add Brave VPN WireGuard setting to brave://settings/system Aug 2, 2023

spylogsster requested review from bsclifton and simonhong August 2, 2023 10:05

spylogsster force-pushed the brave-32002 branch 2 times, most recently from e6d3ad7 to 5274df7 Compare August 2, 2023 11:24

spylogsster force-pushed the brave-32002 branch from 18cd05a to c8b2bf9 Compare August 2, 2023 14:31

spylogsster force-pushed the brave-32002 branch from c8b2bf9 to 2e432fa Compare August 3, 2023 02:22

spylogsster force-pushed the brave-32002 branch 2 times, most recently from 6a5ec6c to fee845f Compare August 3, 2023 11:24

spylogsster force-pushed the brave-32002 branch from fee845f to 82552ef Compare August 3, 2023 11:31

spylogsster force-pushed the brave-32002 branch 2 times, most recently from 4a42fd2 to 1059062 Compare August 4, 2023 04:51

spylogsster force-pushed the brave-32002 branch from 1059062 to 9088b96 Compare August 4, 2023 07:25

spylogsster marked this pull request as ready for review August 4, 2023 07:58

spylogsster requested review from a team as code owners August 4, 2023 07:58

github-actions bot added the needs-security-review label Aug 4, 2023

github-actions bot assigned goodov Aug 4, 2023

spylogsster added 11 commits August 10, 2023 07:07

Added Brave VPN section to brave://settings/system

f82bd08

Update fallback logic for Wireguard

6027c3e

Disable setting if Brave VPN connected

5ead960

Register Wireguard service

e6666d2

Fix strings

fe55b3d

Move to BraveSettingsUI

2a1bed9

Use kBraveVpnWireguardServiceInstallSwitchName

0b15dc3

Replace Wireguard -> WireGuard

70bfe4c

Update initial value

f6eb471

Migrate kBraveVPNWireguardEnabled feature flag

112112c

Close tray icon if wireguard is not active

25e12b5

spylogsster force-pushed the brave-32002 branch from 3b8c72b to 25e12b5 Compare August 10, 2023 04:08

bsclifton approved these changes Aug 10, 2023

View reviewed changes

spylogsster requested a review from sangwoo108 August 10, 2023 12:10

sangwoo108 approved these changes Aug 10, 2023

View reviewed changes

spylogsster and others added 3 commits August 10, 2023 22:41

Update browser/ui/webui/settings/brave_vpn/brave_vpn_handler.h

75b98a1

Co-authored-by: Sangwoo Ko <sko@brave.com>

Update browser/ui/webui/settings/brave_vpn/brave_vpn_handler.h

302ea3e

Co-authored-by: Sangwoo Ko <sko@brave.com>

Fix review comments

9d8db06

spylogsster enabled auto-merge (squash) August 10, 2023 16:06

spylogsster merged commit 7fb458e into master Aug 10, 2023
13 checks passed

spylogsster deleted the brave-32002 branch August 10, 2023 18:34

github-actions bot added this to the 1.59.x - Nightly milestone Aug 10, 2023

github-actions bot added potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false CI/run-upstream-tests Run upstream unit and browser tests on Linux and Windows (otherwise only on Linux) labels Aug 10, 2023

bsclifton mentioned this pull request Aug 12, 2023

brave://settings/autofill panel is broken brave/brave-browser#32213

Closed

bsclifton mentioned this pull request Aug 21, 2023

If #brave-vpn-wireguard flag is disabled, Wireguard services should not run OR old flag should be removed brave/brave-browser#32382

Closed

mkarolin mentioned this pull request Sep 7, 2023

Brave crashes when navigate to More tools->Guest profile>settings menu brave/brave-browser#32825

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Brave VPN WireGuard setting to brave://settings/system #19531

Add Brave VPN WireGuard setting to brave://settings/system #19531

spylogsster commented Aug 2, 2023 •

edited

Loading

brave-builds commented Aug 2, 2023

brave-builds commented Aug 2, 2023

brave-builds commented Aug 2, 2023

brave-builds commented Aug 3, 2023

brave-builds commented Aug 3, 2023

brave-builds commented Aug 3, 2023

brave-builds commented Aug 3, 2023

brave-builds commented Aug 3, 2023

brave-builds commented Aug 4, 2023

brave-builds commented Aug 4, 2023

brave-builds commented Aug 9, 2023

brave-builds commented Aug 10, 2023

bsclifton left a comment

sangwoo108 left a comment

sangwoo108 Aug 10, 2023

spylogsster Aug 10, 2023

sangwoo108 Aug 10, 2023

sangwoo108 Aug 10, 2023

spylogsster Aug 10, 2023

sangwoo108 Aug 10, 2023

spylogsster Aug 10, 2023

bsclifton Aug 10, 2023

sangwoo108 Aug 10, 2023

spylogsster Aug 10, 2023

bsclifton Aug 12, 2023 •

edited

Loading

sangwoo108 Aug 10, 2023

spylogsster Aug 10, 2023 •

edited

Loading

brave-builds commented Aug 10, 2023

Add Brave VPN WireGuard setting to brave://settings/system #19531

Add Brave VPN WireGuard setting to brave://settings/system #19531

Conversation

spylogsster commented Aug 2, 2023 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

brave-builds commented Aug 2, 2023

brave-builds commented Aug 2, 2023

brave-builds commented Aug 2, 2023

brave-builds commented Aug 3, 2023

brave-builds commented Aug 3, 2023

brave-builds commented Aug 3, 2023

brave-builds commented Aug 3, 2023

brave-builds commented Aug 3, 2023

brave-builds commented Aug 4, 2023

brave-builds commented Aug 4, 2023

brave-builds commented Aug 9, 2023

brave-builds commented Aug 10, 2023

bsclifton left a comment

Choose a reason for hiding this comment

sangwoo108 left a 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

bsclifton Aug 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spylogsster Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

brave-builds commented Aug 10, 2023

spylogsster commented Aug 2, 2023 •

edited

Loading

bsclifton Aug 12, 2023 •

edited

Loading

spylogsster Aug 10, 2023 •

edited

Loading