From 4a24d1c31b7d244cb15eced098c5f8c3954f7fda Mon Sep 17 00:00:00 2001 From: Abijeet Date: Sun, 31 Dec 2017 16:25:21 +0530 Subject: [PATCH 1/5] Checks the target and the source book before performing the sort. Signed-off-by: Abijeet --- app/Http/Controllers/BookController.php | 27 +++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/app/Http/Controllers/BookController.php b/app/Http/Controllers/BookController.php index aa8f89ea4..c042c502b 100644 --- a/app/Http/Controllers/BookController.php +++ b/app/Http/Controllers/BookController.php @@ -195,20 +195,43 @@ class BookController extends Controller $sortMap = json_decode($request->get('sort-tree')); $defaultBookId = $book->id; + $permissionsList = [$book->id]; + // Loop through contents of provided map and update entities accordingly foreach ($sortMap as $bookChild) { $priority = $bookChild->sort; $id = intval($bookChild->id); $isPage = $bookChild->type == 'page'; - $bookId = $this->entityRepo->exists('book', $bookChild->book) ? intval($bookChild->book) : $defaultBookId; + $bookId = $defaultBookId; + $targetBook = $this->entityRepo->getById('book', $bookChild->book); + + // Check permission for target book + if (!empty($targetBook)) { + $bookId = $targetBook->id; + if (!in_array($bookId, $permissionsList)) { + $this->checkOwnablePermission('book-update', $targetBook); + // cache the permission for future use. + $permissionsList[] = $bookId; + } + } + $chapterId = ($isPage && $bookChild->parentChapter === false) ? 0 : intval($bookChild->parentChapter); $model = $this->entityRepo->getById($isPage?'page':'chapter', $id); + // Check permissions for the source book + $sourceBook = $model->book; + if (!in_array($sourceBook->id, $permissionsList)) { + $this->checkOwnablePermission('book-update', $sourceBook); + $permissionsList[] = $sourceBook->id; + } + // Update models only if there's a change in parent chain or ordering. if ($model->priority !== $priority || $model->book_id !== $bookId || ($isPage && $model->chapter_id !== $chapterId)) { $this->entityRepo->changeBook($isPage?'page':'chapter', $bookId, $model); $model->priority = $priority; - if ($isPage) $model->chapter_id = $chapterId; + if ($isPage) { + $model->chapter_id = $chapterId; + } $model->save(); $updatedModels->push($model); } From e13e71cbe06f42fae43d5389ba49f86315add44b Mon Sep 17 00:00:00 2001 From: Abijeet Date: Sun, 31 Dec 2017 16:44:46 +0530 Subject: [PATCH 2/5] Changed the sort view to only show books to which we have an update permission. Signed-off-by: Abijeet --- app/Http/Controllers/BookController.php | 6 ++---- app/Repos/EntityRepo.php | 11 ++++++----- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/app/Http/Controllers/BookController.php b/app/Http/Controllers/BookController.php index c042c502b..700f7a06f 100644 --- a/app/Http/Controllers/BookController.php +++ b/app/Http/Controllers/BookController.php @@ -155,7 +155,7 @@ class BookController extends Controller $book = $this->entityRepo->getBySlug('book', $bookSlug); $this->checkOwnablePermission('book-update', $book); $bookChildren = $this->entityRepo->getBookChildren($book, true); - $books = $this->entityRepo->getAll('book', false); + $books = $this->entityRepo->getAll('book', false, 'update'); $this->setPageTitle(trans('entities.books_sort_named', ['bookName'=>$book->getShortName()])); return view('books/sort', ['book' => $book, 'current' => $book, 'books' => $books, 'bookChildren' => $bookChildren]); } @@ -229,9 +229,7 @@ class BookController extends Controller if ($model->priority !== $priority || $model->book_id !== $bookId || ($isPage && $model->chapter_id !== $chapterId)) { $this->entityRepo->changeBook($isPage?'page':'chapter', $bookId, $model); $model->priority = $priority; - if ($isPage) { - $model->chapter_id = $chapterId; - } + if ($isPage) $model->chapter_id = $chapterId; $model->save(); $updatedModels->push($model); } diff --git a/app/Repos/EntityRepo.php b/app/Repos/EntityRepo.php index 24c680234..2c92e1907 100644 --- a/app/Repos/EntityRepo.php +++ b/app/Repos/EntityRepo.php @@ -113,9 +113,9 @@ class EntityRepo * @param bool $allowDrafts * @return \Illuminate\Database\Query\Builder */ - protected function entityQuery($type, $allowDrafts = false) + protected function entityQuery($type, $allowDrafts = false, $permission = 'view') { - $q = $this->permissionService->enforceEntityRestrictions($type, $this->getEntity($type), 'view'); + $q = $this->permissionService->enforceEntityRestrictions($type, $this->getEntity($type), $permission); if (strtolower($type) === 'page' && !$allowDrafts) { $q = $q->where('draft', '=', false); } @@ -196,14 +196,15 @@ class EntityRepo } /** - * Get all entities of a type limited by count unless count if false. + * Get all entities of a type with the given permission, limited by count unless count is false. * @param string $type * @param integer|bool $count + * @param string $permission * @return Collection */ - public function getAll($type, $count = 20) + public function getAll($type, $count = 20, $permission = 'view') { - $q = $this->entityQuery($type)->orderBy('name', 'asc'); + $q = $this->entityQuery($type, false, $permission)->orderBy('name', 'asc'); if ($count !== false) $q = $q->take($count); return $q->get(); } From e269cc7ea71ce385fab112dcd7502fa3936582dc Mon Sep 17 00:00:00 2001 From: Abijeet Date: Sun, 31 Dec 2017 20:17:08 +0530 Subject: [PATCH 3/5] Adds test case for sorting permissions. Signed-off-by: Abijeet --- tests/BrowserKitTest.php | 1 - tests/Permissions/RestrictionsTest.php | 67 ++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 1 deletion(-) diff --git a/tests/BrowserKitTest.php b/tests/BrowserKitTest.php index 1eabc7417..d5c9911f8 100644 --- a/tests/BrowserKitTest.php +++ b/tests/BrowserKitTest.php @@ -3,7 +3,6 @@ use BookStack\Entity; use BookStack\Role; use BookStack\Services\PermissionService; -use BookStack\User; use Illuminate\Contracts\Console\Kernel; use Illuminate\Foundation\Testing\DatabaseTransactions; use Laravel\BrowserKitTesting\TestCase; diff --git a/tests/Permissions/RestrictionsTest.php b/tests/Permissions/RestrictionsTest.php index 218b7a0d8..8f37b2517 100644 --- a/tests/Permissions/RestrictionsTest.php +++ b/tests/Permissions/RestrictionsTest.php @@ -3,6 +3,7 @@ use BookStack\Book; use BookStack\Services\PermissionService; use BookStack\User; +use BookStack\Repos\EntityRepo; class RestrictionsTest extends BrowserKitTest { @@ -554,4 +555,70 @@ class RestrictionsTest extends BrowserKitTest $this->dontSee(substr($bookChapter->name, 0, 15)); } + public function test_book_sort_view_permission() + { + $firstBook = Book::first(); + $secondBook = Book::find(2); + $thirdBook = Book::find(3); + + $this->setEntityRestrictions($firstBook, ['view', 'update']); + $this->setEntityRestrictions($secondBook, ['view']); + $this->setEntityRestrictions($thirdBook, ['view', 'update']); + + // Test sort page visibility + $this->actingAs($this->user)->visit($secondBook->getUrl() . '/sort') + ->see('You do not have permission') + ->seePageIs('/'); + + // Check sort page on first book + $this->actingAs($this->user)->visit($firstBook->getUrl() . '/sort') + ->see($thirdBook->name) + ->dontSee($secondBook->name); + } + + public function test_book_sort_permission() { + $firstBook = Book::first(); + $secondBook = Book::find(2); + + $this->setEntityRestrictions($firstBook, ['view', 'update']); + $this->setEntityRestrictions($secondBook, ['view']); + + $firstBookChapter = $this->app[EntityRepo::class]->createFromInput('chapter', + ['name' => 'first book chapter'], $firstBook); + $secondBookChapter = $this->app[EntityRepo::class]->createFromInput('chapter', + ['name' => 'second book chapter'], $secondBook); + + // Create request data + $reqData = [ + [ + 'id' => $firstBookChapter->id, + 'sort' => 0, + 'parentChapter' => false, + 'type' => 'chapter', + 'book' => $secondBook->id + ] + ]; + + // Move chapter from first book to a second book + $this->actingAs($this->user)->put($firstBook->getUrl() . '/sort', ['sort-tree' => json_encode($reqData)]) + ->followRedirects() + ->see('You do not have permission') + ->seePageIs('/'); + + $reqData = [ + [ + 'id' => $secondBookChapter->id, + 'sort' => 0, + 'parentChapter' => false, + 'type' => 'chapter', + 'book' => $firstBook->id + ] + ]; + + // Move chapter from second book to first book + $this->actingAs($this->user)->put($firstBook->getUrl() . '/sort', ['sort-tree' => json_encode($reqData)]) + ->followRedirects() + ->see('You do not have permission') + ->seePageIs('/'); + } } From a77756a2dad8a0766a7220d6c6d085f321233d5f Mon Sep 17 00:00:00 2001 From: Abijeet Date: Sat, 6 Jan 2018 01:04:48 +0530 Subject: [PATCH 4/5] Refactored the code to first check for the permissions before sorting the book. Signed-off-by: Abijeet --- app/Http/Controllers/BookController.php | 42 +++++++++++++++---------- 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/app/Http/Controllers/BookController.php b/app/Http/Controllers/BookController.php index 700f7a06f..d70f9d0df 100644 --- a/app/Http/Controllers/BookController.php +++ b/app/Http/Controllers/BookController.php @@ -195,7 +195,32 @@ class BookController extends Controller $sortMap = json_decode($request->get('sort-tree')); $defaultBookId = $book->id; + // Check permissions for all target and source books $permissionsList = [$book->id]; + foreach ($sortMap as $bookChild) { + // Check permission for target book + if (!in_array($bookChild->book, $permissionsList)) { + $targetBook = $this->entityRepo->getById('book', $bookChild->book); + if (!empty($targetBook)) { + $bookId = $targetBook->id; + $this->checkOwnablePermission('book-update', $targetBook); + // cache the permission for future use. + $permissionsList[] = $bookId; + } + } + + // Check permissions for the source book + $id = intval($bookChild->id); + $isPage = $bookChild->type == 'page'; + $model = $this->entityRepo->getById($isPage?'page':'chapter', $id); + $sourceBook = $model->book; + if (!in_array($sourceBook->id, $permissionsList)) { + $this->checkOwnablePermission('book-update', $sourceBook); + + // cache the permission for future use. + $permissionsList[] = $sourceBook->id; + } + } // Loop through contents of provided map and update entities accordingly foreach ($sortMap as $bookChild) { @@ -205,26 +230,9 @@ class BookController extends Controller $bookId = $defaultBookId; $targetBook = $this->entityRepo->getById('book', $bookChild->book); - // Check permission for target book - if (!empty($targetBook)) { - $bookId = $targetBook->id; - if (!in_array($bookId, $permissionsList)) { - $this->checkOwnablePermission('book-update', $targetBook); - // cache the permission for future use. - $permissionsList[] = $bookId; - } - } - $chapterId = ($isPage && $bookChild->parentChapter === false) ? 0 : intval($bookChild->parentChapter); $model = $this->entityRepo->getById($isPage?'page':'chapter', $id); - // Check permissions for the source book - $sourceBook = $model->book; - if (!in_array($sourceBook->id, $permissionsList)) { - $this->checkOwnablePermission('book-update', $sourceBook); - $permissionsList[] = $sourceBook->id; - } - // Update models only if there's a change in parent chain or ordering. if ($model->priority !== $priority || $model->book_id !== $bookId || ($isPage && $model->chapter_id !== $chapterId)) { $this->entityRepo->changeBook($isPage?'page':'chapter', $bookId, $model); From 281da59bae04068965213c74ff91d7f92749fe34 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 13 Jan 2018 16:44:47 +0000 Subject: [PATCH 5/5] Refactored book sort using collections --- app/Http/Controllers/BookController.php | 95 +++++++++++-------------- 1 file changed, 40 insertions(+), 55 deletions(-) diff --git a/app/Http/Controllers/BookController.php b/app/Http/Controllers/BookController.php index d70f9d0df..f1645bb4b 100644 --- a/app/Http/Controllers/BookController.php +++ b/app/Http/Controllers/BookController.php @@ -190,71 +190,56 @@ class BookController extends Controller } // Sort pages and chapters - $sortedBooks = []; - $updatedModels = collect(); - $sortMap = json_decode($request->get('sort-tree')); - $defaultBookId = $book->id; + $sortMap = collect(json_decode($request->get('sort-tree'))); + $bookIdsInvolved = collect([$book->id]); - // Check permissions for all target and source books - $permissionsList = [$book->id]; - foreach ($sortMap as $bookChild) { - // Check permission for target book - if (!in_array($bookChild->book, $permissionsList)) { - $targetBook = $this->entityRepo->getById('book', $bookChild->book); - if (!empty($targetBook)) { - $bookId = $targetBook->id; - $this->checkOwnablePermission('book-update', $targetBook); - // cache the permission for future use. - $permissionsList[] = $bookId; - } - } + // Load models into map + $sortMap->each(function($mapItem) use ($bookIdsInvolved) { + $mapItem->type = ($mapItem->type === 'page' ? 'page' : 'chapter'); + $mapItem->model = $this->entityRepo->getById($mapItem->type, $mapItem->id); + // Store source and target books + $bookIdsInvolved->push(intval($mapItem->model->book_id)); + $bookIdsInvolved->push(intval($mapItem->book)); + }); - // Check permissions for the source book - $id = intval($bookChild->id); - $isPage = $bookChild->type == 'page'; - $model = $this->entityRepo->getById($isPage?'page':'chapter', $id); - $sourceBook = $model->book; - if (!in_array($sourceBook->id, $permissionsList)) { - $this->checkOwnablePermission('book-update', $sourceBook); - - // cache the permission for future use. - $permissionsList[] = $sourceBook->id; - } + // Get the books involved in the sort + $bookIdsInvolved = $bookIdsInvolved->unique()->toArray(); + $booksInvolved = $this->entityRepo->book->newQuery()->whereIn('id', $bookIdsInvolved)->get(); + // Throw permission error if invalid ids or inaccessible books given. + if (count($bookIdsInvolved) !== count($booksInvolved)) { + $this->showPermissionError(); } + // Check permissions of involved books + $booksInvolved->each(function(Book $book) { + $this->checkOwnablePermission('book-update', $book); + }); - // Loop through contents of provided map and update entities accordingly - foreach ($sortMap as $bookChild) { - $priority = $bookChild->sort; - $id = intval($bookChild->id); - $isPage = $bookChild->type == 'page'; - $bookId = $defaultBookId; - $targetBook = $this->entityRepo->getById('book', $bookChild->book); + // Perform the sort + $sortMap->each(function($mapItem) { + $model = $mapItem->model; - $chapterId = ($isPage && $bookChild->parentChapter === false) ? 0 : intval($bookChild->parentChapter); - $model = $this->entityRepo->getById($isPage?'page':'chapter', $id); + $priorityChanged = intval($model->priority) !== intval($mapItem->sort); + $bookChanged = intval($model->book_id) !== intval($mapItem->book); + $chapterChanged = ($mapItem->type === 'page') && intval($model->chapter_id) !== $mapItem->parentChapter; - // Update models only if there's a change in parent chain or ordering. - if ($model->priority !== $priority || $model->book_id !== $bookId || ($isPage && $model->chapter_id !== $chapterId)) { - $this->entityRepo->changeBook($isPage?'page':'chapter', $bookId, $model); - $model->priority = $priority; - if ($isPage) $model->chapter_id = $chapterId; + if ($bookChanged) { + $this->entityRepo->changeBook($mapItem->type, $mapItem->book, $model); + } + if ($chapterChanged) { + $model->chapter_id = intval($mapItem->parentChapter); $model->save(); - $updatedModels->push($model); } - - // Store involved books to be sorted later - if (!in_array($bookId, $sortedBooks)) { - $sortedBooks[] = $bookId; + if ($priorityChanged) { + $model->priority = intval($mapItem->sort); + $model->save(); } - } + }); - // Add activity for books - foreach ($sortedBooks as $bookId) { - /** @var Book $updatedBook */ - $updatedBook = $this->entityRepo->getById('book', $bookId); - $this->entityRepo->buildJointPermissionsForBook($updatedBook); - Activity::add($updatedBook, 'book_sort', $updatedBook->id); - } + // Rebuild permissions and add activity for involved books. + $booksInvolved->each(function(Book $book) { + $this->entityRepo->buildJointPermissionsForBook($book); + Activity::add($book, 'book_sort', $book->id); + }); return redirect($book->getUrl()); }