Do not delete stored payment method on 3DS 2 failure or cancel ?

Looks like a bug to me

I'm seeing some dissappearing stored payment methods too, and the context is very probably recurring payments. I suspect that sometimes if a recurring fails then the stored payment method is being deleted.

Particularly concerning to me here is the idea that this might happen if the card has expired. It's concerning because Stripe is clever enough to automatically update expired cards (although there can be a delay and a couple of failures and retries before it happens).

Like you I suspect this code in createPayment may be too hasty to delete stored payment methods.

      if (!in_array($intent->status, [PaymentIntent::STATUS_REQUIRES_CAPTURE, PaymentIntent::STATUS_SUCCEEDED], TRUE)) {
        ...
        $this->deletePaymentMethod($payment_method);

I've only just upgraded to RC4 from RC1 however and can't yet confirm a case of this on RC4.

I think that if a payment fails, we should delete the stored payment method only if it has not been used in a previous payment; i.e. we should do an entity query and check there are no commerce_payment entities that have the current payment as their payment method.

I think I've run into this in production. I have some orders with attached payment methods that have simply disappeared, and I can't figure out how else they are gone. I don't have recurring payments.

I suggest that this is bumped to major or perhaps critical as it is data loss.

I've managed to reconstruct from webserver logs and MySQL binary logs that payment method entities can be deleted without warning when a user fails to complete checkout - without further testing at the moment I assume this is the same as or similar to the 3DS issue in the original issue here.

Uploading patch. I don't think we should remove the payment_method from the order neither, but still need to verify this works.

That failed as it's on top of #3171408-17: Allow off_session payment intents for recurring payments and #3171411-6: Optionally autosubmit the review pane to streamline UX

I don't think we should remove the payment_method from the order neither

Agreed.

Let's see if this version applies...

Patch applies and the test failures don't leap out to me as related.

Found a problem, I think.

I was running the test as anonymous, but this will happen with logged in user too, I think.

1. Added product to the cart
2. Went to cart
3. Clicked Checkout
4. Chose Stripe because I have other gateways
5. Used the Stripe test card 4000 0027 6000 3184 which shows authentication step
6. Chose to fail the authentication
7. Was returned to the step of choosing a payment method - all correct so far.
8. One of the listed methods was Visa 3184 - which is correct: this payment card was not previously deleted because of the patch.
9. So I chose that as a payment method, and subsequently got an error screen - try again later.

In the log there is this exception:

Stripe\Exception\InvalidRequestException: The provided PaymentMethod was previously used with a PaymentIntent without Customer attachment, shared with a connected account without Customer attachment, or was detached from a Customer. It may not be used again. To use a PaymentMethod multiple times, you must attach it to a Customer first. in Stripe\Exception\ApiErrorException::factory() (line 38 of /var/www/html/vendor/stripe/stripe-php/lib/Exception/ApiErrorException.php).

This makes sense: the purchase was anonymous, and there's been no previous setupintent. So I think that the patch might need to be more selective and delete the payment method if there is no stripe customer, and probably also we'll find we need to delete if there's no intent for future use.

One potentially confounding issue - I was running my test on top of https://www.drupal.org/project/commerce/issues/2871483, set to offer the user the choice of saving the payment method, and I chose not to save. This is a problem in itself because I was using anonymous purchase, but I don't think that this is related.

Updated patch which fixes the problem for guests. And keeps the payment method around for logged in users.

I guess this could be more restrictive - e.g keeping the payment method on cancellation but not other failures. Or discarding after 2 or 3 failures.

Nice catch @Jeff. We're going to need tests :(

I don't think we should add extra restrictions like discarding after n failures. I think that's a follow on.

Tests needed imo:

Plain transactions - do they succeed?
- As registered user, does a straight-through transaction run successfully with card 4242 work. This test must already exist.
- As guest/anon, does a straight-through transaction run successfully with card 4242 work. This test might not exist. To check.
- As registered user, does a straight-through run successfully with card 3184. This tests SCA cards. Test should exist.
- As guest user, does a straight-through run successfully with card 3184. This tests SCA cards. Test might not exist.

Do transactions where someone clicks Back succeed - both reusing the new payment method, if it exists, and not using it?
- As registered user, start purchase with 4242, go back, then use a new 4242 payment method to complete (do not re-use the 4242 created payment method, instead enter the card details again). (Does the user end up with two 4242 payment methods in their user settings?)
- As registered user, start purchase with 4242, go back, then use the created 4242 payment method to complete.
- As guest user, start purchase with 4242, go back, then use a new 4242 payment method to complete (there should be no listed 4242 payment method). There's a potential issue here: with https://www.drupal.org/project/commerce/issues/2871483 - what happens with guest 'always save' and guest 'choice to save' when the user creates an account.
- Repeat all these for 3184 card. There should be no difference.

(As a manual test - what happens when Back is clicked more than once?)

Do SCA transactions which are started, then fail or are cancelled, succeed when the user resubmits?
- As registered user, start purchase with 3184, cancel/fail the transaction, then use a new 3184 payment method to complete (do not re-use the 3184 created payment method, instead enter the card details again). (Does the user end up with two 3184 payment methods in their user settings?)
- As registered user, start purchase with 3184, cancel/fail, then use the created 3184 payment method to complete
- As guest user, start purchase with 3184, cancel/fail, then use a new 3184 payment method to complete (there should be no listed 3184 payment method). There's a potential issue here: with https://www.drupal.org/project/commerce/issues/2871483 - what happens with guest 'always save' and guest 'choice to save' when the user creates an account.

I think that covers all the cases, can you see any that have been missed?

I don't think we need to test 2 cancel/fails in a row on the same card, followed by a success. And I don't think we need to test a cancel/fail on one card, followed by a different card.

#13

1. Added product to the cart
2. Went to cart
3. Clicked Checkout
4. Chose Stripe because I have other gateways
5. Used the Stripe test card 4000 0027 6000 3184 which shows authentication step
6. Chose to fail the authentication
7. Was returned to the step of choosing a payment method - all correct so far.
8. One of the listed methods was Visa 3184 - which is correct: this payment card was not previously deleted because of the patch.
9. So I chose that as a payment method, and subsequently got an error screen - try again later.

I don't think 1-8 will trigger the gateway plugin's createPayment() method, which is all we're patching here. So I think your problem is unrelated to this issue actually. createPayment() is only triggered from the payment process pane which comes after the review step.

I'd have a look at other issues about anonymous checkout or create a new one. I suspect your problem may be here in the stripe-review pane:

    if ($intent->status === PaymentIntent::STATUS_REQUIRES_PAYMENT_METHOD) {
      $payment_method = $this->order->get('payment_method')->entity;
      assert($payment_method instanceof PaymentMethodInterface);
      $payment_method_remote_id = $payment_method->getRemoteId();
      $intent = PaymentIntent::update($intent->id, [
        'payment_method' => $payment_method_remote_id,
      ]);

It looks like the payment method is not updated if the intent is reused.

FYI for future readers: there's a chance that this patch may be implicated in https://www.drupal.org/project/commerce_stripe/issues/3079830