From: Dan Brown <redacted>
Date: Wed, 7 Dec 2022 22:07:03 +0000 (+0000)
Subject: Aligned logic to entity_permission role_id usage change
X-Git-Url: http://source.bookstackapp.com/bookstack/commitdiff_plain/f8c4725166ee317b4ec6a19278d1650426dcc419

Aligned logic to entity_permission role_id usage change

Now idenitifies fallback using role_id and user_id = null.
Lays some foundations for handling user_id.
---

diff --git a/app/Auth/Permissions/EntityPermission.php b/app/Auth/Permissions/EntityPermission.php
index 32ebc440d..79fd1a2db 100644
--- a/app/Auth/Permissions/EntityPermission.php
+++ b/app/Auth/Permissions/EntityPermission.php
@@ -10,6 +10,7 @@ use Illuminate\Database\Eloquent\Relations\MorphTo;
 /**
  * @property int $id
  * @property int $role_id
+ * @property int $user_id
  * @property int $entity_id
  * @property string $entity_type
  * @property boolean $view
diff --git a/app/Auth/Permissions/JointPermissionBuilder.php b/app/Auth/Permissions/JointPermissionBuilder.php
index 114cff619..0d9d23942 100644
--- a/app/Auth/Permissions/JointPermissionBuilder.php
+++ b/app/Auth/Permissions/JointPermissionBuilder.php
@@ -245,7 +245,9 @@ class JointPermissionBuilder
         // Create a mapping of explicit entity permissions
         $permissionMap = [];
         foreach ($permissions as $permission) {
-            $key = $permission->entity_type . ':' . $permission->entity_id . ':' . $permission->role_id;
+            $type = $permission->role_id ? 'role' : ($permission->user_id ? 'user' : 'fallback');
+            $id = $permission->role_id ?? $permission->user_id ?? '0';
+            $key = $permission->entity_type . ':' . $permission->entity_id . ':' . $type  . ':' .  $id;
             $permissionMap[$key] = $permission->view;
         }
 
@@ -376,18 +378,18 @@ class JointPermissionBuilder
     protected function entityPermissionsActiveForRole(array $permissionMap, SimpleEntityData $entity, int $roleId): bool
     {
         $keyPrefix = $entity->type . ':' . $entity->id . ':';
-        return isset($permissionMap[$keyPrefix . $roleId]) || isset($permissionMap[$keyPrefix . '0']);
+        return isset($permissionMap[$keyPrefix . 'role:' . $roleId]) || isset($permissionMap[$keyPrefix . 'fallback:0']);
     }
 
     /**
      * Check for an active restriction in an entity map.
      */
-    protected function mapHasActiveRestriction(array $entityMap, SimpleEntityData $entity, int $roleId): bool
+    protected function mapHasActiveRestriction(array $permissionMap, SimpleEntityData $entity, int $roleId): bool
     {
-        $roleKey = $entity->type . ':' . $entity->id . ':' . $roleId;
-        $defaultKey = $entity->type . ':' . $entity->id . ':0';
+        $roleKey = $entity->type . ':' . $entity->id . ':role:' . $roleId;
+        $defaultKey = $entity->type . ':' . $entity->id . ':fallback:0';
 
-        return $entityMap[$roleKey] ?? $entityMap[$defaultKey] ?? false;
+        return $permissionMap[$roleKey] ?? $permissionMap[$defaultKey] ?? false;
     }
 
     /**
diff --git a/app/Auth/Permissions/PermissionApplicator.php b/app/Auth/Permissions/PermissionApplicator.php
index af372cb74..614860263 100644
--- a/app/Auth/Permissions/PermissionApplicator.php
+++ b/app/Auth/Permissions/PermissionApplicator.php
@@ -78,26 +78,53 @@ class PermissionApplicator
         }
 
         foreach ($chain as $currentEntity) {
-            $allowedByRoleId = $currentEntity->permissions()
-                ->whereIn('role_id', [0, ...$userRoleIds])
-                ->pluck($action, 'role_id');
+            $relevantPermissions = $currentEntity->permissions()
+                ->where(function (Builder $query) use ($userRoleIds) {
+                    $query->whereIn('role_id', $userRoleIds)
+                    ->orWhere(function (Builder $query) {
+                        $query->whereNull(['role_id', 'user_id']);
+                    });
+                })
+                ->get(['role_id', 'user_id', $action])
+                ->all();
+
+            // TODO - Update below for user permissions
+
+            // 1. Default fallback set and allows, no role permissions -> True
+            // 2. Default fallback set and prevents, no role permissions -> False
+            // 3. Role permission allows, fallback set and allows -> True
+            // 3. Role permission allows, fallback set and prevents -> True
+            // 3. Role permission allows, fallback not set -> True
+            // 3. Role permission prevents, fallback set and allows -> False
+            // 3. Role permission prevents, fallback set and prevents -> False
+            // 3. Role permission prevents, fallback not set -> False
+            // 4. Nothing exists -> Continue
+
+            // If the default is set, we have to return something here.
+            $allowedById = [];
+            foreach ($relevantPermissions as $permission) {
+                $allowedById[$permission->role_id . ':' . $permission->user_id] = $permission->$action;
+            }
 
             // Continue up the chain if no applicable entity permission overrides.
-            if ($allowedByRoleId->isEmpty()) {
+            if (empty($allowedById)) {
                 continue;
             }
 
             // If we have user-role-specific permissions set, allow if any of those
             // role permissions allow access.
-            $hasDefault = $allowedByRoleId->has(0);
-            if (!$hasDefault || $allowedByRoleId->count() > 1) {
-                return $allowedByRoleId->search(function (bool $allowed, int $roleId) {
-                        return $roleId !== 0 && $allowed;
-                }) !== false;
+            $hasDefault = isset($allowedById[':']);
+            if (!$hasDefault || count($allowedById) > 1) {
+                foreach ($allowedById as $key => $allowed) {
+                    if ($key !== ':' && $allowed) {
+                        return true;
+                    }
+                }
+                return false;
             }
 
             // Otherwise, return the default "Other roles" fallback value.
-            return $allowedByRoleId->get(0);
+            return $allowedById[':'];
         }
 
         return null;
@@ -114,6 +141,7 @@ class PermissionApplicator
         $permissionQuery = EntityPermission::query()
             ->where($action, '=', true)
             ->whereIn('role_id', $this->getCurrentUserRoleIds());
+        // TODO - Update for user permission
 
         if (!empty($entityClass)) {
             /** @var Entity $entityInstance */
@@ -134,6 +162,7 @@ class PermissionApplicator
     {
         return $query->where(function (Builder $parentQuery) {
             $parentQuery->whereHas('jointPermissions', function (Builder $permissionQuery) {
+                // TODO - Update for user permission
                 $permissionQuery->whereIn('role_id', $this->getCurrentUserRoleIds())
                     ->where(function (Builder $query) {
                         $this->addJointHasPermissionCheck($query, $this->currentUser()->id);
@@ -170,6 +199,7 @@ class PermissionApplicator
         $pageMorphClass = (new Page())->getMorphClass();
 
         $q = $query->whereExists(function ($permissionQuery) use (&$tableDetails) {
+            // TODO - Update for user permission
             /** @var Builder $permissionQuery */
             $permissionQuery->select(['role_id'])->from('joint_permissions')
                 ->whereColumn('joint_permissions.entity_id', '=', $tableDetails['tableName'] . '.' . $tableDetails['entityIdColumn'])
@@ -203,6 +233,7 @@ class PermissionApplicator
         $fullPageIdColumn = $tableName . '.' . $pageIdColumn;
         $morphClass = (new Page())->getMorphClass();
 
+        // TODO - Update for user permission
         $existsQuery = function ($permissionQuery) use ($fullPageIdColumn, $morphClass) {
             /** @var Builder $permissionQuery */
             $permissionQuery->select('joint_permissions.role_id')->from('joint_permissions')
diff --git a/app/Auth/Permissions/PermissionFormData.php b/app/Auth/Permissions/PermissionFormData.php
index 8044a3c56..18d45591f 100644
--- a/app/Auth/Permissions/PermissionFormData.php
+++ b/app/Auth/Permissions/PermissionFormData.php
@@ -21,7 +21,7 @@ class PermissionFormData
     {
         return $this->entity->permissions()
             ->with('role')
-            ->where('role_id', '!=', 0)
+            ->whereNotNull('role_id')
             ->get()
             ->sortBy('role.display_name')
             ->all();
@@ -33,7 +33,7 @@ class PermissionFormData
      */
     public function rolesNotAssigned(): array
     {
-        $assigned = $this->entity->permissions()->pluck('role_id');
+        $assigned = $this->entity->permissions()->whereNotNull('role_id')->pluck('role_id');
         return Role::query()
             ->where('system_name', '!=', 'admin')
             ->whereNotIn('id', $assigned)
@@ -49,20 +49,19 @@ class PermissionFormData
     {
         /** @var ?EntityPermission $permission */
         $permission = $this->entity->permissions()
-            ->where('role_id', '=', 0)
+            ->whereNull(['role_id', 'user_id'])
             ->first();
         return $permission ?? (new EntityPermission());
     }
 
     /**
-     * Get the "Everyone Else" role entry.
+     * Check if the "Everyone else" option is inheriting default role system permissions.
+     * Is determined by any system entity_permission existing for the current entity.
      */
-    public function everyoneElseRole(): Role
+    public function everyoneElseInheriting(): bool
     {
-        return (new Role())->forceFill([
-            'id' => 0,
-            'display_name' => trans('entities.permissions_role_everyone_else'),
-            'description' => trans('entities.permissions_role_everyone_else_desc'),
-        ]);
+        return !$this->entity->permissions()
+            ->whereNull(['role_id', 'user_id'])
+            ->exists();
     }
 }
diff --git a/app/Entities/Tools/PermissionsUpdater.php b/app/Entities/Tools/PermissionsUpdater.php
index eb4eb6b48..f13323ba6 100644
--- a/app/Entities/Tools/PermissionsUpdater.php
+++ b/app/Entities/Tools/PermissionsUpdater.php
@@ -58,13 +58,30 @@ class PermissionsUpdater
     protected function formatPermissionsFromRequestToEntityPermissions(array $permissions): array
     {
         $formatted = [];
+        $columnsByType = [
+            'role' => 'role_id',
+            'user' => 'user_id',
+            'fallback' => '',
+        ];
 
-        foreach ($permissions as $roleId => $info) {
-            $entityPermissionData = ['role_id' => $roleId];
-            foreach (EntityPermission::PERMISSIONS as $permission) {
-                $entityPermissionData[$permission] = (($info[$permission] ?? false) === "true");
+        foreach ($permissions as $type => $byId) {
+            $column  = $columnsByType[$type] ?? null;
+            if (is_null($column)) {
+                continue;
+            }
+
+            foreach ($byId as $id => $info) {
+                $entityPermissionData = [];
+
+                if (!empty($column)) {
+                    $entityPermissionData[$column] = $id;
+                }
+
+                foreach (EntityPermission::PERMISSIONS as $permission) {
+                    $entityPermissionData[$permission] = (($info[$permission] ?? false) === "true");
+                }
+                $formatted[] = $entityPermissionData;
             }
-            $formatted[] = $entityPermissionData;
         }
 
         return $formatted;
diff --git a/app/Http/Controllers/PermissionsController.php b/app/Http/Controllers/PermissionsController.php
index 7d908733b..562e8305b 100644
--- a/app/Http/Controllers/PermissionsController.php
+++ b/app/Http/Controllers/PermissionsController.php
@@ -162,10 +162,14 @@ class PermissionsController extends Controller
     {
         $this->checkPermissionOr('restrictions-manage-all', fn() => userCan('restrictions-manage-own'));
 
+        /** @var Role $role */
         $role = Role::query()->findOrFail($roleId);
 
         return view('form.entity-permissions-row', [
-            'role' => $role,
+            'modelType' => 'role',
+            'modelId' => $role->id,
+            'modelName' => $role->display_name,
+            'modelDescription' => $role->description,
             'permission' => new EntityPermission(),
             'entityType' => $entityType,
             'inheriting' => false,
diff --git a/resources/js/components/entity-permissions.js b/resources/js/components/entity-permissions.js
index d4a616ff1..b1b6c5084 100644
--- a/resources/js/components/entity-permissions.js
+++ b/resources/js/components/entity-permissions.js
@@ -28,7 +28,7 @@ export class EntityPermissions extends Component {
         // Remove role row button click
         this.container.addEventListener('click', event => {
             const button = event.target.closest('button');
-            if (button && button.dataset.roleId) {
+            if (button && button.dataset.modelType) {
                 this.removeRowOnButtonClick(button)
             }
         });
@@ -61,14 +61,18 @@ export class EntityPermissions extends Component {
 
     removeRowOnButtonClick(button) {
         const row = button.closest('.item-list-row');
-        const roleId = button.dataset.roleId;
-        const roleName = button.dataset.roleName;
+        const modelId = button.dataset.modelId;
+        const modelName = button.dataset.modelName;
+        const modelType = button.dataset.modelType;
 
         const option = document.createElement('option');
-        option.value = roleId;
-        option.textContent = roleName;
+        option.value = modelId;
+        option.textContent = modelName;
 
-        this.roleSelect.append(option);
+        if (modelType === 'role') {
+            this.roleSelect.append(option);
+        }
+        // TODO - User role!
         row.remove();
     }
 
diff --git a/resources/views/form/entity-permissions-row.blade.php b/resources/views/form/entity-permissions-row.blade.php
index 1cfb6ec98..1d028d7f6 100644
--- a/resources/views/form/entity-permissions-row.blade.php
+++ b/resources/views/form/entity-permissions-row.blade.php
@@ -1,5 +1,8 @@
 {{--
-$role - The Role to display this row for.
+$modelType - The type of permission model; String matching one of: user, role, fallback
+$modelId - The ID of the permission model.
+$modelName - The name of the permission model.
+$modelDescription - The description of the permission model.
 $entityType - String identifier for type of entity having permissions applied.
 $permission - The entity permission containing the permissions.
 $inheriting - Boolean if the current row should be marked as inheriting default permissions. Used for "Everyone Else" role.
@@ -7,21 +10,21 @@ $inheriting - Boolean if the current row should be marked as inheriting default
 
 <div component="permissions-table" class="item-list-row flex-container-row justify-space-between wrap">
     <div class="gap-x-m flex-container-row items-center px-l py-m flex">
-        <div class="text-large" title="{{ $role->id === 0 ? trans('entities.permissions_role_everyone_else') : trans('common.role') }}">
-            @icon($role->id === 0 ? 'groups' : 'role')
+        <div class="text-large" title="{{  $modelType === 'fallback' ? trans('entities.permissions_role_everyone_else') : trans('common.role') }}">
+            @icon($modelType === 'fallback' ? 'groups' : 'role')
         </div>
         <span>
-            <strong>{{ $role->display_name }}</strong> <br>
-            <small class="text-muted">{{ $role->description }}</small>
+            <strong>{{ $modelName }}</strong> <br>
+            <small class="text-muted">{{ $modelDescription }}</small>
         </span>
-        @if($role->id !== 0)
+        @if($modelType !== 'fallback')
             <button type="button"
                 class="ml-auto flex-none text-small text-primary text-button hover-underline item-list-row-toggle-all hide-under-s"
                 refs="permissions-table@toggle-all"
                 ><strong>{{ trans('common.toggle_all') }}</strong></button>
         @endif
     </div>
-    @if($role->id === 0)
+    @if($modelType === 'fallback')
         <div class="px-l flex-container-row items-center" refs="entity-permissions@everyone-inherit">
             @include('form.custom-checkbox', [
                 'name' => 'entity-permissions-inherit',
@@ -32,12 +35,12 @@ $inheriting - Boolean if the current row should be marked as inheriting default
         </div>
     @endif
     <div class="flex-container-row justify-space-between gap-x-xl wrap items-center">
-        <input type="hidden" name="permissions[{{ $role->id }}][active]"
+        <input type="hidden" name="permissions[{{ $modelType }}][{{ $modelId }}][active]"
                @if($inheriting) disabled="disabled" @endif
                value="true">
         <div class="px-l">
             @include('form.custom-checkbox', [
-                'name' =>  'permissions[' . $role->id . '][view]',
+                'name' =>  'permissions[' . $modelType . '][' . $modelId . '][view]',
                 'label' => trans('common.view'),
                 'value' => 'true',
                 'checked' => $permission->view,
@@ -47,7 +50,7 @@ $inheriting - Boolean if the current row should be marked as inheriting default
         @if($entityType !== 'page')
             <div class="px-l">
                 @include('form.custom-checkbox', [
-                    'name' =>  'permissions[' . $role->id . '][create]',
+                    'name' =>  'permissions[' . $modelType . '][' . $modelId . '][create]',
                     'label' => trans('common.create'),
                     'value' => 'true',
                     'checked' => $permission->create,
@@ -57,7 +60,7 @@ $inheriting - Boolean if the current row should be marked as inheriting default
         @endif
         <div class="px-l">
             @include('form.custom-checkbox', [
-                'name' =>  'permissions[' . $role->id . '][update]',
+                'name' =>  'permissions[' . $modelType . '][' . $modelId . '][update]',
                 'label' => trans('common.update'),
                 'value' => 'true',
                 'checked' => $permission->update,
@@ -66,7 +69,7 @@ $inheriting - Boolean if the current row should be marked as inheriting default
         </div>
         <div class="px-l">
             @include('form.custom-checkbox', [
-                'name' =>  'permissions[' . $role->id . '][delete]',
+                'name' =>  'permissions[' . $modelType . '][' . $modelId . '][delete]',
                 'label' => trans('common.delete'),
                 'value' => 'true',
                 'checked' => $permission->delete,
@@ -74,12 +77,13 @@ $inheriting - Boolean if the current row should be marked as inheriting default
             ])
         </div>
     </div>
-    @if($role->id !== 0)
+    @if($modelType !== 'fallback')
         <div class="flex-container-row items-center px-m py-s">
             <button type="button"
                     class="text-neg p-m icon-button"
-                    data-role-id="{{ $role->id }}"
-                    data-role-name="{{ $role->display_name }}"
+                    data-model-type="{{ $modelType }}"
+                    data-model-id="{{ $modelId }}"
+                    data-model-name="{{ $modelName }}"
                     title="{{ trans('common.remove') }}">
                 @icon('close') <span class="hide-over-m ml-xs">{{ trans('common.remove') }}</span>
             </button>
diff --git a/resources/views/form/entity-permissions.blade.php b/resources/views/form/entity-permissions.blade.php
index 9bf309fb8..f56740562 100644
--- a/resources/views/form/entity-permissions.blade.php
+++ b/resources/views/form/entity-permissions.blade.php
@@ -39,7 +39,10 @@
         @foreach($data->permissionsWithRoles() as $permission)
             @include('form.entity-permissions-row', [
                 'permission' => $permission,
-                'role' => $permission->role,
+                'modelType' => 'role',
+                'modelId' => $permission->role->id,
+                'modelName' => $permission->role->display_name,
+                'modelDescription' => $permission->role->description,
                 'entityType' => $model->getType(),
                 'inheriting' => false,
             ])
@@ -60,10 +63,13 @@
 
     <div class="item-list mt-m mb-xl">
         @include('form.entity-permissions-row', [
-                'role' => $data->everyoneElseRole(),
+                'modelType' => 'fallback',
+                'modelId' => 0,
+                'modelName' => trans('entities.permissions_role_everyone_else'),
+                'modelDescription' => trans('entities.permissions_role_everyone_else_desc'),
                 'permission' => $data->everyoneElseEntityPermission(),
                 'entityType' => $model->getType(),
-                'inheriting' => !$model->permissions()->where('role_id', '=', 0)->exists(),
+                'inheriting' => $data->everyoneElseInheriting(),
             ])
     </div>
 
diff --git a/tests/Helpers/EntityProvider.php b/tests/Helpers/EntityProvider.php
index 9e8cf0b73..d285e6b78 100644
--- a/tests/Helpers/EntityProvider.php
+++ b/tests/Helpers/EntityProvider.php
@@ -208,7 +208,7 @@ class EntityProvider
 
         $permissions = [
             // Set default permissions to not allow actions so that only the provided role permissions are at play.
-            ['role_id' => 0, 'view' => false, 'create' => false, 'update' => false, 'delete' => false],
+            ['role_id' => null, 'view' => false, 'create' => false, 'update' => false, 'delete' => false],
         ];
 
         foreach ($roles as $role) {
diff --git a/tests/Permissions/EntityPermissionsTest.php b/tests/Permissions/EntityPermissionsTest.php
index 4b613b49c..9b2a8db86 100644
--- a/tests/Permissions/EntityPermissionsTest.php
+++ b/tests/Permissions/EntityPermissionsTest.php
@@ -378,8 +378,10 @@ class EntityPermissionsTest extends TestCase
 
         $this->put($modelInstance->getUrl('/permissions'), [
             'permissions' => [
-                $roleId => [
-                    $permission => 'true',
+                'role' => [
+                    $roleId => [
+                        $permission => 'true',
+                    ],
                 ],
             ],
         ]);