From d889fc9b62738b5bb4fe5cb40bb4e7257d0511d4 Mon Sep 17 00:00:00 2001 From: romanetar Date: Wed, 7 Oct 2026 19:42:26 +0200 Subject: [PATCH 1/2] feat(mfa): add IRecoveryCodeService::disableTwoFactor Shared service method that disables 2FA for a user: clears two_factor_enabled and two_factor_enforced_at, deletes every recovery code and revokes every trusted device in a single transaction, then records a device_revoked audit event best-effort. The actor decides the password rule: null (console) and another user (admin) need no password, the user acting on their own account must provide the current one. The settings_changed audit event is left to the caller, which knows the context (reason, actor, operator). Group-enforced users are still challenged at login afterwards, as enforcement is derived from group membership. --- app/Services/Auth/IRecoveryCodeService.php | 25 +++ app/Services/Auth/RecoveryCodeService.php | 38 +++++ tests/DisableTwoFactorIntegrationTest.php | 102 +++++++++++ ...ecoveryCodeServiceDisableTwoFactorTest.php | 159 ++++++++++++++++++ 4 files changed, 324 insertions(+) create mode 100644 tests/DisableTwoFactorIntegrationTest.php create mode 100644 tests/unit/RecoveryCodeServiceDisableTwoFactorTest.php diff --git a/app/Services/Auth/IRecoveryCodeService.php b/app/Services/Auth/IRecoveryCodeService.php index aaf38011..845cdd30 100644 --- a/app/Services/Auth/IRecoveryCodeService.php +++ b/app/Services/Auth/IRecoveryCodeService.php @@ -105,4 +105,29 @@ public function countUnusedRecoveryCodes(User $user): int; * @return RecoveryCodesStatus remaining/total/low-threshold/acknowledged/enrollment-required standing */ public function getStatus(User $user): RecoveryCodesStatus; + + /** + * Disables 2FA for the user: clears two_factor_enabled and two_factor_enforced_at, + * deletes every recovery code and revokes every trusted device, all in a single + * transaction. A device_revoked audit event is recorded best-effort afterwards. + * + * $actor identifies who performs the operation: + * - null: a system/console operation (e.g. idp:reset-2fa), no password is required; + * - the same user: self-service, $currentPassword must match; + * - another user: an administrator acting on the account, no password is required. + * Authorizing the administrator is the caller's responsibility. + * + * The caller is responsible for emitting the settings_changed audit event, as only + * it knows the context (reason, actor, operator). + * + * Group-enforced users are still required to complete 2FA at login afterwards, + * as enforcement is derived from group membership (User::shouldRequire2FA()). + * + * @param User $user + * @param string|null $currentPassword + * @param User|null $actor + * @return void + * @throws ValidationException if the actor is the user and $currentPassword does not match + */ + public function disableTwoFactor(User $user, ?string $currentPassword, ?User $actor): void; } diff --git a/app/Services/Auth/RecoveryCodeService.php b/app/Services/Auth/RecoveryCodeService.php index 6394d1ba..4f0ee059 100644 --- a/app/Services/Auth/RecoveryCodeService.php +++ b/app/Services/Auth/RecoveryCodeService.php @@ -17,6 +17,7 @@ use App\libs\Auth\Models\UserRecoveryCode; use Auth\MFAConstants; use Auth\Repositories\IUserRecoveryCodeRepository; +use Auth\Repositories\IUserTrustedDeviceRepository; use Auth\Repositories\IUserRepository; use Auth\User; use Illuminate\Support\Facades\Hash; @@ -42,6 +43,7 @@ final class RecoveryCodeService implements IRecoveryCodeService public function __construct( private readonly IUserRecoveryCodeRepository $repository, private readonly IUserRepository $user_repository, + private readonly IUserTrustedDeviceRepository $trusted_device_repository, private readonly ITransactionService $tx_service, private readonly ITwoFactorAuditService $audit_service, ) { @@ -230,6 +232,42 @@ private function logCodesGenerated(User $user, RecoveryCodesBatch $batch): void } } + /** + * @inheritDoc + */ + public function disableTwoFactor(User $user, ?string $currentPassword, ?User $actor): void + { + // self-service requires the current password; console and admin + // operations do not (see IRecoveryCodeService::disableTwoFactor) + if (!is_null($actor) && $actor->getId() === $user->getId() + && !$user->checkPassword(trim((string)$currentPassword))) { + throw new ValidationException('current_password is not correct.'); + } + + // a single transaction() call: nesting another one (e.g. through the audit + // service) inside it would let an inner failure close the entity manager + // under this still-running outer transaction + $this->tx_service->transaction(function () use ($user) { + $user->disable2FA(); + $this->user_repository->add($user, false); + $this->repository->deleteAllForUser($user); + $this->trusted_device_repository->revokeAllForUser($user); + }); + + // Best-effort: 2FA is already disabled and the codes and devices are gone, + // an audit-logging failure must not make the caller believe it did not happen. + try { + $this->audit_service->log( + $user, + TwoFactorAuditLog::EventDeviceRevoked, + $user->getTwoFactorMethod(), + IPHelper::getUserIp() + ); + } catch (\Throwable $ex) { + Log::warning($ex); + } + } + /** * Invalidates every existing recovery code for the user and generates a * fresh batch, within the caller's already-open transaction. The caller diff --git a/tests/DisableTwoFactorIntegrationTest.php b/tests/DisableTwoFactorIntegrationTest.php new file mode 100644 index 00000000..ccaff2d8 --- /dev/null +++ b/tests/DisableTwoFactorIntegrationTest.php @@ -0,0 +1,102 @@ +service = $this->app->make(IRecoveryCodeService::class); + + $user = new User(); + $user->setEmail('disable-2fa@nomail.com'); + $user->setFirstName('Disable'); + $user->setLastName('TwoFactor'); + $user->setIdentifier('disable-2fa@nomail.com'); + $user->setPassword('P@sswordS3cret'); + $user->verifyEmail(false); + EntityManager::persist($user); + EntityManager::flush(); + $this->user = $user; + + // enrolled, with a code batch and two trusted devices + $this->service->enableTwoFactorAndGenerateCodes($user, User::MFAMethod_OTP); + $devices = $this->app->make(IDeviceTrustService::class); + $devices->trustDevice($user, 'agent-a', '127.0.0.1'); + $devices->trustDevice($user, 'agent-b', '127.0.0.1'); + } + + private function userRow(): object + { + return DB::table('users')->where('id', $this->user->getId())->first(); + } + + public function testDisableClearsEverything(): void + { + $this->assertSame(1, (int)$this->userRow()->two_factor_enabled); + $this->assertSame(10, DB::table('user_recovery_codes')->where('user_id', $this->user->getId())->count()); + + $this->service->disableTwoFactor($this->user, null, null); + + $row = $this->userRow(); + $this->assertSame(0, (int)$row->two_factor_enabled); + $this->assertNull($row->two_factor_enforced_at); + $this->assertSame(0, DB::table('user_recovery_codes')->where('user_id', $this->user->getId())->count()); + + $devices = DB::table('user_trusted_devices')->where('user_id', $this->user->getId())->get(); + $this->assertCount(2, $devices); + foreach ($devices as $device) { + $this->assertSame(1, (int)$device->is_revoked); + } + + $this->assertSame(1, DB::table('two_factor_audit_log') + ->where('user_id', $this->user->getId()) + ->where('event_type', TwoFactorAuditLog::EventDeviceRevoked) + ->count()); + // settings_changed is the caller's responsibility + $this->assertSame(0, DB::table('two_factor_audit_log') + ->where('user_id', $this->user->getId()) + ->where('event_type', TwoFactorAuditLog::EventSettingsChanged) + ->count()); + } + + public function testSelfServiceWithWrongPasswordLeavesStateUntouched(): void + { + try { + $this->service->disableTwoFactor($this->user, 'wrong', $this->user); + $this->fail('ValidationException expected'); + } catch (\models\exceptions\ValidationException $ex) { + } + + $this->assertSame(1, (int)$this->userRow()->two_factor_enabled); + $this->assertSame(10, DB::table('user_recovery_codes')->where('user_id', $this->user->getId())->count()); + $this->assertSame(0, DB::table('user_trusted_devices') + ->where('user_id', $this->user->getId())->where('is_revoked', 1)->count()); + } +} diff --git a/tests/unit/RecoveryCodeServiceDisableTwoFactorTest.php b/tests/unit/RecoveryCodeServiceDisableTwoFactorTest.php new file mode 100644 index 00000000..f65463e9 --- /dev/null +++ b/tests/unit/RecoveryCodeServiceDisableTwoFactorTest.php @@ -0,0 +1,159 @@ +code_repo = Mockery::mock(IUserRecoveryCodeRepository::class); + $this->user_repo = Mockery::mock(IUserRepository::class); + $this->device_repo = Mockery::mock(IUserTrustedDeviceRepository::class); + $this->audit_service = Mockery::mock(ITwoFactorAuditService::class); + $this->tx_service = Mockery::mock(ITransactionService::class); + $this->tx_service->shouldReceive('transaction')->andReturnUsing(fn($cb) => $cb())->byDefault(); + + $this->service = new RecoveryCodeService( + $this->code_repo, + $this->user_repo, + $this->device_repo, + $this->tx_service, + $this->audit_service + ); + } + + protected function tearDown(): void + { + // Mockery expectations are verified on close(): count them as assertions + $this->addToAssertionCount(Mockery::getContainer()->mockery_getExpectationCount()); + Mockery::close(); + parent::tearDown(); + } + + private function buildUser(int $id): \Mockery\MockInterface + { + $user = Mockery::mock(User::class); + $user->shouldReceive('getId')->andReturn($id); + $user->shouldReceive('getTwoFactorMethod')->andReturn(User::MFAMethod_OTP); + return $user; + } + + private function expectFullDisable(\Mockery\MockInterface $user): void + { + $user->shouldReceive('disable2FA')->once(); + $this->user_repo->shouldReceive('add')->once()->with($user, false); + $this->code_repo->shouldReceive('deleteAllForUser')->once()->with($user)->andReturn(10); + $this->device_repo->shouldReceive('revokeAllForUser')->once()->with($user); + $this->audit_service->shouldReceive('log') + ->once() + ->with($user, TwoFactorAuditLog::EventDeviceRevoked, User::MFAMethod_OTP, Mockery::type('string')); + } + + public function testConsoleActorNeedsNoPassword(): void + { + $user = $this->buildUser(1); + $user->shouldNotReceive('checkPassword'); + $this->expectFullDisable($user); + + $this->service->disableTwoFactor($user, null, null); + } + + public function testAdminActorNeedsNoPassword(): void + { + $user = $this->buildUser(1); + $user->shouldNotReceive('checkPassword'); + $admin = $this->buildUser(2); + $this->expectFullDisable($user); + + $this->service->disableTwoFactor($user, null, $admin); + } + + public function testSelfServiceWithCorrectPassword(): void + { + $user = $this->buildUser(1); + $user->shouldReceive('checkPassword')->once()->with('secret')->andReturn(true); + $this->expectFullDisable($user); + + $this->service->disableTwoFactor($user, ' secret ', $user); + } + + public function testSelfServiceWithWrongPasswordChangesNothing(): void + { + $user = $this->buildUser(1); + $user->shouldReceive('checkPassword')->once()->andReturn(false); + $user->shouldNotReceive('disable2FA'); + $this->tx_service->shouldNotReceive('transaction'); + $this->code_repo->shouldNotReceive('deleteAllForUser'); + $this->device_repo->shouldNotReceive('revokeAllForUser'); + $this->audit_service->shouldNotReceive('log'); + + $this->expectException(ValidationException::class); + $this->service->disableTwoFactor($user, 'wrong', $user); + } + + public function testSelfServiceWithoutPasswordIsRejected(): void + { + $user = $this->buildUser(1); + $user->shouldReceive('checkPassword')->once()->with('')->andReturn(false); + $user->shouldNotReceive('disable2FA'); + + $this->expectException(ValidationException::class); + $this->service->disableTwoFactor($user, null, $user); + } + + public function testAuditFailureDoesNotFailTheOperation(): void + { + $user = $this->buildUser(1); + $user->shouldReceive('disable2FA')->once(); + $this->user_repo->shouldReceive('add')->once(); + $this->code_repo->shouldReceive('deleteAllForUser')->once()->andReturn(0); + $this->device_repo->shouldReceive('revokeAllForUser')->once(); + $this->audit_service->shouldReceive('log')->once()->andThrow(new \RuntimeException('audit down')); + + $this->service->disableTwoFactor($user, null, null); + } +} From b106e49d01bc6cafd6ee88df41ffbce44b35b166 Mon Sep 17 00:00:00 2001 From: romanetar Date: Fri, 6 Nov 2026 15:53:18 +0100 Subject: [PATCH 2/2] feat(2fa): audit enrollment_changed and guard group-enforced users in disableTwoFactor - add User::isTwoFactorEnforcedByGroup() and reuse it from shouldRequire2FA() - disableTwoFactor refuses user and admin actors for group-enforced users (console stays allowed as the manual recovery path) - self-service without a password requires the recovery enrollment window - take the users-row lock first and reset recovery_codes_acknowledged_at - audit enrollment_changed with {enabled, actor_id, reason} instead of device_revoked --- app/Services/Auth/IRecoveryCodeService.php | 27 +++-- app/Services/Auth/RecoveryCodeService.php | 40 +++++-- app/libs/Auth/Models/User.php | 18 +++- tests/DisableTwoFactorIntegrationTest.php | 12 ++- ...ecoveryCodeServiceDisableTwoFactorTest.php | 100 +++++++++++++++--- 5 files changed, 158 insertions(+), 39 deletions(-) diff --git a/app/Services/Auth/IRecoveryCodeService.php b/app/Services/Auth/IRecoveryCodeService.php index 845cdd30..53bcaaf1 100644 --- a/app/Services/Auth/IRecoveryCodeService.php +++ b/app/Services/Auth/IRecoveryCodeService.php @@ -107,27 +107,32 @@ public function countUnusedRecoveryCodes(User $user): int; public function getStatus(User $user): RecoveryCodesStatus; /** - * Disables 2FA for the user: clears two_factor_enabled and two_factor_enforced_at, - * deletes every recovery code and revokes every trusted device, all in a single - * transaction. A device_revoked audit event is recorded best-effort afterwards. + * Disables 2FA for the user: clears two_factor_enabled, two_factor_enforced_at and + * recovery_codes_acknowledged_at, deletes every recovery code and revokes every + * trusted device, all in a single transaction under the users-row lock. An + * enrollment_changed audit event with metadata {enabled: false, actor_id, reason} + * is recorded best-effort afterwards; reason is self, admin or console. * * $actor identifies who performs the operation: - * - null: a system/console operation (e.g. idp:reset-2fa), no password is required; - * - the same user: self-service, $currentPassword must match; + * - null: a system/console operation (e.g. idp:reset-2fa), nothing is required; + * - the same user: self-service. With a password, $currentPassword must match. + * Without one (social-only, !hasPasswordSet()) the session must be inside the + * recovery enrollment window (isWithinEnrollmentWindow()); * - another user: an administrator acting on the account, no password is required. * Authorizing the administrator is the caller's responsibility. * - * The caller is responsible for emitting the settings_changed audit event, as only - * it knows the context (reason, actor, operator). - * - * Group-enforced users are still required to complete 2FA at login afterwards, - * as enforcement is derived from group membership (User::shouldRequire2FA()). + * Group-enforced users cannot be disabled by a user or an administrator: enforcement + * is derived from group membership (User::isTwoFactorEnforcedByGroup()), so the call + * is refused. The console path is allowed (manual recovery); such a user is still + * challenged at login afterwards. * * @param User $user * @param string|null $currentPassword * @param User|null $actor * @return void - * @throws ValidationException if the actor is the user and $currentPassword does not match + * @throws ValidationException if the user is enforced by group and $actor is not null, if + * the actor is the user and the password does not match, or the user has no password + * and the session is outside the enrollment window */ public function disableTwoFactor(User $user, ?string $currentPassword, ?User $actor): void; } diff --git a/app/Services/Auth/RecoveryCodeService.php b/app/Services/Auth/RecoveryCodeService.php index 4f0ee059..e9d9168b 100644 --- a/app/Services/Auth/RecoveryCodeService.php +++ b/app/Services/Auth/RecoveryCodeService.php @@ -237,18 +237,39 @@ private function logCodesGenerated(User $user, RecoveryCodesBatch $batch): void */ public function disableTwoFactor(User $user, ?string $currentPassword, ?User $actor): void { - // self-service requires the current password; console and admin - // operations do not (see IRecoveryCodeService::disableTwoFactor) - if (!is_null($actor) && $actor->getId() === $user->getId() - && !$user->checkPassword(trim((string)$currentPassword))) { - throw new ValidationException('current_password is not correct.'); + // Group enforcement does not depend on the stored flag: disabling would only + // delete the recovery codes and trusted devices of a user who is still + // challenged at login. The console (null actor) is the manual recovery path + // (idp:reset-2fa) and stays allowed. + if (!is_null($actor) && $user->isTwoFactorEnforcedByGroup()) { + throw new ValidationException('Two-factor authentication is enforced by group membership and cannot be disabled.'); + } + + $is_self = !is_null($actor) && $actor->getId() === $user->getId(); + + if ($is_self) { + if ($user->hasPasswordSet()) { + if (!$user->checkPassword(trim((string)$currentPassword))) { + throw new ValidationException('current_password is not correct.'); + } + } elseif (!$this->isWithinEnrollmentWindow($user)) { + // Social-only account: no password to confirm. Proof of identity is a + // real 2FA challenge completed moments ago in this session. + throw new ValidationException( + 'Sign out and sign in again, then turn off two-factor authentication right after the sign-in.' + ); + } } // a single transaction() call: nesting another one (e.g. through the audit // service) inside it would let an inner failure close the entity manager // under this still-running outer transaction $this->tx_service->transaction(function () use ($user) { + // serializes with a concurrent regenerate: it also deletes and inserts + // codes. The lock re-hydrates $user, so it must precede disable2FA(). + $this->repository->lockUserRow($user); $user->disable2FA(); + $user->setRecoveryCodesAcknowledgedAt(null); $this->user_repository->add($user, false); $this->repository->deleteAllForUser($user); $this->trusted_device_repository->revokeAllForUser($user); @@ -259,9 +280,14 @@ public function disableTwoFactor(User $user, ?string $currentPassword, ?User $ac try { $this->audit_service->log( $user, - TwoFactorAuditLog::EventDeviceRevoked, + TwoFactorAuditLog::EventEnrollmentChanged, $user->getTwoFactorMethod(), - IPHelper::getUserIp() + IPHelper::getUserIp(), + [ + 'enabled' => false, + 'actor_id' => $actor?->getId(), + 'reason' => $is_self ? 'self' : (is_null($actor) ? 'console' : 'admin'), + ] ); } catch (\Throwable $ex) { Log::warning($ex); diff --git a/app/libs/Auth/Models/User.php b/app/libs/Auth/Models/User.php index 8da14ee8..b4137fcb 100644 --- a/app/libs/Auth/Models/User.php +++ b/app/libs/Auth/Models/User.php @@ -2459,13 +2459,27 @@ public function shouldRequire2FA(): bool if (!config('two_factor.enabled', true)) { return false; } + if ($this->isTwoFactorEnforcedByGroup()) { + return true; + } + return (bool) $this->two_factor_enabled; + } + + /** + * Whether the user belongs to any of the groups listed in + * config('two_factor.enforced_groups'). Group enforcement does not depend on the + * stored two_factor_enabled flag, so it cannot be switched off by disabling it. + * Unlike shouldRequire2FA() this ignores the global two_factor.enabled kill-switch. + */ + public function isTwoFactorEnforcedByGroup(): bool + { $enforcedGroups = config('two_factor.enforced_groups', []); foreach ($enforcedGroups as $slug) { - if($this->belongToGroup($slug)) { + if ($this->belongToGroup($slug)) { return true; } } - return (bool) $this->two_factor_enabled; + return false; } /** diff --git a/tests/DisableTwoFactorIntegrationTest.php b/tests/DisableTwoFactorIntegrationTest.php index ccaff2d8..8065ad5b 100644 --- a/tests/DisableTwoFactorIntegrationTest.php +++ b/tests/DisableTwoFactorIntegrationTest.php @@ -75,10 +75,16 @@ public function testDisableClearsEverything(): void $this->assertSame(1, (int)$device->is_revoked); } - $this->assertSame(1, DB::table('two_factor_audit_log') + $audit = DB::table('two_factor_audit_log') ->where('user_id', $this->user->getId()) - ->where('event_type', TwoFactorAuditLog::EventDeviceRevoked) - ->count()); + ->where('event_type', TwoFactorAuditLog::EventEnrollmentChanged) + ->orderByDesc('id') + ->first(); + $this->assertNotNull($audit); + $metadata = json_decode($audit->metadata, true); + $this->assertFalse($metadata['enabled']); + $this->assertNull($metadata['actor_id']); + $this->assertSame('console', $metadata['reason']); // settings_changed is the caller's responsibility $this->assertSame(0, DB::table('two_factor_audit_log') ->where('user_id', $this->user->getId()) diff --git a/tests/unit/RecoveryCodeServiceDisableTwoFactorTest.php b/tests/unit/RecoveryCodeServiceDisableTwoFactorTest.php index f65463e9..328cefd8 100644 --- a/tests/unit/RecoveryCodeServiceDisableTwoFactorTest.php +++ b/tests/unit/RecoveryCodeServiceDisableTwoFactorTest.php @@ -19,6 +19,8 @@ use Auth\Repositories\IUserRepository; use Auth\Repositories\IUserTrustedDeviceRepository; use Auth\User; +use Auth\MFAConstants; +use Illuminate\Support\Facades\Session; use Mockery; use models\exceptions\ValidationException; use Tests\BrowserKitTestCase; @@ -74,30 +76,57 @@ protected function tearDown(): void parent::tearDown(); } - private function buildUser(int $id): \Mockery\MockInterface + private function buildUser(int $id, bool $enforced = false, bool $hasPassword = true): \Mockery\MockInterface { $user = Mockery::mock(User::class); $user->shouldReceive('getId')->andReturn($id); $user->shouldReceive('getTwoFactorMethod')->andReturn(User::MFAMethod_OTP); + $user->shouldReceive('isTwoFactorEnforcedByGroup')->andReturn($enforced); + $user->shouldReceive('hasPasswordSet')->andReturn($hasPassword); return $user; } - private function expectFullDisable(\Mockery\MockInterface $user): void + private function expectFullDisable(\Mockery\MockInterface $user, ?int $actor_id, string $reason): void { - $user->shouldReceive('disable2FA')->once(); + $this->code_repo->shouldReceive('lockUserRow')->once()->with($user)->ordered(); + $user->shouldReceive('disable2FA')->once()->ordered(); + $user->shouldReceive('setRecoveryCodesAcknowledgedAt')->once()->with(null); $this->user_repo->shouldReceive('add')->once()->with($user, false); $this->code_repo->shouldReceive('deleteAllForUser')->once()->with($user)->andReturn(10); $this->device_repo->shouldReceive('revokeAllForUser')->once()->with($user); $this->audit_service->shouldReceive('log') ->once() - ->with($user, TwoFactorAuditLog::EventDeviceRevoked, User::MFAMethod_OTP, Mockery::type('string')); + ->with( + $user, + TwoFactorAuditLog::EventEnrollmentChanged, + User::MFAMethod_OTP, + Mockery::type('string'), + ['enabled' => false, 'actor_id' => $actor_id, 'reason' => $reason] + ); + } + + private function expectNothingChanges(\Mockery\MockInterface $user): void + { + $user->shouldNotReceive('disable2FA'); + $this->tx_service->shouldNotReceive('transaction'); + $this->code_repo->shouldNotReceive('deleteAllForUser'); + $this->device_repo->shouldNotReceive('revokeAllForUser'); + $this->audit_service->shouldNotReceive('log'); } public function testConsoleActorNeedsNoPassword(): void { $user = $this->buildUser(1); $user->shouldNotReceive('checkPassword'); - $this->expectFullDisable($user); + $this->expectFullDisable($user, null, 'console'); + + $this->service->disableTwoFactor($user, null, null); + } + + public function testConsoleActorMayResetAGroupEnforcedUser(): void + { + $user = $this->buildUser(1, true); + $this->expectFullDisable($user, null, 'console'); $this->service->disableTwoFactor($user, null, null); } @@ -107,8 +136,18 @@ public function testAdminActorNeedsNoPassword(): void $user = $this->buildUser(1); $user->shouldNotReceive('checkPassword'); $admin = $this->buildUser(2); - $this->expectFullDisable($user); + $this->expectFullDisable($user, 2, 'admin'); + + $this->service->disableTwoFactor($user, null, $admin); + } + + public function testAdminActorCannotDisableAGroupEnforcedUser(): void + { + $user = $this->buildUser(1, true); + $admin = $this->buildUser(2); + $this->expectNothingChanges($user); + $this->expectException(ValidationException::class); $this->service->disableTwoFactor($user, null, $admin); } @@ -116,7 +155,7 @@ public function testSelfServiceWithCorrectPassword(): void { $user = $this->buildUser(1); $user->shouldReceive('checkPassword')->once()->with('secret')->andReturn(true); - $this->expectFullDisable($user); + $this->expectFullDisable($user, 1, 'self'); $this->service->disableTwoFactor($user, ' secret ', $user); } @@ -125,21 +164,48 @@ public function testSelfServiceWithWrongPasswordChangesNothing(): void { $user = $this->buildUser(1); $user->shouldReceive('checkPassword')->once()->andReturn(false); - $user->shouldNotReceive('disable2FA'); - $this->tx_service->shouldNotReceive('transaction'); - $this->code_repo->shouldNotReceive('deleteAllForUser'); - $this->device_repo->shouldNotReceive('revokeAllForUser'); - $this->audit_service->shouldNotReceive('log'); + $this->expectNothingChanges($user); $this->expectException(ValidationException::class); $this->service->disableTwoFactor($user, 'wrong', $user); } - public function testSelfServiceWithoutPasswordIsRejected(): void + public function testSelfServiceOnAGroupEnforcedUserIsRefusedBeforeCheckingThePassword(): void { - $user = $this->buildUser(1); - $user->shouldReceive('checkPassword')->once()->with('')->andReturn(false); - $user->shouldNotReceive('disable2FA'); + $user = $this->buildUser(1, true); + $user->shouldNotReceive('checkPassword'); + $this->expectNothingChanges($user); + + $this->expectException(ValidationException::class); + $this->service->disableTwoFactor($user, 'secret', $user); + } + + public function testSelfServiceWithoutPasswordOutsideTheEnrollmentWindowIsRejected(): void + { + Session::forget(MFAConstants::SESSION_KEY_COMPLETED_AT); + $user = $this->buildUser(1, false, false); + $user->shouldNotReceive('checkPassword'); + $this->expectNothingChanges($user); + + $this->expectException(ValidationException::class); + $this->service->disableTwoFactor($user, null, $user); + } + + public function testSelfServiceWithoutPasswordInsideTheEnrollmentWindow(): void + { + Session::put(MFAConstants::SESSION_KEY_COMPLETED_AT, ['user_id' => 1, 'at' => time() - 30]); + $user = $this->buildUser(1, false, false); + $user->shouldNotReceive('checkPassword'); + $this->expectFullDisable($user, 1, 'self'); + + $this->service->disableTwoFactor($user, null, $user); + } + + public function testSelfServiceWithoutPasswordAndAWindowOfAnotherUserIsRejected(): void + { + Session::put(MFAConstants::SESSION_KEY_COMPLETED_AT, ['user_id' => 99, 'at' => time() - 30]); + $user = $this->buildUser(1, false, false); + $this->expectNothingChanges($user); $this->expectException(ValidationException::class); $this->service->disableTwoFactor($user, null, $user); @@ -148,7 +214,9 @@ public function testSelfServiceWithoutPasswordIsRejected(): void public function testAuditFailureDoesNotFailTheOperation(): void { $user = $this->buildUser(1); + $this->code_repo->shouldReceive('lockUserRow')->once(); $user->shouldReceive('disable2FA')->once(); + $user->shouldReceive('setRecoveryCodesAcknowledgedAt')->once(); $this->user_repo->shouldReceive('add')->once(); $this->code_repo->shouldReceive('deleteAllForUser')->once()->andReturn(0); $this->device_repo->shouldReceive('revokeAllForUser')->once();