Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.43k stars 2.8k forks source link

[Payment card / Subscription] make backend handle 3ds intent verification in new dot #44795

Closed blimpich closed 2 months ago

blimpich commented 3 months ago

Problem:

https://github.com/Expensify/App/pull/44321 is implementing the frontend portion of our add payment flow for 3DS secure payment cards. However, the PR is blocked because upon completing the stripe authentication flow the backend doesn't properly update the private_stripeCustomerID nvp.

A lot of slack discussions were had over why this is happening and how to fix it (1, 2, 3).

Basically, the way we did it in Old Dot was very round about and required tunneling into Web-Secure via an iframe in order to fire off a User_VerifyPaymentIntent command. This doesn't work for new dot because (1) it is actually quite tricky to tunnel into old dot with valid auth token passed to the script we were using to fire off the necessary command, and (2) we shouldn't be relying on web-secure to begin with since we intend to get rid of it entirely, so building out new dot functionality to rely on a strange and confusing web-secure redirect would be carrying forward a lot of technical debt.

Solution:

Lets use stripe webhooks to fix this. We already have a stripe webhook endpoint. Hopefully we just need to add these actions to the ProccessStripeWebhooks command in Auth and have them queue a CreateStripeSetupIntent command with parameters accountID and isVerifying.

Stripe webhook events that (I think) need to be added

setup_intent.succeeded
setup_intent.setup_failed
setup_intent.canceled

We also need to make sure that this part of the design doc works / was completed:

We will also update the CreateStripeSetupIntent Auth command to push an Onyx update for the PRIVATE_STRIPE_CUSTOMER_ID NVP with the new status, which will be used by the frontend to complete the auth flow.

Otherwise the frontend won't respond to our new backend data.

Turns out doing this via stripe webhooks is harder than expected. Lets figure out a simpler way to do this using what we already have in place.

blimpich commented 3 months ago

Update: requested developer access to stripe so that we can update our webhooks here. Followed this SO.

blimpich commented 2 months ago

Not overdue, waiting on deployment of the first Auth PR before updating stripe webhook endpoint with new events.

blimpich commented 2 months ago

Update: new webhook events were added, but apparently I still need more dev access in order to test this end to end locally. I'm trying to follow this SO which makes it seem like I should be able to request dev access to Stripe even though I'm not ring1. I asked again about getting dev access here. Waiting on response to that.

blimpich commented 2 months ago

Draft PR up for adding new webhook functionality, waiting to see if I can get access change to test the webhooks end-to-end before moving forward.

blimpich commented 2 months ago

Access change still not allowed. Its still possible for me to complete this issue but its going to be difficult since I'm going to have to ask a ring1 engineer to basically test everything I do.

I'm going to try and see if a ring1 dev can setup a testing endpoint for me on stripe's dashboard next. If that works then I can test this locally and also help verify the PRs that rely on this. Without it I either need to get a ring1 person to take up this issue or play the risky game of guessing what will work using mocked data rather than real Stripe webhook requests.

blimpich commented 2 months ago

Banged my head against this today.

Should probably be using https requests instead of command forwarding in the main auth PR.

Got a web-PR up that I didn't realize I needed.

The sad thing though is that I got a ring1/ring0 engineer to add my ngrok to the stripe dashboard but I learned that I still need to have a stripe developer access to use those test endpoints, which is really frustrating. So now I'm just going to test as best I can using mocked data and then ask a ring0/ring1 engineer to test it end to end locally for me, which will slow down this whole process.

Working on getting it working with mock data, which is proving difficult because there is a lot of data to mock to get it to work.

Soldiering on.

blimpich commented 2 months ago

Was too busy catching up on chores to work on this today. Server bugs, urgent customer issues. Will work on it tomorrow.

blimpich commented 2 months ago

Good progress! I got a proof-of-concept working with mocked data. I got it working in old dot and new dot. I need to figure out how to make it work without needing to refresh the page to see the changes (asked about that here), but I'm 99% sure that is solvable and can be figured out soon.

Next steps:

blimpich commented 2 months ago

Not overdue, trying to prioritize this as my number 1 issue after finishing chores.

blimpich commented 2 months ago

Seems like in order to get it updating nvp_private_stripeCustomerID in real time, and therefore get new dot to update without doing a page refresh, I need to utilize the pusher api in web. Which means I need to get the right data to web too. Working on that now.

blimpich commented 2 months ago

Okay I think the reason the onyx updates aren't working is because the request is being initiated through Stripe and not New Dot. We disabled onyx updates during the 4/29 fire for all requests except new dot requests. So now I need to figure out how to enable onyx updates for stripe requests I think. I asked about how to do that here.

blimpich commented 2 months ago

Pretty stumped on this, can't seem to get the onyx updates to happen despite my best efforts.

I did think of an alternative way of doing this that I am going to test out that might be a lot easier.

blimpich commented 2 months ago

The alternate way works, it doesn't require refreshing the page, and works for both old dot and new dot. Going to get PRs out for that now. It can also be tested without needing to be ring1, so much better for QA purposes.

blimpich commented 2 months ago

Okay, so I figured out how to solve this after more carefully reading through the stripe documentation on 3ds verification. The basic idea is that callback.php will post a message to the parent of the iframe so that we know when the user is done verifying, and new dot will be listening for that message. When new dot sees that message it triggers a backend request User_VerifySetupIntent, which goes and confirms the user's response by pinging stripe, updating our private_stripeCustomerID nvp in the database, and then updates the necessary onyx data we need too. This is basically the exact same way that old dot does it, just instead of calling VerifySetupIntent from Web-Secure we instead call it from new dot directly.

This is a lot easier than making stripe webhooks work, both in terms of implementation and testing.

Next steps:

blimpich commented 2 months ago

Not overdue, making progress, refer to last update's checklist

blimpich commented 2 months ago

Not overdue, I think this might be done actually but I'm going to wait until the frontened can confirm everything works for them.

blimpich commented 2 months ago

All the steps outlined here are either completed or close to being completed in other PRs. Closing.