Added additional case thats known to currently fail

Also removed so no-longer-relevant todo/comments.

diff --git a/app/Auth/Permissions/PermissionApplicator.php b/app/Auth/Permissions/PermissionApplicator.php
index acb08ee3adc75094caad886f312079a0cad2e493..58ee7d93a1db4a58ac3da711bb41b1672e61827a 100644 (file)
--- a/app/Auth/Permissions/PermissionApplicator.php
+++ b/app/Auth/Permissions/PermissionApplicator.php
@@ -94,15 +94,8 @@ class PermissionApplicator
                 ->get(['role_id', 'user_id', $action])
                 ->all();
 
                 ->get(['role_id', 'user_id', $action])
                 ->all();
 

-            // Permissions work on specificity, in order of:
-            // 1. User-specific permissions
-            // 2. Role-specific permissions
-            // 3. Fallback-specific permissions
-            // For role permissions, the system tries to be fairly permissive, in that if the user has two roles,
-            // one lacking and one permitting an action, they will be permitted.
-            // This can be complex when multiple roles and inheritance gets involved. If permission is prevented
-            // via "Role A" on an item, but inheritance is active and permission is granted via "Role B" on parent item,
-            // the user will be granted permission.
+            // See dev/docs/permission-scenario-testing.md for technical details
+            // on how permissions should be enforced.

 
             $allowedByTypeById = ['fallback' => [], 'user' => [], 'role' => []];
             /** @var EntityPermission $permission */
 
             $allowedByTypeById = ['fallback' => [], 'user' => [], 'role' => []];
             /** @var EntityPermission $permission */


@@ -151,10 +144,9 @@ class PermissionApplicator
             return $allowedByTypeById['fallback'][0];
         }
 
             return $allowedByTypeById['fallback'][0];
         }
 

-        // If we have roles that need to be assessed, but we are also inheriting, pass back the prevented
-        // role IDs so they can be excluded from the role permission check.
+        // If we have relevant roles conditions that are actively blocking
+        // return false since these are more specific than potential role-level permissions.

         if (count($blockedRoleIds) > 0) {
         if (count($blockedRoleIds) > 0) {

-            // TODO - Need to use these ids in some form in outer permission check, as blockers when access

             return false;
         }
 
             return false;
         }
 

diff --git a/dev/docs/permission-scenario-testing.md b/dev/docs/permission-scenario-testing.md
index 6a6a9d6661e6a7b2cedab3217010ff3d68807480..bfb5e7aa309468639f58bb72760bd23981e5764d 100644 (file)
--- a/dev/docs/permission-scenario-testing.md
+++ b/dev/docs/permission-scenario-testing.md
@@ -220,6 +220,25 @@ User denied page permission.
 
 User denied page permission.
 
 
 User denied page permission.
 

+#### test_70_multi_role_inheriting_deny
+
+- Page permissions have inherit enabled.
+- Role A has all page role permission.
+- Role B has entity denied page permission.
+- User has Role A and B.
+
+User denied page permission.
+
+#### test_80_multi_role_inherited_deny_via_parent
+
+- Page permissions have inherit enabled.
+- Chapter permissions have inherit enabled.
+- Role A has all-pages role permission.
+- Role B has entity denied chapter permission.
+- User has Role A & B.
+
+User denied page permission.
+

 ---
 
 ### Entity User Permissions
 ---
 
 ### Entity User Permissions

diff --git a/tests/Permissions/Scenarios/EntityRolePermissionsTest.php b/tests/Permissions/Scenarios/EntityRolePermissionsTest.php
index 58870ee63e0b9315610917eb54ce83e70d404b6d..b92ce620b78c595c159c4bf1b021c14a3bd5cc51 100644 (file)
--- a/tests/Permissions/Scenarios/EntityRolePermissionsTest.php
+++ b/tests/Permissions/Scenarios/EntityRolePermissionsTest.php
@@ -175,4 +175,27 @@ class EntityRolePermissionsTest extends PermissionScenarioTestCase
 
         $this->assertNotVisibleToUser($page, $user);
     }
 
         $this->assertNotVisibleToUser($page, $user);
     }

+
+    public function test_70_multi_role_inheriting_deny()
+    {
+        [$user, $roleA] = $this->users->newUserWithRole([], ['page-view-all']);
+        $roleB = $this->users->attachNewRole($user);
+        $page = $this->entities->page();
+
+        $this->permissions->addEntityPermission($page, [], $roleB);
+
+        $this->assertNotVisibleToUser($page, $user);
+    }
+
+    public function test_80_multi_role_inherited_deny_via_parent()
+    {
+        [$user, $roleA] = $this->users->newUserWithRole([], ['page-view-all']);
+        $roleB = $this->users->attachNewRole($user);
+        $page = $this->entities->pageWithinChapter();
+        $chapter = $page->chapter;
+
+        $this->permissions->addEntityPermission($chapter, [], $roleB);
+
+        $this->assertNotVisibleToUser($page, $user);
+    }

 }
 }