bug #73 Webhooks final fix (Zales0123)

This PR was merged into the main branch.

Discussion
----------

So, the problem with webhooks was the (highly possible, as tests on production shown) possibility to handle the payment by webhook **and** synchronously at the same time. Because Payum jumps between different URLs and is not because of that _transactional_, it was impossible to wrap both webhook and synchronous logic in a transaction.

So, after many trials and failures, I've come up with an implementation that marks the payment as already handled by one of these processes and uses the lock to control access to the service that marks the payment. As a result, depending on which process starts first (synchronous or asynchronous), the first would pass, marking a payment as being processed, while the latter would see the information the payment is already handled by the other process.

As far as I understand, we have only 2 places in the code where the payment processing can start: `WebhookAction` and `PrepareAssertAction` - that's why I've put the locking logic into them.

I've also added a lot of logging to make it easier to test on prod 🚀 

This PR requires a lot of testing (on the prod environment!), to see if there are any other places where it fails 🖖

Commits
-------

6d70332 Webhooks final fix
7e044d5 Minor fixes
b8cd71c Bump lock version
5ebd51f Handle it properly on capture
b439488 Fix tests and coding standards

composer.json

@@ -11,6 +11,7 @@
         "sylius/mailer-bundle": "^1.8 || ^2.0",
         "sylius/sylius": "~1.10.0 || ~1.11.0 || ~1.12.0",
         "symfony/http-client": "5.4.* || ^6.0",
+        "symfony/lock": "5.4.* || ^6.0",
         "symfony/messenger": "5.4.* || ^6.0",
         "symfony/uid": "5.4.* || ^6.0",
         "symfony/webpack-encore-bundle": "^1.15"

config/packages/lock.yaml

@@ -0,0 +1,2 @@
+framework:
+    lock: ~

config/packages/monolog.php

@@ -0,0 +1,25 @@
+<?php
+
+declare(strict_types=1);
+
+use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator;
+
+return static function (ContainerConfigurator $containerConfigurator) {
+    $containerConfigurator->extension('monolog', [
+        'channels' => ['saferpay'],
+        'handlers' => [
+            'main' => [
+                'type' => 'stream',
+                'path' => '%kernel.logs_dir%/%kernel.environment%.log',
+                'level' => 'debug',
+                'channels' => ['!event', '!doctrine'],
+            ],
+            'saferpay' => [
+                'type' => 'stream',
+                'path' => '%kernel.logs_dir%/saferpay.log',
+                'level' => 'debug',
+                'channels' => ['saferpay'],
+            ],
+        ],
+    ]);
+};

config/services.php

@@ -2,10 +2,13 @@
 
 declare(strict_types=1);
 
+use CommerceWeavers\SyliusSaferpayPlugin\Processor\SaferpayPaymentProcessor;
+use CommerceWeavers\SyliusSaferpayPlugin\Processor\SaferpayPaymentProcessorInterface;
 use CommerceWeavers\SyliusSaferpayPlugin\Resolver\SaferpayApiBaseUrlResolver;
 use CommerceWeavers\SyliusSaferpayPlugin\Resolver\SaferpayApiBaseUrlResolverInterface;
 use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator;
 use function Symfony\Component\DependencyInjection\Loader\Configurator\param;
+use function Symfony\Component\DependencyInjection\Loader\Configurator\service;
 
 return static function (ContainerConfigurator $containerConfigurator) {
     $containerConfigurator->import(__DIR__ . '/services/**/**');
@@ -19,4 +22,14 @@
             param('commerce_weavers.saferpay.test_api_base_url'),
         ])
     ;
+
+    $services
+        ->set(SaferpayPaymentProcessorInterface::class, SaferpayPaymentProcessor::class)
+        ->public()
+        ->args([
+            service('lock.factory'),
+            service('doctrine.orm.entity_manager'),
+            service('monolog.logger.saferpay'),
+        ])
+    ;
 };

config/services/actions.php

@@ -9,6 +9,8 @@
 use CommerceWeavers\SyliusSaferpayPlugin\Controller\Action\WebhookAction;
 use CommerceWeavers\SyliusSaferpayPlugin\Payum\Factory\AssertFactoryInterface;
 use CommerceWeavers\SyliusSaferpayPlugin\Payum\Provider\TokenProviderInterface;
+use CommerceWeavers\SyliusSaferpayPlugin\Processor\SaferpayPaymentProcessor;
+use CommerceWeavers\SyliusSaferpayPlugin\Processor\SaferpayPaymentProcessorInterface;
 use CommerceWeavers\SyliusSaferpayPlugin\Provider\PaymentProviderInterface;
 use Sylius\Component\Resource\Metadata\MetadataInterface;
 use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator;
@@ -27,6 +29,9 @@
                 ->args(['sylius.order']),
             service(PaymentProviderInterface::class),
             service(TokenProviderInterface::class),
+            service(SaferpayPaymentProcessorInterface::class),
+            service('router'),
+            service('monolog.logger.saferpay'),
         ])
         ->tag('controller.service_arguments')
     ;
@@ -52,6 +57,8 @@
                 ->args(['sylius.order']),
             service(PaymentProviderInterface::class),
             service(TokenProviderInterface::class),
+            service('monolog.logger.saferpay'),
+            service('router'),
         ])
         ->tag('controller.service_arguments')
     ;
@@ -73,6 +80,9 @@
         ->args([
             service('payum'),
             service('sylius.command_bus'),
+            service('monolog.logger.saferpay'),
+            service(PaymentProviderInterface::class),
+            service(SaferpayPaymentProcessorInterface::class),
         ])
         ->tag('controller.service_arguments')
     ;

config/services/command_handler.php

@@ -8,6 +8,8 @@
 use CommerceWeavers\SyliusSaferpayPlugin\Payum\Factory\AssertFactoryInterface;
 use CommerceWeavers\SyliusSaferpayPlugin\Payum\Factory\CaptureFactoryInterface;
 use CommerceWeavers\SyliusSaferpayPlugin\Payum\Factory\ResolveNextCommandFactoryInterface;
+use CommerceWeavers\SyliusSaferpayPlugin\Processor\SaferpayPaymentProcessor;
+use CommerceWeavers\SyliusSaferpayPlugin\Provider\PaymentProviderInterface;
 use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator;
 use function Symfony\Component\DependencyInjection\Loader\Configurator\service;
 

config/shop_routing.yml

@@ -23,7 +23,7 @@ commerce_weavers_sylius_saferpay_prepare_capture:
                 route: sylius_shop_order_after_pay
 
 commerce_weavers_sylius_saferpay_webhook:
-    path: /payment/saferpay/webhook/{payum_token}
+    path: /payment/saferpay/webhook/{payum_token}/{order_token}
     methods: [GET]
     defaults:
         _controller: CommerceWeavers\SyliusSaferpayPlugin\Controller\Action\WebhookAction

spec/Client/SaferpayClientBodyFactorySpec.php

@@ -57,13 +57,14 @@ function it_creates_body_for_authorize_request(
         $payment->getCurrencyCode()->willReturn('CHF');
         $order->getNumber()->willReturn('000000001');
         $order->getCustomer()->willReturn($customer);
+        $order->getTokenValue()->willReturn('TOKEN');
         $customer->getEmail()->willReturn('[email protected]');
 
         $token->getAfterUrl()->willReturn('https://example.com/after');
 
         $tokenProvider->provideForWebhook($payment, 'commerce_weavers_sylius_saferpay_webhook')->willReturn($webhookToken);
         $webhookToken->getHash()->willReturn('webhook_hash');
-        $webhookRouteGenerator->generate('webhook_hash')->willReturn('https://example.com/webhook');
+        $webhookRouteGenerator->generate('webhook_hash', 'TOKEN')->willReturn('https://example.com/webhook');
 
         $this->createForAuthorize($payment, $token)->shouldReturn([
             'RequestHeader' => [

spec/Controller/Action/PrepareAssertActionSpec.php

@@ -4,18 +4,24 @@
 
 namespace spec\CommerceWeavers\SyliusSaferpayPlugin\Controller\Action;
 
+use CommerceWeavers\SyliusSaferpayPlugin\Exception\PaymentAlreadyProcessedException;
 use CommerceWeavers\SyliusSaferpayPlugin\Payum\Provider\TokenProviderInterface;
+use CommerceWeavers\SyliusSaferpayPlugin\Processor\SaferpayPaymentProcessorInterface;
 use CommerceWeavers\SyliusSaferpayPlugin\Provider\PaymentProviderInterface;
 use Payum\Core\Security\TokenInterface;
 use PhpSpec\ObjectBehavior;
 use Prophecy\Argument;
+use Psr\Log\LoggerInterface;
 use Sylius\Bundle\ResourceBundle\Controller\RequestConfiguration;
 use Sylius\Bundle\ResourceBundle\Controller\RequestConfigurationFactoryInterface;
 use Sylius\Component\Core\Model\PaymentInterface;
 use Sylius\Component\Resource\Metadata\MetadataInterface;
 use Symfony\Component\HttpFoundation\RedirectResponse;
 use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpFoundation\Session\Flash\FlashBagInterface;
+use Symfony\Component\HttpFoundation\Session\Session;
 use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
+use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
 
 final class PrepareAssertActionSpec extends ObjectBehavior
 {
@@ -25,29 +31,67 @@ function let(
         PaymentProviderInterface $paymentProvider,
         TokenProviderInterface $tokenProvider,
         RequestConfiguration $requestConfiguration,
+        SaferpayPaymentProcessorInterface $saferpayPaymentProcessor,
+        UrlGeneratorInterface $router,
+        LoggerInterface $logger,
     ): void {
         $requestConfigurationFactory->create($orderMetadata, Argument::type(Request::class))->willReturn($requestConfiguration);
 
-        $this->beConstructedWith($requestConfigurationFactory, $orderMetadata, $paymentProvider, $tokenProvider);
+        $this->beConstructedWith(
+            $requestConfigurationFactory,
+            $orderMetadata,
+            $paymentProvider,
+            $tokenProvider,
+            $saferpayPaymentProcessor,
+            $router,
+            $logger,
+        );
     }
 
     function it_throws_an_exception_when_last_payment_for_given_order_token_value_does_not_exist(
         PaymentProviderInterface $paymentProvider,
+        PaymentInterface $payment,
         Request $request,
     ): void {
-        $paymentProvider->provideForAssert('TOKEN')->willThrow(NotFoundHttpException::class);
+        $paymentProvider->provideForOrder('TOKEN')->willReturn($payment);
+        $paymentProvider->provideForAssert('TOKEN')->willThrow(PaymentAlreadyProcessedException::class);
 
-        $this->shouldThrow(NotFoundHttpException::class)->during('__invoke', [$request, 'TOKEN']);
+        $this->shouldThrow(PaymentAlreadyProcessedException::class)->during('__invoke', [$request, 'TOKEN']);
+    }
+
+    function it_returns_to_thank_you_page_if_payment_is_already_processed(
+        PaymentProviderInterface $paymentProvider,
+        SaferpayPaymentProcessorInterface $saferpayPaymentProcessor,
+        UrlGeneratorInterface $router,
+        Request $request,
+        PaymentInterface $payment,
+        Session $session,
+        FlashBagInterface $flashBag
+    ): void {
+        $paymentProvider->provideForOrder('TOKEN')->willReturn($payment);
+        $saferpayPaymentProcessor->lock($payment)->willThrow(PaymentAlreadyProcessedException::class);
+
+        $request->getSession()->willReturn($session);
+        $session->getFlashBag()->willReturn($flashBag);
+        $flashBag->add('success', 'sylius.payment.completed')->shouldBeCalled();
+
+        $router->generate('sylius_shop_order_thank_you')->willReturn('https://thank-you.com');
+
+        $this($request, 'TOKEN')->shouldBeLike(new RedirectResponse('https://thank-you.com'));
     }
 
     function it_returns_redirect_response_to_target_url_from_token(
         PaymentProviderInterface $paymentProvider,
+        SaferpayPaymentProcessorInterface $saferpayPaymentProcessor,
         TokenProviderInterface $tokenProvider,
         RequestConfiguration $requestConfiguration,
         Request $request,
         PaymentInterface $payment,
         TokenInterface $token,
     ): void {
+        $paymentProvider->provideForOrder('TOKEN')->willReturn($payment);
+        $saferpayPaymentProcessor->lock($payment)->shouldBeCalled();
+
         $paymentProvider->provideForAssert('TOKEN')->willReturn($payment);
         $tokenProvider->provideForAssert($payment, $requestConfiguration)->willReturn($token);
         $token->getTargetUrl()->willReturn('/url');

spec/Controller/Action/PrepareCaptureActionSpec.php

@@ -4,18 +4,23 @@
 
 namespace spec\CommerceWeavers\SyliusSaferpayPlugin\Controller\Action;
 
+use CommerceWeavers\SyliusSaferpayPlugin\Exception\PaymentAlreadyProcessedException;
 use CommerceWeavers\SyliusSaferpayPlugin\Payum\Provider\TokenProviderInterface;
 use CommerceWeavers\SyliusSaferpayPlugin\Provider\PaymentProviderInterface;
 use Payum\Core\Security\TokenInterface;
 use PhpSpec\ObjectBehavior;
 use Prophecy\Argument;
+use Psr\Log\LoggerInterface;
 use Sylius\Bundle\ResourceBundle\Controller\RequestConfiguration;
 use Sylius\Bundle\ResourceBundle\Controller\RequestConfigurationFactoryInterface;
 use Sylius\Component\Core\Model\PaymentInterface;
 use Sylius\Component\Resource\Metadata\MetadataInterface;
 use Symfony\Component\HttpFoundation\RedirectResponse;
 use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpFoundation\Session\Flash\FlashBagInterface;
+use Symfony\Component\HttpFoundation\Session\Session;
 use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
+use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
 
 final class PrepareCaptureActionSpec extends ObjectBehavior
 {
@@ -25,10 +30,19 @@ function let(
         PaymentProviderInterface $paymentProvider,
         TokenProviderInterface $tokenProvider,
         RequestConfiguration $requestConfiguration,
+        LoggerInterface $logger,
+        UrlGeneratorInterface $router,
     ): void {
         $requestConfigurationFactory->create($orderMetadata, Argument::type(Request::class))->willReturn($requestConfiguration);
 
-        $this->beConstructedWith($requestConfigurationFactory, $orderMetadata, $paymentProvider, $tokenProvider);
+        $this->beConstructedWith(
+            $requestConfigurationFactory,
+            $orderMetadata,
+            $paymentProvider,
+            $tokenProvider,
+            $logger,
+            $router,
+        );
     }
 
     function it_throws_an_exception_when_last_payment_for_given_order_token_value_does_not_exist(
@@ -40,6 +54,25 @@ function it_throws_an_exception_when_last_payment_for_given_order_token_value_do
         $this->shouldThrow(NotFoundHttpException::class)->during('__invoke', [$request, 'TOKEN']);
     }
 
+    function it_returns_to_thank_you_page_if_payment_is_already_processed(
+        PaymentProviderInterface $paymentProvider,
+        UrlGeneratorInterface $router,
+        Request $request,
+        PaymentInterface $payment,
+        Session $session,
+        FlashBagInterface $flashBag
+    ): void {
+        $paymentProvider->provideForCapture('TOKEN')->willThrow(PaymentAlreadyProcessedException::class);
+
+        $request->getSession()->willReturn($session);
+        $session->getFlashBag()->willReturn($flashBag);
+        $flashBag->add('success', 'sylius.payment.completed')->shouldBeCalled();
+
+        $router->generate('sylius_shop_order_thank_you')->willReturn('https://thank-you.com');
+
+        $this($request, 'TOKEN')->shouldBeLike(new RedirectResponse('https://thank-you.com'));
+    }
+
     function it_returns_redirect_response_to_target_url_from_token(
         PaymentProviderInterface $paymentProvider,
         TokenProviderInterface $tokenProvider,

spec/Payum/Action/AuthorizeActionSpec.php

@@ -129,7 +129,11 @@ function it_marks_the_payment_as_failed_if_there_is_different_status_code_than_o
         $saferpayClient->authorize($payment, $token)->willReturn($errorResponse);
         $errorResponse->getStatusCode()->willReturn(402);
 
-        $payment->setDetails(['status' => StatusAction::STATUS_FAILED])->shouldBeCalled();
+        $payment->getDetails()->willReturn(['processing' => true]);
+        $payment
+            ->setDetails(['processing' => true, 'status' => StatusAction::STATUS_FAILED])
+            ->shouldBeCalled()
+        ;
 
         $this->execute($request->getWrappedObject());
     }

spec/Payum/Provider/TokenProviderSpec.php

@@ -11,6 +11,7 @@
 use Sylius\Bundle\PayumBundle\Model\GatewayConfigInterface;
 use Sylius\Bundle\ResourceBundle\Controller\Parameters;
 use Sylius\Bundle\ResourceBundle\Controller\RequestConfiguration;
+use Sylius\Component\Core\Model\OrderInterface;
 use Sylius\Component\Core\Model\PaymentInterface;
 use Sylius\Component\Core\Model\PaymentMethodInterface;
 
@@ -154,21 +155,28 @@ function it_provides_token_for_capture_with_route_as_array(
 
     function it_provides_token_for_webhook(
         Payum $payum,
-        RequestConfiguration $requestConfiguration,
-        Parameters $parameters,
         GenericTokenFactoryInterface $tokenFactory,
         PaymentInterface $payment,
         TokenInterface $token,
         PaymentMethodInterface $paymentMethod,
         GatewayConfigInterface $gatewayConfig,
+        OrderInterface $order,
     ): void {
         $payment->getMethod()->willReturn($paymentMethod);
         $paymentMethod->getGatewayConfig()->willReturn($gatewayConfig);
         $gatewayConfig->getGatewayName()->willReturn('saferpay');
 
+        $payment->getOrder()->willReturn($order);
+        $order->getTokenValue()->willReturn('TOKEN');
+
         $payum->getTokenFactory()->willReturn($tokenFactory);
         $tokenFactory
-            ->createToken('saferpay', $payment->getWrappedObject(), 'commerce_weavers_sylius_saferpay_webhook')
+            ->createToken(
+                'saferpay',
+                $payment->getWrappedObject(),
+                'commerce_weavers_sylius_saferpay_webhook',
+                ['order_token' => 'TOKEN'],
+            )
             ->willReturn($token)
         ;
         $this->provideForWebhook($payment, 'commerce_weavers_sylius_saferpay_webhook')->shouldReturn($token);

spec/Provider/PaymentProviderSpec.php

@@ -4,6 +4,7 @@
 
 namespace spec\CommerceWeavers\SyliusSaferpayPlugin\Provider;
 
+use CommerceWeavers\SyliusSaferpayPlugin\Exception\PaymentAlreadyProcessedException;
 use CommerceWeavers\SyliusSaferpayPlugin\Provider\OrderProviderInterface;
 use PhpSpec\ObjectBehavior;
 use Sylius\Component\Core\Model\OrderInterface;
@@ -27,7 +28,7 @@ function it_throws_an_exception_when_last_payment_with_new_state_does_not_exist_
         $order->getTokenValue()->willReturn('TOKEN');
 
         $this
-            ->shouldThrow(new NotFoundHttpException('Order with token "TOKEN" does not have an active payment.'))
+            ->shouldThrow(PaymentAlreadyProcessedException::class)
             ->during('provideForAssert', ['TOKEN'])
         ;
     }
@@ -41,7 +42,7 @@ function it_throws_an_exception_when_last_payment_with_new_state_does_not_exist_
         $order->getTokenValue()->willReturn('TOKEN');
 
         $this
-            ->shouldThrow(new NotFoundHttpException('Order with token "TOKEN" does not have an active payment.'))
+            ->shouldThrow(PaymentAlreadyProcessedException::class)
             ->during('provideForCapture', ['TOKEN'])
         ;
     }

spec/Routing/Generator/WebhookUrlGeneratorSpec.php

@@ -18,10 +18,10 @@ function it_generates_webhook_url(RouterInterface $router): void
     {
         $router->generate(
             'commerce_weavers_sylius_saferpay_webhook',
-            ['payum_token' => 'abc123'],
+            ['payum_token' => 'abc123', 'order_token' => 'TOKEN'],
             0,
-        )->shouldBeCalled()->willReturn('/saferpay/webhook/abc123');
+        )->shouldBeCalled()->willReturn('/saferpay/webhook/abc123/TOKEN');
 
-        $this->generate('abc123')->shouldReturn('/saferpay/webhook/abc123');
+        $this->generate('abc123', 'TOKEN')->shouldReturn('/saferpay/webhook/abc123/TOKEN');
     }
 }