gsadmin/projectsend
1
0
Fork 0
mirror of https://github.com/projectsend/projectsend.git synced 2026-09-12 06:48:55 +00:00
Code Issues Packages Projects Releases Wiki Activity

Rank top clients by roster, not by library, and factor the client guard

Browse Source 
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.
This commit is contained in:
ignacionelson
2026-08-26 18:24:13 -03:00
parent 6a5c9e55aa
commit 12a8ebe380
5 changed files with 85 additions and 69 deletions
Download Patch File Download Diff File Expand all files Collapse all files
app/Modules/Audit/Http/Controllers/DashboardController.php
+12 -4
View File
@@ -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)
app/Modules/Clients/Http/Controllers/Api/ClientsController.php
+14 -31
View File
@@ -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);
app/Modules/Clients/Http/Controllers/ClientsController.php
+18 -31
View File
@@ -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);
docs/api/openapi.json
+3 -3
View File
@@ -975,12 +975,12 @@
}
}
},
"404": {
"$ref": "#/components/responses/ModelNotFoundException"
},
"422": {
"$ref": "#/components/responses/ValidationException"
},
"404": {
"$ref": "#/components/responses/ModelNotFoundException"
},
"401": {
"$ref": "#/components/responses/AuthenticationException"
}
tests/Feature/Audit/DashboardTest.php
+38
View File
@@ -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'),
);
});