Persist bearer token details across page reloads #75

eatyourgreens · 2018-01-26T18:33:13Z

Use window session storage to store the current bearer token across page reloads, in case 3rd party cookies are blocked.

I think this will fix #72, where sign-in does not work on sites like antislaverymanuscripts.org if 3rd party cookies are disabled.

It does not fix token refreshes, for long sessions, being blocked in that case.

eatyourgreens · 2018-01-26T18:34:35Z

lib/oauth.js

@@ -152,9 +153,14 @@ module.exports = new Model({
  },

  _checkForPanoptesSession: function() {
+    var sessionTokenDetails = sessionStorage.getItem(LOCAL_STORAGE_PREFIX + 'tokenDetails');


missing JSON.parse here.

Use window session storage to store the current bearer token across page reloads, in case 3rd party cookies are blocked.

eatyourgreens · 2018-01-29T11:30:32Z

Reading up on sessionStorage, keys are stored for the page session, not browser session, so this code won't preserve the token across tabs or windows.

simoneduca · 2018-01-29T11:58:01Z

I wonder if this might introduce changes that confuse/frustrate volunteers. What if they accidentally close their current window and lose work?

eatyourgreens · 2018-01-29T11:59:10Z

I don't follow. At the moment they can't log in at all.

eatyourgreens · 2018-01-29T12:18:18Z

I've verified on a local copy of Shakespeare's World that this fixes zooniverse/shakespeares_world#347. I can log in when 3rd party cookies are blocked. I'm not sure if login persists across tabs.

simoneduca · 2018-01-29T12:59:11Z

I see that being able to login is critical, but how many people have 3rd parties cookies disabled? Session not persisting in new tabs would affects lots of users I think. Ideally we would like both, no?

eatyourgreens · 2018-01-29T14:20:36Z

Having read the MDN description of sessionStorage, I'm not completely convinced it's appropriate here.

I see that being able to login is critical, but how many people have 3rd parties cookies disabled?

I think this misses the point. @camallen pointed out that the OAuth flow we use means that shakespearesworld.org should not be reading a 3rd party cookie from panoptes.zooniverse.org to log in. It should work with the token that's passed back in the redirect URL from the Zooniverse sign-in page.

If I remember correctly, the original bug report was Safari on OSX, so that may have 3rd party cookies disabled by default. They'll also be blocked by anyone who uses an ad-blocker, or who blocks tracking cookies for privacy reasons.

simoneduca · 2018-01-29T14:40:33Z

Having read the MDN description of sessionStorage, I'm not completely convinced it's appropriate here.

What alternatives do we have then?

eatyourgreens · 2018-01-29T14:46:34Z

Well localStorage, but local storage never expires, so code will probably be needed to manually remove expired tokens when a login session expires. I haven't looked into the oauth code in enough detail to see if it already does this.

eatyourgreens · 2018-01-29T14:49:32Z

Another alternative would be to rewrite the API for the oauth module, so that the client app (Shakespeare's World/ASM/Intro2Astro) is responsible for tracking expired tokens and issuing refresh requests. I'm not sure about making changes that break live sites.

simoneduca · 2018-01-29T15:17:31Z

Well localStorage, but local storage never expires, so code will probably be needed to manually remove expired tokens when a login session expires. I haven't looked into the oauth code in enough detail to see if it already does this.

This is meant to refresh the token before it actually expires https://github.com/zooniverse/panoptes-javascript-client/blob/master/lib/oauth.js#L242

eatyourgreens · 2018-01-29T15:18:54Z

Yes, but that uses an iframe to refresh the token, which is why this 3rd-party domain bug exists in the first place.

simoneduca · 2018-01-29T15:27:04Z

Btw why do you think sessionStorage isn't suitable here @eatyourgreens ?

eatyourgreens · 2018-01-29T15:31:35Z

@simoneduca The MDN docs imply that it uses page sessions, rather than browser sessions. See my original comment in this thread.

eatyourgreens · 2018-01-29T16:33:26Z

Confirmed on a local copy of Shakespeare's World in Chrome with 3rd party cookies disabled, and this version of the client installed. If I login, then cmd-click 'Get started' to open in a new tab, I'm not logged in on the transcription tab. Auth tokens don't persist across tabs with this PR.

If sessionStorage is replaced with localStorage in this PR, then the client will store, and try to use, expired tokens forever, because there's no code in place to check a token's validity before using it in _handleBearerToken.

panoptes-javascript-client/lib/oauth.js

Lines 231 to 240 in 85bdc16

    
           _handleBearerToken: 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); 
        
             return tokenDetails; 
        
           },

simoneduca · 2018-01-29T16:42:28Z

I'd be happy to pair up to add the necessary code. But I'm not sure using localStorage is right either.

Console warn if the iframe fails, rather than swallowing the original error and throwing a new one.

simoneduca · 2018-01-29T16:46:29Z

lib/oauth.js

@@ -179,7 +185,7 @@ module.exports = new Model({
            }
          } catch (error) {
            if (error.name === 'SecurityError') {
-              error = new Error('No existing Panoptes session found');
+              console.warn('No existing Panoptes session found');


why not logging the actual error as well?

That happens on the next line, when the promise rejects.

eatyourgreens · 2018-01-29T17:05:07Z

But I'm not sure using localStorage is right either.

@simoneduca can you expand on why using local storage isn't right?

simoneduca · 2018-01-29T21:20:02Z

I seem to remember Cam having good reasons for not doing it. However, after a bit of research it looks like I am misremembering https://stackoverflow.com/a/25198454/2794702.

Use String.split() to get key/value pairs from the string, rather than regex.

camallen · 2018-01-30T10:08:09Z

By storing the token we increase the attack surface for clickjacking and XSS attacks, as described in the issue you linked above @simoneduca. This is one consideration when implementing the login flows on our apps.

Also the bearer token is a JWT (https://jwt.io/), it is an encoded & signed payload that includes the user details and the expiry of the token. Check the debugger at the link above and paste in your JWT from the panoptes server. It should include details like:

{
  "data": {
    "login": "username",
    "dname": "username",
    ...
  },
  "exp": 1517312782,
   ...
}

So we can use the token expiration time to modify the renewal behaviour instead of relying on the setTimeout callback. Also all our auth tokens are JWT so we could create some shared code to deal with token renewal for PFE, implicit grant oauth apps, etc instead of dealing with them all differently.

The implicit grant (oauth.js) differs from the way PFE handles authentication. Mostly that it authenticates on another site (signin/panoptes.zooniverse.org) and noting that the implicit flow doesn't return a refresh token (used to get a new JWT when the current one expires in PFE). Implicit flows used iframes to request a new token but rely on the session cookie with panoptes to verify they are allowed, blocking these cookies (3rd party) breaks the flow with iframes as their is no session cookie passed so panoptes thinks there is no session :(

We allow PFE to use the credentials flow (submit user creds directly) to gain access to JWT token & refresh tokens, thus zooniverse.org doesn't suffer from the 3rd party cookie issue, though it all runs on the same domain so it would be ok if we did.

We could change our implicit apps to use the credential flows as they get refresh tokens this way and thus avoid the use of iframes / consolidate on one flow / code base. I can't remember the reason why we moved to implicit for custom apps, perhaps to simplify / standardize the authentication code ?

Alternatively we can build out he implicit flows with detection / fallback for 3rd party cookie blocking. As the first login flow uses full page redirects to return the JWT not an iframe, then if i have a JWT but i can't refresh it using iframe them i'm most likely blocking 3rd party cookies. Noting that Jim has debugged this and it appears that the sigin bug i described in the original issue is due to the JWT returned in the hash fragment is ignored and we request one again from an iframe :sadpanda:.

Noting that the UX on blocked 3rd party cookies is poor, notify the user is being logged out, save work? and force them to login again (or whitelist the blocked domain?) through page redirects. Though i am not sure how best to avoid this for blocked sites, maybe just warn they will be logged out in x seconds due to blocking our login cookies...?

I hope the above is useful, happy to clarify the oauth flows / options we have as it can be quite confusing.

eatyourgreens · 2018-01-30T10:19:54Z

Any thoughts on the code in this PR? Before I do any more work on it.

camallen

The authentication code flows seem a little confused but i think this will works fine to solve the borked can't login issue.

However, I don't this this will renew a token correctly once it has expired as it'll gets the data from local storage and returns that if it is set. happy to be wrong but will this break long sessions on existing apps?

camallen · 2018-01-30T10:53:10Z

lib/oauth.js

        this._handleBearerToken(tokenDetails);
+        SESSION_STORAGE.setItem(LOCAL_STORAGE_PREFIX + 'tokenDetails', JSON.stringify(tokenDetails));

        // And redirect to the desired page
        var url = ls.get(LOCAL_STORAGE_PREFIX + 'redirectUri');


Is the redirect straight after this local storage fetch for a saved page location that a user want's to access? E.g. /#!/transcribe ? I'm not sure why we redirect again after the authentication server round trip? Perhaps the redirectUri name is confusing as it's also used in the oauth authentication flow..

I'd assume so, from the comments in the code. I don't know why it requests a new URL from the server, rather than using pushState to update the history without reloading the page.

By the way, location.assign() can't be passed a relative URL. It must be passed a fully qualified URL. This is what is causing the sign-in bug, because the browser will throw away the bearer token when the page refreshes, and initialise the JS app from scratch.

My best guess as to why the code is written like this would be that this library doesn't know how the client app is set up, so doesn't know how to call a new page short of making a new page request. For example, would a React app need different code here from an Angular app and, if so, why should this library suddenly have to know about javascript app frameworks and how they navigate between views? A better oauth API might do the token initialisation, then leave it up to the calling app to decide how to navigate back to the original view and re-render the page.

eatyourgreens · 2018-01-30T11:02:17Z

The authentication code flows seem a little confused but i think this will works fine to solve the borked can't login issue.

Tell me about it! The bug turns out to be really simple, but understanding the code had me going around in circles for a bit. I think the code could be cleaned up a lot, but that might be a long job and I wasn't sure how quickly a fix is needed for the inability to log in.

However, I don't this this will renew a token correctly once it has expired as it'll gets the data from local storage and returns that if it is set. happy to be wrong but will this break long sessions on existing apps?

I'm using sessionStorage, so the token gets deleted when the window closes. Long, continuous sessions should, in theory, be killed by the javascript timeout that's set when you first log in but I haven't tested this.

EDIT: I should add that my reservation about using sessionStorage is that, unlike session cookies, it doesn't persist across windows or tabs. So you'd still have to log in again on each new tab that opens.

eatyourgreens · 2018-01-30T11:26:12Z

A quick note on security:

By storing the token we increase the attack surface for clickjacking and XSS attacks, as described in the issue you linked above @simoneduca. This is one consideration when implementing the login flows on our apps.

Window Storage objects can only be read on the domain that set them eg. javascript code loaded from zooniverse.org or facebook.com or wherever to a page on shakespearesworld.org could not read the stored token.

camallen · 2018-01-30T14:15:25Z

I wasn't sure how quickly a fix is needed for the inability to log in.

It's been broken for a long time, it's not super urgent but something to fix and the current behaviour it's not great UX from us.

Long, continuous sessions should, in theory, be killed by the javascript timeout that's set when you first log in but I haven't tested this.

This code is so entangled it confusing to get the flow right in my head...here is my take on this PR (please correct me where it's wrong). Assuming a blanks slate (no data in session store):

If i log in via the signin button, the changes in this PR will get my token from the URL hash fragment and stored them in local storage. Awesome i can now signin!
At the same time it will schedule a callback to refreshBearerToken in 1hr 59 mins (2 hrs default validity from server).
Lets say I work on transcriptions for the 1hr 59 mins, the callback fires and calls checkForPanoptesSession which retrieves my current token details from session storage and by default will return the expiring / expired token and reset it in local storage and wire up the client auth headers to use this expired token and repeat steps 2-3.

If what i believe is happening in 2-3 is correct in this PR change, then our sites will effectively log the user out after 1hr 59 mins but will not update the UI so to users they will appear logged in. All subsequent API interactions will be not logged in, new subjects fetch, classification submission, etc. Talk will work fine as it's a redirect to zooniverse.org so users will report they worked on a subject but it won't be recorded for them.

This may be already happening if there is a delay between the setTimeout firing compounded by slow network / server not responding, etc. Also i'm not sure how the current code handles a missing server (gateway timeout or something similar).

eatyourgreens · 2018-01-30T14:24:07Z

You're right. Somewhere in the current code, it should be calling either signOut() or _deleteBearerToken before trying to refresh the token when the timeout expires. At that point, either the iframe works and refreshes the token, or the site just signs you out.

I don't think there's any way, with the API defined in this code, for the client app to be notified that you've been signed out. That would probably require a callback to be set when the app initialises. There might be an auth change event that Shakespeare's World could listen to, to detect when the user has been logged out.

Oddly, I'm glad I'm not the only one who finds this code hard to follow. I was starting to think I was just missing something here when I couldn't quite understand what was going on.

eatyourgreens · 2018-01-30T14:28:47Z

Should _refreshBearerToken call _deleteBearerToken() before it checks for a new session? That would be a simple change.

panoptes-javascript-client/lib/oauth.js

Lines 242 to 250 in 85bdc16

    
           _refreshBearerToken: function() { 
        
             return this._checkForPanoptesSession() 
        
               .then(function(tokenDetails) { 
        
                 return this._handleBearerToken(tokenDetails); 
        
               }.bind(this)) 
        
               .catch(function (error) { 
        
                 console.info('Panoptes session has expired'); 
        
               }); 
        
           },

eatyourgreens · 2018-01-30T14:30:46Z

Once again we're swallowing errors and printing an innocuous message instead 😠

panoptes-javascript-client/lib/oauth.js

Lines 247 to 249 in 85bdc16

    
           .catch(function (error) { 
        
             console.info('Panoptes session has expired'); 
        
           });

camallen

This is good as it stands and fixes the original broken login issue when third party cookies are disabled.

Noting that users that block 3rd party cookies will appear logged in but will actual be logged out on cookie expiration as it stands.

camallen · 2018-01-30T15:58:51Z

lib/oauth.js

@@ -240,8 +249,10 @@ module.exports = new Model({
  },

  _refreshBearerToken: function() {
+    this._deleteBearerToken();


If we unset the _currentUserPromise like

panoptes-javascript-client/lib/oauth.js

Line 122 in 85bdc16

_currentUserPromise: Promise.resolve(null),

we may be able to signal to the calling app that the authentication renewal failed.

If we can get the the calling app react to this state we can update the UI. I don't think we can do this in SW / Annotate via auth factory without a code change.

That promise resolves when checkCurrent() resolves, as far as I can tell. Once a promise is resolved, further changes don't affect it. So, I think this line here cancels any calls to checkCurrent() that are in progress from the client app, by immediately resolving the session check with a null value.

srallen · 2018-01-30T20:52:05Z

This is working with edu API front end with 3rd party cookies disabled in chrome, though, it may need some UI work similar to the other apps to signal to re-sign in. Fixing the google sign in will be more involved because I forgot that it's basically impossible to test on staging because of this unresolved issue: zooniverse/panoptes#2024

eatyourgreens · 2018-01-31T11:07:13Z

One more small change, to report that error that's currently being swallowed, then I'm going to merge this and publish a minor version bump for the client.

eatyourgreens requested a review from camallen January 26, 2018 18:33

eatyourgreens commented Jan 26, 2018

View reviewed changes

Persist bearer token details across page reloads

e3bdc3c

Use window session storage to store the current bearer token across page reloads, in case 3rd party cookies are blocked.

eatyourgreens force-pushed the persist-bearer-token branch from 977666e to e3bdc3c Compare January 26, 2018 18:35

eatyourgreens mentioned this pull request Jan 27, 2018

Login not working zooniverse/shakespeares_world#347

Closed

Don't swallow cross-domain security errors

23da6e6

Console warn if the iframe fails, rather than swallowing the original error and throwing a new one.

simoneduca reviewed Jan 29, 2018

View reviewed changes

Specify Storage object in a config variable

941247d

eatyourgreens force-pushed the persist-bearer-token branch from 2f9cbde to 941247d Compare January 29, 2018 17:41

Rewrite parseUrl

c425ec9

Use String.split() to get key/value pairs from the string, rather than regex.

camallen reviewed Jan 30, 2018

View reviewed changes

Delete expired token before refreshing

49f3aae

eatyourgreens mentioned this pull request Jan 30, 2018

Client creates a new token for each page load/refresh #53

Open

camallen approved these changes Jan 30, 2018

View reviewed changes

camallen mentioned this pull request Jan 30, 2018

3rd party cookies disabled can't renew a token - auth code overhaul required. #76

Open

eatyourgreens added 2 commits January 31, 2018 11:08

Log refresh token errors

a87712d

2.9.0

f5983e2

eatyourgreens merged commit 55fd079 into master Jan 31, 2018

eatyourgreens deleted the persist-bearer-token branch January 31, 2018 11:10

Persist bearer token details across page reloads #75

Persist bearer token details across page reloads #75

Conversation

eatyourgreens commented Jan 26, 2018

eatyourgreens Jan 26, 2018

Choose a reason for hiding this comment

eatyourgreens commented Jan 29, 2018

simoneduca commented Jan 29, 2018

eatyourgreens commented Jan 29, 2018

eatyourgreens commented Jan 29, 2018

simoneduca commented Jan 29, 2018

eatyourgreens commented Jan 29, 2018 • edited Loading

simoneduca commented Jan 29, 2018 • edited Loading

eatyourgreens commented Jan 29, 2018

eatyourgreens commented Jan 29, 2018

simoneduca commented Jan 29, 2018

eatyourgreens commented Jan 29, 2018

simoneduca commented Jan 29, 2018

eatyourgreens commented Jan 29, 2018

eatyourgreens commented Jan 29, 2018

simoneduca commented Jan 29, 2018

simoneduca Jan 29, 2018

Choose a reason for hiding this comment

eatyourgreens Jan 29, 2018

Choose a reason for hiding this comment

eatyourgreens commented Jan 29, 2018

simoneduca commented Jan 29, 2018

camallen commented Jan 30, 2018

eatyourgreens commented Jan 30, 2018

camallen left a comment

Choose a reason for hiding this comment

camallen Jan 30, 2018

Choose a reason for hiding this comment

eatyourgreens Jan 30, 2018

Choose a reason for hiding this comment

eatyourgreens commented Jan 30, 2018 • edited Loading

eatyourgreens commented Jan 30, 2018

camallen commented Jan 30, 2018

eatyourgreens commented Jan 30, 2018

eatyourgreens commented Jan 30, 2018

eatyourgreens commented Jan 30, 2018

camallen left a comment

Choose a reason for hiding this comment

camallen Jan 30, 2018

Choose a reason for hiding this comment

eatyourgreens Jan 31, 2018

Choose a reason for hiding this comment

srallen commented Jan 30, 2018

eatyourgreens commented Jan 31, 2018

eatyourgreens commented Jan 29, 2018 •

edited

Loading

simoneduca commented Jan 29, 2018 •

edited

Loading

eatyourgreens commented Jan 30, 2018 •

edited

Loading