diff --git a/app/Modules/Audit/Http/Controllers/DashboardController.php b/app/Modules/Audit/Http/Controllers/DashboardController.php index 5f9c69df..13040904 100644 --- a/app/Modules/Audit/Http/Controllers/DashboardController.php +++ b/app/Modules/Audit/Http/Controllers/DashboardController.php @@ -13,6 +13,7 @@ use App\Modules\Audit\ActivityLogScope; use App\Modules\Audit\ActivityPresenter; use App\Modules\Audit\DashboardWidgetPreferences; use App\Modules\Clients\ClientStorageUsage; +use App\Modules\Files\Access\StaffLibraryScope; use App\Modules\Files\Models\File; use App\Modules\Groups\Models\Group; use App\Modules\Identity\UserType; @@ -54,6 +55,7 @@ class DashboardController extends Controller private readonly SystemEnvironment $environment, private readonly ActivityPresenter $presenter, private readonly ActivityLogScope $scope, + private readonly StaffLibraryScope $library, ) {} public function __invoke(Request $request): Response @@ -85,7 +87,7 @@ class DashboardController extends Controller ? ['preset' => $preset, 'from' => $from->toDateString(), 'to' => $to->toDateString()] : null, 'top_clients_by_storage' => $canStatistics && $prefs->isEnabled($user, 'top_clients_by_storage') - ? $this->topClientsByStorage() + ? $this->topClientsByStorage($user) : null, 'largest_files' => $canStatistics && $prefs->isEnabled($user, 'largest_files') ? $this->largestFiles($user) : null, 'recent' => $canActionsLog && $prefs->isEnabled($user, 'recent') ? $this->recentActivity($user) : null, @@ -210,6 +212,12 @@ class DashboardController extends Controller */ private function counters(): array { + // Deliberately installation-wide, unlike the three widgets below. + // A total carries no names — "417 files" tells a scoped viewer + // nothing about whose they are — and the same reasoning leaves + // transferSeries() alone. If that ever stops being the line, both + // move together. + return [ 'files' => File::query()->count(), 'files_bytes' => (int) File::query()->sum('size'), @@ -280,9 +288,12 @@ class DashboardController extends Controller * * @return list */ - private function topClientsByStorage(): array + private function topClientsByStorage(User $viewer): array { - $rows = File::query() + // 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) ->select('uploaded_by', DB::raw('SUM(size) as total_bytes')) ->whereHas('uploader', fn ($query) => $query->where('type', UserType::Client)) ->groupBy('uploaded_by') @@ -342,7 +353,14 @@ class DashboardController extends Controller $staffModule = $this->capabilities->has(Capability::UsersManage) && $viewer->can('manage_users'); $canStaffUsers = $staffModule && $viewer->can('edit_users'); - return array_values(File::query() + // Narrowed to the viewer's library, not just its links. The + // note above is about a link that 403s; a row that should not be + // here at all is a different problem, and the file's *name* is + // the part that leaks — "Q3 delinquent accounts" says plenty + // without being downloadable. Scoping the query costs one call: + // StaffLibraryScope builds a scoped user's query once per + // request, so this is not a per-row check. + return array_values($this->library->files($viewer) ->with('uploader:id,name,type') ->orderByDesc('size') ->limit(10) @@ -381,8 +399,20 @@ class DashboardController extends Controller $canFiles = $viewer->can('upload') || $viewer->can('edit_files') || $viewer->can('edit_others_files'); return [ - 'count' => File::query()->expired()->count(), - 'files' => array_values(File::query()->expired()->orderBy('expires_at')->limit(10) + // Both the count and the list read the viewer's library, so + // the number cannot describe files the list is not allowed to + // name. Same reason largestFiles() is scoped. + // + // Narrower than it looks for a client-scoped viewer: + // File::scopeVisibleToClient ends in notExpired(), so an + // expired file belonging to one of their clients is not in + // their library, and only their own expired uploads reach + // this list. Safe, and under-inclusive — telling them about + // a client's file that auto-delete is about to take would + // need a library query that keeps expired rows, which is a + // boundary to decide rather than to invent here. + 'count' => $this->library->files($viewer)->expired()->count(), + 'files' => array_values($this->library->files($viewer)->expired()->orderBy('expires_at')->limit(10) ->get(['id', 'name', 'expires_at']) ->map(fn (File $file): array => [ 'id' => $file->id, diff --git a/tests/Feature/Audit/DashboardTest.php b/tests/Feature/Audit/DashboardTest.php index 873967a3..60786986 100644 --- a/tests/Feature/Audit/DashboardTest.php +++ b/tests/Feature/Audit/DashboardTest.php @@ -396,3 +396,71 @@ test('an unscoped viewer still sees the whole installation in the widget', funct fn (AssertableInertia $page) => $page->has('recent', 1)->where('recent.0.replacements.subject', 'Anything'), ); }); + +// The shipped Client Manager role is client-scoped and holds +// view_statistics, so these three widgets are the default configuration, +// not a custom one. A file's name is the part that leaks: "Q3 delinquent +// accounts" says plenty without ever being downloadable. +test('the statistics widgets name only files inside the viewer scope', function () { + $role = Role::query()->create(['name' => 'Scoped stats', 'client_scoped' => true]); + RolePermission::query()->insert([ + ['role_id' => $role->id, 'permission' => 'view_statistics'], + ['role_id' => $role->id, 'permission' => 'upload'], + ]); + + $scoped = User::factory()->create(['role_id' => $role->id]); + $client = User::factory()->client()->create(); + $scoped->assignedClients()->attach($client->id); + + $mine = File::factory()->create([ + 'uploaded_by' => $this->admin->id, + 'name' => 'Statement', + 'size' => 10_000, + ]); + shareFileWith($mine, $client); + + // Expired files reach a scoped viewer only through their own uploads: + // File::scopeVisibleToClient ends in notExpired(), so an expired file + // belonging to one of their clients is not in their library at all. + $ownExpired = File::factory()->create([ + 'uploaded_by' => $scoped->id, + 'name' => 'My Own Expired', + 'size' => 1_000, + 'expires_at' => now()->subDay(), + ]); + + // Bigger, sooner-expired, and none of this viewer's business. + File::factory()->create([ + 'uploaded_by' => $this->admin->id, + 'name' => 'Q3 delinquent accounts', + 'size' => 99_000_000, + 'expires_at' => now()->subDays(5), + ]); + + $this->actingAs($scoped)->get('/dashboard')->assertInertia( + fn (AssertableInertia $page) => $page + ->has('largest_files', 2) + ->where('largest_files.0.name', 'Statement') + ->where('largest_files.1.name', 'My Own Expired') + ->has('expired_files.files', 1) + ->where('expired_files.files.0.name', 'My Own Expired') + // The count has to agree with the list, or the number + // describes files the list is not allowed to name. + ->where('expired_files.count', 1), + ); +}); + +test('an unscoped viewer still sees the whole installation in those widgets', function () { + File::factory()->create([ + 'uploaded_by' => $this->admin->id, + 'name' => 'Anything', + 'size' => 5_000, + 'expires_at' => now()->subDay(), + ]); + + $this->actingAs($this->admin)->get('/dashboard')->assertInertia( + fn (AssertableInertia $page) => $page + ->where('largest_files.0.name', 'Anything') + ->where('expired_files.count', 1), + ); +});