From c76d12d1ded72b5bf74a51fdb1647f87bb935edc Mon Sep 17 00:00:00 2001 From: "Luke T. Shumaker" Date: Fri, 15 Dec 2023 10:58:20 -0700 Subject: [PATCH 001/666] Oidc: Properly query the UserInfo Endpoint BooksStack's OIDC Client requests the 'profile' and 'email' scope values in order to have access to the 'name', 'email', and other claims. It looks for these claims in the ID Token that is returned along with the Access Token. However, the OIDC-core specification section 5.4 [1] only requires that the Provider include those claims in the ID Token *if* an Access Token is not also issued. If an Access Token is issued, the Provider can leave out those claims from the ID Token, and the Client is supposed to obtain them by submitting the Access Token to the UserInfo Endpoint. So I suppose it's just good luck that the OIDC Providers that BookStack has been tested with just so happen to also stick those claims in the ID Token even though they don't have to. But others (in particular: https://login.infomaniak.com) don't do so, and require fetching the UserInfo Endpoint.) A workaround is currently possible by having the user write a theme with a ThemeEvents::OIDC_ID_TOKEN_PRE_VALIDATE hook that fetches the UserInfo Endpoint. This workaround isn't great, for a few reasons: 1. Asking the user to implement core parts of the OIDC protocol is silly. 2. The user either needs to re-fetch the .well-known/openid-configuration file to discover the endpoint (adding yet another round-trip to each login) or hard-code the endpoint, which is fragile. 3. The hook doesn't receive the HTTP client configuration. So, have BookStack's OidcService fetch the UserInfo Endpoint and inject those claims into the ID Token, if a UserInfo Endpoint is defined. Two points about this: - Injecting them into the ID Token's claims is the most obvious approach given the current code structure; though I'm not sure it is the best approach, perhaps it should instead fetch the user info in processAuthorizationResponse() and pass that as an argument to processAccessTokenCallback() which would then need a bit of restructuring. But this made sense because it's also how the ThemeEvents::OIDC_ID_TOKEN_PRE_VALIDATE hook works. - OIDC *requires* that a UserInfo Endpoint exists, so why bother with that "if a UserInfo Endpoint is defined" bit? Simply out of an abundance of caution that there's an existing BookStack user that is relying on it not fetching the UserInfo Endpoint in order to work with a non-compliant OIDC Provider. [1]: https://openid.net/specs/openid-connect-core-1_0.html#ScopeClaims --- .env.example.complete | 1 + app/Access/Oidc/OidcProviderSettings.php | 7 ++++- app/Access/Oidc/OidcService.php | 12 ++++++++ app/Config/oidc.php | 1 + tests/Auth/OidcTest.php | 38 ++++++++++++++++++++++-- 5 files changed, 56 insertions(+), 3 deletions(-) diff --git a/.env.example.complete b/.env.example.complete index e8520a24cae..1242968182a 100644 --- a/.env.example.complete +++ b/.env.example.complete @@ -267,6 +267,7 @@ OIDC_ISSUER_DISCOVER=false OIDC_PUBLIC_KEY=null OIDC_AUTH_ENDPOINT=null OIDC_TOKEN_ENDPOINT=null +OIDC_USERINFO_ENDPOINT=null OIDC_ADDITIONAL_SCOPES=null OIDC_DUMP_USER_DETAILS=false OIDC_USER_TO_GROUPS=false diff --git a/app/Access/Oidc/OidcProviderSettings.php b/app/Access/Oidc/OidcProviderSettings.php index bea6a523e77..49ccab6f0de 100644 --- a/app/Access/Oidc/OidcProviderSettings.php +++ b/app/Access/Oidc/OidcProviderSettings.php @@ -22,6 +22,7 @@ class OidcProviderSettings public ?string $authorizationEndpoint; public ?string $tokenEndpoint; public ?string $endSessionEndpoint; + public ?string $userinfoEndpoint; /** * @var string[]|array[] @@ -128,6 +129,10 @@ protected function loadSettingsFromIssuerDiscovery(ClientInterface $httpClient): $discoveredSettings['tokenEndpoint'] = $result['token_endpoint']; } + if (!empty($result['userinfo_endpoint'])) { + $discoveredSettings['userinfoEndpoint'] = $result['userinfo_endpoint']; + } + if (!empty($result['jwks_uri'])) { $keys = $this->loadKeysFromUri($result['jwks_uri'], $httpClient); $discoveredSettings['keys'] = $this->filterKeys($keys); @@ -177,7 +182,7 @@ protected function loadKeysFromUri(string $uri, ClientInterface $httpClient): ar */ public function arrayForProvider(): array { - $settingKeys = ['clientId', 'clientSecret', 'redirectUri', 'authorizationEndpoint', 'tokenEndpoint']; + $settingKeys = ['clientId', 'clientSecret', 'redirectUri', 'authorizationEndpoint', 'tokenEndpoint', 'userinfoEndpoint']; $settings = []; foreach ($settingKeys as $setting) { $settings[$setting] = $this->$setting; diff --git a/app/Access/Oidc/OidcService.php b/app/Access/Oidc/OidcService.php index f1e5b25af14..24495799195 100644 --- a/app/Access/Oidc/OidcService.php +++ b/app/Access/Oidc/OidcService.php @@ -85,6 +85,7 @@ protected function getProviderSettings(): OidcProviderSettings 'authorizationEndpoint' => $config['authorization_endpoint'], 'tokenEndpoint' => $config['token_endpoint'], 'endSessionEndpoint' => is_string($config['end_session_endpoint']) ? $config['end_session_endpoint'] : null, + 'userinfoEndpoint' => $config['userinfo_endpoint'], ]); // Use keys if configured @@ -228,6 +229,17 @@ protected function processAccessTokenCallback(OidcAccessToken $accessToken, Oidc session()->put("oidc_id_token", $idTokenText); + if (!empty($settings->userinfoEndpoint)) { + $provider = $this->getProvider($settings); + $request = $provider->getAuthenticatedRequest('GET', $settings->userinfoEndpoint, $accessToken->getToken()); + $response = $provider->getParsedResponse($request); + $claims = $idToken->getAllClaims(); + foreach ($response as $key => $value) { + $claims[$key] = $value; + } + $idToken->replaceClaims($claims); + } + $returnClaims = Theme::dispatch(ThemeEvents::OIDC_ID_TOKEN_PRE_VALIDATE, $idToken->getAllClaims(), [ 'access_token' => $accessToken->getToken(), 'expires_in' => $accessToken->getExpires(), diff --git a/app/Config/oidc.php b/app/Config/oidc.php index 7f8f40d4190..8b5470931d0 100644 --- a/app/Config/oidc.php +++ b/app/Config/oidc.php @@ -35,6 +35,7 @@ // OAuth2 endpoints. 'authorization_endpoint' => env('OIDC_AUTH_ENDPOINT', null), 'token_endpoint' => env('OIDC_TOKEN_ENDPOINT', null), + 'userinfo_endpoint' => env('OIDC_USERINFO_ENDPOINT', null), // OIDC RP-Initiated Logout endpoint URL. // A false value force-disables RP-Initiated Logout. diff --git a/tests/Auth/OidcTest.php b/tests/Auth/OidcTest.php index dbf26f1bd30..f47a201005d 100644 --- a/tests/Auth/OidcTest.php +++ b/tests/Auth/OidcTest.php @@ -668,11 +668,44 @@ protected function withAutodiscovery() protected function runLogin($claimOverrides = []): TestResponse { + // These two variables should perhaps be arguments instead of + // assuming that they're tied to whether discovery is enabled, + // but that's how the tests are written for now. + $claimsInIdToken = !config('oidc.discover'); + $tokenEndpoint = config('oidc.discover') + ? OidcJwtHelper::defaultIssuer() . '/oidc/token' + : 'https://oidc.local/token'; + $this->post('/oidc/login'); $state = session()->get('oidc_state'); - $this->mockHttpClient([$this->getMockAuthorizationResponse($claimOverrides)]); - return $this->get('/oidc/callback?code=SplxlOBeZQQYbYS6WxSbIA&state=' . $state); + $providerResponses = [$this->getMockAuthorizationResponse($claimsInIdToken ? $claimOverrides : [])]; + if (!$claimsInIdToken) { + $providerResponses[] = new Response(200, [ + 'Content-Type' => 'application/json', + 'Cache-Control' => 'no-cache, no-store', + 'Pragma' => 'no-cache', + ], json_encode($claimOverrides)); + } + + $transactions = $this->mockHttpClient($providerResponses); + + $response = $this->get('/oidc/callback?code=SplxlOBeZQQYbYS6WxSbIA&state=' . $state); + + if (auth()->check()) { + $this->assertEquals($claimsInIdToken ? 1 : 2, $transactions->requestCount()); + $tokenRequest = $transactions->requestAt(0); + $this->assertEquals($tokenEndpoint, (string) $tokenRequest->getUri()); + $this->assertEquals('POST', $tokenRequest->getMethod()); + if (!$claimsInIdToken) { + $userinfoRequest = $transactions->requestAt(1); + $this->assertEquals(OidcJwtHelper::defaultIssuer() . '/oidc/userinfo', (string) $userinfoRequest->getUri()); + $this->assertEquals('GET', $userinfoRequest->getMethod()); + $this->assertEquals('Bearer abc123', $userinfoRequest->getHeader('Authorization')[0]); + } + } + + return $response; } protected function getAutoDiscoveryResponse($responseOverrides = []): Response @@ -684,6 +717,7 @@ protected function getAutoDiscoveryResponse($responseOverrides = []): Response ], json_encode(array_merge([ 'token_endpoint' => OidcJwtHelper::defaultIssuer() . '/oidc/token', 'authorization_endpoint' => OidcJwtHelper::defaultIssuer() . '/oidc/authorize', + 'userinfo_endpoint' => OidcJwtHelper::defaultIssuer() . '/oidc/userinfo', 'jwks_uri' => OidcJwtHelper::defaultIssuer() . '/oidc/keys', 'issuer' => OidcJwtHelper::defaultIssuer(), 'end_session_endpoint' => OidcJwtHelper::defaultIssuer() . '/oidc/logout', From 70bfebcd7c9396c9d17757ec57a584371dce2e9f Mon Sep 17 00:00:00 2001 From: Sascha Date: Mon, 1 Jan 2024 21:58:49 +0100 Subject: [PATCH 002/666] Added Default Templates for Chapters --- .../Controllers/ChapterController.php | 14 ++++---- app/Entities/Controllers/PageController.php | 9 +++-- app/Entities/Models/Chapter.php | 11 ++++++ app/Entities/Repos/ChapterRepo.php | 34 +++++++++++++++++++ app/Entities/Repos/PageRepo.php | 8 ++++- app/Entities/Tools/TrashCan.php | 6 ++++ ...04542_add_default_template_to_chapters.php | 32 +++++++++++++++++ lang/en/entities.php | 3 ++ resources/views/chapters/parts/form.blade.php | 23 +++++++++++++ 9 files changed, 131 insertions(+), 9 deletions(-) create mode 100644 database/migrations/2024_01_01_104542_add_default_template_to_chapters.php diff --git a/app/Entities/Controllers/ChapterController.php b/app/Entities/Controllers/ChapterController.php index 28ad35fa4b3..00616888a70 100644 --- a/app/Entities/Controllers/ChapterController.php +++ b/app/Entities/Controllers/ChapterController.php @@ -49,9 +49,10 @@ public function create(string $bookSlug) public function store(Request $request, string $bookSlug) { $validated = $this->validate($request, [ - 'name' => ['required', 'string', 'max:255'], - 'description_html' => ['string', 'max:2000'], - 'tags' => ['array'], + 'name' => ['required', 'string', 'max:255'], + 'description_html' => ['string', 'max:2000'], + 'tags' => ['array'], + 'default_template_id' => ['nullable', 'integer'], ]); $book = Book::visible()->where('slug', '=', $bookSlug)->firstOrFail(); @@ -111,9 +112,10 @@ public function edit(string $bookSlug, string $chapterSlug) public function update(Request $request, string $bookSlug, string $chapterSlug) { $validated = $this->validate($request, [ - 'name' => ['required', 'string', 'max:255'], - 'description_html' => ['string', 'max:2000'], - 'tags' => ['array'], + 'name' => ['required', 'string', 'max:255'], + 'description_html' => ['string', 'max:2000'], + 'tags' => ['array'], + 'default_template_id' => ['nullable', 'integer'], ]); $chapter = $this->chapterRepo->getBySlug($bookSlug, $chapterSlug); diff --git a/app/Entities/Controllers/PageController.php b/app/Entities/Controllers/PageController.php index adafcdc7bd9..74dd4f531b4 100644 --- a/app/Entities/Controllers/PageController.php +++ b/app/Entities/Controllers/PageController.php @@ -6,6 +6,7 @@ use BookStack\Activity\Tools\CommentTree; use BookStack\Activity\Tools\UserEntityWatchOptions; use BookStack\Entities\Models\Book; +use BookStack\Entities\Models\Chapter; use BookStack\Entities\Models\Page; use BookStack\Entities\Repos\PageRepo; use BookStack\Entities\Tools\BookContents; @@ -259,7 +260,9 @@ public function showDelete(string $bookSlug, string $pageSlug) $page = $this->pageRepo->getBySlug($bookSlug, $pageSlug); $this->checkOwnablePermission('page-delete', $page); $this->setPageTitle(trans('entities.pages_delete_named', ['pageName' => $page->getShortName()])); - $usedAsTemplate = Book::query()->where('default_template_id', '=', $page->id)->count() > 0; + $usedAsTemplate = + Book::query()->where('default_template_id', '=', $page->id)->count() > 0 || + Chapter::query()->where('default_template_id', '=', $page->id)->count() > 0; return view('pages.delete', [ 'book' => $page->book, @@ -279,7 +282,9 @@ public function showDeleteDraft(string $bookSlug, int $pageId) $page = $this->pageRepo->getById($pageId); $this->checkOwnablePermission('page-update', $page); $this->setPageTitle(trans('entities.pages_delete_draft_named', ['pageName' => $page->getShortName()])); - $usedAsTemplate = Book::query()->where('default_template_id', '=', $page->id)->count() > 0; + $usedAsTemplate = + Book::query()->where('default_template_id', '=', $page->id)->count() > 0 || + Chapter::query()->where('default_template_id', '=', $page->id)->count() > 0; return view('pages.delete', [ 'book' => $page->book, diff --git a/app/Entities/Models/Chapter.php b/app/Entities/Models/Chapter.php index f30d77b5c5c..d3a7101116b 100644 --- a/app/Entities/Models/Chapter.php +++ b/app/Entities/Models/Chapter.php @@ -2,6 +2,7 @@ namespace BookStack\Entities\Models; +use Illuminate\Database\Eloquent\Relations\BelongsTo; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Relations\HasMany; use Illuminate\Support\Collection; @@ -11,6 +12,8 @@ * * @property Collection $pages * @property string $description + * @property ?int $default_template_id + * @property ?Page $defaultTemplate */ class Chapter extends BookChild { @@ -48,6 +51,14 @@ public function getUrl(string $path = ''): string return url('/' . implode('/', $parts)); } + /** + * Get the Page that is used as default template for newly created pages within this Chapter. + */ + public function defaultTemplate(): BelongsTo + { + return $this->belongsTo(Page::class, 'default_template_id'); + } + /** * Get the visible pages in this chapter. */ diff --git a/app/Entities/Repos/ChapterRepo.php b/app/Entities/Repos/ChapterRepo.php index 977193d85bb..9534a406082 100644 --- a/app/Entities/Repos/ChapterRepo.php +++ b/app/Entities/Repos/ChapterRepo.php @@ -4,6 +4,7 @@ use BookStack\Activity\ActivityType; use BookStack\Entities\Models\Book; +use BookStack\Entities\Models\Page; use BookStack\Entities\Models\Chapter; use BookStack\Entities\Models\Entity; use BookStack\Entities\Tools\BookContents; @@ -46,6 +47,7 @@ public function create(array $input, Book $parentBook): Chapter $chapter->book_id = $parentBook->id; $chapter->priority = (new BookContents($parentBook))->getLastPriority() + 1; $this->baseRepo->create($chapter, $input); + $this->updateChapterDefaultTemplate($chapter, intval($input['default_template_id'] ?? null)); Activity::add(ActivityType::CHAPTER_CREATE, $chapter); return $chapter; @@ -57,6 +59,11 @@ public function create(array $input, Book $parentBook): Chapter public function update(Chapter $chapter, array $input): Chapter { $this->baseRepo->update($chapter, $input); + + if (array_key_exists('default_template_id', $input)) { + $this->updateChapterDefaultTemplate($chapter, intval($input['default_template_id'])); + } + Activity::add(ActivityType::CHAPTER_UPDATE, $chapter); return $chapter; @@ -101,6 +108,33 @@ public function move(Chapter $chapter, string $parentIdentifier): Book return $parent; } + /** + * Update the default page template used for this chapter. + * Checks that, if changing, the provided value is a valid template and the user + * has visibility of the provided page template id. + */ + protected function updateChapterDefaultTemplate(Chapter $chapter, int $templateId): void + { + $changing = $templateId !== intval($chapter->default_template_id); + if (!$changing) { + return; + } + + if ($templateId === 0) { + $chapter->default_template_id = null; + $chapter->save(); + return; + } + + $templateExists = Page::query()->visible() + ->where('template', '=', true) + ->where('id', '=', $templateId) + ->exists(); + + $chapter->default_template_id = $templateExists ? $templateId : null; + $chapter->save(); + } + /** * Find a page parent entity via an identifier string in the format: * {type}:{id} diff --git a/app/Entities/Repos/PageRepo.php b/app/Entities/Repos/PageRepo.php index 7b14ea7d278..67c4b222547 100644 --- a/app/Entities/Repos/PageRepo.php +++ b/app/Entities/Repos/PageRepo.php @@ -136,7 +136,13 @@ public function getNewDraftPage(Entity $parent) $page->book_id = $parent->id; } - $defaultTemplate = $page->book->defaultTemplate; + // check for chapter + if ($page->chapter_id) { + $defaultTemplate = $page->chapter->defaultTemplate; + } else { + $defaultTemplate = $page->book->defaultTemplate; + } + if ($defaultTemplate && userCan('view', $defaultTemplate)) { $page->forceFill([ 'html' => $defaultTemplate->html, diff --git a/app/Entities/Tools/TrashCan.php b/app/Entities/Tools/TrashCan.php index b2510398541..e5bcfe71ac2 100644 --- a/app/Entities/Tools/TrashCan.php +++ b/app/Entities/Tools/TrashCan.php @@ -208,6 +208,12 @@ protected function destroyPage(Page $page): int $page->forceDelete(); + // Remove chapter template usages + Chapter::query()->where('default_template_id', '=', $page->id) + ->update(['default_template_id' => null]); + + $page->forceDelete(); + return 1; } diff --git a/database/migrations/2024_01_01_104542_add_default_template_to_chapters.php b/database/migrations/2024_01_01_104542_add_default_template_to_chapters.php new file mode 100644 index 00000000000..5e9ea1de311 --- /dev/null +++ b/database/migrations/2024_01_01_104542_add_default_template_to_chapters.php @@ -0,0 +1,32 @@ +integer('default_template_id')->nullable()->default(null); + }); + } + + /** + * Reverse the migrations. + * + * @return void + */ + public function down() + { + Schema::table('chapters', function (Blueprint $table) { + $table->dropColumn('default_template_id'); + }); + } +} diff --git a/lang/en/entities.php b/lang/en/entities.php index f1f915544d1..4ab9de47dfb 100644 --- a/lang/en/entities.php +++ b/lang/en/entities.php @@ -192,6 +192,9 @@ 'chapters_permissions_success' => 'Chapter Permissions Updated', 'chapters_search_this' => 'Search this chapter', 'chapter_sort_book' => 'Sort Book', + 'chapter_default_template' => 'Default Page Template', + 'chapter_default_template_explain' => 'Assign a page template that will be used as the default content for all new pages in this chapter. Keep in mind this will only be used if the page creator has view access to those chosen template page.', + 'chapter_default_template_select' => 'Select a template page', // Pages 'page' => 'Page', diff --git a/resources/views/chapters/parts/form.blade.php b/resources/views/chapters/parts/form.blade.php index c6052c93af4..ea7f84bc839 100644 --- a/resources/views/chapters/parts/form.blade.php +++ b/resources/views/chapters/parts/form.blade.php @@ -22,6 +22,29 @@ +
+ +
+
+

+ {{ trans('entities.chapter_default_template_explain') }} +

+ +
+ @include('form.page-picker', [ + 'name' => 'default_template_id', + 'placeholder' => trans('entities.chapter_default_template_select'), + 'value' => $chapter->default_template_id ?? null, + 'selectorEndpoint' => '/search/entity-selector-templates', + ]) +
+
+ +
+
+
{{ trans('common.cancel') }} From b4d9029dc301f73f0e87026b8eff78cf2cda6164 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sun, 7 Jan 2024 14:03:13 +0000 Subject: [PATCH 003/666] Range requests: Extracted stream output handling to new class --- app/Http/DownloadResponseFactory.php | 36 ++++-------- app/Http/RangeSupportedStream.php | 57 +++++++++++++++++++ app/Uploads/AttachmentService.php | 10 +++- .../Controllers/AttachmentController.php | 5 +- 4 files changed, 80 insertions(+), 28 deletions(-) create mode 100644 app/Http/RangeSupportedStream.php diff --git a/app/Http/DownloadResponseFactory.php b/app/Http/DownloadResponseFactory.php index 20032f52534..f8c10165c80 100644 --- a/app/Http/DownloadResponseFactory.php +++ b/app/Http/DownloadResponseFactory.php @@ -9,11 +9,9 @@ class DownloadResponseFactory { - protected Request $request; - - public function __construct(Request $request) - { - $this->request = $request; + public function __construct( + protected Request $request + ) { } /** @@ -27,19 +25,11 @@ public function directly(string $content, string $fileName): Response /** * Create a response that forces a download, from a given stream of content. */ - public function streamedDirectly($stream, string $fileName): StreamedResponse + public function streamedDirectly($stream, string $fileName, int $fileSize): StreamedResponse { - return response()->stream(function () use ($stream) { - - // End & flush the output buffer, if we're in one, otherwise we still use memory. - // Output buffer may or may not exist depending on PHP `output_buffering` setting. - // Ignore in testing since output buffers are used to gather a response. - if (!empty(ob_get_status()) && !app()->runningUnitTests()) { - ob_end_clean(); - } - - fpassthru($stream); - fclose($stream); + $rangeStream = new RangeSupportedStream($stream, $fileSize, $this->request->headers); + return response()->stream(function () use ($rangeStream) { + $rangeStream->outputAndClose(); }, 200, $this->getHeaders($fileName)); } @@ -48,15 +38,13 @@ public function streamedDirectly($stream, string $fileName): StreamedResponse * correct for the file, in a way so the browser can show the content in browser, * for a given content stream. */ - public function streamedInline($stream, string $fileName): StreamedResponse + public function streamedInline($stream, string $fileName, int $fileSize): StreamedResponse { - $sniffContent = fread($stream, 2000); - $mime = (new WebSafeMimeSniffer())->sniff($sniffContent); + $rangeStream = new RangeSupportedStream($stream, $fileSize, $this->request->headers); + $mime = $rangeStream->sniffMime(); - return response()->stream(function () use ($sniffContent, $stream) { - echo $sniffContent; - fpassthru($stream); - fclose($stream); + return response()->stream(function () use ($rangeStream) { + $rangeStream->outputAndClose(); }, 200, $this->getHeaders($fileName, $mime)); } diff --git a/app/Http/RangeSupportedStream.php b/app/Http/RangeSupportedStream.php new file mode 100644 index 00000000000..dc310503567 --- /dev/null +++ b/app/Http/RangeSupportedStream.php @@ -0,0 +1,57 @@ +fileSize); + $this->sniffContent = fread($this->stream, $offset); + + return (new WebSafeMimeSniffer())->sniff($this->sniffContent); + } + + /** + * Output the current stream to stdout before closing out the stream. + */ + public function outputAndClose(): void + { + // End & flush the output buffer, if we're in one, otherwise we still use memory. + // Output buffer may or may not exist depending on PHP `output_buffering` setting. + // Ignore in testing since output buffers are used to gather a response. + if (!empty(ob_get_status()) && !app()->runningUnitTests()) { + ob_end_clean(); + } + + $outStream = fopen('php://output', 'w'); + $offset = 0; + + if (!empty($this->sniffContent)) { + fwrite($outStream, $this->sniffContent); + $offset = strlen($this->sniffContent); + } + + $toWrite = $this->fileSize - $offset; + stream_copy_to_stream($this->stream, $outStream, $toWrite); + fpassthru($this->stream); + + fclose($this->stream); + fclose($outStream); + } +} diff --git a/app/Uploads/AttachmentService.php b/app/Uploads/AttachmentService.php index ddabec09f27..72f78e347bf 100644 --- a/app/Uploads/AttachmentService.php +++ b/app/Uploads/AttachmentService.php @@ -66,8 +66,6 @@ protected function adjustPathForStorageDisk(string $path): string /** * Stream an attachment from storage. * - * @throws FileNotFoundException - * * @return resource|null */ public function streamAttachmentFromStorage(Attachment $attachment) @@ -75,6 +73,14 @@ public function streamAttachmentFromStorage(Attachment $attachment) return $this->getStorageDisk()->readStream($this->adjustPathForStorageDisk($attachment->path)); } + /** + * Read the file size of an attachment from storage, in bytes. + */ + public function getAttachmentFileSize(Attachment $attachment): int + { + return $this->getStorageDisk()->size($this->adjustPathForStorageDisk($attachment->path)); + } + /** * Store a new attachment upon user upload. * diff --git a/app/Uploads/Controllers/AttachmentController.php b/app/Uploads/Controllers/AttachmentController.php index 92f23465d22..e61c1033884 100644 --- a/app/Uploads/Controllers/AttachmentController.php +++ b/app/Uploads/Controllers/AttachmentController.php @@ -226,12 +226,13 @@ public function get(Request $request, string $attachmentId) $fileName = $attachment->getFileName(); $attachmentStream = $this->attachmentService->streamAttachmentFromStorage($attachment); + $attachmentSize = $this->attachmentService->getAttachmentFileSize($attachment); if ($request->get('open') === 'true') { - return $this->download()->streamedInline($attachmentStream, $fileName); + return $this->download()->streamedInline($attachmentStream, $fileName, $attachmentSize); } - return $this->download()->streamedDirectly($attachmentStream, $fileName); + return $this->download()->streamedDirectly($attachmentStream, $fileName, $attachmentSize); } /** From d94762549a3ef364d4486a27f1585122f60c10ec Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sun, 7 Jan 2024 20:34:03 +0000 Subject: [PATCH 004/666] Range requests: Added basic HTTP range support --- app/Http/DownloadResponseFactory.php | 28 ++++---- app/Http/RangeSupportedStream.php | 95 +++++++++++++++++++++++++--- 2 files changed, 103 insertions(+), 20 deletions(-) diff --git a/app/Http/DownloadResponseFactory.php b/app/Http/DownloadResponseFactory.php index f8c10165c80..f29aaa2e45a 100644 --- a/app/Http/DownloadResponseFactory.php +++ b/app/Http/DownloadResponseFactory.php @@ -2,7 +2,6 @@ namespace BookStack\Http; -use BookStack\Util\WebSafeMimeSniffer; use Illuminate\Http\Request; use Illuminate\Http\Response; use Symfony\Component\HttpFoundation\StreamedResponse; @@ -19,7 +18,7 @@ public function __construct( */ public function directly(string $content, string $fileName): Response { - return response()->make($content, 200, $this->getHeaders($fileName)); + return response()->make($content, 200, $this->getHeaders($fileName, strlen($content))); } /** @@ -27,10 +26,13 @@ public function directly(string $content, string $fileName): Response */ public function streamedDirectly($stream, string $fileName, int $fileSize): StreamedResponse { - $rangeStream = new RangeSupportedStream($stream, $fileSize, $this->request->headers); - return response()->stream(function () use ($rangeStream) { - $rangeStream->outputAndClose(); - }, 200, $this->getHeaders($fileName)); + $rangeStream = new RangeSupportedStream($stream, $fileSize, $this->request); + $headers = array_merge($this->getHeaders($fileName, $fileSize), $rangeStream->getResponseHeaders()); + return response()->stream( + fn() => $rangeStream->outputAndClose(), + $rangeStream->getResponseStatus(), + $headers, + ); } /** @@ -40,24 +42,28 @@ public function streamedDirectly($stream, string $fileName, int $fileSize): Stre */ public function streamedInline($stream, string $fileName, int $fileSize): StreamedResponse { - $rangeStream = new RangeSupportedStream($stream, $fileSize, $this->request->headers); + $rangeStream = new RangeSupportedStream($stream, $fileSize, $this->request); $mime = $rangeStream->sniffMime(); + $headers = array_merge($this->getHeaders($fileName, $fileSize, $mime), $rangeStream->getResponseHeaders()); - return response()->stream(function () use ($rangeStream) { - $rangeStream->outputAndClose(); - }, 200, $this->getHeaders($fileName, $mime)); + return response()->stream( + fn() => $rangeStream->outputAndClose(), + $rangeStream->getResponseStatus(), + $headers, + ); } /** * Get the common headers to provide for a download response. */ - protected function getHeaders(string $fileName, string $mime = 'application/octet-stream'): array + protected function getHeaders(string $fileName, int $fileSize, string $mime = 'application/octet-stream'): array { $disposition = ($mime === 'application/octet-stream') ? 'attachment' : 'inline'; $downloadName = str_replace('"', '', $fileName); return [ 'Content-Type' => $mime, + 'Content-Length' => $fileSize, 'Content-Disposition' => "{$disposition}; filename=\"{$downloadName}\"", 'X-Content-Type-Options' => 'nosniff', ]; diff --git a/app/Http/RangeSupportedStream.php b/app/Http/RangeSupportedStream.php index dc310503567..b51d4a5498f 100644 --- a/app/Http/RangeSupportedStream.php +++ b/app/Http/RangeSupportedStream.php @@ -3,17 +3,30 @@ namespace BookStack\Http; use BookStack\Util\WebSafeMimeSniffer; -use Symfony\Component\HttpFoundation\HeaderBag; +use Illuminate\Http\Request; +/** + * Helper wrapper for range-based stream response handling. + * Much of this used symfony/http-foundation as a reference during build. + * URL: https://github.com/symfony/http-foundation/blob/v6.0.20/BinaryFileResponse.php + * License: MIT license, Copyright (c) Fabien Potencier. + */ class RangeSupportedStream { protected string $sniffContent; + protected array $responseHeaders; + protected int $responseStatus = 200; + + protected int $responseLength = 0; + protected int $responseOffset = 0; public function __construct( protected $stream, protected int $fileSize, - protected HeaderBag $requestHeaders, + Request $request, ) { + $this->responseLength = $this->fileSize; + $this->parseRequest($request); } /** @@ -40,18 +53,82 @@ public function outputAndClose(): void } $outStream = fopen('php://output', 'w'); - $offset = 0; + $sniffOffset = strlen($this->sniffContent); - if (!empty($this->sniffContent)) { - fwrite($outStream, $this->sniffContent); - $offset = strlen($this->sniffContent); + if (!empty($this->sniffContent) && $this->responseOffset < $sniffOffset) { + $sniffOutput = substr($this->sniffContent, $this->responseOffset, min($sniffOffset, $this->responseLength)); + fwrite($outStream, $sniffOutput); + } else if ($this->responseOffset !== 0) { + fseek($this->stream, $this->responseOffset); } - $toWrite = $this->fileSize - $offset; - stream_copy_to_stream($this->stream, $outStream, $toWrite); - fpassthru($this->stream); + stream_copy_to_stream($this->stream, $outStream, $this->responseLength); fclose($this->stream); fclose($outStream); } + + public function getResponseHeaders(): array + { + return $this->responseHeaders; + } + + public function getResponseStatus(): int + { + return $this->responseStatus; + } + + protected function parseRequest(Request $request): void + { + $this->responseHeaders['Accept-Ranges'] = $request->isMethodSafe() ? 'bytes' : 'none'; + + $range = $this->getRangeFromRequest($request); + if ($range) { + [$start, $end] = $range; + if ($start < 0 || $start > $end) { + $this->responseStatus = 416; + $this->responseHeaders['Content-Range'] = sprintf('bytes */%s', $this->fileSize); + } elseif ($end - $start < $this->fileSize - 1) { + $this->responseLength = $end < $this->fileSize ? $end - $start + 1 : -1; + $this->responseOffset = $start; + $this->responseStatus = 206; + $this->responseHeaders['Content-Range'] = sprintf('bytes %s-%s/%s', $start, $end, $this->fileSize); + $this->responseHeaders['Content-Length'] = $end - $start + 1; + } + } + + if ($request->isMethod('HEAD')) { + $this->responseLength = 0; + } + } + + protected function getRangeFromRequest(Request $request): ?array + { + $range = $request->headers->get('Range'); + if (!$range || !$request->isMethod('GET') || !str_starts_with($range, 'bytes=')) { + return null; + } + + if ($request->headers->has('If-Range')) { + return null; + } + + [$start, $end] = explode('-', substr($range, 6), 2) + [0]; + + $end = ('' === $end) ? $this->fileSize - 1 : (int) $end; + + if ('' === $start) { + $start = $this->fileSize - $end; + $end = $this->fileSize - 1; + } else { + $start = (int) $start; + } + + if ($start > $end) { + return null; + } + + $end = min($end, $this->fileSize - 1); + return [$start, $end]; + } } From 91d8d6eaaa5ae42fc9872901d6fcbcae8e19236e Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sun, 14 Jan 2024 15:50:00 +0000 Subject: [PATCH 005/666] Range requests: Added test cases to cover functionality Fixed some found issues in the process. --- app/Http/RangeSupportedStream.php | 20 +++--- tests/Uploads/AttachmentTest.php | 101 ++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+), 10 deletions(-) diff --git a/app/Http/RangeSupportedStream.php b/app/Http/RangeSupportedStream.php index b51d4a5498f..300f4488cf4 100644 --- a/app/Http/RangeSupportedStream.php +++ b/app/Http/RangeSupportedStream.php @@ -13,8 +13,8 @@ */ class RangeSupportedStream { - protected string $sniffContent; - protected array $responseHeaders; + protected string $sniffContent = ''; + protected array $responseHeaders = []; protected int $responseStatus = 200; protected int $responseLength = 0; @@ -53,16 +53,20 @@ public function outputAndClose(): void } $outStream = fopen('php://output', 'w'); - $sniffOffset = strlen($this->sniffContent); + $sniffLength = strlen($this->sniffContent); + $bytesToWrite = $this->responseLength; - if (!empty($this->sniffContent) && $this->responseOffset < $sniffOffset) { - $sniffOutput = substr($this->sniffContent, $this->responseOffset, min($sniffOffset, $this->responseLength)); + if ($sniffLength > 0 && $this->responseOffset < $sniffLength) { + $sniffEnd = min($sniffLength, $bytesToWrite + $this->responseOffset); + $sniffOutLength = $sniffEnd - $this->responseOffset; + $sniffOutput = substr($this->sniffContent, $this->responseOffset, $sniffOutLength); fwrite($outStream, $sniffOutput); + $bytesToWrite -= $sniffOutLength; } else if ($this->responseOffset !== 0) { fseek($this->stream, $this->responseOffset); } - stream_copy_to_stream($this->stream, $outStream, $this->responseLength); + stream_copy_to_stream($this->stream, $outStream, $bytesToWrite); fclose($this->stream); fclose($outStream); @@ -124,10 +128,6 @@ protected function getRangeFromRequest(Request $request): ?array $start = (int) $start; } - if ($start > $end) { - return null; - } - $end = min($end, $this->fileSize - 1); return [$start, $end]; } diff --git a/tests/Uploads/AttachmentTest.php b/tests/Uploads/AttachmentTest.php index bd03c339c43..2e1a7b3395a 100644 --- a/tests/Uploads/AttachmentTest.php +++ b/tests/Uploads/AttachmentTest.php @@ -316,4 +316,105 @@ public function test_file_upload_works_when_local_secure_restricted_is_in_use() $this->assertFileExists(storage_path($attachment->path)); $this->files->deleteAllAttachmentFiles(); } + + public function test_file_get_range_access() + { + $page = $this->entities->page(); + $this->asAdmin(); + $attachment = $this->files->uploadAttachmentDataToPage($this, $page, 'my_text.txt', 'abc123456', 'text/plain'); + + // Download access + $resp = $this->get($attachment->getUrl(), ['Range' => 'bytes=3-5']); + $resp->assertStatus(206); + $resp->assertStreamedContent('123'); + $resp->assertHeader('Content-Length', '3'); + $resp->assertHeader('Content-Range', 'bytes 3-5/9'); + + // Inline access + $resp = $this->get($attachment->getUrl(true), ['Range' => 'bytes=5-7']); + $resp->assertStatus(206); + $resp->assertStreamedContent('345'); + $resp->assertHeader('Content-Length', '3'); + $resp->assertHeader('Content-Range', 'bytes 5-7/9'); + + $this->files->deleteAllAttachmentFiles(); + } + + public function test_file_head_range_returns_no_content() + { + $page = $this->entities->page(); + $this->asAdmin(); + $attachment = $this->files->uploadAttachmentDataToPage($this, $page, 'my_text.txt', 'abc123456', 'text/plain'); + + $resp = $this->head($attachment->getUrl(), ['Range' => 'bytes=0-9']); + $resp->assertStreamedContent(''); + $resp->assertHeader('Content-Length', '9'); + $resp->assertStatus(200); + + $this->files->deleteAllAttachmentFiles(); + } + + public function test_file_head_range_edge_cases() + { + $page = $this->entities->page(); + $this->asAdmin(); + + // Mime-type "sniffing" happens on first 2k bytes, hence this content (2005 bytes) + $content = '01234' . str_repeat('a', 1990) . '0123456789'; + $attachment = $this->files->uploadAttachmentDataToPage($this, $page, 'my_text.txt', $content, 'text/plain'); + + // Test for both inline and download attachment serving + foreach ([true, false] as $isInline) { + // No end range + $resp = $this->get($attachment->getUrl($isInline), ['Range' => 'bytes=5-']); + $resp->assertStreamedContent(substr($content, 5)); + $resp->assertHeader('Content-Length', '2000'); + $resp->assertHeader('Content-Range', 'bytes 5-2004/2005'); + $resp->assertStatus(206); + + // End only range + $resp = $this->get($attachment->getUrl($isInline), ['Range' => 'bytes=-10']); + $resp->assertStreamedContent('0123456789'); + $resp->assertHeader('Content-Length', '10'); + $resp->assertHeader('Content-Range', 'bytes 1995-2004/2005'); + $resp->assertStatus(206); + + // Range across sniff point + $resp = $this->get($attachment->getUrl($isInline), ['Range' => 'bytes=1997-2002']); + $resp->assertStreamedContent('234567'); + $resp->assertHeader('Content-Length', '6'); + $resp->assertHeader('Content-Range', 'bytes 1997-2002/2005'); + $resp->assertStatus(206); + + // Range up to sniff point + $resp = $this->get($attachment->getUrl($isInline), ['Range' => 'bytes=0-1997']); + $resp->assertHeader('Content-Length', '1998'); + $resp->assertHeader('Content-Range', 'bytes 0-1997/2005'); + $resp->assertStreamedContent(substr($content, 0, 1998)); + $resp->assertStatus(206); + + // Range beyond sniff point + $resp = $this->get($attachment->getUrl($isInline), ['Range' => 'bytes=2001-2003']); + $resp->assertStreamedContent('678'); + $resp->assertHeader('Content-Length', '3'); + $resp->assertHeader('Content-Range', 'bytes 2001-2003/2005'); + $resp->assertStatus(206); + + // Range beyond content + $resp = $this->get($attachment->getUrl($isInline), ['Range' => 'bytes=0-2010']); + $resp->assertStreamedContent($content); + $resp->assertHeader('Content-Length', '2005'); + $resp->assertHeaderMissing('Content-Range'); + $resp->assertStatus(200); + + // Range start before end + $resp = $this->get($attachment->getUrl($isInline), ['Range' => 'bytes=50-10']); + $resp->assertStreamedContent($content); + $resp->assertHeader('Content-Length', '2005'); + $resp->assertHeader('Content-Range', 'bytes */2005'); + $resp->assertStatus(416); + } + + $this->files->deleteAllAttachmentFiles(); + } } From c1552fb799cda7fbb6de27ebcb01aa2580fd5330 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Mon, 15 Jan 2024 11:50:05 +0000 Subject: [PATCH 006/666] Attachments: Drag and drop video support Supports dragging and dropping video attahchments to embed them in the editor as HTML video tags. --- app/Uploads/Attachment.php | 19 +++++++++++++++++-- .../views/attachments/manager-list.blade.php | 2 +- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/app/Uploads/Attachment.php b/app/Uploads/Attachment.php index 4fd6d4cdcb7..57d7cb3346c 100644 --- a/app/Uploads/Attachment.php +++ b/app/Uploads/Attachment.php @@ -77,7 +77,22 @@ public function getUrl($openInline = false): string } /** - * Generate a HTML link to this attachment. + * Get the representation of this attachment in a format suitable for the page editors. + * Detects and adapts video content to use an inline video embed. + */ + public function editorContent(): array + { + $videoExtensions = ['mp4', 'webm', 'mkv', 'ogg', 'avi']; + if (in_array(strtolower($this->extension), $videoExtensions)) { + $html = ''; + return ['text/html' => $html, 'text/plain' => $html]; + } + + return ['text/html' => $this->htmlLink(), 'text/plain' => $this->markdownLink()]; + } + + /** + * Generate the HTML link to this attachment. */ public function htmlLink(): string { @@ -85,7 +100,7 @@ public function htmlLink(): string } /** - * Generate a markdown link to this attachment. + * Generate a MarkDown link to this attachment. */ public function markdownLink(): string { diff --git a/resources/views/attachments/manager-list.blade.php b/resources/views/attachments/manager-list.blade.php index 342b46dcad7..0e841a042f7 100644 --- a/resources/views/attachments/manager-list.blade.php +++ b/resources/views/attachments/manager-list.blade.php @@ -4,7 +4,7 @@
@icon('grip')
From 2dc454d206b518804aecd0551a71d338cf889c08 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Mon, 15 Jan 2024 13:36:04 +0000 Subject: [PATCH 007/666] Uploads: Explicitly disabled s3 streaming in config This was the default option anyway, just adding here for better visibility of this being set. Can't enable without issues as the app will attempt to seek which does not work for these streams. Also have not tested on non-s3, s3-like systems. --- app/Config/filesystems.php | 1 + 1 file changed, 1 insertion(+) diff --git a/app/Config/filesystems.php b/app/Config/filesystems.php index e6ae0fed364..1319c8886f6 100644 --- a/app/Config/filesystems.php +++ b/app/Config/filesystems.php @@ -58,6 +58,7 @@ 'endpoint' => env('STORAGE_S3_ENDPOINT', null), 'use_path_style_endpoint' => env('STORAGE_S3_ENDPOINT', null) !== null, 'throw' => true, + 'stream_reads' => false, ], ], From 655ae5ecaeba3eaea73bd20c970387f1fa993e6a Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Tue, 23 Jan 2024 12:31:44 +0000 Subject: [PATCH 008/666] Text: Tweaks to EN text for consistency/readability As suggested by Tim in discord chat. --- lang/en/activities.php | 6 +++--- lang/en/settings.php | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lang/en/activities.php b/lang/en/activities.php index d5b55c03dcb..092398ef0e1 100644 --- a/lang/en/activities.php +++ b/lang/en/activities.php @@ -93,11 +93,11 @@ 'user_delete_notification' => 'User successfully removed', // API Tokens - 'api_token_create' => 'created api token', + 'api_token_create' => 'created API token', 'api_token_create_notification' => 'API token successfully created', - 'api_token_update' => 'updated api token', + 'api_token_update' => 'updated API token', 'api_token_update_notification' => 'API token successfully updated', - 'api_token_delete' => 'deleted api token', + 'api_token_delete' => 'deleted API token', 'api_token_delete_notification' => 'API token successfully deleted', // Roles diff --git a/lang/en/settings.php b/lang/en/settings.php index 03e9bf462e2..7b7f5d2a2e3 100644 --- a/lang/en/settings.php +++ b/lang/en/settings.php @@ -109,7 +109,7 @@ 'recycle_bin_contents_empty' => 'The recycle bin is currently empty', 'recycle_bin_empty' => 'Empty Recycle Bin', 'recycle_bin_empty_confirm' => 'This will permanently destroy all items in the recycle bin including content contained within each item. Are you sure you want to empty the recycle bin?', - 'recycle_bin_destroy_confirm' => 'This action will permanently delete this item, along with any child elements listed below, from the system and you will not be able to restore this content. Are you sure you want to permanently delete this item?', + 'recycle_bin_destroy_confirm' => 'This action will permanently delete this item from the system, along with any child elements listed below, and you will not be able to restore this content. Are you sure you want to permanently delete this item?', 'recycle_bin_destroy_list' => 'Items to be Destroyed', 'recycle_bin_restore_list' => 'Items to be Restored', 'recycle_bin_restore_confirm' => 'This action will restore the deleted item, including any child elements, to their original location. If the original location has since been deleted, and is now in the recycle bin, the parent item will also need to be restored.', From 8c6b1164724fffd1766792fb3d6fef4972ba1b9e Mon Sep 17 00:00:00 2001 From: Sascha Date: Tue, 23 Jan 2024 21:37:00 +0100 Subject: [PATCH 009/666] Update TrashCan.php remove duplicate call of $page->forceDelete(); --- app/Entities/Tools/TrashCan.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/Entities/Tools/TrashCan.php b/app/Entities/Tools/TrashCan.php index e5bcfe71ac2..8e9f010df0f 100644 --- a/app/Entities/Tools/TrashCan.php +++ b/app/Entities/Tools/TrashCan.php @@ -206,8 +206,6 @@ protected function destroyPage(Page $page): int Book::query()->where('default_template_id', '=', $page->id) ->update(['default_template_id' => null]); - $page->forceDelete(); - // Remove chapter template usages Chapter::query()->where('default_template_id', '=', $page->id) ->update(['default_template_id' => null]); From 0fc02a2532fd33c443838de77f5700df220b79c9 Mon Sep 17 00:00:00 2001 From: Sascha Date: Tue, 23 Jan 2024 22:37:15 +0100 Subject: [PATCH 010/666] fixed error from phpcs --- app/Entities/Controllers/PageController.php | 4 ++-- app/Entities/Repos/PageRepo.php | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/Entities/Controllers/PageController.php b/app/Entities/Controllers/PageController.php index 74dd4f531b4..eaad3c0b79d 100644 --- a/app/Entities/Controllers/PageController.php +++ b/app/Entities/Controllers/PageController.php @@ -260,7 +260,7 @@ public function showDelete(string $bookSlug, string $pageSlug) $page = $this->pageRepo->getBySlug($bookSlug, $pageSlug); $this->checkOwnablePermission('page-delete', $page); $this->setPageTitle(trans('entities.pages_delete_named', ['pageName' => $page->getShortName()])); - $usedAsTemplate = + $usedAsTemplate = Book::query()->where('default_template_id', '=', $page->id)->count() > 0 || Chapter::query()->where('default_template_id', '=', $page->id)->count() > 0; @@ -282,7 +282,7 @@ public function showDeleteDraft(string $bookSlug, int $pageId) $page = $this->pageRepo->getById($pageId); $this->checkOwnablePermission('page-update', $page); $this->setPageTitle(trans('entities.pages_delete_draft_named', ['pageName' => $page->getShortName()])); - $usedAsTemplate = + $usedAsTemplate = Book::query()->where('default_template_id', '=', $page->id)->count() > 0 || Chapter::query()->where('default_template_id', '=', $page->id)->count() > 0; diff --git a/app/Entities/Repos/PageRepo.php b/app/Entities/Repos/PageRepo.php index 67c4b222547..d9bda019892 100644 --- a/app/Entities/Repos/PageRepo.php +++ b/app/Entities/Repos/PageRepo.php @@ -142,7 +142,7 @@ public function getNewDraftPage(Entity $parent) } else { $defaultTemplate = $page->book->defaultTemplate; } - + if ($defaultTemplate && userCan('view', $defaultTemplate)) { $page->forceFill([ 'html' => $defaultTemplate->html, From 3e9e196cdada5a6c515d5bbab971c80a90d333ab Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Thu, 25 Jan 2024 14:24:46 +0000 Subject: [PATCH 011/666] OIDC: Added PKCE functionality Related to #4734. Uses core logic from League AbstractProvider. --- app/Access/Oidc/OidcOAuthProvider.php | 31 +++++++++++---------------- app/Access/Oidc/OidcService.php | 12 ++++++++++- 2 files changed, 23 insertions(+), 20 deletions(-) diff --git a/app/Access/Oidc/OidcOAuthProvider.php b/app/Access/Oidc/OidcOAuthProvider.php index d2dc829b729..371bfcecb54 100644 --- a/app/Access/Oidc/OidcOAuthProvider.php +++ b/app/Access/Oidc/OidcOAuthProvider.php @@ -83,15 +83,9 @@ protected function getScopeSeparator(): string /** * Checks a provider response for errors. - * - * @param ResponseInterface $response - * @param array|string $data Parsed response data - * * @throws IdentityProviderException - * - * @return void */ - protected function checkResponse(ResponseInterface $response, $data) + protected function checkResponse(ResponseInterface $response, $data): void { if ($response->getStatusCode() >= 400 || isset($data['error'])) { throw new IdentityProviderException( @@ -105,13 +99,8 @@ protected function checkResponse(ResponseInterface $response, $data) /** * Generates a resource owner object from a successful resource owner * details request. - * - * @param array $response - * @param AccessToken $token - * - * @return ResourceOwnerInterface */ - protected function createResourceOwner(array $response, AccessToken $token) + protected function createResourceOwner(array $response, AccessToken $token): ResourceOwnerInterface { return new GenericResourceOwner($response, ''); } @@ -121,14 +110,18 @@ protected function createResourceOwner(array $response, AccessToken $token) * * The grant that was used to fetch the response can be used to provide * additional context. - * - * @param array $response - * @param AbstractGrant $grant - * - * @return OidcAccessToken */ - protected function createAccessToken(array $response, AbstractGrant $grant) + protected function createAccessToken(array $response, AbstractGrant $grant): OidcAccessToken { return new OidcAccessToken($response); } + + /** + * Get the method used for PKCE code verifier hashing, which is passed + * in the "code_challenge_method" parameter in the authorization request. + */ + protected function getPkceMethod(): string + { + return static::PKCE_METHOD_S256; + } } diff --git a/app/Access/Oidc/OidcService.php b/app/Access/Oidc/OidcService.php index f1e5b25af14..036c9fc47ef 100644 --- a/app/Access/Oidc/OidcService.php +++ b/app/Access/Oidc/OidcService.php @@ -33,6 +33,8 @@ public function __construct( /** * Initiate an authorization flow. + * Provides back an authorize redirect URL, in addition to other + * details which may be required for the auth flow. * * @throws OidcException * @@ -42,8 +44,12 @@ public function login(): array { $settings = $this->getProviderSettings(); $provider = $this->getProvider($settings); + + $url = $provider->getAuthorizationUrl(); + session()->put('oidc_pkce_code', $provider->getPkceCode() ?? ''); + return [ - 'url' => $provider->getAuthorizationUrl(), + 'url' => $url, 'state' => $provider->getState(), ]; } @@ -63,6 +69,10 @@ public function processAuthorizeResponse(?string $authorizationCode): User $settings = $this->getProviderSettings(); $provider = $this->getProvider($settings); + // Set PKCE code flashed at login + $pkceCode = session()->pull('oidc_pkce_code', ''); + $provider->setPkceCode($pkceCode); + // Try to exchange authorization code for access token $accessToken = $provider->getAccessToken('authorization_code', [ 'code' => $authorizationCode, From 1dc094ffafc216e15c8355b400b21524137de5e4 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 27 Jan 2024 16:41:15 +0000 Subject: [PATCH 012/666] OIDC: Added testing of PKCE flow Also compared full flow to RFC spec during this process --- tests/Auth/OidcTest.php | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/Auth/OidcTest.php b/tests/Auth/OidcTest.php index dbf26f1bd30..345d1dc780b 100644 --- a/tests/Auth/OidcTest.php +++ b/tests/Auth/OidcTest.php @@ -655,6 +655,34 @@ public function test_oidc_id_token_pre_validate_theme_event_with_return() ]); } + public function test_pkce_used_on_authorize_and_access() + { + // Start auth + $resp = $this->post('/oidc/login'); + $state = session()->get('oidc_state'); + + $pkceCode = session()->get('oidc_pkce_code'); + $this->assertGreaterThan(30, strlen($pkceCode)); + + $expectedCodeChallenge = trim(strtr(base64_encode(hash('sha256', $pkceCode, true)), '+/', '-_'), '='); + $redirect = $resp->headers->get('Location'); + $redirectParams = []; + parse_str(parse_url($redirect, PHP_URL_QUERY), $redirectParams); + $this->assertEquals($expectedCodeChallenge, $redirectParams['code_challenge']); + $this->assertEquals('S256', $redirectParams['code_challenge_method']); + + $transactions = $this->mockHttpClient([$this->getMockAuthorizationResponse([ + 'email' => 'benny@example.com', + 'sub' => 'benny1010101', + ])]); + + $this->get('/oidc/callback?code=SplxlOBeZQQYbYS6WxSbIA&state=' . $state); + $tokenRequest = $transactions->latestRequest(); + $bodyParams = []; + parse_str($tokenRequest->getBody(), $bodyParams); + $this->assertEquals($pkceCode, $bodyParams['code_verifier']); + } + protected function withAutodiscovery() { config()->set([ From 2a849894bee6ac7846261938c9606fb18a4a1761 Mon Sep 17 00:00:00 2001 From: Sascha Date: Mon, 29 Jan 2024 19:37:59 +0100 Subject: [PATCH 013/666] Update entities.php changed text of `pages_delete_warning_template` to include chapters --- lang/en/entities.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lang/en/entities.php b/lang/en/entities.php index 4ab9de47dfb..21ef93fed3c 100644 --- a/lang/en/entities.php +++ b/lang/en/entities.php @@ -210,7 +210,7 @@ 'pages_delete_draft' => 'Delete Draft Page', 'pages_delete_success' => 'Page deleted', 'pages_delete_draft_success' => 'Draft page deleted', - 'pages_delete_warning_template' => 'This page is in active use as a book default page template. These books will no longer have a default page template assigned after this page is deleted.', + 'pages_delete_warning_template' => 'This page is in active use as a book or chapter default page template. These books or chapters will no longer have a default page template assigned after this page is deleted.', 'pages_delete_confirm' => 'Are you sure you want to delete this page?', 'pages_delete_draft_confirm' => 'Are you sure you want to delete this draft page?', 'pages_editing_named' => 'Editing Page :pageName', From 64c783c6f8efb351dd73a1cfb4b5f214a731bd57 Mon Sep 17 00:00:00 2001 From: Sascha Date: Mon, 29 Jan 2024 19:55:39 +0100 Subject: [PATCH 014/666] extraded template form to own file and changed translations --- lang/en/entities.php | 9 +++------ resources/views/books/parts/form.blade.php | 18 ++---------------- resources/views/chapters/parts/form.blade.php | 18 ++---------------- .../views/entities/template-selector.blade.php | 14 ++++++++++++++ 4 files changed, 21 insertions(+), 38 deletions(-) create mode 100644 resources/views/entities/template-selector.blade.php diff --git a/lang/en/entities.php b/lang/en/entities.php index 21ef93fed3c..8860e243e77 100644 --- a/lang/en/entities.php +++ b/lang/en/entities.php @@ -39,6 +39,9 @@ 'export_pdf' => 'PDF File', 'export_text' => 'Plain Text File', 'export_md' => 'Markdown File', + 'default_template' => 'Default Page Template', + 'default_template_explain' => 'Assign a page template that will be used as the default content for all new pages in this book/chapter. Keep in mind this will only be used if the page creator has view access to those chosen template page.', + 'default_template_select' => 'Select a template page', // Permissions and restrictions 'permissions' => 'Permissions', @@ -132,9 +135,6 @@ 'books_edit_named' => 'Edit Book :bookName', 'books_form_book_name' => 'Book Name', 'books_save' => 'Save Book', - 'books_default_template' => 'Default Page Template', - 'books_default_template_explain' => 'Assign a page template that will be used as the default content for all new pages in this book. Keep in mind this will only be used if the page creator has view access to those chosen template page.', - 'books_default_template_select' => 'Select a template page', 'books_permissions' => 'Book Permissions', 'books_permissions_updated' => 'Book Permissions Updated', 'books_empty_contents' => 'No pages or chapters have been created for this book.', @@ -192,9 +192,6 @@ 'chapters_permissions_success' => 'Chapter Permissions Updated', 'chapters_search_this' => 'Search this chapter', 'chapter_sort_book' => 'Sort Book', - 'chapter_default_template' => 'Default Page Template', - 'chapter_default_template_explain' => 'Assign a page template that will be used as the default content for all new pages in this chapter. Keep in mind this will only be used if the page creator has view access to those chosen template page.', - 'chapter_default_template_select' => 'Select a template page', // Pages 'page' => 'Page', diff --git a/resources/views/books/parts/form.blade.php b/resources/views/books/parts/form.blade.php index fa8f16e52f4..ee261e72d4a 100644 --- a/resources/views/books/parts/form.blade.php +++ b/resources/views/books/parts/form.blade.php @@ -40,24 +40,10 @@
-
-

- {{ trans('entities.books_default_template_explain') }} -

- -
- @include('form.page-picker', [ - 'name' => 'default_template_id', - 'placeholder' => trans('entities.books_default_template_select'), - 'value' => $book->default_template_id ?? null, - 'selectorEndpoint' => '/search/entity-selector-templates', - ]) -
-
- + @include('entities.template-selector', ['entity' => $book ?? null])
diff --git a/resources/views/chapters/parts/form.blade.php b/resources/views/chapters/parts/form.blade.php index ea7f84bc839..602693916ea 100644 --- a/resources/views/chapters/parts/form.blade.php +++ b/resources/views/chapters/parts/form.blade.php @@ -24,24 +24,10 @@
-
-

- {{ trans('entities.chapter_default_template_explain') }} -

- -
- @include('form.page-picker', [ - 'name' => 'default_template_id', - 'placeholder' => trans('entities.chapter_default_template_select'), - 'value' => $chapter->default_template_id ?? null, - 'selectorEndpoint' => '/search/entity-selector-templates', - ]) -
-
- + @include('entities.template-selector', ['entity' => $chapter ?? null])
diff --git a/resources/views/entities/template-selector.blade.php b/resources/views/entities/template-selector.blade.php new file mode 100644 index 00000000000..80b2f49b2b9 --- /dev/null +++ b/resources/views/entities/template-selector.blade.php @@ -0,0 +1,14 @@ +
+

+ {{ trans('entities.default_template_explain') }} +

+ +
+ @include('form.page-picker', [ + 'name' => 'default_template_id', + 'placeholder' => trans('entities.default_template_select'), + 'value' => $entity->default_template_id ?? null, + 'selectorEndpoint' => '/search/entity-selector-templates', + ]) +
+
\ No newline at end of file From 4a8f70240fd01f28980dfd4031c7df2b1db4949a Mon Sep 17 00:00:00 2001 From: Sascha Date: Mon, 29 Jan 2024 19:59:03 +0100 Subject: [PATCH 015/666] added template to chapter API controller --- .../Controllers/ChapterApiController.php | 26 ++++++++++--------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/app/Entities/Controllers/ChapterApiController.php b/app/Entities/Controllers/ChapterApiController.php index c2132326200..27b8206592d 100644 --- a/app/Entities/Controllers/ChapterApiController.php +++ b/app/Entities/Controllers/ChapterApiController.php @@ -15,20 +15,22 @@ class ChapterApiController extends ApiController { protected $rules = [ 'create' => [ - 'book_id' => ['required', 'integer'], - 'name' => ['required', 'string', 'max:255'], - 'description' => ['string', 'max:1900'], - 'description_html' => ['string', 'max:2000'], - 'tags' => ['array'], - 'priority' => ['integer'], + 'book_id' => ['required', 'integer'], + 'name' => ['required', 'string', 'max:255'], + 'description' => ['string', 'max:1900'], + 'description_html' => ['string', 'max:2000'], + 'tags' => ['array'], + 'priority' => ['integer'], + 'default_template_id' => ['nullable', 'integer'], ], 'update' => [ - 'book_id' => ['integer'], - 'name' => ['string', 'min:1', 'max:255'], - 'description' => ['string', 'max:1900'], - 'description_html' => ['string', 'max:2000'], - 'tags' => ['array'], - 'priority' => ['integer'], + 'book_id' => ['integer'], + 'name' => ['string', 'min:1', 'max:255'], + 'description' => ['string', 'max:1900'], + 'description_html' => ['string', 'max:2000'], + 'tags' => ['array'], + 'priority' => ['integer'], + 'default_template_id' => ['nullable', 'integer'], ], ]; From 24e6dc4b37f857ffe6f8eab85ca177ed290bb38a Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Tue, 30 Jan 2024 11:38:47 +0000 Subject: [PATCH 016/666] WYSIWYG: Altered how custom head added to editors Updated to parse and add as DOM nodes instead of innerHTML to avoid triggering an update of all head content, which would throw warnings in chromium in regard to setting the base URI. For #4814 --- resources/js/wysiwyg/config.js | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/resources/js/wysiwyg/config.js b/resources/js/wysiwyg/config.js index 6c96e47e9c3..36d78b32515 100644 --- a/resources/js/wysiwyg/config.js +++ b/resources/js/wysiwyg/config.js @@ -143,16 +143,23 @@ function gatherPlugins(options) { } /** - * Fetch custom HTML head content from the parent page head into the editor. + * Fetch custom HTML head content nodes from the outer page head + * and add them to the given editor document. + * @param {Document} editorDoc */ -function fetchCustomHeadContent() { +function addCustomHeadContent(editorDoc) { const headContentLines = document.head.innerHTML.split('\n'); const startLineIndex = headContentLines.findIndex(line => line.trim() === ''); const endLineIndex = headContentLines.findIndex(line => line.trim() === ''); if (startLineIndex === -1 || endLineIndex === -1) { - return ''; + return; } - return headContentLines.slice(startLineIndex + 1, endLineIndex).join('\n'); + + const customHeadHtml = headContentLines.slice(startLineIndex + 1, endLineIndex).join('\n'); + const el = editorDoc.createElement('div'); + el.innerHTML = customHeadHtml; + + editorDoc.head.append(...el.children); } /** @@ -284,8 +291,7 @@ export function buildForEditor(options) { } }, init_instance_callback(editor) { - const head = editor.getDoc().querySelector('head'); - head.innerHTML += fetchCustomHeadContent(); + addCustomHeadContent(editor.getDoc()); }, setup(editor) { registerCustomIcons(editor); @@ -335,8 +341,7 @@ export function buildForInput(options) { file_picker_types: 'file', file_picker_callback: filePickerCallback, init_instance_callback(editor) { - const head = editor.getDoc().querySelector('head'); - head.innerHTML += fetchCustomHeadContent(); + addCustomHeadContent(editor.getDoc()); editor.contentDocument.documentElement.classList.toggle('dark-mode', options.darkMode); }, From 5c92b72fdd419ccb6f77bfdf0a1cb1358c51a9d8 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Tue, 30 Jan 2024 14:27:09 +0000 Subject: [PATCH 017/666] Comments: Added input wysiwyg for creating/updating comments Not supporting old content, existing HTML or updating yet. --- app/Activity/Tools/CommentTree.php | 11 ++++++++ resources/js/components/page-comment.js | 27 ++++++++++++++++--- resources/js/components/page-comments.js | 30 ++++++++++++++++++--- resources/js/components/wysiwyg-input.js | 7 ++--- resources/sass/_tinymce.scss | 7 +++++ resources/views/comments/comment.blade.php | 2 ++ resources/views/comments/comments.blade.php | 10 ++++++- resources/views/layouts/base.blade.php | 3 +++ 8 files changed, 85 insertions(+), 12 deletions(-) diff --git a/app/Activity/Tools/CommentTree.php b/app/Activity/Tools/CommentTree.php index 3303add39b8..16f6804ea42 100644 --- a/app/Activity/Tools/CommentTree.php +++ b/app/Activity/Tools/CommentTree.php @@ -41,6 +41,17 @@ public function get(): array return $this->tree; } + public function canUpdateAny(): bool + { + foreach ($this->comments as $comment) { + if (userCan('comment-update', $comment)) { + return true; + } + } + + return false; + } + /** * @param Comment[] $comments */ diff --git a/resources/js/components/page-comment.js b/resources/js/components/page-comment.js index 8284d7f20d8..dc6ca826425 100644 --- a/resources/js/components/page-comment.js +++ b/resources/js/components/page-comment.js @@ -1,5 +1,6 @@ import {Component} from './component'; import {getLoading, htmlToDom} from '../services/dom'; +import {buildForInput} from "../wysiwyg/config"; export class PageComment extends Component { @@ -11,7 +12,12 @@ export class PageComment extends Component { this.deletedText = this.$opts.deletedText; this.updatedText = this.$opts.updatedText; - // Element References + // Editor reference and text options + this.wysiwygEditor = null; + this.wysiwygLanguage = this.$opts.wysiwygLanguage; + this.wysiwygTextDirection = this.$opts.wysiwygTextDirection; + + // Element references this.container = this.$el; this.contentContainer = this.$refs.contentContainer; this.form = this.$refs.form; @@ -50,8 +56,23 @@ export class PageComment extends Component { startEdit() { this.toggleEditMode(true); - const lineCount = this.$refs.input.value.split('\n').length; - this.$refs.input.style.height = `${(lineCount * 20) + 40}px`; + + if (this.wysiwygEditor) { + return; + } + + const config = buildForInput({ + language: this.wysiwygLanguage, + containerElement: this.input, + darkMode: document.documentElement.classList.contains('dark-mode'), + textDirection: this.wysiwygTextDirection, + translations: {}, + translationMap: window.editor_translations, + }); + + window.tinymce.init(config).then(editors => { + this.wysiwygEditor = editors[0]; + }); } async update(event) { diff --git a/resources/js/components/page-comments.js b/resources/js/components/page-comments.js index e2911afc6a0..ebcc95f07f4 100644 --- a/resources/js/components/page-comments.js +++ b/resources/js/components/page-comments.js @@ -1,5 +1,6 @@ import {Component} from './component'; import {getLoading, htmlToDom} from '../services/dom'; +import {buildForInput} from "../wysiwyg/config"; export class PageComments extends Component { @@ -21,6 +22,11 @@ export class PageComments extends Component { this.hideFormButton = this.$refs.hideFormButton; this.removeReplyToButton = this.$refs.removeReplyToButton; + // WYSIWYG options + this.wysiwygLanguage = this.$opts.wysiwygLanguage; + this.wysiwygTextDirection = this.$opts.wysiwygTextDirection; + this.wysiwygEditor = null; + // Translations this.createdText = this.$opts.createdText; this.countText = this.$opts.countText; @@ -96,9 +102,7 @@ export class PageComments extends Component { this.formContainer.toggleAttribute('hidden', false); this.addButtonContainer.toggleAttribute('hidden', true); this.formContainer.scrollIntoView({behavior: 'smooth', block: 'nearest'}); - setTimeout(() => { - this.formInput.focus(); - }, 100); + this.loadEditor(); } hideForm() { @@ -112,6 +116,26 @@ export class PageComments extends Component { this.addButtonContainer.toggleAttribute('hidden', false); } + loadEditor() { + if (this.wysiwygEditor) { + return; + } + + const config = buildForInput({ + language: this.wysiwygLanguage, + containerElement: this.formInput, + darkMode: document.documentElement.classList.contains('dark-mode'), + textDirection: this.wysiwygTextDirection, + translations: {}, + translationMap: window.editor_translations, + }); + + window.tinymce.init(config).then(editors => { + this.wysiwygEditor = editors[0]; + this.wysiwygEditor.focus(); + }); + } + getCommentCount() { return this.container.querySelectorAll('[component="page-comment"]').length; } diff --git a/resources/js/components/wysiwyg-input.js b/resources/js/components/wysiwyg-input.js index 88c06a334c6..ad964aed2c4 100644 --- a/resources/js/components/wysiwyg-input.js +++ b/resources/js/components/wysiwyg-input.js @@ -10,11 +10,8 @@ export class WysiwygInput extends Component { language: this.$opts.language, containerElement: this.elem, darkMode: document.documentElement.classList.contains('dark-mode'), - textDirection: this.textDirection, - translations: { - imageUploadErrorText: this.$opts.imageUploadErrorText, - serverUploadLimitText: this.$opts.serverUploadLimitText, - }, + textDirection: this.$opts.textDirection, + translations: {}, translationMap: window.editor_translations, }); diff --git a/resources/sass/_tinymce.scss b/resources/sass/_tinymce.scss index c4336da7cb7..fb5ea7e6ffe 100644 --- a/resources/sass/_tinymce.scss +++ b/resources/sass/_tinymce.scss @@ -30,6 +30,13 @@ display: block; } +.wysiwyg-input.mce-content-body:before { + padding: 1rem; + top: 4px; + font-style: italic; + color: rgba(34,47,62,.5) +} + // Default styles for our custom root nodes .page-content.mce-content-body doc-root { display: block; diff --git a/resources/views/comments/comment.blade.php b/resources/views/comments/comment.blade.php index 1cb70916087..4340cfdf5c4 100644 --- a/resources/views/comments/comment.blade.php +++ b/resources/views/comments/comment.blade.php @@ -4,6 +4,8 @@ option:page-comment:comment-parent-id="{{ $comment->parent_id }}" option:page-comment:updated-text="{{ trans('entities.comment_updated_success') }}" option:page-comment:deleted-text="{{ trans('entities.comment_deleted_success') }}" + option:page-comment:wysiwyg-language="{{ $locale->htmlLang() }}" + option:page-comment:wysiwyg-text-direction="{{ $locale->htmlDirection() }}" id="comment{{$comment->local_id}}" class="comment-box">
diff --git a/resources/views/comments/comments.blade.php b/resources/views/comments/comments.blade.php index 26d286290c2..2c314864bc2 100644 --- a/resources/views/comments/comments.blade.php +++ b/resources/views/comments/comments.blade.php @@ -2,6 +2,8 @@ option:page-comments:page-id="{{ $page->id }}" option:page-comments:created-text="{{ trans('entities.comment_created_success') }}" option:page-comments:count-text="{{ trans('entities.comment_count') }}" + option:page-comments:wysiwyg-language="{{ $locale->htmlLang() }}" + option:page-comments:wysiwyg-text-direction="{{ $locale->htmlDirection() }}" class="comments-list" aria-label="{{ trans('entities.comments') }}"> @@ -24,7 +26,6 @@ class="button outline">{{ trans('entities.comment_add') }} @if(userCan('comment-create-all')) @include('comments.create') - @if (!$commentTree->empty())
@endif @endif + @if(userCan('comment-create-all') || $commentTree->canUpdateAny()) + @push('post-app-scripts') + + @include('form.editor-translations') + @endpush + @endif + \ No newline at end of file diff --git a/resources/views/layouts/base.blade.php b/resources/views/layouts/base.blade.php index cf15e5426b8..43cca6b14c5 100644 --- a/resources/views/layouts/base.blade.php +++ b/resources/views/layouts/base.blade.php @@ -68,10 +68,13 @@ class="@stack('body-class')">
@yield('bottom') + + @if($cspNonce ?? false) @endif @yield('scripts') + @stack('post-app-scripts') @include('layouts.parts.base-body-end') From adf0baebb9ffc61cc944c0572ec6dbb12a5b41a0 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Tue, 30 Jan 2024 15:16:58 +0000 Subject: [PATCH 018/666] Comments: Added back-end HTML support, fixed editor focus Also fixed handling of editors when moved in DOM, to properly remove then re-init before & after move to avoid issues. --- app/Activity/CommentRepo.php | 26 ++++--------------- .../Controllers/CommentController.php | 17 +++++++----- resources/js/components/page-comment.js | 6 +++-- resources/js/components/page-comments.js | 17 +++++++++--- resources/views/comments/comment.blade.php | 2 +- resources/views/comments/create.blade.php | 2 +- 6 files changed, 34 insertions(+), 36 deletions(-) diff --git a/app/Activity/CommentRepo.php b/app/Activity/CommentRepo.php index ce2950e4d79..3336e17e988 100644 --- a/app/Activity/CommentRepo.php +++ b/app/Activity/CommentRepo.php @@ -5,7 +5,7 @@ use BookStack\Activity\Models\Comment; use BookStack\Entities\Models\Entity; use BookStack\Facades\Activity as ActivityService; -use League\CommonMark\CommonMarkConverter; +use BookStack\Util\HtmlDescriptionFilter; class CommentRepo { @@ -20,13 +20,12 @@ public function getById(int $id): Comment /** * Create a new comment on an entity. */ - public function create(Entity $entity, string $text, ?int $parent_id): Comment + public function create(Entity $entity, string $html, ?int $parent_id): Comment { $userId = user()->id; $comment = new Comment(); - $comment->text = $text; - $comment->html = $this->commentToHtml($text); + $comment->html = HtmlDescriptionFilter::filterFromString($html); $comment->created_by = $userId; $comment->updated_by = $userId; $comment->local_id = $this->getNextLocalId($entity); @@ -42,11 +41,10 @@ public function create(Entity $entity, string $text, ?int $parent_id): Comment /** * Update an existing comment. */ - public function update(Comment $comment, string $text): Comment + public function update(Comment $comment, string $html): Comment { $comment->updated_by = user()->id; - $comment->text = $text; - $comment->html = $this->commentToHtml($text); + $comment->html = HtmlDescriptionFilter::filterFromString($html); $comment->save(); ActivityService::add(ActivityType::COMMENT_UPDATE, $comment); @@ -64,20 +62,6 @@ public function delete(Comment $comment): void ActivityService::add(ActivityType::COMMENT_DELETE, $comment); } - /** - * Convert the given comment Markdown to HTML. - */ - public function commentToHtml(string $commentText): string - { - $converter = new CommonMarkConverter([ - 'html_input' => 'strip', - 'max_nesting_level' => 10, - 'allow_unsafe_links' => false, - ]); - - return $converter->convert($commentText); - } - /** * Get the next local ID relative to the linked entity. */ diff --git a/app/Activity/Controllers/CommentController.php b/app/Activity/Controllers/CommentController.php index 516bcac759a..340524cd069 100644 --- a/app/Activity/Controllers/CommentController.php +++ b/app/Activity/Controllers/CommentController.php @@ -22,8 +22,8 @@ public function __construct( */ public function savePageComment(Request $request, int $pageId) { - $this->validate($request, [ - 'text' => ['required', 'string'], + $input = $this->validate($request, [ + 'html' => ['required', 'string'], 'parent_id' => ['nullable', 'integer'], ]); @@ -39,7 +39,7 @@ public function savePageComment(Request $request, int $pageId) // Create a new comment. $this->checkPermission('comment-create-all'); - $comment = $this->commentRepo->create($page, $request->get('text'), $request->get('parent_id')); + $comment = $this->commentRepo->create($page, $input['html'], $input['parent_id'] ?? null); return view('comments.comment-branch', [ 'readOnly' => false, @@ -57,17 +57,20 @@ public function savePageComment(Request $request, int $pageId) */ public function update(Request $request, int $commentId) { - $this->validate($request, [ - 'text' => ['required', 'string'], + $input = $this->validate($request, [ + 'html' => ['required', 'string'], ]); $comment = $this->commentRepo->getById($commentId); $this->checkOwnablePermission('page-view', $comment->entity); $this->checkOwnablePermission('comment-update', $comment); - $comment = $this->commentRepo->update($comment, $request->get('text')); + $comment = $this->commentRepo->update($comment, $input['html']); - return view('comments.comment', ['comment' => $comment, 'readOnly' => false]); + return view('comments.comment', [ + 'comment' => $comment, + 'readOnly' => false, + ]); } /** diff --git a/resources/js/components/page-comment.js b/resources/js/components/page-comment.js index dc6ca826425..79c9d3c2c67 100644 --- a/resources/js/components/page-comment.js +++ b/resources/js/components/page-comment.js @@ -1,6 +1,6 @@ import {Component} from './component'; import {getLoading, htmlToDom} from '../services/dom'; -import {buildForInput} from "../wysiwyg/config"; +import {buildForInput} from '../wysiwyg/config'; export class PageComment extends Component { @@ -58,6 +58,7 @@ export class PageComment extends Component { this.toggleEditMode(true); if (this.wysiwygEditor) { + this.wysiwygEditor.focus(); return; } @@ -72,6 +73,7 @@ export class PageComment extends Component { window.tinymce.init(config).then(editors => { this.wysiwygEditor = editors[0]; + setTimeout(() => this.wysiwygEditor.focus(), 50); }); } @@ -81,7 +83,7 @@ export class PageComment extends Component { this.form.toggleAttribute('hidden', true); const reqData = { - text: this.input.value, + html: this.wysiwygEditor.getContent(), parent_id: this.parentId || null, }; diff --git a/resources/js/components/page-comments.js b/resources/js/components/page-comments.js index ebcc95f07f4..cfb0634a904 100644 --- a/resources/js/components/page-comments.js +++ b/resources/js/components/page-comments.js @@ -1,6 +1,6 @@ import {Component} from './component'; import {getLoading, htmlToDom} from '../services/dom'; -import {buildForInput} from "../wysiwyg/config"; +import {buildForInput} from '../wysiwyg/config'; export class PageComments extends Component { @@ -65,9 +65,8 @@ export class PageComments extends Component { this.form.after(loading); this.form.toggleAttribute('hidden', true); - const text = this.formInput.value; const reqData = { - text, + html: this.wysiwygEditor.getContent(), parent_id: this.parentId || null, }; @@ -92,6 +91,7 @@ export class PageComments extends Component { } resetForm() { + this.removeEditor(); this.formInput.value = ''; this.parentId = null; this.replyToRow.toggleAttribute('hidden', true); @@ -99,6 +99,7 @@ export class PageComments extends Component { } showForm() { + this.removeEditor(); this.formContainer.toggleAttribute('hidden', false); this.addButtonContainer.toggleAttribute('hidden', true); this.formContainer.scrollIntoView({behavior: 'smooth', block: 'nearest'}); @@ -118,6 +119,7 @@ export class PageComments extends Component { loadEditor() { if (this.wysiwygEditor) { + this.wysiwygEditor.focus(); return; } @@ -132,10 +134,17 @@ export class PageComments extends Component { window.tinymce.init(config).then(editors => { this.wysiwygEditor = editors[0]; - this.wysiwygEditor.focus(); + setTimeout(() => this.wysiwygEditor.focus(), 50); }); } + removeEditor() { + if (this.wysiwygEditor) { + this.wysiwygEditor.remove(); + this.wysiwygEditor = null; + } + } + getCommentCount() { return this.container.querySelectorAll('[component="page-comment"]').length; } diff --git a/resources/views/comments/comment.blade.php b/resources/views/comments/comment.blade.php index 4340cfdf5c4..e00307f0fbc 100644 --- a/resources/views/comments/comment.blade.php +++ b/resources/views/comments/comment.blade.php @@ -77,7 +77,7 @@ class="comment-box"> @if(!$readOnly && userCan('comment-update', $comment))