From 34db9cea11b40e8c5ccf628d887de0a908922f9b Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Tue, 24 Sep 2024 17:44:12 +1200 Subject: [PATCH] ENH Use symfony/validation logic --- _config/backtrace.yml | 3 +- _config/passwords.yml | 11 +- composer.json | 2 +- src/Control/Email/Email.php | 22 +-- src/Control/HTTPRequest.php | 5 +- .../Middleware/TrustedProxyMiddleware.php | 11 +- src/Core/Convert.php | 15 +- src/Forms/EmailField.php | 42 ++---- src/Forms/UrlField.php | 83 +++++++++-- src/Security/Member.php | 23 +-- .../Validation/AbstractPasswordValidator.php | 72 ++++++++++ .../Validation/EntropyPasswordValidator.php | 33 +++++ .../RulesPasswordValidator.php} | 131 +++++------------- .../php/Forms/ConfirmedPasswordFieldTest.php | 5 +- .../php/Security/MemberAuthenticatorTest.php | 7 +- .../php/Security/MemberCsvBulkLoaderTest.php | 6 +- tests/php/Security/MemberTest.php | 81 +++++++---- .../MemberTest/TestPasswordValidator.php | 4 +- .../VerySpecificPasswordValidator.php | 5 +- tests/php/Security/SecurityTest.php | 6 - .../EntropyPasswordValidatorTest.php | 42 ++++++ .../Validation/PasswordValidatorTest.php | 40 ++++++ .../DummyPasswordValidator.php | 11 ++ .../RulesPasswordValidatorTest.php} | 49 ++----- .../VersionedMemberAuthenticatorTest.php | 18 +-- 25 files changed, 443 insertions(+), 284 deletions(-) create mode 100644 src/Security/Validation/AbstractPasswordValidator.php create mode 100644 src/Security/Validation/EntropyPasswordValidator.php rename src/Security/{PasswordValidator.php => Validation/RulesPasswordValidator.php} (54%) create mode 100644 tests/php/Security/Validation/EntropyPasswordValidatorTest.php create mode 100644 tests/php/Security/Validation/PasswordValidatorTest.php create mode 100644 tests/php/Security/Validation/PasswordValidatorTest/DummyPasswordValidator.php rename tests/php/Security/{PasswordValidatorTest.php => Validation/RulesPasswordValidatorTest.php} (57%) 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 4e8fed3e73c..ee46bce49c7 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 52b05dc944f..7f6c82f4eb3 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\View\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 ee014d0fdb0..7dd3b9c65d4 100644 --- a/src/Control/HTTPRequest.php +++ b/src/Control/HTTPRequest.php @@ -7,6 +7,8 @@ use InvalidArgumentException; use SilverStripe\Core\ClassInfo; use SilverStripe\ORM\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 c7fbeed4503..8f37153985b 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/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 1b87e2169fb..453b501c6f1 100644 --- a/src/Security/Member.php +++ b/src/Security/Member.php @@ -44,6 +44,8 @@ use SilverStripe\Forms\SearchableDropdownField; use SilverStripe\Forms\SearchableMultiDropdownField; use SilverStripe\ORM\FieldType\DBForeignKey; +use SilverStripe\Security\Validation\PasswordValidator; +use SilverStripe\Security\Validation\RulesPasswordValidator; /** * The member class which represents the users of the system @@ -378,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); @@ -394,10 +394,8 @@ 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 Injector::inst()->get(PasswordValidator::class); @@ -1763,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..216a5390a58 --- /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..3e566e304f6 --- /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 54% rename from src/Security/PasswordValidator.php rename to src/Security/Validation/RulesPasswordValidator.php index 62288201dc0..5dbf5602680 100644 --- a/src/Security/PasswordValidator.php +++ b/src/Security/Validation/RulesPasswordValidator.php @@ -1,36 +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); * */ -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]/', @@ -38,80 +34,62 @@ class PasswordValidator ]; /** - * @config - * @var int + * Default minimum number of characters for a valid password. */ - private static $min_length = 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 $min_test_score = null; + private static int $min_test_score = 0; - /** - * @config - * @var int - */ - private static $historic_count = null; + protected ?int $minLength = null; - /** - * @var int - */ - protected $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; - - /** - * @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; @@ -122,7 +100,7 @@ public function setMinTestScore($minScore) * * @return string[] */ - public function getTestNames() + public function getTestNames(): array { if ($this->testNames !== null) { return $this->testNames; @@ -134,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(); @@ -222,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 ac614dc4bd3..29218f0abad 100644 --- a/tests/php/Forms/ConfirmedPasswordFieldTest.php +++ b/tests/php/Forms/ConfirmedPasswordFieldTest.php @@ -11,7 +11,6 @@ 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; @@ -24,9 +23,7 @@ protected function setUp(): void { parent::setUp(); - PasswordValidator::singleton() - ->setMinLength(0) - ->setTestNames([]); + Member::set_password_validator(null); } public function testSetValue() diff --git a/tests/php/Security/MemberAuthenticatorTest.php b/tests/php/Security/MemberAuthenticatorTest.php index 53db67188cb..3bcfc636214 100644 --- a/tests/php/Security/MemberAuthenticatorTest.php +++ b/tests/php/Security/MemberAuthenticatorTest.php @@ -18,7 +18,6 @@ use SilverStripe\Security\MemberAuthenticator\CMSMemberLoginForm; use SilverStripe\Security\MemberAuthenticator\MemberAuthenticator; use SilverStripe\Security\MemberAuthenticator\MemberLoginForm; -use SilverStripe\Security\PasswordValidator; use SilverStripe\Security\Security; class MemberAuthenticatorTest extends SapphireTest @@ -43,10 +42,8 @@ protected function setUp(): void } DefaultAdminService::setDefaultAdmin('admin', 'password'); - // Enforce dummy validation (this can otherwise be influenced by recipe config) - 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 e768f612537..c9cefdee1d5 100644 --- a/tests/php/Security/MemberCsvBulkLoaderTest.php +++ b/tests/php/Security/MemberCsvBulkLoaderTest.php @@ -6,7 +6,6 @@ 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; @@ -18,10 +17,7 @@ class MemberCsvBulkLoaderTest extends SapphireTest protected function setUp(): void { parent::setUp(); - - 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 ebe5c03759e..b55ded3414a 100644 --- a/tests/php/Security/MemberTest.php +++ b/tests/php/Security/MemberTest.php @@ -28,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; @@ -36,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 { @@ -74,10 +75,6 @@ protected function setUp(): void Member::config()->set('unique_identifier_field', 'Email'); - PasswordValidator::singleton() - ->setMinLength(0) - ->setTestNames([]); - i18n::set_locale('en_US'); } @@ -1767,11 +1764,23 @@ public function testEmailIsTrimmed() $this->assertNotNull(Member::get()->find('Email', 'trimmed@test.com')); } - public function testChangePasswordToBlankIsValidated() + public static function provideChangePasswordToBlankIsValidated(): array + { + return [ + [ + 'validatorClass' => RulesPasswordValidator::class, + ], + [ + 'validatorClass' => EntropyPasswordValidator::class, + ], + ]; + } + + #[DataProvider('provideChangePasswordToBlankIsValidated')] + public function testChangePasswordToBlankIsValidated(string $validatorClass): void { - Member::set_password_validator(new PasswordValidator()); - // override setup() function which setMinLength(0) - PasswordValidator::singleton()->setMinLength(8); + $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(''); @@ -1893,7 +1902,19 @@ public static function provideMapInCMSGroups() ]; } - public function testGenerateRandomPassword() + public static function provideGenerateRandomPassword(): array + { + return [ + [ + 'validatorClass' => RulesPasswordValidator::class, + ], + [ + 'validatorClass' => EntropyPasswordValidator::class, + ], + ]; + } + + public function testGenerateRandomPassword(string $validatorClass): void { $member = new Member(); // no password validator @@ -1905,22 +1926,32 @@ public function testGenerateRandomPassword() $password = $member->generateRandomPassword(); $this->assertSame(20, strlen($password)); // password validator - $validator = 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 fbb4c727b66..3ace28b403f 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\ORM\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 bad715f7b7b..65fe7766b9b 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); @@ -26,7 +22,7 @@ protected function setUp(): void public function testValidate() { - $v = new PasswordValidator(); + $v = new RulesPasswordValidator(); $r = $v->validate('', new Member()); $this->assertTrue($r->isValid(), 'Empty password is valid by default'); @@ -36,7 +32,7 @@ public function testValidate() public function testValidateMinLength() { - $v = new PasswordValidator(); + $v = new RulesPasswordValidator(); $v->setMinLength(4); $r = $v->validate('123', new Member()); @@ -50,7 +46,7 @@ public function testValidateMinLength() public function testValidateMinScore() { // Set both score and set of tests - $v = new PasswordValidator(); + $v = new RulesPasswordValidator(); $v->setMinTestScore(3); $v->setTestNames(["lowercase", "uppercase", "digits", "punctuation"]); @@ -61,7 +57,7 @@ public function testValidateMinScore() $this->assertTrue($r->isValid(), 'Passing enough tests'); // Ensure min score without tests works (uses default tests) - $v = new PasswordValidator(); + $v = new RulesPasswordValidator(); $v->setMinTestScore(3); $r = $v->validate('aA', new Member()); @@ -75,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 = 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 a2693156796..faf6aa6e8c8 100644 --- a/tests/php/Security/VersionedMemberAuthenticatorTest.php +++ b/tests/php/Security/VersionedMemberAuthenticatorTest.php @@ -3,23 +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\SapphireTest; use SilverStripe\ORM\FieldType\DBDatetime; use SilverStripe\ORM\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 @@ -42,10 +30,8 @@ protected function setUp(): void return; } - // Enforce dummy validation (this can otherwise be influenced by recipe config) - PasswordValidator::singleton() - ->setMinLength(0) - ->setTestNames([]); + // Remove password validation + Member::set_password_validator(null); } protected function tearDown(): void