From 007a89114b9d67b1ecfe1646f85447c1ccb28506 Mon Sep 17 00:00:00 2001 From: Lukasz Bajsarowicz Date: Mon, 9 Mar 2020 17:34:38 +0100 Subject: [PATCH 01/12] DEPRECATED: AbstractAccount for Magento_Customer controllers --- .../Customer/Controller/AbstractAccount.php | 4 ++-- .../Customer/Controller/Plugin/Account.php | 23 ++++++++++++------- app/code/Magento/Customer/etc/frontend/di.xml | 2 +- 3 files changed, 18 insertions(+), 11 deletions(-) diff --git a/app/code/Magento/Customer/Controller/AbstractAccount.php b/app/code/Magento/Customer/Controller/AbstractAccount.php index 21611329ed9bc..aa0eca1423c17 100644 --- a/app/code/Magento/Customer/Controller/AbstractAccount.php +++ b/app/code/Magento/Customer/Controller/AbstractAccount.php @@ -9,9 +9,9 @@ use Magento\Framework\App\Action\Action; /** - * Class AbstractAccount - * @package Magento\Customer\Controller * @SuppressWarnings(PHPMD.NumberOfChildren) + * @deprecated 2.4.0 + * @see \Magento\Customer\Controller\AccountInterface */ abstract class AbstractAccount extends Action implements AccountInterface { diff --git a/app/code/Magento/Customer/Controller/Plugin/Account.php b/app/code/Magento/Customer/Controller/Plugin/Account.php index 179da148e7f78..5fc65eb845563 100644 --- a/app/code/Magento/Customer/Controller/Plugin/Account.php +++ b/app/code/Magento/Customer/Controller/Plugin/Account.php @@ -3,13 +3,15 @@ * Copyright © Magento, Inc. All rights reserved. * See COPYING.txt for license details. */ +declare(strict_types=1); + namespace Magento\Customer\Controller\Plugin; +use Magento\Customer\Controller\AccountInterface; use Magento\Customer\Model\Session; use Magento\Framework\App\ActionInterface; use Magento\Framework\App\RequestInterface; use Magento\Framework\App\ResponseInterface; -use Magento\Framework\App\Action\AbstractAction; use Magento\Framework\Controller\ResultInterface; class Account @@ -23,29 +25,35 @@ class Account * @var array */ private $allowedActions = []; + /** + * @var RequestInterface + */ + private $request; /** + * @param RequestInterface $request * @param Session $customerSession * @param array $allowedActions List of actions that are allowed for not authorized users */ public function __construct( + RequestInterface $request, Session $customerSession, array $allowedActions = [] ) { $this->session = $customerSession; $this->allowedActions = $allowedActions; + $this->request = $request; } /** * Dispatch actions allowed for not authorized users * - * @param AbstractAction $subject - * @param RequestInterface $request + * @param AccountInterface $subject * @return void */ - public function beforeDispatch(AbstractAction $subject, RequestInterface $request) + public function beforeExecute(AccountInterface $subject) { - $action = strtolower($request->getActionName()); + $action = strtolower($this->request->getActionName()); $pattern = '/^(' . implode('|', $this->allowedActions) . ')$/i'; if (!preg_match($pattern, $action)) { @@ -60,13 +68,12 @@ public function beforeDispatch(AbstractAction $subject, RequestInterface $reques /** * Remove No-referer flag from customer session * - * @param AbstractAction $subject + * @param AccountInterface $subject * @param ResponseInterface|ResultInterface $result - * @param RequestInterface $request * @return ResponseInterface|ResultInterface * @SuppressWarnings(PHPMD.UnusedFormalParameter) */ - public function afterDispatch(AbstractAction $subject, $result, RequestInterface $request) + public function afterExecute(AccountInterface $subject, $result) { $this->session->unsNoReferer(false); return $result; diff --git a/app/code/Magento/Customer/etc/frontend/di.xml b/app/code/Magento/Customer/etc/frontend/di.xml index a479d0d2af328..8867042cc6dc9 100644 --- a/app/code/Magento/Customer/etc/frontend/di.xml +++ b/app/code/Magento/Customer/etc/frontend/di.xml @@ -51,7 +51,7 @@ - + From 26aa6bdeba46c180e05941f2592a457537ef83be Mon Sep 17 00:00:00 2001 From: Lukasz Bajsarowicz Date: Mon, 9 Mar 2020 18:10:20 +0100 Subject: [PATCH 02/12] Missing PHPDoc to modified files --- .../Magento/Customer/Controller/AbstractAccount.php | 2 ++ .../Magento/Customer/Controller/Plugin/Account.php | 12 ++++++++---- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/app/code/Magento/Customer/Controller/AbstractAccount.php b/app/code/Magento/Customer/Controller/AbstractAccount.php index aa0eca1423c17..36a96521cc6d1 100644 --- a/app/code/Magento/Customer/Controller/AbstractAccount.php +++ b/app/code/Magento/Customer/Controller/AbstractAccount.php @@ -9,6 +9,8 @@ use Magento\Framework\App\Action\Action; /** + * AbstractAccount class is deprecated, in favour of Composition approach to build Controllers + * * @SuppressWarnings(PHPMD.NumberOfChildren) * @deprecated 2.4.0 * @see \Magento\Customer\Controller\AccountInterface diff --git a/app/code/Magento/Customer/Controller/Plugin/Account.php b/app/code/Magento/Customer/Controller/Plugin/Account.php index 5fc65eb845563..b7352873269e9 100644 --- a/app/code/Magento/Customer/Controller/Plugin/Account.php +++ b/app/code/Magento/Customer/Controller/Plugin/Account.php @@ -14,6 +14,9 @@ use Magento\Framework\App\ResponseInterface; use Magento\Framework\Controller\ResultInterface; +/** + * Plugin verifies permissions using Action Name against injected (`fontend/di.xml`) rules + */ class Account { /** @@ -21,15 +24,16 @@ class Account */ protected $session; - /** - * @var array - */ - private $allowedActions = []; /** * @var RequestInterface */ private $request; + /** + * @var array + */ + private $allowedActions = []; + /** * @param RequestInterface $request * @param Session $customerSession From 54ae63a63dd0fd7101299cf4b4390c98a33a3f29 Mon Sep 17 00:00:00 2001 From: Lukasz Bajsarowicz Date: Mon, 9 Mar 2020 18:19:49 +0100 Subject: [PATCH 03/12] Adjust Unit Tests for the change --- .../Unit/Controller/Plugin/AccountTest.php | 90 +++++++++---------- 1 file changed, 41 insertions(+), 49 deletions(-) diff --git a/app/code/Magento/Customer/Test/Unit/Controller/Plugin/AccountTest.php b/app/code/Magento/Customer/Test/Unit/Controller/Plugin/AccountTest.php index 2c70a8bda28fe..7dd376d57bdb0 100644 --- a/app/code/Magento/Customer/Test/Unit/Controller/Plugin/AccountTest.php +++ b/app/code/Magento/Customer/Test/Unit/Controller/Plugin/AccountTest.php @@ -3,18 +3,22 @@ * Copyright © Magento, Inc. All rights reserved. * See COPYING.txt for license details. */ + namespace Magento\Customer\Test\Unit\Controller\Plugin; +use Magento\Customer\Controller\AccountInterface; use Magento\Customer\Controller\Plugin\Account; use Magento\Customer\Model\Session; use Magento\Framework\App\ActionFlag; use Magento\Framework\App\ActionInterface; -use Magento\Framework\App\Action\AbstractAction; use Magento\Framework\App\Request\Http; +use Magento\Framework\App\Request\Http as HttpRequest; use Magento\Framework\Controller\ResultInterface; use Magento\Framework\TestFramework\Unit\Helper\ObjectManager; +use PHPUnit\Framework\MockObject\MockObject; +use PHPUnit\Framework\TestCase; -class AccountTest extends \PHPUnit\Framework\TestCase +class AccountTest extends TestCase { /** * @var string @@ -27,59 +31,51 @@ class AccountTest extends \PHPUnit\Framework\TestCase protected $plugin; /** - * @var Session | \PHPUnit_Framework_MockObject_MockObject + * @var Session|MockObject */ - protected $session; + protected $sessionMock; /** - * @var AbstractAction | \PHPUnit_Framework_MockObject_MockObject + * @var AccountInterface|MockObject */ - protected $subject; + protected $actionMock; /** - * @var Http | \PHPUnit_Framework_MockObject_MockObject + * @var Http|MockObject */ - protected $request; + protected $requestMock; /** - * @var ActionFlag | \PHPUnit_Framework_MockObject_MockObject + * @var ActionFlag|MockObject */ - protected $actionFlag; + protected $actionFlagMock; /** - * @var ResultInterface|\PHPUnit_Framework_MockObject_MockObject + * @var ResultInterface|MockObject */ - private $resultInterface; + private $resultMock; protected function setUp() { - $this->session = $this->getMockBuilder(\Magento\Customer\Model\Session::class) + $this->sessionMock = $this->getMockBuilder(Session::class) ->disableOriginalConstructor() - ->setMethods([ - 'setNoReferer', - 'unsNoReferer', - 'authenticate', - ]) + ->setMethods(['setNoReferer', 'unsNoReferer', 'authenticate']) ->getMock(); - $this->subject = $this->getMockBuilder(AbstractAction::class) - ->setMethods([ - 'getActionFlag', - ]) + $this->actionMock = $this->getMockBuilder(AccountInterface::class) + ->setMethods(['getActionFlag']) ->disableOriginalConstructor() ->getMockForAbstractClass(); - $this->request = $this->getMockBuilder(\Magento\Framework\App\Request\Http::class) + $this->requestMock = $this->getMockBuilder(HttpRequest::class) ->disableOriginalConstructor() - ->setMethods([ - 'getActionName', - ]) + ->setMethods(['getActionName']) ->getMock(); - $this->resultInterface = $this->getMockBuilder(ResultInterface::class) + $this->resultMock = $this->getMockBuilder(ResultInterface::class) ->getMockForAbstractClass(); - $this->actionFlag = $this->getMockBuilder(\Magento\Framework\App\ActionFlag::class) + $this->actionFlagMock = $this->getMockBuilder(ActionFlag::class) ->disableOriginalConstructor() ->getMock(); } @@ -90,47 +86,43 @@ protected function setUp() * @param boolean $isActionAllowed * @param boolean $isAuthenticated * - * @dataProvider beforeDispatchDataProvider + * @dataProvider beforeExecuteDataProvider */ - public function testBeforeDispatch( - $action, - $allowedActions, - $isActionAllowed, - $isAuthenticated - ) { - $this->request->expects($this->once()) + public function testBeforeExecute($action, $allowedActions, $isActionAllowed, $isAuthenticated) + { + $this->requestMock->expects($this->once()) ->method('getActionName') ->willReturn($action); if ($isActionAllowed) { - $this->session->expects($this->once()) + $this->sessionMock->expects($this->once()) ->method('setNoReferer') ->with(true) ->willReturnSelf(); } else { - $this->session->expects($this->once()) + $this->sessionMock->expects($this->once()) ->method('authenticate') ->willReturn($isAuthenticated); if (!$isAuthenticated) { - $this->subject->expects($this->once()) + $this->actionMock->expects($this->once()) ->method('getActionFlag') - ->willReturn($this->actionFlag); + ->willReturn($this->actionFlagMock); - $this->actionFlag->expects($this->once()) + $this->actionFlagMock->expects($this->once()) ->method('set') ->with('', ActionInterface::FLAG_NO_DISPATCH, true) ->willReturnSelf(); } } - $plugin = new Account($this->session, $allowedActions); - $plugin->beforeDispatch($this->subject, $this->request); + $plugin = new Account($this->requestMock, $this->sessionMock, $allowedActions); + $plugin->beforeExecute($this->actionMock); } /** * @return array */ - public function beforeDispatchDataProvider() + public function beforeExecuteDataProvider() { return [ [ @@ -166,9 +158,9 @@ public function beforeDispatchDataProvider() ]; } - public function testAfterDispatch() + public function testAfterExecute() { - $this->session->expects($this->once()) + $this->sessionMock->expects($this->once()) ->method('unsNoReferer') ->with(false) ->willReturnSelf(); @@ -176,13 +168,13 @@ public function testAfterDispatch() $plugin = (new ObjectManager($this))->getObject( Account::class, [ - 'session' => $this->session, + 'session' => $this->sessionMock, 'allowedActions' => ['testaction'] ] ); $this->assertSame( - $this->resultInterface, - $plugin->afterDispatch($this->subject, $this->resultInterface, $this->request) + $this->resultMock, + $plugin->afterExecute($this->actionMock, $this->resultMock, $this->requestMock) ); } } From 8bbebf38a52474069a672b93bd9d5ecca6cf1852 Mon Sep 17 00:00:00 2001 From: Lukasz Bajsarowicz Date: Wed, 11 Mar 2020 20:04:12 +0100 Subject: [PATCH 04/12] Avoid executing original `execute` method when no permission to do so --- .../Customer/Controller/Plugin/Account.php | 50 +++++++++++-------- 1 file changed, 30 insertions(+), 20 deletions(-) diff --git a/app/code/Magento/Customer/Controller/Plugin/Account.php b/app/code/Magento/Customer/Controller/Plugin/Account.php index b7352873269e9..6974ce92daed1 100644 --- a/app/code/Magento/Customer/Controller/Plugin/Account.php +++ b/app/code/Magento/Customer/Controller/Plugin/Account.php @@ -7,8 +7,10 @@ namespace Magento\Customer\Controller\Plugin; +use Closure; use Magento\Customer\Controller\AccountInterface; use Magento\Customer\Model\Session; +use Magento\Framework\App\ActionFlag; use Magento\Framework\App\ActionInterface; use Magento\Framework\App\RequestInterface; use Magento\Framework\App\ResponseInterface; @@ -33,53 +35,61 @@ class Account * @var array */ private $allowedActions = []; + /** + * @var ActionFlag + */ + private $actionFlag; /** * @param RequestInterface $request * @param Session $customerSession + * @param ActionFlag $actionFlag * @param array $allowedActions List of actions that are allowed for not authorized users */ public function __construct( RequestInterface $request, Session $customerSession, + ActionFlag $actionFlag, array $allowedActions = [] ) { $this->session = $customerSession; $this->allowedActions = $allowedActions; $this->request = $request; + $this->actionFlag = $actionFlag; } /** - * Dispatch actions allowed for not authorized users + * Executes original method if allowed, otherwise - redirects to log in * - * @param AccountInterface $subject - * @return void + * @param AccountInterface $controllerAction + * @param Closure $proceed + * @return ResultInterface|ResponseInterface|void */ - public function beforeExecute(AccountInterface $subject) + public function aroundExecute(AccountInterface $controllerAction, Closure $proceed) { - $action = strtolower($this->request->getActionName()); - $pattern = '/^(' . implode('|', $this->allowedActions) . ')$/i'; - - if (!preg_match($pattern, $action)) { - if (!$this->session->authenticate()) { - $subject->getActionFlag()->set('', ActionInterface::FLAG_NO_DISPATCH, true); - } - } else { + if ($this->isActionAllowed()) { $this->session->setNoReferer(true); + $response = $proceed(); + $this->session->unsNoReferer(false); + + return $response; + } + + if (!$this->session->authenticate()) { + $this->actionFlag->set('', ActionInterface::FLAG_NO_DISPATCH, true); } } /** - * Remove No-referer flag from customer session + * Validates whether currently requested action is one of the allowed * - * @param AccountInterface $subject - * @param ResponseInterface|ResultInterface $result - * @return ResponseInterface|ResultInterface - * @SuppressWarnings(PHPMD.UnusedFormalParameter) + * @return bool */ - public function afterExecute(AccountInterface $subject, $result) + private function isActionAllowed(): bool { - $this->session->unsNoReferer(false); - return $result; + $action = strtolower($this->request->getActionName()); + $pattern = '/^(' . implode('|', $this->allowedActions) . ')$/i'; + + return (bool)preg_match($pattern, $action); } } From 501e370f7dcfd98bfd068b6af6fa3344676e27d3 Mon Sep 17 00:00:00 2001 From: Lukasz Bajsarowicz Date: Wed, 11 Mar 2020 21:27:30 +0100 Subject: [PATCH 05/12] Fix implementation and Unit Tests --- .../Customer/Controller/Plugin/Account.php | 20 ++---- .../Unit/Controller/Plugin/AccountTest.php | 61 ++++++------------- 2 files changed, 24 insertions(+), 57 deletions(-) diff --git a/app/code/Magento/Customer/Controller/Plugin/Account.php b/app/code/Magento/Customer/Controller/Plugin/Account.php index 6974ce92daed1..7d0b954523fa6 100644 --- a/app/code/Magento/Customer/Controller/Plugin/Account.php +++ b/app/code/Magento/Customer/Controller/Plugin/Account.php @@ -35,27 +35,20 @@ class Account * @var array */ private $allowedActions = []; - /** - * @var ActionFlag - */ - private $actionFlag; /** * @param RequestInterface $request * @param Session $customerSession - * @param ActionFlag $actionFlag * @param array $allowedActions List of actions that are allowed for not authorized users */ public function __construct( RequestInterface $request, Session $customerSession, - ActionFlag $actionFlag, array $allowedActions = [] ) { + $this->request = $request; $this->session = $customerSession; $this->allowedActions = $allowedActions; - $this->request = $request; - $this->actionFlag = $actionFlag; } /** @@ -68,16 +61,11 @@ public function __construct( public function aroundExecute(AccountInterface $controllerAction, Closure $proceed) { if ($this->isActionAllowed()) { - $this->session->setNoReferer(true); - $response = $proceed(); - $this->session->unsNoReferer(false); - - return $response; + return $proceed(); } - if (!$this->session->authenticate()) { - $this->actionFlag->set('', ActionInterface::FLAG_NO_DISPATCH, true); - } + /** @FIXME Move Authentication and redirect out of Session model */ + $this->session->authenticate(); } /** diff --git a/app/code/Magento/Customer/Test/Unit/Controller/Plugin/AccountTest.php b/app/code/Magento/Customer/Test/Unit/Controller/Plugin/AccountTest.php index 7dd376d57bdb0..8986675d963a4 100644 --- a/app/code/Magento/Customer/Test/Unit/Controller/Plugin/AccountTest.php +++ b/app/code/Magento/Customer/Test/Unit/Controller/Plugin/AccountTest.php @@ -6,6 +6,7 @@ namespace Magento\Customer\Test\Unit\Controller\Plugin; +use Closure; use Magento\Customer\Controller\AccountInterface; use Magento\Customer\Controller\Plugin\Account; use Magento\Customer\Model\Session; @@ -83,40 +84,38 @@ protected function setUp() /** * @param string $action * @param array $allowedActions - * @param boolean $isActionAllowed + * @param boolean $isAllowed * @param boolean $isAuthenticated * * @dataProvider beforeExecuteDataProvider */ - public function testBeforeExecute($action, $allowedActions, $isActionAllowed, $isAuthenticated) + public function testAroundExecuteInterruptsOriginalCallWhenNotAllowed(string $action, array $allowedActions, bool $isAllowed, bool $isAuthenticated) { + /** @var callable|MockObject $proceedMock */ + $proceedMock = $this->getMockBuilder(\stdClass::class) + ->setMethods(['__invoke']) + ->getMock(); + + $closureMock = Closure::fromCallable($proceedMock); + $this->requestMock->expects($this->once()) ->method('getActionName') ->willReturn($action); - if ($isActionAllowed) { - $this->sessionMock->expects($this->once()) - ->method('setNoReferer') - ->with(true) - ->willReturnSelf(); + if ($isAllowed) { + $proceedMock->expects($this->once())->method('__invoke')->willReturn($this->resultMock); } else { - $this->sessionMock->expects($this->once()) - ->method('authenticate') - ->willReturn($isAuthenticated); - if (!$isAuthenticated) { - $this->actionMock->expects($this->once()) - ->method('getActionFlag') - ->willReturn($this->actionFlagMock); - - $this->actionFlagMock->expects($this->once()) - ->method('set') - ->with('', ActionInterface::FLAG_NO_DISPATCH, true) - ->willReturnSelf(); - } + $proceedMock->expects($this->never())->method('__invoke'); } $plugin = new Account($this->requestMock, $this->sessionMock, $allowedActions); - $plugin->beforeExecute($this->actionMock); + $result = $plugin->aroundExecute($this->actionMock, $closureMock); + + if ($isAllowed) { + $this->assertSame($this->resultMock, $result); + } else { + $this->assertNull($result); + } } /** @@ -157,24 +156,4 @@ public function beforeExecuteDataProvider() ], ]; } - - public function testAfterExecute() - { - $this->sessionMock->expects($this->once()) - ->method('unsNoReferer') - ->with(false) - ->willReturnSelf(); - - $plugin = (new ObjectManager($this))->getObject( - Account::class, - [ - 'session' => $this->sessionMock, - 'allowedActions' => ['testaction'] - ] - ); - $this->assertSame( - $this->resultMock, - $plugin->afterExecute($this->actionMock, $this->resultMock, $this->requestMock) - ); - } } From 79a3a1a1ad97dcd04a96a37625541b19881d1583 Mon Sep 17 00:00:00 2001 From: Lukasz Bajsarowicz Date: Wed, 11 Mar 2020 21:33:51 +0100 Subject: [PATCH 06/12] Fix logical issue --- app/code/Magento/Customer/Controller/Plugin/Account.php | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/app/code/Magento/Customer/Controller/Plugin/Account.php b/app/code/Magento/Customer/Controller/Plugin/Account.php index 7d0b954523fa6..ee927525f2315 100644 --- a/app/code/Magento/Customer/Controller/Plugin/Account.php +++ b/app/code/Magento/Customer/Controller/Plugin/Account.php @@ -60,12 +60,10 @@ public function __construct( */ public function aroundExecute(AccountInterface $controllerAction, Closure $proceed) { - if ($this->isActionAllowed()) { + /** @FIXME Move Authentication and redirect out of Session model */ + if ($this->isActionAllowed() || $this->session->authenticate()) { return $proceed(); } - - /** @FIXME Move Authentication and redirect out of Session model */ - $this->session->authenticate(); } /** From 302392701655d6458d8c976b8491a96a16eb97bb Mon Sep 17 00:00:00 2001 From: Lukasz Bajsarowicz Date: Wed, 11 Mar 2020 21:50:47 +0100 Subject: [PATCH 07/12] Code Review changes --- app/code/Magento/Customer/Controller/AbstractAccount.php | 2 +- app/code/Magento/Customer/Controller/Plugin/Account.php | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/app/code/Magento/Customer/Controller/AbstractAccount.php b/app/code/Magento/Customer/Controller/AbstractAccount.php index 36a96521cc6d1..4f2c80711d292 100644 --- a/app/code/Magento/Customer/Controller/AbstractAccount.php +++ b/app/code/Magento/Customer/Controller/AbstractAccount.php @@ -12,7 +12,7 @@ * AbstractAccount class is deprecated, in favour of Composition approach to build Controllers * * @SuppressWarnings(PHPMD.NumberOfChildren) - * @deprecated 2.4.0 + * @deprecated * @see \Magento\Customer\Controller\AccountInterface */ abstract class AbstractAccount extends Action implements AccountInterface diff --git a/app/code/Magento/Customer/Controller/Plugin/Account.php b/app/code/Magento/Customer/Controller/Plugin/Account.php index ee927525f2315..f02b333b70f7a 100644 --- a/app/code/Magento/Customer/Controller/Plugin/Account.php +++ b/app/code/Magento/Customer/Controller/Plugin/Account.php @@ -10,8 +10,6 @@ use Closure; use Magento\Customer\Controller\AccountInterface; use Magento\Customer\Model\Session; -use Magento\Framework\App\ActionFlag; -use Magento\Framework\App\ActionInterface; use Magento\Framework\App\RequestInterface; use Magento\Framework\App\ResponseInterface; use Magento\Framework\Controller\ResultInterface; From b5fda4c69bf1268f35e2758355e764c11a02a807 Mon Sep 17 00:00:00 2001 From: Lukasz Bajsarowicz Date: Wed, 11 Mar 2020 21:56:52 +0100 Subject: [PATCH 08/12] Code style --- app/code/Magento/Customer/Controller/Plugin/Account.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/code/Magento/Customer/Controller/Plugin/Account.php b/app/code/Magento/Customer/Controller/Plugin/Account.php index f02b333b70f7a..bbdb58d626108 100644 --- a/app/code/Magento/Customer/Controller/Plugin/Account.php +++ b/app/code/Magento/Customer/Controller/Plugin/Account.php @@ -22,7 +22,7 @@ class Account /** * @var Session */ - protected $session; + private $session; /** * @var RequestInterface @@ -55,6 +55,7 @@ public function __construct( * @param AccountInterface $controllerAction * @param Closure $proceed * @return ResultInterface|ResponseInterface|void + * @SuppressWarnings(PHPMD.UnusedFormalParameter) */ public function aroundExecute(AccountInterface $controllerAction, Closure $proceed) { From 1917781506ff523a94514fa63a0a04637ef51f05 Mon Sep 17 00:00:00 2001 From: Lukasz Bajsarowicz Date: Wed, 11 Mar 2020 22:55:29 +0100 Subject: [PATCH 09/12] Fix code style --- .../Customer/Test/Unit/Controller/Plugin/AccountTest.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/app/code/Magento/Customer/Test/Unit/Controller/Plugin/AccountTest.php b/app/code/Magento/Customer/Test/Unit/Controller/Plugin/AccountTest.php index 8986675d963a4..01ff1ced05ff9 100644 --- a/app/code/Magento/Customer/Test/Unit/Controller/Plugin/AccountTest.php +++ b/app/code/Magento/Customer/Test/Unit/Controller/Plugin/AccountTest.php @@ -85,12 +85,14 @@ protected function setUp() * @param string $action * @param array $allowedActions * @param boolean $isAllowed - * @param boolean $isAuthenticated * * @dataProvider beforeExecuteDataProvider */ - public function testAroundExecuteInterruptsOriginalCallWhenNotAllowed(string $action, array $allowedActions, bool $isAllowed, bool $isAuthenticated) - { + public function testAroundExecuteInterruptsOriginalCallWhenNotAllowed( + string $action, + array $allowedActions, + bool $isAllowed + ) { /** @var callable|MockObject $proceedMock */ $proceedMock = $this->getMockBuilder(\stdClass::class) ->setMethods(['__invoke']) From 1b014f8272390816c9a1c7edc038c80c11bbe5f7 Mon Sep 17 00:00:00 2001 From: Lukasz Bajsarowicz Date: Mon, 16 Mar 2020 19:24:11 +0100 Subject: [PATCH 10/12] Integration Tests for Authentication feature on Customer Account --- .../Controller/AuthenticationTest.php | 70 +++++++++++++++++++ 1 file changed, 70 insertions(+) create mode 100644 dev/tests/integration/testsuite/Magento/Customer/Controller/AuthenticationTest.php diff --git a/dev/tests/integration/testsuite/Magento/Customer/Controller/AuthenticationTest.php b/dev/tests/integration/testsuite/Magento/Customer/Controller/AuthenticationTest.php new file mode 100644 index 0000000000000..3ccd415a16c4f --- /dev/null +++ b/dev/tests/integration/testsuite/Magento/Customer/Controller/AuthenticationTest.php @@ -0,0 +1,70 @@ +resetAllowedActions(); + parent::tearDown(); + } + + /** + * After changes to `di.xml` and overriding list of allowed actions, unallowed ones should cause redirect. + */ + public function testExpectRedirectResponseWhenDispatchNotAllowedAction() + { + $this->overrideAllowedActions(['notExistingRoute']); + + $this->dispatch('customer/account/create'); + $this->assertRedirect($this->stringContains('customer/account/login')); + } + + /** + * Allowed actions should be displayed + */ + public function testExpectPageDispatchWhenAllowedAction() + { + $this->overrideAllowedActions(['create']); + + $this->dispatch('customer/account/create'); + $this->assertEquals(200, $this->getResponse()->getStatusCode()); + } + + /** + * Overrides list of `allowedActions` for Authorization Plugin + * + * @param string[] $allowedActions + * @see \Magento\Customer\Controller\Plugin\Account + */ + private function overrideAllowedActions(array $allowedActions): void + { + $allowedActions = array_combine($allowedActions, $allowedActions); + $pluginMock = $this->_objectManager->create(AccountPlugin::class, ['allowedActions' => $allowedActions]); + $this->_objectManager->addSharedInstance($pluginMock, AccountPlugin::class); + } + + /** + * Removes all the customizations applied to `allowedActions` + * @see \Magento\Customer\Controller\Plugin\Account + */ + private function resetAllowedActions() + { + $this->_objectManager->removeSharedInstance(AccountPlugin::class); + } +} From 65ac7b743f8e02105bedecc5a2ff7048e53cca0f Mon Sep 17 00:00:00 2001 From: Lukasz Bajsarowicz Date: Mon, 16 Mar 2020 19:27:30 +0100 Subject: [PATCH 11/12] Invalid name (was `Mock` instead of `Fake`) --- .../Magento/Customer/Controller/AuthenticationTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dev/tests/integration/testsuite/Magento/Customer/Controller/AuthenticationTest.php b/dev/tests/integration/testsuite/Magento/Customer/Controller/AuthenticationTest.php index 3ccd415a16c4f..83f30e730e683 100644 --- a/dev/tests/integration/testsuite/Magento/Customer/Controller/AuthenticationTest.php +++ b/dev/tests/integration/testsuite/Magento/Customer/Controller/AuthenticationTest.php @@ -55,8 +55,8 @@ public function testExpectPageDispatchWhenAllowedAction() private function overrideAllowedActions(array $allowedActions): void { $allowedActions = array_combine($allowedActions, $allowedActions); - $pluginMock = $this->_objectManager->create(AccountPlugin::class, ['allowedActions' => $allowedActions]); - $this->_objectManager->addSharedInstance($pluginMock, AccountPlugin::class); + $pluginFake = $this->_objectManager->create(AccountPlugin::class, ['allowedActions' => $allowedActions]); + $this->_objectManager->addSharedInstance($pluginFake, AccountPlugin::class); } /** From fe21a4348816c95397887ea13690579f39b734c2 Mon Sep 17 00:00:00 2001 From: Lukasz Bajsarowicz Date: Mon, 16 Mar 2020 19:30:23 +0100 Subject: [PATCH 12/12] Rename Test to be more meaningful --- .../Magento/Customer/Controller/AuthenticationTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dev/tests/integration/testsuite/Magento/Customer/Controller/AuthenticationTest.php b/dev/tests/integration/testsuite/Magento/Customer/Controller/AuthenticationTest.php index 83f30e730e683..822b3ab8f35d1 100644 --- a/dev/tests/integration/testsuite/Magento/Customer/Controller/AuthenticationTest.php +++ b/dev/tests/integration/testsuite/Magento/Customer/Controller/AuthenticationTest.php @@ -36,9 +36,9 @@ public function testExpectRedirectResponseWhenDispatchNotAllowedAction() } /** - * Allowed actions should be displayed + * Allowed actions should be rendered normally */ - public function testExpectPageDispatchWhenAllowedAction() + public function testExpectPageResponseWhenAllowedAction() { $this->overrideAllowedActions(['create']);