Altered sort permission checking and started tests

Previous implemenations were hard to read so changing to be more
logically simplistic. Still needs further coverage in tests and
review/alignment of permissions to use.

diff --git a/app/Entities/Tools/BookContents.php b/app/Entities/Tools/BookContents.php
index ff018eda993c0d703762dfa4e9bcef58a95321b5..bdbc4262d21f9fefdab4b9fafa6ba743046bacc5 100644 (file)
--- a/app/Entities/Tools/BookContents.php
+++ b/app/Entities/Tools/BookContents.php
@@ -116,8 +116,17 @@ class BookContents
         // Load models into map
         $modelMap = $this->loadModelsFromSortMap($sortMap);
 
+        // Sort our changes from our map to be chapters first
+        // Since they need to be process to ensure book alignment for child page changes.
+        $sortMapItems = $sortMap->all();
+        usort($sortMapItems, function(BookSortMapItem $itemA, BookSortMapItem $itemB) {
+            $aScore = $itemA->type === 'page' ? 2 : 1;
+            $bScore = $itemB->type === 'page' ? 2 : 1;
+            return $aScore - $bScore;
+        });
+
         // Perform the sort
-        foreach ($sortMap->all() as $item) {
+        foreach ($sortMapItems as $item) {
             $this->applySortUpdates($item, $modelMap);
         }
 
@@ -163,32 +172,23 @@ class BookContents
              $currentParentKey = 'chapter:' . $model->chapter_id;
         }
 
-        $currentParent = $modelMap[$currentParentKey];
+        $currentParent = $modelMap[$currentParentKey] ?? null;
         /** @var Book $newBook */
-        $newBook = $modelMap['book:' . $sortMapItem->parentBookId] ?? null;
+        $newBook = $modelMap['book:' . $sortMapItem->parentBookId];
         /** @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) {
+        if (!$this->isSortChangePermissible($sortMapItem, $model, $currentParent, $newBook, $newChapter)) {
             return;
         }
 
         // Action the required changes
         if ($bookChanged) {
-            $model->changeBook($sortMapItem->parentBookId);
+            $model->changeBook($newBook->id);
         }
 
         if ($chapterChanged) {
-            $model->chapter_id = $sortMapItem->parentChapterId ?? 0;
+            $model->chapter_id = $newChapter->id ?? 0;
         }
 
         if ($priorityChanged) {
@@ -200,6 +200,51 @@ class BookContents
         }
     }
 
+    /**
+     * Check if the current user has permissions to apply the given sorting change.
+     */
+    protected function isSortChangePermissible(BookSortMapItem $sortMapItem, Entity $model, ?Entity $currentParent, ?Entity $newBook, ?Entity $newChapter): bool
+    {
+        // TODO - Move operations check for create permissions, Needs these also/instead?
+
+        // Stop if we can't see the current parent or new book.
+        if (!$currentParent || !$newBook) {
+            return false;
+        }
+
+        if ($model instanceof Chapter) {
+            $hasPermission = userCan('book-update', $currentParent)
+                && userCan('book-update', $newBook);
+            if (!$hasPermission) {
+                return false;
+            }
+        }
+
+        if ($model instanceof Page) {
+            $parentPermission = ($currentParent instanceof Chapter) ? 'chapter-update' : 'book-update';
+            $hasCurrentParentPermission = userCan($parentPermission, $currentParent);
+
+            // This needs to check if there was an intended chapter location in the original sort map
+            // rather than inferring from the $newChapter since that variable may be null
+            // due to other reasons (Visibility).
+            $newParent = $sortMapItem->parentChapterId ? $newChapter : $newBook;
+            if (!$newParent) {
+                return false;
+            }
+
+            $newParentInRightLocation = ($newParent instanceof Book || $newParent->book_id === $newBook->id);
+            $newParentPermission = ($newParent instanceof Chapter) ? 'chapter-update' : 'book-update';
+            $hasNewParentPermission = userCan($newParentPermission, $newParent);
+
+            $hasPermission = $hasCurrentParentPermission && $newParentInRightLocation && $hasNewParentPermission;
+            if (!$hasPermission) {
+                return false;
+            }
+        }
+
+        return true;
+    }
+
     /**
      * Load models from the database into the given sort map.
      * @return array<string, Entity>

diff --git a/tests/Entity/SortTest.php b/tests/Entity/SortTest.php
index 89279bfcf9059b6daef98b853bce0412bdd8a817..07e8b8ca8d322c7c50c7cedcd0b75e368c2fbb64 100644 (file)
--- a/tests/Entity/SortTest.php
+++ b/tests/Entity/SortTest.php
@@ -239,20 +239,20 @@ class SortTest extends TestCase
         // Create request data
         $reqData = [
             [
-                'id'            => $chapterToMove->id,
-                'sort'          => 0,
+                'id' => $chapterToMove->id,
+                'sort' => 0,
                 'parentChapter' => false,
-                'type'          => 'chapter',
-                'book'          => $newBook->id,
+                'type' => 'chapter',
+                'book' => $newBook->id,
             ],
         ];
         foreach ($pagesToMove as $index => $page) {
             $reqData[] = [
-                'id'            => $page->id,
-                'sort'          => $index,
+                'id' => $page->id,
+                'sort' => $index,
                 'parentChapter' => $index === count($pagesToMove) - 1 ? $chapterToMove->id : false,
-                'type'          => 'page',
-                'book'          => $newBook->id,
+                'type' => 'page',
+                'book' => $newBook->id,
             ];
         }
 
@@ -260,18 +260,83 @@ class SortTest extends TestCase
         $sortResp->assertRedirect($newBook->getUrl());
         $sortResp->assertStatus(302);
         $this->assertDatabaseHas('chapters', [
-            'id'       => $chapterToMove->id,
-            'book_id'  => $newBook->id,
+            'id' => $chapterToMove->id,
+            'book_id' => $newBook->id,
             'priority' => 0,
         ]);
         $this->assertTrue($newBook->chapters()->count() === 1);
         $this->assertTrue($newBook->chapters()->first()->pages()->count() === 1);
 
         $checkPage = $pagesToMove[1];
-        $checkResp = $this->get(Page::find($checkPage->id)->getUrl());
+        $checkResp = $this->get($checkPage->refresh()->getUrl());
         $checkResp->assertSee($newBook->name);
     }
 
+    public function test_book_sort_makes_no_changes_if_new_chapter_does_not_align_with_new_book()
+    {
+        /** @var Page $page */
+        $page = Page::query()->where('chapter_id', '!=', 0)->first();
+        $otherChapter = Chapter::query()->where('book_id', '!=', $page->book_id)->first();
+
+        $sortData = [
+            'id' => $page->id,
+            'sort' => 0,
+            'parentChapter' => $otherChapter->id,
+            'type' => 'page',
+            'book' => $page->book_id,
+        ];
+        $this->asEditor()->put($page->book->getUrl('/sort'), ['sort-tree' => json_encode([$sortData])])->assertRedirect();
+
+        $this->assertDatabaseHas('pages', [
+            'id' => $page->id, 'chapter_id' => $page->chapter_id, 'book_id' => $page->book_id,
+        ]);
+    }
+
+    public function test_book_sort_makes_no_changes_if_no_view_permissions_on_new_chapter()
+    {
+        /** @var Page $page */
+        $page = Page::query()->where('chapter_id', '!=', 0)->first();
+        /** @var Chapter $otherChapter */
+        $otherChapter = Chapter::query()->where('book_id', '!=', $page->book_id)->first();
+        $this->setEntityRestrictions($otherChapter);
+
+        $sortData = [
+            'id' => $page->id,
+            'sort' => 0,
+            'parentChapter' => $otherChapter->id,
+            'type' => 'page',
+            'book' => $otherChapter->book_id,
+        ];
+        $this->asEditor()->put($page->book->getUrl('/sort'), ['sort-tree' => json_encode([$sortData])])->assertRedirect();
+
+        $this->assertDatabaseHas('pages', [
+            'id' => $page->id, 'chapter_id' => $page->chapter_id, 'book_id' => $page->book_id,
+        ]);
+    }
+
+    public function test_book_sort_makes_no_changes_if_no_update_permissions_on_new_chapter()
+    {
+        /** @var Page $page */
+        $page = Page::query()->where('chapter_id', '!=', 0)->first();
+        /** @var Chapter $otherChapter */
+        $otherChapter = Chapter::query()->where('book_id', '!=', $page->book_id)->first();
+        $editor = $this->getEditor();
+        $this->setEntityRestrictions($otherChapter, ['view'], [$editor->roles()->first()]);
+
+        $sortData = [
+            'id' => $page->id,
+            'sort' => 0,
+            'parentChapter' => $otherChapter->id,
+            'type' => 'page',
+            'book' => $otherChapter->book_id,
+        ];
+        $this->actingAs($editor)->put($page->book->getUrl('/sort'), ['sort-tree' => json_encode([$sortData])])->assertRedirect();
+
+        $this->assertDatabaseHas('pages', [
+            'id' => $page->id, 'chapter_id' => $page->chapter_id, 'book_id' => $page->book_id,
+        ]);
+    }
+
     public function test_book_sort_item_returns_book_content()
     {
         $books = Book::all();