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 671 forks source link

Customer created even if credit card fails #575

Closed yougotnet closed 5 years ago

yougotnet commented 5 years ago

If I use

Auth::user()->customer->createAsStripeCustomer($stripeToken);

and the credit card fails, it still sets up the customer on Stripe. But if I use the Stripe\Customer::create and the credit card fails, it doesn't setup the customer which is preferred.

By using Cashier's createAsStripeCustomer($stripeToken), if the credit card fails, it doesn't return the new customer id and therefore I don't know how to delete the customer.

driesvints commented 5 years ago

Hmm, I agree with you. This method might be doing too much on its own. I'll try to propose a different approach in a PR but it'll be for a next major release.

For now what you can do is overwrite the createAsStripeCustomer method and keep the part where it creates the customer and make a separate call to the updateCard method.

codebykyle commented 5 years ago

Hello, I ran into this issue recently. I had struggled to pinpoint where the problem actually was. Here is what I did to locate, replicate, and fix the issue. I am using laravel-spark as a basis, which uses laravel-cashier as a back-end for billing on Stripe.

Detecting the issue

We had a number of duplicate accounts created in production. Sometimes we would have a user with 2-3 accounts on Stripe. We reached out to the users to try and figure out what was going on, and how they could have multiple accounts. We locked the form during submission and had no indication that there was a double submission issue from the users side. The users told us that they had entered an incorrect expiration date on their card by mistake, which started us looking for what could have caused this.

Reproducing the issue on a Production website

In order to reproduce the issue, you can do the following in a production site:

Reproducing the issue on a Development website

Notes

I spoke with the people at Stripe via the developer IRC and they told me the account should be rejected at creation if the card does not pass authorization. It is inherently an atomic transaction, allowing a user to resubmit the form in the event the card is rejected. This is the intended workflow. Cashier is breaking this into two different requests. One to create the user, and one to add the card. This is apparently not how Stripe expects customers to be created. On top of that, there are two network requests taking place which significantly slows down signup requests. Stripe already has this handled for us to do customer creation and card authorization in a single request

Specific issue

The line where its checking if the token is not null to add a card to an existing customer is where the issue is:


    /**
     * Create a Stripe customer for the given Stripe model.
     *
     * @param  string  $token
     * @param  array  $options
     * @return \Stripe\Customer
     */
    public function createAsStripeCustomer($token, array $options = [])
    {
        $options = array_key_exists('email', $options)
                ? $options : array_merge($options, ['email' => $this->email]);

        // Here we will create the customer instance on Stripe and store the ID of the
        // user from Stripe. This ID will correspond with the Stripe user instances
        // and allow us to retrieve users from Stripe later when we need to work.
        $customer = StripeCustomer::create(
            $options, $this->getStripeKey()
        );

        $this->stripe_id = $customer->id;

        $this->save();

        // Next we will add the credit card to the user's account on Stripe using this
        // token that was provided to this method. This will allow us to bill users
        // when they subscribe to plans or we need to do one-off charges on them.
        if (! is_null($token)) {
            $this->updateCard($token);
        }

        return $customer;
    }

Solution

Changing the customer creation to hand the source to Stripe at the time the customer is created both authorizes the card and creates the customer in a single step. If the card fails, the customer will be deleted so you're allowed to reattempt the submission. Place the following code in the User model (or whatever model you are using the billable trait):

    public function createAsStripeCustomer($token, array $options = [])
    {
        $options = array_key_exists('email', $options) ? $options : array_merge($options, [
            'email' => $this->email,
        ]);

        if (!is_null($token) && !array_key_exists('token', $options)) {
            $options['source'] = $token;
        }

        // Here we will create the customer instance on Stripe and store the ID of the
        // user from Stripe. This ID will correspond with the Stripe user instances
        // and allow us to retrieve users from Stripe later when we need to work.
        $customer = Customer::create(
            $options, $this->getStripeKey()
        );

        $this->stripe_id = $customer->id;

        $this->save();

        return $customer;
    }

This supports creating a customer without a card, in the same way that the existing code does, but it keeps everything together so if the card fails the customer will be rolled back. It will throw an exception if it fails which you can handle in your script, and you will not need to delete the customer. Additionally, this will cut off an entire network request to Stripe, since creating the customer and adding the card is done in a single request, thereby significantly speeding this action up.

Please let me know if you have any questions or comments about this fix. I could not think of a reason why cashier was doing it in two different steps, but it is entirely possible there is something I am missing.

driesvints commented 5 years ago

I've sent in a PR which fixes this. Please have a look and let me know what you think: https://github.com/laravel/cashier/pull/588

driesvints commented 5 years ago

The behavior for this was changed in 9.0 which will be released soon.

laurencei commented 5 years ago

@driesvints - is there a planned ETA for 9.0?

I've just been bitten hard by this issue on a customer's project, so looking forward to doing an update when available.

driesvints commented 5 years ago

@laurencei probably early next week

IvanBernatovic commented 5 years ago

So is this fixed if I upgrade from Laravel Spark 7. to 8. which uses Cashier 9.*?

driesvints commented 5 years ago

@IvanBernatovic I think @themsaid knows.

yougotnet commented 5 years ago

Yes, it appears to be fixed. I have upgraded to version 9.x and didn't have to modify the code.

themsaid commented 5 years ago

@IvanBernatovic try then report if it's broken :)

IvanBernatovic commented 5 years ago

@themsaid After updating to latest Laravel, Spark and Cashier versions I tried to reproduce this using Stripe test mode with various test cards. I used cards with error responses and every time a new customer was created in Stripe (with the same email) even though the card was declined. So it seems that for the latest Spark version (8.1.0) this issue still persists. Seems that this is Spark issue and I'm in the wrong place.

flexgrip commented 5 years ago

@IvanBernatovic I think I’m running in to the same thing. Although I’m using Spark 7 with L5.7.

Did you find the root cause of this? And is there a way to correct it without modifying the spark core?

yougotnet commented 5 years ago

If you are using Cashier 9.0, then you should be okay. If you are using a version below 9.0, then you might have will have to modify the createAsStripeCustomer function in Cashier as I did.

damayantinama commented 4 years ago

The behavior for this was changed in 9.0 which will be released soon.

I have upgraded Laravel to 6.0, Laravel Spark to 9.0 and Cashier to 10.0

In the Spark Subscription settings, I am seeing undefined as the currency instead of £.

I tried adding CASHIER_CURRENCY=eur to .env but no difference.

brandonmbanks commented 4 years ago

I'm still having this issue. Using Laravel Spark 9.0, Cashier 10. I'm getting duplicate customers and the error This customer has no attached payment source. Looking at the duplicate customers, some of them have a payment method and others don't.