From 451e4ac452f511073c8210cb5c7ec0c85d9a6072 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Fri, 23 Dec 2022 13:56:22 +0000 Subject: [PATCH] Fixed collapsed perm. gen for book sub-items. Also converted the existing "JointPermission" usage to the new collapsed permission system. --- app/Auth/Permissions/CollapsedPermission.php | 18 ++++++ ...der.php => CollapsedPermissionBuilder.php} | 58 ++++++++++--------- app/Auth/Permissions/JointPermission.php | 31 ---------- app/Auth/Permissions/JointUserPermission.php | 31 ---------- app/Auth/Permissions/PermissionsRepo.php | 8 +-- app/Auth/Permissions/SimpleEntityData.php | 1 - app/Auth/Role.php | 18 +++--- app/Auth/User.php | 18 ++++++ app/Auth/UserRepo.php | 2 + .../Commands/RegeneratePermissions.php | 6 +- app/Entities/Models/Entity.php | 21 ++----- app/Entities/Tools/TrashCan.php | 2 +- app/helpers.php | 5 +- ...reate_collapsed_role_permissions_table.php | 3 + database/seeders/DummyContentSeeder.php | 4 +- database/seeders/LargeContentSeeder.php | 4 +- .../RegeneratePermissionsCommandTest.php | 21 ++++--- tests/Helpers/PermissionsProvider.php | 2 - tests/PublicActionTest.php | 1 - 19 files changed, 115 insertions(+), 139 deletions(-) create mode 100644 app/Auth/Permissions/CollapsedPermission.php rename app/Auth/Permissions/{JointPermissionBuilder.php => CollapsedPermissionBuilder.php} (78%) delete mode 100644 app/Auth/Permissions/JointPermission.php delete mode 100644 app/Auth/Permissions/JointUserPermission.php diff --git a/app/Auth/Permissions/CollapsedPermission.php b/app/Auth/Permissions/CollapsedPermission.php new file mode 100644 index 000000000..7cdc9c999 --- /dev/null +++ b/app/Auth/Permissions/CollapsedPermission.php @@ -0,0 +1,18 @@ +bookFetchQuery()->chunk(5, function (EloquentCollection $books) { - $this->buildJointPermissionsForBooks($books); + $this->buildForBooks($books, false); }); // Chunk through all bookshelves Bookshelf::query()->withTrashed() - ->select(['id', 'owned_by']) + ->select(['id']) ->chunk(50, function (EloquentCollection $shelves) { $this->generateCollapsedPermissions($shelves->all()); }); } /** - * Rebuild the entity jointPermissions for a particular entity. + * Rebuild the collapsed permissions for a particular entity. */ public function rebuildForEntity(Entity $entity) { $entities = [$entity]; if ($entity instanceof Book) { $books = $this->bookFetchQuery()->where('id', '=', $entity->id)->get(); - $this->buildJointPermissionsForBooks($books, true); + $this->buildForBooks($books, true); return; } @@ -66,7 +66,7 @@ class JointPermissionBuilder } } - $this->buildJointPermissionsForEntities($entities); + $this->buildForEntities($entities); } /** @@ -75,20 +75,20 @@ class JointPermissionBuilder protected function bookFetchQuery(): Builder { return Book::query()->withTrashed() - ->select(['id', 'owned_by'])->with([ + ->select(['id'])->with([ 'chapters' => function ($query) { - $query->withTrashed()->select(['id', 'owned_by', 'book_id']); + $query->withTrashed()->select(['id', 'book_id']); }, 'pages' => function ($query) { - $query->withTrashed()->select(['id', 'owned_by', 'book_id', 'chapter_id']); + $query->withTrashed()->select(['id', 'book_id', 'chapter_id']); }, ]); } /** - * Build joint permissions for the given book and role combinations. + * Build collapsed permissions for the given books. */ - protected function buildJointPermissionsForBooks(EloquentCollection $books, bool $deleteOld = false) + protected function buildForBooks(EloquentCollection $books, bool $deleteOld) { $entities = clone $books; @@ -103,27 +103,27 @@ class JointPermissionBuilder } if ($deleteOld) { - $this->deleteManyJointPermissionsForEntities($entities->all()); + $this->deleteForEntities($entities->all()); } $this->generateCollapsedPermissions($entities->all()); } /** - * Rebuild the entity jointPermissions for a collection of entities. + * Rebuild the collapsed permissions for a collection of entities. */ - protected function buildJointPermissionsForEntities(array $entities) + protected function buildForEntities(array $entities) { - $this->deleteManyJointPermissionsForEntities($entities); + $this->deleteForEntities($entities); $this->generateCollapsedPermissions($entities); } /** - * Delete all the entity jointPermissions for a list of entities. + * Delete the stored collapsed permissions for a list of entities. * * @param Entity[] $entities */ - protected function deleteManyJointPermissionsForEntities(array $entities) + protected function deleteForEntities(array $entities) { $simpleEntities = $this->entitiesToSimpleEntities($entities); $idsByType = $this->entitiesToTypeIdMap($simpleEntities); @@ -141,6 +141,9 @@ class JointPermissionBuilder } /** + * Convert the given list of entities into "SimpleEntityData" representations + * for faster usage and property access. + * * @param Entity[] $entities * * @return SimpleEntityData[] @@ -154,7 +157,6 @@ class JointPermissionBuilder $simple = new SimpleEntityData(); $simple->id = $attrs['id']; $simple->type = $entity->getMorphClass(); - $simple->owned_by = $attrs['owned_by'] ?? 0; $simple->book_id = $attrs['book_id'] ?? null; $simple->chapter_id = $attrs['chapter_id'] ?? null; $simpleEntities[] = $simple; @@ -171,7 +173,7 @@ class JointPermissionBuilder protected function generateCollapsedPermissions(array $originalEntities) { $entities = $this->entitiesToSimpleEntities($originalEntities); - $jointPermissions = []; + $collapsedPermData = []; // Fetch related entity permissions $permissions = $this->getEntityPermissionsForEntities($entities); @@ -181,12 +183,12 @@ class JointPermissionBuilder // Create Joint Permission Data foreach ($entities as $entity) { - array_push($jointPermissions, ...$this->createCollapsedPermissionData($entity, $permissionMap)); + array_push($collapsedPermData, ...$this->createCollapsedPermissionData($entity, $permissionMap)); } - DB::transaction(function () use ($jointPermissions) { - foreach (array_chunk($jointPermissions, 1000) as $jointPermissionChunk) { - DB::table('entity_permissions_collapsed')->insert($jointPermissionChunk); + DB::transaction(function () use ($collapsedPermData) { + foreach (array_chunk($collapsedPermData, 1000) as $dataChunk) { + DB::table('entity_permissions_collapsed')->insert($dataChunk); } }); } @@ -198,8 +200,8 @@ class JointPermissionBuilder { $chain = [ $entity->type . ':' . $entity->id, - $entity->chapter_id ? null : ('chapter:' . $entity->chapter_id), - $entity->book_id ? null : ('book:' . $entity->book_id), + $entity->chapter_id ? ('chapter:' . $entity->chapter_id) : null, + $entity->book_id ? ('book:' . $entity->book_id) : null, ]; $permissionData = []; diff --git a/app/Auth/Permissions/JointPermission.php b/app/Auth/Permissions/JointPermission.php deleted file mode 100644 index e10c560d0..000000000 --- a/app/Auth/Permissions/JointPermission.php +++ /dev/null @@ -1,31 +0,0 @@ -belongsTo(Role::class); - } - - /** - * Get the entity this points to. - */ - public function entity(): MorphOne - { - return $this->morphOne(Entity::class, 'entity'); - } -} diff --git a/app/Auth/Permissions/JointUserPermission.php b/app/Auth/Permissions/JointUserPermission.php deleted file mode 100644 index 2987bf5e5..000000000 --- a/app/Auth/Permissions/JointUserPermission.php +++ /dev/null @@ -1,31 +0,0 @@ -morphOne(Entity::class, 'entity'); - } -} diff --git a/app/Auth/Permissions/PermissionsRepo.php b/app/Auth/Permissions/PermissionsRepo.php index 01a477a59..416253442 100644 --- a/app/Auth/Permissions/PermissionsRepo.php +++ b/app/Auth/Permissions/PermissionsRepo.php @@ -11,13 +11,13 @@ use Illuminate\Database\Eloquent\Collection; class PermissionsRepo { - protected JointPermissionBuilder $permissionBuilder; - protected $systemRoles = ['admin', 'public']; + protected CollapsedPermissionBuilder $permissionBuilder; + protected array $systemRoles = ['admin', 'public']; /** * PermissionsRepo constructor. */ - public function __construct(JointPermissionBuilder $permissionBuilder) + public function __construct(CollapsedPermissionBuilder $permissionBuilder) { $this->permissionBuilder = $permissionBuilder; } @@ -138,7 +138,7 @@ class PermissionsRepo } $role->entityPermissions()->delete(); - $role->jointPermissions()->delete(); + $role->collapsedPermissions()->delete(); Activity::add(ActivityType::ROLE_DELETE, $role); $role->delete(); } diff --git a/app/Auth/Permissions/SimpleEntityData.php b/app/Auth/Permissions/SimpleEntityData.php index 62f5984f8..75b23faaa 100644 --- a/app/Auth/Permissions/SimpleEntityData.php +++ b/app/Auth/Permissions/SimpleEntityData.php @@ -6,7 +6,6 @@ class SimpleEntityData { public int $id; public string $type; - public int $owned_by; public ?int $book_id; public ?int $chapter_id; } diff --git a/app/Auth/Role.php b/app/Auth/Role.php index b293d1af2..c9dadf4fe 100644 --- a/app/Auth/Role.php +++ b/app/Auth/Role.php @@ -2,8 +2,8 @@ namespace BookStack\Auth; +use BookStack\Auth\Permissions\CollapsedPermission; use BookStack\Auth\Permissions\EntityPermission; -use BookStack\Auth\Permissions\JointPermission; use BookStack\Auth\Permissions\RolePermission; use BookStack\Interfaces\Loggable; use BookStack\Model; @@ -39,14 +39,6 @@ class Role extends Model implements Loggable return $this->belongsToMany(User::class)->orderBy('name', 'asc'); } - /** - * Get all related JointPermissions. - */ - public function jointPermissions(): HasMany - { - return $this->hasMany(JointPermission::class); - } - /** * The RolePermissions that belong to the role. */ @@ -63,6 +55,14 @@ class Role extends Model implements Loggable return $this->hasMany(EntityPermission::class); } + /** + * Get all related entity collapsed permissions. + */ + public function collapsedPermissions(): HasMany + { + return $this->hasMany(CollapsedPermission::class); + } + /** * Check if this role has a permission. */ diff --git a/app/Auth/User.php b/app/Auth/User.php index 6e66bc808..56a089014 100644 --- a/app/Auth/User.php +++ b/app/Auth/User.php @@ -5,6 +5,8 @@ namespace BookStack\Auth; use BookStack\Actions\Favourite; use BookStack\Api\ApiToken; use BookStack\Auth\Access\Mfa\MfaValue; +use BookStack\Auth\Permissions\CollapsedPermission; +use BookStack\Auth\Permissions\EntityPermission; use BookStack\Entities\Tools\SlugGenerator; use BookStack\Interfaces\Loggable; use BookStack\Interfaces\Sluggable; @@ -298,6 +300,22 @@ class User extends Model implements AuthenticatableContract, CanResetPasswordCon }, 'activities', 'users.id', '=', 'activities.user_id'); } + /** + * Get the entity permissions assigned to this specific user. + */ + public function entityPermissions(): HasMany + { + return $this->hasMany(EntityPermission::class); + } + + /** + * Get all related entity collapsed permissions. + */ + public function collapsedPermissions(): HasMany + { + return $this->hasMany(CollapsedPermission::class); + } + /** * Get the url for editing this user. */ diff --git a/app/Auth/UserRepo.php b/app/Auth/UserRepo.php index 78bcb978e..1c9571b24 100644 --- a/app/Auth/UserRepo.php +++ b/app/Auth/UserRepo.php @@ -153,6 +153,8 @@ class UserRepo $user->apiTokens()->delete(); $user->favourites()->delete(); $user->mfaValues()->delete(); + $user->collapsedPermissions()->delete(); + $user->entityPermissions()->delete(); $user->delete(); // Delete user profile images diff --git a/app/Console/Commands/RegeneratePermissions.php b/app/Console/Commands/RegeneratePermissions.php index efb3535d6..2220b35a7 100644 --- a/app/Console/Commands/RegeneratePermissions.php +++ b/app/Console/Commands/RegeneratePermissions.php @@ -2,7 +2,7 @@ namespace BookStack\Console\Commands; -use BookStack\Auth\Permissions\JointPermissionBuilder; +use BookStack\Auth\Permissions\CollapsedPermissionBuilder; use Illuminate\Console\Command; use Illuminate\Support\Facades\DB; @@ -22,12 +22,12 @@ class RegeneratePermissions extends Command */ protected $description = 'Regenerate all system permissions'; - protected JointPermissionBuilder $permissionBuilder; + protected CollapsedPermissionBuilder $permissionBuilder; /** * Create a new command instance. */ - public function __construct(JointPermissionBuilder $permissionBuilder) + public function __construct(CollapsedPermissionBuilder $permissionBuilder) { $this->permissionBuilder = $permissionBuilder; parent::__construct(); diff --git a/app/Entities/Models/Entity.php b/app/Entities/Models/Entity.php index fc83bdd7e..2856d302f 100644 --- a/app/Entities/Models/Entity.php +++ b/app/Entities/Models/Entity.php @@ -7,10 +7,9 @@ use BookStack\Actions\Comment; use BookStack\Actions\Favourite; use BookStack\Actions\Tag; use BookStack\Actions\View; +use BookStack\Auth\Permissions\CollapsedPermission; use BookStack\Auth\Permissions\EntityPermission; -use BookStack\Auth\Permissions\JointPermission; -use BookStack\Auth\Permissions\JointPermissionBuilder; -use BookStack\Auth\Permissions\JointUserPermission; +use BookStack\Auth\Permissions\CollapsedPermissionBuilder; use BookStack\Auth\Permissions\PermissionApplicator; use BookStack\Entities\Tools\SlugGenerator; use BookStack\Interfaces\Deletable; @@ -188,19 +187,11 @@ abstract class Entity extends Model implements Sluggable, Favouritable, Viewable } /** - * Get the entity jointPermissions this is connected to. + * Get the entity collapsed permissions this is connected to. */ - public function jointPermissions(): MorphMany + public function collapsedPermissions(): MorphMany { - return $this->morphMany(JointPermission::class, 'entity'); - } - - /** - * Get the join user permissions for this entity. - */ - public function jointUserPermissions(): MorphMany - { - return $this->morphMany(JointUserPermission::class, 'entity'); + return $this->morphMany(CollapsedPermission::class, 'entity'); } /** @@ -301,7 +292,7 @@ abstract class Entity extends Model implements Sluggable, Favouritable, Viewable */ public function rebuildPermissions() { - app()->make(JointPermissionBuilder::class)->rebuildForEntity(clone $this); + app()->make(CollapsedPermissionBuilder::class)->rebuildForEntity(clone $this); } /** diff --git a/app/Entities/Tools/TrashCan.php b/app/Entities/Tools/TrashCan.php index 7341a0328..c9804d9e2 100644 --- a/app/Entities/Tools/TrashCan.php +++ b/app/Entities/Tools/TrashCan.php @@ -372,7 +372,7 @@ class TrashCan $entity->permissions()->delete(); $entity->tags()->delete(); $entity->comments()->delete(); - $entity->jointPermissions()->delete(); + $entity->collapsedPermissions()->delete(); $entity->searchTerms()->delete(); $entity->deletions()->delete(); $entity->favourites()->delete(); diff --git a/app/helpers.php b/app/helpers.php index 191eddf4d..eb4e07a66 100644 --- a/app/helpers.php +++ b/app/helpers.php @@ -55,8 +55,9 @@ function hasAppAccess(): bool } /** - * Check if the current user has a permission. If an ownable element - * is passed in the jointPermissions are checked against that particular item. + * Check if the current user has a permission. + * Checks a generic role permission or, if an ownable model is passed in, it will + * check against the given entity model, taking into account entity-level permissions. */ function userCan(string $permission, Model $ownable = null): bool { diff --git a/database/migrations/2022_12_22_103318_create_collapsed_role_permissions_table.php b/database/migrations/2022_12_22_103318_create_collapsed_role_permissions_table.php index 748a8809d..c35fe973d 100644 --- a/database/migrations/2022_12_22_103318_create_collapsed_role_permissions_table.php +++ b/database/migrations/2022_12_22_103318_create_collapsed_role_permissions_table.php @@ -13,6 +13,9 @@ class CreateCollapsedRolePermissionsTable extends Migration */ public function up() { + // TODO - Drop joint permissions + // TODO - Run collapsed table rebuild. + Schema::create('entity_permissions_collapsed', function (Blueprint $table) { $table->id(); $table->unsignedInteger('role_id')->nullable(); diff --git a/database/seeders/DummyContentSeeder.php b/database/seeders/DummyContentSeeder.php index f61e4e13d..b670728b3 100644 --- a/database/seeders/DummyContentSeeder.php +++ b/database/seeders/DummyContentSeeder.php @@ -3,7 +3,7 @@ namespace Database\Seeders; use BookStack\Api\ApiToken; -use BookStack\Auth\Permissions\JointPermissionBuilder; +use BookStack\Auth\Permissions\CollapsedPermissionBuilder; use BookStack\Auth\Permissions\RolePermission; use BookStack\Auth\Role; use BookStack\Auth\User; @@ -69,7 +69,7 @@ class DummyContentSeeder extends Seeder ]); $token->save(); - app(JointPermissionBuilder::class)->rebuildForAll(); + app(CollapsedPermissionBuilder::class)->rebuildForAll(); app(SearchIndex::class)->indexAllEntities(); } } diff --git a/database/seeders/LargeContentSeeder.php b/database/seeders/LargeContentSeeder.php index d0e65f974..720a03ed5 100644 --- a/database/seeders/LargeContentSeeder.php +++ b/database/seeders/LargeContentSeeder.php @@ -2,7 +2,7 @@ namespace Database\Seeders; -use BookStack\Auth\Permissions\JointPermissionBuilder; +use BookStack\Auth\Permissions\CollapsedPermissionBuilder; use BookStack\Auth\Role; use BookStack\Auth\User; use BookStack\Entities\Models\Book; @@ -35,7 +35,7 @@ class LargeContentSeeder extends Seeder $largeBook->chapters()->saveMany($chapters); $all = array_merge([$largeBook], array_values($pages->all()), array_values($chapters->all())); - app()->make(JointPermissionBuilder::class)->rebuildForEntity($largeBook); + app()->make(CollapsedPermissionBuilder::class)->rebuildForEntity($largeBook); app()->make(SearchIndex::class)->indexEntities($all); } } diff --git a/tests/Commands/RegeneratePermissionsCommandTest.php b/tests/Commands/RegeneratePermissionsCommandTest.php index d514e5f9d..cc53b460d 100644 --- a/tests/Commands/RegeneratePermissionsCommandTest.php +++ b/tests/Commands/RegeneratePermissionsCommandTest.php @@ -2,8 +2,7 @@ namespace Tests\Commands; -use BookStack\Auth\Permissions\JointPermission; -use BookStack\Entities\Models\Page; +use BookStack\Auth\Permissions\CollapsedPermission; use Illuminate\Support\Facades\Artisan; use Illuminate\Support\Facades\DB; use Tests\TestCase; @@ -13,15 +12,23 @@ class RegeneratePermissionsCommandTest extends TestCase public function test_regen_permissions_command() { DB::rollBack(); - JointPermission::query()->truncate(); - $page = Page::first(); + $page = $this->entities->page(); + $editor = $this->users->editor(); + $this->permissions->addEntityPermission($page, ['view'], null, $editor); + CollapsedPermission::query()->truncate(); - $this->assertDatabaseMissing('joint_permissions', ['entity_id' => $page->id]); + $this->assertDatabaseMissing('entity_permissions_collapsed', ['entity_id' => $page->id]); $exitCode = Artisan::call('bookstack:regenerate-permissions'); $this->assertTrue($exitCode === 0, 'Command executed successfully'); - DB::beginTransaction(); - $this->assertDatabaseHas('joint_permissions', ['entity_id' => $page->id]); + $this->assertDatabaseHas('entity_permissions_collapsed', [ + 'entity_id' => $page->id, + 'user_id' => $editor->id, + 'view' => 1, + ]); + + CollapsedPermission::query()->truncate(); + DB::beginTransaction(); } } diff --git a/tests/Helpers/PermissionsProvider.php b/tests/Helpers/PermissionsProvider.php index bebb5bada..ac9a2a68a 100644 --- a/tests/Helpers/PermissionsProvider.php +++ b/tests/Helpers/PermissionsProvider.php @@ -3,7 +3,6 @@ namespace Tests\Helpers; use BookStack\Auth\Permissions\EntityPermission; -use BookStack\Auth\Permissions\JointPermissionBuilder; use BookStack\Auth\Permissions\RolePermission; use BookStack\Auth\Role; use BookStack\Auth\User; @@ -70,7 +69,6 @@ class PermissionsProvider public function regenerateForEntity(Entity $entity): void { $entity->rebuildPermissions(); - $entity->load('jointPermissions'); } /** diff --git a/tests/PublicActionTest.php b/tests/PublicActionTest.php index 7df5bd422..afc7fcef3 100644 --- a/tests/PublicActionTest.php +++ b/tests/PublicActionTest.php @@ -2,7 +2,6 @@ namespace Tests; -use BookStack\Auth\Permissions\JointPermissionBuilder; use BookStack\Auth\Permissions\RolePermission; use BookStack\Auth\Role; use BookStack\Auth\User;