Merge pull request #1717 from denkfabrik-li/fix/deleted-client-comment-context

file_comments.client_context_id is cascadeOnDelete, but users are 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. resolveClientContext() branched on the relation, so "this is Alice's conversation" read as "this has no conversation" -- and a null context on a clients comment is the branch every client on the file reads. A staff reply into a departed client's private thread became a circular, and canAssignClient() was skipped on the way.

That is the invariant docs/feature-comments.md calls the rule everything hangs off: a clients comment carrying client_context_id = C is never returned to any non-staff viewer other than C, because one customer learning another exists is worse than leaking a comment's text.

Verified before merging: both new tests are red on main and green here, and the three that must not move stay green either way. Suite at 2093, PHPStan clean.

The second half is the same root cause through the other column. authorName() read a deleted client's comment as "Anonymous", which is what a visitor's comment looks like -- and a visitor's comment is governed by different rules, so the two must not be able to look the same. Whether the author is a visitor is now decided by author_id alone, the question isFromGuest() already asks.

Accepted consequence: a soft-deleted client's name is visible on their old comments during the erasure grace period, where it previously read as Anonymous. It goes for good when erasure removes the row.

Reported and fixed by @denkfabrik-li.

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)) {

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

tests/Feature/Comments/DeletedClientThreadTest.php

@@ -0,0 +1,100 @@
+<?php
+
+declare(strict_types=1);
+
+use App\Models\User;
+use App\Modules\Comments\Access\VisibleCommentScope;
+use App\Modules\Comments\CommentVisibility;
+use App\Modules\Comments\Models\FileComment;
+use App\Modules\Files\Models\File;
+use Illuminate\Support\Facades\Storage;
+
+/**
+ * `file_comments.client_context_id` and `author_id` are both
+ * cascadeOnDelete, and neither cascade ever fires: users are
+ * soft-deleted, so the row survives and the column goes on pointing at
+ * it. Everything that asked the *relation* instead of the column read
+ * that as "there is no client here" — and for client_context_id, "no
+ * client" is the branch every client on the file reads.
+ */
+beforeEach(function () {
+    Storage::fake('files');
+    $this->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();
+});