From 086875a28f9bda759d274e11da892cc60aa35d16 Mon Sep 17 00:00:00 2001 From: romanetar Date: Fri, 6 Nov 2026 13:25:16 +0100 Subject: [PATCH 1/3] feat(2fa): enroll recovery codes at the next 2FA challenge and count them only once acknowledged A user who requires 2FA and has no usable batch of recovery codes (none, exhausted or unacknowledged) is now challenged at every sign-in, trusted device included, and gets the non-dismissable "Save Your Recovery Codes" modal right after the challenge. A batch counts as usable only after the user confirms it, and the confirmation is bound to the exact batch shown (SDS idp-mfa.md rev 2.5 + 2.6, ClickUp 86bcy7rv7). Backend - users.recovery_codes_acknowledged_at with a backfill migration. - RecoveryCodesBatch: the generation methods return the codes and the batch id read inside the same locked transaction, so a concurrent regeneration cannot pair dead codes with another batch's id. - Every path that writes or deletes codes (generation, acknowledgment, redemption) locks the users row first and re-hydrates the entity. - POST recovery-codes/enroll (window after a real challenge, 5 per 15 min per authenticated user) and recovery-codes/acknowledge; 2fa/enable is rejected for users who already require 2FA; regenerate accepts a user without a password inside the enrollment window. - Auth::login() regenerates the session CSRF token, so verify2FA and verify2FARecovery return the current csrf_token for the calls that follow. - Authenticate no longer redirects JSON requests to the session backurl. Frontend - Login page enrolls, shows the modal and navigates only after the batch is acknowledged; enrollment failures never block the sign-in. - Profile panel renders the server's status keys and enrolls only when the user clicks "Set up recovery codes". Docs: ADR 0002 supersedes 0001. --- .env.example | 5 + .../workflows/pull_request_frontend_tests.yml | 1 + .github/workflows/push_frontend_tests.yml | 1 + .../Controllers/Api/UserApiController.php | 99 ++++- app/Http/Controllers/UserController.php | 54 ++- app/Http/Middleware/Authenticate.php | 9 + .../DoctrineUserRecoveryCodeRepository.php | 34 ++ app/Services/Auth/IRecoveryCodeService.php | 62 +++- .../Auth/ITwoFactorRateLimitService.php | 1 + app/Services/Auth/MFAGateService.php | 14 +- app/Services/Auth/RecoveryCodeService.php | 196 ++++++++-- app/Services/Auth/RecoveryCodesBatch.php | 52 +++ app/Services/Auth/RecoveryCodesStatus.php | 23 +- .../Auth/TwoFactorRateLimitService.php | 7 + .../Auth/TwoFactorServiceProvider.php | 20 ++ .../MFA/AbstractMFAChallengeStrategy.php | 6 + app/libs/Auth/MFAConstants.php | 8 + app/libs/Auth/Models/TwoFactorAuditLog.php | 2 + app/libs/Auth/Models/User.php | 20 ++ .../IUserRecoveryCodeRepository.php | 19 + config/two_factor.php | 17 + database/migrations/Version20261105120000.php | 78 ++++ ...tion-not-triggered-by-group-enforcement.md | 2 +- ...y-code-enrollment-at-next-2fa-challenge.md | 98 +++++ .../js/components/recovery_code_modal.js | 20 +- .../js/components/recovery_codes_panel.js | 224 ++++++++++-- resources/js/components/two_factor_section.js | 44 ++- resources/js/login/actions.js | 17 + resources/js/login/login.js | 121 ++++++- resources/js/profile/actions.js | 22 +- resources/js/profile/profile.js | 11 +- resources/views/auth/login.blade.php | 4 + resources/views/profile.blade.php | 8 +- routes/web.php | 2 + tests/RecoveryCodeConcurrencyTest.php | 241 +++++++++++++ tests/RecoveryCodeEnrollmentServiceTest.php | 271 ++++++++++++++ tests/RecoveryCodeRegenerationTest.php | 158 +++++++- tests/TwoFactorLoginFlowTest.php | 333 ++++++++++++++++- tests/TwoFactorProfileRoutesCsrfTest.php | 2 + tests/e2e/fixtures/index.ts | 2 + .../auth/recovery-code-enrollment.spec.ts | 69 ++++ tests/e2e/tests/oauth2/auth-code-flow.spec.ts | 12 +- tests/e2e/utils/recovery_enrollment.ts | 39 ++ .../components/recovery_codes_panel.test.js | 338 ++++++++++++++---- .../js/components/two_factor_section.test.js | 29 +- .../login/login.recovery_enrollment.test.js | 276 ++++++++++++++ tests/js/profile/actions.test.js | 41 ++- tests/support/RecoveryCodeWorker.php | 42 +++ .../MFA/AbstractMFAChallengeStrategyTest.php | 12 +- tests/unit/MFAGateServiceTest.php | 44 ++- 50 files changed, 3009 insertions(+), 201 deletions(-) create mode 100644 app/Services/Auth/RecoveryCodesBatch.php create mode 100644 database/migrations/Version20261105120000.php create mode 100644 doc/adrs/0002-recovery-code-enrollment-at-next-2fa-challenge.md create mode 100644 tests/RecoveryCodeConcurrencyTest.php create mode 100644 tests/RecoveryCodeEnrollmentServiceTest.php create mode 100644 tests/e2e/tests/auth/recovery-code-enrollment.spec.ts create mode 100644 tests/e2e/utils/recovery_enrollment.ts create mode 100644 tests/js/login/login.recovery_enrollment.test.js create mode 100644 tests/support/RecoveryCodeWorker.php diff --git a/.env.example b/.env.example index 5bac6f8c2..d65f1e0b6 100644 --- a/.env.example +++ b/.env.example @@ -163,3 +163,8 @@ L5_SWAGGER_CONST_TOKEN_URL='/oauth2/token' # L5_FORMAT_TO_USE_FOR_DOCS=yaml L5_SWAGGER_GENERATE_ALWAYS=true # Dev setting L5_SWAGGER_OPEN_API_SPEC_VERSION=3.1.2 + +# 2FA recovery-code enrollment (SDS idp-mfa.md §4.12, §4.14) +TWO_FACTOR_RECOVERY_ENROLLMENT_WINDOW=600 +TWO_FACTOR_MAX_ENROLL_REQUESTS=5 +TWO_FACTOR_ENROLL_WINDOW_MINUTES=15 diff --git a/.github/workflows/pull_request_frontend_tests.yml b/.github/workflows/pull_request_frontend_tests.yml index 4e5b8f646..25b56bee1 100644 --- a/.github/workflows/pull_request_frontend_tests.yml +++ b/.github/workflows/pull_request_frontend_tests.yml @@ -109,6 +109,7 @@ jobs: php artisan idp:create-super-admin mfa-oauth2@test.com '1Qaz2wsx!' php artisan idp:create-super-admin mfa-oauth2-consent@test.com '1Qaz2wsx!' php artisan idp:create-super-admin mfa-oauth2-trust@test.com '1Qaz2wsx!' + php artisan idp:create-super-admin mfa-enroll@test.com '1Qaz2wsx!' php artisan idp:create-oauth2-test-client - name: Install Playwright Chromium run: npx playwright install --with-deps chromium diff --git a/.github/workflows/push_frontend_tests.yml b/.github/workflows/push_frontend_tests.yml index 2e1d05f58..24c7c34b3 100644 --- a/.github/workflows/push_frontend_tests.yml +++ b/.github/workflows/push_frontend_tests.yml @@ -110,6 +110,7 @@ jobs: php artisan idp:create-super-admin mfa-oauth2@test.com '1Qaz2wsx!' php artisan idp:create-super-admin mfa-oauth2-consent@test.com '1Qaz2wsx!' php artisan idp:create-super-admin mfa-oauth2-trust@test.com '1Qaz2wsx!' + php artisan idp:create-super-admin mfa-enroll@test.com '1Qaz2wsx!' php artisan idp:create-oauth2-test-client - name: Install Playwright Chromium run: npx playwright install --with-deps chromium diff --git a/app/Http/Controllers/Api/UserApiController.php b/app/Http/Controllers/Api/UserApiController.php index b334959b8..fa2274680 100644 --- a/app/Http/Controllers/Api/UserApiController.php +++ b/app/Http/Controllers/Api/UserApiController.php @@ -17,6 +17,7 @@ use App\Http\Controllers\UserValidationRulesFactory; use App\ModelSerializers\SerializerRegistry; use App\Services\Auth\IRecoveryCodeService; +use App\Services\Auth\RecoveryCodesBatch; use Auth\Repositories\IUserRepository; use Auth\User; use Exception; @@ -282,13 +283,18 @@ public function enableTwoFactor() $user = Auth::user(); $method = $data['method']; - if ($user->isTwoFactorEnabled()) { - return $this->error412(['method' => ['Two-factor authentication is already enabled. Use the regenerate recovery codes endpoint to rotate your codes.']]); + // shouldRequire2FA() covers both a stored two_factor_enabled flag and + // group enforcement. An enforced admin must not mint a batch here without + // the password (and would leave two_factor_enabled=true behind, keeping + // them enrolled after leaving the group): they get codes through enroll + // (inside the enrollment window) or regenerate (with the password). + if ($user->shouldRequire2FA()) { + return $this->error412(['method' => ['Two-factor authentication is already required for this account. Use the recovery codes section to set up or regenerate your codes.']]); } - $codes = $this->recovery_code_service->enableTwoFactorAndGenerateCodes($user, $method); + $batch = $this->recovery_code_service->enableTwoFactorAndGenerateCodes($user, $method); - return $this->ok(['recovery_codes' => $codes]); + return $this->ok($this->batchPayload($user, $batch)); }); } @@ -305,20 +311,99 @@ public function regenerateRecoveryCodes() return $this->processRequest(function () { $data = Request::all(); + // current_password is enforced by the service: required for a user + // with a password; a user without one is accepted instead inside the + // enrollment window (SDS idp-mfa.md §4.10.3). $validator = Validator::make($data, [ - 'current_password' => 'required|string', + 'current_password' => 'sometimes|nullable|string', ]); if (!$validator->passes()) { return $this->error412($validator->getMessageBag()->getMessages()); } - $codes = $this->recovery_code_service->regenerateRecoveryCodes(Auth::user(), $data['current_password']); + $user = Auth::user(); + $batch = $this->recovery_code_service->regenerateRecoveryCodes($user, $data['current_password'] ?? null); + + return $this->ok($this->batchPayload($user, $batch)); + }); + } + + /** + * Generates a recovery-code batch without the password for a user who + * requires enrollment, right after a real 2FA challenge (SDS idp-mfa.md + * §4.10.3). Rate limited by 2fa.rate:enroll, keyed by the authenticated user. + * + * @return \Illuminate\Http\JsonResponse|mixed + */ + public function enrollRecoveryCodes() + { + if (!Auth::check()) + return $this->error403(); - return $this->ok(['recovery_codes' => $codes]); + return $this->processRequest(function () { + $user = Auth::user(); + + if (!$this->recovery_code_service->requiresRecoveryCodeEnrollment($user)) { + return $this->error412(['recovery_codes' => ['Recovery codes enrollment is not required for this account.']]); + } + + if (!$this->recovery_code_service->isWithinEnrollmentWindow($user)) { + return $this->error412(['recovery_codes' => ['Your last two-factor verification is too old. Sign out and sign in again, then set up your recovery codes.']]); + } + + $batch = $this->recovery_code_service->generateRecoveryCodes($user); + + return $this->ok($this->batchPayload($user, $batch)); }); } + /** + * Confirms that the user saved the batch they were shown ("I've Saved These + * Codes"). The confirmation is bound to the exact batch. + * + * @return \Illuminate\Http\JsonResponse|mixed + */ + public function acknowledgeRecoveryCodes() + { + if (!Auth::check()) + return $this->error403(); + + return $this->processRequest(function () { + $validator = Validator::make(Request::all(), [ + 'batch_id' => 'required|integer', + ]); + + if (!$validator->passes()) { + return $this->error412($validator->getMessageBag()->getMessages()); + } + + $user = Auth::user(); + + if (!$this->recovery_code_service->acknowledgeRecoveryCodes($user, (int)Request::input('batch_id'))) { + return $this->error412(['batch_id' => ['This is not your current batch of recovery codes. Generate a new one.']]); + } + + return $this->ok($this->recovery_code_service->getStatus($user)->toArray()); + }); + } + + /** + * 200 body of every generation endpoint: the plaintext codes, the batch + * identifier produced by the generating transaction and the recovery-codes + * standing (RecoveryCodesStatus keys), so the UI updates without a reload. + */ + private function batchPayload(User $user, RecoveryCodesBatch $batch): array + { + return array_merge( + [ + 'recovery_codes' => $batch->getCodes(), + 'batch_id' => $batch->getBatchId(), + ], + $this->recovery_code_service->getStatus($user)->toArray() + ); + } + public function revokeAllMyTokens() { if (!Auth::check()) diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index 886b24a3d..f765b5c6d 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -770,6 +770,7 @@ public function verify2FA() // Second factor verified: establish the session. $this->auth_service->loginUser($user, $pending->shouldRemember()); + $this->markMFACompleted($user); if ($trust_device) { // Best-effort: the OTP is already redeemed and the session @@ -806,8 +807,21 @@ public function verify2FA() // code with no way to recover it client-side. A real top-level navigation to // this URL lets the browser complete that chain natively instead - CORS never // applies to page navigations, only to XHR/fetch. + // The recovery-codes standing rides along so the login page knows when + // to show the enrollment modal (recovery_codes_enrollment_required). $redirect = $this->login_strategy->postLogin(); - return $this->ok(['redirect_url' => $redirect->getTargetUrl()]); + return $this->ok(array_merge( + [ + 'redirect_url' => $redirect->getTargetUrl(), + // Auth::login() regenerates the session id AND its CSRF token + // (SessionGuard::updateSession() -> Store::regenerate()), so the + // token the login page was rendered with is stale from here on. + // The enrollment calls the page makes next (enroll / acknowledge) + // must send this one. + 'csrf_token' => csrf_token(), + ], + $this->recovery_code_service->getStatus($user)->toArray() + )); } catch (ValidationException $ex) { Log::warning($ex); return $this->error412($ex->getMessages()); @@ -881,6 +895,7 @@ public function verify2FARecovery() } $this->auth_service->loginUser($user, $pending->shouldRemember()); + $this->markMFACompleted($user); $strategy->clearPendingState(); $this->clearMFAUISessionState(); @@ -904,7 +919,15 @@ public function verify2FARecovery() // when they've just burned into their last few codes (see RecoveryCodesStatus). $redirect = $this->login_strategy->postLogin(); return $this->ok(array_merge( - ['redirect_url' => $redirect->getTargetUrl()], + [ + 'redirect_url' => $redirect->getTargetUrl(), + // Auth::login() regenerates the session id AND its CSRF token + // (SessionGuard::updateSession() -> Store::regenerate()), so the + // token the login page was rendered with is stale from here on. + // The enrollment calls the page makes next (enroll / acknowledge) + // must send this one. + 'csrf_token' => csrf_token(), + ], $this->recovery_code_service->getStatus($user)->toArray() )); } catch (ValidationException $ex) { @@ -916,6 +939,23 @@ public function verify2FARecovery() } } + /** + * Records that this session just completed a real 2FA challenge for the user + * (SDS idp-mfa.md §4.10.3). Called only by verify2FA / verify2FARecovery, + * after AuthService::loginUser() succeeded: Auth::login() migrates the + * session id but keeps its data, so the key survives, and writing it before a + * failed login would leave a stale marker. It bounds the window in which the + * enrollment endpoint may generate a batch without the password. Trusted-device + * sign-ins and every other session establishment never write it. + */ + private function markMFACompleted(User $user): void + { + Session::put(MFAConstants::SESSION_KEY_COMPLETED_AT, [ + 'user_id' => $user->getId(), + 'at' => time(), + ]); + } + /** * Re-issues a 2FA challenge for the pending login and returns the challenge payload. * @@ -1163,6 +1203,9 @@ public function getProfile() $lang2Code[] = $lang; } + $status = $this->recovery_code_service->getStatus($user); + $window_open = $this->recovery_code_service->isWithinEnrollmentWindow($user); + return View::make("profile", array_merge([ 'user' => json_encode(SerializerRegistry::getInstance()->getSerializer( $user, SerializerRegistry::SerializerType_Private)->serialize()), @@ -1172,7 +1215,12 @@ public function getProfile() 'countries' => CountryList::getCountries(), 'languages' => $lang2Code, 'two_factor_enabled' => $user->shouldRequire2FA(), - ], $this->recovery_code_service->getStatus($user)->toArray())); + // The session completed a 2FA challenge within the enrollment window: + // a batch can be generated without the password. + 'recovery_codes_window_open' => $window_open, + // Enrollment is required AND reachable now: the panel offers "Set up recovery codes". + 'recovery_codes_enrollment_available' => $status->isEnrollmentRequired() && $window_open, + ], $status->toArray())); } public function deleteTrustedSite($id) diff --git a/app/Http/Middleware/Authenticate.php b/app/Http/Middleware/Authenticate.php index c48c91352..bb007f0d5 100644 --- a/app/Http/Middleware/Authenticate.php +++ b/app/Http/Middleware/Authenticate.php @@ -56,6 +56,15 @@ public function handle($request, Closure $next, $guard = null) $current_user->getGroupsNice() ) ); + // An XHR that expects JSON must reach its controller: the first + // authenticated request after a deep-link sign-in would otherwise be + // answered with a 302 to backurl, and the caller would read an HTML page + // instead of its JSON (e.g. the recovery-code enrollment call made by + // the login page). backurl stays in the session for the real navigation. + if ($request->expectsJson()) { + return $next($request); + } + $redirect = Session::get('backurl'); if (!empty($redirect)) { Session::forget('backurl'); diff --git a/app/Repositories/DoctrineUserRecoveryCodeRepository.php b/app/Repositories/DoctrineUserRecoveryCodeRepository.php index cda5c4d18..0d5fd4ad7 100644 --- a/app/Repositories/DoctrineUserRecoveryCodeRepository.php +++ b/app/Repositories/DoctrineUserRecoveryCodeRepository.php @@ -60,4 +60,38 @@ public function deleteAllForUser(User $user): int ->setParameter('user', $user); return (int) $qb->getQuery()->execute(); } + + public function lockUserRow(User $user): void + { + // SELECT ... FOR UPDATE with HINT_REFRESH: re-hydrates the managed User + // from the locked row. EntityManager::lock() alone takes the lock but + // leaves the in-memory state untouched, so a value changed by a + // concurrent transaction (recovery_codes_acknowledged_at) would be stale. + $this->getEntityManager() + ->createQueryBuilder() + ->select('u') + ->from(User::class, 'u') + ->where('u.id = :id') + ->setParameter('id', $user->getId()) + ->getQuery() + ->setLockMode(\Doctrine\DBAL\LockMode::PESSIMISTIC_WRITE) + ->setHint(\Doctrine\ORM\Query::HINT_REFRESH, true) + ->getOneOrNullResult(); + } + + public function getCurrentBatchId(User $user): ?int + { + $em = $this->getEntityManager(); + $em->flush(); + + $id = $em->createQueryBuilder() + ->select('MIN(c.id)') + ->from(UserRecoveryCode::class, 'c') + ->where('c.user = :user') + ->setParameter('user', $user) + ->getQuery() + ->getSingleScalarResult(); + + return is_null($id) ? null : (int) $id; + } } diff --git a/app/Services/Auth/IRecoveryCodeService.php b/app/Services/Auth/IRecoveryCodeService.php index ab6de16ec..aaf38011d 100644 --- a/app/Services/Auth/IRecoveryCodeService.php +++ b/app/Services/Auth/IRecoveryCodeService.php @@ -26,23 +26,26 @@ interface IRecoveryCodeService * Invalidates every existing recovery code for the user and generates a fresh * batch. Plaintext codes are returned once and are never persisted/exposed again. * + * A user with a password must confirm it. A user without one (social-only + * accounts) is accepted instead while the session is inside the enrollment + * window (isWithinEnrollmentWindow()). + * * @param User $user - * @param string $currentPassword - * @return string[] plaintext codes formatted as XXXX-XXXX - * @throws ValidationException if $currentPassword does not match the user's password + * @param string|null $currentPassword + * @return RecoveryCodesBatch codes formatted as XXXX-XXXX plus the batch identifier + * @throws ValidationException if the password does not match, or a passwordless user is outside the window */ - public function regenerateRecoveryCodes(User $user, string $currentPassword): array; + public function regenerateRecoveryCodes(User $user, ?string $currentPassword): RecoveryCodesBatch; /** * Invalidates every existing recovery code for the user and generates a fresh - * batch, without requiring password confirmation. Intended for first-time - * generation right after 2FA enrollment, where the user's identity is already - * established by the current session. + * batch, without requiring password confirmation. Callers must already have + * established the user's identity (enrollment endpoint: recent 2FA challenge). * * @param User $user - * @return string[] plaintext codes formatted as XXXX-XXXX + * @return RecoveryCodesBatch codes formatted as XXXX-XXXX plus the batch identifier */ - public function generateRecoveryCodes(User $user): array; + public function generateRecoveryCodes(User $user): RecoveryCodesBatch; /** * Enrolls the user into the given 2FA method and generates the first batch @@ -51,10 +54,45 @@ public function generateRecoveryCodes(User $user): array; * * @param User $user * @param string $method - * @return string[] plaintext codes formatted as XXXX-XXXX + * @return RecoveryCodesBatch codes formatted as XXXX-XXXX plus the batch identifier * @throws ValidationException if $method is not a valid/enabled 2FA method */ - public function enableTwoFactorAndGenerateCodes(User $user, string $method): array; + public function enableTwoFactorAndGenerateCodes(User $user, string $method): RecoveryCodesBatch; + + /** + * Confirms that the user saved the batch they were shown. Runs under the + * users-row lock: the confirmation is bound to the exact batch. + * + * @param User $user + * @param int $batchId identifier returned when the batch was generated + * @return bool false when $batchId is not the user's current batch (nothing + * changes; the controller answers 412). True otherwise, also when + * the batch was already acknowledged (no second audit row). + */ + public function acknowledgeRecoveryCodes(User $user, int $batchId): bool; + + /** + * @param User $user + * @return int|null smallest id of the user's recovery-code rows, null when none + */ + public function getCurrentBatchId(User $user): ?int; + + /** + * shouldRequire2FA() AND (no unused codes OR the batch is unacknowledged). + * + * @param User $user + * @return bool + */ + public function requiresRecoveryCodeEnrollment(User $user): bool; + + /** + * Whether the session completed a real 2FA challenge for this user within + * two_factor.recovery_enrollment_window seconds (MFAConstants::SESSION_KEY_COMPLETED_AT). + * + * @param User $user + * @return bool + */ + public function isWithinEnrollmentWindow(User $user): bool; /** * @param User $user @@ -64,7 +102,7 @@ public function countUnusedRecoveryCodes(User $user): int; /** * @param User $user - * @return RecoveryCodesStatus remaining/total/low-threshold standing for the user + * @return RecoveryCodesStatus remaining/total/low-threshold/acknowledged/enrollment-required standing */ public function getStatus(User $user): RecoveryCodesStatus; } diff --git a/app/Services/Auth/ITwoFactorRateLimitService.php b/app/Services/Auth/ITwoFactorRateLimitService.php index 65740ccc7..e8e66312f 100644 --- a/app/Services/Auth/ITwoFactorRateLimitService.php +++ b/app/Services/Auth/ITwoFactorRateLimitService.php @@ -39,6 +39,7 @@ interface ITwoFactorRateLimitService public const ActionRecovery = 'recovery'; public const ActionResend = 'resend'; public const ActionOtp = 'otp'; + public const ActionEnroll = 'enroll'; public const RATE_LIMIT_ERROR_CODE = MFAConstants::ERROR_CODE_RATE_LIMIT; public const RATE_LIMIT_MESSAGE = 'Too many attempts. Please try again later.'; diff --git a/app/Services/Auth/MFAGateService.php b/app/Services/Auth/MFAGateService.php index 52b66260d..dcaef49e9 100644 --- a/app/Services/Auth/MFAGateService.php +++ b/app/Services/Auth/MFAGateService.php @@ -24,7 +24,8 @@ final class MFAGateService implements ITwoFactorGateService { public function __construct( - private readonly IDeviceTrustService $deviceTrustService + private readonly IDeviceTrustService $deviceTrustService, + private readonly IRecoveryCodeService $recoveryCodeService ) { } @@ -37,6 +38,17 @@ public function requiresChallenge(User $user, ?string $cookieToken): bool if (!$user->shouldRequire2FA()) { return false; } + + // A user with no usable batch (none, exhausted or unacknowledged) is + // challenged at every sign-in, trusted device included, so the + // recovery-code enrollment modal that follows the challenge is always + // reachable - also for accounts without a password. Checked before + // device trust, so a trusted device row is not even touched + // (SDS idp-mfa.md §4.2 step 4). + if ($this->recoveryCodeService->requiresRecoveryCodeEnrollment($user)) { + return true; + } + return !$this->deviceTrustService->isDeviceTrusted($user, $cookieToken); } } diff --git a/app/Services/Auth/RecoveryCodeService.php b/app/Services/Auth/RecoveryCodeService.php index a626dd82e..6394d1ba4 100644 --- a/app/Services/Auth/RecoveryCodeService.php +++ b/app/Services/Auth/RecoveryCodeService.php @@ -15,11 +15,13 @@ use App\libs\Auth\Models\TwoFactorAuditLog; use App\libs\Auth\Models\UserRecoveryCode; +use Auth\MFAConstants; use Auth\Repositories\IUserRecoveryCodeRepository; use Auth\Repositories\IUserRepository; use Auth\User; use Illuminate\Support\Facades\Hash; use Illuminate\Support\Facades\Log; +use Illuminate\Support\Facades\Session; use Laminas\Math\Rand; use models\exceptions\ValidationException; use Utils\Db\ITransactionService; @@ -33,6 +35,10 @@ final class RecoveryCodeService implements IRecoveryCodeService { private const CODE_CHARSET = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789'; + private const AckMismatch = 'mismatch'; + private const AckAlready = 'already'; + private const AckDone = 'done'; + public function __construct( private readonly IUserRecoveryCodeRepository $repository, private readonly IUserRepository $user_repository, @@ -44,10 +50,18 @@ public function __construct( /** * @inheritDoc */ - public function regenerateRecoveryCodes(User $user, string $currentPassword): array + public function regenerateRecoveryCodes(User $user, ?string $currentPassword): RecoveryCodesBatch { - if (!$user->checkPassword(trim($currentPassword))) { - throw new ValidationException('current_password is not correct.'); + 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 regenerate your recovery codes right after the sign-in.' + ); } return $this->generateRecoveryCodes($user); @@ -56,32 +70,22 @@ public function regenerateRecoveryCodes(User $user, string $currentPassword): ar /** * @inheritDoc */ - public function generateRecoveryCodes(User $user): array + public function generateRecoveryCodes(User $user): RecoveryCodesBatch { - $codes = $this->tx_service->transaction(fn() => $this->regenerateCodesForUser($user)); + $batch = $this->tx_service->transaction(function () use ($user) { + $this->repository->lockUserRow($user); + return $this->regenerateCodesForUser($user); + }); - // Best-effort: the codes are already committed and about to be shown to - // the user, so an audit-logging failure must not 500 this response - a - // client retry on a 500 would regenerate and invalidate the codes it - // just received. - try { - $this->audit_service->log( - $user, - TwoFactorAuditLog::EventRecoveryCodesGenerated, - TwoFactorAuditLog::MethodRecovery, - IPHelper::getUserIp() - ); - } catch (\Throwable $ex) { - Log::warning($ex); - } + $this->logCodesGenerated($user, $batch); - return $codes; + return $batch; } /** * @inheritDoc */ - public function enableTwoFactorAndGenerateCodes(User $user, string $method): array + public function enableTwoFactorAndGenerateCodes(User $user, string $method): RecoveryCodesBatch { // Everything must live in a single transaction() call: it opens/commits // its own connection-level transaction and closes the entity manager on @@ -89,7 +93,10 @@ public function enableTwoFactorAndGenerateCodes(User $user, string $method): arr // second call inside it (e.g. by calling generateRecoveryCodes() here) // would let an inner failure tear down the EM out from under this // still-running outer transaction. - $codes = $this->tx_service->transaction(function () use ($user, $method) { + $batch = $this->tx_service->transaction(function () use ($user, $method) { + // The lock re-hydrates $user from the database, so it must come + // before enable2FA() mutates the entity. + $this->repository->lockUserRow($user); $user->enable2FA($method); $this->user_repository->add($user, false); @@ -100,34 +107,138 @@ public function enableTwoFactorAndGenerateCodes(User $user, string $method): arr // committed, so an audit-logging failure must not 500 this response - a // client retry on a 500 would regenerate and invalidate the codes it // just received. + $this->logCodesGenerated($user, $batch); try { $this->audit_service->log( $user, - TwoFactorAuditLog::EventRecoveryCodesGenerated, - TwoFactorAuditLog::MethodRecovery, + TwoFactorAuditLog::EventEnrollmentChanged, + $method, IPHelper::getUserIp() ); + } catch (\Throwable $ex) { + Log::warning($ex); + } + + return $batch; + } + + /** + * @inheritDoc + */ + public function acknowledgeRecoveryCodes(User $user, int $batchId): bool + { + // The batch mismatch is reported through the return value, not by + // throwing inside the callback: a throw makes the transaction service + // close the EntityManager, which nothing after it could use. + $outcome = $this->tx_service->transaction(function () use ($user, $batchId) { + $this->repository->lockUserRow($user); + + $current = $this->repository->getCurrentBatchId($user); + if (is_null($current) || $current !== $batchId) { + return self::AckMismatch; + } + + if (!is_null($user->getRecoveryCodesAcknowledgedAt())) { + return self::AckAlready; + } + + $user->setRecoveryCodesAcknowledgedAt(new \DateTime('now', new \DateTimeZone('UTC'))); + return self::AckDone; + }); + + if ($outcome === self::AckMismatch) { + return false; + } + + if ($outcome === self::AckDone) { + // Best-effort: the acknowledgment is already committed. + try { + $this->audit_service->log( + $user, + TwoFactorAuditLog::EventRecoveryCodesAcknowledged, + TwoFactorAuditLog::MethodRecovery, + IPHelper::getUserIp(), + ['batch_id' => $batchId] + ); + } catch (\Throwable $ex) { + Log::warning($ex); + } + } + + return true; + } + /** + * @inheritDoc + */ + public function getCurrentBatchId(User $user): ?int + { + return $this->repository->getCurrentBatchId($user); + } + + /** + * @inheritDoc + */ + public function requiresRecoveryCodeEnrollment(User $user): bool + { + return $this->enrollmentRequired($user, $this->countUnusedRecoveryCodes($user)); + } + + /** + * @inheritDoc + */ + public function isWithinEnrollmentWindow(User $user): bool + { + $marker = Session::get(MFAConstants::SESSION_KEY_COMPLETED_AT); + if (!is_array($marker) || !isset($marker['user_id'], $marker['at'])) { + return false; + } + + if ((int)$marker['user_id'] !== $user->getId()) { + return false; + } + + $age = time() - (int)$marker['at']; + return $age >= 0 && $age <= (int)config('two_factor.recovery_enrollment_window', 600); + } + + private function enrollmentRequired(User $user, int $unusedCount): bool + { + return $user->shouldRequire2FA() + && ($unusedCount === 0 || is_null($user->getRecoveryCodesAcknowledgedAt())); + } + + /** + * Audit row of a generation. Best-effort: the codes are already committed + * and about to be shown to the user, so an audit-logging failure must not + * 500 this response - a client retry on a 500 would regenerate and + * invalidate the codes it just received. The batch identifier comes from + * the generating transaction, never from a read after the commit. + */ + private function logCodesGenerated(User $user, RecoveryCodesBatch $batch): void + { + try { $this->audit_service->log( $user, - TwoFactorAuditLog::EventEnrollmentChanged, - $method, - IPHelper::getUserIp() + TwoFactorAuditLog::EventRecoveryCodesGenerated, + TwoFactorAuditLog::MethodRecovery, + IPHelper::getUserIp(), + ['batch_id' => $batch->getBatchId()] ); } catch (\Throwable $ex) { Log::warning($ex); } - - return $codes; } /** * Invalidates every existing recovery code for the user and generates a - * fresh batch, within the caller's already-open transaction. + * fresh batch, within the caller's already-open transaction. The caller + * must already hold the users-row lock (IUserRecoveryCodeRepository::lockUserRow()). * - * @return string[] plaintext codes formatted as XXXX-XXXX + * Also clears recovery_codes_acknowledged_at: the new batch counts as usable + * only after the user confirms it. Covers every generation path. */ - private function regenerateCodesForUser(User $user): array + private function regenerateCodesForUser(User $user): RecoveryCodesBatch { $count = (int)config('auth.recovery_codes.count', 10); $length = (int)config('auth.recovery_codes.length', 8); @@ -146,7 +257,18 @@ private function regenerateCodesForUser(User $user): array $this->repository->add($code, false); } - return array_map(static fn(string $code) => implode('-', str_split($code, 4)), $plaintext_codes); + $user->setRecoveryCodesAcknowledgedAt(null); + + // Read inside the locked transaction (getCurrentBatchId() flushes first). + $batchId = $this->repository->getCurrentBatchId($user); + if (is_null($batchId)) { + throw new \RuntimeException('Recovery codes were not persisted.'); + } + + return new RecoveryCodesBatch( + array_map(static fn(string $code) => implode('-', str_split($code, 4)), $plaintext_codes), + $batchId + ); } /** @@ -162,10 +284,14 @@ public function countUnusedRecoveryCodes(User $user): int */ public function getStatus(User $user): RecoveryCodesStatus { + $remaining = $this->countUnusedRecoveryCodes($user); + return new RecoveryCodesStatus( - $this->countUnusedRecoveryCodes($user), + $remaining, (int) config('auth.recovery_codes.count', 10), - (int) config('auth.recovery_codes.low_threshold', 3) + (int) config('auth.recovery_codes.low_threshold', 3), + !is_null($user->getRecoveryCodesAcknowledgedAt()), + $this->enrollmentRequired($user, $remaining) ); } } diff --git a/app/Services/Auth/RecoveryCodesBatch.php b/app/Services/Auth/RecoveryCodesBatch.php new file mode 100644 index 000000000..502e01257 --- /dev/null +++ b/app/Services/Auth/RecoveryCodesBatch.php @@ -0,0 +1,52 @@ +codes; + } + + public function getBatchId(): int + { + return $this->batchId; + } +} diff --git a/app/Services/Auth/RecoveryCodesStatus.php b/app/Services/Auth/RecoveryCodesStatus.php index c99e9f2cc..84e452553 100644 --- a/app/Services/Auth/RecoveryCodesStatus.php +++ b/app/Services/Auth/RecoveryCodesStatus.php @@ -29,6 +29,8 @@ public function __construct( private readonly int $remaining, private readonly int $total, private readonly int $low_threshold, + private readonly bool $acknowledged, + private readonly bool $enrollment_required, ) {} public function getRemaining(): int @@ -47,7 +49,24 @@ public function getLowThreshold(): int } /** - * @return array + * Whether the user confirmed the current batch ("I've Saved These Codes"). + */ + public function isAcknowledged(): bool + { + return $this->acknowledged; + } + + /** + * Whether the user requires 2FA but has no usable batch (none, exhausted + * or unacknowledged): they must be challenged and get the enrollment modal. + */ + public function isEnrollmentRequired(): bool + { + return $this->enrollment_required; + } + + /** + * @return array */ public function toArray(): array { @@ -55,6 +74,8 @@ public function toArray(): array 'recovery_codes_remaining' => $this->remaining, 'recovery_codes_total' => $this->total, 'recovery_codes_low_threshold' => $this->low_threshold, + 'recovery_codes_acknowledged' => $this->acknowledged, + 'recovery_codes_enrollment_required' => $this->enrollment_required, ]; } } diff --git a/app/Services/Auth/TwoFactorRateLimitService.php b/app/Services/Auth/TwoFactorRateLimitService.php index de71e04de..8026b2739 100644 --- a/app/Services/Auth/TwoFactorRateLimitService.php +++ b/app/Services/Auth/TwoFactorRateLimitService.php @@ -96,6 +96,13 @@ private function limitsFor(string $action): array ]; } + if ($action === self::ActionEnroll) { + return [ + (int) Config::get('two_factor.rate_limit.max_enroll_requests', 5), + (int) Config::get('two_factor.rate_limit.enroll_window_minutes', 15) * 60, + ]; + } + return [ (int) Config::get('two_factor.rate_limit.max_attempts', 3), (int) Config::get('two_factor.rate_limit.window_seconds', 900), diff --git a/app/Services/Auth/TwoFactorServiceProvider.php b/app/Services/Auth/TwoFactorServiceProvider.php index 4030fcf1f..4e0ac29bc 100644 --- a/app/Services/Auth/TwoFactorServiceProvider.php +++ b/app/Services/Auth/TwoFactorServiceProvider.php @@ -19,6 +19,7 @@ use Illuminate\Contracts\Support\DeferrableProvider; use Illuminate\Http\Request; use Illuminate\Support\Facades\App; +use Illuminate\Support\Facades\Auth; use Illuminate\Support\Facades\RateLimiter; use Illuminate\Support\Facades\Response; use Illuminate\Support\Facades\Session; @@ -90,6 +91,25 @@ private function registerRateLimiters(): void RateLimiter::for($limiterName(ITwoFactorRateLimitService::ActionRecovery), fn () => $bySessionPendingUser(ITwoFactorRateLimitService::ActionRecovery)); RateLimiter::for($limiterName(ITwoFactorRateLimitService::ActionResend), fn () => $bySessionPendingUser(ITwoFactorRateLimitService::ActionResend)); + // Recovery-code enrollment is called from the login page right after + // verify2FA / verify2FARecovery, which already cleared the pending + // state: a pending-user subject would resolve to nothing and the + // middleware would let every request through unthrottled. The subject + // is the authenticated user instead. + RateLimiter::for($limiterName(ITwoFactorRateLimitService::ActionEnroll), function () use ($rateLimitService, $respondRateLimited) { + $userId = Auth::id(); + + if (is_null($userId)) { + return Limit::none(); + } + + return (new Limit( + (int) $userId, + $rateLimitService->getLimit(ITwoFactorRateLimitService::ActionEnroll), + $rateLimitService->getWindowSeconds(ITwoFactorRateLimitService::ActionEnroll) + ))->response($respondRateLimited); + }); + RateLimiter::for($limiterName(ITwoFactorRateLimitService::ActionOtp), function (Request $request) use ($rateLimitService, $respondRateLimited) { $username = strtolower(trim($request->input('username', ''))); diff --git a/app/Strategies/MFA/AbstractMFAChallengeStrategy.php b/app/Strategies/MFA/AbstractMFAChallengeStrategy.php index 83d5c943a..fb6a788e4 100644 --- a/app/Strategies/MFA/AbstractMFAChallengeStrategy.php +++ b/app/Strategies/MFA/AbstractMFAChallengeStrategy.php @@ -101,6 +101,12 @@ public function verifyRecoveryCode(User $user, string $code): void // or pasted exactly as displayed still matches the stored hash. $code = strtoupper(preg_replace('/[^A-Za-z0-9]/', '', $code)); + // Lock order shared by every path that writes or deletes the user's + // codes: the users row first, then user_recovery_codes (SDS idp-mfa.md + // §3.5). Taken before reading the codes so a concurrent regeneration + // or reset is fully applied or not started when we look at them. + $this->recovery_code_repository->lockUserRow($user); + foreach ($this->recovery_code_repository->getUnusedByUser($user) as $recoveryCode) { if (Hash::check($code, $recoveryCode->getCodeHash())) { // Concurrency: acquire a PESSIMISTIC_WRITE row lock and re-hydrate diff --git a/app/libs/Auth/MFAConstants.php b/app/libs/Auth/MFAConstants.php index aeb3eee4c..bd0114c0d 100644 --- a/app/libs/Auth/MFAConstants.php +++ b/app/libs/Auth/MFAConstants.php @@ -41,4 +41,12 @@ final class MFAConstants public const SESSION_KEY_PENDING_AT = '2fa_pending_at'; public const SESSION_KEY_REMEMBER = '2fa_remember'; public const SESSION_KEY_RECOVERY_ATTEMPTS = '2fa_recovery_attempts'; + + /** + * Completion marker of a real 2FA challenge: ['user_id' => int, 'at' => epoch seconds]. + * Written only by UserController::verify2FA / verify2FARecovery, right after + * the session is established (SDS idp-mfa.md §4.10.3). It bounds the window in + * which a recovery-code batch can be generated without the password. + */ + public const SESSION_KEY_COMPLETED_AT = '2fa_completed_at'; } diff --git a/app/libs/Auth/Models/TwoFactorAuditLog.php b/app/libs/Auth/Models/TwoFactorAuditLog.php index 5258978a6..c43a9903b 100644 --- a/app/libs/Auth/Models/TwoFactorAuditLog.php +++ b/app/libs/Auth/Models/TwoFactorAuditLog.php @@ -30,6 +30,7 @@ class TwoFactorAuditLog extends BaseEntity public const EventRecoveryUsed = 'recovery_used'; public const EventSettingsChanged = 'settings_changed'; public const EventRecoveryCodesGenerated = 'recovery_codes_generated'; + public const EventRecoveryCodesAcknowledged = 'recovery_codes_acknowledged'; public const MethodEmailOtp = 'email_otp'; public const MethodSmsOtp = 'sms_otp'; @@ -48,6 +49,7 @@ class TwoFactorAuditLog extends BaseEntity self::EventRecoveryUsed, self::EventSettingsChanged, self::EventRecoveryCodesGenerated, + self::EventRecoveryCodesAcknowledged, ]; private const ALLOWED_METHODS = [ diff --git a/app/libs/Auth/Models/User.php b/app/libs/Auth/Models/User.php index ab361079c..8da14ee8c 100644 --- a/app/libs/Auth/Models/User.php +++ b/app/libs/Auth/Models/User.php @@ -331,6 +331,15 @@ class User extends BaseEntity #[ORM\Column(name: 'two_factor_enforced_at', nullable: true, type: 'datetime')] private $two_factor_enforced_at; + /** + * When the user confirmed ("I've Saved These Codes") the current batch of + * recovery codes; NULL while the batch is unacknowledged or absent + * (SDS idp-mfa.md §3.1, §3.5). + * @var \DateTime|null + */ + #[ORM\Column(name: 'recovery_codes_acknowledged_at', nullable: true, type: 'datetime')] + private $recovery_codes_acknowledged_at; + /** * @var string */ @@ -488,6 +497,7 @@ public function __construct() $this->two_factor_enabled = false; $this->two_factor_method = self::MFAMethod_OTP; $this->two_factor_enforced_at = null; + $this->recovery_codes_acknowledged_at = null; // user profile settings $this->public_profile_show_photo = false; $this->public_profile_show_email = false; @@ -2425,6 +2435,16 @@ public function setTwoFactorEnforcedAt(?\DateTime $at): void $this->two_factor_enforced_at = $at; } + public function getRecoveryCodesAcknowledgedAt(): ?\DateTime + { + return $this->recovery_codes_acknowledged_at; + } + + public function setRecoveryCodesAcknowledgedAt(?\DateTime $at): void + { + $this->recovery_codes_acknowledged_at = $at; + } + /** * Whether this user is required to complete 2FA to sign in. * diff --git a/app/libs/Auth/Repositories/IUserRecoveryCodeRepository.php b/app/libs/Auth/Repositories/IUserRecoveryCodeRepository.php index 143946fb7..2125eda04 100644 --- a/app/libs/Auth/Repositories/IUserRecoveryCodeRepository.php +++ b/app/libs/Auth/Repositories/IUserRecoveryCodeRepository.php @@ -38,4 +38,23 @@ public function refreshExclusiveLock(UserRecoveryCode $code): bool; * Delete every recovery code for a user (used when regenerating). */ public function deleteAllForUser(User $user): int; + + /** + * Takes a PESSIMISTIC_WRITE lock on the user's `users` row and re-hydrates + * the entity from it, so state read afterwards (recovery_codes_acknowledged_at) + * is current. Every path that writes or deletes the user's recovery codes + * calls this first, inside its transaction, and only then touches + * user_recovery_codes: one lock order everywhere (users row, then codes) + * rules out deadlocks (SDS idp-mfa.md §3.5). Must run inside a transaction. + */ + public function lockUserRow(User $user): void; + + /** + * Identifier of the user's current batch: MIN(id) over all of the user's + * rows (used and unused), or null when the user has none. Regeneration + * deletes every previous row under the user lock, so this always identifies + * the current batch. Flushes pending inserts first so a batch created in the + * caller's transaction is visible. + */ + public function getCurrentBatchId(User $user): ?int; } diff --git a/config/two_factor.php b/config/two_factor.php index 014556baa..954056f88 100644 --- a/config/two_factor.php +++ b/config/two_factor.php @@ -79,6 +79,18 @@ */ 'session_ttl' => env('TWO_FACTOR_SESSION_TTL', 600), + /* + |-------------------------------------------------------------------------- + | Recovery-code enrollment window + |-------------------------------------------------------------------------- + | + | Seconds after a successful 2FA challenge (verify2FA / verify2FARecovery) + | during which the session can generate a recovery-code batch without the + | current password (SDS idp-mfa.md §4.10.3, §4.14). + | + */ + 'recovery_enrollment_window' => env('TWO_FACTOR_RECOVERY_ENROLLMENT_WINDOW', 600), + /* |-------------------------------------------------------------------------- | Rate Limiting @@ -103,5 +115,10 @@ // budget separately. 'max_otp_email_requests' => env('TWO_FACTOR_MAX_OTP_EMAIL_REQUESTS', 5), 'otp_email_window_minutes' => env('TWO_FACTOR_OTP_EMAIL_WINDOW_MINUTES', 15), + + // Recovery-code enrollment (POST recovery-codes/enroll), keyed by the + // authenticated user; every request counts (SDS idp-mfa.md §4.12). + 'max_enroll_requests' => env('TWO_FACTOR_MAX_ENROLL_REQUESTS', 5), + 'enroll_window_minutes' => env('TWO_FACTOR_ENROLL_WINDOW_MINUTES', 15), ], ]; diff --git a/database/migrations/Version20261105120000.php b/database/migrations/Version20261105120000.php new file mode 100644 index 000000000..20a36c824 --- /dev/null +++ b/database/migrations/Version20261105120000.php @@ -0,0 +1,78 @@ +hasTable("users") && !$builder->hasColumn("users", "recovery_codes_acknowledged_at")) { + $builder->table('users', function (Table $table) { + $table->dateTime('recovery_codes_acknowledged_at')->setNotnull(false)->setDefault(null); + }); + } + } + + /** + * The backfill runs in postUp(): up() only records schema changes in the + * $schema object (the SQL is computed from it after up() returns), so the + * new column does not exist yet while up() executes. + */ + public function postUp(Schema $schema): void + { + if (!$schema->hasTable("users") || !$schema->hasTable("user_recovery_codes")) { + return; + } + + // A user with at least one unused code is assumed to have seen the batch: + // acknowledged_at = MAX(created_at) of that user's rows. Users without + // unused codes (none, or only used) stay NULL. + $this->connection->executeStatement( + "UPDATE users u + INNER JOIN ( + SELECT user_id, MAX(created_at) AS batch_created_at + FROM user_recovery_codes + WHERE user_id IN (SELECT DISTINCT user_id FROM user_recovery_codes WHERE used_at IS NULL) + GROUP BY user_id + ) c ON c.user_id = u.id + SET u.recovery_codes_acknowledged_at = c.batch_created_at + WHERE u.recovery_codes_acknowledged_at IS NULL" + ); + } + + public function down(Schema $schema): void + { + $builder = new Builder($schema); + + if ($schema->hasTable("users") && $builder->hasColumn("users", "recovery_codes_acknowledged_at")) { + $builder->table('users', function (Table $table) { + $table->dropColumn('recovery_codes_acknowledged_at'); + }); + } + } +} diff --git a/doc/adrs/0001-recovery-code-generation-not-triggered-by-group-enforcement.md b/doc/adrs/0001-recovery-code-generation-not-triggered-by-group-enforcement.md index 31ed8bbd6..960b90524 100644 --- a/doc/adrs/0001-recovery-code-generation-not-triggered-by-group-enforcement.md +++ b/doc/adrs/0001-recovery-code-generation-not-triggered-by-group-enforcement.md @@ -2,7 +2,7 @@ ## Status -Accepted — 2026-07-08 +Superseded by 0002 ## Context diff --git a/doc/adrs/0002-recovery-code-enrollment-at-next-2fa-challenge.md b/doc/adrs/0002-recovery-code-enrollment-at-next-2fa-challenge.md new file mode 100644 index 000000000..2cc1831c4 --- /dev/null +++ b/doc/adrs/0002-recovery-code-enrollment-at-next-2fa-challenge.md @@ -0,0 +1,98 @@ +# 0002. Recovery codes are enrolled at the next 2FA challenge and counted only once acknowledged + +## Status + +Accepted — 2026-11-05. Supersedes [0001](0001-recovery-code-generation-not-triggered-by-group-enforcement.md). + +## Context + +[0001](0001-recovery-code-generation-not-triggered-by-group-enforcement.md) accepted that a user who is +force-enrolled in 2FA by group membership gets recovery codes only if they open the profile and use +"Regenerate Codes". Two problems followed from it, and a third one was found while closing them: + +1. **The gap of 0001.** A group-enforced admin who is challenged at sign-in before ever visiting the profile + has no recovery codes, and nothing in the sign-in flow prompts them to get a set. For an account without a + password (social-only) the profile path does not even exist: `Regenerate Codes` needs the current password. +2. **The unseen batch.** The profile counts unused rows of `user_recovery_codes`. The system cannot tell + whether the user ever saved the batch it displayed, so a batch the user closed without saving still reads + "10 of 10 remaining". Any design that generates codes for a user who is not looking at the screen (the + Rev 2.4 form of `idp:enforce-admin-2fa`) produces codes nobody can ever see. +3. **Passwordless generation.** `POST 2fa/enable` rejected a user only when the stored `two_factor_enabled` + flag was true, so any authenticated session of a group-enforced admin could mint a fresh batch without the + password, and left the flag set after the user left the group. + +## Decision + +A batch counts as usable only after the user confirms it, and a user who requires 2FA but has no usable batch +is challenged at every sign-in until they confirm one. The design is specified in the SDS +(`ftn-docsnsklz` `sds/idp-mfa.md` rev 2.5 and rev 2.6) and implemented as follows. + +**Acknowledgment.** `users.recovery_codes_acknowledged_at` is set when the user confirms the displayed batch +with "I've Saved These Codes" and cleared in the same transaction as every generation. The confirmation is +bound to the exact batch: the identifier is `MIN(user_recovery_codes.id)` of the user's rows, produced inside +the locked transaction that generates the batch and returned with the codes (`RecoveryCodesBatch`), never read +after the commit. `POST recovery-codes/acknowledge` answers 412 when the identifier is not the current batch. +A user *requires enrollment* when `shouldRequire2FA()` and (no unused codes or `recovery_codes_acknowledged_at` +is NULL). + +**Enrollment at the next challenge.** After a successful `verify2FA` / `verify2FARecovery` the JSON response +carries `recovery_codes_enrollment_required`. The login page calls `POST recovery-codes/enroll`, shows the +non-dismissable "Save Your Recovery Codes" modal, and navigates to `redirect_url` only after the batch is +acknowledged. An enrollment failure never blocks the sign-in: enrollment stays pending and the next sign-in is +challenged again. The profile offers the same modal through an explicit "Set up recovery codes" action, never +on page load, because every call replaces the batch. + +**Trusted devices do not skip the challenge while enrollment is required.** `MFAGateService::requiresChallenge()` +checks enrollment before device trust. This is the only way to guarantee the modal is reachable for accounts +without a password, and it stops as soon as one batch is acknowledged. + +**Passwordless generation only right after a real challenge.** `POST recovery-codes/enroll` (and `regenerate` +for a user without a password) is accepted only inside `two_factor.recovery_enrollment_window` seconds of +`MFAConstants::SESSION_KEY_COMPLETED_AT`, written solely by `verify2FA` and `verify2FARecovery` after the +session is established and bound to the user id. The principal's `auth_time` is not used: every sign-in sets +it, trusted-device sign-ins included. `enroll` is rate limited per authenticated user (`2fa.rate:enroll`, +5 per 15 minutes); the limiter cannot be keyed by the pending-challenge user because `verify2FA` clears that +state before the page calls enroll. `POST 2fa/enable` answers 412 for a user who already requires 2FA. + +**Serialized writes.** Every path that writes or deletes a user's recovery codes (generation, acknowledgment, +redemption, and later disabling 2FA / `idp:reset-2fa`) takes a `PESSIMISTIC_WRITE` lock on the `users` row +first and only then touches `user_recovery_codes`; one lock order everywhere rules out deadlocks. The lock +re-hydrates the user entity (`IUserRecoveryCodeRepository::lockUserRow()`), since a lock that leaves stale +in-memory state would let an acknowledgment decide on an outdated column. Without it two concurrent +generations under `READ_COMMITTED` leave two live batches. + +**CSRF token after sign-in.** `Auth::login()` regenerates the session id *and* the CSRF token +(`SessionGuard::updateSession()` calls `Session::regenerate(true)`), so the token the login page was rendered +with is stale once `verify2FA` succeeds. The verify responses therefore carry the current `csrf_token`, and +the page sends it with the enroll and acknowledge calls. `Authenticate` also stops redirecting JSON requests +to the session `backurl`, which would otherwise answer the first XHR after a deep-link sign-in with an HTML +page. + +## Consequences + +**Positive** + +- Every user who requires 2FA ends up with a batch they confirmed seeing, including admins who never open + their profile and accounts without a password. +- The profile no longer reports "10 of 10 remaining" for a batch nobody saved. +- No passwordless path to new codes remains for users who already require 2FA, outside a short window after a + real challenge. +- No group-membership hook is needed: the trigger is the sign-in itself. + +**Negative** + +- A user who keeps ignoring the modal receives an email OTP at every sign-in, trusted device or not, until + they acknowledge a batch. +- Closing the tab before acknowledging replaces the batch at the next sign-in: codes that were displayed but + never confirmed stop verifying. +- Acknowledgment is a user assertion: the server cannot prove the codes were saved, only that the user + confirmed the batch it generated. +- The enrollment window leaves a passwordless regeneration path open for a few minutes after each challenge + for users without a password. +- Recovery-code redemption holds the `users` row lock while it checks the unused codes (up to 10 password + hashes). + +## SDS references + +`sds/idp-mfa.md` rev 2.5 (ftn-docsnsklz PR #142): §3.1, §3.4, §3.5, §3.6, §4.10.1, §4.10.3, §4.11, §4.13, +§4.14. Rev 2.6 (ftn-docsnsklz PR #179): §3.5, §4.2, §4.10.1, §4.10.3, §4.12, §4.13, §4.14, §5.6. diff --git a/resources/js/components/recovery_code_modal.js b/resources/js/components/recovery_code_modal.js index f0a09c014..8deaa3406 100644 --- a/resources/js/components/recovery_code_modal.js +++ b/resources/js/components/recovery_code_modal.js @@ -13,7 +13,7 @@ import styles from "./recovery_codes.module.scss"; const ACK_DELAY_SECONDS = 5; -const RecoveryCodeModal = ({open, codes, email, appName, onAcknowledge}) => { +const RecoveryCodeModal = ({open, codes, email, appName, onAcknowledge, error = null, busy = false, onRetry}) => { const [secondsLeft, setSecondsLeft] = useState(ACK_DELAY_SECONDS); useEffect(() => { @@ -45,12 +45,28 @@ const RecoveryCodeModal = ({open, codes, email, appName, onAcknowledge}) => { {codes && } + {error && ( + + {error} + + )} + {error && onRetry && ( + + )} + + ); + } + if (hasPassword) { + return ( + <> + + Your recovery codes are not set up yet. Regenerate them to get a set you can save. + + {regenerateControls(true)} + + ); + } + return ( + + Your recovery codes are not set up yet. Sign out and sign in again to set them up. + + ); + } + + const showLowCodeWarning = !warningDismissed && status.remaining < lowCodeThreshold; + // A user without a password regenerates only inside the enrollment window. + const canRegenerate = hasPassword || recoveryCodesWindowOpen; + + return ( + <> + + Recovery Codes: {status.remaining} of {status.total} remaining + + { + showLowCodeWarning && + + + You're running low on recovery codes. Regenerate them to avoid getting locked out. + + + + + + } + {regenerateControls(canRegenerate)} + { + !canRegenerate && + + To regenerate your recovery codes, revoke this device in Trusted Devices, sign out and + sign in again, then regenerate right after the sign-in. + + } + + ); + }; + + return ( + <> + + {renderBody()} - + ); }; diff --git a/resources/js/components/two_factor_section.js b/resources/js/components/two_factor_section.js index ece17eff9..ce632a1bc 100644 --- a/resources/js/components/two_factor_section.js +++ b/resources/js/components/two_factor_section.js @@ -12,22 +12,25 @@ const TwoFactorSection = ({ recoveryCodesRemaining, recoveryCodesTotal, recoveryCodesLowThreshold, + recoveryCodesAcknowledged = true, + recoveryCodesEnrollmentRequired = false, + recoveryCodesEnrollmentAvailable = false, + recoveryCodesWindowOpen = false, + hasPassword = true, email, appName }) => { const [enabled, setEnabled] = useState(twoFactorEnabled); const [loading, setLoading] = useState(false); - const [remaining, setRemaining] = useState(recoveryCodesRemaining); - const [total, setTotal] = useState(recoveryCodesTotal); - const [enrollmentCodes, setEnrollmentCodes] = useState(null); + // Standing and first batch returned by "Enable": the panel is mounted with them and + // asks the user to confirm the batch before it counts as usable. + const [enrollment, setEnrollment] = useState(null); const handleEnable = () => { setLoading(true); enableTwoFactor(DEFAULT_METHOD).then(({response}) => { setLoading(false); - setRemaining(response.recovery_codes.length); - setTotal(response.recovery_codes.length); - setEnrollmentCodes(response.recovery_codes); + setEnrollment(response); setEnabled(true); }).catch((err) => { setLoading(false); @@ -54,14 +57,35 @@ const TwoFactorSection = ({ ); } + if (enrollment) { + return ( + + ); + } + return ( + appName={appName}/> ); }; diff --git a/resources/js/login/actions.js b/resources/js/login/actions.js index 2a0e32859..d79a1a8e4 100644 --- a/resources/js/login/actions.js +++ b/resources/js/login/actions.js @@ -58,6 +58,23 @@ export const verifyRecoveryCode = (recoveryCode, token) => { return postRawRequestFull(window.RECOVERY_2FA_ENDPOINT)(params, {'X-CSRF-TOKEN': token}); } +// Recovery-code enrollment, called by the login page right after a successful +// 2FA verification (SDS idp-mfa.md §4.10.1). Accept: application/json makes the +// `auth` middleware answer with JSON instead of redirecting to the session +// backurl. postRawRequestFull(): no params on the query string. +const JSON_HEADERS = (token) => ({ + 'X-CSRF-TOKEN': token, + 'Accept': 'application/json', +}); + +export const enrollRecoveryCodes = (token) => { + return postRawRequestFull(window.ENROLL_RECOVERY_CODES_ENDPOINT)({}, JSON_HEADERS(token)); +} + +export const acknowledgeRecoveryCodes = (batchId, token) => { + return postRawRequestFull(window.ACKNOWLEDGE_RECOVERY_CODES_ENDPOINT)({batch_id: batchId}, JSON_HEADERS(token)); +} + export const cancelLogin = (token) => { return postRawRequest(window.CANCEL_LOGIN_ENDPOINT)({}, {'X-CSRF-TOKEN': token}); } diff --git a/resources/js/login/login.js b/resources/js/login/login.js index f85e6894b..0605641e1 100644 --- a/resources/js/login/login.js +++ b/resources/js/login/login.js @@ -15,6 +15,8 @@ import { verify2FA, resend2FA, verifyRecoveryCode, + enrollRecoveryCodes, + acknowledgeRecoveryCodes, cancelLogin, } from "./actions"; import { emailValidator } from "../validator"; @@ -31,6 +33,7 @@ import EmailErrorActions from "./components/email_error_actions"; import ThirdPartyIdentityProviders from "./components/third_party_identity_providers"; import TwoFactorForm from "./components/two_factor_form"; import RecoveryCodeForm from "./components/recovery_code_form"; +import RecoveryCodeModal from "../components/recovery_code_modal"; import { RECOVERY_CODES_LOW_WARNING_DISMISSED_KEY, DEFAULT_RECOVERY_CODES_LOW_THRESHOLD, @@ -103,6 +106,11 @@ class LoginPage extends React.Component { // this is the only point where the SPA still controls the page (see // onVerifyRecovery()/onContinueAfterLowRecoveryCodes() below). lowRecoveryCodesWarning: null, + // Set while the recovery-code enrollment modal is up after a 2FA + // verification that reported recovery_codes_enrollment_required: + // {codes, batchId, redirectUrl, csrfToken, busy, error}. Navigation to redirectUrl + // waits until the batch is acknowledged (SDS idp-mfa.md §4.10.1). + recoveryEnrollment: null, }; if (props.authError != "" && !this.state.user_fullname) { @@ -141,6 +149,7 @@ class LoginPage extends React.Component { this.onResend2FA = this.onResend2FA.bind(this); this.onVerifyRecovery = this.onVerifyRecovery.bind(this); this.onContinueAfterLowRecoveryCodes = this.onContinueAfterLowRecoveryCodes.bind(this); + this.onAcknowledgeRecoveryCodes = this.onAcknowledgeRecoveryCodes.bind(this); this.onUseRecovery = this.onUseRecovery.bind(this); this.onBackToOtp = this.onBackToOtp.bind(this); this.resetToPasswordFlow = this.resetToPasswordFlow.bind(this); @@ -465,8 +474,13 @@ class LoginPage extends React.Component { // to it lets the browser complete any further hop natively, cross-origin // included, which this XHR never could. const { response } = payload; - window.location.href = + const redirectUrl = (response && response.redirect_url) || window.location.href; + if (response && response.recovery_codes_enrollment_required === true) { + this.startRecoveryEnrollment(redirectUrl, response.csrf_token); + return; + } + window.location.href = redirectUrl; }, (error) => { this.handleMfaError(error, "twofactor"); @@ -569,6 +583,12 @@ class LoginPage extends React.Component { const { response } = payload; const redirectUrl = (response && response.redirect_url) || window.location.href; + // The enrollment modal replaces the low-code banner: a user who has to + // enroll gets a fresh batch instead of a warning about the old one. + if (response && response.recovery_codes_enrollment_required === true) { + this.startRecoveryEnrollment(redirectUrl, response.csrf_token); + return; + } const remaining = response && response.recovery_codes_remaining; const threshold = (response && response.recovery_codes_low_threshold) ?? @@ -593,6 +613,93 @@ class LoginPage extends React.Component { ); } + /** + * A well-formed enroll response carries a non-empty recovery_codes array and + * an integer batch_id; anything else is treated as an enrollment failure. + */ + isValidBatchResponse(response) { + return ( + !!response && + Array.isArray(response.recovery_codes) && + response.recovery_codes.length > 0 && + Number.isInteger(response.batch_id) + ); + } + + /** + * Generates the user's recovery-code batch and shows it in the non-dismissable + * modal. Never blocks the sign-in: when enrollment fails for any reason (412, + * 429, network, malformed body) the page navigates to redirectUrl anyway; the + * next sign-in is challenged again while enrollment is still required + * (SDS idp-mfa.md §4.2 step 4, §4.10.1). + * + * @param {string} redirectUrl post-login destination returned by verify + * @param {string} [csrfToken] the session's CSRF token after sign-in: Auth::login() + * rotates it, so the one this page was rendered with (props.token) is stale + */ + startRecoveryEnrollment(redirectUrl, csrfToken) { + const token = csrfToken || this.props.token; + enrollRecoveryCodes(token).then( + (payload) => { + const { response } = payload; + if (!this.isValidBatchResponse(response)) { + window.location.href = redirectUrl; + return; + } + this.setState({ + ...this.state, + recoveryEnrollment: { + codes: response.recovery_codes, + batchId: response.batch_id, + redirectUrl, + csrfToken: token, + busy: false, + error: null, + }, + }); + }, + () => { + window.location.href = redirectUrl; + }, + ); + } + + /** + * "I've Saved These Codes" (and Retry): confirms the displayed batch and only + * then navigates. A 412 means the batch was replaced meanwhile: enroll again + * and show the new one. Any other failure keeps the modal open with the error. + */ + onAcknowledgeRecoveryCodes() { + const enrollment = this.state.recoveryEnrollment; + if (!enrollment || enrollment.busy) return; + + this.setState({ + ...this.state, + recoveryEnrollment: { ...enrollment, busy: true, error: null }, + }); + + acknowledgeRecoveryCodes(enrollment.batchId, enrollment.csrfToken).then( + () => { + window.location.href = enrollment.redirectUrl; + }, + (error) => { + if (error && error.status === HTTP_CODES.PRECONDITION_FAILED) { + this.startRecoveryEnrollment(enrollment.redirectUrl, enrollment.csrfToken); + return; + } + this.setState({ + ...this.state, + recoveryEnrollment: { + ...enrollment, + busy: false, + error: + "We could not confirm your recovery codes. Please try again.", + }, + }); + }, + ); + } + onContinueAfterLowRecoveryCodes() { sessionStorage.setItem(RECOVERY_CODES_LOW_WARNING_DISMISSED_KEY, "1"); window.location.href = this.state.lowRecoveryCodesWarning.redirectUrl; @@ -838,6 +945,18 @@ class LoginPage extends React.Component { className={styles.main_container} > + {this.state.recoveryEnrollment && ( + + )} {this.state.showInfoBanner && ( )} diff --git a/resources/js/profile/actions.js b/resources/js/profile/actions.js index 0c9ea422b..e853e7d8d 100644 --- a/resources/js/profile/actions.js +++ b/resources/js/profile/actions.js @@ -87,9 +87,25 @@ export const revokeAllTokens = async () => { return deleteRawRequest(window.REVOKE_ALL_TOKENS_ENDPOINT)({'X-CSRF-TOKEN': window.CSFR_TOKEN}); } -export const regenerateRecoveryCodes = async (currentPassword) => { - const params = {current_password: currentPassword}; - return postRawRequestFull(window.REGENERATE_RECOVERY_CODES_ENDPOINT)(params, {'X-CSRF-TOKEN': window.CSFR_TOKEN}); +// Accept: application/json: the `auth` middleware must answer these XHRs with JSON +// instead of redirecting to a session backurl. +const jsonHeaders = () => ({'X-CSRF-TOKEN': window.CSFR_TOKEN, 'Accept': 'application/json'}); + +// currentPassword is optional: a user without a password (social-only account) +// regenerates inside the recovery enrollment window instead of confirming one. +export const regenerateRecoveryCodes = async (currentPassword = null) => { + const params = currentPassword ? {current_password: currentPassword} : {}; + return postRawRequestFull(window.REGENERATE_RECOVERY_CODES_ENDPOINT)(params, jsonHeaders()); +} + +// Generates a batch without the password, right after a 2FA challenge. +export const enrollRecoveryCodes = async () => { + return postRawRequestFull(window.ENROLL_RECOVERY_CODES_ENDPOINT)({}, jsonHeaders()); +} + +// "I've Saved These Codes": confirms the exact batch that was displayed. +export const acknowledgeRecoveryCodes = async (batchId) => { + return postRawRequestFull(window.ACKNOWLEDGE_RECOVERY_CODES_ENDPOINT)({batch_id: batchId}, jsonHeaders()); } export const enableTwoFactor = async (method) => { diff --git a/resources/js/profile/profile.js b/resources/js/profile/profile.js index acfdc0627..600bfbe43 100644 --- a/resources/js/profile/profile.js +++ b/resources/js/profile/profile.js @@ -49,7 +49,11 @@ const ProfilePage = ({ twoFactorEnabled, recoveryCodesRemaining, recoveryCodesTotal, - recoveryCodesLowThreshold + recoveryCodesLowThreshold, + recoveryCodesAcknowledged, + recoveryCodesEnrollmentRequired, + recoveryCodesEnrollmentAvailable, + recoveryCodesWindowOpen }) => { const [pic, setPic] = useState(null); const [loading, setLoading] = useState(false); @@ -791,6 +795,11 @@ const ProfilePage = ({ recoveryCodesRemaining={recoveryCodesRemaining} recoveryCodesTotal={recoveryCodesTotal} recoveryCodesLowThreshold={recoveryCodesLowThreshold} + recoveryCodesAcknowledged={recoveryCodesAcknowledged} + recoveryCodesEnrollmentRequired={recoveryCodesEnrollmentRequired} + recoveryCodesEnrollmentAvailable={recoveryCodesEnrollmentAvailable} + recoveryCodesWindowOpen={recoveryCodesWindowOpen} + hasPassword={initialValues.has_password_set} email={initialValues.email} appName={appName}/> diff --git a/resources/views/auth/login.blade.php b/resources/views/auth/login.blade.php index 844f2e025..706e901e9 100644 --- a/resources/views/auth/login.blade.php +++ b/resources/views/auth/login.blade.php @@ -38,6 +38,8 @@ resend2faAction: '{{ URL::action("UserController@resend2FA") }}', cancelLogin: '{{ URL::action("UserController@resetLogin") }}', recovery2faAction: '{{ URL::action("UserController@verify2FARecovery") }}', + enrollRecoveryCodesAction: '{{ URL::action("Api\\UserApiController@enrollRecoveryCodes") }}', + acknowledgeRecoveryCodesAction: '{{ URL::action("Api\\UserApiController@acknowledgeRecoveryCodes") }}', mfaMethod: '{{ Session::has("mfa_method") ? Session::get("mfa_method") : "email_otp" }}', authError: authError, captchaPublicKey: '{{ Config::get("services.turnstile.key") }}', @@ -106,6 +108,8 @@ window.RESEND_2FA_ENDPOINT = config.resend2faAction; window.CANCEL_LOGIN_ENDPOINT = config.cancelLogin; window.RECOVERY_2FA_ENDPOINT = config.recovery2faAction; + window.ENROLL_RECOVERY_CODES_ENDPOINT = config.enrollRecoveryCodesAction; + window.ACKNOWLEDGE_RECOVERY_CODES_ENDPOINT = config.acknowledgeRecoveryCodesAction; {!! script_to('assets/login.js') !!} @append \ No newline at end of file diff --git a/resources/views/profile.blade.php b/resources/views/profile.blade.php index 672081ba0..294dcedcc 100644 --- a/resources/views/profile.blade.php +++ b/resources/views/profile.blade.php @@ -107,7 +107,11 @@ twoFactorEnabled: {{ $two_factor_enabled ? 'true' : 'false' }}, recoveryCodesRemaining: {{ (int) $recovery_codes_remaining }}, recoveryCodesTotal: {{ (int) $recovery_codes_total }}, - recoveryCodesLowThreshold: {{ (int) $recovery_codes_low_threshold }} + recoveryCodesLowThreshold: {{ (int) $recovery_codes_low_threshold }}, + recoveryCodesAcknowledged: {{ $recovery_codes_acknowledged ? 'true' : 'false' }}, + recoveryCodesEnrollmentRequired: {{ $recovery_codes_enrollment_required ? 'true' : 'false' }}, + recoveryCodesEnrollmentAvailable: {{ $recovery_codes_enrollment_available ? 'true' : 'false' }}, + recoveryCodesWindowOpen: {{ $recovery_codes_window_open ? 'true' : 'false' }} } window.GET_USER_ACTIONS_ENDPOINT = '{{URL::action("Api\UserActionApiController@getActionsByCurrentUser")}}'; @@ -118,6 +122,8 @@ window.SAVE_PIC_ENDPOINT = '{!!URL::action("Api\UserApiController@updateMyPic")!!}'; window.REGENERATE_RECOVERY_CODES_ENDPOINT = '{!!URL::action("Api\UserApiController@regenerateRecoveryCodes")!!}'; window.ENABLE_TWO_FACTOR_ENDPOINT = '{!!URL::action("Api\UserApiController@enableTwoFactor")!!}'; + window.ENROLL_RECOVERY_CODES_ENDPOINT = '{!!URL::action("Api\UserApiController@enrollRecoveryCodes")!!}'; + window.ACKNOWLEDGE_RECOVERY_CODES_ENDPOINT = '{!!URL::action("Api\UserApiController@acknowledgeRecoveryCodes")!!}'; window.CSFR_TOKEN = document.head.querySelector('meta[name="csrf-token"]').content; {!! script_to('assets/profile.js') !!} diff --git a/routes/web.php b/routes/web.php index d0fa0aef1..281351c29 100644 --- a/routes/web.php +++ b/routes/web.php @@ -199,6 +199,8 @@ Route::put('pic', "UserApiController@updateMyPic"); Route::get('actions', "UserActionApiController@getActionsByCurrentUser"); Route::post('recovery-codes/regenerate', ['middleware' => ['csrf'], 'uses' => "UserApiController@regenerateRecoveryCodes"]); + Route::post('recovery-codes/enroll', ['middleware' => ['csrf', '2fa.rate:enroll'], 'uses' => "UserApiController@enrollRecoveryCodes"]); + Route::post('recovery-codes/acknowledge', ['middleware' => ['csrf'], 'uses' => "UserApiController@acknowledgeRecoveryCodes"]); Route::post('2fa/enable', ['middleware' => ['csrf'], 'uses' => "UserApiController@enableTwoFactor"]); }); diff --git a/tests/RecoveryCodeConcurrencyTest.php b/tests/RecoveryCodeConcurrencyTest.php new file mode 100644 index 000000000..d30dddc82 --- /dev/null +++ b/tests/RecoveryCodeConcurrencyTest.php @@ -0,0 +1,241 @@ +holder !== null) { + try { + if ($this->holder->transactionLevel() > 0) { + $this->holder->rollBack(); + } + } finally { + DB::purge('recovery_lock_holder'); + $this->holder = null; + } + } + parent::tearDown(); + } + + public function testLockUserRowBlocksAConcurrentConnection(): void + { + $admin = $this->admin(); + /** @var IUserRecoveryCodeRepository $repo */ + $repo = App::make(IUserRecoveryCodeRepository::class); + + $primary = EntityManager::getConnection(); + $primary->beginTransaction(); + try { + $repo->lockUserRow($admin); + + $secondary = $this->secondConnection('recovery_lock_probe'); + $secondary->statement('SET SESSION innodb_lock_wait_timeout = 1'); + $secondary->beginTransaction(); + try { + $secondary->selectOne('SELECT id FROM users WHERE id = ? FOR UPDATE', [$admin->getId()]); + $this->fail('a second connection must not lock a users row already held by lockUserRow()'); + } catch (\Illuminate\Database\QueryException $ex) { + $this->assertStringContainsStringIgnoringCase('lock wait timeout', $ex->getMessage()); + } finally { + $secondary->rollBack(); + DB::purge('recovery_lock_probe'); + } + } finally { + $primary->rollBack(); + } + } + + public function testLockUserRowRefreshesStaleState(): void + { + $admin = $this->admin(); + $service = App::make(IRecoveryCodeService::class); + $batch = $service->generateRecoveryCodes($admin); + $this->assertTrue($service->acknowledgeRecoveryCodes($admin, $batch->getBatchId())); + $this->assertNotNull($admin->getRecoveryCodesAcknowledgedAt()); + + // Another connection clears the column behind the managed entity's back. + $other = $this->secondConnection('recovery_lock_stale'); + $other->update('UPDATE users SET recovery_codes_acknowledged_at = NULL WHERE id = ?', [$admin->getId()]); + DB::purge('recovery_lock_stale'); + + $primary = EntityManager::getConnection(); + $primary->beginTransaction(); + try { + App::make(IUserRecoveryCodeRepository::class)->lockUserRow($admin); + $this->assertNull( + $admin->getRecoveryCodesAcknowledgedAt(), + 'the lock must re-hydrate the entity, otherwise acknowledge would decide on stale state' + ); + } finally { + $primary->rollBack(); + } + } + + public function testTwoConcurrentGenerationsLeaveExactlyOneBatch(): void + { + $admin = $this->admin(); + $this->holdUsersRowLock($admin); + + $workers = [ + $this->startWorker('generate', $admin->getId()), + $this->startWorker('generate', $admin->getId()), + ]; + // Let both workers reach (and block on) the users-row lock. + sleep(3); + $this->holder->commit(); + + $results = array_map(fn($w) => $this->finishWorker($w), $workers); + + $this->assertTrue($results[0]['ok'], json_encode($results[0])); + $this->assertTrue($results[1]['ok'], json_encode($results[1])); + $this->assertNotSame($results[0]['batch_id'], $results[1]['batch_id']); + + $rows = $this->codeRows($admin->getId()); + $this->assertCount( + (int)config('auth.recovery_codes.count', 10), + $rows, + 'two concurrent generations must not leave two live batches' + ); + $this->assertSame( + max($results[0]['batch_id'], $results[1]['batch_id']), + min(array_column($rows, 'id')), + 'the surviving batch is the one generated last' + ); + } + + public function testRedemptionOfACodeReplacedDuringRegenerationIsRejected(): void + { + $admin = $this->admin(); + $plain = 'OLDBATCHCODE'; + $this->insertCode($admin->getId(), $plain); + + // The "regeneration" holds the users-row lock; the redemption starts meanwhile + // and must wait for it, then read the codes AFTER the replacement. + $this->holdUsersRowLock($admin); + $worker = $this->startWorker('redeem', $admin->getId(), $plain); + sleep(3); + + $this->holder->delete('DELETE FROM user_recovery_codes WHERE user_id = ?', [$admin->getId()]); + $this->holder->insert( + 'INSERT INTO user_recovery_codes (created_at, user_id, code_hash) VALUES (NOW(), ?, ?)', + [$admin->getId(), Hash::make('NEWBATCHCODE')] + ); + $this->holder->commit(); + + $result = $this->finishWorker($worker); + + $this->assertFalse($result['ok'], 'a code of the replaced batch must not be redeemable'); + $this->assertStringContainsString('Invalid recovery code', $result['error']); + $this->assertCount(1, $this->codeRows($admin->getId())); + } + + // --------------------------------------------------------------------- + + private function admin(): User + { + $admin = EntityManager::getRepository(User::class)->getByEmailOrName(self::ADMIN_EMAIL); + $this->assertInstanceOf(User::class, $admin); + return $admin; + } + + private function secondConnection(string $name): Connection + { + Config::set("database.connections.{$name}", Config::get('database.connections.openstackid')); + return DB::connection($name); + } + + /** Takes the users-row lock on a second physical connection, in an open transaction. */ + private function holdUsersRowLock(User $user): void + { + $this->holder = $this->secondConnection('recovery_lock_holder'); + $this->holder->beginTransaction(); + $this->holder->selectOne('SELECT id FROM users WHERE id = ? FOR UPDATE', [$user->getId()]); + } + + private function insertCode(int $userId, string $plain): void + { + $conn = $this->secondConnection('recovery_lock_seed'); + $conn->insert( + 'INSERT INTO user_recovery_codes (created_at, user_id, code_hash) VALUES (NOW(), ?, ?)', + [$userId, Hash::make($plain)] + ); + DB::purge('recovery_lock_seed'); + } + + /** @return array */ + private function codeRows(int $userId): array + { + $conn = $this->secondConnection('recovery_lock_read'); + $rows = $conn->select('SELECT id FROM user_recovery_codes WHERE user_id = ? ORDER BY id', [$userId]); + DB::purge('recovery_lock_read'); + return array_map(fn($r) => ['id' => (int)$r->id], $rows); + } + + /** @return array{proc:resource,pipes:array} */ + private function startWorker(string $action, int $userId, ?string $code = null): array + { + $cmd = [PHP_BINARY, base_path('tests/support/RecoveryCodeWorker.php'), $action, (string)$userId]; + if ($code !== null) { + $cmd[] = $code; + } + $proc = proc_open( + $cmd, + [1 => ['pipe', 'w'], 2 => ['pipe', 'w']], + $pipes, + base_path(), + array_merge(getenv(), ['APP_ENV' => 'testing', 'XDEBUG_MODE' => 'off']) + ); + $this->assertIsResource($proc, 'could not start the worker process'); + return ['proc' => $proc, 'pipes' => $pipes]; + } + + private function finishWorker(array $worker): array + { + $stdout = stream_get_contents($worker['pipes'][1]); + $stderr = stream_get_contents($worker['pipes'][2]); + fclose($worker['pipes'][1]); + fclose($worker['pipes'][2]); + proc_close($worker['proc']); + + $lines = array_values(array_filter(array_map('trim', explode(PHP_EOL, $stdout)))); + $decoded = $lines ? json_decode(end($lines), true) : null; + $this->assertIsArray($decoded, "worker produced no result. stdout: {$stdout} stderr: {$stderr}"); + return $decoded; + } +} diff --git a/tests/RecoveryCodeEnrollmentServiceTest.php b/tests/RecoveryCodeEnrollmentServiceTest.php new file mode 100644 index 000000000..8b972a37e --- /dev/null +++ b/tests/RecoveryCodeEnrollmentServiceTest.php @@ -0,0 +1,271 @@ +service = App::make(IRecoveryCodeService::class); + } + + // ------------------------------------------------------------------ + // requiresRecoveryCodeEnrollment + // ------------------------------------------------------------------ + + public function testEnforcedUserWithNoCodesRequiresEnrollment(): void + { + $this->assertTrue($this->service->requiresRecoveryCodeEnrollment($this->admin())); + } + + public function testEnforcedUserWithAnUnacknowledgedBatchRequiresEnrollment(): void + { + $admin = $this->admin(); + $this->service->generateRecoveryCodes($admin); + + $this->assertTrue($this->service->requiresRecoveryCodeEnrollment($admin)); + } + + public function testEnforcedUserWithAnAcknowledgedBatchDoesNotRequireEnrollment(): void + { + $admin = $this->admin(); + $batch = $this->service->generateRecoveryCodes($admin); + $this->assertTrue($this->service->acknowledgeRecoveryCodes($admin, $batch->getBatchId())); + + $this->assertFalse($this->service->requiresRecoveryCodeEnrollment($admin)); + } + + public function testAcknowledgedBatchThatWasFullyConsumedRequiresEnrollmentAgain(): void + { + $admin = $this->admin(); + $batch = $this->service->generateRecoveryCodes($admin); + $this->service->acknowledgeRecoveryCodes($admin, $batch->getBatchId()); + DB::table('user_recovery_codes')->where('user_id', $admin->getId())->update(['used_at' => now()]); + EntityManager::clear(); + + $this->assertTrue($this->service->requiresRecoveryCodeEnrollment($this->admin())); + } + + public function testPlainUserWithoutTwoFactorNeverRequiresEnrollment(): void + { + $this->assertFalse($this->service->requiresRecoveryCodeEnrollment($this->plainUser())); + } + + public function testSelfEnrolledUserWithNoCodesRequiresEnrollment(): void + { + $user = $this->plainUser(); + $user->setTwoFactorEnabled(true); + + $this->assertTrue($this->service->requiresRecoveryCodeEnrollment($user)); + } + + public function testKillSwitchTurnsEnrollmentOff(): void + { + config(['two_factor.enabled' => false]); + + $this->assertFalse($this->service->requiresRecoveryCodeEnrollment($this->admin())); + } + + // ------------------------------------------------------------------ + // isWithinEnrollmentWindow + // ------------------------------------------------------------------ + + public function testWindowIsClosedWithoutMarker(): void + { + $this->assertFalse($this->service->isWithinEnrollmentWindow($this->admin())); + } + + public function testWindowIsOpenRightAfterAChallenge(): void + { + $admin = $this->admin(); + $this->putMarker($admin->getId(), time()); + + $this->assertTrue($this->service->isWithinEnrollmentWindow($admin)); + } + + public function testWindowClosesAfterTheConfiguredSeconds(): void + { + $admin = $this->admin(); + config(['two_factor.recovery_enrollment_window' => 120]); + + $this->putMarker($admin->getId(), time() - 100); + $this->assertTrue($this->service->isWithinEnrollmentWindow($admin)); + + $this->putMarker($admin->getId(), time() - 130); + $this->assertFalse($this->service->isWithinEnrollmentWindow($admin)); + } + + public function testWindowIsBoundToTheMarkersUser(): void + { + $admin = $this->admin(); + $this->putMarker($admin->getId() + 1000, time()); + + $this->assertFalse($this->service->isWithinEnrollmentWindow($admin)); + } + + public function testWindowRejectsAMalformedOrFutureMarker(): void + { + $admin = $this->admin(); + + Session::put(MFAConstants::SESSION_KEY_COMPLETED_AT, 'garbage'); + $this->assertFalse($this->service->isWithinEnrollmentWindow($admin)); + + Session::put(MFAConstants::SESSION_KEY_COMPLETED_AT, ['user_id' => $admin->getId()]); + $this->assertFalse($this->service->isWithinEnrollmentWindow($admin)); + + $this->putMarker($admin->getId(), time() + 3600); + $this->assertFalse($this->service->isWithinEnrollmentWindow($admin), 'a marker from the future must not open the window'); + } + + // ------------------------------------------------------------------ + // acknowledgment / batch id / status + // ------------------------------------------------------------------ + + public function testAcknowledgeWithoutAnyBatchIsRejected(): void + { + $this->assertFalse($this->service->acknowledgeRecoveryCodes($this->admin(), 1)); + } + + public function testGenerationReturnsTheMinIdOfTheBatchAndClearsAcknowledgment(): void + { + $admin = $this->admin(); + $first = $this->service->generateRecoveryCodes($admin); + $this->service->acknowledgeRecoveryCodes($admin, $first->getBatchId()); + + $second = $this->service->generateRecoveryCodes($admin); + + $this->assertCount((int)config('auth.recovery_codes.count', 10), $second->getCodes()); + $this->assertSame($second->getBatchId(), $this->service->getCurrentBatchId($admin)); + $this->assertSame( + $second->getBatchId(), + (int)DB::table('user_recovery_codes')->where('user_id', $admin->getId())->min('id') + ); + $this->assertNull($admin->getRecoveryCodesAcknowledgedAt()); + $this->assertFalse($this->service->acknowledgeRecoveryCodes($admin, $first->getBatchId()), 'the replaced batch can no longer be acknowledged'); + } + + public function testStatusExposesAcknowledgmentAndEnrollmentKeys(): void + { + $admin = $this->admin(); + + $before = $this->service->getStatus($admin)->toArray(); + $this->assertFalse($before['recovery_codes_acknowledged']); + $this->assertTrue($before['recovery_codes_enrollment_required']); + $this->assertSame(0, $before['recovery_codes_remaining']); + + $batch = $this->service->generateRecoveryCodes($admin); + $this->service->acknowledgeRecoveryCodes($admin, $batch->getBatchId()); + + $after = $this->service->getStatus($admin)->toArray(); + $this->assertTrue($after['recovery_codes_acknowledged']); + $this->assertFalse($after['recovery_codes_enrollment_required']); + $this->assertSame((int)config('auth.recovery_codes.count', 10), $after['recovery_codes_remaining']); + } + + // ------------------------------------------------------------------ + // migration backfill (SDS §3.6) + // ------------------------------------------------------------------ + + public function testBackfillAcknowledgesUsersWithUnusedCodesOnly(): void + { + $ids = DB::table('users')->orderBy('id')->limit(4)->pluck('id')->all(); + [$withUnused, $onlyUsed, $none, $alreadyAck] = $ids; + + DB::table('users')->whereIn('id', $ids)->update(['recovery_codes_acknowledged_at' => null]); + + // withUnused: used + unused rows, latest created_at wins. + $this->insertCodeRow($withUnused, '2026-01-01 10:00:00', true); + $this->insertCodeRow($withUnused, '2026-02-02 11:30:00', false); + $this->insertCodeRow($withUnused, '2026-02-02 11:00:00', false); + // onlyUsed: nothing usable left, stays NULL. + $this->insertCodeRow($onlyUsed, '2026-03-03 09:00:00', true); + // alreadyAck: an existing value must not be overwritten. + $this->insertCodeRow($alreadyAck, '2026-04-04 09:00:00', false); + DB::table('users')->where('id', $alreadyAck)->update(['recovery_codes_acknowledged_at' => '2026-05-05 05:05:05']); + + $connection = EntityManager::getConnection(); + $migration = new Version20261105120000($connection, new NullLogger()); + $migration->postUp($connection->createSchemaManager()->introspectSchema()); + + $ack = fn(int $id) => DB::table('users')->where('id', $id)->value('recovery_codes_acknowledged_at'); + $this->assertSame('2026-02-02 11:30:00', $ack($withUnused), 'MAX(created_at) of the user rows'); + $this->assertNull($ack($onlyUsed), 'only used codes: stays NULL'); + $this->assertNull($ack($none), 'no codes: stays NULL'); + $this->assertSame('2026-05-05 05:05:05', $ack($alreadyAck), 're-running must not overwrite an acknowledgment'); + } + + // ------------------------------------------------------------------ + + private function admin(): User + { + $admin = EntityManager::getRepository(User::class)->getByEmailOrName('sebastian@tipit.net'); + $this->assertInstanceOf(User::class, $admin); + return $admin; + } + + private function plainUser(): User + { + $user = UserFactory::build([ + 'first_name' => 'Plain', + 'last_name' => 'User', + 'email' => 'plain.' . uniqid() . '@test.invalid', + 'password' => '1Qaz2wsx!', + 'password_enc' => AuthHelper::AlgSHA1_V2_4, + 'active' => true, + 'email_verified' => true, + 'identifier' => 'plain.' . uniqid(), + ]); + EntityManager::persist($user); + EntityManager::flush(); + return $user; + } + + private function putMarker(int $userId, int $at): void + { + Session::put(MFAConstants::SESSION_KEY_COMPLETED_AT, ['user_id' => $userId, 'at' => $at]); + } + + private function insertCodeRow(int $userId, string $createdAt, bool $used): void + { + DB::table('user_recovery_codes')->insert([ + 'user_id' => $userId, + 'code_hash' => Hash::make(uniqid('', true)), + 'created_at' => $createdAt, + 'used_at' => $used ? $createdAt : null, + ]); + } +} diff --git a/tests/RecoveryCodeRegenerationTest.php b/tests/RecoveryCodeRegenerationTest.php index cec67de85..a3f521495 100644 --- a/tests/RecoveryCodeRegenerationTest.php +++ b/tests/RecoveryCodeRegenerationTest.php @@ -12,12 +12,16 @@ * limitations under the License. **/ +use App\libs\Auth\Factories\UserFactory; use App\libs\Auth\Models\TwoFactorAuditLog; use App\libs\Auth\Models\UserRecoveryCode; +use Auth\AuthHelper; +use Auth\MFAConstants; use Auth\User; use Illuminate\Support\Facades\Hash; use Illuminate\Support\Facades\Session; use LaravelDoctrine\ORM\Facades\EntityManager; +use App\Services\Auth\IRecoveryCodeService; use Strategies\MFA\MFAChallengeStrategyFactory; use Utils\Services\IAuthService; @@ -105,24 +109,50 @@ public function testRegenerateLogsAuditEvent(): void public function testEnableTwoFactorGeneratesRecoveryCodes(): void { - $admin = $this->admin(); + // A non-enforced user: group-enforced admins are rejected (see + // testEnableTwoFactorRejectsGroupEnforcedAdmin). + $user = $this->createPlainUser(); + $this->be($user); $response = $this->enableTwoFactor('email_otp'); $this->assertResponseStatus(200); $payload = json_decode($response->getContent(), true); $this->assertArrayHasKey('recovery_codes', $payload); + $this->assertIsInt($payload['batch_id']); + $this->assertFalse($payload['recovery_codes_acknowledged']); + $this->assertTrue($payload['recovery_codes_enrollment_required']); $expectedCount = (int)config('auth.recovery_codes.count', 10); $this->assertCount($expectedCount, $payload['recovery_codes']); + $this->assertSame($expectedCount, $payload['recovery_codes_remaining']); EntityManager::clear(); - $reloaded = EntityManager::getRepository(User::class)->find($admin->getId()); + $reloaded = EntityManager::getRepository(User::class)->find($user->getId()); $this->assertTrue($reloaded->isTwoFactorEnabled(), '2FA must be enabled on the user after enrollment'); $remaining = EntityManager::getRepository(UserRecoveryCode::class) - ->findBy(['user' => $admin->getId(), 'used_at' => null]); + ->findBy(['user' => $user->getId(), 'used_at' => null]); $this->assertCount($expectedCount, $remaining); + $this->assertSame($payload['batch_id'], min(array_map(fn($c) => $c->getId(), $remaining))); + } + + public function testEnableTwoFactorRejectsGroupEnforcedAdmin(): void + { + $admin = $this->admin(); + + $this->enableTwoFactor('email_otp'); + + $this->assertResponseStatus(412); + + EntityManager::clear(); + $reloaded = EntityManager::getRepository(User::class)->find($admin->getId()); + $this->assertFalse($reloaded->isTwoFactorEnabled(), 'the stored flag must stay untouched for an enforced admin'); + $this->assertCount( + 0, + EntityManager::getRepository(UserRecoveryCode::class)->findBy(['user' => $admin->getId()]), + 'no codes may be minted for an enforced admin' + ); } public function testDisplayedRecoveryCodeRedeemsThroughVerifyRecoveryCode(): void @@ -161,23 +191,28 @@ public function testDisplayedRecoveryCodeRedeemsThroughVerifyRecoveryCode(): voi public function testEnableTwoFactorRejectsWhenAlreadyEnabled(): void { + $user = $this->createPlainUser(); + $this->be($user); + $this->enableTwoFactor('email_otp'); $this->assertResponseStatus(200); - $response = $this->enableTwoFactor('email_otp'); + $this->enableTwoFactor('email_otp'); $this->assertResponseStatus(412); EntityManager::clear(); - $admin = $this->admin(); - $this->assertTrue($admin->isTwoFactorEnabled(), '2FA must remain enabled after the rejected second call'); + $reloaded = EntityManager::getRepository(User::class)->find($user->getId()); + $this->assertTrue($reloaded->isTwoFactorEnabled(), '2FA must remain enabled after the rejected second call'); } public function testEnableTwoFactorRejectsUnavailableMethod(): void { // sms_otp is a stub in Phase I (isPhoneNumberVerified() is hardcoded false), - // so enable2FA() must reject it regardless of the requesting user. - $response = $this->enableTwoFactor('sms_otp'); + // so enable2FA() must reject it for a user that is allowed to self-enroll. + $this->be($this->createPlainUser()); + + $this->enableTwoFactor('sms_otp'); $this->assertResponseStatus(412); } @@ -191,14 +226,93 @@ public function testEnableTwoFactorRequiresMethod(): void public function testEnableTwoFactorLogsEnrollmentAuditEvent(): void { - $admin = $this->admin(); + $user = $this->createPlainUser(); + $this->be($user); - $this->enableTwoFactor('email_otp'); + $response = $this->enableTwoFactor('email_otp'); + $payload = json_decode($response->getContent(), true); EntityManager::clear(); - $entries = EntityManager::getRepository(TwoFactorAuditLog::class) - ->findBy(['user' => $admin->getId(), 'event_type' => TwoFactorAuditLog::EventEnrollmentChanged]); + $repo = EntityManager::getRepository(TwoFactorAuditLog::class); + $entries = $repo->findBy(['user' => $user->getId(), 'event_type' => TwoFactorAuditLog::EventEnrollmentChanged]); $this->assertNotEmpty($entries, 'an enrollment_changed audit entry must be recorded'); + + $generated = $repo->findBy(['user' => $user->getId(), 'event_type' => TwoFactorAuditLog::EventRecoveryCodesGenerated]); + $this->assertCount(1, $generated); + $this->assertSame(['batch_id' => $payload['batch_id']], $generated[0]->getMetadata()); + } + + public function testRegenerateReturnsBatchIdAndResetsAcknowledgment(): void + { + $admin = $this->admin(); + $service = app(IRecoveryCodeService::class); + $first = $service->generateRecoveryCodes($admin); + $this->assertTrue($service->acknowledgeRecoveryCodes($admin, $first->getBatchId())); + + $response = $this->regenerate(self::SEED_PASSWORD); + + $this->assertResponseStatus(200); + $payload = json_decode($response->getContent(), true); + $this->assertIsInt($payload['batch_id']); + $this->assertGreaterThan($first->getBatchId(), $payload['batch_id']); + $this->assertFalse($payload['recovery_codes_acknowledged'], 'a new batch is unacknowledged until the user confirms it'); + + EntityManager::clear(); + $reloaded = EntityManager::getRepository(User::class)->find($admin->getId()); + $this->assertNull($reloaded->getRecoveryCodesAcknowledgedAt()); + } + + public function testRegenerateWithoutPasswordIsRejectedForUserWithPassword(): void + { + $this->action('POST', 'Api\\UserApiController@regenerateRecoveryCodes', [], [], [], []); + + $this->assertResponseStatus(412); + } + + public function testRegenerateForUserWithoutPasswordIsAcceptedInsideTheWindow(): void + { + $user = $this->createPlainUser(false); + $this->be($user); + Session::put(MFAConstants::SESSION_KEY_COMPLETED_AT, ['user_id' => $user->getId(), 'at' => time() - 30]); + + $response = $this->action('POST', 'Api\\UserApiController@regenerateRecoveryCodes', [], [], [], []); + + $this->assertResponseStatus(200); + $payload = json_decode($response->getContent(), true); + $this->assertCount((int)config('auth.recovery_codes.count', 10), $payload['recovery_codes']); + } + + public function testRegenerateForUserWithoutPasswordIsRejectedOutsideTheWindow(): void + { + $user = $this->createPlainUser(false); + $this->be($user); + $window = (int)config('two_factor.recovery_enrollment_window', 600); + Session::put(MFAConstants::SESSION_KEY_COMPLETED_AT, ['user_id' => $user->getId(), 'at' => time() - $window - 5]); + + $this->action('POST', 'Api\\UserApiController@regenerateRecoveryCodes', [], [], [], []); + + $this->assertResponseStatus(412); + $this->assertCount(0, EntityManager::getRepository(UserRecoveryCode::class)->findBy(['user' => $user->getId()])); + } + + public function testRegenerateForUserWithoutPasswordIsRejectedWithAnotherUsersMarker(): void + { + $user = $this->createPlainUser(false); + $this->be($user); + Session::put(MFAConstants::SESSION_KEY_COMPLETED_AT, ['user_id' => $user->getId() + 1000, 'at' => time()]); + + $this->action('POST', 'Api\\UserApiController@regenerateRecoveryCodes', [], [], [], []); + + $this->assertResponseStatus(412); + } + + public function testRegenerateForUserWithoutPasswordIsRejectedWithoutMarker(): void + { + $this->be($this->createPlainUser(false)); + + $this->action('POST', 'Api\\UserApiController@regenerateRecoveryCodes', [], [], [], []); + + $this->assertResponseStatus(412); } private function enableTwoFactor(string $method) @@ -222,6 +336,26 @@ private function regenerate(string $password) ], [], [], []); } + private function createPlainUser(bool $withPassword = true): User + { + $payload = [ + 'first_name' => 'Plain', + 'last_name' => 'User', + 'email' => 'plain.' . uniqid() . '@test.invalid', + 'active' => true, + 'email_verified' => true, + 'identifier' => 'plain.' . uniqid(), + ]; + if ($withPassword) { + $payload['password'] = self::SEED_PASSWORD; + $payload['password_enc'] = AuthHelper::AlgSHA1_V2_4; + } + $user = UserFactory::build($payload); + EntityManager::persist($user); + EntityManager::flush(); + return $user; + } + private function createRecoveryCode(User $user, string $plain, bool $used): int { $code = new UserRecoveryCode(); diff --git a/tests/TwoFactorLoginFlowTest.php b/tests/TwoFactorLoginFlowTest.php index 5bd612e80..54294be05 100644 --- a/tests/TwoFactorLoginFlowTest.php +++ b/tests/TwoFactorLoginFlowTest.php @@ -18,6 +18,7 @@ use App\libs\Auth\Models\UserTrustedDevice; use App\Mail\OAuth2PasswordlessOTPMail; use App\Services\Auth\IDeviceTrustService; +use App\Services\Auth\IRecoveryCodeService; use App\Services\Auth\ITwoFactorAuditService; use App\Services\Auth\ITwoFactorRateLimitService; use Auth\AuthHelper; @@ -79,7 +80,7 @@ private function flushRateLimitCounters(): void if ($admin) { $userId = $admin->getId(); $prefix = ITwoFactorRateLimitService::RATE_LIMIT_CACHE_KEY_PREFIX; - foreach (['verify', 'recovery', 'resend'] as $action) { + foreach (['verify', 'recovery', 'resend', 'enroll'] as $action) { Cache::forget("{$prefix}{$action}:{$userId}"); // RateLimiter::hit() also writes a companion ":timer" key holding // the window's reset timestamp - must be cleared too, or a stale @@ -995,6 +996,9 @@ public function testTrustDeviceEnrollmentPersistsRecord(): void public function testTrustedDeviceCookieBypassesMFA(): void { $admin = $this->user(self::ADMIN_EMAIL); + // A trusted device skips the challenge only for a user who already holds an + // acknowledged batch of recovery codes (SDS idp-mfa.md §4.2 step 4). + $this->giveUserAcknowledgedBatch($admin); /** @var IDeviceTrustService $deviceTrust */ $deviceTrust = App::make(IDeviceTrustService::class); @@ -1011,6 +1015,333 @@ public function testTrustedDeviceCookieBypassesMFA(): void $this->assertTrue(Auth::check(), 'a valid trusted-device cookie must bypass MFA'); } + public function testTrustedDeviceCookieDoesNotBypassMFAWhenUserHasNoRecoveryCodes(): void + { + $admin = $this->user(self::ADMIN_EMAIL); + $this->assertSame(0, App::make(IRecoveryCodeService::class)->countUnusedRecoveryCodes($admin)); + + $rawToken = App::make(IDeviceTrustService::class)->trustDevice($admin, 'Mozilla/5.0 (test)', '127.0.0.1'); + + $this->postLogin( + self::ADMIN_EMAIL, + self::SEED_PASSWORD, + [Config::get('two_factor.cookie_name') => $rawToken] + ); + + $this->assertFalse(Auth::check(), 'enrollment is required: the trusted device must not skip the challenge'); + $this->assertSame('2fa', Session::get('flow'), 'the 2FA screen must be shown'); + } + + public function testTrustedDeviceCookieDoesNotBypassMFAWhenBatchIsUnacknowledged(): void + { + $admin = $this->user(self::ADMIN_EMAIL); + // Generated but never confirmed with "I've Saved These Codes". + App::make(IRecoveryCodeService::class)->generateRecoveryCodes($admin); + + $rawToken = App::make(IDeviceTrustService::class)->trustDevice($admin, 'Mozilla/5.0 (test)', '127.0.0.1'); + + $this->postLogin( + self::ADMIN_EMAIL, + self::SEED_PASSWORD, + [Config::get('two_factor.cookie_name') => $rawToken] + ); + + $this->assertFalse(Auth::check(), 'an unacknowledged batch is not usable: the challenge must still be required'); + $this->assertSame('2fa', Session::get('flow')); + } + + // ------------------------------------------------------------------------- + // Recovery-code enrollment right after a 2FA challenge (SDS idp-mfa.md §4.10.3) + // ------------------------------------------------------------------------- + + public function testVerify2FAWritesTheCompletionMarkerAndReportsEnrollmentRequired(): void + { + $admin = $this->user(self::ADMIN_EMAIL); + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + $this->assertNull(Session::get(MFAConstants::SESSION_KEY_COMPLETED_AT), 'no marker before the challenge is completed'); + $code = $this->latestOtpCode(self::ADMIN_EMAIL); + + $response = $this->verify($code); + + $this->assertEquals(200, $response->getStatusCode()); + $payload = json_decode($response->getContent(), true); + $this->assertTrue($payload['recovery_codes_enrollment_required']); + $this->assertFalse($payload['recovery_codes_acknowledged']); + $this->assertSame(0, $payload['recovery_codes_remaining']); + $this->assertArrayHasKey('redirect_url', $payload); + + $marker = Session::get(MFAConstants::SESSION_KEY_COMPLETED_AT); + $this->assertSame($admin->getId(), $marker['user_id']); + $this->assertEqualsWithDelta(time(), $marker['at'], 5); + } + + public function testFailedVerify2FADoesNotWriteTheCompletionMarker(): void + { + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + + $this->verify('000000'); + + $this->assertNull(Session::get(MFAConstants::SESSION_KEY_COMPLETED_AT)); + } + + public function testVerify2FARotatesTheSessionCsrfTokenAndReturnsTheNewOne(): void + { + // Auth::login() regenerates the session id AND its CSRF token. The login page + // was rendered with the old one, so verify2FA must hand over the current token + // for the enroll / acknowledge calls that follow (VerifyCsrfToken is skipped in + // unit tests, so what can be proven here is the token the response carries). + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + $before = Session::token(); + $code = $this->latestOtpCode(self::ADMIN_EMAIL); + + $response = $this->verify($code); + + $payload = json_decode($response->getContent(), true); + $this->assertNotSame($before, Session::token(), 'sign-in rotates the token: the page-rendered one is stale'); + $this->assertSame(Session::token(), $payload['csrf_token']); + } + + public function testVerify2FARecoveryReturnsTheCurrentCsrfToken(): void + { + $admin = $this->user(self::ADMIN_EMAIL); + $this->createRecoveryCode($admin, 'ONLYCODE', false); + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + + $response = $this->recovery('ONLYCODE'); + + $payload = json_decode($response->getContent(), true); + $this->assertSame(Session::token(), $payload['csrf_token']); + } + + public function testEnrollRightAfterVerify2FAReturnsABatchThenAcknowledgeCompletesIt(): void + { + $admin = $this->user(self::ADMIN_EMAIL); + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + $this->verify($this->latestOtpCode(self::ADMIN_EMAIL)); + + $response = $this->enroll(); + + $this->assertEquals(200, $response->getStatusCode()); + $batch = json_decode($response->getContent(), true); + $this->assertCount((int)config('auth.recovery_codes.count', 10), $batch['recovery_codes']); + $this->assertIsInt($batch['batch_id']); + $this->assertFalse($batch['recovery_codes_acknowledged']); + $this->assertTrue($batch['recovery_codes_enrollment_required']); + + $response = $this->acknowledge($batch['batch_id']); + + $this->assertEquals(200, $response->getStatusCode()); + $status = json_decode($response->getContent(), true); + $this->assertTrue($status['recovery_codes_acknowledged']); + $this->assertFalse($status['recovery_codes_enrollment_required']); + $this->assertSame((int)config('auth.recovery_codes.count', 10), $status['recovery_codes_remaining']); + + EntityManager::clear(); + $this->assertNotNull(EntityManager::getRepository(User::class)->find($admin->getId())->getRecoveryCodesAcknowledgedAt()); + $this->assertSame(1, $this->countAudit($admin->getId(), TwoFactorAuditLog::EventRecoveryCodesAcknowledged)); + + // Once acknowledged the batch is usable: enrollment is no longer required. + $this->assertEquals(412, $this->enroll()->getStatusCode()); + } + + public function testEnrollAfterRecoveryCodeSignInThatExhaustedTheCodes(): void + { + $admin = $this->user(self::ADMIN_EMAIL); + $plain = 'LASTCODE'; + $this->createRecoveryCode($admin, $plain, false); + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + + $response = $this->recovery($plain); + + $this->assertEquals(200, $response->getStatusCode()); + $payload = json_decode($response->getContent(), true); + $this->assertTrue($payload['recovery_codes_enrollment_required']); + $this->assertSame(0, $payload['recovery_codes_remaining']); + + $response = $this->enroll(); + + $this->assertEquals(200, $response->getStatusCode()); + $batch = json_decode($response->getContent(), true); + // The old (used) batch is replaced by the new one. + $this->assertCount((int)config('auth.recovery_codes.count', 10), $batch['recovery_codes']); + } + + public function testEnrollIsRejectedForASessionThatNeverPassedVerify(): void + { + $this->be($this->user(self::ADMIN_EMAIL)); + + $response = $this->enroll(); + + $this->assertEquals(412, $response->getStatusCode()); + $this->assertSame(0, App::make(IRecoveryCodeService::class)->countUnusedRecoveryCodes($this->user(self::ADMIN_EMAIL))); + } + + public function testEnrollIsRejectedOutsideTheWindow(): void + { + $admin = $this->user(self::ADMIN_EMAIL); + $this->be($admin); + $window = (int)config('two_factor.recovery_enrollment_window', 600); + Session::put(MFAConstants::SESSION_KEY_COMPLETED_AT, ['user_id' => $admin->getId(), 'at' => time() - $window - 5]); + + $this->assertEquals(412, $this->enroll()->getStatusCode()); + } + + public function testEnrollIsRejectedWithAnotherUsersMarker(): void + { + $admin = $this->user(self::ADMIN_EMAIL); + $this->be($admin); + Session::put(MFAConstants::SESSION_KEY_COMPLETED_AT, ['user_id' => $admin->getId() + 1000, 'at' => time()]); + + $this->assertEquals(412, $this->enroll()->getStatusCode()); + } + + public function testEnrollIsRejectedForAUserWhoDoesNotRequireEnrollment(): void + { + $admin = $this->user(self::ADMIN_EMAIL); + $this->giveUserAcknowledgedBatch($admin); + $this->be($admin); + Session::put(MFAConstants::SESSION_KEY_COMPLETED_AT, ['user_id' => $admin->getId(), 'at' => time()]); + + $this->assertEquals(412, $this->enroll()->getStatusCode()); + } + + public function testEnrollRateLimitIsKeyedByTheAuthenticatedUserAndAppliesRightAfterVerify2FA(): void + { + // verify2FA has already cleared the pending-user state: a limiter keyed by it + // would resolve no subject and let every request through. + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + $this->verify($this->latestOtpCode(self::ADMIN_EMAIL)); + $this->assertNull(Session::get(ITwoFactorRateLimitService::PENDING_USER_SESSION_KEY)); + + $max = (int)config('two_factor.rate_limit.max_enroll_requests', 5); + for ($i = 0; $i < $max; $i++) { + $this->assertEquals(200, $this->enroll()->getStatusCode(), "request #" . ($i + 1) . " must be accepted"); + } + + $response = $this->enroll(); + + $this->assertEquals(429, $response->getStatusCode()); + $this->assertSame((string)$max, $response->headers->get('X-RateLimit-Limit')); + } + + public function testAcknowledgeWithAnEarlierBatchIdIsRejectedAndLeavesTheColumnUnchanged(): void + { + $admin = $this->user(self::ADMIN_EMAIL); + $service = App::make(IRecoveryCodeService::class); + $first = $service->generateRecoveryCodes($admin); + $second = $service->generateRecoveryCodes($admin); + $this->assertNotSame($first->getBatchId(), $second->getBatchId()); + $this->be($admin); + + $this->assertEquals(412, $this->acknowledge($first->getBatchId())->getStatusCode()); + + EntityManager::clear(); + $this->assertNull(EntityManager::getRepository(User::class)->find($admin->getId())->getRecoveryCodesAcknowledgedAt()); + $this->assertSame(0, $this->countAudit($admin->getId(), TwoFactorAuditLog::EventRecoveryCodesAcknowledged)); + } + + public function testAcknowledgingTheCurrentBatchTwiceSucceedsAndWritesOneAuditRow(): void + { + $admin = $this->user(self::ADMIN_EMAIL); + $batch = App::make(IRecoveryCodeService::class)->generateRecoveryCodes($admin); + $this->be($admin); + + $this->assertEquals(200, $this->acknowledge($batch->getBatchId())->getStatusCode()); + $this->assertEquals(200, $this->acknowledge($batch->getBatchId())->getStatusCode()); + + $this->assertSame(1, $this->countAudit($admin->getId(), TwoFactorAuditLog::EventRecoveryCodesAcknowledged)); + EntityManager::clear(); + $entries = EntityManager::getRepository(TwoFactorAuditLog::class) + ->findBy(['user' => $admin->getId(), 'event_type' => TwoFactorAuditLog::EventRecoveryCodesAcknowledged]); + $this->assertSame(['batch_id' => $batch->getBatchId()], $entries[0]->getMetadata()); + } + + public function testAcknowledgeRequiresAnIntegerBatchId(): void + { + $this->be($this->user(self::ADMIN_EMAIL)); + + $response = $this->action('POST', 'Api\\UserApiController@acknowledgeRecoveryCodes', [], [ + 'batch_id' => 'abc', + '_token' => Session::token(), + ]); + + $this->assertEquals(412, $response->getStatusCode()); + } + + public function testRegeneratingResetsTheAcknowledgmentAndTheGenerationAuditCarriesTheBatchId(): void + { + $admin = $this->user(self::ADMIN_EMAIL); + $service = App::make(IRecoveryCodeService::class); + $first = $service->generateRecoveryCodes($admin); + $service->acknowledgeRecoveryCodes($admin, $first->getBatchId()); + + $second = $service->generateRecoveryCodes($admin); + + $this->assertGreaterThan($first->getBatchId(), $second->getBatchId()); + $this->assertSame($second->getBatchId(), $service->getCurrentBatchId($admin)); + $this->assertNull($admin->getRecoveryCodesAcknowledgedAt()); + $this->assertTrue($service->requiresRecoveryCodeEnrollment($admin)); + + EntityManager::clear(); + $generated = EntityManager::getRepository(TwoFactorAuditLog::class) + ->findBy(['user' => $admin->getId(), 'event_type' => TwoFactorAuditLog::EventRecoveryCodesGenerated]); + $ids = array_map(fn($e) => $e->getMetadata()['batch_id'], $generated); + sort($ids); + $this->assertSame([$first->getBatchId(), $second->getBatchId()], $ids); + } + + // ------------------------------------------------------------------------- + // Authenticate middleware: backurl and JSON requests + // ------------------------------------------------------------------------- + + public function testJsonRequestIgnoresTheSessionBackurl(): void + { + $this->be($this->user(self::ADMIN_EMAIL)); + Session::put('backurl', 'https://example.test/deep/link'); + + $response = $this->action( + 'GET', 'Api\\UserActionApiController@getActionsByCurrentUser', [], [], [], [], + ['HTTP_ACCEPT' => 'application/json', 'HTTP_X_REQUESTED_WITH' => 'XMLHttpRequest'] + ); + + $this->assertNotEquals(302, $response->getStatusCode(), 'an XHR must reach its controller, not be redirected to backurl'); + $this->assertSame('https://example.test/deep/link', Session::get('backurl'), 'backurl must stay for the real navigation'); + } + + public function testNonJsonRequestStillFollowsTheSessionBackurl(): void + { + $this->be($this->user(self::ADMIN_EMAIL)); + Session::put('backurl', 'https://example.test/deep/link'); + + $response = $this->action('GET', 'Api\\UserActionApiController@getActionsByCurrentUser'); + + $this->assertEquals(302, $response->getStatusCode()); + $this->assertSame('https://example.test/deep/link', $response->headers->get('Location')); + $this->assertNull(Session::get('backurl')); + } + + private function enroll() + { + return $this->action('POST', 'Api\\UserApiController@enrollRecoveryCodes', [], ['_token' => Session::token()]); + } + + private function acknowledge(int $batchId) + { + return $this->action('POST', 'Api\\UserApiController@acknowledgeRecoveryCodes', [], [ + 'batch_id' => $batchId, + '_token' => Session::token(), + ]); + } + + private function giveUserAcknowledgedBatch(User $user): int + { + /** @var IRecoveryCodeService $service */ + $service = App::make(IRecoveryCodeService::class); + $batch = $service->generateRecoveryCodes($user); + $this->assertTrue($service->acknowledgeRecoveryCodes($user, $batch->getBatchId())); + return $batch->getBatchId(); + } + // ------------------------------------------------------------------------- // post-verify transaction boundary (Task 5: device-trust atomic, audit best-effort) // ------------------------------------------------------------------------- diff --git a/tests/TwoFactorProfileRoutesCsrfTest.php b/tests/TwoFactorProfileRoutesCsrfTest.php index 940ef3cef..a11cd04a9 100644 --- a/tests/TwoFactorProfileRoutesCsrfTest.php +++ b/tests/TwoFactorProfileRoutesCsrfTest.php @@ -44,6 +44,8 @@ public static function stateChangingRoutes(): array return [ 'enable 2fa' => ['/admin/api/v1/users/me/2fa/enable'], 'regenerate recovery codes' => ['/admin/api/v1/users/me/recovery-codes/regenerate'], + 'enroll recovery codes' => ['/admin/api/v1/users/me/recovery-codes/enroll'], + 'acknowledge recovery codes' => ['/admin/api/v1/users/me/recovery-codes/acknowledge'], ]; } } diff --git a/tests/e2e/fixtures/index.ts b/tests/e2e/fixtures/index.ts index f6d80a7cc..2f6757cb4 100644 --- a/tests/e2e/fixtures/index.ts +++ b/tests/e2e/fixtures/index.ts @@ -42,6 +42,8 @@ export const test = base.extend({ 'RESEND_2FA_ENDPOINT', 'CANCEL_LOGIN_ENDPOINT', 'RECOVERY_2FA_ENDPOINT', + 'ENROLL_RECOVERY_CODES_ENDPOINT', + 'ACKNOWLEDGE_RECOVERY_CODES_ENDPOINT', 'FORM_ACTION_ENDPOINT', ]; await page.addInitScript(({ endpoints, internalUrl }) => { diff --git a/tests/e2e/tests/auth/recovery-code-enrollment.spec.ts b/tests/e2e/tests/auth/recovery-code-enrollment.spec.ts new file mode 100644 index 000000000..f4da72aec --- /dev/null +++ b/tests/e2e/tests/auth/recovery-code-enrollment.spec.ts @@ -0,0 +1,69 @@ +import { test, expect } from '../../fixtures'; +import { getLatestOtp } from '../../utils/otp'; +import { ENROLL_URL, acknowledgeRecoveryCodesModal } from '../../utils/recovery_enrollment'; + +// SDS idp-mfa.md §4.10.1: an MFA-enforced user without usable recovery codes +// gets the "Save Your Recovery Codes" modal right after the 2FA challenge and +// reaches their destination only after confirming the batch. +// +// Unlike the specs that mock the verify response, this one runs the real +// sign-in: the enroll and acknowledge calls go through the real CSRF check, +// which a unit test cannot exercise (VerifyCsrfToken is skipped there) and which +// is what breaks if the page keeps using the token it was rendered with - +// Auth::login() rotates the session's CSRF token. +// +// Seeded by CI via idp:create-super-admin in BOTH pull_request_frontend_tests.yml +// and push_frontend_tests.yml, and used by this test only: its first sign-in is +// the one that enrolls the batch. +const ENROLL_USER_EMAIL = 'mfa-enroll@test.com'; +const PASSWORD = '1Qaz2wsx!'; +const VERIFY_URL = '**/auth/login/2fa/verify**'; + +test.describe('Recovery code enrollment after the 2FA challenge', () => { + + test('shows the non-dismissable modal, then continues only after the batch is acknowledged', + async ({ loginPage, page }) => { + await loginPage.goto(); + await loginPage.fillEmail(ENROLL_USER_EMAIL); + await loginPage.fillPassword(PASSWORD); + await expect(loginPage.twoFactorForm).toBeVisible(); + + const otp = getLatestOtp(ENROLL_USER_EMAIL); + await page.locator('[data-testid="two-factor-form"] input[type="tel"]').first().click(); + await page.keyboard.type(otp); + + const [verify, enroll] = await Promise.all([ + page.waitForResponse(VERIFY_URL), + page.waitForResponse(ENROLL_URL), + loginPage.verifyButton.click(), + ]); + + const verifyBody = await verify.json(); + expect(verifyBody.recovery_codes_enrollment_required).toBe(true); + // The rotated token the page must use for the calls that follow. + expect(typeof verifyBody.csrf_token).toBe('string'); + + // Real CSRF check on the enroll call: 200 means the page sent the fresh token. + expect(enroll.status()).toBe(200); + const batch = await enroll.json(); + expect(batch.recovery_codes).toHaveLength(10); + expect(Number.isInteger(batch.batch_id)).toBe(true); + + const modal = page.locator('[data-testid="recovery-code-modal"]'); + await expect(modal).toBeVisible(); + for (const code of batch.recovery_codes.slice(0, 2)) { + await expect(modal).toContainText(code); + } + + // The modal cannot be dismissed with Escape, and the page has not navigated away. + await page.keyboard.press('Escape'); + await expect(modal).toBeVisible(); + + const acknowledge = await acknowledgeRecoveryCodesModal(page); + expect(acknowledge.status).toBe(200); + const status = acknowledge.body; + expect(status.recovery_codes_acknowledged).toBe(true); + expect(status.recovery_codes_enrollment_required).toBe(false); + expect(status.recovery_codes_remaining).toBe(10); + }); +}); diff --git a/tests/e2e/tests/oauth2/auth-code-flow.spec.ts b/tests/e2e/tests/oauth2/auth-code-flow.spec.ts index a9ba2ba7e..35b4f84bd 100644 --- a/tests/e2e/tests/oauth2/auth-code-flow.spec.ts +++ b/tests/e2e/tests/oauth2/auth-code-flow.spec.ts @@ -1,5 +1,6 @@ import { test, expect } from '../../fixtures'; import { getLatestOtp } from '../../utils/otp'; +import { acknowledgeRecoveryCodesModal } from '../../utils/recovery_enrollment'; import type { Page, APIRequestContext } from '@playwright/test'; import type { LoginPage } from '../../pages/LoginPage'; @@ -66,7 +67,7 @@ async function loginToMfaChallenge(loginPage: LoginPage, email: string): Promise * becomes a harmless same-URL no-op. */ async function verifyOtpAndFollowRedirect(page: Page, loginPage: LoginPage, otp: string): Promise { - let verifyPayload: { redirect_url?: string } | undefined; + let verifyPayload: { redirect_url?: string; recovery_codes_enrollment_required?: boolean } | undefined; await page.route(VERIFY_URL, async (route) => { const req = route.request(); const response = await page.request.fetch(req.url(), { @@ -90,6 +91,15 @@ async function verifyOtpAndFollowRedirect(page: Page, loginPage: LoginPage, otp: // default poll timeout - especially the first request against a // just-started stack. await expect.poll(() => verifyPayload, { timeout: 15000 }).toBeTruthy(); + + // Seeded MFA-enforced accounts start without recovery codes, so the page now + // enrolls a batch and holds the navigation until the user confirms it (the + // modal replaces the redirect). Confirming also lets a trusted device skip the + // challenge at the next sign-in, which the trust-device test below relies on. + if (verifyPayload!.recovery_codes_enrollment_required) { + await acknowledgeRecoveryCodesModal(page); + } + await page.goto(sameOriginUrl(page, verifyPayload!.redirect_url!)); } diff --git a/tests/e2e/utils/recovery_enrollment.ts b/tests/e2e/utils/recovery_enrollment.ts new file mode 100644 index 000000000..84de98d71 --- /dev/null +++ b/tests/e2e/utils/recovery_enrollment.ts @@ -0,0 +1,39 @@ +import { expect } from '@playwright/test'; +import type { Page } from '@playwright/test'; + +// Same trailing-wildcard reasoning as VERIFY_URL in login-mfa-flow.spec.ts. +export const ENROLL_URL = '**/admin/api/v1/users/me/recovery-codes/enroll**'; +export const ACKNOWLEDGE_URL = '**/admin/api/v1/users/me/recovery-codes/acknowledge**'; + +/** + * A user who requires 2FA and has no usable batch of recovery codes gets the + * non-dismissable "Save Your Recovery Codes" modal right after the challenge + * (SDS idp-mfa.md §4.10.1), and the page navigates only once the batch is + * acknowledged. Every MFA-enforced account seeded by CI starts without codes, + * so any spec that completes a real challenge has to confirm the batch. + * + * The acknowledge button stays disabled for a few seconds on purpose (the user + * is meant to read the codes first), hence the generous timeouts. + * + * The page navigates as soon as the acknowledgment succeeds, which evicts the + * response body from the browser; the body is captured through a route instead. + * + * @returns the acknowledge response status and JSON body, so callers can assert on them. + */ +export async function acknowledgeRecoveryCodesModal(page: Page): Promise<{ status: number; body: any }> { + await expect(page.locator('[data-testid="recovery-code-modal"]')).toBeVisible({ timeout: 15000 }); + + let captured: { status: number; body: any } | undefined; + await page.route(ACKNOWLEDGE_URL, async (route) => { + const response = await route.fetch(); + captured = { status: response.status(), body: await response.json().catch(() => null) }; + await route.fulfill({ response }); + }); + + const acknowledge = page.locator('[data-testid="acknowledge-codes-button"]'); + await expect(acknowledge).toBeEnabled({ timeout: 15000 }); + await acknowledge.click(); + + await expect.poll(() => captured, { timeout: 15000 }).toBeTruthy(); + return captured!; +} diff --git a/tests/js/components/recovery_codes_panel.test.js b/tests/js/components/recovery_codes_panel.test.js index 6f1b83928..104a532af 100644 --- a/tests/js/components/recovery_codes_panel.test.js +++ b/tests/js/components/recovery_codes_panel.test.js @@ -1,7 +1,11 @@ import React from 'react'; -import {render, screen, fireEvent} from '@testing-library/react'; +import {render, screen, fireEvent, waitFor} from '@testing-library/react'; import RecoveryCodesPanel from '../../../resources/js/components/recovery_codes_panel'; -import {regenerateRecoveryCodes} from '../../../resources/js/profile/actions'; +import { + regenerateRecoveryCodes, + enrollRecoveryCodes, + acknowledgeRecoveryCodes, +} from '../../../resources/js/profile/actions'; jest.mock('../../../resources/js/profile/actions'); jest.mock('sweetalert2', () => jest.fn()); @@ -9,97 +13,289 @@ jest.mock('sweetalert2', () => jest.fn()); // data-testid on a MUI TextField lands on the wrapper
, not the . const getPasswordInput = () => screen.getByTestId('recovery-codes-current-password').querySelector('input'); +const STATUS_ACKED = (remaining = 10) => ({ + recovery_codes_remaining: remaining, + recovery_codes_total: 10, + recovery_codes_low_threshold: 3, + recovery_codes_acknowledged: true, + recovery_codes_enrollment_required: false, +}); + +const GENERATED = (codes, batchId) => ({ + recovery_codes: codes, + batch_id: batchId, + recovery_codes_remaining: codes.length, + recovery_codes_total: codes.length, + recovery_codes_low_threshold: 3, + recovery_codes_acknowledged: false, + recovery_codes_enrollment_required: true, +}); + +const flush = () => new Promise((resolve) => setImmediate(resolve)); + describe('RecoveryCodesPanel', () => { beforeEach(() => { window.sessionStorage.clear(); jest.clearAllMocks(); }); - it('shows the remaining/total count', () => { - render( - - ); - expect(screen.getByTestId('recovery-codes-count')).toHaveTextContent('Recovery Codes: 7 of 10 remaining'); - }); + describe('acknowledged batch', () => { + it('shows the remaining/total count', () => { + render( + + ); + expect(screen.getByTestId('recovery-codes-count')).toHaveTextContent('Recovery Codes: 7 of 10 remaining'); + }); - it('shows a dismissable low-code warning when remaining is below the threshold', () => { - render( - - ); - expect(screen.getByTestId('low-code-warning')).toBeInTheDocument(); + it('shows a dismissable low-code warning when remaining is below the threshold', () => { + render( + + ); + expect(screen.getByTestId('low-code-warning')).toBeInTheDocument(); - fireEvent.click(screen.getByLabelText('dismiss')); - expect(screen.queryByTestId('low-code-warning')).not.toBeInTheDocument(); - }); + fireEvent.click(screen.getByLabelText('dismiss')); + expect(screen.queryByTestId('low-code-warning')).not.toBeInTheDocument(); + }); - it('does not show the low-code warning when there are enough codes', () => { - render( - - ); - expect(screen.queryByTestId('low-code-warning')).not.toBeInTheDocument(); - }); + it('does not show the low-code warning when there are enough codes', () => { + render( + + ); + expect(screen.queryByTestId('low-code-warning')).not.toBeInTheDocument(); + }); - it('respects a custom lowCodeThreshold instead of the default of 3', () => { - // 4 remaining would NOT trigger the default threshold (3), but does with a custom threshold of 5. - render( - - ); - expect(screen.getByTestId('low-code-warning')).toBeInTheDocument(); - }); + it('respects a custom lowCodeThreshold instead of the default of 3', () => { + // 4 remaining would NOT trigger the default threshold (3), but does with a custom threshold of 5. + render( + + ); + expect(screen.getByTestId('low-code-warning')).toBeInTheDocument(); + }); - it('respects a custom lower threshold (2 remaining is not below a threshold of 2)', () => { - // With the default threshold (3) this would show the warning; a custom - // threshold of 2 must be honored instead of the hardcoded default. - render( - - ); - expect(screen.queryByTestId('low-code-warning')).not.toBeInTheDocument(); - }); + it('respects a custom lower threshold (2 remaining is not below a threshold of 2)', () => { + render( + + ); + expect(screen.queryByTestId('low-code-warning')).not.toBeInTheDocument(); + }); + + it('regenerates after confirming the current password, then acknowledges the batch before updating the panel', async () => { + const newCodes = ['NEW1-CODE', 'NEW2-CODE']; + regenerateRecoveryCodes.mockResolvedValue({response: GENERATED(newCodes, 55)}); + acknowledgeRecoveryCodes.mockResolvedValue({response: {...STATUS_ACKED(2), recovery_codes_total: 2}}); + + render( + + ); - it('opens the modal immediately when initialCodes is provided', () => { - const codes = ['AAAA-1111', 'BBBB-2222']; - render( - - ); - expect(screen.getByText('AAAA-1111')).toBeInTheDocument(); + fireEvent.click(screen.getByText('Regenerate Codes')); + fireEvent.change(getPasswordInput(), {target: {value: 'my-password'}}); + fireEvent.click(screen.getByTestId('confirm-regenerate-button')); + + expect(regenerateRecoveryCodes).toHaveBeenCalledWith('my-password'); + expect(await screen.findByText('NEW1-CODE')).toBeInTheDocument(); + // Not confirmed yet: no "N remaining" for a batch the user has not acknowledged. + expect(screen.queryByTestId('recovery-codes-count')).not.toBeInTheDocument(); + expect(acknowledgeRecoveryCodes).not.toHaveBeenCalled(); + + await waitFor(() => expect(screen.getByTestId('acknowledge-codes-button')).not.toBeDisabled(), {timeout: 7000}); + fireEvent.click(screen.getByTestId('acknowledge-codes-button')); + + expect(acknowledgeRecoveryCodes).toHaveBeenCalledWith(55); + await waitFor(() => expect(screen.getByTestId('recovery-codes-count')) + .toHaveTextContent('Recovery Codes: 2 of 2 remaining')); + expect(screen.queryByText('NEW1-CODE')).not.toBeInTheDocument(); + }, 15000); + + it('shows an error and keeps the existing count when the password is wrong', async () => { + regenerateRecoveryCodes.mockRejectedValue({ + status: 412, + response: {body: {errors: ['current_password is not correct.']}}, + }); + + render( + + ); + + fireEvent.click(screen.getByText('Regenerate Codes')); + fireEvent.change(getPasswordInput(), {target: {value: 'wrong'}}); + fireEvent.click(screen.getByTestId('confirm-regenerate-button')); + + expect(regenerateRecoveryCodes).toHaveBeenCalledWith('wrong'); + await flush(); + expect(screen.getByTestId('recovery-codes-count')).toHaveTextContent('Recovery Codes: 7 of 10 remaining'); + }); }); - it('regenerates codes after confirming the current password and opens the modal', async () => { - const newCodes = ['NEW1-CODE', 'NEW2-CODE']; - regenerateRecoveryCodes.mockResolvedValue({response: {recovery_codes: newCodes}}); + describe('acknowledged batch, user without a password', () => { + const props = {recoveryCodesRemaining: 2, recoveryCodesTotal: 10, hasPassword: false, email: 'user@test.com'}; - render( - - ); + it('inside the window: regenerates without asking for a password', async () => { + regenerateRecoveryCodes.mockResolvedValue({response: GENERATED(['AAAA-1111'], 9)}); - fireEvent.click(screen.getByText('Regenerate Codes')); - fireEvent.change(getPasswordInput(), {target: {value: 'my-password'}}); - fireEvent.click(screen.getByTestId('confirm-regenerate-button')); + render(); - expect(regenerateRecoveryCodes).toHaveBeenCalledWith('my-password'); - expect(await screen.findByText('NEW1-CODE')).toBeInTheDocument(); - expect(screen.getByTestId('recovery-codes-count')).toHaveTextContent('Recovery Codes: 2 of 2 remaining'); + expect(screen.queryByTestId('recovery-codes-current-password')).not.toBeInTheDocument(); + fireEvent.click(screen.getByText('Regenerate Codes')); + + expect(regenerateRecoveryCodes).toHaveBeenCalledWith(null); + expect(await screen.findByText('AAAA-1111')).toBeInTheDocument(); + }); + + it('outside the window: explains how to regenerate and offers no action', () => { + render(); + + expect(screen.queryByText('Regenerate Codes')).not.toBeInTheDocument(); + expect(screen.getByTestId('recovery-codes-trusted-devices-note')) + .toHaveTextContent('revoke this device in Trusted Devices, sign out and sign in again'); + }); }); - it('shows an error and keeps the existing count when the password is wrong', async () => { - regenerateRecoveryCodes.mockRejectedValue({ - status: 412, - response: {body: {errors: ['current_password is not correct.']}}, + describe('no usable batch (enrollment required)', () => { + const base = { + recoveryCodesRemaining: 10, + recoveryCodesTotal: 10, + recoveryCodesAcknowledged: false, + recoveryCodesEnrollmentRequired: true, + email: 'user@test.com', + }; + + it('never shows "10 of 10 remaining" for an unacknowledged batch', () => { + render(); + + expect(screen.queryByTestId('recovery-codes-count')).not.toBeInTheDocument(); + expect(screen.getByTestId('recovery-codes-not-set-up')).toBeInTheDocument(); + }); + + it('available: offers "Set up recovery codes" and does not enroll on load', () => { + render(); + + expect(screen.getByTestId('recovery-codes-not-set-up')).toHaveTextContent('Your recovery codes are not set up yet.'); + expect(screen.getByTestId('setup-recovery-codes-button')).toBeInTheDocument(); + expect(enrollRecoveryCodes).not.toHaveBeenCalled(); }); - render( - - ); + it('available: enrolls only when the user clicks, shows the modal and acknowledges the batch', async () => { + enrollRecoveryCodes.mockResolvedValue({response: GENERATED(['EEEE-0001', 'EEEE-0002'], 71)}); + acknowledgeRecoveryCodes.mockResolvedValue({response: {...STATUS_ACKED(10)}}); + + render(); + fireEvent.click(screen.getByTestId('setup-recovery-codes-button')); + + expect(enrollRecoveryCodes).toHaveBeenCalledTimes(1); + expect(await screen.findByText('EEEE-0001')).toBeInTheDocument(); + + await waitFor(() => expect(screen.getByTestId('acknowledge-codes-button')).not.toBeDisabled(), {timeout: 7000}); + fireEvent.click(screen.getByTestId('acknowledge-codes-button')); + + expect(acknowledgeRecoveryCodes).toHaveBeenCalledWith(71); + // The count comes from the server's status keys, not from a hardcoded 10. + await waitFor(() => expect(screen.getByTestId('recovery-codes-count')) + .toHaveTextContent('Recovery Codes: 10 of 10 remaining')); + }, 15000); + + it('available: a failed enroll falls back to the "not set up yet" state', async () => { + enrollRecoveryCodes.mockRejectedValue({status: 412}); + + render(); + fireEvent.click(screen.getByTestId('setup-recovery-codes-button')); + await flush(); + + expect(screen.queryByTestId('setup-recovery-codes-button')).not.toBeInTheDocument(); + expect(screen.getByTestId('recovery-codes-not-set-up')) + .toHaveTextContent('Regenerate them to get a set you can save.'); + expect(screen.getByText('Regenerate Codes')).toBeInTheDocument(); + }); + + it('available: an enroll body without codes counts as a failure', async () => { + enrollRecoveryCodes.mockResolvedValue({response: {recovery_codes: [], batch_id: 1}}); + + render(); + fireEvent.click(screen.getByTestId('setup-recovery-codes-button')); + await flush(); + + expect(screen.queryByTestId('acknowledge-codes-button')).not.toBeInTheDocument(); + expect(screen.getByTestId('recovery-codes-not-set-up')) + .toHaveTextContent('Sign out and sign in again to set them up.'); + }); + + it('not available, has password: asks to regenerate', () => { + render(); + + expect(screen.getByTestId('recovery-codes-not-set-up')) + .toHaveTextContent('Your recovery codes are not set up yet. Regenerate them to get a set you can save.'); + expect(screen.getByText('Regenerate Codes')).toBeInTheDocument(); + }); + + it('not available, no password: asks to sign out and in again, with no regenerate action', () => { + render(); + + expect(screen.getByTestId('recovery-codes-not-set-up')) + .toHaveTextContent('Your recovery codes are not set up yet. Sign out and sign in again to set them up.'); + expect(screen.queryByText('Regenerate Codes')).not.toBeInTheDocument(); + }); + + it('acknowledge 412 (batch replaced): closes the modal and falls back to "not set up yet"', async () => { + enrollRecoveryCodes.mockResolvedValue({response: GENERATED(['EEEE-0001'], 71)}); + acknowledgeRecoveryCodes.mockRejectedValue({status: 412}); + + render(); + fireEvent.click(screen.getByTestId('setup-recovery-codes-button')); + await screen.findByText('EEEE-0001'); + await waitFor(() => expect(screen.getByTestId('acknowledge-codes-button')).not.toBeDisabled(), {timeout: 7000}); + + fireEvent.click(screen.getByTestId('acknowledge-codes-button')); + + await waitFor(() => expect(screen.queryByText('EEEE-0001')).not.toBeInTheDocument()); + expect(screen.getByTestId('recovery-codes-not-set-up')).toBeInTheDocument(); + expect(screen.queryByTestId('recovery-codes-count')).not.toBeInTheDocument(); + }, 15000); + + it('acknowledge failure: keeps the modal open with the error and a Retry', async () => { + enrollRecoveryCodes.mockResolvedValue({response: GENERATED(['EEEE-0001'], 71)}); + acknowledgeRecoveryCodes.mockRejectedValueOnce({status: 500}); + acknowledgeRecoveryCodes.mockResolvedValueOnce({response: STATUS_ACKED(10)}); + + render(); + fireEvent.click(screen.getByTestId('setup-recovery-codes-button')); + await screen.findByText('EEEE-0001'); + await waitFor(() => expect(screen.getByTestId('acknowledge-codes-button')).not.toBeDisabled(), {timeout: 7000}); + + fireEvent.click(screen.getByTestId('acknowledge-codes-button')); + + expect(await screen.findByTestId('recovery-code-modal-error')).toBeInTheDocument(); + expect(screen.getByText('EEEE-0001')).toBeInTheDocument(); + + fireEvent.click(screen.getByTestId('retry-acknowledge-button')); + + expect(acknowledgeRecoveryCodes).toHaveBeenCalledTimes(2); + await waitFor(() => expect(screen.getByTestId('recovery-codes-count')).toBeInTheDocument()); + }, 15000); + }); + + describe('first batch of a self-enrollment', () => { + it('opens the modal immediately when initialBatch is provided and acknowledges it', async () => { + acknowledgeRecoveryCodes.mockResolvedValue({response: {...STATUS_ACKED(2), recovery_codes_total: 2}}); + + render( + + ); + expect(screen.getByText('AAAA-1111')).toBeInTheDocument(); - fireEvent.click(screen.getByText('Regenerate Codes')); - fireEvent.change(getPasswordInput(), {target: {value: 'wrong'}}); - fireEvent.click(screen.getByTestId('confirm-regenerate-button')); + await waitFor(() => expect(screen.getByTestId('acknowledge-codes-button')).not.toBeDisabled(), {timeout: 7000}); + fireEvent.click(screen.getByTestId('acknowledge-codes-button')); - expect(regenerateRecoveryCodes).toHaveBeenCalledWith('wrong'); - await new Promise((resolve) => setImmediate(resolve)); - expect(screen.getByTestId('recovery-codes-count')).toHaveTextContent('Recovery Codes: 7 of 10 remaining'); + expect(acknowledgeRecoveryCodes).toHaveBeenCalledWith(3); + await waitFor(() => expect(screen.getByTestId('recovery-codes-count')) + .toHaveTextContent('Recovery Codes: 2 of 2 remaining')); + }, 15000); }); }); diff --git a/tests/js/components/two_factor_section.test.js b/tests/js/components/two_factor_section.test.js index e5e9dfefe..c47708784 100644 --- a/tests/js/components/two_factor_section.test.js +++ b/tests/js/components/two_factor_section.test.js @@ -24,15 +24,35 @@ describe('TwoFactorSection', () => { it('shows the recovery codes panel when 2FA is already enabled', () => { render( + recoveryCodesAcknowledged={true} email="user@test.com"/> ); expect(screen.queryByTestId('enable-two-factor-button')).not.toBeInTheDocument(); expect(screen.getByTestId('recovery-codes-count')).toHaveTextContent('Recovery Codes: 7 of 10 remaining'); }); - it('enables 2FA and shows the enrollment codes in the modal', async () => { + it('shows "not set up yet" instead of a count when the user requires enrollment', () => { + render( + + ); + expect(screen.queryByTestId('recovery-codes-count')).not.toBeInTheDocument(); + expect(screen.getByTestId('setup-recovery-codes-button')).toBeInTheDocument(); + }); + + it('enables 2FA and shows the first batch in the modal, which still has to be confirmed', async () => { const codes = ['AAAA-1111', 'BBBB-2222']; - enableTwoFactor.mockResolvedValue({response: {recovery_codes: codes}}); + enableTwoFactor.mockResolvedValue({ + response: { + recovery_codes: codes, + batch_id: 12, + recovery_codes_remaining: 2, + recovery_codes_total: 2, + recovery_codes_low_threshold: 3, + recovery_codes_acknowledged: false, + recovery_codes_enrollment_required: true, + }, + }); render( { expect(enableTwoFactor).toHaveBeenCalledWith('email_otp'); expect(await screen.findByText('AAAA-1111')).toBeInTheDocument(); - expect(screen.getByTestId('recovery-codes-count')).toHaveTextContent('Recovery Codes: 2 of 2 remaining'); + // Not acknowledged yet: no count behind the modal. + expect(screen.queryByTestId('recovery-codes-count')).not.toBeInTheDocument(); }); }); diff --git a/tests/js/login/login.recovery_enrollment.test.js b/tests/js/login/login.recovery_enrollment.test.js new file mode 100644 index 000000000..9b644f08b --- /dev/null +++ b/tests/js/login/login.recovery_enrollment.test.js @@ -0,0 +1,276 @@ +import { FLOW, HTTP_CODES } from '../../../resources/js/login/constants'; + +// actions.js makes real XHR calls — stub every export so the module loads cleanly. +jest.mock('../../../resources/js/login/actions', () => ({ + verifyAccount: jest.fn(), + emitOTP: jest.fn(), + resendVerificationEmail: jest.fn(), + verify2FA: jest.fn(), + resend2FA: jest.fn(), + verifyRecoveryCode: jest.fn(), + enrollRecoveryCodes: jest.fn(), + acknowledgeRecoveryCodes: jest.fn(), + cancelLogin: jest.fn(), +})); + +delete window.location; +window.location = { href: '', reload: jest.fn() }; + +import { LoginPage } from '../../../resources/js/login/login'; +import { + verify2FA, + verifyRecoveryCode, + enrollRecoveryCodes, + acknowledgeRecoveryCodes, +} from '../../../resources/js/login/actions'; + +const PROPS = { + userName: 'admin@example.com', + flow: FLOW.PASSWORD, + allowNativeAuth: true, + otpLength: 6, + otpLifetime: 300, + mfaMethod: 'email_otp', + authError: '', + token: 'csrf-token', + appName: 'Test IDP', + appLogo: '', + formAction: '/auth/login', + forgotPasswordAction: '/auth/forgot', + createAccountAction: '/auth/register', + helpAction: '/help', + captchaPublicKey: '', + verifyEmailAction: '/auth/verify', +}; + +const REDIRECT = '/oauth2/auth?client_id=abc'; +const BATCH = { recovery_codes: ['AAAA-BBBB', 'CCCC-DDDD'], batch_id: 41 }; + +function makeInstance() { + const inst = new LoginPage(PROPS); + inst.setState = jest.fn().mockImplementation((updaterOrObject) => { + const patch = + typeof updaterOrObject === 'function' ? updaterOrObject(inst.state) : updaterOrObject; + inst.state = { ...inst.state, ...patch }; + }); + inst.showAlert = jest.fn(); + inst.resetToPasswordFlow = jest.fn(); + return inst; +} + +const flush = () => new Promise((resolve) => setTimeout(resolve, 0)); +// Auth::login() rotates the session CSRF token, so verify returns the current one +// and the enrollment calls must send it instead of the page-rendered props.token. +const FRESH_TOKEN = 'fresh-csrf-token'; +const verifyOk = (extra = {}) => + Promise.resolve({ response: { redirect_url: REDIRECT, csrf_token: FRESH_TOKEN, ...extra } }); + +describe('LoginPage recovery-code enrollment after 2FA', () => { + let inst; + + beforeEach(() => { + jest.clearAllMocks(); + window.location.href = ''; + inst = makeInstance(); + inst.state.twoFactorCode = '123456'; + inst.state.recoveryCode = 'ABCD1234'; + }); + + describe('when enrollment is required', () => { + it('OTP sign-in — enrolls and shows the modal without navigating', async () => { + verify2FA.mockReturnValue(verifyOk({ recovery_codes_enrollment_required: true })); + enrollRecoveryCodes.mockReturnValue(Promise.resolve({ response: BATCH })); + + inst.onVerify2FA(); + await flush(); + + expect(enrollRecoveryCodes).toHaveBeenCalledWith(FRESH_TOKEN); + expect(inst.state.recoveryEnrollment).toMatchObject({ + codes: BATCH.recovery_codes, + batchId: 41, + redirectUrl: REDIRECT, + csrfToken: FRESH_TOKEN, + busy: false, + error: null, + }); + expect(window.location.href).toBe(''); + }); + + it('falls back to the page token when verify does not return one', async () => { + verify2FA.mockReturnValue( + Promise.resolve({ response: { redirect_url: REDIRECT, recovery_codes_enrollment_required: true } }), + ); + enrollRecoveryCodes.mockReturnValue(Promise.resolve({ response: BATCH })); + + inst.onVerify2FA(); + await flush(); + + expect(enrollRecoveryCodes).toHaveBeenCalledWith('csrf-token'); + }); + + it('recovery-code sign-in — shows the modal instead of the low-code banner', async () => { + verifyRecoveryCode.mockReturnValue( + verifyOk({ + recovery_codes_enrollment_required: true, + recovery_codes_remaining: 0, + recovery_codes_low_threshold: 3, + }), + ); + enrollRecoveryCodes.mockReturnValue(Promise.resolve({ response: BATCH })); + + inst.onVerifyRecovery(); + await flush(); + + expect(inst.state.recoveryEnrollment).not.toBeNull(); + expect(inst.state.lowRecoveryCodesWarning).toBeNull(); + expect(window.location.href).toBe(''); + }); + + it.each([ + ['empty codes', { recovery_codes: [], batch_id: 41 }], + ['missing codes', { batch_id: 41 }], + ['non-integer batch id', { recovery_codes: ['AAAA-BBBB'], batch_id: '41' }], + ['missing batch id', { recovery_codes: ['AAAA-BBBB'] }], + ['empty body', null], + ])('invalid enroll body (%s) — treated as a failure, sign-in continues', async (_, body) => { + verify2FA.mockReturnValue(verifyOk({ recovery_codes_enrollment_required: true })); + enrollRecoveryCodes.mockReturnValue(Promise.resolve({ response: body })); + + inst.onVerify2FA(); + await flush(); + + expect(inst.state.recoveryEnrollment).toBeNull(); + expect(window.location.href).toBe(REDIRECT); + }); + + it('enroll failure (412 / 429 / network) — never blocks the sign-in', async () => { + verify2FA.mockReturnValue(verifyOk({ recovery_codes_enrollment_required: true })); + enrollRecoveryCodes.mockReturnValue(Promise.reject({ status: HTTP_CODES.TOO_MANY_REQUESTS })); + + inst.onVerify2FA(); + await flush(); + + expect(inst.state.recoveryEnrollment).toBeNull(); + expect(window.location.href).toBe(REDIRECT); + }); + }); + + describe('when enrollment is not required', () => { + it('OTP sign-in — navigates straight to redirect_url without enrolling', async () => { + verify2FA.mockReturnValue(verifyOk({ recovery_codes_enrollment_required: false })); + + inst.onVerify2FA(); + await flush(); + + expect(enrollRecoveryCodes).not.toHaveBeenCalled(); + expect(window.location.href).toBe(REDIRECT); + }); + + it('recovery-code sign-in with few codes left — keeps the low-code banner', async () => { + verifyRecoveryCode.mockReturnValue( + verifyOk({ + recovery_codes_enrollment_required: false, + recovery_codes_remaining: 1, + recovery_codes_low_threshold: 3, + }), + ); + + inst.onVerifyRecovery(); + await flush(); + + expect(enrollRecoveryCodes).not.toHaveBeenCalled(); + expect(inst.state.lowRecoveryCodesWarning).toEqual({ remaining: 1, redirectUrl: REDIRECT }); + }); + }); + + describe('onAcknowledgeRecoveryCodes', () => { + beforeEach(() => { + inst.state.recoveryEnrollment = { + codes: BATCH.recovery_codes, + batchId: 41, + redirectUrl: REDIRECT, + csrfToken: FRESH_TOKEN, + busy: false, + error: null, + }; + }); + + it('navigates to redirect_url only after the acknowledgment succeeds', async () => { + let resolveAck; + acknowledgeRecoveryCodes.mockReturnValue(new Promise((resolve) => { resolveAck = resolve; })); + + inst.onAcknowledgeRecoveryCodes(); + + expect(acknowledgeRecoveryCodes).toHaveBeenCalledWith(41, FRESH_TOKEN); + expect(inst.state.recoveryEnrollment.busy).toBe(true); + expect(window.location.href).toBe(''); + + resolveAck({ response: { recovery_codes_acknowledged: true } }); + await flush(); + + expect(window.location.href).toBe(REDIRECT); + }); + + it('ignores a second click while the acknowledgment is in flight', () => { + acknowledgeRecoveryCodes.mockReturnValue(new Promise(() => {})); + + inst.onAcknowledgeRecoveryCodes(); + inst.onAcknowledgeRecoveryCodes(); + + expect(acknowledgeRecoveryCodes).toHaveBeenCalledTimes(1); + }); + + it('412 (batch replaced) — enrolls again and shows the new batch', async () => { + acknowledgeRecoveryCodes.mockReturnValue(Promise.reject({ status: HTTP_CODES.PRECONDITION_FAILED })); + enrollRecoveryCodes.mockReturnValue( + Promise.resolve({ response: { recovery_codes: ['EEEE-FFFF'], batch_id: 77 } }), + ); + + inst.onAcknowledgeRecoveryCodes(); + await flush(); + + expect(enrollRecoveryCodes).toHaveBeenCalledTimes(1); + expect(enrollRecoveryCodes).toHaveBeenCalledWith(FRESH_TOKEN); + expect(inst.state.recoveryEnrollment).toMatchObject({ + codes: ['EEEE-FFFF'], + batchId: 77, + busy: false, + error: null, + }); + expect(window.location.href).toBe(''); + }); + + it('412 and the second enroll fails — navigates anyway', async () => { + acknowledgeRecoveryCodes.mockReturnValue(Promise.reject({ status: HTTP_CODES.PRECONDITION_FAILED })); + enrollRecoveryCodes.mockReturnValue(Promise.reject({ status: HTTP_CODES.PRECONDITION_FAILED })); + + inst.onAcknowledgeRecoveryCodes(); + await flush(); + + expect(window.location.href).toBe(REDIRECT); + }); + + it('any other failure — keeps the modal open with an error, ready to retry', async () => { + acknowledgeRecoveryCodes.mockReturnValue( + Promise.reject({ status: HTTP_CODES.INTERNAL_SERVER_ERROR }), + ); + + inst.onAcknowledgeRecoveryCodes(); + await flush(); + + expect(window.location.href).toBe(''); + expect(enrollRecoveryCodes).not.toHaveBeenCalled(); + expect(inst.state.recoveryEnrollment.busy).toBe(false); + expect(inst.state.recoveryEnrollment.error).toEqual(expect.any(String)); + expect(inst.state.recoveryEnrollment.batchId).toBe(41); + + // Retry re-sends the same batch. + acknowledgeRecoveryCodes.mockReturnValue(Promise.resolve({ response: {} })); + inst.onAcknowledgeRecoveryCodes(); + await flush(); + + expect(acknowledgeRecoveryCodes).toHaveBeenLastCalledWith(41, FRESH_TOKEN); + expect(window.location.href).toBe(REDIRECT); + }); + }); +}); diff --git a/tests/js/profile/actions.test.js b/tests/js/profile/actions.test.js index a07af3811..72de63bc1 100644 --- a/tests/js/profile/actions.test.js +++ b/tests/js/profile/actions.test.js @@ -9,7 +9,12 @@ jest.mock("superagent", () => ({ })); import request from "superagent"; -import { enableTwoFactor, regenerateRecoveryCodes } from "profile/actions"; +import { + enableTwoFactor, + regenerateRecoveryCodes, + enrollRecoveryCodes, + acknowledgeRecoveryCodes, +} from "profile/actions"; function makeChainableRequest({ body = {}, error = null } = {}) { const req = {}; @@ -27,6 +32,8 @@ describe("profile/actions (unmocked request layer)", () => { request.post.mockReset(); window.ENABLE_TWO_FACTOR_ENDPOINT = "https://idp.test/api/v2/users/me/2fa/enable"; window.REGENERATE_RECOVERY_CODES_ENDPOINT = "https://idp.test/api/v2/users/me/2fa/recovery-codes"; + window.ENROLL_RECOVERY_CODES_ENDPOINT = "https://idp.test/admin/api/v1/users/me/recovery-codes/enroll"; + window.ACKNOWLEDGE_RECOVERY_CODES_ENDPOINT = "https://idp.test/admin/api/v1/users/me/recovery-codes/acknowledge"; window.CSFR_TOKEN = "test-csrf-token"; }); @@ -54,4 +61,36 @@ describe("profile/actions (unmocked request layer)", () => { expect(req.send).toHaveBeenCalledWith({ current_password: "super-secret-password" }); expect(response.recovery_codes).toEqual(["WXYZ-5678"]); }); + + it("regenerateRecoveryCodes without a password sends an empty body (passwordless regeneration inside the window)", async () => { + const req = makeChainableRequest({ body: { recovery_codes: ["WXYZ-5678"], batch_id: 4 } }); + request.post.mockReturnValue(req); + + await regenerateRecoveryCodes(); + + expect(req.send).toHaveBeenCalledWith({}); + }); + + it("enrollRecoveryCodes posts to the enroll endpoint asking for JSON, so the auth middleware does not redirect an XHR to backurl", async () => { + const req = makeChainableRequest({ body: { recovery_codes: ["ABCD-1234"], batch_id: 9 } }); + request.post.mockReturnValue(req); + + const { response } = await enrollRecoveryCodes(); + + expect(request.post).toHaveBeenCalledWith(window.ENROLL_RECOVERY_CODES_ENDPOINT); + expect(req.set).toHaveBeenCalledWith({ "X-CSRF-TOKEN": "test-csrf-token", Accept: "application/json" }); + expect(response.batch_id).toBe(9); + }); + + it("acknowledgeRecoveryCodes sends the batch id in the body, never in the URL", async () => { + const req = makeChainableRequest({ body: { recovery_codes_acknowledged: true } }); + request.post.mockReturnValue(req); + + const { response } = await acknowledgeRecoveryCodes(9); + + expect(request.post).toHaveBeenCalledWith(window.ACKNOWLEDGE_RECOVERY_CODES_ENDPOINT); + expect(req.send).toHaveBeenCalledWith({ batch_id: 9 }); + expect(req.set).toHaveBeenCalledWith({ "X-CSRF-TOKEN": "test-csrf-token", Accept: "application/json" }); + expect(response.recovery_codes_acknowledged).toBe(true); + }); }); diff --git a/tests/support/RecoveryCodeWorker.php b/tests/support/RecoveryCodeWorker.php new file mode 100644 index 000000000..0a779ea03 --- /dev/null +++ b/tests/support/RecoveryCodeWorker.php @@ -0,0 +1,42 @@ + + * php RecoveryCodeWorker.php redeem + * + * Prints one JSON line: {"ok":true,...} or {"ok":false,"error":"..."}. + */ +use App\Services\Auth\IRecoveryCodeService; +use Auth\User; +use Illuminate\Contracts\Console\Kernel; +use LaravelDoctrine\ORM\Facades\EntityManager; +use Strategies\MFA\MFAChallengeStrategyFactory; +use Utils\Services\IAuthService; + +require __DIR__ . '/../../vendor/autoload.php'; + +$app = require __DIR__ . '/../../bootstrap/app.php'; +$app->make(Kernel::class)->bootstrap(); + +[, $action, $userId] = $argv; + +try { + /** @var User $user */ + $user = EntityManager::getRepository(User::class)->find((int)$userId); + + if ($action === 'generate') { + $batch = $app->make(IRecoveryCodeService::class)->generateRecoveryCodes($user); + echo json_encode(['ok' => true, 'batch_id' => $batch->getBatchId()]), PHP_EOL; + } elseif ($action === 'redeem') { + $strategy = MFAChallengeStrategyFactory::create(User::MFAMethod_OTP); + $app->make(IAuthService::class)->verifyMFARecoveryCode($user, $strategy, $argv[3]); + echo json_encode(['ok' => true]), PHP_EOL; + } else { + echo json_encode(['ok' => false, 'error' => 'unknown action']), PHP_EOL; + } +} catch (\Throwable $ex) { + echo json_encode(['ok' => false, 'error' => get_class($ex) . ': ' . $ex->getMessage()]), PHP_EOL; +} diff --git a/tests/unit/MFA/AbstractMFAChallengeStrategyTest.php b/tests/unit/MFA/AbstractMFAChallengeStrategyTest.php index 1c0ad6c15..8e4271779 100644 --- a/tests/unit/MFA/AbstractMFAChallengeStrategyTest.php +++ b/tests/unit/MFA/AbstractMFAChallengeStrategyTest.php @@ -163,7 +163,9 @@ public function testVerifyRecoveryCode_withMatchingCode_marksAsUsed(): void $recoveryCode->shouldReceive('markUsed')->once(); $repo = \Mockery::mock(IUserRecoveryCodeRepository::class); - $repo->shouldReceive('getUnusedByUser')->with($user)->andReturn([$recoveryCode]); + // Lock order: the users row is locked before any code is read (SDS idp-mfa.md §3.5). + $repo->shouldReceive('lockUserRow')->with($user)->once()->ordered(); + $repo->shouldReceive('getUnusedByUser')->with($user)->ordered()->andReturn([$recoveryCode]); // Lock is taken before mutating (regression guard for 3357348455). $repo->shouldReceive('refreshExclusiveLock')->with($recoveryCode)->once()->andReturn(true); @@ -193,6 +195,8 @@ public function testVerifyRecoveryCode_locksAndRejects_whenUsedAfterLock(): void $recoveryCode->shouldNotReceive('markUsed'); $repo = \Mockery::mock(IUserRecoveryCodeRepository::class); + // Lock order: the users row is locked before any code is read (SDS idp-mfa.md §3.5). + $repo->shouldReceive('lockUserRow')->with($user)->once()->ordered(); $repo->shouldReceive('getUnusedByUser')->with($user)->andReturn([$recoveryCode]); $repo->shouldReceive('refreshExclusiveLock')->with($recoveryCode)->once()->andReturn(true); $repo->shouldNotReceive('add'); @@ -222,6 +226,8 @@ public function testVerifyRecoveryCode_rejects_whenCodeDeletedBeforeLock(): void $recoveryCode->shouldNotReceive('markUsed'); $repo = \Mockery::mock(IUserRecoveryCodeRepository::class); + // Lock order: the users row is locked before any code is read (SDS idp-mfa.md §3.5). + $repo->shouldReceive('lockUserRow')->with($user)->once()->ordered(); $repo->shouldReceive('getUnusedByUser')->with($user)->andReturn([$recoveryCode]); $repo->shouldReceive('refreshExclusiveLock')->with($recoveryCode)->once()->andReturn(false); @@ -245,6 +251,8 @@ public function testVerifyRecoveryCode_withNonMatchingCode_throwsException(): vo $recoveryCode->shouldNotReceive('markUsed'); $repo = \Mockery::mock(IUserRecoveryCodeRepository::class); + // Lock order: the users row is locked before any code is read (SDS idp-mfa.md §3.5). + $repo->shouldReceive('lockUserRow')->with($user)->once()->ordered(); $repo->shouldReceive('getUnusedByUser')->andReturn([$recoveryCode]); $strategy = new class($repo) extends AbstractMFAChallengeStrategy { @@ -263,6 +271,8 @@ public function testVerifyRecoveryCode_withAllCodesUsed_throwsException(): void $user = new User(); $repo = \Mockery::mock(IUserRecoveryCodeRepository::class); + // Lock order: the users row is locked before any code is read (SDS idp-mfa.md §3.5). + $repo->shouldReceive('lockUserRow')->with($user)->once()->ordered(); $repo->shouldReceive('getUnusedByUser')->andReturn([]); $strategy = new class($repo) extends AbstractMFAChallengeStrategy { diff --git a/tests/unit/MFAGateServiceTest.php b/tests/unit/MFAGateServiceTest.php index 84fb1ea80..b8704cd13 100644 --- a/tests/unit/MFAGateServiceTest.php +++ b/tests/unit/MFAGateServiceTest.php @@ -16,6 +16,7 @@ **/ use App\Services\Auth\IDeviceTrustService; +use App\Services\Auth\IRecoveryCodeService; use App\Services\Auth\MFAGateService; use Auth\User; use Mockery; @@ -32,11 +33,17 @@ final class MFAGateServiceTest extends TestCase private MockInterface&IDeviceTrustService $deviceTrustService; + private MockInterface&IRecoveryCodeService $recoveryCodeService; + protected function setUp(): void { parent::setUp(); $this->deviceTrustService = Mockery::mock(IDeviceTrustService::class); - $this->service = new MFAGateService($this->deviceTrustService); + $this->recoveryCodeService = Mockery::mock(IRecoveryCodeService::class); + // Default: the user already has an acknowledged, usable batch, so the + // pre-existing device-trust cases below keep their meaning. + $this->recoveryCodeService->shouldReceive('requiresRecoveryCodeEnrollment')->andReturn(false)->byDefault(); + $this->service = new MFAGateService($this->deviceTrustService, $this->recoveryCodeService); } protected function tearDown(): void @@ -111,6 +118,41 @@ public function testRequiresChallengePassesThroughEmptyStringCookieToDeviceTrust $this->assertTrue($this->service->requiresChallenge($user, '')); } + // ------------------------------------------------------------------------- + // Recovery-code enrollment (SDS idp-mfa.md §4.2 step 4): a user with no + // usable batch is challenged even on a trusted device, and the device row + // is not consulted (so its last_seen_at is not touched) + // ------------------------------------------------------------------------- + + public function testRequiresChallengeReturnsTrueOnTrustedDeviceWhenEnrollmentRequired(): void + { + $user = Mockery::mock(User::class); + $user->shouldReceive('shouldRequire2FA')->once()->andReturn(true); + $this->recoveryCodeService->shouldReceive('requiresRecoveryCodeEnrollment')->once()->with($user)->andReturn(true); + $this->deviceTrustService->shouldNotReceive('isDeviceTrusted'); + + $this->assertTrue($this->service->requiresChallenge($user, 'valid-token')); + } + + public function testRequiresChallengeSkipsOnTrustedDeviceOnceBatchIsAcknowledged(): void + { + $user = Mockery::mock(User::class); + $user->shouldReceive('shouldRequire2FA')->once()->andReturn(true); + $this->recoveryCodeService->shouldReceive('requiresRecoveryCodeEnrollment')->once()->with($user)->andReturn(false); + $this->deviceTrustService->shouldReceive('isDeviceTrusted')->once()->with($user, 'valid-token')->andReturn(true); + + $this->assertFalse($this->service->requiresChallenge($user, 'valid-token')); + } + + public function testEnrollmentIsNotEvaluatedForUsersThatDoNotRequire2FA(): void + { + $user = Mockery::mock(User::class); + $user->shouldReceive('shouldRequire2FA')->once()->andReturn(false); + $this->recoveryCodeService->shouldNotReceive('requiresRecoveryCodeEnrollment'); + + $this->assertFalse($this->service->requiresChallenge($user, 'valid-token')); + } + // ------------------------------------------------------------------------- // Global kill-switch (SDS idp-mfa.md §10.1) is now enforced inside // User::shouldRequire2FA() (single source of truth shared with the From c73205e44c9764f26fe40bf1f3e7ba417f9010c5 Mon Sep 17 00:00:00 2001 From: romanetar Date: Fri, 6 Nov 2026 13:49:34 +0100 Subject: [PATCH 2/3] test(e2e): cover the profile recovery-codes panel against the real server jest covers every panel state in isolation; this spec proves the pieces work together on the profile page: the real status keys of the profile view, the real CSRF check on enroll / regenerate / acknowledge, nothing generated on load or reload, the batch not counted until it is acknowledged, and the panel's buttons never submitting the page's enclosing form. It is a single test made of steps because the steps share the signed-in session and the account's recovery codes. CI seeds its user, mfa-profile@test.com, in both frontend workflows. --- .../workflows/pull_request_frontend_tests.yml | 1 + .github/workflows/push_frontend_tests.yml | 1 + .../profile/recovery-codes-panel.spec.ts | 163 ++++++++++++++++++ 3 files changed, 165 insertions(+) create mode 100644 tests/e2e/tests/profile/recovery-codes-panel.spec.ts diff --git a/.github/workflows/pull_request_frontend_tests.yml b/.github/workflows/pull_request_frontend_tests.yml index 25b56bee1..268cc9afa 100644 --- a/.github/workflows/pull_request_frontend_tests.yml +++ b/.github/workflows/pull_request_frontend_tests.yml @@ -110,6 +110,7 @@ jobs: php artisan idp:create-super-admin mfa-oauth2-consent@test.com '1Qaz2wsx!' php artisan idp:create-super-admin mfa-oauth2-trust@test.com '1Qaz2wsx!' php artisan idp:create-super-admin mfa-enroll@test.com '1Qaz2wsx!' + php artisan idp:create-super-admin mfa-profile@test.com '1Qaz2wsx!' php artisan idp:create-oauth2-test-client - name: Install Playwright Chromium run: npx playwright install --with-deps chromium diff --git a/.github/workflows/push_frontend_tests.yml b/.github/workflows/push_frontend_tests.yml index 24c7c34b3..f92e24a30 100644 --- a/.github/workflows/push_frontend_tests.yml +++ b/.github/workflows/push_frontend_tests.yml @@ -111,6 +111,7 @@ jobs: php artisan idp:create-super-admin mfa-oauth2-consent@test.com '1Qaz2wsx!' php artisan idp:create-super-admin mfa-oauth2-trust@test.com '1Qaz2wsx!' php artisan idp:create-super-admin mfa-enroll@test.com '1Qaz2wsx!' + php artisan idp:create-super-admin mfa-profile@test.com '1Qaz2wsx!' php artisan idp:create-oauth2-test-client - name: Install Playwright Chromium run: npx playwright install --with-deps chromium diff --git a/tests/e2e/tests/profile/recovery-codes-panel.spec.ts b/tests/e2e/tests/profile/recovery-codes-panel.spec.ts new file mode 100644 index 000000000..496afba8f --- /dev/null +++ b/tests/e2e/tests/profile/recovery-codes-panel.spec.ts @@ -0,0 +1,163 @@ +import { test, expect } from '../../fixtures'; +import type { Page } from '@playwright/test'; +import { getLatestOtp } from '../../utils/otp'; +import { ENROLL_URL, ACKNOWLEDGE_URL, acknowledgeRecoveryCodesModal } from '../../utils/recovery_enrollment'; + +// Recovery-code panel of the profile page, against the real server (SDS idp-mfa.md +// §4.10.3). jest covers every panel state in isolation; this spec proves the pieces +// work together: the real status keys of the profile view, the real CSRF check on +// enroll / regenerate / acknowledge, and that the panel lives inside the profile +// page's single
without ever submitting it. +// +// Seeded by CI via idp:create-super-admin in BOTH pull_request_frontend_tests.yml and +// push_frontend_tests.yml, and used by this spec only. It is ONE test made of steps, +// not several tests: the steps share the signed-in session and the account's recovery +// codes, which go from "none" to "acknowledged" (each Playwright test gets a fresh +// browser context, so separate tests would lose the session). +const PROFILE_USER_EMAIL = 'mfa-profile@test.com'; +const PASSWORD = '1Qaz2wsx!'; +const VERIFY_URL = '**/auth/login/2fa/verify**'; +const REGENERATE_URL = '**/admin/api/v1/users/me/recovery-codes/regenerate**'; +const PROFILE_PATH = '/accounts/user/profile'; + +/** Counts requests to a URL pattern from now on. */ +function countRequests(page: Page, urlGlob: string): () => number { + let count = 0; + page.on('request', (req) => { + const re = new RegExp('^' + urlGlob.replace(/[.+?^${}()|[\]\\]/g, '\\$&').replace(/\*\*/g, '.*') + '$'); + if (re.test(req.url())) count += 1; + }); + return () => count; +} + +/** The OTP row can land a moment after the 2FA form shows up: retry instead of failing. */ +async function waitForOtp(email: string): Promise { + let otp = ''; + await expect.poll(() => { + try { + otp = getLatestOtp(email); + } catch { + otp = ''; + } + return otp; + }, { timeout: 15000 }).not.toBe(''); + return otp; +} + +async function signInWithoutSavingTheCodes(page: Page, loginPage: any): Promise { + await loginPage.goto(); + await loginPage.fillEmail(PROFILE_USER_EMAIL); + await loginPage.fillPassword(PASSWORD); + await expect(loginPage.twoFactorForm).toBeVisible(); + + // The login page would enroll and show the modal. Failing the enroll call makes it + // continue to the destination without a batch (an enroll failure never blocks the + // sign-in), which leaves the user with enrollment required and the window open. + await page.route(ENROLL_URL, (route) => route.abort()); + + await page.locator('[data-testid="two-factor-form"] input[type="tel"]').first().click(); + await page.keyboard.type(await waitForOtp(PROFILE_USER_EMAIL)); + await Promise.all([ + page.waitForResponse(VERIFY_URL), + loginPage.verifyButton.click(), + ]); + await page.waitForURL((url) => !url.pathname.startsWith('/auth/login'), { timeout: 15000 }); + await page.unroute(ENROLL_URL); +} + +test('profile recovery-codes panel, from "not set up yet" to an acknowledged batch', async ({ loginPage, page }) => { + await signInWithoutSavingTheCodes(page, loginPage); + + await test.step('a user with no usable batch sees "not set up yet" and nothing is generated on load', async () => { + const enrollCalls = countRequests(page, ENROLL_URL); + await page.goto(PROFILE_PATH); + + await expect(page.getByTestId('recovery-codes-not-set-up')).toContainText('Your recovery codes are not set up yet.'); + await expect(page.getByTestId('setup-recovery-codes-button')).toBeVisible(); + // Never "10 of 10 remaining" for a batch nobody confirmed. + await expect(page.getByTestId('recovery-codes-count')).toHaveCount(0); + + await page.reload(); + await expect(page.getByTestId('setup-recovery-codes-button')).toBeVisible(); + expect(enrollCalls(), 'enrollment must only happen when the user asks for it').toBe(0); + }); + + await test.step('"Set up recovery codes" shows a non-dismissable modal; acknowledging it completes the panel', async () => { + // A marker that survives only while the page is not reloaded or navigated: proves the + // panel's buttons do not submit the profile page's enclosing . + await page.evaluate(() => { (window as any).__noReload = true; }); + + const [enroll] = await Promise.all([ + page.waitForResponse(ENROLL_URL), + page.getByTestId('setup-recovery-codes-button').click(), + ]); + expect(enroll.status(), 'the real CSRF check accepts the profile page token').toBe(200); + const batch = await enroll.json(); + expect(batch.recovery_codes).toHaveLength(10); + + const modal = page.getByTestId('recovery-code-modal'); + await expect(modal).toBeVisible(); + await expect(modal).toContainText(batch.recovery_codes[0]); + await page.keyboard.press('Escape'); + await expect(modal).toBeVisible(); + // Nothing is counted until the batch is confirmed. + await expect(page.getByTestId('recovery-codes-count')).toHaveCount(0); + + const acknowledge = await acknowledgeRecoveryCodesModal(page); + expect(acknowledge.status).toBe(200); + expect(acknowledge.body.recovery_codes_acknowledged).toBe(true); + + await expect(modal).toBeHidden(); + await expect(page.getByTestId('recovery-codes-count')).toContainText('Recovery Codes: 10 of 10 remaining'); + expect(await page.evaluate(() => (window as any).__noReload)).toBe(true); + }); + + await test.step('after a reload the acknowledged batch is shown and the setup action is gone', async () => { + await page.goto(PROFILE_PATH); + + await expect(page.getByTestId('recovery-codes-count')).toContainText('Recovery Codes: 10 of 10 remaining'); + await expect(page.getByTestId('setup-recovery-codes-button')).toHaveCount(0); + await expect(page.getByTestId('recovery-codes-not-set-up')).toHaveCount(0); + }); + + await test.step('a wrong password keeps the existing batch', async () => { + await page.getByText('Regenerate Codes').click(); + await page.getByTestId('recovery-codes-current-password').locator('input').fill('not-the-password'); + const [regenerate] = await Promise.all([ + page.waitForResponse(REGENERATE_URL), + page.getByTestId('confirm-regenerate-button').click(), + ]); + + expect(regenerate.status()).toBe(412); + await expect(page.getByTestId('recovery-code-modal')).toHaveCount(0); + await expect(page.getByTestId('recovery-codes-count')).toContainText('Recovery Codes: 10 of 10 remaining'); + }); + + await test.step('regenerating with the current password replaces the batch and needs a new acknowledgment', async () => { + await page.goto(PROFILE_PATH); + await page.evaluate(() => { (window as any).__noReload = true; }); + const ackCalls = countRequests(page, ACKNOWLEDGE_URL); + + await page.getByText('Regenerate Codes').click(); + await page.getByTestId('recovery-codes-current-password').locator('input').fill(PASSWORD); + + const [regenerate] = await Promise.all([ + page.waitForResponse(REGENERATE_URL), + page.getByTestId('confirm-regenerate-button').click(), + ]); + expect(regenerate.status()).toBe(200); + const batch = await regenerate.json(); + expect(batch.recovery_codes_acknowledged).toBe(false); + expect(batch.recovery_codes_enrollment_required).toBe(true); + + await expect(page.getByTestId('recovery-code-modal')).toBeVisible(); + // The new batch is not usable yet: the panel must not claim "10 remaining". + await expect(page.getByTestId('recovery-codes-count')).toHaveCount(0); + expect(ackCalls()).toBe(0); + + const acknowledge = await acknowledgeRecoveryCodesModal(page); + expect(acknowledge.status).toBe(200); + await expect(page.getByTestId('recovery-codes-count')).toContainText('Recovery Codes: 10 of 10 remaining'); + expect(await page.evaluate(() => (window as any).__noReload)).toBe(true); + }); +}); From 7db1851d5e0ec96ae01974642ff03c2eee162023 Mon Sep 17 00:00:00 2001 From: romanetar Date: Fri, 6 Nov 2026 14:06:14 +0100 Subject: [PATCH 3/3] fix(2fa): restart the acknowledge delay for a replaced batch and wait for the post-acknowledge navigation in e2e Review feedback on #183. - RecoveryCodeModal: the 5 second read-before-confirm delay restarted only when `open` changed. On the login page a 412 from acknowledge re-enrolls and swaps the codes while the modal stays open, so the new batch could be confirmed immediately. The countdown now also restarts when the displayed codes change (keyed by content, so an equal array passed again does not reset it). - e2e helper acknowledgeRecoveryCodesModal: optional `expectNavigation` waits for the load event of the navigation the login page starts after acknowledging, so the page.goto that follows cannot race it. Used by the login flows; the profile panel does not navigate, so it keeps the default. --- .../js/components/recovery_code_modal.js | 6 ++- .../auth/recovery-code-enrollment.spec.ts | 2 +- tests/e2e/tests/oauth2/auth-code-flow.spec.ts | 6 ++- tests/e2e/utils/recovery_enrollment.ts | 13 ++++++- .../js/components/recovery_code_modal.test.js | 38 +++++++++++++++++++ 5 files changed, 60 insertions(+), 5 deletions(-) diff --git a/resources/js/components/recovery_code_modal.js b/resources/js/components/recovery_code_modal.js index 8deaa3406..a93a4aa39 100644 --- a/resources/js/components/recovery_code_modal.js +++ b/resources/js/components/recovery_code_modal.js @@ -15,6 +15,10 @@ const ACK_DELAY_SECONDS = 5; const RecoveryCodeModal = ({open, codes, email, appName, onAcknowledge, error = null, busy = false, onRetry}) => { const [secondsLeft, setSecondsLeft] = useState(ACK_DELAY_SECONDS); + // Identity of the displayed batch. A different batch shown while the modal stays open + // (the login page re-enrolls after a 412) must get its own full delay: keyed by the + // codes' content, so a parent re-rendering with an equal array does not restart it. + const batchKey = codes ? codes.join("|") : ""; useEffect(() => { if (!open) return undefined; @@ -25,7 +29,7 @@ const RecoveryCodeModal = ({open, codes, email, appName, onAcknowledge, error = }, 1000); return () => clearInterval(interval); - }, [open]); + }, [open, batchKey]); return ( { await page.keyboard.press('Escape'); await expect(modal).toBeVisible(); - const acknowledge = await acknowledgeRecoveryCodesModal(page); + const acknowledge = await acknowledgeRecoveryCodesModal(page, { expectNavigation: true }); expect(acknowledge.status).toBe(200); const status = acknowledge.body; expect(status.recovery_codes_acknowledged).toBe(true); diff --git a/tests/e2e/tests/oauth2/auth-code-flow.spec.ts b/tests/e2e/tests/oauth2/auth-code-flow.spec.ts index 35b4f84bd..02f80d562 100644 --- a/tests/e2e/tests/oauth2/auth-code-flow.spec.ts +++ b/tests/e2e/tests/oauth2/auth-code-flow.spec.ts @@ -95,9 +95,11 @@ async function verifyOtpAndFollowRedirect(page: Page, loginPage: LoginPage, otp: // Seeded MFA-enforced accounts start without recovery codes, so the page now // enrolls a batch and holds the navigation until the user confirms it (the // modal replaces the redirect). Confirming also lets a trusted device skip the - // challenge at the next sign-in, which the trust-device test below relies on. + // challenge at the next sign-in, which the trust-device test below relies on. The page then + // navigates on its own (to the same URL, as redirect_url is nulled above); wait for that to + // finish so the goto below cannot race it. if (verifyPayload!.recovery_codes_enrollment_required) { - await acknowledgeRecoveryCodesModal(page); + await acknowledgeRecoveryCodesModal(page, { expectNavigation: true }); } await page.goto(sameOriginUrl(page, verifyPayload!.redirect_url!)); diff --git a/tests/e2e/utils/recovery_enrollment.ts b/tests/e2e/utils/recovery_enrollment.ts index 84de98d71..870bfe523 100644 --- a/tests/e2e/utils/recovery_enrollment.ts +++ b/tests/e2e/utils/recovery_enrollment.ts @@ -18,9 +18,16 @@ export const ACKNOWLEDGE_URL = '**/admin/api/v1/users/me/recovery-codes/acknowle * The page navigates as soon as the acknowledgment succeeds, which evicts the * response body from the browser; the body is captured through a route instead. * + * The login page navigates once the batch is acknowledged (profile panel: it does not). Pass + * `expectNavigation` there and this waits for that navigation to finish, so a `page.goto` + * issued right after cannot race the page's own navigation. + * * @returns the acknowledge response status and JSON body, so callers can assert on them. */ -export async function acknowledgeRecoveryCodesModal(page: Page): Promise<{ status: number; body: any }> { +export async function acknowledgeRecoveryCodesModal( + page: Page, + { expectNavigation = false }: { expectNavigation?: boolean } = {}, +): Promise<{ status: number; body: any }> { await expect(page.locator('[data-testid="recovery-code-modal"]')).toBeVisible({ timeout: 15000 }); let captured: { status: number; body: any } | undefined; @@ -32,8 +39,12 @@ export async function acknowledgeRecoveryCodesModal(page: Page): Promise<{ statu const acknowledge = page.locator('[data-testid="acknowledge-codes-button"]'); await expect(acknowledge).toBeEnabled({ timeout: 15000 }); + + // Registered before the click: the navigation starts as soon as the response is handled. + const navigated = expectNavigation ? page.waitForEvent('load', { timeout: 15000 }) : null; await acknowledge.click(); await expect.poll(() => captured, { timeout: 15000 }).toBeTruthy(); + if (navigated) await navigated; return captured!; } diff --git a/tests/js/components/recovery_code_modal.test.js b/tests/js/components/recovery_code_modal.test.js index 8582e7a0f..95f26f867 100644 --- a/tests/js/components/recovery_code_modal.test.js +++ b/tests/js/components/recovery_code_modal.test.js @@ -42,6 +42,44 @@ describe('RecoveryCodeModal', () => { expect(onAcknowledge).toHaveBeenCalledTimes(1); }); + it('restarts the delay when a different batch replaces the codes while the modal stays open', () => { + const NEW_CODES = ['CCCC-3333', 'DDDD-4444']; + const {rerender} = render( + + ); + + act(() => { + jest.advanceTimersByTime(5000); + }); + expect(screen.getByTestId('acknowledge-codes-button')).not.toBeDisabled(); + + rerender(); + + // The new codes must be readable before they can be confirmed. + expect(screen.getByTestId('acknowledge-codes-button')).toBeDisabled(); + act(() => { + jest.advanceTimersByTime(5000); + }); + expect(screen.getByTestId('acknowledge-codes-button')).not.toBeDisabled(); + }); + + it('does not restart the delay when the same codes are passed again', () => { + const {rerender} = render( + + ); + + act(() => { + jest.advanceTimersByTime(5000); + }); + expect(screen.getByTestId('acknowledge-codes-button')).not.toBeDisabled(); + + // New array identity, same content (e.g. a parent re-rendering while a request is in flight). + rerender(); + + expect(screen.getByTestId('acknowledge-codes-button')).not.toBeDisabled(); + }); + it('cannot be dismissed by pressing Escape', () => { const onAcknowledge = jest.fn(); render();