diff --git a/_config/backtrace.yml b/_config/backtrace.yml index fb19e77fd19..07808fc83d1 100644 --- a/_config/backtrace.yml +++ b/_config/backtrace.yml @@ -28,7 +28,8 @@ SilverStripe\Dev\Backtrace: - ['SilverStripe\Security\PasswordEncryptor_Blowfish', 'encryptA'] - ['SilverStripe\Security\PasswordEncryptor_Blowfish', 'encryptX'] - ['SilverStripe\Security\PasswordEncryptor_Blowfish', 'encryptY'] - - ['SilverStripe\Security\PasswordValidator', 'validate'] + - ['SilverStripe\Security\Validation\RulesPasswordValidator', 'validate'] + - ['SilverStripe\Security\Validation\EntropyPasswordValidator', 'validate'] - ['SilverStripe\Security\RememberLoginHash', 'setToken'] - ['SilverStripe\Security\Security', 'encrypt_password'] - ['*', 'checkPassword'] diff --git a/_config/passwords.yml b/_config/passwords.yml index fc865200a7a..04d27a4cb06 100644 --- a/_config/passwords.yml +++ b/_config/passwords.yml @@ -2,12 +2,5 @@ Name: corepasswords --- SilverStripe\Core\Injector\Injector: - SilverStripe\Security\PasswordValidator: - properties: - MinLength: 8 - HistoricCount: 6 - -# In the case someone uses `new PasswordValidator` instead of Injector, provide some safe defaults through config. -SilverStripe\Security\PasswordValidator: - min_length: 8 - historic_count: 6 + SilverStripe\Security\Validation\PasswordValidator: + class: 'SilverStripe\Security\Validation\EntropyPasswordValidator' diff --git a/composer.json b/composer.json index e064b708adb..9db3b8af3a0 100644 --- a/composer.json +++ b/composer.json @@ -48,7 +48,7 @@ "symfony/mailer": "^7.0", "symfony/mime": "^7.0", "symfony/translation": "^7.0", - "symfony/validator": "^7.0", + "symfony/validator": "^7.1", "symfony/yaml": "^7.0", "ext-ctype": "*", "ext-dom": "*", diff --git a/src/Control/Email/Email.php b/src/Control/Email/Email.php index c7c350906b3..aa8bddd5c82 100644 --- a/src/Control/Email/Email.php +++ b/src/Control/Email/Email.php @@ -4,14 +4,13 @@ use Exception; use RuntimeException; -use Egulias\EmailValidator\EmailValidator; -use Egulias\EmailValidator\Validation\RFCValidation; use SilverStripe\Control\Director; use SilverStripe\Core\Config\Configurable; use SilverStripe\Core\Environment; use SilverStripe\Core\Extensible; use SilverStripe\Core\Injector\Injectable; use SilverStripe\Core\Injector\Injector; +use SilverStripe\Core\Validation\ConstraintValidator; use SilverStripe\ORM\FieldType\DBField; use SilverStripe\Model\ArrayData; use SilverStripe\View\Requirements; @@ -22,6 +21,8 @@ use Symfony\Component\Mime\Address; use Symfony\Component\Mime\Email as SymfonyEmail; use Symfony\Component\Mime\Part\AbstractPart; +use Symfony\Component\Validator\Constraints\Email as EmailConstraint; +use Symfony\Component\Validator\Constraints\NotBlank; class Email extends SymfonyEmail { @@ -63,16 +64,17 @@ class Email extends SymfonyEmail private bool $dataHasBeenSet = false; /** - * Checks for RFC822-valid email format. - * - * @copyright Cal Henderson - * This code is licensed under a Creative Commons Attribution-ShareAlike 2.5 License - * http://creativecommons.org/licenses/by-sa/2.5/ + * Checks for RFC valid email format. */ public static function is_valid_address(string $address): bool { - $validator = new EmailValidator(); - return $validator->isValid($address, new RFCValidation()); + return ConstraintValidator::validate( + $address, + [ + new EmailConstraint(mode: EmailConstraint::VALIDATION_MODE_STRICT), + new NotBlank() + ] + )->isValid(); } public static function getSendAllEmailsTo(): array @@ -117,7 +119,7 @@ private static function convertConfigToAddreses(array|string $config): array $addresses = []; if (is_array($config)) { foreach ($config as $key => $val) { - if (filter_var($key, FILTER_VALIDATE_EMAIL)) { + if (static::is_valid_address($key)) { $addresses[] = new Address($key, $val); } else { $addresses[] = new Address($val); diff --git a/src/Control/HTTPRequest.php b/src/Control/HTTPRequest.php index 160e450d307..dbe4f53513a 100644 --- a/src/Control/HTTPRequest.php +++ b/src/Control/HTTPRequest.php @@ -7,6 +7,8 @@ use InvalidArgumentException; use SilverStripe\Core\ClassInfo; use SilverStripe\Core\ArrayLib; +use Symfony\Component\Validator\Constraints\Ip; +use Symfony\Component\Validator\Constraints\IpValidator; /** * Represents a HTTP-request, including a URL that is tokenised for parsing, and a request method @@ -810,7 +812,8 @@ public function getIP() */ public function setIP($ip) { - if (!filter_var($ip, FILTER_VALIDATE_IP)) { + // We can't use ConstraintValidator here because it relies on injector and the kernel may not have booted yet. + if (!IpValidator::checkIp($ip, Ip::ALL)) { throw new InvalidArgumentException("Invalid ip $ip"); } $this->ip = $ip; diff --git a/src/Control/Middleware/TrustedProxyMiddleware.php b/src/Control/Middleware/TrustedProxyMiddleware.php index e990646917c..c2abf750eff 100644 --- a/src/Control/Middleware/TrustedProxyMiddleware.php +++ b/src/Control/Middleware/TrustedProxyMiddleware.php @@ -3,7 +3,10 @@ namespace SilverStripe\Control\Middleware; use SilverStripe\Control\HTTPRequest; +use SilverStripe\Core\Validation\ConstraintValidator; use Symfony\Component\HttpFoundation\IpUtils; +use Symfony\Component\Validator\Constraints\Ip; +use Symfony\Component\Validator\Constraints\NotBlank; /** * This middleware will rewrite headers that provide IP and host details from an upstream proxy. @@ -220,14 +223,14 @@ protected function getIPFromHeaderValue($headerValue) // Prioritise filters $filters = [ - FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE, - FILTER_FLAG_NO_PRIV_RANGE, - null + Ip::ALL_ONLY_PUBLIC, + Ip::ALL_NO_PRIVATE, + Ip::ALL ]; foreach ($filters as $filter) { // Find best IP foreach ($ips as $ip) { - if (filter_var($ip, FILTER_VALIDATE_IP, $filter ?? 0)) { + if (ConstraintValidator::validate($ip, [new Ip(version: $filter), new NotBlank()])->isValid()) { return $ip; } } diff --git a/src/Core/Convert.php b/src/Core/Convert.php index 4347014e85d..ad8bc950021 100644 --- a/src/Core/Convert.php +++ b/src/Core/Convert.php @@ -2,9 +2,12 @@ namespace SilverStripe\Core; +use SilverStripe\Core\Validation\ConstraintValidator; use SimpleXMLElement; use SilverStripe\ORM\DB; use SilverStripe\View\Parsers\URLSegmentFilter; +use Symfony\Component\Validator\Constraints\NotBlank; +use Symfony\Component\Validator\Constraints\Url; /** * Library of conversion functions, implemented as static methods. @@ -226,16 +229,14 @@ public static function xml2raw($val) /** * Create a link if the string is a valid URL - * - * @param string $string The string to linkify - * @return string A link to the URL if string is a URL */ - public static function linkIfMatch($string) - { - if (preg_match('/^[a-z+]+\:\/\/[a-zA-Z0-9$-_.+?&=!*\'()%]+$/', $string ?? '')) { + public static function linkIfMatch( + string $string, + array $protocols = ['file', 'ftp', 'http', 'https', 'imap', 'nntp'] + ): string { + if (ConstraintValidator::validate($string, [new Url(protocols: $protocols), new NotBlank()])->isValid()) { return "$string"; } - return $string; } diff --git a/src/Forms/ConfirmedPasswordField.php b/src/Forms/ConfirmedPasswordField.php index fa61dc91a38..1856a057eff 100644 --- a/src/Forms/ConfirmedPasswordField.php +++ b/src/Forms/ConfirmedPasswordField.php @@ -10,6 +10,8 @@ use SilverStripe\Security\Security; use SilverStripe\View\HTML; use Closure; +use SilverStripe\Core\Validation\ConstraintValidator; +use Symfony\Component\Validator\Constraints\PasswordStrength; /** * Two masked input fields, checks for matching passwords. @@ -25,34 +27,33 @@ class ConfirmedPasswordField extends FormField /** * Minimum character length of the password. - * - * @var int */ - public $minLength = null; + public int $minLength = 0; /** * Maximum character length of the password. - * - * @var int + * 0 means no maximum length. */ - public $maxLength = null; + public int $maxLength = 0; /** - * Enforces at least one digit and one alphanumeric - * character (in addition to {$minLength} and {$maxLength} - * - * @var boolean + * Enforces password strength validation based on entropy. + * See setMinPasswordStrength() */ - public $requireStrongPassword = false; + public bool $requireStrongPassword = false; /** * Allow empty fields when entering the password for the first time * If this is set to true then a random password may be generated if the field is empty * depending on the value of $ConfirmedPasswordField::generateRandomPasswordOnEmtpy - * - * @var boolean */ - public $canBeEmpty = false; + public bool $canBeEmpty = false; + + /** + * Minimum password strength if requireStrongPassword is true + * See https://symfony.com/doc/current/reference/constraints/PasswordStrength.html#minscore + */ + private int $minPasswordStrength = PasswordStrength::STRENGTH_STRONG; /** * Callback used to generate a random password if $this->canBeEmpty is true and the field is left blank @@ -72,79 +73,54 @@ class ConfirmedPasswordField extends FormField * * Caution: The form field does not include any JavaScript or CSS when used outside of the CMS context, * since the required frontend dependencies are included through CMS bundling. - * - * @param boolean $showOnClick */ - protected $showOnClick = false; + protected bool $showOnClick = false; /** * Check if the existing password should be entered first - * - * @var bool */ - protected $requireExistingPassword = false; + protected bool $requireExistingPassword = false; /** * A place to temporarily store the confirm password value - * - * @var string */ - protected $confirmValue; + protected ?string $confirmValue = null; /** * Store value of "Current Password" field - * - * @var string */ - protected $currentPasswordValue; + protected ?string $currentPasswordValue = null; /** * Title for the link that triggers the visibility of password fields. - * - * @var string */ - public $showOnClickTitle; + public string $showOnClickTitle = ''; /** * Child fields (_Password, _ConfirmPassword) - * - * @var FieldList */ - public $children; + public FieldList $children; protected $schemaDataType = FormField::SCHEMA_DATA_TYPE_STRUCTURAL; - /** - * @var PasswordField - */ - protected $passwordField = null; + protected ?PasswordField $passwordField; - /** - * @var PasswordField - */ - protected $confirmPasswordfield = null; + protected ?PasswordField $confirmPasswordfield; - /** - * @var HiddenField - */ - protected $hiddenField = null; + protected ?HiddenField $hiddenField = null; /** - * @param string $name - * @param string $title - * @param mixed $value * @param Form $form Ignored for ConfirmedPasswordField. - * @param boolean $showOnClick * @param string $titleConfirmField Alternate title (not localizeable) */ public function __construct( - $name, - $title = null, - $value = "", - $form = null, - $showOnClick = false, - $titleConfirmField = null + string $name, + ?string $title = null, + mixed $value = "", + ?Form $form = null, + bool $showOnClick = false, + ?string $titleConfirmField = null ) { // Set field title @@ -528,14 +504,18 @@ public function validate($validator) } if ($this->getRequireStrongPassword()) { - if (!preg_match('/^(([a-zA-Z]+\d+)|(\d+[a-zA-Z]+))[a-zA-Z0-9]*$/', $value ?? '')) { + $strongEnough = ConstraintValidator::validate( + $value, + new PasswordStrength(minScore: $this->getMinPasswordStrength()) + )->isValid(); + if (!$strongEnough) { $validator->validationError( $name, _t( - 'SilverStripe\\Forms\\Form.VALIDATIONSTRONGPASSWORD', - 'Passwords must have at least one digit and one alphanumeric character' + __CLASS__ . '.VALIDATIONSTRONGPASSWORD', + 'The password strength is too low. Please use a stronger password.' ), - "validation" + 'validation' ); return $this->extendValidationResult(false, $validator); @@ -637,24 +617,21 @@ public function performDisabledTransformation() /** * Check if existing password is required - * - * @return bool + * If true, an extra form field will be added to enter the existing password */ - public function getRequireExistingPassword() + public function getRequireExistingPassword(): bool { return $this->requireExistingPassword; } /** * Set if the existing password should be required - * - * @param bool $show Flag to show or hide this field - * @return $this + * If true, an extra form field will be added to enter the existing password */ - public function setRequireExistingPassword($show) + public function setRequireExistingPassword(bool $show): static { // Don't modify if already added / removed - if ((bool)$show === $this->requireExistingPassword) { + if ($show === $this->requireExistingPassword) { return $this; } $this->requireExistingPassword = $show; @@ -670,79 +647,91 @@ public function setRequireExistingPassword($show) } /** - * @return PasswordField + * Get the FormField that represents the main password field */ - public function getPasswordField() + public function getPasswordField(): PasswordField { return $this->passwordField; } /** - * @return PasswordField + * Get the FormField that represents the "confirm" password field */ - public function getConfirmPasswordField() + public function getConfirmPasswordField(): PasswordField { return $this->confirmPasswordfield; } /** * Set the minimum length required for passwords - * - * @param int $minLength - * @return $this */ - public function setMinLength($minLength) + public function setMinLength(int $minLength): static { - $this->minLength = (int) $minLength; + $this->minLength = $minLength; return $this; } /** - * @return int + * Get the minimum length required for passwords */ - public function getMinLength() + public function getMinLength(): int { return $this->minLength; } /** - * Set the maximum length required for passwords - * - * @param int $maxLength - * @return $this + * Set the maximum length required for passwords. + * 0 means no max length. */ - public function setMaxLength($maxLength) + public function setMaxLength(int $maxLength): static { - $this->maxLength = (int) $maxLength; + $this->maxLength = $maxLength; return $this; } /** - * @return int + * Get the maximum length required for passwords. + * 0 means no max length. */ - public function getMaxLength() + public function getMaxLength(): int { return $this->maxLength; } /** - * @param bool $requireStrongPassword - * @return $this + * Set whether password strength validation is enforced. + * See setMinPasswordStrength() */ - public function setRequireStrongPassword($requireStrongPassword) + public function setRequireStrongPassword($requireStrongPassword): static { $this->requireStrongPassword = (bool) $requireStrongPassword; return $this; } /** - * @return bool + * Get whether password strength validation is enforced. + * See setMinPasswordStrength() */ - public function getRequireStrongPassword() + public function getRequireStrongPassword(): bool { return $this->requireStrongPassword; } + /** + * Set minimum password strength. Only applies if requireStrongPassword is true + * See https://symfony.com/doc/current/reference/constraints/PasswordStrength.html#minscore + */ + public function setMinPasswordStrength(int $strength): static + { + $this->minPasswordStrength = $strength; + return $this; + } + + public function getMinPasswordStrength(): int + { + return $this->minPasswordStrength; + } + /** * Appends a warning to the right title, or removes that appended warning. */ diff --git a/src/Forms/EmailField.php b/src/Forms/EmailField.php index 68ea66d4199..4426d67f8c6 100644 --- a/src/Forms/EmailField.php +++ b/src/Forms/EmailField.php @@ -2,52 +2,40 @@ namespace SilverStripe\Forms; +use SilverStripe\Core\Validation\ConstraintValidator; +use Symfony\Component\Validator\Constraints\Email as EmailConstraint; + /** - * Text input field with validation for correct email format according to RFC 2822. + * Text input field with validation for correct email format according to the relevant RFC. */ class EmailField extends TextField { - protected $inputType = 'email'; - /** - * {@inheritdoc} - */ + public function Type() { return 'email text'; } /** - * Validates for RFC 2822 compliant email addresses. - * - * @see http://www.regular-expressions.info/email.html - * @see http://www.ietf.org/rfc/rfc2822.txt + * Validates for RFC compliant email addresses. * * @param Validator $validator - * - * @return string */ public function validate($validator) { - $result = true; $this->value = trim($this->value ?? ''); - $pattern = '^[a-z0-9!#$%&\'*+/=?^_`{|}~-]+(?:\\.[a-z0-9!#$%&\'*+/=?^_`{|}~-]+)*@(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?$'; - - // Escape delimiter characters. - $safePattern = str_replace('/', '\\/', $pattern ?? ''); - - if ($this->value && !preg_match('/' . $safePattern . '/i', $this->value ?? '')) { - $validator->validationError( - $this->name, - _t('SilverStripe\\Forms\\EmailField.VALIDATION', 'Please enter an email address'), - 'validation' - ); - - $result = false; - } + $message = _t('SilverStripe\\Forms\\EmailField.VALIDATION', 'Please enter an email address'); + $result = ConstraintValidator::validate( + $this->value, + new EmailConstraint(message: $message, mode: EmailConstraint::VALIDATION_MODE_STRICT), + $this->getName() + ); + $validator->getResult()->combineAnd($result); + $isValid = $result->isValid(); - return $this->extendValidationResult($result, $validator); + return $this->extendValidationResult($isValid, $validator); } public function getSchemaValidation() diff --git a/src/Forms/UrlField.php b/src/Forms/UrlField.php index a8332d87a7e..4d5493fc6f7 100644 --- a/src/Forms/UrlField.php +++ b/src/Forms/UrlField.php @@ -7,10 +7,24 @@ /** * Text input field with validation for a url - * Url must include a scheme, either http:// or https:// + * Url must include a protocol (aka scheme) such as https:// or http:// */ class UrlField extends TextField { + /** + * The default set of protocols allowed for valid URLs + */ + private static array $default_protocols = ['https', 'http']; + + /** + * The default value for whether a relative protocol (// on its own) is allowed + */ + private static bool $default_allow_relative_protocol = false; + + private array $protocols = []; + + private ?bool $allowRelativeProtocol = null; + public function Type() { return 'text url'; @@ -18,15 +32,64 @@ public function Type() public function validate($validator) { - $result = true; - if ($this->value && !ConstraintValidator::validate($this->value, new Url())->isValid()) { - $validator->validationError( - $this->name, - _t(__CLASS__ . '.INVALID', 'Please enter a valid URL'), - 'validation' - ); - $result = false; + $allowedProtocols = $this->getAllowedProtocols(); + $message = _t( + __CLASS__ . '.INVALID_WITH_PROTOCOL', + 'Please enter a valid URL including a protocol, e.g {protocol}://example.com', + ['protocol' => $allowedProtocols[0]] + ); + $result = ConstraintValidator::validate( + $this->value, + new Url( + message: $message, + protocols: $allowedProtocols, + relativeProtocol: $this->getAllowRelativeProtocol() + ), + $this->getName() + ); + $validator->getResult()->combineAnd($result); + $isValid = $result->isValid(); + return $this->extendValidationResult($isValid, $validator); + } + + /** + * Set which protocols valid URLs are allowed to have + */ + public function setAllowedProtocols(array $protocols): static + { + // Ensure the array isn't associative so we can use 0 index in validate(). + $this->protocols = array_keys($protocols); + return $this; + } + + /** + * Get which protocols valid URLs are allowed to have + */ + public function getAllowedProtocols(): array + { + if (empty($this->protocols)) { + return static::config()->get('default_protocols'); + } + return $this->protocols; + } + + /** + * Set whether a relative protocol (// on its own) is allowed + */ + public function setAllowRelativeProtocol(?bool $allow): static + { + $this->allowRelativeProtocol = $allow; + return $this; + } + + /** + * Get whether a relative protocol (// on its own) is allowed + */ + public function getAllowRelativeProtocol(): bool + { + if ($this->allowRelativeProtocol === null) { + return static::config()->get('default_allow_relative_protocol'); } - return $this->extendValidationResult($result, $validator); + return $this->allowRelativeProtocol; } } diff --git a/src/Security/Member.php b/src/Security/Member.php index 1027cbefafa..700e518b344 100644 --- a/src/Security/Member.php +++ b/src/Security/Member.php @@ -44,7 +44,8 @@ use SilverStripe\Forms\SearchableDropdownField; use SilverStripe\Forms\SearchableMultiDropdownField; use SilverStripe\ORM\FieldType\DBForeignKey; -use SilverStripe\Dev\Deprecation; +use SilverStripe\Security\Validation\PasswordValidator; +use SilverStripe\Security\Validation\RulesPasswordValidator; /** * The member class which represents the users of the system @@ -379,10 +380,8 @@ public function isLockedOut() /** * Set a {@link PasswordValidator} object to use to validate member's passwords. - * - * @param PasswordValidator $validator */ - public static function set_password_validator(PasswordValidator $validator = null) + public static function set_password_validator(?PasswordValidator $validator = null) { // Override existing config Config::modify()->remove(Injector::class, PasswordValidator::class); @@ -395,13 +394,11 @@ public static function set_password_validator(PasswordValidator $validator = nul /** * Returns the default {@link PasswordValidator} - * - * @return PasswordValidator|null */ - public static function password_validator() + public static function password_validator(): ?PasswordValidator { if (Injector::inst()->has(PasswordValidator::class)) { - return Deprecation::withSuppressedNotice(fn() => Injector::inst()->get(PasswordValidator::class)); + return Injector::inst()->get(PasswordValidator::class); } return null; } @@ -1764,11 +1761,16 @@ public function generateRandomPassword(int $length = 0): string { $password = ''; $validator = Member::password_validator(); - if ($length && $validator && $length < $validator->getMinLength()) { - throw new InvalidArgumentException('length argument is less than password validator minLength'); + if ($validator instanceof RulesPasswordValidator) { + $validatorMinLength = $validator->getMinLength(); + if ($length && $length < $validatorMinLength) { + throw new InvalidArgumentException('length argument is less than password validator minLength'); + } + } else { + // Make sure the password is long enough to beat even very strict entropy tests + $validatorMinLength = 128; } - $validatorMinLength = $validator ? $validator->getMinLength() : 0; - $len = $length ?: max($validatorMinLength, 20); + $len = max($length, $validatorMinLength, 20); // The default PasswordValidator checks the password includes the following four character sets $charsets = [ 'abcdefghijklmnopqrstuvwyxz', diff --git a/src/Security/Validation/AbstractPasswordValidator.php b/src/Security/Validation/AbstractPasswordValidator.php new file mode 100644 index 00000000000..fe0d83e2e0f --- /dev/null +++ b/src/Security/Validation/AbstractPasswordValidator.php @@ -0,0 +1,72 @@ +getHistoricCount(); + if ($historicCount) { + $idColumn = DataObject::getSchema()->sqlColumnForField(MemberPassword::class, 'MemberID'); + $previousPasswords = MemberPassword::get() + ->where([$idColumn => $member->ID]) + ->sort(['Created' => 'DESC', 'ID' => 'DESC']) + ->limit($historicCount); + foreach ($previousPasswords as $previousPassword) { + if ($previousPassword->checkPassword($password)) { + $error = _t( + PasswordValidator::class . '.PREVPASSWORD', + 'You\'ve already used that password in the past, please choose a new password' + ); + $result->addError($error, 'bad', 'PREVIOUS_PASSWORD'); + break; + } + } + } + + return $result; + } + + /** + * Get the number of previous passwords to check for a reusing old passwords. + */ + public function getHistoricCount(): int + { + if ($this->historicalPasswordCount !== null) { + return $this->historicalPasswordCount; + } + return $this->config()->get('historic_count') ?? 0; + } + + /** + * Set the number of previous passwords to check for a reusing old passwords. + */ + public function setHistoricCount(int $count): static + { + $this->historicalPasswordCount = $count; + return $this; + } +} diff --git a/src/Security/Validation/EntropyPasswordValidator.php b/src/Security/Validation/EntropyPasswordValidator.php new file mode 100644 index 00000000000..27c7f6186b2 --- /dev/null +++ b/src/Security/Validation/EntropyPasswordValidator.php @@ -0,0 +1,33 @@ +get('password_strength'); + $result = ConstraintValidator::validate($password, [new PasswordStrength(minScore: $minScore), new NotBlank()]); + $result->combineAnd(parent::validate($password, $member)); + $this->extend('updateValidatePassword', $password, $member, $result, $this); + return $result; + } +} diff --git a/src/Security/PasswordValidator.php b/src/Security/Validation/RulesPasswordValidator.php similarity index 51% rename from src/Security/PasswordValidator.php rename to src/Security/Validation/RulesPasswordValidator.php index 388e76fe6f5..27b44619bd3 100644 --- a/src/Security/PasswordValidator.php +++ b/src/Security/Validation/RulesPasswordValidator.php @@ -1,39 +1,32 @@ * $pwdVal = new PasswordValidator(); * $pwdValidator->setMinLength(7); - * $pwdValidator->checkHistoricalPasswords(6); + * $pwdValidator->setHistoricCount(6); * $pwdValidator->setMinTestScore(3); * $pwdValidator->setTestNames(array("lowercase", "uppercase", "digits", "punctuation")); * * Member::set_password_validator($pwdValidator); * - * - * @deprecated 5.4.0 Will be renamed to SilverStripe\Security\Validation\RulesPasswordValidator */ -class PasswordValidator +class RulesPasswordValidator extends PasswordValidator { - use Injectable; - use Configurable; use Extensible; /** - * @config - * @var array + * Regex to test the password against. See min_test_score. */ - private static $character_strength_tests = [ + private static array $character_strength_tests = [ 'lowercase' => '/[a-z]/', 'uppercase' => '/[A-Z]/', 'digits' => '/[0-9]/', @@ -41,89 +34,62 @@ class PasswordValidator ]; /** - * @config - * @var int - */ - private static $min_length = null; - - /** - * @config - * @var int + * Default minimum number of characters for a valid password. */ - private static $min_test_score = null; + private static int $min_length = 8; /** - * @config - * @var int + * Default minimum test score for a valid password. + * The test score is the number of character_strength_tests that the password matches. */ - private static $historic_count = null; + private static int $min_test_score = 0; - /** - * @var int - */ - protected $minLength = null; + protected ?int $minLength = null; - /** - * @var int - */ - protected $minScore = null; + protected ?int $minScore = null; /** * @var string[] */ - protected $testNames = null; + protected ?array $testNames = null; /** - * @var int + * Get the minimum number of characters for a valid password. */ - protected $historicalPasswordCount = null; - - public function __construct() - { - Deprecation::notice( - '5.4.0', - 'Will be renamed to SilverStripe\Security\Validation\RulesPasswordValidator', - Deprecation::SCOPE_CLASS - ); - } - - /** - * @return int - */ - public function getMinLength() + public function getMinLength(): int { if ($this->minLength !== null) { return $this->minLength; } - return $this->config()->get('min_length'); + return $this->config()->get('min_length') ?? 0; } /** - * @param int $minLength - * @return $this + * Set the minimum number of characters for a valid password. */ - public function setMinLength($minLength) + public function setMinLength(int $minLength): static { $this->minLength = $minLength; return $this; } /** - * @return integer + * Get the minimum test score for a valid password. + * The test score is the number of character_strength_tests that the password matches. */ - public function getMinTestScore() + public function getMinTestScore(): int { if ($this->minScore !== null) { return $this->minScore; } - return $this->config()->get('min_test_score'); + return $this->config()->get('min_test_score') ?? 0; } /** - * @param int $minScore - * @return $this + * Set the minimum test score for a valid password. + * The test score is the number of character_strength_tests that the password matches. */ - public function setMinTestScore($minScore) + public function setMinTestScore(int $minScore): static { $this->minScore = $minScore; return $this; @@ -134,7 +100,7 @@ public function setMinTestScore($minScore) * * @return string[] */ - public function getTestNames() + public function getTestNames(): array { if ($this->testNames !== null) { return $this->testNames; @@ -146,51 +112,22 @@ public function getTestNames() * Set list of tests to use for this validator * * @param string[] $testNames - * @return $this */ - public function setTestNames($testNames) + public function setTestNames(array $testNames): static { $this->testNames = $testNames; return $this; } - /** - * @return int - */ - public function getHistoricCount() - { - if ($this->historicalPasswordCount !== null) { - return $this->historicalPasswordCount; - } - return $this->config()->get('historic_count'); - } - - /** - * @param int $count - * @return $this - */ - public function setHistoricCount($count) - { - $this->historicalPasswordCount = $count; - return $this; - } - /** * Gets all possible tests - * - * @return array */ - public function getTests() + public function getTests(): array { return $this->config()->get('character_strength_tests'); } - /** - * @param string $password - * @param Member $member - * @return ValidationResult - */ - public function validate($password, $member) + public function validate(string $password, Member $member): ValidationResult { $valid = ValidationResult::create(); @@ -234,23 +171,7 @@ public function validate($password, $member) } } - $historicCount = $this->getHistoricCount(); - if ($historicCount) { - $previousPasswords = MemberPassword::get() - ->where(['"MemberPassword"."MemberID"' => $member->ID]) - ->sort('"Created" DESC, "ID" DESC') - ->limit($historicCount); - foreach ($previousPasswords as $previousPassword) { - if ($previousPassword->checkPassword($password)) { - $error = _t( - __CLASS__ . '.PREVPASSWORD', - 'You\'ve already used that password in the past, please choose a new password' - ); - $valid->addError($error, 'bad', 'PREVIOUS_PASSWORD'); - break; - } - } - } + $valid->combineAnd(parent::validate($password, $member)); $this->extend('updateValidatePassword', $password, $member, $valid, $this); diff --git a/tests/php/Forms/ConfirmedPasswordFieldTest.php b/tests/php/Forms/ConfirmedPasswordFieldTest.php index db2aed70507..29218f0abad 100644 --- a/tests/php/Forms/ConfirmedPasswordFieldTest.php +++ b/tests/php/Forms/ConfirmedPasswordFieldTest.php @@ -11,11 +11,9 @@ use SilverStripe\Forms\ReadonlyField; use SilverStripe\Forms\RequiredFields; use SilverStripe\Security\Member; -use SilverStripe\Security\PasswordValidator; use SilverStripe\View\SSViewer; use Closure; use PHPUnit\Framework\Attributes\DataProvider; -use SilverStripe\Dev\Deprecation; class ConfirmedPasswordFieldTest extends SapphireTest { @@ -25,11 +23,7 @@ protected function setUp(): void { parent::setUp(); - Deprecation::withSuppressedNotice( - fn() => PasswordValidator::singleton() - ->setMinLength(0) - ->setTestNames([]) - ); + Member::set_password_validator(null); } public function testSetValue() diff --git a/tests/php/Forms/EmailFieldTest.php b/tests/php/Forms/EmailFieldTest.php index 26723e555ad..a3a054f4eee 100644 --- a/tests/php/Forms/EmailFieldTest.php +++ b/tests/php/Forms/EmailFieldTest.php @@ -4,9 +4,7 @@ use SilverStripe\Dev\FunctionalTest; use SilverStripe\Forms\EmailField; -use Exception; -use PHPUnit\Framework\AssertionFailedError; -use SilverStripe\Forms\Tests\EmailFieldTest\TestValidator; +use SilverStripe\Forms\FieldsValidator; class EmailFieldTest extends FunctionalTest { @@ -40,21 +38,14 @@ public function internalCheck($email, $checkText, $expectSuccess) $field = new EmailField("MyEmail"); $field->setValue($email); - $val = new TestValidator(); - try { - $field->validate($val); - // If we expect failure and processing gets here without an exception, the test failed - $this->assertTrue($expectSuccess, $checkText . " (/$email/ passed validation, but not expected to)"); - } catch (Exception $e) { - if ($e instanceof AssertionFailedError) { - // re-throw assertion failure - throw $e; - } elseif ($expectSuccess) { - $this->fail( - $checkText . ": " . $e->getMessage() . " (/$email/ did not pass validation, but was expected to)" - ); - } + if ($expectSuccess) { + $message = $checkText . " (/$email/ did not pass validation, but was expected to)"; + } else { + $message = $checkText . " (/$email/ passed validation, but not expected to)"; } + + $result = $field->validate(new FieldsValidator()); + $this->assertSame($expectSuccess, $result, $message); } /** diff --git a/tests/php/Forms/EmailFieldTest/TestValidator.php b/tests/php/Forms/EmailFieldTest/TestValidator.php deleted file mode 100644 index 21e00a989aa..00000000000 --- a/tests/php/Forms/EmailFieldTest/TestValidator.php +++ /dev/null @@ -1,27 +0,0 @@ - PasswordValidator::singleton() - ->setMinLength(0) - ->setTestNames([]) - ); + // Enforce no password validation + Member::set_password_validator(null); } protected function tearDown(): void diff --git a/tests/php/Security/MemberCsvBulkLoaderTest.php b/tests/php/Security/MemberCsvBulkLoaderTest.php index 6860ee18f13..c9cefdee1d5 100644 --- a/tests/php/Security/MemberCsvBulkLoaderTest.php +++ b/tests/php/Security/MemberCsvBulkLoaderTest.php @@ -2,12 +2,10 @@ namespace SilverStripe\Security\Tests; -use SilverStripe\Dev\Deprecation; use SilverStripe\ORM\DataObject; use SilverStripe\Security\Group; use SilverStripe\Security\MemberCsvBulkLoader; use SilverStripe\Security\Member; -use SilverStripe\Security\PasswordValidator; use SilverStripe\Security\Security; use SilverStripe\Dev\SapphireTest; use SilverStripe\Security\MemberAuthenticator\MemberAuthenticator; @@ -19,12 +17,7 @@ class MemberCsvBulkLoaderTest extends SapphireTest protected function setUp(): void { parent::setUp(); - - Deprecation::withSuppressedNotice( - fn() => PasswordValidator::singleton() - ->setMinLength(0) - ->setTestNames([]) - ); + Member::set_password_validator(null); } public function testNewImport() diff --git a/tests/php/Security/MemberTest.php b/tests/php/Security/MemberTest.php index eb60dabe861..c1132aaea37 100644 --- a/tests/php/Security/MemberTest.php +++ b/tests/php/Security/MemberTest.php @@ -8,7 +8,6 @@ use SilverStripe\Core\Config\Config; use SilverStripe\Core\Convert; use SilverStripe\Core\Injector\Injector; -use SilverStripe\Dev\Deprecation; use SilverStripe\Dev\FunctionalTest; use SilverStripe\Forms\CheckboxField; use SilverStripe\Forms\FieldList; @@ -29,7 +28,6 @@ use SilverStripe\Security\MemberAuthenticator\SessionAuthenticationHandler; use SilverStripe\Security\MemberPassword; use SilverStripe\Security\PasswordEncryptor_Blowfish; -use SilverStripe\Security\PasswordValidator; use SilverStripe\Security\Permission; use SilverStripe\Security\RememberLoginHash; use SilverStripe\Security\Security; @@ -37,6 +35,8 @@ use SilverStripe\SessionManager\Models\LoginSession; use ReflectionMethod; use PHPUnit\Framework\Attributes\DataProvider; +use SilverStripe\Security\Validation\EntropyPasswordValidator; +use SilverStripe\Security\Validation\RulesPasswordValidator; class MemberTest extends FunctionalTest { @@ -75,12 +75,6 @@ protected function setUp(): void Member::config()->set('unique_identifier_field', 'Email'); - Deprecation::withSuppressedNotice( - fn() => PasswordValidator::singleton() - ->setMinLength(0) - ->setTestNames([]) - ); - i18n::set_locale('en_US'); } @@ -1743,7 +1737,7 @@ public function testNewMembersReceiveTheDefaultLocale() public function testChangePasswordOnlyValidatesPlaintext() { // This validator requires passwords to be 17 characters long - Member::set_password_validator(Deprecation::withSuppressedNotice(fn() => new MemberTest\VerySpecificPasswordValidator())); + Member::set_password_validator(new MemberTest\VerySpecificPasswordValidator()); // This algorithm will never return a 17 character hash Security::config()->set('password_encryption_algorithm', 'blowfish'); @@ -1770,11 +1764,23 @@ public function testEmailIsTrimmed() $this->assertNotNull(Member::get()->find('Email', 'trimmed@test.com')); } - public function testChangePasswordToBlankIsValidated() + public static function provideChangePasswordToBlankIsValidated(): array { - Member::set_password_validator(Deprecation::withSuppressedNotice(fn() => new PasswordValidator())); - // override setup() function which setMinLength(0) - PasswordValidator::singleton()->setMinLength(8); + return [ + [ + 'validatorClass' => RulesPasswordValidator::class, + ], + [ + 'validatorClass' => EntropyPasswordValidator::class, + ], + ]; + } + + #[DataProvider('provideChangePasswordToBlankIsValidated')] + public function testChangePasswordToBlankIsValidated(string $validatorClass): void + { + $validator = new $validatorClass(); + Member::set_password_validator($validator); // 'test' member has a password defined in yml $member = $this->objFromFixture(Member::class, 'test'); $result = $member->changePassword(''); @@ -1896,7 +1902,20 @@ public static function provideMapInCMSGroups() ]; } - public function testGenerateRandomPassword() + public static function provideGenerateRandomPassword(): array + { + return [ + [ + 'validatorClass' => RulesPasswordValidator::class, + ], + [ + 'validatorClass' => EntropyPasswordValidator::class, + ], + ]; + } + + #[DataProvider('provideGenerateRandomPassword')] + public function testGenerateRandomPassword(string $validatorClass): void { $member = new Member(); // no password validator @@ -1908,22 +1927,32 @@ public function testGenerateRandomPassword() $password = $member->generateRandomPassword(); $this->assertSame(20, strlen($password)); // password validator - $validator = Deprecation::withSuppressedNotice(fn() => new PasswordValidator()); + $validator = new $validatorClass(); Member::set_password_validator($validator); - // Password length of 20 even if validator minLength is less than 20 - $validator->setMinLength(10); - $password = $member->generateRandomPassword(); - $this->assertSame(20, strlen($password)); - // Password length of 25 if passing length argument, and validator minlength is less than length argument - $password = $member->generateRandomPassword(25); - $this->assertSame(25, strlen($password)); - // Password length is validator minLength if validator minLength is greater than 20 and no length argument - $validator->setMinLength(30); + if ($validator instanceof RulesPasswordValidator) { + // Password length of 20 even if validator minLength is less than 20 + $validator->setMinLength(10); + $minLengthInMember = 20; + } else { + $minLengthInMember = 128; + } $password = $member->generateRandomPassword(); - $this->assertSame(30, strlen($password)); - // Exception throw if length argument is less than validator minLength - $this->expectException(InvalidArgumentException::class); - $this->expectExceptionMessage('length argument is less than password validator minLength'); - $password = $member->generateRandomPassword(15); + $this->assertSame($minLengthInMember, strlen($password)); + // Password length of 256 if passing length argument, and validator minlength is less than length argument + $password = $member->generateRandomPassword(256); + $this->assertSame(256, strlen($password)); + if ($validator instanceof RulesPasswordValidator) { + // Password length is validator minLength if validator minLength is greater than 20 and no length argument + $validator->setMinLength(30); + $password = $member->generateRandomPassword(); + $this->assertSame(30, strlen($password)); + // Exception throw if length argument is less than validator minLength + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('length argument is less than password validator minLength'); + $password = $member->generateRandomPassword(15); + } else { + // No exception for entropy validator + $password = $member->generateRandomPassword(15); + } } } diff --git a/tests/php/Security/MemberTest/TestPasswordValidator.php b/tests/php/Security/MemberTest/TestPasswordValidator.php index 834507f6162..7ea86743e41 100644 --- a/tests/php/Security/MemberTest/TestPasswordValidator.php +++ b/tests/php/Security/MemberTest/TestPasswordValidator.php @@ -3,9 +3,9 @@ namespace SilverStripe\Security\Tests\MemberTest; use SilverStripe\Dev\TestOnly; -use SilverStripe\Security\PasswordValidator; +use SilverStripe\Security\Validation\RulesPasswordValidator; -class TestPasswordValidator extends PasswordValidator implements TestOnly +class TestPasswordValidator extends RulesPasswordValidator implements TestOnly { public function __construct() { diff --git a/tests/php/Security/MemberTest/VerySpecificPasswordValidator.php b/tests/php/Security/MemberTest/VerySpecificPasswordValidator.php index 28c66b5de87..d37af6a0734 100644 --- a/tests/php/Security/MemberTest/VerySpecificPasswordValidator.php +++ b/tests/php/Security/MemberTest/VerySpecificPasswordValidator.php @@ -4,11 +4,12 @@ use SilverStripe\Dev\TestOnly; use SilverStripe\Core\Validation\ValidationResult; -use SilverStripe\Security\PasswordValidator; +use SilverStripe\Security\Member; +use SilverStripe\Security\Validation\PasswordValidator; class VerySpecificPasswordValidator extends PasswordValidator implements TestOnly { - public function validate($password, $member) + public function validate(string $password, Member $member): ValidationResult { $result = ValidationResult::create(); if (strlen($password ?? '') !== 17) { diff --git a/tests/php/Security/SecurityTest.php b/tests/php/Security/SecurityTest.php index 44573e00b52..9f73a925df3 100644 --- a/tests/php/Security/SecurityTest.php +++ b/tests/php/Security/SecurityTest.php @@ -22,7 +22,6 @@ use SilverStripe\Security\LoginAttempt; use SilverStripe\Security\Member; use SilverStripe\Security\MemberAuthenticator\MemberAuthenticator; -use SilverStripe\Security\PasswordValidator; use SilverStripe\Security\Security; use SilverStripe\Security\SecurityToken; @@ -60,11 +59,6 @@ protected function setUp(): void Member::config()->set('unique_identifier_field', 'Email'); - PasswordValidator::config() - ->remove('min_length') - ->remove('historic_count') - ->remove('min_test_score'); - Member::set_password_validator(null); parent::setUp(); diff --git a/tests/php/Security/Validation/EntropyPasswordValidatorTest.php b/tests/php/Security/Validation/EntropyPasswordValidatorTest.php new file mode 100644 index 00000000000..8e856665c5b --- /dev/null +++ b/tests/php/Security/Validation/EntropyPasswordValidatorTest.php @@ -0,0 +1,42 @@ + '', + 'expected' => false, + ], + [ + 'password' => 'password123', + 'expected' => false, + ], + [ + 'password' => 'This is a really long and complex PASSWORD', + 'expected' => true, + ], + ]; + } + + #[DataProvider('provideValidate')] + public function testValidate(string $password, bool $expected): void + { + $validator = new EntropyPasswordValidator(); + $this->assertSame($expected, $validator->validate($password, new Member())->isValid()); + } +} diff --git a/tests/php/Security/Validation/PasswordValidatorTest.php b/tests/php/Security/Validation/PasswordValidatorTest.php new file mode 100644 index 00000000000..c096db10969 --- /dev/null +++ b/tests/php/Security/Validation/PasswordValidatorTest.php @@ -0,0 +1,40 @@ +setHistoricCount(3); + Member::set_password_validator($validator); + + $member = new Member; + $member->FirstName = 'Repeat'; + $member->Surname = 'Password-Man'; + $member->Password = 'honk'; + $member->write(); + + // Create a set of used passwords + $member->changePassword('foobar'); + $member->changePassword('foobaz'); + $member->changePassword('barbaz'); + + $this->assertFalse($member->changePassword('barbaz')->isValid()); + $this->assertFalse($member->changePassword('foobaz')->isValid()); + $this->assertFalse($member->changePassword('foobar')->isValid()); + $this->assertTrue($member->changePassword('honk')->isValid()); + $this->assertTrue($member->changePassword('newpassword')->isValid()); + } +} diff --git a/tests/php/Security/Validation/PasswordValidatorTest/DummyPasswordValidator.php b/tests/php/Security/Validation/PasswordValidatorTest/DummyPasswordValidator.php new file mode 100644 index 00000000000..db066c24fb0 --- /dev/null +++ b/tests/php/Security/Validation/PasswordValidatorTest/DummyPasswordValidator.php @@ -0,0 +1,11 @@ +remove('min_length') ->remove('historic_count') ->set('min_test_score', 0); @@ -27,7 +22,7 @@ protected function setUp(): void public function testValidate() { - $v = Deprecation::withSuppressedNotice(fn() => new PasswordValidator()); + $v = new RulesPasswordValidator(); $r = $v->validate('', new Member()); $this->assertTrue($r->isValid(), 'Empty password is valid by default'); @@ -37,7 +32,7 @@ public function testValidate() public function testValidateMinLength() { - $v = Deprecation::withSuppressedNotice(fn() => new PasswordValidator()); + $v = new RulesPasswordValidator(); $v->setMinLength(4); $r = $v->validate('123', new Member()); @@ -51,7 +46,7 @@ public function testValidateMinLength() public function testValidateMinScore() { // Set both score and set of tests - $v = Deprecation::withSuppressedNotice(fn() => new PasswordValidator()); + $v = new RulesPasswordValidator(); $v->setMinTestScore(3); $v->setTestNames(["lowercase", "uppercase", "digits", "punctuation"]); @@ -62,7 +57,7 @@ public function testValidateMinScore() $this->assertTrue($r->isValid(), 'Passing enough tests'); // Ensure min score without tests works (uses default tests) - $v = Deprecation::withSuppressedNotice(fn() => new PasswordValidator()); + $v = new RulesPasswordValidator(); $v->setMinTestScore(3); $r = $v->validate('aA', new Member()); @@ -76,31 +71,4 @@ public function testValidateMinScore() $r = $v->validate('aA1!', new Member()); $this->assertTrue($r->isValid(), 'Passing enough tests'); } - - /** - * Test that a certain number of historical passwords are checked if specified - */ - public function testHistoricalPasswordCount() - { - $validator = Deprecation::withSuppressedNotice(fn() => new PasswordValidator); - $validator->setHistoricCount(3); - Member::set_password_validator($validator); - - $member = new Member; - $member->FirstName = 'Repeat'; - $member->Surname = 'Password-Man'; - $member->Password = 'honk'; - $member->write(); - - // Create a set of used passwords - $member->changePassword('foobar'); - $member->changePassword('foobaz'); - $member->changePassword('barbaz'); - - $this->assertFalse($member->changePassword('barbaz')->isValid()); - $this->assertFalse($member->changePassword('foobaz')->isValid()); - $this->assertFalse($member->changePassword('foobar')->isValid()); - $this->assertTrue($member->changePassword('honk')->isValid()); - $this->assertTrue($member->changePassword('newpassword')->isValid()); - } } diff --git a/tests/php/Security/VersionedMemberAuthenticatorTest.php b/tests/php/Security/VersionedMemberAuthenticatorTest.php index 7fd813b28b1..0a3dbb21565 100644 --- a/tests/php/Security/VersionedMemberAuthenticatorTest.php +++ b/tests/php/Security/VersionedMemberAuthenticatorTest.php @@ -3,24 +3,11 @@ namespace SilverStripe\Security\Tests; use SilverStripe\Control\Controller; -use SilverStripe\Control\NullHTTPRequest; -use SilverStripe\Core\Config\Config; -use SilverStripe\Core\Injector\Injector; -use SilverStripe\Dev\Deprecation; use SilverStripe\Dev\SapphireTest; use SilverStripe\ORM\FieldType\DBDatetime; use SilverStripe\Core\Validation\ValidationResult; -use SilverStripe\Security\Authenticator; -use SilverStripe\Security\DefaultAdminService; -use SilverStripe\Security\IdentityStore; -use SilverStripe\Security\LoginAttempt; use SilverStripe\Security\Member; -use SilverStripe\Security\MemberAuthenticator\CMSMemberAuthenticator; -use SilverStripe\Security\MemberAuthenticator\CMSMemberLoginForm; use SilverStripe\Security\MemberAuthenticator\MemberAuthenticator; -use SilverStripe\Security\MemberAuthenticator\MemberLoginForm; -use SilverStripe\Security\PasswordValidator; -use SilverStripe\Security\Security; use SilverStripe\Versioned\Versioned; class VersionedMemberAuthenticatorTest extends SapphireTest @@ -43,12 +30,8 @@ protected function setUp(): void return; } - // Enforce dummy validation (this can otherwise be influenced by recipe config) - Deprecation::withSuppressedNotice( - fn() => PasswordValidator::singleton() - ->setMinLength(0) - ->setTestNames([]) - ); + // Remove password validation + Member::set_password_validator(null); } protected function tearDown(): void