From c5bb408569d4e88de73393f00c08a250ce1f2331 Mon Sep 17 00:00:00 2001 From: romanetar Date: Mon, 5 Oct 2026 17:46:13 +0200 Subject: [PATCH 1/7] feat(mfa): add idp:enforce-admin-2fa command Onboards every user of the groups in two_factor.enforced_groups: generates a recovery code batch, emails the user that MFA is now required (no codes in the mail or the console output) and records an enrollment_changed audit event with {"source": "enforce-admin-2fa"}. Users that already have unused recovery codes are skipped so a saved batch is never invalidated. Supports --dry-run, chunked queries and processed / skipped / error counts. It deliberately does not set two_factor_enabled / two_factor_method / two_factor_enforced_at: enforcement is derived from group membership by User::shouldRequire2FA(). --- .../Commands/EnforceAdmin2FACommand.php | 153 +++++++++++++++ app/Console/Kernel.php | 1 + app/Mail/TwoFactorEnforcedMail.php | 68 +++++++ .../emails/auth/two_factor_enforced.blade.php | 16 ++ .../EnforceAdmin2FACommandIntegrationTest.php | 129 +++++++++++++ .../Console/EnforceAdmin2FACommandTest.php | 181 ++++++++++++++++++ 6 files changed, 548 insertions(+) create mode 100644 app/Console/Commands/EnforceAdmin2FACommand.php create mode 100644 app/Mail/TwoFactorEnforcedMail.php create mode 100644 resources/views/emails/auth/two_factor_enforced.blade.php create mode 100644 tests/EnforceAdmin2FACommandIntegrationTest.php create mode 100644 tests/unit/Console/EnforceAdmin2FACommandTest.php diff --git a/app/Console/Commands/EnforceAdmin2FACommand.php b/app/Console/Commands/EnforceAdmin2FACommand.php new file mode 100644 index 000000000..45f231df8 --- /dev/null +++ b/app/Console/Commands/EnforceAdmin2FACommand.php @@ -0,0 +1,153 @@ +option('dry-run'); + $slugs = array_values(array_filter((array)config('two_factor.enforced_groups', []))); + + if (empty($slugs)) { + $this->warn('No groups configured in two_factor.enforced_groups, nothing to do.'); + return self::SUCCESS; + } + + $this->info(sprintf( + '%sEnforced groups: %s', + $dry_run ? '[DRY RUN] ' : '', + implode(', ', $slugs) + )); + + $processed = 0; + $skipped = 0; + $errors = 0; + $last_id = 0; + + do { + // keyset pagination over ids: the loop never holds the whole user set in memory + $ids = $this->getUserIdsChunk($slugs, $last_id); + + foreach ($ids as $id) { + $last_id = $id; + $email = (string)$id; + try { + $user = EntityManager::getRepository(User::class)->find($id); + if (is_null($user)) { + continue; + } + $email = $user->getEmail(); + + // never overwrite an existing batch: the user may have saved those codes + if ($recovery_code_service->countUnusedRecoveryCodes($user) > 0) { + $skipped++; + $this->line(sprintf(' skipped %s (already has unused recovery codes)', $email)); + continue; + } + + if ($dry_run) { + $processed++; + $this->line(sprintf(' would process %s', $email)); + continue; + } + + // plaintext codes are shown once and must never be printed or mailed + $recovery_code_service->generateRecoveryCodes($user); + + $audit_service->log( + $user, + TwoFactorAuditLog::EventEnrollmentChanged, + TwoFactorAuditLog::MethodEmailOtp, + self::ConsoleIp, + ['source' => 'enforce-admin-2fa'] + ); + + // codes are already committed: a mail failure counts as an error + // for this user but is never rolled back, that would invalidate them + Mail::send(new TwoFactorEnforcedMail($user)); + + $processed++; + $this->line(sprintf(' processed %s', $email)); + } catch (\Throwable $ex) { + $errors++; + Log::error($ex); + $this->error(sprintf(' error %s: %s', $email, $ex->getMessage())); + } + } + } while (count($ids) === self::ChunkSize); + + $this->info(sprintf( + '%sSummary: processed %d, skipped %d, errors %d', + $dry_run ? '[DRY RUN] ' : '', + $processed, + $skipped, + $errors + )); + + return $errors > 0 ? self::FAILURE : self::SUCCESS; + } + + /** + * @param string[] $slugs + * @return int[] + */ + private function getUserIdsChunk(array $slugs, int $after_id): array + { + $rows = EntityManager::createQuery( + 'SELECT DISTINCT u.id FROM ' . User::class . ' u JOIN u.groups g ' . + 'WHERE g.slug IN (:slugs) AND u.id > :after_id ORDER BY u.id ASC' + ) + ->setParameter('slugs', $slugs) + ->setParameter('after_id', $after_id) + ->setMaxResults(self::ChunkSize) + ->getScalarResult(); + + return array_map(static fn(array $row) => (int)$row['id'], $rows); + } +} diff --git a/app/Console/Kernel.php b/app/Console/Kernel.php index 03857599f..52489b9ee 100644 --- a/app/Console/Kernel.php +++ b/app/Console/Kernel.php @@ -29,6 +29,7 @@ class Kernel extends ConsoleKernel Commands\CleanOAuth2StaleData::class, Commands\CleanOpenIdStaleData::class, Commands\CreateSuperAdmin::class, + Commands\EnforceAdmin2FACommand::class, Commands\CreateRawUser::class, Commands\CreateOAuth2TestClient::class, Commands\GetLatestOtp::class, diff --git a/app/Mail/TwoFactorEnforcedMail.php b/app/Mail/TwoFactorEnforcedMail.php new file mode 100644 index 000000000..e0f77c5b0 --- /dev/null +++ b/app/Mail/TwoFactorEnforcedMail.php @@ -0,0 +1,68 @@ +user_email = $user->getEmail(); + $this->user_fullname = $user->getFullName(); + $this->profile_url = rtrim(Config::get('app.url'), '/') . '/accounts/user/profile'; + } + + /** + * @return $this + */ + public function build() + { + $subject = sprintf("[%s] Two-factor authentication is now required on your account", Config::get('app.app_name')); + Log::debug(sprintf("TwoFactorEnforcedMail::build to %s", $this->user_email)); + return $this->from(Config::get("mail.from")) + ->to($this->user_email) + ->subject($subject) + ->view('emails.auth.two_factor_enforced'); + } +} diff --git a/resources/views/emails/auth/two_factor_enforced.blade.php b/resources/views/emails/auth/two_factor_enforced.blade.php new file mode 100644 index 000000000..532d5ddf4 --- /dev/null +++ b/resources/views/emails/auth/two_factor_enforced.blade.php @@ -0,0 +1,16 @@ + + + + + + +

Dear {!! $user_fullname !!},

+

Two-factor authentication (MFA) is now required on your account. You will be asked for a second verification step each time you sign in.

+

Recovery codes have been set up for your account. Recovery codes are shown only once, so to get a set you can save, sign in, open the Security section of your profile and regenerate them:

+

{!! $profile_url !!}

+

Keep your recovery codes in a safe place: they let you sign in if you lose access to your second factor.

+
+
+

Cheers,
Your {!! Config::get('app.tenant_name') !!} Support Team

+ + diff --git a/tests/EnforceAdmin2FACommandIntegrationTest.php b/tests/EnforceAdmin2FACommandIntegrationTest.php new file mode 100644 index 000000000..22318b357 --- /dev/null +++ b/tests/EnforceAdmin2FACommandIntegrationTest.php @@ -0,0 +1,129 @@ +setName(self::TestGroupSlug); + $group->setSlug(self::TestGroupSlug); + $group->setDefault(false); + $group->setActive(true); + EntityManager::persist($group); + EntityManager::flush(); + return $group; + } + + protected function setUp(): void + { + parent::setUp(); + config(['two_factor.enforced_groups' => [self::TestGroupSlug]]); + Mail::fake(); + + $user = new User(); + $user->setEmail('enforce-admin@nomail.com'); + $user->setFirstName('Enforce'); + $user->setLastName('Admin'); + $user->setIdentifier('enforce-admin@nomail.com'); + $user->setPassword('P@sswordS3cret'); + $user->verifyEmail(false); + $user->addToGroup($this->createEnforcedGroup()); + EntityManager::persist($user); + EntityManager::flush(); + $this->admin = $user; + } + + private function userRow(): object + { + return DB::table('users')->where('id', $this->admin->getId())->first(); + } + + private function codeHashes(): array + { + return DB::table('user_recovery_codes') + ->where('user_id', $this->admin->getId()) + ->orderBy('id') + ->pluck('code_hash') + ->all(); + } + + public function testEnforceProcessesEnforcedAdmin(): void + { + $before = $this->userRow(); + + $code = Artisan::call('idp:enforce-admin-2fa'); + + $this->assertSame(0, $code); + $this->assertCount(10, $this->codeHashes()); + $this->assertSame(1, DB::table('two_factor_audit_log') + ->where('user_id', $this->admin->getId()) + ->where('event_type', TwoFactorAuditLog::EventEnrollmentChanged) + ->count()); + $metadata = DB::table('two_factor_audit_log') + ->where('user_id', $this->admin->getId()) + ->where('event_type', TwoFactorAuditLog::EventEnrollmentChanged) + ->value('metadata'); + $this->assertSame(['source' => 'enforce-admin-2fa'], json_decode($metadata, true)); + Mail::assertSent(TwoFactorEnforcedMail::class, 1); + + // enforcement is derived from the group: stored flags stay untouched + $after = $this->userRow(); + $this->assertSame($before->two_factor_enabled, $after->two_factor_enabled); + $this->assertSame($before->two_factor_method, $after->two_factor_method); + $this->assertSame($before->two_factor_enforced_at, $after->two_factor_enforced_at); + } + + public function testSecondRunSkipsAndKeepsExistingCodes(): void + { + Artisan::call('idp:enforce-admin-2fa'); + $hashes = $this->codeHashes(); + + Artisan::call('idp:enforce-admin-2fa'); + + $this->assertStringContainsString('processed 0, skipped 1, errors 0', Artisan::output()); + $this->assertSame($hashes, $this->codeHashes()); + Mail::assertSent(TwoFactorEnforcedMail::class, 1); + } + + public function testDryRunWritesNothing(): void + { + $code = Artisan::call('idp:enforce-admin-2fa', ['--dry-run' => true]); + + $this->assertSame(0, $code); + $this->assertStringContainsString('processed 1, skipped 0, errors 0', Artisan::output()); + $this->assertCount(0, $this->codeHashes()); + $this->assertSame(0, DB::table('two_factor_audit_log')->where('user_id', $this->admin->getId())->count()); + Mail::assertNothingSent(); + } +} diff --git a/tests/unit/Console/EnforceAdmin2FACommandTest.php b/tests/unit/Console/EnforceAdmin2FACommandTest.php new file mode 100644 index 000000000..5909b05e4 --- /dev/null +++ b/tests/unit/Console/EnforceAdmin2FACommandTest.php @@ -0,0 +1,181 @@ +setName(self::TestGroupSlug); + $group->setSlug(self::TestGroupSlug); + $group->setDefault(false); + $group->setActive(true); + EntityManager::persist($group); + EntityManager::flush(); + return $group; + } + + protected function setUp(): void + { + parent::setUp(); + config(['two_factor.enforced_groups' => [self::TestGroupSlug]]); + $this->group = $this->createEnforcedGroup(); + Mail::fake(); + $this->recovery_service = Mockery::mock(IRecoveryCodeService::class); + $this->audit_service = Mockery::mock(ITwoFactorAuditService::class); + $this->app->instance(IRecoveryCodeService::class, $this->recovery_service); + $this->app->instance(ITwoFactorAuditService::class, $this->audit_service); + } + + protected function tearDown(): void + { + Mockery::close(); + parent::tearDown(); + } + + private function createUser(string $email, bool $in_group): User + { + $user = new User(); + $user->setEmail($email); + $user->setFirstName($email); + $user->setLastName($email); + $user->setIdentifier($email); + $user->setPassword('P@sswordS3cret'); + $user->verifyEmail(false); + if ($in_group) { + $user->addToGroup($this->group); + } + EntityManager::persist($user); + EntityManager::flush(); + return $user; + } + + public function testProcessesUsersWithoutUnusedCodes(): void + { + $user = $this->createUser('enforce-a@nomail.com', true); + + $this->recovery_service->shouldReceive('countUnusedRecoveryCodes')->once()->andReturn(0); + $this->recovery_service->shouldReceive('generateRecoveryCodes')->once()->andReturn(['AAAA-BBBB']); + $this->audit_service->shouldReceive('log') + ->once() + ->with( + Mockery::on(fn($u) => $u->getId() === $user->getId()), + TwoFactorAuditLog::EventEnrollmentChanged, + TwoFactorAuditLog::MethodEmailOtp, + Mockery::type('string'), + ['source' => 'enforce-admin-2fa'] + ); + + $code = Artisan::call('idp:enforce-admin-2fa'); + $output = Artisan::output(); + + $this->assertSame(0, $code); + $this->assertStringContainsString('processed 1, skipped 0, errors 0', $output); + // plaintext codes are shown once and never printed + $this->assertStringNotContainsString('AAAA-BBBB', $output); + Mail::assertSent(TwoFactorEnforcedMail::class, 1); + } + + public function testSkipsUsersWithUnusedCodes(): void + { + $this->createUser('enforce-b@nomail.com', true); + + $this->recovery_service->shouldReceive('countUnusedRecoveryCodes')->once()->andReturn(10); + $this->recovery_service->shouldNotReceive('generateRecoveryCodes'); + $this->audit_service->shouldNotReceive('log'); + + $code = Artisan::call('idp:enforce-admin-2fa'); + + $this->assertSame(0, $code); + $this->assertStringContainsString('processed 0, skipped 1, errors 0', Artisan::output()); + Mail::assertNothingSent(); + } + + public function testIgnoresUsersOutsideEnforcedGroups(): void + { + $this->createUser('enforce-c@nomail.com', false); + + $this->recovery_service->shouldNotReceive('countUnusedRecoveryCodes'); + $this->recovery_service->shouldNotReceive('generateRecoveryCodes'); + + Artisan::call('idp:enforce-admin-2fa'); + + $this->assertStringContainsString('processed 0, skipped 0, errors 0', Artisan::output()); + } + + public function testDryRunWritesNothingAndReportsSameCounts(): void + { + $this->createUser('enforce-d@nomail.com', true); + $this->createUser('enforce-e@nomail.com', true); + + $this->recovery_service->shouldReceive('countUnusedRecoveryCodes')->twice()->andReturn(0, 10); + $this->recovery_service->shouldNotReceive('generateRecoveryCodes'); + $this->audit_service->shouldNotReceive('log'); + + $code = Artisan::call('idp:enforce-admin-2fa', ['--dry-run' => true]); + + $this->assertSame(0, $code); + $this->assertStringContainsString('processed 1, skipped 1, errors 0', Artisan::output()); + Mail::assertNothingSent(); + } + + public function testErrorOnOneUserDoesNotStopTheRun(): void + { + $this->createUser('enforce-f@nomail.com', true); + $this->createUser('enforce-g@nomail.com', true); + + $this->recovery_service->shouldReceive('countUnusedRecoveryCodes')->twice()->andReturn(0); + $this->recovery_service->shouldReceive('generateRecoveryCodes') + ->twice() + ->andReturnUsing( + fn() => throw new \RuntimeException('boom'), + fn() => ['AAAA-BBBB'] + ); + $this->audit_service->shouldReceive('log')->once(); + + $code = Artisan::call('idp:enforce-admin-2fa'); + + $this->assertSame(1, $code); + $this->assertStringContainsString('processed 1, skipped 0, errors 1', Artisan::output()); + Mail::assertSent(TwoFactorEnforcedMail::class, 1); + } +} From ff0e5d2a884b187fc2642bf50a3492821f69dd00 Mon Sep 17 00:00:00 2001 From: romanetar Date: Thu, 5 Nov 2026 18:07:13 +0100 Subject: [PATCH 2/7] fix(mfa): key enforce-admin-2fa skip on the audit row, written after the mail The command no longer generates recovery codes (users get them when they enable 2FA from the profile). The skip criterion is now an enrollment_changed audit event with source=enforce-admin-2fa, written only after Mail::send succeeds, so a failed run is retried for exactly the users whose email failed. --- .../Commands/EnforceAdmin2FACommand.php | 41 ++++++++---- .../EnforceAdmin2FACommandIntegrationTest.php | 33 ++++++++-- .../Console/EnforceAdmin2FACommandTest.php | 66 ++++++++++--------- 3 files changed, 90 insertions(+), 50 deletions(-) diff --git a/app/Console/Commands/EnforceAdmin2FACommand.php b/app/Console/Commands/EnforceAdmin2FACommand.php index 45f231df8..52d3f4c35 100644 --- a/app/Console/Commands/EnforceAdmin2FACommand.php +++ b/app/Console/Commands/EnforceAdmin2FACommand.php @@ -14,7 +14,6 @@ use App\libs\Auth\Models\TwoFactorAuditLog; use App\Mail\TwoFactorEnforcedMail; -use App\Services\Auth\IRecoveryCodeService; use App\Services\Auth\ITwoFactorAuditService; use Auth\User; use Illuminate\Console\Command; @@ -25,7 +24,7 @@ /** * Class EnforceAdmin2FACommand * Onboards every user of the groups in config('two_factor.enforced_groups'): - * recovery codes + notification email + audit. It deliberately does NOT touch + * notification email + audit row (the audit row is the "already notified" marker). It deliberately does NOT touch * two_factor_enabled / two_factor_method / two_factor_enforced_at: enforcement * is derived from group membership at runtime (User::shouldRequire2FA()). * @package App\Console\Commands @@ -39,6 +38,8 @@ final class EnforceAdmin2FACommand extends Command */ private const ConsoleIp = '127.0.0.1'; + private const AuditSource = 'enforce-admin-2fa'; + /** * @var string */ @@ -47,9 +48,9 @@ final class EnforceAdmin2FACommand extends Command /** * @var string */ - protected $description = 'Generate recovery codes and notify every user of the 2FA enforced groups'; + protected $description = 'Notify every user of the 2FA enforced groups that 2FA is now required'; - public function handle(IRecoveryCodeService $recovery_code_service, ITwoFactorAuditService $audit_service): int + public function handle(ITwoFactorAuditService $audit_service): int { $dry_run = (bool)$this->option('dry-run'); $slugs = array_values(array_filter((array)config('two_factor.enforced_groups', []))); @@ -84,10 +85,11 @@ public function handle(IRecoveryCodeService $recovery_code_service, ITwoFactorAu } $email = $user->getEmail(); - // never overwrite an existing batch: the user may have saved those codes - if ($recovery_code_service->countUnusedRecoveryCodes($user) > 0) { + // the audit row is written only after the mail was sent, so a failed + // run leaves no marker and the next run retries exactly those users + if ($this->wasAlreadyNotified($user)) { $skipped++; - $this->line(sprintf(' skipped %s (already has unused recovery codes)', $email)); + $this->line(sprintf(' skipped %s (already notified)', $email)); continue; } @@ -97,21 +99,18 @@ public function handle(IRecoveryCodeService $recovery_code_service, ITwoFactorAu continue; } - // plaintext codes are shown once and must never be printed or mailed - $recovery_code_service->generateRecoveryCodes($user); + // at-least-once: if the audit write fails after the mail was sent, + // the user is notified again on the next run + Mail::send(new TwoFactorEnforcedMail($user)); $audit_service->log( $user, TwoFactorAuditLog::EventEnrollmentChanged, TwoFactorAuditLog::MethodEmailOtp, self::ConsoleIp, - ['source' => 'enforce-admin-2fa'] + ['source' => self::AuditSource] ); - // codes are already committed: a mail failure counts as an error - // for this user but is never rolled back, that would invalidate them - Mail::send(new TwoFactorEnforcedMail($user)); - $processed++; $this->line(sprintf(' processed %s', $email)); } catch (\Throwable $ex) { @@ -133,6 +132,20 @@ public function handle(IRecoveryCodeService $recovery_code_service, ITwoFactorAu return $errors > 0 ? self::FAILURE : self::SUCCESS; } + private function wasAlreadyNotified(User $user): bool + { + $logs = EntityManager::getRepository(TwoFactorAuditLog::class)->findBy([ + 'user' => $user, + 'event_type' => TwoFactorAuditLog::EventEnrollmentChanged, + ]); + foreach ($logs as $log) { + if (($log->getMetadata()['source'] ?? null) === self::AuditSource) { + return true; + } + } + return false; + } + /** * @param string[] $slugs * @return int[] diff --git a/tests/EnforceAdmin2FACommandIntegrationTest.php b/tests/EnforceAdmin2FACommandIntegrationTest.php index 22318b357..b76146684 100644 --- a/tests/EnforceAdmin2FACommandIntegrationTest.php +++ b/tests/EnforceAdmin2FACommandIntegrationTest.php @@ -85,7 +85,7 @@ public function testEnforceProcessesEnforcedAdmin(): void $code = Artisan::call('idp:enforce-admin-2fa'); $this->assertSame(0, $code); - $this->assertCount(10, $this->codeHashes()); + $this->assertCount(0, $this->codeHashes()); $this->assertSame(1, DB::table('two_factor_audit_log') ->where('user_id', $this->admin->getId()) ->where('event_type', TwoFactorAuditLog::EventEnrollmentChanged) @@ -104,18 +104,43 @@ public function testEnforceProcessesEnforcedAdmin(): void $this->assertSame($before->two_factor_enforced_at, $after->two_factor_enforced_at); } - public function testSecondRunSkipsAndKeepsExistingCodes(): void + public function testSecondRunSkipsAlreadyNotified(): void { Artisan::call('idp:enforce-admin-2fa'); - $hashes = $this->codeHashes(); Artisan::call('idp:enforce-admin-2fa'); $this->assertStringContainsString('processed 0, skipped 1, errors 0', Artisan::output()); - $this->assertSame($hashes, $this->codeHashes()); Mail::assertSent(TwoFactorEnforcedMail::class, 1); } + public function testMailFailureIsRetriedOnNextRun(): void + { + Mail::swap(new class(new \Illuminate\Mail\MailManager(app())) extends \Illuminate\Support\Testing\Fakes\MailFake { + public function send($view, array $data = [], $callback = null) + { + throw new \RuntimeException('smtp down'); + } + }); + + $this->assertSame(1, Artisan::call('idp:enforce-admin-2fa')); + $this->assertSame(0, $this->auditRows()); + + Mail::fake(); + $this->assertSame(0, Artisan::call('idp:enforce-admin-2fa')); + $this->assertStringContainsString('processed 1, skipped 0, errors 0', Artisan::output()); + Mail::assertSent(TwoFactorEnforcedMail::class, 1); + $this->assertSame(1, $this->auditRows()); + } + + private function auditRows(): int + { + return DB::table('two_factor_audit_log') + ->where('user_id', $this->admin->getId()) + ->where('event_type', TwoFactorAuditLog::EventEnrollmentChanged) + ->count(); + } + public function testDryRunWritesNothing(): void { $code = Artisan::call('idp:enforce-admin-2fa', ['--dry-run' => true]); diff --git a/tests/unit/Console/EnforceAdmin2FACommandTest.php b/tests/unit/Console/EnforceAdmin2FACommandTest.php index 5909b05e4..c28a011b4 100644 --- a/tests/unit/Console/EnforceAdmin2FACommandTest.php +++ b/tests/unit/Console/EnforceAdmin2FACommandTest.php @@ -14,7 +14,6 @@ use App\libs\Auth\Models\TwoFactorAuditLog; use App\Mail\TwoFactorEnforcedMail; -use App\Services\Auth\IRecoveryCodeService; use App\Services\Auth\ITwoFactorAuditService; use Auth\Group; use Auth\User; @@ -26,17 +25,13 @@ /** * Class EnforceAdmin2FACommandTest - * Orchestration of idp:enforce-admin-2fa with the recovery code and audit - * services mocked; the real DB is only used to resolve the group members. + * Orchestration of idp:enforce-admin-2fa with the audit service mocked; the real DB is only used to resolve the group members. * @package Tests\unit\Console */ final class EnforceAdmin2FACommandTest extends BrowserKitTestCase { private Group $group; - /** @var \Mockery\MockInterface&IRecoveryCodeService */ - private $recovery_service; - /** @var \Mockery\MockInterface&ITwoFactorAuditService */ private $audit_service; @@ -60,9 +55,7 @@ protected function setUp(): void config(['two_factor.enforced_groups' => [self::TestGroupSlug]]); $this->group = $this->createEnforcedGroup(); Mail::fake(); - $this->recovery_service = Mockery::mock(IRecoveryCodeService::class); $this->audit_service = Mockery::mock(ITwoFactorAuditService::class); - $this->app->instance(IRecoveryCodeService::class, $this->recovery_service); $this->app->instance(ITwoFactorAuditService::class, $this->audit_service); } @@ -89,12 +82,23 @@ private function createUser(string $email, bool $in_group): User return $user; } - public function testProcessesUsersWithoutUnusedCodes(): void + private function markNotified(User $user): void + { + $log = new TwoFactorAuditLog(); + $log->setUser($user); + $log->setEventType(TwoFactorAuditLog::EventEnrollmentChanged); + $log->setMethod(TwoFactorAuditLog::MethodEmailOtp); + $log->setIpAddress('127.0.0.1'); + $log->setUserAgent(''); + $log->setMetadata(['source' => 'enforce-admin-2fa']); + EntityManager::persist($log); + EntityManager::flush(); + } + + public function testProcessesUsersNotYetNotified(): void { $user = $this->createUser('enforce-a@nomail.com', true); - $this->recovery_service->shouldReceive('countUnusedRecoveryCodes')->once()->andReturn(0); - $this->recovery_service->shouldReceive('generateRecoveryCodes')->once()->andReturn(['AAAA-BBBB']); $this->audit_service->shouldReceive('log') ->once() ->with( @@ -106,21 +110,16 @@ public function testProcessesUsersWithoutUnusedCodes(): void ); $code = Artisan::call('idp:enforce-admin-2fa'); - $output = Artisan::output(); $this->assertSame(0, $code); - $this->assertStringContainsString('processed 1, skipped 0, errors 0', $output); - // plaintext codes are shown once and never printed - $this->assertStringNotContainsString('AAAA-BBBB', $output); + $this->assertStringContainsString('processed 1, skipped 0, errors 0', Artisan::output()); Mail::assertSent(TwoFactorEnforcedMail::class, 1); } - public function testSkipsUsersWithUnusedCodes(): void + public function testSkipsUsersAlreadyNotified(): void { - $this->createUser('enforce-b@nomail.com', true); + $this->markNotified($this->createUser('enforce-b@nomail.com', true)); - $this->recovery_service->shouldReceive('countUnusedRecoveryCodes')->once()->andReturn(10); - $this->recovery_service->shouldNotReceive('generateRecoveryCodes'); $this->audit_service->shouldNotReceive('log'); $code = Artisan::call('idp:enforce-admin-2fa'); @@ -134,21 +133,19 @@ public function testIgnoresUsersOutsideEnforcedGroups(): void { $this->createUser('enforce-c@nomail.com', false); - $this->recovery_service->shouldNotReceive('countUnusedRecoveryCodes'); - $this->recovery_service->shouldNotReceive('generateRecoveryCodes'); + $this->audit_service->shouldNotReceive('log'); Artisan::call('idp:enforce-admin-2fa'); $this->assertStringContainsString('processed 0, skipped 0, errors 0', Artisan::output()); + Mail::assertNothingSent(); } public function testDryRunWritesNothingAndReportsSameCounts(): void { $this->createUser('enforce-d@nomail.com', true); - $this->createUser('enforce-e@nomail.com', true); + $this->markNotified($this->createUser('enforce-e@nomail.com', true)); - $this->recovery_service->shouldReceive('countUnusedRecoveryCodes')->twice()->andReturn(0, 10); - $this->recovery_service->shouldNotReceive('generateRecoveryCodes'); $this->audit_service->shouldNotReceive('log'); $code = Artisan::call('idp:enforce-admin-2fa', ['--dry-run' => true]); @@ -158,18 +155,23 @@ public function testDryRunWritesNothingAndReportsSameCounts(): void Mail::assertNothingSent(); } - public function testErrorOnOneUserDoesNotStopTheRun(): void + public function testMailFailureOnOneUserDoesNotStopTheRunNorAuditsIt(): void { $this->createUser('enforce-f@nomail.com', true); $this->createUser('enforce-g@nomail.com', true); - $this->recovery_service->shouldReceive('countUnusedRecoveryCodes')->twice()->andReturn(0); - $this->recovery_service->shouldReceive('generateRecoveryCodes') - ->twice() - ->andReturnUsing( - fn() => throw new \RuntimeException('boom'), - fn() => ['AAAA-BBBB'] - ); + Mail::swap(new class(new \Illuminate\Mail\MailManager(app())) extends \Illuminate\Support\Testing\Fakes\MailFake { + private int $calls = 0; + + public function send($view, array $data = [], $callback = null) + { + if (++$this->calls === 1) { + throw new \RuntimeException('smtp down'); + } + parent::send($view, $data, $callback); + } + }); + // only the user whose mail was sent gets an audit row $this->audit_service->shouldReceive('log')->once(); $code = Artisan::call('idp:enforce-admin-2fa'); From b915a3e28905963624a74c29c0de247c52878750 Mon Sep 17 00:00:00 2001 From: romanetar Date: Thu, 5 Nov 2026 18:09:05 +0100 Subject: [PATCH 3/7] fix(mfa): refuse to run idp:enforce-admin-2fa when two_factor.enabled is off With the global kill-switch off shouldRequire2FA() is false for everyone, so notifying admins that 2FA is required would be wrong. Return FAILURE with a warning, also for --dry-run. --- app/Console/Commands/EnforceAdmin2FACommand.php | 7 +++++++ .../unit/Console/EnforceAdmin2FACommandTest.php | 16 ++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/app/Console/Commands/EnforceAdmin2FACommand.php b/app/Console/Commands/EnforceAdmin2FACommand.php index 52d3f4c35..1cbaee558 100644 --- a/app/Console/Commands/EnforceAdmin2FACommand.php +++ b/app/Console/Commands/EnforceAdmin2FACommand.php @@ -52,6 +52,13 @@ final class EnforceAdmin2FACommand extends Command public function handle(ITwoFactorAuditService $audit_service): int { + // with the global kill-switch off shouldRequire2FA() is false for everyone: + // telling admins that 2FA is required would be wrong + if (!config('two_factor.enabled', true)) { + $this->warn('two_factor.enabled is false (TWO_FACTOR_ENABLED): 2FA is not enforced, refusing to notify. Enable it first.'); + return self::FAILURE; + } + $dry_run = (bool)$this->option('dry-run'); $slugs = array_values(array_filter((array)config('two_factor.enforced_groups', []))); diff --git a/tests/unit/Console/EnforceAdmin2FACommandTest.php b/tests/unit/Console/EnforceAdmin2FACommandTest.php index c28a011b4..4bf1203a9 100644 --- a/tests/unit/Console/EnforceAdmin2FACommandTest.php +++ b/tests/unit/Console/EnforceAdmin2FACommandTest.php @@ -129,6 +129,22 @@ public function testSkipsUsersAlreadyNotified(): void Mail::assertNothingSent(); } + public function testRefusesToRunWhenTwoFactorIsDisabledGlobally(): void + { + config(['two_factor.enabled' => false]); + $this->createUser('enforce-h@nomail.com', true); + + $this->audit_service->shouldNotReceive('log'); + + foreach ([[], ['--dry-run' => true]] as $options) { + $code = Artisan::call('idp:enforce-admin-2fa', $options); + + $this->assertSame(1, $code); + $this->assertStringContainsString('two_factor.enabled is false', Artisan::output()); + } + Mail::assertNothingSent(); + } + public function testIgnoresUsersOutsideEnforcedGroups(): void { $this->createUser('enforce-c@nomail.com', false); From b989a7b9eb1eace348e1c424b753caa7ab10e13a Mon Sep 17 00:00:00 2001 From: romanetar Date: Thu, 5 Nov 2026 18:09:56 +0100 Subject: [PATCH 4/7] docs(adr): add ADR 0002 for idp:enforce-admin-2fa Records that the command only notifies (no recovery code generation, so ADR 0001 stays in force), keys the skip on the audit row written after the mail, and refuses to run when two_factor.enabled is off. --- ...mmand-notifies-without-generating-codes.md | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 doc/adrs/0002-enforce-admin-2fa-command-notifies-without-generating-codes.md diff --git a/doc/adrs/0002-enforce-admin-2fa-command-notifies-without-generating-codes.md b/doc/adrs/0002-enforce-admin-2fa-command-notifies-without-generating-codes.md new file mode 100644 index 000000000..605be547f --- /dev/null +++ b/doc/adrs/0002-enforce-admin-2fa-command-notifies-without-generating-codes.md @@ -0,0 +1,40 @@ +# 0002. `idp:enforce-admin-2fa` notifies enforced users; it does not generate recovery codes + +## Status + +Accepted — 2026-11-05. Complements [ADR 0001](0001-recovery-code-generation-not-triggered-by-group-enforcement.md), +which remains in force. + +## Context + +Group-based 2FA enforcement is derived at runtime from group membership (`User::shouldRequire2FA()`, +`config('two_factor.enforced_groups')`). Rolling it out to existing admins needs a one-off operator step that +tells them 2FA is now required. + +An earlier shape of the command also minted a recovery-code batch per user. That would have contradicted +ADR 0001 (recovery codes for group-enforced users stay on-demand) and had a delivery bug: the codes were +committed before the email was sent, so a transient SMTP or audit failure left the user "skipped" on every +later run (they already had unused codes) and never notified. + +## Decision + +`php artisan idp:enforce-admin-2fa [--dry-run]`: + +- **Only notifies.** It sends `TwoFactorEnforcedMail` to every user of the enforced groups. It never generates + recovery codes and never touches `two_factor_enabled`, `two_factor_method` or `two_factor_enforced_at`. + Users get their codes from the profile ("Enable Two-Factor Authentication" generates the first batch, + "Regenerate Codes" replaces it), as stated in ADR 0001. The mail links to the profile and carries no codes. +- **The audit row is the "already notified" marker.** After `Mail::send` succeeds it writes an + `enrollment_changed` event with metadata `source=enforce-admin-2fa`. Users with such a row are skipped. + Because the row is written only after the mail was sent, a re-run retries exactly the users whose mail failed. + Delivery is at-least-once: if the audit write fails after the mail was sent, the user is mailed again. +- **Refuses to run when `two_factor.enabled` is false** (`TWO_FACTOR_ENABLED`), also with `--dry-run`: with the + global kill-switch off `shouldRequire2FA()` is false for everyone, so telling admins 2FA is required would be + wrong. The command warns and exits with failure. + +## Consequences + +- The command is safe to re-run; `--dry-run` writes and sends nothing. +- A group-enforced user who is challenged for MFA before visiting their profile still has no recovery codes + (the negative consequence recorded in ADR 0001 is unchanged). The mail is what steers them to the profile. +- Operators must enable `TWO_FACTOR_ENABLED` before running the command. From e6f2be3d074ebc2d538537eb27b1624c62edc28c Mon Sep 17 00:00:00 2001 From: romanetar Date: Thu, 5 Nov 2026 18:40:34 +0100 Subject: [PATCH 5/7] fix(mfa): clear the EntityManager after each chunk in idp:enforce-admin-2fa Paging the ids did not bound memory: loaded users and persisted audit rows stayed managed across chunks and every flush grew with them. --- .../Commands/EnforceAdmin2FACommand.php | 7 ++++- .../EnforceAdmin2FACommandIntegrationTest.php | 29 +++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/app/Console/Commands/EnforceAdmin2FACommand.php b/app/Console/Commands/EnforceAdmin2FACommand.php index 1cbaee558..cbc49e10e 100644 --- a/app/Console/Commands/EnforceAdmin2FACommand.php +++ b/app/Console/Commands/EnforceAdmin2FACommand.php @@ -79,7 +79,8 @@ public function handle(ITwoFactorAuditService $audit_service): int $last_id = 0; do { - // keyset pagination over ids: the loop never holds the whole user set in memory + // keyset pagination over ids, plus a clear() after each chunk below: neither the + // id list nor the managed entities of previous chunks stay in memory $ids = $this->getUserIdsChunk($slugs, $last_id); foreach ($ids as $id) { @@ -126,6 +127,10 @@ public function handle(ITwoFactorAuditService $audit_service): int $this->error(sprintf(' error %s: %s', $email, $ex->getMessage())); } } + + // the chunk is done (mails sent, audit rows committed): detach its users and + // audit rows so the unit of work does not grow, and each flush stays cheap + EntityManager::clear(); } while (count($ids) === self::ChunkSize); $this->info(sprintf( diff --git a/tests/EnforceAdmin2FACommandIntegrationTest.php b/tests/EnforceAdmin2FACommandIntegrationTest.php index b76146684..0c568d6f8 100644 --- a/tests/EnforceAdmin2FACommandIntegrationTest.php +++ b/tests/EnforceAdmin2FACommandIntegrationTest.php @@ -141,6 +141,35 @@ private function auditRows(): int ->count(); } + public function testProcessesMoreThanOneChunkAndClearsTheEntityManager(): void + { + // 1 admin from setUp + 100 more = 101 > ChunkSize (100) + $group = EntityManager::getRepository(Group::class)->findOneBy(['slug' => self::TestGroupSlug]); + for ($i = 0; $i < 100; $i++) { + $user = new User(); + $email = sprintf('enforce-bulk-%d@nomail.com', $i); + $user->setEmail($email); + $user->setFirstName('Bulk'); + $user->setLastName((string)$i); + $user->setIdentifier($email); + $user->setPassword('P@sswordS3cret'); + $user->verifyEmail(false); + $user->addToGroup($group); + EntityManager::persist($user); + } + EntityManager::flush(); + + $this->assertSame(0, Artisan::call('idp:enforce-admin-2fa')); + + $this->assertStringContainsString('processed 101, skipped 0, errors 0', Artisan::output()); + Mail::assertSent(TwoFactorEnforcedMail::class, 101); + $this->assertSame(101, DB::table('two_factor_audit_log') + ->where('event_type', TwoFactorAuditLog::EventEnrollmentChanged) + ->count()); + // nothing from the run stays managed after the last chunk + $this->assertSame(0, EntityManager::getUnitOfWork()->size()); + } + public function testDryRunWritesNothing(): void { $code = Artisan::call('idp:enforce-admin-2fa', ['--dry-run' => true]); From a8187e9ada7a14c1bbd7b2f6155329e83b1c3621 Mon Sep 17 00:00:00 2001 From: romanetar Date: Thu, 5 Nov 2026 18:42:30 +0100 Subject: [PATCH 6/7] fix(mfa): escape user-controlled values in the 2FA enforced mail The full name was rendered with {!! !!}, allowing HTML injection into a security notification. Also stop claiming that recovery codes were already set up: the command no longer generates them, the user gets them when enabling 2FA from the profile. --- .../emails/auth/two_factor_enforced.blade.php | 8 +-- tests/unit/MFA/TwoFactorEnforcedMailTest.php | 52 +++++++++++++++++++ 2 files changed, 56 insertions(+), 4 deletions(-) create mode 100644 tests/unit/MFA/TwoFactorEnforcedMailTest.php diff --git a/resources/views/emails/auth/two_factor_enforced.blade.php b/resources/views/emails/auth/two_factor_enforced.blade.php index 532d5ddf4..d4b73b6fb 100644 --- a/resources/views/emails/auth/two_factor_enforced.blade.php +++ b/resources/views/emails/auth/two_factor_enforced.blade.php @@ -4,13 +4,13 @@ -

Dear {!! $user_fullname !!},

+

Dear {{ $user_fullname }},

Two-factor authentication (MFA) is now required on your account. You will be asked for a second verification step each time you sign in.

-

Recovery codes have been set up for your account. Recovery codes are shown only once, so to get a set you can save, sign in, open the Security section of your profile and regenerate them:

-

{!! $profile_url !!}

+

To set it up, sign in and open the Two-Factor Authentication section of your profile. When you enable it, a set of recovery codes is shown only once, so save it right away:

+

{{ $profile_url }}

Keep your recovery codes in a safe place: they let you sign in if you lose access to your second factor.



-

Cheers,
Your {!! Config::get('app.tenant_name') !!} Support Team

+

Cheers,
Your {{ Config::get('app.tenant_name') }} Support Team

diff --git a/tests/unit/MFA/TwoFactorEnforcedMailTest.php b/tests/unit/MFA/TwoFactorEnforcedMailTest.php new file mode 100644 index 000000000..981131f67 --- /dev/null +++ b/tests/unit/MFA/TwoFactorEnforcedMailTest.php @@ -0,0 +1,52 @@ +setEmail('mail-escape@nomail.com'); + $user->setFirstName(''); + $user->setLastName('click'); + + $html = (new TwoFactorEnforcedMail($user))->render(); + + $this->assertStringNotContainsString('assertStringNotContainsString('', $html); + $this->assertStringContainsString('<img src=x onerror=alert(1)>', $html); + } + + public function testDoesNotClaimThatRecoveryCodesWereAlreadySetUp(): void + { + $user = new User(); + $user->setEmail('mail-text@nomail.com'); + $user->setFirstName('Jane'); + $user->setLastName('Doe'); + + $html = (new TwoFactorEnforcedMail($user))->render(); + + $this->assertStringContainsString('Jane Doe', $html); + $this->assertStringContainsString('/accounts/user/profile', $html); + $this->assertStringNotContainsString('have been set up', $html); + } +} From d52b95291b373703bb0404bc5814df97db6ed903 Mon Sep 17 00:00:00 2001 From: romanetar Date: Thu, 5 Nov 2026 18:54:42 +0100 Subject: [PATCH 7/7] test(mfa): set mail.from in TwoFactorEnforcedMailTest CI does not define MAIL_FROM_EMAIL, so Mailable::from() received a null address and rendering failed. The test no longer depends on the environment. --- tests/unit/MFA/TwoFactorEnforcedMailTest.php | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/unit/MFA/TwoFactorEnforcedMailTest.php b/tests/unit/MFA/TwoFactorEnforcedMailTest.php index 981131f67..e834e10e7 100644 --- a/tests/unit/MFA/TwoFactorEnforcedMailTest.php +++ b/tests/unit/MFA/TwoFactorEnforcedMailTest.php @@ -22,6 +22,13 @@ */ final class TwoFactorEnforcedMailTest extends BrowserKitTestCase { + protected function setUp(): void + { + parent::setUp(); + // the sender comes from env vars that CI does not define + config(['mail.from' => ['address' => 'noreply@example.com', 'name' => 'noreply']]); + } + public function testUserControlledNameIsEscaped(): void { $user = new User();