Skip to content
New issue

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

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

Already on GitHub? # to your account

Cache skus credential till we get valid subs credential #17789

Merged
merged 3 commits into from
Mar 28, 2023

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Mar 28, 2023

Resolves brave/brave-browser#29345

If we fail to get subs credential from guardian, we should keep skus credential till it's expired.

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

BraveVPNServiceTest.SkusCredentialCacheTest

fix brave/brave-browser#29345

If we fail to get subs credential from guardian, we should keep
skus credential till it's expired.
@simonhong simonhong force-pushed the cache_skus_credential_vpn branch from e255af7 to f2dafbf Compare March 28, 2023 05:13
@simonhong simonhong marked this pull request as ready for review March 28, 2023 08:17
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.

This is great! 😄 Went over code in detail. It solves the main case where Guardian API is unavailable for some reason (ex: when getting subscriber credential). We were missing a retry capability for that

@simonhong simonhong force-pushed the cache_skus_credential_vpn branch from 39768a7 to 40f722f Compare March 28, 2023 08:40
@simonhong simonhong merged commit 7c80dff into master Mar 28, 2023
@simonhong simonhong deleted the cache_skus_credential_vpn branch March 28, 2023 12:05
@github-actions github-actions bot added this to the 1.51.x - Nightly milestone Mar 28, 2023
brave-builds added a commit that referenced this pull request Mar 28, 2023
brave-builds added a commit that referenced this pull request Mar 28, 2023
kjozwiak pushed a commit that referenced this pull request Mar 29, 2023
kjozwiak pushed a commit that referenced this pull request Mar 29, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SKU credential (in VPN) should be cached indefinitely until redeemed or expired
2 participants