From 2d9d2bba80e853b8071d6d4ff7449321d36761a9 Mon Sep 17 00:00:00 2001
From: Dan Brown <ssddanbrown@googlemail.com>
Date: Wed, 21 Dec 2022 17:00:05 +0000
Subject: [PATCH] Added additional case thats known to currently fail
Also removed so no-longer-relevant todo/comments.
---
app/Auth/Permissions/PermissionApplicator.php | 16 ++++---------
dev/docs/permission-scenario-testing.md | 19 +++++++++++++++
.../Scenarios/EntityRolePermissionsTest.php | 23 +++++++++++++++++++
3 files changed, 46 insertions(+), 12 deletions(-)
diff --git a/app/Auth/Permissions/PermissionApplicator.php b/app/Auth/Permissions/PermissionApplicator.php
index acb08ee3a..58ee7d93a 100644
--- 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();
- // 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 */
@@ -151,10 +144,9 @@ class PermissionApplicator
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) {
- // TODO - Need to use these ids in some form in outer permission check, as blockers when access
return false;
}
diff --git a/dev/docs/permission-scenario-testing.md b/dev/docs/permission-scenario-testing.md
index 6a6a9d666..bfb5e7aa3 100644
--- 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.
+#### 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
diff --git a/tests/Permissions/Scenarios/EntityRolePermissionsTest.php b/tests/Permissions/Scenarios/EntityRolePermissionsTest.php
index 58870ee63..b92ce620b 100644
--- a/tests/Permissions/Scenarios/EntityRolePermissionsTest.php
+++ b/tests/Permissions/Scenarios/EntityRolePermissionsTest.php
@@ -175,4 +175,27 @@ class EntityRolePermissionsTest extends PermissionScenarioTestCase
$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);
+ }
}