diff --git a/app/Modules/Files/Http/Controllers/Api/FilesController.php b/app/Modules/Files/Http/Controllers/Api/FilesController.php index 7362aaad..0f4a083f 100644 --- a/app/Modules/Files/Http/Controllers/Api/FilesController.php +++ b/app/Modules/Files/Http/Controllers/Api/FilesController.php @@ -12,6 +12,7 @@ use App\Modules\Audit\ActivityLogger; use App\Modules\Clients\ClientStorageUsage; use App\Modules\Comments\CommentingRules; use App\Modules\Comments\CommentScope; +use App\Modules\Files\Access\StaffLibraryScope; use App\Modules\Files\Access\ViewableFileScope; use App\Modules\Files\DownloadLimitScope; use App\Modules\Files\Http\Resources\Api\FileResource; @@ -57,6 +58,7 @@ class FilesController extends Controller private readonly ClientStorageUsage $storageUsage, private readonly ActivityLogger $activity, private readonly CommentingRules $commenting, + private readonly StaffLibraryScope $scope, ) {} /** @@ -286,6 +288,21 @@ class FilesController extends Controller 'download_limit_scope' => ['sometimes', Rule::enum(DownloadLimitScope::class)], ]); + // Reparenting through update() must respect the same library scope as + // the web move()/bulkUpdate() paths: the destination folder must be + // one this user can see. Only enforced when folder_id actually + // changes, so re-saving a file that already sits in an out-of-scope + // folder (reachable via a direct client share) still works. The + // integer rule admits numeric strings, so cast before the strict + // change comparison. + if (array_key_exists('folder_id', $validated) && $validated['folder_id'] !== null) { + $validated['folder_id'] = (int) $validated['folder_id']; + + if ($validated['folder_id'] !== $file->folder_id) { + $this->scope->folders($user)->findOrFail($validated['folder_id']); + } + } + $attributes = array_intersect_key($validated, array_flip(['name', 'description', 'folder_id'])); if (array_key_exists('expires_at', $validated) && $user->can('set_file_expiration_date')) { diff --git a/app/Modules/Files/Http/Controllers/FilesController.php b/app/Modules/Files/Http/Controllers/FilesController.php index b74c2cfd..ba45bba8 100644 --- a/app/Modules/Files/Http/Controllers/FilesController.php +++ b/app/Modules/Files/Http/Controllers/FilesController.php @@ -255,10 +255,25 @@ class FilesController extends Controller 'download_limit_scope' => ['nullable', Rule::enum(DownloadLimitScope::class)], ]); + // The edit form posts folder_id as a string; cast so the strict + // change comparison below matches the model's int. + $folderId = isset($validated['folder_id']) ? (int) $validated['folder_id'] : null; + $user = $request->user(); + + // Reparenting through update() is the same privileged write as + // move()/bulkUpdate(), so it needs the same guard: the destination + // must be a folder this user can actually see. Only checked when the + // folder actually changes, so re-saving a file that already sits in + // an out-of-scope folder (reachable via a direct client share) still + // works. + if ($folderId !== null && $folderId !== $file->folder_id && $user !== null) { + $this->scope->folders($user)->findOrFail($folderId); + } + $attributes = [ 'name' => $validated['name'], 'description' => $validated['description'] ?? null, - 'folder_id' => $validated['folder_id'] ?? null, + 'folder_id' => $folderId, ]; // Only meaningful while the comment scope is `selected`, and only diff --git a/tests/Feature/Files/FileUpdateFolderScopeTest.php b/tests/Feature/Files/FileUpdateFolderScopeTest.php new file mode 100644 index 00000000..ddc26996 --- /dev/null +++ b/tests/Feature/Files/FileUpdateFolderScopeTest.php @@ -0,0 +1,106 @@ +admin = User::factory()->create(); + + // A client-scoped staff member (Client Manager holds edit_files) assigned + // to exactly one client. + $this->mine = User::factory()->client()->create(); + $this->manager = User::factory()->role(SystemRole::ClientManager)->create(); + $this->manager->assignedClients()->sync([$this->mine->id]); + + // A folder shared with a *different* client — outside the manager's scope. + $this->outsider = User::factory()->client()->create(); + $this->outOfScopeFolder = app(FolderService::class)->create('Outsider Folder', null); + $this->actingAs($this->admin) + ->post("/folders/{$this->outOfScopeFolder->id}/assignments", ['type' => 'client', 'id' => $this->outsider->id]); + + // A folder shared with the manager's own client — inside their scope. + $this->inScopeFolder = app(FolderService::class)->create('My Client Folder', null); + $this->actingAs($this->admin) + ->post("/folders/{$this->inScopeFolder->id}/assignments", ['type' => 'client', 'id' => $this->mine->id]); +}); + +test('web update() refuses to reparent a file into an out-of-scope folder', function () { + $file = File::factory()->create(['name' => 'Report', 'uploaded_by' => $this->manager->id]); + + $this->actingAs($this->manager) + ->patch("/files/{$file->id}", ['name' => 'Report', 'folder_id' => $this->outOfScopeFolder->id]) + ->assertNotFound(); + + expect($file->fresh()->folder_id)->toBeNull(); +}); + +test('web update() still allows reparenting into an in-scope folder', function () { + $file = File::factory()->create(['name' => 'Report', 'uploaded_by' => $this->manager->id]); + + $this->actingAs($this->manager) + ->patch("/files/{$file->id}", ['name' => 'Report', 'folder_id' => $this->inScopeFolder->id]) + ->assertRedirect(); + + expect($file->fresh()->folder_id)->toBe($this->inScopeFolder->id); +}); + +test('web update() may still re-save a file that already sits in an out-of-scope folder without moving it', function () { + // The manager owns this file (so may edit it) but it lives in a folder + // outside their scope. Re-saving without changing folder_id must not trip + // the scope guard — only an actual move is checked. folder_id is sent as + // a string here because that is what the edit form posts. + $file = File::factory()->create([ + 'name' => 'Report', + 'uploaded_by' => $this->manager->id, + 'folder_id' => $this->outOfScopeFolder->id, + ]); + + $this->actingAs($this->manager) + ->patch("/files/{$file->id}", ['name' => 'Renamed', 'folder_id' => (string) $this->outOfScopeFolder->id]) + ->assertRedirect(); + + expect($file->fresh()) + ->name->toBe('Renamed') + ->folder_id->toBe($this->outOfScopeFolder->id); +}); + +test('api update() refuses to reparent a file into an out-of-scope folder', function () { + $file = File::factory()->create(['name' => 'Report', 'uploaded_by' => $this->manager->id]); + + $token = $this->manager->createToken('t', [ + Permission::EditFiles->value, + Permission::EditOthersFiles->value, + ])->plainTextToken; + + $this->withToken($token) + ->patchJson("/api/v1/files/{$file->id}", ['folder_id' => $this->outOfScopeFolder->id]) + ->assertNotFound(); + + expect($file->fresh()->folder_id)->toBeNull(); +}); + +test('an unscoped admin may reparent into any folder through update()', function () { + $file = File::factory()->create(['name' => 'Report', 'uploaded_by' => $this->admin->id]); + + $this->actingAs($this->admin) + ->patch("/files/{$file->id}", ['name' => 'Report', 'folder_id' => $this->outOfScopeFolder->id]) + ->assertRedirect(); + + expect($file->fresh()->folder_id)->toBe($this->outOfScopeFolder->id); +});