From 1660e72cc5f3420bb704a8f159a94f0632d0b25c Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Tue, 24 Jan 2023 19:04:32 +0000 Subject: [PATCH] Migrated remaining relation permission usages Now all tests are passing. Some level of manual checks to do. --- app/Auth/Permissions/PermissionApplicator.php | 51 +++---------------- app/Auth/User.php | 1 + app/Uploads/Attachment.php | 8 +++ app/Uploads/Image.php | 8 +++ .../RegeneratePermissionsCommandTest.php | 2 +- tests/Permissions/EntityPermissionsTest.php | 5 +- 6 files changed, 29 insertions(+), 46 deletions(-) diff --git a/app/Auth/Permissions/PermissionApplicator.php b/app/Auth/Permissions/PermissionApplicator.php index 4f95465af..437ddb0fb 100644 --- a/app/Auth/Permissions/PermissionApplicator.php +++ b/app/Auth/Permissions/PermissionApplicator.php @@ -150,51 +150,16 @@ class PermissionApplicator */ public function restrictPageRelationQuery(Builder $query, string $tableName, string $pageIdColumn): Builder { - // TODO - Refactor $fullPageIdColumn = $tableName . '.' . $pageIdColumn; - $morphClass = (new Page())->getMorphClass(); - - $existsQuery = function ($permissionQuery) use ($fullPageIdColumn, $morphClass) { - /** @var Builder $permissionQuery */ - $permissionQuery->select('joint_permissions.role_id')->from('joint_permissions') - ->whereColumn('joint_permissions.entity_id', '=', $fullPageIdColumn) - ->where('joint_permissions.entity_type', '=', $morphClass) - ->whereIn('joint_permissions.role_id', $this->getCurrentUserRoleIds()) - ->where(function (QueryBuilder $query) { - $this->addJointHasPermissionCheck($query, $this->currentUser()->id); + return $this->restrictEntityQuery($query) + ->where(function ($query) use ($fullPageIdColumn) { + /** @var Builder $query */ + $query->whereExists(function (QueryBuilder $query) use ($fullPageIdColumn) { + $query->select('id')->from('pages') + ->whereColumn('pages.id', '=', $fullPageIdColumn) + ->where('pages.draft', '=', false); }); - }; - - $q = $query->where(function ($query) use ($existsQuery, $fullPageIdColumn) { - $query->whereExists($existsQuery) - ->orWhere($fullPageIdColumn, '=', 0); - }); - - // Prevent visibility of non-owned draft pages - $q->whereExists(function (QueryBuilder $query) use ($fullPageIdColumn) { - $query->select('id')->from('pages') - ->whereColumn('pages.id', '=', $fullPageIdColumn) - ->where(function (QueryBuilder $query) { - $query->where('pages.draft', '=', false) - ->orWhere('pages.owned_by', '=', $this->currentUser()->id); - }); - }); - - return $q; - } - - /** - * Add the query for checking the given user id has permission - * within the join_permissions table. - * - * @param QueryBuilder|Builder $query - */ - protected function addJointHasPermissionCheck($query, int $userIdToCheck) - { - $query->where('joint_permissions.has_permission', '=', true)->orWhere(function ($query) use ($userIdToCheck) { - $query->where('joint_permissions.has_permission_own', '=', true) - ->where('joint_permissions.owned_by', '=', $userIdToCheck); - }); + }); } /** diff --git a/app/Auth/User.php b/app/Auth/User.php index 6e66bc808..cf9f20e52 100644 --- a/app/Auth/User.php +++ b/app/Auth/User.php @@ -200,6 +200,7 @@ class User extends Model implements AuthenticatableContract, CanResetPasswordCon public function attachRole(Role $role) { $this->roles()->attach($role->id); + $this->unsetRelation('roles'); } /** diff --git a/app/Uploads/Attachment.php b/app/Uploads/Attachment.php index 6c7066ff9..fc86d36ea 100644 --- a/app/Uploads/Attachment.php +++ b/app/Uploads/Attachment.php @@ -2,6 +2,7 @@ namespace BookStack\Uploads; +use BookStack\Auth\Permissions\JointPermission; use BookStack\Auth\Permissions\PermissionApplicator; use BookStack\Auth\User; use BookStack\Entities\Models\Entity; @@ -10,6 +11,7 @@ use BookStack\Model; use BookStack\Traits\HasCreatorAndUpdater; use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Relations\BelongsTo; +use Illuminate\Database\Eloquent\Relations\HasMany; /** * @property int $id @@ -56,6 +58,12 @@ class Attachment extends Model return $this->belongsTo(Page::class, 'uploaded_to'); } + public function jointPermissions(): HasMany + { + return $this->hasMany(JointPermission::class, 'entity_id', 'uploaded_to') + ->where('joint_permissions.entity_type', '=', 'page'); + } + /** * Get the url of this file. */ diff --git a/app/Uploads/Image.php b/app/Uploads/Image.php index bdf10f080..c21a3b03f 100644 --- a/app/Uploads/Image.php +++ b/app/Uploads/Image.php @@ -2,10 +2,12 @@ namespace BookStack\Uploads; +use BookStack\Auth\Permissions\JointPermission; use BookStack\Entities\Models\Page; use BookStack\Model; use BookStack\Traits\HasCreatorAndUpdater; use Illuminate\Database\Eloquent\Factories\HasFactory; +use Illuminate\Database\Eloquent\Relations\HasMany; /** * @property int $id @@ -25,6 +27,12 @@ class Image extends Model protected $fillable = ['name']; protected $hidden = []; + public function jointPermissions(): HasMany + { + return $this->hasMany(JointPermission::class, 'entity_id', 'uploaded_to') + ->where('joint_permissions.entity_type', '=', 'page'); + } + /** * Get a thumbnail for this image. * diff --git a/tests/Commands/RegeneratePermissionsCommandTest.php b/tests/Commands/RegeneratePermissionsCommandTest.php index b916a8060..9cf7dec93 100644 --- a/tests/Commands/RegeneratePermissionsCommandTest.php +++ b/tests/Commands/RegeneratePermissionsCommandTest.php @@ -29,7 +29,7 @@ class RegeneratePermissionsCommandTest extends TestCase 'entity_id' => $page->id, 'entity_type' => 'page', 'role_id' => $role->id, - 'has_permission' => 1, + 'status' => 3, // Explicit allow ]); $page->permissions()->delete(); diff --git a/tests/Permissions/EntityPermissionsTest.php b/tests/Permissions/EntityPermissionsTest.php index ab8b1242d..99a8bd88c 100644 --- a/tests/Permissions/EntityPermissionsTest.php +++ b/tests/Permissions/EntityPermissionsTest.php @@ -663,7 +663,7 @@ class EntityPermissionsTest extends TestCase $chapter = $this->entities->chapter(); $book = $chapter->book; - $this->permissions->setEntityPermissions($book, ['edit'], [$viewerRole], false); + $this->permissions->setEntityPermissions($book, ['update'], [$viewerRole], false); $this->permissions->setEntityPermissions($chapter, [], [$viewerRole], true); $this->assertFalse(userCan('chapter-update', $chapter)); @@ -678,9 +678,10 @@ class EntityPermissionsTest extends TestCase $chapter = $this->entities->chapter(); $book = $chapter->book; - $this->permissions->setEntityPermissions($book, ['edit'], [$editorRole], false); + $this->permissions->setEntityPermissions($book, ['update'], [$editorRole], false); $this->permissions->setEntityPermissions($chapter, [], [$viewerRole], true); + $this->actingAs($user); $this->assertTrue(userCan('chapter-update', $chapter)); }