Add token refresh flow.

[simoneduca]

Not working yet, just a proof of concept. The first I was trying to get working was calling the refresh client method after 20 seconds the project loads the home page. But if you check the logs or the network call on preview we get nothing back.

[eatyourgreens]

SW uses oauth.js, not auth.js, to get the access token, which is why you're getting nothing back from auth.checkBearerToken(). Auth is the module that PFE uses to log in and out, reset passwords and register new volunteers with Panoptes.

[simoneduca]

I thought I could just import the Auth module, but obviously that doesn't work. @eatyourgreens Am I right in thinking that there are no methods in OAuth currently that check sessions/refresh it if needed that don't involve an iframe (which isn't used anymore, since your latest changes to the client)?

[eatyourgreens]

Nothing in my changes touched the iframe code, so it's still in use for refreshing tokens, if you allow it in your browser settings.

One solution would be to mirror the call that the client makes before every request with the Auth module. https://github.com/zooniverse/panoptes-javascript-client/blob/965580bba1ed57280482a11e3a3ff30b2c9cce2d/lib/api-client.js#L10-L13

apiClient.beforeEveryRequest = function() {
    return oauth.checkBearerToken();
  },

but you're right, the OAuth module doesn't have a checkBearerToken method.

[simoneduca]

@eatyourgreens wouldn't that solution still rely on allowing iframes in the browser? Or do you mean it as "we need to write a method similar to checkBearerToke in OAuth, which doesn't rely on iframes"?

[eatyourgreens]

@simoneduca zooniverse/panoptes-javascript-client#76 describes authorisation in more detail.

[simoneduca]

Atm OAuth _refreshBearerToken relies on creating an iframe to check the token. I suppose I could just check sessionStorage, since that's available now. Would that make sense?

[simoneduca]

Thanks, re-reading than now.

[eatyourgreens]

oauth.listen(callback) might be the easiest way to be notified when oauth changes. Roger’s documentation for the API client has a description of the listen method, I think.

Just realised there’s a bug in SW’s use of storage too, in that the version number is part of the storage key so that volunteers couldn’t access their stored data if the version number ever changed. Also, the old data would never be deleted. This is a completely separate issue, however.

[simoneduca]

Cool, nice catch. How did you see that? Decoding the token?

[eatyourgreens]

OAuth extends Model, which is an event emitter. There’s a little bit about it in the Readme for json-api-client too.

[eatyourgreens]

The key bug? By looking at the names of the storage keys in the dev console.

[simoneduca]

Yes. Which key? I can only see the keys access_token, expires_in and token-type.

[simoneduca]

Got it, just saw the issue.

[simoneduca]

I was thinking of checking the token validity every time the user submits a classification. I think this would work fine for most projects, but I know some SW users can be on a single subject for more than two hours before they submit a classification. In that situation I think a warning to save their work and login before submitting should be enough. Alternatively, listening on any changes in the token object could work, but I'm not sure where is the best place in the app to set app such listener.

[eatyourgreens]

How about my suggestion earlier in this thread, to run the check before each request? https://github.com/zooniverse/shakespeares_world/pull/369#issuecomment-364184639

[simoneduca]

That's what I'd love to do and I intended my thought of checking the token before submitting classifications in that vein. But obviously I'm missing something. I don't know how to call it on every request. Do you mean to have a OAuth.checkBearerToken() for login, one for loading the app, for submitting the classification, etc.?

[eatyourgreens]

Doesn’t the code in my comment work?

[simoneduca]

Probably, but my question is about where to put it.

[eatyourgreens]

SW is your app, so that’s up to you, but it should be added wherever the API client is first configured.

[simoneduca]

I've staged the latest changes https://preview.zooniverse.org/shakespearesworld/#!/ ~Angular is complaining about a circular dependency's been introduced. Looking at that.~

[simoneduca]

Restaged https://preview.zooniverse.org/shakespearesworld/#!/

[eatyourgreens]

Good discussion of this, and some questions, from @shaunanoordin in https://github.com/zooniverse/anti-slavery-manuscripts/pull/251#issuecomment-365345194

[eatyourgreens]

Shaun points out, in that ASM PR, that my example code is wrong (I don’t understand promises) and gives some pseudo code that will work.

[eatyourgreens]

zooniverse/panoptes-javascript-client#82 fixes some bugs in the client: you can't actually save your work in progress when a refresh fails, and checkBearerToken() sometimes returned undefined.

@shaunanoordin also pointed out that my example usage was wrong. checkBearerToken() always resolves to a valid token or null, so the code to use it would be something like this, where user is the logged-in user. There's no catch block because the client catches the iframe error and returns null:

apiClient.beforeEveryRequest = function() {
  return oauth.checkBearerToken().then(function(token) {
    if (user && token === null) {
      // We are logged in but don't have a token any more.
      // Need to save any unsaved work and redirect to Panoptes for a new token.
    }
  });
}

Anti-Slavery Manuscripts gets the logged-in user from its redux store. Does SW have some way of passing the current user to the callback here?

[simoneduca]

Rebased to include latest client and restaged https://preview.zooniverse.org/shakespearesworld

[simoneduca]

I think this works as expected and fetches a new token on every network requests. However, something I'm not 100% sure how to handle: in case the app goes offline in between requests the UI still shows the user as logged in. Should I change that and log them out?

[simoneduca]

@camallen or @eatyourgreens would you be able to have a look?

[eatyourgreens]

@simoneduca you need to update your code in line with the example I posted in my earlier comment.

[simoneduca]

Struggling with fixing a circular dependency Angular is picking up. I've introduced the loop when checking if user is defined before refreshing token. I'll have to find another way to get user object.

[eatyourgreens]

Should you be setting up the beforeEveryRequest hook in auth.factory.js, after the user has been returned from Panoptes? Somewhere around here (although I'm not sure how the signIn code works.) https://github.com/zooniverse/shakespeares_world/blob/7072728f5cdbd21ba631411f17ef499628dbcc7f/app/modules/auth/auth.factory.js#L15-L20

[simoneduca]

Commit https://github.com/zooniverse/shakespeares_world/pull/369/commits/29b0a2140e117140f80ba958e36d84f2e8010681 contains the latest effort of trying to incorporate a decent UX in the app oauth flow. Unfortunately, it sort of works but it throws an error which I'm struggling to debug. I can login fine and my check is hit the first time. Then, it errors

Which should be trivial to solve, but I can't see what's wrong so far.

The line that throws is on the JSON-api-client.

You can see it here https://preview.zooniverse.org/shakespearesworld/#/

[eatyourgreens]

You're right, that was trivial to solve 😉 zooAPI.beforeEveryRequest is being set to the returned token, I think.

[simoneduca]

D'oh! OK, that's fixed. Bit more info on why this still isn't right. Once I'm logged in, I clear the session storage to catch the if condition. I would expect to get a new token on the next request, but instead I get Failed to get token error. Restaged preview.zooniverse.org/shakespearesworld/#!/

[eatyourgreens]

Sorry, I don't see Failed to get token in your code. Is that error coming from the client?

[eatyourgreens]

Hang on, if you delete the stored token, then the client won’t request a new token, because you’re logged out. The client requests a new token if the stored token has expired, or is about to expire.

[simoneduca]

Of course, thank you. And sorry about my rushed comment above, it was wrong (see strikethrough part). After clearing my session storage and trigger another request, I do get the alert and saving any work as well as redirecting to login page work fine too. No errors 🎉 However, the alert seem to be showing an arbitrary number of times in the scenario just described. Can you reproduce the behaviour? To reproduce:

1. Clear your session, in case you have one active, and login here https://preview.zooniverse.org/shakespearesworld
2. Clear your session
3. Click on one of the genres.

You should see the session alert (you'll see the alert about unsaved work beforehand, if you have ongoing annotations). On OK, you should be redirected to panoptes login page, but sometimes it took me two or three clicks to dismiss the alert.

[simoneduca]

OK, this is weird, but I can't reproduce the multiple clicking on the alert anymore. I'm tempted to say that the flow seems to be working correctly now. Awaiting more feedback, thanks.

[eatyourgreens]

Yeah, you’ve set the alert to appear before every request, so it could appear more than once. I think @shaunanoordin has fixed this in ASM by cancelling the request when a token check fails. Maybe have a look at his code and copy what he’s doing when a session can’t be refreshed.

[eatyourgreens]

I'm checking this out on localhost (with third-party cookies blocked), and I've edited oauth.js to check after 30s instead of two hours. I'm seeing the token refresh happen every 30s, as expected, but I'm still getting a token back despite the iframe error.

Got new bearer token DKSzd8
:3001/#/transcribe:1 Refused to display 'https://panoptes-staging.zooniverse.org/users/sign_in#/' in a frame because it set 'X-Frame-Options' to 'sameorigin'.
oauth.js:261 Got new bearer token DKSzd8

I think that will be fixed by https://github.com/zooniverse/panoptes-javascript-client/pull/86

Note that you can't test session cookies on preview.zooniverse.org, because preview can read zooniverse.org cookies.

[eatyourgreens]

Also, with third party cookies blocked, I've found a bug that blocks the token refresh in 2.9.4. I'm not sure how this slipped past us when we were testing changes in the client. this._deleteBearerToken is not defined at oauth.js:282 because I forgot to set the this context for that block.

Error getting genres TypeError: this._deleteBearerToken is not a function
    at oauth.js:282
    at h (jspdf.min.js:270)
    at f (jspdf.min.js:270)
    at v (jspdf.min.js:270)
    at MutationObserver.a (jspdf.min.js:270)

[eatyourgreens]

If I log out, I get the alert saying my session has expired. Pressing OK then logs me back in, so basically I can't log out without being logged back in.

[eatyourgreens]

I've deployed a version to staging that has checks for a new token every 30s.

[simoneduca]

Thanks for looking at this again @eatyourgreens.

1. If I log out, I get the alert saying my session has expired. Pressing OK then logs me back in, so basically I can't log out without being logged back in.

I can't reproduce this. Are you seeing this locally or on preview?

2. I've deployed a version to staging that has checks for a new token every 30s.

Thanks. You said we can't test token on preview because it reads zooniverse.org tokens. Doesn't this affect testing your latest deploy?

[eatyourgreens]

1. Both locally and on preview
2. Yes. You can only test third party cookies on a third party domain, and preview is on zooniverse.org.

[eatyourgreens]

Just confirmed on preview.zooniverse.org. Log in, then log out and the next API request fires off an alert telling me I'm not logged in, then logs me back in. I left a comment about this on the code that causes it.

[simoneduca]

I can now reproduce on preview, thanks. However I can't locally, because the session storage is not cleared on logout. That also seems to cause inability to login once you're logged out. Mind confirming if you also see that?

[eatyourgreens]

Is that the bug I found in https://github.com/zooniverse/shakespeares_world/pull/369#issuecomment-369893295? I might have that fixed locally, which is why I didn't encounter it.

[simoneduca]

Ah yes, that sounds like it. Let me try to install the client from that branch.

[eatyourgreens]

I can reproduce that alert locally too, even without the client bugfix. Log in, log out and the next API request nags me to log back in again.

[eatyourgreens]

Also, session storage is cleared for me when I log out. This is with version 2.9.4 of the API client.