From c8716df284e74fad780ed8d36b23669d725b164a Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Mon, 8 Sep 2025 15:59:25 +0100 Subject: [PATCH] Permissions: Removed unused role-perm columns, added permission enum Updated main permission check methods to support our new enum. --- app/App/helpers.php | 3 +- app/Entities/Tools/PermissionsUpdater.php | 11 +- app/Permissions/Models/EntityPermission.php | 2 - app/Permissions/Models/RolePermission.php | 1 - app/Permissions/Permission.php | 151 ++++++++++++++++++ app/Permissions/PermissionApplicator.php | 16 +- app/Users/Models/Role.php | 3 +- app/Users/Models/User.php | 21 ++- ...25_09_02_111542_remove_unused_columns.php} | 5 + tests/Helpers/PermissionsProvider.php | 5 +- 10 files changed, 190 insertions(+), 28 deletions(-) create mode 100644 app/Permissions/Permission.php rename database/migrations/{2025_09_02_111542_remove_comments_text_column.php => 2025_09_02_111542_remove_unused_columns.php} (76%) diff --git a/app/App/helpers.php b/app/App/helpers.php index 2305c2d72..0e26d9974 100644 --- a/app/App/helpers.php +++ b/app/App/helpers.php @@ -3,6 +3,7 @@ use BookStack\App\AppVersion; use BookStack\App\Model; use BookStack\Facades\Theme; +use BookStack\Permissions\Permission; use BookStack\Permissions\PermissionApplicator; use BookStack\Settings\SettingService; use BookStack\Users\Models\User; @@ -39,7 +40,7 @@ function user(): User * Check if the current user has a permission. If an ownable element * is passed in the jointPermissions are checked against that particular item. */ -function userCan(string $permission, ?Model $ownable = null): bool +function userCan(string|Permission $permission, ?Model $ownable = null): bool { if (is_null($ownable)) { return user()->can($permission); diff --git a/app/Entities/Tools/PermissionsUpdater.php b/app/Entities/Tools/PermissionsUpdater.php index 9f3b8f952..4ca53982a 100644 --- a/app/Entities/Tools/PermissionsUpdater.php +++ b/app/Entities/Tools/PermissionsUpdater.php @@ -8,6 +8,7 @@ use BookStack\Entities\Models\Bookshelf; use BookStack\Entities\Models\Entity; use BookStack\Facades\Activity; use BookStack\Permissions\Models\EntityPermission; +use BookStack\Permissions\Permission; use BookStack\Users\Models\Role; use BookStack\Users\Models\User; use Illuminate\Http\Request; @@ -93,8 +94,9 @@ class PermissionsUpdater foreach ($permissions as $roleId => $info) { $entityPermissionData = ['role_id' => $roleId]; - foreach (EntityPermission::PERMISSIONS as $permission) { - $entityPermissionData[$permission] = (($info[$permission] ?? false) === "true"); + foreach (Permission::genericForEntity() as $permission) { + $permName = $permission->value; + $entityPermissionData[$permName] = (($info[$permName] ?? false) === "true"); } $formatted[] = $entityPermissionData; } @@ -108,8 +110,9 @@ class PermissionsUpdater foreach ($permissions as $requestPermissionData) { $entityPermissionData = ['role_id' => $requestPermissionData['role_id']]; - foreach (EntityPermission::PERMISSIONS as $permission) { - $entityPermissionData[$permission] = boolval($requestPermissionData[$permission] ?? false); + foreach (Permission::genericForEntity() as $permission) { + $permName = $permission->value; + $entityPermissionData[$permName] = boolval($requestPermissionData[$permName] ?? false); } $formatted[] = $entityPermissionData; } diff --git a/app/Permissions/Models/EntityPermission.php b/app/Permissions/Models/EntityPermission.php index 1ef9c2886..efad2ba39 100644 --- a/app/Permissions/Models/EntityPermission.php +++ b/app/Permissions/Models/EntityPermission.php @@ -18,8 +18,6 @@ use Illuminate\Database\Eloquent\Relations\BelongsTo; */ class EntityPermission extends Model { - public const PERMISSIONS = ['view', 'create', 'update', 'delete']; - protected $fillable = ['role_id', 'view', 'create', 'update', 'delete']; public $timestamps = false; protected $hidden = ['entity_id', 'entity_type', 'id']; diff --git a/app/Permissions/Models/RolePermission.php b/app/Permissions/Models/RolePermission.php index d43313ea0..67b1c55f5 100644 --- a/app/Permissions/Models/RolePermission.php +++ b/app/Permissions/Models/RolePermission.php @@ -9,7 +9,6 @@ use Illuminate\Database\Eloquent\Relations\BelongsToMany; /** * @property int $id * @property string $name - * @property string $display_name */ class RolePermission extends Model { diff --git a/app/Permissions/Permission.php b/app/Permissions/Permission.php new file mode 100644 index 000000000..539bbe808 --- /dev/null +++ b/app/Permissions/Permission.php @@ -0,0 +1,151 @@ +value; + $explodedPermission = explode('-', $permissionName); $action = $explodedPermission[1] ?? $explodedPermission[0]; - $fullPermission = count($explodedPermission) > 1 ? $permission : $ownable->getMorphClass() . '-' . $permission; + $fullPermission = count($explodedPermission) > 1 ? $permissionName : $ownable->getMorphClass() . '-' . $permissionName; $user = $this->currentUser(); $userRoleIds = $this->getCurrentUserRoleIds(); @@ -235,8 +236,13 @@ class PermissionApplicator */ protected function ensureValidEntityAction(string $action): void { - if (!in_array($action, EntityPermission::PERMISSIONS)) { - throw new InvalidArgumentException('Action should be a simple entity permission action, not a role permission'); + $allowed = Permission::genericForEntity(); + foreach ($allowed as $permission) { + if ($permission->value === $action) { + return; + } } + + throw new InvalidArgumentException('Action should be a simple entity permission action, not a role permission'); } } diff --git a/app/Users/Models/Role.php b/app/Users/Models/Role.php index 912ae81c9..92be22458 100644 --- a/app/Users/Models/Role.php +++ b/app/Users/Models/Role.php @@ -57,7 +57,8 @@ class Role extends Model implements Loggable */ public function permissions(): BelongsToMany { - return $this->belongsToMany(RolePermission::class, 'permission_role', 'role_id', 'permission_id'); + return $this->belongsToMany(RolePermission::class, 'permission_role', 'role_id', 'permission_id') + ->select(['id', 'name']); } /** diff --git a/app/Users/Models/User.php b/app/Users/Models/User.php index f83e12088..0017653b5 100644 --- a/app/Users/Models/User.php +++ b/app/Users/Models/User.php @@ -12,6 +12,7 @@ use BookStack\Api\ApiToken; use BookStack\App\Model; use BookStack\App\SluggableInterface; use BookStack\Entities\Tools\SlugGenerator; +use BookStack\Permissions\Permission; use BookStack\Translation\LocaleDefinition; use BookStack\Translation\LocaleManager; use BookStack\Uploads\Image; @@ -26,7 +27,6 @@ use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Relations\BelongsTo; use Illuminate\Database\Eloquent\Relations\BelongsToMany; use Illuminate\Database\Eloquent\Relations\HasMany; -use Illuminate\Database\Eloquent\Relations\Relation; use Illuminate\Notifications\Notifiable; use Illuminate\Support\Collection; @@ -156,8 +156,9 @@ class User extends Model implements AuthenticatableContract, CanResetPasswordCon /** * Check if the user has a particular permission. */ - public function can(string $permissionName): bool + public function can(string|Permission $permission): bool { + $permissionName = is_string($permission) ? $permission : $permission->value; return $this->permissions()->contains($permissionName); } @@ -181,9 +182,9 @@ class User extends Model implements AuthenticatableContract, CanResetPasswordCon } /** - * Clear any cached permissions on this instance. + * Clear any cached permissions in this instance. */ - public function clearPermissionCache() + public function clearPermissionCache(): void { $this->permissions = null; } @@ -191,7 +192,7 @@ class User extends Model implements AuthenticatableContract, CanResetPasswordCon /** * Attach a role to this user. */ - public function attachRole(Role $role) + public function attachRole(Role $role): void { $this->roles()->attach($role->id); $this->unsetRelation('roles'); @@ -207,15 +208,11 @@ class User extends Model implements AuthenticatableContract, CanResetPasswordCon /** * Check if the user has a social account, - * If a driver is passed it checks for that single account type. - * - * @param bool|string $socialDriver - * - * @return bool + * If a driver is passed, it checks for that single account type. */ - public function hasSocialAccount($socialDriver = false) + public function hasSocialAccount(string $socialDriver = ''): bool { - if ($socialDriver === false) { + if (empty($socialDriver)) { return $this->socialAccounts()->count() > 0; } diff --git a/database/migrations/2025_09_02_111542_remove_comments_text_column.php b/database/migrations/2025_09_02_111542_remove_unused_columns.php similarity index 76% rename from database/migrations/2025_09_02_111542_remove_comments_text_column.php rename to database/migrations/2025_09_02_111542_remove_unused_columns.php index 8caa05e54..1ec2dfdf9 100644 --- a/database/migrations/2025_09_02_111542_remove_comments_text_column.php +++ b/database/migrations/2025_09_02_111542_remove_unused_columns.php @@ -14,6 +14,11 @@ return new class extends Migration Schema::table('comments', function (Blueprint $table) { $table->dropColumn('text'); }); + + Schema::table('role_permissions', function (Blueprint $table) { + $table->dropColumn('display_name'); + $table->dropColumn('description'); + }); } /** diff --git a/tests/Helpers/PermissionsProvider.php b/tests/Helpers/PermissionsProvider.php index cb036fe97..c9ae30919 100644 --- a/tests/Helpers/PermissionsProvider.php +++ b/tests/Helpers/PermissionsProvider.php @@ -5,6 +5,7 @@ namespace Tests\Helpers; use BookStack\Entities\Models\Entity; use BookStack\Permissions\Models\EntityPermission; use BookStack\Permissions\Models\RolePermission; +use BookStack\Permissions\Permission; use BookStack\Settings\SettingService; use BookStack\Users\Models\Role; use BookStack\Users\Models\User; @@ -139,8 +140,8 @@ class PermissionsProvider protected function actionListToEntityPermissionData(array $actionList, int $roleId = 0): array { $permissionData = ['role_id' => $roleId]; - foreach (EntityPermission::PERMISSIONS as $possibleAction) { - $permissionData[$possibleAction] = in_array($possibleAction, $actionList); + foreach (Permission::genericForEntity() as $permission) { + $permissionData[$permission->value] = in_array($permission->value, $actionList); } return $permissionData;