chore: Add PR's requested changes

Add tests changes with suggestion

Author: matiasperrone-exo

app/libs/Auth/AuthService.php

@@ -1,4 +1,5 @@
-<?php namespace Auth;
+<?php
+namespace Auth;
 /**
  * Copyright 2016 OpenStack Foundation
  * Licensed under the Apache License, Version 2.0 (the "License");
@@ -17,6 +18,7 @@
 use App\Services\AbstractService;
 use Auth\Exceptions\AuthenticationException;
 use Auth\Exceptions\AuthenticationLockedUserLoginAttempt;
+use Auth\Exceptions\UnverifiedEmailMemberException;
 use Auth\Repositories\IUserRepository;
 use Illuminate\Support\Facades\Auth;
 use Illuminate\Support\Facades\Config;
@@ -100,17 +102,16 @@ final class AuthService extends AbstractService implements IAuthService
      */
     public function __construct
     (
-        IUserRepository         $user_repository,
-        IOAuth2OTPRepository    $otp_repository,
-        IPrincipalService       $principal_service,
-        IUserService            $user_service,
-        IUserActionService      $user_action_service,
-        ICacheService           $cache_service,
-        IAuthUserService        $auth_user_service,
+        IUserRepository $user_repository,
+        IOAuth2OTPRepository $otp_repository,
+        IPrincipalService $principal_service,
+        IUserService $user_service,
+        IUserActionService $user_action_service,
+        ICacheService $cache_service,
+        IAuthUserService $auth_user_service,
         ISecurityContextService $security_context_service,
-        ITransactionService     $tx_service
-    )
-    {
+        ITransactionService $tx_service
+    ) {
         parent::__construct($tx_service);
         $this->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

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