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..268cc9afa 100644 --- a/.github/workflows/pull_request_frontend_tests.yml +++ b/.github/workflows/pull_request_frontend_tests.yml @@ -109,6 +109,8 @@ 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-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 2e1d05f58..f92e24a30 100644 --- a/.github/workflows/push_frontend_tests.yml +++ b/.github/workflows/push_frontend_tests.yml @@ -110,6 +110,8 @@ 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-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/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..a93a4aa39 100644 --- a/resources/js/components/recovery_code_modal.js +++ b/resources/js/components/recovery_code_modal.js @@ -13,8 +13,12 @@ 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); + // 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}) => { }, 1000); return () => clearInterval(interval); - }, [open]); + }, [open, batchKey]); return ( { {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..8d4482920 --- /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, { expectNavigation: true }); + 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..02f80d562 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,17 @@ 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. 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, { expectNavigation: true }); + } + await page.goto(sameOriginUrl(page, verifyPayload!.redirect_url!)); } 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); + }); +}); diff --git a/tests/e2e/utils/recovery_enrollment.ts b/tests/e2e/utils/recovery_enrollment.ts new file mode 100644 index 000000000..870bfe523 --- /dev/null +++ b/tests/e2e/utils/recovery_enrollment.ts @@ -0,0 +1,50 @@ +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. + * + * 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, + { 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; + 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 }); + + // 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(); 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