Mark AbstractAccount as DEPRECATED for Magento_Customer controllers

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
 {

app/code/Magento/Customer/Controller/Plugin/Account.php

@@ -3,72 +3,78 @@
  * 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
      */
     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);
     }
 }

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,110 +32,98 @@ 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();
     }
 
     /**
      * @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)
-        );
-    }
 }

app/code/Magento/Customer/etc/frontend/di.xml

@@ -51,7 +51,7 @@
             </argument>
         </arguments>
     </type>
-    <type name="Magento\Customer\Controller\AbstractAccount">
+    <type name="Magento\Customer\Controller\AccountInterface">
         <plugin name="customer_account" type="Magento\Customer\Controller\Plugin\Account" />
     </type>
     <type name="Magento\Checkout\Block\Cart\Sidebar">