fix: system config webhook check (#174)

cyl3x · web-flow · commit f8f9d8e66aa6 · 2025-03-19T16:10:00.000+01:00
fixes shopware/shopware#7537
diff --git a/src/Webhook/Registration/WebhookSubscriber.php b/src/Webhook/Registration/WebhookSubscriber.php
@@ -62,33 +62,39 @@ public function removeSalesChannelWebhookConfiguration(EntityDeletedEvent $event
 
     /**
      * system-config should be written by {@see SettingsSaver} only, which checks the webhook on its own.
-     * Just in case new/changed credentials will be saved via the normal system config save route.
+     * Just in case new/changed credentials will be saved via the normal system config.
      */
     public function checkWebhookBefore(BeforeSystemConfigMultipleChangedEvent $event): void
     {
-        $routeName = (string) $this->requestStack->getMainRequest()?->attributes->getString('_route');
-
-        if (!\str_contains($routeName, 'api.action.core.save.system-config')) {
-            return;
+        if ($config = $this->getConfigToCheck($event)) {
+            $this->webhookSystemConfigHelper->checkWebhookBefore([$event->getSalesChannelId() => $config]);
         }
-
-        /** @var array<string, array<string, mixed>> $config */
-        $config = $event->getConfig();
-        $this->webhookSystemConfigHelper->checkWebhookBefore($config);
     }
 
     /**
      * system-config should be written by {@see SettingsSaver} only, which checks the webhook on its own.
-     * Just in case new/changed credentials will be saved via the normal system config save route.
+     * Just in case new/changed credentials will be saved via the normal system config.
      */
     public function checkWebhookAfter(SystemConfigMultipleChangedEvent $event): void
     {
+        if ($this->getConfigToCheck($event)) {
+            $this->webhookSystemConfigHelper->checkWebhookAfter([$event->getSalesChannelId()]);
+        }
+    }
+
+    /**
+     * @return array<string, mixed>|null
+     */
+    private function getConfigToCheck(BeforeSystemConfigMultipleChangedEvent|SystemConfigMultipleChangedEvent $event): ?array
+    {
+        /** @var array<string, mixed> $config */
+        $config = $event->getConfig();
         $routeName = (string) $this->requestStack->getMainRequest()?->attributes->getString('_route');
 
-        if (!\str_contains($routeName, 'api.action.core.save.system-config')) {
-            return;
+        if (\str_contains($routeName, 'api.action.paypal.settings.save') || !$this->webhookSystemConfigHelper->needsCheck($config)) {
+            return null;
         }
 
-        $this->webhookSystemConfigHelper->checkWebhookAfter(\array_keys($event->getConfig()));
+        return $config;
     }
 }
diff --git a/src/Webhook/Registration/WebhookSystemConfigHelper.php b/src/Webhook/Registration/WebhookSystemConfigHelper.php
@@ -59,7 +59,7 @@ public function checkWebhookBefore(array $newData): array
         $errors = [];
 
         foreach ($newData as $salesChannelId => $newSettings) {
-            if ($salesChannelId === 'null') {
+            if (!$salesChannelId || $salesChannelId === 'null') {
                 $salesChannelId = null;
             }
 
@@ -102,7 +102,7 @@ public function checkWebhookAfter(array $salesChannelIds): array
         $errors = [];
 
         foreach ($salesChannelIds as $salesChannelId) {
-            if ($salesChannelId === 'null') {
+            if (!$salesChannelId || $salesChannelId === 'null') {
                 $salesChannelId = null;
             }
 
@@ -129,6 +129,14 @@ public function checkWebhookAfter(array $salesChannelIds): array
         return $errors;
     }
 
+    /**
+     * @param array<string, mixed> $config
+     */
+    public function needsCheck(array $config): bool
+    {
+        return !empty($this->filterSettings($config));
+    }
+
     private function fetchSettings(?string $salesChannelId, bool $inherit = false): array
     {
         $settings = [];
diff --git a/tests/Webhook/Registration/WebhookSubscriberTest.php b/tests/Webhook/Registration/WebhookSubscriberTest.php
@@ -7,6 +7,7 @@
 
 namespace Swag\PayPal\Test\Webhook\Registration;
 
+use PHPUnit\Framework\MockObject\MockObject;
 use PHPUnit\Framework\TestCase;
 use Psr\Log\NullLogger;
 use Shopware\Core\Framework\Context;
@@ -29,6 +30,7 @@
 use Swag\PayPal\Webhook\Registration\WebhookSystemConfigHelper;
 use Swag\PayPal\Webhook\WebhookRegistry;
 use Swag\PayPal\Webhook\WebhookService;
+use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\RequestStack;
 use Symfony\Component\Routing\RouterInterface;
 
@@ -44,9 +46,15 @@ class WebhookSubscriberTest extends TestCase
 
     private SystemConfigService $systemConfigService;
 
+    private WebhookSystemConfigHelper&MockObject $webhookSystemConfigHelper;
+
+    private RequestStack $requestStack;
+
     protected function setUp(): void
     {
         $this->systemConfigService = SystemConfigServiceMock::createWithCredentials();
+        $this->webhookSystemConfigHelper = $this->createMock(WebhookSystemConfigHelper::class);
+        $this->requestStack = new RequestStack();
     }
 
     public function testRemoveWebhookWithInheritedConfiguration(): void
@@ -73,6 +81,156 @@ public function testRemoveWebhookWithNoConfiguration(): void
         static::assertEmpty($this->systemConfigService->getString(Settings::WEBHOOK_ID, TestDefaults::SALES_CHANNEL));
     }
 
+    public function testCheckWebhookBefore(): void
+    {
+        $event = new BeforeSystemConfigMultipleChangedEvent(['some-key' => 'some-value'], null);
+
+        $this->webhookSystemConfigHelper
+            ->expects(static::once())
+            ->method('checkWebhookBefore')
+            ->with(['' => ['some-key' => 'some-value']]);
+
+        $this->webhookSystemConfigHelper
+            ->expects(static::once())
+            ->method('needsCheck')
+            ->with(['some-key' => 'some-value'])
+            ->willReturn(true);
+
+        $this->createWebhookSubscriber(['' => null, TestDefaults::SALES_CHANNEL => null])
+            ->checkWebhookBefore($event);
+    }
+
+    public function testCheckWebhookBeforeWithSalesChannelId(): void
+    {
+        $event = new BeforeSystemConfigMultipleChangedEvent(['some-key' => 'some-value'], 'some-sales-channel-id');
+
+        $this->webhookSystemConfigHelper
+            ->expects(static::once())
+            ->method('checkWebhookBefore')
+            ->with(['some-sales-channel-id' => ['some-key' => 'some-value']]);
+
+        $this->webhookSystemConfigHelper
+            ->expects(static::once())
+            ->method('needsCheck')
+            ->with(['some-key' => 'some-value'])
+            ->willReturn(true);
+
+        $this->createWebhookSubscriber(['' => null, TestDefaults::SALES_CHANNEL => null])
+            ->checkWebhookBefore($event);
+    }
+
+    public function testCheckWebhookBeforeWithSaveRoute(): void
+    {
+        $request = new Request(attributes: ['_route' => 'api.action.paypal.settings.save']);
+        $this->requestStack->push($request);
+
+        $event = new BeforeSystemConfigMultipleChangedEvent(['some-key' => 'some-value'], 'some-sales-channel-id');
+
+        $this->webhookSystemConfigHelper
+            ->expects(static::never())
+            ->method('checkWebhookBefore');
+
+        $this->webhookSystemConfigHelper
+            ->expects(static::never())
+            ->method('needsCheck');
+
+        $this->createWebhookSubscriber(['' => null, TestDefaults::SALES_CHANNEL => null])
+            ->checkWebhookBefore($event);
+    }
+
+    public function testCheckWebhookBeforeWithNoCheckNeeded(): void
+    {
+        $event = new BeforeSystemConfigMultipleChangedEvent(['some-key' => 'some-value'], 'some-sales-channel-id');
+
+        $this->webhookSystemConfigHelper
+            ->expects(static::never())
+            ->method('checkWebhookBefore');
+
+        $this->webhookSystemConfigHelper
+            ->expects(static::once())
+            ->method('needsCheck')
+            ->with(['some-key' => 'some-value'])
+            ->willReturn(false);
+
+        $this->createWebhookSubscriber(['' => null, TestDefaults::SALES_CHANNEL => null])
+            ->checkWebhookBefore($event);
+    }
+
+    public function testCheckWebhookAfter(): void
+    {
+        $event = new SystemConfigMultipleChangedEvent(['some-key' => 'some-value'], null);
+
+        $this->webhookSystemConfigHelper
+            ->expects(static::once())
+            ->method('checkWebhookAfter')
+            ->with(['']);
+
+        $this->webhookSystemConfigHelper
+            ->expects(static::once())
+            ->method('needsCheck')
+            ->with(['some-key' => 'some-value'])
+            ->willReturn(true);
+
+        $this->createWebhookSubscriber(['' => null, TestDefaults::SALES_CHANNEL => null])
+            ->checkWebhookAfter($event);
+    }
+
+    public function testCheckWebhookAfterWithSalesChannelId(): void
+    {
+        $event = new SystemConfigMultipleChangedEvent(['some-key' => 'some-value'], 'some-sales-channel-id');
+
+        $this->webhookSystemConfigHelper
+            ->expects(static::once())
+            ->method('checkWebhookAfter')
+            ->with(['some-sales-channel-id']);
+
+        $this->webhookSystemConfigHelper
+            ->expects(static::once())
+            ->method('needsCheck')
+            ->with(['some-key' => 'some-value'])
+            ->willReturn(true);
+
+        $this->createWebhookSubscriber(['' => null, TestDefaults::SALES_CHANNEL => null])
+            ->checkWebhookAfter($event);
+    }
+
+    public function testCheckWebhookAfterWithSaveRoute(): void
+    {
+        $request = new Request(attributes: ['_route' => 'api.action.paypal.settings.save']);
+        $this->requestStack->push($request);
+
+        $event = new SystemConfigMultipleChangedEvent(['some-key' => 'some-value'], 'some-sales-channel-id');
+
+        $this->webhookSystemConfigHelper
+            ->expects(static::never())
+            ->method('checkWebhookAfter');
+
+        $this->webhookSystemConfigHelper
+            ->expects(static::never())
+            ->method('needsCheck');
+
+        $this->createWebhookSubscriber(['' => null, TestDefaults::SALES_CHANNEL => null])
+            ->checkWebhookAfter($event);
+    }
+
+    public function testCheckWebhookAfterWithNoCheckNeeded(): void
+    {
+        $event = new SystemConfigMultipleChangedEvent(['some-key' => 'some-value'], 'some-sales-channel-id');
+
+        $this->webhookSystemConfigHelper
+            ->expects(static::never())
+            ->method('checkWebhookAfter');
+
+        $this->webhookSystemConfigHelper
+            ->expects(static::once())
+            ->method('needsCheck')
+            ->with(['some-key' => 'some-value'])
+            ->willReturn(false);
+
+        $this->createWebhookSubscriber(['' => null, TestDefaults::SALES_CHANNEL => null])
+            ->checkWebhookAfter($event);
+    }
+
     public function testSubscribedEvents(): void
     {
         static::assertEqualsCanonicalizing([
@@ -105,8 +263,8 @@ private function createWebhookSubscriber(array $configuration): WebhookSubscribe
             new NullLogger(),
             $this->systemConfigService,
             $webhookService,
-            $this->createMock(WebhookSystemConfigHelper::class),
-            new RequestStack(),
+            $this->webhookSystemConfigHelper,
+            $this->requestStack,
         );
     }
 
diff --git a/tests/Webhook/Registration/WebhookSystemConfigHelperTest.php b/tests/Webhook/Registration/WebhookSystemConfigHelperTest.php
@@ -0,0 +1,83 @@
+<?php declare(strict_types=1);
+/*
+ * (c) shopware AG <info@shopware.com>
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Swag\PayPal\Test\Webhook\Registration;
+
+use PHPUnit\Framework\Attributes\CoversClass;
+use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\TestCase;
+use Psr\Log\NullLogger;
+use Shopware\Core\Framework\Log\Package;
+use Shopware\Core\System\SystemConfig\SystemConfigService;
+use Swag\PayPal\Setting\Service\SettingsValidationServiceInterface;
+use Swag\PayPal\Setting\Settings;
+use Swag\PayPal\Webhook\Registration\WebhookSystemConfigHelper;
+use Swag\PayPal\Webhook\WebhookServiceInterface;
+
+/**
+ * @internal
+ */
+#[Package('checkout')]
+#[CoversClass(WebhookSystemConfigHelper::class)]
+class WebhookSystemConfigHelperTest extends TestCase
+{
+    private const WEBHOOK_KEYS = [
+        Settings::CLIENT_ID,
+        Settings::CLIENT_SECRET,
+        Settings::CLIENT_ID_SANDBOX,
+        Settings::CLIENT_SECRET_SANDBOX,
+        Settings::SANDBOX,
+        Settings::WEBHOOK_ID,
+    ];
+
+    private WebhookSystemConfigHelper $helper;
+
+    protected function setUp(): void
+    {
+        $this->helper = new WebhookSystemConfigHelper(
+            new NullLogger(),
+            $this->createMock(WebhookServiceInterface::class),
+            $this->createMock(SystemConfigService::class),
+            $this->createMock(SettingsValidationServiceInterface::class),
+        );
+    }
+
+    /**
+     * @param array<string, mixed> $config
+     */
+    #[DataProvider('providerNeedsCheck')]
+    public function testNeedsCheck(bool $expected, array $config): void
+    {
+        static::assertSame($expected, $this->helper->needsCheck($config));
+    }
+
+    public static function providerNeedsCheck(): \Generator
+    {
+        foreach (self::WEBHOOK_KEYS as $key) {
+            yield \sprintf('needs check for "%s"', $key) => [
+                true,
+                [$key => 'some-value'],
+            ];
+
+            yield \sprintf('needs check for "%s" with null value', $key) => [
+                true,
+                [$key => null],
+            ];
+        }
+
+        foreach (Settings::DEFAULT_VALUES as $key => $value) {
+            if (\in_array($key, self::WEBHOOK_KEYS, true)) {
+                continue;
+            }
+
+            yield \sprintf('no check for "%s"', $key) => [
+                false,
+                [$key => $value],
+            ];
+        }
+    }
+}
