Ready for review

yiisoft · Feb 14, 2024 · a8fc3fc · a8fc3fc
1 parent d683640
commit a8fc3fc
Show file tree

Hide file tree

Showing 4 changed files with 58 additions and 38 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -56,6 +56,7 @@
 - Bug #237: Handle not found base item in access tree (@arogachev)
 - Enh #245: Handle same names during renaming item in `AssignmentsStorage` (@arogachev)
 - Chg #208: Rename `getAccessTree()` to `getHierarchy()` in `ItemsStorageInterface` (@arogachev)
+- Enh #251: Allow checking for user's roles in `ManagerInterface::userHasPermission()` (@arogachev)
 
 ## 1.0.2 April 20, 2023
 

diff --git a/src/Manager.php b/src/Manager.php
@@ -32,15 +32,15 @@ final class Manager implements ManagerInterface
      * @param RuleFactoryInterface $ruleFactory Rule factory.
      * @param bool $enableDirectPermissions Whether to enable assigning permissions directly to user. Prefer assigning
      * roles only.
-     * @param bool $useOnlyPermissionsInAccessChecks Whether to use only permissions during access checks in
-     * {@see Manager::userHasPermission()}. `false` allows using roles as well.
+     * @param bool $includeRolesInAccessChecks Whether to include roles (in addition to permissions) during access
+     * checks in {@see Manager::userHasPermission()}.
      */
     public function __construct(
         private readonly ItemsStorageInterface $itemsStorage,
         private readonly AssignmentsStorageInterface $assignmentsStorage,
         private readonly RuleFactoryInterface $ruleFactory,
         private readonly bool $enableDirectPermissions = false,
-        private readonly bool $useOnlyPermissionsInAccessChecks = false,
+        private readonly bool $includeRolesInAccessChecks = false,
     ) {
     }
 
@@ -54,7 +54,7 @@ public function userHasPermission(
             return false;
         }
 
-        if ($this->useOnlyPermissionsInAccessChecks && $item->getType() !== Item::TYPE_PERMISSION) {
+        if (!$this->includeRolesInAccessChecks && $item->getType() === Item::TYPE_ROLE) {
             return false;
         }
 

diff --git a/tests/Common/ManagerConfigurationTestTrait.php b/tests/Common/ManagerConfigurationTestTrait.php
@@ -24,14 +24,14 @@ protected function createManager(
         ?AssignmentsStorageInterface $assignmentsStorage = null,
         ?RuleFactoryInterface $ruleFactory = null,
         bool $enableDirectPermissions = false,
-        bool $useOnlyPermissionsInAccessChecks = false,
+        bool $includeRolesInAccessChecks = false,
     ): ManagerInterface {
         return new Manager(...[
             $itemsStorage ?? $this->createItemsStorage(),
             $assignmentsStorage ?? $this->createAssignmentsStorage(),
             $ruleFactory ?? new SimpleRuleFactory(),
             $enableDirectPermissions,
-            $useOnlyPermissionsInAccessChecks,
+            $includeRolesInAccessChecks,
         ]);
     }
 
@@ -50,7 +50,7 @@ protected function createFilledManager(
         ?AssignmentsStorageInterface $assignmentsStorage = null,
         ?RuleFactoryInterface $ruleFactory = null,
         bool $enableDirectPermissions = true,
-        bool $useOnlyPermissionsInAccessChecks = false,
+        bool $includeRolesInAccessChecks = false,
 
     ): ManagerInterface {
         return $this
@@ -63,7 +63,7 @@ protected function createFilledManager(
                     'easyFalse' => new EasyRule(false),
                 ]),
                 $enableDirectPermissions,
-                $useOnlyPermissionsInAccessChecks,
+                $includeRolesInAccessChecks,
             )
             ->addPermission(new Permission('Fast Metabolism'))
             ->addPermission(new Permission('createPost'))

diff --git a/tests/Common/ManagerLogicTestTrait.php b/tests/Common/ManagerLogicTestTrait.php
@@ -46,44 +46,44 @@ protected function tearDown(): void
     public static function dataUserHasPermissionGeneric(): array
     {
         return [
-            ['reader A', 'createPost', ['authorId' => 'author B'], false],
-            ['reader A', 'readPost', ['authorId' => 'author B'], true],
+            ['reader A', 'createPost', [], false],
+            ['reader A', 'readPost', [], true],
             ['reader A', 'updatePost', ['authorId' => 'author B'], false],
-            ['reader A', 'updateAnyPost', ['authorId' => 'author B'], false],
-            ['reader A', 'reader', ['authorId' => 'author B'], false],
+            ['reader A', 'updateAnyPost', [], false],
+            ['reader A', 'reader', [], false],
 
-            ['author B', 'createPost', ['authorId' => 'author B'], true],
-            ['author B', 'readPost', ['authorId' => 'author B'], true],
+            ['author B', 'createPost', [], true],
+            ['author B', 'readPost', [], true],
             ['author B', 'updatePost', ['authorId' => 'author B'], true],
-            ['author B', 'deletePost', ['authorId' => 'author B'], true],
-            ['author B', 'updateAnyPost', ['authorId' => 'author B'], false],
+            ['author B', 'deletePost', [], true],
+            ['author B', 'updateAnyPost', [], false],
 
-            ['admin C', 'createPost', ['authorId' => 'author B'], true],
-            ['admin C', 'readPost', ['authorId' => 'author B'], true],
+            ['admin C', 'createPost', [], true],
+            ['admin C', 'readPost', [], true],
             ['admin C', 'updatePost', ['authorId' => 'author B'], false],
-            ['admin C', 'updateAnyPost', ['authorId' => 'author B'], true],
-            ['admin C', 'nonExistingPermission', ['authorId' => 'author B'], false],
+            ['admin C', 'updateAnyPost', [], true],
+            ['admin C', 'nonExistingPermission', [], false],
 
-            ['guest', 'createPost', ['authorId' => 'author B'], false],
-            ['guest', 'readPost', ['authorId' => 'author B'], false],
+            ['guest', 'createPost', [], false],
+            ['guest', 'readPost', [], false],
             ['guest', 'updatePost', ['authorId' => 'author B'], false],
-            ['guest', 'deletePost', ['authorId' => 'author B'], false],
-            ['guest', 'updateAnyPost', ['authorId' => 'author B'], false],
-            ['guest', 'blablabla', ['authorId' => 'author B'], false],
+            ['guest', 'deletePost', [], false],
+            ['guest', 'updateAnyPost', [], false],
+            ['guest', 'blablabla', [], false],
 
-            [12, 'createPost', ['authorId' => 'author B'], false],
-            [12, 'readPost', ['authorId' => 'author B'], false],
+            [12, 'createPost', [], false],
+            [12, 'readPost', [], false],
             [12, 'updatePost', ['authorId' => 'author B'], false],
-            [12, 'deletePost', ['authorId' => 'author B'], false],
-            [12, 'updateAnyPost', ['authorId' => 'author B'], false],
-            [12, 'blablabla', ['authorId' => 'author B'], false],
+            [12, 'deletePost', [], false],
+            [12, 'updateAnyPost', [], false],
+            [12, 'blablabla', [], false],
 
-            [null, 'createPost', ['authorId' => 'author B'], false],
-            [null, 'readPost', ['authorId' => 'author B'], false],
+            [null, 'createPost', [], false],
+            [null, 'readPost', [], false],
             [null, 'updatePost', ['authorId' => 'author B'], false],
-            [null, 'deletePost', ['authorId' => 'author B'], false],
-            [null, 'updateAnyPost', ['authorId' => 'author B'], false],
-            [null, 'blablabla', ['authorId' => 'author B'], false],
+            [null, 'deletePost', [], false],
+            [null, 'updateAnyPost', [], false],
+            [null, 'blablabla', [], false],
         ];
     }
 
@@ -262,10 +262,29 @@ public function testUserHasPermissionWithNonExistingRule(): void
         $manager->userHasPermission('reader A', 'test-permission');
     }
 
-    public function testUserHasPermissionWithRoleNotAllowed(): void
+    public static function dataUserHasPermissionWithRolesAllowed(): array
     {
-        $manager = $this->createFilledManager(useOnlyPermissionsInAccessChecks: true);
-        $this->assertFalse($manager->userHasPermission(userId: 'admin C', permissionName: 'reader'));
+        return [
+            ['reader A', 'reader', true],
+            ['reader A', 'admin', false],
+            ['reader A', 'non-existing', false],
+            ['admin C', 'reader', true],
+        ];
+    }
+
+    /**
+     * @dataProvider dataUserHasPermissionWithRolesAllowed
+     */
+    public function testUserHasPermissionWithRolesAllowed(
+        string $userId,
+        string $permissionName,
+        bool $expectedHasPermission,
+    ): void
+    {
+        $this->assertSame(
+            $expectedHasPermission,
+            $this->createFilledManager(includeRolesInAccessChecks: true)->userHasPermission($userId, $permissionName),
+        );
     }
 
     public function testCanAddExistingChild(): void