mirror of
https://github.com/projectsend/projectsend.git
synced 2026-09-16 16:45:07 +00:00
e272f19045
file_comments.client_context_id is cascadeOnDelete, but users are soft-deleted, so the cascade never fires: the column keeps pointing at a row that is still there while the Eloquent relation resolves to null. resolveClientContext branched on the relation, and a null context on a Clients comment is the branch every client on the file reads -- so a staff reply into one client's private thread became a circular to all of them, with the canAssignClient check skipped on the way. VisibleCommentScope says so in its own docblock: "A Clients comment carrying client_context_id = C is never returned to any non-staff viewer other than C ... A Clients comment with a null context is a staff message to everyone on the file, and every client with access reads it." Measured on main, with one file shared with two clients and the first of them deleted after commenting: column client_context_id 3 relation clientContext null POST reply into her thread 201, stored with client_context_id null read by the other client yes Ask the column, and refuse when the account behind it is gone. There is nobody left to answer, and the one outcome that must not follow from a filled column is the broadcast, so this throws rather than falling through to it. authorName() had the same root cause from the other column: its docblock claimed author_id cascades so there is no deleted author, and a deleted client's comment was going out as "Anonymous" -- which is what a guest comment looks like, and a guest comment is read by different rules. Guest is now decided by author_id alone, the same question isFromGuest() asks, and a trashed author is read with withTrashed(). Nothing comes back only once the grace-period erasure has removed the row for real. That read costs one query per comment whose author is trashed. Measured on a ten-comment thread: 11 queries before, 21 after, against 20 for the same thread with every author alive. Left as a lazy read rather than eager-loading with withTrashed() at every call site, because the callers would each have to remember it and the cost only applies to comments whose author is gone. Five tests, two measured red against the unfixed code (2 failed / 3 passed) -- one per column. The three that stay green either way are the branches that must not move: a staff message with no context still reaches everybody, a reply into a live client's thread still lands in that thread alone, and a genuine guest comment is still anonymous. Full suite passes (2053 passed / 2 skipped), PHPStan level 8 clean.
101 lines
3.8 KiB
PHP
101 lines
3.8 KiB
PHP
<?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();
|
|
});
|