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(); +});