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 96142bb7f4698ad130ae6e0f587d13e390a34dcd..ff018eda993c0d703762dfa4e9bcef58a95321b5 100644 (file)
--- 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<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->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<string, Entity>
      */
-    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 6a2abc42298f018ba5909fb4478945348cc9fe56..7fb9a1db561919ed1b663590de5a01d50695ae55 100644 (file)
--- 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 (file)
index ade9e47..0000000
--- a/app/Exceptions/SortOperationException.php
+++ /dev/null
@@ -1,9 +0,0 @@
-<?php
-
-namespace BookStack\Exceptions;
-
-use Exception;
-
-class SortOperationException extends Exception
-{
-}

diff --git a/app/Http/Controllers/BookSortController.php b/app/Http/Controllers/BookSortController.php
index 8fe05a9beb62a395da399ed7feb2f2588126fdd8..8aac2b76934cd9ca85c0e7b46cb5f5feac049e36 100644 (file)
--- a/app/Http/Controllers/BookSortController.php
+++ b/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();
-
-        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());
     }