Ember-simple-auth: Refresh token fired twice when used with cookie store

[lucasm-iRonin]

Hi!
Thanks for the add-on, it's really good and saves a ton of work.

I've found a small issue when cookie storage is used with token expiry. This can be reproduced by opening the app after the token expired (and request to refresh the token is fired at the beginning).

As you can see in the cookie#init there is a call to this._syncData() in the next run. However internal-session calls restore on the store (which also calls this._syncData() internally).

Basically two calls to _syncData() are made which results in firing token refresh twice. The first one will succeed but the second one will be rejected (if refresh tokens are one time only).

We fixed that with dirty hack:

# app/session-stores/application.js

import Cookie from 'ember-simple-auth/session-stores/cookie';

export default Cookie.extend({
  init() {
    // NOTE:
    // Override original init in order not to call _syncData.
  },
});

but it's not good solution.

Is the _syncData() call needed in the cookie store init?

[marcoow]

This is actually a known bug (see #861) that doesn't directly relate to the cookie store but is caused by the fact that there's no way to synchronize code between multiple browser tabs or windows. Please follow the discussion in #861 for more details.

[lucasm-iRonin]

@marcoow sorry for replying here but this issue is not related to synchronizing code between multiple browser tabs.

Here is what's going on:

1. internal-session calls restore on the cookie store, reads the data and then call restore on the authenticator. When token is expired the authenticator tries to refresh the token.
2. In the next run sessionDataUpdated event is raised (from cookie#init) with old cookie data resulting in calling restore in the authenticator again (trough internal-session object which listens on that event) which calls authenticator#restore one more time.

Does it make sense now?

I'm sorry my initial description was not clear.

[marcoow]

Right, that sounds like an actual bug - totally fine to reply here!

[lucasm-iRonin]

@marcoow do you think that the problem could be potentially solved by modifying internal-session so that, the call to authenticator.restore will be queued and processed one by one? This way we would avoid calling it in parallel (for the same expired data), which causes token refresh request being fired twice.

[marcoow]

I think the short term fix for this is to ignore sessionDattaUpdated events from the store while the session is still restoring. The long term fix will be #916.

[marcoow]

@lucasm-iRonin: can you check whether #965 solves it for you?

[lucasm-iRonin]

@marcoow thank you for the PR. I will try to check it over a weekend and let you know.

[marcoow]

@lucasm-iRonin: any updates on this? Would also be happy to take a look at your app or maybe you can setup a demo project illustrating the bug?

[lucasm-iRonin]

@marcoow sorry for not giving any updates on this - had busy weekend I haven't time to check it.
I will check it by the end of this weekend and give you feedback.

Sorry once again and thank you for your response to the issue.

[lucasm-iRonin]

Hey @marcoow,
Sorry for late response. I've just checked it and it fixes the issue.

Unfortunately I'm not able to provide the source code of the but I can explain you how it can be tested:

1. Create basic Rails app with Doorkeeper gem
2. In Doorkeeper initializer set token expiry to 30 seconds or less (access_token_expires_in 30.seconds)
3. Open the Ember app, sign in and close the tab
4. After 30 seconds (assuming you set timeout to 30 seconds in step 2) open the app again

You will see that on the master branch there will be 2 requests sent to refresh the token. First one will succeed, second one will fail (Doorkeeper provide only one-time refresh tokens). On this branch, only one request is sent.