Skip to content

Only do 1 token update a time#7236

Merged
bramkragten merged 2 commits intodevfrom
do-1-token-update
Oct 6, 2020
Merged

Only do 1 token update a time#7236
bramkragten merged 2 commits intodevfrom
do-1-token-update

Conversation

@bramkragten
Copy link
Member

Proposed change

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

@bramkragten bramkragten merged commit ede9931 into dev Oct 6, 2020
@bramkragten bramkragten deleted the do-1-token-update branch October 6, 2020 08:55
@zacwest
Copy link
Member

zacwest commented Oct 6, 2020

If multiple happen and force is true in any of them, won't that cause the same issue where the external bus call itself is only able to track one at once?

@bramkragten
Copy link
Member Author

Why would there be multiple force requests? I think it is only used when loading the app and hitting a wrong token?

@zacwest
Copy link
Member

zacwest commented Oct 6, 2020

The problem scenario I'm imagining is something like…

  1. refresh, force=false
  2. refresh, force=false
  3. refresh, force=true

would cause 1 and 2 to be lost because of how the callback works in the external bus, if I am understanding that side of things correctly.

@bramkragten
Copy link
Member Author

I don't think that scenario will ever happen, I'll check the code in a sec

@bramkragten
Copy link
Member Author

We only do a force when we load the core and get an invalid auth. The force parameter should trigger the reauth flow in the app, as if it was not authenticated. Since we are not connected at that point, it is not possible that a force=false request is made.

https://github.com/home-assistant/frontend/blob/dev/src/entrypoints/core.ts#L66

@zacwest
Copy link
Member

zacwest commented Oct 6, 2020

My read of that (and I think what I have seen) is that we may use an invalid-due-to-expired auth token and that triggers the forced flow, so it's not just logout events, or am I wrong about that auth error's cases?

@bramkragten
Copy link
Member Author

this is not a logout event, it is when the apps starts, and the auth token is no longer valid (removed?).

When the app starts we connect to HA and try to get the access token (without force) from the app, if that fails with an invalid auth error, we will call refresh access token with force. That means all previous promises can be discarded as we will re-do the entire auth (login page). After the re-login of the user, we will try to create the connection again.

@bramkragten bramkragten mentioned this pull request Oct 21, 2020
@chriss158
Copy link
Contributor

chriss158 commented Nov 18, 2020

Every now and then i can't get a reconnection to my home assistant after the connection is broken (ex. duo to a home assistant restart) with the android app. The frontend just shows the message "Connection lost. Reconnecting..." for ever. I had never problems with HA releases before this PR.

Did some debugging the last couple of days and tracked down the problem to this PR.

I think this is reproducible with following steps:

  1. Deactivate "Close connection automatically" in the profile settings of HA.
  2. Open the android app and let it connect to home assistant
  3. Lock the phone/tablet leaving the app open and in foreground
  4. Wait around 30 minutes (I think this is the session timeout for the token)
  5. Restart HA and wait till it is running fully again
  6. Unlock the phone/tablet.

While HA is restarting the frontend tries to reconnect in background.

  1. RefreshAccessTokenis called

    public async refreshAccessToken(force?: boolean) {

  2. RefreshAccessTokencalls getExternalAuth

    window.externalApp.getExternalAuth(JSON.stringify(payload));

  3. getExternalAuth triggers the onGetExternalAuth of the android app
    https://github.com/home-assistant/android/blob/6e13b162144cfeed69d0d37f050098d2b16ebf6c/app/src/main/java/io/homeassistant/companion/android/webview/WebViewPresenterImpl.kt#L82

  4. The method call view.setExternalAuth will fail because Home assistant is not yet restarted.

  5. The catch block is triggered and calls the following method:
    https://github.com/home-assistant/android/blob/6e13b162144cfeed69d0d37f050098d2b16ebf6c/app/src/main/java/io/homeassistant/companion/android/webview/WebViewPresenterImpl.kt#L88

  6. Now the _tokenCallbackPromise is trigged

    this._tokenCallbackPromise = new Promise<RefreshTokenResponse>(

    with success = false. Meaning the _tokenCallbackPromise is in a rejected state.

  7. Now after one second the next try for a reconnect starts.
    https://github.com/home-assistant/home-assistant-js-websocket/blob/1836b3ae57309a969627fc993619cfd40b47204c/lib/socket.ts#L84

  8. Again RefreshAccessToken is called because of expired session token. This time from the reconnection instead of the app https://github.com/home-assistant/home-assistant-js-websocket/blob/1836b3ae57309a969627fc993619cfd40b47204c/lib/socket.ts#L87

  9. Then this check is called:

    if (this._tokenCallbackPromise && !force) {
    _tokenCallbackPromise is not undefined and force is undefined because the parameter is not defined by the caller. Then the _tokenCallbackPromise is called. And here is the culprint: tokenCallbackPromise is in a rejected state with undefined data. The rejected promise triggers an exception.

  10. The exception is then handled here:
    https://github.com/home-assistant/home-assistant-js-websocket/blob/1836b3ae57309a969627fc993619cfd40b47204c/lib/socket.ts#L90
    This triggers that the websocket will be closed again.

  11. Then the reconnection process is beginning from the start. And we are in a reconnect loop.

On the home assistant side there are following messages. Which explains this process above.

2020-11-16 17:31:00 DEBUG (MainThread) [homeassistant.components.websocket_api.http.connection.140600942755264] Connected from ipaddress
2020-11-16 17:31:00 DEBUG (MainThread) [homeassistant.components.websocket_api.http.connection.140600942755264] Sending {'type': 'auth_required', 'ha_version': '0.117.6'}
2020-11-16 17:31:00 DEBUG (MainThread) [homeassistant.components.websocket_api.http.connection.140600942755264] Disconnected

Long story short

Maybe here

if (this._tokenCallbackPromise && !force) {

should be a check if the promise is rejected. If it is reject then do not return the reject promise. Instead do the stuff below (call getExternalAuth etc.) If the promise is resolved, then return the promise.

I hope i could describe the problem understandable.

Best regards
Chris

@bramkragten
Copy link
Member Author

Hi Chris, thanks for your lengthy tracking, but could you create an issue for this instead of commenting on a merged PR? Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Multiple external auth attempts occur simultaneously, making the first get lost

5 participants