Replace _checkPanoptesSession with _getNewToken

[eatyourgreens]

Replaces _checkPanoptesSession() with _getNewToken(), which attempts to get a new token from Panoptes via an iframe (and should do only that.) Stored tokens are not returned, since we should only be calling this method when the stored token is about to expire.

Fixes a problem mentioned in zooniverse/shakespeares_world#366 (comment) which is that the client would always return an expired token, if it was stored in session storage.

Also fixes a bug where checkBearerToken throws an error when getting a new token, because this._deleteBearerToken is undefined in the catch block here

panoptes-javascript-client/lib/oauth.js

Lines 279 to 284 in 595e694

	.catch(function (error) {
	console.info('Panoptes session has expired');
	console.log(error);
	this._deleteBearerToken();
	return null;
	});

[simoneduca]

Is that the same problem I was refer to in your older PR #85 (comment)?

[eatyourgreens]

Could be. It's hard to tell, since the token issued by Panoptes is valid for 2 hours and the console logs don't tell us if that token came from a network call to Panoptes, or from session storage.

[eatyourgreens]

No wait, 'Found existing Panoptes session' is printed out when a token is obtained from the iframe URL.

panoptes-javascript-client/lib/oauth.js

Lines 201 to 204 in 595e694

	if (checkUrlForToken(iframeLocation.hash)) {
	console.info('Found existing Panoptes session');
	var newTokenDetails = tokenFromLocation(iframeLocation);
	resolve(newTokenDetails);

[shaunanoordin]

The code update looks good and my initial testing looks OK so far, but I was trying to follow up by checking out if there were any other function()s that require .this that weren't bind()-ed properly.

And then I saw how the .bind()s were use in the lib components and thought, 😳 OK, never mind then. I'll continue with standard user testing, but out of curiosity, why does the PJC bind functions when they're being called/returned instead of binding at the function declaration, or equivalent?

[shaunanoordin]

PR Review

This PR fixes two issues (with the current 2.9.4, if I'm correct?) which basically meant that tokens weren't being refreshed properly:

• when a token is about to expire and supposed to be refreshed, it isn't - an old token is returned instead.
• oauth._refreshBearerToken() (or rather, one of the functions in it) wasn't bind()-ed properly so throws an error when referring to this; basically, the error-catching mechanism caused an error.

Testing

Basic testing looks OK:

• User can log in and stay logged in
• User can log out and stay logged out

Primary testing looks good, at least for the success conditions:

• when a user is logged in for a period of time, their login session is auto-refreshed with a new token after a period of time.
  //Replicated by setting the refresh time to a few seconds, then monitoring that continuity of login state is maintained. New tokens at each refresh() call. 👍

I avoided doing too much testing for fail states, however (e.g. sudden network down, etc) as that's a bit of a rabbit hole if I don't go in prepared with a list of possible fail conditions.

Status

LGTM, since far as I can see, this PR handles the issue with token not refreshing properly. I'm happy to give this a 👍 but I'll refrain from merging just yet in case anybody else has more tests to run.

[eatyourgreens]

Sometimes when I'm writing Javascript I want to throw up my hands and say "this is bullshit!" but I can never remember what "this" refers to.
https://twitter.com/bendhalpern/status/578925947245633536?s=21

The use of .bind(), particularly nested bound calls, makes some of the auth code really hard to follow.

Without it, this would refer to the calling promise in those anonymous callbacks. Your comment does make me think the refresh code would be easier to follow as

_getNewToken()
  .then(this._handleNewBearerToken)
  .catch(this._deleteBearerToken);

which would get rid of a couple of those .bind() calls.

[simoneduca]

This works well for me too.

[simoneduca]

I'll wait until tomorrow before merging to see if @camallen wants to add anything to this.

[eatyourgreens]

I've added a couple of changes that should keep your Auth headers intact until the Panoptes session has actually expired, so you can continue to make API calls in the last 5 minutes of your session.

[simoneduca]

This works well for me. Tested on SW.

[eatyourgreens]

No problems here either, browsing SW on localhost in Firefox with no restrictions and Chrome with third-party cookies blocked.