From f4931445b096043579497a58c430a0eb802beff7 Mon Sep 17 00:00:00 2001 From: matiasperrone-exo Date: Wed, 3 Jun 2026 20:31:59 +0000 Subject: [PATCH 01/14] feat: UserController MFA Integration, Device Trust Cookie Management, Audit Wiring, and 2FA Rate Limiting --- .../Controllers/Traits/MFACookieManager.php | 86 ++++ app/Http/Controllers/UserController.php | 329 +++++++++++++- app/Http/Kernel.php | 1 + app/Http/Middleware/EncryptCookies.php | 13 +- .../TwoFactorRateLimitMiddleware.php | 145 ++++++ app/Services/Auth/DeviceTrustService.php | 31 +- app/Services/Auth/TwoFactorAuditService.php | 8 +- .../MFA/AbstractMFAChallengeStrategy.php | 1 + .../MFA/EmailOTPMFAChallengeStrategy.php | 12 +- app/libs/Auth/AuthService.php | 56 ++- app/libs/Utils/Services/IAuthService.php | 28 ++ config/two_factor.php | 19 + phpunit.xml | 1 + routes/web.php | 5 + tests/DeviceTrustServiceTest.php | 47 +- tests/TwoFactorLoginFlowTest.php | 426 ++++++++++++++++++ .../MFA/AbstractMFAChallengeStrategyTest.php | 1 + .../MFA/EmailOTPMFAChallengeStrategyTest.php | 30 ++ tests/Unit/TwoFactorAuditServiceTest.php | 44 +- .../AuthServiceValidateCredentialsTest.php | 8 +- 20 files changed, 1242 insertions(+), 49 deletions(-) create mode 100644 app/Http/Controllers/Traits/MFACookieManager.php create mode 100644 app/Http/Middleware/TwoFactorRateLimitMiddleware.php create mode 100644 tests/TwoFactorLoginFlowTest.php diff --git a/app/Http/Controllers/Traits/MFACookieManager.php b/app/Http/Controllers/Traits/MFACookieManager.php new file mode 100644 index 00000000..718b9305 --- /dev/null +++ b/app/Http/Controllers/Traits/MFACookieManager.php @@ -0,0 +1,86 @@ +device_trust_service. + * + * @package App\Http\Controllers\Traits + */ +trait MFACookieManager +{ + /** + * Reads the raw trusted-device token from the request cookie. + * + * @return string|null + */ + protected function getCookieToken(): ?string + { + return Request::cookie(Config::get('two_factor.cookie_name', 'device_trust_token')); + } + + /** + * Persists a trusted-device record (via IDeviceTrustService) and queues a + * secure, HttpOnly cookie carrying the raw token for the configured lifetime. + * + * @param User $user + * @return void + */ + protected function queueDeviceTrustCookie(User $user): void + { + $rawToken = $this->device_trust_service->trustDevice + ( + $user, + Request::header('User-Agent') ?? '', + IPHelper::getUserIp() + ); + + $name = Config::get('two_factor.cookie_name', 'device_trust_token'); + $lifetimeMinutes = intval(Config::get('two_factor.device_trust_lifetime_days', 30)) * 24 * 60; + $path = Config::get('session.path'); + $domain = Config::get('session.domain'); + $secure = true; + $httpOnly = true; + $raw = false; + $sameSite = 'lax'; + + // Same order as \Illuminate\Cookie\CookieJar::make() + Cookie::queue + ( + $name, + $rawToken, // value + $lifetimeMinutes, + $path, + $domain, + $secure, + $httpOnly, + $raw, + $sameSite + + ); + } +} diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index 3d7c1213..9644ca91 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -17,7 +17,17 @@ use App\Jobs\RevokeUserGrantsOnExplicitLogout; use App\Http\Controllers\OpenId\OpenIdController; use App\Http\Controllers\Traits\JsonResponses; +use App\Http\Controllers\Traits\MFACookieManager; use App\Http\Utils\CountryList; +use App\libs\Auth\Models\TwoFactorAuditLog; +use App\Services\Auth\IDeviceTrustService; +use App\Services\Auth\ITwoFactorAuditService; +use App\Services\Auth\ITwoFactorGateService; +use Auth\User; +use Models\OAuth2\Client; +use Strategies\MFA\MFAChallengeStrategyFactory; +use Symfony\Component\HttpFoundation\Response as HttpResponse; +use Utils\IPHelper; use App\libs\OAuth2\Strategies\LoginHintProcessStrategy; use App\ModelSerializers\SerializerRegistry; use Auth\Exceptions\AuthenticationException; @@ -132,6 +142,21 @@ final class UserController extends OpenIdController */ private $security_context_service; + /** + * @var IDeviceTrustService + */ + private $device_trust_service; + + /** + * @var ITwoFactorAuditService + */ + private $two_factor_audit_service; + + /** + * @var ITwoFactorGateService + */ + private $mfa_gate_service; + /** * @param IMementoOpenIdSerializerService $openid_memento_service * @param IMementoOAuth2SerializerService $oauth2_memento_service @@ -167,7 +192,10 @@ public function __construct IResourceServerService $resource_server_service, IUtilsServerConfigurationService $utils_configuration_service, ISecurityContextService $security_context_service, - LoginHintProcessStrategy $login_hint_process_strategy + LoginHintProcessStrategy $login_hint_process_strategy, + IDeviceTrustService $device_trust_service, + ITwoFactorAuditService $two_factor_audit_service, + ITwoFactorGateService $mfa_gate_service, ) { $this->openid_memento_service = $openid_memento_service; @@ -185,6 +213,9 @@ public function __construct $this->resource_server_service = $resource_server_service; $this->utils_configuration_service = $utils_configuration_service; $this->security_context_service = $security_context_service; + $this->device_trust_service = $device_trust_service; + $this->two_factor_audit_service = $two_factor_audit_service; + $this->mfa_gate_service = $mfa_gate_service; $this->middleware(function ($request, $next) use($login_hint_process_strategy){ @@ -254,6 +285,8 @@ public function cancelLogin() use JsonResponses; + use MFACookieManager; + /** * @return \Illuminate\Http\JsonResponse|mixed */ @@ -436,35 +469,41 @@ public function postLogin() $connection = $data['connection'] ?? null; try { - if ($flow == "password" && $this->auth_service->login($username, $password, $remember)) { - return $this->login_strategy->postLogin(); - } - - if ($flow == "otp") { - - $client = null; - - // check if we have a former oauth2 request - if ($this->oauth2_memento_service->exists()) { - - Log::debug("UserController::postLogin exist a oauth auth request on session"); - - $oauth_auth_request = OAuth2AuthorizationRequestFactory::getInstance()->build - ( - OAuth2Message::buildFromMemento($this->oauth2_memento_service->load()) + if ($flow == "password") { + // Validate credentials WITHOUT establishing a session, so the + // MFA gate can run before the user is authenticated. + $user = $this->auth_service->validateCredentials($username, $password); + + $cookieToken = $this->getCookieToken(); + + if ($this->mfa_gate_service->requiresChallenge($user, $cookieToken)) { + // Issue a challenge and stop short of session creation. + $client = $this->resolveClientFromMemento(); + $method = $user->getTwoFactorMethod(); + $strategy = MFAChallengeStrategyFactory::create($method); + $payload = $this->auth_service->issueMFAChallenge($user, $strategy, $client, $remember); + + $this->two_factor_audit_service->log( + $user, + TwoFactorAuditLog::EventChallengeIssued, + $method, + IPHelper::getUserIp() ); - if ($oauth_auth_request->isValid()) { + return Response::json( + array_merge(['error_code' => 'mfa_required'], $payload), + HttpResponse::HTTP_OK + ); + } - $client_id = $oauth_auth_request->getClientId(); + // No challenge required: establish the session and continue. + $this->auth_service->loginUser($user, $remember); + return $this->login_strategy->postLogin(); + } - $client = $this->client_repository->getClientById($client_id); - if (is_null($client)) - throw new ValidationException("client does not exists"); + if ($flow == "otp") { - $this->oauth2_memento_service->serialize($oauth_auth_request->getMessage()->createMemento()); - } - } + $client = $this->resolveClientFromMemento(); $otpClaim = OAuth2OTP::fromParams($username, $connection, $password); $this->auth_service->loginWithOTP($otpClaim, $client); @@ -558,6 +597,246 @@ public function postLogin() } } + /** + * Resolves the OAuth2 client from a former authorization request stored in + * the session memento, if any. Returns null when there is no pending OAuth2 + * request (e.g. plain IdP login). + * + * @return Client|null + * @throws ValidationException + */ + private function resolveClientFromMemento(): ?Client + { + if (!$this->oauth2_memento_service->exists()) { + return null; + } + + Log::debug("UserController::resolveClientFromMemento exist a oauth auth request on session"); + + $oauth_auth_request = OAuth2AuthorizationRequestFactory::getInstance()->build + ( + OAuth2Message::buildFromMemento($this->oauth2_memento_service->load()) + ); + + if (!$oauth_auth_request->isValid()) { + return null; + } + + $client = $this->client_repository->getClientById($oauth_auth_request->getClientId()); + if (is_null($client)) + throw new ValidationException("client does not exists"); + + $this->oauth2_memento_service->serialize($oauth_auth_request->getMessage()->createMemento()); + + return $client; + } + + /** + * Verifies a 2FA OTP challenge and, on success, establishes the session. + * + * @return \Illuminate\Http\JsonResponse|mixed + */ + public function verify2FA() + { + try { + $data = Request::all(); + $validator = Validator::make($data, [ + 'otp_value' => 'required|string', + 'method' => 'required|string|in:' . implode(',', User::ValidMFAMethods), + 'trust_device' => 'sometimes|boolean', + ]); + + if (!$validator->passes()) { + return $this->error412($validator->getMessageBag()->getMessages()); + } + + $method = $data['method']; + $otp_value = $data['otp_value']; + $trust_device = Request::boolean('trust_device'); + + $strategy = MFAChallengeStrategyFactory::create($method); + $pending = $strategy->getPendingState(); + + if (is_null($pending)) { + return $this->mfaSessionExpired(); + } + + $user = $this->auth_service->getUserById((int) $pending['user_id']); + if (is_null($user) || !$user->isTwoFactorMethodEnabled($method)) { + $strategy->clearPendingState(); + return $this->mfaSessionExpired(); + } + + try { + $this->auth_service->verifyMFAChallenge($user, $strategy, $otp_value); + } catch (AuthenticationException $ex) { + Log::warning($ex); + // Re-fetch user: the tx wrapper closed/reset the EM on failure, detaching the entity. + $userId = (int) $pending['user_id']; + $user = $this->auth_service->getUserById($userId) ?? $user; + $this->two_factor_audit_service->log( + $user, + TwoFactorAuditLog::EventChallengeFailed, + $method, + IPHelper::getUserIp() + ); + return Response::json(['error_code' => 'mfa_verification_failed'], HttpResponse::HTTP_UNAUTHORIZED); + } + + // Second factor verified: establish the session. + $this->auth_service->loginUser($user, (bool) $pending['remember']); + + if ($trust_device) { + $this->queueDeviceTrustCookie($user); + } + + $strategy->clearPendingState(); + + $this->two_factor_audit_service->log( + $user, + TwoFactorAuditLog::EventChallengeSucceeded, + $method, + IPHelper::getUserIp() + ); + + return $this->login_strategy->postLogin(); + } catch (ValidationException $ex) { + Log::warning($ex); + return $this->error412($ex->getMessages()); + } catch (Exception $ex) { + Log::error($ex); + return $this->error500($ex); + } + } + + /** + * Verifies a 2FA recovery code and, on success, establishes the session. + * + * @return \Illuminate\Http\JsonResponse|mixed + */ + public function verify2FARecovery() + { + try { + $data = Request::all(); + $validator = Validator::make($data, [ + 'recovery_code' => 'required|string', + ]); + + if (!$validator->passes()) { + return $this->error412($validator->getMessageBag()->getMessages()); + } + + $recovery_code = $data['recovery_code']; + + // Recovery-code handling lives in the base strategy; session keys are + // method-agnostic, so any concrete strategy can read the pending state. + $strategy = MFAChallengeStrategyFactory::create(User::MFAMethod_OTP); + $pending = $strategy->getPendingState(); + + if (is_null($pending)) { + return $this->mfaSessionExpired(); + } + + $user = $this->auth_service->getUserById((int) $pending['user_id']); + if (is_null($user)) { + $strategy->clearPendingState(); + return $this->mfaSessionExpired(); + } + + try { + $this->auth_service->verifyMFARecoveryCode($user, $strategy, $recovery_code); + } catch (AuthenticationException $ex) { + Log::warning($ex); + // Re-fetch user: the tx wrapper closed/reset the EM on failure, detaching the entity. + $userId = (int) $pending['user_id']; + $user = $this->auth_service->getUserById($userId) ?? $user; + $this->two_factor_audit_service->log( + $user, + TwoFactorAuditLog::EventChallengeFailed, + TwoFactorAuditLog::MethodRecovery, + IPHelper::getUserIp() + ); + return Response::json(['error_code' => 'mfa_invalid_recovery'], HttpResponse::HTTP_UNAUTHORIZED); + } + + $this->auth_service->loginUser($user, (bool) $pending['remember']); + $strategy->clearPendingState(); + + $this->two_factor_audit_service->log( + $user, + TwoFactorAuditLog::EventRecoveryUsed, + TwoFactorAuditLog::MethodRecovery, + IPHelper::getUserIp() + ); + + return $this->login_strategy->postLogin(); + } catch (ValidationException $ex) { + Log::warning($ex); + return $this->error412($ex->getMessages()); + } catch (Exception $ex) { + Log::error($ex); + return $this->error500($ex); + } + } + + /** + * Re-issues a 2FA challenge for the pending login and returns the challenge payload. + * + * @return \Illuminate\Http\JsonResponse|mixed + */ + public function resend2FA() + { + try { + $data = Request::all(); + $validator = Validator::make($data, [ + 'method' => 'required|string|in:' . implode(',', User::ValidMFAMethods), + ]); + + if (!$validator->passes()) { + return $this->error412($validator->getMessageBag()->getMessages()); + } + + $method = $data['method']; + $strategy = MFAChallengeStrategyFactory::create($method); + $pending = $strategy->getPendingState(); + + if (is_null($pending)) { + return $this->mfaSessionExpired(); + } + + $user = $this->auth_service->getUserById((int) $pending['user_id']); + if (is_null($user) || !$user->isTwoFactorMethodEnabled($method)) { + $strategy->clearPendingState(); + return $this->mfaSessionExpired(); + } + + $payload = $this->auth_service->resendMFAChallenge($user, $strategy, $this->resolveClientFromMemento(), (bool) $pending['remember']); + + $this->two_factor_audit_service->log( + $user, + TwoFactorAuditLog::EventChallengeIssued, + $method, + IPHelper::getUserIp() + ); + + return $this->ok($payload); + } catch (ValidationException $ex) { + Log::warning($ex); + return $this->error412($ex->getMessages()); + } catch (Exception $ex) { + Log::error($ex); + return $this->error500($ex); + } + } + + /** + * @return \Illuminate\Http\JsonResponse + */ + private function mfaSessionExpired() + { + return Response::json(['error_code' => 'mfa_session_expired'], HttpResponse::HTTP_UNAUTHORIZED); + } + /** * @return \Illuminate\Http\Response|mixed */ diff --git a/app/Http/Kernel.php b/app/Http/Kernel.php index d81fc8df..6e3c84df 100644 --- a/app/Http/Kernel.php +++ b/app/Http/Kernel.php @@ -75,6 +75,7 @@ class Kernel extends HttpKernel 'guest' => \App\Http\Middleware\RedirectIfAuthenticated::class, 'throttle' => \Illuminate\Routing\Middleware\ThrottleRequestsWithRedis::class, 'csrf' => \App\Http\Middleware\VerifyCsrfToken::class, + '2fa.rate' => \App\Http\Middleware\TwoFactorRateLimitMiddleware::class, 'oauth2.endpoint' => \App\Http\Middleware\OAuth2BearerAccessTokenRequestValidator::class, 'oauth2.currentuser.serveradmin' => \App\Http\Middleware\CurrentUserIsOAuth2ServerAdmin::class, 'oauth2.currentuser.serveradmin.json' => \App\Http\Middleware\CurrentUserIsOAuth2ServerAdminJson::class, diff --git a/app/Http/Middleware/EncryptCookies.php b/app/Http/Middleware/EncryptCookies.php index a613dc08..c682095f 100644 --- a/app/Http/Middleware/EncryptCookies.php +++ b/app/Http/Middleware/EncryptCookies.php @@ -11,6 +11,7 @@ * See the License for the specific language governing permissions and * limitations under the License. **/ +use Illuminate\Contracts\Encryption\Encrypter; use Illuminate\Cookie\Middleware\EncryptCookies as Middleware; use OAuth2\Services\IPrincipalService; /** @@ -22,10 +23,20 @@ class EncryptCookies extends Middleware /** * The names of the cookies that should not be encrypted. * + * The trusted-device token is a high-entropy random secret only ever compared + * against a server-side SHA-256 hash, so cookie-layer encryption adds no + * meaningful protection - exclude it so the value round-trips verbatim. + * * @var array */ protected $except = [ - IPrincipalService::OP_BROWSER_STATE_COOKIE_NAME + IPrincipalService::OP_BROWSER_STATE_COOKIE_NAME, ]; + public function __construct(Encrypter $encrypter) + { + parent::__construct($encrypter); + $this->except[] = config('two_factor.cookie_name', 'device_trust_token'); + } + } diff --git a/app/Http/Middleware/TwoFactorRateLimitMiddleware.php b/app/Http/Middleware/TwoFactorRateLimitMiddleware.php new file mode 100644 index 00000000..a0bba142 --- /dev/null +++ b/app/Http/Middleware/TwoFactorRateLimitMiddleware.php @@ -0,0 +1,145 @@ +limitsFor($action); + $key = $this->cacheKey($action, $userId); + + if ((int) Cache::get($key, 0) >= $maxAttempts) { + Log::debug(sprintf("TwoFactorRateLimitMiddleware: action %s user %s rate limited", $action, $userId)); + return Response::json( + [ + 'error_code' => 'mfa_rate_limit', + 'error_message' => 'Too many attempts. Please try again later.', + ], + HttpResponse::HTTP_TOO_MANY_REQUESTS + ); + } + + $response = $next($request); + + if ($action === self::ActionResend) { + $this->increment($key, $windowSeconds); + } else if ($this->isFailure($response)) { + $this->increment($key, $windowSeconds); + } + + return $response; + } + + /** + * @param string $action + * @return array{0:int,1:int} [maxAttempts, windowSeconds] + */ + private function limitsFor(string $action): array + { + if ($action === self::ActionResend) { + return [ + (int) Config::get('two_factor.rate_limit.max_otp_requests', 5), + (int) Config::get('two_factor.rate_limit.otp_window_minutes', 15) * 60, + ]; + } + + return [ + (int) Config::get('two_factor.rate_limit.max_attempts', 3), + (int) Config::get('two_factor.rate_limit.window_seconds', 900), + ]; + } + + private function cacheKey(string $action, $userId): string + { + return sprintf('2fa_rate:%s:%s', $action, $userId); + } + + /** + * Increment within a fixed window: add() sets the TTL once (only if the key + * is absent), increment() bumps the value while preserving that TTL, so the + * window starts at the first hit and does not slide. + */ + private function increment(string $key, int $windowSeconds): void + { + Cache::add($key, 0, $windowSeconds); + Cache::increment($key); + } + + /** + * @param mixed $response + * @return bool + */ + private function isFailure($response): bool + { + $content = method_exists($response, 'getContent') ? $response->getContent() : null; + if (empty($content)) { + return false; + } + + $decoded = json_decode($content, true); + if (!is_array($decoded) || !isset($decoded['error_code'])) { + return false; + } + + return in_array($decoded['error_code'], self::FAILURE_CODES, true); + } +} diff --git a/app/Services/Auth/DeviceTrustService.php b/app/Services/Auth/DeviceTrustService.php index e6f317b6..d4242c2f 100644 --- a/app/Services/Auth/DeviceTrustService.php +++ b/app/Services/Auth/DeviceTrustService.php @@ -13,12 +13,15 @@ * limitations under the License. **/ +use App\libs\Auth\Models\TwoFactorAuditLog; use App\libs\Auth\Models\UserTrustedDevice; use Auth\Repositories\IUserTrustedDeviceRepository; use Auth\User; use DateTime; use DateInterval; use DateTimeZone; +use Utils\IPHelper; +use Utils\Db\ITransactionService; /** * Class DeviceTrustService @@ -26,7 +29,11 @@ */ final class DeviceTrustService implements IDeviceTrustService { - public function __construct(private readonly IUserTrustedDeviceRepository $repository) + public function __construct( + private readonly IUserTrustedDeviceRepository $repository, + private readonly ITwoFactorAuditService $audit_service, + private readonly ITransactionService $tx_service + ) { } @@ -55,7 +62,16 @@ public function trustDevice(User $user, string $userAgent, string $ipAddress): s $device->setLastSeenAt(clone $now); $device->setIsRevoked(false); - $this->repository->add($device, true); + $this->tx_service->transaction(function () use ($device) { + $this->repository->add($device, false); + }); + + $this->audit_service->log( + $user, + TwoFactorAuditLog::EventDeviceTrusted, + $user->getTwoFactorMethod(), + $ipAddress + ); return $rawToken; } @@ -74,12 +90,21 @@ public function isDeviceTrusted(User $user, ?string $cookieToken): bool } $device->setLastSeenAt(new DateTime('now', new DateTimeZone('UTC'))); - $this->repository->add($device, true); + $this->tx_service->transaction(function () use ($device) { + $this->repository->add($device, false); + }); return true; } public function removeTrustedDevices(User $user): void { $this->repository->revokeAllForUser($user); + + $this->audit_service->log( + $user, + TwoFactorAuditLog::EventDeviceRevoked, + $user->getTwoFactorMethod(), + IPHelper::getUserIp() + ); } } diff --git a/app/Services/Auth/TwoFactorAuditService.php b/app/Services/Auth/TwoFactorAuditService.php index 9a02586d..a1dfd9f8 100644 --- a/app/Services/Auth/TwoFactorAuditService.php +++ b/app/Services/Auth/TwoFactorAuditService.php @@ -18,6 +18,7 @@ use Auth\Repositories\ITwoFactorAuditLogRepository; use Auth\User; use Illuminate\Support\Facades\Log; +use Utils\Db\ITransactionService; /** * Class TwoFactorAuditService @@ -26,7 +27,8 @@ final class TwoFactorAuditService implements ITwoFactorAuditService { public function __construct( - private readonly ITwoFactorAuditLogRepository $repository + private readonly ITwoFactorAuditLogRepository $repository, + private readonly ITransactionService $tx_service ) { } @@ -53,7 +55,9 @@ public function log(User $user, string $eventType, string $method, string $ipAdd $auditLog->setUserAgent(request()?->userAgent() ?? ''); $auditLog->setMetadata($metadata); - $this->repository->add($auditLog, true); + $this->tx_service->transaction(function () use ($auditLog) { + $this->repository->add($auditLog, false); + }); if (config('opentelemetry.enabled', false)) { EmitAuditLogJob::dispatch('two_factor.audit', [ diff --git a/app/Strategies/MFA/AbstractMFAChallengeStrategy.php b/app/Strategies/MFA/AbstractMFAChallengeStrategy.php index 172ab7e6..ce940857 100644 --- a/app/Strategies/MFA/AbstractMFAChallengeStrategy.php +++ b/app/Strategies/MFA/AbstractMFAChallengeStrategy.php @@ -51,6 +51,7 @@ public function verifyRecoveryCode(User $user, string $code): void foreach ($this->recovery_code_repository->getUnusedByUser($user) as $recoveryCode) { if (Hash::check($code, $recoveryCode->getCodeHash())) { $recoveryCode->markUsed(); + $this->recovery_code_repository->add($recoveryCode, false); return; } } diff --git a/app/Strategies/MFA/EmailOTPMFAChallengeStrategy.php b/app/Strategies/MFA/EmailOTPMFAChallengeStrategy.php index 56a35da2..9c25f2e0 100644 --- a/app/Strategies/MFA/EmailOTPMFAChallengeStrategy.php +++ b/app/Strategies/MFA/EmailOTPMFAChallengeStrategy.php @@ -5,7 +5,6 @@ use Auth\Repositories\IUserRecoveryCodeRepository; use Auth\User; use Models\OAuth2\Client; -use Models\OAuth2\OAuth2OTP; use OAuth2\OAuth2Protocol; use OAuth2\Services\ITokenService; @@ -37,7 +36,14 @@ public function issueChallenge(User $user, ?Client $client, bool $remember): arr public function verifyChallenge(User $user, string $code, ?Client $client = null): void { - $otp = OAuth2OTP::fromParams($user->getEmail(), OAuth2Protocol::OAuth2PasswordlessConnectionEmail, $code); + // 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). + $otp = $this->otp_repository->getByValueConnectionAndUserName( + $code, + OAuth2Protocol::OAuth2PasswordlessConnectionEmail, + $user->getEmail(), + $client + ); if (is_null($otp)) { throw new AuthenticationException("Non existent single-use code."); @@ -54,10 +60,12 @@ public function verifyChallenge(User $user, string $code, ?Client $client = null } $otp->redeem(); + $this->otp_repository->add($otp, false); foreach ($this->otp_repository->getByUserNameNotRedeemed($user->getEmail()) as $otpToRevoke) { if ($otpToRevoke->getValue() !== $otp->getValue()) { $otpToRevoke->redeem(); + $this->otp_repository->add($otpToRevoke, false); } } } diff --git a/app/libs/Auth/AuthService.php b/app/libs/Auth/AuthService.php index ae71663d..7a51d92c 100644 --- a/app/libs/Auth/AuthService.php +++ b/app/libs/Auth/AuthService.php @@ -39,6 +39,7 @@ use OAuth2\Services\ISecurityContextService; use OpenId\Services\IUserService; use Services\IUserActionService; +use Strategies\MFA\IMFAChallengeStrategy; use utils\Base64UrlRepresentation; use Utils\Db\ITransactionService; use Utils\IPHelper; @@ -426,15 +427,10 @@ public function validateCredentials(string $username, string $password): User { Log::debug("AuthService::validateCredentials"); - try { - /** - * @var User|null $user - */ - $user = Auth::getProvider()->retrieveByCredentials(['username' => $username, 'password' => $password]); - } catch (UnverifiedEmailMemberException $ex) { - throw new AuthenticationException($ex->getMessage()); - } - + /** + * @var User|null $user + */ + $user = Auth::getProvider()->retrieveByCredentials(['username' => $username, 'password' => $password]); if (is_null($user) || !$user instanceof User || !$user->canLogin()) { throw new AuthenticationException("We are sorry, your username or password does not match an existing record."); } @@ -778,4 +774,46 @@ public function postLoginUserActions(int $user_id): void }); } + + public function issueMFAChallenge( + User $user, + IMFAChallengeStrategy $strategy, + ?Client $client = null, + bool $remember = false + ): array { + return $this->tx_service->transaction(function () use ($user, $strategy, $client, $remember) { + return $strategy->issueChallenge($user, $client, $remember); + }); + } + + public function verifyMFAChallenge( + User $user, + IMFAChallengeStrategy $strategy, + string $value + ): void { + $this->tx_service->transaction(function () use ($user, $strategy, $value) { + $strategy->verifyChallenge($user, $value); + }); + } + + public function verifyMFARecoveryCode( + User $user, + IMFAChallengeStrategy $strategy, + string $inputCode + ): void { + $this->tx_service->transaction(function () use ($user, $strategy, $inputCode) { + $strategy->verifyRecoveryCode($user, $inputCode); + }); + } + + public function resendMFAChallenge( + User $user, + IMFAChallengeStrategy $strategy, + ?Client $client = null, + bool $remember = false + ): array { + return $this->tx_service->transaction(function () use ($user, $strategy, $client, $remember) { + return $strategy->resendChallenge($user, $client, $remember); + }); + } } \ No newline at end of file diff --git a/app/libs/Utils/Services/IAuthService.php b/app/libs/Utils/Services/IAuthService.php index c3d26e62..1990c2ed 100644 --- a/app/libs/Utils/Services/IAuthService.php +++ b/app/libs/Utils/Services/IAuthService.php @@ -18,6 +18,7 @@ use Models\OAuth2\OAuth2OTP; use OAuth2\Models\IClient; use OpenId\Models\IOpenIdUser; +use Strategies\MFA\IMFAChallengeStrategy; /** * Interface IAuthService */ @@ -66,6 +67,7 @@ public function login(string $username, string $password, bool $remember_me): bo * @param string $password * @return User * @throws AuthenticationException on invalid credentials, missing user, or locked account. + * @throws \Auth\Exceptions\UnverifiedEmailMemberException when the user's email is not verified */ public function validateCredentials(string $username, string $password): User; @@ -193,4 +195,30 @@ public function verifyOTPChallenge( ?Client $client = null ): OAuth2OTP; + public function issueMFAChallenge( + User $user, + IMFAChallengeStrategy $strategy, + ?Client $client = null, + bool $remember = false + ): array; + + public function verifyMFAChallenge( + User $user, + IMFAChallengeStrategy $strategy, + string $value + ): void; + + public function verifyMFARecoveryCode( + User $user, + IMFAChallengeStrategy $strategy, + string $inputCode + ): void; + + public function resendMFAChallenge( + User $user, + IMFAChallengeStrategy $strategy, + ?Client $client = null, + bool $remember = false + ): array; + } \ No newline at end of file diff --git a/config/two_factor.php b/config/two_factor.php index 8a399806..78ac21a0 100644 --- a/config/two_factor.php +++ b/config/two_factor.php @@ -38,4 +38,23 @@ */ 'device_trust_lifetime_days' => env('DEVICE_TRUST_LIFETIME_DAYS', 30), 'cookie_name' => env('DEVICE_TRUST_COOKIE_NAME', 'device_trust_token'), + + /* + |-------------------------------------------------------------------------- + | Rate Limiting + |-------------------------------------------------------------------------- + | + | Counters live in the cache (NOT the session) so they survive session + | cleanup and keep an independent, fixed TTL window. + | + | verify/recovery: max_attempts failed attempts per window_seconds. + | resend: max_otp_requests requests per otp_window_minutes. + | + */ + 'rate_limit' => [ + 'max_attempts' => env('TWO_FACTOR_MAX_ATTEMPTS', 3), + 'window_seconds' => env('TWO_FACTOR_RATE_WINDOW_SECONDS', 900), + 'max_otp_requests' => env('TWO_FACTOR_MAX_OTP_REQUESTS', 5), + 'otp_window_minutes' => env('TWO_FACTOR_OTP_WINDOW_MINUTES', 15), + ], ]; diff --git a/phpunit.xml b/phpunit.xml index 4c4f8d12..5d09a0bb 100644 --- a/phpunit.xml +++ b/phpunit.xml @@ -30,6 +30,7 @@ ./tests/Unit/MFA/MFAChallengeStrategyFactoryTest.php ./tests/Unit/TwoFactorAuditServiceTest.php ./tests/Unit/MFAGateServiceTest.php + ./tests/TwoFactorLoginFlowTest.php diff --git a/routes/web.php b/routes/web.php index b49a3547..5c0f0af0 100644 --- a/routes/web.php +++ b/routes/web.php @@ -49,6 +49,11 @@ Route::group(array('prefix' => 'verification'), function () { Route::post('resend', ['middleware' => ['csrf'], 'uses' => 'UserController@resendVerificationEmail']); }); + Route::group(array('prefix' => '2fa'), function () { + Route::post('verify', ['middleware' => ['csrf', '2fa.rate:verify'], 'uses' => 'UserController@verify2FA']); + Route::post('recovery', ['middleware' => ['csrf', '2fa.rate:recovery'], 'uses' => 'UserController@verify2FARecovery']); + Route::post('resend', ['middleware' => ['csrf', '2fa.rate:resend'], 'uses' => 'UserController@resend2FA']); + }); Route::post('', ['middleware' => 'csrf', 'uses' => 'UserController@postLogin']); Route::get('cancel', "UserController@cancelLogin"); Route::group(array('prefix' => '{provider}'), function () { diff --git a/tests/DeviceTrustServiceTest.php b/tests/DeviceTrustServiceTest.php index 5eb2bdfe..4d8ae8fb 100644 --- a/tests/DeviceTrustServiceTest.php +++ b/tests/DeviceTrustServiceTest.php @@ -15,12 +15,14 @@ use App\libs\Auth\Models\UserTrustedDevice; use App\Services\Auth\DeviceTrustService; +use App\Services\Auth\ITwoFactorAuditService; use Auth\Repositories\IUserTrustedDeviceRepository; use Auth\User; use DateTime; use DateInterval; use DateTimeZone; use Mockery; +use Utils\Db\ITransactionService; /** * Class DeviceTrustServiceTest @@ -33,11 +35,21 @@ final class DeviceTrustServiceTest extends BrowserKitTestCase /** @var \Mockery\MockInterface&IUserTrustedDeviceRepository */ private $repo; + /** @var \Mockery\MockInterface&ITwoFactorAuditService */ + private $audit_service; + + /** @var \Mockery\MockInterface&ITransactionService */ + private $tx_service; + public function setUp(): void { parent::setUp(); $this->repo = Mockery::mock(IUserTrustedDeviceRepository::class); - $this->service = new DeviceTrustService($this->repo); + $this->audit_service = Mockery::mock(ITwoFactorAuditService::class); + $this->audit_service->shouldReceive('log')->byDefault(); + $this->tx_service = Mockery::mock(ITransactionService::class); + $this->tx_service->shouldReceive('transaction')->andReturnUsing(fn($cb) => $cb())->byDefault(); + $this->service = new DeviceTrustService($this->repo, $this->audit_service, $this->tx_service); } public function tearDown(): void @@ -53,6 +65,7 @@ public function tearDown(): void public function testIsDeviceTrustedNullCookie(): void { $user = Mockery::mock(User::class); + $user->shouldReceive('getTwoFactorMethod')->andReturn(User::MFAMethod_OTP); $this->repo->shouldNotReceive('getByUserAndDeviceIdentifier'); $this->assertFalse($this->service->isDeviceTrusted($user, null)); @@ -61,6 +74,7 @@ public function testIsDeviceTrustedNullCookie(): void public function testIsDeviceTrustedEmptyCookie(): void { $user = Mockery::mock(User::class); + $user->shouldReceive('getTwoFactorMethod')->andReturn(User::MFAMethod_OTP); $this->repo->shouldNotReceive('getByUserAndDeviceIdentifier'); $this->assertFalse($this->service->isDeviceTrusted($user, '')); @@ -69,6 +83,7 @@ public function testIsDeviceTrustedEmptyCookie(): void public function testIsDeviceTrustedWrongCookie(): void { $user = Mockery::mock(User::class); + $user->shouldReceive('getTwoFactorMethod')->andReturn(User::MFAMethod_OTP); $this->repo ->shouldReceive('getByUserAndDeviceIdentifier') ->once() @@ -80,6 +95,7 @@ public function testIsDeviceTrustedWrongCookie(): void public function testIsDeviceTrustedRevokedDevice(): void { $user = Mockery::mock(User::class); + $user->shouldReceive('getTwoFactorMethod')->andReturn(User::MFAMethod_OTP); $device = $this->makeDevice(expired: false, revoked: true); @@ -94,6 +110,7 @@ public function testIsDeviceTrustedRevokedDevice(): void public function testIsDeviceTrustedExpiredDevice(): void { $user = Mockery::mock(User::class); + $user->shouldReceive('getTwoFactorMethod')->andReturn(User::MFAMethod_OTP); $device = $this->makeDevice(expired: true, revoked: false); @@ -108,6 +125,7 @@ public function testIsDeviceTrustedExpiredDevice(): void public function testIsDeviceTrustedValidDevice(): void { $user = Mockery::mock(User::class); + $user->shouldReceive('getTwoFactorMethod')->andReturn(User::MFAMethod_OTP); $device = $this->makeDevice(expired: false, revoked: false); @@ -123,6 +141,7 @@ public function testIsDeviceTrustedValidDevice(): void public function testIsDeviceTrustedUpdatesLastSeenAt(): void { $user = Mockery::mock(User::class); + $user->shouldReceive('getTwoFactorMethod')->andReturn(User::MFAMethod_OTP); $device = $this->makeDevice(expired: false, revoked: false); // set last_seen_at to a known old value so the update is detectable @@ -148,6 +167,7 @@ public function testIsDeviceTrustedUpdatesLastSeenAt(): void public function testTrustDeviceReturnsToken(): void { $user = Mockery::mock(User::class); + $user->shouldReceive('getTwoFactorMethod')->andReturn(User::MFAMethod_OTP); $this->repo->shouldReceive('add')->once(); @@ -160,6 +180,7 @@ public function testTrustDeviceReturnsToken(): void public function testTrustDeviceStoresHash(): void { $user = Mockery::mock(User::class); + $user->shouldReceive('getTwoFactorMethod')->andReturn(User::MFAMethod_OTP); /** @var UserTrustedDevice|null $persistedDevice */ $persistedDevice = null; @@ -181,6 +202,7 @@ public function testTrustDeviceStoresHash(): void public function testTrustDeviceRawTokenNotStored(): void { $user = Mockery::mock(User::class); + $user->shouldReceive('getTwoFactorMethod')->andReturn(User::MFAMethod_OTP); /** @var UserTrustedDevice|null $persistedDevice */ $persistedDevice = null; @@ -202,15 +224,32 @@ public function testTrustDeviceRawTokenNotStored(): void public function testTrustDeviceCreatesExactlyOneRecord(): void { $user = Mockery::mock(User::class); + $user->shouldReceive('getTwoFactorMethod')->andReturn(User::MFAMethod_OTP); + + $this->repo->shouldReceive('add')->once(); + + $this->service->trustDevice($user, 'Mozilla/5.0', '127.0.0.1'); + } + + public function testTrustDeviceEmitsDeviceTrustedAuditEvent(): void + { + $user = Mockery::mock(User::class); + $user->shouldReceive('getTwoFactorMethod')->andReturn(User::MFAMethod_OTP); $this->repo->shouldReceive('add')->once(); + $this->audit_service + ->shouldReceive('log') + ->once() + ->with($user, \App\libs\Auth\Models\TwoFactorAuditLog::EventDeviceTrusted, User::MFAMethod_OTP, '127.0.0.1'); + $this->service->trustDevice($user, 'Mozilla/5.0', '127.0.0.1'); } public function testTrustDeviceSetsExpiresAtFromConfig(): void { $user = Mockery::mock(User::class); + $user->shouldReceive('getTwoFactorMethod')->andReturn(User::MFAMethod_OTP); /** @var UserTrustedDevice|null $persistedDevice */ $persistedDevice = null; @@ -239,12 +278,18 @@ public function testTrustDeviceSetsExpiresAtFromConfig(): void public function testRemoveTrustedDevicesRevokesAll(): void { $user = Mockery::mock(User::class); + $user->shouldReceive('getTwoFactorMethod')->andReturn(User::MFAMethod_OTP); $this->repo ->shouldReceive('revokeAllForUser') ->once() ->with($user); + $this->audit_service + ->shouldReceive('log') + ->once() + ->with($user, \App\libs\Auth\Models\TwoFactorAuditLog::EventDeviceRevoked, User::MFAMethod_OTP, Mockery::type('string')); + $this->service->removeTrustedDevices($user); } diff --git a/tests/TwoFactorLoginFlowTest.php b/tests/TwoFactorLoginFlowTest.php new file mode 100644 index 00000000..e8d8521e --- /dev/null +++ b/tests/TwoFactorLoginFlowTest.php @@ -0,0 +1,426 @@ +flushRateLimitCounters(); + } + + protected function tearDown(): void + { + $this->flushRateLimitCounters(); + parent::tearDown(); + } + + private function flushRateLimitCounters(): void + { + $admin = EntityManager::getRepository(User::class)->getByEmailOrName(self::ADMIN_EMAIL); + if (!$admin) return; + $userId = $admin->getId(); + foreach (['verify', 'recovery', 'resend'] as $action) { + Cache::forget("2fa_rate:{$action}:{$userId}"); + } + } + + // ------------------------------------------------------------------------- + // postLogin gate + // ------------------------------------------------------------------------- + + public function testAdminLoginTriggersMFAChallenge(): void + { + $response = $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + + $this->assertResponseStatus(200); + $payload = json_decode($response->getContent(), true); + $this->assertSame('mfa_required', $payload['error_code']); + $this->assertFalse(Auth::check(), 'no session must be established when a challenge is required'); + + $admin = $this->user(self::ADMIN_EMAIL); + $this->assertGreaterThan(0, $this->countAudit($admin->getId(), TwoFactorAuditLog::EventChallengeIssued)); + } + + public function testNonAdminWithoutMFALogsInNormally(): void + { + $email = $this->createPlainUser(); + + $response = $this->postLogin($email, self::SEED_PASSWORD); + + $this->assertResponseStatus(302); + $this->assertTrue(Auth::check(), 'a non-MFA user must get an authenticated session'); + } + + // ------------------------------------------------------------------------- + // verify2FA + // ------------------------------------------------------------------------- + + public function testSuccessfulOTPVerificationCompletesLogin(): void + { + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + $code = $this->latestOtpCode(self::ADMIN_EMAIL); + + $response = $this->verify($code); + + $this->assertResponseStatus(302); + $this->assertTrue(Auth::check()); + + $admin = $this->user(self::ADMIN_EMAIL); + $this->assertGreaterThan(0, $this->countAudit($admin->getId(), TwoFactorAuditLog::EventChallengeSucceeded)); + } + + public function testFailedOTPVerificationReturnsErrorAndIncrementsCounter(): void + { + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + $admin = $this->user(self::ADMIN_EMAIL); + $userId = $admin->getId(); + + $response = $this->verify('000000-wrong'); + + $this->assertResponseStatus(401); + $payload = json_decode($response->getContent(), true); + $this->assertSame('mfa_verification_failed', $payload['error_code']); + $this->assertFalse(Auth::check()); + + $this->assertSame(1, (int) Cache::get('2fa_rate:verify:' . $userId, 0), 'verify counter must increment on failure'); + $this->assertGreaterThan(0, $this->countAudit($userId, TwoFactorAuditLog::EventChallengeFailed)); + } + + public function testSuccessfulVerificationDoesNotIncrementCounter(): void + { + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + $userId = $this->user(self::ADMIN_EMAIL)->getId(); + $code = $this->latestOtpCode(self::ADMIN_EMAIL); + + $this->verify($code); + + $this->assertSame(0, (int) Cache::get('2fa_rate:verify:' . $userId, 0), 'success must NOT increment the verify counter'); + } + + public function testOTPVerificationRejectsWrongCode(): void + { + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + // Confirm there is a real OTP issued, then send a wrong value. + $this->latestOtpCode(self::ADMIN_EMAIL); // asserts an OTP exists + $wrongCode = 'WRONG-CODE-THAT-DOES-NOT-EXIST'; + + $response = $this->verify($wrongCode); + + $this->assertResponseStatus(401); + $payload = json_decode($response->getContent(), true); + $this->assertSame('mfa_verification_failed', $payload['error_code'], + 'verifyChallenge must load the stored OTP and reject a non-matching value'); + $this->assertFalse(Auth::check()); + } + + public function testOTPCodeRejectsReuseAfterSuccessfulVerification(): void + { + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + $code = $this->latestOtpCode(self::ADMIN_EMAIL); + + // First use — must succeed. + $this->verify($code); + $this->assertTrue(Auth::check(), 'first OTP use must establish a session'); + + // Second use — OTP must be redeemed (committed by the AuthService tx). + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + $response = $this->verify($code); + + $this->assertResponseStatus(401); + $payload = json_decode($response->getContent(), true); + $this->assertSame('mfa_verification_failed', $payload['error_code'], + 'a reused OTP must be rejected because the redemption was committed by the AuthService transaction'); + } + + public function testRecoveryCodeRejectsReuseAfterTransactionCommit(): void + { + $admin = $this->user(self::ADMIN_EMAIL); + $plain = 'RECOVERY-REUSE-TX-' . uniqid(); + $this->createRecoveryCode($admin, $plain, false); + + // First use — must succeed. + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + $this->recovery($plain); + $this->assertTrue(Auth::check(), 'first recovery-code use must establish a session'); + + // Second use — used_at marking must have been committed by the AuthService tx. + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + $response = $this->recovery($plain); + + $this->assertResponseStatus(401); + $payload = json_decode($response->getContent(), true); + $this->assertSame('mfa_invalid_recovery', $payload['error_code'], + 'recovery code reuse must be rejected because used_at was committed via the AuthService transaction'); + } + + public function testExpiredMFASessionFails(): void + { + // No prior postLogin -> no pending state. + $response = $this->verify('whatever'); + + $this->assertResponseStatus(401); + $payload = json_decode($response->getContent(), true); + $this->assertSame('mfa_session_expired', $payload['error_code']); + } + + // ------------------------------------------------------------------------- + // trusted device + // ------------------------------------------------------------------------- + + public function testTrustDeviceEnrollmentPersistsRecord(): void + { + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + $admin = $this->user(self::ADMIN_EMAIL); + $code = $this->latestOtpCode(self::ADMIN_EMAIL); + + $response = $this->verify($code, true); + $this->assertResponseStatus(302); + + EntityManager::clear(); + $devices = EntityManager::getRepository(UserTrustedDevice::class)->findBy(['user' => $admin->getId()]); + $this->assertNotEmpty($devices, 'a trusted-device record must be persisted'); + $this->assertGreaterThan(0, $this->countAudit($admin->getId(), TwoFactorAuditLog::EventDeviceTrusted)); + } + + public function testTrustedDeviceCookieBypassesMFA(): void + { + $admin = $this->user(self::ADMIN_EMAIL); + + /** @var IDeviceTrustService $deviceTrust */ + $deviceTrust = App::make(IDeviceTrustService::class); + $rawToken = $deviceTrust->trustDevice($admin, 'Mozilla/5.0 (test)', '127.0.0.1'); + + // The device-trust cookie is excluded from encryption, so it is sent verbatim. + $response = $this->postLogin( + self::ADMIN_EMAIL, + self::SEED_PASSWORD, + [Config::get('two_factor.cookie_name') => $rawToken] + ); + + $this->assertResponseStatus(302); + $this->assertTrue(Auth::check(), 'a valid trusted-device cookie must bypass MFA'); + } + + // ------------------------------------------------------------------------- + // recovery codes + // ------------------------------------------------------------------------- + + public function testRecoveryCodeLoginSucceeds(): void + { + $admin = $this->user(self::ADMIN_EMAIL); + $plain = 'RECOVERY-PLAIN-123'; + $codeId = $this->createRecoveryCode($admin, $plain, false); + + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + $response = $this->recovery($plain); + + $this->assertResponseStatus(302); + $this->assertTrue(Auth::check()); + + EntityManager::clear(); + $code = EntityManager::find(UserRecoveryCode::class, $codeId); + $this->assertTrue($code->isUsed(), 'the recovery code must be marked used'); + $this->assertGreaterThan(0, $this->countAudit($admin->getId(), TwoFactorAuditLog::EventRecoveryUsed)); + } + + public function testUsedRecoveryCodeFails(): void + { + $admin = $this->user(self::ADMIN_EMAIL); + $plain = 'RECOVERY-USED-456'; + $this->createRecoveryCode($admin, $plain, true); // already used + + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + $response = $this->recovery($plain); + + $this->assertResponseStatus(401); + $payload = json_decode($response->getContent(), true); + $this->assertSame('mfa_invalid_recovery', $payload['error_code']); + $this->assertFalse(Auth::check()); + } + + // ------------------------------------------------------------------------- + // resend + // ------------------------------------------------------------------------- + + public function testResendEndpointReturnsChallengePayload(): void + { + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + + $response = $this->resend(); + + $this->assertResponseStatus(200); + $payload = json_decode($response->getContent(), true); + $this->assertArrayHasKey('otp_length', $payload); + $this->assertArrayHasKey('otp_lifetime', $payload); + } + + // ------------------------------------------------------------------------- + // rate limiting + // ------------------------------------------------------------------------- + + public function testVerifyRateLimitBlocksAfterThreshold(): 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->verify('bad-code-' . $i); + } + + $response = $this->verify('bad-code-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); + + $max = (int) Config::get('two_factor.rate_limit.max_otp_requests'); + for ($i = 0; $i < $max; $i++) { + $this->resend(); + } + + $response = $this->resend(); + $this->assertResponseStatus(429); + $payload = json_decode($response->getContent(), true); + $this->assertSame('mfa_rate_limit', $payload['error_code']); + } + + // ------------------------------------------------------------------------- + // Helpers + // ------------------------------------------------------------------------- + + private function postLogin(string $username, string $password, array $cookies = []) + { + return $this->action('POST', 'UserController@postLogin', [ + 'username' => $username, + 'password' => $password, + 'flow' => 'password', + '_token' => Session::token(), + ], [], $cookies); + } + + private function verify(string $otp, bool $trustDevice = false) + { + return $this->action('POST', 'UserController@verify2FA', [ + 'otp_value' => $otp, + 'method' => User::MFAMethod_OTP, + 'trust_device' => $trustDevice ? '1' : '0', + '_token' => Session::token(), + ]); + } + + private function recovery(string $code) + { + return $this->action('POST', 'UserController@verify2FARecovery', [ + 'recovery_code' => $code, + '_token' => Session::token(), + ]); + } + + private function resend() + { + return $this->action('POST', 'UserController@resend2FA', [ + 'method' => User::MFAMethod_OTP, + '_token' => Session::token(), + ]); + } + + private function user(string $email): User + { + $repo = EntityManager::getRepository(User::class); + $user = $repo->getByEmailOrName($email); + $this->assertInstanceOf(User::class, $user, "user {$email} not found"); + return $user; + } + + private function createPlainUser(): string + { + $email = 'plain.' . uniqid() . '@test.invalid'; + $user = UserFactory::build([ + 'first_name' => 'Plain', + 'last_name' => 'User', + 'email' => $email, + 'password' => self::SEED_PASSWORD, + 'password_enc' => AuthHelper::AlgSHA1_V2_4, + 'active' => true, + 'email_verified' => true, + 'identifier' => 'plain.' . uniqid(), + ]); + EntityManager::persist($user); + EntityManager::flush(); + return $email; + } + + 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(); + } + + private function latestOtpCode(string $email): string + { + EntityManager::clear(); + /** @var IOAuth2OTPRepository $repo */ + $repo = App::make(IOAuth2OTPRepository::class); + $otps = $repo->getByUserNameNotRedeemed($email); + $this->assertNotEmpty($otps, "no OTP issued for {$email}"); + return end($otps)->getValue(); + } + + private function countAudit(int $userId, string $eventType): int + { + EntityManager::clear(); + return (int) count( + EntityManager::getRepository(TwoFactorAuditLog::class) + ->findBy(['user' => $userId, 'event_type' => $eventType]) + ); + } +} diff --git a/tests/Unit/MFA/AbstractMFAChallengeStrategyTest.php b/tests/Unit/MFA/AbstractMFAChallengeStrategyTest.php index 743cf6f5..d79d13c6 100644 --- a/tests/Unit/MFA/AbstractMFAChallengeStrategyTest.php +++ b/tests/Unit/MFA/AbstractMFAChallengeStrategyTest.php @@ -90,6 +90,7 @@ public function testVerifyRecoveryCode_withMatchingCode_marksAsUsed(): void $repo = \Mockery::mock(IUserRecoveryCodeRepository::class); $repo->shouldReceive('getUnusedByUser')->with($user)->andReturn([$recoveryCode]); + $repo->shouldReceive('add')->with($recoveryCode, false)->once(); $strategy = new class($repo) extends AbstractMFAChallengeStrategy { public function issueChallenge(User $user, ?Client $client, bool $remember): array { return []; } diff --git a/tests/Unit/MFA/EmailOTPMFAChallengeStrategyTest.php b/tests/Unit/MFA/EmailOTPMFAChallengeStrategyTest.php index f2c56a7b..3d9b7809 100644 --- a/tests/Unit/MFA/EmailOTPMFAChallengeStrategyTest.php +++ b/tests/Unit/MFA/EmailOTPMFAChallengeStrategyTest.php @@ -99,6 +99,19 @@ public function testVerifyChallenge_withValidOtp_redeemsAndRevokesOthers(): void $user = $this->buildUser(1, 'verify@example.com'); $code = '123456'; + $storedOtp = \Mockery::mock(OAuth2OTP::class); + $storedOtp->shouldReceive('getValue')->andReturn($code); + $storedOtp->shouldReceive('logRedeemAttempt')->once(); + $storedOtp->shouldReceive('isAlive')->andReturn(true); + $storedOtp->shouldReceive('isValid')->andReturn(true); + $storedOtp->shouldReceive('redeem')->once(); + + $this->otpRepository + ->shouldReceive('getByValueConnectionAndUserName') + ->once() + ->with($code, 'email', 'verify@example.com', null) + ->andReturn($storedOtp); + $otherOtp = \Mockery::mock(OAuth2OTP::class); $otherOtp->shouldReceive('getValue')->andReturn('654321'); $otherOtp->shouldReceive('redeem')->once(); @@ -107,7 +120,24 @@ public function testVerifyChallenge_withValidOtp_redeemsAndRevokesOthers(): void ->shouldReceive('getByUserNameNotRedeemed') ->andReturn([$otherOtp]); + // The redeemed code and the revoked sibling are both persisted with deferred flush. + $this->otpRepository->shouldReceive('add')->with($storedOtp, false)->once(); + $this->otpRepository->shouldReceive('add')->with($otherOtp, false)->once(); + $this->strategy->verifyChallenge($user, $code); $this->addToAssertionCount(1); } + + public function testVerifyChallenge_withNonMatchingCode_throws(): void + { + $user = $this->buildUser(1, 'verify@example.com'); + + $this->otpRepository + ->shouldReceive('getByValueConnectionAndUserName') + ->once() + ->andReturn(null); + + $this->expectException(\Auth\Exceptions\AuthenticationException::class); + $this->strategy->verifyChallenge($user, 'wrong-code'); + } } diff --git a/tests/Unit/TwoFactorAuditServiceTest.php b/tests/Unit/TwoFactorAuditServiceTest.php index e16c5679..23a5cf3f 100644 --- a/tests/Unit/TwoFactorAuditServiceTest.php +++ b/tests/Unit/TwoFactorAuditServiceTest.php @@ -22,6 +22,7 @@ use Illuminate\Support\Facades\Queue; use Mockery; use Tests\TestCase; +use Utils\Db\ITransactionService; /** * Class TwoFactorAuditServiceTest @@ -35,6 +36,9 @@ final class TwoFactorAuditServiceTest extends TestCase /** @var \Mockery\MockInterface&ITwoFactorAuditLogRepository */ private $repository; + /** @var \Mockery\MockInterface&ITransactionService */ + private $tx_service; + /** @var \Mockery\MockInterface&User */ private $user; @@ -44,7 +48,8 @@ protected function setUp(): void Queue::fake(); $this->repository = Mockery::mock(ITwoFactorAuditLogRepository::class); - $this->service = new TwoFactorAuditService($this->repository); + $this->tx_service = Mockery::mock(ITransactionService::class); + $this->service = new TwoFactorAuditService($this->repository, $this->tx_service); $this->user = Mockery::mock(User::class); $this->user->shouldReceive('getId')->andReturn(42); @@ -70,7 +75,14 @@ public function testLogPersistsTwoFactorAuditLogWithCorrectFields(): void ->once() ->withArgs(function (TwoFactorAuditLog $log, bool $sync) use (&$persisted) { $persisted = $log; - return $sync === true; + return $sync === false; + }); + + $this->tx_service + ->shouldReceive('transaction') + ->once() + ->andReturnUsing(function (callable $callback) { + return $callback(); }); $this->service->log( @@ -99,6 +111,13 @@ public function testLogEmitsOtlpAttributes(): void $this->repository->shouldReceive('add')->once(); + $this->tx_service + ->shouldReceive('transaction') + ->once() + ->andReturnUsing(function (callable $callback) { + return $callback(); + }); + $this->service->log( $this->user, TwoFactorAuditLog::EventChallengeSucceeded, @@ -127,6 +146,13 @@ public function testLogEmitsSuccessFalseForChallengeFailed(): void $this->repository->shouldReceive('add')->once(); + $this->tx_service + ->shouldReceive('transaction') + ->once() + ->andReturnUsing(function (callable $callback) { + return $callback(); + }); + $this->service->log( $this->user, TwoFactorAuditLog::EventChallengeFailed, @@ -150,6 +176,13 @@ public function testLogDoesNotDispatchJobWhenOtlpDisabled(): void $this->repository->shouldReceive('add')->once(); + $this->tx_service + ->shouldReceive('transaction') + ->once() + ->andReturnUsing(function (callable $callback) { + return $callback(); + }); + $this->service->log( $this->user, TwoFactorAuditLog::EventChallengeSucceeded, @@ -177,6 +210,13 @@ public function testLogAcceptsNullMetadata(): void return true; }); + $this->tx_service + ->shouldReceive('transaction') + ->once() + ->andReturnUsing(function (callable $callback) { + return $callback(); + }); + $this->service->log( $this->user, TwoFactorAuditLog::EventChallengeIssued, diff --git a/tests/unit/AuthServiceValidateCredentialsTest.php b/tests/unit/AuthServiceValidateCredentialsTest.php index a504adc4..05d0bc65 100644 --- a/tests/unit/AuthServiceValidateCredentialsTest.php +++ b/tests/unit/AuthServiceValidateCredentialsTest.php @@ -182,10 +182,10 @@ public function testLoginUser_throwsException_whenIsNotActive(): void } /** - * UnverifiedEmailMemberException from the provider must be caught and - * re-thrown as AuthenticationException (contract: @throws AuthenticationException only). + * UnverifiedEmailMemberException from the provider propagates to the caller + * so UserController::postLogin() can handle it with a specific UI message. */ - public function testUnverifiedUser_throwsAuthenticationException(): void + public function testUnverifiedUser_throwsUnverifiedEmailMemberException(): void { $username = 'unverified@example.com'; $password = 'any'; @@ -199,7 +199,7 @@ public function testUnverifiedUser_throwsAuthenticationException(): void $this->auth_mock->shouldReceive('getProvider')->once()->andReturn($provider_mock); $this->auth_mock->shouldNotReceive('login'); - $this->expectException(AuthenticationException::class); + $this->expectException(UnverifiedEmailMemberException::class); $this->expectExceptionMessage('Email not verified.'); $this->service->validateCredentials($username, $password); From 10eb2e08135ef83da802a180000a74566c6904bc Mon Sep 17 00:00:00 2001 From: matiasperrone-exo Date: Fri, 5 Jun 2026 22:11:08 +0000 Subject: [PATCH 02/14] chore: Add PR's requested changed --- .../Controllers/Traits/MFACookieManager.php | 2 + app/Http/Controllers/UserController.php | 61 ++++++++++++++--- .../DoctrineUserRecoveryCodeRepository.php | 6 ++ .../MFA/AbstractMFAChallengeStrategy.php | 18 ++++- .../MFA/EmailOTPMFAChallengeStrategy.php | 21 +++++- app/libs/Auth/AuthService.php | 18 ++++- .../IUserRecoveryCodeRepository.php | 7 ++ app/libs/Utils/Services/IAuthService.php | 3 +- storage/framework/cache/data/.gitignore | 2 - tests/TwoFactorLoginFlowTest.php | 68 +++++++++++++++++++ .../MFA/AbstractMFAChallengeStrategyTest.php | 31 +++++++++ .../MFA/EmailOTPMFAChallengeStrategyTest.php | 50 ++++++++++++-- .../AuthServiceValidateCredentialsTest.php | 56 +++++++++++++-- 13 files changed, 315 insertions(+), 28 deletions(-) delete mode 100755 storage/framework/cache/data/.gitignore diff --git a/app/Http/Controllers/Traits/MFACookieManager.php b/app/Http/Controllers/Traits/MFACookieManager.php index 718b9305..086553d5 100644 --- a/app/Http/Controllers/Traits/MFACookieManager.php +++ b/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; /** diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index 9644ca91..6ec54f3a 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -469,7 +469,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); @@ -501,7 +501,7 @@ public function postLogin() return $this->login_strategy->postLogin(); } - if ($flow == "otp") { + if ($flow == IAuthService::AuthenticationFlowPasswordless) { $client = $this->resolveClientFromMemento(); @@ -631,6 +631,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. * @@ -667,8 +685,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. @@ -687,17 +716,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) { diff --git a/app/Repositories/DoctrineUserRecoveryCodeRepository.php b/app/Repositories/DoctrineUserRecoveryCodeRepository.php index b492a0f6..202e48b6 100644 --- a/app/Repositories/DoctrineUserRecoveryCodeRepository.php +++ b/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(); diff --git a/app/Strategies/MFA/AbstractMFAChallengeStrategy.php b/app/Strategies/MFA/AbstractMFAChallengeStrategy.php index ce940857..3bf3055c 100644 --- a/app/Strategies/MFA/AbstractMFAChallengeStrategy.php +++ b/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,6 +45,7 @@ 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); } @@ -50,6 +53,14 @@ 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 diff --git a/app/Strategies/MFA/EmailOTPMFAChallengeStrategy.php b/app/Strategies/MFA/EmailOTPMFAChallengeStrategy.php index 9c25f2e0..a07f366b 100644 --- a/app/Strategies/MFA/EmailOTPMFAChallengeStrategy.php +++ b/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); diff --git a/app/libs/Auth/AuthService.php b/app/libs/Auth/AuthService.php index 7a51d92c..ed1f6510 100644 --- a/app/libs/Auth/AuthService.php +++ b/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); }); } diff --git a/app/libs/Auth/Repositories/IUserRecoveryCodeRepository.php b/app/libs/Auth/Repositories/IUserRecoveryCodeRepository.php index f9733f87..0bfd0335 100644 --- a/app/libs/Auth/Repositories/IUserRecoveryCodeRepository.php +++ b/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). */ diff --git a/app/libs/Utils/Services/IAuthService.php b/app/libs/Utils/Services/IAuthService.php index 1990c2ed..b1b6abfc 100644 --- a/app/libs/Utils/Services/IAuthService.php +++ b/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( diff --git a/storage/framework/cache/data/.gitignore b/storage/framework/cache/data/.gitignore deleted file mode 100755 index d6b7ef32..00000000 --- a/storage/framework/cache/data/.gitignore +++ /dev/null @@ -1,2 +0,0 @@ -* -!.gitignore diff --git a/tests/TwoFactorLoginFlowTest.php b/tests/TwoFactorLoginFlowTest.php index e8d8521e..0fc20bd6 100644 --- a/tests/TwoFactorLoginFlowTest.php +++ b/tests/TwoFactorLoginFlowTest.php @@ -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); diff --git a/tests/Unit/MFA/AbstractMFAChallengeStrategyTest.php b/tests/Unit/MFA/AbstractMFAChallengeStrategyTest.php index d79d13c6..cdf2fcfb 100644 --- a/tests/Unit/MFA/AbstractMFAChallengeStrategyTest.php +++ b/tests/Unit/MFA/AbstractMFAChallengeStrategyTest.php @@ -86,10 +86,13 @@ public function testVerifyRecoveryCode_withMatchingCode_marksAsUsed(): void $recoveryCode = \Mockery::mock(\App\libs\Auth\Models\UserRecoveryCode::class); $recoveryCode->shouldReceive('getCodeHash')->andReturn(Hash::make($code)); + $recoveryCode->shouldReceive('isUsed')->andReturn(false); $recoveryCode->shouldReceive('markUsed')->once(); $repo = \Mockery::mock(IUserRecoveryCodeRepository::class); $repo->shouldReceive('getUnusedByUser')->with($user)->andReturn([$recoveryCode]); + // Lock is taken before mutating (regression guard for 3357348455). + $repo->shouldReceive('refreshExclusiveLock')->with($recoveryCode)->once(); $repo->shouldReceive('add')->with($recoveryCode, false)->once(); $strategy = new class($repo) extends AbstractMFAChallengeStrategy { @@ -102,6 +105,34 @@ public function resendChallenge(User $user, ?Client $client, bool $remember): ar $this->addToAssertionCount(1); // markUsed()->once() verified by Mockery in tearDown } + public function testVerifyRecoveryCode_locksAndRejects_whenUsedAfterLock(): void + { + $user = new User(); + $code = 'VALID-CODE'; + + // Hash matches, but a concurrent winner marked it used; the lock+recheck + // must reject without double-spending (regression guard for 3357348455). + $recoveryCode = \Mockery::mock(\App\libs\Auth\Models\UserRecoveryCode::class); + $recoveryCode->shouldReceive('getCodeHash')->andReturn(Hash::make($code)); + $recoveryCode->shouldReceive('isUsed')->andReturn(true); + $recoveryCode->shouldNotReceive('markUsed'); + + $repo = \Mockery::mock(IUserRecoveryCodeRepository::class); + $repo->shouldReceive('getUnusedByUser')->with($user)->andReturn([$recoveryCode]); + $repo->shouldReceive('refreshExclusiveLock')->with($recoveryCode)->once(); + $repo->shouldNotReceive('add'); + + $strategy = new class($repo) extends AbstractMFAChallengeStrategy { + public function issueChallenge(User $user, ?Client $client, bool $remember): array { return []; } + public function verifyChallenge(User $user, string $code, ?Client $client = null): void {} + public function resendChallenge(User $user, ?Client $client, bool $remember): array { return []; } + }; + + $this->expectException(AuthenticationException::class); + $this->expectExceptionMessage("Invalid recovery code."); + $strategy->verifyRecoveryCode($user, $code); + } + public function testVerifyRecoveryCode_withNonMatchingCode_throwsException(): void { $user = new User(); diff --git a/tests/Unit/MFA/EmailOTPMFAChallengeStrategyTest.php b/tests/Unit/MFA/EmailOTPMFAChallengeStrategyTest.php index 3d9b7809..7db3b1ba 100644 --- a/tests/Unit/MFA/EmailOTPMFAChallengeStrategyTest.php +++ b/tests/Unit/MFA/EmailOTPMFAChallengeStrategyTest.php @@ -4,6 +4,7 @@ use Auth\Repositories\IUserRecoveryCodeRepository; use Auth\User; use Illuminate\Support\Facades\Session; +use Models\OAuth2\Client; use Models\OAuth2\OAuth2OTP; use OAuth2\Services\ITokenService; use Strategies\MFA\EmailOTPMFAChallengeStrategy; @@ -94,40 +95,79 @@ public function testResendChallenge_delegatesToIssueChallenge(): void // ---------- verifyChallenge ---------- - public function testVerifyChallenge_withValidOtp_redeemsAndRevokesOthers(): void + public function testVerifyChallenge_withValidOtp_redeemsAndRevokesOthers_scopedToClient(): void { - $user = $this->buildUser(1, 'verify@example.com'); - $code = '123456'; + $user = $this->buildUser(1, 'verify@example.com'); + $code = '123456'; + $client = \Mockery::mock(Client::class); $storedOtp = \Mockery::mock(OAuth2OTP::class); $storedOtp->shouldReceive('getValue')->andReturn($code); $storedOtp->shouldReceive('logRedeemAttempt')->once(); $storedOtp->shouldReceive('isAlive')->andReturn(true); $storedOtp->shouldReceive('isValid')->andReturn(true); + $storedOtp->shouldReceive('getConnection')->andReturn('email'); + $storedOtp->shouldReceive('isRedeemed')->andReturn(false); $storedOtp->shouldReceive('redeem')->once(); + // Lookup MUST be scoped to the issuing client (regression guard for r3357348448). $this->otpRepository ->shouldReceive('getByValueConnectionAndUserName') ->once() - ->with($code, 'email', 'verify@example.com', null) + ->with($code, 'email', 'verify@example.com', $client) ->andReturn($storedOtp); + // A pessimistic row lock is taken before redeeming (regression guard for 3357348444). + $this->otpRepository->shouldReceive('refreshExclusiveLock')->with($storedOtp)->once(); + $otherOtp = \Mockery::mock(OAuth2OTP::class); $otherOtp->shouldReceive('getValue')->andReturn('654321'); $otherOtp->shouldReceive('redeem')->once(); + // Sibling revoke MUST be scoped to the SAME client so unrelated OTPs survive. $this->otpRepository ->shouldReceive('getByUserNameNotRedeemed') + ->once() + ->with('verify@example.com', $client) ->andReturn([$otherOtp]); // The redeemed code and the revoked sibling are both persisted with deferred flush. $this->otpRepository->shouldReceive('add')->with($storedOtp, false)->once(); $this->otpRepository->shouldReceive('add')->with($otherOtp, false)->once(); - $this->strategy->verifyChallenge($user, $code); + $this->strategy->verifyChallenge($user, $code, $client); $this->addToAssertionCount(1); } + public function testVerifyChallenge_acquiresLock_andRejectsAlreadyRedeemed(): void + { + $user = $this->buildUser(1, 'verify@example.com'); + $code = '123456'; + + $storedOtp = \Mockery::mock(OAuth2OTP::class); + $storedOtp->shouldReceive('getValue')->andReturn($code); + $storedOtp->shouldReceive('logRedeemAttempt')->once(); + $storedOtp->shouldReceive('isAlive')->andReturn(true); + $storedOtp->shouldReceive('isValid')->andReturn(true); + $storedOtp->shouldReceive('getConnection')->andReturn('email'); + // Concurrent winner already redeemed the row under the lock. + $storedOtp->shouldReceive('isRedeemed')->andReturn(true); + // Must NOT redeem again nor sweep siblings. + $storedOtp->shouldReceive('redeem')->never(); + + $this->otpRepository + ->shouldReceive('getByValueConnectionAndUserName') + ->once() + ->andReturn($storedOtp); + + $this->otpRepository->shouldReceive('refreshExclusiveLock')->with($storedOtp)->once(); + $this->otpRepository->shouldReceive('getByUserNameNotRedeemed')->never(); + $this->otpRepository->shouldReceive('add')->never(); + + $this->expectException(\Auth\Exceptions\AuthenticationException::class); + $this->strategy->verifyChallenge($user, $code); + } + public function testVerifyChallenge_withNonMatchingCode_throws(): void { $user = $this->buildUser(1, 'verify@example.com'); diff --git a/tests/unit/AuthServiceValidateCredentialsTest.php b/tests/unit/AuthServiceValidateCredentialsTest.php index 05d0bc65..678cc4c7 100644 --- a/tests/unit/AuthServiceValidateCredentialsTest.php +++ b/tests/unit/AuthServiceValidateCredentialsTest.php @@ -32,8 +32,9 @@ /** * Class AuthServiceValidateCredentialsTest * Verifies that AuthService::validateCredentials() validates the password - * WITHOUT establishing a session, and that AuthService::loginUser() calls - * Auth::login() for the 2FA completion step. + * WITHOUT establishing a session, and that AuthService::loginUser() registers + * the OIDC principal (clear + register with auth time) and calls Auth::login() + * for the 2FA completion step. */ #[\PHPUnit\Framework\Attributes\RunTestsInSeparateProcesses] #[\PHPUnit\Framework\Attributes\PreserveGlobalState(false)] @@ -44,6 +45,7 @@ final class AuthServiceValidateCredentialsTest extends PHPUnitTestCase private AuthService $service; private $mock_user_repository; + private $mock_principal_service; // Facade aliases private $auth_mock; @@ -55,7 +57,7 @@ protected function setUp(): void $this->mock_user_repository = $this->createMock(IUserRepository::class); $mock_otp_repository = $this->createMock(IOAuth2OTPRepository::class); - $mock_principal_service = $this->createMock(IPrincipalService::class); + $this->mock_principal_service = $this->createMock(IPrincipalService::class); $mock_user_service = $this->createMock(IUserService::class); $mock_user_action_service = $this->createMock(IUserActionService::class); $mock_cache_service = $this->createMock(ICacheService::class); @@ -72,7 +74,7 @@ protected function setUp(): void $this->service = new AuthService( $this->mock_user_repository, $mock_otp_repository, - $mock_principal_service, + $this->mock_principal_service, $mock_user_service, $mock_user_action_service, $mock_cache_service, @@ -140,6 +142,7 @@ public function testLoginUser_callsAuthLogin_withRememberTrue(): void { $user = Mockery::mock('Auth\User'); $user->shouldReceive('canLogin')->andReturn(true); + $user->shouldReceive('getId')->andReturn(123); $this->auth_mock ->shouldReceive('login') @@ -156,6 +159,7 @@ public function testLoginUser_callsAuthLogin_withRememberFalse(): void { $user = Mockery::mock('Auth\User'); $user->shouldReceive('canLogin')->andReturn(true); + $user->shouldReceive('getId')->andReturn(123); $this->auth_mock ->shouldReceive('login') @@ -165,6 +169,50 @@ public function testLoginUser_callsAuthLogin_withRememberFalse(): void $this->service->loginUser($user, false); } + /** + * Regression: loginUser() MUST register the OIDC principal before establishing + * the session, mirroring AuthService::login(). Without this, the principal has + * no auth_time, so the id_token auth_time claim is wrong and + * InteractiveGrantType::shouldForceReLogin() silently skips max_age / prompt=login + * enforcement for every password / 2FA / recovery login. + */ + public function testLoginUser_registersPrincipal_withAuthTime(): void + { + $user = Mockery::mock('Auth\User'); + $user->shouldReceive('canLogin')->andReturn(true); + $user->shouldReceive('getId')->andReturn(123); + + $this->mock_principal_service->expects($this->once())->method('clear'); + $this->mock_principal_service->expects($this->once()) + ->method('register') + ->with(123, $this->greaterThan(0)); + + $this->auth_mock + ->shouldReceive('login') + ->once() + ->with($user, true); + + $this->service->loginUser($user, true); + } + + /** + * Regression: when the user cannot log in, loginUser() must throw BEFORE + * touching the principal — no clear()/register() and no Auth::login(). + */ + public function testLoginUser_doesNotRegisterPrincipal_whenCannotLogin(): void + { + $user = Mockery::mock('Auth\User'); + $user->shouldReceive('canLogin')->andReturn(false); + + $this->mock_principal_service->expects($this->never())->method('clear'); + $this->mock_principal_service->expects($this->never())->method('register'); + $this->auth_mock->shouldNotReceive('login'); + + $this->expectException(AuthenticationException::class); + + $this->service->loginUser($user, true); + } + /** * loginUser(user, [true|false]) and isActive or canLogin false throws an Exception. */ From 5fc47a4a942061b13a60bcef7ccd7e9a7f5b0deb Mon Sep 17 00:00:00 2001 From: matiasperrone-exo Date: Mon, 8 Jun 2026 18:19:59 +0000 Subject: [PATCH 03/14] chore: Add PR's requested changes --- app/Http/Controllers/UserController.php | 20 +------------------ .../MFA/AbstractMFAChallengeStrategy.php | 11 +--------- .../MFA/EmailOTPMFAChallengeStrategy.php | 6 +----- app/libs/Auth/AuthService.php | 6 ------ .../MFA/AbstractMFAChallengeStrategyTest.php | 1 - .../MFA/EmailOTPMFAChallengeStrategyTest.php | 2 -- 6 files changed, 3 insertions(+), 43 deletions(-) diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index 6ec54f3a..b37ccdb3 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -631,24 +631,6 @@ 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. * @@ -686,7 +668,7 @@ public function verify2FA() } // Scope verification to the client the challenge was issued for. - $client = $this->resolveClientFromPendingState($pending); + $client = $this->resolveClientFromMemento(); try { // Commits the OTP redeem (+ sibling revoke) in its own tx. The diff --git a/app/Strategies/MFA/AbstractMFAChallengeStrategy.php b/app/Strategies/MFA/AbstractMFAChallengeStrategy.php index 3bf3055c..c2d04fea 100644 --- a/app/Strategies/MFA/AbstractMFAChallengeStrategy.php +++ b/app/Strategies/MFA/AbstractMFAChallengeStrategy.php @@ -13,7 +13,6 @@ 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) {} @@ -36,7 +35,6 @@ 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), ]; } @@ -45,7 +43,6 @@ 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); } @@ -62,23 +59,17 @@ public function verifyRecoveryCode(User $user, string $code): void throw new AuthenticationException("Invalid recovery code."); } $recoveryCode->markUsed(); - $this->recovery_code_repository->add($recoveryCode, false); return; } } throw new AuthenticationException("Invalid recovery code."); } - protected function storePendingState(int $userId, bool $remember, ?string $clientId = null): void + protected function storePendingState(int $userId, bool $remember): 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 diff --git a/app/Strategies/MFA/EmailOTPMFAChallengeStrategy.php b/app/Strategies/MFA/EmailOTPMFAChallengeStrategy.php index a07f366b..5a32052f 100644 --- a/app/Strategies/MFA/EmailOTPMFAChallengeStrategy.php +++ b/app/Strategies/MFA/EmailOTPMFAChallengeStrategy.php @@ -20,9 +20,7 @@ public function __construct( public function issueChallenge(User $user, ?Client $client, bool $remember): array { - // 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()); + $this->storePendingState($user->getId(), $remember); $otp = $this->token_service->createOTPFromPayload([ OAuth2Protocol::OAuth2PasswordlessConnection => OAuth2Protocol::OAuth2PasswordlessConnectionEmail, @@ -75,14 +73,12 @@ public function verifyChallenge(User $user, string $code, ?Client $client = null } $otp->redeem(); - $this->otp_repository->add($otp, false); // 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); } } } diff --git a/app/libs/Auth/AuthService.php b/app/libs/Auth/AuthService.php index ed1f6510..6d52be81 100644 --- a/app/libs/Auth/AuthService.php +++ b/app/libs/Auth/AuthService.php @@ -19,7 +19,6 @@ use App\Services\Auth\IUserService as IAuthUserService; use Auth\Exceptions\AuthenticationException; use Auth\Exceptions\AuthenticationLockedUserLoginAttempt; -use Auth\Exceptions\UnverifiedEmailMemberException; use Auth\Repositories\IUserRepository; use Exception; use Illuminate\Support\Facades\Auth; @@ -393,7 +392,6 @@ public function login(string $username, string $password, bool $remember_me): bo { Log::debug("AuthService::login"); - $this->last_login_error = ""; if (!Auth::attempt(['username' => $username, 'password' => $password], $remember_me)) { throw new AuthenticationException ( @@ -799,10 +797,6 @@ public function verifyMFAChallenge( string $value, ?Client $client = null ): void { - // 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); }); diff --git a/tests/Unit/MFA/AbstractMFAChallengeStrategyTest.php b/tests/Unit/MFA/AbstractMFAChallengeStrategyTest.php index cdf2fcfb..1ac5d20b 100644 --- a/tests/Unit/MFA/AbstractMFAChallengeStrategyTest.php +++ b/tests/Unit/MFA/AbstractMFAChallengeStrategyTest.php @@ -93,7 +93,6 @@ public function testVerifyRecoveryCode_withMatchingCode_marksAsUsed(): void $repo->shouldReceive('getUnusedByUser')->with($user)->andReturn([$recoveryCode]); // Lock is taken before mutating (regression guard for 3357348455). $repo->shouldReceive('refreshExclusiveLock')->with($recoveryCode)->once(); - $repo->shouldReceive('add')->with($recoveryCode, false)->once(); $strategy = new class($repo) extends AbstractMFAChallengeStrategy { public function issueChallenge(User $user, ?Client $client, bool $remember): array { return []; } diff --git a/tests/Unit/MFA/EmailOTPMFAChallengeStrategyTest.php b/tests/Unit/MFA/EmailOTPMFAChallengeStrategyTest.php index 7db3b1ba..cdf374c0 100644 --- a/tests/Unit/MFA/EmailOTPMFAChallengeStrategyTest.php +++ b/tests/Unit/MFA/EmailOTPMFAChallengeStrategyTest.php @@ -132,8 +132,6 @@ public function testVerifyChallenge_withValidOtp_redeemsAndRevokesOthers_scopedT ->andReturn([$otherOtp]); // The redeemed code and the revoked sibling are both persisted with deferred flush. - $this->otpRepository->shouldReceive('add')->with($storedOtp, false)->once(); - $this->otpRepository->shouldReceive('add')->with($otherOtp, false)->once(); $this->strategy->verifyChallenge($user, $code, $client); $this->addToAssertionCount(1); From d2bffb8d8dea43aa86cdcb3e44cfffb1a6c1bbfe Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 22 Jul 2026 16:57:20 -0300 Subject: [PATCH 04/14] Add TWO_FACTOR_ENABLED global kill-switch to MFA gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MFAGateService::requiresChallenge() had no master on/off switch, contradicting the SDS idp-mfa.md §10.1 rollout plan, which requires being able to instantly revert to password-only login without a code rollback if something goes wrong post-deploy. config/two_factor.php gains an 'enabled' key (env TWO_FACTOR_ENABLED, default true) checked first in requiresChallenge(), short-circuiting before any per-user or device-trust evaluation. --- .gitignore | 4 +++- app/Services/Auth/MFAGateService.php | 3 +++ config/two_factor.php | 12 ++++++++++++ tests/Unit/MFAGateServiceTest.php | 17 +++++++++++++++++ 4 files changed, 35 insertions(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index 2b975b7c..75aa0c64 100644 --- a/.gitignore +++ b/.gitignore @@ -52,4 +52,6 @@ model.sql /.phpunit.cache/ docker-compose/mysql/model/*.sql public/assets/*.map -public/assets/css/*.map \ No newline at end of file +public/assets/css/*.map +.codegraph +docs/plans \ No newline at end of file diff --git a/app/Services/Auth/MFAGateService.php b/app/Services/Auth/MFAGateService.php index 8c82bee3..b2acb62e 100644 --- a/app/Services/Auth/MFAGateService.php +++ b/app/Services/Auth/MFAGateService.php @@ -30,6 +30,9 @@ public function __construct( public function requiresChallenge(User $user, ?string $cookieToken): bool { + if (!config('two_factor.enabled', true)) { + return false; + } if (!$user->shouldRequire2FA()) { return false; } diff --git a/config/two_factor.php b/config/two_factor.php index 78ac21a0..64248f84 100644 --- a/config/two_factor.php +++ b/config/two_factor.php @@ -15,6 +15,18 @@ use App\libs\Auth\Models\IGroupSlugs; return [ + /* + |-------------------------------------------------------------------------- + | Global Kill-Switch + |-------------------------------------------------------------------------- + | + | Master switch for the whole 2FA gate (SDS idp-mfa.md §10.1 rollout plan). + | Defaults on; set TWO_FACTOR_ENABLED=false in a specific environment to + | instantly revert to password-only login with no code rollback needed. + | + */ + 'enabled' => env('TWO_FACTOR_ENABLED', true), + /* |-------------------------------------------------------------------------- | Enforced Groups diff --git a/tests/Unit/MFAGateServiceTest.php b/tests/Unit/MFAGateServiceTest.php index 49c66822..a2bfb5df 100644 --- a/tests/Unit/MFAGateServiceTest.php +++ b/tests/Unit/MFAGateServiceTest.php @@ -18,6 +18,7 @@ use App\Services\Auth\IDeviceTrustService; use App\Services\Auth\MFAGateService; use Auth\User; +use Illuminate\Support\Facades\Config; use Mockery; use Tests\TestCase; use Mockery\MockInterface; @@ -110,4 +111,20 @@ public function testRequiresChallengePassesThroughEmptyStringCookieToDeviceTrust $this->assertTrue($this->service->requiresChallenge($user, '')); } + + // ------------------------------------------------------------------------- + // Global kill-switch (SDS idp-mfa.md §10.1): TWO_FACTOR_ENABLED=false must + // short-circuit the gate before any per-user or device-trust evaluation. + // ------------------------------------------------------------------------- + + public function testRequiresChallengeReturnsFalseWhenTwoFactorGloballyDisabled(): void + { + Config::set('two_factor.enabled', false); + + $user = Mockery::mock(User::class); + $user->shouldNotReceive('shouldRequire2FA'); + $this->deviceTrustService->shouldNotReceive('isDeviceTrusted'); + + $this->assertFalse($this->service->requiresChallenge($user, null)); + } } From 2b00f80b3b4e861da472586bd55cc4e94f86183d Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 22 Jul 2026 18:31:58 -0300 Subject: [PATCH 05/14] Route MFA challenge responses through login_strategy, not hardcoded JSON postLogin()'s mfa_required response, and the display-strategy contract it depends on, bypassed $this->login_strategy entirely: every MFA response was Response::json(...) built by hand in the controller, ignoring OAuth2 display-strategy polymorphism (native vs page/popup/touch). Native OAuth2 clients (display=native) got JSON+200 with an ad hoc shape instead of the 412 + required_params/url/method contract every other login error already returns for that display mode. - ILoginStrategy::challengeRequired() / IDisplayResponseStrategy:: getChallengeRequiredResponse(): new methods, distinct from errorLogin() since a pending MFA challenge isn't a failed attempt. - DefaultLoginStrategy: identical bytes to before (200 + JSON) - zero behavior change for the plain IdP flow. - OAuth2LoginStrategy: rebuilds the auth_request from the memento (same pattern as errorLogin()) and delegates to DisplayResponseStrategyFactory. - DisplayResponseJsonStrategy (native): 412, matching its sibling getConsentResponse/getLoginResponse/getLoginErrorResponse methods. - DisplayResponseUserAgentStrategy (page/popup/touch): 200 JSON, same live in-SPA transition as the plain flow, since both render the same login.js. - ILoginStrategy::MFA_REQUIRED constant replaces the 'mfa_required' literal duplicated across three classes. Also closes a refresh-resilience gap PR #142's frontend already expected but the backend never delivered (its login.js constructor comment reads "Two-factor state (populated from the flash redirect...)"): postLogin() now flashes flow/mfa_method/otp_length/otp_lifetime to session on mfa_required so a page refresh mid-challenge restores the 2FA screen instead of dropping back to the password form. Cleared on successful verification/recovery and on session expiry; refreshed on resend2FA() (including method switches). New: OAuth2NativeMFALoginFlowTest exercises the real /oauth2/auth -> memento -> postLogin() path for display=native and asserts 412+mfa_required. TwoFactorLoginFlowTest gains coverage for the session-flash/clear behavior. --- app/Http/Controllers/UserController.php | 48 ++++++++++++-- app/Strategies/DefaultLoginStrategy.php | 14 +++++ .../DisplayResponseJsonStrategy.php | 9 +++ .../DisplayResponseUserAgentStrategy.php | 13 ++++ app/Strategies/IDisplayResponseStrategy.php | 9 +++ app/Strategies/ILoginStrategy.php | 18 +++++- app/Strategies/OAuth2LoginStrategy.php | 17 +++++ tests/OAuth2NativeMFALoginFlowTest.php | 63 +++++++++++++++++++ tests/TwoFactorLoginFlowTest.php | 23 +++++++ 9 files changed, 209 insertions(+), 5 deletions(-) create mode 100644 tests/OAuth2NativeMFALoginFlowTest.php diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index b37ccdb3..b4813545 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -490,10 +490,20 @@ public function postLogin() IPHelper::getUserIp() ); - return Response::json( - array_merge(['error_code' => 'mfa_required'], $payload), - HttpResponse::HTTP_OK - ); + // Restore-on-refresh: a subsequent GET /login can rehydrate + // the 2FA screen from session instead of dropping back to the + // password form (mirrors what a redirect-based flow gets for + // free via session-flashed params). + Session::put('flow', '2fa'); + Session::put('mfa_method', $method); + if (isset($payload['otp_length'])) { + Session::put('otp_length', $payload['otp_length']); + } + if (isset($payload['otp_lifetime'])) { + Session::put('otp_lifetime', $payload['otp_lifetime']); + } + + return $this->login_strategy->challengeRequired($payload); } // No challenge required: establish the session and continue. @@ -710,6 +720,7 @@ public function verify2FA() } $strategy->clearPendingState(); + $this->clearMFAUISessionState(); try { $this->two_factor_audit_service->log( @@ -784,6 +795,7 @@ public function verify2FARecovery() $this->auth_service->loginUser($user, (bool) $pending['remember']); $strategy->clearPendingState(); + $this->clearMFAUISessionState(); $this->two_factor_audit_service->log( $user, @@ -835,6 +847,17 @@ public function resend2FA() $payload = $this->auth_service->resendMFAChallenge($user, $strategy, $this->resolveClientFromMemento(), (bool) $pending['remember']); + // Keep the refresh-restorable session state in sync with the + // fresh challenge (e.g. otp_lifetime countdown resets on resend, + // mfa_method changes if this resend is actually a method switch). + Session::put('mfa_method', $method); + if (isset($payload['otp_length'])) { + Session::put('otp_length', $payload['otp_length']); + } + if (isset($payload['otp_lifetime'])) { + Session::put('otp_lifetime', $payload['otp_lifetime']); + } + $this->two_factor_audit_service->log( $user, TwoFactorAuditLog::EventChallengeIssued, @@ -857,9 +880,26 @@ public function resend2FA() */ private function mfaSessionExpired() { + $this->clearMFAUISessionState(); return Response::json(['error_code' => 'mfa_session_expired'], HttpResponse::HTTP_UNAUTHORIZED); } + /** + * Clears the UI-restoration session keys written when a challenge is + * issued (see postLogin()'s mfa_required branch). Companion to + * IMFAChallengeStrategy::clearPendingState(), which only owns the + * 2fa_* pending-state keys. + * + * @return void + */ + private function clearMFAUISessionState(): void + { + Session::forget('flow'); + Session::forget('mfa_method'); + Session::forget('otp_length'); + Session::forget('otp_lifetime'); + } + /** * @return \Illuminate\Http\Response|mixed */ diff --git a/app/Strategies/DefaultLoginStrategy.php b/app/Strategies/DefaultLoginStrategy.php index 1a90c770..576755d8 100644 --- a/app/Strategies/DefaultLoginStrategy.php +++ b/app/Strategies/DefaultLoginStrategy.php @@ -21,8 +21,10 @@ use Utils\Services\IAuthService; use Illuminate\Support\Facades\Auth; use Illuminate\Support\Facades\Redirect; +use Illuminate\Support\Facades\Response; use Illuminate\Support\Facades\View; use Illuminate\Support\Facades\URL; +use Symfony\Component\HttpFoundation\Response as HttpResponse; /** * Class DefaultLoginStrategy * @package Strategies @@ -113,4 +115,16 @@ public function errorLogin(array $params) $response = $response->with($key, $val); return $response; } + + /** + * @param array $params + * @return mixed + */ + public function challengeRequired(array $params) + { + return Response::json( + array_merge(['error_code' => ILoginStrategy::MFA_REQUIRED], $params), + HttpResponse::HTTP_OK + ); + } } \ No newline at end of file diff --git a/app/Strategies/DisplayResponseJsonStrategy.php b/app/Strategies/DisplayResponseJsonStrategy.php index 3f30a325..150e1596 100644 --- a/app/Strategies/DisplayResponseJsonStrategy.php +++ b/app/Strategies/DisplayResponseJsonStrategy.php @@ -96,4 +96,13 @@ public function getLoginErrorResponse(array $data = []) } return Response::json($data, 412); } + + /** + * @param array $data + * @return SymfonyResponse + */ + public function getChallengeRequiredResponse(array $data = []) + { + return Response::json(array_merge(['error_code' => ILoginStrategy::MFA_REQUIRED], $data), 412); + } } \ No newline at end of file diff --git a/app/Strategies/DisplayResponseUserAgentStrategy.php b/app/Strategies/DisplayResponseUserAgentStrategy.php index 832d5bcb..b25cfe14 100644 --- a/app/Strategies/DisplayResponseUserAgentStrategy.php +++ b/app/Strategies/DisplayResponseUserAgentStrategy.php @@ -72,4 +72,17 @@ public function getLoginErrorResponse(array $data = []) return $response; } + + /** + * Same live-JSON, no-reload contract as DefaultLoginStrategy: OAuth2 + * page/popup/touch flows render the same login.js SPA, so the MFA + * challenge transitions in-place rather than doing a full page reload. + * + * @param array $data + * @return SymfonyResponse + */ + public function getChallengeRequiredResponse(array $data = []) + { + return Response::json(array_merge(['error_code' => ILoginStrategy::MFA_REQUIRED], $data), 200); + } } \ No newline at end of file diff --git a/app/Strategies/IDisplayResponseStrategy.php b/app/Strategies/IDisplayResponseStrategy.php index 019615a8..21e8a8a8 100644 --- a/app/Strategies/IDisplayResponseStrategy.php +++ b/app/Strategies/IDisplayResponseStrategy.php @@ -34,4 +34,13 @@ public function getLoginResponse(array $data = []); * @return SymfonyResponse */ public function getLoginErrorResponse(array $data = []); + + /** + * Factor 1 (password) passed but a 2FA challenge must be completed before + * a session is established. + * + * @param array $data + * @return SymfonyResponse + */ + public function getChallengeRequiredResponse(array $data = []); } \ No newline at end of file diff --git a/app/Strategies/ILoginStrategy.php b/app/Strategies/ILoginStrategy.php index 73120130..5894bc4d 100644 --- a/app/Strategies/ILoginStrategy.php +++ b/app/Strategies/ILoginStrategy.php @@ -5,6 +5,12 @@ */ interface ILoginStrategy { + /** + * error_code returned by challengeRequired() when factor 1 passed but a + * 2FA challenge is pending. + */ + const MFA_REQUIRED = 'mfa_required'; + /** * @return mixed */ @@ -26,4 +32,14 @@ public function cancelLogin(); * @return mixed */ public function errorLogin(array $params); -} \ No newline at end of file + + /** + * Factor 1 (password) passed but a 2FA challenge must be completed before + * a session is established. Distinct from errorLogin(): this is a pending + * mid-flow state, not a failed attempt. + * + * @param array $params + * @return mixed + */ + public function challengeRequired(array $params); +} \ No newline at end of file diff --git a/app/Strategies/OAuth2LoginStrategy.php b/app/Strategies/OAuth2LoginStrategy.php index bcbd5123..8161062c 100644 --- a/app/Strategies/OAuth2LoginStrategy.php +++ b/app/Strategies/OAuth2LoginStrategy.php @@ -127,4 +127,21 @@ public function errorLogin(array $params) return $response_strategy->getLoginErrorResponse($params); } + + /** + * @param array $params + * @return mixed + */ + public function challengeRequired(array $params) + { + $auth_request = OAuth2AuthorizationRequestFactory::getInstance()->build( + OAuth2Message::buildFromMemento( + $this->memento_service->load() + ) + ); + + $response_strategy = DisplayResponseStrategyFactory::build($auth_request->getDisplay()); + + return $response_strategy->getChallengeRequiredResponse($params); + } } \ No newline at end of file diff --git a/tests/OAuth2NativeMFALoginFlowTest.php b/tests/OAuth2NativeMFALoginFlowTest.php new file mode 100644 index 00000000..c3d2f7d0 --- /dev/null +++ b/tests/OAuth2NativeMFALoginFlowTest.php @@ -0,0 +1,63 @@ +action('POST', 'OAuth2\OAuth2ProviderController@auth', [ + 'client_id' => self::CLIENT_ID, + 'redirect_uri' => 'https://www.test.com:443/oauth2?param=1&BackUrl=123344', + 'response_type' => 'code', + 'scope' => sprintf('%s/resource-server/read', Config::get('app.url')), + 'display' => 'native', + ]); + + // Step 2: submit the enforced-2FA admin's password within the same + // session - this is what postLogin() sees as an OAuth2-originated + // login attempt via the persisted memento. + $response = $this->action('POST', 'UserController@postLogin', [ + 'username' => self::ADMIN_EMAIL, + 'password' => self::SEED_PASSWORD, + 'flow' => 'password', + '_token' => Session::token(), + ]); + + $this->assertResponseStatus(412); + $payload = json_decode($response->getContent(), true); + $this->assertSame('mfa_required', $payload['error_code']); + } +} diff --git a/tests/TwoFactorLoginFlowTest.php b/tests/TwoFactorLoginFlowTest.php index 0fc20bd6..2fdbf471 100644 --- a/tests/TwoFactorLoginFlowTest.php +++ b/tests/TwoFactorLoginFlowTest.php @@ -80,6 +80,29 @@ public function testAdminLoginTriggersMFAChallenge(): void $this->assertGreaterThan(0, $this->countAudit($admin->getId(), TwoFactorAuditLog::EventChallengeIssued)); } + public function testAdminLoginPersistsUIStateForRefreshResilience(): void + { + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + + $this->assertSame('2fa', Session::get('flow'), 'a refresh mid-challenge must restore the 2FA screen, not the password form'); + $this->assertNotNull(Session::get('otp_length')); + $this->assertNotNull(Session::get('otp_lifetime')); + $this->assertSame(User::MFAMethod_OTP, Session::get('mfa_method'), 'a refresh must restore the screen for the method actually challenged, not a hardcoded default'); + } + + public function testSuccessfulVerificationClearsUIState(): void + { + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + $code = $this->latestOtpCode(self::ADMIN_EMAIL); + + $this->verify($code); + + $this->assertNull(Session::get('flow'), 'completed challenge must not leave the 2FA screen re-derivable from a stale refresh'); + $this->assertNull(Session::get('otp_length')); + $this->assertNull(Session::get('otp_lifetime')); + $this->assertNull(Session::get('mfa_method')); + } + public function testNonAdminWithoutMFALogsInNormally(): void { $email = $this->createPlainUser(); From 1c3721c7a7b1b535e4d0fc4a7af8ac6693965cf6 Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 22 Jul 2026 18:47:25 -0300 Subject: [PATCH 06/14] Clear pending MFA challenge and UI-restoration state on cancelLogin() None of the three login strategies' cancelLogin() cleared any 2FA session state - not the pre-existing 2fa_pending_user_id/2fa_pending_at/2fa_remember keys, nor the flow/mfa_method/otp_length/otp_lifetime keys added for refresh-resilience. PR #142's Cancel button resets the client's React state immediately and fires cancelLogin() as a best-effort background call, so the broken UX was masked within the same tab - but a subsequent full page load within the challenge's 300s TTL (back button, reopened tab, direct /login navigation) would restore the 2FA screen for a challenge the user explicitly abandoned, and the stale OTP could still complete it. UserController::cancelLogin() now resolves the pending strategy via the mfa_method session key (when present) and clears its pending state before delegating to the login strategy, plus clears the UI-restoration keys via the existing clearMFAUISessionState() helper. New test proves the strongest form of the property: an OTP valid before cancel returns mfa_session_expired afterward, not just that some session keys are gone. --- app/Http/Controllers/UserController.php | 9 +++++++ tests/TwoFactorLoginFlowTest.php | 31 +++++++++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index b4813545..b1b5901a 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -280,6 +280,15 @@ public function getLogin() public function cancelLogin() { + // A cancelled login must invalidate any pending MFA challenge server-side, + // not just reset the client's view of things - otherwise an OTP issued + // before cancel can still complete a login the user explicitly abandoned. + $method = Session::get('mfa_method'); + if (!is_null($method)) { + MFAChallengeStrategyFactory::create($method)->clearPendingState(); + } + $this->clearMFAUISessionState(); + return $this->login_strategy->cancelLogin(); } diff --git a/tests/TwoFactorLoginFlowTest.php b/tests/TwoFactorLoginFlowTest.php index 2fdbf471..8aa69728 100644 --- a/tests/TwoFactorLoginFlowTest.php +++ b/tests/TwoFactorLoginFlowTest.php @@ -113,6 +113,32 @@ public function testNonAdminWithoutMFALogsInNormally(): void $this->assertTrue(Auth::check(), 'a non-MFA user must get an authenticated session'); } + // ------------------------------------------------------------------------- + // cancelLogin + // ------------------------------------------------------------------------- + + public function testCancelClearsUIStateAndPendingChallenge(): void + { + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + $code = $this->latestOtpCode(self::ADMIN_EMAIL); + + $this->cancelLogin(); + + $this->assertNull(Session::get('flow'), 'cancel must not leave a stale 2FA screen restorable on refresh'); + $this->assertNull(Session::get('mfa_method')); + $this->assertNull(Session::get('otp_length')); + $this->assertNull(Session::get('otp_lifetime')); + + // The strongest proof: the OTP issued before cancel must no longer + // complete a login. If pending state survived cancel, this would + // succeed with a 302 despite the user having explicitly cancelled. + $response = $this->verify($code); + $this->assertResponseStatus(401); + $payload = json_decode($response->getContent(), true); + $this->assertSame('mfa_session_expired', $payload['error_code']); + $this->assertFalse(Auth::check(), 'a cancelled challenge must never establish a session'); + } + // ------------------------------------------------------------------------- // verify2FA // ------------------------------------------------------------------------- @@ -431,6 +457,11 @@ private function postLogin(string $username, string $password, array $cookies = ], [], $cookies); } + private function cancelLogin() + { + return $this->action('GET', 'UserController@cancelLogin'); + } + private function verify(string $otp, bool $trustDevice = false) { return $this->action('POST', 'UserController@verify2FA', [ From 2491722c30926acc99005d3fedda77a8b0a4d264 Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 22 Jul 2026 19:31:10 -0300 Subject: [PATCH 07/14] Block passwordless MFA bypass; make challengeRequired self-contained MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two related fixes to the MFA login flow: 1. Passwordless (flow=otp) login never checked shouldRequire2FA(), so an enforced-2FA user could bypass MFA entirely via emitOTP() + postLogin with flow=otp instead of flow=password (SDS idp-mfa.md §7.4 / Open Question #3 explicitly treats passwordless as single-factor). Now throws AuthenticationException before loginWithOTP(), reusing the existing errorLogin() redirect+flash path - the OTP form still submits as a native form POST, so this needed no new response contract. 2. challengeRequired()'s redirect-based implementations (DefaultLoginStrategy, DisplayResponseUserAgentStrategy) previously ignored the $params they received, silently depending on the caller having already flashed otp_length/otp_lifetime to session - an implicit contract that would silently break for any other caller. Both now flash their own $params (persistent, not one-shot, so it survives repeated refreshes) and set error_code, mirroring what DisplayResponseJsonStrategy already sends native clients in JSON. clearMFAUISessionState() now clears error_code too. The '2fa' flow value moves from a new ILoginStrategy constant to IAuthService::AuthenticationFlowMFA, alongside its siblings AuthenticationFlowPassword/AuthenticationFlowPasswordless - all three are the same session 'flow' enum (already flashed together in the AuthenticationException catch block), so splitting the third value into a different interface would have been inconsistent. New test: OAuth2NativeMFALoginFlowTest gains a non-native (page/popup/ touch) case proving the 302+session-flash contract, alongside the existing native 412+JSON case. TwoFactorLoginFlowTest covers the passwordless-bypass rejection (including that it still reuses errorLogin(), not a new JSON contract) and the error_code flash/clear. --- app/Http/Controllers/UserController.php | 24 ++++--- app/Strategies/DefaultLoginStrategy.php | 16 +++-- .../DisplayResponseUserAgentStrategy.php | 17 +++-- app/libs/Utils/Services/IAuthService.php | 1 + tests/OAuth2NativeMFALoginFlowTest.php | 65 ++++++++++++----- tests/TwoFactorLoginFlowTest.php | 70 ++++++++++++++++++- 6 files changed, 155 insertions(+), 38 deletions(-) diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index b1b5901a..58729b56 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -501,16 +501,11 @@ public function postLogin() // Restore-on-refresh: a subsequent GET /login can rehydrate // the 2FA screen from session instead of dropping back to the - // password form (mirrors what a redirect-based flow gets for - // free via session-flashed params). - Session::put('flow', '2fa'); + // password form. otp_length/otp_lifetime (part of $payload) + // are flashed by challengeRequired() itself; flow/mfa_method + // aren't part of the challenge payload, so they're set here. + Session::put('flow', IAuthService::AuthenticationFlowMFA); Session::put('mfa_method', $method); - if (isset($payload['otp_length'])) { - Session::put('otp_length', $payload['otp_length']); - } - if (isset($payload['otp_lifetime'])) { - Session::put('otp_lifetime', $payload['otp_lifetime']); - } return $this->login_strategy->challengeRequired($payload); } @@ -522,6 +517,16 @@ public function postLogin() if ($flow == IAuthService::AuthenticationFlowPasswordless) { + // Passwordless login is single-factor (email access only) and + // must not be usable to satisfy MFA enforcement (SDS idp-mfa.md + // §7.4 / Open Question #3). + $existing_user = $this->auth_service->getUserByUsername($username); + if (!is_null($existing_user) && $existing_user->shouldRequire2FA()) { + throw new AuthenticationException( + "This account requires password and two-factor authentication. Please use the password login option." + ); + } + $client = $this->resolveClientFromMemento(); $otpClaim = OAuth2OTP::fromParams($username, $connection, $password); @@ -907,6 +912,7 @@ private function clearMFAUISessionState(): void Session::forget('mfa_method'); Session::forget('otp_length'); Session::forget('otp_lifetime'); + Session::forget('error_code'); } /** diff --git a/app/Strategies/DefaultLoginStrategy.php b/app/Strategies/DefaultLoginStrategy.php index 576755d8..693ee417 100644 --- a/app/Strategies/DefaultLoginStrategy.php +++ b/app/Strategies/DefaultLoginStrategy.php @@ -21,10 +21,8 @@ use Utils\Services\IAuthService; use Illuminate\Support\Facades\Auth; use Illuminate\Support\Facades\Redirect; -use Illuminate\Support\Facades\Response; use Illuminate\Support\Facades\View; use Illuminate\Support\Facades\URL; -use Symfony\Component\HttpFoundation\Response as HttpResponse; /** * Class DefaultLoginStrategy * @package Strategies @@ -122,9 +120,15 @@ public function errorLogin(array $params) */ public function challengeRequired(array $params) { - return Response::json( - array_merge(['error_code' => ILoginStrategy::MFA_REQUIRED], $params), - HttpResponse::HTTP_OK - ); + // The login form submits as a native form POST, so this must redirect + // like every other outcome (errorLogin()) rather than return JSON. + // Persistent (not one-shot flash) so the state survives repeated + // refreshes while the challenge is still pending. error_code mirrors + // what DisplayResponseJsonStrategy sends native clients in JSON. + Session::put('error_code', ILoginStrategy::MFA_REQUIRED); + foreach ($params as $key => $val) { + Session::put($key, $val); + } + return Redirect::action('UserController@getLogin'); } } \ No newline at end of file diff --git a/app/Strategies/DisplayResponseUserAgentStrategy.php b/app/Strategies/DisplayResponseUserAgentStrategy.php index b25cfe14..ea8358ba 100644 --- a/app/Strategies/DisplayResponseUserAgentStrategy.php +++ b/app/Strategies/DisplayResponseUserAgentStrategy.php @@ -17,6 +17,7 @@ use Symfony\Component\HttpFoundation\Response as SymfonyResponse; use Illuminate\Support\Facades\Response; use Illuminate\Support\Facades\Redirect; +use Illuminate\Support\Facades\Session; /** * Class DisplayResponseUserAgentStrategy @@ -74,15 +75,23 @@ public function getLoginErrorResponse(array $data = []) } /** - * Same live-JSON, no-reload contract as DefaultLoginStrategy: OAuth2 - * page/popup/touch flows render the same login.js SPA, so the MFA - * challenge transitions in-place rather than doing a full page reload. + * Same redirect+session-flash contract as getLoginErrorResponse(): OAuth2 + * page/popup/touch flows render the same login.js SPA via a native form + * POST, so the MFA challenge is delivered the same way every other login + * outcome is. Persistent (not one-shot flash) so the state survives + * repeated refreshes while the challenge is still pending. * * @param array $data * @return SymfonyResponse */ public function getChallengeRequiredResponse(array $data = []) { - return Response::json(array_merge(['error_code' => ILoginStrategy::MFA_REQUIRED], $data), 200); + // error_code mirrors what DisplayResponseJsonStrategy sends native + // clients in JSON. + Session::put('error_code', ILoginStrategy::MFA_REQUIRED); + foreach ($data as $key => $val) { + Session::put($key, $val); + } + return Redirect::action('UserController@getLogin'); } } \ No newline at end of file diff --git a/app/libs/Utils/Services/IAuthService.php b/app/libs/Utils/Services/IAuthService.php index b1b6abfc..ce68d06c 100644 --- a/app/libs/Utils/Services/IAuthService.php +++ b/app/libs/Utils/Services/IAuthService.php @@ -39,6 +39,7 @@ interface IAuthService const AuthenticationFlowPassword = "password"; const AuthenticationFlowPasswordless = "otp"; + const AuthenticationFlowMFA = "2fa"; /** * @return bool */ diff --git a/tests/OAuth2NativeMFALoginFlowTest.php b/tests/OAuth2NativeMFALoginFlowTest.php index c3d2f7d0..ea9ddb27 100644 --- a/tests/OAuth2NativeMFALoginFlowTest.php +++ b/tests/OAuth2NativeMFALoginFlowTest.php @@ -16,9 +16,12 @@ use Illuminate\Support\Facades\Session; /** - * Verifies that an OAuth2 native client (display=native) requesting login - * against an enforced-2FA account gets the SAME JSON+412 contract as every - * other login error for that client, rather than the plain-flow's JSON+200. + * Verifies that an MFA challenge issued mid-OAuth2-authorization-flow gets + * the response shape appropriate to the request's display mode: JSON+412 + * for native clients (matching every other login error for that mode), + * redirect+session-flash for page/popup/touch (matching DefaultLoginStrategy + * and errorLogin(), since both render the same login.js SPA via a native + * form POST). * * @package Tests */ @@ -36,28 +39,58 @@ protected function prepareForTests(): void public function testNativeClientReceives412OnMFARequired(): void { - // Step 1: unauthenticated authorize request with display=native - the - // grant serializes the OAuth2 memento and hands off to the login flow. - $this->action('POST', 'OAuth2\OAuth2ProviderController@auth', [ + $this->authorize('native'); + + $response = $this->postLoginPassword(); + + $this->assertResponseStatus(412); + $payload = json_decode($response->getContent(), true); + $this->assertSame('mfa_required', $payload['error_code']); + } + + public function testNonNativeClientReceives302RedirectOnMFARequired(): void + { + // No display param -> defaults to the non-native (page/popup/touch) + // display strategy. + $this->authorize(); + + $response = $this->postLoginPassword(); + + $this->assertResponseStatus(302, 'must redirect like every other login outcome, not return JSON'); + $this->assertSame('2fa', Session::get('flow'), 'the redirected page must be able to restore the 2FA screen'); + } + + /** + * Unauthenticated authorize request - the grant serializes the OAuth2 + * memento and hands off to the login flow. + */ + private function authorize(?string $display = null) + { + $params = [ 'client_id' => self::CLIENT_ID, 'redirect_uri' => 'https://www.test.com:443/oauth2?param=1&BackUrl=123344', 'response_type' => 'code', 'scope' => sprintf('%s/resource-server/read', Config::get('app.url')), - 'display' => 'native', - ]); + ]; + if (!is_null($display)) { + $params['display'] = $display; + } + + return $this->action('POST', 'OAuth2\OAuth2ProviderController@auth', $params); + } - // Step 2: submit the enforced-2FA admin's password within the same - // session - this is what postLogin() sees as an OAuth2-originated - // login attempt via the persisted memento. - $response = $this->action('POST', 'UserController@postLogin', [ + /** + * Submits the enforced-2FA admin's password within the same session - + * this is what postLogin() sees as an OAuth2-originated login attempt + * via the persisted memento. + */ + private function postLoginPassword() + { + return $this->action('POST', 'UserController@postLogin', [ 'username' => self::ADMIN_EMAIL, 'password' => self::SEED_PASSWORD, 'flow' => 'password', '_token' => Session::token(), ]); - - $this->assertResponseStatus(412); - $payload = json_decode($response->getContent(), true); - $this->assertSame('mfa_required', $payload['error_code']); } } diff --git a/tests/TwoFactorLoginFlowTest.php b/tests/TwoFactorLoginFlowTest.php index 8aa69728..117684dc 100644 --- a/tests/TwoFactorLoginFlowTest.php +++ b/tests/TwoFactorLoginFlowTest.php @@ -28,6 +28,7 @@ use Illuminate\Support\Facades\Session; use App\libs\OAuth2\Repositories\IOAuth2OTPRepository; use LaravelDoctrine\ORM\Facades\EntityManager; +use Strategies\ILoginStrategy; /** * Integration tests for the MFA-gated password login flow wired into UserController. @@ -71,9 +72,12 @@ public function testAdminLoginTriggersMFAChallenge(): void { $response = $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); - $this->assertResponseStatus(200); - $payload = json_decode($response->getContent(), true); - $this->assertSame('mfa_required', $payload['error_code']); + // The password flow submits as a native form POST, so the challenge + // is delivered via the same redirect+session-flash mechanism as every + // other login outcome (errorLogin()), not a live JSON response - + // see testAdminLoginPersistsUIStateForRefreshResilience for the + // session-state assertions the redirected page relies on. + $this->assertResponseStatus(302, 'must redirect back to the login screen, same as errorLogin(), not return JSON'); $this->assertFalse(Auth::check(), 'no session must be established when a challenge is required'); $admin = $this->user(self::ADMIN_EMAIL); @@ -88,6 +92,7 @@ public function testAdminLoginPersistsUIStateForRefreshResilience(): void $this->assertNotNull(Session::get('otp_length')); $this->assertNotNull(Session::get('otp_lifetime')); $this->assertSame(User::MFAMethod_OTP, Session::get('mfa_method'), 'a refresh must restore the screen for the method actually challenged, not a hardcoded default'); + $this->assertSame(ILoginStrategy::MFA_REQUIRED, Session::get('error_code'), 'must match what DisplayResponseJsonStrategy sends native clients in its JSON body'); } public function testSuccessfulVerificationClearsUIState(): void @@ -101,6 +106,7 @@ public function testSuccessfulVerificationClearsUIState(): void $this->assertNull(Session::get('otp_length')); $this->assertNull(Session::get('otp_lifetime')); $this->assertNull(Session::get('mfa_method')); + $this->assertNull(Session::get('error_code')); } public function testNonAdminWithoutMFALogsInNormally(): void @@ -113,6 +119,43 @@ public function testNonAdminWithoutMFALogsInNormally(): void $this->assertTrue(Auth::check(), 'a non-MFA user must get an authenticated session'); } + // ------------------------------------------------------------------------- + // passwordless (flow=otp) login must not bypass the MFA gate + // ------------------------------------------------------------------------- + + public function testEnforcedUserCannotBypassMFAViaPasswordlessLogin(): void + { + $this->emitOTP(self::ADMIN_EMAIL); + $code = $this->latestOtpCode(self::ADMIN_EMAIL); + + $response = $this->postLoginOTP(self::ADMIN_EMAIL, $code); + + $this->assertFalse(Auth::check(), 'passwordless login must not authenticate an enforced-2FA user'); + + // The OTP flow still submits as a native form POST (PR #142 only + // converted the password flow to AJAX), so the rejection must go + // through the pre-existing errorLogin() redirect+flash mechanism, + // not the JSON contract built for the password flow's MFA gate. + $this->assertResponseStatus(302, 'must reuse errorLogin(), not a JSON response'); + $this->assertStringContainsString( + 'two-factor authentication', + Session::get('flash_notice'), + 'the flashed message must explain why passwordless login was rejected' + ); + $this->assertSame('otp', Session::get('flow'), 'a reload must land back on the OTP screen, not silently fall back to password'); + } + + public function testNonEnforcedUserStillLogsInViaPasswordlessLogin(): void + { + $email = $this->createPlainUser(); + $this->emitOTP($email); + $code = $this->latestOtpCode($email); + + $this->postLoginOTP($email, $code); + + $this->assertTrue(Auth::check(), 'passwordless login must keep working unchanged for non-enforced users'); + } + // ------------------------------------------------------------------------- // cancelLogin // ------------------------------------------------------------------------- @@ -462,6 +505,27 @@ private function cancelLogin() return $this->action('GET', 'UserController@cancelLogin'); } + private function emitOTP(string $username) + { + return $this->action('POST', 'UserController@emitOTP', [ + 'username' => $username, + 'connection' => 'email', + 'send' => 'code', + '_token' => Session::token(), + ]); + } + + private function postLoginOTP(string $username, string $code) + { + return $this->action('POST', 'UserController@postLogin', [ + 'username' => $username, + 'password' => $code, + 'connection' => 'email', + 'flow' => 'otp', + '_token' => Session::token(), + ]); + } + private function verify(string $otp, bool $trustDevice = false) { return $this->action('POST', 'UserController@verify2FA', [ From 203850d7657df4016ea8344adb61c8406e1705cc Mon Sep 17 00:00:00 2001 From: smarcet Date: Thu, 23 Jul 2026 01:30:31 -0300 Subject: [PATCH 08/14] Rate-limit the initial MFA challenge issuance in postLogin() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The '2fa.rate' middleware could never gate postLogin()'s initial OTP issuance: its before-phase reads 2fa_pending_user_id from session to know which user to throttle, but that key is only written by issueChallenge() - inside the very request that would need throttling. A user with valid credentials could repeatedly POST to the plain login route and trigger unlimited email-OTP sends, bypassing the 5-per-15-minute resend cap entirely (SDS idp-mfa.md §4.12 explicitly requires the initial issuance to share the same 2fa_rate:resend:{user_id} window as resend()). Extracted the cache-key/window logic that lived only in TwoFactorRateLimitMiddleware into ITwoFactorRateLimitService / TwoFactorRateLimitService (same pattern as DeviceTrustService / TwoFactorAuditService / MFAGateService, registered in TwoFactorServiceProvider), so both the middleware (verify/recovery/resend routes) and UserController::postLogin() (initial issuance, now knows the user id post-validateCredentials()) share one source of truth instead of duplicating cache-key construction. postLogin() checks isRateLimited() before issuing a challenge and calls increment() after a successful issue. The rejection throws AuthenticationException, reusing the existing catch block's errorLogin() redirect+flash path - consistent with challengeRequired() already being redirect-based, since the password form still submits as a native form POST. resend2FA()/verify2FA()/verifyRecoveryCode() stay JSON+429 via the middleware, unaffected, since those are AJAX-only endpoints. New test proves postLogin() and resend() share the same window: after max_otp_requests postLogin() calls, the next one is rejected. --- app/Http/Controllers/UserController.php | 20 ++++++ .../TwoFactorRateLimitMiddleware.php | 70 +++++-------------- .../Auth/ITwoFactorRateLimitService.php | 51 ++++++++++++++ .../Auth/TwoFactorRateLimitService.php | 68 ++++++++++++++++++ .../Auth/TwoFactorServiceProvider.php | 2 + tests/TwoFactorLoginFlowTest.php | 22 ++++++ 6 files changed, 180 insertions(+), 53 deletions(-) create mode 100644 app/Services/Auth/ITwoFactorRateLimitService.php create mode 100644 app/Services/Auth/TwoFactorRateLimitService.php diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index 58729b56..a39de12e 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -23,6 +23,7 @@ use App\Services\Auth\IDeviceTrustService; use App\Services\Auth\ITwoFactorAuditService; use App\Services\Auth\ITwoFactorGateService; +use App\Services\Auth\ITwoFactorRateLimitService; use Auth\User; use Models\OAuth2\Client; use Strategies\MFA\MFAChallengeStrategyFactory; @@ -157,6 +158,11 @@ final class UserController extends OpenIdController */ private $mfa_gate_service; + /** + * @var ITwoFactorRateLimitService + */ + private $two_factor_rate_limit_service; + /** * @param IMementoOpenIdSerializerService $openid_memento_service * @param IMementoOAuth2SerializerService $oauth2_memento_service @@ -196,6 +202,7 @@ public function __construct IDeviceTrustService $device_trust_service, ITwoFactorAuditService $two_factor_audit_service, ITwoFactorGateService $mfa_gate_service, + ITwoFactorRateLimitService $two_factor_rate_limit_service, ) { $this->openid_memento_service = $openid_memento_service; @@ -216,6 +223,7 @@ public function __construct $this->device_trust_service = $device_trust_service; $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->middleware(function ($request, $next) use($login_hint_process_strategy){ @@ -486,11 +494,23 @@ public function postLogin() $cookieToken = $this->getCookieToken(); if ($this->mfa_gate_service->requiresChallenge($user, $cookieToken)) { + // Initial issuance shares the resend rate-limit window + // (SDS idp-mfa.md §4.12) - without this, this route + // would be an unthrottled way to mail-bomb the account + // owner with OTP codes. + if ($this->two_factor_rate_limit_service->isRateLimited( + ITwoFactorRateLimitService::ActionResend, + $user->getId() + )) { + throw new AuthenticationException(ITwoFactorRateLimitService::RATE_LIMIT_MESSAGE); + } + // Issue a challenge and stop short of session creation. $client = $this->resolveClientFromMemento(); $method = $user->getTwoFactorMethod(); $strategy = MFAChallengeStrategyFactory::create($method); $payload = $this->auth_service->issueMFAChallenge($user, $strategy, $client, $remember); + $this->two_factor_rate_limit_service->increment(ITwoFactorRateLimitService::ActionResend, $user->getId()); $this->two_factor_audit_service->log( $user, diff --git a/app/Http/Middleware/TwoFactorRateLimitMiddleware.php b/app/Http/Middleware/TwoFactorRateLimitMiddleware.php index a0bba142..7a4ec2d4 100644 --- a/app/Http/Middleware/TwoFactorRateLimitMiddleware.php +++ b/app/Http/Middleware/TwoFactorRateLimitMiddleware.php @@ -12,9 +12,8 @@ * limitations under the License. **/ +use App\Services\Auth\ITwoFactorRateLimitService; use Closure; -use Illuminate\Support\Facades\Cache; -use Illuminate\Support\Facades\Config; use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Response; use Illuminate\Support\Facades\Session; @@ -23,9 +22,10 @@ /** * Class TwoFactorRateLimitMiddleware * - * Throttles the MFA verify / recovery / resend endpoints. Counters are stored in - * the cache (NOT the session) so they survive session cleanup and keep an - * independent, fixed-window TTL. + * Throttles the MFA verify / recovery / resend endpoints. Counter state and + * window logic live in ITwoFactorRateLimitService (shared with + * UserController::postLogin(), whose initial challenge issuance counts + * against the same resend window - SDS idp-mfa.md §4.12). * * - verify / recovery: increment ONLY on a failed response. * - resend: increment on EVERY request. @@ -34,10 +34,6 @@ */ final class TwoFactorRateLimitMiddleware { - public const ActionVerify = 'verify'; - public const ActionRecovery = 'recovery'; - public const ActionResend = 'resend'; - private const PENDING_USER_KEY = '2fa_pending_user_id'; /** @@ -48,13 +44,17 @@ final class TwoFactorRateLimitMiddleware 'mfa_invalid_recovery', ]; + public function __construct(private readonly ITwoFactorRateLimitService $rate_limit_service) + { + } + /** * @param \Illuminate\Http\Request $request * @param Closure $next * @param string $action one of verify|recovery|resend * @return mixed */ - public function handle($request, Closure $next, string $action = self::ActionVerify) + public function handle($request, Closure $next, string $action = ITwoFactorRateLimitService::ActionVerify) { $userId = Session::get(self::PENDING_USER_KEY); @@ -64,15 +64,14 @@ public function handle($request, Closure $next, string $action = self::ActionVer return $next($request); } - [$maxAttempts, $windowSeconds] = $this->limitsFor($action); - $key = $this->cacheKey($action, $userId); + $userId = (int) $userId; - if ((int) Cache::get($key, 0) >= $maxAttempts) { + if ($this->rate_limit_service->isRateLimited($action, $userId)) { Log::debug(sprintf("TwoFactorRateLimitMiddleware: action %s user %s rate limited", $action, $userId)); return Response::json( [ - 'error_code' => 'mfa_rate_limit', - 'error_message' => 'Too many attempts. Please try again later.', + 'error_code' => ITwoFactorRateLimitService::RATE_LIMIT_ERROR_CODE, + 'error_message' => ITwoFactorRateLimitService::RATE_LIMIT_MESSAGE, ], HttpResponse::HTTP_TOO_MANY_REQUESTS ); @@ -80,50 +79,15 @@ public function handle($request, Closure $next, string $action = self::ActionVer $response = $next($request); - if ($action === self::ActionResend) { - $this->increment($key, $windowSeconds); + if ($action === ITwoFactorRateLimitService::ActionResend) { + $this->rate_limit_service->increment($action, $userId); } else if ($this->isFailure($response)) { - $this->increment($key, $windowSeconds); + $this->rate_limit_service->increment($action, $userId); } return $response; } - /** - * @param string $action - * @return array{0:int,1:int} [maxAttempts, windowSeconds] - */ - private function limitsFor(string $action): array - { - if ($action === self::ActionResend) { - return [ - (int) Config::get('two_factor.rate_limit.max_otp_requests', 5), - (int) Config::get('two_factor.rate_limit.otp_window_minutes', 15) * 60, - ]; - } - - return [ - (int) Config::get('two_factor.rate_limit.max_attempts', 3), - (int) Config::get('two_factor.rate_limit.window_seconds', 900), - ]; - } - - private function cacheKey(string $action, $userId): string - { - return sprintf('2fa_rate:%s:%s', $action, $userId); - } - - /** - * Increment within a fixed window: add() sets the TTL once (only if the key - * is absent), increment() bumps the value while preserving that TTL, so the - * window starts at the first hit and does not slide. - */ - private function increment(string $key, int $windowSeconds): void - { - Cache::add($key, 0, $windowSeconds); - Cache::increment($key); - } - /** * @param mixed $response * @return bool diff --git a/app/Services/Auth/ITwoFactorRateLimitService.php b/app/Services/Auth/ITwoFactorRateLimitService.php new file mode 100644 index 00000000..46b7a835 --- /dev/null +++ b/app/Services/Auth/ITwoFactorRateLimitService.php @@ -0,0 +1,51 @@ +limitsFor($action); + return (int) Cache::get($this->cacheKey($action, $userId), 0) >= $maxAttempts; + } + + public function increment(string $action, int $userId): void + { + [, $windowSeconds] = $this->limitsFor($action); + $key = $this->cacheKey($action, $userId); + + // Fixed window: add() sets the TTL once (only if the key is absent), + // increment() bumps the value while preserving that TTL, so the + // window starts at the first hit and does not slide. + Cache::add($key, 0, $windowSeconds); + Cache::increment($key); + } + + /** + * @param string $action + * @return array{0:int,1:int} [maxAttempts, windowSeconds] + */ + private function limitsFor(string $action): array + { + if ($action === self::ActionResend) { + return [ + (int) Config::get('two_factor.rate_limit.max_otp_requests', 5), + (int) Config::get('two_factor.rate_limit.otp_window_minutes', 15) * 60, + ]; + } + + return [ + (int) Config::get('two_factor.rate_limit.max_attempts', 3), + (int) Config::get('two_factor.rate_limit.window_seconds', 900), + ]; + } + + private function cacheKey(string $action, int $userId): string + { + return sprintf('2fa_rate:%s:%s', $action, $userId); + } +} diff --git a/app/Services/Auth/TwoFactorServiceProvider.php b/app/Services/Auth/TwoFactorServiceProvider.php index 081eed76..76b3750f 100644 --- a/app/Services/Auth/TwoFactorServiceProvider.php +++ b/app/Services/Auth/TwoFactorServiceProvider.php @@ -34,6 +34,7 @@ public function register(): void $this->app->singleton(IDeviceTrustService::class, DeviceTrustService::class); $this->app->singleton(ITwoFactorAuditService::class, TwoFactorAuditService::class); $this->app->singleton(ITwoFactorGateService::class, MFAGateService::class); + $this->app->singleton(ITwoFactorRateLimitService::class, TwoFactorRateLimitService::class); } public function provides(): array @@ -42,6 +43,7 @@ public function provides(): array IDeviceTrustService::class, ITwoFactorAuditService::class, ITwoFactorGateService::class, + ITwoFactorRateLimitService::class, ]; } } diff --git a/tests/TwoFactorLoginFlowTest.php b/tests/TwoFactorLoginFlowTest.php index 117684dc..9275f19c 100644 --- a/tests/TwoFactorLoginFlowTest.php +++ b/tests/TwoFactorLoginFlowTest.php @@ -486,6 +486,28 @@ public function testResendRateLimitBlocksAfterThreshold(): void $this->assertSame('mfa_rate_limit', $payload['error_code']); } + public function testInitialChallengeIssuanceCountsAgainstResendRateLimitWindow(): void + { + // SDS idp-mfa.md §4.12: "The initial OTP issuance during postLogin() + // shares the 2fa_rate:resend:{user_id} cache key, ensuring the first + // challenge counts against the same 5-request issuance window as + // subsequent resends." Each postLogin() call re-issues a challenge and + // must count against that SAME window. + $max = (int) Config::get('two_factor.rate_limit.max_otp_requests'); + for ($i = 0; $i < $max; $i++) { + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + } + + $response = $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + + // Rate-limited postLogin() must reuse errorLogin(), not the JSON + // contract resend()/verify() use - the password form still submits + // as a native form POST. + $this->assertResponseStatus(302, 'must redirect like every other login outcome, not return JSON'); + $this->assertStringContainsString('Too many attempts', Session::get('flash_notice')); + $this->assertFalse(Auth::check()); + } + // ------------------------------------------------------------------------- // Helpers // ------------------------------------------------------------------------- From 23ce114602785e71525c82acea05d8f4d82f9706 Mon Sep 17 00:00:00 2001 From: smarcet Date: Thu, 23 Jul 2026 02:01:15 -0300 Subject: [PATCH 09/14] Fix op_browser_state ordering bug in AuthService::loginUser() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Investigated the "session fixation" finding from the PR review (SDS idp-mfa.md §9.3 asks for a test proving 2fa_pending_user_id cannot be injected). Traced actual runtime behavior via debug instrumentation before writing a fix, since pattern-matching "no explicit Session::regenerate() call" as a vulnerability turned out to be wrong. Laravel's SessionGuard::login() (invoked via Auth::login(), already called unconditionally at the end of loginUser()) already calls $session->migrate(true) internally - the session-fixation window was already closed by the framework, with no code change needed for that property specifically. An added test asserting this (comparing session ID before/after login) passed identically with or without any fix, proving it was a false positive caused by this test harness resetting the session ID between $this->action() calls regardless of production behavior - that test was written and then discarded rather than kept for false confidence. What IS real, found via the same investigation: PrincipalService::register() (called by loginUser() before this fix) hashes the CURRENT session ID into op_browser_state, used for OIDC Session Management (check-session iframe). Since register() ran BEFORE Auth::login(), its hash was computed from a session ID that Auth::login()'s own migrate(true) was about to invalidate moments later - any relying party polling the check-session iframe would see a value that no longer matched what the server would recompute, incorrectly signaling a session change. Fix: call Auth::login() first, then principal_service->clear()/register() after, so the hash uses the final, stable post-login session ID. No new Session::regenerate() call needed - Auth::login() already provides one. New tests: - AuthServiceLoginUserTest (unit, Mockery-alias facades, same pattern as AuthServiceLogoutTest): asserts the call order directly. - TwoFactorLoginFlowTest::testCompletedMFALoginKeepsOPBrowserStateConsistentWithSessionId (integration): proves op_browser_state matches a freshly-computed hash of the post-login session ID end-to-end through the real MFA verify flow. Confirmed failing against the pre-fix ordering, passing after. --- app/libs/Auth/AuthService.php | 9 ++- tests/AuthServiceLoginUserTest.php | 120 +++++++++++++++++++++++++++++ tests/TwoFactorLoginFlowTest.php | 30 ++++++++ 3 files changed, 158 insertions(+), 1 deletion(-) create mode 100644 tests/AuthServiceLoginUserTest.php diff --git a/app/libs/Auth/AuthService.php b/app/libs/Auth/AuthService.php index 6d52be81..665956d7 100644 --- a/app/libs/Auth/AuthService.php +++ b/app/libs/Auth/AuthService.php @@ -446,13 +446,20 @@ public function loginUser(User $user, bool $remember): void if (!$user->canLogin()) throw new AuthenticationException("User is not active or cannot login."); + // Auth::login() first: Laravel's SessionGuard::login() already + // regenerates the session ID internally (session->migrate(true)), + // closing the pre-auth session-fixation window. Principal bookkeeping + // runs AFTER so register()'s op_browser_state hash (used for OIDC + // Session Management) is derived from the FINAL id, not one that + // Auth::login() is about to invalidate. + Auth::login($user, $remember); + $this->principal_service->clear(); $this->principal_service->register ( $user->getId(), time() ); - Auth::login($user, $remember); } /** diff --git a/tests/AuthServiceLoginUserTest.php b/tests/AuthServiceLoginUserTest.php new file mode 100644 index 00000000..e5acba90 --- /dev/null +++ b/tests/AuthServiceLoginUserTest.php @@ -0,0 +1,120 @@ +migrate(true)), closing + * the pre-auth session-fixation window (SDS idp-mfa.md §9.3) - no explicit + * Session::regenerate() call is needed. What matters is ordering: + * register() hashes the CURRENT session ID into op_browser_state (used for + * OIDC Session Management). If it ran BEFORE Auth::login(), that hash would + * be computed from the id Auth::login() is about to invalidate, desyncing + * the check-session iframe contract for any relying party using it. + */ +#[\PHPUnit\Framework\Attributes\RunTestsInSeparateProcesses] +#[\PHPUnit\Framework\Attributes\PreserveGlobalState(false)] +final class AuthServiceLoginUserTest extends PHPUnitTestCase +{ + use MockeryPHPUnitIntegration; + + private AuthService $service; + + private $mock_principal_service; + + // Facade aliases + private $auth_mock; + + protected function setUp(): void + { + parent::setUp(); + + $mock_user_repository = $this->createMock(IUserRepository::class); + $mock_otp_repository = $this->createMock(IOAuth2OTPRepository::class); + $this->mock_principal_service = $this->createMock(IPrincipalService::class); + $mock_user_service = $this->createMock(IUserService::class); + $mock_user_action_service = $this->createMock(IUserActionService::class); + $mock_cache_service = $this->createMock(ICacheService::class); + $mock_auth_user_service = $this->createMock(IAuthUserService::class); + $mock_security_context_service = $this->createMock(ISecurityContextService::class); + $mock_tx_service = $this->createMock(ITransactionService::class); + + $this->auth_mock = Mockery::mock('alias:Illuminate\Support\Facades\Auth'); + + $log_mock = Mockery::mock('alias:Illuminate\Support\Facades\Log'); + $log_mock->shouldReceive('debug')->zeroOrMoreTimes(); + + $this->service = new AuthService( + $mock_user_repository, + $mock_otp_repository, + $this->mock_principal_service, + $mock_user_service, + $mock_user_action_service, + $mock_cache_service, + $mock_auth_user_service, + $mock_security_context_service, + $mock_tx_service + ); + } + + private function mockLoggableUser(): Mockery\MockInterface + { + $user = Mockery::mock('Auth\User'); + $user->shouldReceive('canLogin')->andReturn(true); + $user->shouldReceive('getId')->andReturn(42); + return $user; + } + + public function testLoginUserCallsAuthLoginBeforeRegisteringPrincipal(): void + { + $user = $this->mockLoggableUser(); + $call_order = []; + + $this->auth_mock->shouldReceive('login')->once()->andReturnUsing(function () use (&$call_order) { + $call_order[] = 'auth_login'; + }); + + $this->mock_principal_service->expects($this->once())->method('clear'); + $this->mock_principal_service->expects($this->once())->method('register')->willReturnCallback( + function () use (&$call_order) { + $call_order[] = 'principal_register'; + } + ); + + $this->service->loginUser($user, false); + + $this->assertSame( + ['auth_login', 'principal_register'], + $call_order, + 'Auth::login() must run before register() computes op_browser_state from the session ID - ' . + 'Auth::login() regenerates the session ID internally, so register() must use the post-login id' + ); + } +} diff --git a/tests/TwoFactorLoginFlowTest.php b/tests/TwoFactorLoginFlowTest.php index 9275f19c..3a560a4b 100644 --- a/tests/TwoFactorLoginFlowTest.php +++ b/tests/TwoFactorLoginFlowTest.php @@ -28,6 +28,7 @@ use Illuminate\Support\Facades\Session; use App\libs\OAuth2\Repositories\IOAuth2OTPRepository; use LaravelDoctrine\ORM\Facades\EntityManager; +use Services\OAuth2\PrincipalService; use Strategies\ILoginStrategy; /** @@ -294,6 +295,35 @@ public function testExpiredMFASessionFails(): void $this->assertSame('mfa_session_expired', $payload['error_code']); } + // ------------------------------------------------------------------------- + // session security (SDS idp-mfa.md §9.3: session fixation) + // + // Session-fixation protection itself is already provided by Laravel's + // SessionGuard::login() (session->migrate(true), called via Auth::login() + // inside loginUser()) - not something this branch needs to add. What + // AuthServiceLoginUserTest and the test below actually cover is the + // ordering bug this investigation found: PrincipalService::register() + // must run AFTER Auth::login(), not before, or its op_browser_state hash + // is computed from a session ID Auth::login() is about to invalidate. + // ------------------------------------------------------------------------- + + public function testCompletedMFALoginKeepsOPBrowserStateConsistentWithSessionId(): void + { + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + $code = $this->latestOtpCode(self::ADMIN_EMAIL); + $this->verify($code); + + // PrincipalService::register() hashes the session ID into + // op_browser_state (OIDC Session Management). If it ran BEFORE + // Auth::login()'s internal session->migrate(true), this would be a + // hash of the id Auth::login() was about to invalidate instead. + $this->assertSame( + hash('sha256', Session::getId()), + Session::get(PrincipalService::OPBrowserState), + 'op_browser_state must be derived from the post-regeneration session ID' + ); + } + // ------------------------------------------------------------------------- // trusted device // ------------------------------------------------------------------------- From e84925e0dfb766961c1f4688b9bb99a53fea52eb Mon Sep 17 00:00:00 2001 From: smarcet Date: Thu, 23 Jul 2026 02:07:28 -0300 Subject: [PATCH 10/14] Add test proving OTP redeem rolls back on mid-transaction failure Ticket CU-86ba2zc6p's TESTS list requires: "OTP redeem is persisted only on commit; a failure inside the verify transaction rolls back the redeem." No such test existed anywhere in this branch or PR #142/#146 - the two closest existing tests (testOTPCodeRejectsReuseAfterSuccessfulVerification, testRecoveryCodeRejectsReuseAfterTransactionCommit) only prove the COMMIT path (a successful verification's redeem persists and blocks reuse), not that a FAILED verification's partial redeem rolls back. Pure test-coverage gap, no production fix needed - AuthService::verifyMFAChallenge() already wraps strategy->verifyChallenge() in tx_service->transaction(), and DoctrineTransactionService already rolls back and re-throws on failure. Confirmed the test has teeth: temporarily bypassing the transaction wrapper broke the pessimistic-lock acquisition inside verifyChallenge() (which requires an open transaction), proving the test environment genuinely depends on transactional context, not just coincidentally passing. testOTPRedeemRollsBackOnMidTransactionFailure wraps the real EmailOTPMFAChallengeStrategy in a test double that lets the genuine redeem happen, then throws immediately after - inside the same transaction. Asserts the OTP is refetched from the DB (post-rollback) still unredeemed. --- tests/TwoFactorLoginFlowTest.php | 75 ++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/tests/TwoFactorLoginFlowTest.php b/tests/TwoFactorLoginFlowTest.php index 3a560a4b..4f96983f 100644 --- a/tests/TwoFactorLoginFlowTest.php +++ b/tests/TwoFactorLoginFlowTest.php @@ -28,8 +28,12 @@ use Illuminate\Support\Facades\Session; use App\libs\OAuth2\Repositories\IOAuth2OTPRepository; use LaravelDoctrine\ORM\Facades\EntityManager; +use Models\OAuth2\Client; use Services\OAuth2\PrincipalService; use Strategies\ILoginStrategy; +use Strategies\MFA\IMFAChallengeStrategy; +use Strategies\MFA\MFAChallengeStrategyFactory; +use Utils\Services\IAuthService; /** * Integration tests for the MFA-gated password login flow wired into UserController. @@ -285,6 +289,77 @@ public function testRecoveryCodeRejectsReuseAfterTransactionCommit(): void 'recovery code reuse must be rejected because used_at was committed via the AuthService transaction'); } + public function testOTPRedeemRollsBackOnMidTransactionFailure(): void + { + // Ticket CU-86ba2zc6p TESTS list: "OTP redeem is persisted only on + // commit; a failure inside the verify transaction rolls back the + // redeem." Wraps the REAL strategy so the OTP genuinely gets redeemed + // mid-transaction, then injects a failure before the transaction + // (AuthService::verifyMFAChallenge) can commit. + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + $code = $this->latestOtpCode(self::ADMIN_EMAIL); + $admin = $this->user(self::ADMIN_EMAIL); + + $realStrategy = MFAChallengeStrategyFactory::create(User::MFAMethod_OTP); + $faultyStrategy = new class($realStrategy) implements IMFAChallengeStrategy { + public function __construct(private IMFAChallengeStrategy $inner) + { + } + + public function issueChallenge(User $user, ?Client $client, bool $remember): array + { + return $this->inner->issueChallenge($user, $client, $remember); + } + + public function verifyChallenge(User $user, string $code, ?Client $client = null): void + { + $this->inner->verifyChallenge($user, $code, $client); + throw new \RuntimeException('Simulated mid-transaction failure after redeem'); + } + + public function resendChallenge(User $user, ?Client $client, bool $remember): array + { + return $this->inner->resendChallenge($user, $client, $remember); + } + + public function getPendingState(): ?array + { + return $this->inner->getPendingState(); + } + + public function clearPendingState(): void + { + $this->inner->clearPendingState(); + } + + public function verifyRecoveryCode(User $user, string $code): void + { + $this->inner->verifyRecoveryCode($user, $code); + } + }; + + /** @var IAuthService $authService */ + $authService = App::make(IAuthService::class); + + try { + $authService->verifyMFAChallenge($admin, $faultyStrategy, $code); + $this->fail('Expected the simulated mid-transaction failure to propagate'); + } catch (\RuntimeException $ex) { + $this->assertSame('Simulated mid-transaction failure after redeem', $ex->getMessage()); + } + + EntityManager::clear(); + /** @var IOAuth2OTPRepository $otpRepo */ + $otpRepo = App::make(IOAuth2OTPRepository::class); + $otp = $otpRepo->getByValue($code); + + $this->assertNotNull($otp, 'the OTP row itself must still exist - only the redeem must roll back'); + $this->assertFalse( + $otp->isRedeemed(), + 'a failure inside the verify transaction must roll back the OTP redeem, not persist it' + ); + } + public function testExpiredMFASessionFails(): void { // No prior postLogin -> no pending state. From b673a67e5d9e68876ec005b7a5a3e2e6424a3b3c Mon Sep 17 00:00:00 2001 From: smarcet Date: Thu, 23 Jul 2026 02:25:25 -0300 Subject: [PATCH 11/14] Make verify2FARecovery audit logging best-effort MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit EventRecoveryUsed was logged unguarded after loginUser() and clearPendingState(), so an audit-sink failure at that point propagated to the outer catch(Exception) and returned a 500 to a user who was already authenticated with an already-burned recovery code — the account's last-resort login path. Mirrors the same best-effort try/catch already applied to verify2FA()'s EventChallengeSucceeded audit call. Adds testRecoveryAuditFailureDoesNotBlockLogin, the recovery-path analogue of testAuditFailureDoesNotBlockLogin, reproducing the 500 before the fix and asserting a 302 + established session after it. --- app/Http/Controllers/UserController.php | 19 +++++++++++------ tests/TwoFactorLoginFlowTest.php | 27 +++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 6 deletions(-) diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index a39de12e..25576422 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -831,12 +831,19 @@ public function verify2FARecovery() $strategy->clearPendingState(); $this->clearMFAUISessionState(); - $this->two_factor_audit_service->log( - $user, - TwoFactorAuditLog::EventRecoveryUsed, - TwoFactorAuditLog::MethodRecovery, - IPHelper::getUserIp() - ); + // Best-effort: the recovery code is already redeemed and the session + // established, so an audit-logging failure must not 500 the user + // (which would strand them after burning their last-resort code). + try { + $this->two_factor_audit_service->log( + $user, + TwoFactorAuditLog::EventRecoveryUsed, + TwoFactorAuditLog::MethodRecovery, + IPHelper::getUserIp() + ); + } catch (Exception $ex) { + Log::warning($ex); + } return $this->login_strategy->postLogin(); } catch (ValidationException $ex) { diff --git a/tests/TwoFactorLoginFlowTest.php b/tests/TwoFactorLoginFlowTest.php index 4f96983f..a1379f5c 100644 --- a/tests/TwoFactorLoginFlowTest.php +++ b/tests/TwoFactorLoginFlowTest.php @@ -465,6 +465,33 @@ public function testAuditFailureDoesNotBlockLogin(): void $this->assertTrue(Auth::check(), 'session must be established despite the audit failure'); } + 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(); + $this->createRecoveryCode($admin, $plain, false); + + $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::EventRecoveryUsed) { + throw new \Exception('audit sink unavailable'); + } + }); + $this->app->instance(ITwoFactorAuditService::class, $auditMock); + + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + + $response = $this->recovery($plain); + + $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 From 77146fe30b514d012e1eb0b245d984574165e826 Mon Sep 17 00:00:00 2001 From: smarcet Date: Thu, 23 Jul 2026 02:33:05 -0300 Subject: [PATCH 12/14] Add real concurrent-connection tests for OTP/recovery-code row locks testOTPCodeRejectsReuseAfterSuccessfulVerification and testRecoveryCodeRejectsReuseAfterTransactionCommit only prove sequential reuse is rejected after a transaction commits. Neither exercises the actual property refreshExclusiveLock() exists for: blocking a second, concurrent request from redeeming the same unredeemed OTP or recovery code while the first request's transaction still holds the row. Adds two tests that open a genuinely independent physical DB connection (verified via differing MySQL CONNECTION_ID()) and prove FOR UPDATE from that connection is blocked (lock wait timeout) while EmailOTPMFAChallengeStrategy/AbstractMFAChallengeStrategy's production refreshExclusiveLock() call holds the row. Verified the assertion is non-vacuous by temporarily disabling the lock call and confirming the test fails as expected, then restoring it. --- tests/TwoFactorLoginFlowTest.php | 102 +++++++++++++++++++++++++++++++ 1 file changed, 102 insertions(+) diff --git a/tests/TwoFactorLoginFlowTest.php b/tests/TwoFactorLoginFlowTest.php index a1379f5c..247803ab 100644 --- a/tests/TwoFactorLoginFlowTest.php +++ b/tests/TwoFactorLoginFlowTest.php @@ -24,9 +24,11 @@ use Illuminate\Support\Facades\Auth; use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\Config; +use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Hash; use Illuminate\Support\Facades\Session; use App\libs\OAuth2\Repositories\IOAuth2OTPRepository; +use Auth\Repositories\IUserRecoveryCodeRepository; use LaravelDoctrine\ORM\Facades\EntityManager; use Models\OAuth2\Client; use Services\OAuth2\PrincipalService; @@ -360,6 +362,106 @@ public function verifyRecoveryCode(User $user, string $code): void ); } + // ------------------------------------------------------------------------- + // concurrency: pessimistic-lock proof for OTP / recovery-code redemption + // + // refreshExclusiveLock() (EmailOTPMFAChallengeStrategy::verifyChallenge, + // AbstractMFAChallengeStrategy::verifyRecoveryCode) exists specifically to + // close a check-then-redeem TOCTOU race between two concurrent requests. + // testOTPCodeRejectsReuseAfterSuccessfulVerification / testRecoveryCodeRejects + // ReuseAfterTransactionCommit above only prove SEQUENTIAL reuse is rejected. + // These tests prove the row lock the production code acquires actually + // blocks a second, independent physical DB connection while held. + // ------------------------------------------------------------------------- + + public function testOTPRedeemRowLockBlocksConcurrentConnection(): void + { + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + $code = $this->latestOtpCode(self::ADMIN_EMAIL); + + /** @var IOAuth2OTPRepository $otpRepo */ + $otpRepo = App::make(IOAuth2OTPRepository::class); + $otp = $otpRepo->getByValue($code); + $this->assertNotNull($otp); + + $this->assertLockBlocksSecondConnection( + 'oauth2_otp', + $otp->getId(), + fn() => $otpRepo->refreshExclusiveLock($otp) + ); + } + + public function testRecoveryCodeRowLockBlocksConcurrentConnection(): void + { + $admin = $this->user(self::ADMIN_EMAIL); + $plain = 'RECOVERY-LOCK-' . uniqid(); + $codeId = $this->createRecoveryCode($admin, $plain, false); + + /** @var IUserRecoveryCodeRepository $recoveryRepo */ + $recoveryRepo = App::make(IUserRecoveryCodeRepository::class); + $recoveryCode = EntityManager::find(UserRecoveryCode::class, $codeId); + $this->assertNotNull($recoveryCode); + + $this->assertLockBlocksSecondConnection( + 'user_recovery_codes', + $codeId, + fn() => $recoveryRepo->refreshExclusiveLock($recoveryCode) + ); + } + + /** + * Proves that a PESSIMISTIC_WRITE lock acquired via $acquireLock (the same + * production method the MFA strategies call before redeeming) blocks a + * genuinely separate, concurrent physical DB connection from also locking + * that row - i.e. the fix actually closes the TOCTOU redemption race, not + * just rejects a sequential re-submission. + * + * $table is always an internal literal supplied by this test file, never + * external input, so interpolating it into the probe SQL below is safe. + */ + private function assertLockBlocksSecondConnection(string $table, int $id, \Closure $acquireLock): void + { + $primary = EntityManager::getConnection(); + $primary->beginTransaction(); + + try { + $acquireLock(); + + // Register a second connection under a distinct name so Laravel's + // DatabaseManager opens an independent physical connection instead + // of returning the already-cached primary one. + Config::set('database.connections.mfa_lock_test_secondary', Config::get('database.connections.openstackid')); + $secondary = DB::connection('mfa_lock_test_secondary'); + + $primaryConnId = (int) $primary->executeQuery('SELECT CONNECTION_ID()')->fetchOne(); + $secondaryConnId = (int) $secondary->selectOne('SELECT CONNECTION_ID() AS id')->id; + $this->assertNotSame( + $primaryConnId, + $secondaryConnId, + 'test requires two independent physical DB connections to prove real lock contention' + ); + + $secondary->statement('SET SESSION innodb_lock_wait_timeout = 1'); + $secondary->beginTransaction(); + + try { + $secondary->selectOne("SELECT id FROM {$table} WHERE id = ? FOR UPDATE", [$id]); + $this->fail('a second connection must not be able to lock a row already held by refreshExclusiveLock()'); + } catch (\Illuminate\Database\QueryException $ex) { + $this->assertStringContainsStringIgnoringCase( + 'lock wait timeout', + $ex->getMessage(), + 'the second connection must be blocked by the row lock, not fail for an unrelated reason' + ); + } finally { + $secondary->rollBack(); + DB::purge('mfa_lock_test_secondary'); + } + } finally { + $primary->rollBack(); + } + } + public function testExpiredMFASessionFails(): void { // No prior postLogin -> no pending state. From 1ad38d6a22f75d11868ddfa43ebd120768ddaa9c Mon Sep 17 00:00:00 2001 From: smarcet Date: Thu, 23 Jul 2026 11:12:18 -0300 Subject: [PATCH 13/14] Guard all MFA audit-log calls against Throwable, not just Exception Best-effort audit logging around the MFA flows only caught Exception, which misses Error subtypes (TypeError, ArgumentCountError, etc.). An Error escaping any of these would still turn a clean response into an uncaught 500 or, worse for the two failure-path calls, drop the error_code the rate-limit middleware keys its failure counter on (TwoFactorRateLimitMiddleware::isFailure() only sees the JSON body of whatever response actually gets returned). Applies the codebase's existing convention for this exact situation (see app/Audit/AuditLoggerFactory.php, TrackRequestMiddleware.php) to all 7 best-effort audit/device-trust sites in this controller: - postLogin(): initial challenge issuance audit log (was unguarded) - verify2FA(): failure-path audit log (was unguarded) - verify2FA(): queueDeviceTrustCookie() call (was catch(Exception)) - verify2FA(): success-path audit log (was catch(Exception)) - verify2FARecovery(): failure-path audit log (was unguarded) - verify2FARecovery(): success-path audit log (was catch(Exception)) - resend2FA(): challenge-reissue audit log (was unguarded) Verified: full Two Factor Authentication Test Suite (83 tests, 241 assertions) passes unchanged. Co-Authored-By: Claude --- app/Http/Controllers/UserController.php | 80 ++++++++++++++++--------- 1 file changed, 53 insertions(+), 27 deletions(-) diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index 25576422..6f57856d 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -512,12 +512,19 @@ public function postLogin() $payload = $this->auth_service->issueMFAChallenge($user, $strategy, $client, $remember); $this->two_factor_rate_limit_service->increment(ITwoFactorRateLimitService::ActionResend, $user->getId()); - $this->two_factor_audit_service->log( - $user, - TwoFactorAuditLog::EventChallengeIssued, - $method, - IPHelper::getUserIp() - ); + // Best-effort: the challenge was already issued and the OTP + // sent, so an audit-logging failure must not 500 the user + // out of the mfa_required response they need to proceed. + try { + $this->two_factor_audit_service->log( + $user, + TwoFactorAuditLog::EventChallengeIssued, + $method, + IPHelper::getUserIp() + ); + } catch (\Throwable $ex) { + Log::warning($ex); + } // Restore-on-refresh: a subsequent GET /login can rehydrate // the 2FA screen from session instead of dropping back to the @@ -729,12 +736,19 @@ public function verify2FA() // Re-fetch user: the tx wrapper closed/reset the EM on failure, detaching the entity. $userId = (int) $pending['user_id']; $user = $this->auth_service->getUserById($userId) ?? $user; - $this->two_factor_audit_service->log( - $user, - TwoFactorAuditLog::EventChallengeFailed, - $method, - IPHelper::getUserIp() - ); + // Best-effort: an audit-logging failure here must not turn a + // clean 401 into a 500 (which would also drop the error_code + // the rate-limit middleware keys its failure count on). + try { + $this->two_factor_audit_service->log( + $user, + TwoFactorAuditLog::EventChallengeFailed, + $method, + IPHelper::getUserIp() + ); + } catch (\Throwable $auditEx) { + Log::warning($auditEx); + } return Response::json(['error_code' => 'mfa_verification_failed'], HttpResponse::HTTP_UNAUTHORIZED); } @@ -748,7 +762,7 @@ public function verify2FA() // burned OTP). Log and continue; the device just isn't remembered. try { $this->queueDeviceTrustCookie($user); - } catch (Exception $ex) { + } catch (\Throwable $ex) { Log::warning($ex); } } @@ -763,7 +777,7 @@ public function verify2FA() $method, IPHelper::getUserIp() ); - } catch (Exception $ex) { + } catch (\Throwable $ex) { Log::warning($ex); } @@ -818,12 +832,17 @@ public function verify2FARecovery() // Re-fetch user: the tx wrapper closed/reset the EM on failure, detaching the entity. $userId = (int) $pending['user_id']; $user = $this->auth_service->getUserById($userId) ?? $user; - $this->two_factor_audit_service->log( - $user, - TwoFactorAuditLog::EventChallengeFailed, - TwoFactorAuditLog::MethodRecovery, - IPHelper::getUserIp() - ); + // Best-effort: see verify2FA() for rationale. + try { + $this->two_factor_audit_service->log( + $user, + TwoFactorAuditLog::EventChallengeFailed, + TwoFactorAuditLog::MethodRecovery, + IPHelper::getUserIp() + ); + } catch (\Throwable $auditEx) { + Log::warning($auditEx); + } return Response::json(['error_code' => 'mfa_invalid_recovery'], HttpResponse::HTTP_UNAUTHORIZED); } @@ -841,7 +860,7 @@ public function verify2FARecovery() TwoFactorAuditLog::MethodRecovery, IPHelper::getUserIp() ); - } catch (Exception $ex) { + } catch (\Throwable $ex) { Log::warning($ex); } @@ -899,12 +918,19 @@ public function resend2FA() Session::put('otp_lifetime', $payload['otp_lifetime']); } - $this->two_factor_audit_service->log( - $user, - TwoFactorAuditLog::EventChallengeIssued, - $method, - IPHelper::getUserIp() - ); + // Best-effort: the challenge was already re-issued and the OTP + // sent, so an audit-logging failure must not 500 the user out of + // the payload they need to complete verification. + try { + $this->two_factor_audit_service->log( + $user, + TwoFactorAuditLog::EventChallengeIssued, + $method, + IPHelper::getUserIp() + ); + } catch (\Throwable $ex) { + Log::warning($ex); + } return $this->ok($payload); } catch (ValidationException $ex) { From 349deb8e6bddf93e2d23e591c0f31b6246a89134 Mon Sep 17 00:00:00 2001 From: smarcet Date: Thu, 23 Jul 2026 12:45:05 -0300 Subject: [PATCH 14/14] Honor the global 2FA kill-switch in User::shouldRequire2FA() The passwordless-login guard called shouldRequire2FA() directly, which ignored config('two_factor.enabled'), so an enforced admin stayed blocked from passwordless login even with the kill-switch off (SDS idp-mfa.md rollout, section 10.1). Move the enabled check into shouldRequire2FA() as the single source of truth shared by both the MFA gate and the passwordless guard, and drop the now-redundant check in MFAGateService. --- app/Services/Auth/MFAGateService.php | 7 ++++--- app/libs/Auth/Models/User.php | 10 ++++++++-- tests/TwoFactorLoginFlowTest.php | 20 ++++++++++++++++++++ tests/Unit/MFAGateServiceTest.php | 20 ++++++-------------- tests/unit/UserTwoFactorTest.php | 16 ++++++++++++++++ 5 files changed, 54 insertions(+), 19 deletions(-) diff --git a/app/Services/Auth/MFAGateService.php b/app/Services/Auth/MFAGateService.php index b2acb62e..52b66260 100644 --- a/app/Services/Auth/MFAGateService.php +++ b/app/Services/Auth/MFAGateService.php @@ -30,9 +30,10 @@ public function __construct( public function requiresChallenge(User $user, ?string $cookieToken): bool { - if (!config('two_factor.enabled', true)) { - return false; - } + // shouldRequire2FA() is the single source of truth for enforcement, + // including the global kill-switch (config('two_factor.enabled'), + // SDS idp-mfa.md §10.1) - so the passwordless-login guard, which also + // calls shouldRequire2FA(), stays consistent with this gate. if (!$user->shouldRequire2FA()) { return false; } diff --git a/app/libs/Auth/Models/User.php b/app/libs/Auth/Models/User.php index bb8c5373..33f4ba50 100644 --- a/app/libs/Auth/Models/User.php +++ b/app/libs/Auth/Models/User.php @@ -2428,11 +2428,17 @@ public function setTwoFactorEnforcedAt(?\DateTime $at): void /** * Whether this user is required to complete 2FA to sign in. * - * A user is required when they belong to any of the groups listed in - * config('two_factor.enforced_groups'); otherwise the stored flag applies. + * The global kill-switch is honored first: when config('two_factor.enabled') + * is false the whole 2FA gate is inactive (SDS idp-mfa.md §10.1), so no user + * is required regardless of role or preference. Otherwise a user is required + * when they belong to any of the groups listed in + * config('two_factor.enforced_groups'); failing that, the stored flag applies. */ public function shouldRequire2FA(): bool { + if (!config('two_factor.enabled', true)) { + return false; + } $enforcedGroups = config('two_factor.enforced_groups', []); foreach ($enforcedGroups as $slug) { if($this->belongToGroup($slug)) { diff --git a/tests/TwoFactorLoginFlowTest.php b/tests/TwoFactorLoginFlowTest.php index 247803ab..5c8cc441 100644 --- a/tests/TwoFactorLoginFlowTest.php +++ b/tests/TwoFactorLoginFlowTest.php @@ -163,6 +163,26 @@ public function testNonEnforcedUserStillLogsInViaPasswordlessLogin(): void $this->assertTrue(Auth::check(), 'passwordless login must keep working unchanged for non-enforced users'); } + public function testEnforcedUserCanUsePasswordlessWhenTwoFactorGloballyDisabled(): void + { + // Kill-switch (SDS idp-mfa.md §10.1): with 2FA globally disabled, the + // passwordless-login enforcement block must NOT fire - an enforced admin + // can log in passwordless again, matching "revert to password-only login". + // Regression guard for the gap where the block called shouldRequire2FA() + // without honoring config('two_factor.enabled'). + Config::set('two_factor.enabled', false); + + $this->emitOTP(self::ADMIN_EMAIL); + $code = $this->latestOtpCode(self::ADMIN_EMAIL); + + $this->postLoginOTP(self::ADMIN_EMAIL, $code); + + $this->assertTrue( + Auth::check(), + 'with the kill-switch off, passwordless login must not be blocked for an enforced admin' + ); + } + // ------------------------------------------------------------------------- // cancelLogin // ------------------------------------------------------------------------- diff --git a/tests/Unit/MFAGateServiceTest.php b/tests/Unit/MFAGateServiceTest.php index a2bfb5df..e4d8ea36 100644 --- a/tests/Unit/MFAGateServiceTest.php +++ b/tests/Unit/MFAGateServiceTest.php @@ -18,7 +18,6 @@ use App\Services\Auth\IDeviceTrustService; use App\Services\Auth\MFAGateService; use Auth\User; -use Illuminate\Support\Facades\Config; use Mockery; use Tests\TestCase; use Mockery\MockInterface; @@ -113,18 +112,11 @@ public function testRequiresChallengePassesThroughEmptyStringCookieToDeviceTrust } // ------------------------------------------------------------------------- - // Global kill-switch (SDS idp-mfa.md §10.1): TWO_FACTOR_ENABLED=false must - // short-circuit the gate before any per-user or device-trust evaluation. + // Global kill-switch (SDS idp-mfa.md §10.1) is now enforced inside + // User::shouldRequire2FA() (single source of truth shared with the + // passwordless-login guard); its coverage lives in + // Tests\unit\UserTwoFactorTest::testShouldRequire2FA_returnsFalse_whenGloballyDisabled. + // The gate short-circuiting when shouldRequire2FA() is false is covered by + // testRequiresChallengeReturnsFalseWhenUserDoesNotRequire2FA above. // ------------------------------------------------------------------------- - - public function testRequiresChallengeReturnsFalseWhenTwoFactorGloballyDisabled(): void - { - Config::set('two_factor.enabled', false); - - $user = Mockery::mock(User::class); - $user->shouldNotReceive('shouldRequire2FA'); - $this->deviceTrustService->shouldNotReceive('isDeviceTrusted'); - - $this->assertFalse($this->service->requiresChallenge($user, null)); - } } diff --git a/tests/unit/UserTwoFactorTest.php b/tests/unit/UserTwoFactorTest.php index 6e32921d..2927ca0a 100644 --- a/tests/unit/UserTwoFactorTest.php +++ b/tests/unit/UserTwoFactorTest.php @@ -83,6 +83,22 @@ public function testShouldRequire2FA_adminUser(): void $this->assertTrue($user->shouldRequire2FA()); } + public function testShouldRequire2FA_returnsFalse_whenGloballyDisabled(): void + { + // Global kill-switch (SDS idp-mfa.md §10.1): with 2FA globally disabled, + // even an enforced admin must not require 2FA - the whole gate is inert, + // which is what lets TWO_FACTOR_ENABLED=false revert to password-only login. + Config::set('two_factor.enabled', false); + + $user = new User(); + $this->assignGroups($user, [$this->buildGroup(IGroupSlugs::SuperAdminGroup)]); + + $this->assertFalse( + $user->shouldRequire2FA(), + 'the global kill-switch must override enforced-group membership' + ); + } + public function testShouldRequire2FA_BelongsToAnEnforcedGroup(): void { config(['two_factor.enforced_groups' => []]);