From: Dan Brown Date: Tue, 4 Jan 2022 21:09:34 +0000 (+0000) Subject: Changed model loading and permission checking on book sort X-Git-Tag: v21.12.1~1^2~2^2~4 X-Git-Url: http://source.bookstackapp.com/bookstack/commitdiff_plain/d8c45f574605ef27d662cfa850d06b61e81aedb6 Changed model loading and permission checking on book sort Models are now loaded into their own map to then be used for sorting and reporting back of changed books. Prevents akward logic ordering issues of before where some bits of code assumed/hoped for loaded models on abstract data structures. New levels of permissions are now checked for items within the sort operation. Needs testing to cover. --- diff --git a/app/Entities/Tools/BookContents.php b/app/Entities/Tools/BookContents.php index 96142bb7f..ff018eda9 100644 --- a/app/Entities/Tools/BookContents.php +++ b/app/Entities/Tools/BookContents.php @@ -7,7 +7,6 @@ use BookStack\Entities\Models\BookChild; use BookStack\Entities\Models\Chapter; use BookStack\Entities\Models\Entity; use BookStack\Entities\Models\Page; -use BookStack\Exceptions\SortOperationException; use Illuminate\Support\Collection; class BookContents @@ -110,34 +109,42 @@ class BookContents * Sort the books content using the given sort map. * Returns a list of books that were involved in the operation. * - * @throws SortOperationException + * @returns Book[] */ - public function sortUsingMap(BookSortMap $sortMap): Collection + public function sortUsingMap(BookSortMap $sortMap): array { // Load models into map - $this->loadModelsIntoSortMap($sortMap); - $booksInvolved = $this->getBooksInvolvedInSort($sortMap); + $modelMap = $this->loadModelsFromSortMap($sortMap); // Perform the sort foreach ($sortMap->all() as $item) { - $this->applySortUpdates($item); + $this->applySortUpdates($item, $modelMap); } - // Update permissions and activity. - $booksInvolved->each(function (Book $book) { + /** @var Book[] $booksInvolved */ + $booksInvolved = array_values(array_filter($modelMap, function (string $key) { + return strpos($key, 'book:') === 0; + }, ARRAY_FILTER_USE_KEY)); + + // Update permissions of books involved + foreach ($booksInvolved as $book) { $book->rebuildPermissions(); - }); + } return $booksInvolved; } /** * Using the given sort map item, detect changes for the related model - * and update it if required. + * and update it if required. Changes where permissions are lacking will + * be skipped and not throw an error. + * + * @param array $modelMap */ - protected function applySortUpdates(BookSortMapItem $sortMapItem): void + protected function applySortUpdates(BookSortMapItem $sortMapItem, array $modelMap): void { - $model = $sortMapItem->model; + /** @var BookChild $model */ + $model = $modelMap[$sortMapItem->type . ':' . $sortMapItem->id] ?? null; if (!$model) { return; } @@ -146,73 +153,97 @@ class BookContents $bookChanged = $model->book_id !== $sortMapItem->parentBookId; $chapterChanged = ($model instanceof Page) && $model->chapter_id !== $sortMapItem->parentChapterId; + // Stop if there's no change + if (!$priorityChanged && !$bookChanged && !$chapterChanged) { + return; + } + + $currentParentKey = 'book:' . $model->book_id; + if ($model instanceof Page && $model->chapter_id) { + $currentParentKey = 'chapter:' . $model->chapter_id; + } + + $currentParent = $modelMap[$currentParentKey]; + /** @var Book $newBook */ + $newBook = $modelMap['book:' . $sortMapItem->parentBookId] ?? null; + /** @var ?Chapter $newChapter */ + $newChapter = $sortMapItem->parentChapterId ? ($modelMap['chapter:' . $sortMapItem->parentChapterId] ?? null) : null; + + // Check permissions of our changes to be made + if (!$currentParent || !$newBook) { + return; + } else if (!userCan('chapter-update', $currentParent) && !userCan('book-update', $currentParent)) { + return; + } else if ($bookChanged && !$newChapter && !userCan('book-update', $newBook)) { + return; + } else if ($newChapter && !userCan('chapter-update', $newChapter)) { + return; + } else if (($chapterChanged || $bookChanged) && $newChapter && $newBook->id !== $newChapter->book_id) { + return; + } + + // Action the required changes if ($bookChanged) { $model->changeBook($sortMapItem->parentBookId); } if ($chapterChanged) { - $model->chapter_id = intval($sortMapItem->parentChapterId); - $model->save(); + $model->chapter_id = $sortMapItem->parentChapterId ?? 0; } if ($priorityChanged) { $model->priority = $sortMapItem->sort; + } + + if ($chapterChanged || $priorityChanged) { $model->save(); } } /** * Load models from the database into the given sort map. + * @return array */ - protected function loadModelsIntoSortMap(BookSortMap $sortMap): void + protected function loadModelsFromSortMap(BookSortMap $sortMap): array { - $collection = collect($sortMap->all()); - - $keyMap = $collection->keyBy(function (BookSortMapItem $sortMapItem) { - return $sortMapItem->type . ':' . $sortMapItem->id; - }); - - $pageIds = $collection->where('type', '=', 'page')->pluck('id'); - $chapterIds = $collection->where('type', '=', 'chapter')->pluck('id'); - - $pages = Page::visible()->whereIn('id', $pageIds)->get(); - $chapters = Chapter::visible()->whereIn('id', $chapterIds)->get(); + $modelMap = []; + $ids = [ + 'chapter' => [], + 'page' => [], + 'book' => [], + ]; + + foreach ($sortMap->all() as $sortMapItem) { + $ids[$sortMapItem->type][] = $sortMapItem->id; + $ids['book'][] = $sortMapItem->parentBookId; + if ($sortMapItem->parentChapterId) { + $ids['chapter'][] = $sortMapItem->parentChapterId; + } + } + $pages = Page::visible()->whereIn('id', array_unique($ids['page']))->get(Page::$listAttributes); + /** @var Page $page */ foreach ($pages as $page) { - /** @var BookSortMapItem $sortItem */ - $sortItem = $keyMap->get('page:' . $page->id); - $sortItem->model = $page; + $modelMap['page:' . $page->id] = $page; + $ids['book'][] = $page->book_id; + if ($page->chapter_id) { + $ids['chapter'][] = $page->chapter_id; + } } + $chapters = Chapter::visible()->whereIn('id', array_unique($ids['chapter']))->get(); + /** @var Chapter $chapter */ foreach ($chapters as $chapter) { - /** @var BookSortMapItem $sortItem */ - $sortItem = $keyMap->get('chapter:' . $chapter->id); - $sortItem->model = $chapter; + $modelMap['chapter:' . $chapter->id] = $chapter; + $ids['book'][] = $chapter->book_id; } - } - /** - * Get the books involved in a sort. - * The given sort map should have its models loaded first. - * - * @throws SortOperationException - */ - protected function getBooksInvolvedInSort(BookSortMap $sortMap): Collection - { - $collection = collect($sortMap->all()); - - $bookIdsInvolved = array_unique(array_merge( - [$this->book->id], - $collection->pluck('parentBookId')->values()->all(), - $collection->pluck('model.book_id')->values()->all(), - )); - - $books = Book::hasPermission('update')->whereIn('id', $bookIdsInvolved)->get(); - - if (count($books) !== count($bookIdsInvolved)) { - throw new SortOperationException('Could not find all books requested in sort operation'); + $books = Book::visible()->whereIn('id', array_unique($ids['book']))->get(); + /** @var Book $book */ + foreach ($books as $book) { + $modelMap['book:' . $book->id] = $book; } - return $books; + return $modelMap; } } diff --git a/app/Entities/Tools/BookSortMapItem.php b/app/Entities/Tools/BookSortMapItem.php index 6a2abc422..7fb9a1db5 100644 --- a/app/Entities/Tools/BookSortMapItem.php +++ b/app/Entities/Tools/BookSortMapItem.php @@ -2,8 +2,6 @@ namespace BookStack\Entities\Tools; -use BookStack\Entities\Models\BookChild; - class BookSortMapItem { @@ -32,11 +30,6 @@ class BookSortMapItem */ public $parentBookId; - /** - * @var ?BookChild - */ - public $model = null; - public function __construct(int $id, int $sort, ?int $parentChapterId, string $type, int $parentBookId) { diff --git a/app/Exceptions/SortOperationException.php b/app/Exceptions/SortOperationException.php deleted file mode 100644 index ade9e47d2..000000000 --- a/app/Exceptions/SortOperationException.php +++ /dev/null @@ -1,9 +0,0 @@ -get('sort-tree')); $bookContents = new BookContents($book); - $booksInvolved = collect(); - - try { - $booksInvolved = $bookContents->sortUsingMap($sortMap); - } catch (SortOperationException $exception) { - $this->showPermissionError(); - } + $booksInvolved = $bookContents->sortUsingMap($sortMap); // Rebuild permissions and add activity for involved books. - $booksInvolved->each(function (Book $book) { - Activity::add(ActivityType::BOOK_SORT, $book); - }); + foreach ($booksInvolved as $bookInvolved) { + Activity::add(ActivityType::BOOK_SORT, $bookInvolved); + } return redirect($book->getUrl()); }