Check access token expires before we use it. #1444
Conversation
ghost
added
the
awarecan
force-pushed
the
check-access-token-expire
branch
from
3683af7 to
f14338b
Compare
…expires, we proactive call refresh token.So that we can avoid uncessary invalid auth error in backend.
awarecan
force-pushed
the
check-access-token-expire
branch
from
f14338b to
420ece7
Compare
src/entrypoints/core.js
Outdated
| @@ -39,12 +40,19 @@ function redirectLogin() { | |||
| window.refreshToken = () => | |||
| refreshToken_(clientId(), window.tokens.refresh_token).then((accessTokenResp) => { | |||
| window.tokens.access_token = accessTokenResp.access_token; | |||
| // shorten access token life 30 seconds to cover network cost | |||
| window.tokens.expires = ((accessTokenResp.expires_in - 30) * 1000) + Date.now(); | |||
There was a problem hiding this comment.
No need to calculate, it is returned in the token request
There was a problem hiding this comment.
returned expires_in is seconds, I want to covert to timestamp for easy comparison down the road
There was a problem hiding this comment.
oh my bad, I thought you were calculating 30 min yourself, this seems k. You can just pass it on to the constructor new Date((accessTokenResp.expires_in - 30) * 1000)
There was a problem hiding this comment.
We should however move this calculation to the refreshToken / fetchToken methods.
There was a problem hiding this comment.
new Date((accessTokenResp.expires_in - 30) * 1000)
It will return 1970-01-01 00:29:30
src/entrypoints/core.js
Outdated
| window.hassConnection = init(null, window.tokens.access_token).catch((err) => { | ||
| if (err !== ERR_INVALID_AUTH) throw err; | ||
| if (window.tokens.expires === undefined) { | ||
| window.tokens.expires = Date.now() - 1; |
There was a problem hiding this comment.
For migration. The refresh token in local storage from previous version may not have expires field.
There was a problem hiding this comment.
This is the kind of thing that would be good to have a comment added 👍 Also add a date so that we can remove it in the future.
src/entrypoints/core.js
Outdated
| if (window.tokens.expires === undefined) { | ||
| window.tokens.expires = Date.now() - 1; | ||
| } | ||
| if (Date.now() > window.tokens.expires) { |
There was a problem hiding this comment.
We don't need to do this check if we will do this inside home-assistant-js-websocket before making a connection.
There was a problem hiding this comment.
That reflects your comment in home-assistant-js-websocket, we should not try to connect if we know token has expired. The check in home-assistant-js-websocket is for reconnect, since reconnect is managed by home-assistant-js-websocket
src/common/auth/fetch_token.js
Outdated
| return resp.json(); | ||
| const tokens = resp.json(); | ||
| // shorten access token life 30 seconds to cover network cost | ||
| tokens.expires = ((tokens.expires_in - 30) * 1000) + Date.now(); |
There was a problem hiding this comment.
Let's not shorten it 30 seconds here, it's up to the pace that uses it to decide this.
Also, please pass the value to the constructor of Date.now() instead of depending on type conversion.
There was a problem hiding this comment.
Also, please pass the value to the constructor of Date.now() instead of depending on type conversion.
I didn't understand this point, Date.now() returns number of milliseconds since 1 January, 1970, 00:00:00, UTC. IMO it is perfect to store it for comparison usage, we don't need Date object here.
There was a problem hiding this comment.
Let's not shorten it 30 seconds here
The idea for this change is to avoid unnecessary invalid auth error, If we don't shorten it, it is highly possible the server will expires token earlier than client. If you feel 30 seconds is too long, we can reduce it to 10 or 5.
There was a problem hiding this comment.
Oh, I didn't realize Date.now() returns an integer. My bad.
It is up to the place that checks for expired to decide the offset. So this 30 seconds adjustment should happen inside home-assistant-js-websocket.
If we found access token expires, we proactive call refresh token.
So that we can avoid uncessary invalid auth error in backend.
It is part of work for home-assistant/core#15209