Started aligning permission behaviour across application methods

diff --git a/app/Auth/Permissions/EntityPermission.php b/app/Auth/Permissions/EntityPermission.php
index 8592d25dd2c1bdc3ab4429ee59c6ddd6baaa2c5b..5d9f2c404890fa5e5ee5aa2a96f2864d877739ac 100644 (file)
--- a/app/Auth/Permissions/EntityPermission.php
+++ b/app/Auth/Permissions/EntityPermission.php
@@ -40,4 +40,30 @@ class EntityPermission extends Model
     {
         return $this->belongsTo(User::class);
     }
     {
         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;
+    }

 }
 }

diff --git a/app/Auth/Permissions/PermissionApplicator.php b/app/Auth/Permissions/PermissionApplicator.php
index 0848167fada46f00fb332d6e88b1eaa88bfad8b3..47d152a04d281cbf5df4d48c3673cbaf8f8fbc86 100644 (file)
--- a/app/Auth/Permissions/PermissionApplicator.php
+++ b/app/Auth/Permissions/PermissionApplicator.php
@@ -77,6 +77,11 @@ class PermissionApplicator
             $chain[] = $entity->book;
         }
 
             $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) {
         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.
             // 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) {
             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.
             // 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.
                 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
             }
 
             // 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;
             }
 
                 return false;
             }
 

+            // Continue up the chain if inheriting
+            if ($inheriting) {
+                continue;
+            }
+

             // Otherwise, return the default "Other roles" fallback value.
             // 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;
         }
 
         return null;

diff --git a/resources/js/components/entity-permissions.js b/resources/js/components/entity-permissions.js
index 35c2d25ef101dcfe433f6688cdbc20bbdbe63a4f..61ab98a98df58bf7469cf4b3bd55b703ce31709f 100644 (file)
--- a/resources/js/components/entity-permissions.js
+++ b/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;
         // "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;
             for (const permission of permissions) {
                 permission.disabled = inherit;
                 permission.checked = false;

diff --git a/tests/Helpers/EntityProvider.php b/tests/Helpers/EntityProvider.php
index d285e6b781ef9b8b0b5e2e7d659753b65c909f23..d5cbd866c6ac6a18824a3683b04790e4d266e8d9 100644 (file)
--- a/tests/Helpers/EntityProvider.php
+++ b/tests/Helpers/EntityProvider.php
@@ -202,14 +202,16 @@ class EntityProvider
      * @param string[] $actions
      * @param Role[] $roles
      */
      * @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();
 
     {
         $entity->permissions()->delete();
 

-        $permissions = [
+        $permissions = [];
+
+        if (!$inherit) {

             // Set default permissions to not allow actions so that only the provided role permissions are at play.
             // 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];
 
         foreach ($roles as $role) {
             $permission = ['role_id' => $role->id];

diff --git a/tests/Permissions/EntityPermissionsTest.php b/tests/Permissions/EntityPermissionsTest.php
index 9b2a8db8683ab85683ac9a6148789ee2fe8caae0..85dd32cf566a1a6c1657e8a85b0de52fa6cbbe7c 100644 (file)
--- a/tests/Permissions/EntityPermissionsTest.php
+++ b/tests/Permissions/EntityPermissionsTest.php
@@ -2,6 +2,7 @@
 
 namespace Tests\Permissions;
 
 
 namespace Tests\Permissions;
 

+use BookStack\Auth\Role;

 use BookStack\Auth\User;
 use BookStack\Entities\Models\Book;
 use BookStack\Entities\Models\Bookshelf;
 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'));
     }
 
         $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();
     public function test_book_permissions_can_be_generated_without_error_if_child_chapter_is_in_recycle_bin()
     {
         $book = $this->entities->bookHasChaptersAndPages();