From ab6e9eecf3dc606d2ddd1670eeb38192e64c2bdb Mon Sep 17 00:00:00 2001 From: denkfabrik-li <274324701+denkfabrik-li@users.noreply.github.com> Date: Thu, 27 Aug 2026 23:59:45 +0200 Subject: [PATCH] Ask the seat cap where a pending client is approved through edit() SeatAllowance says a cap is only a cap if every door asks, and has a test per door for that reason. Two doors do not ask. The moment a seat is spent is the moment `account_requested` is cleared. Five places do that. approve(), both store()s and ClientProvisioning ask guardClient(); AccountConversion asks it through guardToClient(). The two update()s -- web and API -- clear the flag with no guard at all, under a comment that names exactly what they are doing: // Activating a pending account through the edit screen counts as // approval and clears the request flag. Measured with clients: 0, one pending registration: POST /account-requests/{id}/approve refused, flag still set PATCH /clients/{id} active=true approved, clientUsed() 0 -> 1 PATCH /api/v1/clients/{id} active=true approved, clientUsed() 0 -> 1 A managed installation at its cap therefore keeps taking clients on, from the edit screen or a PATCH, for as long as registrations keep arriving -- and self-registration is open to strangers, so the supply is not the operator's to control. Inside the branch, not above it. Above it, an installation sitting at its cap could not rename a client it already holds, which would trade one wrong refusal for another. There is a test pinning that. The field is `active` rather than the default `email`: on this screen the administrator is toggling `active`, and an error under the email field would point at the wrong thing. approve() has no form of its own, so it keeps the default. Three tests, per door as the file's other eight are. The two door tests were measured red against the unguarded controllers (2 failed / 18 passed). The third -- that editing an existing client still works at the cap -- is green either way: it guards against the fix being written a line too high, not against the bug. Full suite passes (2051 passed / 2 skipped), PHPStan level 8 clean. One thing worth knowing that this branch does not touch: on a parallel run, `UpdateWelcomeTest > staff who may not read...` fails roughly one run in six on untouched main, with `BindingResolutionException: Target [Inertia\Ssr\Gateway] is not instantiable`. Measured over 24 baseline runs before this change existed. It is not this fix, and it is not in scope here, but it will start being visible as soon as the workflow parses again. --- .../Controllers/Api/ClientsController.php | 4 ++ .../Http/Controllers/ClientsController.php | 7 ++- tests/Feature/Platform/SeatAllowanceTest.php | 48 +++++++++++++++++++ 3 files changed, 58 insertions(+), 1 deletion(-) diff --git a/app/Modules/Clients/Http/Controllers/Api/ClientsController.php b/app/Modules/Clients/Http/Controllers/Api/ClientsController.php index 2eb06003..e2a5df85 100644 --- a/app/Modules/Clients/Http/Controllers/Api/ClientsController.php +++ b/app/Modules/Clients/Http/Controllers/Api/ClientsController.php @@ -191,7 +191,11 @@ class ClientsController extends Controller $client->storage_quota_mb = $validated['storage_quota_mb'] ?? 0; } + // Approval, and so the moment the seat is spent — same rule the + // web edit screen and approve() answer to. Inside the branch, so a + // capped installation can still edit a client it already holds. if (($validated['active'] ?? false) && $client->account_requested) { + $this->seats->guardClient('active'); $client->account_requested = false; } diff --git a/app/Modules/Clients/Http/Controllers/ClientsController.php b/app/Modules/Clients/Http/Controllers/ClientsController.php index a7abe62b..d25ae216 100644 --- a/app/Modules/Clients/Http/Controllers/ClientsController.php +++ b/app/Modules/Clients/Http/Controllers/ClientsController.php @@ -234,8 +234,13 @@ class ClientsController extends Controller ]); // Activating a pending account through the edit screen counts as - // approval and clears the request flag. + // approval and clears the request flag — which is the moment a + // seat is spent, so the cap is asked here for the same reason + // AccountRequestsController::approve() asks it one screen over. + // Inside the branch, not above it: an installation at its cap must + // still be able to rename a client it already has. if ($client->account_requested && $validated['active']) { + $this->seats->guardClient('active'); $client->account_requested = false; } diff --git a/tests/Feature/Platform/SeatAllowanceTest.php b/tests/Feature/Platform/SeatAllowanceTest.php index 80055756..ee730d48 100644 --- a/tests/Feature/Platform/SeatAllowanceTest.php +++ b/tests/Feature/Platform/SeatAllowanceTest.php @@ -9,6 +9,7 @@ use App\Modules\Platform\Seats\SeatAllowance; use App\Modules\Platform\Settings\Setting; use App\Modules\Platform\Settings\Settings; use Illuminate\Validation\ValidationException; +use Laravel\Sanctum\Sanctum; /** * A cap is only a cap if every door asks. @@ -191,6 +192,53 @@ test('door: approving an account request', function () { expect($pending->refresh()->account_requested)->toBeTrue(); }); +test('door: approving a pending client through the edit screen', function () { + seatLimits(clients: 0); + + $pending = User::factory()->client()->create(['account_requested' => true, 'active' => false]); + + $this->actingAs($this->admin)->patch("/clients/{$pending->id}", [ + 'name' => $pending->name, + 'email' => $pending->email, + 'active' => true, + ])->assertSessionHasErrors('active'); + + // Nothing is written: the guard throws before save(), so a refused + // approval does not leave the name or the flag half-applied. + expect($pending->refresh()->account_requested)->toBeTrue() + ->and($pending->active)->toBeFalse(); +}); + +test('door: approving a pending client through the API', function () { + seatLimits(clients: 0); + + $pending = User::factory()->client()->create(['account_requested' => true, 'active' => false]); + + Sanctum::actingAs($this->admin, ['*']); + + $this->patchJson("/api/v1/clients/{$pending->id}", ['active' => true]) + ->assertStatus(422) + ->assertJsonValidationErrors('active'); + + expect($pending->refresh()->account_requested)->toBeTrue(); +}); + +test('the cap does not block editing a client the installation already holds', function () { + // The guard sits inside the approval branch. Above it, an installation + // sitting at its cap could not rename anybody. + seatLimits(clients: 0); + + $client = User::factory()->client()->create(['account_requested' => false, 'active' => true]); + + $this->actingAs($this->admin)->patch("/clients/{$client->id}", [ + 'name' => 'Renamed Ltd', + 'email' => $client->email, + 'active' => true, + ])->assertSessionHasNoErrors(); + + expect($client->refresh()->name)->toBe('Renamed Ltd'); +}); + test('door: demoting a staff account to client', function () { seatLimits(clients: 0);