Refreshing and revoking token with often repeated requests can cause a 401 error

[Amareis]

So, problem in a nutshell:

[20/Jan/2016 09:50:44] "POST /api/auth/revoke-token/ HTTP/1.1" 200 0 [20/Jan/2016 09:50:44] "POST /api/auth/revoke-token/ HTTP/1.1" 200 0 [20/Jan/2016 09:50:44] "GET /api/v1/disputes/9289bb5f-fd8b-4c56-a5db-bc95810e076e/ HTTP/1.1" 401 59

If you do a lot of frequently repeated requests (for example, once per second, it does not really matter, because the problem will sometimes occur at any frequency), then immediately after the refresh or revoke token next request will sometimes use the old token and get a 401 error. It is not very scary, but I was confused by the red line in the console, so I did, um, a temporary solution aka crutch.

My custom authenticator:

session: Ember.inject.service()

invalidate: (data) ->
  @set('session.data.authenticated.access_token', '')
  @_super(data)

This code removes the problem of revocation a token, because the standard authorizer verifies that the token is not empty.

Fix refresh token is more difficult - I had to make a synchronous request to refresh, that suits me, but may be unacceptable for other users.

[marcoow]

Concurrent token refresh requests are actually a known problem (although it is unclear how that could be fixed). I'm not sure what the problem with token revocation is though. Can you explain in a bit more detail?

[marcoow]

@atonse: I guess to make the current implementation a bit more stable (and the failure scenario less likely to happen) we could maybe use the visibility API somehow so that refresh requests would only be made from the visible tab/window or so.

[atonse]

@marcoow I've done some research in using localStorage as a bit of shared state across multiple tabs, using visibility will help to assign a "primary" and maybe ensure that only the visible instance refreshes tokens. Thoughts?

[marcoow]

Yes that would be the idea. Of course that only make multiple requests being sent less likely…

[atonse]

Yes and you also have to handle the case where none of your application tabs are active.

[rhyek]

I have implemented the following solution for my project and so far my tests are proving positive. Basically, the idea is that each tab generates a random v4 UUID and whichever tab is first to set it's own UUID in the localStorage, that tab alone will make all the token refresh requests until it is closed/refreshed. Then another tab will take the responsibility.

// /authenticators/application.js
import Ember from 'ember';
import OAuth2PasswordGrant from 'ember-simple-auth/authenticators/oauth2-password-grant';
import UUID from 'npm:node-uuid';

const refreshTokenTabKey = 'refresh-token-tab';
const uuid = UUID.v4();

export default OAuth2PasswordGrant.extend({
    init() {
        this._super(...arguments);
        window.onbeforeunload = e => {
            let refreshTokenTab = localStorage.getItem(refreshTokenTabKey); 
            if (refreshTokenTab === uuid) {
                localStorage.setItem(refreshTokenTabKey, '');
            }
        };
    },
    _refreshAccessToken(expiresIn, refreshToken) {
        if (Ember.isEmpty(localStorage.getItem(refreshTokenTabKey))) {
            localStorage.setItem(refreshTokenTabKey, uuid);
        }
        if (localStorage.getItem(refreshTokenTabKey) === uuid) {
            this._super(...arguments);
        }
    }
});

Thoughts? onbeforeunload is fired when closing or reloading a tab. I guess a case where this wouldn't work is if the tab currently doing the refreshes crashed for some reason.

Edit: I guess a way to handle the crash scenario is to reset refresh-token-tab during initial authentication with username/password. I'll have to run some tests.

[rhyek]

I modified the above code and came up with the following:

import Ember from 'ember';
import OAuth2PasswordGrant from 'ember-simple-auth/authenticators/oauth2-password-grant';
import UUID from 'npm:node-uuid';

const refreshTokenTabKey = 'refresh-token-tab';
const shouldInvalidateRefreshTokenTabKey = 'should-invalidate-refresh-token-tab';
const uuid = UUID.v4();

export default OAuth2PasswordGrant.extend({
    session: Ember.inject.service(),
    init() {
        this._super(...arguments);
        Ember.Logger.debug(`uuid for ${refreshTokenTabKey}:`, uuid);
        window.onbeforeunload = e => {
            let refreshTokenTab = localStorage.getItem(refreshTokenTabKey);
            if (refreshTokenTab === uuid) {
                localStorage.setItem(refreshTokenTabKey, '');
            }
        };
        window.addEventListener('storage', e => {
            if (e.key === shouldInvalidateRefreshTokenTabKey) {
                let refreshTokenTab = localStorage.getItem(refreshTokenTabKey);
                let contesting = eval(e.newValue);
                if (contesting && refreshTokenTab === uuid) {
                    localStorage.setItem(shouldInvalidateRefreshTokenTabKey, false);
                    Ember.Logger.debug('Avoided invalidation of refresh tab.');
                } else if (!contesting && refreshTokenTab !== uuid) {
                    if (this.contestTimeout !== null) {
                        clearTimeout(this.contestTimeout);
                        this.contestTimeout = null;
                        Ember.Logger.debug('Refresh token tab contest aborted.');
                    }
                }
            }
        });
    },
    contestTimeout: null,
    tryRegisterAsRefreshTokenTab() {
        let success = false;
        if (this.get('session.isAuthenticated')) {
            let refreshTokenTab = localStorage.getItem(refreshTokenTabKey); 
            if (refreshTokenTab !== uuid) {
                if (Ember.isEmpty(refreshTokenTab)) {
                    localStorage.setItem(refreshTokenTabKey, uuid);
                    localStorage.setItem(shouldInvalidateRefreshTokenTabKey, false);
                    success = true;
                }
                Ember.Logger.debug(`Trying to register as refresh token tab... ${success ? 'SUCCESS' : 'FAILED'}.`);
            }
        }
        return success;
    },
    _refreshAccessToken(expiresIn, refreshToken) {
        this.tryRegisterAsRefreshTokenTab();
        if (localStorage.getItem(refreshTokenTabKey) === uuid) {
            this._super(...arguments);
        } else {
            Ember.Logger.debug('Contesting refresh tab...');
            this.contestTimeout = setTimeout(() => {
                let shouldInvalidate = eval(localStorage.getItem(shouldInvalidateRefreshTokenTabKey)); 
                if (shouldInvalidate) {
                    localStorage.setItem(refreshTokenTabKey, '');
                    Ember.Logger.debug('Will invalidate.');
                    if (this.tryRegisterAsRefreshTokenTab()) {
                        this._refreshAccessToken(expiresIn, refreshToken);
                    }
                } else {
                    Ember.Logger.debug('Will not invalidate.');
                }
            }, 500);
            localStorage.setItem(shouldInvalidateRefreshTokenTabKey, true);
        }
    }
});

So, basically as before, every tab runs _refreshAccessToken when scheduled. If the tab is currently registered as the refresh token tab, it will continue. What I'm doing now is when the tab sees it's not the registered one, it will try and contest the responsibility by setting should-invalidate-refresh-token-tab to true and checking it one second later. The registered tab will see this localStorage event, and set that value to false, thus avoiding losing the responsibility.

If the registered tab crashed, 'should-invalidate-refresh-token-tab will remain true and a new tab will now take over.

Thoughts?

Edit: Few changes.

[rhyek]

An issue with this is that if the registered tab fails to refresh the token due to a connection error, then restore() won't be called on the other tabs, therefore no schedule will be made for _refreshAccessToken on them and the session will be left in an invalid state.

Currently, if one tab fails, another might be successful if connectivity is regained in the following seconds before a different tab runs _refreshAccessToken. If something is done about #980 this would not be a problem.

[rhyek]

I have a created a gist with my complete oauth2 authenticator in case it helps anyone. Features are:

• Ensure only one tab/window is handling token refreshes to avoid 400/401 errors due to concurrent requests and lowering bandwidth used.
• If a token refresh is rejected with 400/401 http status codes, invalidate the session.
• Attempt token refreshes when 90% of the access token's lifetime has transpired, leaving the remaining 10% as an opportunity to retry the refresh in case of connection failures.
• If a token refresh fails due to a connection error, keep trying every 5 seconds until the current access token's lifetime has expired, in which case, the session will be invalidated.

[marcoow]

@rhyek: your solution might actually work. I think that should be tested thoroughly though. Also I don't think retrying token refresh requests is within the scope of ESA. The refresh request failing because of a connection problem is only one valid reason for a failed token refresh from ESA's perspective.

@atonse: would you want to work with @rhyek on validating his tab sync mechanism?

Also the tab sync mechanism itself could actually be sth. that might have more value as a separate library that ESA uses instead of tying it to the OAuth2PasswordGrant authenticator.

[rhyek]

@marcoow: I agree that this requires a lot of testing. I'm not sure if automated testing for this is possible? I would certainly like to try. I've been doing it manually for about a week and have had no issues so far. I would love to discuss this further with @atonse.

I like your idea about making the tab syncing mechanism a separate library. I hadn't really thought about it. It would be an ember addon, most likely. I may have to mess around with it soon.

I personally believe that retrying the token refresh after failures should be a part of ESA for a few reasons:

• Failed refreshes due to any sort of error leaves the application in an invalid state with no feedback to the user.
• Any application that wishes to implement this behavior would have very similar code, and although not every developer would take the time to do it or even be aware he needs to, every instance of Ember coupled with ESA is prone to suffer this issue if an unstable internet connection is involved, or maybe an overloaded server, etc.
• Implementing the solution on the application level would not be optimal because there are several intricacies of ESA that are hidden from the developer.
• The retries could be triggered not only on connection failures, but any failure that is not 400/401. A server could be responding with a 500 error due to a temporary memory issue, or something along those lines.
• There could be an option to opt out of the functionality and it should be mentioned in the documentation.
• Implementing this in ESA would add more value to it overall and it would be one less thing developers have to worry about, keeping ESA "simple".

Anyways, my 2 cents. Thanks.

[marcoow]

@marcoow: I agree that this requires a lot of testing. I'm not sure if automated testing for this is possible? I would certainly like to try. I've been doing it manually for about a week and have had no issues so far. I would love to discuss this further with @atonse.

great!

Any application that wishes to implement this behavior would have very similar code, and although not every developer would take the time to do it or even be aware he needs to, every instance of Ember coupled with ESA is prone to suffer this issue if an unstable internet connection is involved, or maybe an overloaded server, etc.

That's one of the reasons I wouldn't want to add the function to ESA - at least not right now. All applications wanting to implement that behavior will have similar code that will actually be quite different wrt the little details. I think we should see how different apps implement the functionality on the application level and then maybe add support for it in ESA once we have enough knowledge to build sth. that would work for all or at least most apps.

[rhyek]

That's one of the reasons I wouldn't want to add the function to ESA - at least not right now. All applications wanting to implement that behavior will have similar code that will actually be quite different wrt the little details.

That's actually a very valid point, and you're right. Hmm, maybe getting a discussion going with an RFC wouldn't be a terrible idea.

We're getting off-topic here, but here's a thought: a great way to have the application handle this is something I mentioned earlier which is have the authenticator trigger an event when a refresh fails and pass a few arguments to handlers such as:

• the jqhxr object
• refresh token
• access token expiresIn and expiresAt

If you have that and make _refreshAccessToken public, developers would have all the tools they need to get something going. An application route could register a handler for the event and once it fires get a timer going to periodically check for connectivity, or just wait a bit and call _refreshAccessToken again.

I know you've mentioned earlier about making that method public, but the event would be necessary too, I think.

[marcoow]

That's actually a very valid point, and you're right. Hmm, maybe getting a discussion going with an RFC wouldn't be a terrible idea.

RFC sounds great.

[rhyek]

Hi, @marcoow, @atonse. I just published the addon we talked about here. I cleaned up the authenticator quite a bit (temporarily removing the handling of status errors) and it now looks like this:

import Ember from 'ember';
const { RSVP, run, assign, computed } = Ember;
import OAuth2PasswordGrant from 'ember-simple-auth/authenticators/oauth2-password-grant';

export default OAuth2PasswordGrant.extend({
  masterTab: Ember.inject.service(),

  tokenRefreshOffset: computed(function() {
    return 0;
  }).volatile(),

  _refreshAccessToken(expiresIn, refreshToken) {
    return new RSVP.Promise((resolve, reject) => {
      this.get('masterTab')
        .lock('refresh-token', () => { // will only run on the master tab
          const data                = { 'grant_type': 'refresh_token', 'refresh_token': refreshToken };
          const serverTokenEndpoint = this.get('serverTokenEndpoint');
          return this.makeRequest(serverTokenEndpoint, data).then((response) => {
            expiresIn       = response['expires_in'] || expiresIn;
            refreshToken    = response['refresh_token'] || refreshToken;
            const expiresAt = this._absolutizeExpirationTime(expiresIn);
            const data      = assign(response, { 'expires_in': expiresIn, 'expires_at': expiresAt, 'refresh_token': refreshToken });
            run(() => {
              this._scheduleAccessTokenRefresh(expiresIn, null, refreshToken);
              this.trigger('sessionDataUpdated', data);
              resolve(data);
            });
            return JSON.stringify(data); // will be consumed by slave tabs
          }, (xhr, status, error) => {
            Ember.Logger.warn(`Access token could not be refreshed - server responded with ${error}.`);
            reject();
          });
        })
        .wait(
          result => { // success on master tab
            const data = JSON.parse(result); // returned from master tab
            this._scheduleAccessTokenRefresh(data['expires_in'], data['expires_at'], data['refresh_token']);
            resolve(data);
          },
          () => { // failure on master tab
            reject();
          }
        );
    });
  },
});

I would love to hear your feedback on this. Thanks.

[marcoow]

@rhyek: sorry for the (extremely) late reply 😊

I think something like your addon would most likely work. I'm not sure this actually belongs into ESA at all really as it seems like we'd be bloating the purpose of this particular library massively. If we decided to add something like that we'd also have to have a solution for cookies (as we'd want to support the case where localStorage isn't available as well). At this point I'm thinking whether we should simply treat that as a known limitation and make it easy to add something like your addon if necessary (e.g. by overriding the _refreshAccessToken method which should then not be private of course).

[marcoow]

Thinking further about this I feel only refreshing tokens from visible tabs might actually be a simple and small improvement that would make the problem pretty much irrelevant for most people?

We already rely on the visibility API in the code that periodically increments the lifetime of the session cookie and this should actually be a pretty simple change.

@atonse, @rhyek: what do you think?