diff --git a/app/Http/Controllers/Api/UserApiController.php b/app/Http/Controllers/Api/UserApiController.php index b32c7307..b334959b 100644 --- a/app/Http/Controllers/Api/UserApiController.php +++ b/app/Http/Controllers/Api/UserApiController.php @@ -16,6 +16,7 @@ use App\Http\Controllers\Traits\RequestProcessor; use App\Http\Controllers\UserValidationRulesFactory; use App\ModelSerializers\SerializerRegistry; +use App\Services\Auth\IRecoveryCodeService; use Auth\Repositories\IUserRepository; use Auth\User; use Exception; @@ -23,6 +24,7 @@ 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; @@ -43,23 +45,31 @@ final class UserApiController extends APICRUDController */ private $token_service; + /** + * @var IRecoveryCodeService + */ + private $recovery_code_service; + /** * UserApiController constructor. * @param IUserRepository $user_repository * @param ILogService $log_service * @param IUserService $user_service * @param ITokenService $token_service + * @param IRecoveryCodeService $recovery_code_service */ public function __construct ( IUserRepository $user_repository, ILogService $log_service, IUserService $user_service, - ITokenService $token_service + ITokenService $token_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; } /** @@ -247,6 +257,68 @@ 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']; + + 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->recovery_code_service->enableTwoFactorAndGenerateCodes($user, $method); + + 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..97a54674 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){ @@ -919,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()); @@ -1184,6 +1199,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..51df966b --- /dev/null +++ b/app/Services/Auth/IRecoveryCodeService.php @@ -0,0 +1,64 @@ +checkPassword(trim($currentPassword))) { + throw new ValidationException('current_password is not correct.'); + } + + return $this->generateRecoveryCodes($user); + } + + /** + * @inheritDoc + */ + public function generateRecoveryCodes(User $user): array + { + $codes = $this->tx_service->transaction(fn() => $this->regenerateCodesForUser($user)); + + // Best-effort: the codes are already committed and about to be shown to + // the user, so an audit-logging failure must not 500 this response - a + // client retry on a 500 would regenerate and invalidate the codes it + // just received. + try { + $this->audit_service->log( + $user, + TwoFactorAuditLog::EventRecoveryCodesGenerated, + TwoFactorAuditLog::MethodRecovery, + IPHelper::getUserIp() + ); + } catch (\Throwable $ex) { + Log::warning($ex); + } + + return $codes; + } + + /** + * @inheritDoc + */ + 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->regenerateCodesForUser($user); + }); + + // 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; + } + + /** + * 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 + */ + 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/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 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/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); diff --git a/resources/js/components/recovery_code_display.js b/resources/js/components/recovery_code_display.js new file mode 100644 index 00000000..08f5192a --- /dev/null +++ b/resources/js/components/recovery_code_display.js @@ -0,0 +1,76 @@ +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 DEFAULT_APP_NAME = "OpenStackID"; + +const buildFileContent = (codes, email, appName) => { + const date = new Date().toISOString().slice(0, 10); + return [ + `${appName} 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, appName = DEFAULT_APP_NAME}) => { + 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); + const appSlug = appName.toLowerCase().replace(/[^a-z0-9]+/g, "-").replace(/^-|-$/g, ""); + downloadTextFile(`${appSlug}-recovery-codes-${date}.txt`, buildFileContent(codes, email, appName)); + }; + + 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..f0a09c01 --- /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, appName, 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..986c70f7 --- /dev/null +++ b/resources/js/components/recovery_codes_panel.js @@ -0,0 +1,144 @@ +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 { + RECOVERY_CODES_LOW_WARNING_DISMISSED_KEY, + DEFAULT_RECOVERY_CODES_LOW_THRESHOLD, +} from "../shared/recovery_codes"; + +import styles from "./recovery_codes.module.scss"; + +const RecoveryCodesPanel = ({ + recoveryCodesRemaining, + recoveryCodesTotal, + lowCodeThreshold = DEFAULT_RECOVERY_CODES_LOW_THRESHOLD, + email, + appName, + 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(RECOVERY_CODES_LOW_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(RECOVERY_CODES_LOW_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..ece17eff --- /dev/null +++ b/resources/js/components/two_factor_section.js @@ -0,0 +1,68 @@ +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, + appName + }) => { + const [enabled, setEnabled] = useState(twoFactorEnabled); + const [loading, setLoading] = useState(false); + const [remaining, setRemaining] = useState(recoveryCodesRemaining); + const [total, setTotal] = useState(recoveryCodesTotal); + const [enrollmentCodes, setEnrollmentCodes] = useState(null); + + 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..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); @@ -318,9 +329,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: "" }, }); } @@ -536,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"); @@ -547,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, @@ -829,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/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..878f86a5 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"; @@ -35,13 +37,18 @@ import styles from "./profile.module.scss"; const ProfilePage = ({ appLogo, + appName, countries, csrfToken, initialValues, languages, menuConfig, passwordPolicy, - redirectUri + redirectUri, + twoFactorEnabled, + recoveryCodesRemaining, + recoveryCodesTotal, + recoveryCodesLowThreshold }) => { const [pic, setPic] = useState(null); const [loading, setLoading] = useState(false); @@ -754,11 +761,27 @@ 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..cec67de8 --- /dev/null +++ b/tests/RecoveryCodeRegenerationTest.php @@ -0,0 +1,237 @@ +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 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. + // 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); + app(IAuthService::class)->verifyMFARecoveryCode($admin, $strategy, $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), + // 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/TwoFactorLoginFlowTest.php b/tests/TwoFactorLoginFlowTest.php index cfd4d379..cff0ff0f 100644 --- a/tests/TwoFactorLoginFlowTest.php +++ b/tests/TwoFactorLoginFlowTest.php @@ -482,7 +482,12 @@ public function testOTPCodeRejectsReuseAfterSuccessfulVerification(): void public function testRecoveryCodeRejectsReuseAfterTransactionCommit(): void { $admin = $this->user(self::ADMIN_EMAIL); - $plain = 'RECOVERY-REUSE-TX-' . uniqid(); + // No "-" and all-uppercase: verifyRecoveryCode() now strips separators + // and uppercases the submitted code before Hash::check() (real codes are + // hashed dash-less and all-uppercase; the dash/case are display-only), + // so a fixture hashed with lowercase uniqid() hex would never match its + // own (normalized) submission. + $plain = 'RECOVERYREUSETX' . strtoupper(uniqid()); $this->createRecoveryCode($admin, $plain, false); // First use — must succeed. @@ -603,7 +608,7 @@ public function testOTPRedeemRowLockBlocksConcurrentConnection(): void public function testRecoveryCodeRowLockBlocksConcurrentConnection(): void { $admin = $this->user(self::ADMIN_EMAIL); - $plain = 'RECOVERY-LOCK-' . uniqid(); + $plain = 'RECOVERYLOCK' . strtoupper(uniqid()); $codeId = $this->createRecoveryCode($admin, $plain, false); /** @var IUserRecoveryCodeRepository $recoveryRepo */ @@ -783,7 +788,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' . strtoupper(uniqid()); $this->createRecoveryCode($admin, $plain, false); $auditMock = \Mockery::mock(ITwoFactorAuditService::class); @@ -836,7 +841,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 +861,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/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/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"]); + }); +}); 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/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). 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==