From 1abf069cb0c901e58ef2909c8855a5ffd54fc77b Mon Sep 17 00:00:00 2001 From: smarcet Date: Sat, 9 May 2026 19:31:53 -0300 Subject: [PATCH 1/6] chore: refactor OTP logic --- app/Models/OAuth2/OAuth2OTP.php | 14 ++ app/libs/Auth/AuthService.php | 290 +++++++++++++---------- app/libs/Utils/Services/IAuthService.php | 20 ++ 3 files changed, 202 insertions(+), 122 deletions(-) diff --git a/app/Models/OAuth2/OAuth2OTP.php b/app/Models/OAuth2/OAuth2OTP.php index db46b89a..b1213d2a 100644 --- a/app/Models/OAuth2/OAuth2OTP.php +++ b/app/Models/OAuth2/OAuth2OTP.php @@ -14,6 +14,7 @@ use App\libs\Utils\PunnyCodeHelper; use App\Models\Utils\BaseEntity; +use Auth\User; use Doctrine\ORM\Mapping AS ORM; use DateTime; use DateInterval; @@ -118,6 +119,12 @@ class OAuth2OTP extends BaseEntity implements Identifier #[ORM\ManyToOne(targetEntity: \Models\OAuth2\Client::class, inversedBy: 'otp_grants', cascade: ['persist'])] private $client; + /** + * @var User + * this is a transient state + */ + private $user; + /** * OAuth2OTP constructor. * @param int $length @@ -499,4 +506,11 @@ public function getUserId() return $this->user_id; } + public function setUser(User $user): void{ + $this->user = $user; + } + + public function getUser():?User{ + return $this->user; + } } \ No newline at end of file diff --git a/app/libs/Auth/AuthService.php b/app/libs/Auth/AuthService.php index fe3b0a28..63a5236f 100644 --- a/app/libs/Auth/AuthService.php +++ b/app/libs/Auth/AuthService.php @@ -15,14 +15,19 @@ use App\libs\OAuth2\Exceptions\ReloadSessionException; use App\libs\OAuth2\Repositories\IOAuth2OTPRepository; use App\Services\AbstractService; +use App\Services\Auth\IUserService as IAuthUserService; use Auth\Exceptions\AuthenticationException; use Auth\Exceptions\AuthenticationLockedUserLoginAttempt; use Auth\Repositories\IUserRepository; +use Exception; use Illuminate\Support\Facades\Auth; use Illuminate\Support\Facades\Config; -use Illuminate\Support\Facades\Crypt; use Illuminate\Support\Facades\Cookie; +use Illuminate\Support\Facades\Crypt; +use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Session; +use jwe\compression_algorithms\CompressionAlgorithms_Registry; +use jwe\compression_algorithms\CompressionAlgorithmsNames; use Models\OAuth2\Client; use Models\OAuth2\OAuth2OTP; use OAuth2\Exceptions\InvalidOTPException; @@ -30,17 +35,12 @@ use OAuth2\Services\IPrincipalService; use OAuth2\Services\ISecurityContextService; use OpenId\Services\IUserService; -use App\Services\Auth\IUserService as IAuthUserService; use Services\IUserActionService; use utils\Base64UrlRepresentation; use Utils\Db\ITransactionService; use Utils\IPHelper; use Utils\Services\IAuthService; use Utils\Services\ICacheService; -use jwe\compression_algorithms\CompressionAlgorithms_Registry; -use jwe\compression_algorithms\CompressionAlgorithmsNames; -use Exception; -use Illuminate\Support\Facades\Log; /** * Class AuthService @@ -131,67 +131,67 @@ public function isUserLogged() } /** - * @return User|null + * @param OAuth2OTP $otpClaim + * @param Client|null $client + * @param bool $remember + * @return OAuth2OTP|null + * @throws Exception */ - public function getCurrentUser(): ?User + public function loginWithOTP(OAuth2OTP $otpClaim, ?Client $client = null, bool $remember = false): ?OAuth2OTP { - return Auth::user(); - } - /** - * @param string $username - * @param string $password - * @param bool $remember_me - * @return bool - * @throws AuthenticationException - */ - public function login(string $username, string $password, bool $remember_me): bool - { - Log::debug("AuthService::login"); + Log::debug(sprintf("AuthService::loginWithOTP otp %s user %s", $otpClaim->getValue(), $otpClaim->getUserName())); - $this->last_login_error = ""; - if (!Auth::attempt(['username' => $username, 'password' => $password], $remember_me)) { - throw new AuthenticationException - ( - "We are sorry, your username or password does not match an existing record." - ); - } - Log::debug("AuthService::login: clearing principal"); - $this->principal_service->clear(); - $current_user = $this->getCurrentUser(); - if(is_null($current_user) || !$current_user->canLogin()) - throw new AuthenticationException - ( - "We are sorry, your username or password does not match an existing record." - ); - $this->principal_service->register - ( - $current_user->getId(), - time() + $otp = $this->verifyOTPChallenge( + $otpClaim->getValue(), + $otpClaim->getUserName(), + $otpClaim->getConnection(), + true, + $otpClaim->getScope(), + $client ); - return true; + $user = $otp->getUser(); + + if(is_null($user)){ + throw new AuthenticationException("Non existent user."); + } + + Auth::login($user, $remember); + + Log::debug(sprintf("AuthService::verifyOTPChallenge user %s logged in.", $user->getId())); + + return $otp; } /** - * @param OAuth2OTP $otpClaim + * @param string $otp_value + * @param string $user_name + * @param string $otp_conn + * @param bool $should_create_user + * @param string|null $otp_required_scopes * @param Client|null $client - * @param bool $remember * @return OAuth2OTP|null * @throws Exception */ - public function loginWithOTP(OAuth2OTP $otpClaim, ?Client $client = null, bool $remember = false): ?OAuth2OTP + public function verifyOTPChallenge + ( + string $otp_value, + string $user_name, + string $otp_conn, + bool $should_create_user = true, + ?string $otp_required_scopes = null, + ?Client $client = null + ): ?OAuth2OTP { - - Log::debug(sprintf("AuthService::loginWithOTP otp %s user %s", $otpClaim->getValue(), $otpClaim->getUserName())); - - $otp = $this->tx_service->transaction(function () use ($otpClaim, $client, $remember) { + Log::debug(sprintf("AuthService::verifyOTPChallenge otp_value %s user %s", $otp_value, $user_name)); + $otp = $this->tx_service->transaction(function () use ($otp_value, $otp_conn, $user_name, $client) { // find by value, connection and user name $otp = $this->otp_repository->getByValueConnectionAndUserName( - $otpClaim->getValue(), - $otpClaim->getConnection(), - $otpClaim->getUserName(), + $otp_value, + $otp_conn, + $user_name, $client ); @@ -201,9 +201,9 @@ public function loginWithOTP(OAuth2OTP $otpClaim, ?Client $client = null, bool $ ( sprintf ( - "AuthService::loginWithOTP otp %s user %s grant not found", - $otpClaim->getValue(), - $otpClaim->getUserName() + "AuthService::verifyOTPChallenge otp %s user %s grant not found", + $otp_value, + $user_name ) ); @@ -214,7 +214,7 @@ public function loginWithOTP(OAuth2OTP $otpClaim, ?Client $client = null, bool $ return $otp; }); - return $this->tx_service->transaction(function () use ($otp, $otpClaim, $client, $remember) { + return $this->tx_service->transaction(function () use ($otp, $otp_value, $otp_required_scopes, $user_name, $should_create_user, $client) { if (!$otp->isAlive()) { throw new AuthenticationException("Single-use code is expired."); @@ -224,11 +224,11 @@ public function loginWithOTP(OAuth2OTP $otpClaim, ?Client $client = null, bool $ throw new AuthenticationException("Single-use code is not valid."); } - if ($otp->getValue() != $otpClaim->getValue()) { + if ($otp->getValue() != $otp_value) { throw new AuthenticationException("Single-use code mismatch."); } - if(!empty($otpClaim->getScope()) && !$otp->allowScope($otpClaim->getScope())) + if (!empty($otp_required_scopes) && !$otp->allowScope($otp_required_scopes)) throw new InvalidOTPException("Single-use code requested scopes escalates former scopes."); if (($otp->hasClient() && is_null($client)) || @@ -239,9 +239,17 @@ public function loginWithOTP(OAuth2OTP $otpClaim, ?Client $client = null, bool $ $user = $this->getUserByUsername($otp->getUserName()); $new_user = is_null($user); if ($new_user) { + + if (!$should_create_user) { + Log::warning(sprintf( + "AuthService::verifyOTPChallenge user %s not found and auto-create disabled.", + $otp->getUserName() + )); + throw new AuthenticationException("We are sorry, your username or password does not match an existing record."); + } // we need to create a new one ( auto register) - Log::debug(sprintf("AuthService::loginWithOTP user %s does not exists ...", $otp->getUserName())); + Log::debug(sprintf("AuthService::verifyOTPChallenge user %s does not exists ...", $otp->getUserName())); $user = $this->auth_user_service->registerUser ( @@ -254,57 +262,107 @@ public function loginWithOTP(OAuth2OTP $otpClaim, ?Client $client = null, bool $ ], $otp ); - } - else{ - if($user->isActive()) { + } else { + if ($user->isActive()) { // verify email $user->verifyEmail(false); } } - if(!$user->canLogin()){ - Log::warning(sprintf("AuthService::loginWithOTP user %s cannot login ( is not active ).", $user->getId())); + if (!$user->canLogin()) { + Log::warning(sprintf("AuthService::verifyOTPChallenge user %s cannot login ( is not active ).", $user->getId())); throw new AuthenticationException("We are sorry, your username or password does not match an existing record."); } $otp->setAuthTime(time()); $otp->setUserId($user->getId()); + $otp->setUser($user); $otp->redeem(); // revoke former ones $grants2Revoke = $this->otp_repository->getByUserNameNotRedeemed ( - $otpClaim->getUserName(), + $user_name, $client ); - foreach ($grants2Revoke as $otp2Revoke){ + foreach ($grants2Revoke as $otp2Revoke) { try { - Log::debug(sprintf("AuthService::loginWithOTP revoking otp %s ", $otp2Revoke->getValue())); - if ($otp2Revoke->getValue() !== $otpClaim->getValue()) + Log::debug(sprintf("AuthService::verifyOTPChallenge revoking otp %s ", $otp2Revoke->getValue())); + if ($otp2Revoke->getValue() !== $otp_value) $otp2Revoke->redeem(); } catch (Exception $ex) { Log::warning($ex); } } - Auth::login($user, $remember); - - Log::debug(sprintf("AuthService::loginWithOTP user %s logged in.", $user->getId())); return $otp; }); + + } + + /** + * @param string $username + * @return null|User + */ + public function getUserByUsername(string $username): ?User + { + return $this->user_repository->getByEmailOrName($username); + } + + /** + * @param string $username + * @param string $password + * @param bool $remember_me + * @return bool + * @throws AuthenticationException + */ + public function login(string $username, string $password, bool $remember_me): bool + { + Log::debug("AuthService::login"); + + $this->last_login_error = ""; + if (!Auth::attempt(['username' => $username, 'password' => $password], $remember_me)) { + throw new AuthenticationException + ( + "We are sorry, your username or password does not match an existing record." + ); + } + Log::debug("AuthService::login: clearing principal"); + $this->principal_service->clear(); + $current_user = $this->getCurrentUser(); + if (is_null($current_user) || !$current_user->canLogin()) + throw new AuthenticationException + ( + "We are sorry, your username or password does not match an existing record." + ); + $this->principal_service->register + ( + $current_user->getId(), + time() + ); + + return true; + } + + /** + * @return User|null + */ + public function getCurrentUser(): ?User + { + return Auth::user(); } /** * @param bool $clear_security_ctx * @return void */ - public function logout(bool $clear_security_ctx = true):void + public function logout(bool $clear_security_ctx = true): void { Log::debug("AuthService::logout"); $current_user = $this->getCurrentUser(); // check if we have user on session - if(!is_null($current_user)) { + if (!is_null($current_user)) { $ip = IPHelper::getUserIp(); Log::debug(sprintf("AuthService::logout we have user %s from ip %s", $current_user->getId(), $ip)); $this->user_action_service->addUserAction @@ -318,7 +376,7 @@ public function logout(bool $clear_security_ctx = true):void // regular flow $this->invalidateSession(); $this->principal_service->clear(); - if($clear_security_ctx) + if ($clear_security_ctx) $this->security_context_service->clear(); Auth::logout(); // put in past @@ -341,6 +399,21 @@ public function logout(bool $clear_security_ctx = true):void Session::regenerate(); } + public function invalidateSession(): void + { + $session_id = Crypt::encrypt(Session::getId()); + $this->cache_service->addSingleValue($session_id . "invalid", $session_id); + } + + /** + * @param string $value + * @return String + */ + private function encrypt(string $value): string + { + return base64_encode(Crypt::encrypt($value)); + } + /** * @return string */ @@ -355,7 +428,6 @@ public function getUserAuthorizationResponse() return IAuthService::AuthorizationResponse_None; } - public function clearUserAuthorizationResponse() { if (Session::has("openid.authorization.response")) { @@ -370,6 +442,8 @@ public function setUserAuthorizationResponse($auth_response) Session::save(); } + // Authentication + /** * @param string $openid * @return User|null @@ -379,26 +453,6 @@ public function getUserByOpenId(string $openid): ?User return $this->user_repository->getByIdentifier($openid); } - /** - * @param string $username - * @return null|User - */ - public function getUserByUsername(string $username): ?User - { - return $this->user_repository->getByEmailOrName($username); - } - - /** - * @param int $id - * @return null|User - */ - public function getUserById(int $id): ?User - { - return $this->user_repository->getByIdWithGroups($id); - } - - // Authentication - public function getUserAuthenticationResponse() { if (Session::has("openstackid.authentication.response")) { @@ -438,44 +492,43 @@ public function unwrapUserId(string $user_id): string $unwrapped_name = $this->decrypt($user_id); $parts = explode(':', $unwrapped_name); return intval($parts[1]); - } - catch (Exception $ex){ + } catch (Exception $ex) { Log::warning($ex); } return $user_id; } /** - * @param int $user_id - * @param IClient $client - * @return string + * @param int $id + * @return null|User */ - public function wrapUserId(int $user_id, IClient $client): string + public function getUserById(int $id): ?User { - if ($client->getSubjectType() === IClient::SubjectType_Public) - return $user_id; - - $wrapped_name = sprintf('%s:%s', $client->getClientId(), $user_id); - return $this->encrypt($wrapped_name); + return $this->user_repository->getByIdWithGroups($id); } /** * @param string $value * @return String */ - private function encrypt(string $value): string + private function decrypt(string $value): string { - return base64_encode(Crypt::encrypt($value)); + $value = base64_decode($value); + return Crypt::decrypt($value); } /** - * @param string $value - * @return String + * @param int $user_id + * @param IClient $client + * @return string */ - private function decrypt(string $value): string + public function wrapUserId(int $user_id, IClient $client): string { - $value = base64_decode($value); - return Crypt::decrypt($value); + if ($client->getSubjectType() === IClient::SubjectType_Public) + return $user_id; + + $wrapped_name = sprintf('%s:%s', $client->getClientId(), $user_id); + return $this->encrypt($wrapped_name); } /** @@ -541,8 +594,7 @@ public function getLoggedRPs(): array $rps = $zlib->uncompress($rps); return explode('|', $rps); } - } - catch (Exception $ex){ + } catch (Exception $ex) { Log::warning($ex); } return []; @@ -595,12 +647,6 @@ public function generateJTI(string $client_id, int $id_token_lifetime): string return $jti; } - public function invalidateSession(): void - { - $session_id = Crypt::encrypt(Session::getId()); - $this->cache_service->addSingleValue($session_id . "invalid", $session_id); - } - /** * @param int $user_id * @return void @@ -609,12 +655,12 @@ public function invalidateSession(): void public function postLoginUserActions(int $user_id): void { Log::debug(sprintf("AuthService::postLoginUserActions user %s", $user_id)); - $this->tx_service->transaction(function () use($user_id){ + $this->tx_service->transaction(function () use ($user_id) { $user = $this->user_repository->getById($user_id); - if(!$user instanceof User) return; + if (!$user instanceof User) return; if (!$user->isActive()) { - Log::warning(sprintf("AuthService::postLoginUserActions user %s is not active.", $user_id)); + Log::warning(sprintf("AuthService::postLoginUserActions user %s is not active.", $user_id)); throw new AuthenticationLockedUserLoginAttempt($user->getEmail(), sprintf("User %s is locked.", $user->getEmail())); } diff --git a/app/libs/Utils/Services/IAuthService.php b/app/libs/Utils/Services/IAuthService.php index d2aebd03..54c4496f 100644 --- a/app/libs/Utils/Services/IAuthService.php +++ b/app/libs/Utils/Services/IAuthService.php @@ -155,4 +155,24 @@ public function invalidateSession(); public function postLoginUserActions(int $user_id):void; + /** + * @param string $otp_value + * @param string $user_name + * @param string $otp_conn + * @param bool $should_create_user + * @param string|null $otp_required_scopes + * @param Client|null $client + * @return OAuth2OTP|null + * @throws Exception + */ + public function verifyOTPChallenge + ( + string $otp_value, + string $user_name, + string $otp_conn, + bool $should_create_user = true, + ?string $otp_required_scopes = null, + ?Client $client = null + ): ?OAuth2OTP; + } \ No newline at end of file From 1cb21b427ce9f34e10d030c392e7b58f8b731b14 Mon Sep 17 00:00:00 2001 From: smarcet Date: Sun, 10 May 2026 07:53:08 -0300 Subject: [PATCH 2/6] refactor(auth): drop $should_create_user flag from verifyOTPChallenge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Split AuthService::verifyOTPChallenge into a strict primitive (no auto-register, no Auth::login) and refactor loginWithOTP to be a full implementation that wraps the same private helpers. Both public methods now take an OAuth2OTP $otpClaim — symmetric API. - Extract findAndValidateOTP private helper: TX-A find+logRedeemAttempt (committed independently to preserve brute-force counter), TX-B lifecycle/value/scope/audience checks. - Extract finalizeRedemption private helper: setAuthTime + setUserId + setUser + redeem + revoke siblings. Reads user_name from $otp->getUserName() — no extra parameter needed. - verifyOTPChallenge: strict primitive for MFA reuse. Throws AuthenticationException (not NPE) on missing user. Return type tightened to non-null OAuth2OTP. - loginWithOTP: signature byte-for-byte unchanged (?OAuth2OTP nullable). Body now does its own user resolution with the auto-register branch inline. - IAuthService updated to match impl signatures. Removes the boolean flag anti-pattern and the $top_required_scopes typo that was silently disabling the scope-escalation check. Plan: docs/plans/2026-05-09-remove-should-create-user-flag.md --- app/libs/Auth/AuthService.php | 224 ++++++++++++----------- app/libs/Utils/Services/IAuthService.php | 23 +-- 2 files changed, 125 insertions(+), 122 deletions(-) diff --git a/app/libs/Auth/AuthService.php b/app/libs/Auth/AuthService.php index 63a5236f..34ff9ca7 100644 --- a/app/libs/Auth/AuthService.php +++ b/app/libs/Auth/AuthService.php @@ -131,63 +131,24 @@ public function isUserLogged() } /** - * @param OAuth2OTP $otpClaim - * @param Client|null $client - * @param bool $remember - * @return OAuth2OTP|null - * @throws Exception - */ - public function loginWithOTP(OAuth2OTP $otpClaim, ?Client $client = null, bool $remember = false): ?OAuth2OTP - { - - Log::debug(sprintf("AuthService::loginWithOTP otp %s user %s", $otpClaim->getValue(), $otpClaim->getUserName())); - - $otp = $this->verifyOTPChallenge( - $otpClaim->getValue(), - $otpClaim->getUserName(), - $otpClaim->getConnection(), - true, - $otpClaim->getScope(), - $client - ); - - $user = $otp->getUser(); - - if(is_null($user)){ - throw new AuthenticationException("Non existent user."); - } - - Auth::login($user, $remember); - - Log::debug(sprintf("AuthService::verifyOTPChallenge user %s logged in.", $user->getId())); - - return $otp; - } - - /** - * @param string $otp_value - * @param string $user_name - * @param string $otp_conn - * @param bool $should_create_user - * @param string|null $otp_required_scopes - * @param Client|null $client - * @return OAuth2OTP|null - * @throws Exception + * Finds the OTP by value/connection/username, logs the redeem attempt (TX-A), + * then validates lifecycle / value / scope / audience (TX-B). + * TX-A is committed independently so the brute-force counter increments even when TX-B throws. + * + * @throws AuthenticationException + * @throws InvalidOTPException */ - public function verifyOTPChallenge - ( + private function findAndValidateOTP( string $otp_value, string $user_name, string $otp_conn, - bool $should_create_user = true, - ?string $otp_required_scopes = null, - ?Client $client = null - ): ?OAuth2OTP + ?string $otp_required_scopes, + ?Client $client + ): OAuth2OTP { - Log::debug(sprintf("AuthService::verifyOTPChallenge otp_value %s user %s", $otp_value, $user_name)); + // TX-A: find + log attempt (committed before any validation can throw) $otp = $this->tx_service->transaction(function () use ($otp_value, $otp_conn, $user_name, $client) { - // find by value, connection and user name $otp = $this->otp_repository->getByValueConnectionAndUserName( $otp_value, $otp_conn, @@ -196,17 +157,11 @@ public function verifyOTPChallenge ); if (is_null($otp)) { - // otp no emitted - Log::warning - ( - sprintf - ( - "AuthService::verifyOTPChallenge otp %s user %s grant not found", - $otp_value, - $user_name - ) - ); - + Log::warning(sprintf( + "AuthService::findAndValidateOTP otp %s user %s grant not found", + $otp_value, + $user_name + )); throw new AuthenticationException("Non existent single-use code."); } @@ -214,7 +169,8 @@ public function verifyOTPChallenge return $otp; }); - return $this->tx_service->transaction(function () use ($otp, $otp_value, $otp_required_scopes, $user_name, $should_create_user, $client) { + // TX-B: lifecycle / value / scope / audience checks + return $this->tx_service->transaction(function () use ($otp, $otp_value, $otp_required_scopes, $client) { if (!$otp->isAlive()) { throw new AuthenticationException("Single-use code is expired."); @@ -228,77 +184,135 @@ public function verifyOTPChallenge throw new AuthenticationException("Single-use code mismatch."); } - if (!empty($otp_required_scopes) && !$otp->allowScope($otp_required_scopes)) + if (!empty($otp_required_scopes) && !$otp->allowScope($otp_required_scopes)) { throw new InvalidOTPException("Single-use code requested scopes escalates former scopes."); + } if (($otp->hasClient() && is_null($client)) || ($otp->hasClient() && !is_null($client) && $client->getClientId() != $otp->getClient()->getClientId())) { throw new AuthenticationException("Single-use code audience mismatch."); } + return $otp; + }); + } + + /** + * Marks the OTP redeemed, attaches the user (transient), revokes sibling pending OTPs. + * Entity methods short-circuit for inline OTPs — no special-casing needed here. + */ + private function finalizeRedemption(OAuth2OTP $otp, User $user, ?Client $client): void + { + $otp->setAuthTime(time()); + $otp->setUserId($user->getId()); + $otp->setUser($user); + $otp->redeem(); + + $grants2Revoke = $this->otp_repository->getByUserNameNotRedeemed($otp->getUserName(), $client); + foreach ($grants2Revoke as $otp2Revoke) { + try { + Log::debug(sprintf("AuthService::finalizeRedemption revoking otp %s", $otp2Revoke->getValue())); + if ($otp2Revoke->getValue() !== $otp->getValue()) + $otp2Revoke->redeem(); + } catch (Exception $ex) { + Log::warning($ex); + } + } + } + + /** + * @param OAuth2OTP $otpClaim + * @param Client|null $client + * @param bool $remember + * @return OAuth2OTP|null + * @throws Exception + */ + public function loginWithOTP(OAuth2OTP $otpClaim, ?Client $client = null, bool $remember = false): ?OAuth2OTP + { + Log::debug(sprintf("AuthService::loginWithOTP otp %s user %s", $otpClaim->getValue(), $otpClaim->getUserName())); + + $otp = $this->findAndValidateOTP( + $otpClaim->getValue(), + $otpClaim->getUserName(), + $otpClaim->getConnection(), + $otpClaim->getScope(), + $client + ); + + // TX-C: resolve or create user, finalize, login + return $this->tx_service->transaction(function () use ($otp, $otpClaim, $client, $remember) { + $user = $this->getUserByUsername($otp->getUserName()); - $new_user = is_null($user); - if ($new_user) { - - if (!$should_create_user) { - Log::warning(sprintf( - "AuthService::verifyOTPChallenge user %s not found and auto-create disabled.", - $otp->getUserName() - )); - throw new AuthenticationException("We are sorry, your username or password does not match an existing record."); - } - // we need to create a new one ( auto register) - - Log::debug(sprintf("AuthService::verifyOTPChallenge user %s does not exists ...", $otp->getUserName())); - - $user = $this->auth_user_service->registerUser - ( + + if (is_null($user)) { + Log::debug(sprintf("AuthService::loginWithOTP user %s does not exist; auto-registering.", $otp->getUserName())); + $user = $this->auth_user_service->registerUser( [ 'email' => $otp->getUserName(), 'email_verified' => true, - // dont send email 'send_email_verified_notice' => false, 'active' => true, ], $otp ); - } else { - if ($user->isActive()) { - // verify email - $user->verifyEmail(false); - } + } else if ($user->isActive()) { + $user->verifyEmail(false); } if (!$user->canLogin()) { - Log::warning(sprintf("AuthService::verifyOTPChallenge user %s cannot login ( is not active ).", $user->getId())); + Log::warning(sprintf("AuthService::loginWithOTP user %s cannot login (not active).", $user->getId())); throw new AuthenticationException("We are sorry, your username or password does not match an existing record."); } - $otp->setAuthTime(time()); - $otp->setUserId($user->getId()); - $otp->setUser($user); - $otp->redeem(); + $this->finalizeRedemption($otp, $user, $client); - // revoke former ones - $grants2Revoke = $this->otp_repository->getByUserNameNotRedeemed - ( - $user_name, - $client - ); + Auth::login($user, $remember); + Log::debug(sprintf("AuthService::loginWithOTP user %s logged in.", $user->getId())); + return $otp; + }); + } + + /** + * Verifies an OTP against an EXISTING user. Marks the OTP redeemed. + * Does NOT auto-register users and does NOT call Auth::login. + * Intended as the strict primitive for MFA challenge verification. + * + * @param OAuth2OTP $otpClaim + * @param Client|null $client + * @return OAuth2OTP + * @throws AuthenticationException when the OTP is invalid or the user does not exist / cannot login. + * @throws InvalidOTPException when the OTP requested scopes escalate. + */ + public function verifyOTPChallenge(OAuth2OTP $otpClaim, ?Client $client = null): OAuth2OTP + { + Log::debug(sprintf("AuthService::verifyOTPChallenge otp %s user %s", $otpClaim->getValue(), $otpClaim->getUserName())); + + $otp = $this->findAndValidateOTP( + $otpClaim->getValue(), + $otpClaim->getUserName(), + $otpClaim->getConnection(), + $otpClaim->getScope(), + $client + ); + + // TX-C: resolve existing user, finalize + return $this->tx_service->transaction(function () use ($otp, $client) { - foreach ($grants2Revoke as $otp2Revoke) { - try { - Log::debug(sprintf("AuthService::verifyOTPChallenge revoking otp %s ", $otp2Revoke->getValue())); - if ($otp2Revoke->getValue() !== $otp_value) - $otp2Revoke->redeem(); - } catch (Exception $ex) { - Log::warning($ex); - } + $user = $this->getUserByUsername($otp->getUserName()); + + if (is_null($user) || !$user->canLogin()) { + Log::warning(sprintf( + "AuthService::verifyOTPChallenge user %s not found or cannot login.", + $otp->getUserName() + )); + throw new AuthenticationException("We are sorry, your username or password does not match an existing record."); } + $this->finalizeRedemption($otp, $user, $client); + + Log::debug(sprintf("AuthService::verifyOTPChallenge user %s verified.", $user->getId())); return $otp; }); - } /** diff --git a/app/libs/Utils/Services/IAuthService.php b/app/libs/Utils/Services/IAuthService.php index 54c4496f..cc5f775d 100644 --- a/app/libs/Utils/Services/IAuthService.php +++ b/app/libs/Utils/Services/IAuthService.php @@ -156,23 +156,12 @@ public function invalidateSession(); public function postLoginUserActions(int $user_id):void; /** - * @param string $otp_value - * @param string $user_name - * @param string $otp_conn - * @param bool $should_create_user - * @param string|null $otp_required_scopes + * @param OAuth2OTP $otpClaim * @param Client|null $client - * @return OAuth2OTP|null - * @throws Exception - */ - public function verifyOTPChallenge - ( - string $otp_value, - string $user_name, - string $otp_conn, - bool $should_create_user = true, - ?string $otp_required_scopes = null, - ?Client $client = null - ): ?OAuth2OTP; + * @return OAuth2OTP + * @throws AuthenticationException + * @throws \OAuth2\Exceptions\InvalidOTPException + */ + public function verifyOTPChallenge(OAuth2OTP $otpClaim, ?Client $client = null): OAuth2OTP; } \ No newline at end of file From 8392deaba45411f11aa8a85444f486a1d592bdbc Mon Sep 17 00:00:00 2001 From: smarcet Date: Sun, 10 May 2026 23:50:40 -0300 Subject: [PATCH 3/6] =?UTF-8?q?fix(auth):=20close=20validate=E2=86=92redee?= =?UTF-8?q?m=20race=20in=20OTP=20finalization?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Acquire a PESSIMISTIC_WRITE row lock and re-hydrate the OTP entity at the start of finalizeRedemption(). Concurrent submitters of the same OTP now serialize on the row; the loser sees redeemed_at populated and gets a clean AuthenticationException instead of silently double-redeeming or surfacing OAuth2OTP::redeem()'s ValidationException as a 500. The split TX-A/TX-B/TX-C structure already widened the pre-existing race window between isRedeemed() check and redeem() write. find() with PESSIMISTIC_WRITE alone is insufficient because tx_service reuses one EntityManager across all three transactions: the OTP loaded in TX-A stays in the UoW identity map, so find() returns the stale cached instance and only acquires the lock — no refresh. refresh() with PESSIMISTIC_WRITE does both in one round-trip (SELECT ... FOR UPDATE that also re-hydrates), which is what's needed here. Inline OTPs (OAuth2PasswordlessConnectionInline) are non-redeemable by design and are intentionally not locked. Also tighten the sibling-revoke comparison from getValue() (collision- prone if two OTPs ever share a value) to getId(). --- .../DoctrineOAuth2OTPRepository.php | 6 +++++ app/libs/Auth/AuthService.php | 24 ++++++++++++++++++- .../Repositories/IOAuth2OTPRepository.php | 8 +++++++ 3 files changed, 37 insertions(+), 1 deletion(-) diff --git a/app/Repositories/DoctrineOAuth2OTPRepository.php b/app/Repositories/DoctrineOAuth2OTPRepository.php index 5385897f..59adc881 100644 --- a/app/Repositories/DoctrineOAuth2OTPRepository.php +++ b/app/Repositories/DoctrineOAuth2OTPRepository.php @@ -138,4 +138,10 @@ public function getByValueConnectionAndUserName ->setMaxResults(1) ->getOneOrNullResult(); } + + public function refreshExclusiveLock(OAuth2OTP $otp): void + { + // Single round-trip: SELECT ... FOR UPDATE that also re-hydrates the entity. + $this->getEntityManager()->refresh($otp, \Doctrine\DBAL\LockMode::PESSIMISTIC_WRITE); + } } \ No newline at end of file diff --git a/app/libs/Auth/AuthService.php b/app/libs/Auth/AuthService.php index 34ff9ca7..6fcd887a 100644 --- a/app/libs/Auth/AuthService.php +++ b/app/libs/Auth/AuthService.php @@ -32,6 +32,7 @@ use Models\OAuth2\OAuth2OTP; use OAuth2\Exceptions\InvalidOTPException; use OAuth2\Models\IClient; +use OAuth2\OAuth2Protocol; use OAuth2\Services\IPrincipalService; use OAuth2\Services\ISecurityContextService; use OpenId\Services\IUserService; @@ -200,9 +201,30 @@ private function findAndValidateOTP( /** * Marks the OTP redeemed, attaches the user (transient), revokes sibling pending OTPs. * Entity methods short-circuit for inline OTPs — no special-casing needed here. + * + * Concurrency: acquires a PESSIMISTIC_WRITE row lock and re-hydrates state + * before checking redemption. This closes the validate→redeem race window: + * a second concurrent submitter blocks on the lock and, on resume, sees the + * row already redeemed and gets a clean AuthenticationException instead of + * silently double-redeeming. + * + * Inline OTPs are intentionally not locked — they are non-redeemable by + * design (see OAuth2OTP::redeem()) and there is no race to close. */ private function finalizeRedemption(OAuth2OTP $otp, User $user, ?Client $client): void { + if ($otp->getConnection() !== OAuth2Protocol::OAuth2PasswordlessConnectionInline) { + $this->otp_repository->refreshExclusiveLock($otp); + + if ($otp->isRedeemed()) { + Log::warning(sprintf( + "AuthService::finalizeRedemption otp %s already redeemed (concurrent submission).", + $otp->getValue() + )); + throw new AuthenticationException("Single-use code is already redeemed."); + } + } + $otp->setAuthTime(time()); $otp->setUserId($user->getId()); $otp->setUser($user); @@ -212,7 +234,7 @@ private function finalizeRedemption(OAuth2OTP $otp, User $user, ?Client $client) foreach ($grants2Revoke as $otp2Revoke) { try { Log::debug(sprintf("AuthService::finalizeRedemption revoking otp %s", $otp2Revoke->getValue())); - if ($otp2Revoke->getValue() !== $otp->getValue()) + if ($otp2Revoke->getId() !== $otp->getId()) $otp2Revoke->redeem(); } catch (Exception $ex) { Log::warning($ex); diff --git a/app/libs/OAuth2/Repositories/IOAuth2OTPRepository.php b/app/libs/OAuth2/Repositories/IOAuth2OTPRepository.php index 8f0de07e..e6dd5652 100644 --- a/app/libs/OAuth2/Repositories/IOAuth2OTPRepository.php +++ b/app/libs/OAuth2/Repositories/IOAuth2OTPRepository.php @@ -66,4 +66,12 @@ public function getByValueConnectionAndUserName string $user_name, ?Client $client = null ): ?OAuth2OTP; + + /** + * Acquires a PESSIMISTIC_WRITE row lock on the given OTP and re-hydrates + * its in-memory state from the database in the same round-trip. Required + * before reading redemption state during finalization, because the entity + * may be stale (loaded under an earlier transaction in the same UoW). + */ + public function refreshExclusiveLock(OAuth2OTP $otp): void; } \ No newline at end of file From a0406ff84da627162a0862cdb34f839a5b9806d3 Mon Sep 17 00:00:00 2001 From: smarcet Date: Mon, 11 May 2026 00:32:49 -0300 Subject: [PATCH 4/6] feat(auth): bind verifyOTPChallenge to the session user MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit verifyOTPChallenge previously took the user identity from the OTP claim itself ($otpClaim->getUserName()) and authenticated whichever account that resolved to. That is unsafe as an MFA primitive: an attacker who obtained a valid OTP for account B could submit it during MFA for an in-session user A and the call would succeed — classic second-factor mismatch bypass. Require the caller to pass the in-session User explicitly. After the OTP resolves to its rightful owner, compare ids; on mismatch, throw AuthenticationException BEFORE finalizeRedemption runs so the OTP is NOT consumed. The redeem-attempt counter (committed in TX-A by findAndValidateOTP) still increments, so brute-force tracking is preserved across rejected attempts. The signature change is a breaking interface change on IAuthService, but the method had zero callers in the codebase prior to this commit, so no production code needs updating. Test (tests/VerifyOTPChallengeTest.php) asserts the two behaviours: - Cross-account: OTP for user B + session user A → throws, AND neither refreshExclusiveLock nor getByUserNameNotRedeemed is called (proves finalizeRedemption was not reached, so the OTP was not consumed). - Same-account: OTP for user A + session user A → succeeds, lock is acquired, sibling sweep runs, and the OTP entity is returned. --- app/libs/Auth/AuthService.php | 55 ++++-- app/libs/Utils/Services/IAuthService.php | 21 ++- tests/VerifyOTPChallengeTest.php | 216 +++++++++++++++++++++++ 3 files changed, 271 insertions(+), 21 deletions(-) create mode 100644 tests/VerifyOTPChallengeTest.php diff --git a/app/libs/Auth/AuthService.php b/app/libs/Auth/AuthService.php index 6fcd887a..ae91508e 100644 --- a/app/libs/Auth/AuthService.php +++ b/app/libs/Auth/AuthService.php @@ -295,19 +295,37 @@ public function loginWithOTP(OAuth2OTP $otpClaim, ?Client $client = null, bool $ } /** - * Verifies an OTP against an EXISTING user. Marks the OTP redeemed. - * Does NOT auto-register users and does NOT call Auth::login. - * Intended as the strict primitive for MFA challenge verification. + * Verifies an OTP against an already-authenticated session user (MFA primitive). * - * @param OAuth2OTP $otpClaim - * @param Client|null $client - * @return OAuth2OTP - * @throws AuthenticationException when the OTP is invalid or the user does not exist / cannot login. - * @throws InvalidOTPException when the OTP requested scopes escalate. + * The OTP is resolved by its own claim (value + connection + user_name) but the + * redemption is only finalized if the resolved user matches $sessionUser. This + * binding is what makes the method safe to use as an MFA second factor: a stolen + * OTP for account B cannot satisfy MFA for an in-session user A. + * + * On user mismatch, the method throws BEFORE finalizeRedemption — the OTP is NOT + * consumed, so an attacker probing OTPs across accounts cannot burn down legitimate + * codes by guessing. The redeem-attempt counter (committed in TX-A) still increments, + * so brute-force tracking is preserved. + * + * Does NOT auto-register users and does NOT call Auth::login — the caller is + * responsible for the session state changes that follow a successful MFA. + * + * @throws AuthenticationException when the OTP is invalid, the resolved user is + * missing/inactive, or the OTP does not belong + * to $sessionUser. + * @throws InvalidOTPException when the OTP requested scopes escalate. */ - public function verifyOTPChallenge(OAuth2OTP $otpClaim, ?Client $client = null): OAuth2OTP + public function verifyOTPChallenge( + OAuth2OTP $otpClaim, + User $sessionUser, + ?Client $client = null + ): OAuth2OTP { - Log::debug(sprintf("AuthService::verifyOTPChallenge otp %s user %s", $otpClaim->getValue(), $otpClaim->getUserName())); + Log::debug(sprintf( + "AuthService::verifyOTPChallenge otp %s session user %s", + $otpClaim->getValue(), + $sessionUser->getId() + )); $otp = $this->findAndValidateOTP( $otpClaim->getValue(), @@ -317,22 +335,31 @@ public function verifyOTPChallenge(OAuth2OTP $otpClaim, ?Client $client = null): $client ); - // TX-C: resolve existing user, finalize - return $this->tx_service->transaction(function () use ($otp, $client) { + // TX-C: resolve OTP's user, enforce session-user binding, then finalize. + return $this->tx_service->transaction(function () use ($otp, $sessionUser, $client) { $user = $this->getUserByUsername($otp->getUserName()); if (is_null($user) || !$user->canLogin()) { Log::warning(sprintf( - "AuthService::verifyOTPChallenge user %s not found or cannot login.", + "AuthService::verifyOTPChallenge otp user %s not found or cannot login.", $otp->getUserName() )); throw new AuthenticationException("We are sorry, your username or password does not match an existing record."); } + if ($user->getId() !== $sessionUser->getId()) { + Log::warning(sprintf( + "AuthService::verifyOTPChallenge MFA mismatch: otp user %s != session user %s", + $user->getId(), + $sessionUser->getId() + )); + throw new AuthenticationException("Single-use code does not belong to the authenticated user."); + } + $this->finalizeRedemption($otp, $user, $client); - Log::debug(sprintf("AuthService::verifyOTPChallenge user %s verified.", $user->getId())); + Log::debug(sprintf("AuthService::verifyOTPChallenge session user %s verified.", $sessionUser->getId())); return $otp; }); } diff --git a/app/libs/Utils/Services/IAuthService.php b/app/libs/Utils/Services/IAuthService.php index cc5f775d..a8fc3dda 100644 --- a/app/libs/Utils/Services/IAuthService.php +++ b/app/libs/Utils/Services/IAuthService.php @@ -156,12 +156,19 @@ public function invalidateSession(); public function postLoginUserActions(int $user_id):void; /** - * @param OAuth2OTP $otpClaim - * @param Client|null $client - * @return OAuth2OTP - * @throws AuthenticationException - * @throws \OAuth2\Exceptions\InvalidOTPException - */ - public function verifyOTPChallenge(OAuth2OTP $otpClaim, ?Client $client = null): OAuth2OTP; + * Verifies an OTP against an already-authenticated session user (MFA primitive). + * Rejects the challenge if the OTP resolves to a different user than $sessionUser + * without consuming the OTP, so a stolen OTP for account B cannot satisfy MFA + * for an in-session user A. + * + * @throws AuthenticationException invalid OTP, missing/inactive user, + * or user mismatch with $sessionUser. + * @throws \OAuth2\Exceptions\InvalidOTPException requested scopes escalate. + */ + public function verifyOTPChallenge( + OAuth2OTP $otpClaim, + User $sessionUser, + ?Client $client = null + ): OAuth2OTP; } \ No newline at end of file diff --git a/tests/VerifyOTPChallengeTest.php b/tests/VerifyOTPChallengeTest.php new file mode 100644 index 00000000..115e3e0f --- /dev/null +++ b/tests/VerifyOTPChallengeTest.php @@ -0,0 +1,216 @@ +mock_user_repository = $this->createMock(IUserRepository::class); + $this->mock_otp_repository = $this->createMock(IOAuth2OTPRepository::class); + $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); + + // Execute each transaction closure in-process; this is the unit under test, + // not Doctrine. + $mock_tx_service + ->method('transaction') + ->willReturnCallback(fn(\Closure $cb) => $cb()); + + $this->log_mock = Mockery::mock('alias:Illuminate\Support\Facades\Log'); + $this->log_mock->shouldReceive('debug')->zeroOrMoreTimes(); + $this->log_mock->shouldReceive('warning')->zeroOrMoreTimes(); + + $this->service = new AuthService( + $this->mock_user_repository, + $this->mock_otp_repository, + $mock_principal_service, + $mock_user_service, + $mock_user_action_service, + $mock_cache_service, + $mock_auth_user_service, + $mock_security_context_service, + $mock_tx_service + ); + } + + /** + * Cross-account: an OTP minted for user B is submitted by a session + * authenticated as user A. The call MUST throw and MUST NOT consume the OTP. + * + * "Not consumed" is observed by asserting that finalizeRedemption never + * runs: neither the row lock (refreshExclusiveLock) nor the sibling-revoke + * lookup (getByUserNameNotRedeemed) is touched. + */ + public function testRejectsAndPreservesOTPWhenSessionUserDiffersFromOTPUser(): void + { + $otp = $this->buildValidOTPMock('user-b@example.com'); + + $this->mock_otp_repository + ->expects($this->once()) + ->method('getByValueConnectionAndUserName') + ->with('123456', 'email', 'user-b@example.com', null) + ->willReturn($otp); + + // OTP-resolved user (the rightful owner of the code) + $otp_owner = Mockery::mock(User::class); + $otp_owner->shouldReceive('getId')->andReturn(200); + $otp_owner->shouldReceive('canLogin')->andReturn(true); + + $this->mock_user_repository + ->expects($this->once()) + ->method('getByEmailOrName') + ->with('user-b@example.com') + ->willReturn($otp_owner); + + // Session-authenticated user — different id; this is the MFA bypass attempt. + $session_user = Mockery::mock(User::class); + $session_user->shouldReceive('getId')->andReturn(100); + + // ⚡ The contract: finalizeRedemption must NOT be reached. Both of its + // observable repository calls are forbidden. + $this->mock_otp_repository + ->expects($this->never()) + ->method('refreshExclusiveLock'); + + $this->mock_otp_repository + ->expects($this->never()) + ->method('getByUserNameNotRedeemed'); + + $this->expectException(AuthenticationException::class); + $this->expectExceptionMessage('Single-use code does not belong to the authenticated user.'); + + $this->service->verifyOTPChallenge($this->buildClaim('user-b@example.com'), $session_user); + } + + /** + * Same-account: OTP belongs to the session user. The call must reach + * finalizeRedemption and consume the OTP (refreshExclusiveLock acquires + * the row lock; redeem() runs; sibling revocation queries the repo). + */ + public function testSucceedsAndConsumesOTPWhenSessionUserMatchesOTPUser(): void + { + $otp = $this->buildValidOTPMock('user-a@example.com'); + + // finalizeRedemption mutations expected + $otp->shouldReceive('setAuthTime')->once(); + $otp->shouldReceive('setUserId')->once()->with(100); + $otp->shouldReceive('setUser')->once(); + $otp->shouldReceive('redeem')->once(); + $otp->shouldReceive('getId')->andReturn(7); + + $this->mock_otp_repository + ->expects($this->once()) + ->method('getByValueConnectionAndUserName') + ->willReturn($otp); + + $session_user = Mockery::mock(User::class); + $session_user->shouldReceive('getId')->andReturn(100); + $session_user->shouldReceive('canLogin')->andReturn(true); + + $this->mock_user_repository + ->expects($this->once()) + ->method('getByEmailOrName') + ->with('user-a@example.com') + ->willReturn($session_user); + + // finalizeRedemption MUST run: lock acquired, sibling sweep performed. + $this->mock_otp_repository + ->expects($this->once()) + ->method('refreshExclusiveLock') + ->with($otp); + + $this->mock_otp_repository + ->expects($this->once()) + ->method('getByUserNameNotRedeemed') + ->with('user-a@example.com', null) + ->willReturn([]); + + $result = $this->service->verifyOTPChallenge($this->buildClaim('user-a@example.com'), $session_user); + + $this->assertSame($otp, $result); + } + + private function buildClaim(string $user_name): OAuth2OTP + { + $claim = Mockery::mock(OAuth2OTP::class); + $claim->shouldReceive('getValue')->andReturn('123456'); + $claim->shouldReceive('getUserName')->andReturn($user_name); + $claim->shouldReceive('getConnection')->andReturn('email'); + $claim->shouldReceive('getScope')->andReturn(null); + return $claim; + } + + /** + * A resolved OTP that passes findAndValidateOTP (alive, valid, value matches, + * scope OK, audience OK) and is not already redeemed. + */ + private function buildValidOTPMock(string $user_name): Mockery\MockInterface + { + $otp = Mockery::mock(OAuth2OTP::class); + $otp->shouldReceive('getValue')->andReturn('123456'); + $otp->shouldReceive('getUserName')->andReturn($user_name); + $otp->shouldReceive('getConnection')->andReturn('email'); + $otp->shouldReceive('logRedeemAttempt')->zeroOrMoreTimes(); + $otp->shouldReceive('isAlive')->andReturn(true); + $otp->shouldReceive('isValid')->andReturn(true); + $otp->shouldReceive('allowScope')->andReturn(true); + $otp->shouldReceive('hasClient')->andReturn(false); + $otp->shouldReceive('isRedeemed')->andReturn(false); + return $otp; + } +} From c225a47281e16db0f274898bfe461f17d20ae05e Mon Sep 17 00:00:00 2001 From: smarcet Date: Mon, 11 May 2026 00:52:37 -0300 Subject: [PATCH 5/6] refactor(auth): drop unused OAuth2OTP transient $user field MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The $user property added in this PR's earlier commits had only one writer (AuthService::finalizeRedemption) and no readers anywhere in the codebase. Since the field is not ORM-mapped, a future caller that re-loaded the OTP from DB and called getUser() would silently receive null — a footgun for no current benefit. Remove the field, its setUser/getUser accessors, and the now-unused Auth\User import from OAuth2OTP. Drop the matching setUser call site in finalizeRedemption and the unused mock expectation in the unit test. YAGNI: re-add (and bind to a defined consumer) when an actual reader materialises. --- app/Models/OAuth2/OAuth2OTP.php | 15 --------------- app/libs/Auth/AuthService.php | 6 +++--- tests/VerifyOTPChallengeTest.php | 1 - 3 files changed, 3 insertions(+), 19 deletions(-) diff --git a/app/Models/OAuth2/OAuth2OTP.php b/app/Models/OAuth2/OAuth2OTP.php index b1213d2a..83f8633c 100644 --- a/app/Models/OAuth2/OAuth2OTP.php +++ b/app/Models/OAuth2/OAuth2OTP.php @@ -14,7 +14,6 @@ use App\libs\Utils\PunnyCodeHelper; use App\Models\Utils\BaseEntity; -use Auth\User; use Doctrine\ORM\Mapping AS ORM; use DateTime; use DateInterval; @@ -119,12 +118,6 @@ class OAuth2OTP extends BaseEntity implements Identifier #[ORM\ManyToOne(targetEntity: \Models\OAuth2\Client::class, inversedBy: 'otp_grants', cascade: ['persist'])] private $client; - /** - * @var User - * this is a transient state - */ - private $user; - /** * OAuth2OTP constructor. * @param int $length @@ -505,12 +498,4 @@ public function getUserId() { return $this->user_id; } - - public function setUser(User $user): void{ - $this->user = $user; - } - - public function getUser():?User{ - return $this->user; - } } \ No newline at end of file diff --git a/app/libs/Auth/AuthService.php b/app/libs/Auth/AuthService.php index ae91508e..928c5af8 100644 --- a/app/libs/Auth/AuthService.php +++ b/app/libs/Auth/AuthService.php @@ -199,8 +199,9 @@ private function findAndValidateOTP( } /** - * Marks the OTP redeemed, attaches the user (transient), revokes sibling pending OTPs. - * Entity methods short-circuit for inline OTPs — no special-casing needed here. + * Marks the OTP redeemed, stores the resolved user id, and revokes sibling + * pending OTPs. Entity methods short-circuit for inline OTPs — no special- + * casing needed here. * * Concurrency: acquires a PESSIMISTIC_WRITE row lock and re-hydrates state * before checking redemption. This closes the validate→redeem race window: @@ -227,7 +228,6 @@ private function finalizeRedemption(OAuth2OTP $otp, User $user, ?Client $client) $otp->setAuthTime(time()); $otp->setUserId($user->getId()); - $otp->setUser($user); $otp->redeem(); $grants2Revoke = $this->otp_repository->getByUserNameNotRedeemed($otp->getUserName(), $client); diff --git a/tests/VerifyOTPChallengeTest.php b/tests/VerifyOTPChallengeTest.php index 115e3e0f..5e8ff7db 100644 --- a/tests/VerifyOTPChallengeTest.php +++ b/tests/VerifyOTPChallengeTest.php @@ -149,7 +149,6 @@ public function testSucceedsAndConsumesOTPWhenSessionUserMatchesOTPUser(): void // finalizeRedemption mutations expected $otp->shouldReceive('setAuthTime')->once(); $otp->shouldReceive('setUserId')->once()->with(100); - $otp->shouldReceive('setUser')->once(); $otp->shouldReceive('redeem')->once(); $otp->shouldReceive('getId')->andReturn(7); From 94d04195f353120dceafff7b767957b8b3af98a9 Mon Sep 17 00:00:00 2001 From: smarcet Date: Mon, 11 May 2026 01:11:38 -0300 Subject: [PATCH 6/6] test(auth): cover already-redeemed race-fix branch in verifyOTPChallenge Adds a third unit test that exercises the race-fix path in finalizeRedemption: after refreshExclusiveLock acquires the lock and re-hydrates the entity, isRedeemed() returns true (modelling a concurrent winner having already committed the redemption between TX-B and TX-C). The test asserts: - AuthenticationException('Single-use code is already redeemed.') is thrown. - redeem() / setAuthTime() / setUserId() are never called on the OTP (no double-write of the redemption state). - getByUserNameNotRedeemed() is never queried (sibling-revoke sweep is skipped, since this caller is not the redeemer). The stale-then-fresh state is modelled by flipping a by-reference flag inside refreshExclusiveLock's callback and reading it from isRedeemed, which mirrors the production semantics (in-memory state was null until the lock+refresh re-loaded the DB row the winner had updated). Also pulls isRedeemed() out of the shared buildValidOTPMock helper so each test states its assumption explicitly (false for the happy path, true for the race-loser path). --- tests/VerifyOTPChallengeTest.php | 72 +++++++++++++++++++++++++++++++- 1 file changed, 70 insertions(+), 2 deletions(-) diff --git a/tests/VerifyOTPChallengeTest.php b/tests/VerifyOTPChallengeTest.php index 5e8ff7db..6a01043b 100644 --- a/tests/VerifyOTPChallengeTest.php +++ b/tests/VerifyOTPChallengeTest.php @@ -145,6 +145,7 @@ public function testRejectsAndPreservesOTPWhenSessionUserDiffersFromOTPUser(): v public function testSucceedsAndConsumesOTPWhenSessionUserMatchesOTPUser(): void { $otp = $this->buildValidOTPMock('user-a@example.com'); + $otp->shouldReceive('isRedeemed')->andReturn(false); // finalizeRedemption mutations expected $otp->shouldReceive('setAuthTime')->once(); @@ -184,6 +185,72 @@ public function testSucceedsAndConsumesOTPWhenSessionUserMatchesOTPUser(): void $this->assertSame($otp, $result); } + /** + * Race-fix path: a concurrent submitter of the same OTP has already + * redeemed the row between TX-B (validation) and TX-C (finalize). After + * refreshExclusiveLock acquires the lock and re-hydrates the entity, + * isRedeemed() returns true. The method MUST reject cleanly with + * AuthenticationException AND MUST NOT call redeem()/setAuthTime/ + * setUserId, and MUST NOT run the sibling-revoke sweep — otherwise the + * loser would double-write the redemption (the bug the race fix exists + * to prevent). + * + * "Already redeemed" is simulated by flipping isRedeemed() to true ONLY + * after refreshExclusiveLock fires, modelling what actually happens in + * production: the in-memory state was stale until the lock+refresh + * re-loaded fresh DB state showing the concurrent winner's commit. + */ + public function testRejectsAndDoesNotRedeemWhenLockedOTPWasRedeemedByConcurrentWinner(): void + { + $otp = $this->buildValidOTPMock('user-a@example.com'); + + // Models stale-then-fresh: before refresh the entity says "not redeemed", + // after refresh it reflects the DB row the concurrent winner committed. + // (Arrow fns capture by value — must use a long closure with `use (&…)`.) + $lock_acquired = false; + $otp->shouldReceive('isRedeemed')->andReturnUsing(function () use (&$lock_acquired) { + return $lock_acquired; + }); + + $this->mock_otp_repository + ->expects($this->once()) + ->method('getByValueConnectionAndUserName') + ->willReturn($otp); + + $session_user = Mockery::mock(User::class); + $session_user->shouldReceive('getId')->andReturn(100); + $session_user->shouldReceive('canLogin')->andReturn(true); + + $this->mock_user_repository + ->expects($this->once()) + ->method('getByEmailOrName') + ->with('user-a@example.com') + ->willReturn($session_user); + + $this->mock_otp_repository + ->expects($this->once()) + ->method('refreshExclusiveLock') + ->with($otp) + ->willReturnCallback(function () use (&$lock_acquired) { + $lock_acquired = true; + }); + + // ⚡ Contract: redemption side-effects MUST NOT run when the lock reveals + // the OTP was already redeemed. + $otp->shouldNotReceive('setAuthTime'); + $otp->shouldNotReceive('setUserId'); + $otp->shouldNotReceive('redeem'); + + $this->mock_otp_repository + ->expects($this->never()) + ->method('getByUserNameNotRedeemed'); + + $this->expectException(AuthenticationException::class); + $this->expectExceptionMessage('Single-use code is already redeemed.'); + + $this->service->verifyOTPChallenge($this->buildClaim('user-a@example.com'), $session_user); + } + private function buildClaim(string $user_name): OAuth2OTP { $claim = Mockery::mock(OAuth2OTP::class); @@ -196,7 +263,9 @@ private function buildClaim(string $user_name): OAuth2OTP /** * A resolved OTP that passes findAndValidateOTP (alive, valid, value matches, - * scope OK, audience OK) and is not already redeemed. + * scope OK, audience OK). isRedeemed() is intentionally left unconfigured — + * each test that reaches finalizeRedemption sets it explicitly (false for + * the happy path, true to simulate a concurrent winner having committed). */ private function buildValidOTPMock(string $user_name): Mockery\MockInterface { @@ -209,7 +278,6 @@ private function buildValidOTPMock(string $user_name): Mockery\MockInterface $otp->shouldReceive('isValid')->andReturn(true); $otp->shouldReceive('allowScope')->andReturn(true); $otp->shouldReceive('hasClient')->andReturn(false); - $otp->shouldReceive('isRedeemed')->andReturn(false); return $otp; } }