Replace stubs on ./spec/lib/stripe/account_connector_spec.rb

filipefurtad0 commented 7 months ago

What we should change and why (this is tech debt)

Same as for other specs on the epic, we can replace the stubs in this file and provide real responses to the code, to test it.

Some useful documentation: https://datatracker.ietf.org/doc/html/rfc6749#section-1.2 https://stripe.com/docs/stripe-apps/api-authentication/oauth#install-app

A sequence diagram (WIP):


title OFN / Stripe OAuth2 Flow

entryspacing 1.1

participant Client (OFN)

participant Resource Owner (User)

participant Authorization Server (Stripe OAuth)

participant Resource Server (Stripe Account)

note over Client (OFN): Initiates access request

Client (OFN)->Resource Owner (User): requests validation for access request
Resource Owner (User)-->Client (OFN): validates the application’s access request\n        (authenticates and authorizes)
Client (OFN)->Authorization Server (Stripe OAuth): authorization grant (redirect URI)
note over Authorization Server (Stripe OAuth): validates authorization
Authorization Server (Stripe OAuth)-->Client (OFN): token
Client (OFN)->Resource Server (Stripe Account): sends token
Resource Server (Stripe Account)-->Client (OFN): returns requested data

Context

Part of #11890

Impact and timeline

cyrillefr commented 5 months ago

Hello @filipefurtad0 ,

May I work on this issue ?

It is already assigned to you, but you mentioned this issue to me here in
https://github.com/openfoodfoundation/openfoodnetwork/issues/12201#issuecomment-2007574796

filipefurtad0 commented 5 months ago

Yes sure @cyrillefr , Thanks - I've unassigned myself and assigned you.

cyrillefr commented 5 months ago

Hello @filipefurtad0 ,

I am trying to make an entire OAuth flow, but I think this is not possible. At line context "and the decoded state param contains an 'enterprise_id' key", there is a mock below, with a code param. The code is to be get after a user click a form. With the code, then one can retrieve a token. So, unless we put this in a system spec (my guess is that this is no place for a capybara/selenium test), the entire workflow is not testable. I have watched at the doc and SO to get a hint of which url one can directly call to get the authorization code, but could not find it. I do not know how to test this.

What are you thoughts on this ?

filipefurtad0 commented 5 months ago

Hey @cyrillefr ,

Thanks for looking into this.

Unfortunately, I also cannot answer... I've spent quite some time looking at this one, but also could not understand where to retrieve the a (test?) token, or how to proceed.

there is a mock below, with a code param.

It's a good catch indeed that the mock contains a body: param, which I guess would point out to a system spec. Recently, we've aimed at implementing a solution to set up a proxy and intercept browser calls to external websites (like Stripe) and record these with VCR [*], but we were unable to resolve a port conflict, within the setup. So, I also don't know how we could test this, if a system spec is needed.

[*] If you are interested in knowing more about this effort, you can check this (closed PR) and this discussion on Slack.

filipefurtad0 commented 5 months ago

Unfortunately, I also cannot answer...

Maybe a good idea to ping a @openfoodfoundation/developers for their thoughts on this one.

mkllnk commented 5 months ago

That spec file has only one stub_request call in there to receive a token. If you can observe a valid real authorisation request then you can supply the params and record that one with VCR. After the recording, the token may not be valid any more and re-recording is not easily possible but we would have a valid real recording to execute the test. And a comment can explain how you observe the token to supply to the spec. That's probably the minimal implementation here.

I'm not sure in which circumstances OAuth gets called with deauthorize but one limitation of VCR is that we can't record and replay incoming requests. So if Stripe is calling a callback endoint on our side then we can't record that with VCR. It's better to use mocks there. Stripe can't call your local dev machine anyway.

filipefurtad0 commented 5 months ago

Thanks @mkllnk @cyrillefr ,

This is helpful, I think we're almost there. I've tried following your leads together with the documentation here - https://docs.stripe.com/connect/oauth-standard-accounts.

So, I think the flow would be (and as @cyrillefr already pointed out): 1) getting an authorization_code 2) exchanging it for a token with something like:

response = Stripe::OAuth.token({
  grant_type: 'authorization_code',
  code: 'ac_123456789',
})

3) then replace the stubs on the test case "but the user doesn't manage own or manage the corresponding enterprise" and deauthorize the account, with something like:

Stripe::OAuth.deauthorize({
    client_id: 'ca_xyz...,
    stripe_user_id: 'acct_xyz...,
})

4) Finally, assert that we get the expected error CanCan::AccessDenied error with connector.create_account.

If you can observe a valid real authorisation request then you can supply the params and record that one with VCR

Now, point 2) fails, of course, because code: 'ac_123456789' is not a valid authorization code. I'm not entirelly sure how to get it. It seems to me, we would need to record the response from the GET request: GET https://connect.stripe.com/oauth/authorize

with params as described here.

I have not been able to make this request work, but I think this would be the way forward.

cyrillefr commented 5 months ago

Hello @filipefurtad0 , @mkllnk ,

I think the important part is to get the (first) code (the ac_xxx). From what I remember from my various attempts:

one must connect to the web page of Stripe (https://connect.stripe.com/oauth/authorize) available in the ctrl
click the test mode -> there will be a redirection and the url will then have the parameter code in it
with the value of code, you can then go on.

Which means, you can only do this in a system spec, because you will need Capybara/Selenium. I have tried to look what was happening after clicking the "test mode" button, but haven't found anything. It is hidden, and even if we find it, it is not meant to be public and so subject to change. That is why I think the need for Capybara/Selenium, but I do not know if it can integrate with VCR.

filipefurtad0 commented 5 months ago

one must connect to the web page of Stripe (https://connect.stripe.com/oauth/authorize) available in the ctrl

Could we not record the GET request from this URL (as an HTTP request), instead of using a system spec? Maybe I'm missing something here...

cyrillefr commented 4 months ago

Hello @filipefurtad0 ,

I don't know if it possible. If you open the dev web tools, you can look at the requests, but since there is a redirection after you click the test mode button, you cannot see anything as the request panel is re-initialized each time a page is loaded. I have looked a bit, and there are some Firefox add-ons that could do the job, but I am not sure if it does the job. To me, it is a bit of a hack. so I did not get any further. It would be cool if we could get this url, but the fact it is hidden is maybe due to security reasons. Every search I have done implied at one moment to click a button. I am testing right now a way to see what the url is. I will post a comment when finished.

cyrillefr commented 4 months ago

Hello @filipefurtad0 ,

there is an interesting way. Here, it tells you how to log the Firefox console: https://help.mypurecloud.com/articles/gather-firefox-console-log/

So, if you enter in the url https://connect.stripe.com/oauth/authorize? state=the_state&scope=read_write&client_id=ca_The_client_id_test_we_use&response_type=code

You will be able to see 2 interresting calls

a POST to connect.stripe.com/ajax ... to create a test mode
a GET to t.stripe.com/?domain=connect.stripe.com&page=oauth...with a lots of parameters The GET seems very complicated with a lot of ids .... I don't know if we can replicate. There is a code parameter to be returned, I think this is the one, but there are a lot of others parameters as well.

filipefurtad0 commented 4 months ago

Hi @cyrillefr ,

Thanks for your investigation.

Instead of using a system spec, I've tried running the HTTP request suggested here:

https://connect.stripe.com/oauth/authorize?response_type=code&client_id=<our_client_id>&scope=read_write

On the draft PR, we can see that VCR indeed captures the reply... But I don't know how we can go from there to get the actual authorization code, which is what we get when we hit "Skip this form on the UI":

Then, the return URL looks like something like this: http://localhost:3000/stripe/callback?scope=read_write&code=ac_****

This reply would contain the code to run the request below, and receive the token:

                response = Stripe::OAuth.token({
                                 grant_type: 'authorization_code',
                                 code: 'ac_****',
                 })

So if Stripe is calling a callback endoint on our side then we can't record that with VCR. It's better to use mocks there. Stripe can't call your local dev machine anyway.

So maybe we can't really go further on this one...

cyrillefr commented 4 months ago

Hello @filipefurtad0 ,

I think this is hidden. But since this is about security, this is a good reason :) I think also we cannot go further here.

filipefurtad0 commented 4 months ago

Thanks so much again @cyrillefr :pray: Closing the PR and this issue.

openfoodfoundation / openfoodnetwork