From 5e60d2ef88a16dc50427cb784116c2d888296fbe Mon Sep 17 00:00:00 2001 From: denkfabrik-li <274324701+denkfabrik-li@users.noreply.github.com> Date: Fri, 28 Aug 2026 01:01:13 +0200 Subject: [PATCH] Say what expiry does to a client-scoped staff member's library File::isExpired() documents the rule the application is supposed to follow: once past, the file is hidden from clients and the public site "but staff keep full access to view, download, and manage it". The second half is not true of a client-scoped staff member. StaffLibraryScope::buildFiles() builds their library as their own uploads union what each assigned client may see, and that second half runs through File::scopeVisibleToClient, which ends in notExpired() -- a client-side rule. Measured on main, with a rep holding one client and a file the administrator uploaded and shared with that client: before expiry in_library true GET .../download -> 200 after expiry in_library false GET .../download -> 403 the rep's own expired upload in_library true an unscoped administrator, same expired file in_library true Api\FilesController says it the same way -- "Only the client branch of the visibility rules drops them" -- which reads as though a staff caller is unaffected, when a client-scoped one is reached through that very branch. This does not change that behaviour. c8078f65 weighed exactly this and decided against it: widening it would mean a library query that keeps expired rows, and scopeVisibleToClient is the single source of truth for client file access, the highest-stakes function to go changing for a dashboard widget. The widget was relabelled instead. That decision lives in a commit message and in one widget's label. Nothing in the code said it, and the docblock nearest the rule went on promising the opposite -- which is how the next person re-derives "staff keep full access" and widens something. So both comments now state the boundary and why it is where it is, and ExpiredFileStaffAccessTest makes it executable: an unscoped staff member keeps an expired file, a client-scoped one keeps their own expired upload, a client-scoped one loses a client's file when it expires. Not changed: scopeVisibleToClient, StaffLibraryScope, and the expired-files widget. If the boundary should move, that is a separate conversation and a separate change. Counter-check inverted, since these pass on unmodified main by construction -- there is no behaviour fix for them to prove. What they have to do is fail if the boundary moves, so the mutation is the widening itself. Deleting the closing notExpired() call from scopeVisibleToClient turns the file red, 1 failed / 2 passed, and it is the third case, the one carrying the decision, that falls. Suite 2108 passed / 2 skipped, 11416 assertions, PHPStan level 8 clean. Measured on base 06c364d2, where main itself is 2105 / 2. --- .../Http/Controllers/Api/FilesController.php | 9 ++- app/Modules/Files/Models/File.php | 16 +++- .../Files/ExpiredFileStaffAccessTest.php | 80 +++++++++++++++++++ 3 files changed, 100 insertions(+), 5 deletions(-) create mode 100644 tests/Feature/Files/ExpiredFileStaffAccessTest.php diff --git a/app/Modules/Files/Http/Controllers/Api/FilesController.php b/app/Modules/Files/Http/Controllers/Api/FilesController.php index 0f4a083f..747d4eb6 100644 --- a/app/Modules/Files/Http/Controllers/Api/FilesController.php +++ b/app/Modules/Files/Http/Controllers/Api/FilesController.php @@ -130,9 +130,12 @@ class FilesController extends Controller } // Expiry is a filter, not a default: staff see expired files in the - // UI too (that is how they notice and act on them). Only the client - // branch of the visibility rules drops them, and it does so inside - // ViewableFileScope where it belongs. + // UI too (that is how they notice and act on them). Dropping them + // is the client branch's rule, applied inside the visibility scopes + // where it belongs — which is also why a client-scoped caller does + // not get their clients' expired files back here whatever this + // filter says: their library is built on that same branch. See + // File::isExpired. if ($request->has('expired') && ($filters['expired'] ?? null) !== null) { $request->boolean('expired') ? $query->expired() : $query->notExpired(); } diff --git a/app/Modules/Files/Models/File.php b/app/Modules/Files/Models/File.php index 53802a4e..a8f67a9f 100644 --- a/app/Modules/Files/Models/File.php +++ b/app/Modules/Files/Models/File.php @@ -247,8 +247,20 @@ class File extends Model /** * A file's own expiration date — independent of any share link's. * Null means never expires. Once past, the file is hidden from - * clients and the public site (see scopeNotExpired) but staff keep - * full access to view, download, and manage it. + * clients and the public site (see scopeNotExpired) and staff keep + * full access to view, download, and manage it — with one boundary + * this used to leave out. + * + * A client-scoped staff member's library is their own uploads ∪ what + * each assigned client may see (StaffLibraryScope::buildFiles), and + * that second half is scopeVisibleToClient, which ends in + * notExpired(). So an expired file they held only through a client + * leaves their library too, while their own expired upload stays. + * That is deliberate: c8078f65 weighed widening it and left the + * boundary where it is, because scopeVisibleToClient is the single + * source of truth for client file access, and relabelled the + * expired-files widget instead. ExpiredFileStaffAccessTest pins both + * halves so the sentence above cannot drift from the code again. */ public function isExpired(): bool { diff --git a/tests/Feature/Files/ExpiredFileStaffAccessTest.php b/tests/Feature/Files/ExpiredFileStaffAccessTest.php new file mode 100644 index 00000000..5170aef3 --- /dev/null +++ b/tests/Feature/Files/ExpiredFileStaffAccessTest.php @@ -0,0 +1,80 @@ +admin = User::factory()->create(); + + $role = Role::query()->create(['name' => 'Reps '.Str::random(6), 'client_scoped' => true]); + foreach ([Permission::Upload, Permission::EditFiles] as $permission) { + RolePermission::query()->create(['role_id' => $role->id, 'permission' => $permission->value]); + } + + $this->rep = User::factory()->create(['role_id' => $role->id]); + $this->client = User::factory()->client()->create(); + $this->rep->assignedClients()->sync([$this->client->id]); +}); + +function libraryHoldsFile(User $staff, File $file): bool +{ + // The scope memoises its built query per user id, so a fresh + // container is what makes a second question in one test honest. + app()->forgetInstance(StaffLibraryScope::class); + + return app(StaffLibraryScope::class)->files($staff)->whereKey($file->id)->exists(); +} + +test('an unscoped staff member keeps a file after it expires', function () { + $file = uploadNamedFile($this->admin, 'annual-report'); + shareFileWith($file, $this->client); + + $file->forceFill(['expires_at' => now()->subDay()])->save(); + + expect(libraryHoldsFile($this->admin, $file))->toBeTrue(); + $this->actingAs($this->admin)->get("/files/{$file->id}/download")->assertOk(); +}); + +test('a client-scoped staff member keeps their own expired upload', function () { + $file = uploadNamedFile($this->rep, 'my-own-note'); + + $file->forceFill(['expires_at' => now()->subDay()])->save(); + + expect(libraryHoldsFile($this->rep, $file))->toBeTrue(); +}); + +test('a client-scoped staff member loses a client file when it expires', function () { + $file = uploadNamedFile($this->admin, 'annual-report'); + shareFileWith($file, $this->client); + + expect(libraryHoldsFile($this->rep, $file))->toBeTrue(); + $this->actingAs($this->rep)->get("/files/{$file->id}/download")->assertOk(); + + $file->forceFill(['expires_at' => now()->subDay()])->save(); + + // Deliberate, not an oversight — see the file docblock above. If this + // ever changes, File::isExpired's docblock changes with it. + expect(libraryHoldsFile($this->rep, $file))->toBeFalse(); + $this->actingAs($this->rep)->get("/files/{$file->id}/download")->assertForbidden(); +});