From 0323ebccd3747cc757819e265ccad87889ec0acf Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Tue, 30 May 2023 20:55:24 +0100 Subject: [PATCH] Chapters API: Allowed move via book_id property Aligns it with pages and with the book_id property already being part of the API. For #4272. --- .../Controllers/ChapterApiController.php | 36 +++++++++++++------ tests/Api/ChaptersApiTest.php | 28 +++++++++++++++ tests/TestCase.php | 18 ++++------ 3 files changed, 60 insertions(+), 22 deletions(-) diff --git a/app/Entities/Controllers/ChapterApiController.php b/app/Entities/Controllers/ChapterApiController.php index ce20e6b96..403c58de3 100644 --- a/app/Entities/Controllers/ChapterApiController.php +++ b/app/Entities/Controllers/ChapterApiController.php @@ -5,14 +5,14 @@ namespace BookStack\Entities\Controllers; use BookStack\Entities\Models\Book; use BookStack\Entities\Models\Chapter; use BookStack\Entities\Repos\ChapterRepo; +use BookStack\Exceptions\PermissionsException; use BookStack\Http\ApiController; +use Exception; use Illuminate\Database\Eloquent\Relations\HasMany; use Illuminate\Http\Request; class ChapterApiController extends ApiController { - protected $chapterRepo; - protected $rules = [ 'create' => [ 'book_id' => ['required', 'integer'], @@ -28,12 +28,9 @@ class ChapterApiController extends ApiController ], ]; - /** - * ChapterController constructor. - */ - public function __construct(ChapterRepo $chapterRepo) - { - $this->chapterRepo = $chapterRepo; + public function __construct( + protected ChapterRepo $chapterRepo + ) { } /** @@ -54,13 +51,13 @@ class ChapterApiController extends ApiController */ public function create(Request $request) { - $this->validate($request, $this->rules['create']); + $requestData = $this->validate($request, $this->rules['create']); $bookId = $request->get('book_id'); $book = Book::visible()->findOrFail($bookId); $this->checkOwnablePermission('chapter-create', $book); - $chapter = $this->chapterRepo->create($request->all(), $book); + $chapter = $this->chapterRepo->create($requestData, $book); return response()->json($chapter->load(['tags'])); } @@ -79,13 +76,30 @@ class ChapterApiController extends ApiController /** * Update the details of a single chapter. + * Providing a 'book_id' property will essentially move the chapter + * into that parent element if you have permissions to do so. */ public function update(Request $request, string $id) { + $requestData = $this->validate($request, $this->rules()['update']); $chapter = Chapter::visible()->findOrFail($id); $this->checkOwnablePermission('chapter-update', $chapter); - $updatedChapter = $this->chapterRepo->update($chapter, $request->all()); + if ($request->has('book_id') && $chapter->book_id !== intval($requestData['book_id'])) { + $this->checkOwnablePermission('chapter-delete', $chapter); + + try { + $this->chapterRepo->move($chapter, "book:{$requestData['book_id']}"); + } catch (Exception $exception) { + if ($exception instanceof PermissionsException) { + $this->showPermissionError(); + } + + return $this->jsonError(trans('errors.selected_book_not_found')); + } + } + + $updatedChapter = $this->chapterRepo->update($chapter, $requestData); return response()->json($updatedChapter->load(['tags'])); } diff --git a/tests/Api/ChaptersApiTest.php b/tests/Api/ChaptersApiTest.php index a48e3b026..713d8bba4 100644 --- a/tests/Api/ChaptersApiTest.php +++ b/tests/Api/ChaptersApiTest.php @@ -2,6 +2,7 @@ namespace Tests\Api; +use BookStack\Entities\Models\Book; use BookStack\Entities\Models\Chapter; use Carbon\Carbon; use Illuminate\Support\Facades\DB; @@ -163,6 +164,33 @@ class ChaptersApiTest extends TestCase $this->assertGreaterThan(Carbon::now()->subDay()->unix(), $chapter->updated_at->unix()); } + public function test_update_with_book_id_moves_chapter() + { + $this->actingAsApiEditor(); + $chapter = $this->entities->chapterHasPages(); + $page = $chapter->pages()->first(); + $newBook = Book::query()->where('id', '!=', $chapter->book_id)->first(); + + $resp = $this->putJson($this->baseEndpoint . "/{$chapter->id}", ['book_id' => $newBook->id]); + $resp->assertOk(); + $chapter->refresh(); + + $this->assertDatabaseHas('chapters', ['id' => $chapter->id, 'book_id' => $newBook->id]); + $this->assertDatabaseHas('pages', ['id' => $page->id, 'book_id' => $newBook->id, 'chapter_id' => $chapter->id]); + } + + public function test_update_with_new_book_id_requires_delete_permission() + { + $editor = $this->users->editor(); + $this->permissions->removeUserRolePermissions($editor, ['chapter-delete-all', 'chapter-delete-own']); + $this->actingAs($editor); + $chapter = $this->entities->chapterHasPages(); + $newBook = Book::query()->where('id', '!=', $chapter->book_id)->first(); + + $resp = $this->putJson($this->baseEndpoint . "/{$chapter->id}", ['book_id' => $newBook->id]); + $this->assertPermissionError($resp); + } + public function test_delete_endpoint() { $this->actingAsApiEditor(); diff --git a/tests/TestCase.php b/tests/TestCase.php index abee1d3b3..322ab0370 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -213,18 +213,14 @@ abstract class TestCase extends BaseTestCase */ private function isPermissionError($response): bool { + if ($response->status() === 403 && $response instanceof JsonResponse) { + $errMessage = $response->getData(true)['error']['message'] ?? ''; + return $errMessage === 'You do not have permission to perform the requested action.'; + } + return $response->status() === 302 - && ( - ( - $response->headers->get('Location') === url('/') - && strpos(session()->pull('error', ''), 'You do not have permission to access') === 0 - ) - || - ( - $response instanceof JsonResponse && - $response->json(['error' => 'You do not have permission to perform the requested action.']) - ) - ); + && $response->headers->get('Location') === url('/') + && str_starts_with(session()->pull('error', ''), 'You do not have permission to access'); } /**