From da08a72fa562a414df794487a1ab1138b17a038f Mon Sep 17 00:00:00 2001 From: "Matthew Wire (MJW)" Date: Mon, 3 Aug 2026 16:46:45 +0100 Subject: [PATCH] Financial - Prevent duplicate payments recorded by concurrent Payment::create calls A payment processor's webhook notification for a charge can race a synchronous front-end/back-office confirmation of the same charge (e.g. paying an existing pending contribution via CRM_Contribute_Form_Contribution_Confirm), both calling Payment::create for the same contribution at once. The existing 'contribution already completed' guard only re-checks status right before completing the order, which isn't atomic, and doesn't stop the FinancialTrxn payment row itself being created twice earlier in the function. Add a per-contribution lock via Civi::lockManager() around create(), and reject a second payment with the same trxn_id for a contribution. Includes a regression test using APIv4 Payment::create/get. --- CRM/Financial/BAO/Payment.php | 50 +++++++++++++++++++++++++++ tests/phpunit/api/v3/PaymentTest.php | 51 ++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+) diff --git a/CRM/Financial/BAO/Payment.php b/CRM/Financial/BAO/Payment.php index bdd0e9b384b4..28e930a48ce2 100644 --- a/CRM/Financial/BAO/Payment.php +++ b/CRM/Financial/BAO/Payment.php @@ -19,12 +19,21 @@ use Civi\Api4\FinancialItem; use Civi\Api4\LineItem; use Civi\Api4\EntityFinancialTrxn; +use Civi\Api4\Payment; /** * This class contains payment related functions. */ class CRM_Financial_BAO_Payment { + /** + * Seconds to wait for the per-contribution lock in create() before giving up. + * + * Needs to comfortably cover the slowest thing that can happen while the lock is held - + * completing an order can send a receipt email synchronously. + */ + const PAYMENT_CREATE_LOCK_TIMEOUT = 15; + /** * Function to process additional payment for partial and refund * contributions. @@ -44,6 +53,47 @@ class CRM_Financial_BAO_Payment { * @throws \CRM_Core_Exception */ public static function create(array $params, $disableActionsOnCompleteOrder = FALSE): CRM_Financial_DAO_FinancialTrxn { + // Serialise payment-recording per contribution, and reject a second payment carrying the same + // trxn_id. Without this, a payment processor webhook racing a synchronous front-end/back-office + // confirmation of the same charge (e.g. paying an existing pending contribution via + // CRM_Contribute_Form_Contribution_Confirm) can both read the contribution as not-yet-completed + // and both go on to record a payment for it. + $lock = \Civi::lockManager()->acquire('data.contribute.paymentCreate.' . $params['contribution_id'], self::PAYMENT_CREATE_LOCK_TIMEOUT); + if (!$lock->isAcquired()) { + throw new CRM_Core_Exception(ts('Could not acquire a lock to record a payment for contribution %1. Another payment may currently be being recorded for the same contribution.', [ + 1 => $params['contribution_id'], + ]), 'payment_create_lock_failed'); + } + try { + if (!empty($params['trxn_id'])) { + $existingPaymentCount = Payment::get(FALSE) + ->addWhere('contribution_id', '=', $params['contribution_id']) + ->addWhere('trxn_id', '=', $params['trxn_id']) + ->selectRowCount() + ->execute() + ->count(); + if ($existingPaymentCount) { + throw new CRM_Core_Exception(ts('A payment with transaction ID "%1" has already been recorded for contribution %2.', [ + 1 => $params['trxn_id'], + 2 => $params['contribution_id'], + ]), 'payment_already_recorded'); + } + } + return self::completePayment($params, $disableActionsOnCompleteOrder); + } + finally { + $lock->release(); + } + } + + /** + * @param array $params + * @param bool $disableActionsOnCompleteOrder + * + * @return \CRM_Financial_DAO_FinancialTrxn + * @throws \CRM_Core_Exception + */ + private static function completePayment(array $params, $disableActionsOnCompleteOrder): CRM_Financial_DAO_FinancialTrxn { $contribution = Contribution::get(FALSE) ->addWhere('id', '=', $params['contribution_id']) ->addSelect('*', 'contribution_status_id:name', 'balance_amount', 'paid_amount') diff --git a/tests/phpunit/api/v3/PaymentTest.php b/tests/phpunit/api/v3/PaymentTest.php index 41bd7e26fb94..de1bf0a15439 100644 --- a/tests/phpunit/api/v3/PaymentTest.php +++ b/tests/phpunit/api/v3/PaymentTest.php @@ -1481,6 +1481,57 @@ public function testPaymentCreateTrxnIdAndDates(): void { } + /** + * A second APIv4 Payment::create for the same contribution & trxn_id should be rejected rather + * than recording a second payment. + * + * This is the scenario that occurs when a payment processor's webhook notification for a charge + * races a synchronous front-end/back-office confirmation of the same charge (e.g. + * CRM_Contribute_Form_Contribution_Confirm::processPaymentOnExistingContribution(), which calls + * APIv4 Payment::create directly) - both attempt to record the payment against the same + * contribution using the same processor trxn_id. + */ + public function testCreatePaymentDuplicateTrxnIDIsRejected(): void { + $contributionID = $this->contributionCreate([ + 'contact_id' => $this->individualCreate(), + 'total_amount' => 100, + 'contribution_status_id' => 'Pending', + 'fee_amount' => 0, + ]); + + \Civi\Api4\Payment::create(FALSE) + ->addValue('contribution_id', $contributionID) + ->addValue('total_amount', 100) + ->addValue('trxn_id', 'ch_race_condition') + ->execute(); + + try { + \Civi\Api4\Payment::create(FALSE) + ->addValue('contribution_id', $contributionID) + ->addValue('total_amount', 100) + ->addValue('trxn_id', 'ch_race_condition') + ->execute(); + $this->fail('Expected a duplicate payment to be rejected.'); + } + catch (CRM_Core_Exception $e) { + $this->assertStringContainsString('already been recorded', $e->getMessage()); + $this->assertEquals('payment_already_recorded', $e->getErrorCode()); + } + + $paymentCount = \Civi\Api4\Payment::get(FALSE) + ->addWhere('contribution_id', '=', $contributionID) + ->selectRowCount() + ->execute() + ->count(); + $this->assertEquals(1, $paymentCount, 'Only one payment should have been recorded for the contribution.'); + + $contribution = \Civi\Api4\Contribution::get(FALSE) + ->addWhere('id', '=', $contributionID) + ->addSelect('contribution_status_id:name') + ->execute()->single(); + $this->assertEquals('Completed', $contribution['contribution_status_id:name']); + } + public function testPaymentGetNonPaymentRecords(): void { $this->_apiversion = 4; Civi::settings()->set('always_post_to_accounts_receivable', 1);