From 48a1c9f22797f9056f2deeff4cb4c815f09408e6 Mon Sep 17 00:00:00 2001 From: ignacionelson Date: Fri, 2 Oct 2026 23:45:51 -0300 Subject: [PATCH] Trim the staff breadcrumb to the library's reach, and ask before nesting into a public folder Two edges of the staff folder screens, found while the folder API was built to answer the same questions. A client-scoped staff member can hold one of their clients' folders that sits inside somebody else's tree. The breadcrumb above it named every folder on the way up, including ones their library does not show them. It now starts at the first folder they can reach, as the client portal's already does (BreadcrumbBuilder::visible). Unscoped staff see the whole trail as before. Creating a folder did not ask Folder::uploadableBy for its parent, though every other write of a parent_id does: a folder inside a public one is public. Files were already refused there by the upload check, so what this closes is an empty folder's name appearing on a public page without upload_public. Staff holding upload_public, or creating inside a private folder, are unaffected. --- .../Http/Controllers/FoldersController.php | 34 ++++++- .../Feature/Files/StaffFolderBoundaryTest.php | 96 +++++++++++++++++++ 2 files changed, 128 insertions(+), 2 deletions(-) create mode 100644 tests/Feature/Files/StaffFolderBoundaryTest.php diff --git a/app/Modules/Files/Http/Controllers/FoldersController.php b/app/Modules/Files/Http/Controllers/FoldersController.php index 43f2fde5..f6a4e10e 100644 --- a/app/Modules/Files/Http/Controllers/FoldersController.php +++ b/app/Modules/Files/Http/Controllers/FoldersController.php @@ -252,7 +252,7 @@ class FoldersController extends Controller return Inertia::render('files/index', [ 'folder' => $current === null ? null : ['id' => $current->id, 'name' => $current->name], - 'breadcrumb' => $flat ? [] : $this->breadcrumbs->for($current), + 'breadcrumb' => $flat ? [] : $this->breadcrumb($user, $current), 'folders' => $folderRows->map(fn (Folder $folder): array => $this->folderRow($user, $folder))->all(), 'files' => $fileRows->map(fn (File $file): array => $this->fileRow($user, $file, $commentCounts, $pendingCounts, $versions))->all(), 'pagination' => Pagination::meta($sliced['paginator']), @@ -426,7 +426,7 @@ class FoldersController extends Controller 'public_url' => $folder->public ? $this->publicUrl->for($folder) : null, - 'breadcrumb' => $this->breadcrumbs->for($folder), + 'breadcrumb' => $this->breadcrumb($user, $folder), 'can_update' => Gate::forUser($user)->allows('update', $folder), 'can_manage_public' => $user->can('upload_public'), ...$this->shareTargets->forSubject($folder, $user), @@ -450,6 +450,11 @@ class FoldersController extends Controller $parent = $this->resolveParent($user, $validated['parent_id'] ?? null); + // A folder inside a public one is public, so creating it there is + // placing content into a public folder: the question every other + // write of a parent_id already asks (Folder::uploadableBy). + abort_unless(Folder::uploadableBy($user, $parent), 403); + $folder = $this->folders->create($validated['name'], $parent); // Only a user who can manage public state may set it on create — @@ -631,6 +636,31 @@ class FoldersController extends Controller ->count(); } + /** + * The trail to $folder, trimmed for a client-scoped staff member to + * start at the first folder their library shows them: one of their + * clients' folders can sit inside somebody else's tree, and the names + * above it are not theirs to read. The client portal trims the same way. + * + * @return list + */ + private function breadcrumb(User $user, ?Folder $folder): array + { + if ($folder === null || ! $user->isClientScoped()) { + return $this->breadcrumbs->for($folder); + } + + $visibleIds = array_values(array_map( + 'intval', + $this->scope->folders($user) + ->whereIn('folders.id', [...$folder->ancestorIds(), $folder->id]) + ->pluck('folders.id') + ->all(), + )); + + return $this->breadcrumbs->visible($folder, $visibleIds); + } + private function resolveParent(?User $user, ?int $parentId): ?Folder { if ($user === null || $parentId === null) { diff --git a/tests/Feature/Files/StaffFolderBoundaryTest.php b/tests/Feature/Files/StaffFolderBoundaryTest.php new file mode 100644 index 00000000..b53f3f23 --- /dev/null +++ b/tests/Feature/Files/StaffFolderBoundaryTest.php @@ -0,0 +1,96 @@ +admin = User::factory()->create(); +}); + +/* + * A client-scoped staff member can hold a client's folder that sits inside + * somebody else's tree. The trail above it named every folder on the way, + * including the ones their library does not show them. The client portal + * already trims the same trail (BreadcrumbBuilder::visible). + */ +test('a client-scoped staff member is not told the names of folders above their reach', function () { + $client = User::factory()->client()->create(); + $manager = User::factory()->role(SystemRole::ClientManager)->create(); + $manager->assignedClients()->sync([$client->id]); + + $secret = makeFolder('Board minutes'); + $acme = makeFolder('Acme', $secret); + $year = makeFolder('2026', $acme); + $this->actingAs($this->admin)->post("/folders/{$acme->id}/assignments", ['type' => 'client', 'id' => $client->id]); + + $this->actingAs($manager)->get("/files?folder={$year->id}") + ->assertOk() + ->assertInertia(fn (AssertableInertia $page) => $page->where('breadcrumb', [ + ['id' => $acme->id, 'name' => 'Acme'], + ['id' => $year->id, 'name' => '2026'], + ])); + + $this->actingAs($manager)->get("/folders/{$acme->id}") + ->assertOk() + ->assertInertia(fn (AssertableInertia $page) => $page->where('breadcrumb', [ + ['id' => $acme->id, 'name' => 'Acme'], + ])); +}); + +test('an unscoped staff member still sees the whole trail', function () { + $secret = makeFolder('Board minutes'); + $acme = makeFolder('Acme', $secret); + + $this->actingAs($this->admin)->get("/files?folder={$acme->id}") + ->assertOk() + ->assertInertia(fn (AssertableInertia $page) => $page->where('breadcrumb', [ + ['id' => $secret->id, 'name' => 'Board minutes'], + ['id' => $acme->id, 'name' => 'Acme'], + ])); +}); + +/* + * A folder inside a public folder is public, so creating one there is + * placing something into a public folder — the question + * Folder::uploadableBy answers for every other write of a parent_id. + * Creation was the one write that did not ask it. + */ +test('a folder cannot be created inside a public folder without permission to publish', function () { + $public = makeFolder('Press kit'); + $public->update(['public' => true]); + + $staff = staffWithPermissions(['create_own_folders', 'upload', 'edit_others_files']); + + $this->actingAs($staff)->post('/folders', ['name' => 'Drafts', 'parent_id' => $public->id])->assertForbidden(); + + expect(Folder::query()->where('name', 'Drafts')->exists())->toBeFalse(); +}); + +test('with upload_public, creating inside a public folder still works', function () { + $public = makeFolder('Press kit'); + $public->update(['public' => true]); + + $staff = staffWithPermissions(['create_own_folders', 'upload', 'edit_others_files', 'upload_public']); + + $this->actingAs($staff)->post('/folders', ['name' => 'Drafts', 'parent_id' => $public->id])->assertRedirect(); + + expect(Folder::query()->where('name', 'Drafts')->value('parent_id'))->toBe($public->id); +}); + +test('creating inside a private folder needs nothing extra', function () { + $private = makeFolder('Internal'); + $staff = staffWithPermissions(['create_own_folders', 'upload', 'edit_others_files']); + + $this->actingAs($staff)->post('/folders', ['name' => 'Drafts', 'parent_id' => $private->id])->assertRedirect(); + + expect(Folder::query()->where('name', 'Drafts')->value('parent_id'))->toBe($private->id); +});