Add checkBearerToken() to OAuth #80

eatyourgreens · 2018-02-12T11:34:29Z

Towards #76

Stores an expires_at timestamp with new tokens. Adds a checkBearerToken method that can be used by client apps to check the token, and refresh it if possible.

Tries, as far as possible, to copy the equivalent methods in the Auth module.

oauth.checkBearerToken()
.then(function (token) {
  // I'm probably still logged in
})
.catch(function (error) {
  // refreshing the token failed
}

Keep method names consistent with the Auth module.

Calculate an expires_at timestamp and store it with the new token.

Add a public method to check if the client has a current, valid token. Also add an internal method to check if the stored token has expired.

eatyourgreens · 2018-02-12T11:35:44Z

@shaunanoordin this might help towards your problem with long transcription sessions on Anti-Slavery Manuscripts.

simoneduca · 2018-02-12T11:42:35Z

lib/oauth.js

@@ -155,6 +165,15 @@ module.exports = new Model({
    ].join('');
  },

+  _bearerTokenIsExpired: function() {


Do we need a new method to check if token is expired? One already existed.

zooniverse/shakespeares_world#366 (comment)

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.

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

It never worked, and nothing seems to use it.

simoneduca · 2018-02-12T13:30:52Z

I'm going to test with SW as part of the review.

simoneduca · 2018-02-12T14:23:30Z

I haven't waited to see the refresh failure, but this seems to work well. The way I was thinking of handling the failure in SW is to logout the user.
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. Thoughts?

simoneduca

LGTM

simoneduca · 2018-02-12T14:29:20Z

I realised the above discussion belongs on the SW PR.

camallen

The check hook and renew flow LGTM

camallen · 2018-02-13T12:25:15Z

lib/oauth.js

+  _bearerTokenIsExpired: function() {
+    var tokenDetails = JSON.parse(SESSION_STORAGE.getItem(LOCAL_STORAGE_PREFIX + 'tokenDetails'));
+    if (tokenDetails) {
+      return Date.now() >= tokenDetails.expires_at - TOKEN_EXPIRATION_ALLOWANCE;


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

I don't know. What's a reasonable time to allow for Panoptes to respond?

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?

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.

eatyourgreens · 2018-02-13T12:58:15Z

Published as 2.9.2

eatyourgreens added 3 commits February 12, 2018 11:16

Rename _handlerBearerToken to handleNewBearerToken

a00220c

Keep method names consistent with the Auth module.

Add expires_at and store new token

a98a643

Calculate an expires_at timestamp and store it with the new token.

Add checkBearerToken()

9a1cab1

Add a public method to check if the client has a current, valid token. Also add an internal method to check if the stored token has expired.

eatyourgreens requested review from camallen and simoneduca February 12, 2018 11:34

simoneduca reviewed Feb 12, 2018

View reviewed changes

Remove isTokenStillValid

9bba154

It never worked, and nothing seems to use it.

simoneduca approved these changes Feb 12, 2018

View reviewed changes

eatyourgreens mentioned this pull request Feb 12, 2018

2 hour OAuth token refresh timer is probably unreliable #81

Open

camallen approved these changes Feb 13, 2018

View reviewed changes

eatyourgreens added 2 commits February 13, 2018 12:33

Increase the expiration allowance to 5 minutes

eaaabdf

2.9.2

1d61595

eatyourgreens merged commit 8dbd507 into master Feb 13, 2018

eatyourgreens deleted the oauth-check-token branch February 13, 2018 12:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add checkBearerToken() to OAuth #80

Add checkBearerToken() to OAuth #80

eatyourgreens commented Feb 12, 2018 •

edited

Loading

eatyourgreens commented Feb 12, 2018

simoneduca Feb 12, 2018

eatyourgreens Feb 12, 2018

eatyourgreens Feb 12, 2018

simoneduca Feb 12, 2018

simoneduca commented Feb 12, 2018

simoneduca commented Feb 12, 2018

simoneduca left a comment

simoneduca commented Feb 12, 2018

camallen left a comment

camallen Feb 13, 2018

eatyourgreens Feb 13, 2018

camallen Feb 13, 2018

eatyourgreens Feb 13, 2018

eatyourgreens commented Feb 13, 2018

Add checkBearerToken() to OAuth #80

Add checkBearerToken() to OAuth #80

Conversation

eatyourgreens commented Feb 12, 2018 • edited Loading

eatyourgreens commented Feb 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simoneduca commented Feb 12, 2018

simoneduca commented Feb 12, 2018

simoneduca left a comment

Choose a reason for hiding this comment

simoneduca commented Feb 12, 2018

camallen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eatyourgreens commented Feb 13, 2018

eatyourgreens commented Feb 12, 2018 •

edited

Loading