-
-
Notifications
You must be signed in to change notification settings - Fork 417
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
use callback_url without query_string when building access_token #205
Conversation
@zquestz when will be merged? |
We have resolved the issue a different way. We have locked omniauth-oauth2 to ~> 1.3.1 and are not upgrading until we hear from the maintainer of that project. If this is a permanent change in the omniauth-oauth2 library then we will consider this PR. |
@@ -100,14 +100,18 @@ def custom_build_access_token | |||
elsif verify_token(request.params['id_token'], request.params['access_token']) | |||
::OAuth2::AccessToken.from_hash(client, request.params.dup) | |||
else | |||
orig_build_access_token | |||
verifier = request.params["code"] | |||
client.auth_code.get_token(verifier, get_token_options(callback_url), deep_symbolize(options.auth_token_params)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: we should use the same coding style as above.
deep_symbolize(options.auth_token_params || {})
This change set looks good. When we move to supporting 1.4 then this will be merged if they don't address the issue upstream. The changes in 1.4 broke a lot of people, and it is very possible some things will be fixed in the next release. |
There is a much cleaner fix on this thread: omniauth/omniauth-oauth2@2615267 Merely need to add:
Probably going to just go that route as that claims to work and is a much smaller change. |
I lied, just went through and tested their fix and it flat out doesn't work to resolve the issue. It causes an issue with the oauth2 gem itself. I am going to go with this fix as I think your solution is actually the correct one. Thanks for the PR! |
@zquestz any luck with this? |
This was fixed in master by locking to an older version of omniauth-oauth2. I need to a bit more testing on this before I merge, but it should be soon, although the need is not immediate. |
K going to merge. Prepping a new release and going to update to omniauth-oauth2 1.4. |
use callback_url without query_string when building access_token
This reverts 50b067a. The incompatibility with omniauth-oauth 1.4.0 was fixed in zquestz/omniauth-google-oauth2#205 and released in version 0.3.0
This reverts 50b067a. The incompatibility with omniauth-oauth 1.4.0 was fixed in zquestz/omniauth-google-oauth2#205 and released in version 0.3.0
icoretech/omniauth-spotify#8 (comment) zquestz/omniauth-google-oauth2#205 Google OAuth 2 talks about the problem with new Omniauth version's callback_url. Manually specific the callback url to remove the query string
after
callback_url
override was removed in gemomniauth-oauth2
in version 1.4.0omniauth/omniauth-oauth2@2615267
there is a
build_access_token
error:this error can be fixed with
callback_url
override, similarly as it was in previous versions of the gemomniauth-oauth2