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