From 12a8ebe3802c4c7fe1464a265dd7d41e219ecc78 Mon Sep 17 00:00:00 2001 From: ignacionelson Date: Wed, 26 Aug 2026 18:24:13 -0300 Subject: [PATCH] Rank top clients by roster, not by library, and factor the client guard Two things found by checking #1696 and #1699 -- open branches carrying the same fixes I wrote this morning -- against what I actually shipped. **topClientsByStorage was scoped with the wrong question.** 4b8220a narrowed it with StaffLibraryScope::files(), which is right for the two widgets that name files and wrong for the one that names clients: a stranger client's upload can sit legitimately inside a scoped viewer's library, shared with a group one of their own clients belongs to. So the file was theirs to read and the uploader's name was not theirs to see. Measured: "Stranger Client Ltd", on nobody's roster, ranked on a scoped dashboard. assignableClientIds is what the widget is actually asking, and it is what #1699 used. Their version was right and mine was not. **The client guard is one method now, not eight copies.** #1696 wrote it as a private guardTarget() rather than repeating viewer-resolve plus abort at each site, which is better, and this is a change whose whole argument is that a rule stated in many places drifts. Behaviour is identical; the eight sites now read as one rule. The published document reorders a 404 below a 422 on one path. Scramble reads abort_unless out of a method body but not out of a helper it calls, so the 404 now comes from route model binding instead of from the inline abort -- same response, different position. #1701's body names this trap; worth knowing it costs ordering and not content. Credit where it is due: both come from denkfabrik-li's #1696 and #1699, which were open while I was writing the same fixes. Those two are closed against this and against e7b5b6a, 4b8220a and 67e9204. --- .../Http/Controllers/DashboardController.php | 16 ++++-- .../Controllers/Api/ClientsController.php | 45 ++++++----------- .../Http/Controllers/ClientsController.php | 49 +++++++------------ docs/api/openapi.json | 6 +-- tests/Feature/Audit/DashboardTest.php | 38 ++++++++++++++ 5 files changed, 85 insertions(+), 69 deletions(-) diff --git a/app/Modules/Audit/Http/Controllers/DashboardController.php b/app/Modules/Audit/Http/Controllers/DashboardController.php index 0ff45a87..b8931b45 100644 --- a/app/Modules/Audit/Http/Controllers/DashboardController.php +++ b/app/Modules/Audit/Http/Controllers/DashboardController.php @@ -27,6 +27,7 @@ use App\Modules\Platform\Settings\Settings; use App\Modules\Platform\Storage\StorageDurability; use App\Modules\Platform\System\SystemEnvironment; use App\Modules\Platform\Updates\LatestReleaseInfo; +use Illuminate\Database\Eloquent\Builder; use Illuminate\Http\Request; use Illuminate\Support\Carbon; use Illuminate\Support\Facades\DB; @@ -290,12 +291,19 @@ class DashboardController extends Controller */ private function topClientsByStorage(User $viewer): array { - // Scoped like the other two: this one names clients rather than - // files, which is the same thing MembershipRequest::approvableBy - // and ActivityLogScope exist to keep inside a viewer's roster. - $rows = $this->library->files($viewer) + // Narrowed by roster, not by library. This widget names *clients*, + // and files() is the wrong lens for that: a stranger client's + // upload can be inside a scoped viewer's library — shared with a + // group one of their own clients is in — which put the stranger's + // name on the widget. Measured: a client called "Stranger Client + // Ltd", on nobody's roster, ranked on a scoped dashboard. + // assignableClientIds is the question actually being asked. + $clientIds = $this->library->assignableClientIds($viewer); + + $rows = File::query() ->select('uploaded_by', DB::raw('SUM(size) as total_bytes')) ->whereHas('uploader', fn ($query) => $query->where('type', UserType::Client)) + ->when($clientIds !== null, fn (Builder $query) => $query->whereIn('uploaded_by', $clientIds)) ->groupBy('uploaded_by') ->orderByDesc('total_bytes') ->limit(5) diff --git a/app/Modules/Clients/Http/Controllers/Api/ClientsController.php b/app/Modules/Clients/Http/Controllers/Api/ClientsController.php index dcc0cd23..a26f8661 100644 --- a/app/Modules/Clients/Http/Controllers/Api/ClientsController.php +++ b/app/Modules/Clients/Http/Controllers/Api/ClientsController.php @@ -86,17 +86,24 @@ class ClientsController extends Controller return ClientResource::collection($this->polling->paginate($request, $query, 'users')); } - public function show(Request $request, User $client): ClientResource + /** + * Mirrors the web controller's guard, as every API twin here does: + * the token's `edit_clients` says its owner manages clients, not + * that they manage *this* one. + */ + private function guardTarget(Request $request, User $client): void { abort_unless($client->isClient(), 404); $viewer = $request->user(); assert($viewer !== null); - // A permission is not a boundary: the token's `edit_clients` - // says its owner manages clients, not that they manage *this* - // one. Mirrors the web controller, as every API twin here does. abort_unless($this->scope->canAssignClient($viewer, $client), 404); + } + + public function show(Request $request, User $client): ClientResource + { + $this->guardTarget($request, $client); return $this->resourceFor($client); @@ -149,15 +156,7 @@ class ClientsController extends Controller public function update(Request $request, User $client): ClientResource { - abort_unless($client->isClient(), 404); - - $viewer = $request->user(); - assert($viewer !== null); - - // A permission is not a boundary: the token's `edit_clients` - // says its owner manages clients, not that they manage *this* - // one. Mirrors the web controller, as every API twin here does. - abort_unless($this->scope->canAssignClient($viewer, $client), 404); + $this->guardTarget($request, $client); $validated = $request->validate([ @@ -230,15 +229,7 @@ class ClientsController extends Controller */ public function destroyTwoFactor(Request $request, User $client, TwoFactorAdministration $twoFactor): JsonResponse { - abort_unless($client->isClient(), 404); - - $viewer = $request->user(); - assert($viewer !== null); - - // A permission is not a boundary: the token's `edit_clients` - // says its owner manages clients, not that they manage *this* - // one. Mirrors the web controller, as every API twin here does. - abort_unless($this->scope->canAssignClient($viewer, $client), 404); + $this->guardTarget($request, $client); $twoFactor->reset($client); @@ -264,15 +255,7 @@ class ClientsController extends Controller */ public function destroy(Request $request, User $client): JsonResponse { - abort_unless($client->isClient(), 404); - - $viewer = $request->user(); - assert($viewer !== null); - - // A permission is not a boundary: the token's `edit_clients` - // says its owner manages clients, not that they manage *this* - // one. Mirrors the web controller, as every API twin here does. - abort_unless($this->scope->canAssignClient($viewer, $client), 404); + $this->guardTarget($request, $client); $validated = $this->accountDeletion->validate($request, $client); diff --git a/app/Modules/Clients/Http/Controllers/ClientsController.php b/app/Modules/Clients/Http/Controllers/ClientsController.php index 278c51de..2fe33b2e 100644 --- a/app/Modules/Clients/Http/Controllers/ClientsController.php +++ b/app/Modules/Clients/Http/Controllers/ClientsController.php @@ -141,17 +141,28 @@ class ClientsController extends Controller return redirect()->route('clients.edit', $client)->with('success', __('Client created.')); } - public function edit(Request $request, User $client): Response + /** + * The one question every route binding a client has to ask. + * + * A permission is not a boundary: `edit_clients` says this staff + * member manages clients, not that they manage *this* one — the same + * rule ClientFilesController::index applies one route over. 404 + * rather than 403, so a client outside the roster is not + * distinguishable from one that is not there. + */ + private function guardTarget(Request $request, User $client): void { abort_unless($client->isClient(), 404); $viewer = $request->user(); assert($viewer !== null); - // A permission is not a boundary: `edit_clients` says this staff - // member manages clients, not that they manage *this* one. The - // same rule ClientFilesController::index applies one route over. abort_unless($this->scope->canAssignClient($viewer, $client), 404); + } + + public function edit(Request $request, User $client): Response + { + $this->guardTarget($request, $client); return Inertia::render('clients/edit', [ @@ -177,15 +188,7 @@ class ClientsController extends Controller public function update(Request $request, User $client): RedirectResponse { - abort_unless($client->isClient(), 404); - - $viewer = $request->user(); - assert($viewer !== null); - - // A permission is not a boundary: `edit_clients` says this staff - // member manages clients, not that they manage *this* one. The - // same rule ClientFilesController::index applies one route over. - abort_unless($this->scope->canAssignClient($viewer, $client), 404); + $this->guardTarget($request, $client); $validated = $request->validate(array_merge([ @@ -247,15 +250,7 @@ class ClientsController extends Controller */ public function destroyTwoFactor(Request $request, User $client, TwoFactorAdministration $twoFactor): RedirectResponse { - abort_unless($client->isClient(), 404); - - $viewer = $request->user(); - assert($viewer !== null); - - // A permission is not a boundary: `edit_clients` says this staff - // member manages clients, not that they manage *this* one. The - // same rule ClientFilesController::index applies one route over. - abort_unless($this->scope->canAssignClient($viewer, $client), 404); + $this->guardTarget($request, $client); $twoFactor->reset($client); @@ -265,15 +260,7 @@ class ClientsController extends Controller public function destroy(Request $request, User $client): RedirectResponse { - abort_unless($client->isClient(), 404); - - $viewer = $request->user(); - assert($viewer !== null); - - // A permission is not a boundary: `edit_clients` says this staff - // member manages clients, not that they manage *this* one. The - // same rule ClientFilesController::index applies one route over. - abort_unless($this->scope->canAssignClient($viewer, $client), 404); + $this->guardTarget($request, $client); $validated = $this->accountDeletion->validate($request, $client); diff --git a/docs/api/openapi.json b/docs/api/openapi.json index aabb7674..d651d7d5 100644 --- a/docs/api/openapi.json +++ b/docs/api/openapi.json @@ -975,12 +975,12 @@ } } }, - "404": { - "$ref": "#/components/responses/ModelNotFoundException" - }, "422": { "$ref": "#/components/responses/ValidationException" }, + "404": { + "$ref": "#/components/responses/ModelNotFoundException" + }, "401": { "$ref": "#/components/responses/AuthenticationException" } diff --git a/tests/Feature/Audit/DashboardTest.php b/tests/Feature/Audit/DashboardTest.php index 8cd06971..db13433a 100644 --- a/tests/Feature/Audit/DashboardTest.php +++ b/tests/Feature/Audit/DashboardTest.php @@ -471,3 +471,41 @@ test('an unscoped viewer still sees the whole installation in those widgets', fu ->where('expired_files.scoped', false), ); }); + +// The top-clients widget names clients, so the roster is the question, +// not the library. A stranger's upload can sit inside a scoped viewer's +// library — shared with a group one of their own clients is in — which +// put the stranger's name on the widget while the file itself was +// legitimately visible. +test('the top-clients widget names only clients on the viewer roster', function () { + $role = Role::query()->create(['name' => 'Scoped stats roster', 'client_scoped' => true]); + RolePermission::query()->insert([ + ['role_id' => $role->id, 'permission' => 'view_statistics'], + ['role_id' => $role->id, 'permission' => 'upload'], + ]); + + $viewer = User::factory()->create(['role_id' => $role->id]); + $mine = User::factory()->client()->create(['name' => 'My Own Client']); + $viewer->assignedClients()->attach($mine->id); + + $stranger = User::factory()->client()->create(['name' => 'Stranger Client Ltd']); + + // Both clients are in one group, and the stranger's upload is shared + // with it — so my client may legitimately read the file, and the + // file is legitimately inside my library. + $group = Group::query()->create(['name' => 'Shared', 'slug' => 'shared-stats', 'public' => false]); + $group->members()->syncWithoutDetaching([$mine->id, $stranger->id]); + + $file = File::factory()->create(['uploaded_by' => $stranger->id, 'name' => 'Theirs', 'size' => 5_000_000]); + shareFileWithGroup($file, $group); + + // Something of my own client's, so the widget is not empty for the + // wrong reason. + File::factory()->create(['uploaded_by' => $mine->id, 'name' => 'Ours', 'size' => 1_000]); + + $this->actingAs($viewer)->get('/dashboard')->assertInertia( + fn (AssertableInertia $page) => $page + ->has('top_clients_by_storage', 1) + ->where('top_clients_by_storage.0.name', 'My Own Client'), + ); +});