From aebc4ccca3abfc71825c87d923991b6b34d9ae47 Mon Sep 17 00:00:00 2001 From: matiasperrone-exo Date: Wed, 29 Apr 2026 14:30:20 +0000 Subject: [PATCH 1/4] feat: Add AuthService validateCredentials method - test: cover canLogin()=false branch in validateCredentials() unit tests - docs: document known double-query cost in validateCredentials() - fix: use consistent error message in validateCredentials() --- app/libs/Auth/AuthService.php | 62 +++- app/libs/Utils/Services/IAuthService.php | 22 ++ ...viceValidateCredentialsIntegrationTest.php | 106 +++++++ .../AuthServiceValidateCredentialsTest.php | 273 ++++++++++++++++++ 4 files changed, 457 insertions(+), 6 deletions(-) create mode 100644 tests/AuthServiceValidateCredentialsIntegrationTest.php create mode 100644 tests/unit/AuthServiceValidateCredentialsTest.php diff --git a/app/libs/Auth/AuthService.php b/app/libs/Auth/AuthService.php index fe3b0a28..7857c5e8 100644 --- a/app/libs/Auth/AuthService.php +++ b/app/libs/Auth/AuthService.php @@ -96,7 +96,6 @@ final class AuthService extends AbstractService implements IAuthService * @param IAuthUserService $auth_user_service * @param ISecurityContextService $security_context_service * @param ITransactionService $tx_service - * @params ISecurityContextService $security_context_service */ public function __construct ( @@ -135,7 +134,11 @@ public function isUserLogged() */ public function getCurrentUser(): ?User { - return Auth::user(); + $user = Auth::user(); + if ($user instanceof User) { + return $user; + } + return null; } /** @@ -149,11 +152,10 @@ 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 ( - "We are sorry, your username or password does not match an existing record." + "username or password does not match an existing record." ); } Log::debug("AuthService::login: clearing principal"); @@ -162,7 +164,7 @@ public function login(string $username, string $password, bool $remember_me): bo if(is_null($current_user) || !$current_user->canLogin()) throw new AuthenticationException ( - "We are sorry, your username or password does not match an existing record." + "username or password does not match an existing record." ); $this->principal_service->register ( @@ -173,6 +175,54 @@ public function login(string $username, string $password, bool $remember_me): bo return true; } + /** + * @param string $username + * @param string $password + * @return User + * @throws AuthenticationException + */ + public function validateCredentials(string $username, string $password): User + { + Log::debug("AuthService::validateCredentials"); + + // retrieveByCredentials swallows AuthenticationLockedUserLoginAttempt and returns null, + // so pre-check lock state here to surface a distinct message for locked accounts. + $existing = $this->user_repository->getByEmailOrName($username); + if (!is_null($existing) && !$existing->isActive()) { + throw new AuthenticationException( + sprintf("User %s is locked.", $username) + ); + } + + // Known cost: retrieveByCredentials() calls user_repository->getByEmailOrName() internally + // (CustomAuthProvider line ~122), duplicating the query above. Eliminating it would require + // either changing the provider API to accept a pre-fetched User, or moving + // LockUserCounterMeasure checkpoint logic out of the provider — both out of scope here. + $user = Auth::getProvider()->retrieveByCredentials([ + 'username' => $username, + 'password' => $password, + ]); + + if (is_null($user) || !$user instanceof User || !$user->canLogin()) { + throw new AuthenticationException( + "username or password does not match an existing record." + ); + } + + return $user; + } + + /** + * @param User $user + * @param bool $remember + * @return void + */ + public function loginUser(User $user, bool $remember): void + { + Log::debug("AuthService::loginUser"); + Auth::login($user, $remember); + } + /** * @param OAuth2OTP $otpClaim * @param Client|null $client @@ -264,7 +314,7 @@ public function loginWithOTP(OAuth2OTP $otpClaim, ?Client $client = null, bool $ if(!$user->canLogin()){ Log::warning(sprintf("AuthService::loginWithOTP 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."); + throw new AuthenticationException("username or password does not match an existing record."); } $otp->setAuthTime(time()); diff --git a/app/libs/Utils/Services/IAuthService.php b/app/libs/Utils/Services/IAuthService.php index d2aebd03..0d4fe437 100644 --- a/app/libs/Utils/Services/IAuthService.php +++ b/app/libs/Utils/Services/IAuthService.php @@ -57,6 +57,28 @@ public function getCurrentUser():?User; */ public function login(string $username, string $password, bool $remember_me): bool; + /** + * Validates the supplied credentials without establishing a session. + * Delegates to CustomAuthProvider::retrieveByCredentials() so security + * checkpoints (LockUserCounterMeasure, etc.) still fire on failure. + * + * @param string $username + * @param string $password + * @return User + * @throws AuthenticationException on invalid credentials, missing user, or locked account. + */ + public function validateCredentials(string $username, string $password): User; + + /** + * Establishes a Laravel session for an already-authenticated user. + * Used by the 2FA flow after the second factor is verified. + * + * @param User $user + * @param bool $remember + * @return void + */ + public function loginUser(User $user, bool $remember): void; + /** * @param OAuth2OTP $otpClaim * @param Client|null $client diff --git a/tests/AuthServiceValidateCredentialsIntegrationTest.php b/tests/AuthServiceValidateCredentialsIntegrationTest.php new file mode 100644 index 00000000..ab512a6b --- /dev/null +++ b/tests/AuthServiceValidateCredentialsIntegrationTest.php @@ -0,0 +1,106 @@ +auth_service = $this->app[UtilsServiceCatalog::AuthenticationService]; + } + + /** + * A failed validateCredentials() call must: + * - throw AuthenticationException, + * - NOT establish a session (Auth::check() stays false), + * - trigger LockUserCounterMeasure so the user's login_failed_attempt counter increments. + */ + public function testFailedAttempt_incrementsLoginFailedAttemptCounter(): void + { + $initial_attempts = $this->getLoginFailedAttempt(self::SEEDED_USERNAME); + $this->assertFalse(Auth::check(), 'precondition: no authenticated user'); + + $threw = false; + try { + $this->auth_service->validateCredentials(self::SEEDED_USERNAME, 'wrong-password'); + } catch (AuthenticationException $ex) { + $threw = true; + } + + $this->assertTrue($threw, 'Expected AuthenticationException on wrong password'); + $this->assertFalse(Auth::check(), 'No session should be established after a failed attempt'); + + $new_attempts = $this->getLoginFailedAttempt(self::SEEDED_USERNAME); + $this->assertSame( + $initial_attempts + 1, + $new_attempts, + 'login_failed_attempt counter must increment via LockUserCounterMeasure' + ); + } + + /** + * A successful validateCredentials() call must return the user without + * establishing a session — Auth::check() must remain false afterwards. + */ + public function testSuccessfulValidation_doesNotEstablishSession(): void + { + $this->assertFalse(Auth::check(), 'precondition: no authenticated user'); + + $user = $this->auth_service->validateCredentials( + self::SEEDED_USERNAME, + self::SEEDED_PASSWORD + ); + + $this->assertInstanceOf(User::class, $user); + $this->assertFalse( + Auth::check(), + 'validateCredentials() must NOT call Auth::login() on success' + ); + } + + private function getLoginFailedAttempt(string $username): int + { + // Clear Doctrine's identity map so we read fresh state from the DB, + // not a cached in-memory entity from a prior transaction. + EntityManager::clear(); + $repo = EntityManager::getRepository(User::class); + /** @var IUserRepository $repo */ + $user = $repo->getByEmailOrName($username); + $this->assertInstanceOf(User::class, $user, "Seeded user {$username} not found"); + return $user->getLoginFailedAttempt(); + } +} diff --git a/tests/unit/AuthServiceValidateCredentialsTest.php b/tests/unit/AuthServiceValidateCredentialsTest.php new file mode 100644 index 00000000..b6f18948 --- /dev/null +++ b/tests/unit/AuthServiceValidateCredentialsTest.php @@ -0,0 +1,273 @@ +mock_user_repository = $this->createMock(IUserRepository::class); + $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); + + $this->auth_mock = Mockery::mock('alias:Illuminate\Support\Facades\Auth'); + $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, + $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 + ); + } + + /** + * Valid credentials return the User WITHOUT establishing a session. + * Auth::login() and Auth::attempt() must NEVER be called. + */ + public function testValidCredentials_returnsUser_withoutEstablishingSession(): void + { + $username = 'jane.doe'; + $password = 'Str0ng!Pass'; + + $this->mock_user_repository + ->expects($this->once()) + ->method('getByEmailOrName') + ->with($username) + ->willReturn(null); + + $resolved_user = Mockery::mock('Auth\User'); + $resolved_user->shouldReceive('canLogin')->andReturn(true); + + $provider_mock = Mockery::mock('Illuminate\Contracts\Auth\UserProvider'); + $provider_mock->shouldReceive('retrieveByCredentials') + ->once() + ->with(['username' => $username, 'password' => $password]) + ->andReturn($resolved_user); + + $this->auth_mock->shouldReceive('getProvider')->once()->andReturn($provider_mock); + $this->auth_mock->shouldNotReceive('login'); + $this->auth_mock->shouldNotReceive('attempt'); + + $returned = $this->service->validateCredentials($username, $password); + + $this->assertSame($resolved_user, $returned); + } + + /** + * Invalid credentials (provider returns null) throw AuthenticationException + * and do NOT establish a session. + */ + public function testInvalidCredentials_throwsAuthenticationException(): void + { + $username = 'jane.doe'; + $password = 'wrong'; + + $this->mock_user_repository + ->expects($this->once()) + ->method('getByEmailOrName') + ->with($username) + ->willReturn(null); + + $provider_mock = Mockery::mock('Illuminate\Contracts\Auth\UserProvider'); + $provider_mock->shouldReceive('retrieveByCredentials') + ->once() + ->with(['username' => $username, 'password' => $password]) + ->andReturn(null); + + $this->auth_mock->shouldReceive('getProvider')->once()->andReturn($provider_mock); + $this->auth_mock->shouldNotReceive('login'); + $this->auth_mock->shouldNotReceive('attempt'); + + $this->expectException(AuthenticationException::class); + + $this->service->validateCredentials($username, $password); + } + + /** + * Provider returns a User whose canLogin() is false — must throw AuthenticationException. + * This guards against future providers or provider changes that bypass the internal canLogin() + * check inside CustomAuthProvider::retrieveByCredentials(). + */ + public function testProviderReturnsUserThatCannotLogin_throwsAuthenticationException(): void + { + $username = 'jane.doe'; + $password = 'Str0ng!Pass'; + + // Pre-check: user not found in repository, so the locked-account short-circuit is not taken. + $this->mock_user_repository + ->expects($this->once()) + ->method('getByEmailOrName') + ->with($username) + ->willReturn(null); + + // Provider returns a valid User instance, but canLogin() is false. + $non_loginable_user = Mockery::mock('Auth\User'); + $non_loginable_user->shouldReceive('canLogin')->andReturn(false); + + $provider_mock = Mockery::mock('Illuminate\Contracts\Auth\UserProvider'); + $provider_mock->shouldReceive('retrieveByCredentials') + ->once() + ->with(['username' => $username, 'password' => $password]) + ->andReturn($non_loginable_user); + + $this->auth_mock->shouldReceive('getProvider')->once()->andReturn($provider_mock); + $this->auth_mock->shouldNotReceive('login'); + $this->auth_mock->shouldNotReceive('attempt'); + + $this->expectException(AuthenticationException::class); + + $this->service->validateCredentials($username, $password); + } + + /** + * A user that exists but is inactive (locked) short-circuits the password check + * and throws AuthenticationException with a "is locked" message. + */ + public function testLockedAccount_throwsAuthenticationException_withLockedMessage(): void + { + $username = 'locked.user'; + + $locked_user = Mockery::mock('Auth\User'); + $locked_user->shouldReceive('isActive')->andReturn(false); + + $this->mock_user_repository + ->expects($this->once()) + ->method('getByEmailOrName') + ->with($username) + ->willReturn($locked_user); + + // Provider must NOT be consulted when the user is locked. + $this->auth_mock->shouldNotReceive('getProvider'); + $this->auth_mock->shouldNotReceive('login'); + $this->auth_mock->shouldNotReceive('attempt'); + + $this->expectException(AuthenticationException::class); + $this->expectExceptionMessageMatches('/is locked/i'); + + $this->service->validateCredentials($username, 'irrelevant'); + } + + /** + * When the existing user is active, the locked-path is not taken: + * the provider is consulted and the resolved User is returned. + */ + public function testActiveUser_doesNotTriggerLockedPath(): void + { + $username = 'jane.doe'; + $password = 'Str0ng!Pass'; + + $active_user = Mockery::mock('Auth\User'); + $active_user->shouldReceive('isActive')->andReturn(true); + + $this->mock_user_repository + ->expects($this->once()) + ->method('getByEmailOrName') + ->with($username) + ->willReturn($active_user); + + $resolved_user = Mockery::mock('Auth\User'); + $resolved_user->shouldReceive('canLogin')->andReturn(true); + + $provider_mock = Mockery::mock('Illuminate\Contracts\Auth\UserProvider'); + $provider_mock->shouldReceive('retrieveByCredentials') + ->once() + ->andReturn($resolved_user); + + $this->auth_mock->shouldReceive('getProvider')->once()->andReturn($provider_mock); + $this->auth_mock->shouldNotReceive('login'); + + $returned = $this->service->validateCredentials($username, $password); + + $this->assertSame($resolved_user, $returned); + } + + /** + * loginUser(user, true) delegates to Auth::login with the remember flag set. + */ + public function testLoginUser_callsAuthLogin_withRememberTrue(): void + { + $user = Mockery::mock('Auth\User'); + + $this->auth_mock + ->shouldReceive('login') + ->once() + ->with($user, true); + + $this->service->loginUser($user, true); + } + + /** + * loginUser(user, false) delegates to Auth::login with remember disabled. + */ + public function testLoginUser_callsAuthLogin_withRememberFalse(): void + { + $user = Mockery::mock('Auth\User'); + + $this->auth_mock + ->shouldReceive('login') + ->once() + ->with($user, false); + + $this->service->loginUser($user, false); + } +} From 9fc38a0975a694b3c1ee3547ee4b113d84f38160 Mon Sep 17 00:00:00 2001 From: matiasperrone-exo Date: Wed, 29 Apr 2026 21:30:34 +0000 Subject: [PATCH 2/4] chore: lint file app/libs/Auth/AuthService.php --- app/libs/Auth/AuthService.php | 69 ++++++++++++++++++----------------- 1 file changed, 36 insertions(+), 33 deletions(-) diff --git a/app/libs/Auth/AuthService.php b/app/libs/Auth/AuthService.php index 7857c5e8..0000e2b6 100644 --- a/app/libs/Auth/AuthService.php +++ b/app/libs/Auth/AuthService.php @@ -1,4 +1,5 @@ -user_repository = $user_repository; $this->principal_service = $principal_service; @@ -161,7 +161,7 @@ public function login(string $username, string $password, bool $remember_me): bo Log::debug("AuthService::login: clearing principal"); $this->principal_service->clear(); $current_user = $this->getCurrentUser(); - if(is_null($current_user) || !$current_user->canLogin()) + if (is_null($current_user) || !$current_user->canLogin()) throw new AuthenticationException ( "username or password does not match an existing record." @@ -278,11 +278,13 @@ public function loginWithOTP(OAuth2OTP $otpClaim, ?Client $client = null, bool $ throw new AuthenticationException("Single-use code mismatch."); } - if(!empty($otpClaim->getScope()) && !$otp->allowScope($otpClaim->getScope())) + if (!empty($otpClaim->getScope()) && !$otp->allowScope($otpClaim->getScope())) 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())) { + if ( + ($otp->hasClient() && is_null($client)) || + ($otp->hasClient() && !is_null($client) && $client->getClientId() != $otp->getClient()->getClientId()) + ) { throw new AuthenticationException("Single-use code audience mismatch."); } @@ -304,15 +306,14 @@ 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()){ + if (!$user->canLogin()) { Log::warning(sprintf("AuthService::loginWithOTP user %s cannot login ( is not active ).", $user->getId())); throw new AuthenticationException("username or password does not match an existing record."); } @@ -328,7 +329,7 @@ public function loginWithOTP(OAuth2OTP $otpClaim, ?Client $client = null, bool $ $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()) @@ -349,12 +350,12 @@ public function loginWithOTP(OAuth2OTP $otpClaim, ?Client $client = null, bool $ * @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 @@ -368,7 +369,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 @@ -488,8 +489,7 @@ 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; @@ -552,7 +552,8 @@ public function registerRPLogin(string $client_id): void $rps = $zlib->uncompress($rps); $rps .= '|'; } - if (is_null($rps)) $rps = ""; + if (is_null($rps)) + $rps = ""; if (!str_contains($rps, $client_id)) $rps .= $client_id; @@ -591,8 +592,7 @@ public function getLoggedRPs(): array $rps = $zlib->uncompress($rps); return explode('|', $rps); } - } - catch (Exception $ex){ + } catch (Exception $ex) { Log::warning($ex); } return []; @@ -659,14 +659,17 @@ 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)); - throw new AuthenticationLockedUserLoginAttempt($user->getEmail(), - sprintf("User %s is locked.", $user->getEmail())); + Log::warning(sprintf("AuthService::postLoginUserActions user %s is not active.", $user_id)); + throw new AuthenticationLockedUserLoginAttempt( + $user->getEmail(), + sprintf("User %s is locked.", $user->getEmail()) + ); } //update user fields From 70ba198eda13e3e711cb64866173062e2dcb47d0 Mon Sep 17 00:00:00 2001 From: matiasperrone-exo Date: Tue, 5 May 2026 14:58:46 +0000 Subject: [PATCH 3/4] chore: Add PR's requested changes --- app/libs/Auth/AuthService.php | 114 ++++++--------- .../AuthServiceValidateCredentialsTest.php | 135 +++--------------- 2 files changed, 69 insertions(+), 180 deletions(-) diff --git a/app/libs/Auth/AuthService.php b/app/libs/Auth/AuthService.php index 0000e2b6..38e665eb 100644 --- a/app/libs/Auth/AuthService.php +++ b/app/libs/Auth/AuthService.php @@ -1,5 +1,4 @@ -user_repository = $user_repository; $this->principal_service = $principal_service; @@ -134,11 +135,7 @@ public function isUserLogged() */ public function getCurrentUser(): ?User { - $user = Auth::user(); - if ($user instanceof User) { - return $user; - } - return null; + return Auth::user(); } /** @@ -152,19 +149,20 @@ 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 ( - "username or password does not match an existing record." + "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()) + if(is_null($current_user) || !$current_user->canLogin()) throw new AuthenticationException ( - "username or password does not match an existing record." + "We are sorry, your username or password does not match an existing record." ); $this->principal_service->register ( @@ -178,35 +176,19 @@ public function login(string $username, string $password, bool $remember_me): bo /** * @param string $username * @param string $password - * @return User + * @return User|null * @throws AuthenticationException */ public function validateCredentials(string $username, string $password): User { Log::debug("AuthService::validateCredentials"); - // retrieveByCredentials swallows AuthenticationLockedUserLoginAttempt and returns null, - // so pre-check lock state here to surface a distinct message for locked accounts. - $existing = $this->user_repository->getByEmailOrName($username); - if (!is_null($existing) && !$existing->isActive()) { - throw new AuthenticationException( - sprintf("User %s is locked.", $username) - ); - } - - // Known cost: retrieveByCredentials() calls user_repository->getByEmailOrName() internally - // (CustomAuthProvider line ~122), duplicating the query above. Eliminating it would require - // either changing the provider API to accept a pre-fetched User, or moving - // LockUserCounterMeasure checkpoint logic out of the provider — both out of scope here. - $user = Auth::getProvider()->retrieveByCredentials([ - 'username' => $username, - 'password' => $password, - ]); - - if (is_null($user) || !$user instanceof User || !$user->canLogin()) { - throw new AuthenticationException( - "username or password does not match an existing record." - ); + /** + * @var User|null $user + */ + $user = Auth::getProvider()->retrieveByCredentials(['username' => $username, 'password' => $password]); + if (!$user) { + throw new AuthenticationException(); } return $user; @@ -220,6 +202,7 @@ public function validateCredentials(string $username, string $password): User 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."); Auth::login($user, $remember); } @@ -278,13 +261,11 @@ public function loginWithOTP(OAuth2OTP $otpClaim, ?Client $client = null, bool $ throw new AuthenticationException("Single-use code mismatch."); } - if (!empty($otpClaim->getScope()) && !$otp->allowScope($otpClaim->getScope())) + if(!empty($otpClaim->getScope()) && !$otp->allowScope($otpClaim->getScope())) 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()) - ) { + if (($otp->hasClient() && is_null($client)) || + ($otp->hasClient() && !is_null($client) && $client->getClientId() != $otp->getClient()->getClientId())) { throw new AuthenticationException("Single-use code audience mismatch."); } @@ -306,16 +287,17 @@ 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()) { + if(!$user->canLogin()){ Log::warning(sprintf("AuthService::loginWithOTP user %s cannot login ( is not active ).", $user->getId())); - throw new AuthenticationException("username or password does not match an existing record."); + throw new AuthenticationException("We are sorry, your username or password does not match an existing record."); } $otp->setAuthTime(time()); @@ -329,7 +311,7 @@ public function loginWithOTP(OAuth2OTP $otpClaim, ?Client $client = null, bool $ $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()) @@ -350,12 +332,12 @@ public function loginWithOTP(OAuth2OTP $otpClaim, ?Client $client = null, bool $ * @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 @@ -369,7 +351,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 @@ -489,7 +471,8 @@ 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; @@ -552,8 +535,7 @@ public function registerRPLogin(string $client_id): void $rps = $zlib->uncompress($rps); $rps .= '|'; } - if (is_null($rps)) - $rps = ""; + if (is_null($rps)) $rps = ""; if (!str_contains($rps, $client_id)) $rps .= $client_id; @@ -592,7 +574,8 @@ public function getLoggedRPs(): array $rps = $zlib->uncompress($rps); return explode('|', $rps); } - } catch (Exception $ex) { + } + catch (Exception $ex){ Log::warning($ex); } return []; @@ -659,17 +642,14 @@ 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)); - throw new AuthenticationLockedUserLoginAttempt( - $user->getEmail(), - sprintf("User %s is locked.", $user->getEmail()) - ); + Log::warning(sprintf("AuthService::postLoginUserActions user %s is not active.", $user_id)); + throw new AuthenticationLockedUserLoginAttempt($user->getEmail(), + sprintf("User %s is locked.", $user->getEmail())); } //update user fields diff --git a/tests/unit/AuthServiceValidateCredentialsTest.php b/tests/unit/AuthServiceValidateCredentialsTest.php index b6f18948..bd79f57a 100644 --- a/tests/unit/AuthServiceValidateCredentialsTest.php +++ b/tests/unit/AuthServiceValidateCredentialsTest.php @@ -14,6 +14,7 @@ use App\libs\OAuth2\Repositories\IOAuth2OTPRepository; use Auth\AuthService; +use Auth\CustomAuthProvider; use Auth\Exceptions\AuthenticationException; use Auth\Repositories\IUserRepository; use Mockery; @@ -89,16 +90,9 @@ public function testValidCredentials_returnsUser_withoutEstablishingSession(): v $username = 'jane.doe'; $password = 'Str0ng!Pass'; - $this->mock_user_repository - ->expects($this->once()) - ->method('getByEmailOrName') - ->with($username) - ->willReturn(null); - $resolved_user = Mockery::mock('Auth\User'); - $resolved_user->shouldReceive('canLogin')->andReturn(true); - $provider_mock = Mockery::mock('Illuminate\Contracts\Auth\UserProvider'); + $provider_mock = Mockery::mock(CustomAuthProvider::class); $provider_mock->shouldReceive('retrieveByCredentials') ->once() ->with(['username' => $username, 'password' => $password]) @@ -122,13 +116,7 @@ public function testInvalidCredentials_throwsAuthenticationException(): void $username = 'jane.doe'; $password = 'wrong'; - $this->mock_user_repository - ->expects($this->once()) - ->method('getByEmailOrName') - ->with($username) - ->willReturn(null); - - $provider_mock = Mockery::mock('Illuminate\Contracts\Auth\UserProvider'); + $provider_mock = Mockery::mock(CustomAuthProvider::class); $provider_mock->shouldReceive('retrieveByCredentials') ->once() ->with(['username' => $username, 'password' => $password]) @@ -143,110 +131,13 @@ public function testInvalidCredentials_throwsAuthenticationException(): void $this->service->validateCredentials($username, $password); } - /** - * Provider returns a User whose canLogin() is false — must throw AuthenticationException. - * This guards against future providers or provider changes that bypass the internal canLogin() - * check inside CustomAuthProvider::retrieveByCredentials(). - */ - public function testProviderReturnsUserThatCannotLogin_throwsAuthenticationException(): void - { - $username = 'jane.doe'; - $password = 'Str0ng!Pass'; - - // Pre-check: user not found in repository, so the locked-account short-circuit is not taken. - $this->mock_user_repository - ->expects($this->once()) - ->method('getByEmailOrName') - ->with($username) - ->willReturn(null); - - // Provider returns a valid User instance, but canLogin() is false. - $non_loginable_user = Mockery::mock('Auth\User'); - $non_loginable_user->shouldReceive('canLogin')->andReturn(false); - - $provider_mock = Mockery::mock('Illuminate\Contracts\Auth\UserProvider'); - $provider_mock->shouldReceive('retrieveByCredentials') - ->once() - ->with(['username' => $username, 'password' => $password]) - ->andReturn($non_loginable_user); - - $this->auth_mock->shouldReceive('getProvider')->once()->andReturn($provider_mock); - $this->auth_mock->shouldNotReceive('login'); - $this->auth_mock->shouldNotReceive('attempt'); - - $this->expectException(AuthenticationException::class); - - $this->service->validateCredentials($username, $password); - } - - /** - * A user that exists but is inactive (locked) short-circuits the password check - * and throws AuthenticationException with a "is locked" message. - */ - public function testLockedAccount_throwsAuthenticationException_withLockedMessage(): void - { - $username = 'locked.user'; - - $locked_user = Mockery::mock('Auth\User'); - $locked_user->shouldReceive('isActive')->andReturn(false); - - $this->mock_user_repository - ->expects($this->once()) - ->method('getByEmailOrName') - ->with($username) - ->willReturn($locked_user); - - // Provider must NOT be consulted when the user is locked. - $this->auth_mock->shouldNotReceive('getProvider'); - $this->auth_mock->shouldNotReceive('login'); - $this->auth_mock->shouldNotReceive('attempt'); - - $this->expectException(AuthenticationException::class); - $this->expectExceptionMessageMatches('/is locked/i'); - - $this->service->validateCredentials($username, 'irrelevant'); - } - - /** - * When the existing user is active, the locked-path is not taken: - * the provider is consulted and the resolved User is returned. - */ - public function testActiveUser_doesNotTriggerLockedPath(): void - { - $username = 'jane.doe'; - $password = 'Str0ng!Pass'; - - $active_user = Mockery::mock('Auth\User'); - $active_user->shouldReceive('isActive')->andReturn(true); - - $this->mock_user_repository - ->expects($this->once()) - ->method('getByEmailOrName') - ->with($username) - ->willReturn($active_user); - - $resolved_user = Mockery::mock('Auth\User'); - $resolved_user->shouldReceive('canLogin')->andReturn(true); - - $provider_mock = Mockery::mock('Illuminate\Contracts\Auth\UserProvider'); - $provider_mock->shouldReceive('retrieveByCredentials') - ->once() - ->andReturn($resolved_user); - - $this->auth_mock->shouldReceive('getProvider')->once()->andReturn($provider_mock); - $this->auth_mock->shouldNotReceive('login'); - - $returned = $this->service->validateCredentials($username, $password); - - $this->assertSame($resolved_user, $returned); - } - /** * loginUser(user, true) delegates to Auth::login with the remember flag set. */ public function testLoginUser_callsAuthLogin_withRememberTrue(): void { $user = Mockery::mock('Auth\User'); + $user->shouldReceive('canLogin')->andReturn(true); $this->auth_mock ->shouldReceive('login') @@ -262,6 +153,7 @@ public function testLoginUser_callsAuthLogin_withRememberTrue(): void public function testLoginUser_callsAuthLogin_withRememberFalse(): void { $user = Mockery::mock('Auth\User'); + $user->shouldReceive('canLogin')->andReturn(true); $this->auth_mock ->shouldReceive('login') @@ -270,4 +162,21 @@ public function testLoginUser_callsAuthLogin_withRememberFalse(): void $this->service->loginUser($user, false); } + + /** + * loginUser(user, [true|false]) and isActive or canLogin false throws an Exception. + */ + public function testLoginUser_throwsException_whenIsNotActive(): void + { + $user = Mockery::mock('Auth\User'); + $user->shouldReceive('canLogin')->andReturn(false); + + $this->auth_mock->shouldNotReceive('login'); + + $this->expectException(AuthenticationException::class); + $this->expectExceptionMessageMatches('/User is not active or cannot login\./'); + + $this->service->loginUser($user, true); + } + } From 28541f5581d3596ec2f7e9a379caff5faac353e3 Mon Sep 17 00:00:00 2001 From: matiasperrone-exo Date: Thu, 7 May 2026 21:29:37 +0000 Subject: [PATCH 4/4] chore: Add PR's requested changes Add tests changes with suggestion --- app/libs/Auth/AuthService.php | 89 ++++++++++--------- .../AuthServiceValidateCredentialsTest.php | 52 +++++++++++ 2 files changed, 101 insertions(+), 40 deletions(-) diff --git a/app/libs/Auth/AuthService.php b/app/libs/Auth/AuthService.php index 38e665eb..b31a3d91 100644 --- a/app/libs/Auth/AuthService.php +++ b/app/libs/Auth/AuthService.php @@ -1,4 +1,5 @@ -user_repository = $user_repository; $this->principal_service = $principal_service; @@ -159,7 +160,7 @@ public function login(string $username, string $password, bool $remember_me): bo Log::debug("AuthService::login: clearing principal"); $this->principal_service->clear(); $current_user = $this->getCurrentUser(); - if(is_null($current_user) || !$current_user->canLogin()) + if (is_null($current_user) || !$current_user->canLogin()) throw new AuthenticationException ( "We are sorry, your username or password does not match an existing record." @@ -183,12 +184,16 @@ public function validateCredentials(string $username, string $password): User { Log::debug("AuthService::validateCredentials"); - /** - * @var User|null $user - */ - $user = Auth::getProvider()->retrieveByCredentials(['username' => $username, 'password' => $password]); - if (!$user) { - throw new AuthenticationException(); + try { + /** + * @var User|null $user + */ + $user = Auth::getProvider()->retrieveByCredentials(['username' => $username, 'password' => $password]); + if (!$user instanceof User || !$user->canLogin()) { + throw new AuthenticationException("We are sorry, your username or password does not match an existing record."); + } + } catch (UnverifiedEmailMemberException $ex) { + throw new AuthenticationException($ex->getMessage()); } return $user; @@ -202,7 +207,8 @@ public function validateCredentials(string $username, string $password): User 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."); + if (!$user->canLogin()) + throw new AuthenticationException("User is not active or cannot login."); Auth::login($user, $remember); } @@ -261,11 +267,13 @@ public function loginWithOTP(OAuth2OTP $otpClaim, ?Client $client = null, bool $ throw new AuthenticationException("Single-use code mismatch."); } - if(!empty($otpClaim->getScope()) && !$otp->allowScope($otpClaim->getScope())) + if (!empty($otpClaim->getScope()) && !$otp->allowScope($otpClaim->getScope())) 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())) { + if ( + ($otp->hasClient() && is_null($client)) || + ($otp->hasClient() && !is_null($client) && $client->getClientId() != $otp->getClient()->getClientId()) + ) { throw new AuthenticationException("Single-use code audience mismatch."); } @@ -287,15 +295,14 @@ 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()){ + if (!$user->canLogin()) { Log::warning(sprintf("AuthService::loginWithOTP 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."); } @@ -311,7 +318,7 @@ public function loginWithOTP(OAuth2OTP $otpClaim, ?Client $client = null, bool $ $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()) @@ -332,12 +339,12 @@ public function loginWithOTP(OAuth2OTP $otpClaim, ?Client $client = null, bool $ * @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 @@ -351,7 +358,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 @@ -471,8 +478,7 @@ 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; @@ -535,7 +541,8 @@ public function registerRPLogin(string $client_id): void $rps = $zlib->uncompress($rps); $rps .= '|'; } - if (is_null($rps)) $rps = ""; + if (is_null($rps)) + $rps = ""; if (!str_contains($rps, $client_id)) $rps .= $client_id; @@ -574,8 +581,7 @@ public function getLoggedRPs(): array $rps = $zlib->uncompress($rps); return explode('|', $rps); } - } - catch (Exception $ex){ + } catch (Exception $ex) { Log::warning($ex); } return []; @@ -642,14 +648,17 @@ 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)); - throw new AuthenticationLockedUserLoginAttempt($user->getEmail(), - sprintf("User %s is locked.", $user->getEmail())); + Log::warning(sprintf("AuthService::postLoginUserActions user %s is not active.", $user_id)); + throw new AuthenticationLockedUserLoginAttempt( + $user->getEmail(), + sprintf("User %s is locked.", $user->getEmail()) + ); } //update user fields diff --git a/tests/unit/AuthServiceValidateCredentialsTest.php b/tests/unit/AuthServiceValidateCredentialsTest.php index bd79f57a..a504adc4 100644 --- a/tests/unit/AuthServiceValidateCredentialsTest.php +++ b/tests/unit/AuthServiceValidateCredentialsTest.php @@ -16,6 +16,7 @@ use Auth\AuthService; use Auth\CustomAuthProvider; use Auth\Exceptions\AuthenticationException; +use Auth\Exceptions\UnverifiedEmailMemberException; use Auth\Repositories\IUserRepository; use Mockery; use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration; @@ -91,6 +92,7 @@ public function testValidCredentials_returnsUser_withoutEstablishingSession(): v $password = 'Str0ng!Pass'; $resolved_user = Mockery::mock('Auth\User'); + $resolved_user->shouldReceive('canLogin')->once()->andReturn(true); $provider_mock = Mockery::mock(CustomAuthProvider::class); $provider_mock->shouldReceive('retrieveByCredentials') @@ -179,4 +181,54 @@ public function testLoginUser_throwsException_whenIsNotActive(): void $this->service->loginUser($user, true); } + /** + * UnverifiedEmailMemberException from the provider must be caught and + * re-thrown as AuthenticationException (contract: @throws AuthenticationException only). + */ + public function testUnverifiedUser_throwsAuthenticationException(): void + { + $username = 'unverified@example.com'; + $password = 'any'; + + $provider_mock = Mockery::mock(CustomAuthProvider::class); + $provider_mock->shouldReceive('retrieveByCredentials') + ->once() + ->with(['username' => $username, 'password' => $password]) + ->andThrow(new UnverifiedEmailMemberException('Email not verified.')); + + $this->auth_mock->shouldReceive('getProvider')->once()->andReturn($provider_mock); + $this->auth_mock->shouldNotReceive('login'); + + $this->expectException(AuthenticationException::class); + $this->expectExceptionMessage('Email not verified.'); + + $this->service->validateCredentials($username, $password); + } + + /** + * Provider returns a valid User but canLogin() is false (locked/inactive): + * must throw AuthenticationException — not silently return the user. + */ + public function testUserCannotLogin_throwsAuthenticationException(): void + { + $username = 'locked@example.com'; + $password = 'any'; + + $locked_user = Mockery::mock('Auth\User'); + $locked_user->shouldReceive('canLogin')->once()->andReturn(false); + + $provider_mock = Mockery::mock(CustomAuthProvider::class); + $provider_mock->shouldReceive('retrieveByCredentials') + ->once() + ->with(['username' => $username, 'password' => $password]) + ->andReturn($locked_user); + + $this->auth_mock->shouldReceive('getProvider')->once()->andReturn($provider_mock); + $this->auth_mock->shouldNotReceive('login'); + + $this->expectException(AuthenticationException::class); + + $this->service->validateCredentials($username, $password); + } + }