From e3554bdd3958920164e0bf7bde1dfa97899b1c64 Mon Sep 17 00:00:00 2001 From: denkfabrik-li <274324701+denkfabrik-li@users.noreply.github.com> Date: Wed, 26 Aug 2026 03:34:40 +0200 Subject: [PATCH] Serve the public comment thread to the public, whoever happens to be logged in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit VisibleCommentScope says at the top of the class that its callers must already have established that the viewer may see the file. The public listing's comment endpoint establishes only the guest half of that — the file is reachable without an account — and then hands $request->user() straight to the authenticated reading. For anyone the file's own gate would refuse, that reading is far too wide. A staff account outside its library, or one holding no file permission at all, read the file's staff-only notes; a client the file was never shared with read the messages staff addressed to that file's clients. Both get a 403 from GET /files/{file}/comments and needed only to ask the public URL instead. The endpoint now asks the file's own gate which reading applies. A reader it admits sees no less than before. A reader it refuses gets what a visitor gets, widened by their own comments — which is what this controller has always promised them, and all it promised. The held-comment rule moves into a method both readings share rather than being restated. The same root reaches PATCH and DELETE /comments/{comment}, which bind a comment rather than a file and so never authorized `view` on it either. They answer with the thread, and now with the one the file's gate allows. Refusing them outright would be wrong: somebody who commented through the public page is exactly the person entitled to edit their own words. --- .../Comments/Access/VisibleCommentScope.php | 85 +++++++-- app/Modules/Comments/CommentPresenter.php | 14 +- .../Controllers/FileCommentsController.php | 29 ++- .../PublicFileCommentsController.php | 30 ++- .../Comments/PublicThreadScopeTest.php | 179 ++++++++++++++++++ 5 files changed, 318 insertions(+), 19 deletions(-) create mode 100644 tests/Feature/Comments/PublicThreadScopeTest.php diff --git a/app/Modules/Comments/Access/VisibleCommentScope.php b/app/Modules/Comments/Access/VisibleCommentScope.php index de1854a4..8f8e923f 100644 --- a/app/Modules/Comments/Access/VisibleCommentScope.php +++ b/app/Modules/Comments/Access/VisibleCommentScope.php @@ -68,6 +68,50 @@ class VisibleCommentScope ); } + /** + * The thread as somebody reading the public listing sees it: what a + * visitor is shown, plus their own comments if they happen to be + * signed in. + * + * for() above is the authenticated reading, and it assumes what the + * top of this class demands — that the caller established the viewer + * may see the *file*. The public listing establishes only the other + * half of that, namely that the file is reachable without logging in, + * which is the whole of it for a visitor and not nearly enough for an + * account: for() hands any staff member the file's StaffOnly notes and + * any client the messages addressed to all clients on it. + * + * So a signed-in reader is answered as a visitor is, widened by their + * own writing — which is what "being logged in should not show you + * less than a stranger sees, and their own comments should be theirs + * to edit" asks for and all it asks for. A reader the file's own gate + * would admit is not narrowed at all; the caller sends them through + * for() instead. + * + * The held-comment rule is not restated here — hideUnapproved() is the + * same one for()'s readings get, so a comment waiting for a moderator + * stays exactly as visible, or invisible, as it was. + * + * @return Builder + */ + public function forPublicReader(?User $viewer, File $file): Builder + { + $query = FileComment::query()->where('file_id', $file->id); + + if ($viewer === null) { + return $this->applyVisibility($query, null, $file->isEffectivelyPublic()); + } + + $this->hideUnapproved($query, $viewer); + + return $query->where(fn (Builder $outer) => $outer + ->where('author_id', $viewer->id) + ->when( + $file->isEffectivelyPublic(), + fn (Builder $stranger) => $stranger->orWhere('visibility', CommentVisibility::Everyone), + )); + } + /** * Every comment this staff member may read, across their whole library * — the management screen's query, rather than one file's thread. @@ -171,25 +215,40 @@ class VisibleCommentScope return $counts; } + /** + * A comment awaiting moderation exists only for those who can act on + * it — and for whoever wrote it, who would otherwise watch their own + * comment vanish on posting and conclude it had failed. A visitor is + * recognised by their session (see GuestCommentIdentity); that is weak + * on purpose, and only ever widens what somebody sees of their own + * writing. + * + * Its own method because forPublicReader() answers a different + * audience question and the same held-comment one, and a rule this + * sharp stated twice is a rule that drifts. + * + * @param Builder $query + */ + private function hideUnapproved(Builder $query, ?User $viewer): void + { + if ($viewer !== null && $viewer->can('moderate_comments')) { + return; + } + + $ownPending = $viewer === null ? $this->guests->ownCommentIds() : []; + + $query->where(fn (Builder $visible) => $visible + ->whereNotNull('approved_at') + ->when($ownPending !== [], fn (Builder $mine) => $mine->orWhereIn('id', $ownPending))); + } + /** * @param Builder $query * @return Builder */ private function applyVisibility(Builder $query, ?User $viewer, bool $isPublic): Builder { - // A comment awaiting moderation exists only for those who can act - // on it — and for whoever wrote it, who would otherwise watch their - // own comment vanish on posting and conclude it had failed. A - // visitor is recognised by their session (see GuestCommentIdentity); - // that is weak on purpose, and only ever widens what somebody sees - // of their own writing. - if ($viewer === null || ! $viewer->can('moderate_comments')) { - $ownPending = $viewer === null ? $this->guests->ownCommentIds() : []; - - $query->where(fn (Builder $visible) => $visible - ->whereNotNull('approved_at') - ->when($ownPending !== [], fn (Builder $mine) => $mine->orWhereIn('id', $ownPending))); - } + $this->hideUnapproved($query, $viewer); if ($viewer === null) { // Publicness is re-derived here on every read rather than diff --git a/app/Modules/Comments/CommentPresenter.php b/app/Modules/Comments/CommentPresenter.php index 0dbf41f3..447deed5 100644 --- a/app/Modules/Comments/CommentPresenter.php +++ b/app/Modules/Comments/CommentPresenter.php @@ -31,13 +31,23 @@ class CommentPresenter ) {} /** + * The whole payload for one file's thread. + * + * $viewerMaySeeFile is the precondition VisibleCommentScope states at + * the top of its class: the caller must already have established that + * this viewer may see the file. A caller that has not says so, and the + * thread is narrowed to the public reading instead of the + * authenticated one. + * * @return array{comments: list>, can_comment: bool, cannot_comment_reason: string|null, is_guest: bool, guest_moderated: bool, captcha_required: bool, visibilities: list>, default_visibility: string|null, edit_window_minutes: int} */ - public function thread(?User $viewer, File $file): array + public function thread(?User $viewer, File $file, bool $viewerMaySeeFile = true): array { $forStaff = $viewer?->isStaff() === true; - $comments = $this->scope->for($viewer, $file) + $comments = ($viewerMaySeeFile + ? $this->scope->for($viewer, $file) + : $this->scope->forPublicReader($viewer, $file)) ->with(['author', 'clientContext']) ->orderBy('created_at') ->orderBy('id') diff --git a/app/Modules/Comments/Http/Controllers/FileCommentsController.php b/app/Modules/Comments/Http/Controllers/FileCommentsController.php index fc03ad8a..34f2a3bd 100644 --- a/app/Modules/Comments/Http/Controllers/FileCommentsController.php +++ b/app/Modules/Comments/Http/Controllers/FileCommentsController.php @@ -77,7 +77,7 @@ class FileCommentsController extends Controller $this->comments->edit($comment, $validated['body']); - return response()->json($this->payload($viewer, $comment->file)); + return response()->json($this->payloadAfterChange($viewer, $comment->file)); } public function destroy(Request $request, FileComment $comment): JsonResponse @@ -90,10 +90,13 @@ class FileCommentsController extends Controller $this->comments->remove($comment); - return response()->json($this->payload($viewer, $file)); + return response()->json($this->payloadAfterChange($viewer, $file)); } /** + * The thread for the two routes that bind a file, after their own + * `view` authorization has passed. + * * @return array */ private function payload(User $viewer, File $file): array @@ -101,6 +104,28 @@ class FileCommentsController extends Controller return $this->presenter->thread($viewer, $file); } + /** + * The thread that goes back with a change to one comment. + * + * update() and destroy() bind a comment rather than a file, so nothing + * in the request has established that this viewer may read the file's + * conversation — only that this one comment is theirs to change. + * Somebody who commented through the public listing is exactly that + * person, and refusing them on their own edit would be wrong, so the + * reading they get back is the one the file's own gate allows them: + * the public page's, if that is how they arrived. + * + * @return array + */ + private function payloadAfterChange(User $viewer, File $file): array + { + return $this->presenter->thread( + $viewer, + $file, + viewerMaySeeFile: Gate::forUser($viewer)->allows('view', $file), + ); + } + /** * The comment being answered, resolved through the same scope that * decided what this viewer may read. A reply can therefore only ever diff --git a/app/Modules/Comments/Http/Controllers/PublicFileCommentsController.php b/app/Modules/Comments/Http/Controllers/PublicFileCommentsController.php index cac592c0..5faa36ab 100644 --- a/app/Modules/Comments/Http/Controllers/PublicFileCommentsController.php +++ b/app/Modules/Comments/Http/Controllers/PublicFileCommentsController.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace App\Modules\Comments\Http\Controllers; use App\Http\Controllers\Controller; +use App\Models\User; use App\Modules\Comments\CommentingRules; use App\Modules\Comments\CommentPresenter; use App\Modules\Comments\CommentVisibility; @@ -17,6 +18,7 @@ use App\Modules\Platform\Settings\Settings; use App\Support\Rules; use Illuminate\Http\JsonResponse; use Illuminate\Http\Request; +use Illuminate\Support\Facades\Gate; /** * Comments on a publicly-listed file, for visitors who are not logged in. @@ -46,7 +48,7 @@ class PublicFileCommentsController extends Controller { $this->guard($publicSlug, $file); - return response()->json($this->presenter->thread($request->user(), $file)); + return response()->json($this->thread($request->user(), $file)); } public function store(Request $request, string $publicSlug, File $file): JsonResponse @@ -86,7 +88,31 @@ class PublicFileCommentsController extends Controller $this->guests->remember($comment->id); } - return response()->json($this->presenter->thread($viewer, $file), 201); + return response()->json($this->thread($viewer, $file), 201); + } + + /** + * The thread as this endpoint may serve it. + * + * guard() establishes the guest half of VisibleCommentScope's + * precondition — the file is reachable without logging in — and that + * is the whole of it for a visitor. It says nothing about an account, + * and handing a signed-in viewer to the authenticated reading anyway + * is what let any staff account read a public file's StaffOnly notes + * and any client account read the messages addressed to that file's + * clients. The file's own gate decides which reading applies; the one + * it does not admit still reads what a visitor reads plus their own + * comments, which is what this endpoint has always promised them. + * + * @return array + */ + private function thread(?User $viewer, File $file): array + { + return $this->presenter->thread( + $viewer, + $file, + viewerMaySeeFile: $viewer !== null && Gate::forUser($viewer)->allows('view', $file), + ); } /** diff --git a/tests/Feature/Comments/PublicThreadScopeTest.php b/tests/Feature/Comments/PublicThreadScopeTest.php new file mode 100644 index 00000000..62fb64b9 --- /dev/null +++ b/tests/Feature/Comments/PublicThreadScopeTest.php @@ -0,0 +1,179 @@ +admin = User::factory()->create(); + $this->settings = app(Settings::class); + + $this->settings->set(Setting::PublicListingEnabled, true); + $this->settings->set(Setting::PublicListingSlug, 'public'); + $this->settings->set(Setting::CommentsScope, 'all'); + $this->settings->set(Setting::CommentsAuthors, 'everyone'); + $this->settings->set(Setting::PublicCommentsEnabled, true); + $this->settings->set(Setting::CommentsGuestModeration, true); + $this->settings->set(Setting::CaptchaProvider, 'none'); + + $this->group = Group::query()->create(['name' => 'Showcase', 'public' => true]); + $this->file = File::factory()->public()->create(['uploaded_by' => $this->admin->id]); + shareFileWithGroup($this->file, $this->group); + + // The conversation on that file: one line anybody may read, one note + // for staff, and one message from staff to every client on it. + $this->everyone = FileComment::factory()->for($this->file)->everyone() + ->create(['author_id' => $this->admin->id, 'body' => 'Nice work']); + $this->staffNote = FileComment::factory()->for($this->file)->staffOnly() + ->create(['author_id' => $this->admin->id, 'body' => 'Internal only']); + $this->broadcast = FileComment::factory()->for($this->file)->toAllClients() + ->create(['author_id' => $this->admin->id, 'body' => 'For our clients']); +}); + +/** The thread as $viewer reads it from the public listing. */ +function publicThreadFor(?User $viewer): array +{ + $test = test(); + $request = $viewer === null ? $test : $test->actingAs($viewer); + + return $request->getJson("/public/files/{$test->file->slug}/comments")->assertOk()->json('comments'); +} + +test('a staff member the file gate does not admit reads no staff-only note there', function () { + // A client-scoped manager whose assigned client has nothing to do + // with this file: GET /files/{file}/comments answers them 403, and + // the public page must not be the way around that. + $manager = User::factory()->role(SystemRole::ClientManager)->create(); + $manager->assignedClients()->sync([User::factory()->client()->create()->id]); + + $this->actingAs($manager)->getJson("/files/{$this->file->id}/comments")->assertForbidden(); + + $bodies = array_column(publicThreadFor($manager), 'body'); + + expect($bodies)->toBe(['Nice work']); +}); + +test('a staff member with no file permissions at all reads no staff-only note there either', function () { + // The other half of the same door: FilePolicy::view wants one of + // upload / edit_files / edit_others_files before it looks at scope, + // so a staff account holding none of them cannot open this file. + $staff = staffWithPermissions(['view_news']); + + $this->actingAs($staff)->getJson("/files/{$this->file->id}/comments")->assertForbidden(); + + expect(array_column(publicThreadFor($staff), 'body'))->toBe(['Nice work']); +}); + +test('a client the file was never shared with reads no message to its clients', function () { + $stranger = User::factory()->client()->create(); + + $this->actingAs($stranger)->getJson("/files/{$this->file->id}/comments")->assertForbidden(); + + expect(array_column(publicThreadFor($stranger), 'body'))->toBe(['Nice work']); +}); + +test('a client the file was shared with still reads their whole conversation', function () { + // The fix is not deny-everything: this reader passes the file's own + // gate, so the public page shows them exactly what their portal does. + $client = User::factory()->client()->create(); + $client->memberOfGroups()->attach($this->group->id); + + FileComment::factory()->for($this->file)->inThreadOf($client) + ->create(['author_id' => $this->admin->id, 'body' => 'Just for you']); + + $this->actingAs($client)->getJson("/files/{$this->file->id}/comments")->assertOk(); + + expect(array_column(publicThreadFor($client), 'body')) + ->toContain('Nice work', 'For our clients', 'Just for you') + ->not->toContain('Internal only'); +}); + +test('a signed-in visitor is still served as themselves', function () { + // The endpoint's own promise, and the one thing narrowing must not + // take away: their own writing is theirs wherever they read it. + $stranger = User::factory()->client()->create(); + + $mine = FileComment::factory()->for($this->file)->inThreadOf($stranger) + ->create(['author_id' => $stranger->id, 'body' => 'I wrote this']); + // And held stays held. The approval rule is shared with the + // authenticated reading rather than restated, so this endpoint keeps + // answering it exactly as it did. + $held = FileComment::factory()->for($this->file)->everyone()->pending() + ->create(['author_id' => $stranger->id, 'body' => 'Mine, waiting']); + + $ids = array_column(publicThreadFor($stranger), 'id'); + + expect($ids)->toContain($mine->id, $this->everyone->id) + ->not->toContain($held->id) + ->not->toContain($this->staffNote->id) + ->not->toContain($this->broadcast->id); +}); + +test('a visitor with no account reads exactly what they read before', function () { + expect(array_column(publicThreadFor(null), 'body'))->toBe(['Nice work']); + + // Including their own comment while it waits, which rides on the + // session rather than on an account. + $this->postJson("/public/files/{$this->file->slug}/comments", [ + 'body' => 'Passing through', + 'guest_name' => 'A visitor', + ])->assertCreated(); + + expect(array_column(publicThreadFor(null), 'body'))->toBe(['Nice work', 'Passing through']); +}); + +test('editing a comment posted through the public listing does not hand back the private thread', function () { + $stranger = User::factory()->client()->create(); + + $this->actingAs($stranger)->postJson("/public/files/{$this->file->slug}/comments", ['body' => 'Hello']) + ->assertCreated(); + + $mine = FileComment::query()->where('author_id', $stranger->id)->sole(); + + $bodies = array_column( + $this->actingAs($stranger)->patchJson("/comments/{$mine->id}", ['body' => 'Hello again']) + ->assertOk()->json('comments'), + 'body', + ); + + expect($bodies)->toBe(['Nice work', 'Hello again']); +}); + +test('deleting one does not either', function () { + $stranger = User::factory()->client()->create(); + + $this->actingAs($stranger)->postJson("/public/files/{$this->file->slug}/comments", ['body' => 'Hello']) + ->assertCreated(); + + $mine = FileComment::query()->where('author_id', $stranger->id)->sole(); + + $bodies = array_column( + $this->actingAs($stranger)->deleteJson("/comments/{$mine->id}")->assertOk()->json('comments'), + 'body', + ); + + expect($bodies)->toBe(['Nice work']); +}); + +test('the file-bound endpoint is untouched for a reader who may see the file', function () { + // The routes that bind a file authorize `view` before they present + // anything, so nothing about them changes — this pins that the shared + // presenter still gives them the full authenticated reading. + $bodies = array_column( + $this->actingAs($this->admin)->getJson("/files/{$this->file->id}/comments")->assertOk()->json('comments'), + 'body', + ); + + expect($bodies)->toContain('Nice work', 'Internal only', 'For our clients'); +});