isaacsanders / omniauth-stripe-connect

Stripe Connect OAuth2 Strategy for OmniAuth 1.0
MIT License
130 stars 75 forks source link

Enable passing of stripe_landing and stripe_user fields in the URL #6

Closed excid3 closed 11 years ago

excid3 commented 11 years ago

This commit lets you pass in the stripe_user and stripe_landing parameters into the URL so that you can dynamically populate them according to the Stripe Connect Reference

For example this will populate the sign up form with an email address:

http://localhost:3000/auth/stripe_connect?stripe_user[email]=test@test.com

Or with a Devise example:

<%= link_to "stripe connect", user_omniauth_authorize_path(:stripe_connect, stripe_user: {email: "test@test.com"}) %>
cmalpeli commented 11 years ago

This does not appear to be working. I'm passing in multiple values (default to register view, user email, custom data in state param, etc: /auth/stripe_connect?response_type=code&scope=read_write&stripe_landing=register&stripe_user[email]=#{u(@current_tenant.user.email)}&state=#{@current_tenant.id}"

But I get taken to the login view every time (not register) w/ none of the querystring params retained and with some (like state) containing different values.

excid3 commented 11 years ago

Are you already signed into Stripe by chance?

cmalpeli commented 11 years ago

I am not signed in to Stripe. I did notice that in your commit you did not add support for "state" specifically, so I tested with just stripe_landing and stripe_user[email] and it gave the same result.

cmalpeli commented 11 years ago

I'm sorry - I thought this was merged into master....it is not, correct?

excid3 commented 11 years ago

No, it hasn't been merged yet. You'd need to checkout from my fork. Check the git commit log locally to double check you're using this commit.

A couple of other things to check, are the parameters still in the URL when you get to Stripe? I'm assuming their not. it looks like I accidentally left a debug line, so you should also see some debugging information printed in the console.

cmalpeli commented 11 years ago

I'm working from the main gem, so don't know. Would you consider adding support for the "state" querystring param? This allows custom data to be passed in and sent back to the redirect_url....

cmalpeli commented 11 years ago

Also - FYI - this commit (merged into master) seems to handle params in a different manner- in the initializer. He's added support for only the scope param - but I prefer your method as it makes more sense to do this at the point of building the auth link where certain data may be properly in scope and accessible...

https://github.com/isaacsanders/omniauth-stripe-connect/commit/c440ed8ff6ead2f08bd4585d8cf70b2db911e670

excid3 commented 11 years ago

Ah, you'll want to swap your Gemfile to use gem 'omniauth-stripe-connect', git: 'git://github.com/excid3/omniauth-stripe-connect.git' instead, that's probably why it wasn't working.

I missed adding state so we should definitely get that added if it's not already. I'm also not sure if this is the best way to handle these parameters or not. I'm basing it off of arunagw/omniauth-twitter.

I did try adding these parameters like the scope one, but wasn't able to get any data passed through. Definitely open to better solutions.

cmalpeli commented 11 years ago

Thanks - would be great if you added state. I think your way is best actually :)

isaacsanders commented 11 years ago

I think there is support to do this in a way that OmniAuth has already solved. I have not responded because I am looking into it.

isaacsanders commented 11 years ago

In the OmniAuth::Builder block, when you declare a strategy, you can pass along options (for example, scope lives there). I have added the capability to version 2.1.0.

The state parameter is created by omniauth-oauth2. I believe that it is better to let this important security measure remain. And since it is not any type of information that one can use, I will not work on making that available.

Let me know if there are any issues.

cmalpeli commented 11 years ago

I think state: should be accesible - as this is what Stripe lets you use to pass in custom information: https://stripe.com/docs/connect/oauth

from their docs "state: We'll pass this parameter back to you on redirect (useful for CSRF protection or identifying users)"

https://stripe.com/docs/connect/reference#get-authorize

Thoughts?

isaacsanders commented 11 years ago

It turns out that I inadvertently added support to send your own state.

What Luck!

Let me know if there are any issues.

Just by adding state to the params, you can send your own state.

excid3 commented 11 years ago

Originally, I tried making these parameters the same implementation as state however it didn't work. It may be because the stripe_user parameters are nested. I'm fine if we don't accept this patch, but I do need a way to supply these other supported parameters. I'd rather not maintain my own copy of the gem, so any pointers to how this should be handled with Omniauth would be helpful. Thanks!

isaacsanders commented 11 years ago

Here is something that I believe should work with devise, right now.

# assuming the devise scope used is 'user'
user_omniauth_authorize_path(:stripe_connect, :stripe_user => {
  :email => 'foo@bar.com',
  :url => 'http://bar.com'
})

Please let me know if this doesn't work with v2.1.0

isaacsanders commented 11 years ago

I must have forgotten to let you all know that I added the functionality... I apologize for being so long in getting this squared away. I am a college freshman, and I just finished my first term, so things have been a little hectic.

Please use version 2.1.0 for the feature to be accessible.

cmalpeli commented 11 years ago

Hi Isaac - how would you pass state? As you say, it seems OmniAuth populates this parameter and passes it over to Stripe, yet Stripe says in their docs you should be able to use this param to provide custom values that you need appended to the redirect_url. I'm at a bit of a loss....

isaacsanders commented 11 years ago

If you pass it as a query parameter, that should work...

On Nov 24, 2012, at 2:38 PM, Christian Malpeli notifications@github.com wrote:

Hi Isaac - how would you pass state? As you say, it seems OmniAuth populates this parameter and passes it over to Stripe, yet Stripe says in their docs you should be able to use this param to provide custom values that you need appended to the redirect_url. I'm at a bit of a loss....

— Reply to this email directly or view it on GitHub.

cmalpeli commented 11 years ago

Doing something like

link_to "click here", "/auth/stripe_connect?state=#{@current_tenant.id}"

results in the state param being replaced w/ the OmniAuth generated state string. Are you seeing different behavior?

isaacsanders commented 11 years ago

hmm...

Sorry, I thought it would work...

I will push up a change and version it 2.1.1

On Nov 24, 2012, at 7:15 PM, Christian Malpeli wrote:

Doing something like

link_to "click here", "/auth/stripe_connect?state=#{@current_tenant.id}" results in the state param being replaced w/ the OmniAuth generated state string. Are you seeing different behavior?

— Reply to this email directly or view it on GitHub.

isaacsanders commented 11 years ago

should be good. You can probably do:

link_to "click here", "/auth/stripe_connect", :state => @current_tenant.id
cmalpeli commented 11 years ago

Still doesn't seem to work. I've tried:

link_to "click here", "/auth/stripe_connect?state=#{@current_tenant.id}"

and

link_to "click here", "/auth/stripe_connect", :state => @current_tenant.id

The second method, while correct per the link_to API docs, does not maintain the querystring param. But either way state seems to be overwritten by OmniAuth.

isaacsanders commented 11 years ago

Can you verify the version of the gem that you are using?

On Nov 24, 2012, at 8:34 PM, Christian Malpeli wrote:

Still doesn't seem to work. I've tried:

link_to "click here", "/auth/stripe_connect?state=#{@current_tenant.id}" and

link_to "click here", "/auth/stripe_connect", :state => @current_tenant.id The second method, while correct per the link_to API docs, does not maintain the querystring param. But either way state seems to be overwritten by OmniAuth.

— Reply to this email directly or view it on GitHub.

cmalpeli commented 11 years ago

2.1.1

isaacsanders commented 11 years ago

I have removed version 2.1.1, version 2.1.0 seems to work. I would try it again.

On Nov 24, 2012, at 8:37 PM, Christian Malpeli wrote:

2.1.1

— Reply to this email directly or view it on GitHub.

cmalpeli commented 11 years ago

hmm - i've switched to 2.1.0 but am not seeing any difference. How are you testing/verifying? Perhaps i'm making a silly mistake somewhere....do you have sample?

isaacsanders commented 11 years ago

I am using pry to analyze the objects... And I cannot think of another issue.

I get the state I give it.

On Nov 24, 2012, at 9:01 PM, Christian Malpeli wrote:

hmm - i've switched to 2.1.0 but am not seeing any difference. How are you testing/verifying? Perhaps i'm making a silly mistake somewhere....do you have sample?

— Reply to this email directly or view it on GitHub.

cmalpeli commented 11 years ago

Just to confirm, the state=foo querystring param is visible in the resulting https://connect.stripe.com/oauth/authorize URL and matches what you passed in to the /auth/stripe_connnect url?

isaacsanders commented 11 years ago

I am not sure if it is visible there, but it is visible when stripe hits the callback url. On Nov 24, 2012, at 10:00 PM, Christian Malpeli wrote:

Just to confirm, the state=foo querystring param is visible in the resulting https://connect.stripe.com/oauth/authorize URL and matches what you passed in to the /auth/stripe_connnect url?

— Reply to this email directly or view it on GitHub.

cmalpeli commented 11 years ago

Hmm - definitely not happening here - getting the same state CSRF returned back. Seems to be a similar issue on the Github OmniAuth gem: https://github.com/intridea/omniauth-github/pull/17

cmalpeli commented 11 years ago

OK - figured it out.

So this works:

link_to "click here", "/auth/stripe_connect?state=#{@current_tenant.id}"

but this doesn't:

link_to "click here", "/auth/stripe_connect", :state => @current_tenant.id

HOWEVER - this results in a CSRF warning error from OmniAuth. As OmniAuth uses the :state parameter to detect against CSRF, but Stripe doesn't honor this (as they instruct you to use :state to pass any application specific information). In order to prevent the CSRF error, you need to set Stripe as provide which ignores state.

provider :stripe_connect, ENV['STRIPE_CONNECT_CLIENT_ID'], ENV['STRIPE_API_KEY'], :scope => "read_write", :provider_ignores_state => true

Hope this helps anyone else running into this.

isaacsanders commented 11 years ago

I think I will add the provider_ignores_state to the main gem. Just ran into this myself. Sorry for being an ass, if I came across that way.

ev46 commented 11 years ago

if your app is giving you route /auth/stripe_connect not found error, add the /auth prefix inside the omniauth.rb file:

configure do |config| config.path_prefix = '/auth' end