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.

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);
+        $modelMap = $this->loadModelsFromSortMap($sortMap);
-        $booksInvolved = $this->getBooksInvolvedInSort($sortMap);
 
         // Perform the sort
         foreach ($sortMap->all() as $item) {
-            $this->applySortUpdates($item);
+            $this->applySortUpdates($item, $modelMap);
         }
 
-        // Update permissions and activity.
+        /** @var Book[] $booksInvolved */
-        $booksInvolved->each(function (Book $book) {
+        $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<string, Entity> $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->chapter_id = $sortMapItem->parentChapterId ?? 0;
-            $model->save();
         }
 
         if ($priorityChanged) {
             $model->priority = $sortMapItem->sort;
+        }
+
+        if ($chapterChanged || $priorityChanged) {
             $model->save();
         }
     }
 
     /**
      * Load models from the database into the given sort map.
+     * @return array<string, Entity>
      */
-    protected function loadModelsIntoSortMap(BookSortMap $sortMap): void
+    protected function loadModelsFromSortMap(BookSortMap $sortMap): array
     {
-        $collection = collect($sortMap->all());
+        $modelMap = [];
+        $ids = [
+            'chapter' => [],
+            'page' => [],
+            'book' => [],
+        ];
 
-        $keyMap = $collection->keyBy(function (BookSortMapItem $sortMapItem) {
+        foreach ($sortMap->all() as $sortMapItem) {
-            return  $sortMapItem->type . ':' . $sortMapItem->id;
+            $ids[$sortMapItem->type][] = $sortMapItem->id;
-        });
+            $ids['book'][] = $sortMapItem->parentBookId;
-
+            if ($sortMapItem->parentChapterId) {
-        $pageIds = $collection->where('type', '=', 'page')->pluck('id');
+                $ids['chapter'][] = $sortMapItem->parentChapterId;
-        $chapterIds = $collection->where('type', '=', 'chapter')->pluck('id');
+            }
-
+        }
-        $pages = Page::visible()->whereIn('id', $pageIds)->get();
-        $chapters = Chapter::visible()->whereIn('id', $chapterIds)->get();
 
+        $pages = Page::visible()->whereIn('id', array_unique($ids['page']))->get(Page::$listAttributes);
+        /** @var Page $page */
         foreach ($pages as $page) {
-            /** @var BookSortMapItem $sortItem */
+            $modelMap['page:' . $page->id] = $page;
-            $sortItem = $keyMap->get('page:' . $page->id);
+            $ids['book'][] = $page->book_id;
-            $sortItem->model = $page;
+            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 */
+            $modelMap['chapter:' . $chapter->id] = $chapter;
-            $sortItem = $keyMap->get('chapter:' . $chapter->id);
+            $ids['book'][] = $chapter->book_id;
-            $sortItem->model = $chapter;
-        }
-    }
-
-    /**
-     * 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');
         }
 
-        return $books;
+        $books = Book::visible()->whereIn('id', array_unique($ids['book']))->get();
+        /** @var Book $book */
+        foreach ($books as $book) {
+            $modelMap['book:' . $book->id] = $book;
+        }
+
+        return $modelMap;
     }
 }

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)
     {

app/Exceptions/SortOperationException.php

@@ -1,9 +0,0 @@
-<?php
-
-namespace BookStack\Exceptions;
-
-use Exception;
-
-class SortOperationException extends Exception
-{
-}

app/Http/Controllers/BookSortController.php

@@ -3,11 +3,9 @@
 namespace BookStack\Http\Controllers;
 
 use BookStack\Actions\ActivityType;
-use BookStack\Entities\Models\Book;
 use BookStack\Entities\Repos\BookRepo;
 use BookStack\Entities\Tools\BookContents;
 use BookStack\Entities\Tools\BookSortMap;
-use BookStack\Exceptions\SortOperationException;
 use BookStack\Facades\Activity;
 use Illuminate\Http\Request;
 
@@ -62,18 +60,12 @@ class BookSortController extends Controller
 
         $sortMap = BookSortMap::fromJson($request->get('sort-tree'));
         $bookContents = new BookContents($book);
-        $booksInvolved = collect();
+        $booksInvolved = $bookContents->sortUsingMap($sortMap);
-
-        try {
-            $booksInvolved = $bookContents->sortUsingMap($sortMap);
-        } catch (SortOperationException $exception) {
-            $this->showPermissionError();
-        }
 
         // Rebuild permissions and add activity for involved books.
-        $booksInvolved->each(function (Book $book) {
+        foreach ($booksInvolved as $bookInvolved) {
-            Activity::add(ActivityType::BOOK_SORT, $book);
+            Activity::add(ActivityType::BOOK_SORT, $bookInvolved);
-        });
+        }
 
         return redirect($book->getUrl());
     }