chore: Add PR's requested changes

Author: matiasperrone-exo

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);
     }
 

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);
+    }
+
 }