Refresh token #4
Conversation
…x some other bugs, might be ready for a merge
Chris927
left a comment
Chris927
left a comment
There was a problem hiding this comment.
The refresh function itself looks good, but we need a timer... see my detailed comments.
src/lib/createAuthContext.js
Outdated
| const now = new Date() | ||
| const elapsed = now.getTime() - new Date(token.expires_at).getTime() | ||
| const slack = 1000 | ||
| if(elapsed > slack){ |
There was a problem hiding this comment.
useToken() is a React hook, which means it gets called on render().
This further means that if we then (at render time) detect that the token is expired, we refresh it. That's basically the right logic, but: refreshing the token is asynchronous, meaning that until the refresh is finished, we will return (in line 46) an expired token.
The contract with the users of this package is, however: useToken() either gives you a valid (non-expired) token, or no token at all. The README.md doesn't say this explicitly, but implicitly:
... This will ensure the user gets authenticated, before
anything wrapped byAuthenticatedgets mounted / rendered. ...
("Authenticated" should mean that there is a valid access token, not an expired one.)
Thus, the implementation here is not good, as it breaks this contract.
The right logic, IMHO, is: right after receiving a valid (and not-yet-expired) token, we set a timer that fires slack milliseconds before the token will expire. When the timer fires, we refresh the token, most likely (hopefully) before the (old) token expires. (slack currently is 1000ms, I believe it should be a little longer, maybe 10 seconds.)
| if (expires_in && Number.isFinite(expires_in)) { | ||
| const slackSeconds = 10; | ||
| // add 'expires_at', with the given slack | ||
| token.expires_at = new Date(new Date().getTime() + expires_in * 1000 - (slackSeconds * 1000)); |
There was a problem hiding this comment.
Ah, I see, you have slack here, too. I wouldn't change the value of expires_at, but rather calculate the correct delay for the timer with the help of slack.
There was a problem hiding this comment.
I am not sure what you mean here, I was basically following how you implemented the expired_at in the fetchToken.js. Maybe I don't fully understand
| client_id: clientId, | ||
| grant_type: "refresh_token", | ||
| scope: "openid, profile", | ||
| refresh_token: token.refresh_token |
There was a problem hiding this comment.
Some auth providers don't provide refresh tokens. Before setting a timer to use the refresh token, we must check if there actually is a refresh token.
…t. I don't fully undersntad why I don't have the token value there.
…tus is 400, check the react-u5auth configuration (wrong provider or token endpoint?) but I do receive new tokens. Investigating the cause. In the file createAuthContext.js in line 53 time interval is going to be (elapsed - slack), i did it this way since I am testing. Also in line 47 its going to be (10000) for slack. Also changed the header content-type and also the body
|
Hello @Chris927 what about this PR ? 😄 |
4 participants
No description provided.