From 3439537efe4e02ca1d84c721d3ed35cc0311be75 Mon Sep 17 00:00:00 2001 From: denkfabrik-li <274324701+denkfabrik-li@users.noreply.github.com> Date: Wed, 26 Aug 2026 01:09:47 +0200 Subject: [PATCH] Keep comment moderation inside the moderator's own library MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FileCommentPolicy::moderate() asked only whether somebody is staff and holds moderate_comments. It never weighed the file the comment sits on, and delete() returns true the moment moderate() does — so a client-scoped moderator could delete any comment on the installation by naming its id. Three call sites already knew this and wrote the boundary out by hand, each with its own abort_unless($library->allowsFile(...), 403) after the gate. The two that did not are FileCommentsController::destroy(), web and API: both bind a comment directly, so nothing earlier in the request establishes that the viewer may see its file. The intent was documented in three places and enforced in none of them by the policy — StaffLibraryScope says "the policies consult allowsFile() so direct access respects the same boundary", VisibleCommentScope says "a moderation screen is not a way around the visibility model". Put the rule where those docblocks already say it lives. moderate() now takes the comment when there is one. Named against the class it still answers the coarser "does this user moderate at all", which is what the queue's gate and the affordances ask. Membership is tested by file id, so a file soft-deleted out from under its comments is not in a scoped moderator's library either. The author branch of delete() is deliberately untouched: deleting your own words inside the edit window is not moderation, and a client is not client-scoped in StaffLibraryScope's sense. Approving through the API now derives its 403 from Gate::authorize rather than the removed abort_unless, so the committed OpenAPI document gains the shared AuthorizationException ref in place of an inline "An error" schema — the shape nine of the other twelve documented 403s already use. --- app/Modules/Comments/FileCommentPolicy.php | 30 +++- .../Api/CommentModerationController.php | 6 +- .../Http/Controllers/CommentsController.php | 9 +- docs/api/openapi.json | 21 +-- .../Comments/ModerationLibraryScopeTest.php | 133 ++++++++++++++++++ 5 files changed, 167 insertions(+), 32 deletions(-) create mode 100644 tests/Feature/Comments/ModerationLibraryScopeTest.php diff --git a/app/Modules/Comments/FileCommentPolicy.php b/app/Modules/Comments/FileCommentPolicy.php index 67c64dee..420ef2f1 100644 --- a/app/Modules/Comments/FileCommentPolicy.php +++ b/app/Modules/Comments/FileCommentPolicy.php @@ -7,6 +7,7 @@ namespace App\Modules\Comments; use App\Models\User; use App\Modules\Comments\Access\VisibleCommentScope; use App\Modules\Comments\Models\FileComment; +use App\Modules\Files\Access\StaffLibraryScope; use Illuminate\Support\Facades\Gate; /** @@ -20,6 +21,7 @@ class FileCommentPolicy public function __construct( private readonly VisibleCommentScope $scope, private readonly CommentingRules $rules, + private readonly StaffLibraryScope $library, ) {} public function view(User $user, FileComment $comment): bool @@ -44,16 +46,38 @@ class FileCommentPolicy public function delete(User $user, FileComment $comment): bool { - if ($this->moderate($user)) { + if ($this->moderate($user, $comment)) { return true; } return $comment->author_id === $user->id && $this->withinEditWindow($comment); } - public function moderate(User $user): bool + /** + * Called both ways: with a comment, to decide about that one, and + * against the class, to ask whether this user moderates at all (the + * queue's own gate, and the affordances that offer it). + * + * The library boundary belongs here rather than in each caller. Named + * against the class it cannot be applied — there is no file to weigh — + * so that form answers the coarser question and every caller holding a + * comment should pass it. + */ + public function moderate(User $user, ?FileComment $comment = null): bool { - return $user->isStaff() && $user->can('moderate_comments'); + if (! $user->isStaff() || ! $user->can('moderate_comments')) { + return false; + } + + if ($comment === null || ! $user->isClientScoped()) { + return true; + } + + // By file id rather than through the relation: a file soft-deleted + // out from under its comments resolves to null there, and the + // answer for a scoped moderator is the same either way — it is not + // in their library. Unscoped staff never reach this line. + return $this->library->files($user)->whereKey($comment->file_id)->exists(); } private function withinEditWindow(FileComment $comment): bool diff --git a/app/Modules/Comments/Http/Controllers/Api/CommentModerationController.php b/app/Modules/Comments/Http/Controllers/Api/CommentModerationController.php index 27d524f1..fdaea2d1 100644 --- a/app/Modules/Comments/Http/Controllers/Api/CommentModerationController.php +++ b/app/Modules/Comments/Http/Controllers/Api/CommentModerationController.php @@ -70,9 +70,9 @@ class CommentModerationController extends Controller { $viewer = $request->user(); assert($viewer !== null); - Gate::forUser($viewer)->authorize('moderate', FileComment::class); - // Moderation rights are not a way around the library boundary. - abort_unless($this->library->allowsFile($viewer, $comment->file), 403); + // Moderation rights are not a way around the library boundary; the + // policy weighs the comment's file, so name the comment. + Gate::authorize('moderate', $comment); $this->comments->approve($comment, $viewer); diff --git a/app/Modules/Comments/Http/Controllers/CommentsController.php b/app/Modules/Comments/Http/Controllers/CommentsController.php index fffc0612..cf2e2ecc 100644 --- a/app/Modules/Comments/Http/Controllers/CommentsController.php +++ b/app/Modules/Comments/Http/Controllers/CommentsController.php @@ -11,7 +11,6 @@ use App\Modules\Comments\CommentPresenter; use App\Modules\Comments\CommentVisibility; use App\Modules\Comments\FileComments; use App\Modules\Comments\Models\FileComment; -use App\Modules\Files\Access\StaffLibraryScope; use App\Modules\Platform\Localization\LocalDay; use App\Modules\Platform\Localization\TimezoneRegistry; use Carbon\Carbon; @@ -44,7 +43,6 @@ class CommentsController extends Controller public function __construct( private readonly FileComments $comments, - private readonly StaffLibraryScope $library, private readonly CommentPresenter $presenter, private readonly VisibleCommentScope $scope, private readonly TimezoneRegistry $timezones, @@ -95,9 +93,9 @@ class CommentsController extends Controller { $viewer = $request->user(); assert($viewer !== null); - Gate::forUser($viewer)->authorize('moderate', FileComment::class); - // Moderation rights are not a way around the library boundary. - abort_unless($this->library->allowsFile($viewer, $comment->file), 403); + // Moderation rights are not a way around the library boundary; the + // policy weighs the comment's file, so name the comment. + Gate::forUser($viewer)->authorize('moderate', $comment); $this->comments->approve($comment, $viewer); @@ -113,7 +111,6 @@ class CommentsController extends Controller $viewer = $request->user(); assert($viewer !== null); Gate::forUser($viewer)->authorize('delete', $comment); - abort_unless($this->library->allowsFile($viewer, $comment->file), 403); $this->comments->remove($comment); diff --git a/docs/api/openapi.json b/docs/api/openapi.json index dd322939..0b8102c2 100644 --- a/docs/api/openapi.json +++ b/docs/api/openapi.json @@ -1140,26 +1140,7 @@ } }, "403": { - "description": "An error", - "content": { - "application/json": { - "schema": { - "type": "object", - "properties": { - "message": { - "type": "string", - "description": "Error overview.", - "examples": [ - "" - ] - } - }, - "required": [ - "message" - ] - } - } - } + "$ref": "#/components/responses/AuthorizationException" }, "404": { "$ref": "#/components/responses/ModelNotFoundException" diff --git a/tests/Feature/Comments/ModerationLibraryScopeTest.php b/tests/Feature/Comments/ModerationLibraryScopeTest.php new file mode 100644 index 00000000..73a85f00 --- /dev/null +++ b/tests/Feature/Comments/ModerationLibraryScopeTest.php @@ -0,0 +1,133 @@ +admin = User::factory()->create(); + $this->settings = app(Settings::class); + + $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::CommentsEditWindowMinutes, 15); +}); + +/** + * A moderator who is scoped to their own clients — the role that holds + * moderate_comments without holding the whole library. + */ +function commentModeratorScopedTo(?User $client = null): User +{ + $role = Role::query()->create(['name' => 'Scoped moderator', 'client_scoped' => true]); + RolePermission::query()->insert([ + ['role_id' => $role->id, 'permission' => 'moderate_comments'], + ['role_id' => $role->id, 'permission' => 'upload'], + ]); + + $manager = User::factory()->create(['role_id' => $role->id]); + $manager->assignedClients()->attach(($client ?? User::factory()->client()->create())->id); + + return $manager; +} + +/** + * A commented-on file belonging to somebody else's client. + * + * @return array{0: File, 1: FileComment} + */ +function strangersCommentedFile(User $uploader): array +{ + $stranger = User::factory()->client()->create(); + $file = File::factory()->public()->create(['uploaded_by' => $uploader->id]); + shareFileWith($file, $stranger); + + return [$file, FileComment::factory()->for($file)->fromGuest()->pending()->create()]; +} + +test('a scoped moderator cannot delete a comment on a file outside their library', function () { + [, $comment] = strangersCommentedFile($this->admin); + + // The per-file endpoint, not the moderation screen: the policy is the + // whole gate here, since the route carries no permission of its own. + $this->actingAs(commentModeratorScopedTo()) + ->deleteJson("/comments/{$comment->id}") + ->assertForbidden(); + + expect(FileComment::query()->whereKey($comment->id)->exists())->toBeTrue(); +}); + +test('a scoped moderator cannot delete an out-of-library comment through the API', function () { + [, $comment] = strangersCommentedFile($this->admin); + + Sanctum::actingAs(commentModeratorScopedTo(), ['upload']); + + $this->deleteJson("/api/v1/comments/{$comment->id}")->assertForbidden(); + + expect(FileComment::query()->whereKey($comment->id)->exists())->toBeTrue(); +}); + +test('a scoped moderator cannot delete an out-of-library comment from the moderation screen', function () { + [, $comment] = strangersCommentedFile($this->admin); + + $this->actingAs(commentModeratorScopedTo()) + ->delete("/comments/{$comment->id}/moderate") + ->assertForbidden(); + + expect(FileComment::query()->whereKey($comment->id)->exists())->toBeTrue(); +}); + +test('a scoped moderator cannot approve an out-of-library comment through the API', function () { + [, $comment] = strangersCommentedFile($this->admin); + + Sanctum::actingAs(commentModeratorScopedTo(), ['moderate_comments']); + + $this->postJson("/api/v1/comments/{$comment->id}/approve")->assertForbidden(); + + expect($comment->fresh()->isPending())->toBeTrue(); +}); + +test('a scoped moderator still moderates the files that are theirs', function () { + $mine = User::factory()->client()->create(); + $file = File::factory()->public()->create(['uploaded_by' => $this->admin->id]); + shareFileWith($file, $mine); + $comment = FileComment::factory()->for($file)->fromGuest()->pending()->create(); + + // Same role, same permission — the only difference is that this file + // belongs to a client they are assigned to. + $this->actingAs(commentModeratorScopedTo($mine)) + ->deleteJson("/comments/{$comment->id}") + ->assertOk(); + + expect(FileComment::query()->whereKey($comment->id)->exists())->toBeFalse(); +}); + +test('an unscoped moderator still reaches every comment', function () { + [, $comment] = strangersCommentedFile($this->admin); + + $this->actingAs($this->admin)->deleteJson("/comments/{$comment->id}")->assertOk(); + + expect(FileComment::query()->whereKey($comment->id)->exists())->toBeFalse(); +}); + +test('the library boundary does not follow an author onto their own comment', function () { + $client = User::factory()->client()->create(); + $file = File::factory()->public()->create(['uploaded_by' => $this->admin->id]); + shareFileWith($file, $client); + $comment = FileComment::factory()->for($file)->inThreadOf($client)->create(['author_id' => $client->id]); + + // Deleting your own words inside the edit window is not moderation, and + // the boundary the moderation branch now carries must not reach it. + $this->actingAs($client)->deleteJson("/comments/{$comment->id}")->assertOk(); + + expect(FileComment::query()->whereKey($comment->id)->exists())->toBeFalse(); +});