Various OAuth bug fixes

[eatyourgreens]

Fixes a few bugs in 2.9.3:

• The Promise returned from oauth.init() never resolves.
• Refreshing the token via an iframe did not return a valid token object. (Closes _handleNewBearerToken() florps on .slice() #84)

[simoneduca]

I find these changes clearer than the older code and the app runs well. I can login/logout no problem. LGTM, but you might want to wait for another pair of eyes.

[eatyourgreens]

Does it fix #84?

[simoneduca]

It does. I've hacked the expiry time and I was able to see the token being returned correctly. However, it looks like the same token is returned every time, which I wasn't expecting.

[shaunanoordin]

The code changes LGTM, but gimme a second to run this on my local ASM project to test the updates in a running environment. 👌

[shaunanoordin]

What the derp? I have issues signing in on ASM local; although I do get a login token, I don't seem to register a user at the login process. (user === null) Let me run a quick investigation; if anything, it's likely that I didn't set up my environment properly.

Current details:

• On Anti-Slavery Manuscripts, localhost, I removed and reinstalled node modules, making sure in package.json I have "panoptes-client": "panoptes-client": "git://github.com/zooniverse/panoptes-javascript-client.git#oauth-refresh-token",
• I accessed ASM local on an incognito Chrome window.
• Upon clicking Sign In and inputting my login details, I'm returned to ASM local with an access token (visible at the return URL for a millisecond, and in apiClient.beforeEveryRequest/oauth.checkBearerToken) but my user object from oauth.checkCurrent() returns null.

🤷‍♂️

🕵️ OK, it's investigation time.

[eatyourgreens]

@shaunanoordin I am not sure, but Roger's original code for the OAuth module might rely on there being a # in the redirect URL, which ASM doesn't use. Does the user get loaded, then lost again when the page redirects?

[eatyourgreens]

With a local version of ASM, I'm seeing that the user session is lost when the page reloads after login. SW doesn't do this, I think because it uses # in the URL, so the redirect doesn't reload the page.

[eatyourgreens]

@shaunanoordin I think 36121ca will solve that problem. Also, I see that ASM isn't using a package lock file, so npm install will install the latest version of the client, which is currently 2.9.3 and is actually broken.

[eatyourgreens]

But I've now introduced this error, if you aren't logged in. Cannot read property 'access_token' of null.

The original code did not work with ASM.

[eatyourgreens]

@shaunanoordin @simoneduca I think this is working now. I've been trying it out with a local copy of ASM, and I'm not running into errors after the last two commits. Over to you for testing!

[shaunanoordin]

Yikes, so it looks like there are some compatibility issues with certain projects - thanks for catching that, Jim! 👍 I should expand my tests to Anti-Slavery Manuscripts and Scribes of the Cairo Genizah, maybe Education API Front End, just to get a better sample size.

[shaunanoordin]

PR Review

This PR fixes:

• login issues introduced in 2.9.3 (couldn't login on Shakespeare's World and certain other websites)
• an issue in _handleNewBearerToken() where it received wrong input. (window.location.hash vs location.href) ( fixes _handleNewBearerToken() florps on .slice() #84 )

Code changes look good, but given the impact of the update, the tests will be done across a few different CFEs.

Testing

All testing done on localhost using OSX+Chrome64 on a new incognito window every time.

Testing checks for:

• A non-logged in user can login.
• A logged-in user can refresh the page and remain logged in.
• A logged-in user can access user-only functionality (e.g. Collections in ASM & Scribes, classrooms in Edu API FE)
• A logged-in user can log out.
• A logged-out user can refresh the page and remain logged out.

Looks good for:

• Anti-Slavery Manuscripts 👍
• Scribes of the Cairo Geniza 👍
• Education API Front End 👍

Status

Woot, this is looking good! I'm going to give this a 👍, but will hold off on merging until @simoneduca or a second dev gives an all clear.

[shaunanoordin]

Bonus piece of good news: testing looks good on IE11 with ASM local, too. 👌

[simoneduca]

Thanks for these changes @eatyourgreens. This version works great on SW and Annotate too. I've tested:

• Login and logout
• Get fresh token when clearing session storage

LGTM.

[eatyourgreens]

Published as 2.9.4.