diff --git a/app/Modules/Identity/Http/Controllers/Api/UsersController.php b/app/Modules/Identity/Http/Controllers/Api/UsersController.php index df1163f6..73953871 100644 --- a/app/Modules/Identity/Http/Controllers/Api/UsersController.php +++ b/app/Modules/Identity/Http/Controllers/Api/UsersController.php @@ -176,15 +176,27 @@ class UsersController extends Controller 'assigned_clients.*' => ['integer', Rule::in($this->accounts->assignableClientIds($actor))], ]); + // Read through Request::boolean() rather than off the validated + // array, for the reason RolesController::guardScopeRemoval spells + // out: the `boolean` rule accepts 0 and "0" as well as false but + // does not cast, so a strict comparison lets through a value the + // model's own `boolean` cast then stores as false anyway. The same + // value goes to the guard and to the write. + $deactivating = array_key_exists('active', $validated) && ! $request->boolean('active'); + // The same refusal the web screen makes, and for the same reason: // locking yourself out is never what was meant. - if ($user->is($actor) && ($validated['active'] ?? true) === false) { + if ($user->is($actor) && $deactivating) { throw ValidationException::withMessages([ 'active' => __('You cannot deactivate your own account.'), ]); } - $attributes = array_intersect_key($validated, array_flip(['name', 'email', 'active', 'password'])); + $attributes = array_intersect_key($validated, array_flip(['name', 'email', 'password'])); + + if (array_key_exists('active', $validated)) { + $attributes['active'] = $request->boolean('active'); + } if (array_key_exists('role_id', $validated)) { $attributes['role_id'] = (int) $validated['role_id']; diff --git a/tests/Feature/Api/UsersTest.php b/tests/Feature/Api/UsersTest.php index c4a001a1..66c3f9d4 100644 --- a/tests/Feature/Api/UsersTest.php +++ b/tests/Feature/Api/UsersTest.php @@ -351,6 +351,35 @@ test('a caller cannot deactivate or delete their own account', function () { ->assertJsonPath('errors.user.0', 'You cannot delete your own account.'); }); +test('self-deactivation is refused however the boolean is written', function (mixed $active) { + // A second administrator, so the last-administrator guard is not what + // refuses this: with only one, that guard answers first and the refusal + // under test here is never reached. + User::factory()->create(); + + $this->withToken($this->token)->patchJson("/api/v1/users/{$this->admin->id}", ['active' => $active]) + ->assertStatus(422) + ->assertJsonPath('errors.active.0', 'You cannot deactivate your own account.'); + + expect($this->admin->refresh()->active)->toBeTrue(); +})->with([ + 'false' => [false], + 'zero' => [0], + 'the string zero' => ['0'], +]); + +test('deactivating somebody else still works in every one of those forms', function (mixed $active) { + $user = User::factory()->role(SystemRole::Uploader)->create(); + + $this->withToken($this->token)->patchJson("/api/v1/users/{$user->id}", ['active' => $active])->assertOk(); + + expect($user->refresh()->active)->toBeFalse(); +})->with([ + 'false' => [false], + 'zero' => [0], + 'the string zero' => ['0'], +]); + /* |-------------------------------------------------------------------------- | Deletion and its content