Added additional permission checks and tests for book sorts

- Aligned permissions control with move operations to check
  delete/create permissions against old/new locations.
- Added tests to cover additional permissions scenarios.

diff --git a/app/Entities/Repos/ChapterRepo.php b/app/Entities/Repos/ChapterRepo.php
index 87f9e9e40cc851f14e011b0b3ce918e1faf1aa00..672c2140c3714c9efb970af0c8c56436449b041f 100644 (file)
--- a/app/Entities/Repos/ChapterRepo.php
+++ b/app/Entities/Repos/ChapterRepo.php
@@ -94,6 +94,8 @@ class ChapterRepo
             throw new MoveOperationException('Book to move chapter into not found');
         }
 
             throw new MoveOperationException('Book to move chapter into not found');
         }
 

+        // TODO - Check create permissions for new parent?
+

         $chapter->changeBook($parent->id);
         $chapter->rebuildPermissions();
         Activity::add(ActivityType::CHAPTER_MOVE, $chapter);
         $chapter->changeBook($parent->id);
         $chapter->rebuildPermissions();
         Activity::add(ActivityType::CHAPTER_MOVE, $chapter);

diff --git a/app/Entities/Tools/BookContents.php b/app/Entities/Tools/BookContents.php
index bdbc4262d21f9fefdab4b9fafa6ba743046bacc5..99602de414034a44ca9db88a4cd47655d814254f 100644 (file)
--- a/app/Entities/Tools/BookContents.php
+++ b/app/Entities/Tools/BookContents.php
@@ -174,7 +174,7 @@ class BookContents
 
         $currentParent = $modelMap[$currentParentKey] ?? null;
         /** @var Book $newBook */
 
         $currentParent = $modelMap[$currentParentKey] ?? null;
         /** @var Book $newBook */

-        $newBook = $modelMap['book:' . $sortMapItem->parentBookId];
+        $newBook = $modelMap['book:' . $sortMapItem->parentBookId] ?? null;

         /** @var ?Chapter $newChapter */
         $newChapter = $sortMapItem->parentChapterId ? ($modelMap['chapter:' . $sortMapItem->parentChapterId] ?? null) : null;
 
         /** @var ?Chapter $newChapter */
         $newChapter = $sortMapItem->parentChapterId ? ($modelMap['chapter:' . $sortMapItem->parentChapterId] ?? null) : null;
 


@@ -202,19 +202,27 @@ class BookContents
 
     /**
      * Check if the current user has permissions to apply the given sorting change.
 
     /**
      * Check if the current user has permissions to apply the given sorting change.

+     * Is quite complex since items can gain a different parent change. Acts as a:
+     * - Update of old parent element (Change of content/order).
+     * - Update of sorted/moved element.
+     * - Deletion of element (Relative to parent upon move).
+     * - Creation of element within parent (Upon move to new parent).

      */
      */

-    protected function isSortChangePermissible(BookSortMapItem $sortMapItem, Entity $model, ?Entity $currentParent, ?Entity $newBook, ?Entity $newChapter): bool
+    protected function isSortChangePermissible(BookSortMapItem $sortMapItem, BookChild $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;
         }
 
         // Stop if we can't see the current parent or new book.
         if (!$currentParent || !$newBook) {
             return false;
         }
 

+        $hasNewParent = $newBook->id !== $model->book_id || ($model instanceof Page && $model->chapter_id !== ($sortMapItem->parentChapterId ?? 0));

         if ($model instanceof Chapter) {
             $hasPermission = userCan('book-update', $currentParent)
         if ($model instanceof Chapter) {
             $hasPermission = userCan('book-update', $currentParent)

-                && userCan('book-update', $newBook);
+                && userCan('book-update', $newBook)
+                && userCan('chapter-update', $model)
+                && (!$hasNewParent || userCan('chapter-create', $newBook))
+                && (!$hasNewParent || userCan('chapter-delete', $model));
+

             if (!$hasPermission) {
                 return false;
             }
             if (!$hasPermission) {
                 return false;
             }


@@ -232,11 +240,21 @@ class BookContents
                 return false;
             }
 
                 return false;
             }
 

+            $hasPageEditPermission = userCan('page-update', $model);

             $newParentInRightLocation = ($newParent instanceof Book || $newParent->book_id === $newBook->id);
             $newParentPermission = ($newParent instanceof Chapter) ? 'chapter-update' : 'book-update';
             $hasNewParentPermission = userCan($newParentPermission, $newParent);
 
             $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;
+            $hasDeletePermissionIfMoving = (!$hasNewParent || userCan('page-delete', $model));
+            $hasCreatePermissionIfMoving = (!$hasNewParent || userCan('page-create', $newParent));
+
+            $hasPermission = $hasCurrentParentPermission
+                && $newParentInRightLocation
+                && $hasNewParentPermission
+                && $hasPageEditPermission
+                && $hasDeletePermissionIfMoving
+                && $hasCreatePermissionIfMoving;
+

             if (!$hasPermission) {
                 return false;
             }
             if (!$hasPermission) {
                 return false;
             }

diff --git a/app/Http/Controllers/ChapterController.php b/app/Http/Controllers/ChapterController.php
index 7541ad0dbd9811d76d17b7036fd631de07c1916f..5cd720f02b97ac360ad0fc65e4e61dcd38500c6a 100644 (file)
--- a/app/Http/Controllers/ChapterController.php
+++ b/app/Http/Controllers/ChapterController.php
@@ -178,6 +178,8 @@ class ChapterController extends Controller
             return redirect($chapter->getUrl());
         }
 
             return redirect($chapter->getUrl());
         }
 

+        // TODO - Check permissions against pages
+

         try {
             $newBook = $this->chapterRepo->move($chapter, $entitySelection);
         } catch (MoveOperationException $exception) {
         try {
             $newBook = $this->chapterRepo->move($chapter, $entitySelection);
         } catch (MoveOperationException $exception) {

diff --git a/tests/Entity/SortTest.php b/tests/Entity/SortTest.php
index 07e8b8ca8d322c7c50c7cedcd0b75e368c2fbb64..dcca426f77b79cf6a6c69946808b273856993ed2 100644 (file)
--- a/tests/Entity/SortTest.php
+++ b/tests/Entity/SortTest.php
@@ -33,9 +33,9 @@ class SortTest extends TestCase
 
     public function test_page_move_into_book()
     {
 
     public function test_page_move_into_book()
     {

-        $page = Page::first();
+        $page = Page::query()->first();

         $currentBook = $page->book;
         $currentBook = $page->book;

-        $newBook = Book::where('id', '!=', $currentBook->id)->first();
+        $newBook = Book::query()->where('id', '!=', $currentBook->id)->first();

 
         $resp = $this->asEditor()->get($page->getUrl('/move'));
         $resp->assertSee('Move Page');
 
         $resp = $this->asEditor()->get($page->getUrl('/move'));
         $resp->assertSee('Move Page');


@@ -43,7 +43,7 @@ class SortTest extends TestCase
         $movePageResp = $this->put($page->getUrl('/move'), [
             'entity_selection' => 'book:' . $newBook->id,
         ]);
         $movePageResp = $this->put($page->getUrl('/move'), [
             'entity_selection' => 'book:' . $newBook->id,
         ]);

-        $page = Page::find($page->id);
+        $page = Page::query()->find($page->id);

 
         $movePageResp->assertRedirect($page->getUrl());
         $this->assertTrue($page->book->id == $newBook->id, 'Page book is now the new book');
 
         $movePageResp->assertRedirect($page->getUrl());
         $this->assertTrue($page->book->id == $newBook->id, 'Page book is now the new book');


@@ -55,15 +55,15 @@ class SortTest extends TestCase
 
     public function test_page_move_into_chapter()
     {
 
     public function test_page_move_into_chapter()
     {

-        $page = Page::first();
+        $page = Page::query()->first();

         $currentBook = $page->book;
         $currentBook = $page->book;

-        $newBook = Book::where('id', '!=', $currentBook->id)->first();
+        $newBook = Book::query()->where('id', '!=', $currentBook->id)->first();

         $newChapter = $newBook->chapters()->first();
 
         $movePageResp = $this->actingAs($this->getEditor())->put($page->getUrl('/move'), [
             'entity_selection' => 'chapter:' . $newChapter->id,
         ]);
         $newChapter = $newBook->chapters()->first();
 
         $movePageResp = $this->actingAs($this->getEditor())->put($page->getUrl('/move'), [
             'entity_selection' => 'chapter:' . $newChapter->id,
         ]);

-        $page = Page::find($page->id);
+        $page = Page::query()->find($page->id);

 
         $movePageResp->assertRedirect($page->getUrl());
         $this->assertTrue($page->book->id == $newBook->id, 'Page parent is now the new chapter');
 
         $movePageResp->assertRedirect($page->getUrl());
         $this->assertTrue($page->book->id == $newBook->id, 'Page parent is now the new chapter');


@@ -74,9 +74,9 @@ class SortTest extends TestCase
 
     public function test_page_move_from_chapter_to_book()
     {
 
     public function test_page_move_from_chapter_to_book()
     {

-        $oldChapter = Chapter::first();
+        $oldChapter = Chapter::query()->first();

         $page = $oldChapter->pages()->first();
         $page = $oldChapter->pages()->first();

-        $newBook = Book::where('id', '!=', $oldChapter->book_id)->first();
+        $newBook = Book::query()->where('id', '!=', $oldChapter->book_id)->first();

 
         $movePageResp = $this->actingAs($this->getEditor())->put($page->getUrl('/move'), [
             'entity_selection' => 'book:' . $newBook->id,
 
         $movePageResp = $this->actingAs($this->getEditor())->put($page->getUrl('/move'), [
             'entity_selection' => 'book:' . $newBook->id,


@@ -110,7 +110,7 @@ class SortTest extends TestCase
             'entity_selection' => 'book:' . $newBook->id,
         ]);
 
             'entity_selection' => 'book:' . $newBook->id,
         ]);
 

-        $page = Page::find($page->id);
+        $page = Page::query()->find($page->id);

         $movePageResp->assertRedirect($page->getUrl());
 
         $this->assertTrue($page->book->id == $newBook->id, 'Page book is now the new book');
         $movePageResp->assertRedirect($page->getUrl());
 
         $this->assertTrue($page->book->id == $newBook->id, 'Page book is now the new book');


@@ -118,9 +118,9 @@ class SortTest extends TestCase
 
     public function test_page_move_requires_delete_permissions()
     {
 
     public function test_page_move_requires_delete_permissions()
     {

-        $page = Page::first();
+        $page = Page::query()->first();

         $currentBook = $page->book;
         $currentBook = $page->book;

-        $newBook = Book::where('id', '!=', $currentBook->id)->first();
+        $newBook = Book::query()->where('id', '!=', $currentBook->id)->first();

         $editor = $this->getEditor();
 
         $this->setEntityRestrictions($newBook, ['view', 'update', 'create', 'delete'], $editor->roles->all());
         $editor = $this->getEditor();
 
         $this->setEntityRestrictions($newBook, ['view', 'update', 'create', 'delete'], $editor->roles->all());


@@ -138,17 +138,17 @@ class SortTest extends TestCase
             'entity_selection' => 'book:' . $newBook->id,
         ]);
 
             'entity_selection' => 'book:' . $newBook->id,
         ]);
 

-        $page = Page::find($page->id);
+        $page = Page::query()->find($page->id);

         $movePageResp->assertRedirect($page->getUrl());
         $this->assertTrue($page->book->id == $newBook->id, 'Page book is now the new book');
     }
 
     public function test_chapter_move()
     {
         $movePageResp->assertRedirect($page->getUrl());
         $this->assertTrue($page->book->id == $newBook->id, 'Page book is now the new book');
     }
 
     public function test_chapter_move()
     {

-        $chapter = Chapter::first();
+        $chapter = Chapter::query()->first();

         $currentBook = $chapter->book;
         $pageToCheck = $chapter->pages->first();
         $currentBook = $chapter->book;
         $pageToCheck = $chapter->pages->first();

-        $newBook = Book::where('id', '!=', $currentBook->id)->first();
+        $newBook = Book::query()->where('id', '!=', $currentBook->id)->first();

 
         $chapterMoveResp = $this->asEditor()->get($chapter->getUrl('/move'));
         $chapterMoveResp->assertSee('Move Chapter');
 
         $chapterMoveResp = $this->asEditor()->get($chapter->getUrl('/move'));
         $chapterMoveResp->assertSee('Move Chapter');


@@ -157,7 +157,7 @@ class SortTest extends TestCase
             'entity_selection' => 'book:' . $newBook->id,
         ]);
 
             'entity_selection' => 'book:' . $newBook->id,
         ]);
 

-        $chapter = Chapter::find($chapter->id);
+        $chapter = Chapter::query()->find($chapter->id);

         $moveChapterResp->assertRedirect($chapter->getUrl());
         $this->assertTrue($chapter->book->id === $newBook->id, 'Chapter Book is now the new book');
 
         $moveChapterResp->assertRedirect($chapter->getUrl());
         $this->assertTrue($chapter->book->id === $newBook->id, 'Chapter Book is now the new book');
 


@@ -165,7 +165,7 @@ class SortTest extends TestCase
         $newBookResp->assertSee('moved chapter');
         $newBookResp->assertSee($chapter->name);
 
         $newBookResp->assertSee('moved chapter');
         $newBookResp->assertSee($chapter->name);
 

-        $pageToCheck = Page::find($pageToCheck->id);
+        $pageToCheck = Page::query()->find($pageToCheck->id);

         $this->assertTrue($pageToCheck->book_id === $newBook->id, 'Chapter child page\'s book id has changed to the new book');
         $pageCheckResp = $this->get($pageToCheck->getUrl());
         $pageCheckResp->assertSee($newBook->name);
         $this->assertTrue($pageToCheck->book_id === $newBook->id, 'Chapter child page\'s book id has changed to the new book');
         $pageCheckResp = $this->get($pageToCheck->getUrl());
         $pageCheckResp->assertSee($newBook->name);


@@ -173,9 +173,9 @@ class SortTest extends TestCase
 
     public function test_chapter_move_requires_delete_permissions()
     {
 
     public function test_chapter_move_requires_delete_permissions()
     {

-        $chapter = Chapter::first();
+        $chapter = Chapter::query()->first();

         $currentBook = $chapter->book;
         $currentBook = $chapter->book;

-        $newBook = Book::where('id', '!=', $currentBook->id)->first();
+        $newBook = Book::query()->where('id', '!=', $currentBook->id)->first();

         $editor = $this->getEditor();
 
         $this->setEntityRestrictions($newBook, ['view', 'update', 'create', 'delete'], $editor->roles->all());
         $editor = $this->getEditor();
 
         $this->setEntityRestrictions($newBook, ['view', 'update', 'create', 'delete'], $editor->roles->all());


@@ -193,7 +193,7 @@ class SortTest extends TestCase
             'entity_selection' => 'book:' . $newBook->id,
         ]);
 
             'entity_selection' => 'book:' . $newBook->id,
         ]);
 

-        $chapter = Chapter::find($chapter->id);
+        $chapter = Chapter::query()->find($chapter->id);

         $moveChapterResp->assertRedirect($chapter->getUrl());
         $this->assertTrue($chapter->book->id == $newBook->id, 'Page book is now the new book');
     }
         $moveChapterResp->assertRedirect($chapter->getUrl());
         $this->assertTrue($chapter->book->id == $newBook->id, 'Page book is now the new book');
     }


@@ -314,14 +314,83 @@ class SortTest extends TestCase
         ]);
     }
 
         ]);
     }
 

-    public function test_book_sort_makes_no_changes_if_no_update_permissions_on_new_chapter()
+    public function test_book_sort_makes_no_changes_if_no_view_permissions_on_new_book()
+    {
+        /** @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->book, ['update', 'delete'], [$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_makes_no_changes_if_no_update_or_create_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', 'delete'], [$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_makes_no_changes_if_no_update_permissions_on_moved_item()
+    {
+        /** @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($page, ['view', 'delete'], [$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_makes_no_changes_if_no_delete_permissions_on_moved_item()

     {
         /** @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();
     {
         /** @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()]);
+        $this->setEntityRestrictions($page, ['view', 'update'], [$editor->roles()->first()]);

 
         $sortData = [
             'id' => $page->id,
 
         $sortData = [
             'id' => $page->id,


@@ -337,6 +406,7 @@ class SortTest extends TestCase
         ]);
     }
 
         ]);
     }
 

+

     public function test_book_sort_item_returns_book_content()
     {
         $books = Book::all();
     public function test_book_sort_item_returns_book_content()
     {
         $books = Book::all();