Merge pull request #1681 from denkfabrik-li/fix/file-update-folder-scope

Scope a file's destination folder on update(), as move() already does

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')) {

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

tests/Feature/Files/FileUpdateFolderScopeTest.php

@@ -0,0 +1,106 @@
+<?php
+
+declare(strict_types=1);
+
+use App\Models\User;
+use App\Modules\Files\Folders\FolderService;
+use App\Modules\Files\Models\File;
+use App\Modules\Identity\Permissions\Permission;
+use App\Modules\Identity\Permissions\SystemRole;
+use Illuminate\Support\Facades\Storage;
+
+/**
+ * Setting a file's folder_id is the same privileged write in update(),
+ * move() and bulkUpdate(). move()/bulkUpdate() already refuse a destination
+ * outside the caller's library (StaffLibraryScope::folders); update() must
+ * do the same, or a client-scoped staff member could reparent an in-scope
+ * file into a folder shared with a client they are not assigned to — which
+ * File::scopeVisibleToClient then exposes to that client, sidestepping the
+ * very boundary the sharing endpoints enforce.
+ */
+beforeEach(function () {
+    Storage::fake('files');
+    $this->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);
+});