When processing a succeeded payment_intent why do we assume the payment intent covered the whole order balance?

Ok funny enough (or not), because of many sales that happened on our website during Black Friday and Cyber Monday, I have had for the first time, 3 use cases where the commerce_payment has been created with the order balance as its amount BUT the PaymentIntent's amount was actually not the same as the order balance!
So I definitely think we should assign the amount that has really been paid by the PaymentIntent when we create the commerce_payment. And if this amount is not equal to the order's balance, we should raise an error and not complete it automatically.

So what happened to us, well, we had a coupon code that expired at 23:59:59 and a customer initiated the payment at 23:59 and it finished at 00:00. Because of another bug that I will have to investigate on later, the product price was re-updated after the payment was successful, so the order total was changed. So when creating the commerce_payment, the order balance was not the correct price anymore.

The second case was pretty much the same, someone bought a product while the market team manually udpated a coupon code.

The last case was different but I won't speak of it for now because it might be an important security breach so I need to reproduce it first and I will contact maintainers in private about it, but in the end the PaymentIntent's amount was not the order's expected total amount anymore.

BTW, since the problem resides in both StripePaymentElement->processWebhook and PaymentProcess->createPayment, I suggest we also create an issue in Commerce Core (payment).

In the same way, at the end of StripePaymentElement->processWebhook, we call $this->placeOrder, which calls $order->getState()->applyTransitionById('place');
So here we assume that there can only be one webhook / PaymentIntent for a given order and because this payment was successful, we can assume the order is complete. Are we sure about that?

I have noticed OrderStorage->doOrderPreSave() checks if the order has been fully paid and if so, it dispatches the OrderEvents::ORDER_PAID event. OrderPaidSubscriber listens to this event and calls $order->getState()->applyTransitionById('place') for OffsitePaymentGatewayInterface.
Shouldn't we rely on this instead, allowing us to have multiple payments / PaymentIntents / webhooks on one same order before it is fully paid and only then can we complete it.

I've just asked chatGPT a few questions. Here is what I can report: the way Stripe and Commerce Stripe are working right now, there can only be one successful PaymentIntent per order. Indeed, no matter what kind of payment method Stripe lets the customer choose from, even for payments splitted over time like Klarna or Afterpay, there will be only one PaymentIntent for one transaction. Stripe handles the multiple payments internally and should transfer us the total amount right away. There might be multiple "charges" inside one PaymentIntent in some scenari, but always only one PaymentIntent. This actually makes sense because we are actually the ones technically creating the PaymentIntent in the first place with the order balance as amount.

Even for people using Stripe Checkout, thus only creating the Checkout Session and letting Stripe being responsible for creating the PaymentIntent, Stripe guarantees it will only create one PaymentIntent for one given Checkout Session.

All that being said, the problem I raised still remains. When the customer comes back to our site after a successful payment, we cannot create the commerce_payment with the order balance as the amount and assume this is the amount that has just been paid. Because as I myself have been able to witness, it is technically possible that by the time the customer returns to the site, the order balance has changed and is not the same as the amount that has truly been paid! Which is bad and we should stop completing the order right there and raise an error.

There are two scenari:

1/ webhook handling:
In StripePaymentElement->processWebhook I think we should simply stop setting the order balance inside the commerce_payment and directly set the PaymentIntent's amount instead.
Btw, this requires to create a Price object from the PaymentIntent's amount and currency. And convert the PaymentIntent's amount format from 2499 into 24.99.
We could also still verify that the PaymentIntent's amount matches the order balance and raise an error if it's not the case but the most important is to make sure what we write in the database is the reality.

I also still believe calling $order->getState()->applyTransitionById('place'); shall not be done manually here and that, as I mentioned earlier above, let OrderStorage->doOrderPreSave() check if the order has been fully paid before placing it as completed.

2/ return page handling:
The returnPage callback leeds us to the generic PaymentProcess->createPayment() that creates the commerce_payment with the order balance. This code belongs to Commerce Payment (core). It does not depend on Stripe at all so there is no way to set the PaymentIntent's amount here. Furthermore, this code is also called when creating the commerce_payment before the remote payment has been processed, which is actually why we set the order balance. So obviously no issue shall be created on Commerce Payment's side.
StripePaymentElement->createPayment() is called right after, and this is where we can verify that the amount of the commerce_payment matches the PaymentIntent's amount before we mark the commerce_payment completed.

Hi nicolas bouteille, thanks for creating this issue.

We've had this in progress for a minute, but now that we've completed this work(in concert with several other changes), I'm going to go ahead and just create the MR as part of this issue.

I did want to note a few things:

1. For now, we'll still be marking the order as checkout complete and placed. If we threw an error, then the customer would be left in an indeterminate state, as the payment would be processed, but the order would still be in the checkout flow...which would allow the customer to add/delete items from their cart, causing more issues. More likely, this order should be treated similarly to a failed ACH transaction(if you accepted ACh payments) or a chargeback. Were we to actually throw an error, we would likely need a change in commerce core.

2. return page handling invokes onReturn() on the StripePaymentElement gateway. PaymentProcess should actually not even be encountered, but if it is, it should actually short-circuit because the order has been placed, moving it to the checkout complete pane.

Hi Tom,

It's nice to see you've been working on it :)

Are you sure you wanna set the order balance as a fallback?
IntentHelper::getPrice($payment_intent) ?? $order->getBalance()
I say if we are not able to retrieve the amount from the PaymentIntent it means something bad has happened and we should stop here.

which would allow the customer to add/delete items from their cart

as mentioned here 🐛 Lock the order before stripe.confirmPayment() is called Active , the order needs to be locked before triggering the payment. As long as the order is locked, the order will not be visible anymore on /cart and adding new stuff to the cart will create a new cart and not affect this specific order.
If we place the order with the order total not being the paid amount, if the site is responsible for creating the invoice, it will be created with the wrong amount, which can cause legal problems because an invoice shall not be generated twice.

return page handling invokes onReturn() on the StripePaymentElement gateway. PaymentProcess should actually not even be encountered

I think you might be wrong here. From my understanding, PaymentCheckoutController::returnPage is the entry point. It calls first StripePaymentElement::onReturn() but only to store the Payment Method, and PaymentProcess is called right after (the next checkout step) in order so store the Commerce Payment regarding the successful Stripe payment.
And as I mentioned before, because of the delay for queued webhook processing, return url will be responsible for finalizing the order 99% of the time.

Automatically closed - issue fixed for 2 weeks with no activity.