mirror of
https://github.com/projectsend/projectsend.git
synced 2026-09-23 12:03:20 +00:00
Narrow the dashboard's file widgets to the viewer's own library
The sweep after #1685 turned up the same leak two widgets further down the same controller. largestFiles() and expiredFiles() already take the viewer -- to decide whether their rows get links -- but queried with a bare File::query(), so a client-scoped staff member's dashboard named files belonging to clients they hold nothing of. The note above largestFiles() says a link that 403s is accepted rather than adding per-row scope checks. That reasoning is about the link. A row that should not be there at all is a different problem, and the name is the part that leaks: "Q3 delinquent accounts" says plenty without ever being downloadable. Scoping the query is also cheaper than the per-row check that note declined -- StaffLibraryScope builds a scoped user's query once per request. Reachable in the default configuration, unlike the last few of these: the Client Manager role ships client-scoped and holds view_statistics. topClientsByStorage() goes with them; it names clients rather than files, which is the thing MembershipRequest::approvableBy and ActivityLogScope already exist to keep inside a roster. counters() and transferSeries() stay installation-wide, and now say so. A total carries no names -- "417 files" tells a scoped viewer nothing about whose they are -- and if that ever stops being the line, both move together. One consequence worth stating rather than discovering: scopeVisibleToClient ends in notExpired(), so a scoped viewer's expired-files widget now lists only their own expired uploads, not a client's. Safe, and under-inclusive -- telling them about a file auto-delete is about to take needs a library query that keeps expired rows, which is a boundary to decide rather than to invent inside a leak fix.
This commit is contained in:
@@ -13,6 +13,7 @@ use App\Modules\Audit\ActivityLogScope;
|
|||||||
use App\Modules\Audit\ActivityPresenter;
|
use App\Modules\Audit\ActivityPresenter;
|
||||||
use App\Modules\Audit\DashboardWidgetPreferences;
|
use App\Modules\Audit\DashboardWidgetPreferences;
|
||||||
use App\Modules\Clients\ClientStorageUsage;
|
use App\Modules\Clients\ClientStorageUsage;
|
||||||
|
use App\Modules\Files\Access\StaffLibraryScope;
|
||||||
use App\Modules\Files\Models\File;
|
use App\Modules\Files\Models\File;
|
||||||
use App\Modules\Groups\Models\Group;
|
use App\Modules\Groups\Models\Group;
|
||||||
use App\Modules\Identity\UserType;
|
use App\Modules\Identity\UserType;
|
||||||
@@ -54,6 +55,7 @@ class DashboardController extends Controller
|
|||||||
private readonly SystemEnvironment $environment,
|
private readonly SystemEnvironment $environment,
|
||||||
private readonly ActivityPresenter $presenter,
|
private readonly ActivityPresenter $presenter,
|
||||||
private readonly ActivityLogScope $scope,
|
private readonly ActivityLogScope $scope,
|
||||||
|
private readonly StaffLibraryScope $library,
|
||||||
) {}
|
) {}
|
||||||
|
|
||||||
public function __invoke(Request $request): Response
|
public function __invoke(Request $request): Response
|
||||||
@@ -85,7 +87,7 @@ class DashboardController extends Controller
|
|||||||
? ['preset' => $preset, 'from' => $from->toDateString(), 'to' => $to->toDateString()]
|
? ['preset' => $preset, 'from' => $from->toDateString(), 'to' => $to->toDateString()]
|
||||||
: null,
|
: null,
|
||||||
'top_clients_by_storage' => $canStatistics && $prefs->isEnabled($user, 'top_clients_by_storage')
|
'top_clients_by_storage' => $canStatistics && $prefs->isEnabled($user, 'top_clients_by_storage')
|
||||||
? $this->topClientsByStorage()
|
? $this->topClientsByStorage($user)
|
||||||
: null,
|
: null,
|
||||||
'largest_files' => $canStatistics && $prefs->isEnabled($user, 'largest_files') ? $this->largestFiles($user) : null,
|
'largest_files' => $canStatistics && $prefs->isEnabled($user, 'largest_files') ? $this->largestFiles($user) : null,
|
||||||
'recent' => $canActionsLog && $prefs->isEnabled($user, 'recent') ? $this->recentActivity($user) : null,
|
'recent' => $canActionsLog && $prefs->isEnabled($user, 'recent') ? $this->recentActivity($user) : null,
|
||||||
@@ -210,6 +212,12 @@ class DashboardController extends Controller
|
|||||||
*/
|
*/
|
||||||
private function counters(): array
|
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 [
|
return [
|
||||||
'files' => File::query()->count(),
|
'files' => File::query()->count(),
|
||||||
'files_bytes' => (int) File::query()->sum('size'),
|
'files_bytes' => (int) File::query()->sum('size'),
|
||||||
@@ -280,9 +288,12 @@ class DashboardController extends Controller
|
|||||||
*
|
*
|
||||||
* @return list<array{id: int, name: string, used_bytes: int, quota_mb: int}>
|
* @return list<array{id: int, name: string, used_bytes: int, quota_mb: int}>
|
||||||
*/
|
*/
|
||||||
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'))
|
->select('uploaded_by', DB::raw('SUM(size) as total_bytes'))
|
||||||
->whereHas('uploader', fn ($query) => $query->where('type', UserType::Client))
|
->whereHas('uploader', fn ($query) => $query->where('type', UserType::Client))
|
||||||
->groupBy('uploaded_by')
|
->groupBy('uploaded_by')
|
||||||
@@ -342,7 +353,14 @@ class DashboardController extends Controller
|
|||||||
$staffModule = $this->capabilities->has(Capability::UsersManage) && $viewer->can('manage_users');
|
$staffModule = $this->capabilities->has(Capability::UsersManage) && $viewer->can('manage_users');
|
||||||
$canStaffUsers = $staffModule && $viewer->can('edit_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')
|
->with('uploader:id,name,type')
|
||||||
->orderByDesc('size')
|
->orderByDesc('size')
|
||||||
->limit(10)
|
->limit(10)
|
||||||
@@ -381,8 +399,20 @@ class DashboardController extends Controller
|
|||||||
$canFiles = $viewer->can('upload') || $viewer->can('edit_files') || $viewer->can('edit_others_files');
|
$canFiles = $viewer->can('upload') || $viewer->can('edit_files') || $viewer->can('edit_others_files');
|
||||||
|
|
||||||
return [
|
return [
|
||||||
'count' => File::query()->expired()->count(),
|
// Both the count and the list read the viewer's library, so
|
||||||
'files' => array_values(File::query()->expired()->orderBy('expires_at')->limit(10)
|
// 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'])
|
->get(['id', 'name', 'expires_at'])
|
||||||
->map(fn (File $file): array => [
|
->map(fn (File $file): array => [
|
||||||
'id' => $file->id,
|
'id' => $file->id,
|
||||||
|
|||||||
@@ -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'),
|
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),
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user