diff --git a/app/code/Magento/Customer/Controller/AbstractAccount.php b/app/code/Magento/Customer/Controller/AbstractAccount.php index 21611329ed9bc..4f2c80711d292 100644 --- a/app/code/Magento/Customer/Controller/AbstractAccount.php +++ b/app/code/Magento/Customer/Controller/AbstractAccount.php @@ -9,9 +9,11 @@ use Magento\Framework\App\Action\Action; /** - * Class AbstractAccount - * @package Magento\Customer\Controller + * AbstractAccount class is deprecated, in favour of Composition approach to build Controllers + * * @SuppressWarnings(PHPMD.NumberOfChildren) + * @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 179da148e7f78..bbdb58d626108 100644 --- a/app/code/Magento/Customer/Controller/Plugin/Account.php +++ b/app/code/Magento/Customer/Controller/Plugin/Account.php @@ -3,21 +3,31 @@ * Copyright © Magento, Inc. All rights reserved. * See COPYING.txt for license details. */ +declare(strict_types=1); + namespace Magento\Customer\Controller\Plugin; +use Closure; +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; +/** + * Plugin verifies permissions using Action Name against injected (`fontend/di.xml`) rules + */ class Account { /** * @var Session */ - protected $session; + private $session; + + /** + * @var RequestInterface + */ + private $request; /** * @var array @@ -25,50 +35,46 @@ class Account private $allowedActions = []; /** + * @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->request = $request; $this->session = $customerSession; $this->allowedActions = $allowedActions; } /** - * Dispatch actions allowed for not authorized users + * Executes original method if allowed, otherwise - redirects to log in * - * @param AbstractAction $subject - * @param RequestInterface $request - * @return void + * @param AccountInterface $controllerAction + * @param Closure $proceed + * @return ResultInterface|ResponseInterface|void + * @SuppressWarnings(PHPMD.UnusedFormalParameter) */ - public function beforeDispatch(AbstractAction $subject, RequestInterface $request) + public function aroundExecute(AccountInterface $controllerAction, Closure $proceed) { - $action = strtolower($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 { - $this->session->setNoReferer(true); + /** @FIXME Move Authentication and redirect out of Session model */ + if ($this->isActionAllowed() || $this->session->authenticate()) { + return $proceed(); } } /** - * Remove No-referer flag from customer session + * Validates whether currently requested action is one of the allowed * - * @param AbstractAction $subject - * @param ResponseInterface|ResultInterface $result - * @param RequestInterface $request - * @return ResponseInterface|ResultInterface - * @SuppressWarnings(PHPMD.UnusedFormalParameter) + * @return bool */ - public function afterDispatch(AbstractAction $subject, $result, RequestInterface $request) + 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); } } 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..01ff1ced05ff9 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,23 @@ * Copyright © Magento, Inc. All rights reserved. * See COPYING.txt for license details. */ + 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; 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 +32,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(); } @@ -87,50 +84,46 @@ protected function setUp() /** * @param string $action * @param array $allowedActions - * @param boolean $isActionAllowed - * @param boolean $isAuthenticated + * @param boolean $isAllowed * - * @dataProvider beforeDispatchDataProvider + * @dataProvider beforeExecuteDataProvider */ - public function testBeforeDispatch( - $action, - $allowedActions, - $isActionAllowed, - $isAuthenticated + public function testAroundExecuteInterruptsOriginalCallWhenNotAllowed( + string $action, + array $allowedActions, + bool $isAllowed ) { - $this->request->expects($this->once()) + /** @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->session->expects($this->once()) - ->method('setNoReferer') - ->with(true) - ->willReturnSelf(); + if ($isAllowed) { + $proceedMock->expects($this->once())->method('__invoke')->willReturn($this->resultMock); } else { - $this->session->expects($this->once()) - ->method('authenticate') - ->willReturn($isAuthenticated); - if (!$isAuthenticated) { - $this->subject->expects($this->once()) - ->method('getActionFlag') - ->willReturn($this->actionFlag); - - $this->actionFlag->expects($this->once()) - ->method('set') - ->with('', ActionInterface::FLAG_NO_DISPATCH, true) - ->willReturnSelf(); - } + $proceedMock->expects($this->never())->method('__invoke'); } - $plugin = new Account($this->session, $allowedActions); - $plugin->beforeDispatch($this->subject, $this->request); + $plugin = new Account($this->requestMock, $this->sessionMock, $allowedActions); + $result = $plugin->aroundExecute($this->actionMock, $closureMock); + + if ($isAllowed) { + $this->assertSame($this->resultMock, $result); + } else { + $this->assertNull($result); + } } /** * @return array */ - public function beforeDispatchDataProvider() + public function beforeExecuteDataProvider() { return [ [ @@ -165,24 +158,4 @@ public function beforeDispatchDataProvider() ], ]; } - - public function testAfterDispatch() - { - $this->session->expects($this->once()) - ->method('unsNoReferer') - ->with(false) - ->willReturnSelf(); - - $plugin = (new ObjectManager($this))->getObject( - Account::class, - [ - 'session' => $this->session, - 'allowedActions' => ['testaction'] - ] - ); - $this->assertSame( - $this->resultInterface, - $plugin->afterDispatch($this->subject, $this->resultInterface, $this->request) - ); - } } 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 @@ - + 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..822b3ab8f35d1 --- /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 rendered normally + */ + public function testExpectPageResponseWhenAllowedAction() + { + $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); + $pluginFake = $this->_objectManager->create(AccountPlugin::class, ['allowedActions' => $allowedActions]); + $this->_objectManager->addSharedInstance($pluginFake, AccountPlugin::class); + } + + /** + * Removes all the customizations applied to `allowedActions` + * @see \Magento\Customer\Controller\Plugin\Account + */ + private function resetAllowedActions() + { + $this->_objectManager->removeSharedInstance(AccountPlugin::class); + } +}