From c5e9dfa168f2f02106491779349663f9c3f5827b Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sun, 10 Jul 2022 13:45:04 +0100 Subject: [PATCH 01/12] Optimized pre-joint-permission logic efficiency --- app/Auth/Permissions/PermissionService.php | 108 +++++++++++++-------- 1 file changed, 67 insertions(+), 41 deletions(-) diff --git a/app/Auth/Permissions/PermissionService.php b/app/Auth/Permissions/PermissionService.php index 59ff37dc9..1363fe86e 100644 --- a/app/Auth/Permissions/PermissionService.php +++ b/app/Auth/Permissions/PermissionService.php @@ -37,7 +37,7 @@ class PermissionService protected $db; /** - * @var array + * @var array> */ protected $entityCache; @@ -68,10 +68,12 @@ class PermissionService foreach ($entities as $entity) { $class = get_class($entity); + if (!isset($this->entityCache[$class])) { - $this->entityCache[$class] = collect(); + $this->entityCache[$class] = []; } - $this->entityCache[$class]->put($entity->id, $entity); + + $this->entityCache[$class][$entity->getRawAttribute('id')] = $entity; } } @@ -80,8 +82,8 @@ class PermissionService */ protected function getBook(int $bookId): ?Book { - if (isset($this->entityCache[Book::class]) && $this->entityCache[Book::class]->has($bookId)) { - return $this->entityCache[Book::class]->get($bookId); + if ($this->entityCache[Book::class][$bookId] ?? false) { + return $this->entityCache[Book::class][$bookId]; } return Book::query()->withTrashed()->find($bookId); @@ -92,8 +94,8 @@ class PermissionService */ protected function getChapter(int $chapterId): ?Chapter { - if (isset($this->entityCache[Chapter::class]) && $this->entityCache[Chapter::class]->has($chapterId)) { - return $this->entityCache[Chapter::class]->get($chapterId); + if ($this->entityCache[Chapter::class][$chapterId] ?? false) { + return $this->entityCache[Chapter::class][$chapterId]; } return Chapter::query() @@ -206,7 +208,7 @@ class PermissionService $entities = [$entity]; if ($entity instanceof Book) { $books = $this->bookFetchQuery()->where('id', '=', $entity->id)->get(); - $this->buildJointPermissionsForBooks($books, Role::query()->get()->all(), true); + $this->buildJointPermissionsForBooks($books, Role::query()->with('permissions')->get()->all(), true); return; } @@ -270,7 +272,7 @@ class PermissionService } /** - * Delete all of the entity jointPermissions for a list of entities. + * Delete all the entity jointPermissions for a list of entities. * * @param Role[] $roles */ @@ -283,19 +285,7 @@ class PermissionService } /** - * Delete the entity jointPermissions for a particular entity. - * - * @param Entity $entity - * - * @throws Throwable - */ - public function deleteJointPermissionsForEntity(Entity $entity) - { - $this->deleteManyJointPermissionsForEntities([$entity]); - } - - /** - * Delete all of the entity jointPermissions for a list of entities. + * Delete all the entity jointPermissions for a list of entities. * * @param Entity[] $entities * @@ -303,20 +293,16 @@ class PermissionService */ protected function deleteManyJointPermissionsForEntities(array $entities) { - if (count($entities) === 0) { - return; - } + $idsByType = $this->entitiesToTypeIdMap($entities); - $this->db->transaction(function () use ($entities) { - foreach (array_chunk($entities, 1000) as $entityChunk) { - $query = $this->db->table('joint_permissions'); - foreach ($entityChunk as $entity) { - $query->orWhere(function (QueryBuilder $query) use ($entity) { - $query->where('entity_id', '=', $entity->id) - ->where('entity_type', '=', $entity->getMorphClass()); - }); + $this->db->transaction(function () use ($idsByType) { + foreach ($idsByType as $type => $ids) { + foreach (array_chunk($ids, 1000) as $idChunk) { + $this->db->table('joint_permissions') + ->where('entity_type', '=', $type) + ->whereIn('entity_id', $idChunk) + ->delete(); } - $query->delete(); } }); } @@ -334,16 +320,14 @@ class PermissionService $this->readyEntityCache($entities); $jointPermissions = []; - // Fetch Entity Permissions and create a mapping of entity restricted statuses + // Create a mapping of entity restricted statuses $entityRestrictedMap = []; - $permissionFetch = EntityPermission::query(); foreach ($entities as $entity) { - $entityRestrictedMap[$entity->getMorphClass() . ':' . $entity->id] = boolval($entity->getRawAttribute('restricted')); - $permissionFetch->orWhere(function ($query) use ($entity) { - $query->where('restrictable_id', '=', $entity->id)->where('restrictable_type', '=', $entity->getMorphClass()); - }); + $entityRestrictedMap[$entity->getMorphClass() . ':' . $entity->getRawAttribute('id')] = boolval($entity->getRawAttribute('restricted')); } - $permissions = $permissionFetch->get(); + + // Fetch related entity permissions + $permissions = $this->getEntityPermissionsForEntities($entities); // Create a mapping of explicit entity permissions $permissionMap = []; @@ -377,6 +361,48 @@ class PermissionService }); } + /** + * From the given entity list, provide back a mapping of entity types to + * the ids of that given type. The type used is the DB morph class. + * @param Entity[] $entities + * @return array + */ + protected function entitiesToTypeIdMap(array $entities): array + { + $idsByType = []; + + foreach ($entities as $entity) { + $type = $entity->getMorphClass(); + + if (!isset($idsByType[$type])) { + $idsByType[$type] = []; + } + + $idsByType[$type][] = $entity->getRawAttribute('id'); + } + + return $idsByType; + } + + /** + * Get the entity permissions for all the given entities + * @param Entity[] $entities + * @return EloquentCollection + */ + protected function getEntityPermissionsForEntities(array $entities) + { + $idsByType = $this->entitiesToTypeIdMap($entities); + $permissionFetch = EntityPermission::query(); + + foreach ($idsByType as $type => $ids) { + $permissionFetch->orWhere(function (Builder $query) use ($type, $ids) { + $query->where('restrictable_type', '=', $type)->whereIn('restrictable_id', $ids); + }); + } + + return $permissionFetch->get(); + } + /** * Get the actions related to an entity. */ From 2d4f708c7961f7ef7da3fd1d89d39a99581afa9e Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Tue, 12 Jul 2022 19:38:11 +0100 Subject: [PATCH 02/12] Extracted permission building out of permission service --- .../Permissions/JointPermissionBuilder.php | 423 ++++++++++++++++ app/Auth/Permissions/PermissionService.php | 452 ------------------ app/Auth/Permissions/PermissionsRepo.php | 39 +- .../Commands/RegeneratePermissions.php | 26 +- database/seeders/DummyContentSeeder.php | 4 +- database/seeders/LargeContentSeeder.php | 4 +- tests/PublicActionTest.php | 4 +- tests/SharedTestHelpers.php | 11 +- 8 files changed, 463 insertions(+), 500 deletions(-) create mode 100644 app/Auth/Permissions/JointPermissionBuilder.php diff --git a/app/Auth/Permissions/JointPermissionBuilder.php b/app/Auth/Permissions/JointPermissionBuilder.php new file mode 100644 index 000000000..c87560fe0 --- /dev/null +++ b/app/Auth/Permissions/JointPermissionBuilder.php @@ -0,0 +1,423 @@ +> + */ + protected $entityCache; + + /** + * Prepare the local entity cache and ensure it's empty. + * + * @param Entity[] $entities + */ + protected function readyEntityCache(array $entities = []) + { + $this->entityCache = []; + + foreach ($entities as $entity) { + $class = get_class($entity); + + if (!isset($this->entityCache[$class])) { + $this->entityCache[$class] = []; + } + + $this->entityCache[$class][$entity->getRawAttribute('id')] = $entity; + } + } + + /** + * Get a book via ID, Checks local cache. + */ + protected function getBook(int $bookId): ?Book + { + if ($this->entityCache[Book::class][$bookId] ?? false) { + return $this->entityCache[Book::class][$bookId]; + } + + return Book::query()->withTrashed()->find($bookId); + } + + /** + * Get a chapter via ID, Checks local cache. + */ + protected function getChapter(int $chapterId): ?Chapter + { + if ($this->entityCache[Chapter::class][$chapterId] ?? false) { + return $this->entityCache[Chapter::class][$chapterId]; + } + + return Chapter::query() + ->withTrashed() + ->find($chapterId); + } + + /** + * Re-generate all entity permission from scratch. + */ + public function buildJointPermissions() + { + JointPermission::query()->truncate(); + $this->readyEntityCache(); + + // Get all roles (Should be the most limited dimension) + $roles = Role::query()->with('permissions')->get()->all(); + + // Chunk through all books + $this->bookFetchQuery()->chunk(5, function (EloquentCollection $books) use ($roles) { + $this->buildJointPermissionsForBooks($books, $roles); + }); + + // Chunk through all bookshelves + Bookshelf::query()->withTrashed()->select(['id', 'restricted', 'owned_by']) + ->chunk(50, function (EloquentCollection $shelves) use ($roles) { + $this->buildJointPermissionsForShelves($shelves, $roles); + }); + } + + /** + * Get a query for fetching a book with it's children. + */ + protected function bookFetchQuery(): Builder + { + return Book::query()->withTrashed() + ->select(['id', 'restricted', 'owned_by'])->with([ + 'chapters' => function ($query) { + $query->withTrashed()->select(['id', 'restricted', 'owned_by', 'book_id']); + }, + 'pages' => function ($query) { + $query->withTrashed()->select(['id', 'restricted', 'owned_by', 'book_id', 'chapter_id']); + }, + ]); + } + + /** + * Build joint permissions for the given shelf and role combinations. + * + * @throws Throwable + */ + protected function buildJointPermissionsForShelves(EloquentCollection $shelves, array $roles, bool $deleteOld = false) + { + if ($deleteOld) { + $this->deleteManyJointPermissionsForEntities($shelves->all()); + } + $this->createManyJointPermissions($shelves->all(), $roles); + } + + /** + * Build joint permissions for the given book and role combinations. + * + * @throws Throwable + */ + protected function buildJointPermissionsForBooks(EloquentCollection $books, array $roles, bool $deleteOld = false) + { + $entities = clone $books; + + /** @var Book $book */ + foreach ($books->all() as $book) { + foreach ($book->getRelation('chapters') as $chapter) { + $entities->push($chapter); + } + foreach ($book->getRelation('pages') as $page) { + $entities->push($page); + } + } + + if ($deleteOld) { + $this->deleteManyJointPermissionsForEntities($entities->all()); + } + + $this->createManyJointPermissions($entities->all(), $roles); + } + + /** + * Rebuild the entity jointPermissions for a particular entity. + * + * @throws Throwable + */ + public function buildJointPermissionsForEntity(Entity $entity) + { + $entities = [$entity]; + if ($entity instanceof Book) { + $books = $this->bookFetchQuery()->where('id', '=', $entity->id)->get(); + $this->buildJointPermissionsForBooks($books, Role::query()->with('permissions')->get()->all(), true); + + return; + } + + /** @var BookChild $entity */ + if ($entity->book) { + $entities[] = $entity->book; + } + + if ($entity instanceof Page && $entity->chapter_id) { + $entities[] = $entity->chapter; + } + + if ($entity instanceof Chapter) { + foreach ($entity->pages as $page) { + $entities[] = $page; + } + } + + $this->buildJointPermissionsForEntities($entities); + } + + /** + * Rebuild the entity jointPermissions for a collection of entities. + * + * @throws Throwable + */ + protected function buildJointPermissionsForEntities(array $entities) + { + $roles = Role::query()->get()->values()->all(); + $this->deleteManyJointPermissionsForEntities($entities); + $this->createManyJointPermissions($entities, $roles); + } + + /** + * Build the entity jointPermissions for a particular role. + */ + public function buildJointPermissionForRole(Role $role) + { + $roles = [$role]; + $role->jointPermissions()->delete(); + + // Chunk through all books + $this->bookFetchQuery()->chunk(20, function ($books) use ($roles) { + $this->buildJointPermissionsForBooks($books, $roles); + }); + + // Chunk through all bookshelves + Bookshelf::query()->select(['id', 'restricted', 'owned_by']) + ->chunk(50, function ($shelves) use ($roles) { + $this->buildJointPermissionsForShelves($shelves, $roles); + }); + } + + /** + * Delete all the entity jointPermissions for a list of entities. + * + * @param Entity[] $entities + * + * @throws Throwable + */ + protected function deleteManyJointPermissionsForEntities(array $entities) + { + $idsByType = $this->entitiesToTypeIdMap($entities); + + DB::transaction(function () use ($idsByType) { + foreach ($idsByType as $type => $ids) { + foreach (array_chunk($ids, 1000) as $idChunk) { + DB::table('joint_permissions') + ->where('entity_type', '=', $type) + ->whereIn('entity_id', $idChunk) + ->delete(); + } + } + }); + } + + /** + * Create & Save entity jointPermissions for many entities and roles. + * + * @param Entity[] $entities + * @param Role[] $roles + * + * @throws Throwable + */ + protected function createManyJointPermissions(array $entities, array $roles) + { + $this->readyEntityCache($entities); + $jointPermissions = []; + + // Create a mapping of entity restricted statuses + $entityRestrictedMap = []; + foreach ($entities as $entity) { + $entityRestrictedMap[$entity->getMorphClass() . ':' . $entity->getRawAttribute('id')] = boolval($entity->getRawAttribute('restricted')); + } + + // Fetch related entity permissions + $permissions = $this->getEntityPermissionsForEntities($entities); + + // Create a mapping of explicit entity permissions + $permissionMap = []; + foreach ($permissions as $permission) { + $key = $permission->restrictable_type . ':' . $permission->restrictable_id . ':' . $permission->role_id . ':' . $permission->action; + $isRestricted = $entityRestrictedMap[$permission->restrictable_type . ':' . $permission->restrictable_id]; + $permissionMap[$key] = $isRestricted; + } + + // Create a mapping of role permissions + $rolePermissionMap = []; + foreach ($roles as $role) { + foreach ($role->permissions as $permission) { + $rolePermissionMap[$role->getRawAttribute('id') . ':' . $permission->getRawAttribute('name')] = true; + } + } + + // Create Joint Permission Data + foreach ($entities as $entity) { + foreach ($roles as $role) { + foreach ($this->getActions($entity) as $action) { + $jointPermissions[] = $this->createJointPermissionData($entity, $role, $action, $permissionMap, $rolePermissionMap); + } + } + } + + DB::transaction(function () use ($jointPermissions) { + foreach (array_chunk($jointPermissions, 1000) as $jointPermissionChunk) { + DB::table('joint_permissions')->insert($jointPermissionChunk); + } + }); + } + + /** + * From the given entity list, provide back a mapping of entity types to + * the ids of that given type. The type used is the DB morph class. + * @param Entity[] $entities + * @return array + */ + protected function entitiesToTypeIdMap(array $entities): array + { + $idsByType = []; + + foreach ($entities as $entity) { + $type = $entity->getMorphClass(); + + if (!isset($idsByType[$type])) { + $idsByType[$type] = []; + } + + $idsByType[$type][] = $entity->getRawAttribute('id'); + } + + return $idsByType; + } + + /** + * Get the entity permissions for all the given entities + * @param Entity[] $entities + * @return EloquentCollection + */ + protected function getEntityPermissionsForEntities(array $entities) + { + $idsByType = $this->entitiesToTypeIdMap($entities); + $permissionFetch = EntityPermission::query(); + + foreach ($idsByType as $type => $ids) { + $permissionFetch->orWhere(function (Builder $query) use ($type, $ids) { + $query->where('restrictable_type', '=', $type)->whereIn('restrictable_id', $ids); + }); + } + + return $permissionFetch->get(); + } + + /** + * Get the actions related to an entity. + */ + protected function getActions(Entity $entity): array + { + $baseActions = ['view', 'update', 'delete']; + if ($entity instanceof Chapter || $entity instanceof Book) { + $baseActions[] = 'page-create'; + } + if ($entity instanceof Book) { + $baseActions[] = 'chapter-create'; + } + + return $baseActions; + } + + /** + * Create entity permission data for an entity and role + * for a particular action. + */ + protected function createJointPermissionData(Entity $entity, Role $role, string $action, array $permissionMap, array $rolePermissionMap): array + { + $permissionPrefix = (strpos($action, '-') === false ? ($entity->getType() . '-') : '') . $action; + $roleHasPermission = isset($rolePermissionMap[$role->getRawAttribute('id') . ':' . $permissionPrefix . '-all']); + $roleHasPermissionOwn = isset($rolePermissionMap[$role->getRawAttribute('id') . ':' . $permissionPrefix . '-own']); + $explodedAction = explode('-', $action); + $restrictionAction = end($explodedAction); + + if ($role->system_name === 'admin') { + return $this->createJointPermissionDataArray($entity, $role, $action, true, true); + } + + if ($entity->restricted) { + $hasAccess = $this->mapHasActiveRestriction($permissionMap, $entity, $role, $restrictionAction); + + return $this->createJointPermissionDataArray($entity, $role, $action, $hasAccess, $hasAccess); + } + + if ($entity instanceof Book || $entity instanceof Bookshelf) { + return $this->createJointPermissionDataArray($entity, $role, $action, $roleHasPermission, $roleHasPermissionOwn); + } + + // For chapters and pages, Check if explicit permissions are set on the Book. + $book = $this->getBook($entity->book_id); + $hasExplicitAccessToParents = $this->mapHasActiveRestriction($permissionMap, $book, $role, $restrictionAction); + $hasPermissiveAccessToParents = !$book->restricted; + + // For pages with a chapter, Check if explicit permissions are set on the Chapter + if ($entity instanceof Page && intval($entity->chapter_id) !== 0) { + $chapter = $this->getChapter($entity->chapter_id); + $hasPermissiveAccessToParents = $hasPermissiveAccessToParents && !$chapter->restricted; + if ($chapter->restricted) { + $hasExplicitAccessToParents = $this->mapHasActiveRestriction($permissionMap, $chapter, $role, $restrictionAction); + } + } + + return $this->createJointPermissionDataArray( + $entity, + $role, + $action, + ($hasExplicitAccessToParents || ($roleHasPermission && $hasPermissiveAccessToParents)), + ($hasExplicitAccessToParents || ($roleHasPermissionOwn && $hasPermissiveAccessToParents)) + ); + } + + /** + * Check for an active restriction in an entity map. + */ + protected function mapHasActiveRestriction(array $entityMap, Entity $entity, Role $role, string $action): bool + { + $key = $entity->getMorphClass() . ':' . $entity->getRawAttribute('id') . ':' . $role->getRawAttribute('id') . ':' . $action; + + return $entityMap[$key] ?? false; + } + + /** + * Create an array of data with the information of an entity jointPermissions. + * Used to build data for bulk insertion. + */ + protected function createJointPermissionDataArray(Entity $entity, Role $role, string $action, bool $permissionAll, bool $permissionOwn): array + { + return [ + 'action' => $action, + 'entity_id' => $entity->getRawAttribute('id'), + 'entity_type' => $entity->getMorphClass(), + 'has_permission' => $permissionAll, + 'has_permission_own' => $permissionOwn, + 'owned_by' => $entity->getRawAttribute('owned_by'), + 'role_id' => $role->getRawAttribute('id'), + ]; + } + +} \ No newline at end of file diff --git a/app/Auth/Permissions/PermissionService.php b/app/Auth/Permissions/PermissionService.php index 1363fe86e..bcd4a3675 100644 --- a/app/Auth/Permissions/PermissionService.php +++ b/app/Auth/Permissions/PermissionService.php @@ -4,20 +4,13 @@ namespace BookStack\Auth\Permissions; use BookStack\Auth\Role; use BookStack\Auth\User; -use BookStack\Entities\Models\Book; -use BookStack\Entities\Models\BookChild; -use BookStack\Entities\Models\Bookshelf; -use BookStack\Entities\Models\Chapter; use BookStack\Entities\Models\Entity; use BookStack\Entities\Models\Page; use BookStack\Model; use BookStack\Traits\HasCreatorAndUpdater; use BookStack\Traits\HasOwner; -use Illuminate\Database\Connection; use Illuminate\Database\Eloquent\Builder; -use Illuminate\Database\Eloquent\Collection as EloquentCollection; use Illuminate\Database\Query\Builder as QueryBuilder; -use Throwable; class PermissionService { @@ -31,77 +24,6 @@ class PermissionService */ protected $currentUserModel = null; - /** - * @var Connection - */ - protected $db; - - /** - * @var array> - */ - protected $entityCache; - - /** - * PermissionService constructor. - */ - public function __construct(Connection $db) - { - $this->db = $db; - } - - /** - * Set the database connection. - */ - public function setConnection(Connection $connection) - { - $this->db = $connection; - } - - /** - * Prepare the local entity cache and ensure it's empty. - * - * @param Entity[] $entities - */ - protected function readyEntityCache(array $entities = []) - { - $this->entityCache = []; - - foreach ($entities as $entity) { - $class = get_class($entity); - - if (!isset($this->entityCache[$class])) { - $this->entityCache[$class] = []; - } - - $this->entityCache[$class][$entity->getRawAttribute('id')] = $entity; - } - } - - /** - * Get a book via ID, Checks local cache. - */ - protected function getBook(int $bookId): ?Book - { - if ($this->entityCache[Book::class][$bookId] ?? false) { - return $this->entityCache[Book::class][$bookId]; - } - - return Book::query()->withTrashed()->find($bookId); - } - - /** - * Get a chapter via ID, Checks local cache. - */ - protected function getChapter(int $chapterId): ?Chapter - { - if ($this->entityCache[Chapter::class][$chapterId] ?? false) { - return $this->entityCache[Chapter::class][$chapterId]; - } - - return Chapter::query() - ->withTrashed() - ->find($chapterId); - } /** * Get the roles for the current logged in user. @@ -121,380 +43,6 @@ class PermissionService return $this->userRoles; } - /** - * Re-generate all entity permission from scratch. - */ - public function buildJointPermissions() - { - JointPermission::query()->truncate(); - $this->readyEntityCache(); - - // Get all roles (Should be the most limited dimension) - $roles = Role::query()->with('permissions')->get()->all(); - - // Chunk through all books - $this->bookFetchQuery()->chunk(5, function (EloquentCollection $books) use ($roles) { - $this->buildJointPermissionsForBooks($books, $roles); - }); - - // Chunk through all bookshelves - Bookshelf::query()->withTrashed()->select(['id', 'restricted', 'owned_by']) - ->chunk(50, function (EloquentCollection $shelves) use ($roles) { - $this->buildJointPermissionsForShelves($shelves, $roles); - }); - } - - /** - * Get a query for fetching a book with it's children. - */ - protected function bookFetchQuery(): Builder - { - return Book::query()->withTrashed() - ->select(['id', 'restricted', 'owned_by'])->with([ - 'chapters' => function ($query) { - $query->withTrashed()->select(['id', 'restricted', 'owned_by', 'book_id']); - }, - 'pages' => function ($query) { - $query->withTrashed()->select(['id', 'restricted', 'owned_by', 'book_id', 'chapter_id']); - }, - ]); - } - - /** - * Build joint permissions for the given shelf and role combinations. - * - * @throws Throwable - */ - protected function buildJointPermissionsForShelves(EloquentCollection $shelves, array $roles, bool $deleteOld = false) - { - if ($deleteOld) { - $this->deleteManyJointPermissionsForEntities($shelves->all()); - } - $this->createManyJointPermissions($shelves->all(), $roles); - } - - /** - * Build joint permissions for the given book and role combinations. - * - * @throws Throwable - */ - protected function buildJointPermissionsForBooks(EloquentCollection $books, array $roles, bool $deleteOld = false) - { - $entities = clone $books; - - /** @var Book $book */ - foreach ($books->all() as $book) { - foreach ($book->getRelation('chapters') as $chapter) { - $entities->push($chapter); - } - foreach ($book->getRelation('pages') as $page) { - $entities->push($page); - } - } - - if ($deleteOld) { - $this->deleteManyJointPermissionsForEntities($entities->all()); - } - $this->createManyJointPermissions($entities->all(), $roles); - } - - /** - * Rebuild the entity jointPermissions for a particular entity. - * - * @throws Throwable - */ - public function buildJointPermissionsForEntity(Entity $entity) - { - $entities = [$entity]; - if ($entity instanceof Book) { - $books = $this->bookFetchQuery()->where('id', '=', $entity->id)->get(); - $this->buildJointPermissionsForBooks($books, Role::query()->with('permissions')->get()->all(), true); - - return; - } - - /** @var BookChild $entity */ - if ($entity->book) { - $entities[] = $entity->book; - } - - if ($entity instanceof Page && $entity->chapter_id) { - $entities[] = $entity->chapter; - } - - if ($entity instanceof Chapter) { - foreach ($entity->pages as $page) { - $entities[] = $page; - } - } - - $this->buildJointPermissionsForEntities($entities); - } - - /** - * Rebuild the entity jointPermissions for a collection of entities. - * - * @throws Throwable - */ - public function buildJointPermissionsForEntities(array $entities) - { - $roles = Role::query()->get()->values()->all(); - $this->deleteManyJointPermissionsForEntities($entities); - $this->createManyJointPermissions($entities, $roles); - } - - /** - * Build the entity jointPermissions for a particular role. - */ - public function buildJointPermissionForRole(Role $role) - { - $roles = [$role]; - $this->deleteManyJointPermissionsForRoles($roles); - - // Chunk through all books - $this->bookFetchQuery()->chunk(20, function ($books) use ($roles) { - $this->buildJointPermissionsForBooks($books, $roles); - }); - - // Chunk through all bookshelves - Bookshelf::query()->select(['id', 'restricted', 'owned_by']) - ->chunk(50, function ($shelves) use ($roles) { - $this->buildJointPermissionsForShelves($shelves, $roles); - }); - } - - /** - * Delete the entity jointPermissions attached to a particular role. - */ - public function deleteJointPermissionsForRole(Role $role) - { - $this->deleteManyJointPermissionsForRoles([$role]); - } - - /** - * Delete all the entity jointPermissions for a list of entities. - * - * @param Role[] $roles - */ - protected function deleteManyJointPermissionsForRoles($roles) - { - $roleIds = array_map(function ($role) { - return $role->id; - }, $roles); - JointPermission::query()->whereIn('role_id', $roleIds)->delete(); - } - - /** - * Delete all the entity jointPermissions for a list of entities. - * - * @param Entity[] $entities - * - * @throws Throwable - */ - protected function deleteManyJointPermissionsForEntities(array $entities) - { - $idsByType = $this->entitiesToTypeIdMap($entities); - - $this->db->transaction(function () use ($idsByType) { - foreach ($idsByType as $type => $ids) { - foreach (array_chunk($ids, 1000) as $idChunk) { - $this->db->table('joint_permissions') - ->where('entity_type', '=', $type) - ->whereIn('entity_id', $idChunk) - ->delete(); - } - } - }); - } - - /** - * Create & Save entity jointPermissions for many entities and roles. - * - * @param Entity[] $entities - * @param Role[] $roles - * - * @throws Throwable - */ - protected function createManyJointPermissions(array $entities, array $roles) - { - $this->readyEntityCache($entities); - $jointPermissions = []; - - // Create a mapping of entity restricted statuses - $entityRestrictedMap = []; - foreach ($entities as $entity) { - $entityRestrictedMap[$entity->getMorphClass() . ':' . $entity->getRawAttribute('id')] = boolval($entity->getRawAttribute('restricted')); - } - - // Fetch related entity permissions - $permissions = $this->getEntityPermissionsForEntities($entities); - - // Create a mapping of explicit entity permissions - $permissionMap = []; - foreach ($permissions as $permission) { - $key = $permission->restrictable_type . ':' . $permission->restrictable_id . ':' . $permission->role_id . ':' . $permission->action; - $isRestricted = $entityRestrictedMap[$permission->restrictable_type . ':' . $permission->restrictable_id]; - $permissionMap[$key] = $isRestricted; - } - - // Create a mapping of role permissions - $rolePermissionMap = []; - foreach ($roles as $role) { - foreach ($role->permissions as $permission) { - $rolePermissionMap[$role->getRawAttribute('id') . ':' . $permission->getRawAttribute('name')] = true; - } - } - - // Create Joint Permission Data - foreach ($entities as $entity) { - foreach ($roles as $role) { - foreach ($this->getActions($entity) as $action) { - $jointPermissions[] = $this->createJointPermissionData($entity, $role, $action, $permissionMap, $rolePermissionMap); - } - } - } - - $this->db->transaction(function () use ($jointPermissions) { - foreach (array_chunk($jointPermissions, 1000) as $jointPermissionChunk) { - $this->db->table('joint_permissions')->insert($jointPermissionChunk); - } - }); - } - - /** - * From the given entity list, provide back a mapping of entity types to - * the ids of that given type. The type used is the DB morph class. - * @param Entity[] $entities - * @return array - */ - protected function entitiesToTypeIdMap(array $entities): array - { - $idsByType = []; - - foreach ($entities as $entity) { - $type = $entity->getMorphClass(); - - if (!isset($idsByType[$type])) { - $idsByType[$type] = []; - } - - $idsByType[$type][] = $entity->getRawAttribute('id'); - } - - return $idsByType; - } - - /** - * Get the entity permissions for all the given entities - * @param Entity[] $entities - * @return EloquentCollection - */ - protected function getEntityPermissionsForEntities(array $entities) - { - $idsByType = $this->entitiesToTypeIdMap($entities); - $permissionFetch = EntityPermission::query(); - - foreach ($idsByType as $type => $ids) { - $permissionFetch->orWhere(function (Builder $query) use ($type, $ids) { - $query->where('restrictable_type', '=', $type)->whereIn('restrictable_id', $ids); - }); - } - - return $permissionFetch->get(); - } - - /** - * Get the actions related to an entity. - */ - protected function getActions(Entity $entity): array - { - $baseActions = ['view', 'update', 'delete']; - if ($entity instanceof Chapter || $entity instanceof Book) { - $baseActions[] = 'page-create'; - } - if ($entity instanceof Book) { - $baseActions[] = 'chapter-create'; - } - - return $baseActions; - } - - /** - * Create entity permission data for an entity and role - * for a particular action. - */ - protected function createJointPermissionData(Entity $entity, Role $role, string $action, array $permissionMap, array $rolePermissionMap): array - { - $permissionPrefix = (strpos($action, '-') === false ? ($entity->getType() . '-') : '') . $action; - $roleHasPermission = isset($rolePermissionMap[$role->getRawAttribute('id') . ':' . $permissionPrefix . '-all']); - $roleHasPermissionOwn = isset($rolePermissionMap[$role->getRawAttribute('id') . ':' . $permissionPrefix . '-own']); - $explodedAction = explode('-', $action); - $restrictionAction = end($explodedAction); - - if ($role->system_name === 'admin') { - return $this->createJointPermissionDataArray($entity, $role, $action, true, true); - } - - if ($entity->restricted) { - $hasAccess = $this->mapHasActiveRestriction($permissionMap, $entity, $role, $restrictionAction); - - return $this->createJointPermissionDataArray($entity, $role, $action, $hasAccess, $hasAccess); - } - - if ($entity instanceof Book || $entity instanceof Bookshelf) { - return $this->createJointPermissionDataArray($entity, $role, $action, $roleHasPermission, $roleHasPermissionOwn); - } - - // For chapters and pages, Check if explicit permissions are set on the Book. - $book = $this->getBook($entity->book_id); - $hasExplicitAccessToParents = $this->mapHasActiveRestriction($permissionMap, $book, $role, $restrictionAction); - $hasPermissiveAccessToParents = !$book->restricted; - - // For pages with a chapter, Check if explicit permissions are set on the Chapter - if ($entity instanceof Page && intval($entity->chapter_id) !== 0) { - $chapter = $this->getChapter($entity->chapter_id); - $hasPermissiveAccessToParents = $hasPermissiveAccessToParents && !$chapter->restricted; - if ($chapter->restricted) { - $hasExplicitAccessToParents = $this->mapHasActiveRestriction($permissionMap, $chapter, $role, $restrictionAction); - } - } - - return $this->createJointPermissionDataArray( - $entity, - $role, - $action, - ($hasExplicitAccessToParents || ($roleHasPermission && $hasPermissiveAccessToParents)), - ($hasExplicitAccessToParents || ($roleHasPermissionOwn && $hasPermissiveAccessToParents)) - ); - } - - /** - * Check for an active restriction in an entity map. - */ - protected function mapHasActiveRestriction(array $entityMap, Entity $entity, Role $role, string $action): bool - { - $key = $entity->getMorphClass() . ':' . $entity->getRawAttribute('id') . ':' . $role->getRawAttribute('id') . ':' . $action; - - return $entityMap[$key] ?? false; - } - - /** - * Create an array of data with the information of an entity jointPermissions. - * Used to build data for bulk insertion. - */ - protected function createJointPermissionDataArray(Entity $entity, Role $role, string $action, bool $permissionAll, bool $permissionOwn): array - { - return [ - 'role_id' => $role->getRawAttribute('id'), - 'entity_id' => $entity->getRawAttribute('id'), - 'entity_type' => $entity->getMorphClass(), - 'action' => $action, - 'has_permission' => $permissionAll, - 'has_permission_own' => $permissionOwn, - 'owned_by' => $entity->getRawAttribute('owned_by'), - ]; - } - /** * Checks if an entity has a restriction set upon it. * diff --git a/app/Auth/Permissions/PermissionsRepo.php b/app/Auth/Permissions/PermissionsRepo.php index 988146700..0527875ae 100644 --- a/app/Auth/Permissions/PermissionsRepo.php +++ b/app/Auth/Permissions/PermissionsRepo.php @@ -11,20 +11,15 @@ use Illuminate\Database\Eloquent\Collection; class PermissionsRepo { - protected $permission; - protected $role; - protected $permissionService; - + protected JointPermissionBuilder $permissionBuilder; protected $systemRoles = ['admin', 'public']; /** * PermissionsRepo constructor. */ - public function __construct(RolePermission $permission, Role $role, PermissionService $permissionService) + public function __construct(JointPermissionBuilder $permissionBuilder) { - $this->permission = $permission; - $this->role = $role; - $this->permissionService = $permissionService; + $this->permissionBuilder = $permissionBuilder; } /** @@ -32,7 +27,7 @@ class PermissionsRepo */ public function getAllRoles(): Collection { - return $this->role->all(); + return Role::query()->all(); } /** @@ -40,7 +35,7 @@ class PermissionsRepo */ public function getAllRolesExcept(Role $role): Collection { - return $this->role->where('id', '!=', $role->id)->get(); + return Role::query()->where('id', '!=', $role->id)->get(); } /** @@ -48,7 +43,7 @@ class PermissionsRepo */ public function getRoleById($id): Role { - return $this->role->newQuery()->findOrFail($id); + return Role::query()->findOrFail($id); } /** @@ -56,13 +51,14 @@ class PermissionsRepo */ public function saveNewRole(array $roleData): Role { - $role = $this->role->newInstance($roleData); + $role = new Role($roleData); $role->mfa_enforced = ($roleData['mfa_enforced'] ?? 'false') === 'true'; $role->save(); $permissions = isset($roleData['permissions']) ? array_keys($roleData['permissions']) : []; $this->assignRolePermissions($role, $permissions); - $this->permissionService->buildJointPermissionForRole($role); + $this->permissionBuilder->buildJointPermissionForRole($role); + Activity::add(ActivityType::ROLE_CREATE, $role); return $role; @@ -74,8 +70,7 @@ class PermissionsRepo */ public function updateRole($roleId, array $roleData) { - /** @var Role $role */ - $role = $this->role->newQuery()->findOrFail($roleId); + $role = $this->getRoleById($roleId); $permissions = isset($roleData['permissions']) ? array_keys($roleData['permissions']) : []; if ($role->system_name === 'admin') { @@ -93,12 +88,13 @@ class PermissionsRepo $role->fill($roleData); $role->mfa_enforced = ($roleData['mfa_enforced'] ?? 'false') === 'true'; $role->save(); - $this->permissionService->buildJointPermissionForRole($role); + $this->permissionBuilder->buildJointPermissionForRole($role); + Activity::add(ActivityType::ROLE_UPDATE, $role); } /** - * Assign an list of permission names to an role. + * Assign a list of permission names to a role. */ protected function assignRolePermissions(Role $role, array $permissionNameArray = []) { @@ -106,7 +102,7 @@ class PermissionsRepo $permissionNameArray = array_values($permissionNameArray); if ($permissionNameArray) { - $permissions = $this->permission->newQuery() + $permissions = EntityPermission::query() ->whereIn('name', $permissionNameArray) ->pluck('id') ->toArray(); @@ -126,8 +122,7 @@ class PermissionsRepo */ public function deleteRole($roleId, $migrateRoleId) { - /** @var Role $role */ - $role = $this->role->newQuery()->findOrFail($roleId); + $role = $this->getRoleById($roleId); // Prevent deleting admin role or default registration role. if ($role->system_name && in_array($role->system_name, $this->systemRoles)) { @@ -137,14 +132,14 @@ class PermissionsRepo } if ($migrateRoleId) { - $newRole = $this->role->newQuery()->find($migrateRoleId); + $newRole = Role::query()->find($migrateRoleId); if ($newRole) { $users = $role->users()->pluck('id')->toArray(); $newRole->users()->sync($users); } } - $this->permissionService->deleteJointPermissionsForRole($role); + $role->jointPermissions()->delete(); Activity::add(ActivityType::ROLE_DELETE, $role); $role->delete(); } diff --git a/app/Console/Commands/RegeneratePermissions.php b/app/Console/Commands/RegeneratePermissions.php index 4fde08e6b..558ae9fea 100644 --- a/app/Console/Commands/RegeneratePermissions.php +++ b/app/Console/Commands/RegeneratePermissions.php @@ -2,8 +2,9 @@ namespace BookStack\Console\Commands; -use BookStack\Auth\Permissions\PermissionService; +use BookStack\Auth\Permissions\JointPermissionBuilder; use Illuminate\Console\Command; +use Illuminate\Support\Facades\DB; class RegeneratePermissions extends Command { @@ -21,19 +22,14 @@ class RegeneratePermissions extends Command */ protected $description = 'Regenerate all system permissions'; - /** - * The service to handle the permission system. - * - * @var PermissionService - */ - protected $permissionService; + protected JointPermissionBuilder $permissionBuilder; /** * Create a new command instance. */ - public function __construct(PermissionService $permissionService) + public function __construct(JointPermissionBuilder $permissionBuilder) { - $this->permissionService = $permissionService; + $this->permissionBuilder = $permissionBuilder; parent::__construct(); } @@ -44,15 +40,15 @@ class RegeneratePermissions extends Command */ public function handle() { - $connection = \DB::getDefaultConnection(); - if ($this->option('database') !== null) { - \DB::setDefaultConnection($this->option('database')); - $this->permissionService->setConnection(\DB::connection($this->option('database'))); + $connection = DB::getDefaultConnection(); + + if ($this->hasOption('database')) { + DB::setDefaultConnection($this->option('database')); } - $this->permissionService->buildJointPermissions(); + $this->permissionBuilder->buildJointPermissions(); - \DB::setDefaultConnection($connection); + DB::setDefaultConnection($connection); $this->comment('Permissions regenerated'); } } diff --git a/database/seeders/DummyContentSeeder.php b/database/seeders/DummyContentSeeder.php index d54732b26..cb9987df7 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\PermissionService; +use BookStack\Auth\Permissions\JointPermissionBuilder; use BookStack\Auth\Permissions\RolePermission; use BookStack\Auth\Role; use BookStack\Auth\User; @@ -69,7 +69,7 @@ class DummyContentSeeder extends Seeder ]); $token->save(); - app(PermissionService::class)->buildJointPermissions(); + app(JointPermissionBuilder::class)->buildJointPermissions(); app(SearchIndex::class)->indexAllEntities(); } } diff --git a/database/seeders/LargeContentSeeder.php b/database/seeders/LargeContentSeeder.php index dd9165978..041b3161a 100644 --- a/database/seeders/LargeContentSeeder.php +++ b/database/seeders/LargeContentSeeder.php @@ -2,7 +2,7 @@ namespace Database\Seeders; -use BookStack\Auth\Permissions\PermissionService; +use BookStack\Auth\Permissions\JointPermissionBuilder; 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(PermissionService::class)->buildJointPermissionsForEntity($largeBook); + app()->make(JointPermissionBuilder::class)->buildJointPermissionsForEntity($largeBook); app()->make(SearchIndex::class)->indexEntities($all); } } diff --git a/tests/PublicActionTest.php b/tests/PublicActionTest.php index 499c0c9f9..745fa7660 100644 --- a/tests/PublicActionTest.php +++ b/tests/PublicActionTest.php @@ -2,7 +2,7 @@ namespace Tests; -use BookStack\Auth\Permissions\PermissionService; +use BookStack\Auth\Permissions\JointPermissionBuilder; use BookStack\Auth\Permissions\RolePermission; use BookStack\Auth\Role; use BookStack\Auth\User; @@ -89,7 +89,7 @@ class PublicActionTest extends TestCase foreach (RolePermission::all() as $perm) { $publicRole->attachPermission($perm); } - $this->app[PermissionService::class]->buildJointPermissionForRole($publicRole); + $this->app->make(JointPermissionBuilder::class)->buildJointPermissionForRole($publicRole); /** @var Chapter $chapter */ $chapter = Chapter::query()->first(); diff --git a/tests/SharedTestHelpers.php b/tests/SharedTestHelpers.php index ce57d56f5..2d05b0520 100644 --- a/tests/SharedTestHelpers.php +++ b/tests/SharedTestHelpers.php @@ -2,6 +2,7 @@ namespace Tests; +use BookStack\Auth\Permissions\JointPermissionBuilder; use BookStack\Auth\Permissions\PermissionService; use BookStack\Auth\Permissions\PermissionsRepo; use BookStack\Auth\Permissions\RolePermission; @@ -176,7 +177,7 @@ trait SharedTestHelpers $entity->save(); $entity->load('permissions'); - $this->app[PermissionService::class]->buildJointPermissionsForEntity($entity); + $this->app->make(JointPermissionBuilder::class)->buildJointPermissionsForEntity($entity); $entity->load('jointPermissions'); } @@ -196,7 +197,7 @@ trait SharedTestHelpers */ protected function removePermissionFromUser(User $user, string $permissionName) { - $permissionService = app()->make(PermissionService::class); + $permissionBuilder = app()->make(JointPermissionBuilder::class); /** @var RolePermission $permission */ $permission = RolePermission::query()->where('name', '=', $permissionName)->firstOrFail(); @@ -208,7 +209,7 @@ trait SharedTestHelpers /** @var Role $role */ foreach ($roles as $role) { $role->detachPermission($permission); - $permissionService->buildJointPermissionForRole($role); + $permissionBuilder->buildJointPermissionForRole($role); } $user->clearPermissionCache(); @@ -241,8 +242,8 @@ trait SharedTestHelpers $book = Book::factory()->create($userAttrs); $chapter = Chapter::factory()->create(array_merge(['book_id' => $book->id], $userAttrs)); $page = Page::factory()->create(array_merge(['book_id' => $book->id, 'chapter_id' => $chapter->id], $userAttrs)); - $restrictionService = $this->app[PermissionService::class]; - $restrictionService->buildJointPermissionsForEntity($book); + + $this->app->make(JointPermissionBuilder::class)->buildJointPermissionsForEntity($book); return compact('book', 'chapter', 'page'); } From b0a4d3d0597dcdbe2cd15261ecc874f2d5a7f666 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Tue, 12 Jul 2022 20:15:41 +0100 Subject: [PATCH 03/12] Renamed and cleaned up existing permission service classes use --- app/Actions/ActivityLogger.php | 8 - app/Actions/ActivityQueries.php | 12 +- app/Actions/TagRepo.php | 15 +- .../Permissions/JointPermissionBuilder.php | 164 ++++++++---------- ...onService.php => PermissionApplicator.php} | 5 +- app/Auth/Permissions/PermissionsRepo.php | 8 +- app/Config/app.php | 1 - .../Commands/RegeneratePermissions.php | 4 +- app/Entities/Models/Entity.php | 12 +- app/Entities/Models/Page.php | 4 +- app/Entities/Queries/EntityQuery.php | 6 +- app/Entities/Tools/SearchRunner.php | 20 +-- app/Facades/Permissions.php | 18 -- app/Providers/CustomFacadeProvider.php | 10 +- app/Uploads/Attachment.php | 6 +- app/Uploads/ImageRepo.php | 12 +- app/helpers.php | 10 +- database/seeders/DummyContentSeeder.php | 2 +- database/seeders/LargeContentSeeder.php | 2 +- .../RegeneratePermissionsCommandTest.php | 5 +- tests/PublicActionTest.php | 2 +- tests/SharedTestHelpers.php | 8 +- 22 files changed, 140 insertions(+), 194 deletions(-) rename app/Auth/Permissions/{PermissionService.php => PermissionApplicator.php} (99%) delete mode 100644 app/Facades/Permissions.php diff --git a/app/Actions/ActivityLogger.php b/app/Actions/ActivityLogger.php index 0d1391b43..eea5409fb 100644 --- a/app/Actions/ActivityLogger.php +++ b/app/Actions/ActivityLogger.php @@ -2,7 +2,6 @@ namespace BookStack\Actions; -use BookStack\Auth\Permissions\PermissionService; use BookStack\Entities\Models\Entity; use BookStack\Interfaces\Loggable; use Illuminate\Database\Eloquent\Builder; @@ -10,13 +9,6 @@ use Illuminate\Support\Facades\Log; class ActivityLogger { - protected $permissionService; - - public function __construct(PermissionService $permissionService) - { - $this->permissionService = $permissionService; - } - /** * Add a generic activity event to the database. * diff --git a/app/Actions/ActivityQueries.php b/app/Actions/ActivityQueries.php index f900fbb05..0b8cd3b5e 100644 --- a/app/Actions/ActivityQueries.php +++ b/app/Actions/ActivityQueries.php @@ -2,7 +2,7 @@ namespace BookStack\Actions; -use BookStack\Auth\Permissions\PermissionService; +use BookStack\Auth\Permissions\PermissionApplicator; use BookStack\Auth\User; use BookStack\Entities\Models\Book; use BookStack\Entities\Models\Chapter; @@ -13,11 +13,11 @@ use Illuminate\Database\Eloquent\Relations\Relation; class ActivityQueries { - protected $permissionService; + protected PermissionApplicator $permissions; - public function __construct(PermissionService $permissionService) + public function __construct(PermissionApplicator $permissions) { - $this->permissionService = $permissionService; + $this->permissions = $permissions; } /** @@ -25,7 +25,7 @@ class ActivityQueries */ public function latest(int $count = 20, int $page = 0): array { - $activityList = $this->permissionService + $activityList = $this->permissions ->filterRestrictedEntityRelations(Activity::query(), 'activities', 'entity_id', 'entity_type') ->orderBy('created_at', 'desc') ->with(['user', 'entity']) @@ -78,7 +78,7 @@ class ActivityQueries */ public function userActivity(User $user, int $count = 20, int $page = 0): array { - $activityList = $this->permissionService + $activityList = $this->permissions ->filterRestrictedEntityRelations(Activity::query(), 'activities', 'entity_id', 'entity_type') ->orderBy('created_at', 'desc') ->where('user_id', '=', $user->id) diff --git a/app/Actions/TagRepo.php b/app/Actions/TagRepo.php index 8cf107601..c5807108e 100644 --- a/app/Actions/TagRepo.php +++ b/app/Actions/TagRepo.php @@ -2,7 +2,7 @@ namespace BookStack\Actions; -use BookStack\Auth\Permissions\PermissionService; +use BookStack\Auth\Permissions\PermissionApplicator; use BookStack\Entities\Models\Entity; use Illuminate\Database\Eloquent\Builder; use Illuminate\Support\Collection; @@ -10,12 +10,11 @@ use Illuminate\Support\Facades\DB; class TagRepo { - protected $tag; - protected $permissionService; + protected PermissionApplicator $permissions; - public function __construct(PermissionService $ps) + public function __construct(PermissionApplicator $permissions) { - $this->permissionService = $ps; + $this->permissions = $permissions; } /** @@ -51,7 +50,7 @@ class TagRepo }); } - return $this->permissionService->filterRestrictedEntityRelations($query, 'tags', 'entity_id', 'entity_type'); + return $this->permissions->filterRestrictedEntityRelations($query, 'tags', 'entity_id', 'entity_type'); } /** @@ -70,7 +69,7 @@ class TagRepo $query = $query->orderBy('count', 'desc')->take(50); } - $query = $this->permissionService->filterRestrictedEntityRelations($query, 'tags', 'entity_id', 'entity_type'); + $query = $this->permissions->filterRestrictedEntityRelations($query, 'tags', 'entity_id', 'entity_type'); return $query->get(['name'])->pluck('name'); } @@ -96,7 +95,7 @@ class TagRepo $query = $query->where('name', '=', $tagName); } - $query = $this->permissionService->filterRestrictedEntityRelations($query, 'tags', 'entity_id', 'entity_type'); + $query = $this->permissions->filterRestrictedEntityRelations($query, 'tags', 'entity_id', 'entity_type'); return $query->get(['value'])->pluck('value'); } diff --git a/app/Auth/Permissions/JointPermissionBuilder.php b/app/Auth/Permissions/JointPermissionBuilder.php index c87560fe0..fbf77741a 100644 --- a/app/Auth/Permissions/JointPermissionBuilder.php +++ b/app/Auth/Permissions/JointPermissionBuilder.php @@ -20,6 +20,82 @@ class JointPermissionBuilder */ protected $entityCache; + /** + * Re-generate all entity permission from scratch. + */ + public function rebuildForAll() + { + JointPermission::query()->truncate(); + $this->readyEntityCache(); + + // Get all roles (Should be the most limited dimension) + $roles = Role::query()->with('permissions')->get()->all(); + + // Chunk through all books + $this->bookFetchQuery()->chunk(5, function (EloquentCollection $books) use ($roles) { + $this->buildJointPermissionsForBooks($books, $roles); + }); + + // Chunk through all bookshelves + Bookshelf::query()->withTrashed()->select(['id', 'restricted', 'owned_by']) + ->chunk(50, function (EloquentCollection $shelves) use ($roles) { + $this->createManyJointPermissions($shelves->all(), $roles); + }); + } + + /** + * Rebuild the entity jointPermissions for a particular entity. + * + * @throws Throwable + */ + public function rebuildForEntity(Entity $entity) + { + $entities = [$entity]; + if ($entity instanceof Book) { + $books = $this->bookFetchQuery()->where('id', '=', $entity->id)->get(); + $this->buildJointPermissionsForBooks($books, Role::query()->with('permissions')->get()->all(), true); + + return; + } + + /** @var BookChild $entity */ + if ($entity->book) { + $entities[] = $entity->book; + } + + if ($entity instanceof Page && $entity->chapter_id) { + $entities[] = $entity->chapter; + } + + if ($entity instanceof Chapter) { + foreach ($entity->pages as $page) { + $entities[] = $page; + } + } + + $this->buildJointPermissionsForEntities($entities); + } + + /** + * Build the entity jointPermissions for a particular role. + */ + public function rebuildForRole(Role $role) + { + $roles = [$role]; + $role->jointPermissions()->delete(); + + // Chunk through all books + $this->bookFetchQuery()->chunk(20, function ($books) use ($roles) { + $this->buildJointPermissionsForBooks($books, $roles); + }); + + // Chunk through all bookshelves + Bookshelf::query()->select(['id', 'restricted', 'owned_by']) + ->chunk(50, function ($shelves) use ($roles) { + $this->createManyJointPermissions($shelves->all(), $roles); + }); + } + /** * Prepare the local entity cache and ensure it's empty. * @@ -66,29 +142,6 @@ class JointPermissionBuilder ->find($chapterId); } - /** - * Re-generate all entity permission from scratch. - */ - public function buildJointPermissions() - { - JointPermission::query()->truncate(); - $this->readyEntityCache(); - - // Get all roles (Should be the most limited dimension) - $roles = Role::query()->with('permissions')->get()->all(); - - // Chunk through all books - $this->bookFetchQuery()->chunk(5, function (EloquentCollection $books) use ($roles) { - $this->buildJointPermissionsForBooks($books, $roles); - }); - - // Chunk through all bookshelves - Bookshelf::query()->withTrashed()->select(['id', 'restricted', 'owned_by']) - ->chunk(50, function (EloquentCollection $shelves) use ($roles) { - $this->buildJointPermissionsForShelves($shelves, $roles); - }); - } - /** * Get a query for fetching a book with it's children. */ @@ -105,18 +158,6 @@ class JointPermissionBuilder ]); } - /** - * Build joint permissions for the given shelf and role combinations. - * - * @throws Throwable - */ - protected function buildJointPermissionsForShelves(EloquentCollection $shelves, array $roles, bool $deleteOld = false) - { - if ($deleteOld) { - $this->deleteManyJointPermissionsForEntities($shelves->all()); - } - $this->createManyJointPermissions($shelves->all(), $roles); - } /** * Build joint permissions for the given book and role combinations. @@ -144,39 +185,6 @@ class JointPermissionBuilder $this->createManyJointPermissions($entities->all(), $roles); } - /** - * Rebuild the entity jointPermissions for a particular entity. - * - * @throws Throwable - */ - public function buildJointPermissionsForEntity(Entity $entity) - { - $entities = [$entity]; - if ($entity instanceof Book) { - $books = $this->bookFetchQuery()->where('id', '=', $entity->id)->get(); - $this->buildJointPermissionsForBooks($books, Role::query()->with('permissions')->get()->all(), true); - - return; - } - - /** @var BookChild $entity */ - if ($entity->book) { - $entities[] = $entity->book; - } - - if ($entity instanceof Page && $entity->chapter_id) { - $entities[] = $entity->chapter; - } - - if ($entity instanceof Chapter) { - foreach ($entity->pages as $page) { - $entities[] = $page; - } - } - - $this->buildJointPermissionsForEntities($entities); - } - /** * Rebuild the entity jointPermissions for a collection of entities. * @@ -189,26 +197,6 @@ class JointPermissionBuilder $this->createManyJointPermissions($entities, $roles); } - /** - * Build the entity jointPermissions for a particular role. - */ - public function buildJointPermissionForRole(Role $role) - { - $roles = [$role]; - $role->jointPermissions()->delete(); - - // Chunk through all books - $this->bookFetchQuery()->chunk(20, function ($books) use ($roles) { - $this->buildJointPermissionsForBooks($books, $roles); - }); - - // Chunk through all bookshelves - Bookshelf::query()->select(['id', 'restricted', 'owned_by']) - ->chunk(50, function ($shelves) use ($roles) { - $this->buildJointPermissionsForShelves($shelves, $roles); - }); - } - /** * Delete all the entity jointPermissions for a list of entities. * diff --git a/app/Auth/Permissions/PermissionService.php b/app/Auth/Permissions/PermissionApplicator.php similarity index 99% rename from app/Auth/Permissions/PermissionService.php rename to app/Auth/Permissions/PermissionApplicator.php index bcd4a3675..3dc529e32 100644 --- a/app/Auth/Permissions/PermissionService.php +++ b/app/Auth/Permissions/PermissionApplicator.php @@ -12,10 +12,10 @@ use BookStack\Traits\HasOwner; use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Query\Builder as QueryBuilder; -class PermissionService +class PermissionApplicator { /** - * @var ?array + * @var ?array */ protected $userRoles = null; @@ -24,7 +24,6 @@ class PermissionService */ protected $currentUserModel = null; - /** * Get the roles for the current logged in user. */ diff --git a/app/Auth/Permissions/PermissionsRepo.php b/app/Auth/Permissions/PermissionsRepo.php index 0527875ae..2c2bedb72 100644 --- a/app/Auth/Permissions/PermissionsRepo.php +++ b/app/Auth/Permissions/PermissionsRepo.php @@ -27,7 +27,7 @@ class PermissionsRepo */ public function getAllRoles(): Collection { - return Role::query()->all(); + return Role::query()->get(); } /** @@ -57,7 +57,7 @@ class PermissionsRepo $permissions = isset($roleData['permissions']) ? array_keys($roleData['permissions']) : []; $this->assignRolePermissions($role, $permissions); - $this->permissionBuilder->buildJointPermissionForRole($role); + $this->permissionBuilder->rebuildForRole($role); Activity::add(ActivityType::ROLE_CREATE, $role); @@ -88,7 +88,7 @@ class PermissionsRepo $role->fill($roleData); $role->mfa_enforced = ($roleData['mfa_enforced'] ?? 'false') === 'true'; $role->save(); - $this->permissionBuilder->buildJointPermissionForRole($role); + $this->permissionBuilder->rebuildForRole($role); Activity::add(ActivityType::ROLE_UPDATE, $role); } @@ -102,7 +102,7 @@ class PermissionsRepo $permissionNameArray = array_values($permissionNameArray); if ($permissionNameArray) { - $permissions = EntityPermission::query() + $permissions = RolePermission::query() ->whereIn('name', $permissionNameArray) ->pluck('id') ->toArray(); diff --git a/app/Config/app.php b/app/Config/app.php index 2b1de6f45..5f516f4ad 100644 --- a/app/Config/app.php +++ b/app/Config/app.php @@ -202,7 +202,6 @@ return [ // Custom BookStack 'Activity' => BookStack\Facades\Activity::class, - 'Permissions' => BookStack\Facades\Permissions::class, 'Theme' => BookStack\Facades\Theme::class, ], diff --git a/app/Console/Commands/RegeneratePermissions.php b/app/Console/Commands/RegeneratePermissions.php index 558ae9fea..3396a445f 100644 --- a/app/Console/Commands/RegeneratePermissions.php +++ b/app/Console/Commands/RegeneratePermissions.php @@ -42,11 +42,11 @@ class RegeneratePermissions extends Command { $connection = DB::getDefaultConnection(); - if ($this->hasOption('database')) { + if ($this->option('database')) { DB::setDefaultConnection($this->option('database')); } - $this->permissionBuilder->buildJointPermissions(); + $this->permissionBuilder->rebuildForAll(); DB::setDefaultConnection($connection); $this->comment('Permissions regenerated'); diff --git a/app/Entities/Models/Entity.php b/app/Entities/Models/Entity.php index 7ad78f1d1..84d31d82d 100644 --- a/app/Entities/Models/Entity.php +++ b/app/Entities/Models/Entity.php @@ -9,9 +9,10 @@ use BookStack\Actions\Tag; use BookStack\Actions\View; use BookStack\Auth\Permissions\EntityPermission; use BookStack\Auth\Permissions\JointPermission; +use BookStack\Auth\Permissions\JointPermissionBuilder; +use BookStack\Auth\Permissions\PermissionApplicator; use BookStack\Entities\Tools\SearchIndex; use BookStack\Entities\Tools\SlugGenerator; -use BookStack\Facades\Permissions; use BookStack\Interfaces\Deletable; use BookStack\Interfaces\Favouritable; use BookStack\Interfaces\Loggable; @@ -76,7 +77,7 @@ abstract class Entity extends Model implements Sluggable, Favouritable, Viewable */ public function scopeHasPermission(Builder $query, string $permission) { - return Permissions::restrictEntityQuery($query, $permission); + return app()->make(PermissionApplicator::class)->restrictEntityQuery($query, $permission); } /** @@ -284,8 +285,7 @@ abstract class Entity extends Model implements Sluggable, Favouritable, Viewable */ public function rebuildPermissions() { - /** @noinspection PhpUnhandledExceptionInspection */ - Permissions::buildJointPermissionsForEntity(clone $this); + app()->make(JointPermissionBuilder::class)->rebuildForEntity(clone $this); } /** @@ -293,7 +293,7 @@ abstract class Entity extends Model implements Sluggable, Favouritable, Viewable */ public function indexForSearch() { - app(SearchIndex::class)->indexEntity(clone $this); + app()->make(SearchIndex::class)->indexEntity(clone $this); } /** @@ -301,7 +301,7 @@ abstract class Entity extends Model implements Sluggable, Favouritable, Viewable */ public function refreshSlug(): string { - $this->slug = app(SlugGenerator::class)->generate($this); + $this->slug = app()->make(SlugGenerator::class)->generate($this); return $this->slug; } diff --git a/app/Entities/Models/Page.php b/app/Entities/Models/Page.php index ed69bcf8b..a2690b2a0 100644 --- a/app/Entities/Models/Page.php +++ b/app/Entities/Models/Page.php @@ -2,8 +2,8 @@ namespace BookStack\Entities\Models; +use BookStack\Auth\Permissions\PermissionApplicator; use BookStack\Entities\Tools\PageContent; -use BookStack\Facades\Permissions; use BookStack\Uploads\Attachment; use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Collection; @@ -51,7 +51,7 @@ class Page extends BookChild */ public function scopeVisible(Builder $query): Builder { - $query = Permissions::enforceDraftVisibilityOnQuery($query); + $query = app()->make(PermissionApplicator::class)->enforceDraftVisibilityOnQuery($query); return parent::scopeVisible($query); } diff --git a/app/Entities/Queries/EntityQuery.php b/app/Entities/Queries/EntityQuery.php index 76ab16ffc..5ab882ca8 100644 --- a/app/Entities/Queries/EntityQuery.php +++ b/app/Entities/Queries/EntityQuery.php @@ -2,14 +2,14 @@ namespace BookStack\Entities\Queries; -use BookStack\Auth\Permissions\PermissionService; +use BookStack\Auth\Permissions\PermissionApplicator; use BookStack\Entities\EntityProvider; abstract class EntityQuery { - protected function permissionService(): PermissionService + protected function permissionService(): PermissionApplicator { - return app()->make(PermissionService::class); + return app()->make(PermissionApplicator::class); } protected function entityProvider(): EntityProvider diff --git a/app/Entities/Tools/SearchRunner.php b/app/Entities/Tools/SearchRunner.php index a591914f3..5d94379c9 100644 --- a/app/Entities/Tools/SearchRunner.php +++ b/app/Entities/Tools/SearchRunner.php @@ -2,7 +2,7 @@ namespace BookStack\Entities\Tools; -use BookStack\Auth\Permissions\PermissionService; +use BookStack\Auth\Permissions\PermissionApplicator; use BookStack\Auth\User; use BookStack\Entities\EntityProvider; use BookStack\Entities\Models\BookChild; @@ -21,20 +21,14 @@ use SplObjectStorage; class SearchRunner { - /** - * @var EntityProvider - */ - protected $entityProvider; - /** - * @var PermissionService - */ - protected $permissionService; + protected EntityProvider $entityProvider; + protected PermissionApplicator $permissions; /** * Acceptable operators to be used in a query. * - * @var array + * @var string[] */ protected $queryOperators = ['<=', '>=', '=', '<', '>', 'like', '!=']; @@ -46,10 +40,10 @@ class SearchRunner */ protected $termAdjustmentCache; - public function __construct(EntityProvider $entityProvider, PermissionService $permissionService) + public function __construct(EntityProvider $entityProvider, PermissionApplicator $permissions) { $this->entityProvider = $entityProvider; - $this->permissionService = $permissionService; + $this->permissions = $permissions; $this->termAdjustmentCache = new SplObjectStorage(); } @@ -199,7 +193,7 @@ class SearchRunner } } - return $this->permissionService->enforceEntityRestrictions($entityModelInstance, $entityQuery, $action); + return $this->permissions->enforceEntityRestrictions($entityModelInstance, $entityQuery, $action); } /** diff --git a/app/Facades/Permissions.php b/app/Facades/Permissions.php deleted file mode 100644 index 74cbe46fe..000000000 --- a/app/Facades/Permissions.php +++ /dev/null @@ -1,18 +0,0 @@ -app->make(ActivityLogger::class); }); - $this->app->singleton('images', function () { - return $this->app->make(ImageService::class); - }); - - $this->app->singleton('permissions', function () { - return $this->app->make(PermissionService::class); - }); - $this->app->singleton('theme', function () { return $this->app->make(ThemeService::class); }); diff --git a/app/Uploads/Attachment.php b/app/Uploads/Attachment.php index 5e637246a..d9e0ce7e1 100644 --- a/app/Uploads/Attachment.php +++ b/app/Uploads/Attachment.php @@ -2,7 +2,7 @@ namespace BookStack\Uploads; -use BookStack\Auth\Permissions\PermissionService; +use BookStack\Auth\Permissions\PermissionApplicator; use BookStack\Auth\User; use BookStack\Entities\Models\Entity; use BookStack\Entities\Models\Page; @@ -89,9 +89,9 @@ class Attachment extends Model */ public function scopeVisible(): Builder { - $permissionService = app()->make(PermissionService::class); + $permissions = app()->make(PermissionApplicator::class); - return $permissionService->filterRelatedEntity( + return $permissions->filterRelatedEntity( Page::class, self::query(), 'attachments', diff --git a/app/Uploads/ImageRepo.php b/app/Uploads/ImageRepo.php index bfe4b5977..13d3344dd 100644 --- a/app/Uploads/ImageRepo.php +++ b/app/Uploads/ImageRepo.php @@ -2,7 +2,7 @@ namespace BookStack\Uploads; -use BookStack\Auth\Permissions\PermissionService; +use BookStack\Auth\Permissions\PermissionApplicator; use BookStack\Entities\Models\Page; use BookStack\Exceptions\ImageUploadException; use Exception; @@ -11,16 +11,16 @@ use Symfony\Component\HttpFoundation\File\UploadedFile; class ImageRepo { - protected $imageService; - protected $restrictionService; + protected ImageService $imageService; + protected PermissionApplicator $permissions; /** * ImageRepo constructor. */ - public function __construct(ImageService $imageService, PermissionService $permissionService) + public function __construct(ImageService $imageService, PermissionApplicator $permissions) { $this->imageService = $imageService; - $this->restrictionService = $permissionService; + $this->permissions = $permissions; } /** @@ -74,7 +74,7 @@ class ImageRepo } // Filter by page access - $imageQuery = $this->restrictionService->filterRelatedEntity(Page::class, $imageQuery, 'images', 'uploaded_to'); + $imageQuery = $this->permissions->filterRelatedEntity(Page::class, $imageQuery, 'images', 'uploaded_to'); if ($whereClause !== null) { $imageQuery = $imageQuery->where($whereClause); diff --git a/app/helpers.php b/app/helpers.php index 9edc22c40..cfdf55445 100644 --- a/app/helpers.php +++ b/app/helpers.php @@ -1,6 +1,6 @@ checkOwnableUserAccess($ownable, $permission); + return $permissions->checkOwnableUserAccess($ownable, $permission); } /** @@ -76,9 +76,9 @@ function userCan(string $permission, Model $ownable = null): bool */ function userCanOnAny(string $permission, string $entityClass = null): bool { - $permissionService = app(PermissionService::class); + $permissions = app(PermissionApplicator::class); - return $permissionService->checkUserHasPermissionOnAnything($permission, $entityClass); + return $permissions->checkUserHasPermissionOnAnything($permission, $entityClass); } /** diff --git a/database/seeders/DummyContentSeeder.php b/database/seeders/DummyContentSeeder.php index cb9987df7..e97069e7f 100644 --- a/database/seeders/DummyContentSeeder.php +++ b/database/seeders/DummyContentSeeder.php @@ -69,7 +69,7 @@ class DummyContentSeeder extends Seeder ]); $token->save(); - app(JointPermissionBuilder::class)->buildJointPermissions(); + app(JointPermissionBuilder::class)->rebuildForAll(); app(SearchIndex::class)->indexAllEntities(); } } diff --git a/database/seeders/LargeContentSeeder.php b/database/seeders/LargeContentSeeder.php index 041b3161a..b2d8a4d1b 100644 --- a/database/seeders/LargeContentSeeder.php +++ b/database/seeders/LargeContentSeeder.php @@ -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)->buildJointPermissionsForEntity($largeBook); + app()->make(JointPermissionBuilder::class)->rebuildForEntity($largeBook); app()->make(SearchIndex::class)->indexEntities($all); } } diff --git a/tests/Commands/RegeneratePermissionsCommandTest.php b/tests/Commands/RegeneratePermissionsCommandTest.php index 2090c991b..d514e5f9d 100644 --- a/tests/Commands/RegeneratePermissionsCommandTest.php +++ b/tests/Commands/RegeneratePermissionsCommandTest.php @@ -4,6 +4,7 @@ namespace Tests\Commands; use BookStack\Auth\Permissions\JointPermission; use BookStack\Entities\Models\Page; +use Illuminate\Support\Facades\Artisan; use Illuminate\Support\Facades\DB; use Tests\TestCase; @@ -11,13 +12,13 @@ class RegeneratePermissionsCommandTest extends TestCase { public function test_regen_permissions_command() { - \DB::rollBack(); + DB::rollBack(); JointPermission::query()->truncate(); $page = Page::first(); $this->assertDatabaseMissing('joint_permissions', ['entity_id' => $page->id]); - $exitCode = \Artisan::call('bookstack:regenerate-permissions'); + $exitCode = Artisan::call('bookstack:regenerate-permissions'); $this->assertTrue($exitCode === 0, 'Command executed successfully'); DB::beginTransaction(); diff --git a/tests/PublicActionTest.php b/tests/PublicActionTest.php index 745fa7660..2841f4ef4 100644 --- a/tests/PublicActionTest.php +++ b/tests/PublicActionTest.php @@ -89,7 +89,7 @@ class PublicActionTest extends TestCase foreach (RolePermission::all() as $perm) { $publicRole->attachPermission($perm); } - $this->app->make(JointPermissionBuilder::class)->buildJointPermissionForRole($publicRole); + $this->app->make(JointPermissionBuilder::class)->rebuildForRole($publicRole); /** @var Chapter $chapter */ $chapter = Chapter::query()->first(); diff --git a/tests/SharedTestHelpers.php b/tests/SharedTestHelpers.php index 2d05b0520..99bc92494 100644 --- a/tests/SharedTestHelpers.php +++ b/tests/SharedTestHelpers.php @@ -3,7 +3,7 @@ namespace Tests; use BookStack\Auth\Permissions\JointPermissionBuilder; -use BookStack\Auth\Permissions\PermissionService; +use BookStack\Auth\Permissions\PermissionApplicator; use BookStack\Auth\Permissions\PermissionsRepo; use BookStack\Auth\Permissions\RolePermission; use BookStack\Auth\Role; @@ -177,7 +177,7 @@ trait SharedTestHelpers $entity->save(); $entity->load('permissions'); - $this->app->make(JointPermissionBuilder::class)->buildJointPermissionsForEntity($entity); + $this->app->make(JointPermissionBuilder::class)->rebuildForEntity($entity); $entity->load('jointPermissions'); } @@ -209,7 +209,7 @@ trait SharedTestHelpers /** @var Role $role */ foreach ($roles as $role) { $role->detachPermission($permission); - $permissionBuilder->buildJointPermissionForRole($role); + $permissionBuilder->rebuildForRole($role); } $user->clearPermissionCache(); @@ -243,7 +243,7 @@ trait SharedTestHelpers $chapter = Chapter::factory()->create(array_merge(['book_id' => $book->id], $userAttrs)); $page = Page::factory()->create(array_merge(['book_id' => $book->id, 'chapter_id' => $chapter->id], $userAttrs)); - $this->app->make(JointPermissionBuilder::class)->buildJointPermissionsForEntity($book); + $this->app->make(JointPermissionBuilder::class)->rebuildForEntity($book); return compact('book', 'chapter', 'page'); } From 2989852520ee05581abbf156a0b6aa8aad2cc910 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Tue, 12 Jul 2022 21:13:02 +0100 Subject: [PATCH 04/12] Added simple data model for faster permission generation --- .../Permissions/JointPermissionBuilder.php | 153 +++++++++--------- app/Auth/Permissions/SimpleEntityData.php | 13 ++ tests/Api/AttachmentsApiTest.php | 2 +- 3 files changed, 95 insertions(+), 73 deletions(-) create mode 100644 app/Auth/Permissions/SimpleEntityData.php diff --git a/app/Auth/Permissions/JointPermissionBuilder.php b/app/Auth/Permissions/JointPermissionBuilder.php index fbf77741a..fe25e02ff 100644 --- a/app/Auth/Permissions/JointPermissionBuilder.php +++ b/app/Auth/Permissions/JointPermissionBuilder.php @@ -16,7 +16,7 @@ use Illuminate\Support\Facades\DB; class JointPermissionBuilder { /** - * @var array> + * @var array> */ protected $entityCache; @@ -26,7 +26,6 @@ class JointPermissionBuilder public function rebuildForAll() { JointPermission::query()->truncate(); - $this->readyEntityCache(); // Get all roles (Should be the most limited dimension) $roles = Role::query()->with('permissions')->get()->all(); @@ -45,8 +44,6 @@ class JointPermissionBuilder /** * Rebuild the entity jointPermissions for a particular entity. - * - * @throws Throwable */ public function rebuildForEntity(Entity $entity) { @@ -99,51 +96,39 @@ class JointPermissionBuilder /** * Prepare the local entity cache and ensure it's empty. * - * @param Entity[] $entities + * @param SimpleEntityData[] $entities */ - protected function readyEntityCache(array $entities = []) + protected function readyEntityCache(array $entities) { $this->entityCache = []; foreach ($entities as $entity) { - $class = get_class($entity); - - if (!isset($this->entityCache[$class])) { - $this->entityCache[$class] = []; + if (!isset($this->entityCache[$entity->type])) { + $this->entityCache[$entity->type] = []; } - $this->entityCache[$class][$entity->getRawAttribute('id')] = $entity; + $this->entityCache[$entity->type][$entity->id] = $entity; } } /** * Get a book via ID, Checks local cache. */ - protected function getBook(int $bookId): ?Book + protected function getBook(int $bookId): SimpleEntityData { - if ($this->entityCache[Book::class][$bookId] ?? false) { - return $this->entityCache[Book::class][$bookId]; - } - - return Book::query()->withTrashed()->find($bookId); + return $this->entityCache['book'][$bookId]; } /** * Get a chapter via ID, Checks local cache. */ - protected function getChapter(int $chapterId): ?Chapter + protected function getChapter(int $chapterId): SimpleEntityData { - if ($this->entityCache[Chapter::class][$chapterId] ?? false) { - return $this->entityCache[Chapter::class][$chapterId]; - } - - return Chapter::query() - ->withTrashed() - ->find($chapterId); + return $this->entityCache['chapter'][$chapterId]; } /** - * Get a query for fetching a book with it's children. + * Get a query for fetching a book with its children. */ protected function bookFetchQuery(): Builder { @@ -161,8 +146,6 @@ class JointPermissionBuilder /** * Build joint permissions for the given book and role combinations. - * - * @throws Throwable */ protected function buildJointPermissionsForBooks(EloquentCollection $books, array $roles, bool $deleteOld = false) { @@ -187,8 +170,6 @@ class JointPermissionBuilder /** * Rebuild the entity jointPermissions for a collection of entities. - * - * @throws Throwable */ protected function buildJointPermissionsForEntities(array $entities) { @@ -201,12 +182,11 @@ class JointPermissionBuilder * Delete all the entity jointPermissions for a list of entities. * * @param Entity[] $entities - * - * @throws Throwable */ protected function deleteManyJointPermissionsForEntities(array $entities) { - $idsByType = $this->entitiesToTypeIdMap($entities); + $simpleEntities = $this->entitiesToSimpleEntities($entities); + $idsByType = $this->entitiesToTypeIdMap($simpleEntities); DB::transaction(function () use ($idsByType) { foreach ($idsByType as $type => $ids) { @@ -220,23 +200,45 @@ class JointPermissionBuilder }); } + /** + * @param Entity[] $entities + * @return SimpleEntityData[] + */ + protected function entitiesToSimpleEntities(array $entities): array + { + $simpleEntities = []; + + foreach ($entities as $entity) { + $attrs = $entity->getAttributes(); + $simple = new SimpleEntityData(); + $simple->id = $attrs['id']; + $simple->type = $entity->getMorphClass(); + $simple->restricted = boolval($attrs['restricted'] ?? 0); + $simple->owned_by = $attrs['owned_by'] ?? 0; + $simple->book_id = $attrs['book_id'] ?? null; + $simple->chapter_id = $attrs['chapter_id'] ?? null; + $simpleEntities[] = $simple; + } + + return $simpleEntities; + } + /** * Create & Save entity jointPermissions for many entities and roles. * * @param Entity[] $entities * @param Role[] $roles - * - * @throws Throwable */ - protected function createManyJointPermissions(array $entities, array $roles) + protected function createManyJointPermissions(array $originalEntities, array $roles) { + $entities = $this->entitiesToSimpleEntities($originalEntities); $this->readyEntityCache($entities); $jointPermissions = []; // Create a mapping of entity restricted statuses $entityRestrictedMap = []; foreach ($entities as $entity) { - $entityRestrictedMap[$entity->getMorphClass() . ':' . $entity->getRawAttribute('id')] = boolval($entity->getRawAttribute('restricted')); + $entityRestrictedMap[$entity->type . ':' . $entity->id] = $entity->restricted; } // Fetch related entity permissions @@ -262,7 +264,14 @@ class JointPermissionBuilder foreach ($entities as $entity) { foreach ($roles as $role) { foreach ($this->getActions($entity) as $action) { - $jointPermissions[] = $this->createJointPermissionData($entity, $role, $action, $permissionMap, $rolePermissionMap); + $jointPermissions[] = $this->createJointPermissionData( + $entity, + $role->getRawAttribute('id'), + $action, + $permissionMap, + $rolePermissionMap, + $role->system_name === 'admin' + ); } } } @@ -277,7 +286,7 @@ class JointPermissionBuilder /** * From the given entity list, provide back a mapping of entity types to * the ids of that given type. The type used is the DB morph class. - * @param Entity[] $entities + * @param SimpleEntityData[] $entities * @return array */ protected function entitiesToTypeIdMap(array $entities): array @@ -285,13 +294,11 @@ class JointPermissionBuilder $idsByType = []; foreach ($entities as $entity) { - $type = $entity->getMorphClass(); - - if (!isset($idsByType[$type])) { - $idsByType[$type] = []; + if (!isset($idsByType[$entity->type])) { + $idsByType[$entity->type] = []; } - $idsByType[$type][] = $entity->getRawAttribute('id'); + $idsByType[$entity->type][] = $entity->id; } return $idsByType; @@ -299,10 +306,10 @@ class JointPermissionBuilder /** * Get the entity permissions for all the given entities - * @param Entity[] $entities - * @return EloquentCollection + * @param SimpleEntityData[] $entities + * @return EntityPermission[] */ - protected function getEntityPermissionsForEntities(array $entities) + protected function getEntityPermissionsForEntities(array $entities): array { $idsByType = $this->entitiesToTypeIdMap($entities); $permissionFetch = EntityPermission::query(); @@ -313,19 +320,21 @@ class JointPermissionBuilder }); } - return $permissionFetch->get(); + return $permissionFetch->get()->all(); } /** * Get the actions related to an entity. */ - protected function getActions(Entity $entity): array + protected function getActions(SimpleEntityData $entity): array { $baseActions = ['view', 'update', 'delete']; - if ($entity instanceof Chapter || $entity instanceof Book) { + + if ($entity->type === 'chapter' || $entity->type === 'book') { $baseActions[] = 'page-create'; } - if ($entity instanceof Book) { + + if ($entity->type === 'book') { $baseActions[] = 'chapter-create'; } @@ -336,45 +345,45 @@ class JointPermissionBuilder * Create entity permission data for an entity and role * for a particular action. */ - protected function createJointPermissionData(Entity $entity, Role $role, string $action, array $permissionMap, array $rolePermissionMap): array + protected function createJointPermissionData(SimpleEntityData $entity, int $roleId, string $action, array $permissionMap, array $rolePermissionMap, bool $isAdminRole): array { - $permissionPrefix = (strpos($action, '-') === false ? ($entity->getType() . '-') : '') . $action; - $roleHasPermission = isset($rolePermissionMap[$role->getRawAttribute('id') . ':' . $permissionPrefix . '-all']); - $roleHasPermissionOwn = isset($rolePermissionMap[$role->getRawAttribute('id') . ':' . $permissionPrefix . '-own']); + $permissionPrefix = (strpos($action, '-') === false ? ($entity->type . '-') : '') . $action; + $roleHasPermission = isset($rolePermissionMap[$roleId . ':' . $permissionPrefix . '-all']); + $roleHasPermissionOwn = isset($rolePermissionMap[$roleId . ':' . $permissionPrefix . '-own']); $explodedAction = explode('-', $action); $restrictionAction = end($explodedAction); - if ($role->system_name === 'admin') { - return $this->createJointPermissionDataArray($entity, $role, $action, true, true); + if ($isAdminRole) { + return $this->createJointPermissionDataArray($entity, $roleId, $action, true, true); } if ($entity->restricted) { - $hasAccess = $this->mapHasActiveRestriction($permissionMap, $entity, $role, $restrictionAction); + $hasAccess = $this->mapHasActiveRestriction($permissionMap, $entity, $roleId, $restrictionAction); - return $this->createJointPermissionDataArray($entity, $role, $action, $hasAccess, $hasAccess); + return $this->createJointPermissionDataArray($entity, $roleId, $action, $hasAccess, $hasAccess); } - if ($entity instanceof Book || $entity instanceof Bookshelf) { - return $this->createJointPermissionDataArray($entity, $role, $action, $roleHasPermission, $roleHasPermissionOwn); + if ($entity->type === 'book' || $entity->type === 'bookshelf') { + return $this->createJointPermissionDataArray($entity, $roleId, $action, $roleHasPermission, $roleHasPermissionOwn); } // For chapters and pages, Check if explicit permissions are set on the Book. $book = $this->getBook($entity->book_id); - $hasExplicitAccessToParents = $this->mapHasActiveRestriction($permissionMap, $book, $role, $restrictionAction); + $hasExplicitAccessToParents = $this->mapHasActiveRestriction($permissionMap, $book, $roleId, $restrictionAction); $hasPermissiveAccessToParents = !$book->restricted; // For pages with a chapter, Check if explicit permissions are set on the Chapter - if ($entity instanceof Page && intval($entity->chapter_id) !== 0) { + if ($entity->type === 'page' && $entity->chapter_id !== 0) { $chapter = $this->getChapter($entity->chapter_id); $hasPermissiveAccessToParents = $hasPermissiveAccessToParents && !$chapter->restricted; if ($chapter->restricted) { - $hasExplicitAccessToParents = $this->mapHasActiveRestriction($permissionMap, $chapter, $role, $restrictionAction); + $hasExplicitAccessToParents = $this->mapHasActiveRestriction($permissionMap, $chapter, $roleId, $restrictionAction); } } return $this->createJointPermissionDataArray( $entity, - $role, + $roleId, $action, ($hasExplicitAccessToParents || ($roleHasPermission && $hasPermissiveAccessToParents)), ($hasExplicitAccessToParents || ($roleHasPermissionOwn && $hasPermissiveAccessToParents)) @@ -384,9 +393,9 @@ class JointPermissionBuilder /** * Check for an active restriction in an entity map. */ - protected function mapHasActiveRestriction(array $entityMap, Entity $entity, Role $role, string $action): bool + protected function mapHasActiveRestriction(array $entityMap, SimpleEntityData $entity, int $roleId, string $action): bool { - $key = $entity->getMorphClass() . ':' . $entity->getRawAttribute('id') . ':' . $role->getRawAttribute('id') . ':' . $action; + $key = $entity->type . ':' . $entity->id . ':' . $roleId . ':' . $action; return $entityMap[$key] ?? false; } @@ -395,16 +404,16 @@ class JointPermissionBuilder * Create an array of data with the information of an entity jointPermissions. * Used to build data for bulk insertion. */ - protected function createJointPermissionDataArray(Entity $entity, Role $role, string $action, bool $permissionAll, bool $permissionOwn): array + protected function createJointPermissionDataArray(SimpleEntityData $entity, int $roleId, string $action, bool $permissionAll, bool $permissionOwn): array { return [ 'action' => $action, - 'entity_id' => $entity->getRawAttribute('id'), - 'entity_type' => $entity->getMorphClass(), + 'entity_id' => $entity->id, + 'entity_type' => $entity->type, 'has_permission' => $permissionAll, 'has_permission_own' => $permissionOwn, - 'owned_by' => $entity->getRawAttribute('owned_by'), - 'role_id' => $role->getRawAttribute('id'), + 'owned_by' => $entity->owned_by, + 'role_id' => $roleId, ]; } diff --git a/app/Auth/Permissions/SimpleEntityData.php b/app/Auth/Permissions/SimpleEntityData.php new file mode 100644 index 000000000..0d1c94b0d --- /dev/null +++ b/app/Auth/Permissions/SimpleEntityData.php @@ -0,0 +1,13 @@ +first(); $page->draft = true; - $page->owned_by = $editor; + $page->owned_by = $editor->id; $page->save(); $this->regenEntityPermissions($page); From 4fb85a9a5cd75f1f0a0f605a916d4c3a746ee672 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Wed, 13 Jul 2022 15:23:03 +0100 Subject: [PATCH 05/12] Started removal of non-view permission queries Updated ajax search and entity selector usage to display and handle items that the user does not have permission to interact with. Started logic changes to not allow permission type to be passed around, with views instead being the fixed sole permission. --- app/Auth/Permissions/PermissionApplicator.php | 5 +++-- app/Auth/User.php | 2 +- app/Entities/Queries/Popular.php | 4 ++-- app/Entities/Tools/SearchRunner.php | 8 ++++---- app/Http/Controllers/SearchController.php | 7 +++---- resources/sass/_lists.scss | 8 ++++++++ resources/sass/_variables.scss | 6 ++++++ resources/views/entities/list-item.blade.php | 8 +++++++- resources/views/search/parts/entity-ajax-list.blade.php | 7 ++++++- routes/web.php | 7 +++++++ 10 files changed, 47 insertions(+), 15 deletions(-) diff --git a/app/Auth/Permissions/PermissionApplicator.php b/app/Auth/Permissions/PermissionApplicator.php index 3dc529e32..40a7f6116 100644 --- a/app/Auth/Permissions/PermissionApplicator.php +++ b/app/Auth/Permissions/PermissionApplicator.php @@ -72,6 +72,7 @@ class PermissionApplicator $action = $permission; } + // TODO - Use a non-query based check $hasAccess = $this->entityRestrictionQuery($baseQuery, $action)->count() > 0; $this->clean(); @@ -163,14 +164,14 @@ class PermissionApplicator /** * Add restrictions for a generic entity. */ - public function enforceEntityRestrictions(Entity $entity, Builder $query, string $action = 'view'): Builder + public function enforceEntityRestrictions(Entity $entity, Builder $query): Builder { if ($entity instanceof Page) { // Prevent drafts being visible to others. $this->enforceDraftVisibilityOnQuery($query); } - return $this->entityRestrictionQuery($query, $action); + return $this->entityRestrictionQuery($query, 'view'); } /** diff --git a/app/Auth/User.php b/app/Auth/User.php index 4e2183244..c060d5ec8 100644 --- a/app/Auth/User.php +++ b/app/Auth/User.php @@ -163,7 +163,7 @@ class User extends Model implements AuthenticatableContract, CanResetPasswordCon } /** - * Get all permissions belonging to a the current user. + * Get all permissions belonging to the current user. */ protected function permissions(): Collection { diff --git a/app/Entities/Queries/Popular.php b/app/Entities/Queries/Popular.php index e6b22a1c9..66006df1b 100644 --- a/app/Entities/Queries/Popular.php +++ b/app/Entities/Queries/Popular.php @@ -7,10 +7,10 @@ use Illuminate\Support\Facades\DB; class Popular extends EntityQuery { - public function run(int $count, int $page, array $filterModels = null, string $action = 'view') + public function run(int $count, int $page, array $filterModels = null) { $query = $this->permissionService() - ->filterRestrictedEntityRelations(View::query(), 'views', 'viewable_id', 'viewable_type', $action) + ->filterRestrictedEntityRelations(View::query(), 'views', 'viewable_id', 'viewable_type', 'view') ->select('*', 'viewable_id', 'viewable_type', DB::raw('SUM(views) as view_count')) ->groupBy('viewable_id', 'viewable_type') ->orderBy('view_count', 'desc'); diff --git a/app/Entities/Tools/SearchRunner.php b/app/Entities/Tools/SearchRunner.php index 5d94379c9..1dcc2eb44 100644 --- a/app/Entities/Tools/SearchRunner.php +++ b/app/Entities/Tools/SearchRunner.php @@ -54,7 +54,7 @@ class SearchRunner * * @return array{total: int, count: int, has_more: bool, results: Entity[]} */ - public function searchEntities(SearchOptions $searchOpts, string $entityType = 'all', int $page = 1, int $count = 20, string $action = 'view'): array + public function searchEntities(SearchOptions $searchOpts, string $entityType = 'all', int $page = 1, int $count = 20): array { $entityTypes = array_keys($this->entityProvider->all()); $entityTypesToSearch = $entityTypes; @@ -75,7 +75,7 @@ class SearchRunner } $entityModelInstance = $this->entityProvider->get($entityType); - $searchQuery = $this->buildQuery($searchOpts, $entityModelInstance, $action); + $searchQuery = $this->buildQuery($searchOpts, $entityModelInstance); $entityTotal = $searchQuery->count(); $searchResults = $this->getPageOfDataFromQuery($searchQuery, $entityModelInstance, $page, $count); @@ -159,7 +159,7 @@ class SearchRunner /** * Create a search query for an entity. */ - protected function buildQuery(SearchOptions $searchOpts, Entity $entityModelInstance, string $action = 'view'): EloquentBuilder + protected function buildQuery(SearchOptions $searchOpts, Entity $entityModelInstance): EloquentBuilder { $entityQuery = $entityModelInstance->newQuery(); @@ -193,7 +193,7 @@ class SearchRunner } } - return $this->permissions->enforceEntityRestrictions($entityModelInstance, $entityQuery, $action); + return $this->permissions->enforceEntityRestrictions($entityModelInstance, $entityQuery); } /** diff --git a/app/Http/Controllers/SearchController.php b/app/Http/Controllers/SearchController.php index 6b2be5a2d..4a002298c 100644 --- a/app/Http/Controllers/SearchController.php +++ b/app/Http/Controllers/SearchController.php @@ -12,7 +12,6 @@ use Illuminate\Http\Request; class SearchController extends Controller { protected $searchRunner; - protected $entityContextManager; public function __construct(SearchRunner $searchRunner) { @@ -79,12 +78,12 @@ class SearchController extends Controller // Search for entities otherwise show most popular if ($searchTerm !== false) { $searchTerm .= ' {type:' . implode('|', $entityTypes) . '}'; - $entities = $this->searchRunner->searchEntities(SearchOptions::fromString($searchTerm), 'all', 1, 20, $permission)['results']; + $entities = $this->searchRunner->searchEntities(SearchOptions::fromString($searchTerm), 'all', 1, 20)['results']; } else { - $entities = (new Popular())->run(20, 0, $entityTypes, $permission); + $entities = (new Popular())->run(20, 0, $entityTypes); } - return view('search.parts.entity-ajax-list', ['entities' => $entities]); + return view('search.parts.entity-ajax-list', ['entities' => $entities, 'permission' => $permission]); } /** diff --git a/resources/sass/_lists.scss b/resources/sass/_lists.scss index 19060fbbf..5e251f9c7 100644 --- a/resources/sass/_lists.scss +++ b/resources/sass/_lists.scss @@ -443,6 +443,14 @@ ul.pagination { } } +.entity-list-item.disabled { + pointer-events: none; + cursor: not-allowed; + opacity: 0.8; + user-select: none; + background: var(--bg-disabled); +} + .entity-list-item-path-sep { display: inline-block; vertical-align: top; diff --git a/resources/sass/_variables.scss b/resources/sass/_variables.scss index 6b57147ef..3cb2dd4ed 100644 --- a/resources/sass/_variables.scss +++ b/resources/sass/_variables.scss @@ -45,6 +45,12 @@ $fs-s: 12px; --color-chapter: #af4d0d; --color-book: #077b70; --color-bookshelf: #a94747; + + --bg-disabled: url("data:image/svg+xml,%3Csvg xmlns='http://www.w3.org/2000/svg' height='100%25' width='100%25'%3E%3Cdefs%3E%3Cpattern id='doodad' width='19' height='19' viewBox='0 0 40 40' patternUnits='userSpaceOnUse' patternTransform='rotate(143)'%3E%3Crect width='100%25' height='100%25' fill='rgba(42, 67, 101,0)'/%3E%3Cpath d='M-10 30h60v20h-60zM-10-10h60v20h-60' fill='rgba(26, 32, 44,0)'/%3E%3Cpath d='M-10 10h60v20h-60zM-10-30h60v20h-60z' fill='rgba(0, 0, 0,0.05)'/%3E%3C/pattern%3E%3C/defs%3E%3Crect fill='url(%23doodad)' height='200%25' width='200%25'/%3E%3C/svg%3E"); +} + +:root.dark-mode { + --bg-disabled: url("data:image/svg+xml,%3Csvg xmlns='http://www.w3.org/2000/svg' height='100%25' width='100%25'%3E%3Cdefs%3E%3Cpattern id='doodad' width='19' height='19' viewBox='0 0 40 40' patternUnits='userSpaceOnUse' patternTransform='rotate(143)'%3E%3Crect width='100%25' height='100%25' fill='rgba(42, 67, 101,0)'/%3E%3Cpath d='M-10 30h60v20h-60zM-10-10h60v20h-60' fill='rgba(26, 32, 44,0)'/%3E%3Cpath d='M-10 10h60v20h-60zM-10-30h60v20h-60z' fill='rgba(255, 255, 255,0.05)'/%3E%3C/pattern%3E%3C/defs%3E%3Crect fill='url(%23doodad)' height='200%25' width='200%25'/%3E%3C/svg%3E"); } $positive: #0f7d15; diff --git a/resources/views/entities/list-item.blade.php b/resources/views/entities/list-item.blade.php index 44e06753d..5314c8446 100644 --- a/resources/views/entities/list-item.blade.php +++ b/resources/views/entities/list-item.blade.php @@ -1,7 +1,13 @@ -@component('entities.list-item-basic', ['entity' => $entity]) +@component('entities.list-item-basic', ['entity' => $entity, 'classes' => (($locked ?? false) ? 'disabled ' : '') . ($classes ?? '') ])
+ @if($locked ?? false) +
+ @icon('lock')You don't have the required permissions to select this item. +
+ @endif + @if($showPath ?? false) @if($entity->relationLoaded('book') && $entity->book) {{ $entity->book->getShortName(42) }} diff --git a/resources/views/search/parts/entity-ajax-list.blade.php b/resources/views/search/parts/entity-ajax-list.blade.php index a4eedf75e..9340ccdc5 100644 --- a/resources/views/search/parts/entity-ajax-list.blade.php +++ b/resources/views/search/parts/entity-ajax-list.blade.php @@ -2,7 +2,12 @@ @if(count($entities) > 0) @foreach($entities as $index => $entity) - @include('entities.list-item', ['entity' => $entity, 'showPath' => true]) + @include('entities.list-item', [ + 'entity' => $entity, + 'showPath' => true, + 'locked' => $permission !== 'view' && !userCan($permission, $entity) + ]) + @if($index !== count($entities) - 1)
@endif diff --git a/routes/web.php b/routes/web.php index 5e16e5333..9b562703c 100644 --- a/routes/web.php +++ b/routes/web.php @@ -38,6 +38,13 @@ use Illuminate\View\Middleware\ShareErrorsFromSession; Route::get('/status', [StatusController::class, 'show']); Route::get('/robots.txt', [HomeController::class, 'robots']); +Route::get('/test', function() { + $book = \BookStack\Entities\Models\Book::query()->where('slug', '=', 'k5TrhXxaNb')->firstOrFail(); + $builder= app()->make(\BookStack\Auth\Permissions\JointPermissionBuilder::class); + $builder->rebuildForEntity($book); + return 'finished'; +})->withoutMiddleware('web'); + // Authenticated routes... Route::middleware('auth')->group(function () { From 1d875ccfb7c62854c9c3253a0d83b39310fefbf8 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 16 Jul 2022 13:17:08 +0100 Subject: [PATCH 06/12] Continued removal of joint permission non-view queries Cleaned up PermissionApplicator to remove old cache system which was hardly ever actuall caching anything since it was reset after each public method run. Changed the scope of 'userCanOnAny' to just check entity permissions, and added protections of action scope creep, in case a role permission action was passed by mistake. --- app/Auth/Permissions/PermissionApplicator.php | 109 ++++++------------ app/Entities/Models/Entity.php | 11 +- app/Entities/Queries/Popular.php | 2 +- app/Entities/Queries/RecentlyViewed.php | 3 +- app/Entities/Queries/TopFavourites.php | 2 +- app/Http/Controllers/BookshelfController.php | 4 +- app/helpers.php | 8 +- resources/views/chapters/show.blade.php | 2 +- resources/views/pages/show.blade.php | 2 +- 9 files changed, 49 insertions(+), 94 deletions(-) diff --git a/app/Auth/Permissions/PermissionApplicator.php b/app/Auth/Permissions/PermissionApplicator.php index 40a7f6116..cf95f2854 100644 --- a/app/Auth/Permissions/PermissionApplicator.php +++ b/app/Auth/Permissions/PermissionApplicator.php @@ -11,37 +11,10 @@ use BookStack\Traits\HasCreatorAndUpdater; use BookStack\Traits\HasOwner; use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Query\Builder as QueryBuilder; +use InvalidArgumentException; class PermissionApplicator { - /** - * @var ?array - */ - protected $userRoles = null; - - /** - * @var ?User - */ - protected $currentUserModel = null; - - /** - * Get the roles for the current logged in user. - */ - protected function getCurrentUserRoles(): array - { - if (!is_null($this->userRoles)) { - return $this->userRoles; - } - - if (auth()->guest()) { - $this->userRoles = [Role::getSystemRole('public')->id]; - } else { - $this->userRoles = $this->currentUser()->roles->pluck('id')->values()->all(); - } - - return $this->userRoles; - } - /** * Checks if an entity has a restriction set upon it. * @@ -74,7 +47,6 @@ class PermissionApplicator // TODO - Use a non-query based check $hasAccess = $this->entityRestrictionQuery($baseQuery, $action)->count() > 0; - $this->clean(); return $hasAccess; } @@ -83,25 +55,23 @@ class PermissionApplicator * Checks if a user has the given permission for any items in the system. * Can be passed an entity instance to filter on a specific type. */ - public function checkUserHasPermissionOnAnything(string $permission, ?string $entityClass = null): bool + public function checkUserHasEntityPermissionOnAny(string $action, string $entityClass = ''): bool { - $userRoleIds = $this->currentUser()->roles()->select('id')->pluck('id')->toArray(); - $userId = $this->currentUser()->id; + if (strpos($action, '-') !== false) { + throw new InvalidArgumentException("Action should be a simple entity permission action, not a role permission"); + } - $permissionQuery = JointPermission::query() - ->where('action', '=', $permission) - ->whereIn('role_id', $userRoleIds) - ->where(function (Builder $query) use ($userId) { - $this->addJointHasPermissionCheck($query, $userId); - }); + $permissionQuery = EntityPermission::query() + ->where('action', '=', $action) + ->whereIn('role_id', $this->getCurrentUserRoleIds()); - if (!is_null($entityClass)) { - $entityInstance = app($entityClass); - $permissionQuery = $permissionQuery->where('entity_type', '=', $entityInstance->getMorphClass()); + if (!empty($entityClass)) { + /** @var Entity $entityInstance */ + $entityInstance = app()->make($entityClass); + $permissionQuery = $permissionQuery->where('restrictable_type', '=', $entityInstance->getMorphClass()); } $hasPermission = $permissionQuery->count() > 0; - $this->clean(); return $hasPermission; } @@ -114,7 +84,8 @@ class PermissionApplicator { $q = $query->where(function ($parentQuery) use ($action) { $parentQuery->whereHas('jointPermissions', function ($permissionQuery) use ($action) { - $permissionQuery->whereIn('role_id', $this->getCurrentUserRoles()) + $permissionQuery->whereIn('role_id', $this->getCurrentUserRoleIds()) + // TODO - Delete line once only views ->where('action', '=', $action) ->where(function (Builder $query) { $this->addJointHasPermissionCheck($query, $this->currentUser()->id); @@ -122,23 +93,20 @@ class PermissionApplicator }); }); - $this->clean(); - return $q; } /** * Limited the given entity query so that the query will only - * return items that the user has permission for the given ability. + * return items that the user has view permission for. */ - public function restrictEntityQuery(Builder $query, string $ability = 'view'): Builder + public function restrictEntityQuery(Builder $query): Builder { - $this->clean(); - - return $query->where(function (Builder $parentQuery) use ($ability) { - $parentQuery->whereHas('jointPermissions', function (Builder $permissionQuery) use ($ability) { - $permissionQuery->whereIn('role_id', $this->getCurrentUserRoles()) - ->where('action', '=', $ability) + return $query->where(function (Builder $parentQuery) { + $parentQuery->whereHas('jointPermissions', function (Builder $permissionQuery) { + $permissionQuery->whereIn('role_id', $this->getCurrentUserRoleIds()) + // TODO - Delete line once only views + ->where('action', '=', 'view') ->where(function (Builder $query) { $this->addJointHasPermissionCheck($query, $this->currentUser()->id); }); @@ -181,18 +149,18 @@ class PermissionApplicator * * @param Builder|QueryBuilder $query */ - public function filterRestrictedEntityRelations($query, string $tableName, string $entityIdColumn, string $entityTypeColumn, string $action = 'view') + public function filterRestrictedEntityRelations($query, string $tableName, string $entityIdColumn, string $entityTypeColumn) { $tableDetails = ['tableName' => $tableName, 'entityIdColumn' => $entityIdColumn, 'entityTypeColumn' => $entityTypeColumn]; $pageMorphClass = (new Page())->getMorphClass(); - $q = $query->whereExists(function ($permissionQuery) use (&$tableDetails, $action) { + $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']) - ->where('joint_permissions.action', '=', $action) - ->whereIn('joint_permissions.role_id', $this->getCurrentUserRoles()) + ->where('joint_permissions.action', '=', 'view') + ->whereIn('joint_permissions.role_id', $this->getCurrentUserRoleIds()) ->where(function (QueryBuilder $query) { $this->addJointHasPermissionCheck($query, $this->currentUser()->id); }); @@ -207,8 +175,6 @@ class PermissionApplicator }); }); - $this->clean(); - return $q; } @@ -228,7 +194,7 @@ class PermissionApplicator ->whereColumn('joint_permissions.entity_id', '=', $fullEntityIdColumn) ->where('joint_permissions.entity_type', '=', $morphClass) ->where('joint_permissions.action', '=', 'view') - ->whereIn('joint_permissions.role_id', $this->getCurrentUserRoles()) + ->whereIn('joint_permissions.role_id', $this->getCurrentUserRoleIds()) ->where(function (QueryBuilder $query) { $this->addJointHasPermissionCheck($query, $this->currentUser()->id); }); @@ -251,8 +217,6 @@ class PermissionApplicator }); } - $this->clean(); - return $q; } @@ -273,21 +237,22 @@ class PermissionApplicator /** * Get the current user. */ - private function currentUser(): User + protected function currentUser(): User { - if (is_null($this->currentUserModel)) { - $this->currentUserModel = user(); - } - - return $this->currentUserModel; + return user(); } /** - * Clean the cached user elements. + * Get the roles for the current logged-in user. + * + * @return int[] */ - private function clean(): void + protected function getCurrentUserRoleIds(): array { - $this->currentUserModel = null; - $this->userRoles = null; + if (auth()->guest()) { + return [Role::getSystemRole('public')->id]; + } + + return $this->currentUser()->roles->pluck('id')->values()->all(); } } diff --git a/app/Entities/Models/Entity.php b/app/Entities/Models/Entity.php index 84d31d82d..17f018a56 100644 --- a/app/Entities/Models/Entity.php +++ b/app/Entities/Models/Entity.php @@ -44,7 +44,6 @@ use Illuminate\Database\Eloquent\SoftDeletes; * @property Collection $tags * * @method static Entity|Builder visible() - * @method static Entity|Builder hasPermission(string $permission) * @method static Builder withLastView() * @method static Builder withViewCount() */ @@ -69,15 +68,7 @@ abstract class Entity extends Model implements Sluggable, Favouritable, Viewable */ public function scopeVisible(Builder $query): Builder { - return $this->scopeHasPermission($query, 'view'); - } - - /** - * Scope the query to those entities that the current user has the given permission for. - */ - public function scopeHasPermission(Builder $query, string $permission) - { - return app()->make(PermissionApplicator::class)->restrictEntityQuery($query, $permission); + return app()->make(PermissionApplicator::class)->restrictEntityQuery($query); } /** diff --git a/app/Entities/Queries/Popular.php b/app/Entities/Queries/Popular.php index 66006df1b..82786e3c6 100644 --- a/app/Entities/Queries/Popular.php +++ b/app/Entities/Queries/Popular.php @@ -10,7 +10,7 @@ class Popular extends EntityQuery public function run(int $count, int $page, array $filterModels = null) { $query = $this->permissionService() - ->filterRestrictedEntityRelations(View::query(), 'views', 'viewable_id', 'viewable_type', 'view') + ->filterRestrictedEntityRelations(View::query(), 'views', 'viewable_id', 'viewable_type') ->select('*', 'viewable_id', 'viewable_type', DB::raw('SUM(views) as view_count')) ->groupBy('viewable_id', 'viewable_type') ->orderBy('view_count', 'desc'); diff --git a/app/Entities/Queries/RecentlyViewed.php b/app/Entities/Queries/RecentlyViewed.php index 5a29ecd72..38d1f1be4 100644 --- a/app/Entities/Queries/RecentlyViewed.php +++ b/app/Entities/Queries/RecentlyViewed.php @@ -18,8 +18,7 @@ class RecentlyViewed extends EntityQuery View::query(), 'views', 'viewable_id', - 'viewable_type', - 'view' + 'viewable_type' ) ->orderBy('views.updated_at', 'desc') ->where('user_id', '=', user()->id); diff --git a/app/Entities/Queries/TopFavourites.php b/app/Entities/Queries/TopFavourites.php index 7522a894d..5d138679a 100644 --- a/app/Entities/Queries/TopFavourites.php +++ b/app/Entities/Queries/TopFavourites.php @@ -15,7 +15,7 @@ class TopFavourites extends EntityQuery } $query = $this->permissionService() - ->filterRestrictedEntityRelations(Favourite::query(), 'favourites', 'favouritable_id', 'favouritable_type', 'view') + ->filterRestrictedEntityRelations(Favourite::query(), 'favourites', 'favouritable_id', 'favouritable_type') ->select('favourites.*') ->leftJoin('views', function (JoinClause $join) { $join->on('favourites.favouritable_id', '=', 'views.viewable_id'); diff --git a/app/Http/Controllers/BookshelfController.php b/app/Http/Controllers/BookshelfController.php index a294bf731..18adaa627 100644 --- a/app/Http/Controllers/BookshelfController.php +++ b/app/Http/Controllers/BookshelfController.php @@ -68,7 +68,7 @@ class BookshelfController extends Controller public function create() { $this->checkPermission('bookshelf-create-all'); - $books = Book::hasPermission('update')->get(); + $books = Book::visible()->get(); $this->setPageTitle(trans('entities.shelves_create')); return view('shelves.create', ['books' => $books]); @@ -139,7 +139,7 @@ class BookshelfController extends Controller $this->checkOwnablePermission('bookshelf-update', $shelf); $shelfBookIds = $shelf->books()->get(['id'])->pluck('id'); - $books = Book::hasPermission('update')->whereNotIn('id', $shelfBookIds)->get(); + $books = Book::visible()->whereNotIn('id', $shelfBookIds)->get(); $this->setPageTitle(trans('entities.shelves_edit_named', ['name' => $shelf->getShortName()])); diff --git a/app/helpers.php b/app/helpers.php index cfdf55445..191eddf4d 100644 --- a/app/helpers.php +++ b/app/helpers.php @@ -71,14 +71,14 @@ function userCan(string $permission, Model $ownable = null): bool } /** - * Check if the current user has the given permission - * on any item in the system. + * Check if the current user can perform the given action on any items in the system. + * Can be provided the class name of an entity to filter ability to that specific entity type. */ -function userCanOnAny(string $permission, string $entityClass = null): bool +function userCanOnAny(string $action, string $entityClass = ''): bool { $permissions = app(PermissionApplicator::class); - return $permissions->checkUserHasPermissionOnAnything($permission, $entityClass); + return $permissions->checkUserHasEntityPermissionOnAny($action, $entityClass); } /** diff --git a/resources/views/chapters/show.blade.php b/resources/views/chapters/show.blade.php index 3e015616a..8a86900fb 100644 --- a/resources/views/chapters/show.blade.php +++ b/resources/views/chapters/show.blade.php @@ -120,7 +120,7 @@ {{ trans('common.edit') }} @endif - @if(userCanOnAny('chapter-create')) + @if(userCanOnAny('create', \BookStack\Entities\Models\Book::class) || userCan('chapter-create-all') || userCan('chapter-create-own')) @icon('copy') {{ trans('common.copy') }} diff --git a/resources/views/pages/show.blade.php b/resources/views/pages/show.blade.php index 2a71c6021..f1aed730b 100644 --- a/resources/views/pages/show.blade.php +++ b/resources/views/pages/show.blade.php @@ -148,7 +148,7 @@ {{ trans('common.edit') }} @endif - @if(userCanOnAny('page-create')) + @if(userCanOnAny('create', \BookStack\Entities\Models\Book::class) || userCanOnAny('create', \BookStack\Entities\Models\Chapter::class) || userCan('page-create-all') || userCan('page-create-own')) @icon('copy') {{ trans('common.copy') }} From f459a68535fc42ef5079e2514b82bff28504fc50 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 16 Jul 2022 19:28:04 +0100 Subject: [PATCH 07/12] Removed remaining dynamic action usages in joint permission queries --- app/Auth/Permissions/PermissionApplicator.php | 100 +++++++++++------- 1 file changed, 64 insertions(+), 36 deletions(-) diff --git a/app/Auth/Permissions/PermissionApplicator.php b/app/Auth/Permissions/PermissionApplicator.php index cf95f2854..e73db5157 100644 --- a/app/Auth/Permissions/PermissionApplicator.php +++ b/app/Auth/Permissions/PermissionApplicator.php @@ -4,6 +4,7 @@ namespace BookStack\Auth\Permissions; use BookStack\Auth\Role; use BookStack\Auth\User; +use BookStack\Entities\Models\Chapter; use BookStack\Entities\Models\Entity; use BookStack\Entities\Models\Page; use BookStack\Model; @@ -23,32 +24,59 @@ class PermissionApplicator public function checkOwnableUserAccess(Model $ownable, string $permission): bool { $explodedPermission = explode('-', $permission); - - $baseQuery = $ownable->newQuery()->where('id', '=', $ownable->id); - $action = end($explodedPermission); + $action = $explodedPermission[1] ?? $explodedPermission[0]; $user = $this->currentUser(); + $userRoleIds = $this->getCurrentUserRoleIds(); + $allRolePermission = $user->can($permission . '-all'); + $ownRolePermission = $user->can($permission . '-own'); $nonJointPermissions = ['restrictions', 'image', 'attachment', 'comment']; + $ownerField = ($ownable instanceof Entity) ? 'owned_by' : 'created_by'; + $isOwner = $user->id === $ownable->getAttribute($ownerField); + $hasRolePermission = $allRolePermission || ($isOwner && $ownRolePermission); // Handle non entity specific jointPermissions if (in_array($explodedPermission[0], $nonJointPermissions)) { - $allPermission = $user && $user->can($permission . '-all'); - $ownPermission = $user && $user->can($permission . '-own'); - $ownerField = ($ownable instanceof Entity) ? 'owned_by' : 'created_by'; - $isOwner = $user && $user->id === $ownable->$ownerField; - - return $allPermission || ($isOwner && $ownPermission); + return $hasRolePermission; } - // Handle abnormal create jointPermissions - if ($action === 'create') { - $action = $permission; + $entityPermissions = $this->getApplicableEntityPermissions($ownable, $userRoleIds, $action); + if (is_null($entityPermissions)) { + return $hasRolePermission; } - // TODO - Use a non-query based check - $hasAccess = $this->entityRestrictionQuery($baseQuery, $action)->count() > 0; + return count($entityPermissions) > 0; + } - return $hasAccess; + /** + * Get the permissions that are applicable for the given entity item. + * Returns null when no entity permissions apply otherwise entity permissions + * are active, even if the returned array is empty. + * + * @returns EntityPermission[] + */ + protected function getApplicableEntityPermissions(Entity $entity, array $userRoleIds, string $action): ?array + { + $chain = [$entity]; + if ($entity instanceof Page && $entity->chapter_id) { + $chain[] = $entity->chapter; + } + + if ($entity instanceof Page || $entity instanceof Chapter) { + $chain[] = $entity->book; + } + + foreach ($chain as $currentEntity) { + if ($currentEntity->restricted) { + return $currentEntity->permissions() + ->whereIn('role_id', $userRoleIds) + ->where('action', '=', $action) + ->get() + ->all(); + } + } + + return null; } /** @@ -76,26 +104,6 @@ class PermissionApplicator return $hasPermission; } - /** - * The general query filter to remove all entities - * that the current user does not have access to. - */ - protected function entityRestrictionQuery(Builder $query, string $action): Builder - { - $q = $query->where(function ($parentQuery) use ($action) { - $parentQuery->whereHas('jointPermissions', function ($permissionQuery) use ($action) { - $permissionQuery->whereIn('role_id', $this->getCurrentUserRoleIds()) - // TODO - Delete line once only views - ->where('action', '=', $action) - ->where(function (Builder $query) { - $this->addJointHasPermissionCheck($query, $this->currentUser()->id); - }); - }); - }); - - return $q; - } - /** * Limited the given entity query so that the query will only * return items that the user has view permission for. @@ -139,7 +147,27 @@ class PermissionApplicator $this->enforceDraftVisibilityOnQuery($query); } - return $this->entityRestrictionQuery($query, 'view'); + return $this->entityRestrictionQuery($query); + } + + /** + * The general query filter to remove all entities + * that the current user does not have access to. + */ + protected function entityRestrictionQuery(Builder $query): Builder + { + $q = $query->where(function ($parentQuery) { + $parentQuery->whereHas('jointPermissions', function ($permissionQuery) { + $permissionQuery->whereIn('role_id', $this->getCurrentUserRoleIds()) + // TODO - Delete line once only views + ->where('action', '=', 'view') + ->where(function (Builder $query) { + $this->addJointHasPermissionCheck($query, $this->currentUser()->id); + }); + }); + }); + + return $q; } /** From afe1a042396454e071b4b3bb5bb0043586ba333a Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 16 Jul 2022 19:54:25 +0100 Subject: [PATCH 08/12] Aligned permission applicator method names Also removed lesser used function, that was mostly a duplicate of an existing function, and only used for search. --- app/Actions/ActivityQueries.php | 4 +- app/Actions/TagRepo.php | 6 +- app/Auth/Permissions/PermissionApplicator.php | 80 ++++++------------- app/Entities/Models/Page.php | 2 +- app/Entities/Queries/Popular.php | 2 +- app/Entities/Queries/RecentlyViewed.php | 2 +- app/Entities/Queries/TopFavourites.php | 2 +- app/Entities/Tools/SearchRunner.php | 4 +- app/Uploads/Attachment.php | 3 +- app/Uploads/ImageRepo.php | 2 +- 10 files changed, 36 insertions(+), 71 deletions(-) diff --git a/app/Actions/ActivityQueries.php b/app/Actions/ActivityQueries.php index 0b8cd3b5e..0e9cbdebb 100644 --- a/app/Actions/ActivityQueries.php +++ b/app/Actions/ActivityQueries.php @@ -26,7 +26,7 @@ class ActivityQueries public function latest(int $count = 20, int $page = 0): array { $activityList = $this->permissions - ->filterRestrictedEntityRelations(Activity::query(), 'activities', 'entity_id', 'entity_type') + ->restrictEntityRelationQuery(Activity::query(), 'activities', 'entity_id', 'entity_type') ->orderBy('created_at', 'desc') ->with(['user', 'entity']) ->skip($count * $page) @@ -79,7 +79,7 @@ class ActivityQueries public function userActivity(User $user, int $count = 20, int $page = 0): array { $activityList = $this->permissions - ->filterRestrictedEntityRelations(Activity::query(), 'activities', 'entity_id', 'entity_type') + ->restrictEntityRelationQuery(Activity::query(), 'activities', 'entity_id', 'entity_type') ->orderBy('created_at', 'desc') ->where('user_id', '=', $user->id) ->skip($count * $page) diff --git a/app/Actions/TagRepo.php b/app/Actions/TagRepo.php index c5807108e..172d8ec6e 100644 --- a/app/Actions/TagRepo.php +++ b/app/Actions/TagRepo.php @@ -50,7 +50,7 @@ class TagRepo }); } - return $this->permissions->filterRestrictedEntityRelations($query, 'tags', 'entity_id', 'entity_type'); + return $this->permissions->restrictEntityRelationQuery($query, 'tags', 'entity_id', 'entity_type'); } /** @@ -69,7 +69,7 @@ class TagRepo $query = $query->orderBy('count', 'desc')->take(50); } - $query = $this->permissions->filterRestrictedEntityRelations($query, 'tags', 'entity_id', 'entity_type'); + $query = $this->permissions->restrictEntityRelationQuery($query, 'tags', 'entity_id', 'entity_type'); return $query->get(['name'])->pluck('name'); } @@ -95,7 +95,7 @@ class TagRepo $query = $query->where('name', '=', $tagName); } - $query = $this->permissions->filterRestrictedEntityRelations($query, 'tags', 'entity_id', 'entity_type'); + $query = $this->permissions->restrictEntityRelationQuery($query, 'tags', 'entity_id', 'entity_type'); return $query->get(['value'])->pluck('value'); } diff --git a/app/Auth/Permissions/PermissionApplicator.php b/app/Auth/Permissions/PermissionApplicator.php index e73db5157..91a7c72ae 100644 --- a/app/Auth/Permissions/PermissionApplicator.php +++ b/app/Auth/Permissions/PermissionApplicator.php @@ -105,7 +105,7 @@ class PermissionApplicator } /** - * Limited the given entity query so that the query will only + * Limit the given entity query so that the query will only * return items that the user has view permission for. */ public function restrictEntityQuery(Builder $query): Builder @@ -126,7 +126,7 @@ class PermissionApplicator * Extend the given page query to ensure draft items are not visible * unless created by the given user. */ - public function enforceDraftVisibilityOnQuery(Builder $query): Builder + public function restrictDraftsOnPageQuery(Builder $query): Builder { return $query->where(function (Builder $query) { $query->where('draft', '=', false) @@ -137,39 +137,6 @@ class PermissionApplicator }); } - /** - * Add restrictions for a generic entity. - */ - public function enforceEntityRestrictions(Entity $entity, Builder $query): Builder - { - if ($entity instanceof Page) { - // Prevent drafts being visible to others. - $this->enforceDraftVisibilityOnQuery($query); - } - - return $this->entityRestrictionQuery($query); - } - - /** - * The general query filter to remove all entities - * that the current user does not have access to. - */ - protected function entityRestrictionQuery(Builder $query): Builder - { - $q = $query->where(function ($parentQuery) { - $parentQuery->whereHas('jointPermissions', function ($permissionQuery) { - $permissionQuery->whereIn('role_id', $this->getCurrentUserRoleIds()) - // TODO - Delete line once only views - ->where('action', '=', 'view') - ->where(function (Builder $query) { - $this->addJointHasPermissionCheck($query, $this->currentUser()->id); - }); - }); - }); - - return $q; - } - /** * Filter items that have entities set as a polymorphic relation. * For simplicity, this will not return results attached to draft pages. @@ -177,7 +144,7 @@ class PermissionApplicator * * @param Builder|QueryBuilder $query */ - public function filterRestrictedEntityRelations($query, string $tableName, string $entityIdColumn, string $entityTypeColumn) + public function restrictEntityRelationQuery($query, string $tableName, string $entityIdColumn, string $entityTypeColumn) { $tableDetails = ['tableName' => $tableName, 'entityIdColumn' => $entityIdColumn, 'entityTypeColumn' => $entityTypeColumn]; $pageMorphClass = (new Page())->getMorphClass(); @@ -207,19 +174,20 @@ class PermissionApplicator } /** - * Add conditions to a query to filter the selection to related entities - * where view permissions are granted. + * Add conditions to a query for a model that's a relation of a page, so only the model results + * on visible pages are returned by the query. + * Is effectively the same as "restrictEntityRelationQuery" but takes into account page drafts + * while not expecting a polymorphic relation, Just a simpler one-page-to-many-relations set-up. */ - public function filterRelatedEntity(string $entityClass, Builder $query, string $tableName, string $entityIdColumn): Builder + public function restrictPageRelationQuery(Builder $query, string $tableName, string $pageIdColumn): Builder { - $fullEntityIdColumn = $tableName . '.' . $entityIdColumn; - $instance = new $entityClass(); - $morphClass = $instance->getMorphClass(); + $fullPageIdColumn = $tableName . '.' . $pageIdColumn; + $morphClass = (new Page())->getMorphClass(); - $existsQuery = function ($permissionQuery) use ($fullEntityIdColumn, $morphClass) { + $existsQuery = function ($permissionQuery) use ($fullPageIdColumn, $morphClass) { /** @var Builder $permissionQuery */ $permissionQuery->select('joint_permissions.role_id')->from('joint_permissions') - ->whereColumn('joint_permissions.entity_id', '=', $fullEntityIdColumn) + ->whereColumn('joint_permissions.entity_id', '=', $fullPageIdColumn) ->where('joint_permissions.entity_type', '=', $morphClass) ->where('joint_permissions.action', '=', 'view') ->whereIn('joint_permissions.role_id', $this->getCurrentUserRoleIds()) @@ -228,22 +196,20 @@ class PermissionApplicator }); }; - $q = $query->where(function ($query) use ($existsQuery, $fullEntityIdColumn) { + $q = $query->where(function ($query) use ($existsQuery, $fullPageIdColumn) { $query->whereExists($existsQuery) - ->orWhere($fullEntityIdColumn, '=', 0); + ->orWhere($fullPageIdColumn, '=', 0); }); - if ($instance instanceof Page) { - // Prevent visibility of non-owned draft pages - $q->whereExists(function (QueryBuilder $query) use ($fullEntityIdColumn) { - $query->select('id')->from('pages') - ->whereColumn('pages.id', '=', $fullEntityIdColumn) - ->where(function (QueryBuilder $query) { - $query->where('pages.draft', '=', false) - ->orWhere('pages.owned_by', '=', $this->currentUser()->id); - }); - }); - } + // 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; } diff --git a/app/Entities/Models/Page.php b/app/Entities/Models/Page.php index a2690b2a0..93729d7f2 100644 --- a/app/Entities/Models/Page.php +++ b/app/Entities/Models/Page.php @@ -51,7 +51,7 @@ class Page extends BookChild */ public function scopeVisible(Builder $query): Builder { - $query = app()->make(PermissionApplicator::class)->enforceDraftVisibilityOnQuery($query); + $query = app()->make(PermissionApplicator::class)->restrictDraftsOnPageQuery($query); return parent::scopeVisible($query); } diff --git a/app/Entities/Queries/Popular.php b/app/Entities/Queries/Popular.php index 82786e3c6..fafd60c59 100644 --- a/app/Entities/Queries/Popular.php +++ b/app/Entities/Queries/Popular.php @@ -10,7 +10,7 @@ class Popular extends EntityQuery public function run(int $count, int $page, array $filterModels = null) { $query = $this->permissionService() - ->filterRestrictedEntityRelations(View::query(), 'views', 'viewable_id', 'viewable_type') + ->restrictEntityRelationQuery(View::query(), 'views', 'viewable_id', 'viewable_type') ->select('*', 'viewable_id', 'viewable_type', DB::raw('SUM(views) as view_count')) ->groupBy('viewable_id', 'viewable_type') ->orderBy('view_count', 'desc'); diff --git a/app/Entities/Queries/RecentlyViewed.php b/app/Entities/Queries/RecentlyViewed.php index 38d1f1be4..0f5c68041 100644 --- a/app/Entities/Queries/RecentlyViewed.php +++ b/app/Entities/Queries/RecentlyViewed.php @@ -14,7 +14,7 @@ class RecentlyViewed extends EntityQuery return collect(); } - $query = $this->permissionService()->filterRestrictedEntityRelations( + $query = $this->permissionService()->restrictEntityRelationQuery( View::query(), 'views', 'viewable_id', diff --git a/app/Entities/Queries/TopFavourites.php b/app/Entities/Queries/TopFavourites.php index 5d138679a..0b9a5ba23 100644 --- a/app/Entities/Queries/TopFavourites.php +++ b/app/Entities/Queries/TopFavourites.php @@ -15,7 +15,7 @@ class TopFavourites extends EntityQuery } $query = $this->permissionService() - ->filterRestrictedEntityRelations(Favourite::query(), 'favourites', 'favouritable_id', 'favouritable_type') + ->restrictEntityRelationQuery(Favourite::query(), 'favourites', 'favouritable_id', 'favouritable_type') ->select('favourites.*') ->leftJoin('views', function (JoinClause $join) { $join->on('favourites.favouritable_id', '=', 'views.viewable_id'); diff --git a/app/Entities/Tools/SearchRunner.php b/app/Entities/Tools/SearchRunner.php index 1dcc2eb44..459409084 100644 --- a/app/Entities/Tools/SearchRunner.php +++ b/app/Entities/Tools/SearchRunner.php @@ -161,7 +161,7 @@ class SearchRunner */ protected function buildQuery(SearchOptions $searchOpts, Entity $entityModelInstance): EloquentBuilder { - $entityQuery = $entityModelInstance->newQuery(); + $entityQuery = $entityModelInstance->newQuery()->scopes('visible'); if ($entityModelInstance instanceof Page) { $entityQuery->select($entityModelInstance::$listAttributes); @@ -193,7 +193,7 @@ class SearchRunner } } - return $this->permissions->enforceEntityRestrictions($entityModelInstance, $entityQuery); + return $entityQuery; } /** diff --git a/app/Uploads/Attachment.php b/app/Uploads/Attachment.php index d9e0ce7e1..6c7066ff9 100644 --- a/app/Uploads/Attachment.php +++ b/app/Uploads/Attachment.php @@ -91,8 +91,7 @@ class Attachment extends Model { $permissions = app()->make(PermissionApplicator::class); - return $permissions->filterRelatedEntity( - Page::class, + return $permissions->restrictPageRelationQuery( self::query(), 'attachments', 'uploaded_to' diff --git a/app/Uploads/ImageRepo.php b/app/Uploads/ImageRepo.php index 13d3344dd..8770402ad 100644 --- a/app/Uploads/ImageRepo.php +++ b/app/Uploads/ImageRepo.php @@ -74,7 +74,7 @@ class ImageRepo } // Filter by page access - $imageQuery = $this->permissions->filterRelatedEntity(Page::class, $imageQuery, 'images', 'uploaded_to'); + $imageQuery = $this->permissions->restrictPageRelationQuery($imageQuery, 'images', 'uploaded_to'); if ($whereClause !== null) { $imageQuery = $imageQuery->where($whereClause); From 23324018540624d7a6beafd0514f4b7dbe327431 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 16 Jul 2022 20:55:32 +0100 Subject: [PATCH 09/12] Fixed a couple of non-intended logical permission issues Both caught in tests: Fixed loss of permissions for admin users when entity restrictions were active, since there are no entity-restrictions for the admin role but we'd force generate them in joint permissions, which would be queried. Fixed new role permission checks when permissions given with only the action (eg. 'view'), since the type prefix would be required for role permission checks. Was previously not needed as only the simpler form was used in the jointpermissions after merge & calculation. --- app/Auth/Permissions/PermissionApplicator.php | 30 +++++++++---------- app/Entities/Models/Bookshelf.php | 6 ---- app/Entities/Tools/ShelfContext.php | 1 + app/Http/Controllers/BookshelfController.php | 2 +- 4 files changed, 17 insertions(+), 22 deletions(-) diff --git a/app/Auth/Permissions/PermissionApplicator.php b/app/Auth/Permissions/PermissionApplicator.php index 91a7c72ae..4b648532a 100644 --- a/app/Auth/Permissions/PermissionApplicator.php +++ b/app/Auth/Permissions/PermissionApplicator.php @@ -25,11 +25,13 @@ class PermissionApplicator { $explodedPermission = explode('-', $permission); $action = $explodedPermission[1] ?? $explodedPermission[0]; + $fullPermission = count($explodedPermission) > 1 ? $permission : $ownable->getMorphClass() . '-' . $permission; + $user = $this->currentUser(); $userRoleIds = $this->getCurrentUserRoleIds(); - $allRolePermission = $user->can($permission . '-all'); - $ownRolePermission = $user->can($permission . '-own'); + $allRolePermission = $user->can($fullPermission . '-all'); + $ownRolePermission = $user->can($fullPermission . '-own'); $nonJointPermissions = ['restrictions', 'image', 'attachment', 'comment']; $ownerField = ($ownable instanceof Entity) ? 'owned_by' : 'created_by'; $isOwner = $user->id === $ownable->getAttribute($ownerField); @@ -40,23 +42,22 @@ class PermissionApplicator return $hasRolePermission; } - $entityPermissions = $this->getApplicableEntityPermissions($ownable, $userRoleIds, $action); - if (is_null($entityPermissions)) { - return $hasRolePermission; - } + $hasApplicableEntityPermissions = $this->hasEntityPermission($ownable, $userRoleIds, $action); - return count($entityPermissions) > 0; + return is_null($hasApplicableEntityPermissions) ? $hasRolePermission : $hasApplicableEntityPermissions; } /** - * Get the permissions that are applicable for the given entity item. - * Returns null when no entity permissions apply otherwise entity permissions - * are active, even if the returned array is empty. - * - * @returns EntityPermission[] + * Check if there are permissions that are applicable for the given entity item, action and roles. + * Returns null when no entity permissions are in force. */ - protected function getApplicableEntityPermissions(Entity $entity, array $userRoleIds, string $action): ?array + protected function hasEntityPermission(Entity $entity, array $userRoleIds, string $action): ?bool { + $adminRoleId = Role::getSystemRole('admin')->id; + if (in_array($adminRoleId, $userRoleIds)) { + return true; + } + $chain = [$entity]; if ($entity instanceof Page && $entity->chapter_id) { $chain[] = $entity->chapter; @@ -71,8 +72,7 @@ class PermissionApplicator return $currentEntity->permissions() ->whereIn('role_id', $userRoleIds) ->where('action', '=', $action) - ->get() - ->all(); + ->count() > 0; } } diff --git a/app/Entities/Models/Bookshelf.php b/app/Entities/Models/Bookshelf.php index b9ebab92e..b2dab252a 100644 --- a/app/Entities/Models/Bookshelf.php +++ b/app/Entities/Models/Bookshelf.php @@ -91,10 +91,6 @@ class Bookshelf extends Entity implements HasCoverImage /** * Check if this shelf contains the given book. - * - * @param Book $book - * - * @return bool */ public function contains(Book $book): bool { @@ -103,8 +99,6 @@ class Bookshelf extends Entity implements HasCoverImage /** * Add a book to the end of this shelf. - * - * @param Book $book */ public function appendBook(Book $book) { diff --git a/app/Entities/Tools/ShelfContext.php b/app/Entities/Tools/ShelfContext.php index 50d798171..50c7047d9 100644 --- a/app/Entities/Tools/ShelfContext.php +++ b/app/Entities/Tools/ShelfContext.php @@ -20,6 +20,7 @@ class ShelfContext return null; } + /** @var Bookshelf $shelf */ $shelf = Bookshelf::visible()->find($contextBookshelfId); $shelfContainsBook = $shelf && $shelf->contains($book); diff --git a/app/Http/Controllers/BookshelfController.php b/app/Http/Controllers/BookshelfController.php index 18adaa627..feb581c78 100644 --- a/app/Http/Controllers/BookshelfController.php +++ b/app/Http/Controllers/BookshelfController.php @@ -104,7 +104,7 @@ class BookshelfController extends Controller public function show(ActivityQueries $activities, string $slug) { $shelf = $this->bookshelfRepo->getBySlug($slug); - $this->checkOwnablePermission('book-view', $shelf); + $this->checkOwnablePermission('bookshelf-view', $shelf); $sort = setting()->getForCurrentUser('shelf_books_sort', 'default'); $order = setting()->getForCurrentUser('shelf_books_sort_order', 'asc'); From 8f90996ceff6dca02c6f80d6f7d0d3f837dd1601 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 16 Jul 2022 21:50:42 +0100 Subject: [PATCH 10/12] Dropped use of non-view joint permissions --- .../Permissions/JointPermissionBuilder.php | 82 ++++++++----------- app/Auth/Permissions/PermissionApplicator.php | 4 - ...7_16_170051_drop_joint_permission_type.php | 41 ++++++++++ tests/PublicActionTest.php | 1 + 4 files changed, 74 insertions(+), 54 deletions(-) create mode 100644 database/migrations/2022_07_16_170051_drop_joint_permission_type.php diff --git a/app/Auth/Permissions/JointPermissionBuilder.php b/app/Auth/Permissions/JointPermissionBuilder.php index fe25e02ff..9ee09a3a6 100644 --- a/app/Auth/Permissions/JointPermissionBuilder.php +++ b/app/Auth/Permissions/JointPermissionBuilder.php @@ -13,6 +13,10 @@ use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Collection as EloquentCollection; use Illuminate\Support\Facades\DB; +/** + * Joint permissions provide a pre-query "cached" table of view permissions for all core entity + * types for all roles in the system. This class generates out that table for different scenarios. + */ class JointPermissionBuilder { /** @@ -80,6 +84,7 @@ class JointPermissionBuilder { $roles = [$role]; $role->jointPermissions()->delete(); + $role->load('permissions'); // Chunk through all books $this->bookFetchQuery()->chunk(20, function ($books) use ($roles) { @@ -247,7 +252,7 @@ class JointPermissionBuilder // Create a mapping of explicit entity permissions $permissionMap = []; foreach ($permissions as $permission) { - $key = $permission->restrictable_type . ':' . $permission->restrictable_id . ':' . $permission->role_id . ':' . $permission->action; + $key = $permission->restrictable_type . ':' . $permission->restrictable_id . ':' . $permission->role_id; $isRestricted = $entityRestrictedMap[$permission->restrictable_type . ':' . $permission->restrictable_id]; $permissionMap[$key] = $isRestricted; } @@ -263,16 +268,13 @@ class JointPermissionBuilder // Create Joint Permission Data foreach ($entities as $entity) { foreach ($roles as $role) { - foreach ($this->getActions($entity) as $action) { - $jointPermissions[] = $this->createJointPermissionData( - $entity, - $role->getRawAttribute('id'), - $action, - $permissionMap, - $rolePermissionMap, - $role->system_name === 'admin' - ); - } + $jointPermissions[] = $this->createJointPermissionData( + $entity, + $role->getRawAttribute('id'), + $permissionMap, + $rolePermissionMap, + $role->system_name === 'admin' + ); } } @@ -312,64 +314,46 @@ class JointPermissionBuilder protected function getEntityPermissionsForEntities(array $entities): array { $idsByType = $this->entitiesToTypeIdMap($entities); - $permissionFetch = EntityPermission::query(); - - foreach ($idsByType as $type => $ids) { - $permissionFetch->orWhere(function (Builder $query) use ($type, $ids) { - $query->where('restrictable_type', '=', $type)->whereIn('restrictable_id', $ids); + $permissionFetch = EntityPermission::query() + ->where('action', '=', 'view') + ->where(function(Builder $query) use ($idsByType) { + foreach ($idsByType as $type => $ids) { + $query->orWhere(function (Builder $query) use ($type, $ids) { + $query->where('restrictable_type', '=', $type)->whereIn('restrictable_id', $ids); + }); + } }); - } return $permissionFetch->get()->all(); } - /** - * Get the actions related to an entity. - */ - protected function getActions(SimpleEntityData $entity): array - { - $baseActions = ['view', 'update', 'delete']; - - if ($entity->type === 'chapter' || $entity->type === 'book') { - $baseActions[] = 'page-create'; - } - - if ($entity->type === 'book') { - $baseActions[] = 'chapter-create'; - } - - return $baseActions; - } - /** * Create entity permission data for an entity and role * for a particular action. */ - protected function createJointPermissionData(SimpleEntityData $entity, int $roleId, string $action, array $permissionMap, array $rolePermissionMap, bool $isAdminRole): array + protected function createJointPermissionData(SimpleEntityData $entity, int $roleId, array $permissionMap, array $rolePermissionMap, bool $isAdminRole): array { - $permissionPrefix = (strpos($action, '-') === false ? ($entity->type . '-') : '') . $action; + $permissionPrefix = $entity->type . '-view'; $roleHasPermission = isset($rolePermissionMap[$roleId . ':' . $permissionPrefix . '-all']); $roleHasPermissionOwn = isset($rolePermissionMap[$roleId . ':' . $permissionPrefix . '-own']); - $explodedAction = explode('-', $action); - $restrictionAction = end($explodedAction); if ($isAdminRole) { - return $this->createJointPermissionDataArray($entity, $roleId, $action, true, true); + return $this->createJointPermissionDataArray($entity, $roleId, true, true); } if ($entity->restricted) { - $hasAccess = $this->mapHasActiveRestriction($permissionMap, $entity, $roleId, $restrictionAction); + $hasAccess = $this->mapHasActiveRestriction($permissionMap, $entity, $roleId); - return $this->createJointPermissionDataArray($entity, $roleId, $action, $hasAccess, $hasAccess); + return $this->createJointPermissionDataArray($entity, $roleId, $hasAccess, $hasAccess); } if ($entity->type === 'book' || $entity->type === 'bookshelf') { - return $this->createJointPermissionDataArray($entity, $roleId, $action, $roleHasPermission, $roleHasPermissionOwn); + return $this->createJointPermissionDataArray($entity, $roleId, $roleHasPermission, $roleHasPermissionOwn); } // For chapters and pages, Check if explicit permissions are set on the Book. $book = $this->getBook($entity->book_id); - $hasExplicitAccessToParents = $this->mapHasActiveRestriction($permissionMap, $book, $roleId, $restrictionAction); + $hasExplicitAccessToParents = $this->mapHasActiveRestriction($permissionMap, $book, $roleId); $hasPermissiveAccessToParents = !$book->restricted; // For pages with a chapter, Check if explicit permissions are set on the Chapter @@ -377,14 +361,13 @@ class JointPermissionBuilder $chapter = $this->getChapter($entity->chapter_id); $hasPermissiveAccessToParents = $hasPermissiveAccessToParents && !$chapter->restricted; if ($chapter->restricted) { - $hasExplicitAccessToParents = $this->mapHasActiveRestriction($permissionMap, $chapter, $roleId, $restrictionAction); + $hasExplicitAccessToParents = $this->mapHasActiveRestriction($permissionMap, $chapter, $roleId); } } return $this->createJointPermissionDataArray( $entity, $roleId, - $action, ($hasExplicitAccessToParents || ($roleHasPermission && $hasPermissiveAccessToParents)), ($hasExplicitAccessToParents || ($roleHasPermissionOwn && $hasPermissiveAccessToParents)) ); @@ -393,9 +376,9 @@ class JointPermissionBuilder /** * Check for an active restriction in an entity map. */ - protected function mapHasActiveRestriction(array $entityMap, SimpleEntityData $entity, int $roleId, string $action): bool + protected function mapHasActiveRestriction(array $entityMap, SimpleEntityData $entity, int $roleId): bool { - $key = $entity->type . ':' . $entity->id . ':' . $roleId . ':' . $action; + $key = $entity->type . ':' . $entity->id . ':' . $roleId; return $entityMap[$key] ?? false; } @@ -404,10 +387,9 @@ class JointPermissionBuilder * 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, string $action, bool $permissionAll, bool $permissionOwn): array + protected function createJointPermissionDataArray(SimpleEntityData $entity, int $roleId, bool $permissionAll, bool $permissionOwn): array { return [ - 'action' => $action, 'entity_id' => $entity->id, 'entity_type' => $entity->type, 'has_permission' => $permissionAll, diff --git a/app/Auth/Permissions/PermissionApplicator.php b/app/Auth/Permissions/PermissionApplicator.php index 4b648532a..b89857c5c 100644 --- a/app/Auth/Permissions/PermissionApplicator.php +++ b/app/Auth/Permissions/PermissionApplicator.php @@ -113,8 +113,6 @@ class PermissionApplicator return $query->where(function (Builder $parentQuery) { $parentQuery->whereHas('jointPermissions', function (Builder $permissionQuery) { $permissionQuery->whereIn('role_id', $this->getCurrentUserRoleIds()) - // TODO - Delete line once only views - ->where('action', '=', 'view') ->where(function (Builder $query) { $this->addJointHasPermissionCheck($query, $this->currentUser()->id); }); @@ -154,7 +152,6 @@ class PermissionApplicator $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']) - ->where('joint_permissions.action', '=', 'view') ->whereIn('joint_permissions.role_id', $this->getCurrentUserRoleIds()) ->where(function (QueryBuilder $query) { $this->addJointHasPermissionCheck($query, $this->currentUser()->id); @@ -189,7 +186,6 @@ class PermissionApplicator $permissionQuery->select('joint_permissions.role_id')->from('joint_permissions') ->whereColumn('joint_permissions.entity_id', '=', $fullPageIdColumn) ->where('joint_permissions.entity_type', '=', $morphClass) - ->where('joint_permissions.action', '=', 'view') ->whereIn('joint_permissions.role_id', $this->getCurrentUserRoleIds()) ->where(function (QueryBuilder $query) { $this->addJointHasPermissionCheck($query, $this->currentUser()->id); diff --git a/database/migrations/2022_07_16_170051_drop_joint_permission_type.php b/database/migrations/2022_07_16_170051_drop_joint_permission_type.php new file mode 100644 index 000000000..f34f73636 --- /dev/null +++ b/database/migrations/2022_07_16_170051_drop_joint_permission_type.php @@ -0,0 +1,41 @@ +where('action', '!=', 'view') + ->delete(); + + Schema::table('joint_permissions', function (Blueprint $table) { + $table->dropPrimary(['role_id', 'entity_type', 'entity_id', 'action']); + $table->dropColumn('action'); + $table->primary(['role_id', 'entity_type', 'entity_id'], 'joint_primary'); + }); + } + + /** + * Reverse the migrations. + * + * @return void + */ + public function down() + { + Schema::table('joint_permissions', function (Blueprint $table) { + $table->string('action'); + $table->dropPrimary(['role_id', 'entity_type', 'entity_id']); + $table->primary(['role_id', 'entity_type', 'entity_id', 'action']); + }); + } +} diff --git a/tests/PublicActionTest.php b/tests/PublicActionTest.php index 2841f4ef4..ced40d0a8 100644 --- a/tests/PublicActionTest.php +++ b/tests/PublicActionTest.php @@ -90,6 +90,7 @@ class PublicActionTest extends TestCase $publicRole->attachPermission($perm); } $this->app->make(JointPermissionBuilder::class)->rebuildForRole($publicRole); + user()->clearPermissionCache(); /** @var Chapter $chapter */ $chapter = Chapter::query()->first(); From e6e6d259749f789a4a3e8b87bc6e86ed94a4df9c Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sun, 17 Jul 2022 10:18:24 +0100 Subject: [PATCH 11/12] Removed test web route, extracted text, added test --- resources/lang/en/entities.php | 1 + resources/views/entities/list-item.blade.php | 2 +- routes/web.php | 7 ------- tests/Entity/EntitySearchTest.php | 17 ++++++++++++++++- 4 files changed, 18 insertions(+), 9 deletions(-) diff --git a/resources/lang/en/entities.php b/resources/lang/en/entities.php index 27d67487a..aa353bdac 100644 --- a/resources/lang/en/entities.php +++ b/resources/lang/en/entities.php @@ -24,6 +24,7 @@ return [ 'meta_updated_name' => 'Updated :timeLength by :user', 'meta_owned_name' => 'Owned by :user', 'entity_select' => 'Entity Select', + 'entity_select_lack_permission' => 'You don\'t have the required permissions to select this item', 'images' => 'Images', 'my_recent_drafts' => 'My Recent Drafts', 'my_recently_viewed' => 'My Recently Viewed', diff --git a/resources/views/entities/list-item.blade.php b/resources/views/entities/list-item.blade.php index 5314c8446..2fadef191 100644 --- a/resources/views/entities/list-item.blade.php +++ b/resources/views/entities/list-item.blade.php @@ -4,7 +4,7 @@ @if($locked ?? false)
- @icon('lock')You don't have the required permissions to select this item. + @icon('lock'){{ trans('entities.entity_select_lack_permission') }}
@endif diff --git a/routes/web.php b/routes/web.php index 9b562703c..5e16e5333 100644 --- a/routes/web.php +++ b/routes/web.php @@ -38,13 +38,6 @@ use Illuminate\View\Middleware\ShareErrorsFromSession; Route::get('/status', [StatusController::class, 'show']); Route::get('/robots.txt', [HomeController::class, 'robots']); -Route::get('/test', function() { - $book = \BookStack\Entities\Models\Book::query()->where('slug', '=', 'k5TrhXxaNb')->firstOrFail(); - $builder= app()->make(\BookStack\Auth\Permissions\JointPermissionBuilder::class); - $builder->rebuildForEntity($book); - return 'finished'; -})->withoutMiddleware('web'); - // Authenticated routes... Route::middleware('auth')->group(function () { diff --git a/tests/Entity/EntitySearchTest.php b/tests/Entity/EntitySearchTest.php index b535f5aaa..55c54695e 100644 --- a/tests/Entity/EntitySearchTest.php +++ b/tests/Entity/EntitySearchTest.php @@ -214,7 +214,7 @@ class EntitySearchTest extends TestCase $defaultListTest->assertDontSee($notVisitedPage->name); } - public function test_ajax_entity_serach_shows_breadcrumbs() + public function test_ajax_entity_search_shows_breadcrumbs() { $chapter = Chapter::first(); $page = $chapter->pages->first(); @@ -230,6 +230,21 @@ class EntitySearchTest extends TestCase $chapterSearch->assertSee($chapter->book->getShortName(42)); } + public function test_ajax_entity_search_reflects_items_without_permission() + { + $page = Page::query()->first(); + $baseSelector = 'a[data-entity-type="page"][data-entity-id="' . $page->id . '"]'; + $searchUrl = "/ajax/search/entities?permission=update&term=" . urlencode($page->name); + + $resp = $this->asEditor()->get($searchUrl); + $resp->assertElementContains($baseSelector, $page->name); + $resp->assertElementNotContains($baseSelector, "You don't have the required permissions to select this item"); + + $resp = $this->actingAs($this->getViewer())->get($searchUrl); + $resp->assertElementContains($baseSelector, $page->name); + $resp->assertElementContains($baseSelector, "You don't have the required permissions to select this item"); + } + public function test_sibling_search_for_pages() { $chapter = Chapter::query()->with('pages')->first(); From 9cf05944f6c6d74603e1be1e6c96bd7ae6d7e3fb Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sun, 17 Jul 2022 10:32:16 +0100 Subject: [PATCH 12/12] Applied StyleCI changes --- app/Auth/Permissions/JointPermissionBuilder.php | 13 ++++++++----- app/Auth/Permissions/PermissionApplicator.php | 2 +- app/Auth/Permissions/SimpleEntityData.php | 2 +- app/Entities/Tools/SearchRunner.php | 1 - app/Providers/CustomFacadeProvider.php | 2 -- tests/Entity/EntitySearchTest.php | 2 +- tests/SharedTestHelpers.php | 1 - 7 files changed, 11 insertions(+), 12 deletions(-) diff --git a/app/Auth/Permissions/JointPermissionBuilder.php b/app/Auth/Permissions/JointPermissionBuilder.php index 9ee09a3a6..f377eef5c 100644 --- a/app/Auth/Permissions/JointPermissionBuilder.php +++ b/app/Auth/Permissions/JointPermissionBuilder.php @@ -148,7 +148,6 @@ class JointPermissionBuilder ]); } - /** * Build joint permissions for the given book and role combinations. */ @@ -207,6 +206,7 @@ class JointPermissionBuilder /** * @param Entity[] $entities + * * @return SimpleEntityData[] */ protected function entitiesToSimpleEntities(array $entities): array @@ -288,7 +288,9 @@ class JointPermissionBuilder /** * From the given entity list, provide back a mapping of entity types to * the ids of that given type. The type used is the DB morph class. + * * @param SimpleEntityData[] $entities + * * @return array */ protected function entitiesToTypeIdMap(array $entities): array @@ -307,8 +309,10 @@ class JointPermissionBuilder } /** - * Get the entity permissions for all the given entities + * Get the entity permissions for all the given entities. + * * @param SimpleEntityData[] $entities + * * @return EntityPermission[] */ protected function getEntityPermissionsForEntities(array $entities): array @@ -316,7 +320,7 @@ class JointPermissionBuilder $idsByType = $this->entitiesToTypeIdMap($entities); $permissionFetch = EntityPermission::query() ->where('action', '=', 'view') - ->where(function(Builder $query) use ($idsByType) { + ->where(function (Builder $query) use ($idsByType) { foreach ($idsByType as $type => $ids) { $query->orWhere(function (Builder $query) use ($type, $ids) { $query->where('restrictable_type', '=', $type)->whereIn('restrictable_id', $ids); @@ -398,5 +402,4 @@ class JointPermissionBuilder 'role_id' => $roleId, ]; } - -} \ No newline at end of file +} diff --git a/app/Auth/Permissions/PermissionApplicator.php b/app/Auth/Permissions/PermissionApplicator.php index b89857c5c..d855a6170 100644 --- a/app/Auth/Permissions/PermissionApplicator.php +++ b/app/Auth/Permissions/PermissionApplicator.php @@ -86,7 +86,7 @@ class PermissionApplicator public function checkUserHasEntityPermissionOnAny(string $action, string $entityClass = ''): bool { if (strpos($action, '-') !== false) { - throw new InvalidArgumentException("Action should be a simple entity permission action, not a role permission"); + throw new InvalidArgumentException('Action should be a simple entity permission action, not a role permission'); } $permissionQuery = EntityPermission::query() diff --git a/app/Auth/Permissions/SimpleEntityData.php b/app/Auth/Permissions/SimpleEntityData.php index 0d1c94b0d..6ec0c4179 100644 --- a/app/Auth/Permissions/SimpleEntityData.php +++ b/app/Auth/Permissions/SimpleEntityData.php @@ -10,4 +10,4 @@ class SimpleEntityData public int $owned_by; public ?int $book_id; public ?int $chapter_id; -} \ No newline at end of file +} diff --git a/app/Entities/Tools/SearchRunner.php b/app/Entities/Tools/SearchRunner.php index 459409084..78659b786 100644 --- a/app/Entities/Tools/SearchRunner.php +++ b/app/Entities/Tools/SearchRunner.php @@ -21,7 +21,6 @@ use SplObjectStorage; class SearchRunner { - protected EntityProvider $entityProvider; protected PermissionApplicator $permissions; diff --git a/app/Providers/CustomFacadeProvider.php b/app/Providers/CustomFacadeProvider.php index 04de82250..6ba5632e6 100644 --- a/app/Providers/CustomFacadeProvider.php +++ b/app/Providers/CustomFacadeProvider.php @@ -3,9 +3,7 @@ namespace BookStack\Providers; use BookStack\Actions\ActivityLogger; -use BookStack\Auth\Permissions\PermissionApplicator; use BookStack\Theming\ThemeService; -use BookStack\Uploads\ImageService; use Illuminate\Support\ServiceProvider; class CustomFacadeProvider extends ServiceProvider diff --git a/tests/Entity/EntitySearchTest.php b/tests/Entity/EntitySearchTest.php index 55c54695e..a23a2fd26 100644 --- a/tests/Entity/EntitySearchTest.php +++ b/tests/Entity/EntitySearchTest.php @@ -234,7 +234,7 @@ class EntitySearchTest extends TestCase { $page = Page::query()->first(); $baseSelector = 'a[data-entity-type="page"][data-entity-id="' . $page->id . '"]'; - $searchUrl = "/ajax/search/entities?permission=update&term=" . urlencode($page->name); + $searchUrl = '/ajax/search/entities?permission=update&term=' . urlencode($page->name); $resp = $this->asEditor()->get($searchUrl); $resp->assertElementContains($baseSelector, $page->name); diff --git a/tests/SharedTestHelpers.php b/tests/SharedTestHelpers.php index 99bc92494..bc7b10266 100644 --- a/tests/SharedTestHelpers.php +++ b/tests/SharedTestHelpers.php @@ -3,7 +3,6 @@ namespace Tests; use BookStack\Auth\Permissions\JointPermissionBuilder; -use BookStack\Auth\Permissions\PermissionApplicator; use BookStack\Auth\Permissions\PermissionsRepo; use BookStack\Auth\Permissions\RolePermission; use BookStack\Auth\Role;