diff --git a/.phpstorm.meta.php b/.phpstorm.meta.php index 10cdc1ee..84b6687b 100644 --- a/.phpstorm.meta.php +++ b/.phpstorm.meta.php @@ -25,6 +25,5 @@ namespace PHPSTORM_META { override(\Psr\Container\ContainerInterface::get(0), type(0)); - override(\SP\Infrastructure\Database\QueryResult::getData(0), type(0)); override(\SP\Util\Util::unserialize(0), type(0)); } diff --git a/app/modules/web/Controllers/User/UserSaveBase.php b/app/modules/web/Controllers/User/UserSaveBase.php index 45f2a3a5..85d4faea 100644 --- a/app/modules/web/Controllers/User/UserSaveBase.php +++ b/app/modules/web/Controllers/User/UserSaveBase.php @@ -34,9 +34,9 @@ use SP\Domain\Core\Exceptions\QueryException; use SP\Domain\CustomField\Ports\CustomFieldDataService; use SP\Domain\Notification\Ports\MailService; use SP\Domain\User\Models\User; -use SP\Domain\User\Ports\UserPassRecoverServiceInterface; +use SP\Domain\User\Ports\UserPassRecoverService; use SP\Domain\User\Ports\UserServiceInterface; -use SP\Domain\User\Services\UserPassRecoverService; +use SP\Domain\User\Services\UserPassRecover; use SP\Modules\Web\Controllers\ControllerBase; use SP\Modules\Web\Forms\UserForm; use SP\Mvc\Controller\WebControllerHelper; @@ -49,8 +49,8 @@ abstract class UserSaveBase extends ControllerBase protected UserServiceInterface $userService; protected CustomFieldDataService $customFieldService; protected UserForm $form; - private MailService $mailService; - private UserPassRecoverServiceInterface $userPassRecoverService; + private MailService $mailService; + private UserPassRecoverService $userPassRecoverService; public function __construct( Application $application, @@ -58,7 +58,7 @@ abstract class UserSaveBase extends ControllerBase UserServiceInterface $userService, CustomFieldDataService $customFieldService, MailService $mailService, - UserPassRecoverServiceInterface $userPassRecoverService + UserPassRecoverService $userPassRecoverService ) { parent::__construct($application, $webControllerHelper); @@ -73,7 +73,7 @@ abstract class UserSaveBase extends ControllerBase /** * @param int $userId - * @param \SP\Domain\User\Models\User $userData + * @param User $userData * * @throws EnvironmentIsBrokenException * @throws Exception @@ -89,7 +89,7 @@ abstract class UserSaveBase extends ControllerBase $this->mailService->send( __('Password Change'), $userData->getEmail(), - UserPassRecoverService::getMailMessage($hash) + UserPassRecover::getMailMessage($hash) ); } } diff --git a/app/modules/web/Controllers/UserPassReset/SaveRequestController.php b/app/modules/web/Controllers/UserPassReset/SaveRequestController.php index 8fa19265..e5f58ac0 100644 --- a/app/modules/web/Controllers/UserPassReset/SaveRequestController.php +++ b/app/modules/web/Controllers/UserPassReset/SaveRequestController.php @@ -4,7 +4,7 @@ * * @author nuxsmin * @link https://syspass.org - * @copyright 2012-2023, Rubén Domínguez nuxsmin@$syspass.org + * @copyright 2012-2024, Rubén Domínguez nuxsmin@$syspass.org * * This file is part of sysPass. * @@ -30,7 +30,7 @@ use JsonException; use SP\Core\Events\Event; use SP\Core\Events\EventMessage; use SP\Domain\Core\Exceptions\SPException; -use SP\Domain\User\Services\UserPassRecoverService; +use SP\Domain\User\Services\UserPassRecover; use SP\Http\JsonMessage; use SP\Modules\Web\Controllers\Traits\JsonTrait; @@ -82,7 +82,7 @@ final class SaveRequestController extends UserPassResetSaveBase $this->mailService->send( __('Password Change'), $email, - UserPassRecoverService::getMailMessage($hash) + UserPassRecover::getMailMessage($hash) ); return $this->returnJsonResponse( diff --git a/app/modules/web/Controllers/UserPassReset/UserPassResetSaveBase.php b/app/modules/web/Controllers/UserPassReset/UserPassResetSaveBase.php index 4a20521a..ef90ccc8 100644 --- a/app/modules/web/Controllers/UserPassReset/UserPassResetSaveBase.php +++ b/app/modules/web/Controllers/UserPassReset/UserPassResetSaveBase.php @@ -34,7 +34,7 @@ use SP\Domain\Core\Exceptions\SPException; use SP\Domain\Notification\Ports\MailService; use SP\Domain\Security\Dtos\TrackRequest; use SP\Domain\Security\Ports\TrackService; -use SP\Domain\User\Ports\UserPassRecoverServiceInterface; +use SP\Domain\User\Ports\UserPassRecoverService; use SP\Domain\User\Ports\UserServiceInterface; use SP\Modules\Web\Controllers\ControllerBase; use SP\Mvc\Controller\WebControllerHelper; @@ -44,8 +44,8 @@ use SP\Mvc\Controller\WebControllerHelper; */ abstract class UserPassResetSaveBase extends ControllerBase { - protected UserPassRecoverServiceInterface $userPassRecoverService; - protected UserServiceInterface $userService; + protected UserPassRecoverService $userPassRecoverService; + protected UserServiceInterface $userService; protected MailService $mailService; private TrackService $trackService; private TrackRequest $trackRequest; @@ -56,12 +56,12 @@ abstract class UserPassResetSaveBase extends ControllerBase * @throws JsonException */ public function __construct( - Application $application, - WebControllerHelper $webControllerHelper, - UserPassRecoverServiceInterface $userPassRecoverService, - UserServiceInterface $userService, - MailService $mailService, - TrackService $trackService + Application $application, + WebControllerHelper $webControllerHelper, + UserPassRecoverService $userPassRecoverService, + UserServiceInterface $userService, + MailService $mailService, + TrackService $trackService ) { parent::__construct($application, $webControllerHelper); diff --git a/lib/SP/Domain/Auth/Services/Login.php b/lib/SP/Domain/Auth/Services/Login.php index 8d6e4d20..922a8396 100644 --- a/lib/SP/Domain/Auth/Services/Login.php +++ b/lib/SP/Domain/Auth/Services/Login.php @@ -46,7 +46,7 @@ use SP\Domain\Http\RequestInterface; use SP\Domain\Security\Dtos\TrackRequest; use SP\Domain\Security\Ports\TrackService; use SP\Domain\User\Models\UserPreferences; -use SP\Domain\User\Ports\UserPassRecoverServiceInterface; +use SP\Domain\User\Ports\UserPassRecoverService; use SP\Domain\User\Ports\UserPassServiceInterface; use SP\Domain\User\Ports\UserProfileServiceInterface; use SP\Domain\User\Ports\UserServiceInterface; @@ -86,16 +86,16 @@ final class Login extends Service implements LoginService * @throws InvalidArgumentException */ public function __construct( - Application $application, - private readonly AuthProviderInterface $authProvider, - private readonly LanguageInterface $language, - private readonly TrackService $trackService, - private readonly RequestInterface $request, - private readonly UserServiceInterface $userService, - private readonly UserPassRecoverServiceInterface $userPassRecoverService, - private readonly TemporaryMasterPassService $temporaryMasterPassService, - private readonly UserPassServiceInterface $userPassService, - private readonly UserProfileServiceInterface $userProfileService + Application $application, + private readonly AuthProviderInterface $authProvider, + private readonly LanguageInterface $language, + private readonly TrackService $trackService, + private readonly RequestInterface $request, + private readonly UserServiceInterface $userService, + private readonly UserPassRecoverService $userPassRecoverService, + private readonly TemporaryMasterPassService $temporaryMasterPassService, + private readonly UserPassServiceInterface $userPassService, + private readonly UserProfileServiceInterface $userProfileService ) { parent::__construct($application); diff --git a/lib/SP/Domain/User/Models/UserPassRecover.php b/lib/SP/Domain/User/Models/UserPassRecover.php index 57ee65cc..10a67944 100644 --- a/lib/SP/Domain/User/Models/UserPassRecover.php +++ b/lib/SP/Domain/User/Models/UserPassRecover.php @@ -33,10 +33,10 @@ class UserPassRecover extends Model { public const TABLE = 'UserPassRecover'; - public ?int $userId = null; - public ?string $hash = null; - public ?int $date = null; - public ?bool $used = null; + protected ?int $userId = null; + protected ?string $hash = null; + protected ?int $date = null; + protected ?bool $used = null; public function getUserId(): ?int { diff --git a/lib/SP/Domain/User/Ports/UserPassRecoverRepository.php b/lib/SP/Domain/User/Ports/UserPassRecoverRepository.php index 7c6286d4..04789810 100644 --- a/lib/SP/Domain/User/Ports/UserPassRecoverRepository.php +++ b/lib/SP/Domain/User/Ports/UserPassRecoverRepository.php @@ -67,7 +67,7 @@ interface UserPassRecoverRepository * @param string $hash * @param int $time * - * @return int + * @return int The updated rows. If no rows are updated, it means that the hash doesn't exist or it's expired * @throws SPException */ public function toggleUsedByHash(string $hash, int $time): int; @@ -78,7 +78,7 @@ interface UserPassRecoverRepository * @param string $hash * @param int $time * - * @return QueryResult + * @return QueryResult */ public function getUserIdForHash(string $hash, int $time): QueryResult; } diff --git a/lib/SP/Domain/User/Ports/UserPassRecoverServiceInterface.php b/lib/SP/Domain/User/Ports/UserPassRecoverService.php similarity index 82% rename from lib/SP/Domain/User/Ports/UserPassRecoverServiceInterface.php rename to lib/SP/Domain/User/Ports/UserPassRecoverService.php index 4a5dfb48..28874c2d 100644 --- a/lib/SP/Domain/User/Ports/UserPassRecoverServiceInterface.php +++ b/lib/SP/Domain/User/Ports/UserPassRecoverService.php @@ -4,7 +4,7 @@ * * @author nuxsmin * @link https://syspass.org - * @copyright 2012-2022, Rubén Domínguez nuxsmin@$syspass.org + * @copyright 2012-2024, Rubén Domínguez nuxsmin@$syspass.org * * This file is part of sysPass. * @@ -35,7 +35,7 @@ use SP\Domain\Core\Exceptions\SPException; * * @package SP\Domain\Common\Services\UserPassRecover */ -interface UserPassRecoverServiceInterface +interface UserPassRecoverService { /** * @throws SPException @@ -52,18 +52,10 @@ interface UserPassRecoverServiceInterface public function requestForUserId(int $id): string; /** - * Comprobar el límite de recuperaciones de clave. - * * @throws ConstraintException * @throws QueryException */ - public function checkAttemptsByUserId(int $userId): bool; - - /** - * @throws ConstraintException - * @throws QueryException - */ - public function add(int $userId, string $hash): bool; + public function add(int $userId, string $hash): void; /** * Comprobar el hash de recuperación de clave. diff --git a/lib/SP/Domain/User/Services/UserPassRecoverService.php b/lib/SP/Domain/User/Services/UserPassRecover.php similarity index 67% rename from lib/SP/Domain/User/Services/UserPassRecoverService.php rename to lib/SP/Domain/User/Services/UserPassRecover.php index f55bd973..9db3c21d 100644 --- a/lib/SP/Domain/User/Services/UserPassRecoverService.php +++ b/lib/SP/Domain/User/Services/UserPassRecover.php @@ -26,24 +26,25 @@ namespace SP\Domain\User\Services; use Defuse\Crypto\Exception\EnvironmentIsBrokenException; use SP\Core\Application; -use SP\Core\Bootstrap\BootstrapBase; use SP\Core\Messages\MailMessage; use SP\Domain\Common\Services\Service; use SP\Domain\Common\Services\ServiceException; use SP\Domain\Core\Exceptions\ConstraintException; use SP\Domain\Core\Exceptions\QueryException; use SP\Domain\Core\Exceptions\SPException; +use SP\Domain\User\Models\UserPassRecover as UserPassRecoverModel; use SP\Domain\User\Ports\UserPassRecoverRepository; -use SP\Domain\User\Ports\UserPassRecoverServiceInterface; +use SP\Domain\User\Ports\UserPassRecoverService; use SP\Html\Html; use SP\Util\PasswordUtil; +use function SP\__; +use function SP\__u; + /** * Class UserPassRecoverService - * - * @package SP\Domain\Common\Services\UserPassRecover */ -final class UserPassRecoverService extends Service implements UserPassRecoverServiceInterface +final class UserPassRecover extends Service implements UserPassRecoverService { /** * Tiempo máximo para recuperar la clave @@ -54,16 +55,14 @@ final class UserPassRecoverService extends Service implements UserPassRecoverSer */ public const MAX_PASS_RECOVER_LIMIT = 3; - protected UserPassRecoverRepository $userPassRecoverRepository; - - public function __construct(Application $application, UserPassRecoverRepository $userPassRecoverRepository) - { + public function __construct( + Application $application, + private readonly UserPassRecoverRepository $userPassRecoverRepository + ) { parent::__construct($application); - - $this->userPassRecoverRepository = $userPassRecoverRepository; } - public static function getMailMessage(string $hash): MailMessage + public static function getMailMessage(string $hash, string $baseUri): MailMessage { $mailMessage = new MailMessage(); $mailMessage->setTitle(__('Password Change')); @@ -72,7 +71,7 @@ final class UserPassRecoverService extends Service implements UserPassRecoverSer $mailMessage->addDescription(__('In order to complete the process, please go to this URL:')); $mailMessage->addDescriptionLine(); $mailMessage->addDescription( - Html::anchorText(BootstrapBase::$WEBURI . '/index.php?r=userPassReset/reset/' . $hash) + Html::anchorText(sprintf('%s/index.php?r=userPassReset/reset/%s', $baseUri, $hash)) ); $mailMessage->addDescriptionLine(); $mailMessage->addDescription(__('If you have not requested this action, please dismiss this message.')); @@ -86,12 +85,10 @@ final class UserPassRecoverService extends Service implements UserPassRecoverSer */ public function toggleUsedByHash(string $hash): void { - if ($this->userPassRecoverRepository->toggleUsedByHash( - $hash, - time() - self::MAX_PASS_RECOVER_TIME - ) === 0 - ) { - throw new ServiceException(__u('Wrong hash or expired'), SPException::INFO); + $time = time() - self::MAX_PASS_RECOVER_TIME; + + if ($this->userPassRecoverRepository->toggleUsedByHash($hash, $time) === 0) { + throw ServiceException::info(__u('Wrong hash or expired')); } } @@ -104,7 +101,7 @@ final class UserPassRecoverService extends Service implements UserPassRecoverSer public function requestForUserId(int $id): string { if ($this->checkAttemptsByUserId($id)) { - throw new ServiceException(__u('Attempts exceeded'), SPException::WARNING); + throw ServiceException::warning(__u('Attempts exceeded')); } $hash = PasswordUtil::generateRandomBytes(16); @@ -120,41 +117,38 @@ final class UserPassRecoverService extends Service implements UserPassRecoverSer * @throws ConstraintException * @throws QueryException */ - public function checkAttemptsByUserId(int $userId): bool + private function checkAttemptsByUserId(int $userId): bool { - return $this->userPassRecoverRepository->getAttemptsByUserId( - $userId, - time() - self::MAX_PASS_RECOVER_TIME - ) >= self::MAX_PASS_RECOVER_LIMIT; + $time = time() - self::MAX_PASS_RECOVER_TIME; + + return $this->userPassRecoverRepository->getAttemptsByUserId($userId, $time) >= self::MAX_PASS_RECOVER_LIMIT; } /** * @throws ConstraintException * @throws QueryException */ - public function add(int $userId, string $hash): bool + public function add(int $userId, string $hash): void { - return $this->userPassRecoverRepository->add($userId, $hash); + $this->userPassRecoverRepository->add($userId, $hash); } /** * Comprobar el hash de recuperación de clave. * + * @param string $hash + * @return int * @throws ServiceException - * @throws ConstraintException - * @throws QueryException */ public function getUserIdForHash(string $hash): int { - $result = $this->userPassRecoverRepository->getUserIdForHash( - $hash, - time() - self::MAX_PASS_RECOVER_TIME - ); + $time = time() - self::MAX_PASS_RECOVER_TIME; + $result = $this->userPassRecoverRepository->getUserIdForHash($hash, $time); if ($result->getNumRows() === 0) { - throw new ServiceException(__u('Wrong hash or expired'), SPException::INFO); + throw ServiceException::info(__u('Wrong hash or expired')); } - return (int)$result->getData()->userId; + return $result->getData(UserPassRecoverModel::class)->getUserId(); } } diff --git a/tests/SPT/Domain/User/Services/UserPassRecoverServiceTest.php b/tests/SPT/Domain/User/Services/UserPassRecoverServiceTest.php new file mode 100644 index 00000000..0a7becc8 --- /dev/null +++ b/tests/SPT/Domain/User/Services/UserPassRecoverServiceTest.php @@ -0,0 +1,188 @@ +. + */ + +namespace SPT\Domain\User\Services; + +use Defuse\Crypto\Exception\EnvironmentIsBrokenException; +use PHPUnit\Framework\Attributes\Group; +use PHPUnit\Framework\MockObject\MockObject; +use SP\Domain\Common\Services\ServiceException; +use SP\Domain\Core\Exceptions\ConstraintException; +use SP\Domain\Core\Exceptions\QueryException; +use SP\Domain\Core\Exceptions\SPException; +use SP\Domain\User\Models\UserPassRecover as UserPassRecoverModel; +use SP\Domain\User\Ports\UserPassRecoverRepository; +use SP\Domain\User\Services\UserPassRecover; +use SP\Infrastructure\Database\QueryResult; +use SPT\UnitaryTestCase; + +/** + * Class UserPassRecoverServiceTest + */ +#[Group('unitary')] +class UserPassRecoverServiceTest extends UnitaryTestCase +{ + + private UserPassRecoverRepository|MockObject $userPassRecoverRepository; + private UserPassRecover $userPassRecover; + + /** + * @throws ServiceException + */ + public function testGetUserIdForHash() + { + $this->userPassRecoverRepository + ->expects($this->once()) + ->method('getUserIdForHash') + ->with('a_hash', self::callback(static fn(int $time) => $time < time())) + ->willReturn(new QueryResult([new UserPassRecoverModel(['userId' => 100])])); + + $out = $this->userPassRecover->getUserIdForHash('a_hash'); + + $this->assertEquals(100, $out); + } + + /** + * @throws ServiceException + */ + public function testGetUserIdForHashWithNoRows() + { + $this->userPassRecoverRepository + ->expects($this->once()) + ->method('getUserIdForHash') + ->with('a_hash', self::callback(static fn(int $time) => $time < time())) + ->willReturn(new QueryResult()); + + $this->expectException(ServiceException::class); + $this->expectExceptionMessage('Wrong hash or expired'); + + $this->userPassRecover->getUserIdForHash('a_hash'); + } + + /** + * @throws ServiceException + * @throws ConstraintException + * @throws QueryException + * @throws EnvironmentIsBrokenException + */ + public function testRequestForUserId() + { + $this->userPassRecoverRepository + ->expects($this->once()) + ->method('getAttemptsByUserId') + ->with(100, self::callback(static fn(int $time) => $time < time())) + ->willReturn(1); + + $this->userPassRecoverRepository + ->expects($this->once()) + ->method('add') + ->with(100, self::anything()); + + $out = $this->userPassRecover->requestForUserId(100); + + $this->assertNotEmpty($out); + } + + /** + * @throws ServiceException + * @throws ConstraintException + * @throws QueryException + * @throws EnvironmentIsBrokenException + */ + public function testRequestForUserIdWithMaxAttempts() + { + $this->userPassRecoverRepository + ->expects($this->once()) + ->method('getAttemptsByUserId') + ->with(100, self::callback(static fn(int $time) => $time < time())) + ->willReturn(3); + + $this->userPassRecoverRepository + ->expects($this->never()) + ->method('add'); + + $this->expectException(ServiceException::class); + $this->expectExceptionMessage('Attempts exceeded'); + + $this->userPassRecover->requestForUserId(100); + } + + /** + * @throws ServiceException + * @throws SPException + */ + public function testToggleUsedByHash() + { + $this->userPassRecoverRepository + ->expects($this->once()) + ->method('toggleUsedByHash') + ->with('a_hash', self::callback(static fn(int $time) => $time < time())) + ->willReturn(1); + + $this->userPassRecover->toggleUsedByHash('a_hash'); + } + + /** + * @throws ServiceException + * @throws SPException + */ + public function testToggleUsedByHashWithNoRows() + { + $this->userPassRecoverRepository + ->expects($this->once()) + ->method('toggleUsedByHash') + ->with('a_hash', self::callback(static fn(int $time) => $time < time())) + ->willReturn(0); + + $this->expectException(ServiceException::class); + $this->expectExceptionMessage('Wrong hash or expired'); + + $this->userPassRecover->toggleUsedByHash('a_hash'); + } + + /** + * @throws ConstraintException + * @throws QueryException + */ + public function testAdd() + { + $this->userPassRecoverRepository + ->expects($this->once()) + ->method('add') + ->with(100, 'a_hash'); + + $this->userPassRecover->add(100, 'a_hash'); + } + + protected function setUp(): void + { + parent::setUp(); + + $this->userPassRecoverRepository = $this->createMock(UserPassRecoverRepository::class); + + $this->userPassRecover = new UserPassRecover($this->application, $this->userPassRecoverRepository); + } + + +}