Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added panel handler to run actions from vpn popup #13348

Merged
merged 1 commit into from
May 19, 2022
Merged

Added panel handler to run actions from vpn popup #13348

merged 1 commit into from
May 19, 2022

Conversation

spylogsster
Copy link
Contributor

@spylogsster spylogsster commented May 16, 2022 •
edited
Loading

Resolves brave/brave-browser#22874
Resolves brave/brave-browser#22711
Resolves brave/brave-browser#22715
Resolves brave/brave-browser#22694

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • 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, npm run lint, 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:

  • Open popup with clean profile and click 'already purchased' link
  • Check a new tab opened

@spylogsster spylogsster requested a review from bsclifton May 16, 2022 20:27
@spylogsster spylogsster self-assigned this May 16, 2022
@spylogsster spylogsster changed the title Fix aliases for chrome.tabs api to be available in untrusted brave vp… Fix aliases for chrome.tabs to be available in untrusted brave vpn popup May 16, 2022
bsclifton
bsclifton approved these changes May 16, 2022
View reviewed changes
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++

@petemill
Copy link
Member

Please check with security team that they are ok putting an extension API in chrome-untrusted. As far as I can see, this defeats the point of using chrome-untrusted. If we deem the code in the frame is not trustworthy and could be susceptible to being abused, then we would want to limit the abuse surface - and chrome.tabs has some things we might not want to be abused (e.g. opening a tab to a phishing site).

bsclifton
bsclifton requested changes May 16, 2022
View reviewed changes
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually; let's do with handlers as we discussed 😄

petemill
petemill requested changes May 16, 2022
View reviewed changes
Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marking as "Request changes" until we have a security review or take a different approach. I think a preferred approach would be to add Mojom methods for UI actions, e.g. VisitSupportPage would open the necessary URL in a new tab. That would be better for cross-platform too.

@petemill
Copy link
Member

brave_vpn.mojom's ServiceHandler also seems like a good cross-platform place to put the handler's for these UI actions.

@spylogsster spylogsster changed the title Fix aliases for chrome.tabs to be available in untrusted brave vpn popup Added panel handler to run actions from vpn popup May 16, 2022
@spylogsster spylogsster force-pushed the brave-22874 branch 3 times, most recently from d8a0859 to b155347 Compare May 16, 2022 21:32
@bsclifton
Copy link
Member

This looks great! There are a few other places where chrome.tabs.create was used (doh; my bad). I think we can effectively remove this panel from that whitelist after this change is complete (no reason to keep brave://vpn-panel.top-chrome/)

Here are the places:

  • src\brave\components\brave_vpn\resources\panel\components\contact-support\index.tsx
  • src\brave\components\brave_vpn\resources\panel\components\error-subscription-failed-panel\index.tsx
  • src\brave\components\brave_vpn\resources\panel\components\settings-panel\index.tsx

petemill
petemill reviewed May 16, 2022
View reviewed changes
components/brave_vpn/mojom/brave_vpn.mojom Outdated
@@ -20,6 +20,9 @@ interface PanelHandler {

// Notify the backend that the dialog should be closed.
CloseUI();

// Run purchase action.
RunPurchaseFlow(string action);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You think PanelHandler is a better place than ServiceHandler?

Copy link
Contributor Author

@spylogsster spylogsster May 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, we need access to chrome/browser/ui and VPNPanelHandler has it. ServiceHandler is on component level. Also that seems just click handlers, not the service logic

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's fine and if android needs similar we can think about it or duplicate it. But it's a click handler just as much as any other buttons which result in calls to service handler.

@spylogsster
Copy link
Contributor Author

  • resources\panel\components\settings-panel\index.tsx

done

@spylogsster spylogsster force-pushed the brave-22874 branch 6 times, most recently from ede9319 to eb5e166 Compare May 17, 2022 19:59
@spylogsster spylogsster added this to the 1.40.x - Nightly milestone May 17, 2022
bsclifton
bsclifton approved these changes May 18, 2022
View reviewed changes
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works beautifully 😄👍

@spylogsster spylogsster force-pushed the brave-22874 branch 2 times, most recently from 82feaff to d528505 Compare May 18, 2022 10:37
@spylogsster spylogsster merged commit 472af24 into master May 19, 2022
@spylogsster spylogsster deleted the brave-22874 branch May 19, 2022 06:01
@spylogsster spylogsster mentioned this pull request Jun 8, 2022
Closed
This was referenced Jun 21, 2022
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bsclifton bsclifton bsclifton approved these changes

@petemill petemill petemill approved these changes

Assignees

@spylogsster spylogsster

Labels
None yet
Projects
None yet
Milestone
1.41.x - Release
3 participants
@spylogsster @petemill @bsclifton