From 0a8b609e8b39c282a856b9e6efedca85e2c3d8d2 Mon Sep 17 00:00:00 2001 From: ignacionelson Date: Wed, 26 Aug 2026 13:30:50 -0300 Subject: [PATCH] Build a scoped staff member's library query once per request, not once per row MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #1698 moved the library boundary into FileCommentPolicy, where it belongs, and said plainly what that cost: the moderation screen went from 65 queries to 465 for a client-scoped moderator with five assigned clients. Measured here, those numbers are exactly right. The cost is not in asking. It is that StaffLibraryScope::files() rebuilds its query every time, and building one runs four immediate lookups per assigned client — the client's group ids, the same ids again inside Folder::sharedFolderIds(), that method's own assignment lookup, and the shared-folder get() in Folder::scopeVisibleToClient(). None of them depend on the query being built. Gate resolves a fresh policy for every check, so a listing paid for all of it once per row. The built query is now memoised per user and handed back as a clone, since every caller adds to it, and the scope is registered as `scoped` rather than transient so the memo survives a request. Scoped rather than a singleton on purpose: a long-lived queue worker keeps singletons between jobs, and a library query built from one job's data has no business answering the next one's question. That is 465 queries down to 60 on the same page — below the 65 it cost before #1698, because the memo also helps the callers that were already asking repeatedly. FileVersions::sharedAudience(), which runs the same helper twice per candidate while resolving notification recipients, gets it for free. So the answer to the question #1698 left open is neither of the two it offered. can_delete stays a real question asked of the policy; nothing restates the boundary; and the page is faster than it was before the fix. Three tests: one user's query never answers another's, one caller's constraints never follow the next, and the moderation screen does not ask once per row. --- CHANGELOG.md | 12 ++ .../Files/Access/StaffLibraryScope.php | 35 ++++++ app/Modules/Files/FilesServiceProvider.php | 12 ++ .../Files/StaffLibraryScopeReuseTest.php | 108 ++++++++++++++++++ 4 files changed, 167 insertions(+) create mode 100644 tests/Feature/Files/StaffLibraryScopeReuseTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index c2076596..8d9295ef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -171,6 +171,18 @@ a version is cut. (found, diagnosed and fixed by [@denkfabrik-li](https://github.com/denkfabrik-li) in [#1687](https://github.com/projectsend/projectsend/pull/1687)) +- **Comment moderation now stops at the same boundary everything else does.** A staff role can be + limited to its own assigned clients, and everything in the library respects that — listings, + downloads, file details, and the moderation queue itself. Deleting or approving a single comment + did not. Someone with a client-limited role who also held the comment moderation permission could + remove any comment on the installation by its id, including conversations belonging to clients + they were not assigned to, on files they could not open. No role that ships with ProjectSend + combines those two things, so this needed a custom role to reach; if you have built one, it is + worth updating for. The boundary now lives in the rule itself rather than being restated by each + screen, which is how the gap opened in the first place. + (found, diagnosed and fixed by [@denkfabrik-li](https://github.com/denkfabrik-li) in + [#1698](https://github.com/projectsend/projectsend/pull/1698)) + ## 2.1.0 — 18 August 2026 Updating, mostly. ProjectSend now tells you when there is a new version, ends an update somewhere diff --git a/app/Modules/Files/Access/StaffLibraryScope.php b/app/Modules/Files/Access/StaffLibraryScope.php index 491e3726..8300bd20 100644 --- a/app/Modules/Files/Access/StaffLibraryScope.php +++ b/app/Modules/Files/Access/StaffLibraryScope.php @@ -26,10 +26,37 @@ use Illuminate\Database\Eloquent\Builder; */ class StaffLibraryScope { + /** + * Built queries, by user id. Building one is not free: it walks the + * assigned clients and File::scopeVisibleToClient runs four immediate + * lookups for each of them, none of which depend on the query being + * built. Callers ask over and over — the policies ask once per row on + * a listing, and Gate resolves a fresh policy for every check — so the + * same handful of lookups were being repeated per row. + * + * A clone goes back rather than the query itself, since every caller + * adds to it. Registered with the container as `scoped`, so the memo + * lasts a request and is dropped between queue jobs. + * + * @var array> + */ + private array $files = []; + + /** @var array> */ + private array $folders = []; + /** * @return Builder */ public function files(User $user): Builder + { + return clone ($this->files[$user->id] ??= $this->buildFiles($user)); + } + + /** + * @return Builder + */ + private function buildFiles(User $user): Builder { $query = File::query(); @@ -53,6 +80,14 @@ class StaffLibraryScope * @return Builder */ public function folders(User $user): Builder + { + return clone ($this->folders[$user->id] ??= $this->buildFolders($user)); + } + + /** + * @return Builder + */ + private function buildFolders(User $user): Builder { $query = Folder::query(); diff --git a/app/Modules/Files/FilesServiceProvider.php b/app/Modules/Files/FilesServiceProvider.php index f750b6de..0087077a 100644 --- a/app/Modules/Files/FilesServiceProvider.php +++ b/app/Modules/Files/FilesServiceProvider.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace App\Modules\Files; +use App\Modules\Files\Access\StaffLibraryScope; use App\Modules\Files\Models\File; use App\Modules\Files\Models\Folder; use App\Modules\Files\Notifications\FileShareDigestNotification; @@ -20,6 +21,17 @@ use Illuminate\Support\ServiceProvider; class FilesServiceProvider extends ServiceProvider { + public function register(): void + { + // Scoped rather than transient: the library query it builds costs + // several lookups per assigned client, and the policies ask for it + // once per row on a listing — Gate resolves a fresh policy for + // every check, so without this the instance memo would never be + // reached twice. Scoped rather than a singleton so a long-lived + // queue worker starts each job with an empty memo. + $this->app->scoped(StaffLibraryScope::class); + } + public function boot(): void { Gate::policy(File::class, FilePolicy::class); diff --git a/tests/Feature/Files/StaffLibraryScopeReuseTest.php b/tests/Feature/Files/StaffLibraryScopeReuseTest.php new file mode 100644 index 00000000..4d0c4ea0 --- /dev/null +++ b/tests/Feature/Files/StaffLibraryScopeReuseTest.php @@ -0,0 +1,108 @@ +admin = User::factory()->create(); + + $settings = app(Settings::class); + $settings->set(Setting::CommentsScope, 'all'); + $settings->set(Setting::CommentsAuthors, 'everyone'); +}); + +function scopedModeratorWithLibrary(int $clients, int $filesEach): User +{ + $role = Role::query()->create(['name' => 'Scoped moderator '.Role::query()->count(), '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]); + + for ($i = 0; $i < $clients; $i++) { + $client = User::factory()->client()->create(); + $manager->assignedClients()->attach($client->id); + + for ($f = 0; $f < $filesEach; $f++) { + $file = File::factory()->public()->create(['uploaded_by' => $manager->id]); + shareFileWith($file, $client); + FileComment::factory()->for($file)->fromGuest()->pending()->create(); + } + } + + return $manager; +} + +test('what one scoped user may see is never handed to another', function () { + $scope = app(StaffLibraryScope::class); + + $mine = User::factory()->client()->create(); + $theirs = User::factory()->client()->create(); + + $me = scopedModeratorWithLibrary(0, 0); + $me->assignedClients()->attach($mine->id); + $them = scopedModeratorWithLibrary(0, 0); + $them->assignedClients()->attach($theirs->id); + + $myFile = File::factory()->public()->create(['uploaded_by' => $this->admin->id]); + shareFileWith($myFile, $mine); + + expect($scope->files($me)->whereKey($myFile->id)->exists())->toBeTrue() + ->and($scope->files($them)->whereKey($myFile->id)->exists())->toBeFalse() + // Asked again, in the other order: a memo that answered from the + // wrong user's query would only show up on the second call. + ->and($scope->files($them)->whereKey($myFile->id)->exists())->toBeFalse() + ->and($scope->files($me)->whereKey($myFile->id)->exists())->toBeTrue(); +}); + +test('what one caller adds to the query does not follow the next one', function () { + $scope = app(StaffLibraryScope::class); + + $client = User::factory()->client()->create(); + $manager = scopedModeratorWithLibrary(0, 0); + $manager->assignedClients()->attach($client->id); + + $first = File::factory()->public()->create(['uploaded_by' => $this->admin->id]); + $second = File::factory()->public()->create(['uploaded_by' => $this->admin->id]); + shareFileWith($first, $client); + shareFileWith($second, $client); + + // Narrow one copy to a single file, then ask again from scratch. + expect($scope->files($manager)->whereKey($first->id)->count())->toBe(1) + ->and($scope->files($manager)->count())->toBe(2); +}); + +test('the moderation screen does not ask the library once per row', function () { + $manager = scopedModeratorWithLibrary(clients: 5, filesEach: 5); + + $queries = 0; + DB::listen(function () use (&$queries): void { + $queries++; + }); + + $this->actingAs($manager)->get('/comments')->assertOk(); + + // Twenty-five rows, five assigned clients. Built once per request the + // page costs about sixty queries; built per row it was 465. The bound + // is deliberately loose — it is here to catch the shape coming back, + // not to pin an exact number. + expect($queries)->toBeLessThan(150); +});