diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ae44d3a..f79594b6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -226,6 +226,16 @@ a version is cut. (found, diagnosed and fixed by [@denkfabrik-li](https://github.com/denkfabrik-li) in [#1705](https://github.com/projectsend/projectsend/pull/1705)) +- **A limited staff role no longer reaches every client record, or every file name on the + dashboard.** Two more places where holding a permission was treated as holding a boundary. The + clients screen listed every client on the installation by name and email, and a role limited to + its own assigned clients could open, rename, or delete any of them — the same through the API. + Separately, the dashboard's largest-files, expired-files and top-clients widgets named files and + clients from across the whole installation, which mattered more because the Client Manager role + that ships with ProjectSend holds the permission those widgets need. Both now use the same rule + the rest of the library already did. Installation-wide totals stay installation-wide: a count + carries no names. Nothing changes for an administrator or any unrestricted role. + ## 2.1.0 — 18 August 2026 Updating, mostly. ProjectSend now tells you when there is a new version, ends an update somewhere diff --git a/app/Modules/Clients/Http/Controllers/Api/ClientsController.php b/app/Modules/Clients/Http/Controllers/Api/ClientsController.php index 919ebf1d..dcc0cd23 100644 --- a/app/Modules/Clients/Http/Controllers/Api/ClientsController.php +++ b/app/Modules/Clients/Http/Controllers/Api/ClientsController.php @@ -11,6 +11,7 @@ use App\Modules\Audit\Action; use App\Modules\Audit\ActivityLogger; use App\Modules\Clients\ClientCustomFieldType; use App\Modules\Clients\ClientStorageUsage; +use App\Modules\Files\Access\StaffLibraryScope; use App\Modules\Clients\Http\Resources\Api\ClientResource; use App\Modules\Clients\Models\ClientCustomField; use App\Modules\Clients\Models\ClientCustomFieldValue; @@ -54,6 +55,7 @@ class ClientsController extends Controller private readonly ClientStorageUsage $storageUsage, private readonly DeletedAccountContent $accountContent, private readonly AccountContentDeletion $accountDeletion, + private readonly StaffLibraryScope $scope, ) {} public function index(Request $request): AnonymousResourceCollection @@ -63,7 +65,12 @@ class ClientsController extends Controller 'status' => ['nullable', Rule::in(['active', 'inactive'])], ]); - $query = User::query()->where('type', UserType::Client); + // Narrowed the same way the web listing is, and by the same + // rule the object routes below are guarded with. + $viewer = $request->user(); + assert($viewer !== null); + + $query = $this->scope->clients($viewer); if (($filters['search'] ?? null) !== null) { $search = $filters['search']; @@ -79,10 +86,19 @@ class ClientsController extends Controller return ClientResource::collection($this->polling->paginate($request, $query, 'users')); } - public function show(User $client): ClientResource + public function show(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); + + return $this->resourceFor($client); } @@ -135,6 +151,15 @@ class ClientsController extends Controller { 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); + + $validated = $request->validate([ 'name' => ['sometimes', 'string', 'max:255'], 'email' => ['sometimes', 'string', 'lowercase', 'email', 'max:255', Rule::unique('users', 'email')->ignore($client->id)], @@ -203,10 +228,19 @@ class ClientsController extends Controller * in the activity log against the caller. Answers 204 whether or not a * second factor was actually in force. */ - public function destroyTwoFactor(User $client, TwoFactorAdministration $twoFactor): JsonResponse + 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); + + $twoFactor->reset($client); return response()->json(status: 204); @@ -232,6 +266,15 @@ class ClientsController extends Controller { 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); + + $validated = $this->accountDeletion->validate($request, $client); $name = $client->name; diff --git a/app/Modules/Clients/Http/Controllers/ClientsController.php b/app/Modules/Clients/Http/Controllers/ClientsController.php index b3d58555..278c51de 100644 --- a/app/Modules/Clients/Http/Controllers/ClientsController.php +++ b/app/Modules/Clients/Http/Controllers/ClientsController.php @@ -10,6 +10,7 @@ use App\Modules\Audit\Action; use App\Modules\Audit\ActivityLogger; use App\Modules\Clients\ClientCustomFieldType; use App\Modules\Clients\ClientStorageUsage; +use App\Modules\Files\Access\StaffLibraryScope; use App\Modules\Clients\Models\ClientCustomField; use App\Modules\Clients\Models\ClientCustomFieldValue; use App\Modules\Clients\Notifications\ClientAccountEditedNotification; @@ -44,6 +45,7 @@ class ClientsController extends Controller private readonly ClientStorageUsage $storageUsage, private readonly DeletedAccountContent $accountContent, private readonly AccountContentDeletion $accountDeletion, + private readonly StaffLibraryScope $scope, ) {} public function index(Request $request): Response @@ -58,8 +60,14 @@ class ClientsController extends Controller 'status' => $validated['status'] ?? null, ]; - $clients = User::query() - ->where('type', UserType::Client) + // Narrowed by the same rule the buttons on each row are guarded + // with. A client-scoped staff member is not shown the name and + // email of somebody they can reach nothing of — the same thing + // MembershipRequest::approvableBy does for its queue. + $viewer = $request->user(); + assert($viewer !== null); + + $clients = $this->scope->clients($viewer) ->when($filters['search'], fn (Builder $query, string $search) => $query->where(fn (Builder $q) => $q ->where('name', 'like', "%{$search}%") ->orWhere('email', 'like', "%{$search}%"))) @@ -133,10 +141,19 @@ class ClientsController extends Controller return redirect()->route('clients.edit', $client)->with('success', __('Client created.')); } - public function edit(User $client): Response + public function edit(Request $request, User $client): Response { 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); + + return Inertia::render('clients/edit', [ 'client' => [ 'id' => $client->id, @@ -162,6 +179,15 @@ class ClientsController extends Controller { 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); + + $validated = $request->validate(array_merge([ 'name' => ['required', 'string', 'max:255'], 'email' => ['required', 'string', 'lowercase', 'email', 'max:255', Rule::unique('users', 'email')->ignore($client->id)], @@ -219,10 +245,19 @@ class ClientsController extends Controller * Remove this account's second factor, for the client who has lost * their authenticator and their recovery codes. */ - public function destroyTwoFactor(User $client, TwoFactorAdministration $twoFactor): RedirectResponse + 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); + + $twoFactor->reset($client); return back()->with('success', __('Two-factor authentication removed.')); @@ -232,6 +267,15 @@ class ClientsController extends Controller { 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); + + $validated = $this->accountDeletion->validate($request, $client); $name = $client->name; diff --git a/app/Modules/Files/Access/StaffLibraryScope.php b/app/Modules/Files/Access/StaffLibraryScope.php index 486e1d8b..bcd8e9d6 100644 --- a/app/Modules/Files/Access/StaffLibraryScope.php +++ b/app/Modules/Files/Access/StaffLibraryScope.php @@ -10,6 +10,7 @@ use App\Modules\Files\Models\FileAssignment; use App\Modules\Files\Models\Folder; use App\Modules\Files\Models\FolderAssignment; use App\Modules\Groups\Models\Group; +use App\Modules\Identity\UserType; use Illuminate\Database\Eloquent\Builder; /** @@ -176,6 +177,25 @@ class StaffLibraryScope return $ids === null || in_array($client->id, $ids, true); } + /** + * Every client this staff member may act on, as a query. + * + * The listing half of canAssignClient(), so a screen narrows by the + * same rule its buttons are guarded with rather than restating it — + * which is how ClientsController came to list every client on the + * installation, name and email, to a viewer who could reach nothing + * of theirs. An unscoped user gets the whole roster, unchanged. + * + * @return Builder + */ + public function clients(User $user): Builder + { + $query = User::query()->where('type', UserType::Client); + $ids = $this->assignableClientIds($user); + + return $ids === null ? $query : $query->whereIn('id', $ids); + } + public function canAssignGroup(User $user, Group $group): bool { $ids = $this->assignableGroupIds($user); diff --git a/tests/Feature/Clients/ClientLibraryScopeTest.php b/tests/Feature/Clients/ClientLibraryScopeTest.php new file mode 100644 index 00000000..ee5ec99d --- /dev/null +++ b/tests/Feature/Clients/ClientLibraryScopeTest.php @@ -0,0 +1,92 @@ +admin = User::factory()->create(); + + $role = Role::query()->create(['name' => 'Scoped manager', 'client_scoped' => true]); + foreach ([Permission::ManageClients, Permission::EditClients, Permission::DeleteClients, Permission::Upload] as $permission) { + RolePermission::query()->create(['role_id' => $role->id, 'permission' => $permission->value]); + } + + $this->manager = User::factory()->create(['role_id' => $role->id]); + $this->mine = User::factory()->client()->create(['name' => 'Mine']); + $this->manager->assignedClients()->sync([$this->mine->id]); + + $this->stranger = User::factory()->client()->create(['name' => 'Stranger', 'email' => 'stranger@example.test']); +}); + +test('the client list names only the clients this manager holds', function () { + $this->actingAs($this->manager)->get('/clients')->assertInertia( + fn (AssertableInertia $page) => $page + ->has('clients', 1) + ->where('clients.0.name', 'Mine'), + ); +}); + +test('a stranger client cannot be opened, changed, or deleted', function () { + $this->actingAs($this->manager)->get("/clients/{$this->stranger->id}")->assertNotFound(); + + $this->actingAs($this->manager)->patch("/clients/{$this->stranger->id}", [ + 'name' => 'Renamed By Somebody Else', + 'email' => $this->stranger->email, + 'active' => true, + ])->assertNotFound(); + + // password.confirm sits in front of this one, so the session has to + // say it was confirmed or the redirect answers before the guard does. + $this->actingAs($this->manager) + ->withSession(['auth.password_confirmed_at' => time()]) + ->delete("/clients/{$this->stranger->id}/two-factor") + ->assertNotFound(); + $this->actingAs($this->manager)->delete("/clients/{$this->stranger->id}")->assertNotFound(); + + expect($this->stranger->fresh()->name)->toBe('Stranger'); +}); + +test('the API answers the same way', function () { + Sanctum::actingAs($this->manager, ['manage_clients', 'edit_clients', 'delete_clients']); + + $this->getJson('/api/v1/clients')->assertOk()->assertJsonCount(1, 'data'); + $this->getJson("/api/v1/clients/{$this->stranger->id}")->assertNotFound(); + $this->patchJson("/api/v1/clients/{$this->stranger->id}", ['name' => 'Nope'])->assertNotFound(); + $this->deleteJson("/api/v1/clients/{$this->stranger->id}/two-factor")->assertNotFound(); + $this->deleteJson("/api/v1/clients/{$this->stranger->id}")->assertNotFound(); + + expect(User::query()->whereKey($this->stranger->id)->exists())->toBeTrue(); +}); + +test('a client this manager does hold stays fully manageable', function () { + $this->actingAs($this->manager)->get("/clients/{$this->mine->id}")->assertOk(); + + $this->actingAs($this->manager)->patch("/clients/{$this->mine->id}", [ + 'name' => 'Mine, Renamed', + 'email' => $this->mine->email, + 'active' => true, + ])->assertRedirect(); + + expect($this->mine->fresh()->name)->toBe('Mine, Renamed'); +}); + +test('an unscoped administrator reaches every client exactly as before', function () { + $this->actingAs($this->admin)->get('/clients')->assertInertia( + fn (AssertableInertia $page) => $page->has('clients', 2), + ); + + $this->actingAs($this->admin)->get("/clients/{$this->stranger->id}")->assertOk(); +});