The ::onNotify and ::onReturn methods can collide causing a race condition.

We need to implement a lock backend.

@see: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Lock%21Lo...

CommentFileSizeAuthor
#4 commerce_sermepa-race_condition-3003454-4.patch11.46 KBfacine
#2 commerce_sermepa-race_condition-3003454-2.patch8.45 KBfacine

Comments

facine created an issue. See original summary.

facine’s picture

facine commented
Status: Active » Needs review
StatusFileSize
new commerce_sermepa-race_condition-3003454-2.patch8.45 KB
pcambra’s picture

pcambra
he/him
Primary language Spanish
commented
Status: Needs review » Needs work 
  1. +++ b/src/Plugin/Commerce/PaymentGateway/Sermepa.php
@@ -309,51 +325,58 @@ class Sermepa extends OffsitePaymentGatewayBase implements HasPaymentInstruction
+        throw new PaymentGatewayException('Bad feedback response, signatures does not match.');

    I think this message is way too technical, is this displayed to the user making a purchase? We should also log this. 

  2. +++ b/src/Plugin/Commerce/PaymentGateway/Sermepa.php
@@ -309,51 +325,58 @@ class Sermepa extends OffsitePaymentGatewayBase implements HasPaymentInstruction
+      // Process the transaction.

    We really need a better comment here, what is "99"? maybe it should be a constant instead. 

  3. +++ b/src/Plugin/Commerce/PaymentGateway/Sermepa.php
@@ -309,51 +325,58 @@ class Sermepa extends OffsitePaymentGatewayBase implements HasPaymentInstruction
+          $credit_card = $order->get('payment_method')->first()->get('entity')->getTarget()->getValue();

    Is there not a method already in commerce to get the payment method out of an order? 

  4. +++ b/src/Plugin/Commerce/PaymentGateway/Sermepa.php
@@ -309,51 +325,58 @@ class Sermepa extends OffsitePaymentGatewayBase implements HasPaymentInstruction
+        $payment->save();

    Review is confusing here, is this block captured by a try? not sure what happens if all this fails. 

  5. +++ b/src/Plugin/Commerce/PaymentGateway/Sermepa.php
@@ -309,51 +325,58 @@ class Sermepa extends OffsitePaymentGatewayBase implements HasPaymentInstruction
+      $this->lock->release('commerce_sermepa_process_request_' . $order->id());

    I'd go for order uuid instead for locking/releasing the wait 

  6. +++ b/src/Plugin/Commerce/PaymentGateway/Sermepa.php
@@ -309,51 +325,58 @@ class Sermepa extends OffsitePaymentGatewayBase implements HasPaymentInstruction
+      throw new PaymentGatewayException('Failed attempt, the payment could not be made.');

    Same as above, we should log this and make messages friendlier if they're displayed to the user.

facine’s picture

facine commented
Status: Needs work » Needs review
StatusFileSize
new commerce_sermepa-race_condition-3003454-4.patch11.46 KB

Hi @pcambra attach a new patch, sorry but I missed the previous code and I have not an interdiff file.

* All exceptions are cached and logged in the PaymentCheckoutController.
* Added a new method to obtains the lock id.
* Added a new method in the external library to validate authorization codes.
* Also comment that commerce has not any method to get the payment method out of an order.

  • facine committed 64d8b3a on 8.x-2.x 
    Issue #3003454 by facine, pcambra: The ::onNotify and ::onReturn methods...
facine’s picture

facine commented

Fixed, thanks!

facine’s picture

facine commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.