chore: Add PR's requested changed

Author: matiasperrone-exo

app/Http/Controllers/Traits/MFACookieManager.php

@@ -13,9 +13,11 @@
  **/
 
 use Auth\User;
+use Exception;
 use Illuminate\Support\Facades\Config;
 use Illuminate\Support\Facades\Cookie;
 use Illuminate\Support\Facades\Request;
+use Keepsuit\LaravelOpenTelemetry\Facades\Logger;
 use Utils\IPHelper;
 
 /**

app/Http/Controllers/UserController.php

@@ -468,7 +468,7 @@ public function postLogin()
                 $connection = $data['connection'] ?? null;
 
                 try {
-                    if ($flow == "password") {
+                    if ($flow == IAuthService::AuthenticationFlowPassword) {
                         // Validate credentials WITHOUT establishing a session, so the
                         // MFA gate can run before the user is authenticated.
                         $user = $this->auth_service->validateCredentials($username, $password);
@@ -500,7 +500,7 @@ public function postLogin()
                         return $this->login_strategy->postLogin();
                     }
 
-                    if ($flow == "otp") {
+                    if ($flow == IAuthService::AuthenticationFlowPasswordless) {
 
                         $client = $this->resolveClientFromMemento();
 
@@ -630,6 +630,24 @@ private function resolveClientFromMemento(): ?Client
         return $client;
     }
 
+    /**
+     * Resolves the OAuth2 client carried in the pending MFA state (the client the
+     * challenge was issued for), so verification scopes the OTP lookup and
+     * sibling-revoke to the same client. Returns null when the challenge was not
+     * issued in a client context.
+     *
+     * @param array $pending
+     * @return Client|null
+     */
+    private function resolveClientFromPendingState(array $pending): ?Client
+    {
+        $clientId = $pending['client_id'] ?? null;
+        if (is_null($clientId)) {
+            return null;
+        }
+        return $this->client_repository->getClientById($clientId);
+    }
+
     /**
      * Verifies a 2FA OTP challenge and, on success, establishes the session.
      *
@@ -666,8 +684,19 @@ public function verify2FA()
                 return $this->mfaSessionExpired();
             }
 
+            // Scope verification to the client the challenge was issued for.
+            $client = $this->resolveClientFromPendingState($pending);
+
             try {
-                $this->auth_service->verifyMFAChallenge($user, $strategy, $otp_value);
+                // Commits the OTP redeem (+ sibling revoke) in its own tx. The
+                // session, trusted-device enrollment and audit are applied below
+                // as separate post-verification steps.
+                $this->auth_service->verifyMFAChallenge(
+                    $user,
+                    $strategy,
+                    $otp_value,
+                    $client
+                );
             } catch (AuthenticationException $ex) {
                 Log::warning($ex);
                 // Re-fetch user: the tx wrapper closed/reset the EM on failure, detaching the entity.
@@ -686,17 +715,29 @@ public function verify2FA()
             $this->auth_service->loginUser($user, (bool) $pending['remember']);
 
             if ($trust_device) {
-                $this->queueDeviceTrustCookie($user);
+                // Best-effort: the OTP is already redeemed and the session
+                // established, so a trusted-device enrollment failure must not
+                // 500 the user (which would lock them out on retry against a
+                // burned OTP). Log and continue; the device just isn't remembered.
+                try {
+                    $this->queueDeviceTrustCookie($user);
+                } catch (Exception $ex) {
+                    Log::warning($ex);
+                }
             }
 
             $strategy->clearPendingState();
 
-            $this->two_factor_audit_service->log(
-                $user,
-                TwoFactorAuditLog::EventChallengeSucceeded,
-                $method,
-                IPHelper::getUserIp()
-            );
+            try {
+                $this->two_factor_audit_service->log(
+                    $user,
+                    TwoFactorAuditLog::EventChallengeSucceeded,
+                    $method,
+                    IPHelper::getUserIp()
+                );
+            } catch (Exception $ex) {
+                Log::warning($ex);
+            }
 
             return $this->login_strategy->postLogin();
         } catch (ValidationException $ex) {

app/Repositories/DoctrineUserRecoveryCodeRepository.php

@@ -31,6 +31,12 @@ public function getUnusedByUser(User $user): array
         ]);
     }
 
+    public function refreshExclusiveLock(UserRecoveryCode $code): void
+    {
+        // Single round-trip: SELECT ... FOR UPDATE that also re-hydrates the entity.
+        $this->getEntityManager()->refresh($code, \Doctrine\DBAL\LockMode::PESSIMISTIC_WRITE);
+    }
+
     public function deleteAllForUser(User $user): int
     {
         $em = $this->getEntityManager();

app/Strategies/MFA/AbstractMFAChallengeStrategy.php

@@ -13,6 +13,7 @@ abstract class AbstractMFAChallengeStrategy implements IMFAChallengeStrategy
     private const KEY_USER_ID           = '2fa_pending_user_id';
     private const KEY_PENDING_AT        = '2fa_pending_at';
     private const KEY_REMEMBER          = '2fa_remember';
+    private const KEY_CLIENT_ID         = '2fa_pending_client_id';
     private const KEY_RECOVERY_ATTEMPTS = '2fa_recovery_attempts';
 
     public function __construct(protected IUserRecoveryCodeRepository $recovery_code_repository) {}
@@ -35,6 +36,7 @@ public function getPendingState(): ?array
             'user_id'    => $user_id,
             'pending_at' => $pending_at,
             'remember'   => Session::get(self::KEY_REMEMBER, false),
+            'client_id'  => Session::get(self::KEY_CLIENT_ID),
         ];
     }
 
@@ -43,13 +45,22 @@ public function clearPendingState(): void
         Session::remove(self::KEY_USER_ID);
         Session::remove(self::KEY_PENDING_AT);
         Session::remove(self::KEY_REMEMBER);
+        Session::remove(self::KEY_CLIENT_ID);
         Session::remove(self::KEY_RECOVERY_ATTEMPTS);
     }
 
     public function verifyRecoveryCode(User $user, string $code): void
     {
         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
+                // used_at before mutating. This closes the check->markUsed race
+                // window: a second concurrent submitter blocks on the lock and, on
+                // resume, sees the code already used instead of double-spending it.
+                $this->recovery_code_repository->refreshExclusiveLock($recoveryCode);
+                if ($recoveryCode->isUsed()) {
+                    throw new AuthenticationException("Invalid recovery code.");
+                }
                 $recoveryCode->markUsed();
                 $this->recovery_code_repository->add($recoveryCode, false);
                 return;
@@ -58,11 +69,16 @@ public function verifyRecoveryCode(User $user, string $code): void
         throw new AuthenticationException("Invalid recovery code.");
     }
 
-    protected function storePendingState(int $userId, bool $remember): void
+    protected function storePendingState(int $userId, bool $remember, ?string $clientId = null): void
     {
         Session::put(self::KEY_USER_ID,    $userId);
         Session::put(self::KEY_PENDING_AT, time());
         Session::put(self::KEY_REMEMBER,   $remember);
+        if (is_null($clientId)) {
+            Session::remove(self::KEY_CLIENT_ID);
+        } else {
+            Session::put(self::KEY_CLIENT_ID, $clientId);
+        }
     }
 
     public function verifyChallenge(User $user, string $code, ?Client $client = null): void

app/Strategies/MFA/EmailOTPMFAChallengeStrategy.php

@@ -20,7 +20,9 @@ public function __construct(
 
     public function issueChallenge(User $user, ?Client $client, bool $remember): array
     {
-        $this->storePendingState($user->getId(), $remember);
+        // Carry the issuing client into the pending state so verification scopes
+        // the OTP lookup and sibling-revoke to the same client (see verifyChallenge).
+        $this->storePendingState($user->getId(), $remember, $client?->getClientId());
 
         $otp = $this->token_service->createOTPFromPayload([
             OAuth2Protocol::OAuth2PasswordlessConnection => OAuth2Protocol::OAuth2PasswordlessConnectionEmail,
@@ -38,6 +40,8 @@ public function verifyChallenge(User $user, string $code, ?Client $client = null
     {
         // Look up the STORED single-use code so the submitted value is actually
         // validated against what was issued (a non-matching code resolves to null).
+        // Scope the lookup to the issuing client so an MFA OTP is only matched
+        // against the client it was issued for.
         $otp = $this->otp_repository->getByValueConnectionAndUserName(
             $code,
             OAuth2Protocol::OAuth2PasswordlessConnectionEmail,
@@ -59,10 +63,23 @@ public function verifyChallenge(User $user, string $code, ?Client $client = null
             throw new AuthenticationException("Verification code is not valid.");
         }
 
+        // Concurrency: acquire a PESSIMISTIC_WRITE row lock and re-hydrate redemption
+        // state before redeeming, mirroring AuthService::finalizeRedemption(). This
+        // closes the validate->redeem race so two concurrent submissions of the same
+        // valid code cannot both succeed. Runs inside the verifyMFAChallenge tx.
+        if ($otp->getConnection() !== OAuth2Protocol::OAuth2PasswordlessConnectionInline) {
+            $this->otp_repository->refreshExclusiveLock($otp);
+            if ($otp->isRedeemed()) {
+                throw new AuthenticationException("Verification code is already redeemed.");
+            }
+        }
+
         $otp->redeem();
         $this->otp_repository->add($otp, false);
 
-        foreach ($this->otp_repository->getByUserNameNotRedeemed($user->getEmail()) as $otpToRevoke) {
+        // Revoke other pending OTPs for this user, scoped to the same client so we
+        // never burn unrelated OTPs (e.g. passwordless-login codes for other clients).
+        foreach ($this->otp_repository->getByUserNameNotRedeemed($user->getEmail(), $client) as $otpToRevoke) {
             if ($otpToRevoke->getValue() !== $otp->getValue()) {
                 $otpToRevoke->redeem();
                 $this->otp_repository->add($otpToRevoke, false);

app/libs/Auth/AuthService.php

@@ -447,6 +447,13 @@ public function loginUser(User $user, bool $remember): void
         Log::debug("AuthService::loginUser");
         if (!$user->canLogin())
             throw new AuthenticationException("User is not active or cannot login.");
+
+        $this->principal_service->clear();
+        $this->principal_service->register
+        (
+            $user->getId(),
+            time()
+        );
         Auth::login($user, $remember);
     }
 
@@ -789,10 +796,15 @@ public function issueMFAChallenge(
     public function verifyMFAChallenge(
         User $user,
         IMFAChallengeStrategy $strategy,
-        string $value
+        string $value,
+        ?Client $client = null
     ): void {
-        $this->tx_service->transaction(function () use ($user, $strategy, $value) {
-            $strategy->verifyChallenge($user, $value);
+        // Commits the OTP redeem (+ sibling revoke) as a single tx. Trusted-device
+        // enrollment and audit are applied by the caller as best-effort,
+        // non-blocking side effects after this commits, so a failure in either
+        // does not block (or roll back) an already-verified second factor.
+        $this->tx_service->transaction(function () use ($user, $strategy, $value, $client) {
+            $strategy->verifyChallenge($user, $value, $client);
         });
     }
 

app/libs/Auth/Repositories/IUserRecoveryCodeRepository.php

@@ -22,6 +22,13 @@ interface IUserRecoveryCodeRepository extends IBaseRepository
      */
     public function getUnusedByUser(User $user): array;
 
+    /**
+     * Acquires a PESSIMISTIC_WRITE row lock on the given recovery code and
+     * re-hydrates its used_at state in the same round-trip. Required before
+     * redeeming a recovery code to close the check->markUsed double-spend race.
+     */
+    public function refreshExclusiveLock(UserRecoveryCode $code): void;
+
     /**
      * Delete every recovery code for a user (used when regenerating).
      */

app/libs/Utils/Services/IAuthService.php

@@ -205,7 +205,8 @@ public function issueMFAChallenge(
     public function verifyMFAChallenge(
         User $user,
         IMFAChallengeStrategy $strategy,
-        string $value
+        string $value,
+        ?Client $client = null
     ): void;
 
     public function verifyMFARecoveryCode(

storage/framework/cache/data/.gitignore

@@ -17,6 +17,7 @@
 use App\libs\Auth\Models\UserRecoveryCode;
 use App\libs\Auth\Models\UserTrustedDevice;
 use App\Services\Auth\IDeviceTrustService;
+use App\Services\Auth\ITwoFactorAuditService;
 use Auth\AuthHelper;
 use Auth\User;
 use Illuminate\Support\Facades\App;
@@ -239,6 +240,58 @@ public function testTrustedDeviceCookieBypassesMFA(): void
         $this->assertTrue(Auth::check(), 'a valid trusted-device cookie must bypass MFA');
     }
 
+    // -------------------------------------------------------------------------
+    // post-verify transaction boundary (Task 5: device-trust atomic, audit best-effort)
+    // -------------------------------------------------------------------------
+
+    public function testAuditFailureDoesNotBlockLogin(): void
+    {
+        // Audit is best-effort: a failure emitting challenge_succeeded must NOT
+        // 500 a user whose OTP is already redeemed and session established.
+        $auditMock = \Mockery::mock(ITwoFactorAuditService::class);
+        $auditMock->shouldReceive('log')
+            ->andReturnUsing(function (User $user, string $eventType) {
+                // Allow challenge_issued (postLogin) so the challenge is created;
+                // blow up only on the post-success event.
+                if ($eventType === TwoFactorAuditLog::EventChallengeSucceeded) {
+                    throw new \Exception('audit sink unavailable');
+                }
+            });
+        $this->app->instance(ITwoFactorAuditService::class, $auditMock);
+
+        $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD);
+        $code = $this->latestOtpCode(self::ADMIN_EMAIL);
+
+        $response = $this->verify($code);
+
+        $this->assertEquals(302, $response->getStatusCode(), 'a best-effort audit failure must not fail the login');
+        $this->assertTrue(Auth::check(), 'session must be established despite the audit failure');
+    }
+
+    public function testDeviceTrustFailureDoesNotBlockLogin(): void
+    {
+        // Device-trust enrollment is best-effort: by the time it runs the OTP is
+        // already redeemed and the session established, so a failure must NOT 500
+        // the user (which would lock them out on retry against a now-burned OTP),
+        // and the pending MFA state must still be cleared.
+        $deviceTrustMock = \Mockery::mock(IDeviceTrustService::class);
+        // Gate path: no cookie -> not trusted, so the challenge is still issued.
+        $deviceTrustMock->shouldReceive('isDeviceTrusted')->andReturn(false);
+        // Enrollment blows up AFTER the OTP has been redeemed and the session set.
+        $deviceTrustMock->shouldReceive('trustDevice')
+            ->andThrow(new \Exception('trusted-device store unavailable'));
+        $this->app->instance(IDeviceTrustService::class, $deviceTrustMock);
+
+        $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD);
+        $code = $this->latestOtpCode(self::ADMIN_EMAIL);
+
+        $response = $this->verify($code, true); // trust_device = true
+
+        $this->assertEquals(302, $response->getStatusCode(), 'a best-effort device-trust failure must not fail the login');
+        $this->assertTrue(Auth::check(), 'session must be established despite the device-trust failure');
+        $this->assertNull(Session::get('2fa_pending_user_id'), 'pending MFA state must be cleared even when device-trust enrollment fails');
+    }
+
     // -------------------------------------------------------------------------
     // recovery codes
     // -------------------------------------------------------------------------
@@ -311,6 +364,21 @@ public function testVerifyRateLimitBlocksAfterThreshold(): void
         $this->assertSame('mfa_rate_limit', $payload['error_code']);
     }
 
+    public function testRecoveryRateLimitBlocksAfterThreshold(): void
+    {
+        $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD);
+
+        $max = (int) Config::get('two_factor.rate_limit.max_attempts');
+        for ($i = 0; $i < $max; $i++) {
+            $this->recovery('bad-recovery-' . $i);
+        }
+
+        $response = $this->recovery('bad-recovery-final');
+        $this->assertResponseStatus(429);
+        $payload = json_decode($response->getContent(), true);
+        $this->assertSame('mfa_rate_limit', $payload['error_code']);
+    }
+
     public function testResendRateLimitBlocksAfterThreshold(): void
     {
         $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD);