Fix silent error in token refresh #581
Conversation
…ken is present there is a silent failure If `AccessTokenRequest` is requested with `refresh: true` and no refresh_token is present in the session, there is no error thrown esp if there is an existing session, this causes a silent error.
|
@kanalo-shrek is attempting to deploy a commit to the Auth0 Team on Vercel. A member of the Team first needs to authorize it. |
| // There is an edge case where we might have some clock skew where our code assumes the token is still valid. | ||
| // Adding a skew of 1 minute to compensate. | ||
| if (!session.refreshToken && session.accessTokenExpiresAt * 1000 - 60000 < Date.now()) { | ||
| throw new AccessTokenError( |
There was a problem hiding this comment.
Hi @kanalo-shrek - can you fix the case where The access token expired and a refresh token is not available
and also add a test case for the scenario that you've introduced
There was a problem hiding this comment.
Ah I was relying on both the cases, but your comment on detecting the errors early makes sense, just for the sake of readability though do you feel like the SDK should declare variables as const hasAccessTokenExpired = ... etc, instead of repeating the same logic everywhere?
There was a problem hiding this comment.
Yes, thats true, although you don't need to touch that part of the code for your change (just add a couple of lines for the !refreshToken && refresh if statement and a single test case for it)
| // Check if the token has expired. | ||
| // There is an edge case where we might have some clock skew where our code assumes the token is still valid. | ||
| // Adding a skew of 1 minute to compensate. | ||
| if (!session.refreshToken) { |
There was a problem hiding this comment.
Also, for readability, rather than nesting these if branches, can you create a top level one, along the lines of
if (!session.refreshToken && accessTokenRequest?.refresh) {
throw new AccessTokenError('no_refesh_token', ...)
}
|
Hi @kanalo-shrek - let me know if you'd like to continue this review, if not - we'll add something to our backlog to fix |
|
Hey @adamjmcgrath haven't had a chance to update your comments, might take this out this week-ish. |
|
Superseded by #624 |
This fixes the Scenario where if
refreshis passed astrueand no refresh token is present there is a silent failureIf
AccessTokenRequestis requested withrefresh: trueand no refresh_token is present in the session, there is no error thrown esp if there is an existing session, this causes a silent error.