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

Doorkeeper 4.4.1 compatibility #103

Merged
merged 1 commit into from
Aug 14, 2018
Merged

Conversation

shaunanoordin
Copy link
Member

PR Overview

The doorkeeper dependency in Panoptes has been bumped up to 4.4.1, which changes the bearer token to the Bearer token (capital B). This change means that users of Custom Front End projects won't be able to sign in, as the panoptes-client won't recognise the (lower-case) bearer token being passed back to the app.

This PR fixes the issue by changing the check to accept either token_type=Bearer or token_type=bearer.

Status

Ready for review, and more thorough testing needed; I'm not sure if I'm missing anything beyond the sign-in problems on localhost that I'm experiencing.

@shaunanoordin
Copy link
Member Author

Notes

  • At the moment (Tue 14 Aug 2018 at 14:15 BST) only Panoptes.Staging uses the new Bearer token, whereas Panoptes.Production uses the old bearer token. This means that only CFE projects on localhost/staging will have signing-in issues, whereas CFE projects on production will look OK, at least for the time being.
  • The Regex fix in this patch should allow CFE projects to continue working during the transition period as Panoptes-Production moves from doorkeeper 4.4.0 to 4.4.1. Future updates to panoptes-client can revert the check back to the old String.indexOf code.

WARNING: once Panoptes.Production uses the new doorkeeper, several other CFE projects may need to update to the latest panoptes-client. We need to follow up on this.

@shaunanoordin
Copy link
Member Author

有難うございます , Roger-さん 👍 I still need some help with merging this pull request, as I unfortunately (and strangely) don't have write access to this repo? 😵 🤷‍♂️ Dangit Github, I have enough authority to push this branch up, what is up with your settings??

Also, I skipped this earlier, but here's the testing section for documentation:

Testing

To test that this fix works,

  1. Try the baseline.
  • Do a clean install of zoo-reduxify and its dependencies. panoptes-client should install ~2.11.0
  • Access localhost:3000, and attempt to sign in once taken to panoptes-staging.
  • Expected: user is returned to localhost, logged in.
  • Actual: user is returned to localhost, but not logged in.
  1. Then try the fix
  • change the package.json to import "panoptes-client": "zooniverse/panoptes-javascript-client#doorkeeper-compatibility" then reinstall all dependencies.
  • Access localhost:3000, and attempt to sign in once taken to panoptes-staging.
  • Expected: user is returned to localhost, logged in.

Looks good on OSX+Chrome68, but this is dependent on the external setup of panoptes-staging

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

Successfully merging this pull request may close these issues.

3 participants