craftcms / commerce-stripe

Stripe payment gateway for Craft Commerce
https://plugins.craftcms.com/commerce-stripe
MIT License
31 stars 48 forks source link

Add mutex to stripe payment method creation #281

Closed timkelty closed 10 months ago

timkelty commented 10 months ago

Worked through this with @nfourtythree…

Without a mutex, when creating a payment source, the webhook for payment_method.attach can come in before the payment source has been created, resulting in 2 payment sources, with the same token.

Questions for discussion

Unique commerce_paymentsources.token?

We need the mutex to cover this, but also begs the question, should it even be possible for commerce_paymentsources.token to be non-unique? I guess other gateways might not have unique identifier tokens?

Unnecessary saves

Even with the mutex, saving a payment source still results in the payment source getting saved a min of 3 times.

It seems like we could be more judicious about bailing early in \craft\commerce\stripe\base\SubscriptionGateway::handlePaymentMethodUpdated to cut that down. Eg, if an existing payment source is found, and this is a payment_method.attach event, we can probably just bail and not save?

Furthermore, with the webhook will currently always overwrite the description, which you may not want.

Mutex timeout

Right now we have a timeout of 5 seconds when acquiring the mutex. It seems like that should be based on the Stripe webhook timeout, but I couldn't find that docuemented. From what I can guess, it is pretty quick, probably closer to 2 seconds.

Duplicate stripe events for "connect"

I don't know if this is a local dev problem only or not, but when using stripe listen as prescribed, you'll end up with duplicate webhooks for everything (one for "account", one for "connect").