From 2a63e66b8c043e316f3ae31eee360fd892542cfa Mon Sep 17 00:00:00 2001 From: Steve Boyd Date: Thu, 16 Jan 2025 18:04:35 +1300 Subject: [PATCH] NEW Show more information in ValidationException message --- src/Core/Validation/ConstraintValidator.php | 14 +- src/Core/Validation/ValidationException.php | 29 ++ src/Core/Validation/ValidationResult.php | 264 +++++++++++++----- src/Forms/Form.php | 47 ++-- src/Forms/Validation/Validator.php | 2 +- src/ORM/DataObject.php | 2 +- src/Security/Member.php | 8 +- .../Validation/ValidationExceptionTest.php | 14 +- .../Core/Validation/ValidationResultTest.php | 55 ++-- 9 files changed, 310 insertions(+), 125 deletions(-) diff --git a/src/Core/Validation/ConstraintValidator.php b/src/Core/Validation/ConstraintValidator.php index 9a65689986d..2d5d16a2bc4 100644 --- a/src/Core/Validation/ConstraintValidator.php +++ b/src/Core/Validation/ConstraintValidator.php @@ -17,8 +17,9 @@ class ConstraintValidator /** * Validate a value by a constraint * + * @param $value The value to validate * @param Constraint|Constraint[] $constraints a constraint or array of constraints to validate against - * @param string $fieldName The field name the value relates to, if relevant + * @param $fieldName The field name the value relates to, if relevant */ public static function validate(mixed $value, Constraint|array $constraints, string $fieldName = ''): ValidationResult { @@ -35,9 +36,16 @@ public static function validate(mixed $value, Constraint|array $constraints, str /** @var ConstraintViolationInterface $violation */ foreach ($violations as $violation) { if ($fieldName) { - $result->addFieldError($fieldName, $violation->getMessage()); + $result->addFieldError( + $fieldName, + $violation->getMessage(), + value: $value, + ); } else { - $result->addError($violation->getMessage()); + $result->addError( + $violation->getMessage(), + value: $value, + ); } } diff --git a/src/Core/Validation/ValidationException.php b/src/Core/Validation/ValidationException.php index 155da158819..b5fc5d4c76b 100644 --- a/src/Core/Validation/ValidationException.php +++ b/src/Core/Validation/ValidationException.php @@ -5,6 +5,10 @@ use Exception; use InvalidArgumentException; use SilverStripe\Core\Injector\Injectable; +use SilverStripe\Control\Director; +use SilverStripe\Dev\DevelopmentAdmin; +use SilverStripe\ORM\DataObject; +use SilverStripe\Control\Controller; /** * Exception thrown by {@link DataObject}::write if validation fails. By throwing an @@ -48,6 +52,14 @@ public function __construct($result = null, $code = 0) // Pick first message foreach ($result->getMessages() as $message) { $exceptionMessage = $message['message']; + if ($this->getShouldShowAdditionalInfo()) { + $exceptionMessage .= ' - fieldName: ' . $message['fieldName']; + $exceptionMessage .= ', value: ' . (string) $message['value']; + if (is_subclass_of($result->getDataClass(), DataObject::class, true)) { + $exceptionMessage .= ', recordID: ' . $result->getRecordID(); + $exceptionMessage .= ', dataClass: ' . $result->getDataClass(); + } + } break; } } elseif (is_string($result)) { @@ -71,4 +83,21 @@ public function getResult() { return $this->result; } + + /** + * Whether to show additional information in the error message depending on the context + * + * We do not want this to show this in a context where it's easy to know the record and value that + * triggered the error e.g. form submission, API calls, etc + */ + private function getShouldShowAdditionalInfo() + { + if (Director::is_cli()) { + return true; + } + // e.g. dev/build + if (is_a(Controller::curr(), DevelopmentAdmin::class)) { + return true; + } + } } diff --git a/src/Core/Validation/ValidationResult.php b/src/Core/Validation/ValidationResult.php index 552dc391bd4..46bc864d994 100644 --- a/src/Core/Validation/ValidationResult.php +++ b/src/Core/Validation/ValidationResult.php @@ -4,6 +4,7 @@ use InvalidArgumentException; use SilverStripe\Core\Injector\Injectable; +use Stringable; /** * A class that combined as a boolean result with an optional list of error messages. @@ -49,130 +50,195 @@ class ValidationResult /** * Is the result valid or not. * Note that there can be non-error messages in the list. - * - * @var bool */ - protected $isValid = true; + protected bool $isValid = true; /** * List of messages - * - * @var array */ - protected $messages = []; + protected array $messages = []; + + /** + * The class of the DataObject being validated. + */ + private string $dataClass = ''; + + /** + * The record ID of the object being validated. + */ + private int $recordID = 0; + + public function __construct(string $dataClass = '', int $recordID = 0) + { + $this->dataClass = $dataClass; + $this->recordID = $recordID; + } + + public function getDataClass(): string + { + return $this->dataClass; + } + + public function setDataClass(string $dataClass): static + { + $this->dataClass = $dataClass; + return $this; + } + + public function getRecordID(): int + { + return $this->recordID; + } + + public function setRecordID(int $recordID): static + { + $this->recordID = $recordID; + return $this; + } /** * Record an error against this validation result, * - * @param string $message The message string. - * @param string $messageType Passed as a CSS class to the form, so other values can be used if desired. - * Standard types are defined by the TYPE_ constant definitions. - * @param string $code A codename for this error. Only one message per codename will be added. - * This can be usedful for ensuring no duplicate messages - * @param string|bool $cast Cast type; One of the CAST_ constant definitions. - * Bool values will be treated as plain text flag. + * @param $message The message string. + * @param $messageType The type of message: e.g. "bad", "warning", "good", or "required" + * Passed as a CSS class to the form, so other values can be used if desired + * @param $code A codename for this error. Only one message per codename will be added. + * This can be usedful for ensuring no duplicate messages + * @param $cast Cast type; One of the CAST_ constant definitions. + * @param $value The value that caused the error. This may show in the message so be careful + * and DO NOT pass in if the value is sensitive data such as a password * @return $this */ public function addError( - $message, - $messageType = ValidationResult::TYPE_ERROR, - $code = null, - $cast = ValidationResult::CAST_TEXT, - ) { - return $this->addFieldError(null, $message, $messageType, $code, $cast); + string $message, + string $messageType = ValidationResult::TYPE_ERROR, + string $code = '', + string $cast = ValidationResult::CAST_TEXT, + mixed $value = null, + ): static { + return $this->addFieldError( + '', + $message, + $messageType, + $code, + $cast, + $value, + ); } /** * Record an error against this validation result, * - * @param string $fieldName The field to link the message to. If omitted; a form-wide message is assumed. - * @param string $message The message string. - * @param string $messageType The type of message: e.g. "bad", "warning", "good", or "required". Passed as a CSS - * class to the form, so other values can be used if desired. - * @param string $code A codename for this error. Only one message per codename will be added. - * This can be usedful for ensuring no duplicate messages - * @param string|bool $cast Cast type; One of the CAST_ constant definitions. - * Bool values will be treated as plain text flag. + * @param $fieldName The field to link the message to. If omitted; a form-wide message is assumed. + * @param $message The message string. + * @param $messageType The type of message: e.g. "bad", "warning", "good", or "required" + * Passed as a CSS class to the form, so other values can be used if desired + * @param $code A codename for this error. Only one message per codename will be added. + * This can be usedful for ensuring no duplicate messages + * @param $cast Cast type; One of the CAST_ constant definitions. + * @param $value The value that caused the error. This may show in the message so be careful + * and DO NOT pass in if the value is sensitive data such as a password * @return $this */ public function addFieldError( - $fieldName, - $message, - $messageType = ValidationResult::TYPE_ERROR, - $code = null, - $cast = ValidationResult::CAST_TEXT, - ) { + string $fieldName, + string $message, + string $messageType = ValidationResult::TYPE_ERROR, + string $code = '', + string $cast = ValidationResult::CAST_TEXT, + mixed $value = null, + ): static { $this->isValid = false; - return $this->addFieldMessage($fieldName, $message, $messageType, $code, $cast); + return $this->addFieldMessage( + $fieldName, + $message, + $messageType, + $code, + $cast, + $value, + ); } /** * Add a message to this ValidationResult without necessarily marking it as an error * - * @param string $message The message string. - * @param string $messageType The type of message: e.g. "bad", "warning", "good", or "required". Passed as a CSS - * class to the form, so other values can be used if desired. - * @param string $code A codename for this error. Only one message per codename will be added. - * This can be usedful for ensuring no duplicate messages - * @param string|bool $cast Cast type; One of the CAST_ constant definitions. - * Bool values will be treated as plain text flag. + * @param $message The message string. + * @param $messageType The type of message: e.g. "bad", "warning", "good", or "required" + * Passed as a CSS class to the form, so other values can be used if desired + * @param $code A codename for this error. Only one message per codename will be added. + * This can be usedful for ensuring no duplicate messages + * @param $cast Cast type; One of the CAST_ constant definitions. + * @param $value The value that caused the error. This may show in the message so be careful + * and DO NOT pass in if the value is sensitive data such as a password * @return $this */ public function addMessage( - $message, - $messageType = ValidationResult::TYPE_ERROR, - $code = null, - $cast = ValidationResult::CAST_TEXT, - ) { - return $this->addFieldMessage(null, $message, $messageType, $code, $cast); + string $message, + string $messageType = ValidationResult::TYPE_ERROR, + string $code = '', + string $cast = ValidationResult::CAST_TEXT, + mixed $value = null, + ): static { + return $this->addFieldMessage( + '', + $message, + $messageType, + $code, + $cast, + $value, + ); } /** * Add a message to this ValidationResult without necessarily marking it as an error * - * @param string $fieldName The field to link the message to. If omitted; a form-wide message is assumed. - * @param string $message The message string. - * @param string $messageType The type of message: e.g. "bad", "warning", "good", or "required". Passed as a CSS - * class to the form, so other values can be used if desired. - * @param string $code A codename for this error. Only one message per codename will be added. - * This can be usedful for ensuring no duplicate messages - * @param string|bool $cast Cast type; One of the CAST_ constant definitions. - * Bool values will be treated as plain text flag. + * @param $fieldName The field to link the message to. If omitted; a form-wide message is assumed. + * @param $message The message string. + * @param $messageType The type of message: e.g. "bad", "warning", "good", or "required" + * Passed as a CSS class to the form, so other values can be used if desired + * @param $code A codename for this error. Only one message per codename will be added. + * This can be usedful for ensuring no duplicate messages + * @param $cast Cast type; One of the CAST_ constant definitions. + * @param $value The value that caused the error. This may show in the message so be careful + * and DO NOT pass in if the value is sensitive data such as a password * @return $this */ public function addFieldMessage( - $fieldName, - $message, - $messageType = ValidationResult::TYPE_ERROR, - $code = null, - $cast = ValidationResult::CAST_TEXT, - ) { + string $fieldName, + string $message, + string $messageType = ValidationResult::TYPE_ERROR, + string $code = '', + string $cast = ValidationResult::CAST_TEXT, + mixed $value = null, + ): static { if ($code && is_numeric($code)) { - throw new InvalidArgumentException("Don't use a numeric code '$code'. Use a string."); + throw new InvalidArgumentException("Don't use a numeric code '$code'. Use a string."); } - if (is_bool($cast)) { - $cast = $cast ? ValidationResult::CAST_TEXT : ValidationResult::CAST_HTML; + $value = $this->ensureValueCanBeCastToString($value); + if ($this->shouldScrubSensitiveValue($fieldName)) { + $value = '*****'; } $metadata = [ 'message' => $message, 'fieldName' => $fieldName, 'messageType' => $messageType, 'messageCast' => $cast, + 'value' => $value, + 'dataClass' => $this->dataClass, + 'recordID' => $this->recordID, ]; if ($code) { $this->messages[$code] = $metadata; } else { $this->messages[] = $metadata; } - return $this; } /** * Returns true if the result is valid. - * @return boolean */ - public function isValid() + public function isValid(): bool { return $this->isValid; } @@ -180,9 +246,9 @@ public function isValid() /** * Return the full error meta-data, suitable for combining with another ValidationResult. * - * @return array Array of messages, where each item is an array of data for that message. + * @return Array of messages, where each item is an array of data for that message. */ - public function getMessages() + public function getMessages(): array { return $this->messages; } @@ -193,12 +259,12 @@ public function getMessages() * This object will be modified to contain the new validation information. * * @param ValidationResult $other the validation result object to combine - * @return $this */ - public function combineAnd(ValidationResult $other) + public function combineAnd(ValidationResult $other): static { $this->isValid = $this->isValid && $other->isValid(); $this->messages = array_merge($this->messages, $other->getMessages()); + $this->combineDataClassAndRecordID($other); return $this; } @@ -215,4 +281,60 @@ public function __unserialize(array $data): void $this->messages = $data['messages']; $this->isValid = $data['isValid']; } + + /** + * Combine the data class and record ID from another ValidationResult object. + */ + private function combineDataClassAndRecordID(ValidationResult $other): void + { + $thisDataClass = $this->getDataClass(); + $otherDataClass = $other->getDataClass(); + $thisRecordID = $this->getRecordID(); + $otherRecordID = $other->getRecordID(); + if ($thisDataClass === '' && $otherDataClass !== '') { + $this->setDataClass($otherDataClass); + } elseif ($thisDataClass !== '' && $otherDataClass === '') { + $other->setDataClass($thisDataClass); + } + if ($thisRecordID === 0 && $otherRecordID !== 0) { + $this->setRecordID($otherRecordID); + } elseif ($thisRecordID !== 0 && $otherRecordID === 0) { + $other->setRecordID($thisRecordID); + } + } + + /** + * Ensure that the value can be cast to a string, and if not, return a string representation of the value + */ + private function ensureValueCanBeCastToString(mixed $value): mixed + { + if (is_object($value) && !is_a($value, Stringable::class)) { + return '[object ' . get_class($value) . ']'; + } + return $value; + } + + /** + * Whether to scrub any potentially sensitive values based on $fieldName, as these values + * may end up in a log file or something similar + * This is by no means a reliable method, it's far better to ensure that sensitive data is not + * passed in as an argument in the first place. + * This method should only be considered a safety net at best + */ + private function shouldScrubSensitiveValue(string $fieldName): bool + { + $names = [ + 'Password', + 'PW', + 'Token', + 'Hash', + 'Salt', + ]; + foreach ($names as $name) { + if (str_contains($fieldName, $name)) { + return true; + } + } + return false; + } } diff --git a/src/Forms/Form.php b/src/Forms/Form.php index c53d6d51d7a..70c70b23bfd 100644 --- a/src/Forms/Form.php +++ b/src/Forms/Form.php @@ -1179,32 +1179,36 @@ public function FieldMap() /** * Set a message to the session, for display next time this form is shown. * - * @param string $message the text of the message - * @param string $type Should be set to good, bad, or warning. - * @param string|bool $cast Cast type; One of the CAST_ constant definitions. - * Bool values will be treated as plain text flag. - */ - public function sessionMessage($message, $type = ValidationResult::TYPE_ERROR, $cast = ValidationResult::CAST_TEXT) - { + * @param $message the text of the message + * @param $type Should be set to good, bad, or warning. + * @param $cast Cast type; One of the CAST_ constant definitions. + */ + public function sessionMessage( + string $message, + string $type = ValidationResult::TYPE_ERROR, + string $cast = ValidationResult::CAST_TEXT + ) { $this->setMessage($message, $type, $cast); $result = $this->getSessionValidationResult() ?: ValidationResult::create(); - $result->addMessage($message, $type, null, $cast); + $result->addMessage($message, $type, '', $cast); $this->setSessionValidationResult($result); } /** * Set an error to the session, for display next time this form is shown. * - * @param string $message the text of the message - * @param string $type Should be set to good, bad, or warning. - * @param string|bool $cast Cast type; One of the CAST_ constant definitions. - * Bool values will be treated as plain text flag. + * @param $message the text of the message + * @param $type Should be set to good, bad, or warning. + * @param $cast Cast type; One of the CAST_ constant definitions. */ - public function sessionError($message, $type = ValidationResult::TYPE_ERROR, $cast = ValidationResult::CAST_TEXT) - { + public function sessionError( + string $message, + string $type = ValidationResult::TYPE_ERROR, + string $cast = ValidationResult::CAST_TEXT + ) { $this->setMessage($message, $type, $cast); $result = $this->getSessionValidationResult() ?: ValidationResult::create(); - $result->addError($message, $type, null, $cast); + $result->addError($message, $type, '', $cast); $this->setSessionValidationResult($result); } @@ -1214,14 +1218,17 @@ public function sessionError($message, $type = ValidationResult::TYPE_ERROR, $ca * @param string $message the text of the message * @param string $fieldName Name of the field to set the error message on it. * @param string $type Should be set to good, bad, or warning. - * @param string|bool $cast Cast type; One of the CAST_ constant definitions. - * Bool values will be treated as plain text flag. + * @param string $cast Cast type; One of the CAST_ constant definitions. */ - public function sessionFieldError($message, $fieldName, $type = ValidationResult::TYPE_ERROR, $cast = ValidationResult::CAST_TEXT) - { + public function sessionFieldError( + string $message, + string $fieldName, + string $type = ValidationResult::TYPE_ERROR, + string $cast = ValidationResult::CAST_TEXT + ) { $this->setMessage($message, $type, $cast); $result = $this->getSessionValidationResult() ?: ValidationResult::create(); - $result->addFieldMessage($fieldName, $message, $type, null, $cast); + $result->addFieldMessage($fieldName, $message, $type, '', $cast); $this->setSessionValidationResult($result); } diff --git a/src/Forms/Validation/Validator.php b/src/Forms/Validation/Validator.php index 04df3d263cb..e36a85b80da 100644 --- a/src/Forms/Validation/Validator.php +++ b/src/Forms/Validation/Validator.php @@ -83,7 +83,7 @@ public function validationError( $messageType = ValidationResult::TYPE_ERROR, $cast = ValidationResult::CAST_TEXT ) { - $this->result->addFieldError($fieldName, $message, $messageType, null, $cast); + $this->result->addFieldError($fieldName, $message, $messageType, '', $cast); return $this; } diff --git a/src/ORM/DataObject.php b/src/ORM/DataObject.php index 2f0903d4c9b..00d78789cb2 100644 --- a/src/ORM/DataObject.php +++ b/src/ORM/DataObject.php @@ -1256,7 +1256,7 @@ public function forceChange() */ public function validate(): ValidationResult { - $result = ValidationResult::create(); + $result = ValidationResult::create(get_class($this), $this->ID); // Call DBField::validate() on every DBField $specs = DataObject::getSchema()->fieldSpecs(static::class); foreach (array_keys($specs) as $fieldName) { diff --git a/src/Security/Member.php b/src/Security/Member.php index ef5ef2a9261..54d93b529e1 100644 --- a/src/Security/Member.php +++ b/src/Security/Member.php @@ -325,7 +325,7 @@ public function validateCanLogin(?ValidationResult &$result = null) _t( __CLASS__ . '.ERRORLOCKEDOUT2', 'Your account has been temporarily disabled because of too many failed attempts at ' . 'logging in. Please try again in {count} minutes.', - null, + '', ['count' => static::config()->get('lock_out_delay_mins')] ) ); @@ -1625,17 +1625,17 @@ public function validate(): ValidationResult return ValidationResult::create(); } - $valid = parent::validate(); + $result = parent::validate(); $validator = static::password_validator(); if ($validator) { if ((!$this->ID && $this->Password) || $this->isChanged('Password')) { $userValid = $validator->validate($this->Password, $this); - $valid->combineAnd($userValid); + $result->combineAnd($userValid); } } - return $valid; + return $result; } /** diff --git a/tests/php/Core/Validation/ValidationExceptionTest.php b/tests/php/Core/Validation/ValidationExceptionTest.php index dbc23b1de33..2c1bc525112 100644 --- a/tests/php/Core/Validation/ValidationExceptionTest.php +++ b/tests/php/Core/Validation/ValidationExceptionTest.php @@ -35,7 +35,7 @@ public function testCreateFromValidationResult() 'message' => 'Not a valid result', 'messageCast' => ValidationResult::CAST_TEXT, 'messageType' => ValidationResult::TYPE_ERROR, - 'fieldName' => null, + 'fieldName' => '', ], $exception->getResult()->getMessages()); $this->assertTrue($b, 'Messages array should contain expected messaged'); } @@ -60,7 +60,7 @@ public function testCreateFromComplexValidationResult() 'message' => 'Invalid type', 'messageCast' => ValidationResult::CAST_TEXT, 'messageType' => ValidationResult::TYPE_ERROR, - 'fieldName' => null, + 'fieldName' => '', ], $exception->getResult()->getMessages()); $this->assertTrue($b, 'Messages array should contain expected messaged'); @@ -68,7 +68,7 @@ public function testCreateFromComplexValidationResult() 'message' => 'Out of kiwis', 'messageCast' => ValidationResult::CAST_TEXT, 'messageType' => ValidationResult::TYPE_ERROR, - 'fieldName' => null, + 'fieldName' => '', ], $exception->getResult()->getMessages()); $this->assertTrue($b, 'Messages array should contain expected messaged'); } @@ -113,7 +113,7 @@ public function testCreateWithComplexValidationResultAndMessage() 'message' => 'A spork is not a knife', 'messageCast' => ValidationResult::CAST_TEXT, 'messageType' => ValidationResult::TYPE_ERROR, - 'fieldName' => null, + 'fieldName' => '', ], $exception->getResult()->getMessages()); $this->assertTrue($b, 'Messages array should contain expected messaged'); @@ -121,7 +121,7 @@ public function testCreateWithComplexValidationResultAndMessage() 'message' => 'A knife is not a back scratcher', 'messageCast' => ValidationResult::CAST_TEXT, 'messageType' => ValidationResult::TYPE_ERROR, - 'fieldName' => null, + 'fieldName' => '', ], $exception->getResult()->getMessages()); $this->assertTrue($b, 'Messages array should contain expected messaged'); } @@ -150,13 +150,13 @@ public function testCombineResults() 'message' => 'Eat with your mouth closed', 'messageType' => 'bad', 'messageCast' => ValidationResult::CAST_TEXT, - 'fieldName' => null, + 'fieldName' => '', ], 'BECLEAN' => [ 'message' => 'You didn\'t wash your hands', 'messageType' => 'bad', 'messageCast' => ValidationResult::CAST_HTML, - 'fieldName' => null, + 'fieldName' => '', ], ], $result->getMessages() diff --git a/tests/php/Core/Validation/ValidationResultTest.php b/tests/php/Core/Validation/ValidationResultTest.php index ac725919928..c8c6a5ae0d2 100644 --- a/tests/php/Core/Validation/ValidationResultTest.php +++ b/tests/php/Core/Validation/ValidationResultTest.php @@ -11,28 +11,47 @@ class ValidationResultTest extends SapphireTest public function testSerialise() { $result = new ValidationResult(); - $result->addError("Error", ValidationResult::TYPE_ERROR, null, ValidationResult::CAST_HTML); - $result->addMessage("Message", ValidationResult::TYPE_GOOD); + $result->addError( + 'Error', + ValidationResult::TYPE_ERROR, + 'code-a', + ValidationResult::CAST_HTML, + 'value-a', + 'Some\\DataObject', + 123 + ); + $result->addMessage( + 'Message', + ValidationResult::TYPE_GOOD, + 'code-b', + ValidationResult::CAST_HTML, + 'value-b', + 'Some\\Other\\DataObject', + 456 + ); $serialised = serialize($result); - - /** - * @var ValidationResult $result2 -*/ + /** @var ValidationResult $result2 */ $result2 = unserialize($serialised ?? ''); $this->assertEquals( [ - [ - 'message' => 'Error', - 'fieldName' => null, - 'messageCast' => ValidationResult::CAST_HTML, - 'messageType' => ValidationResult::TYPE_ERROR, - ], - [ - 'message' => 'Message', - 'fieldName' => null, - 'messageCast' => ValidationResult::CAST_TEXT, - 'messageType' => ValidationResult::TYPE_GOOD, - ] + 'code-a' => [ + 'message' => 'Error', + 'fieldName' => '', + 'messageCast' => ValidationResult::CAST_HTML, + 'messageType' => ValidationResult::TYPE_ERROR, + 'value' => 'value-a', + 'dataClass' => 'Some\\DataObject', + 'recordID' => 123, + ], + 'code-b' => [ + 'message' => 'Message', + 'fieldName' => '', + 'messageCast' => ValidationResult::CAST_HTML, + 'messageType' => ValidationResult::TYPE_GOOD, + 'value' => 'value-b', + 'dataClass' => 'Some\\Other\\DataObject', + 'recordID' => 456, + ], ], $result2->getMessages() );