From 9be1d2de4f8612d1479a4edf30a3ab35ce75da3f Mon Sep 17 00:00:00 2001 From: Fabien O'Carroll Date: Wed, 10 Mar 2021 11:12:44 +0000 Subject: [PATCH] Updated invoice webhook handling for payment events no-issue 1. We do not want to store payment events for payments of 0 value 2. Stripe webhooks can arrive and be processed "out of order", which can result in us attempting to add a payment event for a member which does not yet exist. The change here will 404 in such (edge) cases, so that Stripe will retry the webhook at a later point, when the Member has been created, allowing us to store the payment event. --- ghost/members-api/index.js | 3 +- .../lib/services/stripe-webhook/index.js | 19 +++++++- ghost/members-api/package.json | 1 + .../unit/lib/services/stripe-webhook.test.js | 45 +++++++++++++++++++ 4 files changed, 66 insertions(+), 2 deletions(-) create mode 100644 ghost/members-api/test/unit/lib/services/stripe-webhook.test.js diff --git a/ghost/members-api/index.js b/ghost/members-api/index.js index c9f91b6d28..5f3e7d3ef9 100644 --- a/ghost/members-api/index.js +++ b/ghost/members-api/index.js @@ -90,6 +90,7 @@ module.exports = function MembersApi({ const stripeWebhookService = new StripeWebhookService({ StripeWebhook, stripeAPIService, + stripePlansService, memberRepository, eventRepository, sendEmailWithMagicLink @@ -312,7 +313,7 @@ module.exports = function MembersApi({ res.end(); } catch (err) { common.logging.error(`Error handling webhook ${event.type}`, err); - res.writeHead(400); + res.writeHead(err.statusCode || 500); res.end(); } }); diff --git a/ghost/members-api/lib/services/stripe-webhook/index.js b/ghost/members-api/lib/services/stripe-webhook/index.js index 91f7954b71..04753a026d 100644 --- a/ghost/members-api/lib/services/stripe-webhook/index.js +++ b/ghost/members-api/lib/services/stripe-webhook/index.js @@ -1,10 +1,12 @@ const _ = require('lodash'); +const errors = require('@tryghost/errors'); module.exports = class StripeWebhookService { /** * @param {object} deps * @param {any} deps.StripeWebhook * @param {import('../stripe-api')} deps.stripeAPIService + * @param {import('../stripe-plans')} deps.stripePlansService * @param {import('../../repositories/member')} deps.memberRepository * @param {import('../../repositories/event')} deps.eventRepository * @param {any} deps.sendEmailWithMagicLink @@ -12,12 +14,14 @@ module.exports = class StripeWebhookService { constructor({ StripeWebhook, stripeAPIService, + stripePlansService, memberRepository, eventRepository, sendEmailWithMagicLink }) { this._StripeWebhook = StripeWebhook; this._stripeAPIService = stripeAPIService; + this._stripePlansService = stripePlansService; this._memberRepository = memberRepository; this._eventRepository = eventRepository; this._sendEmailWithMagicLink = sendEmailWithMagicLink; @@ -152,13 +156,26 @@ module.exports = class StripeWebhookService { }); if (member) { - if (invoice.paid) { + if (invoice.paid && invoice.amount_paid !== 0) { await this._eventRepository.registerPayment({ member_id: member.id, currency: invoice.currency, amount: invoice.amount_paid }); } + } else { + // Subscription has more than one plan - meaning it is not one created by us - ignore. + if (!subscription.plan) { + return; + } + // Subscription is for a different product - ignore. + if (this._stripePlansService.getProduct().id !== subscription.plan.product) { + return; + } + // Could not find the member, which we need in order to insert an payment event. + throw new errors.NotFoundError({ + message: `No member found for customer ${subscription.customer}` + }); } } diff --git a/ghost/members-api/package.json b/ghost/members-api/package.json index 3b08f776d2..f4d525fdd8 100644 --- a/ghost/members-api/package.json +++ b/ghost/members-api/package.json @@ -25,6 +25,7 @@ "sinon": "7.5.0" }, "dependencies": { + "@tryghost/errors": "^0.2.9", "@tryghost/magic-link": "^0.6.7", "bluebird": "^3.5.4", "body-parser": "^1.19.0", diff --git a/ghost/members-api/test/unit/lib/services/stripe-webhook.test.js b/ghost/members-api/test/unit/lib/services/stripe-webhook.test.js new file mode 100644 index 0000000000..d9eb0d29d0 --- /dev/null +++ b/ghost/members-api/test/unit/lib/services/stripe-webhook.test.js @@ -0,0 +1,45 @@ +const {describe, it} = require('mocha'); +const should = require('should'); +const sinon = require('sinon'); +const StripeAPIService = require('../../../../lib/services/stripe-api'); +const StripePlansService = require('../../../../lib/services/stripe-plans'); +const StripeWebhookService = require('../../../../lib/services/stripe-webhook'); +const MemberRepository = require('../../../../lib/repositories/member'); + +function mock(Class) { + return sinon.stub(Object.create(Class.prototype)); +} + +describe('StripeWebhookService', function () { + describe('invoice.payment_succeeded webhooks', function () { + it('Should throw a 404 error when a member is not found for a valid Ghost Members invoice', async function () { + const stripeWebhookService = new StripeWebhookService({ + stripeAPIService: mock(StripeAPIService), + stripePlansService: mock(StripePlansService), + memberRepository: mock(MemberRepository) + }); + + stripeWebhookService._stripeAPIService.getSubscription.resolves({ + customer: 'customer_id', + plan: { + product: 'product_id' + } + }); + + stripeWebhookService._memberRepository.get.resolves(null); + + stripeWebhookService._stripePlansService.getProduct.returns({ + id: 'product_id' + }); + + try { + await stripeWebhookService.invoiceEvent({ + subscription: 'sub_id' + }); + should.fail(); + } catch (err) { + should.equal(err.statusCode, 404); + } + }); + }); +});