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 ba05c379..df1163f6 100644 --- a/app/Modules/Identity/Http/Controllers/Api/UsersController.php +++ b/app/Modules/Identity/Http/Controllers/Api/UsersController.php @@ -127,7 +127,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([ @@ -165,7 +169,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 0ab113c2..d240b36f 100644 --- a/app/Modules/Identity/Http/Controllers/UsersController.php +++ b/app/Modules/Identity/Http/Controllers/UsersController.php @@ -118,7 +118,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([ @@ -178,7 +181,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 @@ -272,13 +278,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 017377ff..f23c8562 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\Erasure\ErasureSchedule; use App\Modules\Identity\Models\Role; use App\Modules\Identity\Permissions\PermissionChecker; @@ -36,6 +37,7 @@ class StaffAccounts private readonly ActivityLogger $activity, private readonly PermissionChecker $permissions, private readonly ErasureSchedule $erasure, + private readonly StaffLibraryScope $library, ) {} /** @@ -92,6 +94,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 d651d7d5..aa87ab86 100644 --- a/docs/api/openapi.json +++ b/docs/api/openapi.json @@ -3240,7 +3240,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." } } }, @@ -3376,7 +3377,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..ae3c2273 --- /dev/null +++ b/tests/Feature/Identity/AssignedClientsAuthorityTest.php @@ -0,0 +1,210 @@ +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 () { + // A client this rep actually holds. #1702 refuses the promotion of a + // stranger client at 404 before validation runs, which is the stronger + // refusal but not the one this case is about: the point here is that + // the assigned_clients rule refuses, so the request has to reach it. + $target = $this->mine; + + $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); +});