diff --git a/app/Modules/Comments/FileComments.php b/app/Modules/Comments/FileComments.php index c61672b2..5bd66da0 100644 --- a/app/Modules/Comments/FileComments.php +++ b/app/Modules/Comments/FileComments.php @@ -220,10 +220,27 @@ class FileComments return null; } + // Asked of the column, not of the relation. client_context_id is + // cascadeOnDelete, but a user is soft-deleted, so the cascade + // never fires: the column goes on pointing at a row that is still + // there while the relation resolves to null. Branching on the + // relation therefore read "this is Alice's conversation" as "this + // has no conversation" — and a null context on a Clients comment + // is the branch every client on the file reads (see + // VisibleCommentScope's opening rule). A private reply became a + // circular, and canAssignClient below was skipped on the way. + if ($replyTo->client_context_id === null) { + return null; + } + $client = $replyTo->clientContext; if ($client === null) { - return null; + // The column points at somebody, and that somebody is gone. + // There is nobody to answer, and the one outcome that must + // not follow from a filled column is the broadcast above, so + // this refuses rather than falling through to it. + throw new AuthorizationException('You cannot reply in this conversation.'); } if (! $this->library->canAssignClient($author, $client)) { diff --git a/app/Modules/Comments/Models/FileComment.php b/app/Modules/Comments/Models/FileComment.php index 7cdd80a9..2be4cd19 100644 --- a/app/Modules/Comments/Models/FileComment.php +++ b/app/Modules/Comments/Models/FileComment.php @@ -113,18 +113,32 @@ class FileComment extends Model * The name to show. Snapshotted for guests at write time; read live * for accounts so a rename is reflected everywhere at once. * - * author_id cascades on delete, so a row that has one always has the - * account behind it — there is no deleted-author case to snapshot - * against, unlike the activity log's actor_name. + * A deleted account is still read. author_id cascades on delete, but + * a user is soft-deleted and the cascade never fires, so the row + * behind a deleted commenter is still there — and reading it through + * the plain relation returned null, which sent a named client's + * comment out as "Anonymous". That is what a guest comment looks + * like, and a guest comment is governed by different rules; the two + * must not be able to look the same. Whether the author is a guest is + * decided by author_id alone, which is also what isFromGuest() asks. */ public function authorName(): string { + if ($this->author_id === null) { + return $this->guest_name ?? (string) __('Anonymous'); + } + $author = $this->author; if ($author !== null) { return $author->name; } - return $this->guest_name ?? (string) __('Anonymous'); + // Trashed: the row is still there, the relation simply will not + // hand it over. Nothing comes back only once the grace-period + // erasure has removed the row for real. + $name = $this->author()->withTrashed()->value('name'); + + return is_string($name) ? $name : (string) __('Anonymous'); } } diff --git a/tests/Feature/Comments/DeletedClientThreadTest.php b/tests/Feature/Comments/DeletedClientThreadTest.php new file mode 100644 index 00000000..8bacd09a --- /dev/null +++ b/tests/Feature/Comments/DeletedClientThreadTest.php @@ -0,0 +1,100 @@ +admin = User::factory()->create(); + $this->file = File::factory()->create(['name' => 'Quote']); + $this->alice = User::factory()->client()->create(['name' => 'Alice Ltd']); + $this->bob = User::factory()->client()->create(['name' => 'Bob GmbH']); + + shareFileWith($this->file, $this->alice); + shareFileWith($this->file, $this->bob); + + $this->fromAlice = FileComment::factory()->for($this->file)->inThreadOf($this->alice)->create([ + 'author_id' => $this->alice->id, + 'body' => 'What is your best price?', + ]); +}); + +function readableBy(User $viewer, File $file): array +{ + return app(VisibleCommentScope::class)->for($viewer, $file)->pluck('id')->map(intval(...))->all(); +} + +test('a reply into a deleted client\'s thread is refused, not broadcast', function () { + $this->alice->delete(); + + $this->actingAs($this->admin) + ->postJson("/files/{$this->file->id}/comments", [ + 'body' => 'For you only: 40% off.', + 'visibility' => CommentVisibility::Clients->value, + 'reply_to' => $this->fromAlice->id, + ]) + ->assertForbidden(); + + expect(FileComment::query()->where('body', 'For you only: 40% off.')->exists())->toBeFalse() + ->and(readableBy($this->bob, $this->file))->toBe([]); +}); + +test('a staff message to everybody still reaches everybody', function () { + // The null context is a real branch, not only a failure mode: staff + // writing fresh address every client on the file, and that must keep + // working. + $this->actingAs($this->admin) + ->postJson("/files/{$this->file->id}/comments", [ + 'body' => 'The catalogue is attached.', + 'visibility' => CommentVisibility::Clients->value, + ]) + ->assertCreated(); + + $broadcast = FileComment::query()->where('body', 'The catalogue is attached.')->sole(); + + expect($broadcast->client_context_id)->toBeNull() + ->and(readableBy($this->bob, $this->file))->toContain($broadcast->id); +}); + +test('a reply into a live client\'s thread still lands in that thread alone', function () { + $this->actingAs($this->admin) + ->postJson("/files/{$this->file->id}/comments", [ + 'body' => 'For you only: 40% off.', + 'visibility' => CommentVisibility::Clients->value, + 'reply_to' => $this->fromAlice->id, + ]) + ->assertCreated(); + + $reply = FileComment::query()->where('body', 'For you only: 40% off.')->sole(); + + expect($reply->client_context_id)->toBe($this->alice->id) + ->and(readableBy($this->alice, $this->file))->toContain($reply->id) + ->and(readableBy($this->bob, $this->file))->not->toContain($reply->id); +}); + +test('a deleted client\'s comment keeps their name instead of reading as a guest', function () { + $this->alice->delete(); + + expect($this->fromAlice->fresh()->authorName())->toBe('Alice Ltd'); +}); + +test('a genuine guest comment is still anonymous', function () { + $guest = FileComment::factory()->for($this->file)->fromGuest()->create(); + + expect($guest->authorName())->not->toBe('Alice Ltd') + ->and($guest->author_id)->toBeNull(); +});