Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Classifier: Check bearer token on annotation update #2563

Merged
merged 3 commits into from
Dec 1, 2021

Conversation

eatyourgreens
Copy link
Contributor

@eatyourgreens eatyourgreens commented Nov 23, 2021

When we update an annotation, call authClient.checkBearerToken(). checkBearerToken() will refresh the current token if it's close to expiring, allowing volunteers to stay authenticated for another two hours.

This should help volunteers stay logged in on projects with lengthy workflow tasks, like Scarlets & Blues. annotation.update() is used by all workflow tasks except creating marks from purple transcribed lines for the transcription task.

Package:
lib-classifier

Review Checklist

General

  • Are the tests passing locally and on Travis?
  • Is the documentation up to date?

Components

Apps

  • Does it work in all major browsers: Firefox, Chrome, Edge, Safari?
  • Does it work on mobile?
  • Can you yarn panic && yarn bootstrap or docker-compose up --build and app works as expected?

Publishing

  • Is the changelog updated?
  • Are the dependencies updated for apps and libraries that are using the newly published library?

Post-merging

When we update an annotation, check for expiring tokens and refresh them.
@eatyourgreens
Copy link
Contributor Author

I think this will be superseded by zooniverse/panoptes-javascript-client#149, which fixes token refresh so that it works even if we leave several hours between checking the token status.

Copy link
Member

@shaunanoordin shaunanoordin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review

Package: lib-classifier
Context: if a user spends a long time per Subject/Classification, their session might expire without any visual indicators.

This PR updates the Classifier so that logged-in users will be less likely to be silently logged out between long classification sessions, and therefore less likely to submit anonymous Classifications.

Code read looks good, and functionality appears to work fine, although I'm unable to do more thorough feature tests at the moment.

Testing

I can confirm that checkBearerToken() is firing as expected, but trying to confirm that checkBearerToken() will ensure a user's login state will continue to be refreshed will require far more testing time to simulate properly.

Dev Notes

  • This update works by adding a bearer token check (i.e. "are you still logged in?" check) every time an annotation updates (e.g. when a Single Question Task has an answer selected, or on every key press of a Text Task)
  • checkBearerToken() works as follows:
    • When a user logs in, a token (read: blob of data) gets saved on local storage. This token has an expiry date on it.
    • When checkBearerToken() is called, it first checks if there's a valid un-expired token in local storage. If yes, that token is returned. (No further network calls are required.)
    • If no, a network call is made to fetch a fresh token from the (Panotes?) login/auth service.

Possible caveats:

  • The implementation of checkBearerToken() doesn't guarantee that a valid refreshed token will be returned by the time the user submits the classification, but it sure as heck tilts the odds in favour of a logged-in state.

Advanced notes:

  • checkBearerToken() returns a Promise, and can be chained into something like... authClient?.checkBearerToken().then(token => { if (user_is_logged_in_BUT_no_token_is_returned()) { then_alert_user_or_something() } })
  • That's the more advanced implementation if want to implement a more guaranteed "you can't submit unless you're logged in properly" check, but that's way, way, way beyond the scope of anything we'd want to tackle right now.

Status

LGTM 👍 I say we get this merged & deployed so we can see that this improves volunteers' experience on those long-classification transcription projects.

@github-actions github-actions bot added the approved This PR is approved for merging label Dec 1, 2021
@eatyourgreens
Copy link
Contributor Author

As long as you have a valid Panoptes session cookie, token refresh requests should always work (now that the auth client is fixed in PJC v3.3.4.) I think the session cookie lasts for about two weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved This PR is approved for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants