From 45653379e219b522a3d197c754558e45fe911ff3 Mon Sep 17 00:00:00 2001 From: matiasperrone-exo Date: Wed, 29 Apr 2026 14:30:20 +0000 Subject: [PATCH 1/5] 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 | 117 ++++++-- app/libs/Utils/Services/IAuthService.php | 22 ++ ...viceValidateCredentialsIntegrationTest.php | 106 +++++++ .../AuthServiceValidateCredentialsTest.php | 273 ++++++++++++++++++ 4 files changed, 486 insertions(+), 32 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 928c5af8..975a752d 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; @@ -131,6 +130,17 @@ public function isUserLogged() return Auth::check(); } + /** + * @return User|null + */ + public function getCurrentUser(): ?User + { + $user = Auth::user(); + if ($user instanceof User) { + return $user; + } + } + /** * Finds the OTP by value/connection/username, logs the redeem attempt (TX-A), * then validates lifecycle / value / scope / audience (TX-B). @@ -140,13 +150,12 @@ public function isUserLogged() * @throws InvalidOTPException */ private function findAndValidateOTP( - string $otp_value, - string $user_name, - string $otp_conn, + string $otp_value, + string $user_name, + string $otp_conn, ?string $otp_required_scopes, ?Client $client - ): OAuth2OTP - { + ): OAuth2OTP { // 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) { @@ -189,8 +198,10 @@ private function findAndValidateOTP( 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."); } @@ -319,8 +330,7 @@ public function verifyOTPChallenge( OAuth2OTP $otpClaim, User $sessionUser, ?Client $client = null - ): OAuth2OTP - { + ): OAuth2OTP { Log::debug(sprintf( "AuthService::verifyOTPChallenge otp %s session user %s", $otpClaim->getValue(), @@ -384,11 +394,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"); @@ -397,7 +406,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 ( @@ -409,11 +418,51 @@ public function login(string $username, string $password, bool $remember_me): bo } /** - * @return User|null + * @param string $username + * @param string $password + * @return User + * @throws AuthenticationException */ - public function getCurrentUser(): ?User + public function validateCredentials(string $username, string $password): User { - return Auth::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); } /** @@ -618,7 +667,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; @@ -720,12 +770,15 @@ public function postLoginUserActions(int $user_id): void Log::debug(sprintf("AuthService::postLoginUserActions user %s", $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())); + throw new AuthenticationLockedUserLoginAttempt( + $user->getEmail(), + sprintf("User %s is locked.", $user->getEmail()) + ); } //update user fields @@ -736,4 +789,4 @@ public function postLoginUserActions(int $user_id): void }); } -} \ No newline at end of file +} diff --git a/app/libs/Utils/Services/IAuthService.php b/app/libs/Utils/Services/IAuthService.php index a8fc3dda..c3d26e62 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 63ac94a4c9ed4ba90199061ac48f5844eff2714e Mon Sep 17 00:00:00 2001 From: matiasperrone-exo Date: Wed, 29 Apr 2026 21:30:34 +0000 Subject: [PATCH 2/5] chore: lint file app/libs/Auth/AuthService.php --- app/libs/Auth/AuthService.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/libs/Auth/AuthService.php b/app/libs/Auth/AuthService.php index 975a752d..71387fc0 100644 --- a/app/libs/Auth/AuthService.php +++ b/app/libs/Auth/AuthService.php @@ -789,4 +789,4 @@ public function postLoginUserActions(int $user_id): void }); } -} +} \ No newline at end of file From a339a47f5216b50bb8b139a9a3ce270dd8114190 Mon Sep 17 00:00:00 2001 From: matiasperrone-exo Date: Tue, 5 May 2026 14:58:46 +0000 Subject: [PATCH 3/5] chore: Add PR's requested changes --- app/libs/Auth/AuthService.php | 38 ++--- .../AuthServiceValidateCredentialsTest.php | 135 +++--------------- 2 files changed, 35 insertions(+), 138 deletions(-) diff --git a/app/libs/Auth/AuthService.php b/app/libs/Auth/AuthService.php index 71387fc0..776b459b 100644 --- a/app/libs/Auth/AuthService.php +++ b/app/libs/Auth/AuthService.php @@ -98,6 +98,7 @@ 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 ( @@ -394,10 +395,11 @@ 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"); @@ -406,7 +408,7 @@ public function login(string $username, string $password, bool $remember_me): bo 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 ( @@ -420,35 +422,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; @@ -462,6 +448,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."); Auth::login($user, $remember); } 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 d4f4b255a55179f2bd54ebab6dd8338a64d1305c Mon Sep 17 00:00:00 2001 From: matiasperrone-exo Date: Thu, 7 May 2026 21:29:37 +0000 Subject: [PATCH 4/5] chore: Add PR's requested changes Add tests changes with suggestion --- app/libs/Auth/AuthService.php | 17 +++--- .../AuthServiceValidateCredentialsTest.php | 52 +++++++++++++++++++ 2 files changed, 63 insertions(+), 6 deletions(-) diff --git a/app/libs/Auth/AuthService.php b/app/libs/Auth/AuthService.php index 776b459b..4971929e 100644 --- a/app/libs/Auth/AuthService.php +++ b/app/libs/Auth/AuthService.php @@ -19,6 +19,7 @@ use App\Services\Auth\IUserService as IAuthUserService; use Auth\Exceptions\AuthenticationException; use Auth\Exceptions\AuthenticationLockedUserLoginAttempt; +use Auth\Exceptions\UnverifiedEmailMemberException; use Auth\Repositories\IUserRepository; use Exception; use Illuminate\Support\Facades\Auth; @@ -429,12 +430,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; 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); + } + } From a0c22ff6b0805afbb63eaabd7c796c293990b2c7 Mon Sep 17 00:00:00 2001 From: matiasperrone-exo Date: Thu, 21 May 2026 20:02:27 +0000 Subject: [PATCH 5/5] chore: Fix issues created on rebase --- app/libs/Auth/AuthService.php | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/app/libs/Auth/AuthService.php b/app/libs/Auth/AuthService.php index 4971929e..ae71663d 100644 --- a/app/libs/Auth/AuthService.php +++ b/app/libs/Auth/AuthService.php @@ -99,7 +99,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 ( @@ -137,10 +136,7 @@ public function isUserLogged() */ public function getCurrentUser(): ?User { - $user = Auth::user(); - if ($user instanceof User) { - return $user; - } + return Auth::user(); } /** @@ -435,13 +431,13 @@ public function validateCredentials(string $username, string $password): User * @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()); } + if (is_null($user) || !$user instanceof User || !$user->canLogin()) { + throw new AuthenticationException("We are sorry, your username or password does not match an existing record."); + } return $user; }