Merge pull request #1745 from denkfabrik-li/fix/moderation-view-read-permission

FilePolicy::view() has two halves for a staff member: one of the three file keys (upload / edit_files / edit_others_files), AND StaffLibraryScope. Every comment surface that spans files narrowed by the library half alone -- VisibleCommentScope::across(), pendingTotal(), and the API's GET /comments/pending. A role holding moderate_comments and no file key at all therefore read, on /comments, every comment in the installation: the text, staff-only notes, the client's name in conversation, and a visitor's IP, while getting a 403 on every file those comments were about. POST /api/v1/comments/{id}/approve was the same door on the write side, and its response carries the comment body, so an id was enough to read one.

The project already states the rule this breaks in four places, including across()'s own docblock -- "a moderation screen is not a way around the visibility model: moderating means deciding about comments you can already see" -- and only the cross-file queries did not ask it.

The cross-file queries now take their files from ViewableFileScope, which is FilePolicy::view() expressed as a query and already in the codebase for exactly this, instead of from StaffLibraryScope, which is only its second half. The permission half becomes a named method there, permitsAnyFile(), because three modules now ask it, and FileCommentPolicy::moderate() asks it in both of its forms. This is the other half of #1698, which library-scoped the same screen: library is not readability.

Verified before merging: tests/Feature/Comments at 180 passed on the trial-merge; with app/ reset and the new test file kept, 5 failed / 2 passed. The two green either way are the right two -- the premise, that the file itself 403s for this viewer, and the guard that a moderator who does hold a file key still moderates the whole installation.

Compatibility was the question worth asking, and it is clean: the only shipped roles holding moderate_comments are Account Manager, which also holds Upload, EditFiles and EditOthersFiles, and System Administrator, which holds everything. No shipped role loses moderation. The only configuration whose behaviour changes is a custom role granting moderate_comments with no file key, which is precisely the leaking one.

This PR also edits docs/api/openapi.json, which #1727 edited too, so the merged result was checked rather than trusted: scramble:export on the merged tree reproduces the committed file byte for byte, with both endpoints' descriptions present.

Reported and fixed by @denkfabrik-li.

app/Modules/Comments/Access/VisibleCommentScope.php

@@ -10,6 +10,7 @@ use App\Modules\Comments\GuestCommentIdentity;
 use App\Modules\Comments\Models\FileComment;
 use App\Modules\Files\Access\ShareTargets;
 use App\Modules\Files\Access\StaffLibraryScope;
+use App\Modules\Files\Access\ViewableFileScope;
 use App\Modules\Files\Models\File;
 use App\Modules\Files\Models\Folder;
 use App\Modules\Identity\UserType;
@@ -51,6 +52,7 @@ class VisibleCommentScope
 {
     public function __construct(
         private readonly StaffLibraryScope $scope,
+        private readonly ViewableFileScope $viewable,
         private readonly ShareTargets $shareTargets,
         private readonly GuestCommentIdentity $guests,
     ) {}
@@ -123,6 +125,13 @@ class VisibleCommentScope
      * way around the visibility model** — moderating means deciding about
      * comments you can already see.
      *
+     * Which is why the files come from ViewableFileScope rather than from
+     * StaffLibraryScope: FilePolicy::view() is a permission half AND a
+     * library half, and narrowing by the library alone would hand every
+     * comment in the installation to a role holding moderate_comments and
+     * none of the three file keys — somebody who gets a 403 on every file
+     * these comments are about.
+     *
      * Staff only. A client has no cross-file view of comments and asking
      * for one is a mistake rather than an empty result, but returning
      * nothing is the safe way to be wrong.
@@ -136,7 +145,7 @@ class VisibleCommentScope
         }
 
         return $this->applyVisibility(
-            FileComment::query()->whereIn('file_id', $this->scope->files($viewer)->select('files.id')),
+            FileComment::query()->whereIn('file_id', $this->viewable->for($viewer)->select('files.id')),
             $viewer,
             // Publicness is a property of each file, so it cannot be one
             // value for a query spanning many. It does not have to be: the
@@ -156,6 +165,12 @@ class VisibleCommentScope
      * than about what this viewer may read, and a moderator who cannot see
      * a particular client's thread must still be told the file has
      * something waiting.
+     *
+     * The file boundary is still the same one, though. ViewableFileScope
+     * rather than StaffLibraryScope: which files is the part that varies
+     * per client, whether any is the part that does not, and a badge
+     * counting the whole installation for somebody who may open none of it
+     * is a number about other people's files.
      */
     public function pendingTotal(User $viewer): int
     {
@@ -165,7 +180,7 @@ class VisibleCommentScope
 
         return FileComment::query()
             ->whereNull('approved_at')
-            ->whereIn('file_id', $this->scope->files($viewer)->select('files.id'))
+            ->whereIn('file_id', $this->viewable->for($viewer)->select('files.id'))
             ->count();
     }
 

app/Modules/Comments/FileCommentPolicy.php

@@ -8,6 +8,7 @@ use App\Models\User;
 use App\Modules\Comments\Access\VisibleCommentScope;
 use App\Modules\Comments\Models\FileComment;
 use App\Modules\Files\Access\StaffLibraryScope;
+use App\Modules\Files\Access\ViewableFileScope;
 use Illuminate\Support\Facades\Gate;
 
 /**
@@ -22,6 +23,7 @@ class FileCommentPolicy
         private readonly VisibleCommentScope $scope,
         private readonly CommentingRules $rules,
         private readonly StaffLibraryScope $library,
+        private readonly ViewableFileScope $viewable,
     ) {}
 
     public function view(User $user, FileComment $comment): bool
@@ -69,6 +71,15 @@ class FileCommentPolicy
             return false;
         }
 
+        // Moderating is deciding about comments you can already see, so the
+        // permission half of file reading is part of the answer in both
+        // forms. Without one of the three file keys this user gets a 403 on
+        // every file these comments are about, and approving one hands back
+        // its body — so this is a reading door, not only a writing one.
+        if (! $this->viewable->permitsAnyFile($user)) {
+            return false;
+        }
+
         if ($comment === null || ! $user->isClientScoped()) {
             return true;
         }

app/Modules/Comments/Http/Controllers/Api/CommentModerationController.php

@@ -8,7 +8,7 @@ use App\Http\Controllers\Controller;
 use App\Modules\Comments\FileComments;
 use App\Modules\Comments\Http\Resources\Api\FileCommentResource;
 use App\Modules\Comments\Models\FileComment;
-use App\Modules\Files\Access\StaffLibraryScope;
+use App\Modules\Files\Access\ViewableFileScope;
 use Illuminate\Http\Request;
 use Illuminate\Http\Resources\Json\AnonymousResourceCollection;
 use Illuminate\Support\Facades\Gate;
@@ -30,16 +30,17 @@ class CommentModerationController extends Controller
 {
     public function __construct(
         private readonly FileComments $comments,
-        private readonly StaffLibraryScope $library,
+        private readonly ViewableFileScope $viewable,
     ) {}
 
     /**
      * List comments awaiting approval.
      *
-     * Scoped by the same library boundary as everything else: a
-     * client-scoped token sees pending comments only on files its owner
-     * could already open. Oldest first, so working through the list means
-     * working through the backlog.
+     * Scoped by the same file boundary as everything else — the whole of
+     * it, not just its library half: a client-scoped token sees pending
+     * comments only on files its owner could already open, and a token
+     * whose owner holds no file key at all sees none. Oldest first, so
+     * working through the list means working through the backlog.
      */
     public function index(Request $request): AnonymousResourceCollection
     {
@@ -49,7 +50,7 @@ class CommentModerationController extends Controller
 
         $pending = FileComment::query()
             ->whereNull('approved_at')
-            ->whereIn('file_id', $this->library->files($viewer)->select('id'))
+            ->whereIn('file_id', $this->viewable->for($viewer)->select('id'))
             ->with(['author', 'clientContext'])
             ->orderBy('created_at')
             ->orderBy('id')

app/Modules/Files/Access/ViewableFileScope.php

@@ -40,15 +40,25 @@ class ViewableFileScope
             return File::query()->visibleToClient($user);
         }
 
-        // Mirrors FilePolicy::view()'s staff branch: the permission half is
-        // a property of the viewer, not the row, so it either opens the
-        // whole scope or closes it entirely.
-        $permitted = $user->can('upload') || $user->can('edit_files') || $user->can('edit_others_files');
-
-        if (! $permitted) {
+        if (! $this->permitsAnyFile($user)) {
             return File::query()->whereRaw('1 = 0');
         }
 
         return $this->scope->files($user);
     }
+
+    /**
+     * Whether a staff member holds any of the three keys that open file
+     * reading at all — the permission half of FilePolicy::view()'s staff
+     * branch, named once because more than one module has to ask it.
+     *
+     * It is a property of the viewer rather than of a row, so it either
+     * opens the whole scope or closes it entirely. That is also why a
+     * query narrowed by StaffLibraryScope alone is only half the check:
+     * the library says *which* files, this says *whether any*.
+     */
+    public function permitsAnyFile(User $user): bool
+    {
+        return $user->can('upload') || $user->can('edit_files') || $user->can('edit_others_files');
+    }
 }

docs/api/openapi.json

@@ -1068,7 +1068,7 @@
         "/comments/pending": {
             "get": {
                 "operationId": "comments.pending",
-                "description": "Scoped by the same library boundary as everything else: a\nclient-scoped token sees pending comments only on files its owner\ncould already open. Oldest first, so working through the list means\nworking through the backlog.\n\nRequires a token with the ability: `moderate_comments`.",
+                "description": "Scoped by the same file boundary as everything else \u2014 the whole of\nit, not just its library half: a client-scoped token sees pending\ncomments only on files its owner could already open, and a token\nwhose owner holds no file key at all sees none. Oldest first, so\nworking through the list means working through the backlog.\n\nRequires a token with the ability: `moderate_comments`.",
                 "summary": "List comments awaiting approval",
                 "tags": [
                     "CommentModeration"

tests/Feature/Comments/ModerationReadPermissionTest.php

@@ -0,0 +1,146 @@
+<?php
+
+declare(strict_types=1);
+
+use App\Models\User;
+use App\Modules\Comments\Models\FileComment;
+use App\Modules\Files\Models\File;
+use App\Modules\Identity\Models\Role;
+use App\Modules\Identity\Models\RolePermission;
+use App\Modules\Platform\Settings\Setting;
+use App\Modules\Platform\Settings\Settings;
+use Laravel\Sanctum\Sanctum;
+
+/*
+|--------------------------------------------------------------------------
+| Moderating is not a way to read
+|--------------------------------------------------------------------------
+|
+| FilePolicy::view() has two halves for staff: one of the three file keys
+| (upload / edit_files / edit_others_files), AND the library scope. The
+| comment surfaces narrowed by the library half alone, so a role holding
+| moderate_comments and nothing else got a 403 on every file while reading
+| every comment written about them — text, staff-only notes, the client
+| name in `conversation`, and a visitor's IP.
+|
+*/
+
+beforeEach(function () {
+    $this->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);
+});
+
+/** A moderator holding moderate_comments and no file key whatsoever. */
+function moderatorWithoutFileKeys(): User
+{
+    $role = Role::query()->create(['name' => 'Comment moderator']);
+    RolePermission::query()->insert([
+        ['role_id' => $role->id, 'permission' => 'moderate_comments'],
+    ]);
+
+    return User::factory()->create(['role_id' => $role->id]);
+}
+
+/** A moderator who may also read files — the shipped Account Manager shape. */
+function moderatorWithFileKeys(): User
+{
+    $role = Role::query()->create(['name' => 'Account manager shape']);
+    RolePermission::query()->insert([
+        ['role_id' => $role->id, 'permission' => 'moderate_comments'],
+        ['role_id' => $role->id, 'permission' => 'upload'],
+    ]);
+
+    return User::factory()->create(['role_id' => $role->id]);
+}
+
+/** @return array{0: File, 1: FileComment} */
+function commentedFile(User $uploader): array
+{
+    $file = File::factory()->public()->create(['uploaded_by' => $uploader->id, 'name' => 'merger-terms']);
+
+    return [$file, FileComment::factory()->for($file)->fromGuest()->pending()->create()];
+}
+
+test('a moderator who may read no file is refused the file itself', function () {
+    // The premise the rest of this file rests on, measured rather than
+    // assumed: the door next to the comment is shut for this viewer.
+    [$file] = commentedFile($this->admin);
+
+    $this->actingAs(moderatorWithoutFileKeys())
+        ->get("/files/{$file->id}/download")
+        ->assertForbidden();
+});
+
+test('the moderation screen shows no comment about a file the viewer cannot open', function () {
+    [, $comment] = commentedFile($this->admin);
+
+    $props = $this->actingAs(moderatorWithoutFileKeys())
+        ->get('/comments')->assertOk()->viewData('page')['props'];
+
+    expect(collect($props['entries'])->pluck('id')->all())->not->toContain($comment->id)
+        ->and($props['entries'])->toBe([]);
+});
+
+test('the pending badge counts nothing a viewer may not open', function () {
+    commentedFile($this->admin);
+
+    $props = $this->actingAs(moderatorWithoutFileKeys())
+        ->get('/comments')->assertOk()->viewData('page')['props'];
+
+    expect($props['pending_total'])->toBe(0)
+        ->and($props['pending']['comments'] ?? 0)->toBe(0);
+});
+
+test('the API queue refuses a moderator without a file key', function () {
+    // The class form of the policy answers "does this user moderate at
+    // all", and without a file key the answer is no — so the queue is
+    // refused rather than returned empty, exactly as it is for somebody
+    // holding no moderation permission.
+    commentedFile($this->admin);
+
+    Sanctum::actingAs(moderatorWithoutFileKeys(), ['moderate_comments']);
+
+    $this->getJson('/api/v1/comments/pending')->assertForbidden();
+});
+
+test('approving is refused, so the response cannot hand back the body', function () {
+    [, $comment] = commentedFile($this->admin);
+
+    Sanctum::actingAs(moderatorWithoutFileKeys(), ['moderate_comments']);
+
+    $this->postJson("/api/v1/comments/{$comment->id}/approve")->assertForbidden();
+
+    expect($comment->fresh()->isPending())->toBeTrue();
+});
+
+test('deleting from the moderation screen is refused too', function () {
+    [, $comment] = commentedFile($this->admin);
+
+    $this->actingAs(moderatorWithoutFileKeys())
+        ->delete("/comments/{$comment->id}/moderate")
+        ->assertForbidden();
+
+    expect(FileComment::query()->whereKey($comment->id)->exists())->toBeTrue();
+});
+
+test('a moderator who may read files still moderates the whole installation', function () {
+    // Guards against narrowing by more than the permission half: this role
+    // is unscoped, so every comment stays in reach.
+    [, $comment] = commentedFile($this->admin);
+
+    $moderator = moderatorWithFileKeys();
+
+    $props = $this->actingAs($moderator)->get('/comments')->assertOk()->viewData('page')['props'];
+
+    expect(collect($props['entries'])->pluck('id')->all())->toContain($comment->id)
+        ->and($props['pending_total'])->toBe(1);
+
+    $this->actingAs($moderator)->delete("/comments/{$comment->id}/moderate")->assertRedirect();
+
+    expect(FileComment::query()->whereKey($comment->id)->exists())->toBeFalse();
+});