diff --git a/src/Security/PasswordValidator.php b/src/Security/PasswordValidator.php index e07ef6fbb..2358aba1b 100644 --- a/src/Security/PasswordValidator.php +++ b/src/Security/PasswordValidator.php @@ -39,39 +39,39 @@ class PasswordValidator /** * @config - * @var integer + * @var int */ private static $min_length = null; /** * @config - * @var integer + * @var int */ private static $min_test_score = null; /** * @config - * @var integer + * @var int */ private static $historic_count = null; /** - * @var integer + * @var int */ protected $minLength = null; /** - * @var integer + * @var int */ protected $minScore = null; /** - * @var array + * @var string[] */ protected $testNames = null; /** - * @var integer + * @var int */ protected $historicalPasswordCount = null; @@ -95,18 +95,20 @@ class PasswordValidator * Eg: $this->characterStrength(3, array("lowercase", "uppercase", "digits", "punctuation")) * * @param int $minScore The minimum number of character tests that must pass - * @param array $testNames The names of the tests to perform + * @param string[] $testNames The names of the tests to perform * @return $this */ public function characterStrength($minScore, $testNames = null) { - Deprecation::notice( '5.0', 'Use ->setMinTestScore($score) and ->setTextNames($names) instead.' ); - return $this->setMinTestScore($minScore) - ->setTestNames($testNames); + $this->setMinTestScore($minScore); + if ($testNames) { + $this->setTestNames($testNames); + } + return $this; } /** @@ -123,7 +125,7 @@ class PasswordValidator } /** - * @return integer + * @return int */ public function getMinLength() { @@ -134,7 +136,7 @@ class PasswordValidator } /** - * @param $minLength + * @param int $minLength * @return $this */ public function setMinLength($minLength) @@ -155,7 +157,7 @@ class PasswordValidator } /** - * @param $minScore + * @param int $minScore * @return $this */ public function setMinTestScore($minScore) @@ -165,7 +167,9 @@ class PasswordValidator } /** - * @return array + * Gets the list of tests to use for this validator + * + * @return string[] */ public function getTestNames() { @@ -176,7 +180,9 @@ class PasswordValidator } /** - * @param $testNames + * Set list of tests to use for this validator + * + * @param string[] $testNames * @return $this */ public function setTestNames($testNames) @@ -186,7 +192,7 @@ class PasswordValidator } /** - * @return integer + * @return int */ public function getHistoricCount() { @@ -197,7 +203,7 @@ class PasswordValidator } /** - * @param $count + * @param int $count * @return $this */ public function setHistoricCount($count) @@ -207,6 +213,8 @@ class PasswordValidator } /** + * Gets all possible tests + * * @return array */ public function getTests() @@ -226,7 +234,7 @@ class PasswordValidator $minLength = $this->getMinLength(); if ($minLength && strlen($password) < $minLength) { $error = _t( - 'SilverStripe\\Security\\PasswordValidator.TOOSHORT', + __CLASS__ . '.TOOSHORT', 'Password is too short, it must be {minimum} or more characters long', ['minimum' => $this->minLength] ); @@ -245,16 +253,16 @@ class PasswordValidator continue; } $missedTests[] = _t( - 'SilverStripe\\Security\\PasswordValidator.STRENGTHTEST' . strtoupper($name), + __CLASS__ . '.STRENGTHTEST' . strtoupper($name), $name, 'The user needs to add this to their password for more complexity' ); } - $score = count($this->testNames) - count($missedTests); - if ($score < $minTestScore) { + $score = count($testNames) - count($missedTests); + if ($missedTests && $score < $minTestScore) { $error = _t( - 'SilverStripe\\Security\\PasswordValidator.LOWCHARSTRENGTH', + __CLASS__ . '.LOWCHARSTRENGTH', 'Please increase password strength by adding some of the following characters: {chars}', ['chars' => implode(', ', $missedTests)] ); @@ -271,8 +279,8 @@ class PasswordValidator /** @var MemberPassword $previousPassword */ foreach ($previousPasswords as $previousPassword) { if ($previousPassword->checkPassword($password)) { - $error = _t( - 'SilverStripe\\Security\\PasswordValidator.PREVPASSWORD', + $error = _t( + __CLASS__ . '.PREVPASSWORD', 'You\'ve already used that password in the past, please choose a new password' ); $valid->addError($error, 'bad', 'PREVIOUS_PASSWORD'); diff --git a/tests/php/Security/MemberTest/TestPasswordValidator.php b/tests/php/Security/MemberTest/TestPasswordValidator.php index 6e7270a8c..8907b7830 100644 --- a/tests/php/Security/MemberTest/TestPasswordValidator.php +++ b/tests/php/Security/MemberTest/TestPasswordValidator.php @@ -8,8 +8,9 @@ class TestPasswordValidator extends PasswordValidator { public function __construct() { - $this->minLength(7); - $this->checkHistoricalPasswords(6); - $this->characterStrength(3, array('lowercase', 'uppercase', 'digits', 'punctuation')); + $this->setMinLength(7); + $this->setHistoricCount(6); + $this->setMinTestScore(3); + $this->setTestNames(['lowercase', 'uppercase', 'digits', 'punctuation']); } } diff --git a/tests/php/Security/PasswordValidatorTest.php b/tests/php/Security/PasswordValidatorTest.php index 35824e036..4fd99ed08 100644 --- a/tests/php/Security/PasswordValidatorTest.php +++ b/tests/php/Security/PasswordValidatorTest.php @@ -28,25 +28,42 @@ class PasswordValidatorTest extends SapphireTest { $v = new PasswordValidator(); - $v->minLength(4); + $v->setMinLength(4); $r = $v->validate('123', new Member()); $this->assertFalse($r->isValid(), 'Password too short'); - $v->minLength(4); + $v->setMinLength(4); $r = $v->validate('1234', new Member()); $this->assertTrue($r->isValid(), 'Password long enough'); } public function testValidateMinScore() { + // Set both score and set of tests $v = new PasswordValidator(); - $v->characterStrength(3, array("lowercase", "uppercase", "digits", "punctuation")); + $v->setMinTestScore(3); + $v->setTestNames(["lowercase", "uppercase", "digits", "punctuation"]); $r = $v->validate('aA', new Member()); $this->assertFalse($r->isValid(), 'Passing too few tests'); $r = $v->validate('aA1', new Member()); $this->assertTrue($r->isValid(), 'Passing enough tests'); + + // Ensure min score without tests works (uses default tests) + $v = new PasswordValidator(); + $v->setMinTestScore(3); + + $r = $v->validate('aA', new Member()); + $this->assertFalse($r->isValid(), 'Passing too few tests'); + + $r = $v->validate('aA1', new Member()); + $this->assertTrue($r->isValid(), 'Passing enough tests'); + + // Ensure that min score is only triggered if there are any failing tests at all + $v->setMinTestScore(1000); + $r = $v->validate('aA1!', new Member()); + $this->assertTrue($r->isValid(), 'Passing enough tests'); } /** @@ -55,7 +72,7 @@ class PasswordValidatorTest extends SapphireTest public function testHistoricalPasswordCount() { $validator = new PasswordValidator; - $validator->checkHistoricalPasswords(3); + $validator->setHistoricCount(3); Member::set_password_validator($validator); $member = new Member;