From d695fe3600857abee9965dcab2f833c4783cd7e2 Mon Sep 17 00:00:00 2001 From: Samuel Gfeller Date: Thu, 19 Oct 2023 19:32:26 +0200 Subject: [PATCH] Added cakephp validator and started implementation [SLE-194] --- composer.json | 3 +- .../Ajax/NewPasswordResetSubmitAction.php | 9 - .../Client/Ajax/ClientCreateAction.php | 72 +++-- src/Domain/Client/Service/ClientCreator.php | 2 +- .../Service/ClientCreatorFromClientSubmit.php | 2 +- src/Domain/Client/Service/ClientUpdater.php | 2 +- src/Domain/Client/Service/ClientValidator.php | 96 ++++++- .../Client/Service/ClientValidatorVanilla.php | 250 ++++++++++++++++++ src/Domain/Note/Service/NoteValidator.php | 6 +- src/Domain/User/Service/UserValidator.php | 4 +- .../{Validator.php => ValidatorNative.php} | 4 +- .../ClientStatusFinderRepository.php | 20 ++ 12 files changed, 402 insertions(+), 68 deletions(-) create mode 100644 src/Domain/Client/Service/ClientValidatorVanilla.php rename src/Domain/Validation/{Validator.php => ValidatorNative.php} (98%) diff --git a/composer.json b/composer.json index 70aeae78..ee391f2b 100644 --- a/composer.json +++ b/composer.json @@ -19,7 +19,8 @@ "fig/http-message-util": "^1.1", "ext-gettext": "*", "ext-intl": "*", - "php": "^8.1" + "php": "^8.1", + "cakephp/validation": "^5.0" }, "autoload": { "psr-4": { diff --git a/src/Application/Actions/Authentication/Ajax/NewPasswordResetSubmitAction.php b/src/Application/Actions/Authentication/Ajax/NewPasswordResetSubmitAction.php index 3bf99d44..1697090d 100644 --- a/src/Application/Actions/Authentication/Ajax/NewPasswordResetSubmitAction.php +++ b/src/Application/Actions/Authentication/Ajax/NewPasswordResetSubmitAction.php @@ -18,15 +18,6 @@ class NewPasswordResetSubmitAction { private LoggerInterface $logger; - /** - * The constructor. - * - * @param Responder $responder - * @param SessionInterface $session - * @param PasswordChanger $passwordChanger - * @param MalformedRequestBodyChecker $malformedRequestBodyChecker - * @param LoggerFactory $loggerFactory - */ public function __construct( private readonly Responder $responder, private readonly SessionInterface $session, diff --git a/src/Application/Actions/Client/Ajax/ClientCreateAction.php b/src/Application/Actions/Client/Ajax/ClientCreateAction.php index 0f71d96f..046e3c5c 100644 --- a/src/Application/Actions/Client/Ajax/ClientCreateAction.php +++ b/src/Application/Actions/Client/Ajax/ClientCreateAction.php @@ -41,21 +41,22 @@ public function __construct( * @param ResponseInterface $response The response * @param array $args * + * @return ResponseInterface The response * @throws \JsonException * - * @return ResponseInterface The response */ public function __invoke( ServerRequestInterface $request, ResponseInterface $response, array $args ): ResponseInterface { - if (($loggedInUserId = $this->session->get('user_id')) !== null) { - $clientValues = $request->getParsedBody(); + $clientValues = $request->getParsedBody(); - // If html form names change they have to be adapted in the data class attributes too (e.g. ClientData) - // Check that request body syntax is formatted right (tested in ClientCreateActionTest - malformedRequest) - if ($this->malformedRequestBodyChecker->requestBodyHasValidKeys($clientValues, [ + // If html form names change they have to be adapted in the data class attributes too (e.g. ClientData) + // Check that request body syntax is formatted right (tested in ClientCreateActionTest - malformedRequest) + if ($this->malformedRequestBodyChecker->requestBodyHasValidKeys( + $clientValues, + [ 'client_status_id', 'user_id', 'first_name', @@ -66,39 +67,36 @@ public function __invoke( 'birthdate', 'email', ], // Html radio buttons and checkboxes are not sent over by the client if they are not set hence optional - ['sex', 'client_message', 'vigilance_level'])) { - try { - $insertId = $this->clientCreator->createClient($clientValues); - } catch (ValidationException $exception) { - return $this->responder->respondWithJsonOnValidationError( - $exception->getValidationResult(), - $response - ); - } catch (ForbiddenException $forbiddenException) { - return $this->responder->respondWithJson( - $response, - [ - 'status' => 'error', - 'message' => 'Not allowed to create client.', - ], - StatusCodeInterface::STATUS_FORBIDDEN - ); - } - - if (0 !== $insertId) { - return $this->responder->respondWithJson($response, ['status' => 'success', 'data' => null], 201); - } - $response = $this->responder->respondWithJson($response, [ - 'status' => 'warning', - 'message' => 'Client not created', - ]); + ['sex', 'client_message', 'vigilance_level'] + )) { + try { + $insertId = $this->clientCreator->createClient($clientValues); + } catch (ValidationException $exception) { + return $this->responder->respondWithJsonOnValidationError( + $exception->getValidationResult(), + $response + ); + } catch (ForbiddenException $forbiddenException) { + return $this->responder->respondWithJson( + $response, + [ + 'status' => 'error', + 'message' => 'Not allowed to create client.', + ], + StatusCodeInterface::STATUS_FORBIDDEN + ); + } - return $response->withAddedHeader('Warning', 'The post could not be created'); + if (0 !== $insertId) { + return $this->responder->respondWithJson($response, ['status' => 'success', 'data' => null], 201); } - throw new HttpBadRequestException($request, 'Request body malformed.'); - } + $response = $this->responder->respondWithJson($response, [ + 'status' => 'warning', + 'message' => 'Client not created', + ]); - // Handled by AuthenticationMiddleware - return $response; + return $response->withAddedHeader('Warning', 'The post could not be created'); + } + throw new HttpBadRequestException($request, 'Request body malformed.'); } } diff --git a/src/Domain/Client/Service/ClientCreator.php b/src/Domain/Client/Service/ClientCreator.php index 8ea06972..8c5de658 100644 --- a/src/Domain/Client/Service/ClientCreator.php +++ b/src/Domain/Client/Service/ClientCreator.php @@ -16,7 +16,7 @@ class ClientCreator { public function __construct( - private readonly ClientValidator $clientValidator, + private readonly ClientValidatorVanilla $clientValidator, private readonly ClientCreatorRepository $clientCreatorRepository, private readonly ClientAuthorizationChecker $clientAuthorizationChecker, private readonly NoteCreator $noteCreator, diff --git a/src/Domain/Client/Service/ClientCreatorFromClientSubmit.php b/src/Domain/Client/Service/ClientCreatorFromClientSubmit.php index 728f4b6e..aba8fd65 100644 --- a/src/Domain/Client/Service/ClientCreatorFromClientSubmit.php +++ b/src/Domain/Client/Service/ClientCreatorFromClientSubmit.php @@ -11,7 +11,7 @@ class ClientCreatorFromClientSubmit { public function __construct( - private readonly ClientValidator $clientValidator, + private readonly ClientValidatorVanilla $clientValidator, private readonly ClientCreatorRepository $clientCreatorRepository, private readonly UserActivityManager $userActivityManager, private readonly ClientStatusFinderRepository $clientStatusFinderRepository, diff --git a/src/Domain/Client/Service/ClientUpdater.php b/src/Domain/Client/Service/ClientUpdater.php index cc6d0d89..a13b6601 100644 --- a/src/Domain/Client/Service/ClientUpdater.php +++ b/src/Domain/Client/Service/ClientUpdater.php @@ -17,7 +17,7 @@ class ClientUpdater public function __construct( private readonly ClientUpdaterRepository $clientUpdaterRepository, - private readonly ClientValidator $clientValidator, + private readonly ClientValidatorVanilla $clientValidator, private readonly ClientFinder $clientFinder, LoggerFactory $logger, private readonly ClientAuthorizationChecker $clientAuthorizationChecker, diff --git a/src/Domain/Client/Service/ClientValidator.php b/src/Domain/Client/Service/ClientValidator.php index 3a10b529..c287e870 100644 --- a/src/Domain/Client/Service/ClientValidator.php +++ b/src/Domain/Client/Service/ClientValidator.php @@ -5,26 +5,98 @@ use App\Domain\Client\Enum\ClientVigilanceLevel; use App\Domain\Factory\LoggerFactory; use App\Domain\Validation\ValidationResult; -use App\Domain\Validation\Validator; +use App\Domain\Validation\ValidatorNative; +use App\Infrastructure\Client\ClientStatus\ClientStatusFinderRepository; +use Cake\Validation\Validator; -/** - * Client user input validator. - */ class ClientValidator { - /** - * PostValidator constructor. - * - * @param LoggerFactory $logger - * @param Validator $validator - */ public function __construct( LoggerFactory $logger, - private readonly Validator $validator, + private readonly ValidatorNative $validator, + private readonly ClientStatusFinderRepository $clientStatusFinderRepository, ) { $logger->addFileHandler('error.log')->createLogger('post-validation'); } + public function validateClientCreation() + { + $validator = new Validator(); + + $validator + // Require presence indicates that the key is required in the request body + ->requirePresence('first_name') + ->minLength('first_name', 3, __('Minimum length is 3')) + ->maxLength('first_name', 100, __('Maximum length is 100')) + ->requirePresence('last_name') + ->minLength('last_name', 3, __('Minimum length is 3')) + ->maxLength('last_name', 100, __('Maximum length is 100')) + ->requirePresence('email') + ->email('email', false, __('Invalid e-mail')) + ->requirePresence('birthdate') + ->date('birthdate', ['ymd', 'mdy', 'dmy'], 'Invalid date value') + ->add('birthdate', 'validateNotInFuture', [ + 'rule' => function ($value, $context) { + $today = new \DateTime(); + $birthdate = new \DateTime($value); + + return $birthdate <= $today; + }, + 'message' => __('Cannot be in the future') + ]) + ->add('birthdate', 'validateOldestAge', [ + 'rule' => function ($value, $context) { + $birthdate = new \DateTime($value); + // check that birthdate is not older than 130 years + $oldestBirthdate = new \DateTime('-130 years'); + return $birthdate >= $oldestBirthdate; + }, + 'message' => __('Cannot be older than 130 years') + ]) + ->requirePresence('location') + ->minLength('location', 2, __('Minimum length is 2')) + ->maxLength('location', 100, __('Maximum length is 100')) + ->requirePresence('phone') + ->minLength('phone', 3, __('Minimum length is 3')) + ->maxLength('phone', 20, __('Maximum length is 20')) + // Sex should not have requirePresence as it's not required and the client won't send the key over if not set + ->inList('sex', ['M', 'F', 'O', ''], 'Invalid option') + // This is too complex and will have to be changed in the future. Enums are not ideal in general. + // No requirePresence for vigilance_level + ->add('vigilance_level', 'validateBackedEnum', [ + 'rule' => function ($value, $context) { + return $this->isBackedEnum($value, ClientVigilanceLevel::class, 'vigilance_level'); + }, + 'message' => __('Invalid option') + ]) + ->requirePresence('client_message') + ->minLength('client_message', 3, __('Minimum length is 3')) + ->maxLength('client_message', 1000, __('Maximum length is 1000')) + ->requirePresence('client_status_id', true, __('Required')) + ->numeric('client_status_id', __('Invalid option format')) + ->add('client_status_id', 'valid', [ + 'rule' => function ($value, $context) { + return $this->clientStatusFinderRepository->clientStatusExists((int)$value); + }, + 'message' => 'Invalid option' + ]); + } + + /** + * Check if value is a specific backed enum case or string + * that can be converted into one. + * + * @param \BackedEnum|string|null $value + * @param string $enum + * @return bool + */ + public function isBackedEnum(\BackedEnum|string|null $value, string $enum): bool + { + // If $value is already an enum case, it means that its valid otherwise try to convert it to enum case + return is_a($value, $enum, true) || is_a($enum::tryFrom($value), $enum, true); + } + + /** * Validate client creation. * Validate client values as array and not object to prevent exception on @@ -34,7 +106,7 @@ public function __construct( * * @param array $clientValues */ - public function validateClientCreation(array $clientValues): void + public function validateClientCreationOld(array $clientValues): void { // Validation error message asserted in ClientCreateActionTest $validationResult = new ValidationResult('There is something in the client data that couldn\'t be validated'); diff --git a/src/Domain/Client/Service/ClientValidatorVanilla.php b/src/Domain/Client/Service/ClientValidatorVanilla.php new file mode 100644 index 00000000..7bd5be1a --- /dev/null +++ b/src/Domain/Client/Service/ClientValidatorVanilla.php @@ -0,0 +1,250 @@ +addFileHandler('error.log')->createLogger('post-validation'); + } + + /** + * Validate client creation. + * Validate client values as array and not object to prevent exception on + * invalid data such as datetime is used in the constructor. + * *All keys that may not be in the request body (malformedRequestBodyChecker - optional keys) + * *such as radio buttons have to be accessed with null coalescing alternative: $values['key'] ?? null. + * + * @param array $clientValues + */ + public function validateClientCreation(array $clientValues): void + { + // Validation error message asserted in ClientCreateActionTest + $validationResult = new ValidationResult('There is something in the client data that couldn\'t be validated'); + + $this->validateClientStatusId($clientValues['client_status_id'], $validationResult, true); + + // User id should not be required when client is created from authenticated area and public portal + $this->validateUserId($clientValues['user_id'], $validationResult, false); + + // First and last name not required in case advisor only knows either first or last name + $this->validator->validateName($clientValues['first_name'], 'first_name', $validationResult, false); + $this->validator->validateName($clientValues['last_name'], 'last_name', $validationResult, false); + + $this->validator->validateEmail($clientValues['email'], $validationResult, false); + // With birthdate original user input value as it's transformed into a DateTimeImmutable when object gets populated + $this->validator->validateBirthdate($clientValues['birthdate'], $validationResult, false); + + $this->validateLocation($clientValues['location'], $validationResult, false); + + $this->validatePhone($clientValues['phone'], $validationResult, false); + + $this->validateSex($clientValues['sex'] ?? null, $validationResult, false); + + $this->validateClientMessage($clientValues['client_message'] ?? null, $validationResult, false); + + $this->validator->validateBackedEnum( + $clientValues['vigilance_level'] ?? null, + ClientVigilanceLevel::class, + 'vigilance_level', + $validationResult, + false + ); + + $this->validator->throwOnError($validationResult); + } + + /** + * Validate post update. + * + * @param array $clientValues values that user wants to change + */ + public function validateClientUpdate(array $clientValues): void + { + // Validation error message asserted in ClientUpdateActionTest + $validationResult = new ValidationResult('There is something in the client data that couldn\'t be validated'); + + // Using array_key_exists instead of isset as isset returns false if value is null and key exists + if (array_key_exists('client_status_id', $clientValues)) { + $this->validateClientStatusId($clientValues['client_status_id'], $validationResult, false); + } + if (array_key_exists('user_id', $clientValues)) { + $this->validateUserId($clientValues['user_id'], $validationResult, false); + } + if (array_key_exists('first_name', $clientValues)) { + $this->validator->validateName($clientValues['first_name'], 'first_name', $validationResult, false); + } + if (array_key_exists('last_name', $clientValues)) { + $this->validator->validateName($clientValues['last_name'], 'last_name', $validationResult, false); + } + if (array_key_exists('email', $clientValues)) { + $this->validator->validateEmail($clientValues['email'], $validationResult, false); + } + if (array_key_exists('birthdate', $clientValues)) { + $this->validator->validateBirthdate($clientValues['birthdate'], $validationResult, false); + } + if (array_key_exists('location', $clientValues)) { + $this->validateLocation($clientValues['location'], $validationResult, false); + } + if (array_key_exists('phone', $clientValues)) { + $this->validatePhone($clientValues['phone'], $validationResult, false); + } + if (array_key_exists('sex', $clientValues)) { + $this->validateSex($clientValues['sex'], $validationResult, false); + } + if (array_key_exists('vigilance_level', $clientValues)) { + $this->validator->validateBackedEnum( + $clientValues['vigilance_level'], + ClientVigilanceLevel::class, + 'vigilance_level', + $validationResult, + false + ); + } + + $this->validator->throwOnError($validationResult); + } + + // Validate functions for each field + + /** + * Validate client user id dropdown. + * + * @param $value + * @param ValidationResult $validationResult + * @param bool $required + * + * @return void + */ + protected function validateUserId($value, ValidationResult $validationResult, bool $required = false): void + { + if (null !== $value && '' !== $value) { + $this->validator->validateNumeric($value, 'user_id', $validationResult, $required); + $this->validator->validateExistence((int)$value, 'user', $validationResult, $required); + } elseif (true === $required) { + // If it is null or empty string and required + $validationResult->setError('user_id', __('Required')); + } + } + + /** + * Validate client status dropdown. + * + * @param mixed $value + * @param ValidationResult $validationResult + * @param bool $required + * + * @return void + */ + protected function validateClientStatusId( + mixed $value, + ValidationResult $validationResult, + bool $required = false + ): void { + if (null !== $value && '' !== $value) { + $this->validator->validateNumeric($value, 'client_status_id', $validationResult, $required); + $this->validator->validateExistence((int)$value, 'client_status', $validationResult, $required); + } elseif (true === $required) { + // If it is null or empty string and required + $validationResult->setError('client_status_id', __('Required')); + } + } + + /** + * Validate client location input. + * + * @param $location + * @param ValidationResult $validationResult + * @param bool $required + * + * @return void + */ + protected function validateLocation($location, ValidationResult $validationResult, bool $required = false): void + { + if (null !== $location && '' !== $location) { + $this->validator->validateLengthMax($location, 'location', $validationResult, 100); + $this->validator->validateLengthMin($location, 'location', $validationResult, 2); + } elseif (true === $required) { + // If it is null or empty string and required + $validationResult->setError('location', __('Required')); + } + } + + /** + * Validate client phone input. + * + * @param $value + * @param ValidationResult $validationResult + * @param bool $required + * + * @return void + */ + protected function validatePhone($value, ValidationResult $validationResult, bool $required = false): void + { + if (null !== $value && '' !== $value) { + $this->validator->validateLengthMax($value, 'phone', $validationResult, 20); + $this->validator->validateLengthMin($value, 'phone', $validationResult, 3); + } elseif (true === $required) { + // If it is null or empty string and required + $validationResult->setError('phone', __('Required')); + } + } + + /** + * Validate client sex options. + * + * @param $value + * @param ValidationResult $validationResult + * @param bool $required + * + * @return void + */ + protected function validateSex($value, ValidationResult $validationResult, bool $required = false): void + { + if (null !== $value && '' !== $value) { + if (!in_array($value, ['M', 'F', 'O', ''], true)) { + $validationResult->setError('sex', __('Invalid option')); + } + } elseif (true === $required) { + // If it is null or empty string and required + $validationResult->setError('sex', __('Required')); + } + } + + /** + * Validate client message input. + * + * @param $value + * @param ValidationResult $validationResult + * @param bool $required + * + * @return void + */ + private function validateClientMessage($value, ValidationResult $validationResult, bool $required = false): void + { + if (null !== $value && '' !== $value) { + $this->validator->validateLengthMax($value, 'client_message', $validationResult, 1000); + $this->validator->validateLengthMin($value, 'client_message', $validationResult, 3); + } elseif (true === $required) { + // If it is null or empty string and required + $validationResult->setError('client_message', __('Required')); + } + } +} diff --git a/src/Domain/Note/Service/NoteValidator.php b/src/Domain/Note/Service/NoteValidator.php index 9de98cb3..f52d6d79 100644 --- a/src/Domain/Note/Service/NoteValidator.php +++ b/src/Domain/Note/Service/NoteValidator.php @@ -6,7 +6,7 @@ use App\Domain\Note\Data\NoteData; use App\Domain\Validation\ValidationException; use App\Domain\Validation\ValidationResult; -use App\Domain\Validation\Validator; +use App\Domain\Validation\ValidatorNative; use App\Infrastructure\Note\NoteValidatorRepository; use Psr\Log\LoggerInterface; @@ -21,12 +21,12 @@ class NoteValidator * NoteValidator constructor. * * @param NoteValidatorRepository $noteValidatorRepository - * @param Validator $validator + * @param ValidatorNative $validator * @param LoggerFactory $loggerFactory */ public function __construct( private readonly NoteValidatorRepository $noteValidatorRepository, - private readonly Validator $validator, + private readonly ValidatorNative $validator, private readonly LoggerFactory $loggerFactory, ) { $this->logger = $this->loggerFactory->addFileHandler('error.log')->createLogger('note-validation'); diff --git a/src/Domain/User/Service/UserValidator.php b/src/Domain/User/Service/UserValidator.php index 504bd668..91e0272c 100644 --- a/src/Domain/User/Service/UserValidator.php +++ b/src/Domain/User/Service/UserValidator.php @@ -8,7 +8,7 @@ use App\Domain\User\Enum\UserTheme; use App\Domain\Validation\ValidationException; use App\Domain\Validation\ValidationResult; -use App\Domain\Validation\Validator; +use App\Domain\Validation\ValidatorNative; use App\Infrastructure\User\UserFinderRepository; /** @@ -17,7 +17,7 @@ class UserValidator { public function __construct( - private readonly Validator $validator, + private readonly ValidatorNative $validator, private readonly UserFinderRepository $userFinderRepository, ) { } diff --git a/src/Domain/Validation/Validator.php b/src/Domain/Validation/ValidatorNative.php similarity index 98% rename from src/Domain/Validation/Validator.php rename to src/Domain/Validation/ValidatorNative.php index 277f7a55..bedfa620 100644 --- a/src/Domain/Validation/Validator.php +++ b/src/Domain/Validation/ValidatorNative.php @@ -10,7 +10,7 @@ /** * User input validator. */ -final class Validator +final class ValidatorNative { public LoggerInterface $logger; @@ -206,6 +206,8 @@ public function validateExistence( bool $excludingSoftDelete = true ): void { if (null !== $rowId && $rowId !== 0) { + // ! This should not be used anymore. The domain/service layer should NOT know about the table name + // Create a function xtableExists in the relevant repository $exists = $this->resourceExistenceCheckerRepository->rowExists( ['id' => $rowId], $table, diff --git a/src/Infrastructure/Client/ClientStatus/ClientStatusFinderRepository.php b/src/Infrastructure/Client/ClientStatus/ClientStatusFinderRepository.php index 1246a08a..1215b062 100644 --- a/src/Infrastructure/Client/ClientStatus/ClientStatusFinderRepository.php +++ b/src/Infrastructure/Client/ClientStatus/ClientStatusFinderRepository.php @@ -60,4 +60,24 @@ public function findAllClientStatusesMappedByIdName(bool $withoutTranslation = f return $statuses; } + + /** + * Check if given client status id exists. + * + * @param int $clientStatusId + * @return bool + */ + public function clientStatusExists(int $clientStatusId): bool + { + $query = $this->queryFactory->selectQuery()->from('client_status'); + + $query->select(['id']) + ->where( + ['id' => $clientStatusId], + ['deleted_at IS' => null] + ); + $resultRow = $query->execute()->fetch('assoc') ?: []; + + return !empty($resultRow); + } }