laravel / cashier-stripe

Laravel Cashier provides an expressive, fluent interface to Stripe's subscription billing services.
https://laravel.com/docs/billing
MIT License
2.37k stars 669 forks source link

Support Idempotency Option #492

Closed slaughter550 closed 4 years ago

slaughter550 commented 6 years ago

@taylorotwell or @GrahamCampbell is there a desire to support idempotent requests as a part of this API? If there is I'd be happy to submit a PR for it.

Reff https://stripe.com/docs/api?lang=php#idempotent_requests

Background I'm currently moving our company's native stripe implementation over to cashier and we use the property idempotency_key like so as a part of our Charge request in the second method option that is currently hardcoded to only api_key in cashier.

Charge::create(array_merge_recursive($array, [
    'currency' => 'usd',
    'customer' => $this->getStripeCustomer($source)->id,
    'receipt_email' => $this->email,
    'description' => $this->fullname,
]), [
    'idempotency_key' => session($key),
]);
slaughter550 commented 6 years ago

ping @taylorotwell

driesvints commented 5 years ago

I've been looking at this but my concern is that the public api will get littered with extra array arguments. This allows for a lot of flexibility but isn't very appealing.

Here's an example on how that could be done:

public function charge($amount, array $options = [], array $paymentIntentOptions = [])
{
    ...

    $payment = new Payment(
        StripePaymentIntent::create(
            $options, $paymentIntentOptions + Cashier::stripeOptions()
        )
    );

    ...
}

Like I said, if we had to add the arguments everywhere so you could use it in every situation it would clog up the public api too much imo.

SlyDave commented 5 years ago

Maybe a generic solution for intent options clogs up the public API too much, maybe it doesn't.

But I think we can all agree that the Idempotency option should be used, even if by default and have Cashier generating the per charge/subscription etc. key.

It is a fairly important option not to mention the cost of having to deal with accidental charges, refunds, support time, damage to reputation etc, that can be caused by not using it.

The work around (currently) would be to not use Cashier... which doesn't seem good.

fzldn commented 4 years ago

This is my temporary solution for this enhancement until this update is arrive. because maybe our project can't wait for this to be fixed.

we can extend Laravel\Cashier\Billable and modify charge method to our custom trait like mine MyBillable

namespace App\Traits;

use Laravel\Cashier\Billable;
use InvalidArgumentException;
use Stripe\Charge as StripeCharge;

/**
 * extend Laravel Cashier Billable
 */
trait MyBillable
{
    use Billable;

    /**
     * Make a "one off" charge on the customer for the given amount.
     *
     * @param  int  $amount
     * @param  array  $options
     * @return \Stripe\Charge
     *
     * @throws \InvalidArgumentException
     */
    public function charge($amount, array $options = [])
    {
        $options = array_merge([
            'currency' => $this->preferredCurrency(),
        ], $options);

        $options['amount'] = $amount;

        if (!array_key_exists('source', $options) && $this->stripe_id) {
            $options['customer'] = $this->stripe_id;
        }

        if (!array_key_exists('source', $options) && !array_key_exists('customer', $options)) {
            throw new InvalidArgumentException('No payment source provided.');
        }

        // my modification starts here
        $params = ['api_key' => $this->getStripeKey()];

        if (array_key_exists('idempotency_key', $options)) {
            $params['idempotency_key'] = $options['idempotency_key'];
            unset($options['idempotency_key']);
        }

        return StripeCharge::create($options, $params);
        // end of my modification
    }
}

and here's in my billable model User.php

namespace App;

use Illuminate\Notifications\Notifiable;
use Illuminate\Foundation\Auth\User as Authenticatable;
use App\Person;
use App\Traits\MyBillable;
use Laravel\Cashier\Billable;
use Illuminate\Support\Facades\DB;
use Spatie\Permission\Traits\HasRoles;

class User extends Authenticatable
{
    use Notifiable;
    // use Billable;
    use MyBillable;
    use HasRoles;

    ...

so in our controller simply put idempotency_key like this

...

$charge = Auth::user()->charge($amount, [
    'description'     => 'Charge Description',
    'source'          => $token,
    'metadata'        => $metadata,
    'idempotency_key' => $request->input('idempotency_key'), // like this
]);

...

I hope this will help you out

ProgrammerZ commented 4 years ago

@fzldn Thank you for temp solution.

If the request needs to be sent again for any reason, idempotency key is extremely useful feature to exclude double charge.

amsoell commented 4 years ago

Came across this issue looking for some sort of native support. Short of that, thanks @fzldn for inspiring a workaround. I'm curious if anyone is working on a similar workaround that handles subscriptions? Cracking that nut looks a bit more complicated, as subscriptions are handled through the SubscriptionBuilder first. Curious if there's a cleaner way than extending Billable and extending SubscriptionBuilder

stueynet commented 4 years ago

Is there any plan to support this?

slaughter550 commented 4 years ago

@driesvints are you still good with your solution above? This came up this week for us and looks like I'll get to revisit it

driesvints commented 4 years ago

What you could do for charges is to serve the idempotency_key through the user's stripeOptions:

class User extends Model
{
    public $idempotencyKey;
    /**
     * Get the default Stripe API options for the current Billable model.
     *
     * @param  array  $options
     * @return array
     */
    public function stripeOptions(array $options = [])
    {
        return array_merge(Cashier::stripeOptions($options), [
            'idempotency_key' => $this->idempotency_key,
        ]);
    }
}

And then use it as:

$user->idempotency_key = $key;

$user->charge();

I'm not familiar with idempotency keys so I wouldn't know how it handled conflicting methods calls. For that reason I don't think it's possible that this will ever work for subscriptions.

If anyone can ever figure out a nice clean way to tackle this then we're welcoming prs, thanks.

garygreen commented 3 years ago

@driesvints

Using stripeOptions is not a solution because you cannot use the same idempotency key across requests. In other words, it's very dependant on the request you are making.

This is the error you'll get if you attempt to use the same idempotent key when creating customers + a subscription (which would happen if you are generally using the same key in stripeOptions - because you wouldn't know what calls Cashier is doing to the stripe API):

Keys for idempotent requests can only be used for the same endpoint they were first used for ('/v1/customers/cus_JkrF3bJCNDIqpY' vs '/v1/payment_methods/pm_1J7M56ECjXaRLq4rqwef3Nqd/attach'). Try using a key other than 'X6gcBUWlN89E3sP' if you meant to execute a different request.

There needs to be a way to pass an idempotency key, maybe as a 4th parameter to newSubscription ?

garygreen commented 3 years ago

Need a way of hooking into this, specifically:

image

stripeOptions() could be used for loads of other calls to the API, such as creating customers, etc.

driesvints commented 3 years ago

@garygreen stripeOptions went away in v13. I don't think I'll take the time to look into this at the moment. If you want to propose something concrete it's best that you send in a PR so we can look at some actual code. Thanks.

garygreen commented 3 years ago

Idempotency key is critical because it helps prevent duplicate subscriptions due to race conditions, e.g. slow/patchy mobile internet connections. We've had problems with Cashier creating multiple subscriptions and without idempotency keys it's challenging to prevent this.

https://stripe.com/docs/api/idempotent_requests

The API supports idempotency for safely retrying requests without accidentally performing the same operation twice. This is useful when an API call is disrupted in transit and you do not receive a response. For example, if a request to create a charge does not respond due to a network connection error, you can retry the request with the same idempotency key to guarantee that no more than one charge is created.

matthewdanepeavoy commented 1 month ago

I was hoping to use idempotency_key as well to prevent double charges, and haven't found a way in Laravel 11 to make it work.

As a solution purely for preventing double charges due to someone spamming a button, I'm using a rate limiter on the route that is double submitting. Only allow 1 request per minute from that user's ip address (plus whatever other key you want, like order #, etc).

I have other logic supporting this on the front end and backend, but somehow had a user still sneak through with a double payment. So this solution is only a last resort.

driesvints commented 1 month ago

Someone sent in a PR to provide extra request options when creating customers here: https://github.com/laravel/cashier-stripe/pull/1692. This will also offer a way to provide an idempotent key. Would a similar solution help for some other Cashier methods?