From 2d1f1abce4a6372b6be1833d88354149cbc7e40c Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Tue, 24 Jan 2023 14:55:34 +0000 Subject: [PATCH] Implemented alternate approach to current joint_permissions Is a tweak upon the existing approach, mainly to store and query role permission access in a way that allows muli-level states that may override eachother. These states are represented in the new PermissionStatus class. This also simplifies how own permissions are stored and queried, to be part of a single column. --- app/Actions/Activity.php | 8 +++ app/Actions/Favourite.php | 8 +++ app/Actions/Tag.php | 8 +++ app/Actions/View.php | 8 +++ .../Permissions/JointPermissionBuilder.php | 23 ++++---- app/Auth/Permissions/PermissionApplicator.php | 33 ++++-------- app/Auth/Permissions/PermissionStatus.php | 11 ++++ app/References/Reference.php | 8 +++ app/References/ReferenceFetcher.php | 17 +++--- ...625_refactor_joint_permissions_storage.php | 52 +++++++++++++++++++ 10 files changed, 137 insertions(+), 39 deletions(-) create mode 100644 app/Auth/Permissions/PermissionStatus.php create mode 100644 database/migrations/2023_01_24_104625_refactor_joint_permissions_storage.php diff --git a/app/Actions/Activity.php b/app/Actions/Activity.php index 3b1408cb9..0789fe123 100644 --- a/app/Actions/Activity.php +++ b/app/Actions/Activity.php @@ -2,10 +2,12 @@ namespace BookStack\Actions; +use BookStack\Auth\Permissions\JointPermission; use BookStack\Auth\User; use BookStack\Entities\Models\Entity; use BookStack\Model; use Illuminate\Database\Eloquent\Relations\BelongsTo; +use Illuminate\Database\Eloquent\Relations\HasMany; use Illuminate\Database\Eloquent\Relations\MorphTo; use Illuminate\Support\Str; @@ -40,6 +42,12 @@ class Activity extends Model return $this->belongsTo(User::class); } + public function jointPermissions(): HasMany + { + return $this->hasMany(JointPermission::class, 'entity_id', 'entity_id') + ->whereColumn('activities.entity_type', '=', 'joint_permissions.entity_type'); + } + /** * Returns text from the language files, Looks up by using the activity key. */ diff --git a/app/Actions/Favourite.php b/app/Actions/Favourite.php index f45894182..c5d12a151 100644 --- a/app/Actions/Favourite.php +++ b/app/Actions/Favourite.php @@ -2,7 +2,9 @@ namespace BookStack\Actions; +use BookStack\Auth\Permissions\JointPermission; use BookStack\Model; +use Illuminate\Database\Eloquent\Relations\HasMany; use Illuminate\Database\Eloquent\Relations\MorphTo; class Favourite extends Model @@ -16,4 +18,10 @@ class Favourite extends Model { return $this->morphTo(); } + + public function jointPermissions(): HasMany + { + return $this->hasMany(JointPermission::class, 'entity_id', 'favouritable_id') + ->whereColumn('favourites.favouritable_type', '=', 'joint_permissions.entity_type'); + } } diff --git a/app/Actions/Tag.php b/app/Actions/Tag.php index 609c299ad..e173faea0 100644 --- a/app/Actions/Tag.php +++ b/app/Actions/Tag.php @@ -2,8 +2,10 @@ namespace BookStack\Actions; +use BookStack\Auth\Permissions\JointPermission; use BookStack\Model; use Illuminate\Database\Eloquent\Factories\HasFactory; +use Illuminate\Database\Eloquent\Relations\HasMany; use Illuminate\Database\Eloquent\Relations\MorphTo; /** @@ -27,6 +29,12 @@ class Tag extends Model return $this->morphTo('entity'); } + public function jointPermissions(): HasMany + { + return $this->hasMany(JointPermission::class, 'entity_id', 'entity_id') + ->whereColumn('tags.entity_type', '=', 'joint_permissions.entity_type'); + } + /** * Get a full URL to start a tag name search for this tag name. */ diff --git a/app/Actions/View.php b/app/Actions/View.php index 16961bd91..706467133 100644 --- a/app/Actions/View.php +++ b/app/Actions/View.php @@ -2,8 +2,10 @@ namespace BookStack\Actions; +use BookStack\Auth\Permissions\JointPermission; use BookStack\Interfaces\Viewable; use BookStack\Model; +use Illuminate\Database\Eloquent\Relations\HasMany; use Illuminate\Database\Eloquent\Relations\MorphTo; /** @@ -28,6 +30,12 @@ class View extends Model return $this->morphTo(); } + public function jointPermissions(): HasMany + { + return $this->hasMany(JointPermission::class, 'entity_id', 'viewable_id') + ->whereColumn('views.viewable_type', '=', 'joint_permissions.entity_type'); + } + /** * Increment the current user's view count for the given viewable model. */ diff --git a/app/Auth/Permissions/JointPermissionBuilder.php b/app/Auth/Permissions/JointPermissionBuilder.php index 91207e3ab..bbdf4d6f8 100644 --- a/app/Auth/Permissions/JointPermissionBuilder.php +++ b/app/Auth/Permissions/JointPermissionBuilder.php @@ -256,35 +256,38 @@ class JointPermissionBuilder { // Ensure system admin role retains permissions if ($isAdminRole) { - return $this->createJointPermissionDataArray($entity, $roleId, true, true); + return $this->createJointPermissionDataArray($entity, $roleId, PermissionStatus::EXPLICIT_ALLOW, true); } // Return evaluated entity permission status if it has an affect. $entityPermissionStatus = $permissionMap->evaluateEntityForRole($entity, $roleId); if ($entityPermissionStatus !== null) { - return $this->createJointPermissionDataArray($entity, $roleId, $entityPermissionStatus, $entityPermissionStatus); + $status = $entityPermissionStatus ? PermissionStatus::EXPLICIT_ALLOW : PermissionStatus::EXPLICIT_DENY; + return $this->createJointPermissionDataArray($entity, $roleId, $status, $entityPermissionStatus); } // Otherwise default to the role-level permissions $permissionPrefix = $entity->type . '-view'; $roleHasPermission = isset($rolePermissionMap[$roleId . ':' . $permissionPrefix . '-all']); $roleHasPermissionOwn = isset($rolePermissionMap[$roleId . ':' . $permissionPrefix . '-own']); - return $this->createJointPermissionDataArray($entity, $roleId, $roleHasPermission, $roleHasPermissionOwn); + $status = $roleHasPermission ? PermissionStatus::IMPLICIT_ALLOW : PermissionStatus::IMPLICIT_DENY; + return $this->createJointPermissionDataArray($entity, $roleId, $status, $roleHasPermissionOwn); } /** * Create an array of data with the information of an entity jointPermissions. * Used to build data for bulk insertion. */ - protected function createJointPermissionDataArray(SimpleEntityData $entity, int $roleId, bool $permissionAll, bool $permissionOwn): array + protected function createJointPermissionDataArray(SimpleEntityData $entity, int $roleId, int $permissionStatus, bool $hasPermissionOwn): array { + $ownPermissionActive = ($hasPermissionOwn && $permissionStatus !== PermissionStatus::EXPLICIT_DENY && $entity->owned_by); + return [ - 'entity_id' => $entity->id, - 'entity_type' => $entity->type, - 'has_permission' => $permissionAll, - 'has_permission_own' => $permissionOwn, - 'owned_by' => $entity->owned_by, - 'role_id' => $roleId, + 'entity_id' => $entity->id, + 'entity_type' => $entity->type, + 'role_id' => $roleId, + 'status' => $permissionStatus, + 'owner_id' => $ownPermissionActive ? $entity->owned_by : null, ]; } } diff --git a/app/Auth/Permissions/PermissionApplicator.php b/app/Auth/Permissions/PermissionApplicator.php index e4564ddf5..4f95465af 100644 --- a/app/Auth/Permissions/PermissionApplicator.php +++ b/app/Auth/Permissions/PermissionApplicator.php @@ -95,13 +95,11 @@ class PermissionApplicator return $query->where(function (Builder $parentQuery) { $parentQuery->whereHas('jointPermissions', function (Builder $permissionQuery) { $permissionQuery->select(['entity_id', 'entity_type']) - ->selectRaw('max(owned_by) as owned_by') - ->selectRaw('max(has_permission) as has_permission') - ->selectRaw('max(has_permission_own) as has_permission_own') + ->selectRaw('max(owner_id) as owner_id') + ->selectRaw('max(status) as status') ->whereIn('role_id', $this->getCurrentUserRoleIds()) ->groupBy(['entity_type', 'entity_id']) - ->havingRaw('has_permission > 0') - ->orHavingRaw('(has_permission_own > 0 and owned_by = ?)', [$this->currentUser()->id]); + ->havingRaw('(status IN (1, 3) or owner_id = ?)', [$this->currentUser()->id]); }); }); } @@ -125,35 +123,23 @@ class PermissionApplicator * Filter items that have entities set as a polymorphic relation. * For simplicity, this will not return results attached to draft pages. * Draft pages should never really have related items though. - * - * @param Builder|QueryBuilder $query */ - public function restrictEntityRelationQuery($query, string $tableName, string $entityIdColumn, string $entityTypeColumn) + public function restrictEntityRelationQuery(Builder $query, string $tableName, string $entityIdColumn, string $entityTypeColumn): Builder { $tableDetails = ['tableName' => $tableName, 'entityIdColumn' => $entityIdColumn, 'entityTypeColumn' => $entityTypeColumn]; $pageMorphClass = (new Page())->getMorphClass(); - $q = $query->whereExists(function ($permissionQuery) use (&$tableDetails) { - /** @var Builder $permissionQuery */ - $permissionQuery->select(['role_id'])->from('joint_permissions') - ->whereColumn('joint_permissions.entity_id', '=', $tableDetails['tableName'] . '.' . $tableDetails['entityIdColumn']) - ->whereColumn('joint_permissions.entity_type', '=', $tableDetails['tableName'] . '.' . $tableDetails['entityTypeColumn']) - ->whereIn('joint_permissions.role_id', $this->getCurrentUserRoleIds()) - ->where(function (QueryBuilder $query) { - $this->addJointHasPermissionCheck($query, $this->currentUser()->id); - }); - })->where(function ($query) use ($tableDetails, $pageMorphClass) { - /** @var Builder $query */ - $query->where($tableDetails['entityTypeColumn'], '!=', $pageMorphClass) + return $this->restrictEntityQuery($query) + ->where(function ($query) use ($tableDetails, $pageMorphClass) { + /** @var Builder $query */ + $query->where($tableDetails['entityTypeColumn'], '!=', $pageMorphClass) ->orWhereExists(function (QueryBuilder $query) use ($tableDetails, $pageMorphClass) { $query->select('id')->from('pages') ->whereColumn('pages.id', '=', $tableDetails['tableName'] . '.' . $tableDetails['entityIdColumn']) ->where($tableDetails['tableName'] . '.' . $tableDetails['entityTypeColumn'], '=', $pageMorphClass) ->where('pages.draft', '=', false); }); - }); - - return $q; + }); } /** @@ -164,6 +150,7 @@ class PermissionApplicator */ public function restrictPageRelationQuery(Builder $query, string $tableName, string $pageIdColumn): Builder { + // TODO - Refactor $fullPageIdColumn = $tableName . '.' . $pageIdColumn; $morphClass = (new Page())->getMorphClass(); diff --git a/app/Auth/Permissions/PermissionStatus.php b/app/Auth/Permissions/PermissionStatus.php new file mode 100644 index 000000000..f8e55c20b --- /dev/null +++ b/app/Auth/Permissions/PermissionStatus.php @@ -0,0 +1,11 @@ +morphTo('to'); } + + public function jointPermissions(): HasMany + { + return $this->hasMany(JointPermission::class, 'entity_id', 'from_id') + ->whereColumn('references.from_type', '=', 'joint_permissions.entity_type'); + } } diff --git a/app/References/ReferenceFetcher.php b/app/References/ReferenceFetcher.php index a73463a95..415791857 100644 --- a/app/References/ReferenceFetcher.php +++ b/app/References/ReferenceFetcher.php @@ -5,6 +5,7 @@ namespace BookStack\References; use BookStack\Auth\Permissions\PermissionApplicator; use BookStack\Entities\Models\Entity; use BookStack\Entities\Models\Page; +use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Collection; use Illuminate\Database\Eloquent\Relations\Relation; @@ -23,8 +24,7 @@ class ReferenceFetcher */ public function getPageReferencesToEntity(Entity $entity): Collection { - $baseQuery = $entity->referencesTo() - ->where('from_type', '=', (new Page())->getMorphClass()) + $baseQuery = $this->queryPageReferencesToEntity($entity) ->with([ 'from' => fn (Relation $query) => $query->select(Page::$listAttributes), 'from.book' => fn (Relation $query) => $query->scopes('visible'), @@ -47,11 +47,8 @@ class ReferenceFetcher */ public function getPageReferenceCountToEntity(Entity $entity): int { - $baseQuery = $entity->referencesTo() - ->where('from_type', '=', (new Page())->getMorphClass()); - $count = $this->permissions->restrictEntityRelationQuery( - $baseQuery, + $this->queryPageReferencesToEntity($entity), 'references', 'from_id', 'from_type' @@ -59,4 +56,12 @@ class ReferenceFetcher return $count; } + + protected function queryPageReferencesToEntity(Entity $entity): Builder + { + return Reference::query() + ->where('to_type', '=', $entity->getMorphClass()) + ->where('to_id', '=', $entity->id) + ->where('from_type', '=', (new Page())->getMorphClass()); + } } diff --git a/database/migrations/2023_01_24_104625_refactor_joint_permissions_storage.php b/database/migrations/2023_01_24_104625_refactor_joint_permissions_storage.php new file mode 100644 index 000000000..49ebf5c5a --- /dev/null +++ b/database/migrations/2023_01_24_104625_refactor_joint_permissions_storage.php @@ -0,0 +1,52 @@ +truncate(); + + if (Schema::hasColumn('joint_permissions', 'owned_by')) { + Schema::table('joint_permissions', function (Blueprint $table) { + $table->dropColumn(['has_permission', 'has_permission_own', 'owned_by']); + + $table->unsignedTinyInteger('status')->index(); + $table->unsignedInteger('owner_id')->nullable()->index(); + }); + } + + // Rebuild permissions + app(JointPermissionBuilder::class)->rebuildForAll(); + } + + /** + * Reverse the migrations. + * + * @return void + */ + public function down() + { + DB::table('joint_permissions')->truncate(); + + Schema::table('joint_permissions', function (Blueprint $table) { + $table->dropColumn(['status', 'owner_id']); + + $table->boolean('has_permission')->index(); + $table->boolean('has_permission_own')->index(); + $table->unsignedInteger('created_by')->index(); + }); + } +}