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.

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

SKU credential (in VPN) should be cached indefinitely until redeemed or expired #29345

Closed
bsclifton opened this issue Mar 27, 2023 · 8 comments · Fixed by brave/brave-core#17789

Comments

@bsclifton
Copy link
Member

bsclifton commented Mar 27, 2023

Description

A pool of SKU credentials are issued for the client when they connect over account.brave.com. These credentials are each good for 24 hours (each having their own 24 hour time window) and have a boolean tracking them for if they're "spent".

The credentials are considered "spent" after they are presented. In the VPN code, that happens here:
https://github.com/brave/brave-core/blob/12648373743b58d164a00e0c2a4e00f76e2ab9cd/components/brave_vpn/browser/brave_vpn_service.cc#L719-L776

I believe we're missing a case for when Guardian VPN authorization fails for some reason (service outage, etc). When the credential is presented (above), we need to cache this (save to local state). We need to re-use that until:

  • we authorize successfully with Guardian VPN
    OR
  • the date in expires_at (GMT) is reached

Right now, I believe we're not handling failure scenario for Guardian VPN API. This may not even be a problem with Guardian- the customer themselves may have an outage during the call. We then try to present the credential AGAIN - and there are no unspent credentials that meet the current time window.

  • STR for checking normal(success) behavior
  1. Complete purchase and refresh
  2. Click vpn button and see main panel is shown
  3. If fetching subs creds is failed, brave_vpn.subscriber_credential will have skus_credential value.
    When not failed, brave_vpn.subscriber_credential will have credential value.
@bsclifton bsclifton added bug priority/P2 A bad problem. We might uplift this to the next planned release. OS/Desktop feature/vpn feature/SKUs labels Mar 27, 2023
simonhong added a commit to brave/brave-core that referenced this issue Mar 28, 2023
fix brave/brave-browser#29345

If we fail to get subs credential from guardian, we should keep
skus credential till it's expired.
@simonhong
Copy link
Member

Set QA/NO because it seems very difficult to simulate subs credential fetching failure.

@stephendonner
Copy link

Set QA/NO because it seems very difficult to simulate subs credential fetching failure.

Even blocklisting/routing to 127.0.0.1 connect-api.guardian.com (or whatever the appropriate hostname is)?

@bsclifton
Copy link
Member Author

bsclifton commented Mar 28, 2023

@stephendonner to simulate, we had to edit the code - so that the first call fails on purpose (ex: it goes to a bad URL). The second call then would work.

We can't make connect-api.guardian.com non-routable because it'll fail before it gets to the authentication portion (ex: when it gets server list). I wish there was an easier way to test - but we both did compile/test/verify locally. We can always hop on a call and do a live debugging

@simonhong
Copy link
Member

@stephendonner @bsclifton Although it's difficult to test with official build, we can check there is no failure.
If brave://local-state has brave_vpn.subscriber_credential.credential key, all good.
If it has brave_vpn.subscriber_credential.skus_credential key, we failed to get subs credential from guardian.

1. Complete purchase and refresh
2. Click vpn button and see main panel is shown
3. If fetching subs creds is failed, brave_vpn.subscriber_credential will have sksu_credential value.
   When not failed, brave_vpn.subscriber_credential will have credential value.

@brave-builds brave-builds modified the milestones: 1.51.x - Beta, 1.50.x - Release, 1.49.x - Release #6 Mar 29, 2023
@kjozwiak kjozwiak added QA/Yes and removed QA/No labels Mar 29, 2023
@kjozwiak
Copy link
Member

@stephendonner @bsclifton Although it's difficult to test with official build, we can check there is no failure. If brave://local-state has brave_vpn.subscriber_credential.credential key, all good. If it has brave_vpn.subscriber_credential.skus_credential key, we failed to get subs credential from guardian.

1. Complete purchase and refresh
2. Click vpn button and see main panel is shown
3. If fetching subs creds is failed, brave_vpn.subscriber_credential will have sksu_credential value.
   When not failed, brave_vpn.subscriber_credential will have credential value.

@stephendonner @bsclifton labelled as QA/Yes for now due to the above comment from @simonhong. However, if this is still hard to verify even with the above, please feel free to slap the QA/No. I'll leave this in the 1.49.x milestone till ~Thursday before moving it into 1.50.x. At that point, I doubt we'll get another C111 or need a 1.49.x HF.

@kjozwiak
Copy link
Member

Moving this into 1.50.x as per the above as I doubt we'll get another C111 update or require a 1.49.x HF as 1.50.x is scheduled for next week. If we do end up needing a 1.49.x, we'll move the above back into a new 1.49.x milestone.

@kjozwiak kjozwiak modified the milestones: 1.49.x - Release #6, 1.50.x - Release Mar 30, 2023
@stephendonner
Copy link

This affects macOS, too, but a check on Windows should suffice, here.

@stephendonner
Copy link

stephendonner commented Mar 31, 2023

Verification PASSED using

Brave 1.50.110 Chromium: 112.0.5615.49 (Official Build) (64-bit)
Revision bd2a7bcb881c11e8cfe3078709382934e3916914-refs/branch-heads/5615@{#936}
OS Windows 10 Version 22H2 (Build 19045.2788)

Steps:

  1. installed 1.50.110
  2. launched Brave
  3. loaded account.brave.software (dev, as a stopgap)
  4. entered throwaway email from temp-mail.org to create new account
  5. logged back in via link in email
  6. purchased Brave VPN & Firewall
  7. confirmed I saw You have active credentials loaded! green banner/messaging
  8. clicked on the VPN button in the browser toolbar
  9. clicked to toggle to Connected
  10. loaded brave://local-state
  11. examined the value for brave.vpn.subscriber_credentials

Confirmed via both brave://local-state as well as general usage that Brave VPN credentials are working as expected 👍

just-purchased state Connected brave://local-state
image (1) image image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants