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
Closed

Add token refresh flow. #369

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
4e08716
Experiment in token refresh.
Feb 2, 2018
0a603ee
use new oauth.checkBearerToken
Feb 12, 2018
03e6cec
Update logout view if token refresh fails.
Feb 12, 2018
42d06c9
Warn of session expiration, save their work and refresh token for them
Feb 13, 2018
2caa387
Improve UX for token refresh failure
Feb 27, 2018
29b0a21
Move check to auth factory
Mar 1, 2018
d770466
Fix silly error
Mar 1, 2018
834a8da
Fixes user check loop.
Mar 5, 2018
f3c4e28
Update client
Mar 7, 2018
3ca3b8c
Break promise chain, maybe
Mar 7, 2018
090cb17
Remove SubjectLoadError
Mar 7, 2018
b156158
Don't catch errors mid-chain.
Mar 8, 2018
2a6d6b6
Remove catch block to let error propagate to handler.
Mar 8, 2018
0f925ad
Prevent transcribe page from loading.
Mar 8, 2018
9469307
Remove route check.
Mar 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 29 additions & 6 deletions app/modules/auth/auth.factory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,41 @@ require('./auth.module.js')
var OAuth = require('panoptes-client/lib/oauth');

// @ngInject
function authFactory($interval, $timeout, $location, $window, localStorageService, ModalsFactory, zooAPI, zooAPIConfig, CribsheetFactory, $rootScope) {
function authFactory($location, $rootScope, $state, $transitions, $window, AnnotationsFactory, zooAPI, CribsheetFactory) {

var factory;

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();
}
})
.then(function() {
zooAPI.beforeEveryRequest = function() {
return OAuth.checkBearerToken()
.then(function (token) {
// We catch any request to access the transcribe route.
$transitions.onStart({ to: 'Transcribe'}, function(transition) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens here if my session expires while I'm navigating to a different page, such as the home page, or if I'm already transcribing when my session expires (which is the most likely scenario)? Correct me if I'm wrong, but this block looks like it only checks if I'm still logged in when I'm about to load the transcription page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. Removing the Transcribe restriction will take care of navigating e.g. from /transcribe to home /. However, if my session expires while I'm transcribing and I click I'm done, it'll just look like I'm still logged in and new subject will be served, which isn't right. I'll have to catch the event too.

Copy link
Contributor

Choose a reason for hiding this comment

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

You want to check for a valid token before every API request anyway, so just remove this check.

token = $window.sessionStorage.getItem('panoptesClientOAuth_tokenDetails');
Copy link
Contributor

Choose a reason for hiding this comment

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

Never, ever access the client's storage directly like this. You should always go through OAuth in order to validate the session.

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 added that like because token is still defined even when storage is cleared. Would var cachedToken = $window.sessionStorage.getItem('panoptesClientOAuth_tokenDetails'); be better?

Copy link
Contributor

@eatyourgreens eatyourgreens Mar 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Absolutely not. Don't load the client's token directly because a) you don't know what name the client code uses internally to store the token, and this may well change b) the client has a public method defined to get the token for you: checkBearerToken.

Copy link
Contributor

Choose a reason for hiding this comment

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

Clearing storage logs your session out, so the client logs you back in if you still have a valid Panoptes session. It doesn't simulate your session having expired.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for clarify. What's the alternative then?

Copy link
Contributor

@eatyourgreens eatyourgreens Mar 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Set the token expires_in to something short, or change this line in oauth.js to run the check every 30s (for example).
var TOKEN_EXPIRATION_ALLOWANCE = 119.5 * 60 * 1000;

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'm confused. If you mean "hack the local copy of the client in node_modules", I'm already doing that; if you don't mean that, I don't know what you mean, sorry.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's exactly right. Hack the installed copy of oauth.js so that the client checks with Panoptes for a session, rather than using the stored token.

Don't forget to block third-party cookies too, since that's the bug you're addressing in this PR.

// The Panoptes client doesn't return an error but just null when it can't refresh the token.
// So we check for null, instead of using a catch block.
if (_user.id && token === null) {
// We're logged in but don't have a token any more.
alert('Your session is expired. Press OK to save your work and start a new one.')
// Save any unsaved work and redirect to Panoptes for a new token.
AnnotationsFactory.updateCache();
OAuth.signIn($location.absUrl());
// Abort ui-router state transition
return Promise.reject(new Error('ui-router transition aborted'));
}
}
)
})
}
});

factory = {
signIn: signIn,
Expand Down
5 changes: 1 addition & 4 deletions app/modules/transcribe/subjects.factory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,10 +114,7 @@ function SubjectsFactory($q, AnnotationsFactory, localStorageService, zooAPI, zo
return zooAPI.type('workflows').get(zooAPIConfig.workflow_id)
.then(function(wf) {
var randomSet = _.sample(wf.links.subject_sets)
return randomSet
})
.catch(function(error) {
console.log('Error fetching active subject sets', error)
return randomSet;
})
}

Expand Down
13 changes: 6 additions & 7 deletions app/modules/transcribe/transcribe.controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,9 @@ function TranscribeController($stateParams, $modal, $scope, $window, Annotations

function loadSubject() {
return SubjectsFactory.$getData($stateParams.subjectSet)
.then(subjectLoaded, subjectLoadError)
.then(loadAggregations);
.then(subjectLoaded)
.then(loadAggregations)
.catch(handleErrors);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is catching any errors in loadAggregations, but not errors in loading subjects, because subjectLoadError already handled those.

}

function openTutorial() {
Expand All @@ -93,12 +94,10 @@ function TranscribeController($stateParams, $modal, $scope, $window, Annotations
vm.subject = SubjectsFactory.current;
}

function subjectLoadError(result) {
if (result === 'outOfData') {
function handleErrors(error) {
if (error === 'outOfData') {
$scope.$broadcast('subject:outOfData');
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to delete this?

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 did, because your pseudo-code didn't use it, so I thought deleting was implied in your comment. Was it not?

Copy link
Contributor

Choose a reason for hiding this comment

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

That code was an example of how to catch errors in a chain of promises, that's all.

What did this line do? It looks, to me, like it notifies when the workflow is out of subjects, or something like that. Won't removing it break SW wherever it listens for this event?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. I've been looking into the same 10 lines for too long.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mind you, not adding it back in helps with stopping requests...

Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly, you're adding it back in like this?

loadSubjects()
.catch(handleSubjectError)
.then(loadAggregations)
.catch(handleErrors)

in which case loadAggregations will always be run, even if loadSubjects throws an error, because you're catching errors mid-chain then recovering and continuing the chain. Read the section about Promise chains at MDN for more detail.

What you really want to be doing is interrupting the chain, then handling your errors at the end ie. use handleErrors to handle any errors that occurred while subjects were loading, or while aggregations were loading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I was adding it back like this:

function loadSubject() {
        return SubjectsFactory.$getData($stateParams.subjectSet)
            .then(subjectLoaded, subjectLoadError)
            .then(loadAggregations)
            .catch(handleErrors);
    }

But I think it's equivalent.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, that's functionally the same as the pseudo-code I posted. A catch block is equivalent to passing a second argument to then. I think the MDN docs explain that too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I just remove subjectLoadError then (provided I find another decent place for the out-of-date message)?

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 read that. That's why I said they are equivalent.

} else {
console.error('Error loading subject', result);
}
console.log(error);
}

}
2 changes: 1 addition & 1 deletion app/modules/zoo-api/zooapi.factory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ require('./zooapi.module.js')
var ApiClient = require('panoptes-client/lib/api-client');

// @ngInject
function zooAPI(zooAPIConfig) {
function zooAPI() {

// There's only a version of this project on production, so rather than
// defer to the client we manually override the API root.
Expand Down
4 changes: 3 additions & 1 deletion app/modules/zoo-api/zooapi.module.js
View file
Open in desktop
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
View file
Open in desktop
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
6 changes: 3 additions & 3 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@
"nib": "^1.1.0",
"node-dir": "0.1.16",
"npm": "^5.3.0",
"panoptes-client": "^2.9.4",
"panoptes-client": "^2.9.6",
"pretty-hrtime": "^1.0.3",
"q": "^1.2.0",
"run-sequence": "^2.1.0",
Expand Down