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

Allow passing a callback_url param #107

Merged
merged 3 commits into from
Feb 7, 2017

Conversation

bzf
Copy link
Contributor

@bzf bzf commented Aug 12, 2016

When building an application we need to do two types of Twitter
authentication - one for signing in to our application (and creating a
User) and another for associating Twitter accounts to their User
account.

By passing a custom callback_url param we can have one controller
method for signing in and another one for the associating part and just
have the application set the callback when starting the authentication
process.

When building an application we need to do two types of Twitter
authentication - one for signing in to our application (and creating a
`User`) and another for associating Twitter accounts to their `User`
account.

By passing a custom `callback_url` param we can have one controller
method for signing in and another one for the associating part and just
have the application set the callback when starting the authentication
process.
@bzf
Copy link
Contributor Author

bzf commented Aug 12, 2016

There seems to be some install errors on CI. It's also dipping under the 94% coverage limit due to not adding a test - should I add one?

Also, when the request comes back from Twitter the request.env["omniauth.auth"] (and lands in a method that's not the default def callback one) value is nil - do you know anything about what might've caused that and if that is expected?

@raysrashmi
Copy link
Collaborator

Thanks for the PR.
Reason behind getting request.env["omniauth.auth"] as nil is because it is not going in to callback_phase method where we assign auth_hash to env['omniauth.auth'] see here. So basically we need to set options[:callback] same as callback_url or either override callaback_path method

@bzf
Copy link
Contributor Author

bzf commented Aug 21, 2016

@raysrashmi Alright, I've tried overriding the callback_path method and I did not get it to work. I've looked through the links you sent but could not understand how everything fits together. Would you have time to pair on this sometime, or help me by pointing me into the right direction? 😃

EDIT: I got it working now, but I'm not sure if accessing the session variable is a good idea. Also it needs some tests, so I'll add some!

@bzf bzf force-pushed the al-allow-custom-callback-url branch 3 times, most recently from 460e43d to 1de0705 Compare August 22, 2016 13:12
@raysrashmi
Copy link
Collaborator

@bzf I think its fine to use session and I don't think there is any other way to get callback_url. 👍 for tests.

@raysrashmi
Copy link
Collaborator

@bzf Any update on this?

@bzf
Copy link
Contributor Author

bzf commented Jan 27, 2017

@raysrashmi Sorry that I haven't responded! I'll try to write up a test today!

@bzf bzf force-pushed the al-allow-custom-callback-url branch from 1de0705 to 2894c8d Compare January 27, 2017 13:29
@bzf
Copy link
Contributor Author

bzf commented Jan 27, 2017

Added a spec in the latest commit

@bzf bzf force-pushed the al-allow-custom-callback-url branch from 2894c8d to 4b6122a Compare January 27, 2017 14:03
@raysrashmi
Copy link
Collaborator

raysrashmi commented Feb 3, 2017

@bzf Thank you for the specs. I found on issue in callback_url when there is no params['callback_url'] it just redirect for Pin based login so we need to call parent't class callback_url. I have pushed a commit 91485a4 have a look and if you find it good cherry-pick in your branch 😊

@bzf
Copy link
Contributor Author

bzf commented Feb 6, 2017

@raysrashmi Cherry picked! 👍

@raysrashmi raysrashmi merged commit e83ff39 into arunagw:master Feb 7, 2017
@raysrashmi
Copy link
Collaborator

@bzf Released a new version 1.4.0 😊 . Thank you so much for the PR ❤️

@bzf
Copy link
Contributor Author

bzf commented Feb 7, 2017

@raysrashmi Thanks for merging it! 😄 ❤️

@bzf bzf deleted the al-allow-custom-callback-url branch February 7, 2017 07:03
@chriso0710
Copy link

also see #103

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