From 93bf14a92da10018a98418db1bb6cea598888fe3 Mon Sep 17 00:00:00 2001 From: Kamil Kokot Date: Mon, 9 Jul 2018 17:11:09 +0200 Subject: [PATCH 1/4] Make Travis passing again --- .../Resources/config/routing/order.yml | 1 + .../Controller/ResourceController.php | 2 +- .../Controller/ResourceControllerSpec.php | 54 ++++++++++++++++++- 3 files changed, 55 insertions(+), 2 deletions(-) diff --git a/src/Sylius/Bundle/AdminApiBundle/Resources/config/routing/order.yml b/src/Sylius/Bundle/AdminApiBundle/Resources/config/routing/order.yml index 1ad33a0f02e..969dc8e3cf2 100644 --- a/src/Sylius/Bundle/AdminApiBundle/Resources/config/routing/order.yml +++ b/src/Sylius/Bundle/AdminApiBundle/Resources/config/routing/order.yml @@ -20,6 +20,7 @@ sylius_admin_api_order_cancel: state_machine: graph: sylius_order transition: cancel + csrf_protection: false return_content: false sylius_admin_api_order_shipment_ship: diff --git a/src/Sylius/Bundle/ResourceBundle/Controller/ResourceController.php b/src/Sylius/Bundle/ResourceBundle/Controller/ResourceController.php index 8ca359dc6e3..7ae2d52600c 100644 --- a/src/Sylius/Bundle/ResourceBundle/Controller/ResourceController.php +++ b/src/Sylius/Bundle/ResourceBundle/Controller/ResourceController.php @@ -469,7 +469,7 @@ public function applyStateMachineTransitionAction(Request $request): Response $this->isGrantedOr403($configuration, ResourceActions::UPDATE); $resource = $this->findOr404($configuration); - if ($configuration->isCsrfProtectionEnabled() && !$this->isCsrfTokenValid($resource->getId(), $request->request->get('_csrf_token'))) { + if ($configuration->isCsrfProtectionEnabled() && !$this->isCsrfTokenValid($resource->getId(), $request->get('_csrf_token'))) { throw new HttpException(Response::HTTP_FORBIDDEN, 'Invalid CSRF token.'); } diff --git a/src/Sylius/Bundle/ResourceBundle/spec/Controller/ResourceControllerSpec.php b/src/Sylius/Bundle/ResourceBundle/spec/Controller/ResourceControllerSpec.php index 0f0e85f30d2..ce1ab562dc1 100644 --- a/src/Sylius/Bundle/ResourceBundle/spec/Controller/ResourceControllerSpec.php +++ b/src/Sylius/Bundle/ResourceBundle/spec/Controller/ResourceControllerSpec.php @@ -1822,6 +1822,8 @@ function it_does_not_apply_state_machine_transition_on_resource_if_not_applicabl ResourceInterface $resource, FlashHelperInterface $flashHelper, EventDispatcherInterface $eventDispatcher, + CsrfTokenManagerInterface $csrfTokenManager, + ContainerInterface $container, ResourceControllerEvent $event, Request $request ): void { @@ -1831,10 +1833,18 @@ function it_does_not_apply_state_machine_transition_on_resource_if_not_applicabl $requestConfigurationFactory->create($metadata, $request)->willReturn($configuration); $configuration->hasPermission()->willReturn(true); $configuration->getPermission(ResourceActions::UPDATE)->willReturn('sylius.product.update'); + $configuration->isCsrfProtectionEnabled()->willReturn(true); + $request->get('_csrf_token')->willReturn('xyz'); + + $container->has('security.csrf.token_manager')->willReturn(true); + $container->get('security.csrf.token_manager')->willReturn($csrfTokenManager); + $csrfTokenManager->isTokenValid(new CsrfToken('1', 'xyz'))->willReturn(true); $authorizationChecker->isGranted($configuration, 'sylius.product.update')->willReturn(true); $singleResourceProvider->get($configuration, $repository)->willReturn($resource); + $resource->getId()->willReturn('1'); + $configuration->isHtmlRequest()->willReturn(true); $eventDispatcher->dispatchPreEvent(ResourceActions::UPDATE, $configuration, $resource)->willReturn($event); @@ -1865,6 +1875,8 @@ function it_applies_state_machine_transition_to_resource_and_redirects_for_html_ FlashHelperInterface $flashHelper, AuthorizationCheckerInterface $authorizationChecker, EventDispatcherInterface $eventDispatcher, + CsrfTokenManagerInterface $csrfTokenManager, + ContainerInterface $container, StateMachineInterface $stateMachine, ResourceUpdateHandlerInterface $resourceUpdateHandler, RequestConfiguration $configuration, @@ -1880,10 +1892,18 @@ function it_applies_state_machine_transition_to_resource_and_redirects_for_html_ $requestConfigurationFactory->create($metadata, $request)->willReturn($configuration); $configuration->hasPermission()->willReturn(true); $configuration->getPermission(ResourceActions::UPDATE)->willReturn('sylius.product.update'); + $configuration->isCsrfProtectionEnabled()->willReturn(true); + $request->get('_csrf_token')->willReturn('xyz'); + + $container->has('security.csrf.token_manager')->willReturn(true); + $container->get('security.csrf.token_manager')->willReturn($csrfTokenManager); + $csrfTokenManager->isTokenValid(new CsrfToken('1', 'xyz'))->willReturn(true); $authorizationChecker->isGranted($configuration, 'sylius.product.update')->willReturn(true); $singleResourceProvider->get($configuration, $repository)->willReturn($resource); + $resource->getId()->willReturn('1'); + $configuration->isHtmlRequest()->willReturn(true); $eventDispatcher->dispatchPreEvent(ResourceActions::UPDATE, $configuration, $resource)->willReturn($event); @@ -1909,10 +1929,11 @@ function it_uses_response_from_post_apply_state_machine_transition_event_if_defi RepositoryInterface $repository, ObjectManager $manager, SingleResourceProviderInterface $singleResourceProvider, - RedirectHandlerInterface $redirectHandler, FlashHelperInterface $flashHelper, AuthorizationCheckerInterface $authorizationChecker, EventDispatcherInterface $eventDispatcher, + CsrfTokenManagerInterface $csrfTokenManager, + ContainerInterface $container, StateMachineInterface $stateMachine, ResourceUpdateHandlerInterface $resourceUpdateHandler, RequestConfiguration $configuration, @@ -1928,10 +1949,18 @@ function it_uses_response_from_post_apply_state_machine_transition_event_if_defi $requestConfigurationFactory->create($metadata, $request)->willReturn($configuration); $configuration->hasPermission()->willReturn(true); $configuration->getPermission(ResourceActions::UPDATE)->willReturn('sylius.product.update'); + $configuration->isCsrfProtectionEnabled()->willReturn(true); + $request->get('_csrf_token')->willReturn('xyz'); + + $container->has('security.csrf.token_manager')->willReturn(true); + $container->get('security.csrf.token_manager')->willReturn($csrfTokenManager); + $csrfTokenManager->isTokenValid(new CsrfToken('1', 'xyz'))->willReturn(true); $authorizationChecker->isGranted($configuration, 'sylius.product.update')->willReturn(true); $singleResourceProvider->get($configuration, $repository)->willReturn($resource); + $resource->getId()->willReturn('1'); + $configuration->isHtmlRequest()->willReturn(true); $eventDispatcher->dispatchPreEvent(ResourceActions::UPDATE, $configuration, $resource)->willReturn($event); @@ -1963,6 +1992,8 @@ function it_does_not_apply_state_machine_transition_on_resource_and_redirects_fo RedirectHandlerInterface $redirectHandler, FlashHelperInterface $flashHelper, EventDispatcherInterface $eventDispatcher, + CsrfTokenManagerInterface $csrfTokenManager, + ContainerInterface $container, ResourceControllerEvent $event, Request $request, Response $redirectResponse @@ -1973,10 +2004,18 @@ function it_does_not_apply_state_machine_transition_on_resource_and_redirects_fo $requestConfigurationFactory->create($metadata, $request)->willReturn($configuration); $configuration->hasPermission()->willReturn(true); $configuration->getPermission(ResourceActions::UPDATE)->willReturn('sylius.product.update'); + $configuration->isCsrfProtectionEnabled()->willReturn(true); + $request->get('_csrf_token')->willReturn('xyz'); + + $container->has('security.csrf.token_manager')->willReturn(true); + $container->get('security.csrf.token_manager')->willReturn($csrfTokenManager); + $csrfTokenManager->isTokenValid(new CsrfToken('1', 'xyz'))->willReturn(true); $authorizationChecker->isGranted($configuration, 'sylius.product.update')->willReturn(true); $singleResourceProvider->get($configuration, $repository)->willReturn($resource); + $resource->getId()->willReturn('1'); + $configuration->isHtmlRequest()->willReturn(true); $eventDispatcher->dispatchPreEvent(ResourceActions::UPDATE, $configuration, $resource)->willReturn($event); @@ -2008,6 +2047,8 @@ function it_does_not_apply_state_machine_transition_on_resource_and_return_event ResourceInterface $resource, FlashHelperInterface $flashHelper, EventDispatcherInterface $eventDispatcher, + CsrfTokenManagerInterface $csrfTokenManager, + ContainerInterface $container, ResourceControllerEvent $event, Request $request, Response $response @@ -2018,10 +2059,18 @@ function it_does_not_apply_state_machine_transition_on_resource_and_return_event $requestConfigurationFactory->create($metadata, $request)->willReturn($configuration); $configuration->hasPermission()->willReturn(true); $configuration->getPermission(ResourceActions::UPDATE)->willReturn('sylius.product.update'); + $configuration->isCsrfProtectionEnabled()->willReturn(true); + $request->get('_csrf_token')->willReturn('xyz'); + + $container->has('security.csrf.token_manager')->willReturn(true); + $container->get('security.csrf.token_manager')->willReturn($csrfTokenManager); + $csrfTokenManager->isTokenValid(new CsrfToken('1', 'xyz'))->willReturn(true); $authorizationChecker->isGranted($configuration, 'sylius.product.update')->willReturn(true); $singleResourceProvider->get($configuration, $repository)->willReturn($resource); + $resource->getId()->willReturn('1'); + $configuration->isHtmlRequest()->willReturn(true); $eventDispatcher->dispatchPreEvent(ResourceActions::UPDATE, $configuration, $resource)->willReturn($event); @@ -2066,6 +2115,7 @@ function it_applies_state_machine_transition_on_resource_and_returns_200_for_non $configuration->getParameters()->willReturn($parameterBag); $configuration->hasPermission()->willReturn(true); $configuration->getPermission(ResourceActions::UPDATE)->willReturn('sylius.product.update'); + $configuration->isCsrfProtectionEnabled()->willReturn(false); $parameterBag->get('return_content', true)->willReturn(true); @@ -2114,6 +2164,7 @@ function it_applies_state_machine_transition_on_resource_and_returns_204_for_non $configuration->getParameters()->willReturn($parameterBag); $configuration->hasPermission()->willReturn(true); $configuration->getPermission(ResourceActions::UPDATE)->willReturn('sylius.product.update'); + $configuration->isCsrfProtectionEnabled()->willReturn(false); $parameterBag->get('return_content', true)->willReturn(false); @@ -2158,6 +2209,7 @@ function it_does_not_apply_state_machine_transition_resource_and_throws_http_exc $requestConfigurationFactory->create($metadata, $request)->willReturn($configuration); $configuration->hasPermission()->willReturn(true); $configuration->getPermission(ResourceActions::UPDATE)->willReturn('sylius.product.update'); + $configuration->isCsrfProtectionEnabled()->willReturn(false); $authorizationChecker->isGranted($configuration, 'sylius.product.update')->willReturn(true); $singleResourceProvider->get($configuration, $repository)->willReturn($resource); From 85ebf7eeef509af645087515d3dc90440772bfa5 Mon Sep 17 00:00:00 2001 From: Kamil Kokot Date: Tue, 10 Jul 2018 11:27:23 +0200 Subject: [PATCH 2/4] Fix cancel order button in the admin panel --- .../AdminBundle/Menu/OrderShowMenuBuilder.php | 16 ++- .../Resources/config/services/menu.xml | 1 + .../spec/Menu/OrderShowMenuBuilderSpec.php | 128 ------------------ 3 files changed, 12 insertions(+), 133 deletions(-) delete mode 100644 src/Sylius/Bundle/AdminBundle/spec/Menu/OrderShowMenuBuilderSpec.php diff --git a/src/Sylius/Bundle/AdminBundle/Menu/OrderShowMenuBuilder.php b/src/Sylius/Bundle/AdminBundle/Menu/OrderShowMenuBuilder.php index f0eac1c6a27..9d7aa4a32a7 100644 --- a/src/Sylius/Bundle/AdminBundle/Menu/OrderShowMenuBuilder.php +++ b/src/Sylius/Bundle/AdminBundle/Menu/OrderShowMenuBuilder.php @@ -19,6 +19,7 @@ use Sylius\Bundle\AdminBundle\Event\OrderShowMenuBuilderEvent; use Sylius\Component\Order\OrderTransitions; use Symfony\Component\EventDispatcher\EventDispatcherInterface; +use Symfony\Component\Security\Csrf\CsrfTokenManagerInterface; final class OrderShowMenuBuilder { @@ -40,18 +41,20 @@ final class OrderShowMenuBuilder private $stateMachineFactory; /** - * @param FactoryInterface $factory - * @param EventDispatcherInterface $eventDispatcher - * @param StateMachineFactoryInterface $stateMachineFactory, + * @var CsrfTokenManagerInterface */ + private $csrfTokenManager; + public function __construct( FactoryInterface $factory, EventDispatcherInterface $eventDispatcher, - StateMachineFactoryInterface $stateMachineFactory + StateMachineFactoryInterface $stateMachineFactory, + CsrfTokenManagerInterface $csrfTokenManager ) { $this->factory = $factory; $this->eventDispatcher = $eventDispatcher; $this->stateMachineFactory = $stateMachineFactory; + $this->csrfTokenManager = $csrfTokenManager; } /** @@ -84,7 +87,10 @@ public function createMenu(array $options): ItemInterface $menu ->addChild('cancel', [ 'route' => 'sylius_admin_order_cancel', - 'routeParameters' => ['id' => $order->getId()], + 'routeParameters' => [ + 'id' => $order->getId(), + '_csrf_token' => $this->csrfTokenManager->getToken((string) $order->getId())->getValue(), + ], ]) ->setAttribute('type', 'transition') ->setLabel('sylius.ui.cancel') diff --git a/src/Sylius/Bundle/AdminBundle/Resources/config/services/menu.xml b/src/Sylius/Bundle/AdminBundle/Resources/config/services/menu.xml index 4dfa61de08e..b440ff879f4 100644 --- a/src/Sylius/Bundle/AdminBundle/Resources/config/services/menu.xml +++ b/src/Sylius/Bundle/AdminBundle/Resources/config/services/menu.xml @@ -29,6 +29,7 @@ + diff --git a/src/Sylius/Bundle/AdminBundle/spec/Menu/OrderShowMenuBuilderSpec.php b/src/Sylius/Bundle/AdminBundle/spec/Menu/OrderShowMenuBuilderSpec.php deleted file mode 100644 index dd5d9762eae..00000000000 --- a/src/Sylius/Bundle/AdminBundle/spec/Menu/OrderShowMenuBuilderSpec.php +++ /dev/null @@ -1,128 +0,0 @@ -beConstructedWith($factory, $eventDispatcher, $stateMachineFactory); - } - - function it_creates_an_order_show_menu( - FactoryInterface $factory, - EventDispatcherInterface $eventDispatcher, - StateMachineFactoryInterface $stateMachineFactory, - ItemInterface $menu, - StateMachineInterface $stateMachine, - OrderInterface $order - ): void { - $factory->createItem('root')->willReturn($menu); - - $order->getId()->willReturn(7); - - $menu - ->addChild('order_history', [ - 'route' => 'sylius_admin_order_history', - 'routeParameters' => ['id' => 7], - ]) - ->shouldBeCalled() - ->willReturn($menu) - ; - $menu->setAttribute('type', 'link')->shouldBeCalled()->willReturn($menu); - $menu->setLabel('sylius.ui.history')->shouldBeCalled()->willReturn($menu); - $menu->setLabelAttribute('icon', 'history')->shouldBeCalled()->willReturn($menu); - - $stateMachineFactory->get($order, OrderTransitions::GRAPH)->willReturn($stateMachine); - $stateMachine->can(OrderTransitions::TRANSITION_CANCEL)->willReturn(true); - - $menu - ->addChild('cancel', [ - 'route' => 'sylius_admin_order_cancel', - 'routeParameters' => ['id' => 7], - ]) - ->shouldBeCalled() - ->willReturn($menu) - ; - $menu->setAttribute('type', 'transition')->shouldBeCalled()->willReturn($menu); - $menu->setLabel('sylius.ui.cancel')->shouldBeCalled()->willReturn($menu); - $menu->setLabelAttribute('icon', 'ban')->shouldBeCalled()->willReturn($menu); - $menu->setLabelAttribute('color', 'yellow')->shouldBeCalled()->willReturn($menu); - - $eventDispatcher - ->dispatch('sylius.menu.admin.order.show', Argument::type(MenuBuilderEvent::class)) - ->shouldBeCalled() - ; - - $this->createMenu(['order' => $order])->shouldReturn($menu); - } - - function it_creates_an_order_show_menu_when_cancel_transition_is_impossible( - FactoryInterface $factory, - EventDispatcherInterface $eventDispatcher, - StateMachineFactoryInterface $stateMachineFactory, - ItemInterface $menu, - StateMachineInterface $stateMachine, - OrderInterface $order - ): void { - $factory->createItem('root')->willReturn($menu); - - $order->getId()->willReturn(7); - - $menu - ->addChild('order_history', [ - 'route' => 'sylius_admin_order_history', - 'routeParameters' => ['id' => 7], - ]) - ->shouldBeCalled() - ->willReturn($menu) - ; - $menu->setAttribute('type', 'link')->shouldBeCalled()->willReturn($menu); - $menu->setLabel('sylius.ui.history')->shouldBeCalled()->willReturn($menu); - $menu->setLabelAttribute('icon', 'history')->shouldBeCalled()->willReturn($menu); - - $stateMachineFactory->get($order, OrderTransitions::GRAPH)->willReturn($stateMachine); - $stateMachine->can(OrderTransitions::TRANSITION_CANCEL)->willReturn(false); - - $eventDispatcher - ->dispatch('sylius.menu.admin.order.show', Argument::type(MenuBuilderEvent::class)) - ->shouldBeCalled() - ; - - $this->createMenu(['order' => $order])->shouldReturn($menu); - } - - function it_returns_an_empty_order_show_menu_when_there_is_no_order_in_options( - FactoryInterface $factory, - ItemInterface $menu - ): void { - $factory->createItem('root')->willReturn($menu); - - $this->createMenu([])->shouldReturn($menu); - } -} From dff366eec7cad91cd053b5ed801d2947dd9afa38 Mon Sep 17 00:00:00 2001 From: K_A_S Date: Tue, 10 Jul 2018 12:59:49 +0300 Subject: [PATCH 3/4] Exception while apply refund transition Admin refund action throws exception "Argument 1 passed to Symfony\Bundle\FrameworkBundle\Controller\Controller::isCsrfTokenValid() must be of the type string, integer given" --- .../Bundle/ResourceBundle/Controller/ResourceController.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Sylius/Bundle/ResourceBundle/Controller/ResourceController.php b/src/Sylius/Bundle/ResourceBundle/Controller/ResourceController.php index 7ae2d52600c..8713dff7298 100644 --- a/src/Sylius/Bundle/ResourceBundle/Controller/ResourceController.php +++ b/src/Sylius/Bundle/ResourceBundle/Controller/ResourceController.php @@ -469,7 +469,7 @@ public function applyStateMachineTransitionAction(Request $request): Response $this->isGrantedOr403($configuration, ResourceActions::UPDATE); $resource = $this->findOr404($configuration); - if ($configuration->isCsrfProtectionEnabled() && !$this->isCsrfTokenValid($resource->getId(), $request->get('_csrf_token'))) { + if ($configuration->isCsrfProtectionEnabled() && !$this->isCsrfTokenValid((string) $resource->getId(), $request->get('_csrf_token'))) { throw new HttpException(Response::HTTP_FORBIDDEN, 'Invalid CSRF token.'); } From 1f6e410dcbb4f2c31024886ab91b7668fe411fd9 Mon Sep 17 00:00:00 2001 From: Kamil Kokot Date: Tue, 10 Jul 2018 13:01:53 +0200 Subject: [PATCH 4/4] Make sure Admin API does not require CSRF protection --- .../AdminApiBundle/Resources/config/routing/product_review.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Sylius/Bundle/AdminApiBundle/Resources/config/routing/product_review.yml b/src/Sylius/Bundle/AdminApiBundle/Resources/config/routing/product_review.yml index 64025592f37..0b999e16c0c 100644 --- a/src/Sylius/Bundle/AdminApiBundle/Resources/config/routing/product_review.yml +++ b/src/Sylius/Bundle/AdminApiBundle/Resources/config/routing/product_review.yml @@ -78,6 +78,7 @@ sylius_admin_api_product_review_accept: state_machine: graph: sylius_product_review transition: accept + csrf_protection: false sylius_admin_api_product_review_reject: path: /reviews/{id}/reject @@ -88,3 +89,4 @@ sylius_admin_api_product_review_reject: state_machine: graph: sylius_product_review transition: reject + csrf_protection: false