gsadmin/projectsend
1
0
Fork 0
mirror of https://github.com/projectsend/projectsend.git synced 2026-09-28 23:00:49 +00:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #1695 from denkfabrik-li/fix/public-comment-thread-scope

Browse Source 
Serve the public comment thread to the public, whoever happens to be logged in
This commit is contained in:
Ignacio Nelson
2026-08-26 22:35:13 -03:00
committed by GitHub
parent f12692520a e3554bdd39
commit 4cb46c954f
5 changed files with 318 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files
app/Modules/Comments/Access/VisibleCommentScope.php
+72 -13
View File
@@ -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<FileComment>
*/
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<FileComment> $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<FileComment> $query
* @return Builder<FileComment>
*/
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
app/Modules/Comments/CommentPresenter.php
+12 -2
View File
@@ -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<array<string, mixed>>, can_comment: bool, cannot_comment_reason: string|null, is_guest: bool, guest_moderated: bool, captcha_required: bool, visibilities: list<array<string, mixed>>, 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')
app/Modules/Comments/Http/Controllers/FileCommentsController.php
+27 -2
View File
@@ -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<string, mixed>
*/
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<string, mixed>
*/
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
app/Modules/Comments/Http/Controllers/PublicFileCommentsController.php
+28 -2
View File
@@ -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<string, mixed>
*/
private function thread(?User $viewer, File $file): array
{
return $this->presenter->thread(
$viewer,
$file,
viewerMaySeeFile: $viewer !== null && Gate::forUser($viewer)->allows('view', $file),
);
}
/**
tests/Feature/Comments/PublicThreadScopeTest.php
+179
View File
@@ -0,0 +1,179 @@
<?php
declare(strict_types=1);
use App\Models\User;
use App\Modules\Comments\Models\FileComment;
use App\Modules\Files\Models\File;
use App\Modules\Groups\Models\Group;
use App\Modules\Identity\Permissions\SystemRole;
use App\Modules\Platform\Settings\Setting;
use App\Modules\Platform\Settings\Settings;
/**
* What the public listing's comment endpoint may serve a reader who
* happens to be signed in — and what it may not, because being signed in
* is not the same as being allowed to open the file.
*/
beforeEach(function () {
$this->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');
});