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

Check access token expires before we use #21

Merged
merged 2 commits into from
Jul 15, 2018

Conversation

awarecan
Copy link
Contributor

Need first implement home-assistant/frontend#1444

Part of home-assistant/core#15209

WS will use access token to auth during the reconnect. There are 3 scenario

  1. User has long run server, restart for some reason. Since the access token used in websocket connect only be given in init phase, it probably already expires, then we will first refresh token.
  2. User has an access token to start a connection, and server stopped for 30 mintues, then server started, at that time, token expired, we will refresh token
  3. User is a developer, who restart server very frequency, so the websocket still think access token is not expired, however restart server will invalid all access token, websocket connection will get invalid auth, back end will get invalid auth event as well. To resolve that, we need some sort of session or tag, so that front end knows server restart, it need invalid the access token in localstorage,

After implement, back end log will likes following if access token expires during the reconnect . No ipban message.

2018-07-13 17:14:59 INFO (MainThread) [homeassistant.components.http.view] Serving /api/websocket to 127.0.0.1 (auth: False)
2018-07-13 17:14:59 DEBUG (MainThread) [homeassistant.components.websocket_api] WS 139905478490488: Connected 
2018-07-13 17:14:59 DEBUG (MainThread) [homeassistant.components.websocket_api] WS 139905478490488: Request auth 
2018-07-13 17:14:59 DEBUG (MainThread) [homeassistant.components.websocket_api] WS 139905478490488: Connection closed by client 
2018-07-13 17:14:59 DEBUG (MainThread) [homeassistant.components.websocket_api] WS 139905478490488: Closed connection 
2018-07-13 17:14:59 INFO (MainThread) [homeassistant.components.http.view] Serving /auth/token to 127.0.0.1 (auth: False)
2018-07-13 17:14:59 INFO (MainThread) [homeassistant.components.http.view] Serving /api/websocket to 127.0.0.1 (auth: False)
2018-07-13 17:14:59 DEBUG (MainThread) [homeassistant.components.websocket_api] WS 139905478490320: Connected 
2018-07-13 17:14:59 DEBUG (MainThread) [homeassistant.components.websocket_api] WS 139905478490320: Request auth 
2018-07-13 17:14:59 DEBUG (MainThread) [homeassistant.components.websocket_api] WS 139905478490320: Received access_token 
2018-07-13 17:14:59 DEBUG (MainThread) [homeassistant.components.websocket_api] WS 139905478490320: Auth OK 

@balloob
Copy link
Member

balloob commented Jul 14, 2018

I think that we shouldn't even try to make a connection if we know the access token has expired.

@@ -63,7 +63,13 @@ function getSocket(url, options) {
if (options.authToken) {
socket.send(JSON.stringify(messages.auth(options.authToken)));
} else if (options.accessToken) {
socket.send(JSON.stringify(messages.authAccessToken(options.accessToken)));
if (options.expires !== undefined && Date.now() > options.expires) {
// We know access token expires, no need to try
Copy link
Member

Choose a reason for hiding this comment

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

expires -> has expired

@awarecan
Copy link
Contributor Author

If we don't make connection, we don't know server is up, and then we don't know whether refresh access_token will work.

@balloob
Copy link
Member

balloob commented Jul 14, 2018

But we just show a spinner while refreshing the access token (Like we do when promise is resolving)

Detecting that the server is down can be done by a failing fetch request too.

@balloob
Copy link
Member

balloob commented Jul 14, 2018

I guess for now it's fine but something for the future

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants