diff --git a/app/Models/OAuth2/OAuth2OTP.php b/app/Models/OAuth2/OAuth2OTP.php index db46b89a..83f8633c 100644 --- a/app/Models/OAuth2/OAuth2OTP.php +++ b/app/Models/OAuth2/OAuth2OTP.php @@ -498,5 +498,4 @@ public function getUserId() { return $this->user_id; } - } \ No newline at end of file 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 fe3b0a28..928c5af8 100644 --- a/app/libs/Auth/AuthService.php +++ b/app/libs/Auth/AuthService.php @@ -15,32 +15,33 @@ 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; use OAuth2\Models\IClient; +use OAuth2\OAuth2Protocol; 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,82 +132,37 @@ public function isUserLogged() } /** - * @return User|null - */ - public function getCurrentUser(): ?User - { - return Auth::user(); - } - - /** - * @param string $username - * @param string $password - * @param bool $remember_me - * @return bool + * 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 login(string $username, string $password, bool $remember_me): bool + private function findAndValidateOTP( + string $otp_value, + string $user_name, + string $otp_conn, + ?string $otp_required_scopes, + ?Client $client + ): OAuth2OTP { - Log::debug("AuthService::login"); + // 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) { - $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; - } - - /** - * @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->tx_service->transaction(function () use ($otpClaim, $client, $remember) { - - // 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 ); if (is_null($otp)) { - // otp no emitted - Log::warning - ( - sprintf - ( - "AuthService::loginWithOTP otp %s user %s grant not found", - $otpClaim->getValue(), - $otpClaim->getUserName() - ) - ); - + 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 +170,8 @@ public function loginWithOTP(OAuth2OTP $otpClaim, ?Client $client = null, bool $ return $otp; }); - return $this->tx_service->transaction(function () use ($otp, $otpClaim, $client, $remember) { + // 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."); @@ -224,87 +181,251 @@ 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)) || ($otp->hasClient() && !is_null($client) && $client->getClientId() != $otp->getClient()->getClientId())) { throw new AuthenticationException("Single-use code audience mismatch."); } - $user = $this->getUserByUsername($otp->getUserName()); - $new_user = is_null($user); - if ($new_user) { - // we need to create a new one ( auto register) + return $otp; + }); + } + + /** + * 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: + * 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->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->getId() !== $otp->getId()) + $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) { - Log::debug(sprintf("AuthService::loginWithOTP user %s does not exists ...", $otp->getUserName())); + $user = $this->getUserByUsername($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::loginWithOTP user %s cannot login ( is not active ).", $user->getId())); + if (!$user->canLogin()) { + 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->redeem(); + $this->finalizeRedemption($otp, $user, $client); - // revoke former ones - $grants2Revoke = $this->otp_repository->getByUserNameNotRedeemed - ( - $otpClaim->getUserName(), - $client - ); + Auth::login($user, $remember); + Log::debug(sprintf("AuthService::loginWithOTP user %s logged in.", $user->getId())); + return $otp; + }); + } + + /** + * Verifies an OTP against an already-authenticated session user (MFA primitive). + * + * 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, + User $sessionUser, + ?Client $client = null + ): OAuth2OTP + { + Log::debug(sprintf( + "AuthService::verifyOTPChallenge otp %s session user %s", + $otpClaim->getValue(), + $sessionUser->getId() + )); + + $otp = $this->findAndValidateOTP( + $otpClaim->getValue(), + $otpClaim->getUserName(), + $otpClaim->getConnection(), + $otpClaim->getScope(), + $client + ); - foreach ($grants2Revoke as $otp2Revoke){ - try { - Log::debug(sprintf("AuthService::loginWithOTP revoking otp %s ", $otp2Revoke->getValue())); - if ($otp2Revoke->getValue() !== $otpClaim->getValue()) - $otp2Revoke->redeem(); - } catch (Exception $ex) { - Log::warning($ex); - } + // 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 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."); } - Auth::login($user, $remember); + 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."); + } - Log::debug(sprintf("AuthService::loginWithOTP user %s logged in.", $user->getId())); + $this->finalizeRedemption($otp, $user, $client); + + Log::debug(sprintf("AuthService::verifyOTPChallenge session user %s verified.", $sessionUser->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 +439,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 +462,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 +491,6 @@ public function getUserAuthorizationResponse() return IAuthService::AuthorizationResponse_None; } - public function clearUserAuthorizationResponse() { if (Session::has("openid.authorization.response")) { @@ -370,6 +505,8 @@ public function setUserAuthorizationResponse($auth_response) Session::save(); } + // Authentication + /** * @param string $openid * @return User|null @@ -379,26 +516,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 +555,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 +657,7 @@ public function getLoggedRPs(): array $rps = $zlib->uncompress($rps); return explode('|', $rps); } - } - catch (Exception $ex){ + } catch (Exception $ex) { Log::warning($ex); } return []; @@ -595,12 +710,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 +718,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/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 diff --git a/app/libs/Utils/Services/IAuthService.php b/app/libs/Utils/Services/IAuthService.php index d2aebd03..a8fc3dda 100644 --- a/app/libs/Utils/Services/IAuthService.php +++ b/app/libs/Utils/Services/IAuthService.php @@ -155,4 +155,20 @@ public function invalidateSession(); public function postLoginUserActions(int $user_id):void; + /** + * 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..6a01043b --- /dev/null +++ b/tests/VerifyOTPChallengeTest.php @@ -0,0 +1,283 @@ +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'); + $otp->shouldReceive('isRedeemed')->andReturn(false); + + // finalizeRedemption mutations expected + $otp->shouldReceive('setAuthTime')->once(); + $otp->shouldReceive('setUserId')->once()->with(100); + $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); + } + + /** + * 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); + $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). 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 + { + $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); + return $otp; + } +}