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

Timed out access_token is not handled gracefully on page load #102

Closed
dschmidt opened this issue Mar 7, 2014 · 24 comments
Closed

Timed out access_token is not handled gracefully on page load #102

dschmidt opened this issue Mar 7, 2014 · 24 comments

Comments

@dschmidt
Copy link
Contributor

dschmidt commented Mar 7, 2014

Hey,

I was wondering why my timed out access_token was not refreshed on page load or my session was at least invalidated (granted I'm using a custom authentication provider but it's based on the upstream OAuth2 implementation).

I think the expiresAt > now check here
https://github.com/simplabs/ember-simple-auth/blob/master/packages/ember-simple-auth/lib/authenticators/oauth2.js#L151
does not make sense because according to
http://tools.ietf.org/html/rfc6749#section-4.2.2
http://tools.ietf.org/html/rfc6749#section-5.1
expires_in defines the lifetime of the access_token not of the refresh_token, so it might very well be possible to request a new access_token even if the old one is timed out because the refresh_token is still valid.

Afaict trying to refresh with an invalid refresh_token would trigger a logout anyway.
If you still don't want to refresh the access_token in this case, you should consider to at least invalidate the session.

P.S.: A lot of people seem to use a refresh_token_expires_in value in their server responses to mark how long the refresh_token is valid. It's not part of the standard afaik tho.

@marcoow
Copy link
Member

marcoow commented Mar 8, 2014

I think you're right; a reasonable behavior would probably be to refresh the access_token immediately on #restore when a refresh_token is present in the data to restore from and expiresAt is < now and then to resolve the promise with the updated access_token.

@marcoow marcoow added this to the next milestone Mar 8, 2014
@marcoow
Copy link
Member

marcoow commented Mar 10, 2014

There's one potential problem though - when a browser window is reopened that has multiple tabs of the Ember.js app open, all of the authenticators will try to restore the session resulting in a race condition again (all authenticators will refresh the token potentially receive different tokens that override each other etc.) I guess for now I'd just reject restoration when there's an expiresAt that's in the past in the restored session data.

Of course the same is true for regular token refreshes - the server gets one token refresh request per open tab/window as there's no master or so.

@marcoow marcoow added the bug label Mar 10, 2014
@dschmidt
Copy link
Contributor Author

Not really feasible for reopening the browser with an outdated token, but at least for several open tabs when the token is timing out, you could try to reduce the likelihood of a race condition again by not refreshing on timeout but $random seconds before, with a different $random value in each tab maybe?

@marcoow
Copy link
Member

marcoow commented Mar 14, 2014

Sure that would reduce the likelihood. The problem is that this is a relatively long running operation as it includes a server request so you would have large differences between the random timeouts so there's an actual chance that one tab already finished updating the token before another one tries to start updating it. Thus that random time would increase quite fast with the number of tabs and as you don't know that number you have to assume it's high.

Actually thinking about dropping the feature as I think it might cause more trouble than it brings good.

Am 14.03.2014 um 05:19 schrieb Dominik Schmidt [email protected]:

Not really feasible for reopening the browser with an outdated token, but at least for several open tabs when the token is timing out, you could try to reduce the likelihood of a race condition again by not refreshing on timeout but $random seconds before, with a different $random value in each tab maybe?


Reply to this email directly or view it on GitHub.

@KeKs0r
Copy link

KeKs0r commented Mar 22, 2014

I am running across the same issue and wanted to address it. I think the random value would be a good shot at it. But I am not an ember pro, so maybe there are options to "sync" the status between tabs?

Nevertheless, the current situation is kind of unsatisfying, because my frontend says that a person is authenticated but every request to the backend is unauthorized. I think just refreshing the access token after restore would be better than nothing.

@marcoow
Copy link
Member

marcoow commented Mar 24, 2014

Still haven't figured out what the best way to fix this would be; for now I'd suggest to simply not use automatic token refresh.

Also if the authenticator erroneously resolves on restore while the access token is actually expired, the first request the app makes to the server should trigger the authorizationFailed action that immediately invalidates the session (if the server responds correctly with status 401).

@KeKs0r
Copy link

KeKs0r commented Apr 1, 2014

I am not to sure how ember apps in different tabs are "synchronized" anyways. Can they talk to each other? If so the first tap could set a property under the authenticator which is changed during refreshing the token. And which is set back afterwards. And every tap looks at this property and checks if it is set already, if not it triggers a refresh, otherwise it just waits.

I am currently still trying to implement the automatic refresh after restore when the token is expired, since I want to build a mobile app, where I know that my user has only one window / tab. And even in this scenario it is very difficult. This is my current approach but it is still not 100% working. It is working that the refresh is triggered, but it stays in deferred readiness. I might have implemented it correctly, but its hardly to plug this behaviour into the component.

Update: FYI now the code itself is working, unfortunately it might be that my refresh token is invalid. In that the authentication fails. My subsequent Ember-Data Request returns a 401. I thought after 401 responses the session is automatically invalidated, which does not seem to happen.

GambifyApp.BackendAuthenticator = Ember.SimpleAuth.Authenticators.OAuth2.extend({

      serverTokenEndpoint: '/'+GambifyApp.config.backend_base + 'oauth/v2/token',

      makeRequest: function(data) {
        // data.client_id = window.ENV.OAUTH_ID;
        // 1_3byk9vldpooww4c8skw4c8wwk08cw84sgkksccc8c8go88cws8
        data.client_id = GambifyApp.config.backend_client_id;
        data.client_secret = GambifyApp.config.backend_client_secret;


        return this._super(data);
      },

      restore: function(properties){
          var _this = this;
          console.log('GambifyApp.BackendAuthenticator:restore');
          console.log(properties);
          if(properties.expires_at < Date.now()){
              console.log('token is expired');
              GambifyApp.deferReadiness();
              this.set('isRestoring', true);
              console.log('isRestoring was set to:');
              console.log(this.get('isRestoring'));
              this.refreshAccessToken(3500, properties.refresh_token);
          }
          return this._super(properties);
      },

    /**
     @method refreshAccessToken
     @private
     */
    refreshAccessToken: function(expiresIn, refreshToken) {
        var _this = this;
        var data  = { grant_type: 'refresh_token', refresh_token: refreshToken };
        this.makeRequest(data).then(function(response) {
            if(_this.get('isRestoring')){
                Ember.Logger.info('Restore Request done');
                GambifyApp.advanceReadiness();
                _this.set('isRestoring', false);
            }
            Ember.Logger.info('is Restoring is not set:'+_this.get('isRestoring'));
            Ember.run(function() {
                expiresIn     = response.expires_in || expiresIn;
                refreshToken  = response.refresh_token || refreshToken;
                var expiresAt = _this.absolutizeExpirationTime(expiresIn);
                _this.scheduleAccessTokenRefresh(expiresIn, null, refreshToken);
                _this.trigger('ember-simple-auth:session-updated', Ember.$.extend(response, { expires_in: expiresIn, expires_at: expiresAt, refresh_token: refreshToken }));
            });
        }, function(xhr, status, error) {
            var response = xhr.responseJSON;
            if(_this.get('isRestoring')){
                Ember.Logger.warn('error advancereadiness');
                GambifyApp.advanceReadiness();
                _this.set('isRestoring', false);
            }
            Ember.Logger.warn('Access token could not be refreshed - server responded with ' + error + '. ('+response.error+')');
        });
    }
});

@marcoow
Copy link
Member

marcoow commented Apr 2, 2014

Ember apps in different tabs/windows are not synchronized nor can they talk to each other. The synchronization of the session's authentication status (and only that) is sth. provided by Ember.SimpleAuth (via the session store).

Regarding your code: you should never modify your application's readiness state in an authenticator. Also the session doesn't have to be restored successfully in order for your application to be ready - that's sth. many people get wrong. I think the best way to fix the bug with the outdated access token we're discussing here would be to return a promise from the refreshAccessToken method that resolves when the refresh was successful. Then you could simply do sty. like this in the restore method:

restore: function(properties){
  var _this = this;
  return new Ember.RSVP.Promise(function(resolve, reject) {
    if(properties.expires_at < Date.now() && !Ember.isEmpty(properties.refresh_token)) {
      this.refreshAccessToken(properties.expires_in, properties.refresh_token).then(function(properties) {
        if (!Ember.isEmpty(properties.access_token)) {
          resolve(properties);
        } else {
          reject();
        }
      }, function() {
        reject();
      });
    } else {
      if (!Ember.isEmpty(data.access_token)) {
        resolve(properties);
      } else {
        reject();
      }
    }
  });
}

However, the 2nd problem with the multiple refresh requests I still have no (reliable) solution for.

@KeKs0r
Copy link

KeKs0r commented Apr 2, 2014

The reason why I am deferring Readiness is that as soon as my route is triggered the request is going to the backend. If I don't have the token yet my model hook returns with 401. I just want to wait until I am authorized in order to avoid sending multiple requests.

Also the session doesn't have to be restored successfully in order for your application to be ready - that's sth. many people get wrong. 

I would now know how else to get around having a unauthorized request from my model hook.

Regarding your function, that was one of my main issues that refreshAccessToken is not yet returning a promise. Thats why I am pluggin into makeRequest.

And the syncing issue, is it possible to put some kind of lock into the session store? And if it is set, the tab is not trying to refresh the token?

@marcoow
Copy link
Member

marcoow commented Apr 2, 2014

I think that's more a conceptual problem then - you simply cannot be sure that the session is authenticated on your initial route. Instead you could add some functionality that automatically transitions when it becomes authenticated or so.

If you want to supply a pull request that changes refreshAccessToken so it returns a promise that would be great of course!

The syncing issue is really a pain - locking doesn't really work with the store mechanisms (as there are no atomic read-updates); also I don't want the authenticator to have access to the store. Besides that locking also causes some problems as you also have to make sure you remove that lock once the user closes the tab etc. I thought about solutions for some time and couldn't come up with sth. that would be reliable.

@marcoow
Copy link
Member

marcoow commented Apr 2, 2014

I added randomization of token refresh requests here: 2de80a9; that's probably the best the library can do

@marcoow
Copy link
Member

marcoow commented Apr 7, 2014

will be released with 0.3.0

@marcoow marcoow closed this as completed Apr 7, 2014
@basz
Copy link

basz commented May 21, 2015

I think I have devised a workaround the refresh token race issue - unfortunately not without a server

In a custom authenticator I create an uuid (random v4) when the authentication method receives a token, so that is then stored onto the session. When refreshing the token I send that uuid along, special REST endpoint PATCH /refresh-session/uuid.

This endpoint accepts this uuid and creates a hash from the uuid and refresh token. After sending the refreshed token it stores a flag in a shortlived memcached under that hash key.

Whenever a second or third window tries to refresh with the same uuid and refresh_token the server simply ignores the request (send an HTTP 429 response).

@marcoow
Copy link
Member

marcoow commented May 21, 2015

@basz: this should actually long be fixed. Are you on the latest version of Ember Simple Auth actually?

@basz
Copy link

basz commented May 21, 2015

yes, think so '0.8.0-beta.2'

does that fix entails the random subtraction from the refresh timeout as mentioned previously in this issue? Could be that my local server is just really slow (~>5 seconds per request).

@basz https://github.com/basz: this should actually long be fixed. Are you on the latest version of Ember Simple Auth actually?

@marcoow
Copy link
Member

marcoow commented May 21, 2015

Yes, the fix also includes randomization of refresh to decrease the likelihood of multiple refresh requests.

@basz
Copy link

basz commented May 21, 2015

So with a fast responding server this issue should -almost- never arrise?

@marcoow
Copy link
Member

marcoow commented May 21, 2015

exaclty; also if the situation should actually arise it's not a hard problem as you're only refreshing the token more often than necessary but don't log the user out or so.

@basz
Copy link

basz commented May 21, 2015

I’m under the impression that when a server refuses to refresh a token (because the refresh_token has expired) it should logout asap?

On the other hand, any calls to any Bearer token protected endpoints would do that anyway.

@marcoow
Copy link
Member

marcoow commented May 21, 2015

The problem that the random delay tries to solve is that when you have multiple tabs of the same application open and the access token expires all the tabs would send a request to refresh the tokens while only one refresh would be sufficient as the new access token would propagate to the other tabs through the store anyway. If in that situation there are multiple refresh requests that's still not actually a problem as you're just refreshing a few times too often which should't really have any side effects though.

@basz
Copy link

basz commented May 21, 2015

I understand the problem. But does't an auth server will return an invalid_grant any subsequent time a refresh_token is being used whenever that server is configured to always always create a new refresh_token.

// time goes by here

i guess it depends on these settings for the oauth server - think I found a bug in apigility where unset_refresh_token_after_use was always true.

'always_issue_new_refresh_token' => false,
'unset_refresh_token_after_use' => false,

Problem might still happen with incorrect settings (basically unset_refresh_token_after_use = true).

thanks marco for pingponging some thoughts

@marcoow
Copy link
Member

marcoow commented May 21, 2015

You're right of course. The situation I describe above would actually be a problem. Not sure how to make sure it can ever happen though. Maybe it could be stored in the store somehow if there's a refresh going on already.

@basz
Copy link

basz commented May 21, 2015

The syncing issue is really a pain - locking doesn't really work with the store mechanisms (as there are no atomic read-updates); also I don't want the authenticator to have access to the store. Besides that locking also causes some problems as you also have to make sure you remove that lock once the user closes the tab etc.

Considering the complications I would 'box' this issue.

@marcoow
Copy link
Member

marcoow commented May 21, 2015

Yeah, unfortunately that's a bit of a problem actually. Maybe someone comes up with a genius idea ;)

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

No branches or pull requests

4 participants