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'), + ); +});