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); + } }