From 548dcd4db1754fc0f05cd578c6993b78dd279193 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sun, 11 Feb 2018 12:37:02 +0000 Subject: [PATCH] Fixed error when accessing non-authed attachment Also updated attachment tests to use standard test-case. Fixes #681 --- app/Http/Controllers/AttachmentController.php | 7 ++ resources/lang/en/errors.php | 1 + tests/AttachmentTest.php | 105 ++++++++++++------ tests/TestCase.php | 12 ++ 4 files changed, 91 insertions(+), 34 deletions(-) diff --git a/app/Http/Controllers/AttachmentController.php b/app/Http/Controllers/AttachmentController.php index 3c325d0fe..ea41278ae 100644 --- a/app/Http/Controllers/AttachmentController.php +++ b/app/Http/Controllers/AttachmentController.php @@ -2,6 +2,7 @@ use BookStack\Exceptions\FileUploadException; use BookStack\Attachment; +use BookStack\Exceptions\NotFoundException; use BookStack\Repos\EntityRepo; use BookStack\Services\AttachmentService; use Illuminate\Http\Request; @@ -182,11 +183,16 @@ class AttachmentController extends Controller * Get an attachment from storage. * @param $attachmentId * @return \Illuminate\Contracts\Routing\ResponseFactory|\Illuminate\Http\RedirectResponse|\Illuminate\Routing\Redirector|\Symfony\Component\HttpFoundation\Response + * @throws \Illuminate\Contracts\Filesystem\FileNotFoundException */ public function get($attachmentId) { $attachment = $this->attachment->findOrFail($attachmentId); $page = $this->entityRepo->getById('page', $attachment->uploaded_to); + if ($page === null) { + throw new NotFoundException(trans('errors.attachment_not_found')); + } + $this->checkOwnablePermission('page-view', $page); if ($attachment->external) { @@ -204,6 +210,7 @@ class AttachmentController extends Controller * Delete a specific attachment in the system. * @param $attachmentId * @return mixed + * @throws \Exception */ public function delete($attachmentId) { diff --git a/resources/lang/en/errors.php b/resources/lang/en/errors.php index bbcbdaec2..3b1d6e8b3 100644 --- a/resources/lang/en/errors.php +++ b/resources/lang/en/errors.php @@ -40,6 +40,7 @@ return [ // Attachments 'attachment_page_mismatch' => 'Page mismatch during attachment update', + 'attachment_not_found' => 'Attachment not found', // Pages 'page_draft_autosave_fail' => 'Failed to save draft. Ensure you have internet connection before saving this page', diff --git a/tests/AttachmentTest.php b/tests/AttachmentTest.php index a17f003db..bb3a92706 100644 --- a/tests/AttachmentTest.php +++ b/tests/AttachmentTest.php @@ -1,6 +1,10 @@ asAdmin(); $admin = $this->getAdmin(); $fileName = 'upload_test_file.txt'; @@ -63,37 +67,41 @@ class AttachmentTest extends BrowserKitTest 'path' => $this->getUploadPath($fileName) ]; - $this->uploadFile($fileName, $page->id); - $this->assertResponseOk(); - $this->seeJsonContains($expectedResp); - $this->seeInDatabase('attachments', $expectedResp); + $upload = $this->uploadFile($fileName, $page->id); + $upload->assertStatus(200); + $upload->assertJson($expectedResp); + $this->assertDatabaseHas('attachments', $expectedResp); $this->deleteUploads(); } public function test_file_display_and_access() { - $page = \BookStack\Page::first(); + $page = Page::first(); $this->asAdmin(); $fileName = 'upload_test_file.txt'; - $this->uploadFile($fileName, $page->id); - $this->assertResponseOk(); - $this->visit($page->getUrl()) - ->seeLink($fileName) - ->click($fileName) - ->see('Hi, This is a test file for testing the upload process.'); + $upload = $this->uploadFile($fileName, $page->id); + $upload->assertStatus(200); + $attachment = Attachment::orderBy('id', 'desc')->take(1)->first(); + + $pageGet = $this->get($page->getUrl()); + $pageGet->assertSeeText($fileName); + $pageGet->assertSee($attachment->getUrl()); + + $attachmentGet = $this->get($attachment->getUrl()); + $attachmentGet->assertSee('Hi, This is a test file for testing the upload process.'); $this->deleteUploads(); } public function test_attaching_link_to_page() { - $page = \BookStack\Page::first(); + $page = Page::first(); $admin = $this->getAdmin(); $this->asAdmin(); - $this->call('POST', 'attachments/link', [ + $linkReq = $this->call('POST', 'attachments/link', [ 'link' => 'https://example.com', 'name' => 'Example Attachment Link', 'uploaded_to' => $page->id, @@ -110,19 +118,24 @@ class AttachmentTest extends BrowserKitTest 'extension' => '' ]; - $this->assertResponseOk(); - $this->seeJsonContains($expectedResp); - $this->seeInDatabase('attachments', $expectedResp); + $linkReq->assertStatus(200); + $linkReq->assertJson($expectedResp); + $this->assertDatabaseHas('attachments', $expectedResp); + $attachment = Attachment::orderBy('id', 'desc')->take(1)->first(); - $this->visit($page->getUrl())->seeLink('Example Attachment Link') - ->click('Example Attachment Link')->seePageIs('https://example.com'); + $pageGet = $this->get($page->getUrl()); + $pageGet->assertSeeText('Example Attachment Link'); + $pageGet->assertSee($attachment->getUrl()); + + $attachmentGet = $this->get($attachment->getUrl()); + $attachmentGet->assertRedirect('https://example.com'); $this->deleteUploads(); } public function test_attachment_updating() { - $page = \BookStack\Page::first(); + $page = Page::first(); $this->asAdmin(); $this->call('POST', 'attachments/link', [ @@ -133,7 +146,7 @@ class AttachmentTest extends BrowserKitTest $attachmentId = \BookStack\Attachment::first()->id; - $this->call('PUT', 'attachments/' . $attachmentId, [ + $update = $this->call('PUT', 'attachments/' . $attachmentId, [ 'uploaded_to' => $page->id, 'name' => 'My new attachment name', 'link' => 'https://test.example.com' @@ -145,28 +158,27 @@ class AttachmentTest extends BrowserKitTest 'uploaded_to' => $page->id ]; - $this->assertResponseOk(); - $this->seeJsonContains($expectedResp); - $this->seeInDatabase('attachments', $expectedResp); + $update->assertStatus(200); + $update->assertJson($expectedResp); + $this->assertDatabaseHas('attachments', $expectedResp); $this->deleteUploads(); } public function test_file_deletion() { - $page = \BookStack\Page::first(); + $page = Page::first(); $this->asAdmin(); $fileName = 'deletion_test.txt'; $this->uploadFile($fileName, $page->id); $filePath = base_path('storage/' . $this->getUploadPath($fileName)); - $this->assertTrue(file_exists($filePath), 'File at path ' . $filePath . ' does not exist'); - $attachmentId = \BookStack\Attachment::first()->id; - $this->call('DELETE', 'attachments/' . $attachmentId); + $attachment = \BookStack\Attachment::first(); + $this->delete($attachment->getUrl()); - $this->dontSeeInDatabase('attachments', [ + $this->assertDatabaseMissing('attachments', [ 'name' => $fileName ]); $this->assertFalse(file_exists($filePath), 'File at path ' . $filePath . ' was not deleted as expected'); @@ -176,7 +188,7 @@ class AttachmentTest extends BrowserKitTest public function test_attachment_deletion_on_page_deletion() { - $page = \BookStack\Page::first(); + $page = Page::first(); $this->asAdmin(); $fileName = 'deletion_test.txt'; $this->uploadFile($fileName, $page->id); @@ -184,17 +196,42 @@ class AttachmentTest extends BrowserKitTest $filePath = base_path('storage/' . $this->getUploadPath($fileName)); $this->assertTrue(file_exists($filePath), 'File at path ' . $filePath . ' does not exist'); - $this->seeInDatabase('attachments', [ + $this->assertDatabaseHas('attachments', [ 'name' => $fileName ]); $this->call('DELETE', $page->getUrl()); - $this->dontSeeInDatabase('attachments', [ + $this->assertDatabaseMissing('attachments', [ 'name' => $fileName ]); $this->assertFalse(file_exists($filePath), 'File at path ' . $filePath . ' was not deleted as expected'); $this->deleteUploads(); } + + public function test_attachment_access_without_permission_shows_404() + { + $admin = $this->getAdmin(); + $viewer = $this->getViewer(); + $page = Page::first(); + + $this->actingAs($admin); + $fileName = 'permission_test.txt'; + $this->uploadFile($fileName, $page->id); + $attachment = Attachment::orderBy('id', 'desc')->take(1)->first(); + + $page->restricted = true; + $page->permissions()->delete(); + $page->save(); + $this->app[PermissionService::class]->buildJointPermissionsForEntity($page); + $page->load('jointPermissions'); + + $this->actingAs($viewer); + $attachmentGet = $this->get($attachment->getUrl()); + $attachmentGet->assertStatus(404); + $attachmentGet->assertSee("Attachment not found"); + + $this->deleteUploads(); + } } diff --git a/tests/TestCase.php b/tests/TestCase.php index 94751b004..5c37b6179 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -65,6 +65,18 @@ abstract class TestCase extends BaseTestCase return $this->editor; } + /** + * Get an instance of a user with 'viewer' permissions + * @param $attributes + * @return mixed + */ + protected function getViewer($attributes = []) + { + $user = \BookStack\Role::getRole('viewer')->users()->first(); + if (!empty($attributes)) $user->forceFill($attributes)->save(); + return $user; + } + /** * Create and return a new book. * @param array $input