Skip to content

Devise is hard-coded to OmniAuth 1.x.x #5326

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

Closed
BobbyMcWho opened this issue Jan 6, 2021 · 18 comments · Fixed by #5327
Closed

Devise is hard-coded to OmniAuth 1.x.x #5326

BobbyMcWho opened this issue Jan 6, 2021 · 18 comments · Fixed by #5327
Assignees

Comments

@BobbyMcWho
Copy link

Hello, maintainer of OmniAuth here.

I’ve been floating a release candidate of OmniAuth 2.0.0, and had a user report this

omniauth/omniauth#1017 (comment)

Any chance a maintainer here would be willing to assist me in relaxing that restriction and testing it with the release candidate?

@carlosantoniodasilva
Copy link
Member

@BobbyMcWho hey, I'm happy to help enable OmniAuth 2.x support in Devise.

Is there any possible breaking change we should be aware of that you could point me to? I'll look into allowing 2.x so we can run the build and see if it all passes.

Thanks for reporting.

@carlosantoniodasilva
Copy link
Member

I have #5327 to allow 2.x to be installed (so we won't blow up in that case), but I'm locked to try upgrading the gem in our Gemfile due to omniauth-oauth2 requiring OmniAuth 1.x still. I think our tests are using that, I'll look into that later, but this should at least let someone test out this new branch with OmniAuth 2.

@BobbyMcWho
Copy link
Author

Thanks @carlosantoniodasilva, I do have a branch under my own repo that allows that version https://github.com/omniauth/omniauth-oauth2/pull/134/files, but I will be releasing 2.0 when I have a chance as well.

@BobbyMcWho
Copy link
Author

Here are the release notes for the release candidate, I am not super familiar with how Devise uses OmniAuth, so unsure if it will affect the typical configuration https://github.com/omniauth/omniauth/releases/tag/v2.0.0-rc1

@BobbyMcWho
Copy link
Author

I have released OmniAuth-oauth2 v1.7.1 that allows v2.0.0 to be used, and I have also released omniauth v2.0.0

@carlosantoniodasilva
Copy link
Member

Well that was fast @BobbyMcWho, thanks for the info here. I'll double check our tests are okay with the new version and if it's all good I can look into releasing a patch update enabling that.

@BobbyMcWho
Copy link
Author

Yeah, I had the release candidate discussion open for 31 days, so I figured it was time. I'm happy to help out if it ends up that Devise needs any changes for it

@carlosantoniodasilva
Copy link
Member

carlosantoniodasilva commented Jan 12, 2021

@BobbyMcWho no worries, I'm behind on things here.

It looks like omniauth-openid is also set to OmniAuth ~> 1.0: https://github.com/omniauth/omniauth-openid/blob/8a46fa98dabe3ea7ced336fef1201ab4870adb15/omniauth-openid.gemspec#L5, is that something on your radar?

It's just prevented me from updating the bundle and test against 2.0, but I think only our test suite relies on it and I can remove and/or move it to something else probably.

@BobbyMcWho
Copy link
Author

I'll have to take a look, it looks like that library hasn't had activity in 3 years

@carlosantoniodasilva
Copy link
Member

No worries, thanks for your help... if bumping that doesn't really work/make sense I can move to something else for our tests.

I commented those out to run the suite now and got a handful of failures, I'll take a closer look at those in the meantime.

@carlosantoniodasilva
Copy link
Member

carlosantoniodasilva commented Jan 22, 2021

@BobbyMcWho hopefully last question for you before I work on the release of a new version soon :)

It sounds like it's extremely discouraged, but still possible, to use GET requests to initiate the OmniAuth flow, right? Devise generate both routes allowing GET/POST, and I'm inclined to keep that for now. Alternatively I guess I could look at OmniAuth.config.allowed_request_methods and use that, so it only generates what's configured for OmniAuth.

That only applies to the initial request, right?

@BobbyMcWho
Copy link
Author

@BobbyMcWho hopefully last question for you before I work on the release of a new version soon :)

It sounds like it's extremely discouraged, but still possible, to use GET requests to initiate the OmniAuth flow, right? Devise generate both routes allowing GET/POST, and I'm inclined to keep that for now. Alternatively I guess I could look at OmniAuth.config.allowed_request_methods and use that, so it only generates what's configured for OmniAuth.

That only applies to the initial request, right?

Sorry, I've had quite the busy day.

Right, if folks want to override the default POST only and use GET as well they can, they'll be met with a big ol' warning in their logs on every auth request unless they config to shut it off and acknowledge it.

And correct, the allowed methods are checked during the request phase check

@carlosantoniodasilva
Copy link
Member

carlosantoniodasilva commented Jan 25, 2021

Perfect, thanks again for the help @BobbyMcWho. And don't worry, I can totally relate on the busy days. ;)

I'm hoping to get a new release out at some point this week with the changes included.

@jpowell
Copy link

jpowell commented Jan 29, 2021

Any update here? Thank you!

Update: Saw your PR + comments. Thanks again for your updates!

@carlosantoniodasilva
Copy link
Member

This is on master branch now, please feel free to bundle from there while we prepare a new release soon. (there's a few other things to wrap up before it.) Thanks all.

@emancu
Copy link

emancu commented Feb 10, 2021

@carlosantoniodasilva is there an ETA for the new release?

@carlosantoniodasilva
Copy link
Member

@emancu no ETA yet, but I hope to be releasing it at some point this month, just need to take a look at a couple other things first that may go with it.

@emancu
Copy link

emancu commented Feb 10, 2021

Awesome, thanks for the quick reply. No rush, just wondering :)

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

Successfully merging a pull request may close this issue.

4 participants