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

fix: improve check for validity of token #76

Merged
merged 4 commits into from
Apr 29, 2024

Conversation

ewanharris
Copy link
Member

@ewanharris ewanharris commented Apr 25, 2024

Description

  • 1d20960 - Extends the testing of the token validity check to include checks for closer to the expiry time (expires in 6 minutes, 5 minutes)
  • 2084b0a - Refactors the validity check to split it out into separate null checks and time checks.

I've removed the jitter here as I'm not sure what it added and it made testing harder as we didn't have predictability. If we want to guard against a potential "thundering herd" on the token refresh then I think we'd be better off attempting to only allow one call to the token endpoints to occur at a time (not sure how to do this in Java myself, maybe if we used synchronized on exchangeToken?

The jitter calculation has been changed to use nextInt(TOKEN_EXPIRY_BUFFER_THRESHOLD_IN_SEC), this will return a value up to our 300 seconds limit that we can use to prevent multiple calls at the same time. From my testing nextLong could potentially return negative values so I believe this is not safe to use, given we want our jitter to be up to 300 seconds more then we nextInt with the bound is safer.

References

Resolves #75

Review Checklist

  • I have clicked on "allow edits by maintainers".
  • I have added documentation for new/changed functionality in this PR or in a PR to openfga.dev [Provide a link to any relevant PRs in the references section above]
  • The correct base branch is being used, if not main
  • I have added tests to validate that the change in functionality is working as expected

@ewanharris ewanharris requested a review from a team as a code owner April 25, 2024 21:34
@codecov-commenter
Copy link

codecov-commenter commented Apr 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 28.65%. Comparing base (6717967) to head (f98569c).

Additional details and impacted files
@@             Coverage Diff              @@
##               main      #76      +/-   ##
============================================
+ Coverage     28.51%   28.65%   +0.13%     
- Complexity      721      723       +2     
============================================
  Files           145      145              
  Lines          5277     5283       +6     
  Branches        567      567              
============================================
+ Hits           1505     1514       +9     
+ Misses         3714     3712       -2     
+ Partials         58       57       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ewanharris ewanharris changed the title test: extend tests for access token validity check fix: improve check for validity of token Apr 26, 2024
@ewanharris ewanharris force-pushed the fix/refactor-validity-check branch 2 times, most recently from 4676fa4 to a0ce129 Compare April 26, 2024 14:11
@ewanharris ewanharris added this pull request to the merge queue Apr 29, 2024
Merged via the queue into main with commit a7ad2c9 Apr 29, 2024
19 checks passed
@ewanharris ewanharris deleted the fix/refactor-validity-check branch April 29, 2024 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Intermittent FgaApiAuthenticationError when using Oauth2 Client Credentials
3 participants