diff --git a/app/Modules/Files/Http/Controllers/FilesController.php b/app/Modules/Files/Http/Controllers/FilesController.php index 9d3e0736..5f9e0600 100644 --- a/app/Modules/Files/Http/Controllers/FilesController.php +++ b/app/Modules/Files/Http/Controllers/FilesController.php @@ -93,6 +93,17 @@ class FilesController extends Controller $user = $request->user(); assert($user !== null); + // The check the other two upload paths make and this one did not: + // a folder outside the uploader's library is not a place to put a + // file. Without it this route reached any folder on the + // installation, and File::scopeVisibleToClient then hands the file + // to whoever that folder's subtree is shared with. + $folder = isset($validated['folder_id']) + ? Folder::query()->whereKey($validated['folder_id'])->first() + : null; + + abort_unless(Folder::uploadableBy($user, $folder), 403); + if (! app(UploadExtensionPolicy::class)->isAllowed($user, $upload->getClientOriginalName())) { throw ValidationException::withMessages([ 'file' => __('This file type is not allowed for upload.'), @@ -197,7 +208,11 @@ class FilesController extends Controller 'url' => route('files.edit', $member, false), 'is_current' => $member->id === $file->id, ])->values(), - 'folder_options' => Folder::query()->orderBy('path')->orderBy('name')->get() + // Narrowed like every other folder listing: an unscoped staff + // member gets the whole tree, a client-scoped one only their + // own. Unfiltered this handed a scoped staffer every folder + // name and id on the installation. + 'folder_options' => $this->scope->folders($viewer)->orderBy('path')->orderBy('name')->get() ->map(fn (Folder $folder): array => ['id' => $folder->id, 'name' => $folder->name])->all(), 'categories' => Category::query()->orderBy('name')->get(['id', 'name', 'color']) ->map(fn (Category $category): array => ['id' => $category->id, 'name' => $category->name, 'color' => $category->color])->all(), diff --git a/app/Modules/Files/Http/Controllers/FoldersController.php b/app/Modules/Files/Http/Controllers/FoldersController.php index de25005e..120a3561 100644 --- a/app/Modules/Files/Http/Controllers/FoldersController.php +++ b/app/Modules/Files/Http/Controllers/FoldersController.php @@ -186,7 +186,11 @@ class FoldersController extends Controller 'expired' => $expired, 'categories' => Category::query()->orderBy('name')->get(['id', 'name', 'color']) ->map(fn (Category $category): array => ['id' => $category->id, 'name' => $category->name, 'color' => $category->color])->all(), - 'folder_options' => Folder::query()->orderBy('path')->orderBy('name')->get() + // Narrowed like every other folder listing on this screen: an + // unscoped staff member gets the whole tree, a client-scoped + // one only their own. Unfiltered this handed a scoped staffer + // every folder name and id on the installation. + 'folder_options' => $this->scope->folders($user)->orderBy('path')->orderBy('name')->get() ->map(fn (Folder $folder): array => ['id' => $folder->id, 'name' => $folder->name])->all(), 'can_create_folders' => $user->can('create_own_folders'), 'can_upload' => $user->can('upload'), diff --git a/app/Modules/Files/Models/Folder.php b/app/Modules/Files/Models/Folder.php index 5631cd66..53c45e99 100644 --- a/app/Modules/Files/Models/Folder.php +++ b/app/Modules/Files/Models/Folder.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace App\Modules\Files\Models; use App\Models\User; +use App\Modules\Files\Access\StaffLibraryScope; use App\Modules\Groups\Models\Group; use App\Support\Concerns\HasUniqueSlug; use Illuminate\Database\Eloquent\Builder; @@ -152,11 +153,19 @@ class Folder extends Model /** * Whether $user may upload a new file directly into $folder (null = - * loose at the root, always allowed). Staff already validate folder_id - * through FilesController's own flow — this is the client-facing - * check, used by ChunkedUploadsController: the client owns the - * folder, or it's a public folder that opts into client uploads and - * the client's role permits uploading into public folders at all. + * loose at the root, always allowed). + * + * Staff are held to the library boundary they are held to everywhere + * else: an unscoped staff member may use any folder, a client-scoped + * one only the folders StaffLibraryScope already shows them. This is + * the only place that decides it: every upload path — the web form, + * the API and the chunked flow the browser actually posts to — comes + * through here rather than checking folder_id for itself. + * + * For a client this is unchanged, and is still the whole of the + * check: they own the folder, or it is a public folder that opts into + * client uploads and their role permits uploading into public folders + * at all. */ public static function uploadableBy(User $user, ?self $folder): bool { @@ -165,7 +174,7 @@ class Folder extends Model } if ($user->isStaff()) { - return true; + return app(StaffLibraryScope::class)->allowsFolder($user, $folder); } return $folder->isOwnedBy($user) diff --git a/tests/Feature/Files/UploadFolderScopeTest.php b/tests/Feature/Files/UploadFolderScopeTest.php new file mode 100644 index 00000000..b2a92541 --- /dev/null +++ b/tests/Feature/Files/UploadFolderScopeTest.php @@ -0,0 +1,194 @@ +admin = User::factory()->create(); + + // A folder shared with a client the manager below is not assigned to: + // in their library it does not exist, and everything in its subtree + // belongs to that other client. + $this->stranger = User::factory()->client()->create(['name' => 'Not Mine']); + $this->strangersFolder = makeFolder('Strangers'); + $this->actingAs($this->admin) + ->post("/folders/{$this->strangersFolder->id}/assignments", ['type' => 'client', 'id' => $this->stranger->id]); + + $this->mine = User::factory()->client()->create(['name' => 'Mine']); + $this->manager = User::factory()->role(SystemRole::ClientManager)->create(); + $this->manager->assignedClients()->sync([$this->mine->id]); + + $this->myFolder = makeFolder('Mine'); + $this->actingAs($this->admin) + ->post("/folders/{$this->myFolder->id}/assignments", ['type' => 'client', 'id' => $this->mine->id]); +}); + +/** The opening request of the resumable flow, which is where the folder is named. */ +function beginChunkedUpload(int $folderId): array +{ + return ['filename' => 'resumable.pdf', 'size' => 2048, 'type' => 'application/pdf', 'folder_id' => $folderId]; +} + +/** @return list */ +function folderOptionNames(mixed $options): array +{ + return collect($options)->pluck('name')->all(); +} + +test('a scoped manager cannot upload into a folder outside their library over the web route', function () { + $this->actingAs($this->manager)->post('/files', [ + 'file' => UploadedFile::fake()->create('smuggled.pdf', 12, 'application/pdf'), + 'name' => 'Smuggled', + 'description' => '', + 'folder_id' => $this->strangersFolder->id, + ])->assertForbidden(); + + expect(File::query()->where('name', 'Smuggled')->exists())->toBeFalse(); +}); + +test('a scoped manager cannot upload into a folder outside their library over the API', function () { + $token = $this->manager->createToken('t', [Permission::Upload->value])->plainTextToken; + + $this->withToken($token)->post('/api/v1/files', [ + 'file' => UploadedFile::fake()->create('smuggled.pdf', 12, 'application/pdf'), + 'name' => 'Smuggled', + 'folder_id' => $this->strangersFolder->id, + ], ['Accept' => 'application/json'])->assertForbidden(); + + expect(File::query()->where('name', 'Smuggled')->exists())->toBeFalse(); +}); + +test('a scoped manager cannot open a chunked upload into a folder outside their library', function () { + // The production path for both surfaces: files/create.tsx posts here, + // not to files.store. + $this->actingAs($this->manager) + ->postJson('/uploads', beginChunkedUpload($this->strangersFolder->id)) + ->assertForbidden(); + + $token = $this->manager->createToken('t', [Permission::Upload->value])->plainTextToken; + + $this->withToken($token) + ->postJson('/api/v1/uploads', beginChunkedUpload($this->strangersFolder->id)) + ->assertForbidden(); +}); + +test('a file landing in that folder would be the stranger content, which is why the folder is the check', function () { + // The stranger is never assigned anything here. Landing a file inside + // a folder shared with them is the whole of it: File::scopeVisibleToClient + // reads the folder subtree, so the file is theirs the moment it is + // written, without an assignment row ever being touched. This is the + // outcome the three refusals above prevent. + $planted = uploadNamedFile($this->admin, 'planted', $this->strangersFolder->id); + + expect(File::query()->whereKey($planted->id)->visibleToClient($this->stranger)->exists())->toBeTrue(); +}); + +test('a scoped manager still uploads into a folder of their own client, on every path', function () { + $this->actingAs($this->manager)->post('/files', [ + 'file' => UploadedFile::fake()->create('ok.pdf', 12, 'application/pdf'), + 'name' => 'Allowed', + 'description' => '', + 'folder_id' => $this->myFolder->id, + ])->assertRedirect(); + + expect(File::query()->where('name', 'Allowed')->value('folder_id'))->toBe($this->myFolder->id); + + $this->actingAs($this->manager)->postJson('/uploads', beginChunkedUpload($this->myFolder->id))->assertOk(); + + // And their own folder, which is theirs by creation rather than + // through any client assignment. + $this->actingAs($this->manager); + $ownFolder = app(FolderService::class)->create('My Own', null); + + $this->actingAs($this->manager)->postJson('/uploads', beginChunkedUpload($ownFolder->id))->assertOk(); + + // Token last: a bearer request swaps the default guard for the rest of + // the test, which a following session request cannot recover from. + $token = $this->manager->createToken('t', [Permission::Upload->value])->plainTextToken; + + $this->withToken($token)->post('/api/v1/files', [ + 'file' => UploadedFile::fake()->create('ok.pdf', 12, 'application/pdf'), + 'name' => 'Allowed via API', + 'folder_id' => $this->myFolder->id, + ], ['Accept' => 'application/json'])->assertStatus(201); +}); + +test('an unscoped staff member reaches every folder exactly as before', function () { + $this->actingAs($this->admin)->post('/files', [ + 'file' => UploadedFile::fake()->create('ok.pdf', 12, 'application/pdf'), + 'name' => 'Admin Upload', + 'description' => '', + 'folder_id' => $this->strangersFolder->id, + ])->assertRedirect(); + + expect(File::query()->where('name', 'Admin Upload')->value('folder_id'))->toBe($this->strangersFolder->id); + + $this->actingAs($this->admin)->postJson('/uploads', beginChunkedUpload($this->strangersFolder->id))->assertOk(); +}); + +test('the client side of uploadableBy is untouched', function () { + // The same helper answers for clients, whose branch is a different + // rule entirely: ownership, or a public folder opting into client + // uploads. A client is never client-scoped, so nothing the staff + // branch now asks can reach them. Both directions pinned. + $client = User::factory()->client()->create(); + + $this->actingAs($client); + $ownFolder = app(FolderService::class)->create('Client Folder', null); + + expect(Folder::uploadableBy($client, $ownFolder))->toBeTrue() + ->and(Folder::uploadableBy($client, $this->strangersFolder))->toBeFalse(); + + $this->actingAs($client)->postJson('/uploads', beginChunkedUpload($ownFolder->id))->assertOk(); + $this->actingAs($client)->postJson('/uploads', beginChunkedUpload($this->strangersFolder->id))->assertForbidden(); +}); + +test('the folder pickers offer a scoped manager only their own folders', function () { + $this->actingAs($this->manager)->get('/files')->assertInertia( + fn (AssertableInertia $page) => $page->where( + 'folder_options', + fn ($options) => in_array('Mine', folderOptionNames($options), true) + && ! in_array('Strangers', folderOptionNames($options), true), + ), + ); + + $file = uploadNamedFile($this->manager, 'mine-to-edit'); + + $this->actingAs($this->manager)->get("/files/{$file->id}")->assertInertia( + fn (AssertableInertia $page) => $page->where( + 'folder_options', + fn ($options) => in_array('Mine', folderOptionNames($options), true) + && ! in_array('Strangers', folderOptionNames($options), true), + ), + ); +}); + +test('an unscoped staff member still sees the whole tree in both pickers', function () { + $this->actingAs($this->admin)->get('/files')->assertInertia( + fn (AssertableInertia $page) => $page->where( + 'folder_options', + fn ($options) => in_array('Mine', folderOptionNames($options), true) + && in_array('Strangers', folderOptionNames($options), true), + ), + ); + + $file = uploadNamedFile($this->admin, 'admins'); + + $this->actingAs($this->admin)->get("/files/{$file->id}")->assertInertia( + fn (AssertableInertia $page) => $page->where( + 'folder_options', + fn ($options) => in_array('Mine', folderOptionNames($options), true) + && in_array('Strangers', folderOptionNames($options), true), + ), + ); +});