Update refresh strategy to use a separate refresh token #190

evoactivity · 2017-02-13T22:12:15Z

As discussed in this issue #162 I've made the library use a separate token to refresh the JWT.

I've updated the tests and they all pass but someone should really check what I've done, testing is not my strong point.

I've ran this on my app and confirmed it refreshes the token as expected.

…esh the JWT Made makeRefreshToken use the refresh token propert name to create it's object updated tests to deal with new refresh token method

eluciano11 · 2017-02-14T15:31:20Z

@evoactivity thank you for your work!

I've looked at the test and the implementation and it looks good. I think that we should wait until @jpadilla can take a look at this before we merge the PR. This is a breaking change so we'll need to update the docs and demo too. We can do that after @jpadilla takes a look.

evoactivity · 2017-02-14T15:36:08Z

@eluciano11 No problem :)

I was wondering about the docs/demos, but decided against updating them until this had been reviewed incase there are any changes needed.

alechirsch · 2017-02-16T21:52:19Z

addon/authenticators/jwt.js

@@ -120,11 +122,11 @@ export default TokenAuthenticator.extend({



if (Ember.isEmpty(expiresAt)) { // Fetch the expire time from the token data since `expiresAt` // wasn't included in the data object that was passed in. const tokenData = this.getTokenData(token); expiresAt = tokenData[this.tokenExpireName]; if (Ember.isEmpty(expiresAt)) { return resolve(data); } } if (expiresAt > now) {

Shouldn't we be checking if the refresh token expiresAt > now rather than the auth token expiresAt > now?

With the way it currently is, if the auth token expired but the refresh token has not expired, it would not attempt to refresh.
I think we would want to attempt to refresh if the refresh token is still valid.

My API's refresh token library creates the token like so $this->refreshToken = bin2hex(openssl_random_pseudo_bytes(64));, so the refresh token is not just another JWT with an expires property to be decoded. Looking at the auth0 page on refresh tokens it looks like they're implementing them in a similar way https://auth0.com/learn/refresh-tokens/

If the auth token is expired that's exactly when this tries to refresh using the refresh token, so I'm not entirely sure what you mean by that?

From what I am seeing, if the auth token is expired when it enters the restore function then it will reject on line 132

reject(new Error('token is expired'));

Oh yes, I see what you mean now, my mistake.

I guess if the token is expired and we have refreshAccessTokens set to true it should also attempt to refresh the token there as well, and if the server rejects the refresh token then the user is unauthorised sending them back to the login form.

Yes, sorry for the confusion.

@evoactivity Do you have time to implement the fix for this? I am considering forking off, I will be trying to use this feature in an app for this week.

@alechirsch I will be looking at this again maybe later today or tomorrow.

@alechirsch I've updated this to attempt a refresh if the JWT is expired. If the server rejects the request the session is invalidated.

@evoactivity Awesome, thanks for your work on this, it is perfect timing.

jpadilla · 2017-02-20T22:27:52Z

@evoactivity At a first pass, this lgtm. Thanks for putting this together.
@eluciano11 is there anything we'd need after this, docs and dummy app?

I'm fine introducing this as a new major release, but would also love to sneak in updating everything else including to the latest ESA.

evoactivity · 2017-02-21T16:57:58Z

@jpadilla no worries :) I've just pushed a small update to address the expiry issue pointed out by @alechirsch

The docs will need updating to explain the new refreshTokenPropertyName option, if people want to use the current strategy they should be able to simply give that the same name as tokenPropertyName.

The demo app should probably be updated to show this being used, the demo server will need to return two tokens now, the JWT and a standard refresh token.

…imple-auth-token into use-refresh-token

eluciano11 · 2017-02-21T17:43:57Z

@jpadilla @evoactivity yup, updated the docs and the demo app but it's not using the two tokens it's falling back to the old strategy.

alechirsch · 2017-02-21T18:02:32Z

I just did some testing in my own application, there seems to be an issue with old timeouts not being removed. I will do more testing to find out what is really going on.

alechirsch · 2017-02-21T18:15:10Z

@evoactivity From what I am finding if you log in then refresh the page to make it call restore, it will refresh the token, and then call restore again exponentially. Meaning it calls restore once, then twice, then 4 times, then 8. Something about resolve(this.refreshAccessToken(refreshToken)); is making it call restore twice for every time it is called.

Restore should be called only once, when the page is refresh or opened in a new tab. I am unsure why this is happening at the moment.

evoactivity · 2017-02-21T18:18:20Z

@alechirsch I've not seen that behaviour at all in my own app.

What sort of expiry time do you have on your JWT? Also, what leeway do you have set?

alechirsch · 2017-02-21T18:33:38Z

For testing purposes I set it to 60 seconds, with 55 second leeway. Maybe that is the problem?

evoactivity · 2017-02-21T18:36:32Z

I've been testing with my JWT set to 1 minute and my refresh token set to 2 minutes and my leeway set to 0.

I'm not sure why a 5 second expiry would call exponential calls to refreshAccessToken though.

alechirsch · 2017-02-21T18:38:22Z

What's odd is that the restore function is being called more than once.

alechirsch · 2017-02-21T18:49:47Z

I blew away my node_modules and reinstalled, set my expire to 10 minutes and leeway to 60 seconds. I am running into the same problem, the restore function is being called in an infinite loop somehow. It works great if the restore function rejects the first time it is hit, however if it resolves, it calls itself again. It hogs up the CPU at 100% as well.

alechirsch · 2017-02-21T19:01:48Z

I have traced it back to this file ember-simple-auth/instance-initializers/setup-session-restoration.

The beforeModel hook is being called on an infinite loop, which is in turn calling restore. This might be a problem with my app, looking into it more.

alechirsch · 2017-02-21T20:00:23Z

@evoactivity It was definitely a problem with my application, the feature works as expected now.

jpadilla · 2017-02-22T14:48:00Z

This is the current server implementation that the dummy app uses https://github.com/jpadilla/ember-simple-auth-token-server

alechirsch · 2017-03-22T13:20:37Z

What is currently holding up this PR? I have been using this for the past few weeks in my apps and it works perfectly by the way.

jpadilla · 2017-03-22T14:17:51Z

@alechirsch thanks for bumping this again, great to know you've been using it for a while now. Since this is a breaking change, it'll be released on v3, but before doing so I would love to make sure we update ember-cli, ESA, etc. Thanks to all!

PS If anyone else wants to help with that before I do, please feel free to do so. Thanks!

jpadilla · 2017-03-22T14:17:53Z

@homu r+

homu · 2017-03-22T14:17:55Z

📌 Commit 765e926 has been approved by jpadilla

homu · 2017-03-22T14:18:11Z

⚡ Test exempted - status

Update refresh strategy to use a separate refresh token As discussed in this issue #162 I've made the library use a separate token to refresh the JWT. I've updated the tests and they all pass but someone should really check what I've done, testing is not my strong point. I've ran this on my app and confirmed it refreshes the token as expected.

jpadilla · 2017-04-21T00:05:38Z

@alechirsch @evoactivity I'm looking for help with maintenance, would any of you be up to it?

alechirsch · 2017-04-21T00:29:44Z

@jpadilla I can help out with pull requests and issues

jpadilla · 2017-04-21T01:00:05Z

@alechirsch that's so great, just sent you an invite, thank you so much!

evoactivity · 2017-05-05T11:19:34Z

@jpadilla I'm too busy to offer much help at the moment but I'd be interested in helping when things settle down a bit for me. That's probably a month or two away though.

jpadilla · 2017-05-06T15:02:30Z

@evoactivity understandable! Feel free to ping back whenever you have a few cycles free, thanks!

knownasilya · 2017-05-18T15:06:00Z

This is a breaking change, because if the refreshToken name isn't set it defaults to refresh_token, which used to be the token name. If the property isn't set, it should default to the token name.

jpadilla · 2017-12-16T17:29:12Z

Thank you all again! Just released v3.0.0 🎉

updated to expect a refreshTokenProperty name and to use that to refr…

c34e3e9

…esh the JWT Made makeRefreshToken use the refresh token propert name to create it's object updated tests to deal with new refresh token method

evoactivity mentioned this pull request Feb 13, 2017

how does refreshing a token make my app secure? #162

Closed

jpadilla self-requested a review February 14, 2017 16:39

alechirsch reviewed Feb 16, 2017

View reviewed changes

jpadilla approved these changes Feb 20, 2017

View reviewed changes

eluciano11 and others added 2 commits February 21, 2017 08:58

Updates demo to use a refreshTokenPropertyName

b472042

Still attempt refresh if JWT is expired

d823067

Updates docs

e05570a

alechirsch approved these changes Feb 21, 2017

View reviewed changes

eluciano11 added 2 commits February 21, 2017 13:34

Merge branch 'use-refresh-token' of https://github.com/Mashbo/ember-s…

bac0ad1

…imple-auth-token into use-refresh-token

Fixes error on doc

765e926

homu merged commit 765e926 into fenichelar:master Mar 22, 2017

homu mentioned this pull request Mar 22, 2017

Add warning when refreshLeeway is set to high #122

Closed

jpadilla mentioned this pull request Dec 8, 2017

Refresh Token Support #211

Closed

dcyriller mentioned this pull request Dec 15, 2017

Upgrade ESA + ember-{cli,source} to 2.17 #212

Merged

		@@ -120,11 +122,11 @@ export default TokenAuthenticator.extend({

Update refresh strategy to use a separate refresh token #190

Update refresh strategy to use a separate refresh token #190

Conversation

evoactivity commented Feb 13, 2017

eluciano11 commented Feb 14, 2017

evoactivity commented Feb 14, 2017

alechirsch Feb 16, 2017

Choose a reason for hiding this comment

evoactivity Feb 17, 2017

Choose a reason for hiding this comment

alechirsch Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

evoactivity Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

alechirsch Feb 17, 2017

Choose a reason for hiding this comment

alechirsch Feb 20, 2017

Choose a reason for hiding this comment

evoactivity Feb 21, 2017

Choose a reason for hiding this comment

evoactivity Feb 21, 2017

Choose a reason for hiding this comment

alechirsch Feb 21, 2017

Choose a reason for hiding this comment

jpadilla commented Feb 20, 2017

evoactivity commented Feb 21, 2017 • edited Loading

eluciano11 commented Feb 21, 2017

alechirsch commented Feb 21, 2017 • edited Loading

alechirsch commented Feb 21, 2017 • edited Loading

evoactivity commented Feb 21, 2017 • edited Loading

alechirsch commented Feb 21, 2017 • edited Loading

evoactivity commented Feb 21, 2017

alechirsch commented Feb 21, 2017 • edited Loading

alechirsch commented Feb 21, 2017 • edited Loading

alechirsch commented Feb 21, 2017 • edited Loading

alechirsch commented Feb 21, 2017

jpadilla commented Feb 22, 2017

alechirsch commented Mar 22, 2017 • edited Loading

jpadilla commented Mar 22, 2017

jpadilla commented Mar 22, 2017

homu commented Mar 22, 2017

homu commented Mar 22, 2017

jpadilla commented Apr 21, 2017

alechirsch commented Apr 21, 2017

jpadilla commented Apr 21, 2017 • edited Loading

evoactivity commented May 5, 2017

jpadilla commented May 6, 2017

knownasilya commented May 18, 2017 • edited Loading

jpadilla commented Dec 16, 2017

alechirsch Feb 17, 2017 •

edited

Loading

evoactivity Feb 17, 2017 •

edited

Loading

evoactivity commented Feb 21, 2017 •

edited

Loading

alechirsch commented Feb 21, 2017 •

edited

Loading

alechirsch commented Feb 21, 2017 •

edited

Loading

evoactivity commented Feb 21, 2017 •

edited

Loading

alechirsch commented Feb 21, 2017 •

edited

Loading

alechirsch commented Feb 21, 2017 •

edited

Loading

alechirsch commented Feb 21, 2017 •

edited

Loading

alechirsch commented Feb 21, 2017 •

edited

Loading

alechirsch commented Mar 22, 2017 •

edited

Loading

jpadilla commented Apr 21, 2017 •

edited

Loading

knownasilya commented May 18, 2017 •

edited

Loading