Advanced server selection ux #25089

deeppandya · 2024-08-12T16:57:37Z

Resolves brave/brave-browser#36277
Resolves brave/brave-browser#38186

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:

Android

Automatic

Buy IAP subscription on fresh profile
Create VPN profile
Turn on VPN
Go to Server selection page
Observe -> Automatic should be selected

Optimal server location from the selected region

Go to Server selection page
Select USA as region
On the next page, select Optimal
Observe - > Best server location would be selected and it could give different location based on the available resources on the location at the time.

Specific server selection

Go to Server selection page
Select USA as region
On the next page, select Atlanta
Observe -> Atlanta should be selected as server location

Upgrade scenario

After the upgrade, Go to Server selection page
Observe -> Selected region from previous version should also be selected in the new UI
Tap on the region
And on the next page, Observe -> Optimal should be selected as we didn't have a granular details from previous version.
New version of the app would follow the new changes moving forward

Desktop

BraveVPNServiceTest.*
BraveVPNUtilsUnitTest.*

Open vpn panel and check main panel has sub label such as Optimal or city name.

Go to select page in vpn panel. Each country shows detailed server/city info in the sublabel

Click country entry and it shows Optimal sub entry and its cities

When click connect on Optimal, main panel's sub label has Optimal or city name if clicked on each city entry like the screen shot above at 1 step
When click connen on country, it goes with optimal.
Select server panel has Automatic entry at the top. When clicks it, it picks current timezone's region
Existing users will see previously selected country with Optimal region

brave-builds · 2024-08-26T07:56:16Z

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

brave-builds · 2024-08-29T07:57:25Z

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

brave-builds · 2024-08-29T08:02:57Z

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

brave-builds · 2024-08-29T08:12:05Z

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

components/brave_vpn/browser/brave_vpn_service.cc

components/brave_vpn/browser/connection/brave_vpn_region_data_manager.cc

Resolve desktop unittest

components/brave_vpn/resources/panel/state/async.ts

components/brave_vpn/resources/panel/api/region_interface.ts

fallaciousreasoning

components/brave_vpn/resources LGTM with some nits

components/brave_vpn/resources/panel/components/main-panel/index.tsx

components/brave_vpn/resources/panel/components/main-panel/style.ts

components/brave_vpn/resources/panel/components/select-region-list/index.tsx

components/brave_vpn/resources/panel/components/select-region-list/style.ts

fallaciousreasoning · 2024-09-06T06:37:34Z

components/brave_vpn/resources/panel/state/async.ts

 })

 handler.on(Actions.purchaseExpired.getType(), async (store) => {
  const [{ currentRegion }, { regions }] = await Promise.all([
    getPanelBrowserAPI().serviceHandler.getSelectedRegion(),
-    getPanelBrowserAPI().serviceHandler.getAllRegions()
+    getPanelBrowserAPI().serviceHandler.getAllRegions(
+      REGION_PRECISION_CITY_BY_COUNTRY


Do we ever call this function with a different precision?

Right, we only call with this arg.
@deeppandya maybe we don't need to have param for this api(GetAllRegions)?
If you don't mind, I could change this method.

I assumed that it may be useful for the desktop as it provides a different response based on the region_precision. If it's not useful for the desktop, we can surely get rid of it.

Fixed by 6f24b64

components/brave_vpn/resources/panel/stories/mock-data/region-list.ts

fallaciousreasoning

whoops accidentally commented instead of approving

github-actions · 2024-09-06T09:08:17Z

[puLL-Merge] - brave/brave-core@25089

Description

This PR implements significant changes to the Brave VPN feature, particularly focusing on the server selection functionality and UI improvements. The changes include updates to the region data structure, new API endpoints, UI enhancements for server selection, and modifications to the VPN service handling.

Changes

Changes

Android Java Changes:
- Added new activities and adapters for VPN server selection.
- Updated existing VPN-related activities and adapters.
- Modified BraveVpnNativeWorker to support new region data structure.
- Added BraveVpnServiceFactoryAndroid for mojo interface handling.
C++ Changes:
- Updated BraveVpnService and related classes to support new region data structure.
- Added new methods for handling region selection and data fetching.
- Modified API request handling for new endpoints.
UI/UX Changes:
- Updated panel UI components for improved server selection experience.
- Added new localized strings for server selection UI.
- Implemented new UI for displaying country and city-level server information.
Data Structure Changes:
- Updated Region struct to include more detailed information (cities, server count, etc.).
- Modified region data parsing and storage methods.
Preference Handling:
- Added new preferences for VPN region list version and automatic server selection.
- Implemented migration for older region selection preferences.
Mojo Interface Updates:
- Modified brave_vpn.mojom to support new region data structure and methods.
Resource Updates:
- Added new icons and updated existing ones for VPN UI.

Possible Issues

The changes to the region data structure and selection process may require careful testing to ensure backward compatibility with existing user configurations.
The new UI for server selection introduces more complexity, which could potentially lead to performance issues on lower-end devices.

Security Hotspots

No significant security hotspots were identified in this change. However, as with any network-related feature, careful review of the new API endpoints and data handling should be conducted to ensure no sensitive information is exposed.

github-actions · 2024-09-06T09:11:39Z

components/common/locale.ts

@@ -24,13 +28,36 @@ export const getLocale = (key: string, replacements?: Record<string, string>) =>

  if (replacements) {
    for (let item in replacements) {
-      returnVal = returnVal.replace(new RegExp('\\[\\[\\s*' + item + '\\s*\\]\\]'), replacements[item].toString())
+      returnVal = returnVal.replace(
+        new RegExp('\\[\\[\\s*' + item + '\\s*\\]\\]'),


_{reported by reviewdog 🐶}
[semgrep] RegExp() called with a replacements function argument, this might allow an attacker to cause a Regular Expression Denial-of-Service (ReDoS) within your application as RegExP blocks the main thread. For this reason, it is recommended to use hardcoded regexes instead. If your regex is run on user-controlled input, consider performing input validation or use a regex checking/sanitization library such as https://www.npmjs.com/package/recheck to verify that the regex does not appear vulnerable to ReDoS.

Source: https://semgrep.dev/r/javascript.lang.security.audit.detect-non-literal-regexp.detect-non-literal-regexp

Cc @thypon

This is just a formatting change

components/common/locale.ts

fallaciousreasoning

lgtm thanks @simonhong !

simonhong · 2024-09-06T09:24:36Z

lgtm thanks @simonhong !

Thanks for great review :)

github-actions · 2024-09-06T09:31:29Z

components/brave_vpn/browser/brave_vpn_service.cc

+#if BUILDFLAG(IS_ANDROID)
+  api_request_->GetServerRegions(
+      base::BindOnce(&BraveVpnService::OnFetchRegionList,
+                     base::Unretained(this), std::move(callback)),


_{reported by reviewdog 🐶}
[semgrep] base::Unretained is most of the time unrequited, and a weak reference is better suited for secure coding.
Consider swapping Unretained for a weak reference.
base::Unretained usage may be acceptable when a callback owner is guaranteed
to be destroyed with the object base::Unretained is pointing to, for example:

- PrefChangeRegistrar
- base::*Timer
- mojo::Receiver
- any other class member destroyed when the class is deallocated

Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/chromium-uaf.yaml

Cc @thypon @goodov @iefremov

github-actions bot assigned deeppandya Aug 12, 2024

deeppandya force-pushed the advanced_server_selection_ux branch 2 times, most recently from 4de31c7 to 9390ef8 Compare August 19, 2024 20:33

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

deeppandya force-pushed the advanced_server_selection_ux branch 4 times, most recently from 1cef4c4 to 0aaa6dc Compare August 21, 2024 23:50

simonhong self-assigned this Aug 27, 2024

simonhong force-pushed the advanced_server_selection_ux branch from 618fbfe to b490aea Compare August 29, 2024 07:15

simonhong force-pushed the advanced_server_selection_ux branch 5 times, most recently from be17e54 to 02989ef Compare September 2, 2024 08:14

deeppandya force-pushed the advanced_server_selection_ux branch from 243e82b to 1bcc82e Compare September 4, 2024 21:43

deeppandya requested review from AlexeyBarabash and samartnik September 4, 2024 21:44

deeppandya marked this pull request as ready for review September 4, 2024 21:44

deeppandya requested review from a team as code owners September 4, 2024 21:44

github-actions bot added the puLL-Merge label Sep 5, 2024

github-actions bot reviewed Sep 5, 2024

View reviewed changes

components/brave_vpn/browser/brave_vpn_service.cc Show resolved Hide resolved

components/brave_vpn/browser/connection/brave_vpn_region_data_manager.cc Show resolved Hide resolved

github-actions bot added the needs-security-review label Sep 5, 2024

github-actions bot assigned goodov Sep 5, 2024

deeppandya and others added 10 commits September 6, 2024 03:22

Resolve brave vpn constant issue

19f2a69

Resolve desktop unittest

Update server selection behaviour for automatic selection

39730f9

Fixed storybook build failure

c863587

Fixed test failures

bdf524a

VPN server selection UI on desktop

6a86f93

Migrated region list v1's selected region name to v2's name

76123e4

Add UI changes for selected server selection android

5274ff6

fixup! Migrated region list v1's selected region name to v2's name

0f81d79

fixup! VPN server selection UI on desktop

2ad1d5b

Address PR comments

6965c93

deeppandya force-pushed the advanced_server_selection_ux branch from 9100adc to 6965c93 Compare September 5, 2024 21:59

bsclifton approved these changes Sep 5, 2024

View reviewed changes

simonhong reviewed Sep 6, 2024

View reviewed changes

fallaciousreasoning reviewed Sep 6, 2024

View reviewed changes

fallaciousreasoning approved these changes Sep 6, 2024

View reviewed changes

Addressed review comment

cc55ae9

github-actions bot reviewed Sep 6, 2024

View reviewed changes

fallaciousreasoning reviewed Sep 6, 2024

View reviewed changes

components/common/locale.ts Show resolved Hide resolved

fallaciousreasoning approved these changes Sep 6, 2024

View reviewed changes

Removed unnecessary param from GetAllRegions()

6f24b64

github-actions bot reviewed Sep 6, 2024

View reviewed changes

deeppandya merged commit f67bdee into master Sep 6, 2024
17 checks passed

deeppandya deleted the advanced_server_selection_ux branch September 6, 2024 12:36

github-actions bot added this to the 1.71.x - Nightly milestone Sep 6, 2024

brave-builds added a commit that referenced this pull request Sep 6, 2024

Uplift of #25089 (squashed) to beta

2f329ac

brave-builds mentioned this pull request Sep 6, 2024

Advanced server selection ux (uplift to 1.70.x) #25465

Closed

7 tasks

bsclifton mentioned this pull request Sep 17, 2024

Brave crashed when attempted to refresh Brave VPN credentials brave/brave-browser#41093

Closed

6 tasks

deeppandya mentioned this pull request Sep 17, 2024

[Android] Advanced server selection follow up brave/brave-browser#41106

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Advanced server selection ux #25089

Advanced server selection ux #25089

deeppandya commented Aug 12, 2024 •

edited

Loading

brave-builds commented Aug 26, 2024

brave-builds commented Aug 29, 2024

brave-builds commented Aug 29, 2024

brave-builds commented Aug 29, 2024

fallaciousreasoning left a comment

fallaciousreasoning Sep 6, 2024

simonhong Sep 6, 2024

deeppandya Sep 6, 2024

simonhong Sep 6, 2024

fallaciousreasoning left a comment

github-actions bot commented Sep 6, 2024

Changes

github-actions bot Sep 6, 2024

fallaciousreasoning Sep 6, 2024

fallaciousreasoning left a comment

simonhong commented Sep 6, 2024

github-actions bot Sep 6, 2024

Advanced server selection ux #25089

Advanced server selection ux #25089

Conversation

deeppandya commented Aug 12, 2024 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Android

Desktop

brave-builds commented Aug 26, 2024

brave-builds commented Aug 29, 2024

brave-builds commented Aug 29, 2024

brave-builds commented Aug 29, 2024

fallaciousreasoning left a comment

Choose a reason for hiding this comment

fallaciousreasoning Sep 6, 2024

Choose a reason for hiding this comment

simonhong Sep 6, 2024

Choose a reason for hiding this comment

deeppandya Sep 6, 2024

Choose a reason for hiding this comment

simonhong Sep 6, 2024

Choose a reason for hiding this comment

fallaciousreasoning left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 6, 2024

Description

Changes

Possible Issues

Security Hotspots

github-actions bot Sep 6, 2024

Choose a reason for hiding this comment

fallaciousreasoning Sep 6, 2024

Choose a reason for hiding this comment

fallaciousreasoning left a comment

Choose a reason for hiding this comment

simonhong commented Sep 6, 2024

github-actions bot Sep 6, 2024

Choose a reason for hiding this comment

deeppandya commented Aug 12, 2024 •

edited

Loading