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

Remove query string from redirect_uri on callback #221

Merged
merged 1 commit into from
Oct 27, 2015
Merged

Remove query string from redirect_uri on callback #221

merged 1 commit into from
Oct 27, 2015

Conversation

gioblu
Copy link
Contributor

@gioblu gioblu commented Oct 22, 2015

No description provided.

@mkdynamic
Copy link
Collaborator

Thanks for the PR. Can you explain what this is fixing?

@gioblu
Copy link
Contributor Author

gioblu commented Oct 22, 2015

Hi, after a while debugging with byebug with a friend (that is the real genious, who found the solution), we found that the callback uri generated by the super call contains also the code parameter and for this reason facebook bounces (probably they switched to a different policy over callback uri??) in any case without parameter works. Hope someone will discover what happened facebook side.

@jakoss
Copy link

jakoss commented Oct 22, 2015

Yes, this PR fixes problem with logging with newest version of facebook api

#220

@vemv
Copy link

vemv commented Oct 24, 2015

Hey @gioblu, thanks for this fix. I'm using it.

It would be ideal though that the fix was backed by a test, else the PR is incomplete.

@mkdynamic
Copy link
Collaborator

If folks want to keep the querystring (i.e. they specify that in the Facebook app config) then this would break that, I think. Is that correct? I think perhaps it would be better to use the existing callback_url option and specify the callback_url to solve this issue... Thoughts?

@vemv
Copy link

vemv commented Oct 24, 2015

Sounds good, but by default omniauth should 'just work' I believe! No config required

@jonatassalgado
Copy link

@mkdynamic I use with Devise and Rails, and I did what you suggested to configure callback_url, and work!

After some adjusts in Facebook > Settings, and adding the callback url to Valid OAuth redirect URIs.

After did this I received error csrf_detected, then adjust info_fields.

The result (in my case only in devise.rb):

config.omniauth :facebook, ENV['FACEBOOK_APP_ID'], ENV['FACEBOOK_APP_KEY'],
                  image_size: 'square',
                  scope: 'email, user_friends',
                  info_fields: 'name,email',
                  callback_url: 'http://www.mysite.com/users/auth/facebook/callback'

@robban
Copy link

robban commented Oct 26, 2015

Thanks, worked for me. I also had to add the info_fields: 'name,email' to the config since facebook does not seem to be sending email by default anymore.

@tyler-ament
Copy link

I had the same issue of this happening (today) without any of my code changing, and the fix detailed by @robban worked for me. Rails 4.2.4 app using Devise and OmniAuth.

@vemv
Copy link

vemv commented Oct 26, 2015

I see that the recommended fix is trivial to add to one's project, but improving omniauth-facebook to work without this config needed seems easy too.

Having to add callback_url: 'http://www.mysite.com/users/auth/facebook/callback' kinda sucks because it's env-dependent, so it's more stuff one has to take care of.

@mkdynamic
Copy link
Collaborator

Thanks for the thoughts everyone, I'm sold this is a good fix -- agree things should work out the box. It is a backwards incompatible change, technically speaking, so it will make sense bump the major version when releasing. That seems like a good trade-off.

Could you add a test case to cover this? @gioblu

I'll try to get this merged and push a new release later this evening.

mkdynamic added a commit that referenced this pull request Oct 27, 2015
Remove query string from redirect_uri on callback
@mkdynamic mkdynamic merged commit 5a74e8b into simi:master Oct 27, 2015
@mkdynamic
Copy link
Collaborator

Fix released in 3.0.0.

Thanks for the contribution @gioblu and feedback everyone else.

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.

7 participants