ACP2E-4114: Production ACL Permission Check caused Performance Degradation – populateAcl Method is the bottleneck

abukatar · abukatar · commit 05f06c11f0ad · 2025-08-29T20:46:13.000-07:00
diff --git a/app/code/Magento/Authorization/Test/Unit/Model/Acl/AclRetrieverTest.php b/app/code/Magento/Authorization/Test/Unit/Model/Acl/AclRetrieverTest.php
@@ -18,6 +18,7 @@
 use Magento\Authorization\Model\UserContextInterface;
 use Magento\Framework\Acl;
 use Magento\Framework\Acl\Builder;
+use Magento\Framework\Acl\Role\CurrentRoleContext;
 use Magento\Framework\Exception\AuthorizationException;
 use PHPUnit\Framework\MockObject\MockObject;
 use PHPUnit\Framework\TestCase;
@@ -172,14 +173,78 @@ protected function createAclRetriever()
         /**
          * @var Builder|MockObject $aclBuilderMock
          */
-        $aclBuilderMock = $this->createPartialMock(Builder::class, ['getAcl']);
+        $aclBuilderMock = $this->createPartialMock(Builder::class, ['getAcl', 'resetRuntimeAcl']);
         $aclBuilderMock->expects($this->any())->method('getAcl')->willReturn($aclMock);
 
         return new AclRetriever(
             $aclBuilderMock,
             $roleCollectionFactoryMock,
             $rulesCollectionFactoryMock,
-            $this->getMockForAbstractClass(LoggerInterface::class)
+            $this->getMockForAbstractClass(LoggerInterface::class),
+            new CurrentRoleContext()
         );
     }
+
+    public function testResetAclWhenRoleChanges(): void
+    {
+        // Set up rules collection
+        $rulesMock = $this->getMockBuilder(Rules::class)
+            ->addMethods(['getResourceId'])
+            ->onlyMethods(['__wakeup'])
+            ->disableOriginalConstructor()
+            ->getMock();
+        $rulesMock->method('getResourceId')->willReturn('Magento_Backend::dashboard');
+
+        $rulesCollectionMock = $this->createPartialMock(
+            RulesCollection::class,
+            ['getByRoles', 'load', 'getItems']
+        );
+        $rulesCollectionMock->method('getByRoles')->willReturnSelf();
+        $rulesCollectionMock->method('load')->willReturnSelf();
+        $rulesCollectionMock->method('getItems')->willReturn([$rulesMock]);
+
+        $rulesCollectionFactoryMock = $this->createPartialMock(
+            RulesCollectionFactory::class,
+            ['create']
+        );
+        $rulesCollectionFactoryMock->method('create')->willReturn($rulesCollectionMock);
+
+        // ACL and builder mocks
+        $aclMock = $this->createPartialMock(Acl::class, ['hasResource', 'isAllowed']);
+        $aclMock->method('hasResource')->willReturn(true);
+        $aclMock->method('isAllowed')->willReturn(true);
+
+        $aclBuilderMock = $this->createPartialMock(Builder::class, ['getAcl', 'resetRuntimeAcl']);
+        $aclBuilderMock->expects($this->any())->method('getAcl')->willReturn($aclMock);
+        $aclBuilderMock->expects($this->exactly(2))->method('resetRuntimeAcl');
+
+        $roleCollectionFactoryMock = $this->createPartialMock(
+            RoleCollectionFactory::class,
+            ['create']
+        );
+        $roleCollectionMock = $this->createPartialMock(
+            RoleCollection::class,
+            ['setUserFilter', 'getFirstItem']
+        );
+        $roleCollectionMock->method('setUserFilter')->willReturnSelf();
+        $roleMock = $this->createPartialMock(Role::class, ['getId', '__wakeup']);
+        $roleMock->method('getId')->willReturn(2);
+        $roleCollectionMock->method('getFirstItem')->willReturn($roleMock);
+        $roleCollectionFactoryMock->method('create')->willReturn($roleCollectionMock);
+
+        $currentRoleContext = new CurrentRoleContext();
+
+        $retriever = new AclRetriever(
+            $aclBuilderMock,
+            $roleCollectionFactoryMock,
+            $rulesCollectionFactoryMock,
+            $this->getMockForAbstractClass(LoggerInterface::class),
+            $currentRoleContext
+        );
+
+        // First call sets roleId to 2
+        $retriever->getAllowedResourcesByRole(2);
+        // Second call with different role should trigger resetRuntimeAcl()
+        $retriever->getAllowedResourcesByRole(3);
+    }
 }
diff --git a/app/code/Magento/Authorization/Test/Unit/Model/Acl/Loader/RuleTest.php b/app/code/Magento/Authorization/Test/Unit/Model/Acl/Loader/RuleTest.php
@@ -11,6 +11,7 @@
 use Magento\Framework\Acl;
 use Magento\Framework\Acl\Data\CacheInterface;
 use Magento\Framework\Acl\RootResource;
+use Magento\Framework\Acl\Role\CurrentRoleContext;
 use Magento\Framework\App\ResourceConnection;
 use Magento\Framework\Serialize\Serializer\Json;
 use Magento\Framework\TestFramework\Unit\Helper\ObjectManager;
@@ -54,8 +55,8 @@ protected function setUp(): void
     {
         $this->rootResource = new RootResource('Magento_Backend::all');
         $this->resourceMock = $this->getMockBuilder(ResourceConnection::class)
+            ->onlyMethods(['getConnection', 'getTableName'])
             ->addMethods(['getTable'])
-            ->onlyMethods(['getConnection'])
             ->disableOriginalConstructor()
             ->getMock();
         $this->aclDataCacheMock = $this->getMockForAbstractClass(CacheInterface::class);
@@ -147,4 +148,81 @@ public function testPopulateAclFromCache(): void
 
         $this->model->populateAcl($aclMock);
     }
+
+    /**
+     * Ensure that when a role context is present, rules are loaded from the role-specific cache key
+     * and applied accordingly.
+     */
+    public function testPopulateAclForSpecificRoleFromCache(): void
+    {
+        $roleId = 10;
+        $rules = [
+            ['role_id' => $roleId, 'resource_id' => 'Magento_Backend::all', 'permission' => 'allow'],
+            ['role_id' => $roleId, 'resource_id' => 'Magento_Backend::admin', 'permission' => 'allow'],
+        ];
+
+        $roleContext = $this->createMock(CurrentRoleContext::class);
+        $roleContext->method('getRoleId')->willReturn($roleId);
+
+        // Expect the role-specific cache key to be read
+        $this->aclDataCacheMock->expects($this->once())
+            ->method('load')
+            ->with(Rule::ACL_RULE_CACHE_KEY . '_' . $roleId)
+            ->willReturn(json_encode($rules));
+
+        // ACL expectations: allow for root, then for specific resource
+        $aclMock = $this->createMock(Acl::class);
+        $aclMock->method('hasResource')->willReturn(true);
+        $calls = [];
+        $aclMock->method('allow')
+            ->willReturnCallback(function ($role, $resource, $privilege) use (&$calls) {
+                $calls[] = [$role, $resource, $privilege];
+                return null;
+            });
+
+        $connectionMock = $this->getMockBuilder(\Magento\Framework\DB\Adapter\AdapterInterface::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+        $connectionMock->method('fetchRow')->willReturn([]); // Return empty array for any DB fetchRow() call
+
+        $selectMock = $this->getMockBuilder('stdClass')
+            ->addMethods(['from', 'where', 'limit'])
+            ->getMock();
+        $selectMock->method('from')->willReturnSelf();
+        $selectMock->method('where')->willReturnSelf();
+        $selectMock->method('limit')->willReturnSelf();
+        $connectionMock->method('select')->willReturn($selectMock);
+        $this->resourceMock->method('getConnection')->willReturn($connectionMock);
+        $this->resourceMock->method('getTableName')->willReturn('authorization_role'); // Return dummy table name
+
+        $objectManager = new ObjectManager($this);
+        $model = $objectManager->getObject(
+            Rule::class,
+            [
+                'rootResource' => $this->rootResource,
+                'resource' => $this->resourceMock,
+                'aclDataCache' => $this->aclDataCacheMock,
+                'serializer' => $this->serializerMock,
+                'roleContext' => $roleContext,
+            ]
+        );
+
+        $model->populateAcl($aclMock);
+
+        $foundRootResourceAllow = false;
+        $foundAdminResourceAllow = false;
+        foreach ($calls as $call) {
+            [$role, $resource, $privilege] = $call;
+            if ($privilege === null && (int)$role === $roleId) {
+                if ($resource === 'Magento_Backend::all') {
+                    $foundRootResourceAllow = true;
+                }
+                if ($resource === 'Magento_Backend::admin') {
+                    $foundAdminResourceAllow = true;
+                }
+            }
+        }
+        $this->assertTrue($foundRootResourceAllow, 'Expected allow() call for Magento_Backend::all with given role');
+        $this->assertTrue($foundAdminResourceAllow, 'Expected allow() call for Magento_Backend::admin with given role');
+    }
 }
diff --git a/lib/internal/Magento/Framework/Authorization/Test/Unit/Policy/AclTest.php b/lib/internal/Magento/Framework/Authorization/Test/Unit/Policy/AclTest.php
@@ -1,14 +1,15 @@
 <?php
 /**
- * Copyright © Magento, Inc. All rights reserved.
- * See COPYING.txt for license details.
+ * Copyright 2012 Adobe
+ * All Rights Reserved.
  */
 declare(strict_types=1);
 
 namespace Magento\Framework\Authorization\Test\Unit\Policy;
 
 use Laminas\Permissions\Acl\Exception\InvalidArgumentException;
 use Magento\Framework\Acl\Builder;
+use Magento\Framework\Acl\Role\CurrentRoleContext;
 use Magento\Framework\Authorization\Policy\Acl;
 use PHPUnit\Framework\MockObject\MockObject;
 use PHPUnit\Framework\TestCase;
@@ -39,7 +40,7 @@ protected function setUp(): void
         $this->_aclMock = $this->createMock(FrameworkAcl::class);
         $this->_aclBuilderMock = $this->createMock(Builder::class);
         $this->_aclBuilderMock->expects($this->any())->method('getAcl')->willReturn($this->_aclMock);
-        $this->_model = new Acl($this->_aclBuilderMock);
+        $this->_model = new Acl($this->_aclBuilderMock, $this->createMock(CurrentRoleContext::class));
     }
 
     /**
@@ -61,7 +62,7 @@ public function testIsAllowedReturnsTrueIfResourceIsAllowedToRole(): void
     public function testIsAllowedReturnsFalseIfRoleDoesntExist(): void
     {
         $this->_aclMock->expects($this->once())
-        ->method('isAllowed')
+            ->method('isAllowed')
             ->with('some_role', 'some_resource')
             ->willThrowException(new InvalidArgumentException());
 
@@ -89,4 +90,22 @@ function ($arg1, $arg2) {
         $this->_aclMock->expects($this->once())->method('hasResource')->with('some_resource')->willReturn(false);
         $this->assertTrue($this->_model->isAllowed('some_role', 'some_resource'));
     }
+
+    public function testSetsAndResetsRoleContext(): void
+    {
+        $roleId = 123;
+        $roleContext = $this->getMockBuilder(CurrentRoleContext::class)
+            ->onlyMethods(['setRoleId', '_resetState'])
+            ->getMock();
+        $model = new Acl($this->_aclBuilderMock, $roleContext);
+
+        $roleContext->expects($this->once())->method('setRoleId')->with($roleId);
+        $this->_aclMock->expects($this->once())
+            ->method('isAllowed')
+            ->with($roleId, 'some_resource')
+            ->willReturn(true);
+        $roleContext->expects($this->once())->method('_resetState');
+
+        $this->assertTrue($model->isAllowed($roleId, 'some_resource'));
+    }
 }
