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

Android using Desktop VPN credentials #14715

Merged
merged 9 commits into from
Sep 1, 2022
Merged

Android using Desktop VPN credentials #14715

merged 9 commits into from
Sep 1, 2022

Conversation

bsclifton
Copy link
Member

@bsclifton bsclifton commented Aug 19, 2022 •
edited
Loading

Fixes brave/brave-browser#20374

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:

  1. Purchase VPN on Desktop via https://account.bravesoftware.com
  2. Switch to an Android device for the rest of the test plan
  3. If VPN doesn't show in hamburger menu use the QA menu to enable VPN
  4. Navigate to accounts website (ex: https://account.bravesoftware.com/)
  5. After logging in, you should see the VPN product showing
  6. Purchased VPN product (from step 1) will show a button Refresh Brave VPN. Click this
  7. Now try to enable VPN (it should show up on hamburger menu)
  8. You should be prompted to add a profile to your device - and VPN should connect
  9. Verify you're connected by visiting https://whatismyipaddress.com/
  10. Try changing servers (in settings) and verify via https://whatismyipaddress.com/

@bsclifton bsclifton force-pushed the bsc-fetch-order-credentials-handler branch 3 times, most recently from 001e8ce to 9b0fec7 Compare August 19, 2022 09:01
@deeppandya deeppandya force-pushed the bsc-fetch-order-credentials-handler branch from 9b0fec7 to 995f45a Compare August 24, 2022 04:10
@bsclifton bsclifton changed the title WIP: Android using Desktop VPN credentials Android using Desktop VPN credentials Aug 25, 2022
@bsclifton bsclifton marked this pull request as ready for review August 25, 2022 23:07
SergeyZhukovsky
SergeyZhukovsky reviewed Aug 26, 2022
View reviewed changes
android/java/org/chromium/chrome/browser/app/BraveActivity.java Outdated
@@ -326,6 +326,8 @@ public boolean onMenuOrKeyboardAction(int id, boolean fromMenu) {
} else if (id == R.id.brave_news_id) {
openBraveNewsSettings();
} else if (id == R.id.request_brave_vpn_id || id == R.id.request_brave_vpn_check_id) {
Log.e("BraveVPN",
"purchased user : " + BraveVpnNativeWorker.getInstance().isPurchasedUser());
Copy link
Member

Choose a reason for hiding this comment

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

why do we need that log on menu click?

SergeyZhukovsky
SergeyZhukovsky reviewed Aug 26, 2022
View reviewed changes
android/java/org/chromium/chrome/browser/vpn/utils/BraveVpnApiResponseUtils.java Outdated
InAppPurchaseWrapper.getInstance().processPurchases(
activity, InAppPurchaseWrapper.getInstance().queryPurchases());
// InAppPurchaseWrapper.getInstance().processPurchases(
// activity, InAppPurchaseWrapper.getInstance().queryPurchases());
Copy link
Member

Choose a reason for hiding this comment

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

not needed?

@deeppandya
Copy link
Contributor

I need to add a commit for the refactoring part. Sorry for the commented code.

@deeppandya deeppandya force-pushed the bsc-fetch-order-credentials-handler branch from 995f45a to 6d0461a Compare August 29, 2022 16:26
@bsclifton bsclifton force-pushed the bsc-fetch-order-credentials-handler branch from 6d0461a to 18e07af Compare August 29, 2022 19:51
@bsclifton bsclifton mentioned this pull request Aug 30, 2022
Closed
@bsclifton
Copy link
Member Author

bsclifton commented Aug 31, 2022 •
edited
Loading

OK gave this a try with the latest code 👍

  • You can login to accounts website on your Android phone
  • Click Refresh Brave VPN for the VPN you bought on desktop
  • The website itself shows a banner You have active credentials loaded (nothing seems to change in the native Brave UI)
  • Turn on VPN (toggle using the ... > Brave VPN menu item)
  • Brave asks to add a profile. When accepted, it'll connect you 😄
  • Visiting Settings > Brave Firewall + VPN lets you change servers (which works great)
  • Status and Expires fields are empty (we can always populate those in a follow up PR)

I think we're good to go here 😄👍 WDYT, @deeppandya?

samartnik
samartnik approved these changes Sep 1, 2022
View reviewed changes
Copy link
Contributor

@samartnik samartnik left a comment

Choose a reason for hiding this comment

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

lgtm

SergeyZhukovsky
SergeyZhukovsky approved these changes Sep 1, 2022
View reviewed changes
Copy link
Member

@SergeyZhukovsky SergeyZhukovsky left a comment

Choose a reason for hiding this comment

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

++

@bsclifton bsclifton merged commit 6007616 into master Sep 1, 2022
@bsclifton bsclifton deleted the bsc-fetch-order-credentials-handler branch September 1, 2022 17:47
@bsclifton bsclifton added this to the 1.45.x - Nightly milestone Sep 1, 2022
@bsclifton bsclifton mentioned this pull request Sep 29, 2022
Closed
8 tasks
@bsclifton bsclifton mentioned this pull request Oct 31, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonhong simonhong simonhong left review comments

@deeppandya deeppandya deeppandya left review comments

@samartnik samartnik samartnik approved these changes

@SergeyZhukovsky SergeyZhukovsky SergeyZhukovsky approved these changes

@spylogsster spylogsster Awaiting requested review from spylogsster

Assignees

@bsclifton bsclifton

@deeppandya deeppandya

Labels
None yet
Projects
None yet
Milestone
1.45.x - Release
Development

Successfully merging this pull request may close these issues.

Detect and store VPN state after restoring on account.brave.com
5 participants
@bsclifton @deeppandya @simonhong @SergeyZhukovsky @samartnik