diff --git a/app/Repositories/DoctrineUserTrustedDeviceRepository.php b/app/Repositories/DoctrineUserTrustedDeviceRepository.php index 37267066..a94833b3 100644 --- a/app/Repositories/DoctrineUserTrustedDeviceRepository.php +++ b/app/Repositories/DoctrineUserTrustedDeviceRepository.php @@ -42,19 +42,6 @@ public function getByUserAndDeviceIdentifier(User $user, string $deviceIdentifie return $result instanceof UserTrustedDevice ? $result : null; } - public function revokeAllForUser(User $user): void - { - $this->getEntityManager() - ->createQueryBuilder() - ->update($this->getBaseEntity(), 'd') - ->set('d.is_revoked', ':revoked') - ->where('d.user = :user') - ->setParameter('revoked', true) - ->setParameter('user', $user) - ->getQuery() - ->execute(); - } - public function getActiveByUserAndIdentifier(User $user, string $deviceIdentifier): ?UserTrustedDevice { $criteria = Criteria::create() @@ -77,4 +64,14 @@ public function getActiveByUser(User $user): array return $this->matching($criteria)->toArray(); } + + public function revokeAllActiveForUser(User $user): array + { + $devices = $this->getActiveByUser($user); + foreach ($devices as $device) { + $device->setIsRevoked(true); + $this->add($device, false); + } + return $devices; + } } diff --git a/app/Services/Auth/DeviceTrustService.php b/app/Services/Auth/DeviceTrustService.php index d4242c2f..846282d9 100644 --- a/app/Services/Auth/DeviceTrustService.php +++ b/app/Services/Auth/DeviceTrustService.php @@ -20,6 +20,7 @@ use DateTime; use DateInterval; use DateTimeZone; +use Illuminate\Support\Facades\Log; use Utils\IPHelper; use Utils\Db\ITransactionService; @@ -98,13 +99,32 @@ public function isDeviceTrusted(User $user, ?string $cookieToken): bool public function removeTrustedDevices(User $user): void { - $this->repository->revokeAllForUser($user); + $devices = $this->tx_service->transaction(function () use ($user) { + return $this->repository->revokeAllActiveForUser($user); + }); - $this->audit_service->log( - $user, - TwoFactorAuditLog::EventDeviceRevoked, - $user->getTwoFactorMethod(), - IPHelper::getUserIp() - ); + foreach ($devices as $device) { + $this->logDeviceRevoked($user, $device); + } + } + + /** + * Best-effort, after the revocation is committed: an audit failure must not + * fail an operation whose devices are already revoked, and audit_service->log() + * opens its own transaction, which cannot be nested inside ours. + */ + private function logDeviceRevoked(User $user, UserTrustedDevice $device): void + { + try { + $this->audit_service->log( + $user, + TwoFactorAuditLog::EventDeviceRevoked, + $user->getTwoFactorMethod(), + IPHelper::getUserIp(), + ['device_id' => $device->getId()] + ); + } catch (\Throwable $ex) { + Log::warning($ex); + } } } diff --git a/app/Services/Auth/IRecoveryCodeService.php b/app/Services/Auth/IRecoveryCodeService.php index ab6de16e..ec5555fe 100644 --- a/app/Services/Auth/IRecoveryCodeService.php +++ b/app/Services/Auth/IRecoveryCodeService.php @@ -67,4 +67,36 @@ public function countUnusedRecoveryCodes(User $user): int; * @return RecoveryCodesStatus remaining/total/low-threshold standing for the user */ 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. + * + * Passwordless users (social-only accounts, !$user->hasPasswordSet()) can never + * pass the self-service password check, as User::checkPassword() returns false + * for an empty password, so self-service always throws for them. Such a caller + * must first require another proof of identity (under SDS rev 2.5, a recent 2FA + * challenge: MFAConstants::SESSION_KEY_COMPLETED_AT) and only then invoke this + * method on the user's behalf with $actor = null. + * + * 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 a626dd82..01e7bd1d 100644 --- a/app/Services/Auth/RecoveryCodeService.php +++ b/app/Services/Auth/RecoveryCodeService.php @@ -16,6 +16,7 @@ use App\libs\Auth\Models\TwoFactorAuditLog; use App\libs\Auth\Models\UserRecoveryCode; use Auth\Repositories\IUserRecoveryCodeRepository; +use Auth\Repositories\IUserTrustedDeviceRepository; use Auth\Repositories\IUserRepository; use Auth\User; use Illuminate\Support\Facades\Hash; @@ -36,6 +37,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, ) { @@ -121,6 +123,47 @@ public function enableTwoFactorAndGenerateCodes(User $user, string $method): arr return $codes; } + /** + * @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 + $revoked_devices = $this->tx_service->transaction(function () use ($user) { + $user->disable2FA(); + $this->user_repository->add($user, false); + $this->repository->deleteAllForUser($user); + return $this->trusted_device_repository->revokeAllActiveForUser($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. + // One device_revoked event per device actually revoked, with its device_id + // (same semantics as DeviceTrustService::removeTrustedDevices). + foreach ($revoked_devices as $device) { + try { + $this->audit_service->log( + $user, + TwoFactorAuditLog::EventDeviceRevoked, + $user->getTwoFactorMethod(), + IPHelper::getUserIp(), + ['device_id' => $device->getId()] + ); + } 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. diff --git a/app/libs/Auth/Repositories/IUserTrustedDeviceRepository.php b/app/libs/Auth/Repositories/IUserTrustedDeviceRepository.php index 04e86edf..cfdfb613 100644 --- a/app/libs/Auth/Repositories/IUserTrustedDeviceRepository.php +++ b/app/libs/Auth/Repositories/IUserTrustedDeviceRepository.php @@ -23,9 +23,14 @@ interface IUserTrustedDeviceRepository extends IBaseRepository public function getByUserAndDeviceIdentifier(User $user, string $deviceIdentifier): ?UserTrustedDevice; /** - * Revoke all trusted devices for the given user (sets is_revoked = true). + * Marks every active (non-revoked, non-expired) trusted device of the user as revoked + * and returns them. Revokes entity by entity and opens no transaction of its own: + * the caller owns the transaction and logs one audit event per returned device + * after it commits. + * + * @return UserTrustedDevice[] */ - public function revokeAllForUser(User $user): void; + public function revokeAllActiveForUser(User $user): array; /** * Look up an active (non-revoked, non-expired) trusted device for a user by its hashed identifier. diff --git a/tests/DeviceTrustServiceTest.php b/tests/DeviceTrustServiceTest.php index 4d8ae8fb..ea768a20 100644 --- a/tests/DeviceTrustServiceTest.php +++ b/tests/DeviceTrustServiceTest.php @@ -275,20 +275,33 @@ public function testTrustDeviceSetsExpiresAtFromConfig(): void // removeTrustedDevices // ------------------------------------------------------------------------- - public function testRemoveTrustedDevicesRevokesAll(): void + public function testRemoveTrustedDevicesRevokesAllAndLogsOneEventPerDevice(): void { $user = Mockery::mock(User::class); $user->shouldReceive('getTwoFactorMethod')->andReturn(User::MFAMethod_OTP); + $devices = [new UserTrustedDevice(), new UserTrustedDevice()]; + $this->repo - ->shouldReceive('revokeAllForUser') + ->shouldReceive('revokeAllActiveForUser') ->once() - ->with($user); + ->with($user) + ->andReturn($devices); $this->audit_service ->shouldReceive('log') - ->once() - ->with($user, \App\libs\Auth\Models\TwoFactorAuditLog::EventDeviceRevoked, User::MFAMethod_OTP, Mockery::type('string')); + ->twice() + ->with($user, \App\libs\Auth\Models\TwoFactorAuditLog::EventDeviceRevoked, User::MFAMethod_OTP, Mockery::type('string'), Mockery::type('array')); + + $this->service->removeTrustedDevices($user); + } + + public function testRemoveTrustedDevicesWithoutActiveDevicesLogsNothing(): void + { + $user = Mockery::mock(User::class); + + $this->repo->shouldReceive('revokeAllActiveForUser')->once()->with($user)->andReturn([]); + $this->audit_service->shouldNotReceive('log'); $this->service->removeTrustedDevices($user); } diff --git a/tests/DisableTwoFactorIntegrationTest.php b/tests/DisableTwoFactorIntegrationTest.php new file mode 100644 index 00000000..e4ba09eb --- /dev/null +++ b/tests/DisableTwoFactorIntegrationTest.php @@ -0,0 +1,103 @@ +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); + } + + // one device_revoked event per revoked device + $this->assertSame(2, 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..18d68233 --- /dev/null +++ b/tests/unit/RecoveryCodeServiceDisableTwoFactorTest.php @@ -0,0 +1,195 @@ +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('revokeAllActiveForUser')->once()->with($user) + ->andReturn([new UserTrustedDevice(), new UserTrustedDevice()]); + $this->audit_service->shouldReceive('log') + ->twice() + ->with($user, TwoFactorAuditLog::EventDeviceRevoked, User::MFAMethod_OTP, Mockery::type('string'), Mockery::type('array')); + } + + 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('revokeAllActiveForUser'); + $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); + } + + /** + * A passwordless (social-only) user can never satisfy the self-service + * password check: checkPassword() is false for an empty password, whatever + * is supplied. The caller must obtain another proof of identity (a recent + * 2FA challenge) and call this service as console/admin instead. + * See the IRecoveryCodeService::disableTwoFactor PHPDoc. + */ + public function testSelfServiceForPasswordlessUserIsRejectedAndChangesNothing(): void + { + $user = $this->buildUser(1); + $user->shouldReceive('hasPasswordSet')->andReturn(false); + $user->shouldReceive('checkPassword')->once()->andReturn(false); + $user->shouldNotReceive('disable2FA'); + $this->tx_service->shouldNotReceive('transaction'); + $this->code_repo->shouldNotReceive('deleteAllForUser'); + $this->device_repo->shouldNotReceive('revokeAllActiveForUser'); + $this->audit_service->shouldNotReceive('log'); + + $this->expectException(ValidationException::class); + $this->service->disableTwoFactor($user, 'anything', $user); + } + + public function testNoActiveDevicesLogsNoDeviceRevokedEvent(): 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('revokeAllActiveForUser')->once()->andReturn([]); + $this->audit_service->shouldNotReceive('log'); + + $this->service->disableTwoFactor($user, null, null); + } + + 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('revokeAllActiveForUser')->once()->andReturn([new UserTrustedDevice()]); + $this->audit_service->shouldReceive('log')->once()->andThrow(new \RuntimeException('audit down')); + + $this->service->disableTwoFactor($user, null, null); + } +}