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'); +});