From fd8a77746ccc566cb0edc7e6d19ca3dcf36a10a5 Mon Sep 17 00:00:00 2001 From: Andreas Sonnleitner <56999154+asonnleitner@users.noreply.github.com> Date: Tue, 19 Sep 2023 16:14:58 +0200 Subject: [PATCH] fix(payments-plugin): Fix stripe payment transaction handling (#2402) Implement transactional handling for Stripe Webhook. This is in response to race conditions arising in setups using DB replication. - Wrap all Stripe webhook-related operations inside a single transaction using `TransactionalConnection.withTransaction()`. - Ensures that all database operations within the webhook use the "master" instance. This change aims to solve the issue of database operations in the Stripe webhook not consistently using the master instance, which led to inconsistencies in low-latency environments. --- packages/payments-plugin/src/stripe/index.ts | 3 +- .../src/stripe/stripe-client.ts | 2 +- .../src/stripe/stripe-utils.ts | 14 +- .../src/stripe/stripe.controller.ts | 131 +++++++++--------- .../src/stripe/stripe.resolver.ts | 15 +- .../src/stripe/stripe.service.ts | 20 +-- packages/payments-plugin/src/stripe/types.ts | 6 +- 7 files changed, 81 insertions(+), 110 deletions(-) diff --git a/packages/payments-plugin/src/stripe/index.ts b/packages/payments-plugin/src/stripe/index.ts index ae146a8dac..cdd439f581 100644 --- a/packages/payments-plugin/src/stripe/index.ts +++ b/packages/payments-plugin/src/stripe/index.ts @@ -1,2 +1 @@ -export * from './stripe.plugin'; -export * from './'; +export { StripePlugin } from './stripe.plugin'; diff --git a/packages/payments-plugin/src/stripe/stripe-client.ts b/packages/payments-plugin/src/stripe/stripe-client.ts index 86d1277931..6891b7b23d 100644 --- a/packages/payments-plugin/src/stripe/stripe-client.ts +++ b/packages/payments-plugin/src/stripe/stripe-client.ts @@ -6,7 +6,7 @@ import Stripe from 'stripe'; export class VendureStripeClient extends Stripe { constructor(private apiKey: string, public webhookSecret: string) { super(apiKey, { - apiVersion: null as any, // Use accounts default version + apiVersion: null as unknown as Stripe.LatestApiVersion, // Use accounts default version }); } } diff --git a/packages/payments-plugin/src/stripe/stripe-utils.ts b/packages/payments-plugin/src/stripe/stripe-utils.ts index 92daeccab8..3f5c55ef71 100644 --- a/packages/payments-plugin/src/stripe/stripe-utils.ts +++ b/packages/payments-plugin/src/stripe/stripe-utils.ts @@ -12,10 +12,7 @@ import { CurrencyCode, Order } from '@vendure/core'; * stores money amounts multiplied by 100). See https://github.com/vendure-ecommerce/vendure/issues/1630 */ export function getAmountInStripeMinorUnits(order: Order): number { - const amountInStripeMinorUnits = currencyHasFractionPart(order.currencyCode) - ? order.totalWithTax - : Math.round(order.totalWithTax / 100); - return amountInStripeMinorUnits; + return currencyHasFractionPart(order.currencyCode) ? order.totalWithTax : Math.round(order.totalWithTax / 100); } /** @@ -24,10 +21,7 @@ export function getAmountInStripeMinorUnits(order: Order): number { * used by Vendure. */ export function getAmountFromStripeMinorUnits(order: Order, stripeAmount: number): number { - const amountInVendureMinorUnits = currencyHasFractionPart(order.currencyCode) - ? stripeAmount - : stripeAmount * 100; - return amountInVendureMinorUnits; + return currencyHasFractionPart(order.currencyCode) ? stripeAmount : stripeAmount * 100; } function currencyHasFractionPart(currencyCode: CurrencyCode): boolean { @@ -36,6 +30,6 @@ function currencyHasFractionPart(currencyCode: CurrencyCode): boolean { currency: currencyCode, currencyDisplay: 'symbol', }).formatToParts(123.45); - const hasFractionPart = !!parts.find(p => p.type === 'fraction'); - return hasFractionPart; + + return !!parts.find(p => p.type === 'fraction'); } diff --git a/packages/payments-plugin/src/stripe/stripe.controller.ts b/packages/payments-plugin/src/stripe/stripe.controller.ts index f23d40f24f..672fbb1e89 100644 --- a/packages/payments-plugin/src/stripe/stripe.controller.ts +++ b/packages/payments-plugin/src/stripe/stripe.controller.ts @@ -1,18 +1,9 @@ import { Controller, Headers, HttpStatus, Post, Req, Res } from '@nestjs/common'; -import { - InternalServerError, - LanguageCode, - Logger, - Order, - OrderService, - PaymentMethod, - PaymentMethodService, - RequestContext, - RequestContextService, -} from '@vendure/core'; +import type { PaymentMethod, RequestContext } from '@vendure/core'; +import { InternalServerError, LanguageCode, Logger, Order, OrderService, PaymentMethodService, RequestContextService, TransactionalConnection } from '@vendure/core'; import { OrderStateTransitionError } from '@vendure/core/dist/common/error/generated-graphql-shop-errors'; -import { Response } from 'express'; -import Stripe from 'stripe'; +import type { Response } from 'express'; +import type Stripe from 'stripe'; import { loggerCtx } from './constants'; import { stripePaymentMethodHandler } from './stripe.handler'; @@ -30,6 +21,7 @@ export class StripeController { private orderService: OrderService, private stripeService: StripeService, private requestContextService: RequestContextService, + private connection: TransactionalConnection, ) {} @Post('stripe') @@ -43,72 +35,76 @@ export class StripeController { response.status(HttpStatus.BAD_REQUEST).send(missingHeaderErrorMessage); return; } + const event = request.body as Stripe.Event; const paymentIntent = event.data.object as Stripe.PaymentIntent; + if (!paymentIntent) { Logger.error(noPaymentIntentErrorMessage, loggerCtx); response.status(HttpStatus.BAD_REQUEST).send(noPaymentIntentErrorMessage); return; } + const { metadata: { channelToken, orderCode, orderId } = {} } = paymentIntent; - const ctx = await this.createContext(channelToken, request); - const order = await this.orderService.findOneByCode(ctx, orderCode); - if (!order) { - throw Error(`Unable to find order ${orderCode}, unable to settle payment ${paymentIntent.id}!`); - } - try { - // Throws an error if the signature is invalid - await this.stripeService.constructEventFromPayload(ctx, order, request.rawBody, signature); - } catch (e: any) { - Logger.error(`${signatureErrorMessage} ${signature}: ${(e as Error)?.message}`, loggerCtx); - response.status(HttpStatus.BAD_REQUEST).send(signatureErrorMessage); - return; - } - if (event.type === 'payment_intent.payment_failed') { - const message = paymentIntent.last_payment_error?.message ?? 'unknown error'; - Logger.warn(`Payment for order ${orderCode} failed: ${message}`, loggerCtx); - response.status(HttpStatus.OK).send('Ok'); - return; - } - if (event.type !== 'payment_intent.succeeded') { - // This should never happen as the webhook is configured to receive - // payment_intent.succeeded and payment_intent.payment_failed events only - Logger.info(`Received ${event.type} status update for order ${orderCode}`, loggerCtx); - return; - } - if (order.state !== 'ArrangingPayment') { - const transitionToStateResult = await this.orderService.transitionToState( - ctx, - orderId, - 'ArrangingPayment', - ); - - if (transitionToStateResult instanceof OrderStateTransitionError) { - Logger.error( - `Error transitioning order ${orderCode} to ArrangingPayment state: ${transitionToStateResult.message}`, - loggerCtx, - ); + const outerCtx = await this.createContext(channelToken, request); + + await this.connection.withTransaction(outerCtx, async (ctx) => { + const order = await this.orderService.findOneByCode(ctx, orderCode); + + if (!order) { + throw new Error(`Unable to find order ${orderCode}, unable to settle payment ${paymentIntent.id}!`); + } + + try { + // Throws an error if the signature is invalid + await this.stripeService.constructEventFromPayload(ctx, order, request.rawBody, signature); + } catch (e: any) { + Logger.error(`${signatureErrorMessage} ${signature}: ${(e as Error)?.message}`, loggerCtx); + response.status(HttpStatus.BAD_REQUEST).send(signatureErrorMessage); return; } - } - const paymentMethod = await this.getPaymentMethod(ctx); + if (event.type === 'payment_intent.payment_failed') { + const message = paymentIntent.last_payment_error?.message ?? 'unknown error'; + Logger.warn(`Payment for order ${orderCode} failed: ${message}`, loggerCtx); + response.status(HttpStatus.OK).send('Ok'); + return; + } - const addPaymentToOrderResult = await this.orderService.addPaymentToOrder(ctx, orderId, { - method: paymentMethod.code, - metadata: { - paymentIntentAmountReceived: paymentIntent.amount_received, - paymentIntentId: paymentIntent.id, - }, - }); + if (event.type !== 'payment_intent.succeeded') { + // This should never happen as the webhook is configured to receive + // payment_intent.succeeded and payment_intent.payment_failed events only + Logger.info(`Received ${event.type} status update for order ${orderCode}`, loggerCtx); + return; + } - if (!(addPaymentToOrderResult instanceof Order)) { - Logger.error( - `Error adding payment to order ${orderCode}: ${addPaymentToOrderResult.message}`, - loggerCtx, - ); - return; - } + if (order.state !== 'ArrangingPayment') { + const transitionToStateResult = await this.orderService.transitionToState( + ctx, + orderId, + 'ArrangingPayment', + ); + + if (transitionToStateResult instanceof OrderStateTransitionError) { + Logger.error(`Error transitioning order ${orderCode} to ArrangingPayment state: ${transitionToStateResult.message}`, loggerCtx); + return; + } + } + + const paymentMethod = await this.getPaymentMethod(ctx); + + const addPaymentToOrderResult = await this.orderService.addPaymentToOrder(ctx, orderId, { + method: paymentMethod.code, + metadata: { + paymentIntentAmountReceived: paymentIntent.amount_received, + paymentIntentId: paymentIntent.id, + }, + }); + + if (!(addPaymentToOrderResult instanceof Order)) { + Logger.error(`Error adding payment to order ${orderCode}: ${addPaymentToOrderResult.message}`, loggerCtx); + } + }); Logger.info(`Stripe payment intent id ${paymentIntent.id} added to order ${orderCode}`, loggerCtx); response.status(HttpStatus.OK).send('Ok'); @@ -127,8 +123,9 @@ export class StripeController { const method = (await this.paymentMethodService.findAll(ctx)).items.find( m => m.handler.code === stripePaymentMethodHandler.code, ); + if (!method) { - throw new InternalServerError(`[${loggerCtx}] Could not find Stripe PaymentMethod`); + throw new InternalServerError(`[${loggerCtx}] Could not find Stripe PaymentMethod`); } return method; diff --git a/packages/payments-plugin/src/stripe/stripe.resolver.ts b/packages/payments-plugin/src/stripe/stripe.resolver.ts index efbec0dd78..6addf8cdad 100644 --- a/packages/payments-plugin/src/stripe/stripe.resolver.ts +++ b/packages/payments-plugin/src/stripe/stripe.resolver.ts @@ -1,19 +1,14 @@ import { Mutation, Resolver } from '@nestjs/graphql'; -import { - ActiveOrderService, - Allow, - Ctx, - Permission, - RequestContext, - UnauthorizedError, - UserInputError, -} from '@vendure/core'; +import { ActiveOrderService, Allow, Ctx, Permission, RequestContext, UnauthorizedError, UserInputError } from '@vendure/core'; import { StripeService } from './stripe.service'; @Resolver() export class StripeResolver { - constructor(private stripeService: StripeService, private activeOrderService: ActiveOrderService) {} + constructor( + private stripeService: StripeService, + private activeOrderService: ActiveOrderService, + ) {} @Mutation() @Allow(Permission.Owner) diff --git a/packages/payments-plugin/src/stripe/stripe.service.ts b/packages/payments-plugin/src/stripe/stripe.service.ts index 7c3342a076..2a094da02d 100644 --- a/packages/payments-plugin/src/stripe/stripe.service.ts +++ b/packages/payments-plugin/src/stripe/stripe.service.ts @@ -1,18 +1,7 @@ import { Inject, Injectable } from '@nestjs/common'; import { ModuleRef } from '@nestjs/core'; import { ConfigArg } from '@vendure/common/lib/generated-types'; -import { - Ctx, - Customer, - Injector, - Logger, - Order, - Payment, - PaymentMethodService, - RequestContext, - TransactionalConnection, - UserInputError, -} from '@vendure/core'; +import { Customer, Injector, Logger, Order, Payment, PaymentMethodService, RequestContext, TransactionalConnection, UserInputError } from '@vendure/core'; import Stripe from 'stripe'; import { loggerCtx, STRIPE_PLUGIN_OPTIONS } from './constants'; @@ -25,9 +14,9 @@ import { StripePluginOptions } from './types'; @Injectable() export class StripeService { constructor( + @Inject(STRIPE_PLUGIN_OPTIONS) private options: StripePluginOptions, private connection: TransactionalConnection, private paymentMethodService: PaymentMethodService, - @Inject(STRIPE_PLUGIN_OPTIONS) private options: StripePluginOptions, private moduleRef: ModuleRef, ) {} @@ -64,10 +53,7 @@ export class StripeService { if (!client_secret) { // This should never happen - Logger.warn( - `Payment intent creation for order ${order.code} did not return client secret`, - loggerCtx, - ); + Logger.warn(`Payment intent creation for order ${order.code} did not return client secret`, loggerCtx); throw Error('Failed to create payment intent'); } diff --git a/packages/payments-plugin/src/stripe/types.ts b/packages/payments-plugin/src/stripe/types.ts index 998783f243..7532c2bd1d 100644 --- a/packages/payments-plugin/src/stripe/types.ts +++ b/packages/payments-plugin/src/stripe/types.ts @@ -1,7 +1,7 @@ -import { Injector, Order, RequestContext } from '@vendure/core'; import '@vendure/core/dist/entity/custom-entity-fields'; -import { Request } from 'express'; -import Stripe from 'stripe'; +import type { Injector, Order, RequestContext } from '@vendure/core'; +import type { Request } from 'express'; +import type Stripe from 'stripe'; // Note: deep import is necessary here because CustomCustomerFields is also extended in the Braintree // plugin. Reference: https://github.com/microsoft/TypeScript/issues/46617