Merge pull request #35 from shopware5/pt-13138/fix-orderId-transactio…

…nId-mixup PT-13138 - Fix orderId and transactionId mixup in webhooks
shopware5 · Sep 22, 2023 · e781bc2 · e781bc2
2 parents b96e683 + bd38dea
commit e781bc2
Show file tree

Hide file tree

Showing 7 changed files with 158 additions and 21 deletions.
diff --git a/Tests/Functional/WebhookHandler/AbstractWebhookTest.php b/Tests/Functional/WebhookHandler/AbstractWebhookTest.php
@@ -56,7 +56,7 @@ public function testGetOrderServiceResultOrderServiceReturnsNullShouldReturnNull
         $abstractWebhook = $this->createAbstractWebhook(true);
 
         $webhook = new Webhook();
-        $webhook->setResource(['id' => 'anyPayPalOrderID']);
+        $webhook->setResource(TestWebhookResource::create('anyPayPalOrderID'));
 
         $result = $abstractWebhook->getResult($webhook);
 
@@ -76,7 +76,7 @@ public function testGetOrderServiceResultShouldReturnOrderServiceResult()
         $abstractWebhook = $this->createAbstractWebhook();
 
         $webhook = new Webhook();
-        $webhook->setResource(['id' => 'unitTestTransactionId']);
+        $webhook->setResource(TestWebhookResource::create('unitTestTransactionId'));
 
         $result = $abstractWebhook->getResult($webhook);
 
@@ -86,6 +86,96 @@ public function testGetOrderServiceResultShouldReturnOrderServiceResult()
         static::assertSame(1, $result->getPaymentStatusId());
     }
 
+    /**
+     * @return void
+     */
+    public function testGetOrderServiceResultLogErrorWithResourceArrayWithoutSupplementaryData()
+    {
+        $abstractWebhook = $this->createAbstractWebhook(true);
+
+        $webhook = new Webhook();
+        $webhook->setResource([]);
+
+        $result = $abstractWebhook->getResult($webhook);
+
+        static::assertNull($result);
+    }
+
+    /**
+     * @return void
+     */
+    public function testGetOrderServiceResultLogErrorWithResourceArrayWithSupplementaryDataIsNotAnArray()
+    {
+        $abstractWebhook = $this->createAbstractWebhook(true);
+
+        $webhook = new Webhook();
+        $webhook->setResource(['supplementary_data' => '']);
+
+        $result = $abstractWebhook->getResult($webhook);
+
+        static::assertNull($result);
+    }
+
+    /**
+     * @return void
+     */
+    public function testGetOrderServiceResultLogErrorWithResourceArrayWithRelatedIdsIsNotAnArray()
+    {
+        $abstractWebhook = $this->createAbstractWebhook(true);
+
+        $webhook = new Webhook();
+        $webhook->setResource(['supplementary_data' => ['related_ids' => '']]);
+
+        $result = $abstractWebhook->getResult($webhook);
+
+        static::assertNull($result);
+    }
+
+    /**
+     * @return void
+     */
+    public function testGetOrderServiceResultLogErrorWithResourceArrayWithoutRelatedIds()
+    {
+        $abstractWebhook = $this->createAbstractWebhook(true);
+
+        $webhook = new Webhook();
+        $webhook->setResource(['supplementary_data' => []]);
+
+        $result = $abstractWebhook->getResult($webhook);
+
+        static::assertNull($result);
+    }
+
+    /**
+     * @return void
+     */
+    public function testGetOrderServiceResultLogErrorWithResourceArrayWithoutOrderId()
+    {
+        $abstractWebhook = $this->createAbstractWebhook(true);
+
+        $webhook = new Webhook();
+        $webhook->setResource(['supplementary_data' => ['related_ids' => []]]);
+
+        $result = $abstractWebhook->getResult($webhook);
+
+        static::assertNull($result);
+    }
+
+    /**
+     * @return void
+     */
+    public function testGetOrderServiceResultLogErrorWithResourceArrayWithOrderIdIsNotAString()
+    {
+        $abstractWebhook = $this->createAbstractWebhook(true);
+
+        $webhook = new Webhook();
+        $webhook->setResource(['supplementary_data' => ['related_ids' => ['order_id' => false]]]);
+
+        $result = $abstractWebhook->getResult($webhook);
+
+        static::assertNull($result);
+    }
+
     /**
      * @param bool $loggerExpectCallErrorMethod
      *

diff --git a/Tests/Functional/WebhookHandler/CheckoutPaymentApprovalReversedTest.php b/Tests/Functional/WebhookHandler/CheckoutPaymentApprovalReversedTest.php
@@ -42,7 +42,7 @@ public function testGetEventType()
     public function testInvokeShouldThrowExceptionBecauseThereIsNoOrderServiceResult()
     {
         $webhook = new Webhook();
-        $webhook->setResource(['id' => 'unitTestTransactionId']);
+        $webhook->setResource(TestWebhookResource::create('unitTestTransactionId'));
 
         $this->expectException(WebhookException::class);
         $this->expectExceptionMessage('SwagPaymentPayPalUnified\WebhookHandlers\CheckoutPaymentApprovalReversed::invoke expect OrderAndPaymentStatusResult, got NULL');
@@ -61,7 +61,7 @@ public function testInvoke()
         $this->getContainer()->get('dbal_connection')->exec($sql);
 
         $webhook = new Webhook();
-        $webhook->setResource(['id' => 'unitTestTransactionId']);
+        $webhook->setResource(TestWebhookResource::create('unitTestTransactionId'));
 
         $result = $this->createCheckoutPaymentApprovalReversed()->invoke($webhook);
         $databaseResult = $this->getContainer()->get('dbal_connection')->createQueryBuilder()

diff --git a/Tests/Functional/WebhookHandler/PaymentCaptureCompletedTest.php b/Tests/Functional/WebhookHandler/PaymentCaptureCompletedTest.php
@@ -42,7 +42,7 @@ public function testGetEventType()
     public function testInvokeShouldReturnFalseBecauseThereIsNoOrderServiceResult()
     {
         $webhook = new Webhook();
-        $webhook->setResource(['id' => 'unitTestTransactionId']);
+        $webhook->setResource(TestWebhookResource::create('unitTestTransactionId'));
 
         $this->expectException(WebhookException::class);
         $this->expectExceptionMessage('SwagPaymentPayPalUnified\WebhookHandlers\PaymentCaptureCompleted::invoke expect OrderAndPaymentStatusResult, got NULL');
@@ -62,7 +62,7 @@ public function testInvokeShouldReturnTrueAndUpdateOrderAndPaymentStatus()
         $this->updatePaymentStatus();
 
         $webhook = new Webhook();
-        $webhook->setResource(['id' => 'unitTestTransactionId']);
+        $webhook->setResource(TestWebhookResource::create('unitTestTransactionId'));
 
         $result = $this->createPaymentCaptureCompleted()->invoke($webhook);
 

diff --git a/Tests/Functional/WebhookHandler/PaymentCaptureDeniedTest.php b/Tests/Functional/WebhookHandler/PaymentCaptureDeniedTest.php
@@ -42,7 +42,7 @@ public function testGetEventType()
     public function testInvokeShouldThrowExceptionBecauseThereIsNoOrderServiceResult()
     {
         $webhook = new Webhook();
-        $webhook->setResource(['id' => 'unitTestTransactionId']);
+        $webhook->setResource(TestWebhookResource::create('unitTestTransactionId'));
 
         $this->expectException(WebhookException::class);
         $this->expectExceptionMessage('SwagPaymentPayPalUnified\WebhookHandlers\PaymentCaptureDenied::invoke expect OrderAndPaymentStatusResult, got NULL');
@@ -61,7 +61,7 @@ public function testInvokeShouldReturnTrueAndUpdateOrderAndPaymentStatus()
         $this->getContainer()->get('dbal_connection')->exec($sql);
 
         $webhook = new Webhook();
-        $webhook->setResource(['id' => 'unitTestTransactionId']);
+        $webhook->setResource(TestWebhookResource::create('unitTestTransactionId'));
 
         $result = $this->createPaymentCaptureDenied()->invoke($webhook);
         $databaseResult = $this->getContainer()->get('dbal_connection')->createQueryBuilder()

diff --git a/Tests/Functional/WebhookHandler/TestWebhookResource.php b/Tests/Functional/WebhookHandler/TestWebhookResource.php
@@ -0,0 +1,28 @@
+<?php
+/**
+ * (c) shopware AG <[email protected]>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace SwagPaymentPayPalUnified\Tests\Functional\WebhookHandler;
+
+class TestWebhookResource
+{
+    /**
+     * @param string $orderId
+     *
+     * @return array<string,mixed>
+     */
+    public static function create($orderId)
+    {
+        return [
+            'supplementary_data' => [
+                'related_ids' => [
+                    'order_id' => $orderId,
+                ],
+            ],
+        ];
+    }
+}
diff --git a/WebhookHandlers/AbstractWebhook.php b/WebhookHandlers/AbstractWebhook.php
@@ -13,6 +13,7 @@
 use SwagPaymentPayPalUnified\PayPalBundle\Components\LoggerServiceInterface;
 use SwagPaymentPayPalUnified\PayPalBundle\Components\Webhook\WebhookEventTypes;
 use SwagPaymentPayPalUnified\PayPalBundle\Structs\Webhook;
+use UnexpectedValueException;
 
 abstract class AbstractWebhook
 {
@@ -49,9 +50,10 @@ protected function getOrderServiceResult(Webhook $webhook)
             return null;
         }
 
-        $transactionId = $resource['id'];
-        if (!\is_string($transactionId)) {
-            $this->logger->error(sprintf('[Webhook]Event: %s. ResourceID is not an string got: %s', $this->getEventType(), \gettype($transactionId)), $webhook->toArray());
+        try {
+            $transactionId = $this->getOrderIdFromResource($resource);
+        } catch (UnexpectedValueException $exception) {
+            $this->logger->error(sprintf('[Webhook]Event: %s. Resource structure is not valid. Message: %s', $this->getEventType(), $exception->getMessage()));
 
             return null;
         }
@@ -66,4 +68,28 @@ protected function getOrderServiceResult(Webhook $webhook)
 
         return $shopwareOrderServiceResult;
     }
+
+    /**
+     * @param array<string,mixed> $resource
+     *
+     * @return string
+     */
+    private function getOrderIdFromResource(array $resource)
+    {
+        if (!\array_key_exists('supplementary_data', $resource) || !\is_array($resource['supplementary_data'])) {
+            throw new UnexpectedValueException('Expect resource has array key "supplementary_data" with array value');
+        }
+
+        $supplementaryData = $resource['supplementary_data'];
+        if (!\array_key_exists('related_ids', $supplementaryData) || !\is_array($supplementaryData['related_ids'])) {
+            throw new UnexpectedValueException('Expect supplementary_data has array key "related_ids" with array value');
+        }
+
+        $relatedIds = $supplementaryData['related_ids'];
+        if (!\array_key_exists('order_id', $relatedIds) || !\is_string($relatedIds['order_id'])) {
+            throw new UnexpectedValueException('Expect related_ids has array key "order_id" with string value');
+        }
+
+        return $relatedIds['order_id'];
+    }
 }
diff --git a/plugin.xml b/plugin.xml
@@ -12,28 +12,21 @@
     <author>shopware AG</author>
     <compatibility minVersion="5.2.27" maxVersion="5.99.99"/>
 
-    <changelog version="6.1.2">
+    <changelog version="6.1.1">
         <changes lang="de">
+            PT-13127 - Verbessert die Benutzerfreundlichkeit für den Rechnungskauf im Checkout-Formular;
             PT-13134 - Verhindert das Erstellen einer PayPal Zahlung mit einem leeren Warenkorb;
             PT-13138 - Verbessert die Bearbeitung von Webhooks;
             PT-13140 - Die Zahlungsart "MyBank" ist nun standardmäßig deaktiviert;
         </changes>
         <changes lang="en">
+            PT-13127 - Improves the usability for pay upon invoice in the checkout form;
             PT-13134 - Prevents the creation of a PayPal payment with an empty shopping cart;
             PT-13138 - Improves the processing of webhooks;
             PT-13140 - The payment method "MyBank" is now deactivated by default;
         </changes>
     </changelog>
 
-    <changelog version="6.1.1">
-        <changes lang="de">
-            PT-13127 - Verbessert die Benutzerfreundlichkeit für den Rechnungskauf im Checkout-Formular;
-        </changes>
-        <changes lang="en">
-            PT-13127 - Improves the usability for pay upon invoice in the checkout form;
-        </changes>
-    </changelog>
-
     <changelog version="6.1.0">
         <changes lang="de">
             PT-13108 - Nutzt Standard-Landingpage, wenn die Einstellung nicht korrekt gesetzt wurde;