Started aligning permission behaviour across application methods

app/Auth/Permissions/EntityPermission.php

@@ -40,4 +40,30 @@ class EntityPermission extends Model
     {
         return $this->belongsTo(User::class);
     }
+
+    /**
+     * Get the type of entity permission this is.
+     * Will be one of: user, role, fallback
+     */
+    public function getAssignedType(): string
+    {
+        if ($this->user_id) {
+            return 'user';
+        }
+
+        if ($this->role_id) {
+            return 'role';
+        }
+
+        return 'fallback';
+    }
+
+    /**
+     * Get the ID for the assigned type of permission.
+     * (Role/User ID). Defaults to 0 for fallback.
+     */
+    public function getAssignedTypeId(): int
+    {
+        return $this->user_id ?? $this->role_id ?? 0;
+    }
 }

app/Auth/Permissions/PermissionApplicator.php

@@ -77,6 +77,11 @@ class PermissionApplicator
             $chain[] = $entity->book;
         }
 
+        // Record role access preventions.
+        // Used when we encounter a negative role permission where inheritance is active and therefore
+        // need to check permissive status on parent items.
+        $blockedRoleIds = [];
+
         foreach ($chain as $currentEntity) {
             $relevantPermissions = $currentEntity->permissions()
                 ->where(function (Builder $query) use ($userRoleIds, $userId) {
@@ -95,39 +100,62 @@ class PermissionApplicator
             // 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.
 
-            // If the default is set, we have to return something here.
-            $allowedById = [];
+            $allowedByTypeById = ['fallback' => [], 'user' => [], 'role' => []];
+            /** @var EntityPermission $permission */
             foreach ($relevantPermissions as $permission) {
-                $allowedById[($permission->role_id ?? '') . ':' . ($permission->user_id ?? '')] = $permission->$action;
+                $allowedByTypeById[$permission->getAssignedType()][$permission->getAssignedTypeId()] = $permission->$permission;
             }
 
+            $inheriting = !isset($allowedByTypeById['fallback'][0]);
+
             // Continue up the chain if no applicable entity permission overrides.
-            if (empty($allowedById)) {
+            if (count($relevantPermissions) === 0) {
                 continue;
             }
 
             // If we have user-specific permissions set, return the status of that
             // since it's the most specific possible.
-            $userKey = ':' . $userId;
-            if (isset($allowedById[$userKey])) {
-                return $allowedById[$userKey];
+            if (isset($allowedByTypeById['user'][$userId])) {
+                return $allowedByTypeById['user'][$userId];
             }
 
             // If we have role-specific permissions set, allow if any of those
-            // role permissions allow access.
-            $hasDefault = isset($allowedById[':']);
-            if (!$hasDefault || count($allowedById) > 1) {
-                foreach ($allowedById as $key => $allowed) {
-                    if ($key !== ':' && $allowed) {
-                        return true;
-                    }
+            // role permissions allow access. We do not allow if the role has been previously
+            // blocked by a high-priority inheriting level.
+            // If we're inheriting at this level, and there's an explicit non-allow permission, we record
+            // it for checking up the chain.
+            foreach ($allowedByTypeById['role'] as $roleId => $allowed) {
+                if ($allowed && !in_array($roleId, $blockedRoleIds)) {
+                    return true;
+                } else if (!$allowed) {
+                    $blockedRoleIds[] = $roleId;
                 }
+            }
+
+            // If we had role permissions, and none of them allowed (via above loop), and
+            // we are not inheriting, exit here since we only have role permissions in play blocking access.
+            if (count($allowedByTypeById['role']) > 0 && !$inheriting) {
                 return false;
             }
 
+            // Continue up the chain if inheriting
+            if ($inheriting) {
+                continue;
+            }
+
             // Otherwise, return the default "Other roles" fallback value.
-            return $allowedById[':'];
+            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 (count($blockedRoleIds) > 0) {
+            // TODO - Need to use these ids in some form in outer permission check, as blockers when access
+            return false;
         }
 
         return null;

resources/js/components/entity-permissions.js

@@ -20,7 +20,7 @@ export class EntityPermissions extends Component {
         // "Everyone Else" inherit toggle
         this.everyoneInheritToggle.addEventListener('change', event => {
             const inherit = event.target.checked;
-            const permissions = document.querySelectorAll('input[name^="permissions[0]["]');
+            const permissions = document.querySelectorAll('input[name^="permissions[fallback]"]');
             for (const permission of permissions) {
                 permission.disabled = inherit;
                 permission.checked = false;

tests/Helpers/EntityProvider.php

@@ -202,14 +202,16 @@ class EntityProvider
      * @param string[] $actions
      * @param Role[] $roles
      */
-    public function setPermissions(Entity $entity, array $actions = [], array $roles = []): void
+    public function setPermissions(Entity $entity, array $actions = [], array $roles = [], $inherit = false): void
     {
         $entity->permissions()->delete();
 
-        $permissions = [
+        $permissions = [];
+
+        if (!$inherit) {
             // Set default permissions to not allow actions so that only the provided role permissions are at play.
-            ['role_id' => null, 'view' => false, 'create' => false, 'update' => false, 'delete' => false],
-        ];
+            $permissions[] = ['role_id' => null, 'user_id' => null, 'view' => false, 'create' => false, 'update' => false, 'delete' => false];
+        }
 
         foreach ($roles as $role) {
             $permission = ['role_id' => $role->id];

tests/Permissions/EntityPermissionsTest.php

@@ -2,6 +2,7 @@
 
 namespace Tests\Permissions;
 
+use BookStack\Auth\Role;
 use BookStack\Auth\User;
 use BookStack\Entities\Models\Book;
 use BookStack\Entities\Models\Bookshelf;
@@ -657,6 +658,34 @@ class EntityPermissionsTest extends TestCase
         $resp->assertRedirect($book->getUrl('/page/test-page'));
     }
 
+    public function test_access_to_item_prevented_if_inheritance_active_but_permission_prevented_via_role()
+    {
+        $user = $this->getViewer();
+        $viewerRole = $user->roles->first();
+        $chapter = $this->entities->chapter();
+        $book = $chapter->book;
+
+        $this->entities->setPermissions($book, ['edit'], [$viewerRole], false);
+        $this->entities->setPermissions($chapter, [], [$viewerRole], true);
+
+        $this->assertFalse(userCan('chapter-update', $chapter));
+    }
+
+    public function test_access_to_item_allowed_if_inheritance_active_and_permission_prevented_via_role_but_allowed_via_parent()
+    {
+        $user = $this->getViewer();
+        $viewerRole = $user->roles->first();
+        $editorRole = Role::getRole('Editor');
+        $user->attachRole($editorRole);
+        $chapter = $this->entities->chapter();
+        $book = $chapter->book;
+
+        $this->entities->setPermissions($book, ['edit'], [$editorRole], false);
+        $this->entities->setPermissions($chapter, [], [$viewerRole], true);
+
+        $this->assertTrue(userCan('chapter-update', $chapter));
+    }
+
     public function test_book_permissions_can_be_generated_without_error_if_child_chapter_is_in_recycle_bin()
     {
         $book = $this->entities->bookHasChaptersAndPages();