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

Token expiration test is not accurate #77

Closed
GaryAustin1 opened this issue Apr 19, 2022 · 6 comments · Fixed by #92
Closed

Token expiration test is not accurate #77

GaryAustin1 opened this issue Apr 19, 2022 · 6 comments · Fixed by #92
Labels
enhancement New feature or request released

Comments

@GaryAustin1
Copy link

GaryAustin1 commented Apr 19, 2022

The math in at least two pieces of code yields up to 1/2 second expired token because of rounding.

utils/getUser.ts
image
utils/getAccessToken.ts
image

const timeNow = Math.round(Date.now() / 1000);
jwtUser.exp < timeNow yields:

jwtUser.exp       timeNow      Date.now()    
xxx1               xxx0             xxx0.499            "good" 1/2 sec to expire left  
xxx1               xxx1             xxx0.999            "good" .001 sec to expire! (not really good as no time to do anything)
xxx1               xxx1             xxx1.499            "bad" expired 1/2 sec ago but treated as not expired
xxx1               xxx2             xxx1.501             test works as it is expired for real

I also don't believe it is useful to have a token verified as good from cache if it can expire before even leaving the function call. What good is a token that expires before it can even be used?

@dabarrell
Copy link

There's minimal cost in refreshing a token, so it makes sense to refresh if it expires in the next minute (?), rather than waiting for it to expire

@thorwebdev
Copy link
Member

Yes, that's a good point. The token refresh strategy isn't ideal yet. Happy to hear ideas 🙏

@thorwebdev thorwebdev added the enhancement New feature or request label Apr 20, 2022
@dabarrell
Copy link

dabarrell commented Apr 20, 2022

@thorwebdev I've already got an open PR to set up automatic token refreshes (#78) - I'd be happy to add this

it makes sense to refresh if it expires in the next minute

to that PR if it works for you? It'd basically be

const threshold = 60 * 1000; // One minute
if (jwtUser.exp < timeNow + threshold) {
  // refresh
}

@thorwebdev
Copy link
Member

Thanks @dabarrell, yah, I think that makes sense. I also commented on your PR, thanks for that! One note, we're currently revamping our repo setup (#75) and then are hoping to get this in right after 👍

@dabarrell
Copy link

@thorwebdev In that case I'll wait for #75 to be merged to save a messy rebase :)

@github-actions
Copy link
Contributor

🎉 This issue has been resolved in version 1.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants