Skip to content
This repository has been archived by the owner on Jun 3, 2020. It is now read-only.

Add token refresh flow. #369

Closed
wants to merge 15 commits into from
10 changes: 5 additions & 5 deletions app/modules/auth/auth.factory.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@ function authFactory($interval, $timeout, $location, $window, localStorageServic
var _user = {};

OAuth.checkCurrent()
.then(function (user) {
if (user) {
_setUserData();
}
});
.then(function (user) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to keep these whitespace changes only?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is the indenting at 4 spaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it was like that when I started working on this app and it was too late when I realised I could change it. It annoys me every time I work on it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok - this PR has the change in whitespace here it seems unrelated and that's why i'm confused by it. It's not blocking for sure, just this PR is adding spacing to a function for some reason...

if (user) {
_setUserData();
}
});

factory = {
signIn: signIn,
Expand Down
16 changes: 13 additions & 3 deletions app/modules/zoo-api/zooapi.factory.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,20 @@ require('./zooapi.module.js')
.factory('zooAPI', zooAPI);

var ApiClient = require('panoptes-client/lib/api-client');

var oauth = require('panoptes-client/lib/oauth')
// @ngInject
function zooAPI(zooAPIConfig) {

function zooAPI(AnnotationsFactory, $location) {
ApiClient.beforeEveryRequest = function() {
return oauth.checkBearerToken()
.then(function (token) {
console.log('Token refreshed: ', token);
})
.catch(function (error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checkBearerToken() doesn't throw an error if it can't refresh an expired token. It resolves to null instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what's best practice with promises, but couldn't we reject the promise and throw an error when it resolves to null?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@eatyourgreens eatyourgreens Feb 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: When a login token could not be refreshed when it's supposed to, should it reject a promise or return null (as it does in PJC 2.9.2 onwards)? I'm not sure what the best practice is either, since I haven't worked with many other login/session libraries - but whatever it is, we'll need to document it clearly.

Thoughts:

  • Rejecting Promises has been one of those fiddly things that I've run into, and judging from the weirdness I encounter it's worth opening a separate discussion (Slack, yo?) to discuss expectations on how they're supposed to work.
  • If the best practice is to reject a promise, don't worry about ASM PR251 - that PR's locked to using PJC 2.9.2 and I can write a separate PR to future-proof ASM once PJC 2.9.5 or etc is released.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because that's a catch block, so the original promise has already rejected at this point. Have a look at the sections about chaining promises, and handling errors, at MDN.

The return value of the catch is passed to the next then block in the chain.

Copy link
Contributor

@eatyourgreens eatyourgreens Feb 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This discussion seems to be diverting away from SW, and more into how the client should work. Would it be better off having that discussion on zooniverse/panoptes-javascript-client?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As to why null is returned, have a look at zooniverse/panoptes-javascript-client#82 @shaunanoordin found that the client was returning undefined for the token in some cases.

console.log('Failed to refresh token: ', error);
AnnotationsFactory.updateCache();
oauth.signIn($location.absUrl());
})
}
// There's only a version of this project on production, so rather than
// defer to the client we manually override the API root.
// Panoptes.apiClient.root = 'https://www.zooniverse.org/api';
Expand Down
4 changes: 3 additions & 1 deletion app/modules/zoo-api/zooapi.module.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
'use strict';

module.exports = require('angular')
.module('app.zooapi', []);
.module('app.zooapi', [
'app.transcribe.annotations'
]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will also need to be reverted if token check is done in AuthFactory.

2 changes: 1 addition & 1 deletion gulp/tasks/browserify.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ function buildScript(file) {
.pipe(gulpif(createSourcemap, sourcemaps.init()))
.pipe(gulpif(global.isProd, streamify(
uglify({
compress: { drop_console: true }
compress: { } // leave console.log in prod, for testing
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change should be reverted before this is merged to master.

})
.on('error', handleErrors)
)))
Expand Down