From 74fd9b08d9008bd84a374fbe63c50ba2a5b501af Mon Sep 17 00:00:00 2001 From: Samuel Gfeller Date: Mon, 23 Oct 2023 18:26:54 +0200 Subject: [PATCH] Client creation validation refactoring [SLE-194] --- config/container.php | 2 - config/middleware.php | 7 +- docs/error-handling.md | 5 +- .../general/ajax/ajax-util/fail-handler.js | 17 +- .../general/validation/form-validation.js | 5 +- .../Authentication/Ajax/LoginSubmitAction.php | 4 +- .../Ajax/NewPasswordResetSubmitAction.php | 4 +- .../PasswordForgottenEmailSubmitAction.php | 4 +- .../Client/Ajax/ApiClientCreateAction.php | 4 +- .../Client/Ajax/ClientCreateAction.php | 66 +---- .../Client/Ajax/ClientUpdateAction.php | 4 +- .../Actions/Note/Ajax/NoteCreateAction.php | 4 +- .../Actions/Note/Ajax/NoteUpdateAction.php | 4 +- .../User/Ajax/PasswordChangeSubmitAction.php | 4 +- .../Actions/User/Ajax/UserCreateAction.php | 4 +- .../Actions/User/Ajax/UserUpdateAction.php | 4 +- .../ErrorHandler/DefaultErrorHandler.php | 4 +- .../Middleware/ErrorHandlerMiddleware.php | 2 +- .../ForbiddenExceptionMiddleware.php | 39 +++ .../ValidationExceptionMiddleware.php | 103 ++++++++ src/Application/Responder/Responder.php | 26 +- .../Service/PasswordRecoveryEmailSender.php | 4 +- src/Domain/Client/Data/ClientData.php | 7 +- src/Domain/Client/Service/ClientCreator.php | 7 +- src/Domain/Client/Service/ClientValidator.php | 126 +++++----- src/Domain/Note/Service/NoteCreator.php | 9 +- src/Domain/Note/Service/NoteValidator.php | 56 ++++- src/Domain/User/Service/UserValidator.php | 10 +- src/Domain/Validation/ValidationException.php | 21 +- .../Validation/ValidationExceptionOld.php | 31 +++ src/Domain/Validation/ValidatorNative.php | 4 +- .../Client/ClientCreateActionTest.php | 127 ++++++---- .../Provider/Client/ClientCreateProvider.php | 231 ++++++++++-------- 33 files changed, 580 insertions(+), 369 deletions(-) create mode 100644 src/Application/Middleware/ForbiddenExceptionMiddleware.php create mode 100644 src/Application/Middleware/ValidationExceptionMiddleware.php create mode 100644 src/Domain/Validation/ValidationExceptionOld.php diff --git a/config/container.php b/config/container.php index 3f76eabe..5aace53c 100644 --- a/config/container.php +++ b/config/container.php @@ -101,11 +101,9 @@ }, PDO::class => function (ContainerInterface $container) { $driver = $container->get(Connection::class)->getDriver(); - $class = new ReflectionClass($driver); $method = $class->getMethod('getPdo'); $method->setAccessible(true); - return $method->invoke($driver); }, // Used by command line to generate `schema.sql` for integration testing diff --git a/config/middleware.php b/config/middleware.php index d1c5885f..e2d14c7e 100644 --- a/config/middleware.php +++ b/config/middleware.php @@ -1,7 +1,9 @@ add(BasePathMiddleware::class); - // Error middleware should be added last. It will not handle any exceptions/errors + // Error middlewares should be added last as they will be the first on the response (LIFO). + $app->add(ValidationExceptionMiddleware::class); + $app->add(ForbiddenExceptionMiddleware::class); + $app->add(ErrorHandlerMiddleware::class); $app->add(ErrorMiddleware::class); }; diff --git a/docs/error-handling.md b/docs/error-handling.md index 9a9f45b1..9c469790 100644 --- a/docs/error-handling.md +++ b/docs/error-handling.md @@ -317,13 +317,14 @@ and I do that with the color red and orange. File: `src/Application/ErrorHandler/DefaultErrorHandler.php` (most up to date version can be found [here](https://github.com/samuelgfeller/slim-example-project/blob/master/src/Application/ErrorHandler/DefaultErrorHandler.php)) + ```php getCode(); } - if ($exception instanceof ValidationException) { + if ($exception instanceof ValidationExceptionOld) { $statusCode = StatusCodeInterface::STATUS_UNPROCESSABLE_ENTITY; // 422 } diff --git a/public/assets/general/ajax/ajax-util/fail-handler.js b/public/assets/general/ajax/ajax-util/fail-handler.js index 264d1ca9..08a6ab18 100644 --- a/public/assets/general/ajax/ajax-util/fail-handler.js +++ b/public/assets/general/ajax/ajax-util/fail-handler.js @@ -51,20 +51,25 @@ export function handleFail(xhr, domFieldId = null) { if (xhr.status === 422) { if (xhr.getResponseHeader('Content-type') === 'application/json') { errorMsg = ''; - let json = xhr.response; - let validationResponse = JSON.parse(json); + let validationResponse = JSON.parse(xhr.response); + const validationErrors = validationResponse.data.errors; removeValidationErrorMessages(); // Best foreach loop method according to https://stackoverflow.com/a/9329476/9013718 - for (const error of validationResponse.data.errors) { - displayValidationErrorMessage(error.field, error.message, domFieldId); + for (const fieldName in validationErrors) { + const fieldMessages = validationErrors[fieldName]; + // There may be the case that there are multiple error messages for a single field. In such case, + // the previous error message is simply replaced by the newer one which isn't ideal but acceptable in + // this scope especially since its so rare and the worst case would be that user has to submit form once + // more to get the updated (other) error message (that he couldn't see before) + displayValidationErrorMessage(fieldName, fieldMessages[0], domFieldId); // Flash error message with details - errorMsg += error.message + '.
Field "' + error.field.replace(/[^a-zA-Z0-9 ]/g, ' ') + errorMsg += fieldMessages[0] + '.
Field "' + fieldName.replace(/[^a-zA-Z0-9 ]/g, ' ') + '".
'; } noFlashMessage = true; } else { // Default error message when server returns 422 but not json - errorMsg = 'Validation error. Something could not have been validate on the server.'; + errorMsg = 'Validation error. Something could not be validated on the server.'; } } diff --git a/public/assets/general/validation/form-validation.js b/public/assets/general/validation/form-validation.js index 6f1961c3..033cb7c2 100644 --- a/public/assets/general/validation/form-validation.js +++ b/public/assets/general/validation/form-validation.js @@ -13,7 +13,7 @@ export function displayValidationErrorMessage(fieldName, errorMessage, domFieldI field = document.querySelector(`[name="${fieldName}"]`); } if (field === null) { - // Contenteditable field + // Contenteditable field accessed with data-name field = document.querySelector(`[data-name="${fieldName}"]`); } // console.log(domFieldId, field); @@ -29,6 +29,9 @@ export function displayValidationErrorMessage(fieldName, errorMessage, domFieldI } // Remove any existing message in case there was one // (this is an additional for when this function is called not from the handleFail() that removes previous error msg) + // If there are multiple error messages for a single field, the previous one is simply replaced by the newer one + // which isn't ideal but acceptable in this scope especially since its so rare and worst case would be that user + // has to submit form one more time to get the updated (other) error message (that he couldn't see before) field.parentNode.querySelector('strong.err-msg')?.remove(); field.insertAdjacentHTML('afterend', `${errorMessage}`); let label = field.parentNode.querySelector('label'); diff --git a/src/Application/Actions/Authentication/Ajax/LoginSubmitAction.php b/src/Application/Actions/Authentication/Ajax/LoginSubmitAction.php index efda952d..be0b152d 100644 --- a/src/Application/Actions/Authentication/Ajax/LoginSubmitAction.php +++ b/src/Application/Actions/Authentication/Ajax/LoginSubmitAction.php @@ -10,7 +10,7 @@ use App\Domain\Factory\LoggerFactory; use App\Domain\Security\Exception\SecurityException; use App\Domain\User\Service\UserFinder; -use App\Domain\Validation\ValidationException; +use App\Domain\Validation\ValidationExceptionOld; use Odan\Session\SessionInterface; use Psr\Http\Message\ResponseInterface as Response; use Psr\Http\Message\ServerRequestInterface as ServerRequest; @@ -75,7 +75,7 @@ public function __invoke(ServerRequest $request, Response $response): Response } return $this->responder->redirectToRouteName($response, 'home-page', [], $themeQueryParams); - } catch (ValidationException $ve) { + } catch (ValidationExceptionOld $ve) { return $this->responder->renderOnValidationError( $response, 'authentication/login.html.php', diff --git a/src/Application/Actions/Authentication/Ajax/NewPasswordResetSubmitAction.php b/src/Application/Actions/Authentication/Ajax/NewPasswordResetSubmitAction.php index 1697090d..0d531737 100644 --- a/src/Application/Actions/Authentication/Ajax/NewPasswordResetSubmitAction.php +++ b/src/Application/Actions/Authentication/Ajax/NewPasswordResetSubmitAction.php @@ -7,7 +7,7 @@ use App\Domain\Authentication\Exception\InvalidTokenException; use App\Domain\Authentication\Service\PasswordChanger; use App\Domain\Factory\LoggerFactory; -use App\Domain\Validation\ValidationException; +use App\Domain\Validation\ValidationExceptionOld; use Odan\Session\SessionInterface; use Psr\Http\Message\ResponseInterface as Response; use Psr\Http\Message\ServerRequestInterface as ServerRequest; @@ -83,7 +83,7 @@ public function __invoke(ServerRequest $request, Response $response): Response // Render password return $this->responder->render($response, 'authentication/login.html.php'); - } catch (ValidationException $validationException) { + } catch (ValidationExceptionOld $validationException) { $flash->add('error', $validationException->getMessage()); // Add token and id to php view attribute like PasswordResetAction does $this->responder->addPhpViewAttribute('token', $parsedBody['token']); diff --git a/src/Application/Actions/Authentication/Ajax/PasswordForgottenEmailSubmitAction.php b/src/Application/Actions/Authentication/Ajax/PasswordForgottenEmailSubmitAction.php index eaf0ec82..756b0d67 100644 --- a/src/Application/Actions/Authentication/Ajax/PasswordForgottenEmailSubmitAction.php +++ b/src/Application/Actions/Authentication/Ajax/PasswordForgottenEmailSubmitAction.php @@ -8,7 +8,7 @@ use App\Domain\Exception\DomainRecordNotFoundException; use App\Domain\Factory\LoggerFactory; use App\Domain\Security\Exception\SecurityException; -use App\Domain\Validation\ValidationException; +use App\Domain\Validation\ValidationExceptionOld; use Odan\Session\SessionInterface; use Psr\Http\Message\ResponseInterface as Response; use Psr\Http\Message\ServerRequestInterface as ServerRequest; @@ -62,7 +62,7 @@ public function __invoke(ServerRequest $request, Response $response): Response 'User with email ' . $userValues['email'] . ' tried to request password reset link but account doesn\'t exist' ); - } catch (ValidationException $validationException) { + } catch (ValidationExceptionOld $validationException) { // Form error messages set in function below return $this->responder->renderOnValidationError( $response, diff --git a/src/Application/Actions/Client/Ajax/ApiClientCreateAction.php b/src/Application/Actions/Client/Ajax/ApiClientCreateAction.php index 214002c3..f5b670ae 100644 --- a/src/Application/Actions/Client/Ajax/ApiClientCreateAction.php +++ b/src/Application/Actions/Client/Ajax/ApiClientCreateAction.php @@ -5,7 +5,7 @@ use App\Application\Responder\Responder; use App\Application\Validation\MalformedRequestBodyChecker; use App\Domain\Client\Service\ClientCreatorFromClientSubmit; -use App\Domain\Validation\ValidationException; +use App\Domain\Validation\ValidationExceptionOld; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\ServerRequestInterface; use Slim\Exception\HttpBadRequestException; @@ -64,7 +64,7 @@ public function __invoke( )) { try { $insertId = $this->clientCreatorFromClientSubmit->createClientFromClientSubmit($clientValues); - } catch (ValidationException $exception) { + } catch (ValidationExceptionOld $exception) { return $this->responder->respondWithJsonOnValidationError( $exception->getValidationResult(), $response diff --git a/src/Application/Actions/Client/Ajax/ClientCreateAction.php b/src/Application/Actions/Client/Ajax/ClientCreateAction.php index 046e3c5c..499de5eb 100644 --- a/src/Application/Actions/Client/Ajax/ClientCreateAction.php +++ b/src/Application/Actions/Client/Ajax/ClientCreateAction.php @@ -3,15 +3,9 @@ namespace App\Application\Actions\Client\Ajax; use App\Application\Responder\Responder; -use App\Application\Validation\MalformedRequestBodyChecker; -use App\Domain\Authentication\Exception\ForbiddenException; use App\Domain\Client\Service\ClientCreator; -use App\Domain\Validation\ValidationException; -use Fig\Http\Message\StatusCodeInterface; -use Odan\Session\SessionInterface; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\ServerRequestInterface; -use Slim\Exception\HttpBadRequestException; /** * Action. @@ -23,14 +17,10 @@ final class ClientCreateAction * * @param Responder $responder The responder * @param ClientCreator $clientCreator - * @param SessionInterface $session - * @param MalformedRequestBodyChecker $malformedRequestBodyChecker */ public function __construct( private readonly Responder $responder, private readonly ClientCreator $clientCreator, - private readonly SessionInterface $session, - private readonly MalformedRequestBodyChecker $malformedRequestBodyChecker, ) { } @@ -42,8 +32,6 @@ public function __construct( * @param array $args * * @return ResponseInterface The response - * @throws \JsonException - * */ public function __invoke( ServerRequestInterface $request, @@ -52,51 +40,17 @@ public function __invoke( ): ResponseInterface { $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, - [ - 'client_status_id', - 'user_id', - 'first_name', - 'last_name', - 'phone', - 'location', - 'message', - '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', - ]); + // Validation and Forbidden exception caught in respective middlewares + $insertId = $this->clientCreator->createClient($clientValues); - 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', + ]); + + return $response->withAddedHeader('Warning', 'The client could not be created'); } } diff --git a/src/Application/Actions/Client/Ajax/ClientUpdateAction.php b/src/Application/Actions/Client/Ajax/ClientUpdateAction.php index 9bf60cb3..5cb79f86 100644 --- a/src/Application/Actions/Client/Ajax/ClientUpdateAction.php +++ b/src/Application/Actions/Client/Ajax/ClientUpdateAction.php @@ -7,7 +7,7 @@ use App\Domain\Authentication\Exception\ForbiddenException; use App\Domain\Client\Service\ClientUpdater; use App\Domain\Factory\LoggerFactory; -use App\Domain\Validation\ValidationException; +use App\Domain\Validation\ValidationExceptionOld; use Fig\Http\Message\StatusCodeInterface; use Odan\Session\SessionInterface; use Psr\Http\Message\ResponseInterface; @@ -88,7 +88,7 @@ public function __invoke( ]); return $response->withAddedHeader('Warning', 'The client was not updated.'); - } catch (ValidationException $exception) { + } catch (ValidationExceptionOld $exception) { return $this->responder->respondWithJsonOnValidationError( $exception->getValidationResult(), $response diff --git a/src/Application/Actions/Note/Ajax/NoteCreateAction.php b/src/Application/Actions/Note/Ajax/NoteCreateAction.php index c26a61c9..449e7fd6 100644 --- a/src/Application/Actions/Note/Ajax/NoteCreateAction.php +++ b/src/Application/Actions/Note/Ajax/NoteCreateAction.php @@ -8,7 +8,7 @@ use App\Domain\Note\Service\NoteCreator; use App\Domain\Note\Service\NoteFinder; use App\Domain\User\Service\UserFinder; -use App\Domain\Validation\ValidationException; +use App\Domain\Validation\ValidationExceptionOld; use Fig\Http\Message\StatusCodeInterface; use IntlDateFormatter; use Odan\Session\SessionInterface; @@ -68,7 +68,7 @@ public function __invoke( try { $insertId = $this->noteCreator->createNote($noteValues); $noteDataFromDb = $this->noteFinder->findNote($insertId); - } catch (ValidationException $exception) { + } catch (ValidationExceptionOld $exception) { return $this->responder->respondWithJsonOnValidationError( $exception->getValidationResult(), $response diff --git a/src/Application/Actions/Note/Ajax/NoteUpdateAction.php b/src/Application/Actions/Note/Ajax/NoteUpdateAction.php index fd9dd3b1..07c54f1f 100644 --- a/src/Application/Actions/Note/Ajax/NoteUpdateAction.php +++ b/src/Application/Actions/Note/Ajax/NoteUpdateAction.php @@ -7,7 +7,7 @@ use App\Domain\Authentication\Exception\ForbiddenException; use App\Domain\Factory\LoggerFactory; use App\Domain\Note\Service\NoteUpdater; -use App\Domain\Validation\ValidationException; +use App\Domain\Validation\ValidationExceptionOld; use Fig\Http\Message\StatusCodeInterface; use Odan\Session\SessionInterface; use Psr\Http\Message\ResponseInterface; @@ -75,7 +75,7 @@ public function __invoke( ]); return $response->withAddedHeader('Warning', 'The note was not updated.'); - } catch (ValidationException $exception) { + } catch (ValidationExceptionOld $exception) { return $this->responder->respondWithJsonOnValidationError( $exception->getValidationResult(), $response diff --git a/src/Application/Actions/User/Ajax/PasswordChangeSubmitAction.php b/src/Application/Actions/User/Ajax/PasswordChangeSubmitAction.php index 00c40fa9..64bac821 100644 --- a/src/Application/Actions/User/Ajax/PasswordChangeSubmitAction.php +++ b/src/Application/Actions/User/Ajax/PasswordChangeSubmitAction.php @@ -7,7 +7,7 @@ use App\Domain\Authentication\Exception\ForbiddenException; use App\Domain\Authentication\Service\PasswordChanger; use App\Domain\Factory\LoggerFactory; -use App\Domain\Validation\ValidationException; +use App\Domain\Validation\ValidationExceptionOld; use Fig\Http\Message\StatusCodeInterface; use Odan\Session\SessionInterface; use Psr\Http\Message\ResponseInterface as Response; @@ -71,7 +71,7 @@ public function __invoke(ServerRequest $request, Response $response, array $args ); return $this->responder->respondWithJson($response, ['status' => 'success', 'data' => null]); - } catch (ValidationException $validationException) { + } catch (ValidationExceptionOld $validationException) { return $this->responder->respondWithJsonOnValidationError( $validationException->getValidationResult(), $response, diff --git a/src/Application/Actions/User/Ajax/UserCreateAction.php b/src/Application/Actions/User/Ajax/UserCreateAction.php index 936175e2..39627781 100644 --- a/src/Application/Actions/User/Ajax/UserCreateAction.php +++ b/src/Application/Actions/User/Ajax/UserCreateAction.php @@ -8,7 +8,7 @@ use App\Domain\Factory\LoggerFactory; use App\Domain\Security\Exception\SecurityException; use App\Domain\User\Service\UserCreator; -use App\Domain\Validation\ValidationException; +use App\Domain\Validation\ValidationExceptionOld; use Fig\Http\Message\StatusCodeInterface; use Odan\Session\SessionInterface; use Psr\Http\Message\ResponseInterface as Response; @@ -67,7 +67,7 @@ public function __invoke(ServerRequest $request, Response $response): Response } return $this->responder->respondWithJson($response, ['status' => 'success', 'data' => null], 201); - } catch (ValidationException $validationException) { + } catch (ValidationExceptionOld $validationException) { return $this->responder->respondWithJsonOnValidationError( $validationException->getValidationResult(), $response diff --git a/src/Application/Actions/User/Ajax/UserUpdateAction.php b/src/Application/Actions/User/Ajax/UserUpdateAction.php index 398601b0..a98b2f14 100644 --- a/src/Application/Actions/User/Ajax/UserUpdateAction.php +++ b/src/Application/Actions/User/Ajax/UserUpdateAction.php @@ -7,7 +7,7 @@ use App\Domain\Authentication\Exception\ForbiddenException; use App\Domain\Factory\LoggerFactory; use App\Domain\User\Service\UserUpdater; -use App\Domain\Validation\ValidationException; +use App\Domain\Validation\ValidationExceptionOld; use Fig\Http\Message\StatusCodeInterface; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\ServerRequestInterface; @@ -67,7 +67,7 @@ public function __invoke( ])) { try { $updated = $this->userUpdater->updateUser($userIdToChange, $userValuesToChange); - } catch (ValidationException $exception) { + } catch (ValidationExceptionOld $exception) { return $this->responder->respondWithJsonOnValidationError( $exception->getValidationResult(), $response diff --git a/src/Application/ErrorHandler/DefaultErrorHandler.php b/src/Application/ErrorHandler/DefaultErrorHandler.php index f4d2de6e..ab425f78 100644 --- a/src/Application/ErrorHandler/DefaultErrorHandler.php +++ b/src/Application/ErrorHandler/DefaultErrorHandler.php @@ -3,7 +3,7 @@ namespace App\Application\ErrorHandler; use App\Domain\Factory\LoggerFactory; -use App\Domain\Validation\ValidationException; +use App\Domain\Validation\ValidationExceptionOld; use Fig\Http\Message\StatusCodeInterface; use Psr\Http\Message\ResponseFactoryInterface; use Psr\Http\Message\ResponseInterface; @@ -134,7 +134,7 @@ private function getHttpStatusCode(Throwable $exception): int $statusCode = (int)$exception->getCode(); } - if ($exception instanceof ValidationException) { + if ($exception instanceof ValidationExceptionOld) { $statusCode = StatusCodeInterface::STATUS_UNPROCESSABLE_ENTITY; // 422 } diff --git a/src/Application/Middleware/ErrorHandlerMiddleware.php b/src/Application/Middleware/ErrorHandlerMiddleware.php index 92abe76b..0e7cd7fb 100644 --- a/src/Application/Middleware/ErrorHandlerMiddleware.php +++ b/src/Application/Middleware/ErrorHandlerMiddleware.php @@ -65,7 +65,7 @@ function ($severity, $message, $file, $line) { $this->logger->warning( "Warning [$severity] $message on line $line in file $file" ); - } // If error is non fatal but not warning (default) + } // If error is non-fatal but not warning (default) else { $this->logger->notice( "Notice [$severity] $message on line $line in file $file" diff --git a/src/Application/Middleware/ForbiddenExceptionMiddleware.php b/src/Application/Middleware/ForbiddenExceptionMiddleware.php new file mode 100644 index 00000000..31e38fd2 --- /dev/null +++ b/src/Application/Middleware/ForbiddenExceptionMiddleware.php @@ -0,0 +1,39 @@ +handle($request); + } catch (ForbiddenException $forbiddenException) { + // Create response (status code and header are added later) + $response = $this->responder->createResponse(); + + return $this->responder->respondWithJson( + $response, + [ + 'status' => 'error', + 'message' => $forbiddenException->getMessage(), + ], + StatusCodeInterface::STATUS_FORBIDDEN + ); + } + } +} \ No newline at end of file diff --git a/src/Application/Middleware/ValidationExceptionMiddleware.php b/src/Application/Middleware/ValidationExceptionMiddleware.php new file mode 100644 index 00000000..7e04c885 --- /dev/null +++ b/src/Application/Middleware/ValidationExceptionMiddleware.php @@ -0,0 +1,103 @@ +handle($request); + } catch (ValidationException $validationException) { + // Create response (status code and header are added later) + $response = $this->responder->createResponse(); + // For an added abstraction layer, in order to not be dependent on validation library's error output format, + // transform cakephp validation errors into the format that is used in the frontend + $validationErrors = $this->transformCakephpValidationErrorsToOutputFormat( + $validationException->validationErrors + ); + $responseData = [ + 'status' => 'error', + 'message' => $validationException->getMessage(), + 'data' => ['errors' => $validationErrors], + ]; + return $this->responder->respondWithJson($response, $responseData, 422); + } + } + + /** + * Transform the validation error output from the library to array that is used by the frontend. + * The changes are tiny but the main purpose is to add an abstraction layer in case the validation + * library changes its error output format in the future so that only this function has to be + * changed instead of the frontend. + * + * In cakephp/validation 5 the error array is output in the following format: + * $cakeValidationErrors = [ + * 'field_name' => [ + * 'validation_rule_name' => 'Validation error message for that field', + * 'other_validation_rule_name' => 'Another validation error message for that field', + * ], + * 'email' => [ + * '_required' => 'This field is required', + * ], + * 'first_name' => [ + * 'minLength' => 'Minimum length is 3', + * ], + * ] + * + * This function transforms this into the format that is used by the frontend + * (which is roughly the same except we don't need the infringed rule name as key): + * $outputValidationErrors = [ + * 'field_name' => [ + * 0 => 'Validation error message for that field', + * 1 => 'Another validation error message for that field', + * ], + * 'email' => [ + * 0 => 'This field is required', + * ], + * 'first_name' => [ + * 0 => 'Minimum length is 3', + * ], + * ] + * + * Previously the output format was like this: + * [ + * 0 => [ + * 'field' => 'field_name', + * 'message' => 'Validation error message for that field', + * ], + * // ... and so on + * ] + * But this makes it unnecessarily harder to test as the order of the array elements is not + * guaranteed in the response. + * + * @param array $validationErrors The cakephp validation errors + * @return array The transformed result in the format documented above. + */ + private function transformCakephpValidationErrorsToOutputFormat(array $validationErrors): array + { + $validationErrorsForOutput = []; + foreach ($validationErrors as $fieldName => $fieldErrors) { + // There may be the case that there are multiple error messages for a single field. + foreach ($fieldErrors as $infringedRuleName => $infringedRuleMessage) { + // Output is basically the same except we don't need the infringed rule name as key + $validationErrorsForOutput[$fieldName][] = $infringedRuleMessage; + } + } + + return $validationErrorsForOutput; + } +} diff --git a/src/Application/Responder/Responder.php b/src/Application/Responder/Responder.php index 91dcf2fd..ce8d3b8c 100644 --- a/src/Application/Responder/Responder.php +++ b/src/Application/Responder/Responder.php @@ -3,7 +3,7 @@ namespace App\Application\Responder; use App\Domain\Security\Exception\SecurityException; -use App\Domain\Validation\ValidationException; +use App\Domain\Validation\ValidationExceptionOld; use App\Domain\Validation\ValidationResult; use Psr\Http\Message\ResponseFactoryInterface; use Psr\Http\Message\ResponseInterface; @@ -48,9 +48,9 @@ public function createResponse(): ResponseInterface * @param string $template Template pathname relative to templates directory * @param array $data Associative array of template variables * + * @return ResponseInterface The response * @throws \Throwable * - * @return ResponseInterface The response */ public function render( ResponseInterface $response, @@ -138,18 +138,18 @@ public function urlFor(string $routeName, array $data = [], array $queryParams = * * @param ResponseInterface $response * @param string $template - * @param ValidationException $validationException + * @param ValidationExceptionOld $validationException * @param array $queryParams same query params passed to page to be added again to form after validation error * @param array|null $preloadValues * + * @return ResponseInterface|null * @throws \Throwable * - * @return ResponseInterface|null */ public function renderOnValidationError( ResponseInterface $response, string $template, - ValidationException $validationException, + ValidationExceptionOld $validationException, array $queryParams = [], ?array $preloadValues = null, ): ?ResponseInterface { @@ -176,9 +176,9 @@ public function renderOnValidationError( * @param array|null $preloadValues * @param array $queryParams same query params passed to page to be added again to form after validation error * + * @return ResponseInterface * @throws \Throwable * - * @return ResponseInterface */ public function respondWithThrottle( ResponseInterface $response, @@ -206,9 +206,9 @@ public function respondWithThrottle( * @param array|null $preloadValues * @param array $queryParams same query params passed to page to be added again to form after validation error * + * @return ResponseInterface * @throws \Throwable * - * @return ResponseInterface */ public function respondWithFormThrottle( ResponseInterface $response, @@ -245,13 +245,22 @@ public function respondWithJson( mixed $data = null, int $status = 200 ): ResponseInterface { - $response->getBody()->write((string)json_encode($data, JSON_THROW_ON_ERROR)); + $response->getBody()->write((string)json_encode($data, JSON_UNESCAPED_SLASHES | JSON_PARTIAL_OUTPUT_ON_ERROR)); $response = $response->withStatus($status); return $response->withHeader('Content-Type', 'application/json'); } public function respondWithJsonOnValidationError( + array $validationErrors, + ResponseInterface $response + ): ?ResponseInterface { + + + + } + + public function respondWithJsonOnValidationErrorOld( ValidationResult $validationResult, ResponseInterface $response ): ?ResponseInterface { @@ -263,4 +272,5 @@ public function respondWithJsonOnValidationError( return $this->respondWithJson($response, $responseData, $validationResult->getStatusCode()); } + } diff --git a/src/Domain/Authentication/Service/PasswordRecoveryEmailSender.php b/src/Domain/Authentication/Service/PasswordRecoveryEmailSender.php index 83dc0cf2..b68b401a 100644 --- a/src/Domain/Authentication/Service/PasswordRecoveryEmailSender.php +++ b/src/Domain/Authentication/Service/PasswordRecoveryEmailSender.php @@ -8,7 +8,7 @@ use App\Domain\Settings; use App\Domain\User\Service\UserValidator; use App\Domain\Utility\Mailer; -use App\Domain\Validation\ValidationException; +use App\Domain\Validation\ValidationExceptionOld; use App\Infrastructure\User\UserFinderRepository; use Symfony\Component\Mailer\Exception\TransportExceptionInterface; use Symfony\Component\Mime\Address; @@ -56,7 +56,7 @@ public function __construct( * @param array $userValues * @param string|null|null $captcha * - * @throws ValidationException|TransportExceptionInterface|\JsonException + * @throws ValidationExceptionOld|TransportExceptionInterface|\JsonException */ public function sendPasswordRecoveryEmail(array $userValues, ?string $captcha = null): void { diff --git a/src/Domain/Client/Data/ClientData.php b/src/Domain/Client/Data/ClientData.php index bb7878a8..d6d6aca6 100644 --- a/src/Domain/Client/Data/ClientData.php +++ b/src/Domain/Client/Data/ClientData.php @@ -32,8 +32,6 @@ class ClientData implements \JsonSerializable * Client Data constructor. * * @param array|null $clientData - * - * @throws \Exception */ public function __construct(?array $clientData = []) { @@ -48,8 +46,9 @@ public function __construct(?array $clientData = []) $this->clientMessage = $clientData['client_message'] ?? null; $this->vigilanceLevel = $clientData['vigilance_level'] ?? null ? ClientVigilanceLevel::tryFrom($clientData['vigilance_level']) : null; - $this->userId = $clientData['user_id'] ?? null; - $this->clientStatusId = $clientData['client_status_id'] ?? null; + // Cast to int if user id is set and not empty (null / empty string) + $this->userId = !empty($clientData['user_id']) ? (int)$clientData['user_id'] : null; + $this->clientStatusId = !empty($clientData['client_status_id']) ? (int)$clientData['client_status_id'] : null; $this->updatedAt = $clientData['updated_at'] ?? null ? new \DateTimeImmutable($clientData['updated_at']) : null; $this->createdAt = $clientData['created_at'] ?? null ? new \DateTimeImmutable($clientData['created_at']) : null; $this->deletedAt = $clientData['deleted_at'] ?? null ? new \DateTimeImmutable($clientData['deleted_at']) : null; diff --git a/src/Domain/Client/Service/ClientCreator.php b/src/Domain/Client/Service/ClientCreator.php index 8c5de658..6430ca58 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 ClientValidatorVanilla $clientValidator, + private readonly ClientValidator $clientValidator, private readonly ClientCreatorRepository $clientCreatorRepository, private readonly ClientAuthorizationChecker $clientAuthorizationChecker, private readonly NoteCreator $noteCreator, @@ -31,14 +31,13 @@ public function __construct( * * @param array $clientValues * - * @throws ForbiddenException|\JsonException + * @throws ForbiddenException * * @return int insert id */ public function createClient(array $clientValues): int { - // Validate entire client values before object creation to prevent exception on invalid data such as datetime - $this->clientValidator->validateClientCreation($clientValues); + $this->clientValidator->validateClientValues($clientValues, true); $client = new ClientData($clientValues); diff --git a/src/Domain/Client/Service/ClientValidator.php b/src/Domain/Client/Service/ClientValidator.php index c287e870..c6c1dff0 100644 --- a/src/Domain/Client/Service/ClientValidator.php +++ b/src/Domain/Client/Service/ClientValidator.php @@ -4,9 +4,11 @@ use App\Domain\Client\Enum\ClientVigilanceLevel; use App\Domain\Factory\LoggerFactory; +use App\Domain\Validation\ValidationException; use App\Domain\Validation\ValidationResult; use App\Domain\Validation\ValidatorNative; use App\Infrastructure\Client\ClientStatus\ClientStatusFinderRepository; +use App\Infrastructure\User\UserFinderRepository; use Cake\Validation\Validator; class ClientValidator @@ -15,31 +17,47 @@ public function __construct( LoggerFactory $logger, private readonly ValidatorNative $validator, private readonly ClientStatusFinderRepository $clientStatusFinderRepository, + private readonly UserFinderRepository $userFinderRepository, ) { $logger->addFileHandler('error.log')->createLogger('post-validation'); } - public function validateClientCreation() + /** + * Validate client creation and modification. + * + * @param array $clientCreationValues user submitted values + * @param bool $isCreateMode true when fields presence is required in values. For update, they're optional + * + * @throws ValidationException + */ + public function validateClientValues(array $clientCreationValues, bool $isCreateMode = true): void { $validator = new Validator(); $validator // Require presence indicates that the key is required in the request body - ->requirePresence('first_name') + // When second parameter "mode" is false, the fields presence is not required + ->requirePresence('first_name', $isCreateMode, __('Key is required')) + // Cake validation library automatically sets a rule that field cannot be null as soon as there is any + // validation rule set for the field. This is why we have to allow empty string (null is also allowed by it) + ->allowEmptyString('first_name') ->minLength('first_name', 3, __('Minimum length is 3')) ->maxLength('first_name', 100, __('Maximum length is 100')) - ->requirePresence('last_name') + ->requirePresence('last_name', $isCreateMode, __('Key is required')) + ->allowEmptyString('last_name') ->minLength('last_name', 3, __('Minimum length is 3')) ->maxLength('last_name', 100, __('Maximum length is 100')) - ->requirePresence('email') + ->requirePresence('email', $isCreateMode, __('Key is required')) + ->allowEmptyString('email') ->email('email', false, __('Invalid e-mail')) - ->requirePresence('birthdate') - ->date('birthdate', ['ymd', 'mdy', 'dmy'], 'Invalid date value') + ->requirePresence('birthdate', $isCreateMode, __('Key is required')) + ->allowEmptyDate('birthdate') + ->date('birthdate', ['ymd', 'mdy', 'dmy'], __('Invalid date value')) ->add('birthdate', 'validateNotInFuture', [ 'rule' => function ($value, $context) { $today = new \DateTime(); $birthdate = new \DateTime($value); - + // check that birthdate is not in the future return $birthdate <= $today; }, 'message' => __('Cannot be in the future') @@ -53,33 +71,58 @@ public function validateClientCreation() }, 'message' => __('Cannot be older than 130 years') ]) - ->requirePresence('location') + ->requirePresence('location', $isCreateMode, __('Key is required')) + ->allowEmptyString('location') ->minLength('location', 2, __('Minimum length is 2')) ->maxLength('location', 100, __('Maximum length is 100')) - ->requirePresence('phone') + ->requirePresence('phone', $isCreateMode, __('Key is required')) + ->allowEmptyString('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 + // Sex requirePresence false as it's not required and the browser won't send the key over if not set + ->requirePresence('sex', false) + ->allowEmptyString('sex') ->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', [ + // requirePresence false for vigilance_level as it doesn't even exist on the create form, only possible to update + ->requirePresence('vigilance_level', false) + ->allowEmptyString('vigilance_level') + ->add('vigilance_level', 'validateBackedEnum', [ 'rule' => function ($value, $context) { - return $this->isBackedEnum($value, ClientVigilanceLevel::class, 'vigilance_level'); + return $this->isBackedEnum($value, ClientVigilanceLevel::class); }, 'message' => __('Invalid option') ]) - ->requirePresence('client_message') + // Client message presence is not required as it's only set if user submits the form via api + ->requirePresence('client_message', false) + ->allowEmptyString('client_message') ->minLength('client_message', 3, __('Minimum length is 3')) ->maxLength('client_message', 1000, __('Maximum length is 1000')) - ->requirePresence('client_status_id', true, __('Required')) + ->requirePresence('client_status_id', $isCreateMode, __('Key is required')) + ->notEmptyString('client_status_id', __('Required')) ->numeric('client_status_id', __('Invalid option format')) - ->add('client_status_id', 'valid', [ + ->add('client_status_id', 'exists', [ 'rule' => function ($value, $context) { return $this->clientStatusFinderRepository->clientStatusExists((int)$value); }, - 'message' => 'Invalid option' - ]); + 'message' => __('Invalid option') + ]) + // Presence not required as client can be created via form submit by another frontend that doesn't have user id + ->requirePresence('user_id', false) + ->allowEmptyString('user_id', __('Required')) + ->numeric('user_id', __('Invalid option format')) + ->add('user_id', 'exists', [ + 'rule' => function ($value, $context) { + return !empty($this->userFinderRepository->findUserById((int)$value)); + }, + 'message' => __('Invalid option') + ]) + ; + + $errors = $validator->validate($clientCreationValues); + if ($errors) { + throw new ValidationException($errors); + } } /** @@ -96,53 +139,6 @@ public function isBackedEnum(\BackedEnum|string|null $value, string $enum): bool 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 - * 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 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'); - - $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. * diff --git a/src/Domain/Note/Service/NoteCreator.php b/src/Domain/Note/Service/NoteCreator.php index 3a39e306..df6b09dd 100644 --- a/src/Domain/Note/Service/NoteCreator.php +++ b/src/Domain/Note/Service/NoteCreator.php @@ -5,7 +5,6 @@ use App\Domain\Authentication\Exception\ForbiddenException; use App\Domain\Factory\LoggerFactory; use App\Domain\Note\Authorization\NoteAuthorizationChecker; -use App\Domain\Note\Data\NoteData; use App\Domain\User\Enum\UserActivity; use App\Domain\User\Service\UserActivityManager; use App\Infrastructure\Note\NoteCreatorRepository; @@ -33,12 +32,12 @@ public function __construct( public function createNote(array $noteValues): int { if (($loggedInUserId = $this->session->get('user_id')) !== null) { - $note = new NoteData($noteValues); - $note->userId = $loggedInUserId; - $this->noteValidator->validateNoteCreation($note); + $noteValues['user_id'] = $loggedInUserId; + // Throws exception if validation fails + $this->noteValidator->validateNoteCreation($noteValues); if ($this->noteAuthorizationChecker->isGrantedToCreate((int)$noteValues['is_main'])) { - $noteId = $this->noteCreatorRepository->insertNote($note->toArray()); + $noteId = $this->noteCreatorRepository->insertNote($noteValues); if (!empty($noteId)) { $this->userActivityManager->addUserActivity( UserActivity::CREATED, diff --git a/src/Domain/Note/Service/NoteValidator.php b/src/Domain/Note/Service/NoteValidator.php index f52d6d79..31fe4254 100644 --- a/src/Domain/Note/Service/NoteValidator.php +++ b/src/Domain/Note/Service/NoteValidator.php @@ -5,9 +5,11 @@ use App\Domain\Factory\LoggerFactory; use App\Domain\Note\Data\NoteData; use App\Domain\Validation\ValidationException; +use App\Domain\Validation\ValidationExceptionOld; use App\Domain\Validation\ValidationResult; use App\Domain\Validation\ValidatorNative; use App\Infrastructure\Note\NoteValidatorRepository; +use Cake\Validation\Validator; use Psr\Log\LoggerInterface; /** @@ -21,12 +23,12 @@ class NoteValidator * NoteValidator constructor. * * @param NoteValidatorRepository $noteValidatorRepository - * @param ValidatorNative $validator + * @param ValidatorNative $validatorNative * @param LoggerFactory $loggerFactory */ public function __construct( private readonly NoteValidatorRepository $noteValidatorRepository, - private readonly ValidatorNative $validator, + private readonly ValidatorNative $validatorNative, private readonly LoggerFactory $loggerFactory, ) { $this->logger = $this->loggerFactory->addFileHandler('error.log')->createLogger('note-validation'); @@ -35,11 +37,39 @@ public function __construct( /** * Validate note creation. * - * @param NoteData $note - * - * @throws ValidationException + * @param array $noteValues */ - public function validateNoteCreation(NoteData $note): void + public function validateNoteCreation(array $noteValues): void + { + $validator = new Validator(); + $validator = $validator->requirePresence('message') + ->maxLength('message', 1000, __('Maximum length is 1000', 1000)); + + if ((int)$noteValues['is_main'] === 1) { + // If main note, the min length can be 0 as we can't delete it + $validator + ->add('is_main', 'mainNoteAlreadyExists', [ + 'rule' => function ($value, $context) { + $clientId = $context['data']['client_id']; + // Log error as this should not be possible if frontend behaves correctly + $this->logger->error('Attempt to create main note but it already exists. Client: ' . $clientId); + return $this->noteValidatorRepository->mainNoteAlreadyExistsForClient($clientId) === false; + }, + 'message' => __('Main note already exists'), + ]); + } else { + // If not main note, min length is 4 + $validator + ->minLength('message', 4, __('Minimum length is 4', 4)); + } + + $errors = $validator->validate($noteValues); + if ($errors) { + throw new ValidationException($errors); + } + } + + public function validateNoteCreationOld(NoteData $note): void { // Validation error message asserted via NoteProvider.php $validationResult = new ValidationResult('There is something in the note data that couldn\'t be validated'); @@ -51,9 +81,9 @@ public function validateNoteCreation(NoteData $note): void $this->validateNoteMessage($note->message, $validationResult, false); } // It's a bit pointless to check user existence as user should always exist if he's logged in but here is how I'd do it - $this->validator->validateExistence($note->userId, 'user', $validationResult, true); + $this->validatorNative->validateExistence($note->userId, 'user', $validationResult, true); - $this->validator->throwOnError($validationResult); + $this->validatorNative->throwOnError($validationResult); } /** @@ -61,7 +91,7 @@ public function validateNoteCreation(NoteData $note): void * * @param NoteData $note * - * @throws ValidationException + * @throws ValidationExceptionOld */ public function validateNoteUpdate(NoteData $note): void { @@ -78,10 +108,10 @@ public function validateNoteUpdate(NoteData $note): void } if (null !== $note->hidden) { // Has to be either 0 or 1 - $this->validator->validateNumeric($note->hidden, 'hidden', $validationResult); + $this->validatorNative->validateNumeric($note->hidden, 'hidden', $validationResult); } - $this->validator->throwOnError($validationResult); + $this->validatorNative->throwOnError($validationResult); } /** @@ -103,8 +133,8 @@ protected function validateNoteMessage( ): void { // Not test if empty string as user could submit note with empty string which has to be checked if (null !== $noteMsg) { - $this->validator->validateLengthMax($noteMsg, 'message', $validationResult, 1000); - $this->validator->validateLengthMin($noteMsg, 'message', $validationResult, $minLength); + $this->validatorNative->validateLengthMax($noteMsg, 'message', $validationResult, 1000); + $this->validatorNative->validateLengthMin($noteMsg, 'message', $validationResult, $minLength); } elseif (true === $required) { // If it is null or empty string and required $validationResult->setError('message', __('Required')); diff --git a/src/Domain/User/Service/UserValidator.php b/src/Domain/User/Service/UserValidator.php index 91e0272c..69cfe528 100644 --- a/src/Domain/User/Service/UserValidator.php +++ b/src/Domain/User/Service/UserValidator.php @@ -6,7 +6,7 @@ use App\Domain\User\Enum\UserLang; use App\Domain\User\Enum\UserStatus; use App\Domain\User\Enum\UserTheme; -use App\Domain\Validation\ValidationException; +use App\Domain\Validation\ValidationExceptionOld; use App\Domain\Validation\ValidationResult; use App\Domain\Validation\ValidatorNative; use App\Infrastructure\User\UserFinderRepository; @@ -87,9 +87,9 @@ public function validateUserUpdate(int $userId, array $userValues): ValidationRe * * @param UserData $user * - * @throws ValidationException|\JsonException - * * @return ValidationResult + *@throws ValidationExceptionOld|\JsonException + * */ public function validateUserCreation(UserData $user): ValidationResult { @@ -124,9 +124,9 @@ public function validateUserCreation(UserData $user): ValidationResult * * @param array{email: string|null, password: string|null} $userLoginValues * - * @throws ValidationException - * * @return ValidationResult + *@throws ValidationExceptionOld + * */ public function validateUserLogin(array $userLoginValues): ValidationResult { diff --git a/src/Domain/Validation/ValidationException.php b/src/Domain/Validation/ValidationException.php index 2373efc8..4750a5f1 100644 --- a/src/Domain/Validation/ValidationException.php +++ b/src/Domain/Validation/ValidationException.php @@ -9,23 +9,12 @@ */ class ValidationException extends RuntimeException { - /** - * ValidationException constructor. - * - * @param ValidationResult $validationResult - */ - public function __construct(private readonly ValidationResult $validationResult) - { - parent::__construct($validationResult->getMessage()); - } + public array $validationErrors = []; - /** - * Get the validation result. - * - * @return ValidationResult - */ - public function getValidationResult(): ValidationResult + public function __construct(array $validationErrors, string $message = 'Validation error') { - return $this->validationResult; + parent::__construct($message); + + $this->validationErrors = $validationErrors; } } diff --git a/src/Domain/Validation/ValidationExceptionOld.php b/src/Domain/Validation/ValidationExceptionOld.php new file mode 100644 index 00000000..93b34d4b --- /dev/null +++ b/src/Domain/Validation/ValidationExceptionOld.php @@ -0,0 +1,31 @@ +getMessage()); + } + + /** + * Get the validation result. + * + * @return ValidationResult + */ + public function getValidationResult(): ValidationResult + { + return $this->validationResult; + } +} diff --git a/src/Domain/Validation/ValidatorNative.php b/src/Domain/Validation/ValidatorNative.php index bedfa620..eef2b074 100644 --- a/src/Domain/Validation/ValidatorNative.php +++ b/src/Domain/Validation/ValidatorNative.php @@ -35,7 +35,7 @@ public function __construct( * * @param ValidationResult $validationResult * - * @throws ValidationException + * @throws ValidationExceptionOld */ public function throwOnError(ValidationResult $validationResult): void { @@ -46,7 +46,7 @@ public function throwOnError(ValidationResult $validationResult): void JSON_UNESCAPED_SLASHES | JSON_PARTIAL_OUTPUT_ON_ERROR ) ); - throw new ValidationException($validationResult); + throw new ValidationExceptionOld($validationResult); } } diff --git a/tests/Integration/Client/ClientCreateActionTest.php b/tests/Integration/Client/ClientCreateActionTest.php index 30c28062..26230b8f 100644 --- a/tests/Integration/Client/ClientCreateActionTest.php +++ b/tests/Integration/Client/ClientCreateActionTest.php @@ -19,7 +19,6 @@ use Selective\TestTrait\Traits\HttpJsonTestTrait; use Selective\TestTrait\Traits\HttpTestTrait; use Selective\TestTrait\Traits\RouteTestTrait; -use Slim\Exception\HttpBadRequestException; /** * Client creation submit tests @@ -41,15 +40,15 @@ class ClientCreateActionTest extends TestCase /** * Client creation with valid data. * - * @dataProvider \App\Test\Provider\Client\ClientCreateProvider::clientCreationUsersAndExpectedResultProvider() + * @dataProvider \App\Test\Provider\Client\ClientCreateProvider::clientCreationAuthorizationProvider() * * @param array|null $userLinkedToClientRow client owner attributes containing the user_role_id or null if none * @param array $authenticatedUserRow authenticated user attributes containing the user_role_id * @param array $expectedResult HTTP status code, bool if db_entry_created and json_response * + * @return void * @throws \JsonException|ContainerExceptionInterface|NotFoundExceptionInterface * - * @return void */ public function testClientSubmitCreateActionAuthorization( ?array $userLinkedToClientRow, @@ -70,8 +69,6 @@ public function testClientSubmitCreateActionAuthorization( 'phone' => '+41 77 222 22 22', 'email' => 'new-user@email.com', 'sex' => 'M', - 'client_message' => null, - 'vigilance_level' => null, 'user_id' => $userLinkedToClientRow['id'], 'client_status_id' => $clientStatusId, 'message' => 'Test main note.', @@ -86,7 +83,8 @@ public function testClientSubmitCreateActionAuthorization( $clientCreationValues ); $response = $this->app->handle($request); - // Assert response status code: 201 Created + + // Assert response status code: 201 Created or 403 Forbidden self::assertSame($expectedResult[StatusCodeInterface::class], $response->getStatusCode()); // If db record is expected to be created assert that @@ -107,18 +105,25 @@ public function testClientSubmitCreateActionAuthorization( $noteId = $this->findLastInsertedTableRow('note')['id']; $this->assertTableRowEquals($noteValues, 'note', $noteId); - // Assert user activity - // Add client_message to creation values as they are inserted in user_activity + // Assert user activity database row + $userActivityRow = $this->findTableRowsByColumn('user_activity', 'table', 'client')[0]; + // Assert user activity row without json data $this->assertTableRowEquals( - [ - 'action' => UserActivity::CREATED->value, - 'table' => 'client', - 'row_id' => $clientDbRow['id'], - 'data' => json_encode($clientCreationValues, JSON_THROW_ON_ERROR), - ], + ['action' => UserActivity::CREATED->value, 'table' => 'client', 'row_id' => $clientDbRow['id'],], 'user_activity', - (int)$this->findTableRowsByColumn('user_activity', 'table', 'client')[0]['id'] + $userActivityRow['id'] + ); + // Assert relevant user activity data + $decodedUserActivityDataFromDb = json_decode($userActivityRow['data'], true, 512, JSON_THROW_ON_ERROR); + // Done separately as we only want to test the relevant data for the creation, and we cannot control the order + self::assertEqualsCanonicalizing( + $clientCreationValues, + // We only want to test if the keys present in $clientCreationValues are in the decoded data from the + // userActivity database row thus removing any keys that are not present in $clientCreationValues + // with array_intersect_key. + array_intersect_key($decodedUserActivityDataFromDb, $clientCreationValues) ); + // Note user activity entry // Add other note values $noteValues['client_id'] = $clientDbRow['id']; @@ -144,16 +149,17 @@ public function testClientSubmitCreateActionAuthorization( } /** - * Test client creation values validation. + * Test validation errors when user submits values that are invalid and when client + * doesn't send the required keys (previously done via malformed body checker). * - * @dataProvider \App\Test\Provider\Client\ClientCreateProvider::invalidClientCreationValuesAndExpectedResponseProvider() + * @dataProvider \App\Test\Provider\Client\ClientCreateProvider::invalidClientCreationProvider() * * @param array $requestBody * @param array $jsonResponse * + * @return void * @throws ContainerExceptionInterface|NotFoundExceptionInterface * - * @return void */ public function testClientSubmitCreateActionInvalid(array $requestBody, array $jsonResponse): void { @@ -188,6 +194,58 @@ public function testClientSubmitCreateActionInvalid(array $requestBody, array $j $this->assertJsonData($jsonResponse, $response); } + /** + * Tests that client creation is possible with only the required values set and the other + * set to null or an empty string. + * + * The reason for this test is that cakephp validation library treats null values + * as invalid when a validation method is set on a field. + * E.g. ->maxLength('first_name', 100) has the consequence that it expects + * a non-null value for the first_name. Without ->allowEmptyString('first_name') + * the validation would fail with "This field cannot be left empty". + * I did not expect this behaviour and ran into this when testing in the GUI so this test + * makes sense to me in order to not forget to always add ->allow[Whatever] when value is optional. + * + * @dataProvider \App\Test\Provider\Client\ClientCreateProvider::validClientCreationProvider() + * + * @param array $requestBody + * + * @return void + * @throws ContainerExceptionInterface|NotFoundExceptionInterface + */ + public function testClientSubmitCreateActionValid(array $requestBody): void + { + // Insert managing advisor user which is allowed to create clients + $userId = $this->insertFixturesWithAttributes( + $this->addUserRoleId(['user_role_id' => UserRole::MANAGING_ADVISOR]), + UserFixture::class + )['id']; + // Insert mandatory field client status id + $clientStatusId = $this->insertFixturesWithAttributes([], ClientStatusFixture::class)['id']; + // Add valid client status id to request body + $requestBody['client_status_id'] = $clientStatusId; + + // Simulate session + $this->container->get(SessionInterface::class)->set('user_id', $userId); + + $request = $this->createJsonRequest( + 'POST', + $this->urlFor('client-create-submit'), + $requestBody + ); + + $response = $this->app->handle($request); + + // Assert 201 Created + self::assertSame(StatusCodeInterface::STATUS_CREATED, $response->getStatusCode()); + + // No client should have been created + $this->assertTableRowCount(1, 'client'); + + $this->assertJsonData(['status' => 'success', 'data' => null,], $response); + } + + /** * Client creation with valid data. * @@ -213,37 +271,4 @@ public function testClientSubmitCreateActionUnauthenticated(): void // Assert that response contains correct login url $this->assertJsonData(['loginUrl' => $expectedLoginUrl], $response); } - - /** - * Test client creation with malformed request body. - * - * @dataProvider \App\Test\Provider\Client\ClientCreateProvider::malformedRequestBodyCases() - * - * @param array $requestBody - * - * @throws ContainerExceptionInterface|NotFoundExceptionInterface - * - * @return void - */ - public function testClientSubmitCreateActionMalformedRequestBody(array $requestBody): void - { - // Action class should directly return error so only logged-in user has to be inserted - $userData = $this->insertFixturesWithAttributes([], UserFixture::class); - - // Simulate logged-in user with same user as linked to client - $this->container->get(SessionInterface::class)->set('user_id', $userData['id']); - - $request = $this->createJsonRequest( - 'POST', - $this->urlFor('client-create-submit'), - $requestBody - ); - - // Bad Request (400) means that the client sent the request wrongly; it's a frontend error - $this->expectException(HttpBadRequestException::class); - $this->expectExceptionMessage('Request body malformed.'); - - // Handle request after defining expected exceptions - $this->app->handle($request); - } } diff --git a/tests/Provider/Client/ClientCreateProvider.php b/tests/Provider/Client/ClientCreateProvider.php index abedc5da..144d9def 100644 --- a/tests/Provider/Client/ClientCreateProvider.php +++ b/tests/Provider/Client/ClientCreateProvider.php @@ -10,46 +10,6 @@ class ClientCreateProvider { use FixtureTestTrait; - /** - * Provide malformed request body for client creation. - * - * @return array[] - */ - public static function malformedRequestBodyCases(): array - { - return [ - [ - // If any of the list except client_message is missing it's a bad request - 'missing_first_name' => [ - 'last_name' => 'value', - 'birthdate' => 'value', - 'location' => 'value', - 'phone' => 'value', - 'email' => 'value', - 'sex' => 'value', - 'client_message' => 'value', - 'user_id' => 'value', - 'client_status_id' => 'value', - 'message' => 'value', // main note - ], - 'key_too_much_without_client_message' => [ - 'first_name' => 'value', - 'last_name' => 'value', - 'birthdate' => 'value', - 'location' => 'value', - 'phone' => 'value', - 'email' => 'value', - 'sex' => 'value', - // 'client_message' => 'value', - 'user_id' => 'value', - 'client_status_id' => 'value', - 'message' => 'value', - 'key_too_much' => 'value', - ], - ], - ]; - } - /** * @return \array[][] */ @@ -92,7 +52,7 @@ public static function clientCreationDropdownOptionsCases(): array * * @return array[] */ - public static function clientCreationUsersAndExpectedResultProvider(): array + public static function clientCreationAuthorizationProvider(): array { // Get users with different roles $managingAdvisorAttributes = ['user_role_id' => UserRole::MANAGING_ADVISOR]; @@ -153,72 +113,64 @@ public static function clientCreationUsersAndExpectedResultProvider(): array * * @return array */ - public static function invalidClientCreationValuesAndExpectedResponseProvider(): array + public static function invalidClientCreationProvider(): array { // Including as many values as possible that trigger validation errors in each case return [ [ - // Most values too short + // Most values too short, birthdate too old and user_id has 2 validation error messages 'request_body' => [ 'first_name' => 'T', 'last_name' => 'A', - 'birthdate' => '1850-01-01', // too old + 'birthdate' => '1850-01-01', // over 130 years old 'location' => 'L', 'phone' => '07', 'email' => 'test@test', // missing extension 'sex' => 'A', // invalid value - 'user_id' => '999', // non-existing user - 'client_status_id' => '999', // non-existing status + 'user_id' => 'a', // wrong format and non-existing user + 'client_status_id' => 'a', // wrong format and non-existing status 'message' => '', // valid vor now as note validation is done only if all client values are valid ], 'json_response' => [ 'status' => 'error', 'message' => 'Validation error', 'data' => [ - 'message' => 'There is something in the client data that couldn\'t be validated', 'errors' => [ - 0 => [ - 'field' => 'client_status_id', - 'message' => 'Invalid option', + 'first_name' => [ + 0 => 'Minimum length is 3', ], - 1 => [ - 'field' => 'user_id', - 'message' => 'Invalid option', + 'last_name' => [ + 0 => 'Minimum length is 3', ], - 2 => [ - 'field' => 'first_name', - 'message' => 'Minimum length is 2', + 'email' => [ + 0 => 'Invalid e-mail', ], - 3 => [ - 'field' => 'last_name', - 'message' => 'Minimum length is 2', + 'birthdate' => [ + 0 => 'Cannot be older than 130 years', ], - 4 => [ - 'field' => 'email', - 'message' => 'Invalid value', + 'location' => [ + 0 => 'Minimum length is 2', ], - 5 => [ - 'field' => 'birthdate', - 'message' => 'Invalid value', + 'phone' => [ + 0 => 'Minimum length is 3', ], - 6 => [ - 'field' => 'location', - 'message' => 'Minimum length is 2', + 'sex' => [ + 0 => 'Invalid option', ], - 7 => [ - 'field' => 'phone', - 'message' => 'Minimum length is 3', + 'client_status_id' => [ + 0 => 'Invalid option format', + 1 => 'Invalid option', ], - 8 => [ - 'field' => 'sex', - 'message' => 'Invalid option', + 'user_id' => [ + 0 => 'Invalid option format', + 1 => 'Invalid option', ], ], ], ], ], [ - // Most values too long + // Most values too long, birthdate in the future 'request_body' => [ 'first_name' => str_repeat('i', 101), // 101 chars 'last_name' => str_repeat('i', 101), @@ -237,41 +189,33 @@ public static function invalidClientCreationValuesAndExpectedResponseProvider(): 'status' => 'error', 'message' => 'Validation error', 'data' => [ - 'message' => 'There is something in the client data that couldn\'t be validated', 'errors' => [ - 0 => [ - 'field' => 'client_status_id', - 'message' => 'Invalid option', + 'first_name' => [ + 0 => 'Maximum length is 100', ], - 1 => [ - 'field' => 'user_id', - 'message' => 'Invalid option', + 'last_name' => [ + 0 => 'Maximum length is 100', ], - 2 => [ - 'field' => 'first_name', - 'message' => 'Maximum length is 100', + 'birthdate' => [ + 0 => 'Cannot be in the future', ], - 3 => [ - 'field' => 'last_name', - 'message' => 'Maximum length is 100', + 'location' => [ + 0 => 'Maximum length is 100', ], - 4 => [ - 'field' => 'birthdate', - 'message' => 'Invalid value', + 'phone' => [ + 0 => 'Maximum length is 20', ], - 5 => [ - 'field' => 'location', - 'message' => 'Maximum length is 100', + 'client_status_id' => [ + 0 => 'Invalid option', ], - 6 => [ - 'field' => 'phone', - 'message' => 'Maximum length is 20', + 'user_id' => [ + 0 => 'Invalid option', ], - ], + ] ], ], ], - [ // Main note validation + [ // Main note validation when user creates a new client directly with a main note // All client values valid but not main note message 'request_body' => [ 'first_name' => 'Test', @@ -289,16 +233,97 @@ public static function invalidClientCreationValuesAndExpectedResponseProvider(): 'status' => 'error', 'message' => 'Validation error', 'data' => [ - 'message' => 'There is something in the note data that couldn\'t be validated', 'errors' => [ - 0 => [ - 'field' => 'message', - 'message' => 'Maximum length is 1000', + 'message' => [ + 0 => 'Maximum length is 1000', ], ], ], ], ], + [ // Keys missing, check for request body key presence (previously done via malformedBodyRequestChecker) + // Empty request body + 'request_body' => [ + ], + 'json_response' => [ + 'status' => 'error', + 'message' => 'Validation error', + 'data' => [ + 'errors' => [ + 'first_name' => [ + 0 => 'Key is required', + ], + 'last_name' => [ + 0 => 'Key is required', + ], + 'email' => [ + 0 => 'Key is required', + ], + 'birthdate' => [ + 0 => 'Key is required', + ], + 'location' => [ + 0 => 'Key is required', + ], + 'phone' => [ + 0 => 'Key is required', + ], + 'client_status_id' => [ + 0 => 'Key is required', + ], + ], + ], + ], + ], + ]; + } + + /** + * Returns combinations of valid client creation data to assert that + * validation doesn't fail. + * The reason for this test is that cakephp validation library treats null values + * as invalid when a validation method is set on a field. + * E.g. ->maxLength('first_name', 100) has the consequence that it expects + * a non-null value for the first_name. Without ->allowEmptyString('first_name') + * the validation would fail with "This field cannot be left empty". + * I did not expect this behaviour and ran into this when testing in the GUI so this test + * makes sense to me in order to not forget to always add ->allow[Whatever] when value is optional. + * + * @return array + */ + public static function validClientCreationProvider(): array + { + return [ + [ + // Test with null values on all optional fields (either first_name or last_name has to be set) + 'request_body' => [ + 'first_name' => 'First name', + 'last_name' => null, + 'birthdate' => null, + 'location' => null, + 'phone' => null, + 'email' => null, + 'sex' => null, + 'user_id' => null, + 'client_status_id' => 'valid', // 'valid' replaced by inserted client status id in test function + 'message' => null, + ], + ], + [ + // Test with empty string values on all optional fields (either first_name or last_name has to be set) + 'request_body' => [ + 'first_name' => '', + 'last_name' => '', + 'birthdate' => '', + 'location' => '', + 'phone' => '', + 'email' => '', + 'sex' => '', + 'user_id' => '', + 'client_status_id' => 'valid', // 'valid' replaced by inserted client status id in test function + 'message' => '', + ], + ], ]; } }