diff --git a/app/Console/Commands/EnforceAdmin2FACommand.php b/app/Console/Commands/EnforceAdmin2FACommand.php new file mode 100644 index 000000000..cbc49e10e --- /dev/null +++ b/app/Console/Commands/EnforceAdmin2FACommand.php @@ -0,0 +1,178 @@ +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', []))); + + 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, 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) { + $last_id = $id; + $email = (string)$id; + try { + $user = EntityManager::getRepository(User::class)->find($id); + if (is_null($user)) { + continue; + } + $email = $user->getEmail(); + + // 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 notified)', $email)); + continue; + } + + if ($dry_run) { + $processed++; + $this->line(sprintf(' would process %s', $email)); + continue; + } + + // 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' => self::AuditSource] + ); + + $processed++; + $this->line(sprintf(' processed %s', $email)); + } catch (\Throwable $ex) { + $errors++; + Log::error($ex); + $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( + '%sSummary: processed %d, skipped %d, errors %d', + $dry_run ? '[DRY RUN] ' : '', + $processed, + $skipped, + $errors + )); + + 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[] + */ + 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/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. 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..d4b73b6fb --- /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.

+

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

+ + diff --git a/tests/EnforceAdmin2FACommandIntegrationTest.php b/tests/EnforceAdmin2FACommandIntegrationTest.php new file mode 100644 index 000000000..0c568d6f8 --- /dev/null +++ b/tests/EnforceAdmin2FACommandIntegrationTest.php @@ -0,0 +1,183 @@ +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(0, $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 testSecondRunSkipsAlreadyNotified(): void + { + Artisan::call('idp:enforce-admin-2fa'); + + Artisan::call('idp:enforce-admin-2fa'); + + $this->assertStringContainsString('processed 0, skipped 1, errors 0', Artisan::output()); + 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 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]); + + $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..4bf1203a9 --- /dev/null +++ b/tests/unit/Console/EnforceAdmin2FACommandTest.php @@ -0,0 +1,199 @@ +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->audit_service = Mockery::mock(ITwoFactorAuditService::class); + $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; + } + + 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->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'); + + $this->assertSame(0, $code); + $this->assertStringContainsString('processed 1, skipped 0, errors 0', Artisan::output()); + Mail::assertSent(TwoFactorEnforcedMail::class, 1); + } + + public function testSkipsUsersAlreadyNotified(): void + { + $this->markNotified($this->createUser('enforce-b@nomail.com', true)); + + $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 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); + + $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->markNotified($this->createUser('enforce-e@nomail.com', true)); + + $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 testMailFailureOnOneUserDoesNotStopTheRunNorAuditsIt(): void + { + $this->createUser('enforce-f@nomail.com', true); + $this->createUser('enforce-g@nomail.com', true); + + 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'); + + $this->assertSame(1, $code); + $this->assertStringContainsString('processed 1, skipped 0, errors 1', Artisan::output()); + Mail::assertSent(TwoFactorEnforcedMail::class, 1); + } +} diff --git a/tests/unit/MFA/TwoFactorEnforcedMailTest.php b/tests/unit/MFA/TwoFactorEnforcedMailTest.php new file mode 100644 index 000000000..e834e10e7 --- /dev/null +++ b/tests/unit/MFA/TwoFactorEnforcedMailTest.php @@ -0,0 +1,59 @@ + ['address' => 'noreply@example.com', 'name' => 'noreply']]); + } + + public function testUserControlledNameIsEscaped(): void + { + $user = new User(); + $user->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); + } +}