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.

Sign up for GitHub

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

Add checkBearerToken() to OAuth #80

Merged
merged 6 commits into from
Feb 13, 2018
Merged

Add checkBearerToken() to OAuth #80

Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a00220c
Rename _handlerBearerToken to handleNewBearerToken
eatyourgreens Feb 12, 2018
a98a643
Add expires_at and store new token
eatyourgreens Feb 12, 2018
9a1cab1
Add checkBearerToken()
eatyourgreens Feb 12, 2018
9bba154
Remove isTokenStillValid
eatyourgreens Feb 12, 2018
eaaabdf
Increase the expiration allowance to 5 minutes
eatyourgreens Feb 13, 2018
1d61595
2.9.2
eatyourgreens Feb 13, 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
36 changes: 26 additions & 10 deletions lib/oauth.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,17 @@ module.exports = new Model({
_currentUserPromise: null,
_tokenDetails: null,

checkBearerToken: function() {
var awaitBearerToken;
if (this._bearerTokenIsExpired()) {
awaitBearerToken = this._refreshBearerToken();
} else {
var tokenDetails = JSON.parse(SESSION_STORAGE.getItem(LOCAL_STORAGE_PREFIX + 'tokenDetails'));
awaitBearerToken = Promise.resolve(tokenDetails);
}
return awaitBearerToken;
},

checkCurrent: function() {
console.log('Checking current user');

Expand Down Expand Up @@ -68,8 +79,7 @@ module.exports = new Model({
if (checkUrlForToken(window.location.hash)) {
console.log('Token found in URL');
var tokenDetails = parseUrl(window.location.hash);
this._handleBearerToken(tokenDetails);
SESSION_STORAGE.setItem(LOCAL_STORAGE_PREFIX + 'tokenDetails', JSON.stringify(tokenDetails));
this._handleNewBearerToken(tokenDetails);

// And redirect to the desired page
var url = ls.get(LOCAL_STORAGE_PREFIX + 'redirectUri');
Expand All @@ -79,7 +89,7 @@ module.exports = new Model({
// If not, let's try and pick up an existing Panoptes session anyway
this._checkForPanoptesSession()
.then(function(tokenDetails) {
this._handleBearerToken(tokenDetails);
this._handleNewBearerToken(tokenDetails);
resolve(tokenDetails)
}.bind(this))
.catch(function (error) {
Expand Down Expand Up @@ -155,6 +165,15 @@ module.exports = new Model({
].join('');
},

_bearerTokenIsExpired: function() {
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 need a new method to check if token is expired? One already existed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

zooniverse/shakespeares_world#366 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need a method that works. I'd prefer if the OAuth class has the same API as Auth, so that common code can be consolidated as per the recommendations in #76.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, sorry I forgot you already commented on that.

var tokenDetails = JSON.parse(SESSION_STORAGE.getItem(LOCAL_STORAGE_PREFIX + 'tokenDetails'));
if (tokenDetails) {
return Date.now() >= tokenDetails.expires_at - TOKEN_EXPIRATION_ALLOWANCE;
Copy link
Contributor

Choose a reason for hiding this comment

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

is 60s long enough to get a new token via an iFrame on slow network / a slow busy server?

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 a reasonable time to allow for Panoptes to respond?

Copy link
Contributor

Choose a reason for hiding this comment

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

The load balancer kills connections that have not started responding after 60s so in theory it could be up to that long for a refresh promise to fulfill on a slow server.

I prefer to be overly cautious on this as i'm pretty sure slow / late token refreshes have caused some auth issues with ASM. E.g. say if the refresh via iFrame setTimeout is 10s late to fire and the token refresh takes 55s then there is a 5s period where the token will be expired and invalid.

Perhaps add 30s to it to be 180secs or maybe 2 mins?

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've bumped it to 5 minutes. I've also been thinking that the solution to the unreliable timeout in #81 is to set the timer interval to the same allowance.

} else {
return false;
}
},

_checkForPanoptesSession: function() {
var sessionTokenDetails = JSON.parse(SESSION_STORAGE.getItem(LOCAL_STORAGE_PREFIX + 'tokenDetails'));
var redirectUri = ls.get(LOCAL_STORAGE_PREFIX + 'redirectUri');
Expand Down Expand Up @@ -237,23 +256,24 @@ module.exports = new Model({
});
},

_handleBearerToken: function(tokenDetails) {
_handleNewBearerToken: function(tokenDetails) {
console.log('Got new bearer token', tokenDetails.access_token.slice(-6));
this._tokenDetails = tokenDetails;
apiClient.headers.Authorization = 'Bearer ' + tokenDetails.access_token;

var refresh = this._refreshBearerToken.bind(this);
var timeToRefresh = (tokenDetails.expires_in * 1000) - TOKEN_EXPIRATION_ALLOWANCE;
this._bearerRefreshTimeout = setTimeout(refresh, timeToRefresh);
tokenDetails.expires_at = Date.now() + (tokenDetails.expires_in * 1000);
SESSION_STORAGE.setItem(LOCAL_STORAGE_PREFIX + 'tokenDetails', JSON.stringify(tokenDetails));
return tokenDetails;
},

_refreshBearerToken: function() {
this._deleteBearerToken();
return this._checkForPanoptesSession()
.then(function(tokenDetails) {
SESSION_STORAGE.setItem(LOCAL_STORAGE_PREFIX + 'tokenDetails', JSON.stringify(tokenDetails));
return this._handleBearerToken(tokenDetails);
return this._handleNewBearerToken(tokenDetails);
}.bind(this))
.catch(function (error) {
console.info('Panoptes session has expired');
Expand Down Expand Up @@ -284,10 +304,6 @@ function parseUrl(string) {
return tokenDetails;
}

function isTokenStillValid(tokenDetails) {
return (tokenDetails.started_at + tokenDetails.expires_in) > Date.now();
}

function createIFrame(url) {
var iframe = document.createElement('iframe');
iframe.setAttribute('src', url);
Expand Down