From 3af82357298864c26e202978678919fabf0714ed Mon Sep 17 00:00:00 2001 From: denkfabrik-li <274324701+denkfabrik-li@users.noreply.github.com> Date: Wed, 26 Aug 2026 03:46:12 +0200 Subject: [PATCH] Nobody hands out reach they do not hold either MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit StaffAccounts opens with the rule for roles: "Nobody hands out authority they do not hold … that turns one permission into every permission and makes the rest of the matrix decorative." mayGrant enforces it, and guardTarget applies the same test to an existing account. The client roster never got the same treatment. `assigned_clients` was validated as `exists:users,id where type = client` and passed straight to syncAssignedClients, with nothing anywhere asking whether the actor holds the clients they are handing out. Assigning a client is not a label: it is that client's whole library, given to whoever is on the other end. The case that matters is the actor's own account. guardTarget returns immediately when the target is the actor — editing your own name and email is not a question of authority — so a client-scoped staff member with edit_users could PATCH their own id with every client on the installation and read the whole library from then on. assignableClientIds() answers the roster question the way assignableRoleIds() answers the role one, and the five places that accept `assigned_clients` — users store and update on both surfaces, and the account conversion — validate against it. It returns the full roster for an unrestricted actor, so every caller validates against one list instead of composing a conditional rule; that list is already client-typed, which is why one rule replaces the exists() and the type filter together. The two pickers that offer the roster are narrowed to the same list, so a form no longer offers a client the request behind it will refuse. syncAssignedClients is untouched: which ids stick to which role is its decision and it was never the problem. --- .../AccountConversionController.php | 7 +- .../Http/Controllers/Api/UsersController.php | 12 +- .../Http/Controllers/UsersController.php | 16 +- app/Modules/Identity/StaffAccounts.php | 35 +++ docs/api/openapi.json | 6 +- .../Identity/AssignedClientsAuthorityTest.php | 206 ++++++++++++++++++ 6 files changed, 272 insertions(+), 10 deletions(-) create mode 100644 tests/Feature/Identity/AssignedClientsAuthorityTest.php diff --git a/app/Modules/Identity/Http/Controllers/AccountConversionController.php b/app/Modules/Identity/Http/Controllers/AccountConversionController.php index 191cab3c..8a174bd7 100644 --- a/app/Modules/Identity/Http/Controllers/AccountConversionController.php +++ b/app/Modules/Identity/Http/Controllers/AccountConversionController.php @@ -120,7 +120,9 @@ class AccountConversionController extends Controller 'is_system' => $role->is_system, 'client_scoped' => $role->client_scoped, ])->all(), - 'clients' => User::query()->where('type', UserType::Client)->orderBy('name')->get() + // Narrowed like `roles` beside it: the picker offers what this + // actor may hand out, which is what store() will accept. + 'clients' => User::query()->whereIn('id', $this->accounts->assignableClientIds($actor))->orderBy('name')->get() ->map(fn (User $client): array => ['id' => $client->id, 'name' => $client->name]) ->values()->all(), ]); @@ -152,7 +154,8 @@ class AccountConversionController extends Controller 'assigned_clients' => ['array'], 'assigned_clients.*' => [ 'integer', - Rule::exists('users', 'id')->where('type', UserType::Client->value), + // Reach, not a label: see StaffAccounts::assignableClientIds. + Rule::in($this->accounts->assignableClientIds($actor)), Rule::notIn([$user->id]), ], // Required only for an account whose credential lives in the diff --git a/app/Modules/Identity/Http/Controllers/Api/UsersController.php b/app/Modules/Identity/Http/Controllers/Api/UsersController.php index 3874459f..0742fb81 100644 --- a/app/Modules/Identity/Http/Controllers/Api/UsersController.php +++ b/app/Modules/Identity/Http/Controllers/Api/UsersController.php @@ -125,7 +125,11 @@ class UsersController extends Controller // to mistype. Password::defaults() still applies. 'password' => ['required', Password::defaults()], 'assigned_clients' => ['array'], - 'assigned_clients.*' => ['integer', Rule::exists('users', 'id')->where('type', UserType::Client->value)], + // Only clients you can reach yourself: an unrestricted account may + // assign any client, a client-scoped one only the clients already + // assigned to it. Assigning a client hands over everything that + // client can see, so it follows the same rule as role_id above. + 'assigned_clients.*' => ['integer', Rule::in($this->accounts->assignableClientIds($actor))], ]); $user = $this->accounts->create([ @@ -163,7 +167,11 @@ class UsersController extends Controller 'active' => ['sometimes', 'boolean'], 'password' => ['sometimes', 'nullable', Password::defaults()], 'assigned_clients' => ['sometimes', 'array'], - 'assigned_clients.*' => ['integer', Rule::exists('users', 'id')->where('type', UserType::Client->value)], + // Only clients you can reach yourself: an unrestricted account may + // assign any client, a client-scoped one only the clients already + // assigned to it. Assigning a client hands over everything that + // client can see, so it follows the same rule as role_id above. + 'assigned_clients.*' => ['integer', Rule::in($this->accounts->assignableClientIds($actor))], ]); // The same refusal the web screen makes, and for the same reason: diff --git a/app/Modules/Identity/Http/Controllers/UsersController.php b/app/Modules/Identity/Http/Controllers/UsersController.php index b09c438a..75396574 100644 --- a/app/Modules/Identity/Http/Controllers/UsersController.php +++ b/app/Modules/Identity/Http/Controllers/UsersController.php @@ -116,7 +116,10 @@ class UsersController extends Controller 'role_id' => ['required', 'integer', Rule::in($this->accounts->assignableRoleIds($this->actor()))], 'password' => ['required', 'confirmed', Password::defaults()], 'assigned_clients' => ['array'], - 'assigned_clients.*' => ['integer', Rule::exists('users', 'id')->where('type', UserType::Client->value)], + // Reach, not a label: see StaffAccounts::assignableClientIds. + // The list is client-typed already, so this is one rule where + // an exists() plus a type filter used to be two. + 'assigned_clients.*' => ['integer', Rule::in($this->accounts->assignableClientIds($this->actor()))], ]); $user = $this->accounts->create([ @@ -171,7 +174,10 @@ class UsersController extends Controller 'active' => ['required', 'boolean'], 'password' => ['nullable', 'confirmed', Password::defaults()], 'assigned_clients' => ['array'], - 'assigned_clients.*' => ['integer', Rule::exists('users', 'id')->where('type', UserType::Client->value)], + // Reach, not a label: see StaffAccounts::assignableClientIds. + // The list is client-typed already, so this is one rule where + // an exists() plus a type filter used to be two. + 'assigned_clients.*' => ['integer', Rule::in($this->accounts->assignableClientIds($this->actor()))], ]); // Deactivating yourself is refused here rather than in StaffAccounts @@ -259,13 +265,15 @@ class UsersController extends Controller } /** - * The client roster, for the assigned-clients picker. + * The client roster, for the assigned-clients picker — narrowed to + * what this actor may actually hand out, the same way roleOptions() + * is narrowed to the roles they may grant. * * @return array */ private function clientOptions(): array { - return User::query()->where('type', UserType::Client)->orderBy('name')->get() + return User::query()->whereIn('id', $this->accounts->assignableClientIds($this->actor()))->orderBy('name')->get() ->map(fn (User $client): array => ['id' => $client->id, 'name' => $client->name]) ->values()->all(); } diff --git a/app/Modules/Identity/StaffAccounts.php b/app/Modules/Identity/StaffAccounts.php index ff86dcd1..49745e08 100644 --- a/app/Modules/Identity/StaffAccounts.php +++ b/app/Modules/Identity/StaffAccounts.php @@ -7,6 +7,7 @@ namespace App\Modules\Identity; use App\Models\User; use App\Modules\Audit\Action; use App\Modules\Audit\ActivityLogger; +use App\Modules\Files\Access\StaffLibraryScope; use App\Modules\Identity\Models\Role; use App\Modules\Identity\Permissions\PermissionChecker; use App\Modules\Identity\Permissions\SystemRole; @@ -34,6 +35,7 @@ class StaffAccounts public function __construct( private readonly ActivityLogger $activity, private readonly PermissionChecker $permissions, + private readonly StaffLibraryScope $library, ) {} /** @@ -90,6 +92,39 @@ class StaffAccounts return array_values($this->assignableRoles($actor)->map(fn (Role $role): int => $role->id)->all()); } + /** + * Client ids this actor may put on a staff account's roster — the same + * rule as mayGrant(), applied to reach instead of to authority. + * + * An assigned client is not a label: it is everything that client can + * see, handed to whoever holds it. So a client-scoped actor may hand + * out the clients they hold and no others — including to themselves, + * which is the case that matters, since guardTarget() lets anybody + * edit their own account and `assigned_clients` was never checked + * against the actor at all. Without this a scoped staff member with + * `edit_users` could PATCH their own id with every client id on the + * installation and read the whole library from then on. + * + * An unrestricted actor gets the full roster back rather than null, so + * every caller can validate against one list instead of composing a + * conditional rule. That list is already client-typed, which is why it + * replaces the `exists:users,id where type = client` rule rather than + * joining it. + * + * @return list + */ + public function assignableClientIds(User $actor): array + { + $ids = $this->library->assignableClientIds($actor); + + if ($ids !== null) { + return $ids; + } + + return array_values(User::query()->where('type', UserType::Client) + ->pluck('id')->map(fn ($id): int => (int) $id)->all()); + } + /** * The same rule applied to an existing account: if the actor could not * grant the target's role, they have no business editing or deleting diff --git a/docs/api/openapi.json b/docs/api/openapi.json index dd322939..e26a063b 100644 --- a/docs/api/openapi.json +++ b/docs/api/openapi.json @@ -3215,7 +3215,8 @@ "assigned_clients": { "type": "array", "items": { - "type": "integer" + "type": "integer", + "description": "Only clients you can reach yourself: an unrestricted account may\nassign any client, a client-scoped one only the clients already\nassigned to it. Assigning a client hands over everything that\nclient can see, so it follows the same rule as role_id above." } } }, @@ -3351,7 +3352,8 @@ "assigned_clients": { "type": "array", "items": { - "type": "integer" + "type": "integer", + "description": "Only clients you can reach yourself: an unrestricted account may\nassign any client, a client-scoped one only the clients already\nassigned to it. Assigning a client hands over everything that\nclient can see, so it follows the same rule as role_id above." } } } diff --git a/tests/Feature/Identity/AssignedClientsAuthorityTest.php b/tests/Feature/Identity/AssignedClientsAuthorityTest.php new file mode 100644 index 00000000..cdda104d --- /dev/null +++ b/tests/Feature/Identity/AssignedClientsAuthorityTest.php @@ -0,0 +1,206 @@ +admin = User::factory()->create(); + + $this->mine = User::factory()->client()->create(['name' => 'Mine']); + $this->stranger = User::factory()->client()->create(['name' => 'Not Mine']); + + // A client-scoped role that manages staff accounts. Nothing shipped + // combines the two; the roles screen offers every combination, and + // ClientScopingTest pins that a custom role can be made scoped. + $this->role = Role::query()->create(['name' => 'Reps '.Str::random(6), 'client_scoped' => true]); + foreach ([Permission::ManageUsers, Permission::EditUsers, Permission::CreateUsers, Permission::EditClients] as $permission) { + RolePermission::query()->create(['role_id' => $this->role->id, 'permission' => $permission->value]); + } + + $this->rep = User::factory()->create(['role_id' => $this->role->id]); + $this->rep->assignedClients()->sync([$this->mine->id]); +}); + +test('a scoped staff member cannot widen their own reach', function () { + // guardTarget() returns immediately for your own account — editing + // your own name and email is not a question of authority — so this + // payload had nothing standing in front of it at all. + $this->actingAs($this->rep)->patch("/users/{$this->rep->id}", [ + 'name' => $this->rep->name, + 'email' => $this->rep->email, + 'role_id' => $this->role->id, + 'active' => true, + 'assigned_clients' => [$this->mine->id, $this->stranger->id], + ])->assertSessionHasErrors('assigned_clients.1'); + + expect($this->rep->assignedClients()->pluck('users.id')->all())->toBe([$this->mine->id]); +}); + +test('a scoped staff member cannot hand a stranger client to somebody else either', function () { + $colleague = User::factory()->create(['role_id' => $this->role->id]); + + $this->actingAs($this->rep)->patch("/users/{$colleague->id}", [ + 'name' => $colleague->name, + 'email' => $colleague->email, + 'role_id' => $this->role->id, + 'active' => true, + 'assigned_clients' => [$this->stranger->id], + ])->assertSessionHasErrors('assigned_clients.0'); + + expect($colleague->assignedClients()->count())->toBe(0); +}); + +test('a scoped staff member cannot mint a new account holding clients they do not', function () { + $this->actingAs($this->rep)->post('/users', [ + 'name' => 'New Rep', + 'email' => 'new-rep@example.test', + 'role_id' => $this->role->id, + 'password' => 'a-strong-password-1', + 'password_confirmation' => 'a-strong-password-1', + 'assigned_clients' => [$this->stranger->id], + ])->assertSessionHasErrors('assigned_clients.0'); + + expect(User::query()->where('email', 'new-rep@example.test')->exists())->toBeFalse(); +}); + +test('their own clients still pass, on create and on edit', function () { + // Not deny-everything: handing on what you hold is the point of the + // roster, and the rule only says you cannot hand on what you do not. + $this->actingAs($this->rep)->post('/users', [ + 'name' => 'New Rep', + 'email' => 'new-rep@example.test', + 'role_id' => $this->role->id, + 'password' => 'a-strong-password-1', + 'password_confirmation' => 'a-strong-password-1', + 'assigned_clients' => [$this->mine->id], + ])->assertSessionHasNoErrors(); + + $created = User::query()->where('email', 'new-rep@example.test')->sole(); + + expect($created->assignedClients()->pluck('users.id')->all())->toBe([$this->mine->id]); +}); + +test('an administrator still assigns any client at all', function () { + $rep = User::factory()->create(['role_id' => $this->role->id]); + + $this->actingAs($this->admin)->patch("/users/{$rep->id}", [ + 'name' => $rep->name, + 'email' => $rep->email, + 'role_id' => $this->role->id, + 'active' => true, + 'assigned_clients' => [$this->mine->id, $this->stranger->id], + ])->assertSessionHasNoErrors(); + + expect($rep->assignedClients()->count())->toBe(2); +}); + +test('a staff account is still not assignable as a client', function () { + // The type filter the old exists() rule carried is inside the list + // this now validates against, so it did not go anywhere. + $staff = User::factory()->create(); + + $this->actingAs($this->admin)->patch("/users/{$this->rep->id}", [ + 'name' => $this->rep->name, + 'email' => $this->rep->email, + 'role_id' => $this->role->id, + 'active' => true, + 'assigned_clients' => [$staff->id], + ])->assertSessionHasErrors('assigned_clients.0'); + + $this->actingAs($this->admin)->patch("/users/{$this->rep->id}", [ + 'name' => $this->rep->name, + 'email' => $this->rep->email, + 'role_id' => $this->role->id, + 'active' => true, + 'assigned_clients' => [999999], + ])->assertSessionHasErrors('assigned_clients.0'); +}); + +test('the API answers the same on both verbs', function () { + $token = $this->rep->createToken('t', [ + Permission::ManageUsers->value, + Permission::EditUsers->value, + Permission::CreateUsers->value, + ])->plainTextToken; + + $this->withToken($token)->patchJson("/api/v1/users/{$this->rep->id}", [ + 'assigned_clients' => [$this->mine->id, $this->stranger->id], + ])->assertJsonValidationErrors('assigned_clients.1'); + + $this->withToken($token)->postJson('/api/v1/users', [ + 'name' => 'New Rep', + 'email' => 'new-rep@example.test', + 'role_id' => $this->role->id, + 'password' => 'a-strong-password-1', + 'assigned_clients' => [$this->stranger->id], + ])->assertJsonValidationErrors('assigned_clients.0'); + + expect($this->rep->assignedClients()->pluck('users.id')->all())->toBe([$this->mine->id]); + + // Their own client still goes through the same endpoint. + $this->withToken($token)->patchJson("/api/v1/users/{$this->rep->id}", [ + 'assigned_clients' => [$this->mine->id], + ])->assertOk(); +}); + +test('converting an account to staff cannot hand out clients either', function () { + $target = User::factory()->client()->create(); + + $this->actingAs($this->rep)->post("/users/convert/{$target->id}", [ + 'direction' => 'to_staff', + 'role_id' => $this->role->id, + 'assigned_clients' => [$this->stranger->id], + 'password' => 'a-strong-password-1', + ])->assertSessionHasErrors('assigned_clients.0'); + + expect($target->refresh()->isClient())->toBeTrue(); +}); + +test('the picker offers what the rule accepts', function () { + // A form that offers a client the server will refuse is a form that + // teaches people the boundary by hitting it — roleOptions() is + // narrowed for exactly this reason, and now so is this. + $names = fn ($clients) => collect($clients)->pluck('name')->all(); + + $this->actingAs($this->rep)->get('/users/create')->assertInertia( + fn (AssertableInertia $page) => $page->where( + 'clients', + fn ($clients) => $names($clients) === ['Mine'], + ), + ); + + $this->actingAs($this->admin)->get('/users/create')->assertInertia( + fn (AssertableInertia $page) => $page->where( + 'clients', + fn ($clients) => in_array('Not Mine', $names($clients), true), + ), + ); +}); + +test('an unscoped role clears the roster as before', function () { + // syncAssignedClients still decides that, and it is untouched: this + // change is about which ids may be offered, not about when they stick. + $plain = Role::query()->where('name', SystemRole::AccountManager->value)->sole(); + + $this->actingAs($this->admin)->patch("/users/{$this->rep->id}", [ + 'name' => $this->rep->name, + 'email' => $this->rep->email, + 'role_id' => $plain->id, + 'active' => true, + 'assigned_clients' => [$this->mine->id], + ])->assertSessionHasNoErrors(); + + expect($this->rep->refresh()->assignedClients()->count())->toBe(0); +});