diff --git a/app/Models/OAuth2/Factories/OTPFactory.php b/app/Models/OAuth2/Factories/OTPFactory.php index 167597f98..6991a9461 100644 --- a/app/Models/OAuth2/Factories/OTPFactory.php +++ b/app/Models/OAuth2/Factories/OTPFactory.php @@ -80,13 +80,18 @@ static public function buildFromRequest * @param array $payload * @param IdentifierGenerator $identifier_generator * @param Client|null $client + * @param int|null $lifetime_override seconds; when given it wins over both the + * global otp.lifetime and the client's own + * passwordless otp_lifetime (MFA codes must + * never inherit a client's "0 = never expires") * @return OAuth2OTP */ static public function buildFromPayload ( array $payload, IdentifierGenerator $identifier_generator, - ?Client $client = null + ?Client $client = null, + ?int $lifetime_override = null ):OAuth2OTP{ $lifetime = Config::get("otp.lifetime", 120); @@ -97,6 +102,19 @@ static public function buildFromPayload $length = $client->getOtpLength(); } + if (!is_null($lifetime_override)) { + // An explicit override exists to guarantee expiry; 0 would re-create + // the "never expires" code this parameter was added to prevent, and + // silently falling back could land on a client's 0 again. + if ($lifetime_override <= 0) { + throw new \InvalidArgumentException(sprintf( + "OTPFactory::buildFromPayload lifetime override must be positive, %d given", + $lifetime_override + )); + } + $lifetime = $lifetime_override; + } + $otp = new OAuth2OTP($length, $lifetime); $otp->setConnection($payload[OAuth2Protocol::OAuth2PasswordlessConnection]); diff --git a/app/Services/OAuth2/TokenService.php b/app/Services/OAuth2/TokenService.php index cbdd79233..a6aed0f8f 100644 --- a/app/Services/OAuth2/TokenService.php +++ b/app/Services/OAuth2/TokenService.php @@ -1802,15 +1802,16 @@ public function createOTPFromRequest(OAuth2PasswordlessAuthenticationRequest $re /** * @param array $payload * @param Client|null $client + * @param int|null $lifetime_override seconds; overrides the client/global OTP lifetime when given * @return OAuth2OTP * @throws Exception */ - public function createOTPFromPayload(array $payload, ?Client $client): OAuth2OTP + public function createOTPFromPayload(array $payload, ?Client $client, ?int $lifetime_override = null): OAuth2OTP { - $otp = $this->tx_service->transaction(function () use ($payload, $client) { + $otp = $this->tx_service->transaction(function () use ($payload, $client, $lifetime_override) { - $otp = OTPFactory::buildFromPayload($payload, $this->identifier_generator, $client); + $otp = OTPFactory::buildFromPayload($payload, $this->identifier_generator, $client, $lifetime_override); $user = $this->auth_service->getUserByUsername($otp->getUserName()); if (!is_null($user)) { diff --git a/app/Strategies/MFA/AbstractMFAChallengeStrategy.php b/app/Strategies/MFA/AbstractMFAChallengeStrategy.php index c5ff49e97..83d5c943a 100644 --- a/app/Strategies/MFA/AbstractMFAChallengeStrategy.php +++ b/app/Strategies/MFA/AbstractMFAChallengeStrategy.php @@ -6,6 +6,7 @@ use Auth\User; use Illuminate\Support\Facades\Config; use Illuminate\Support\Facades\Hash; +use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Session; use Models\OAuth2\Client; @@ -16,6 +17,13 @@ abstract class AbstractMFAChallengeStrategy implements IMFAChallengeStrategy private const KEY_REMEMBER = MFAConstants::SESSION_KEY_REMEMBER; private const KEY_RECOVERY_ATTEMPTS = MFAConstants::SESSION_KEY_RECOVERY_ATTEMPTS; + /** + * Fallback for two_factor.otp_lifetime (missing or non-positive) and the + * default of two_factor.session_ttl, so the pending session never + * outlives the code (SDS idp-mfa.md §4.14). + */ + protected const DEFAULT_OTP_LIFETIME = 600; + public function __construct(protected IUserRecoveryCodeRepository $recovery_code_repository) {} public function getPendingState(): ?MFAPendingState @@ -27,7 +35,7 @@ public function getPendingState(): ?MFAPendingState return null; } - if ((time() - $pending_at) > (int) Config::get('two_factor.session_ttl', 600)) { + if ((time() - $pending_at) > self::resolvePendingSessionTtl()) { $this->clearPendingState(); return null; } @@ -47,6 +55,45 @@ public function clearPendingState(): void Session::remove(self::KEY_RECOVERY_ATTEMPTS); } + /** + * Lifetime (seconds) of every MFA challenge code. A non-positive value is a + * misconfiguration (a second factor must always expire), so it falls back + * to the default instead of issuing a never-expiring code. + * + * @return int + */ + public static function resolveOtpLifetime(): int + { + $lifetime = intval(Config::get('two_factor.otp_lifetime', self::DEFAULT_OTP_LIFETIME)); + + if ($lifetime <= 0) { + Log::warning(sprintf( + "AbstractMFAChallengeStrategy::resolveOtpLifetime two_factor.otp_lifetime is %d, falling back to %d seconds", + $lifetime, + self::DEFAULT_OTP_LIFETIME + )); + $lifetime = self::DEFAULT_OTP_LIFETIME; + } + + return $lifetime; + } + + /** + * Seconds a pending challenge stays valid in the session: two_factor.session_ttl, + * raised to the effective code lifetime so a configuration that lengthens the + * code (or lowers the session TTL) can never expire the challenge while the + * code, and its UI countdown, are still valid. + * + * @return int + */ + public static function resolvePendingSessionTtl(): int + { + return max( + intval(Config::get('two_factor.session_ttl', self::DEFAULT_OTP_LIFETIME)), + self::resolveOtpLifetime() + ); + } + public function verifyRecoveryCode(User $user, string $code): void { // Recovery codes are hashed without the "-" separator; it is added only @@ -73,10 +120,19 @@ public function verifyRecoveryCode(User $user, string $code): void throw new AuthenticationException("Invalid recovery code."); } - protected function storePendingState(int $userId, bool $remember): void + /** + * @param int $userId + * @param bool $remember + * @param int|null $pendingAt epoch seconds the challenge was issued at; pass the + * code's own creation time so the pending session is + * measured on the same clock as the code's expiry and + * a slow issuance cannot leave the code valid for a + * few seconds after the session expired. Defaults to now. + */ + protected function storePendingState(int $userId, bool $remember, ?int $pendingAt = null): void { Session::put(self::KEY_USER_ID, $userId); - Session::put(self::KEY_PENDING_AT, time()); + Session::put(self::KEY_PENDING_AT, $pendingAt ?? time()); Session::put(self::KEY_REMEMBER, $remember); } diff --git a/app/Strategies/MFA/EmailOTPMFAChallengeStrategy.php b/app/Strategies/MFA/EmailOTPMFAChallengeStrategy.php index d5d24f8fa..a7b9084a5 100644 --- a/app/Strategies/MFA/EmailOTPMFAChallengeStrategy.php +++ b/app/Strategies/MFA/EmailOTPMFAChallengeStrategy.php @@ -20,20 +20,33 @@ public function __construct( public function issueChallenge(User $user, ?Client $client, bool $remember): array { - $this->storePendingState($user->getId(), $remember); - + // The MFA code always uses the IDP-wide two_factor.otp_lifetime, never + // the client's passwordless otp_lifetime that OTPFactory would pick up + // when an OAuth2 client is in play: that per-client value belongs to + // passwordless login, where 0 means "never expires". For a second + // factor that is both a security hole and an unusable screen (the UI + // renders a 0 lifetime as already expired and locks VERIFY, and no + // resend can get it out of that state). $otp = $this->token_service->createOTPFromPayload([ OAuth2Protocol::OAuth2PasswordlessConnection => OAuth2Protocol::OAuth2PasswordlessConnectionEmail, OAuth2Protocol::OAuth2PasswordlessSend => OAuth2Protocol::OAuth2PasswordlessSendCode, OAuth2Protocol::OAuth2PasswordlessEmail => $user->getEmail(), - ], $client); + ], $client, self::resolveOtpLifetime()); + + // Same source isAlive()/getRemainingLifetime() use server-side, so a UI + // countdown seeded from it can never drift from the actual expiry check. + $issuedAt = $otp->getCreatedAt()?->getTimestamp() ?? time(); + + // Stored AFTER the code exists and anchored to its creation time: issuing + // (mail transport included) can take seconds, and a pending timestamp + // taken before it would expire the session while the code is still alive. + // It also leaves no orphan pending state when issuance throws. + $this->storePendingState($user->getId(), $remember, $issuedAt); return [ 'otp_length' => $otp->getLength(), 'otp_lifetime' => $otp->getLifetime(), - // Same source isAlive()/getRemainingLifetime() use server-side, so a - // UI countdown seeded from it can never drift from the actual expiry check. - 'otp_issued_at' => $otp->getCreatedAt()?->getTimestamp() ?? time(), + 'otp_issued_at' => $issuedAt, ]; } diff --git a/app/libs/OAuth2/Services/ITokenService.php b/app/libs/OAuth2/Services/ITokenService.php index 84b267df1..c8cb7dd49 100644 --- a/app/libs/OAuth2/Services/ITokenService.php +++ b/app/libs/OAuth2/Services/ITokenService.php @@ -205,10 +205,11 @@ public function createOTPFromRequest(OAuth2PasswordlessAuthenticationRequest $re /** * @param array $payload * @param Client|null $client + * @param int|null $lifetime_override seconds; overrides the client/global OTP lifetime when given * @return OAuth2OTP * @throws \Exception */ - public function createOTPFromPayload(array $payload, ?Client $client):OAuth2OTP; + public function createOTPFromPayload(array $payload, ?Client $client, ?int $lifetime_override = null):OAuth2OTP; /** * @param OAuth2OTP $otp diff --git a/config/two_factor.php b/config/two_factor.php index d057f9d9e..014556baa 100644 --- a/config/two_factor.php +++ b/config/two_factor.php @@ -51,15 +51,30 @@ 'device_trust_lifetime_days' => env('DEVICE_TRUST_LIFETIME_DAYS', 30), 'cookie_name' => env('DEVICE_TRUST_COOKIE_NAME', 'device_trust_token'), + /* + |-------------------------------------------------------------------------- + | Challenge Code Lifetime + |-------------------------------------------------------------------------- + | + | Seconds an MFA email code stays valid. Applies to every challenge, + | including logins started from an OAuth2 client: the client's own + | passwordless otp_lifetime (where 0 means "never expires") is never + | used for a second factor. Non-positive values fall back to 600. + | + */ + 'otp_lifetime' => env('TWO_FACTOR_OTP_LIFETIME', 600), + /* |-------------------------------------------------------------------------- | Pending Challenge Session |-------------------------------------------------------------------------- | | Seconds a pending 2FA challenge stays valid in the session after the - | password step (SDS idp-mfa.md §4.14). Defaults to the default OTP - | lifetime (otp.lifetime) so the session never expires before the code - | and its UI countdown do. + | password step (SDS idp-mfa.md §4.14). The effective TTL is + | max(session_ttl, effective otp_lifetime), enforced in + | AbstractMFAChallengeStrategy::resolvePendingSessionTtl(), so the session + | can never expire before the code and its UI countdown do. Raise this + | value only to keep the challenge alive longer than the code. | */ 'session_ttl' => env('TWO_FACTOR_SESSION_TTL', 600), diff --git a/resources/js/login/components/two_factor_form.js b/resources/js/login/components/two_factor_form.js index c06ff08e6..73ec092bc 100644 --- a/resources/js/login/components/two_factor_form.js +++ b/resources/js/login/components/two_factor_form.js @@ -1,4 +1,4 @@ -import React, {useState, useEffect} from 'react'; +import React, {useState, useEffect, useRef} from 'react'; import Button from '@material-ui/core/Button'; import Link from '@material-ui/core/Link'; import FormControlLabel from '@material-ui/core/FormControlLabel'; @@ -26,13 +26,19 @@ const TwoFactorForm = ({ const {secondsLeft, expired} = useOtpCountdown(otpLifetime, codeVersion); const [cooldown, setCooldown] = useState(0); + // A failed resend may resolve after the parent swapped this form out + // (session expired -> password flow); don't touch state once unmounted. + const mounted = useRef(true); // 1s ticker for the resend cooldown (the expiry countdown lives in the hook). useEffect(() => { const timer = setInterval(() => { setCooldown(prev => (prev > 0 ? prev - 1 : 0)); }, 1000); - return () => clearInterval(timer); + return () => { + clearInterval(timer); + mounted.current = false; + }; }, []); const handleSubmit = (ev) => { @@ -46,10 +52,14 @@ const TwoFactorForm = ({ if (cooldown > 0 || disableInput) return; setCooldown(RESEND_COOLDOWN_SECONDS); // A successful resend resets the expiry countdown through the parent's - // codeVersion bump; failures are surfaced by the parent as well. + // codeVersion bump. A failed one is surfaced inline by the parent, so + // release the link right away instead of locking it for a cooldown + // that only makes sense when a code was actually sent. const result = onResend(); if (result && typeof result.catch === 'function') { - result.catch(() => {}); + result.catch(() => { + if (mounted.current) setCooldown(0); + }); } }; diff --git a/resources/js/login/login.js b/resources/js/login/login.js index 3d1ac13e1..f85e6894b 100644 --- a/resources/js/login/login.js +++ b/resources/js/login/login.js @@ -514,14 +514,28 @@ class LoginPage extends React.Component { ); return; } + // Failures are surfaced INLINE, like handleMfaError() does for verify: + // a snackbar auto-hides in seconds and leaves the screen looking as if + // the resend had worked (the link keeps counting down, the old expiry + // text stays) with no hint of what happened. if (status === HTTP_CODES.TOO_MANY_REQUESTS) { const msg = body && body.error_message ? body.error_message : "Too many attempts. Please try again later."; - this.showAlert(msg, "warning"); + this.setState({ + ...this.state, + errors: { ...this.state.errors, twofactor: msg }, + }); return; } + this.setState({ + ...this.state, + errors: { + ...this.state.errors, + twofactor: "We couldn't send a new code. Please try again.", + }, + }); this.showAlert( "Oops... Something went wrong while resending the code.", "error", @@ -529,7 +543,8 @@ class LoginPage extends React.Component { }, ); - // Returned so the form can reset its expiry countdown once the resend resolves. + // Returned so the form can reset its expiry countdown once the resend + // resolves, and release the resend link if it fails. return promise; } diff --git a/tests/OTPModelTest.php b/tests/OTPModelTest.php index d594f908c..6aa3d8b05 100644 --- a/tests/OTPModelTest.php +++ b/tests/OTPModelTest.php @@ -83,6 +83,40 @@ public function testCreateFromRequest(){ $this->assertTrue(strlen($otp->getValue()) == $client->getOtpLength()); } + public function testCreateFromPayloadLifetimeOverrideWinsOverClientLifetime(){ + $client_repository = EntityManager::getRepository(Client::class); + $client = $client_repository->findAll()[0]; + $this->assertInstanceOf(Client::class, $client); + + $client->enablePasswordless(); + $client->setOtpLifetime(0); // passwordless "never expires" + $client->setOtpLength(6); + EntityManager::persist($client); + + $payload = [ + OAuth2Protocol::OAuth2Protocol_ResponseType => OAuth2Protocol::OAuth2Protocol_ResponseType_OTP, + OAuth2Protocol::OAuth2PasswordlessConnection => OAuth2Protocol::OAuth2PasswordlessConnectionEmail, + OAuth2Protocol::OAuth2PasswordlessSend => OAuth2Protocol::OAuth2PasswordlessSendCode, + OAuth2Protocol::OAuth2PasswordlessEmail => "override@test.com", + ]; + + $otp = OTPFactory::buildFromPayload($payload, App::make(IdentifierGenerator::class), $client, 450); + $this->assertSame(450, $otp->getLifetime()); + $this->assertSame(6, strlen($otp->getValue()), 'length still comes from the client'); + } + + public function testCreateFromPayloadRejectsNonPositiveLifetimeOverride(){ + $payload = [ + OAuth2Protocol::OAuth2Protocol_ResponseType => OAuth2Protocol::OAuth2Protocol_ResponseType_OTP, + OAuth2Protocol::OAuth2PasswordlessConnection => OAuth2Protocol::OAuth2PasswordlessConnectionEmail, + OAuth2Protocol::OAuth2PasswordlessSend => OAuth2Protocol::OAuth2PasswordlessSendCode, + OAuth2Protocol::OAuth2PasswordlessEmail => "override@test.com", + ]; + + $this->expectException(\InvalidArgumentException::class); + OTPFactory::buildFromPayload($payload, App::make(IdentifierGenerator::class), null, 0); + } + public function testCreateFromPayloadNoClient(){ $payload = [ diff --git a/tests/TwoFactorLoginFlowTest.php b/tests/TwoFactorLoginFlowTest.php index 09e5314bf..5bd612e80 100644 --- a/tests/TwoFactorLoginFlowTest.php +++ b/tests/TwoFactorLoginFlowTest.php @@ -1334,6 +1334,43 @@ public function testFullOIDCFlowWithMFAChallengeAndConsentDeliversAuthCode(): vo $this->completeConsentAndGetAuthCode(); } + public function testOIDCFlowMFACodeIgnoresClientPasswordlessLifetime(): void + { + // Regression: a client whose passwordless otp_lifetime is 0 ("never + // expires") made every MFA code it triggered come out with lifetime 0, + // which the login screen renders as already expired - VERIFY stayed + // disabled and resends (200, still lifetime 0) could not recover it. + $client = EntityManager::getRepository(Client::class)->findOneBy(['client_id' => self::OIDC_CLIENT_ID]); + $this->assertInstanceOf(Client::class, $client); + $originalLifetime = $client->getOtpLifetime(); + $client->setOtpLifetime(0); + EntityManager::persist($client); + EntityManager::flush(); + + try { + $this->startOIDCFlowUpToChallenge(); + + $expected = intval(Config::get('two_factor.otp_lifetime')); + $this->assertGreaterThan(0, $expected); + + EntityManager::clear(); + $otps = App::make(IOAuth2OTPRepository::class)->getByUserNameNotRedeemed(self::ADMIN_EMAIL); + $this->assertNotEmpty($otps); + $this->assertSame($expected, end($otps)->getLifetime(), 'the MFA code must not inherit the client passwordless lifetime'); + $this->assertSame($expected, intval(Session::get('otp_lifetime')), 'the countdown seed persisted for refreshes must match'); + + $response = $this->action('POST', 'UserController@resend2FA', ['method' => User::MFAMethod_OTP]); + $this->assertResponseStatus(200); + $payload = json_decode($response->getContent(), true); + $this->assertSame($expected, $payload['otp_lifetime'], 'a resend must return the same positive lifetime'); + } finally { + $client = EntityManager::getRepository(Client::class)->findOneBy(['client_id' => self::OIDC_CLIENT_ID]); + $client->setOtpLifetime($originalLifetime); + EntityManager::persist($client); + EntityManager::flush(); + } + } + public function testFullOIDCFlowWithRecoveryCodeAndConsentDeliversAuthCode(): void { $admin = $this->user(self::ADMIN_EMAIL); diff --git a/tests/js/login/components/two-factor-form.test.js b/tests/js/login/components/two-factor-form.test.js index d7278eec1..b7dff560c 100644 --- a/tests/js/login/components/two-factor-form.test.js +++ b/tests/js/login/components/two-factor-form.test.js @@ -1,10 +1,15 @@ import React from 'react'; -import { render, screen } from '@testing-library/react'; +import { render, screen, act, fireEvent, cleanup } from '@testing-library/react'; import TwoFactorForm from '../../../../resources/js/login/components/two_factor_form'; // Suppress the 1-second interval so tests don't bleed real timers into each other. +// Unmount while the fake clock is still installed so the component's +// clearInterval() targets the fake timer it created. beforeEach(() => jest.useFakeTimers()); -afterEach(() => jest.useRealTimers()); +afterEach(() => { + cleanup(); + jest.useRealTimers(); +}); const baseProps = { otpCode: '123456', @@ -53,4 +58,36 @@ describe('TwoFactorForm', () => { expect(label).toHaveTextContent(msg); }); + it('an expired code is revived by a fresh one (codeVersion bump), re-enabling VERIFY', () => { + const { rerender } = render(); + act(() => { jest.advanceTimersByTime(6000); }); + expect(screen.getByText(/has expired/)).toBeInTheDocument(); + expect(screen.getByTestId('verify-button')).toBeDisabled(); + + // Same lifetime, new code: the parent bumps codeVersion on a successful resend. + rerender(); + expect(screen.queryByText(/has expired/)).not.toBeInTheDocument(); + expect(screen.getByTestId('verify-button')).not.toBeDisabled(); + }); + + it('a successful resend keeps the cooldown running', async () => { + const onResend = jest.fn(() => Promise.resolve()); + render(); + + await act(async () => { fireEvent.click(screen.getByTestId('resend-link')); }); + + expect(onResend).toHaveBeenCalledTimes(1); + expect(screen.getByTestId('resend-link')).toHaveTextContent(/resend code \(30s\)/); + }); + + it('a failed resend releases the cooldown so the user can retry', async () => { + const onResend = jest.fn(() => Promise.reject(new Error('429'))); + render(); + + await act(async () => { fireEvent.click(screen.getByTestId('resend-link')); }); + + expect(onResend).toHaveBeenCalledTimes(1); + expect(screen.getByTestId('resend-link')).toHaveTextContent(/^resend code$/); + }); + }); diff --git a/tests/js/login/login.mfa.test.js b/tests/js/login/login.mfa.test.js index 89928a6d9..62524fb96 100644 --- a/tests/js/login/login.mfa.test.js +++ b/tests/js/login/login.mfa.test.js @@ -22,7 +22,7 @@ delete window.location; window.location = { href: '', reload: jest.fn() }; import { LoginPage } from '../../../resources/js/login/login'; -import { verifyRecoveryCode, cancelLogin } from '../../../resources/js/login/actions'; +import { verifyRecoveryCode, cancelLogin, resend2FA } from '../../../resources/js/login/actions'; // ─── Minimal props that satisfy the LoginPage constructor ──────────────────── @@ -85,6 +85,69 @@ const flush = () => new Promise((resolve) => setTimeout(resolve, 0)); describe('LoginPage', () => { + describe('onResend2FA', () => { + let inst; + + beforeEach(() => { + inst = makeInstance(); + inst.state = { ...inst.state, authFlow: FLOW.MFA, otpLifetime: 0, codeVersion: 0 }; + }); + + it('200 — bumps codeVersion, takes the fresh lifetime and clears the inline error', async () => { + inst.state = { ...inst.state, errors: { ...inst.state.errors, twofactor: 'stale' } }; + resend2FA.mockReturnValue(Promise.resolve({ response: { otp_length: 6, otp_lifetime: 600 } })); + + inst.onResend2FA(); + await flush(); + + expect(inst.state.codeVersion).toBe(1); + expect(inst.state.otpLifetime).toBe(600); + expect(inst.state.errors.twofactor).toBe(''); + expect(inst.showAlert).toHaveBeenCalledWith(expect.any(String), 'success'); + }); + + it('429 — surfaces the server message INLINE and leaves the code untouched', async () => { + const msg = 'Too many codes requested. Try again in 10 minutes.'; + resend2FA.mockReturnValue( + Promise.reject(makeError(HTTP_CODES.TOO_MANY_REQUESTS, { error_code: 'mfa_rate_limit', error_message: msg })), + ); + + const promise = inst.onResend2FA(); + await promise.catch(() => {}); + + expect(inst.state.errors.twofactor).toBe(msg); + expect(inst.state.codeVersion).toBe(0); + expect(inst.showAlert).not.toHaveBeenCalled(); + expect(inst.resetToPasswordFlow).not.toHaveBeenCalled(); + }); + + it('500 — inline error plus snackbar, code untouched', async () => { + resend2FA.mockReturnValue(Promise.reject(makeError(500, { message: 'boom' }))); + + const promise = inst.onResend2FA(); + await promise.catch(() => {}); + + expect(inst.state.errors.twofactor).toBe("We couldn't send a new code. Please try again."); + expect(inst.state.codeVersion).toBe(0); + expect(inst.showAlert).toHaveBeenCalledWith(expect.any(String), 'error'); + }); + + it('401 + mfa_session_expired — back to the password flow with a warning', async () => { + resend2FA.mockReturnValue( + Promise.reject(makeError(HTTP_CODES.UNAUTHORIZED, { error_code: MFA_ERROR_CODE.MFA_SESSION_EXPIRED })), + ); + + const promise = inst.onResend2FA(); + await promise.catch(() => {}); + + expect(inst.resetToPasswordFlow).toHaveBeenCalledTimes(1); + expect(inst.showAlert).toHaveBeenCalledWith( + 'Your verification session has expired. Please sign in again.', + 'warning', + ); + }); + }); + describe('handleMfaError', () => { let inst; diff --git a/tests/unit/MFA/AbstractMFAChallengeStrategyTest.php b/tests/unit/MFA/AbstractMFAChallengeStrategyTest.php index b5cad7f52..1c0ad6c15 100644 --- a/tests/unit/MFA/AbstractMFAChallengeStrategyTest.php +++ b/tests/unit/MFA/AbstractMFAChallengeStrategyTest.php @@ -86,7 +86,9 @@ public function testGetPendingState_withinDefaultTtl_matchesDefaultOtpLifetime() public function testGetPendingState_honorsConfiguredSessionTtl(): void { + // Both knobs lowered: the session TTL is the effective limit. Config::set('two_factor.session_ttl', 120); + Config::set('two_factor.otp_lifetime', 120); Session::put(MFAConstants::SESSION_KEY_PENDING_USER_ID, 99); Session::put(MFAConstants::SESSION_KEY_PENDING_AT, time() - 121); @@ -95,6 +97,36 @@ public function testGetPendingState_honorsConfiguredSessionTtl(): void $this->assertNull($this->strategy->getPendingState()); } + public function testGetPendingState_neverExpiresBeforeTheCode(): void + { + // Regression: a longer code lifetime with an untouched (or lower) + // session_ttl must not expire the challenge while the code, and its UI + // countdown, are still valid. + Config::set('two_factor.session_ttl', 60); + Config::set('two_factor.otp_lifetime', 900); + + Session::put(MFAConstants::SESSION_KEY_PENDING_USER_ID, 99); + Session::put(MFAConstants::SESSION_KEY_PENDING_AT, time() - 601); + Session::put(MFAConstants::SESSION_KEY_REMEMBER, false); + + $this->assertNotNull($this->strategy->getPendingState(), 'pending state must live as long as the code'); + $this->assertSame(900, AbstractMFAChallengeStrategy::resolvePendingSessionTtl()); + + Session::put(MFAConstants::SESSION_KEY_PENDING_AT, time() - 901); + $this->assertNull($this->strategy->getPendingState(), 'and no longer than the code'); + } + + public function testResolvePendingSessionTtl_usesTheEffectiveOtpLifetime(): void + { + // otp_lifetime 0 falls back to 600 for the code, so the session must + // cover those 600 s even when session_ttl says less. + Config::set('two_factor.session_ttl', 60); + Config::set('two_factor.otp_lifetime', 0); + + $this->assertSame(600, AbstractMFAChallengeStrategy::resolveOtpLifetime()); + $this->assertSame(600, AbstractMFAChallengeStrategy::resolvePendingSessionTtl()); + } + public function testGetPendingState_withMissingSession_returnsNull(): void { $state = $this->strategy->getPendingState(); diff --git a/tests/unit/MFA/EmailOTPMFAChallengeStrategyTest.php b/tests/unit/MFA/EmailOTPMFAChallengeStrategyTest.php index bfbd664a1..43f766f33 100644 --- a/tests/unit/MFA/EmailOTPMFAChallengeStrategyTest.php +++ b/tests/unit/MFA/EmailOTPMFAChallengeStrategyTest.php @@ -17,6 +17,7 @@ use App\libs\OAuth2\Repositories\IOAuth2OTPRepository; use Auth\Repositories\IUserRecoveryCodeRepository; use Auth\User; +use Illuminate\Support\Facades\Config; use Illuminate\Support\Facades\Session; use Models\OAuth2\Client; use Models\OAuth2\OAuth2OTP; @@ -92,6 +93,112 @@ public function testIssueChallenge_storesPendingStateAndReturnsOtpInfo(): void $this->assertTrue(Session::get(MFAConstants::SESSION_KEY_REMEMBER)); } + public function testIssueChallenge_forcesTwoFactorLifetime_evenWhenAnOAuth2ClientIsInPlay(): void + { + // Regression: with an OAuth2 client in the memento, OTPFactory picks the + // client's PASSWORDLESS otp_lifetime (0 = never expires on at least one + // dev client), which the UI rendered as "already expired" and locked + // VERIFY for good. The MFA code must use two_factor.otp_lifetime instead. + Config::set('two_factor.otp_lifetime', 450); + + $user = $this->buildUser(42, 'user@example.com'); + $client = \Mockery::mock(Client::class); + $client->shouldReceive('getOtpLifetime')->never(); + + $otp = \Mockery::mock(OAuth2OTP::class); + $otp->shouldReceive('getLength')->andReturn(6); + $otp->shouldReceive('getLifetime')->andReturn(450); + $otp->shouldReceive('getCreatedAt')->andReturn(new \DateTime('2026-01-01 00:00:00', new \DateTimeZone('UTC'))); + + $this->tokenService + ->shouldReceive('createOTPFromPayload') + ->once() + ->withArgs(function (array $payload, $passedClient, $lifetime) use ($client) { + return $passedClient === $client && $lifetime === 450; + }) + ->andReturn($otp); + + $result = $this->strategy->issueChallenge($user, $client, false); + + $this->assertSame(450, $result['otp_lifetime']); + } + + public function testIssueChallenge_anchorsPendingSessionToTheCodeCreationTime(): void + { + // Issuing the code (mail transport included) can take seconds. The pending + // session must be measured from the code's own created_at, the clock + // isAlive() uses, or it expires while the code is still valid. + $user = $this->buildUser(42, 'user@example.com'); + + $createdAt = new \DateTime('now', new \DateTimeZone('UTC')); + $createdAt->modify('+3 seconds'); // creation lands after issueChallenge() started + + $otp = \Mockery::mock(OAuth2OTP::class); + $otp->shouldReceive('getLength')->andReturn(6); + $otp->shouldReceive('getLifetime')->andReturn(600); + $otp->shouldReceive('getCreatedAt')->andReturn($createdAt); + + $this->tokenService + ->shouldReceive('createOTPFromPayload') + ->once() + ->andReturnUsing(function () use ($otp) { + $this->assertNull( + Session::get(MFAConstants::SESSION_KEY_PENDING_AT), + 'no pending state before the code exists' + ); + return $otp; + }); + + $result = $this->strategy->issueChallenge($user, null, false); + + $this->assertSame($createdAt->getTimestamp(), Session::get(MFAConstants::SESSION_KEY_PENDING_AT)); + $this->assertSame($createdAt->getTimestamp(), $result['otp_issued_at']); + $this->assertNotNull($this->strategy->getPendingState()); + } + + public function testIssueChallenge_leavesNoPendingState_whenIssuanceThrows(): void + { + $user = $this->buildUser(42, 'user@example.com'); + + $this->tokenService + ->shouldReceive('createOTPFromPayload') + ->once() + ->andThrow(new \RuntimeException('mailer down')); + + try { + $this->strategy->issueChallenge($user, null, false); + $this->fail('expected the issuance failure to propagate'); + } catch (\RuntimeException $ex) { + $this->assertNull(Session::get(MFAConstants::SESSION_KEY_PENDING_USER_ID)); + $this->assertNull(Session::get(MFAConstants::SESSION_KEY_PENDING_AT)); + } + } + + public function testIssueChallenge_fallsBackToDefaultLifetime_whenConfigIsNonPositive(): void + { + // A second factor must always expire: a 0/negative config value is a + // misconfiguration, not a request for a never-expiring code. + Config::set('two_factor.otp_lifetime', 0); + + $user = $this->buildUser(42, 'user@example.com'); + + $otp = \Mockery::mock(OAuth2OTP::class); + $otp->shouldReceive('getLength')->andReturn(6); + $otp->shouldReceive('getLifetime')->andReturn(600); + $otp->shouldReceive('getCreatedAt')->andReturn(new \DateTime('2026-01-01 00:00:00', new \DateTimeZone('UTC'))); + + $this->tokenService + ->shouldReceive('createOTPFromPayload') + ->once() + ->withArgs(function (array $payload, $client, $lifetime) { + return $lifetime === 600; + }) + ->andReturn($otp); + + $this->strategy->issueChallenge($user, null, false); + $this->assertSame(600, EmailOTPMFAChallengeStrategy::resolveOtpLifetime()); + } + // ---------- resendChallenge ---------- public function testResendChallenge_delegatesToIssueChallenge(): void