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: add reuse interval for token refresh #466

Merged
merged 12 commits into from
May 5, 2022
Merged

Conversation

kangmingtay
Copy link
Member

What kind of change does this PR introduce?

token | revoked | parent
foo        t      null
bar        t      foo
baz        f      bar

Assuming the refresh token requests are being made within the reuse interval:

  1. Using bar in the request will return baz since it's the previous revoked token.
  2. Using foo in the request will not return baz since it's the second-last previous revoked token. This will trigger the reuse detection and cause bar and baz to be revoked.
  3. Using baz in the request will create a new refresh token.

Other considerations

@kangmingtay kangmingtay self-assigned this May 3, 2022
Copy link
Member

@thorwebdev thorwebdev left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

Reuse interval should preferably be set to a value greater than the refresh token interval in gotrue-js

I'm currently defaulting to retrying failed requests after 5s: https://github.com/supabase/gotrue-js/pull/278/files#diff-8c943ccfb5928fff63708e99eaafa93f8845c9405ed8e4b32afed23e5003025aR6 So maybe the default interval should be larger than that? Or we shorten the retries to every second and make the default server interval 5s? Wdyt

@kangmingtay
Copy link
Member Author

kangmingtay commented May 4, 2022

So maybe the default interval should be larger than that? Or we shorten the retries to every second and make the default server interval 5s?

@thorwebdev Hmm i think there are 2 cases we need to consider:

  1. To prevent the case where there are concurrent tabs and the JWT expires, the reuse interval has to be larger than the expiresIn - refreshDurationBeforeExpires
  2. Offline retries. Reuse interval > Offline retry interval

I was thinking somewhere between 10-30s to also account for potential delay in network or clock skew.

@kangmingtay kangmingtay merged commit 6a6e3be into master May 5, 2022
@kangmingtay kangmingtay deleted the km/add-reuse-interval branch May 5, 2022 01:19
@github-actions
Copy link
Contributor

github-actions bot commented May 5, 2022

🎉 This PR is included in version 2.6.28 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@thorwebdev
Copy link
Member

@kangmingtay can you post here when this version is deployed to hosted Supabase instances?

@kangmingtay
Copy link
Member Author

hey everyone, we've deployed this update to all hosted Supabase instance already!

@Benjamin-Dobell
Copy link

Benjamin-Dobell commented Oct 24, 2022

It looks like this solution is mostly just kicking the can down the road. We've handled one specific case i.e. when several clients have the same refresh token and there's a race to refresh them. However, we're not handling the situation where a client refreshes their token twice before another client refreshes theirs once.

Client 1 has Token A
Client 2 has Token A

Client 2: Refresh (Token A)

  • Token A is valid
  • Issue Token B
  • Revoke Token A
  • Return Token B

Client 2: Refresh (Token B)

  • Token B is valid
  • Issue Token C
  • Revoke Token B
  • Return Token C

Client 1: Refresh (Token A)

  • Token A is revoked
  • GetValidChildToken (parent = Token A and revoked = false) = NULL
  • Return "Invalid Refresh Token"

I'm seeing this in practice.

Rather than GetValidChildToken querying for parent = ? and revoked = false, it should query for parent = ? and loop until it finds an descendant token that isn't revoked or no more results.

uxodb pushed a commit to uxodb/auth that referenced this pull request Nov 13, 2024
* add reuse interval to config

* add test for refresh token reuse detection

* fix: add reuse interval to refresh token grant

* add tests for reuse interval

* refactor token test

* ignore reuse interval if revoked token is last token

* add test case

* refactor query to get child token

* remove unnecessary check in refresh token grant

* update readme

* update example env file
LashaJini pushed a commit to LashaJini/auth that referenced this pull request Nov 13, 2024
* add reuse interval to config

* add test for refresh token reuse detection

* fix: add reuse interval to refresh token grant

* add tests for reuse interval

* refactor token test

* ignore reuse interval if revoked token is last token

* add test case

* refactor query to get child token

* remove unnecessary check in refresh token grant

* update readme

* update example env file
LashaJini pushed a commit to LashaJini/auth that referenced this pull request Nov 15, 2024
* add reuse interval to config

* add test for refresh token reuse detection

* fix: add reuse interval to refresh token grant

* add tests for reuse interval

* refactor token test

* ignore reuse interval if revoked token is last token

* add test case

* refactor query to get child token

* remove unnecessary check in refresh token grant

* update readme

* update example env file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants