fix(2fa): issue MFA codes with two_factor.otp_lifetime, not the client's passwordless lifetime

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]);

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)) {

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);
     }
 

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,
         ];
     }
 

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

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),

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);
+            });
         }
     };
 

resources/js/login/login.js

@@ -514,22 +514,37 @@ 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",
         );
       },
     );
 
-    // 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;
   }
 

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 =
             [