From eda9e89c55caf6a98ab57a084d1ce03e0c5fe993 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 28 Aug 2021 21:48:17 +0100 Subject: [PATCH 1/3] Added role permissions for exporting content --- .../Api/BookExportApiController.php | 1 + .../Api/ChapterExportApiController.php | 1 + .../Api/PageExportApiController.php | 1 + app/Http/Controllers/BookExportController.php | 1 + .../Controllers/ChapterExportController.php | 1 + app/Http/Controllers/PageExportController.php | 1 + app/Http/Kernel.php | 3 +- .../Middleware/CheckUserHasPermission.php | 38 +++++++++++++++ app/Http/Middleware/PermissionMiddleware.php | 28 ----------- ...8_28_161743_add_export_role_permission.php | 48 +++++++++++++++++++ resources/lang/en/settings.php | 1 + resources/views/books/show.blade.php | 4 +- resources/views/chapters/show.blade.php | 4 +- resources/views/pages/show.blade.php | 4 +- .../views/settings/roles/parts/form.blade.php | 3 +- tests/Api/BooksApiTest.php | 13 +++++ tests/Api/ChaptersApiTest.php | 13 +++++ tests/Api/PagesApiTest.php | 13 +++++ tests/Entity/ExportTest.php | 26 ++++++++++ tests/SharedTestHelpers.php | 28 ++++++++++- 20 files changed, 196 insertions(+), 36 deletions(-) create mode 100644 app/Http/Middleware/CheckUserHasPermission.php delete mode 100644 app/Http/Middleware/PermissionMiddleware.php create mode 100644 database/migrations/2021_08_28_161743_add_export_role_permission.php diff --git a/app/Http/Controllers/Api/BookExportApiController.php b/app/Http/Controllers/Api/BookExportApiController.php index c7d121f88..028bc3a81 100644 --- a/app/Http/Controllers/Api/BookExportApiController.php +++ b/app/Http/Controllers/Api/BookExportApiController.php @@ -13,6 +13,7 @@ class BookExportApiController extends ApiController public function __construct(ExportFormatter $exportFormatter) { $this->exportFormatter = $exportFormatter; + $this->middleware('can:content-export'); } /** diff --git a/app/Http/Controllers/Api/ChapterExportApiController.php b/app/Http/Controllers/Api/ChapterExportApiController.php index 5dfcea344..5715ab2e3 100644 --- a/app/Http/Controllers/Api/ChapterExportApiController.php +++ b/app/Http/Controllers/Api/ChapterExportApiController.php @@ -16,6 +16,7 @@ class ChapterExportApiController extends ApiController public function __construct(ExportFormatter $exportFormatter) { $this->exportFormatter = $exportFormatter; + $this->middleware('can:content-export'); } /** diff --git a/app/Http/Controllers/Api/PageExportApiController.php b/app/Http/Controllers/Api/PageExportApiController.php index 7cee2fbe7..ce5700c79 100644 --- a/app/Http/Controllers/Api/PageExportApiController.php +++ b/app/Http/Controllers/Api/PageExportApiController.php @@ -13,6 +13,7 @@ class PageExportApiController extends ApiController public function __construct(ExportFormatter $exportFormatter) { $this->exportFormatter = $exportFormatter; + $this->middleware('can:content-export'); } /** diff --git a/app/Http/Controllers/BookExportController.php b/app/Http/Controllers/BookExportController.php index 5e6d7c6ad..7f6dd8017 100644 --- a/app/Http/Controllers/BookExportController.php +++ b/app/Http/Controllers/BookExportController.php @@ -18,6 +18,7 @@ class BookExportController extends Controller { $this->bookRepo = $bookRepo; $this->exportFormatter = $exportFormatter; + $this->middleware('can:content-export'); } /** diff --git a/app/Http/Controllers/ChapterExportController.php b/app/Http/Controllers/ChapterExportController.php index 2758496d2..480280c99 100644 --- a/app/Http/Controllers/ChapterExportController.php +++ b/app/Http/Controllers/ChapterExportController.php @@ -19,6 +19,7 @@ class ChapterExportController extends Controller { $this->chapterRepo = $chapterRepo; $this->exportFormatter = $exportFormatter; + $this->middleware('can:content-export'); } /** diff --git a/app/Http/Controllers/PageExportController.php b/app/Http/Controllers/PageExportController.php index 5a3315216..0287916de 100644 --- a/app/Http/Controllers/PageExportController.php +++ b/app/Http/Controllers/PageExportController.php @@ -20,6 +20,7 @@ class PageExportController extends Controller { $this->pageRepo = $pageRepo; $this->exportFormatter = $exportFormatter; + $this->middleware('can:content-export'); } /** diff --git a/app/Http/Kernel.php b/app/Http/Kernel.php index d1f5de917..2d85c870d 100644 --- a/app/Http/Kernel.php +++ b/app/Http/Kernel.php @@ -48,10 +48,9 @@ class Kernel extends HttpKernel */ protected $routeMiddleware = [ 'auth' => \BookStack\Http\Middleware\Authenticate::class, - 'can' => \Illuminate\Auth\Middleware\Authorize::class, + 'can' => \BookStack\Http\Middleware\CheckUserHasPermission::class, 'guest' => \BookStack\Http\Middleware\RedirectIfAuthenticated::class, 'throttle' => \Illuminate\Routing\Middleware\ThrottleRequests::class, - 'perm' => \BookStack\Http\Middleware\PermissionMiddleware::class, 'guard' => \BookStack\Http\Middleware\CheckGuard::class, 'mfa-setup' => \BookStack\Http\Middleware\AuthenticatedOrPendingMfa::class, ]; diff --git a/app/Http/Middleware/CheckUserHasPermission.php b/app/Http/Middleware/CheckUserHasPermission.php new file mode 100644 index 000000000..4340152e7 --- /dev/null +++ b/app/Http/Middleware/CheckUserHasPermission.php @@ -0,0 +1,38 @@ +can($permission)) { + return $this->errorResponse($request); + } + + return $next($request); + } + + + protected function errorResponse(Request $request) + { + if ($request->wantsJson()) { + return response()->json(['error' => trans('errors.permissionJson')], 403); + } + + session()->flash('error', trans('errors.permission')); + return redirect('/'); + } +} diff --git a/app/Http/Middleware/PermissionMiddleware.php b/app/Http/Middleware/PermissionMiddleware.php deleted file mode 100644 index 1d7e4aaa1..000000000 --- a/app/Http/Middleware/PermissionMiddleware.php +++ /dev/null @@ -1,28 +0,0 @@ -user() || !$request->user()->can($permission)) { - session()->flash('error', trans('errors.permission')); - - return redirect()->back(); - } - - return $next($request); - } -} diff --git a/database/migrations/2021_08_28_161743_add_export_role_permission.php b/database/migrations/2021_08_28_161743_add_export_role_permission.php new file mode 100644 index 000000000..57abea070 --- /dev/null +++ b/database/migrations/2021_08_28_161743_add_export_role_permission.php @@ -0,0 +1,48 @@ +get('id'); + $permissionId = DB::table('role_permissions')->insertGetId([ + 'name' => 'content-export', + 'display_name' => 'Export Content', + 'created_at' => Carbon::now()->toDateTimeString(), + 'updated_at' => Carbon::now()->toDateTimeString(), + ]); + + $permissionRoles = $roles->map(function ($role) use ($permissionId) { + return [ + 'role_id' => $role->id, + 'permission_id' => $permissionId, + ]; + })->values()->toArray(); + + DB::table('permission_role')->insert($permissionRoles); + } + + /** + * Reverse the migrations. + * + * @return void + */ + public function down() + { + // Remove content-export permission + $contentExportPermission = DB::table('role_permissions') + ->where('name', '=', 'content-export')->first(); + + DB::table('permission_role')->where('permission_id', '=', $contentExportPermission->id)->delete(); + DB::table('role_permissions')->where('id', '=', 'content-export')->delete(); + } +} diff --git a/resources/lang/en/settings.php b/resources/lang/en/settings.php index 8b4215141..4c1ae1345 100755 --- a/resources/lang/en/settings.php +++ b/resources/lang/en/settings.php @@ -148,6 +148,7 @@ return [ 'role_manage_page_templates' => 'Manage page templates', 'role_access_api' => 'Access system API', 'role_manage_settings' => 'Manage app settings', + 'role_export_content' => 'Export content', 'role_asset' => 'Asset Permissions', 'roles_system_warning' => 'Be aware that access to any of the above three permissions can allow a user to alter their own privileges or the privileges of others in the system. Only assign roles with these permissions to trusted users.', 'role_asset_desc' => 'These permissions control default access to the assets within the system. Permissions on Books, Chapters and Pages will override these permissions.', diff --git a/resources/views/books/show.blade.php b/resources/views/books/show.blade.php index cc1fceb13..25a6f69fa 100644 --- a/resources/views/books/show.blade.php +++ b/resources/views/books/show.blade.php @@ -128,7 +128,9 @@ @if(signedInUser()) @include('entities.favourite-action', ['entity' => $book]) @endif - @include('entities.export-menu', ['entity' => $book]) + @if(userCan('content-export')) + @include('entities.export-menu', ['entity' => $book]) + @endif diff --git a/resources/views/chapters/show.blade.php b/resources/views/chapters/show.blade.php index 9e78ec946..1646d4f18 100644 --- a/resources/views/chapters/show.blade.php +++ b/resources/views/chapters/show.blade.php @@ -132,7 +132,9 @@ @if(signedInUser()) @include('entities.favourite-action', ['entity' => $chapter]) @endif - @include('entities.export-menu', ['entity' => $chapter]) + @if(userCan('content-export')) + @include('entities.export-menu', ['entity' => $chapter]) + @endif @stop diff --git a/resources/views/pages/show.blade.php b/resources/views/pages/show.blade.php index ca31ee06a..0111047c6 100644 --- a/resources/views/pages/show.blade.php +++ b/resources/views/pages/show.blade.php @@ -165,7 +165,9 @@ @if(signedInUser()) @include('entities.favourite-action', ['entity' => $page]) @endif - @include('entities.export-menu', ['entity' => $page]) + @if(userCan('content-export')) + @include('entities.export-menu', ['entity' => $page]) + @endif diff --git a/resources/views/settings/roles/parts/form.blade.php b/resources/views/settings/roles/parts/form.blade.php index 2c1ed7fed..2f94398b5 100644 --- a/resources/views/settings/roles/parts/form.blade.php +++ b/resources/views/settings/roles/parts/form.blade.php @@ -41,6 +41,7 @@
@include('settings.roles.parts.checkbox', ['permission' => 'restrictions-manage-own', 'label' => trans('settings.role_manage_own_entity_permissions')])
@include('settings.roles.parts.checkbox', ['permission' => 'templates-manage', 'label' => trans('settings.role_manage_page_templates')])
@include('settings.roles.parts.checkbox', ['permission' => 'access-api', 'label' => trans('settings.role_access_api')])
+
@include('settings.roles.parts.checkbox', ['permission' => 'content-export', 'label' => trans('settings.role_export_content')])
@include('settings.roles.parts.checkbox', ['permission' => 'settings-manage', 'label' => trans('settings.role_manage_settings')])
@@ -239,7 +240,7 @@

{{ trans('settings.role_users') }}

- @if(isset($role) && count($role->users) > 0) + @if(count($role->users ?? []) > 0)
@foreach($role->users as $user)
diff --git a/tests/Api/BooksApiTest.php b/tests/Api/BooksApiTest.php index 279c7ad9a..91e2db9e5 100644 --- a/tests/Api/BooksApiTest.php +++ b/tests/Api/BooksApiTest.php @@ -155,4 +155,17 @@ class BooksApiTest extends TestCase $resp->assertSee('# ' . $book->pages()->first()->name); $resp->assertSee('# ' . $book->chapters()->first()->name); } + + public function test_cant_export_when_not_have_permission() + { + $types = ['html', 'plaintext', 'pdf', 'markdown']; + $this->actingAsApiEditor(); + $this->removePermissionFromUser($this->getEditor(), 'content-export'); + + $book = Book::visible()->first(); + foreach ($types as $type) { + $resp = $this->get($this->baseEndpoint . "/{$book->id}/export/{$type}"); + $this->assertPermissionError($resp); + } + } } diff --git a/tests/Api/ChaptersApiTest.php b/tests/Api/ChaptersApiTest.php index b3dd0ae6b..c9ed1a289 100644 --- a/tests/Api/ChaptersApiTest.php +++ b/tests/Api/ChaptersApiTest.php @@ -200,4 +200,17 @@ class ChaptersApiTest extends TestCase $resp->assertSee('# ' . $chapter->name); $resp->assertSee('# ' . $chapter->pages()->first()->name); } + + public function test_cant_export_when_not_have_permission() + { + $types = ['html', 'plaintext', 'pdf', 'markdown']; + $this->actingAsApiEditor(); + $this->removePermissionFromUser($this->getEditor(), 'content-export'); + + $chapter = Chapter::visible()->has('pages')->first(); + foreach ($types as $type) { + $resp = $this->get($this->baseEndpoint . "/{$chapter->id}/export/{$type}"); + $this->assertPermissionError($resp); + } + } } diff --git a/tests/Api/PagesApiTest.php b/tests/Api/PagesApiTest.php index eca606234..4eb109d9d 100644 --- a/tests/Api/PagesApiTest.php +++ b/tests/Api/PagesApiTest.php @@ -292,4 +292,17 @@ class PagesApiTest extends TestCase $resp->assertSee('# ' . $page->name); $resp->assertHeader('Content-Disposition', 'attachment; filename="' . $page->slug . '.md"'); } + + public function test_cant_export_when_not_have_permission() + { + $types = ['html', 'plaintext', 'pdf', 'markdown']; + $this->actingAsApiEditor(); + $this->removePermissionFromUser($this->getEditor(), 'content-export'); + + $page = Page::visible()->first(); + foreach ($types as $type) { + $resp = $this->get($this->baseEndpoint . "/{$page->id}/export/{$type}"); + $this->assertPermissionError($resp); + } + } } diff --git a/tests/Entity/ExportTest.php b/tests/Entity/ExportTest.php index 4c6fb1a74..32077aebc 100644 --- a/tests/Entity/ExportTest.php +++ b/tests/Entity/ExportTest.php @@ -2,6 +2,7 @@ namespace Tests\Entity; +use BookStack\Auth\Role; use BookStack\Entities\Models\Book; use BookStack\Entities\Models\Chapter; use BookStack\Entities\Models\Page; @@ -340,4 +341,29 @@ class ExportTest extends TestCase $resp->assertSee('# ' . $chapter->name); $resp->assertSee('# ' . $page->name); } + + public function test_export_option_only_visible_and_accessible_with_permission() + { + $book = Book::query()->whereHas('pages')->whereHas('chapters')->first(); + $chapter = $book->chapters()->first(); + $page = $chapter->pages()->first(); + $entities = [$book, $chapter, $page]; + $user = $this->getViewer(); + $this->actingAs($user); + + foreach ($entities as $entity) { + $resp = $this->get($entity->getUrl()); + $resp->assertSee("/export/pdf"); + } + + /** @var Role $role */ + $this->removePermissionFromUser($user, 'content-export'); + + foreach ($entities as $entity) { + $resp = $this->get($entity->getUrl()); + $resp->assertDontSee("/export/pdf"); + $resp = $this->get($entity->getUrl("/export/pdf")); + $this->assertPermissionError($resp); + } + } } diff --git a/tests/SharedTestHelpers.php b/tests/SharedTestHelpers.php index 0bc80924e..df6c613df 100644 --- a/tests/SharedTestHelpers.php +++ b/tests/SharedTestHelpers.php @@ -4,6 +4,7 @@ namespace Tests; use BookStack\Auth\Permissions\PermissionService; use BookStack\Auth\Permissions\PermissionsRepo; +use BookStack\Auth\Permissions\RolePermission; use BookStack\Auth\Role; use BookStack\Auth\User; use BookStack\Entities\Models\Book; @@ -18,6 +19,7 @@ use BookStack\Entities\Repos\PageRepo; use BookStack\Settings\SettingService; use BookStack\Uploads\HttpFetcher; use Illuminate\Foundation\Testing\Assert as PHPUnit; +use Illuminate\Http\JsonResponse; use Illuminate\Support\Env; use Illuminate\Support\Facades\Log; use Mockery; @@ -184,6 +186,19 @@ trait SharedTestHelpers $user->clearPermissionCache(); } + /** + * Completely remove the given permission name from the given user. + */ + protected function removePermissionFromUser(User $user, string $permission) + { + $permission = RolePermission::query()->where('name', '=', $permission)->first(); + /** @var Role $role */ + foreach ($user->roles as $role) { + $role->detachPermission($permission); + } + $user->clearPermissionCache(); + } + /** * Create a new basic role for testing purposes. */ @@ -274,8 +289,17 @@ trait SharedTestHelpers private function isPermissionError($response): bool { return $response->status() === 302 - && $response->headers->get('Location') === url('/') - && strpos(session()->pull('error', ''), 'You do not have permission to access') === 0; + && ( + ( + $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.']) + ) + ); } /** From 7d9de23a25a5a055adf79e81f67768686f3a8c94 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 28 Aug 2021 21:51:15 +0100 Subject: [PATCH 2/3] Applied styleci patches --- app/Http/Kernel.php | 2 +- app/Http/Middleware/CheckUserHasPermission.php | 2 +- .../2021_08_28_161743_add_export_role_permission.php | 8 ++++---- tests/Entity/ExportTest.php | 6 +++--- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/app/Http/Kernel.php b/app/Http/Kernel.php index 2d85c870d..1733d29b3 100644 --- a/app/Http/Kernel.php +++ b/app/Http/Kernel.php @@ -48,7 +48,7 @@ class Kernel extends HttpKernel */ protected $routeMiddleware = [ 'auth' => \BookStack\Http\Middleware\Authenticate::class, - 'can' => \BookStack\Http\Middleware\CheckUserHasPermission::class, + 'can' => \BookStack\Http\Middleware\CheckUserHasPermission::class, 'guest' => \BookStack\Http\Middleware\RedirectIfAuthenticated::class, 'throttle' => \Illuminate\Routing\Middleware\ThrottleRequests::class, 'guard' => \BookStack\Http\Middleware\CheckGuard::class, diff --git a/app/Http/Middleware/CheckUserHasPermission.php b/app/Http/Middleware/CheckUserHasPermission.php index 4340152e7..4a6a06468 100644 --- a/app/Http/Middleware/CheckUserHasPermission.php +++ b/app/Http/Middleware/CheckUserHasPermission.php @@ -25,7 +25,6 @@ class CheckUserHasPermission return $next($request); } - protected function errorResponse(Request $request) { if ($request->wantsJson()) { @@ -33,6 +32,7 @@ class CheckUserHasPermission } session()->flash('error', trans('errors.permission')); + return redirect('/'); } } diff --git a/database/migrations/2021_08_28_161743_add_export_role_permission.php b/database/migrations/2021_08_28_161743_add_export_role_permission.php index 57abea070..184c24ecf 100644 --- a/database/migrations/2021_08_28_161743_add_export_role_permission.php +++ b/database/migrations/2021_08_28_161743_add_export_role_permission.php @@ -15,15 +15,15 @@ class AddExportRolePermission extends Migration // Create new templates-manage permission and assign to admin role $roles = \Illuminate\Support\Facades\DB::table('roles')->get('id'); $permissionId = DB::table('role_permissions')->insertGetId([ - 'name' => 'content-export', + 'name' => 'content-export', 'display_name' => 'Export Content', - 'created_at' => Carbon::now()->toDateTimeString(), - 'updated_at' => Carbon::now()->toDateTimeString(), + 'created_at' => Carbon::now()->toDateTimeString(), + 'updated_at' => Carbon::now()->toDateTimeString(), ]); $permissionRoles = $roles->map(function ($role) use ($permissionId) { return [ - 'role_id' => $role->id, + 'role_id' => $role->id, 'permission_id' => $permissionId, ]; })->values()->toArray(); diff --git a/tests/Entity/ExportTest.php b/tests/Entity/ExportTest.php index 32077aebc..7031c3875 100644 --- a/tests/Entity/ExportTest.php +++ b/tests/Entity/ExportTest.php @@ -353,7 +353,7 @@ class ExportTest extends TestCase foreach ($entities as $entity) { $resp = $this->get($entity->getUrl()); - $resp->assertSee("/export/pdf"); + $resp->assertSee('/export/pdf'); } /** @var Role $role */ @@ -361,8 +361,8 @@ class ExportTest extends TestCase foreach ($entities as $entity) { $resp = $this->get($entity->getUrl()); - $resp->assertDontSee("/export/pdf"); - $resp = $this->get($entity->getUrl("/export/pdf")); + $resp->assertDontSee('/export/pdf'); + $resp = $this->get($entity->getUrl('/export/pdf')); $this->assertPermissionError($resp); } } From 0e7166f7f60a98030ca00273bbd12730893d4241 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 28 Aug 2021 21:55:04 +0100 Subject: [PATCH 3/3] Cleaned up DB usage in migration --- .../2021_08_28_161743_add_export_role_permission.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/database/migrations/2021_08_28_161743_add_export_role_permission.php b/database/migrations/2021_08_28_161743_add_export_role_permission.php index 184c24ecf..1da607655 100644 --- a/database/migrations/2021_08_28_161743_add_export_role_permission.php +++ b/database/migrations/2021_08_28_161743_add_export_role_permission.php @@ -2,6 +2,7 @@ use Carbon\Carbon; use Illuminate\Database\Migrations\Migration; +use Illuminate\Support\Facades\DB; class AddExportRolePermission extends Migration { @@ -13,7 +14,7 @@ class AddExportRolePermission extends Migration public function up() { // Create new templates-manage permission and assign to admin role - $roles = \Illuminate\Support\Facades\DB::table('roles')->get('id'); + $roles = DB::table('roles')->get('id'); $permissionId = DB::table('role_permissions')->insertGetId([ 'name' => 'content-export', 'display_name' => 'Export Content',