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

Adds JWT Authenticator with refresh functionality #24

Merged
merged 35 commits into from
Feb 21, 2015
Merged

Adds JWT Authenticator with refresh functionality #24

merged 35 commits into from
Feb 21, 2015

Conversation

hoIIer
Copy link
Contributor

@hoIIer hoIIer commented Jan 17, 2015

No description provided.

@jpadilla
Copy link
Contributor

@erichonkanen hey mind check out the failing build?

https://travis-ci.org/jpadilla/ember-cli-simple-auth-token/builds/47388220

@hoIIer
Copy link
Contributor Author

hoIIer commented Jan 18, 2015

@jpadilla there we go...

Eric Honkanen added 3 commits January 18, 2015 19:42
… token refresh after a page refresh, also added invalidate method that cancels token refresh
@hoIIer
Copy link
Contributor Author

hoIIer commented Jan 19, 2015

Added restore ability so that automatic refresh begins with valid token on page refresh.. Now what's left seems a little daunting but writing more tests? It might take me a bit to read up on how to write them..

@jpadilla
Copy link
Contributor

@erichonkanen definitely needs tests. There's tons of things going on in this PR so tests will help me understand and check everything is working correctly, great work!

@hoIIer
Copy link
Contributor Author

hoIIer commented Jan 20, 2015

@jpadilla definitely agree! I'll be working on tests and let you know when it's updated..

@hoIIer
Copy link
Contributor Author

hoIIer commented Feb 9, 2015

@jpadilla made progress on first tests for restore method, check them out and lmk if they on the right path. They cover each of the possible outcomes within the method... https://github.com/erichonkanen/ember-cli-simple-auth-token/blob/561d31a3d6a1fd46b8275bd0bd881ebf6ba8f5f2/tests/unit/authenticators/jwt-test.js

Also I don't see the travis ci build in this PR anymore, not sure if it needs to be resubmitted or whatnot? I'll aim to have this done by friday!

@jpadilla
Copy link
Contributor

jpadilla commented Feb 9, 2015

@erichonkanen nice, glad you got them working! Looking good. I think you might need to update your fork. There were some changes that happened a while back that are causing conflicts, that might be why builds aren't running.

@hoIIer
Copy link
Contributor Author

hoIIer commented Feb 10, 2015

@jpadilla good news and bad news.. the good news is I finished all the tests so hypothetically this can be reviewed now... the bad news is that I discovered that opening a 2nd tab with the app instantly boots me out of both tabs. So I'll have to do some digging into that, if you have any ideas around that lmk! otherwise tell me if anything outside of that needs changes... thanks!

@hoIIer
Copy link
Contributor Author

hoIIer commented Feb 11, 2015

Fixed multiple tab logout issue, added one more test, reviewed test coverage and all seems well!

@jpadilla
Copy link
Contributor

@erichonkanen what was causing the "multiple tab logout issue" ?

@hoIIer
Copy link
Contributor Author

hoIIer commented Feb 11, 2015

@jpadilla For some reason I had made a barely noticeable change and added parenthesis to reject in call to _this.refreshAccessToken. That made it fire the reject always when restore was called and thus boot me out on page refresh or loading new tab...

_this.refreshAccessToken(data.token).then(function(data){
     resolve(data);
}, reject());

Changed reject() back to reject

of

restore: function(data){
    var _this = this;
    return new Ember.RSVP.Promise(function(resolve, reject){
      var now = (new Date()).getTime(),
        expiresAt = _this.resolveTime(data.expiresAt);
      if(!Ember.isEmpty(data.expiresAt) && !Ember.isEmpty(data.token) && data.expiresAt > now){
        if(_this.refreshAccessTokens){
          _this.refreshAccessToken(data.token).then(function(data){
            resolve(data);
          }, reject);
        }else{
          reject();
        }
      }else{
        if(Ember.isEmpty(data.token)){
          reject();
        }else{
          var tokenData = _this.getTokenData({'token': data.token}),
            tokenExpiresAt = tokenData[_this.tokenExpireName];
          expiresAt = _this.resolveTime(tokenExpiresAt);
          _this.scheduleAccessTokenRefresh(expiresAt, data.token);
          resolve(data);
        }
      }
    });
  },

@jpadilla
Copy link
Contributor

@erichonkanen I don't think timeFactor defaulting to 1 is a good idea, perhaps that was just for testing?

This'll take me more than I thought to review. I think it'll be a good idea to build a demo app that uses with a demo backend server.

@hoIIer
Copy link
Contributor Author

hoIIer commented Feb 12, 2015

@jpadilla so the purpose of timeFactor was to normalize converting between e.g. python's default seconds to javasripts default milliseconds... I'm not sure which should be the default, in my case since Im using python it would make sense to have it be "1000". Maybe there is a better way to handle that conversion?

Also, yes I recommend setting up a demo app. In fact I have one up you could log into but its private so pm me.. Ive tested it with 2 separate apps and it works great. Its helpful to set the token refresh on python side to e.g. 15 seconds for testing that it works

@hoIIer
Copy link
Contributor Author

hoIIer commented Feb 12, 2015

shoot I think you're right in that time factor should be default 1000 since this package is being developed against django rest jwt...

I had just set it in my project config to timeFactor: 1000,

@jpadilla
Copy link
Contributor

@erichonkanen whenever you can hit me up with access wherever you've got this working on.

jpadilla added a commit that referenced this pull request Feb 21, 2015
Adds JWT Authenticator with refresh functionality
@jpadilla jpadilla merged commit a6cb47a into fenichelar:master Feb 21, 2015
@hoIIer
Copy link
Contributor Author

hoIIer commented Feb 21, 2015

@jpadilla sweet! everything good? thanks a ton for all the help in this!

@jpadilla
Copy link
Contributor

@erichonkanen I noticed that you weren't using tokenPropertyName, so in a few places I think you're assuming that token will be in the root of the response object.

Also, I think getTokenData should receive the token string, instead of a response object.

@hoIIer
Copy link
Contributor Author

hoIIer commented Feb 22, 2015

@jpadilla ok gotcha, ill take a look and make some updates hopefully tomorrow if not early this week.. thx for the review

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

Successfully merging this pull request may close these issues.

2 participants