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

Add omniauth gem #151

Merged
merged 7 commits into from
Jun 4, 2024
Merged

Add omniauth gem #151

merged 7 commits into from
Jun 4, 2024

Conversation

leefaisonr
Copy link
Contributor

No description provided.

Copy link
Contributor

@jrgriffiniii jrgriffiniii left a comment

Choose a reason for hiding this comment

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

I apologize, this is currently breaking for me in staging: https://orcid-staging.princeton.edu/sign_in. I can investigate Honeybadger to determine the source of the error.

jrgriffiniii and others added 2 commits May 31, 2024 12:24
Devise and OmniAuth routes for `session_path` and CAS authentication.

Co-authored-by: Robert-Anthony Lee-Faison <[email protected]>
@jrgriffiniii jrgriffiniii self-requested a review May 31, 2024 19:21
Copy link
Contributor

@jrgriffiniii jrgriffiniii left a comment

Choose a reason for hiding this comment

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

I have just tested this using a deployment on https://orcid-staging.princeton.edu, and I have confirmed that this is working for me.

Copy link
Member

@carolyncole carolyncole left a comment

Choose a reason for hiding this comment

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

This seems to work. That said this solution on the wiki looks a bit more straight forward https://github.com/omniauth/omniauth/wiki/Upgrading-to-2.0#rails. It also seems like the solution in the PR will allow ANY token and that may not be great security wise. So @leefaisonr I'm not going to hit the big green button, but if you are done with this work feel free to. We could ticket trying to make it more secure... The other thing I saw on the internet was that the login link should be a post, which might be worth a try

@jrgriffiniii jrgriffiniii merged commit 4f54896 into main Jun 4, 2024
4 of 5 checks passed
@jrgriffiniii jrgriffiniii deleted the omniauth-update branch June 4, 2024 14:10
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.

4 participants