From e0620efd0f7b5702223df15c1d9859c0be3ce03a Mon Sep 17 00:00:00 2001 From: romanetar Date: Mon, 6 Jul 2026 16:03:28 +0200 Subject: [PATCH 01/12] feat: recovery code management Signed-off-by: romanetar --- .../Controllers/Api/UserApiController.php | 102 +++++++++- app/Http/Controllers/UserController.php | 12 ++ app/Services/Auth/IRecoveryCodeService.php | 52 +++++ app/Services/Auth/RecoveryCodeService.php | 94 +++++++++ .../Auth/TwoFactorServiceProvider.php | 2 + app/libs/Auth/Models/TwoFactorAuditLog.php | 2 + config/auth.php | 6 + ...tion-not-triggered-by-group-enforcement.md | 91 +++++++++ package.json | 4 +- .../js/components/recovery_code_display.js | 73 +++++++ .../js/components/recovery_code_modal.js | 64 ++++++ .../js/components/recovery_codes.module.scss | 43 ++++ .../js/components/recovery_codes_panel.js | 142 +++++++++++++ resources/js/components/two_factor_section.js | 66 +++++++ resources/js/login/login.js | 6 +- resources/js/profile/actions.js | 12 +- resources/js/profile/profile.js | 33 +++- resources/js/profile/profile.module.scss | 10 + resources/js/utils.js | 12 ++ resources/views/profile.blade.php | 8 +- routes/web.php | 2 + tests/RecoveryCodeRegenerationTest.php | 187 ++++++++++++++++++ .../components/recovery_code_display.test.js | 53 +++++ .../js/components/recovery_code_modal.test.js | 60 ++++++ .../components/recovery_codes_panel.test.js | 105 ++++++++++ .../js/components/two_factor_section.test.js | 48 +++++ tests/js/setup.js | 11 ++ yarn.lock | 4 +- 28 files changed, 1290 insertions(+), 14 deletions(-) create mode 100644 app/Services/Auth/IRecoveryCodeService.php create mode 100644 app/Services/Auth/RecoveryCodeService.php create mode 100644 doc/adrs/0001-recovery-code-generation-not-triggered-by-group-enforcement.md create mode 100644 resources/js/components/recovery_code_display.js create mode 100644 resources/js/components/recovery_code_modal.js create mode 100644 resources/js/components/recovery_codes.module.scss create mode 100644 resources/js/components/recovery_codes_panel.js create mode 100644 resources/js/components/two_factor_section.js create mode 100644 tests/RecoveryCodeRegenerationTest.php create mode 100644 tests/js/components/recovery_code_display.test.js create mode 100644 tests/js/components/recovery_code_modal.test.js create mode 100644 tests/js/components/recovery_codes_panel.test.js create mode 100644 tests/js/components/two_factor_section.test.js diff --git a/app/Http/Controllers/Api/UserApiController.php b/app/Http/Controllers/Api/UserApiController.php index b32c7307..78b051ba 100644 --- a/app/Http/Controllers/Api/UserApiController.php +++ b/app/Http/Controllers/Api/UserApiController.php @@ -15,7 +15,10 @@ use App\Http\Controllers\APICRUDController; use App\Http\Controllers\Traits\RequestProcessor; use App\Http\Controllers\UserValidationRulesFactory; +use App\libs\Auth\Models\TwoFactorAuditLog; use App\ModelSerializers\SerializerRegistry; +use App\Services\Auth\IRecoveryCodeService; +use App\Services\Auth\ITwoFactorAuditService; use Auth\Repositories\IUserRepository; use Auth\User; use Exception; @@ -23,10 +26,13 @@ use Illuminate\Support\Facades\Auth; use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Request; +use Illuminate\Support\Facades\Validator; use models\exceptions\EntityNotFoundException; use models\exceptions\ValidationException; use OAuth2\Services\ITokenService; use OpenId\Services\IUserService; +use Utils\Db\ITransactionService; +use Utils\IPHelper; use Utils\Services\ILogService; /** @@ -43,23 +49,47 @@ final class UserApiController extends APICRUDController */ private $token_service; + /** + * @var IRecoveryCodeService + */ + private $recovery_code_service; + + /** + * @var ITransactionService + */ + private $tx_service; + + /** + * @var ITwoFactorAuditService + */ + private $two_factor_audit_service; + /** * UserApiController constructor. * @param IUserRepository $user_repository * @param ILogService $log_service * @param IUserService $user_service * @param ITokenService $token_service + * @param IRecoveryCodeService $recovery_code_service + * @param ITransactionService $tx_service + * @param ITwoFactorAuditService $two_factor_audit_service */ public function __construct ( IUserRepository $user_repository, ILogService $log_service, IUserService $user_service, - ITokenService $token_service + ITokenService $token_service, + IRecoveryCodeService $recovery_code_service, + ITransactionService $tx_service, + ITwoFactorAuditService $two_factor_audit_service ) { parent::__construct($user_repository, $user_service, $log_service); $this->token_service = $token_service; + $this->recovery_code_service = $recovery_code_service; + $this->tx_service = $tx_service; + $this->two_factor_audit_service = $two_factor_audit_service; } /** @@ -247,6 +277,76 @@ public function updateMe() return $this->update(Auth::user()->getId()); } + /** + * Enables a 2FA method for the current user and generates the first batch of + * recovery codes for them. Plaintext codes are returned once in the response + * and never persisted. + * + * @return \Illuminate\Http\JsonResponse|mixed + */ + public function enableTwoFactor() + { + if (!Auth::check()) + return $this->error403(); + + return $this->processRequest(function () { + $data = Request::all(); + $validator = Validator::make($data, [ + 'method' => 'required|string|in:' . implode(',', User::ValidMFAMethods), + ]); + + if (!$validator->passes()) { + return $this->error412($validator->getMessageBag()->getMessages()); + } + + $user = Auth::user(); + $method = $data['method']; + + $codes = $this->tx_service->transaction(function () use ($user, $method) { + $user->enable2FA($method); + $this->repository->add($user, false); + + return $this->recovery_code_service->generateRecoveryCodes($user); + }); + + $this->two_factor_audit_service->log( + $user, + TwoFactorAuditLog::EventEnrollmentChanged, + $method, + IPHelper::getUserIp() + ); + + return $this->ok(['recovery_codes' => $codes]); + }); + } + + /** + * Invalidates the current user's recovery codes and generates a fresh batch. + * Plaintext codes are returned once in the response and never persisted. + * + * @return \Illuminate\Http\JsonResponse|mixed + */ + public function regenerateRecoveryCodes() + { + if (!Auth::check()) + return $this->error403(); + + return $this->processRequest(function () { + $data = Request::all(); + $validator = Validator::make($data, [ + 'current_password' => 'required|string', + ]); + + if (!$validator->passes()) { + return $this->error412($validator->getMessageBag()->getMessages()); + } + + $codes = $this->recovery_code_service->regenerateRecoveryCodes(Auth::user(), $data['current_password']); + + return $this->ok(['recovery_codes' => $codes]); + }); + } + public function revokeAllMyTokens() { if (!Auth::check()) diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index ef3c49ce..00ddf724 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -21,6 +21,7 @@ use App\libs\OAuth2\Strategies\LoginHintProcessStrategy; use App\ModelSerializers\SerializerRegistry; use App\Services\Auth\IDeviceTrustService; +use App\Services\Auth\IRecoveryCodeService; use App\Services\Auth\ITwoFactorAuditService; use App\Services\Auth\ITwoFactorGateService; use App\Services\Auth\ITwoFactorRateLimitService; @@ -160,6 +161,11 @@ final class UserController extends OpenIdController */ private $two_factor_rate_limit_service; + /** + * @var IRecoveryCodeService + */ + private $recovery_code_service; + /** * @param IMementoOpenIdSerializerService $openid_memento_service * @param IMementoOAuth2SerializerService $oauth2_memento_service @@ -200,6 +206,7 @@ public function __construct ITwoFactorAuditService $two_factor_audit_service, ITwoFactorGateService $mfa_gate_service, ITwoFactorRateLimitService $two_factor_rate_limit_service, + IRecoveryCodeService $recovery_code_service, ) { $this->openid_memento_service = $openid_memento_service; @@ -221,6 +228,7 @@ public function __construct $this->two_factor_audit_service = $two_factor_audit_service; $this->mfa_gate_service = $mfa_gate_service; $this->two_factor_rate_limit_service = $two_factor_rate_limit_service; + $this->recovery_code_service = $recovery_code_service; $this->middleware(function ($request, $next) use($login_hint_process_strategy){ @@ -1184,6 +1192,10 @@ public function getProfile() 'actions' => $actions, 'countries' => CountryList::getCountries(), 'languages' => $lang2Code, + 'two_factor_enabled' => $user->shouldRequire2FA(), + 'recovery_codes_remaining' => $this->recovery_code_service->countUnusedRecoveryCodes($user), + 'recovery_codes_total' => (int)config('auth.recovery_codes.count', 10), + 'recovery_codes_low_threshold' => (int)config('auth.recovery_codes.low_threshold', 3), ]); } diff --git a/app/Services/Auth/IRecoveryCodeService.php b/app/Services/Auth/IRecoveryCodeService.php new file mode 100644 index 00000000..56c9d464 --- /dev/null +++ b/app/Services/Auth/IRecoveryCodeService.php @@ -0,0 +1,52 @@ +checkPassword(trim($currentPassword))) { + throw new ValidationException('current_password is not correct.'); + } + + return $this->generateRecoveryCodes($user); + } + + /** + * @inheritDoc + */ + public function generateRecoveryCodes(User $user): array + { + $count = (int)config('auth.recovery_codes.count', 10); + $length = (int)config('auth.recovery_codes.length', 8); + + $plaintext_codes = []; + + $this->tx_service->transaction(function () use ($user, $count, $length, &$plaintext_codes) { + $this->repository->deleteAllForUser($user); + + for ($i = 0; $i < $count; $i++) { + $plain = Rand::getString($length, self::CODE_CHARSET, true); + $plaintext_codes[] = $plain; + + $code = new UserRecoveryCode(); + $code->setUser($user); + $code->setCodeHash(Hash::make($plain)); + $this->repository->add($code, false); + } + }); + + $this->audit_service->log( + $user, + TwoFactorAuditLog::EventRecoveryCodesGenerated, + TwoFactorAuditLog::MethodRecovery, + IPHelper::getUserIp() + ); + + return array_map(static fn(string $code) => implode('-', str_split($code, 4)), $plaintext_codes); + } + + /** + * @inheritDoc + */ + public function countUnusedRecoveryCodes(User $user): int + { + return count($this->repository->getUnusedByUser($user)); + } +} diff --git a/app/Services/Auth/TwoFactorServiceProvider.php b/app/Services/Auth/TwoFactorServiceProvider.php index 0bb18a6f..b82bdd10 100644 --- a/app/Services/Auth/TwoFactorServiceProvider.php +++ b/app/Services/Auth/TwoFactorServiceProvider.php @@ -42,6 +42,7 @@ public function register(): void $this->app->singleton(ITwoFactorAuditService::class, TwoFactorAuditService::class); $this->app->singleton(ITwoFactorGateService::class, MFAGateService::class); $this->app->singleton(ITwoFactorRateLimitService::class, TwoFactorRateLimitService::class); + $this->app->singleton(IRecoveryCodeService::class, RecoveryCodeService::class); } /** @@ -110,6 +111,7 @@ public function provides(): array ITwoFactorAuditService::class, ITwoFactorGateService::class, ITwoFactorRateLimitService::class, + IRecoveryCodeService::class, ]; } } diff --git a/app/libs/Auth/Models/TwoFactorAuditLog.php b/app/libs/Auth/Models/TwoFactorAuditLog.php index 4938345a..5258978a 100644 --- a/app/libs/Auth/Models/TwoFactorAuditLog.php +++ b/app/libs/Auth/Models/TwoFactorAuditLog.php @@ -29,6 +29,7 @@ class TwoFactorAuditLog extends BaseEntity public const EventDeviceRevoked = 'device_revoked'; public const EventRecoveryUsed = 'recovery_used'; public const EventSettingsChanged = 'settings_changed'; + public const EventRecoveryCodesGenerated = 'recovery_codes_generated'; public const MethodEmailOtp = 'email_otp'; public const MethodSmsOtp = 'sms_otp'; @@ -46,6 +47,7 @@ class TwoFactorAuditLog extends BaseEntity self::EventDeviceRevoked, self::EventRecoveryUsed, self::EventSettingsChanged, + self::EventRecoveryCodesGenerated, ]; private const ALLOWED_METHODS = [ diff --git a/config/auth.php b/config/auth.php index 5da1b184..69282ef7 100644 --- a/config/auth.php +++ b/config/auth.php @@ -107,6 +107,12 @@ 'password_shape_warning' => env('AUTH_PASSWORD_SHAPE_WARNING', 'Password must include at least one uppercase letter, one lowercase letter, one number, and one special character (#?!@$%^&*+-).'), 'verification_email_lifetime' => env("AUTH_VERIFICATION_EMAIL_LIFETIME", 600), 'allows_native_auth' => env('AUTH_ALLOWS_NATIVE_AUTH', 1), + + 'recovery_codes' => [ + 'count' => env('MFA_RECOVERY_CODES_COUNT', 10), + 'length' => env('MFA_RECOVERY_CODE_LENGTH', 8), + 'low_threshold' => env('MFA_RECOVERY_CODES_LOW_THRESHOLD', 3), + ], 'allows_native_on_config' => env('AUTH_ALLOWS_NATIVE_AUTH_CONFIG', 1), 'allows_opt_auth' => env('AUTH_ALLOWS_OTP_AUTH', 1), ]; 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 new file mode 100644 index 00000000..31ed8bbd --- /dev/null +++ b/doc/adrs/0001-recovery-code-generation-not-triggered-by-group-enforcement.md @@ -0,0 +1,91 @@ +# 0001. Recovery code generation is not automatically triggered by group-based 2FA enforcement + +## Status + +Accepted — 2026-07-08 + +## Context + +Ticket [CU-86ba2zp66](https://app.clickup.com/t/86ba2zp66) ("Recovery Code Management UI and Endpoints") requires: + +> Create endpoint to generate recovery codes **when 2FA is enabled or admin enforcement requires code creation**. + +Two-factor authentication in this project can become mandatory for a user in two distinct ways: + +1. **Explicit self-enrollment** — the user calls the new `UserApiController::enableTwoFactor()` endpoint + (`app/Http/Controllers/Api/UserApiController.php`), which invokes `User::enable2FA($method)` and, in the + same transaction, calls `RecoveryCodeService::generateRecoveryCodes()` to mint the user's first batch of + recovery codes. +2. **Group-based enforcement** — `User::shouldRequire2FA()` (`app/libs/Auth/Models/User.php`) returns `true` + for any user belonging to one of the groups listed in `config('two_factor.enforced_groups')` + (`config/two_factor.php`: SuperAdmin, Admin, OAuth2ServerAdmin, OpenIdServerAdmins), **independently** of + whether that user ever called `enableTwoFactor()` or has the `two_factor_enabled` column set. + +Only path (1) generates recovery codes automatically. Path (2) has no corresponding hook: there is no +listener on group-membership assignment (e.g. when a user is added to the `Admin` group via +`GroupApiController` or any other admin-management flow) that generates a recovery-code batch for that user. + +Practical consequence: a user who is force-enrolled into MFA purely by being added to an enforced group — +and who never separately visits their profile to enable 2FA or regenerate codes — has **zero recovery codes** +until they proactively open their profile's "Two-Factor Authentication" section and click "Regenerate Codes" +(`resources/js/components/recovery_codes_panel.js`). That manual path works correctly and is not gated on +prior enrollment, but it is not automatic, so the literal wording of the ticket ("or admin enforcement +requires code creation") is not satisfied for this path. + +Implementing the automatic path would require identifying every place group membership can be granted +(direct admin action, bulk import, programmatic group assignment, etc.) and wiring a listener/hook into each +one to call `RecoveryCodeService::generateRecoveryCodes()` exactly once per user, without duplicating codes +on repeated grants or interfering with a user who already regenerated codes themselves. No single +well-defined integration point for "group membership changed" was identified during implementation without +a dedicated exploration pass, and building one was judged to be a meaningfully larger change than the rest +of this ticket. + +## Decision + +We accept the gap as a documented scope limitation for this ticket. Recovery code generation for +group-enforced users remains **on-demand**: the user (or an admin acting on their behalf, e.g. via a support +flow) must visit the profile's Two-Factor Authentication section and use "Regenerate Codes" at least once +after being enrolled through group enforcement. + +No code changes accompany this decision; it documents the trade-off already present in the shipped +implementation (`feat/recovery-codes-management` branch). + +## Consequences + +**Positive** + +- No new event/listener infrastructure needed for group-membership changes, keeping the change surface of + this ticket limited to the profile self-service flow it was originally scoped around. +- The manual path is simple, already implemented, and requires no additional user-facing concept: a + group-enforced user sees the same "Two-Factor Authentication" section and the same "Regenerate Codes" + action as anyone who self-enrolled. +- Avoids the risk of generating recovery codes an admin never asked for or expects during unrelated + group-management operations (e.g. bulk group imports). + +**Negative** + +- A user who is force-enrolled by group membership and is challenged for MFA (e.g. at their next login) + before ever visiting their profile has **no recovery codes available** if they lose access to their normal + 2FA method (email, for Phase I) at that point. Their only recourse is out-of-band administrative + intervention (e.g. a server admin resetting their 2FA state directly), not a self-service recovery path. +- The literal acceptance criterion "generate recovery codes ... when admin enforcement requires code + creation" is not met for the group-enforcement path — only for explicit self-enrollment. + +**Follow-up (not scheduled)** + +If this gap needs to be closed later, the natural integration point is wherever group membership is granted +(`GroupApiController` and any other code path that adds a user to `Group`) — call +`RecoveryCodeService::generateRecoveryCodes($user)` immediately after granting membership to a group in +`config('two_factor.enforced_groups')`, guarded so it only fires when the user currently has zero unused +codes (to avoid clobbering codes on every re-grant). + +## Alternatives considered + +- **Hook into group-assignment code paths now.** Rejected for this ticket: requires auditing every place + group membership can change (there is more than one — see `GroupApiController` and related admin flows) + to guarantee the hook fires exactly once and doesn't silently invalidate codes a user already saved. Judged + to be new scope beyond "Recovery Code Management UI and Endpoints," better handled as its own ticket if + the org decides to close this gap. +- **Generate codes lazily on first MFA challenge instead of on group grant.** Rejected: the MFA challenge + screen itself has no natural place to show a one-time "here are your recovery codes" modal without + interrupting the login flow the ticket explicitly requires to remain "unaffected." diff --git a/package.json b/package.json index 0992db7c..96e436f3 100644 --- a/package.json +++ b/package.json @@ -27,9 +27,9 @@ "@babel/preset-react": "^7.7.4", "@babel/runtime": "^7.20.7", "@playwright/test": "^1.61.1", - "@testing-library/jest-dom": "^5", - "@testing-library/react": "^12", "@testing-library/user-event": "^13", + "@testing-library/jest-dom": "^5.16.5", + "@testing-library/react": "^12.1.5", "babel-cli": "^6.26.0", "babel-jest": "^26.6.3", "babel-loader": "^8.2.4", diff --git a/resources/js/components/recovery_code_display.js b/resources/js/components/recovery_code_display.js new file mode 100644 index 00000000..32a24c2d --- /dev/null +++ b/resources/js/components/recovery_code_display.js @@ -0,0 +1,73 @@ +import React, {useState} from "react"; +import Box from "@material-ui/core/Box"; +import Button from "@material-ui/core/Button"; +import Grid from "@material-ui/core/Grid"; +import Typography from "@material-ui/core/Typography"; +import AssignmentIcon from "@material-ui/icons/Assignment"; +import CheckCircleIcon from "@material-ui/icons/CheckCircle"; +import {downloadTextFile} from "../utils"; + +import styles from "./recovery_codes.module.scss"; + +const buildFileContent = (codes, email) => { + const date = new Date().toISOString().slice(0, 10); + return [ + "FNTECH Recovery Codes", + `Generated: ${date}`, + `Account: ${email}`, + "", + "Keep these codes somewhere safe. Each code can only be used once to sign in, and they will not be shown again.", + "", + ...codes, + ].join("\n"); +}; + +const RecoveryCodeDisplay = ({codes, email}) => { + const [copied, setCopied] = useState(false); + + const handleCopy = () => { + navigator.clipboard.writeText(codes.join("\n")).then(() => { + setCopied(true); + setTimeout(() => setCopied(false), 2000); + }); + }; + + const handleDownload = () => { + const date = new Date().toISOString().slice(0, 10); + downloadTextFile(`fntech-recovery-codes-${date}.txt`, buildFileContent(codes, email)); + }; + + return ( + + + {codes.map((code, idx) => ( + + {code} + + ))} + + + +   + + + + ); +}; + +export default RecoveryCodeDisplay; diff --git a/resources/js/components/recovery_code_modal.js b/resources/js/components/recovery_code_modal.js new file mode 100644 index 00000000..0e460137 --- /dev/null +++ b/resources/js/components/recovery_code_modal.js @@ -0,0 +1,64 @@ +import React, {useEffect, useState} from "react"; +import Box from "@material-ui/core/Box"; +import Button from "@material-ui/core/Button"; +import Dialog from "@material-ui/core/Dialog"; +import DialogActions from "@material-ui/core/DialogActions"; +import DialogContent from "@material-ui/core/DialogContent"; +import DialogTitle from "@material-ui/core/DialogTitle"; +import Typography from "@material-ui/core/Typography"; +import WarningRoundedIcon from "@material-ui/icons/WarningRounded"; +import RecoveryCodeDisplay from "./recovery_code_display"; + +import styles from "./recovery_codes.module.scss"; + +const ACK_DELAY_SECONDS = 5; + +const RecoveryCodeModal = ({open, codes, email, onAcknowledge}) => { + const [secondsLeft, setSecondsLeft] = useState(ACK_DELAY_SECONDS); + + useEffect(() => { + if (!open) return undefined; + + setSecondsLeft(ACK_DELAY_SECONDS); + const interval = setInterval(() => { + setSecondsLeft((prev) => (prev > 0 ? prev - 1 : 0)); + }, 1000); + + return () => clearInterval(interval); + }, [open]); + + return ( + + Save Your Recovery Codes + + + + + These codes will not be shown again. Copy or download them now and store them somewhere safe. + + + {codes && } + + + + + + ); +}; + +export default RecoveryCodeModal; diff --git a/resources/js/components/recovery_codes.module.scss b/resources/js/components/recovery_codes.module.scss new file mode 100644 index 00000000..c4437a1e --- /dev/null +++ b/resources/js/components/recovery_codes.module.scss @@ -0,0 +1,43 @@ +.recovery_codes_panel { + margin-top: 15px; +} + +.codes_grid { + margin: 8px 0; + padding: 12px; + background-color: #f5f5f5; + border-radius: 4px; +} + +.code { + font-family: monospace; + font-size: 1rem; + letter-spacing: 1px; +} + +.warning_banner { + display: flex; + align-items: flex-start; + margin-bottom: 16px; + padding: 10px 14px; + background-color: #fdecea; + border-left: 4px solid #f44336; + border-radius: 4px; +} + +.warning_icon { + color: #f44336; + margin-right: 10px; + margin-top: 1px; + flex-shrink: 0; +} + +.low_code_warning { + display: flex; + align-items: center; + justify-content: space-between; + margin-top: 8px; + padding: 8px 12px; + background-color: #fff3e0; + border-radius: 4px; +} diff --git a/resources/js/components/recovery_codes_panel.js b/resources/js/components/recovery_codes_panel.js new file mode 100644 index 00000000..9b8bf9f6 --- /dev/null +++ b/resources/js/components/recovery_codes_panel.js @@ -0,0 +1,142 @@ +import React, {useState} from "react"; +import Box from "@material-ui/core/Box"; +import Button from "@material-ui/core/Button"; +import Grid from "@material-ui/core/Grid"; +import IconButton from "@material-ui/core/IconButton"; +import Link from "@material-ui/core/Link"; +import TextField from "@material-ui/core/TextField"; +import Typography from "@material-ui/core/Typography"; +import CloseIcon from "@material-ui/icons/Close"; +import {regenerateRecoveryCodes} from "../profile/actions"; +import {handleErrorResponse} from "../utils"; +import RecoveryCodeModal from "./recovery_code_modal"; + +import styles from "./recovery_codes.module.scss"; + +const DEFAULT_LOW_CODE_THRESHOLD = 3; +const LOW_CODE_WARNING_DISMISSED_KEY = "recovery_codes_low_warning_dismissed"; + +const RecoveryCodesPanel = ({ + recoveryCodesRemaining, + recoveryCodesTotal, + lowCodeThreshold = DEFAULT_LOW_CODE_THRESHOLD, + email, + initialCodes = null + }) => { + const [regenerating, setRegenerating] = useState(false); + const [currentPassword, setCurrentPassword] = useState(""); + const [loading, setLoading] = useState(false); + const [remaining, setRemaining] = useState(recoveryCodesRemaining); + const [total, setTotal] = useState(recoveryCodesTotal); + const [codes, setCodes] = useState(initialCodes); + const [warningDismissed, setWarningDismissed] = useState( + sessionStorage.getItem(LOW_CODE_WARNING_DISMISSED_KEY) === "1" + ); + + const handleRegenerate = () => { + setLoading(true); + regenerateRecoveryCodes(currentPassword).then(({response}) => { + setLoading(false); + setRegenerating(false); + setCurrentPassword(""); + setCodes(response.recovery_codes); + setRemaining(response.recovery_codes.length); + setTotal(response.recovery_codes.length); + }).catch((err) => { + setLoading(false); + handleErrorResponse(err); + }); + }; + + const handleAcknowledge = () => { + setCodes(null); + }; + + const dismissLowCodeWarning = () => { + sessionStorage.setItem(LOW_CODE_WARNING_DISMISSED_KEY, "1"); + setWarningDismissed(true); + }; + + const showLowCodeWarning = !warningDismissed && remaining < lowCodeThreshold; + + return ( + <> + + + Recovery Codes: {remaining} of {total} remaining + + { + showLowCodeWarning && + + + You're running low on recovery codes. Regenerate them to avoid getting locked out. + + + + + + } + { + !regenerating && + { + e.preventDefault(); + setRegenerating(true); + }}> + Regenerate Codes + + } + { + regenerating && + + setCurrentPassword(e.target.value)} + onKeyDown={(e) => { + // This panel lives inside the profile page's own
; + // it must not let Enter bubble up and submit that form too. + if (e.key === "Enter") { + e.preventDefault(); + if (currentPassword && !loading) handleRegenerate(); + } + }} + // Detaches this input from the ancestor (the profile page + // wraps everything in one big form) so the browser's native + // "Enter submits the enclosing form" / password-manager-driven + // auto-submit can never fire a GET on it, regardless of the + // onKeyDown handler above. + inputProps={{form: "recovery-codes-detached-form", autoComplete: "off"}} + data-testid="recovery-codes-current-password" + /> +   + +   + { + e.preventDefault(); + setRegenerating(false); + setCurrentPassword(""); + }}> + Cancel + + + } + + + + ); +}; + +export default RecoveryCodesPanel; diff --git a/resources/js/components/two_factor_section.js b/resources/js/components/two_factor_section.js new file mode 100644 index 00000000..6d0c7e74 --- /dev/null +++ b/resources/js/components/two_factor_section.js @@ -0,0 +1,66 @@ +import React, {useState} from "react"; +import Button from "@material-ui/core/Button"; +import Typography from "@material-ui/core/Typography"; +import {enableTwoFactor} from "../profile/actions"; +import {handleErrorResponse} from "../utils"; +import RecoveryCodesPanel from "./recovery_codes_panel"; + +const DEFAULT_METHOD = "email_otp"; + +const TwoFactorSection = ({ + twoFactorEnabled, + recoveryCodesRemaining, + recoveryCodesTotal, + recoveryCodesLowThreshold, + email + }) => { + 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); + + 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); + setEnabled(true); + }).catch((err) => { + setLoading(false); + handleErrorResponse(err); + }); + }; + + if (!enabled) { + return ( + <> + + Two-factor authentication is not enabled for your account. + + + + ); + } + + return ( + + ); +}; + +export default TwoFactorSection; diff --git a/resources/js/login/login.js b/resources/js/login/login.js index 6db6e96b..623279c6 100644 --- a/resources/js/login/login.js +++ b/resources/js/login/login.js @@ -318,9 +318,13 @@ class LoginPage extends React.Component { onRecoveryCodeChange(ev) { let { value } = ev.target; + // Recovery codes are generated and hashed without the "-" separator; it is + // added only for on-screen readability (XXXX-XXXX). Strip any non-alphanumeric + // characters here so a code typed or pasted exactly as displayed still matches. + const normalized = value.replace(/[^A-Za-z0-9]/g, "").toUpperCase(); this.setState({ ...this.state, - recoveryCode: value, + recoveryCode: normalized, errors: { ...this.state.errors, recovery: "" }, }); } diff --git a/resources/js/profile/actions.js b/resources/js/profile/actions.js index c1c6c2f0..0c9ea422 100644 --- a/resources/js/profile/actions.js +++ b/resources/js/profile/actions.js @@ -1,4 +1,4 @@ -import {deleteRawRequest, getRawRequest, putFile, putRawRequest} from "../base_actions"; +import {deleteRawRequest, getRawRequest, postRawRequestFull, putFile, putRawRequest} from "../base_actions"; import moment from "moment"; export const PAGE_SIZE = 10; @@ -87,6 +87,16 @@ 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}); +} + +export const enableTwoFactor = async (method) => { + const params = {method}; + return postRawRequestFull(window.ENABLE_TWO_FACTOR_ENDPOINT)(params, {'X-CSRF-TOKEN': window.CSFR_TOKEN}); +} + const normalizeEntity = (entity) => { entity.public_profile_show_photo = entity.public_profile_show_photo ? 1 : 0; entity.public_profile_show_fullname = entity.public_profile_show_fullname ? 1 : 0; diff --git a/resources/js/profile/profile.js b/resources/js/profile/profile.js index a530d04a..817845c6 100644 --- a/resources/js/profile/profile.js +++ b/resources/js/profile/profile.js @@ -1,5 +1,6 @@ import React, {useState} from "react"; import ReactDOM from "react-dom"; +import Box from "@material-ui/core/Box"; import Button from "@material-ui/core/Button"; import Card from "@material-ui/core/Card"; import CardContent from "@material-ui/core/CardContent"; @@ -26,6 +27,7 @@ import Navbar from "../components/navbar/navbar"; import Divider from "@material-ui/core/Divider"; import Link from "@material-ui/core/Link"; import PasswordChangePanel from "../components/password_change_panel"; +import TwoFactorSection from "../components/two_factor_section"; import LoadingIndicator from "../components/loading_indicator"; import TopLogo from "../components/top_logo/top_logo"; import {handleErrorResponse} from "../utils"; @@ -41,7 +43,11 @@ const ProfilePage = ({ languages, menuConfig, passwordPolicy, - redirectUri + redirectUri, + twoFactorEnabled, + recoveryCodesRemaining, + recoveryCodesTotal, + recoveryCodesLowThreshold }) => { const [pic, setPic] = useState(null); const [loading, setLoading] = useState(false); @@ -754,11 +760,26 @@ const ProfilePage = ({ )} - - + + + + + + + Two-Factor Authentication + + + + { return res; } +export const downloadTextFile = (filename, content) => { + const blob = new Blob([content], {type: 'text/plain'}); + const url = URL.createObjectURL(blob); + const link = document.createElement('a'); + link.href = url; + link.download = filename; + document.body.appendChild(link); + link.click(); + document.body.removeChild(link); + URL.revokeObjectURL(url); +}; + export const decodeHtmlEntities = (text) => { const textarea = document.createElement('textarea'); textarea.innerHTML = text; diff --git a/resources/views/profile.blade.php b/resources/views/profile.blade.php index 335094e7..672081ba 100644 --- a/resources/views/profile.blade.php +++ b/resources/views/profile.blade.php @@ -103,7 +103,11 @@ initialValues: initialValues, languages: languages, menuConfig: menuConfig, - passwordPolicy: passwordPolicy + passwordPolicy: passwordPolicy, + twoFactorEnabled: {{ $two_factor_enabled ? 'true' : 'false' }}, + recoveryCodesRemaining: {{ (int) $recovery_codes_remaining }}, + recoveryCodesTotal: {{ (int) $recovery_codes_total }}, + recoveryCodesLowThreshold: {{ (int) $recovery_codes_low_threshold }} } window.GET_USER_ACTIONS_ENDPOINT = '{{URL::action("Api\UserActionApiController@getActionsByCurrentUser")}}'; @@ -112,6 +116,8 @@ window.REVOKE_ALL_TOKENS_ENDPOINT = '{{URL::action("Api\UserApiController@revokeAllMyTokens")}}'; window.SAVE_PROFILE_ENDPOINT = '{!!URL::action("Api\UserApiController@updateMe")!!}'; 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.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 ecbeda35..61d97573 100644 --- a/routes/web.php +++ b/routes/web.php @@ -197,6 +197,8 @@ Route::put('', "UserApiController@updateMe"); Route::put('pic', "UserApiController@updateMyPic"); Route::get('actions', "UserActionApiController@getActionsByCurrentUser"); + Route::post('recovery-codes/regenerate', "UserApiController@regenerateRecoveryCodes"); + Route::post('2fa/enable', "UserApiController@enableTwoFactor"); }); Route::get('access-tokens', ['middleware' => ['openstackid.currentuser.serveradmin.json'], 'uses' => 'ClientApiController@getAllAccessTokens']); diff --git a/tests/RecoveryCodeRegenerationTest.php b/tests/RecoveryCodeRegenerationTest.php new file mode 100644 index 00000000..f878e972 --- /dev/null +++ b/tests/RecoveryCodeRegenerationTest.php @@ -0,0 +1,187 @@ +withoutMiddleware(); + $this->be($this->admin()); + Session::start(); + } + + public function testRegenerateWithCorrectPasswordInvalidatesOldCodesAndReturnsNewOnes(): void + { + $admin = $this->admin(); + $this->createRecoveryCode($admin, 'OLD-CODE-' . uniqid(), false); + + $response = $this->regenerate(self::SEED_PASSWORD); + + $this->assertResponseStatus(200); + $payload = json_decode($response->getContent(), true); + $this->assertArrayHasKey('recovery_codes', $payload); + + $expectedCount = (int)config('auth.recovery_codes.count', 10); + $this->assertCount($expectedCount, $payload['recovery_codes']); + foreach ($payload['recovery_codes'] as $code) { + $this->assertMatchesRegularExpression('/^[A-Z0-9]+-[A-Z0-9]+$/', $code); + } + + EntityManager::clear(); + $remaining = EntityManager::getRepository(UserRecoveryCode::class) + ->findBy(['user' => $admin->getId(), 'used_at' => null]); + $this->assertCount( + $expectedCount, + $remaining, + 'old codes must be invalidated and replaced by exactly the configured count' + ); + } + + public function testRegenerateWithWrongPasswordFailsAndDoesNotTouchExistingCodes(): void + { + $admin = $this->admin(); + $plain = 'KEEP-ME-' . uniqid(); + $this->createRecoveryCode($admin, $plain, false); + + $response = $this->regenerate('this-is-not-the-password'); + + $this->assertResponseStatus(412); + + EntityManager::clear(); + $remaining = EntityManager::getRepository(UserRecoveryCode::class) + ->findBy(['user' => $admin->getId(), 'used_at' => null]); + $this->assertNotEmpty($remaining, 'existing codes must not be touched when the password confirmation fails'); + } + + public function testRegenerateRequiresCurrentPassword(): void + { + $response = $this->action('POST', 'Api\\UserApiController@regenerateRecoveryCodes', [], [], [], []); + + $this->assertResponseStatus(412); + } + + public function testRegenerateLogsAuditEvent(): void + { + $admin = $this->admin(); + + $this->regenerate(self::SEED_PASSWORD); + + EntityManager::clear(); + $entries = EntityManager::getRepository(TwoFactorAuditLog::class) + ->findBy(['user' => $admin->getId(), 'event_type' => TwoFactorAuditLog::EventRecoveryCodesGenerated]); + $this->assertNotEmpty($entries, 'a recovery_codes_generated audit entry must be recorded'); + } + + public function testEnableTwoFactorGeneratesRecoveryCodes(): void + { + $admin = $this->admin(); + + $response = $this->enableTwoFactor('email_otp'); + + $this->assertResponseStatus(200); + $payload = json_decode($response->getContent(), true); + $this->assertArrayHasKey('recovery_codes', $payload); + + $expectedCount = (int)config('auth.recovery_codes.count', 10); + $this->assertCount($expectedCount, $payload['recovery_codes']); + + EntityManager::clear(); + $reloaded = EntityManager::getRepository(User::class)->find($admin->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]); + $this->assertCount($expectedCount, $remaining); + } + + 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'); + + $this->assertResponseStatus(412); + } + + public function testEnableTwoFactorRequiresMethod(): void + { + $response = $this->action('POST', 'Api\\UserApiController@enableTwoFactor', [], [], [], []); + + $this->assertResponseStatus(412); + } + + public function testEnableTwoFactorLogsEnrollmentAuditEvent(): void + { + $admin = $this->admin(); + + $this->enableTwoFactor('email_otp'); + + EntityManager::clear(); + $entries = EntityManager::getRepository(TwoFactorAuditLog::class) + ->findBy(['user' => $admin->getId(), 'event_type' => TwoFactorAuditLog::EventEnrollmentChanged]); + $this->assertNotEmpty($entries, 'an enrollment_changed audit entry must be recorded'); + } + + private function enableTwoFactor(string $method) + { + return $this->action('POST', 'Api\\UserApiController@enableTwoFactor', [ + 'method' => $method, + ], [], [], []); + } + + private function admin(): User + { + $user = EntityManager::getRepository(User::class)->findOneBy(['identifier' => self::ADMIN_IDENTIFIER]); + $this->assertInstanceOf(User::class, $user, 'seeded admin user not found'); + return $user; + } + + private function regenerate(string $password) + { + return $this->action('POST', 'Api\\UserApiController@regenerateRecoveryCodes', [ + 'current_password' => $password, + ], [], [], []); + } + + private function createRecoveryCode(User $user, string $plain, bool $used): int + { + $code = new UserRecoveryCode(); + $code->setUser($user); + $code->setCodeHash(Hash::make($plain)); + if ($used) { + $code->markUsed(); + } + EntityManager::persist($code); + EntityManager::flush(); + return $code->getId(); + } +} diff --git a/tests/js/components/recovery_code_display.test.js b/tests/js/components/recovery_code_display.test.js new file mode 100644 index 00000000..03ff60a4 --- /dev/null +++ b/tests/js/components/recovery_code_display.test.js @@ -0,0 +1,53 @@ +import React from 'react'; +import {render, screen, fireEvent} from '@testing-library/react'; +import RecoveryCodeDisplay from '../../../resources/js/components/recovery_code_display'; + +const CODES = ['AAAA-1111', 'BBBB-2222', 'CCCC-3333']; + +describe('RecoveryCodeDisplay', () => { + beforeEach(() => { + Object.assign(navigator, { + clipboard: { + writeText: jest.fn().mockResolvedValue(undefined), + }, + }); + global.URL.createObjectURL = jest.fn(() => 'blob:mock-url'); + global.URL.revokeObjectURL = jest.fn(); + }); + + it('renders every recovery code', () => { + render(); + + CODES.forEach((code) => { + expect(screen.getByText(code)).toBeInTheDocument(); + }); + }); + + it('copies all codes as plain text to the clipboard', async () => { + render(); + + fireEvent.click(screen.getByTestId('copy-codes-button')); + + expect(navigator.clipboard.writeText).toHaveBeenCalledWith(CODES.join('\n')); + }); + + it('triggers a text file download containing the codes', () => { + const clickSpy = jest.fn(); + const originalCreateElement = document.createElement.bind(document); + const createElementSpy = jest.spyOn(document, 'createElement').mockImplementation((tag) => { + const el = originalCreateElement(tag); + if (tag === 'a') { + el.click = clickSpy; + } + return el; + }); + + render(); + fireEvent.click(screen.getByTestId('download-codes-button')); + + expect(global.URL.createObjectURL).toHaveBeenCalled(); + expect(clickSpy).toHaveBeenCalled(); + + createElementSpy.mockRestore(); + }); +}); diff --git a/tests/js/components/recovery_code_modal.test.js b/tests/js/components/recovery_code_modal.test.js new file mode 100644 index 00000000..8582e7a0 --- /dev/null +++ b/tests/js/components/recovery_code_modal.test.js @@ -0,0 +1,60 @@ +import React from 'react'; +import {render, screen, fireEvent, act} from '@testing-library/react'; +import RecoveryCodeModal from '../../../resources/js/components/recovery_code_modal'; + +const CODES = ['AAAA-1111', 'BBBB-2222']; + +describe('RecoveryCodeModal', () => { + beforeEach(() => { + jest.useFakeTimers(); + }); + + afterEach(() => { + jest.useRealTimers(); + }); + + it('renders the title, warning banner and codes when open', () => { + render(); + + expect(screen.getByText('Save Your Recovery Codes')).toBeInTheDocument(); + expect(screen.getByTestId('recovery-code-warning')).toBeInTheDocument(); + CODES.forEach((code) => expect(screen.getByText(code)).toBeInTheDocument()); + }); + + it('keeps the acknowledge button disabled until the 5 second delay elapses', () => { + const onAcknowledge = jest.fn(); + render(); + + const ackButton = screen.getByTestId('acknowledge-codes-button'); + expect(ackButton).toBeDisabled(); + + act(() => { + jest.advanceTimersByTime(4000); + }); + expect(ackButton).toBeDisabled(); + + act(() => { + jest.advanceTimersByTime(1000); + }); + expect(ackButton).not.toBeDisabled(); + + fireEvent.click(ackButton); + expect(onAcknowledge).toHaveBeenCalledTimes(1); + }); + + it('cannot be dismissed by pressing Escape', () => { + const onAcknowledge = jest.fn(); + render(); + + fireEvent.keyDown(screen.getByRole('dialog'), {key: 'Escape', code: 'Escape', keyCode: 27}); + + expect(screen.getByText('Save Your Recovery Codes')).toBeInTheDocument(); + expect(onAcknowledge).not.toHaveBeenCalled(); + }); + + it('renders nothing sensitive when there are no codes yet (closed state)', () => { + render(); + + expect(screen.queryByText('Save Your Recovery Codes')).not.toBeInTheDocument(); + }); +}); diff --git a/tests/js/components/recovery_codes_panel.test.js b/tests/js/components/recovery_codes_panel.test.js new file mode 100644 index 00000000..6f1b8392 --- /dev/null +++ b/tests/js/components/recovery_codes_panel.test.js @@ -0,0 +1,105 @@ +import React from 'react'; +import {render, screen, fireEvent} from '@testing-library/react'; +import RecoveryCodesPanel from '../../../resources/js/components/recovery_codes_panel'; +import {regenerateRecoveryCodes} from '../../../resources/js/profile/actions'; + +jest.mock('../../../resources/js/profile/actions'); +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'); + +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'); + }); + + 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(); + }); + + 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 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('opens the modal immediately when initialCodes is provided', () => { + const codes = ['AAAA-1111', 'BBBB-2222']; + render( + + ); + expect(screen.getByText('AAAA-1111')).toBeInTheDocument(); + }); + + 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}}); + + render( + + ); + + 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(); + expect(screen.getByTestId('recovery-codes-count')).toHaveTextContent('Recovery Codes: 2 of 2 remaining'); + }); + + 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 new Promise((resolve) => setImmediate(resolve)); + expect(screen.getByTestId('recovery-codes-count')).toHaveTextContent('Recovery Codes: 7 of 10 remaining'); + }); +}); diff --git a/tests/js/components/two_factor_section.test.js b/tests/js/components/two_factor_section.test.js new file mode 100644 index 00000000..e5e9dfef --- /dev/null +++ b/tests/js/components/two_factor_section.test.js @@ -0,0 +1,48 @@ +import React from 'react'; +import {render, screen, fireEvent} from '@testing-library/react'; +import TwoFactorSection from '../../../resources/js/components/two_factor_section'; +import {enableTwoFactor} from '../../../resources/js/profile/actions'; + +jest.mock('../../../resources/js/profile/actions'); +jest.mock('sweetalert2', () => jest.fn()); + +describe('TwoFactorSection', () => { + beforeEach(() => { + window.sessionStorage.clear(); + jest.clearAllMocks(); + }); + + it('shows the enable button when 2FA is not enabled', () => { + render( + + ); + expect(screen.getByTestId('enable-two-factor-button')).toBeInTheDocument(); + expect(screen.queryByTestId('recovery-codes-count')).not.toBeInTheDocument(); + }); + + it('shows the recovery codes panel when 2FA is already enabled', () => { + render( + + ); + 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 () => { + const codes = ['AAAA-1111', 'BBBB-2222']; + enableTwoFactor.mockResolvedValue({response: {recovery_codes: codes}}); + + render( + + ); + + fireEvent.click(screen.getByTestId('enable-two-factor-button')); + + 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'); + }); +}); diff --git a/tests/js/setup.js b/tests/js/setup.js index 7b0828bf..dfc57086 100644 --- a/tests/js/setup.js +++ b/tests/js/setup.js @@ -1 +1,12 @@ import '@testing-library/jest-dom'; +import {TextEncoder, TextDecoder} from 'util'; + +// jsdom does not provide these globals; superagent (via base_actions.js) needs +// them transitively (through @paralleldrive/cuid2), even when the module ends +// up auto-mocked by jest.mock() — Jest still evaluates the real module once. +if (typeof global.TextEncoder === 'undefined') { + global.TextEncoder = TextEncoder; +} +if (typeof global.TextDecoder === 'undefined') { + global.TextDecoder = TextDecoder; +} diff --git a/yarn.lock b/yarn.lock index 5c8cb8e6..0b5e712a 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1792,7 +1792,7 @@ lz-string "^1.5.0" pretty-format "^27.0.2" -"@testing-library/jest-dom@^5": +"@testing-library/jest-dom@^5.16.5": version "5.17.0" resolved "https://registry.yarnpkg.com/@testing-library/jest-dom/-/jest-dom-5.17.0.tgz#5e97c8f9a15ccf4656da00fecab505728de81e0c" integrity sha512-ynmNeT7asXyH3aSVv4vvX4Rb+0qjOhdNHnO/3vuZNqPmhDpV/+rCSGwQ7bLcmU2cJ4dvoheIO85LQj0IbJHEtg== @@ -1807,7 +1807,7 @@ lodash "^4.17.15" redent "^3.0.0" -"@testing-library/react@^12": +"@testing-library/react@^12.1.5": version "12.1.5" resolved "https://registry.yarnpkg.com/@testing-library/react/-/react-12.1.5.tgz#bb248f72f02a5ac9d949dea07279095fa577963b" integrity sha512-OfTXCJUFgjd/digLUuPxa0+/3ZxsQmE7ub9kcbW/wi96Bh3o/p5vrETcBGfP17NWPGqeYYl5LTRpwyGoMC4ysg== From cf6e2a7d262758fe2d86749ec90c9d7fd0667d96 Mon Sep 17 00:00:00 2001 From: romanetar Date: Tue, 11 Aug 2026 16:26:40 +0200 Subject: [PATCH 02/12] fix: add missing postRawRequestFull to base_actions.js profile/actions.js imports postRawRequestFull for the new enableTwoFactor and regenerateRecoveryCodes flows, but it was never exported, causing a runtime TypeError on both actions. Falling back to postRawRequest is unsafe here since it copies params into the URL query string, which would leak current_password into access logs. --- resources/js/base_actions.js | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/resources/js/base_actions.js b/resources/js/base_actions.js index 18cd3b49..e6c1ea06 100644 --- a/resources/js/base_actions.js +++ b/resources/js/base_actions.js @@ -92,6 +92,30 @@ export const postRawRequest = (endpoint) => (params, headers = {}) => { }) } +export const postRawRequestFull = (endpoint) => (params, headers = {}) => { + let url = URI(endpoint); + + let key = url.toString(); + + cancel(key); + + let req = http.post(url.toString()); + + schedule(key, req); + + return req.set(headers).send(params).timeout({ + response: 60000, + deadline: 60000, + }).then((res) => { + let json = res.body; + end(key); + return Promise.resolve({response: json}); + }).catch((error) => { + end(key); + return Promise.reject(error); + }) +} + export const putRawRequest = (endpoint) => (payload = null, params={}, headers = {}) => { let url = URI(endpoint); From b2d84d1f34a85ddeb932819532fef3b4807a0f86 Mon Sep 17 00:00:00 2001 From: romanetar Date: Tue, 11 Aug 2026 16:30:26 +0200 Subject: [PATCH 03/12] fix: reject enableTwoFactor when 2FA is already enabled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit enable2FA() had no already-enrolled guard, so a second POST to /2fa/enable for an enrolled user silently regenerated recovery codes with no password confirmation, bypassing the password-gated rotation flow required by CU-86ba2zp66 and sds/idp-mfa.md §4.10.3. --- app/Http/Controllers/Api/UserApiController.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/Http/Controllers/Api/UserApiController.php b/app/Http/Controllers/Api/UserApiController.php index 78b051ba..639b6a38 100644 --- a/app/Http/Controllers/Api/UserApiController.php +++ b/app/Http/Controllers/Api/UserApiController.php @@ -302,6 +302,10 @@ 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.']]); + } + $codes = $this->tx_service->transaction(function () use ($user, $method) { $user->enable2FA($method); $this->repository->add($user, false); From 7fbff848c0802bdc2641dbd43ef55156290d3e10 Mon Sep 17 00:00:00 2001 From: romanetar Date: Tue, 11 Aug 2026 16:35:21 +0200 Subject: [PATCH 04/12] refactor: move 2FA enrollment orchestration into RecoveryCodeService The transaction plus enable2FA + repository->add + code generation lived in UserApiController, breaking the thin-controllers/fat-services convention and diverging from the regenerateRecoveryCodes path, which already delegates to the service. UserApiController::enableTwoFactor now only validates input and calls RecoveryCodeService::enableTwoFactorAndGenerateCodes. --- .../Controllers/Api/UserApiController.php | 36 ++----------------- app/Services/Auth/IRecoveryCodeService.php | 12 +++++++ app/Services/Auth/RecoveryCodeService.php | 24 +++++++++++++ 3 files changed, 38 insertions(+), 34 deletions(-) diff --git a/app/Http/Controllers/Api/UserApiController.php b/app/Http/Controllers/Api/UserApiController.php index 639b6a38..b334959b 100644 --- a/app/Http/Controllers/Api/UserApiController.php +++ b/app/Http/Controllers/Api/UserApiController.php @@ -15,10 +15,8 @@ use App\Http\Controllers\APICRUDController; use App\Http\Controllers\Traits\RequestProcessor; use App\Http\Controllers\UserValidationRulesFactory; -use App\libs\Auth\Models\TwoFactorAuditLog; use App\ModelSerializers\SerializerRegistry; use App\Services\Auth\IRecoveryCodeService; -use App\Services\Auth\ITwoFactorAuditService; use Auth\Repositories\IUserRepository; use Auth\User; use Exception; @@ -31,8 +29,6 @@ use models\exceptions\ValidationException; use OAuth2\Services\ITokenService; use OpenId\Services\IUserService; -use Utils\Db\ITransactionService; -use Utils\IPHelper; use Utils\Services\ILogService; /** @@ -54,16 +50,6 @@ final class UserApiController extends APICRUDController */ private $recovery_code_service; - /** - * @var ITransactionService - */ - private $tx_service; - - /** - * @var ITwoFactorAuditService - */ - private $two_factor_audit_service; - /** * UserApiController constructor. * @param IUserRepository $user_repository @@ -71,8 +57,6 @@ final class UserApiController extends APICRUDController * @param IUserService $user_service * @param ITokenService $token_service * @param IRecoveryCodeService $recovery_code_service - * @param ITransactionService $tx_service - * @param ITwoFactorAuditService $two_factor_audit_service */ public function __construct ( @@ -80,16 +64,12 @@ public function __construct ILogService $log_service, IUserService $user_service, ITokenService $token_service, - IRecoveryCodeService $recovery_code_service, - ITransactionService $tx_service, - ITwoFactorAuditService $two_factor_audit_service + IRecoveryCodeService $recovery_code_service ) { parent::__construct($user_repository, $user_service, $log_service); $this->token_service = $token_service; $this->recovery_code_service = $recovery_code_service; - $this->tx_service = $tx_service; - $this->two_factor_audit_service = $two_factor_audit_service; } /** @@ -306,19 +286,7 @@ public function enableTwoFactor() return $this->error412(['method' => ['Two-factor authentication is already enabled. Use the regenerate recovery codes endpoint to rotate your codes.']]); } - $codes = $this->tx_service->transaction(function () use ($user, $method) { - $user->enable2FA($method); - $this->repository->add($user, false); - - return $this->recovery_code_service->generateRecoveryCodes($user); - }); - - $this->two_factor_audit_service->log( - $user, - TwoFactorAuditLog::EventEnrollmentChanged, - $method, - IPHelper::getUserIp() - ); + $codes = $this->recovery_code_service->enableTwoFactorAndGenerateCodes($user, $method); return $this->ok(['recovery_codes' => $codes]); }); diff --git a/app/Services/Auth/IRecoveryCodeService.php b/app/Services/Auth/IRecoveryCodeService.php index 56c9d464..51df966b 100644 --- a/app/Services/Auth/IRecoveryCodeService.php +++ b/app/Services/Auth/IRecoveryCodeService.php @@ -44,6 +44,18 @@ public function regenerateRecoveryCodes(User $user, string $currentPassword): ar */ public function generateRecoveryCodes(User $user): array; + /** + * Enrolls the user into the given 2FA method and generates the first batch + * of recovery codes for them, without requiring password confirmation. + * Intended for enrollment via an already-authenticated session. + * + * @param User $user + * @param string $method + * @return string[] plaintext codes formatted as XXXX-XXXX + * @throws ValidationException if $method is not a valid/enabled 2FA method + */ + public function enableTwoFactorAndGenerateCodes(User $user, string $method): array; + /** * @param User $user * @return int count of unused recovery codes diff --git a/app/Services/Auth/RecoveryCodeService.php b/app/Services/Auth/RecoveryCodeService.php index 1d2c1efc..f5e67c0c 100644 --- a/app/Services/Auth/RecoveryCodeService.php +++ b/app/Services/Auth/RecoveryCodeService.php @@ -16,6 +16,7 @@ use App\libs\Auth\Models\TwoFactorAuditLog; use App\libs\Auth\Models\UserRecoveryCode; use Auth\Repositories\IUserRecoveryCodeRepository; +use Auth\Repositories\IUserRepository; use Auth\User; use Illuminate\Support\Facades\Hash; use Laminas\Math\Rand; @@ -33,6 +34,7 @@ final class RecoveryCodeService implements IRecoveryCodeService public function __construct( private readonly IUserRecoveryCodeRepository $repository, + private readonly IUserRepository $user_repository, private readonly ITransactionService $tx_service, private readonly ITwoFactorAuditService $audit_service, ) { @@ -84,6 +86,28 @@ public function generateRecoveryCodes(User $user): array return array_map(static fn(string $code) => implode('-', str_split($code, 4)), $plaintext_codes); } + /** + * @inheritDoc + */ + public function enableTwoFactorAndGenerateCodes(User $user, string $method): array + { + $codes = $this->tx_service->transaction(function () use ($user, $method) { + $user->enable2FA($method); + $this->user_repository->add($user, false); + + return $this->generateRecoveryCodes($user); + }); + + $this->audit_service->log( + $user, + TwoFactorAuditLog::EventEnrollmentChanged, + $method, + IPHelper::getUserIp() + ); + + return $codes; + } + /** * @inheritDoc */ From b729e552eaea21a02ef39263e91bdef782a2a9e2 Mon Sep 17 00:00:00 2001 From: romanetar Date: Tue, 11 Aug 2026 16:37:49 +0200 Subject: [PATCH 05/12] fix: normalize recovery code server-side before hash check Hash::check() compared the raw submitted code against the dash-less uppercase hash, so the "strip separators + uppercase" contract was only enforced by the login.js client. Any other consumer submitting a code exactly as displayed (XXXX-XXXX) would fail verification on this lockout-critical path. Apply the same normalization in AbstractMFAChallengeStrategy::verifyRecoveryCode() before Hash::check. --- app/Strategies/MFA/AbstractMFAChallengeStrategy.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/Strategies/MFA/AbstractMFAChallengeStrategy.php b/app/Strategies/MFA/AbstractMFAChallengeStrategy.php index c2d04fea..95da987e 100644 --- a/app/Strategies/MFA/AbstractMFAChallengeStrategy.php +++ b/app/Strategies/MFA/AbstractMFAChallengeStrategy.php @@ -48,6 +48,11 @@ public function clearPendingState(): void public function verifyRecoveryCode(User $user, string $code): void { + // Recovery codes are hashed without the "-" separator; it is added only + // for on-screen readability (XXXX-XXXX). Normalize here so a code typed + // or pasted exactly as displayed still matches the stored hash. + $code = strtoupper(preg_replace('/[^A-Za-z0-9]/', '', $code)); + 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 From bcd03304cf4fa02c0ad48cceba89369940dcf82a Mon Sep 17 00:00:00 2001 From: romanetar Date: Tue, 11 Aug 2026 16:46:37 +0200 Subject: [PATCH 06/12] feat: warn on low recovery codes after MFA recovery login MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CU-86ba2zp66 / sds/idp-mfa.md §4.10.3, §4.11 step 5 require a dismissable low-code warning after a successful MFA login, but it was only wired into the profile page - a user who burns codes at login never saw it unless they happened to visit their profile. verify2FARecovery now returns recovery_codes_remaining and the configured low threshold; login.js holds the post-login redirect and shows a dismissable banner when the count is low, before navigating away. The sessionStorage dismissal key is shared with the profile page's RecoveryCodesPanel via a new shared module so dismissing in either place suppresses it everywhere for the rest of the session. --- app/Http/Controllers/UserController.php | 9 ++- .../js/components/recovery_codes_panel.js | 13 ++-- resources/js/login/login.js | 59 ++++++++++++++++++- resources/js/shared/recovery_codes.js | 5 ++ 4 files changed, 76 insertions(+), 10 deletions(-) create mode 100644 resources/js/shared/recovery_codes.js diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index 00ddf724..97a54674 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -927,7 +927,14 @@ public function verify2FARecovery() // See verify2FA() for rationale: return the destination as data so a real // top-level navigation (not this XHR) performs any cross-origin hop. $redirect = $this->login_strategy->postLogin(); - return $this->ok(['redirect_url' => $redirect->getTargetUrl()]); + return $this->ok([ + 'redirect_url' => $redirect->getTargetUrl(), + // CU-86ba2zp66 / sds/idp-mfa.md §4.10.3, §4.11 step 5: the login page + // must be able to warn the user when they've just burned into their + // last few recovery codes, since it may be their only way back in. + 'recovery_codes_remaining' => $this->recovery_code_service->countUnusedRecoveryCodes($user), + 'recovery_codes_low_threshold' => (int) config('auth.recovery_codes.low_threshold', 3), + ]); } catch (ValidationException $ex) { Log::warning($ex); return $this->error412($ex->getMessages()); diff --git a/resources/js/components/recovery_codes_panel.js b/resources/js/components/recovery_codes_panel.js index 9b8bf9f6..ae9b22a1 100644 --- a/resources/js/components/recovery_codes_panel.js +++ b/resources/js/components/recovery_codes_panel.js @@ -10,16 +10,17 @@ import CloseIcon from "@material-ui/icons/Close"; import {regenerateRecoveryCodes} from "../profile/actions"; import {handleErrorResponse} from "../utils"; import RecoveryCodeModal from "./recovery_code_modal"; +import { + RECOVERY_CODES_LOW_WARNING_DISMISSED_KEY, + DEFAULT_RECOVERY_CODES_LOW_THRESHOLD, +} from "../shared/recovery_codes"; import styles from "./recovery_codes.module.scss"; -const DEFAULT_LOW_CODE_THRESHOLD = 3; -const LOW_CODE_WARNING_DISMISSED_KEY = "recovery_codes_low_warning_dismissed"; - const RecoveryCodesPanel = ({ recoveryCodesRemaining, recoveryCodesTotal, - lowCodeThreshold = DEFAULT_LOW_CODE_THRESHOLD, + lowCodeThreshold = DEFAULT_RECOVERY_CODES_LOW_THRESHOLD, email, initialCodes = null }) => { @@ -30,7 +31,7 @@ const RecoveryCodesPanel = ({ const [total, setTotal] = useState(recoveryCodesTotal); const [codes, setCodes] = useState(initialCodes); const [warningDismissed, setWarningDismissed] = useState( - sessionStorage.getItem(LOW_CODE_WARNING_DISMISSED_KEY) === "1" + sessionStorage.getItem(RECOVERY_CODES_LOW_WARNING_DISMISSED_KEY) === "1" ); const handleRegenerate = () => { @@ -53,7 +54,7 @@ const RecoveryCodesPanel = ({ }; const dismissLowCodeWarning = () => { - sessionStorage.setItem(LOW_CODE_WARNING_DISMISSED_KEY, "1"); + sessionStorage.setItem(RECOVERY_CODES_LOW_WARNING_DISMISSED_KEY, "1"); setWarningDismissed(true); }; diff --git a/resources/js/login/login.js b/resources/js/login/login.js index 623279c6..3aa2100c 100644 --- a/resources/js/login/login.js +++ b/resources/js/login/login.js @@ -31,8 +31,13 @@ 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 { + RECOVERY_CODES_LOW_WARNING_DISMISSED_KEY, + DEFAULT_RECOVERY_CODES_LOW_THRESHOLD, +} from "../shared/recovery_codes"; import styles from "./login.module.scss"; +import recoveryCodesStyles from "../components/recovery_codes.module.scss"; import "./third_party_identity_providers.scss"; import { FLOW, @@ -93,6 +98,11 @@ class LoginPage extends React.Component { // change writes otp_lifetime atomically with flow, so props.otpLifetime // is only ever missing when there's no pending OTP to show a countdown for. passwordlessLifetime: props.otpLifetime ?? null, + // Set once a recovery-code login succeeds with a low remaining count, so + // the redirect can be held until the user acknowledges the warning - + // this is the only point where the SPA still controls the page (see + // onVerifyRecovery()/onContinueAfterLowRecoveryCodes() below). + lowRecoveryCodesWarning: null, }; if (props.authError != "" && !this.state.user_fullname) { @@ -130,6 +140,7 @@ class LoginPage extends React.Component { this.onVerify2FA = this.onVerify2FA.bind(this); this.onResend2FA = this.onResend2FA.bind(this); this.onVerifyRecovery = this.onVerifyRecovery.bind(this); + this.onContinueAfterLowRecoveryCodes = this.onContinueAfterLowRecoveryCodes.bind(this); this.onUseRecovery = this.onUseRecovery.bind(this); this.onBackToOtp = this.onBackToOtp.bind(this); this.resetToPasswordFlow = this.resetToPasswordFlow.bind(this); @@ -540,10 +551,26 @@ class LoginPage extends React.Component { verifyRecoveryCode(recoveryCode, this.props.token).then( (payload) => { - // See onVerify2FA() for rationale. const { response } = payload; - window.location.href = + const redirectUrl = (response && response.redirect_url) || window.location.href; + const remaining = response && response.recovery_codes_remaining; + const threshold = + (response && response.recovery_codes_low_threshold) ?? + DEFAULT_RECOVERY_CODES_LOW_THRESHOLD; + const alreadyDismissed = + sessionStorage.getItem(RECOVERY_CODES_LOW_WARNING_DISMISSED_KEY) === "1"; + + if (typeof remaining === "number" && remaining < threshold && !alreadyDismissed) { + this.setState({ + ...this.state, + lowRecoveryCodesWarning: { remaining, redirectUrl }, + }); + return; + } + + // See onVerify2FA() for rationale on using a real top-level navigation. + window.location.href = redirectUrl; }, (error) => { this.handleMfaError(error, "recovery"); @@ -551,6 +578,11 @@ class LoginPage extends React.Component { ); } + onContinueAfterLowRecoveryCodes() { + sessionStorage.setItem(RECOVERY_CODES_LOW_WARNING_DISMISSED_KEY, "1"); + window.location.href = this.state.lowRecoveryCodesWarning.redirectUrl; + } + onUseRecovery() { this.setState({ ...this.state, @@ -833,7 +865,7 @@ class LoginPage extends React.Component { onVerify={this.onVerify2FA} /> )} - {showRecoveryForm && ( + {showRecoveryForm && !this.state.lowRecoveryCodesWarning && ( )} + {showRecoveryForm && this.state.lowRecoveryCodesWarning && ( +
+ + You have {this.state.lowRecoveryCodesWarning.remaining} recovery + code{this.state.lowRecoveryCodesWarning.remaining === 1 ? "" : "s"} left. + Regenerate them from your profile after signing in to avoid getting + locked out. + + +
+ )} {isPasswordFlow && ( // proceed to ask for password ( 2nd step )
diff --git a/resources/js/shared/recovery_codes.js b/resources/js/shared/recovery_codes.js new file mode 100644 index 00000000..c144f52b --- /dev/null +++ b/resources/js/shared/recovery_codes.js @@ -0,0 +1,5 @@ +// Shared between the profile page's RecoveryCodesPanel and the login page's +// post-MFA-recovery-login warning, so dismissing the low-code warning in +// either place suppresses it everywhere else for the rest of the session. +export const RECOVERY_CODES_LOW_WARNING_DISMISSED_KEY = "recovery_codes_low_warning_dismissed"; +export const DEFAULT_RECOVERY_CODES_LOW_THRESHOLD = 3; From 89e058c71f273eeca1cb8d46ec9e527a64073f53 Mon Sep 17 00:00:00 2001 From: romanetar Date: Tue, 11 Aug 2026 16:53:42 +0200 Subject: [PATCH 07/12] test: cover recovery-code redemption, re-enrollment, and the real request layer Three gaps mapped to the riskiest parts of this PR were unpinned: 1. Nothing proved a code returned as XXXX-XXXX actually redeems through AbstractMFAChallengeStrategy::verifyRecoveryCode() - the hash is of the dash-less string, so the generate->display->redeem contract (including the dash normalization) was untested. 2. enableTwoFactor()'s already-enrolled guard (412) had no regression test. 3. Every JS test mocked profile/actions, so nothing exercised the real request layer - exactly where the missing postRawRequestFull export lived. tests/js/profile/actions.test.js only stubs the transport (superagent) and calls the real enableTwoFactor/regenerateRecoveryCodes; verified it reproduces the original "postRawRequestFull is not a function" TypeError when that export is removed. --- tests/RecoveryCodeRegenerationTest.php | 46 +++++++++++++++++++++ tests/js/profile/actions.test.js | 57 ++++++++++++++++++++++++++ 2 files changed, 103 insertions(+) create mode 100644 tests/js/profile/actions.test.js diff --git a/tests/RecoveryCodeRegenerationTest.php b/tests/RecoveryCodeRegenerationTest.php index f878e972..0a3a4b4e 100644 --- a/tests/RecoveryCodeRegenerationTest.php +++ b/tests/RecoveryCodeRegenerationTest.php @@ -18,6 +18,7 @@ use Illuminate\Support\Facades\Hash; use Illuminate\Support\Facades\Session; use LaravelDoctrine\ORM\Facades\EntityManager; +use Strategies\MFA\MFAChallengeStrategyFactory; /** * Integration tests for regenerating recovery codes from the user profile @@ -123,6 +124,51 @@ public function testEnableTwoFactorGeneratesRecoveryCodes(): void $this->assertCount($expectedCount, $remaining); } + public function testDisplayedRecoveryCodeRedeemsThroughVerifyRecoveryCode(): void + { + $admin = $this->admin(); + + $response = $this->regenerate(self::SEED_PASSWORD); + $this->assertResponseStatus(200); + $payload = json_decode($response->getContent(), true); + $displayedCode = $payload['recovery_codes'][0]; + $this->assertMatchesRegularExpression('/^[A-Z0-9]+-[A-Z0-9]+$/', $displayedCode); + + EntityManager::clear(); + $admin = EntityManager::getRepository(User::class)->find($admin->getId()); + $unusedBefore = EntityManager::getRepository(UserRecoveryCode::class) + ->findBy(['user' => $admin->getId(), 'used_at' => null]); + + // The hash was generated over the dash-less string; redeeming the code + // exactly as it was displayed (with its "-" separator) must still work. + $strategy = MFAChallengeStrategyFactory::create(User::MFAMethod_OTP); + $strategy->verifyRecoveryCode($admin, $displayedCode); + + EntityManager::clear(); + $unusedAfter = EntityManager::getRepository(UserRecoveryCode::class) + ->findBy(['user' => $admin->getId(), 'used_at' => null]); + + $this->assertCount( + count($unusedBefore) - 1, + $unusedAfter, + 'the code redeemed exactly as displayed must be consumed exactly once' + ); + } + + public function testEnableTwoFactorRejectsWhenAlreadyEnabled(): void + { + $this->enableTwoFactor('email_otp'); + $this->assertResponseStatus(200); + + $response = $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'); + } + public function testEnableTwoFactorRejectsUnavailableMethod(): void { // sms_otp is a stub in Phase I (isPhoneNumberVerified() is hardcoded false), diff --git a/tests/js/profile/actions.test.js b/tests/js/profile/actions.test.js new file mode 100644 index 00000000..a07af381 --- /dev/null +++ b/tests/js/profile/actions.test.js @@ -0,0 +1,57 @@ +// Unlike the RecoveryCodesPanel/TwoFactorSection tests, this suite does NOT +// mock "profile/actions" - it exercises the real enableTwoFactor()/ +// regenerateRecoveryCodes() against the real base_actions.js request layer, +// only stubbing the transport (superagent) itself. This is the layer where a +// broken import (e.g. a named export that doesn't exist) actually blows up - +// a test that mocks profile/actions can never catch that. +jest.mock("superagent", () => ({ + post: jest.fn(), +})); + +import request from "superagent"; +import { enableTwoFactor, regenerateRecoveryCodes } from "profile/actions"; + +function makeChainableRequest({ body = {}, error = null } = {}) { + const req = {}; + req.set = jest.fn(() => req); + req.send = jest.fn(() => req); + req.timeout = jest.fn(() => req); + req.then = (onFulfilled, onRejected) => + (error ? Promise.reject(error) : Promise.resolve({ body })).then(onFulfilled, onRejected); + req.catch = (onRejected) => req.then(undefined, onRejected); + return req; +} + +describe("profile/actions (unmocked request layer)", () => { + beforeEach(() => { + 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.CSFR_TOKEN = "test-csrf-token"; + }); + + it("enableTwoFactor resolves the recovery codes through postRawRequestFull", async () => { + const req = makeChainableRequest({ body: { recovery_codes: ["ABCD-1234"] } }); + request.post.mockReturnValue(req); + + const { response } = await enableTwoFactor("email_otp"); + + expect(request.post).toHaveBeenCalledWith(window.ENABLE_TWO_FACTOR_ENDPOINT); + expect(req.send).toHaveBeenCalledWith({ method: "email_otp" }); + expect(req.set).toHaveBeenCalledWith({ "X-CSRF-TOKEN": "test-csrf-token" }); + expect(response.recovery_codes).toEqual(["ABCD-1234"]); + }); + + it("regenerateRecoveryCodes sends current_password only in the request body, never in the URL", async () => { + const req = makeChainableRequest({ body: { recovery_codes: ["WXYZ-5678"] } }); + request.post.mockReturnValue(req); + + const { response } = await regenerateRecoveryCodes("super-secret-password"); + + const calledUrl = request.post.mock.calls[0][0]; + expect(calledUrl).toBe(window.REGENERATE_RECOVERY_CODES_ENDPOINT); + expect(calledUrl).not.toContain("super-secret-password"); + expect(req.send).toHaveBeenCalledWith({ current_password: "super-secret-password" }); + expect(response.recovery_codes).toEqual(["WXYZ-5678"]); + }); +}); From 1e688cb151ff99819e97c8c409920f4152d17f5c Mon Sep 17 00:00:00 2001 From: romanetar Date: Tue, 11 Aug 2026 17:07:12 +0200 Subject: [PATCH 08/12] fix: fix CI failures from the recovery-code round-trip test and dash normalization 1. testDisplayedRecoveryCodeRedeemsThroughVerifyRecoveryCode called AbstractMFAChallengeStrategy::verifyRecoveryCode() directly, but it takes a PESSIMISTIC_WRITE row lock that requires an open transaction (Doctrine\ORM\TransactionRequiredException in CI). Route it through IAuthService::verifyMFARecoveryCode(), like the real login flow, which wraps the call in a transaction. 2. Several pre-existing test fixtures hashed a "plain" recovery code with a literal "-" baked in (e.g. 'RECOVERY-REUSE-TX-' . uniqid()) and then submitted that same string for verification. The dash normalization added earlier in this PR strips separators from the submitted code before Hash::check(), so a hash made from a dash-containing string can never match its own normalized submission - a real generated code never contains a dash in its raw/hashed form, only in its display formatting. Fixed the 7 affected fixtures across TwoFactorLoginFlowTest and AbstractMFAChallengeStrategyTest to drop the literal dash. --- tests/RecoveryCodeRegenerationTest.php | 6 +++++- tests/TwoFactorLoginFlowTest.php | 13 ++++++++----- tests/unit/MFA/AbstractMFAChallengeStrategyTest.php | 10 ++++++++-- 3 files changed, 21 insertions(+), 8 deletions(-) diff --git a/tests/RecoveryCodeRegenerationTest.php b/tests/RecoveryCodeRegenerationTest.php index 0a3a4b4e..cec67de8 100644 --- a/tests/RecoveryCodeRegenerationTest.php +++ b/tests/RecoveryCodeRegenerationTest.php @@ -19,6 +19,7 @@ use Illuminate\Support\Facades\Session; use LaravelDoctrine\ORM\Facades\EntityManager; use Strategies\MFA\MFAChallengeStrategyFactory; +use Utils\Services\IAuthService; /** * Integration tests for regenerating recovery codes from the user profile @@ -141,8 +142,11 @@ public function testDisplayedRecoveryCodeRedeemsThroughVerifyRecoveryCode(): voi // The hash was generated over the dash-less string; redeeming the code // exactly as it was displayed (with its "-" separator) must still work. + // Goes through IAuthService, like the real login flow, because + // verifyRecoveryCode() takes a PESSIMISTIC_WRITE row lock that requires + // an open transaction. $strategy = MFAChallengeStrategyFactory::create(User::MFAMethod_OTP); - $strategy->verifyRecoveryCode($admin, $displayedCode); + app(IAuthService::class)->verifyMFARecoveryCode($admin, $strategy, $displayedCode); EntityManager::clear(); $unusedAfter = EntityManager::getRepository(UserRecoveryCode::class) diff --git a/tests/TwoFactorLoginFlowTest.php b/tests/TwoFactorLoginFlowTest.php index cfd4d379..e28b79b0 100644 --- a/tests/TwoFactorLoginFlowTest.php +++ b/tests/TwoFactorLoginFlowTest.php @@ -482,7 +482,10 @@ public function testOTPCodeRejectsReuseAfterSuccessfulVerification(): void public function testRecoveryCodeRejectsReuseAfterTransactionCommit(): void { $admin = $this->user(self::ADMIN_EMAIL); - $plain = 'RECOVERY-REUSE-TX-' . uniqid(); + // No "-" in the fixture: verifyRecoveryCode() now strips separators before + // Hash::check() (real codes are hashed dash-less; the dash is display-only), + // so a fixture hashed with one baked in would never match its own submission. + $plain = 'RECOVERYREUSETX' . uniqid(); $this->createRecoveryCode($admin, $plain, false); // First use — must succeed. @@ -603,7 +606,7 @@ public function testOTPRedeemRowLockBlocksConcurrentConnection(): void public function testRecoveryCodeRowLockBlocksConcurrentConnection(): void { $admin = $this->user(self::ADMIN_EMAIL); - $plain = 'RECOVERY-LOCK-' . uniqid(); + $plain = 'RECOVERYLOCK' . uniqid(); $codeId = $this->createRecoveryCode($admin, $plain, false); /** @var IUserRecoveryCodeRepository $recoveryRepo */ @@ -783,7 +786,7 @@ public function testRecoveryAuditFailureDoesNotBlockLogin(): void // Audit is best-effort: a failure emitting recovery_used must NOT 500 a // user whose recovery code is already burned and session established. $admin = $this->user(self::ADMIN_EMAIL); - $plain = 'RECOVERY-AUDIT-FAIL-' . uniqid(); + $plain = 'RECOVERYAUDITFAIL' . uniqid(); $this->createRecoveryCode($admin, $plain, false); $auditMock = \Mockery::mock(ITwoFactorAuditService::class); @@ -836,7 +839,7 @@ public function testDeviceTrustFailureDoesNotBlockLogin(): void public function testRecoveryCodeLoginSucceeds(): void { $admin = $this->user(self::ADMIN_EMAIL); - $plain = 'RECOVERY-PLAIN-123'; + $plain = 'RECOVERYPLAIN123'; $codeId = $this->createRecoveryCode($admin, $plain, false); $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); @@ -856,7 +859,7 @@ public function testRecoveryCodeLoginSucceeds(): void public function testUsedRecoveryCodeFails(): void { $admin = $this->user(self::ADMIN_EMAIL); - $plain = 'RECOVERY-USED-456'; + $plain = 'RECOVERYUSED456'; $this->createRecoveryCode($admin, $plain, true); // already used $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); diff --git a/tests/unit/MFA/AbstractMFAChallengeStrategyTest.php b/tests/unit/MFA/AbstractMFAChallengeStrategyTest.php index d87c8bce..afa2432b 100644 --- a/tests/unit/MFA/AbstractMFAChallengeStrategyTest.php +++ b/tests/unit/MFA/AbstractMFAChallengeStrategyTest.php @@ -95,7 +95,10 @@ public function testClearPendingState_removesAllSessionKeys(): void public function testVerifyRecoveryCode_withMatchingCode_marksAsUsed(): void { $user = new User(); - $code = 'VALID-CODE'; + // No "-" in the fixture: verifyRecoveryCode() strips separators before + // Hash::check() (real codes are hashed dash-less), so a fixture hashed + // with one baked in would never match its own submission. + $code = 'VALIDCODE'; $recoveryCode = \Mockery::mock(\App\libs\Auth\Models\UserRecoveryCode::class); $recoveryCode->shouldReceive('getCodeHash')->andReturn(Hash::make($code)); @@ -120,7 +123,10 @@ public function resendChallenge(User $user, ?Client $client, bool $remember): ar public function testVerifyRecoveryCode_locksAndRejects_whenUsedAfterLock(): void { $user = new User(); - $code = 'VALID-CODE'; + // No "-" in the fixture: verifyRecoveryCode() strips separators before + // Hash::check() (real codes are hashed dash-less), so a fixture hashed + // with one baked in would never match its own submission. + $code = 'VALIDCODE'; // Hash matches, but a concurrent winner marked it used; the lock+recheck // must reject without double-spending (regression guard for 3357348455). From c44ed61e6694df58133059ca1b499d38eb68e677 Mon Sep 17 00:00:00 2001 From: romanetar Date: Tue, 11 Aug 2026 17:16:49 +0200 Subject: [PATCH 09/12] fix: remove nested transaction in enableTwoFactorAndGenerateCodes enableTwoFactorAndGenerateCodes() wrapped enable2FA()/user persist in one transaction() call while also calling generateRecoveryCodes(), which opens its own. DoctrineTransactionService::transaction() closes the entity manager and connection on failure, so an inner failure could tear down the EM out from under the still-running outer transaction. Extracted the shared code-generation logic into a transaction-free regenerateCodesForUser(), so each public method now opens exactly one transaction. --- app/Services/Auth/RecoveryCodeService.php | 64 ++++++++++++++++------- 1 file changed, 44 insertions(+), 20 deletions(-) diff --git a/app/Services/Auth/RecoveryCodeService.php b/app/Services/Auth/RecoveryCodeService.php index f5e67c0c..e72b9188 100644 --- a/app/Services/Auth/RecoveryCodeService.php +++ b/app/Services/Auth/RecoveryCodeService.php @@ -57,24 +57,7 @@ public function regenerateRecoveryCodes(User $user, string $currentPassword): ar */ public function generateRecoveryCodes(User $user): array { - $count = (int)config('auth.recovery_codes.count', 10); - $length = (int)config('auth.recovery_codes.length', 8); - - $plaintext_codes = []; - - $this->tx_service->transaction(function () use ($user, $count, $length, &$plaintext_codes) { - $this->repository->deleteAllForUser($user); - - for ($i = 0; $i < $count; $i++) { - $plain = Rand::getString($length, self::CODE_CHARSET, true); - $plaintext_codes[] = $plain; - - $code = new UserRecoveryCode(); - $code->setUser($user); - $code->setCodeHash(Hash::make($plain)); - $this->repository->add($code, false); - } - }); + $codes = $this->tx_service->transaction(fn() => $this->regenerateCodesForUser($user)); $this->audit_service->log( $user, @@ -83,7 +66,7 @@ public function generateRecoveryCodes(User $user): array IPHelper::getUserIp() ); - return array_map(static fn(string $code) => implode('-', str_split($code, 4)), $plaintext_codes); + return $codes; } /** @@ -91,13 +74,26 @@ public function generateRecoveryCodes(User $user): array */ public function enableTwoFactorAndGenerateCodes(User $user, string $method): array { + // Everything must live in a single transaction() call: it opens/commits + // its own connection-level transaction and closes the entity manager on + // failure (see DoctrineTransactionService::transaction()), so nesting a + // 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) { $user->enable2FA($method); $this->user_repository->add($user, false); - return $this->generateRecoveryCodes($user); + return $this->regenerateCodesForUser($user); }); + $this->audit_service->log( + $user, + TwoFactorAuditLog::EventRecoveryCodesGenerated, + TwoFactorAuditLog::MethodRecovery, + IPHelper::getUserIp() + ); + $this->audit_service->log( $user, TwoFactorAuditLog::EventEnrollmentChanged, @@ -108,6 +104,34 @@ public function enableTwoFactorAndGenerateCodes(User $user, string $method): arr return $codes; } + /** + * Invalidates every existing recovery code for the user and generates a + * fresh batch, within the caller's already-open transaction. + * + * @return string[] plaintext codes formatted as XXXX-XXXX + */ + private function regenerateCodesForUser(User $user): array + { + $count = (int)config('auth.recovery_codes.count', 10); + $length = (int)config('auth.recovery_codes.length', 8); + + $plaintext_codes = []; + + $this->repository->deleteAllForUser($user); + + for ($i = 0; $i < $count; $i++) { + $plain = Rand::getString($length, self::CODE_CHARSET, true); + $plaintext_codes[] = $plain; + + $code = new UserRecoveryCode(); + $code->setUser($user); + $code->setCodeHash(Hash::make($plain)); + $this->repository->add($code, false); + } + + return array_map(static fn(string $code) => implode('-', str_split($code, 4)), $plaintext_codes); + } + /** * @inheritDoc */ From cee6309853e246266ab8215b0435114fba1ee6db Mon Sep 17 00:00:00 2001 From: romanetar Date: Tue, 11 Aug 2026 17:19:58 +0200 Subject: [PATCH 10/12] fix: make recovery-code audit logging best-effort Both generateRecoveryCodes() and enableTwoFactorAndGenerateCodes() logged audit events after the codes were already committed and about to be returned to the client. An audit-logging failure there would 500 a response whose side effects already succeeded, and a client retry on that 500 would regenerate and invalidate the codes it was never shown. Wrap both in try/catch + Log::warning, matching the best-effort pattern already used for audit logging in UserController. --- app/Services/Auth/RecoveryCodeService.php | 55 +++++++++++++++-------- 1 file changed, 36 insertions(+), 19 deletions(-) diff --git a/app/Services/Auth/RecoveryCodeService.php b/app/Services/Auth/RecoveryCodeService.php index e72b9188..1abaeb9c 100644 --- a/app/Services/Auth/RecoveryCodeService.php +++ b/app/Services/Auth/RecoveryCodeService.php @@ -19,6 +19,7 @@ use Auth\Repositories\IUserRepository; use Auth\User; use Illuminate\Support\Facades\Hash; +use Illuminate\Support\Facades\Log; use Laminas\Math\Rand; use models\exceptions\ValidationException; use Utils\Db\ITransactionService; @@ -59,12 +60,20 @@ public function generateRecoveryCodes(User $user): array { $codes = $this->tx_service->transaction(fn() => $this->regenerateCodesForUser($user)); - $this->audit_service->log( - $user, - TwoFactorAuditLog::EventRecoveryCodesGenerated, - TwoFactorAuditLog::MethodRecovery, - IPHelper::getUserIp() - ); + // 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); + } return $codes; } @@ -87,19 +96,27 @@ public function enableTwoFactorAndGenerateCodes(User $user, string $method): arr return $this->regenerateCodesForUser($user); }); - $this->audit_service->log( - $user, - TwoFactorAuditLog::EventRecoveryCodesGenerated, - TwoFactorAuditLog::MethodRecovery, - IPHelper::getUserIp() - ); - - $this->audit_service->log( - $user, - TwoFactorAuditLog::EventEnrollmentChanged, - $method, - IPHelper::getUserIp() - ); + // Best-effort: 2FA is already enabled and the codes are already + // 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. + try { + $this->audit_service->log( + $user, + TwoFactorAuditLog::EventRecoveryCodesGenerated, + TwoFactorAuditLog::MethodRecovery, + IPHelper::getUserIp() + ); + + $this->audit_service->log( + $user, + TwoFactorAuditLog::EventEnrollmentChanged, + $method, + IPHelper::getUserIp() + ); + } catch (\Throwable $ex) { + Log::warning($ex); + } return $codes; } From 2c603c9d457b80179693043a186979ee81db8621 Mon Sep 17 00:00:00 2001 From: romanetar Date: Tue, 11 Aug 2026 17:25:30 +0200 Subject: [PATCH 11/12] fix: use the configured app name in the downloaded recovery-codes file recovery_code_display.js hardcoded "FNTECH" in both the file header and the downloaded filename, which would misbrand any non-FNTECH deployment. Threaded the existing appName prop (already exposed by profile.blade.php as config.appName, sourced from Config::get('app.app_name')) down through ProfilePage -> TwoFactorSection -> RecoveryCodesPanel -> RecoveryCodeModal -> RecoveryCodeDisplay, with an OpenStackID fallback matching the config default. --- resources/js/components/recovery_code_display.js | 11 +++++++---- resources/js/components/recovery_code_modal.js | 4 ++-- resources/js/components/recovery_codes_panel.js | 3 ++- resources/js/components/two_factor_section.js | 4 +++- resources/js/profile/profile.js | 4 +++- 5 files changed, 17 insertions(+), 9 deletions(-) diff --git a/resources/js/components/recovery_code_display.js b/resources/js/components/recovery_code_display.js index 32a24c2d..08f5192a 100644 --- a/resources/js/components/recovery_code_display.js +++ b/resources/js/components/recovery_code_display.js @@ -9,10 +9,12 @@ import {downloadTextFile} from "../utils"; import styles from "./recovery_codes.module.scss"; -const buildFileContent = (codes, email) => { +const DEFAULT_APP_NAME = "OpenStackID"; + +const buildFileContent = (codes, email, appName) => { const date = new Date().toISOString().slice(0, 10); return [ - "FNTECH Recovery Codes", + `${appName} Recovery Codes`, `Generated: ${date}`, `Account: ${email}`, "", @@ -22,7 +24,7 @@ const buildFileContent = (codes, email) => { ].join("\n"); }; -const RecoveryCodeDisplay = ({codes, email}) => { +const RecoveryCodeDisplay = ({codes, email, appName = DEFAULT_APP_NAME}) => { const [copied, setCopied] = useState(false); const handleCopy = () => { @@ -34,7 +36,8 @@ const RecoveryCodeDisplay = ({codes, email}) => { const handleDownload = () => { const date = new Date().toISOString().slice(0, 10); - downloadTextFile(`fntech-recovery-codes-${date}.txt`, buildFileContent(codes, email)); + const appSlug = appName.toLowerCase().replace(/[^a-z0-9]+/g, "-").replace(/^-|-$/g, ""); + downloadTextFile(`${appSlug}-recovery-codes-${date}.txt`, buildFileContent(codes, email, appName)); }; return ( diff --git a/resources/js/components/recovery_code_modal.js b/resources/js/components/recovery_code_modal.js index 0e460137..f0a09c01 100644 --- a/resources/js/components/recovery_code_modal.js +++ b/resources/js/components/recovery_code_modal.js @@ -13,7 +13,7 @@ import styles from "./recovery_codes.module.scss"; const ACK_DELAY_SECONDS = 5; -const RecoveryCodeModal = ({open, codes, email, onAcknowledge}) => { +const RecoveryCodeModal = ({open, codes, email, appName, onAcknowledge}) => { const [secondsLeft, setSecondsLeft] = useState(ACK_DELAY_SECONDS); useEffect(() => { @@ -44,7 +44,7 @@ const RecoveryCodeModal = ({open, codes, email, onAcknowledge}) => { These codes will not be shown again. Copy or download them now and store them somewhere safe. - {codes && } + {codes && }