diff --git a/src/Security/Member.php b/src/Security/Member.php index 40bdd9389..40168e1e8 100644 --- a/src/Security/Member.php +++ b/src/Security/Member.php @@ -1659,7 +1659,8 @@ class Member extends DataObject if (!$this->ID || $this->isChanged('Password')) { if ($this->Password && $validator) { - $valid->combineAnd($validator->validate($this->Password, $this)); + $userValid = $validator->validate($this->Password, $this); + $valid->combineAnd($userValid); } } diff --git a/src/Security/PasswordValidator.php b/src/Security/PasswordValidator.php index c9e4ce0df..e07ef6fbb 100644 --- a/src/Security/PasswordValidator.php +++ b/src/Security/PasswordValidator.php @@ -3,7 +3,9 @@ namespace SilverStripe\Security; use SilverStripe\Core\Config\Configurable; +use SilverStripe\Core\Extensible; use SilverStripe\Core\Injector\Injectable; +use SilverStripe\Dev\Deprecation; use SilverStripe\ORM\ValidationResult; /** @@ -22,21 +24,59 @@ class PasswordValidator { use Injectable; use Configurable; + use Extensible; /** * @config * @var array */ - private static $character_strength_tests = array( + private static $character_strength_tests = [ 'lowercase' => '/[a-z]/', 'uppercase' => '/[A-Z]/', 'digits' => '/[0-9]/', 'punctuation' => '/[^A-Za-z0-9]/', - ); - - protected $minLength, $minScore, $testNames, $historicalPasswordCount; + ]; /** + * @config + * @var integer + */ + private static $min_length = null; + + /** + * @config + * @var integer + */ + private static $min_test_score = null; + + /** + * @config + * @var integer + */ + private static $historic_count = null; + + /** + * @var integer + */ + protected $minLength = null; + + /** + * @var integer + */ + protected $minScore = null; + + /** + * @var array + */ + protected $testNames = null; + + /** + * @var integer + */ + protected $historicalPasswordCount = null; + + /** + * @deprecated 5.0 * Minimum password length * * @param int $minLength @@ -44,11 +84,12 @@ class PasswordValidator */ public function minLength($minLength) { - $this->minLength = $minLength; - return $this; + Deprecation::notice('5.0', 'Use ->setMinLength($value) instead.'); + return $this->setMinLength($minLength); } /** + * @deprecated 5.0 * Check the character strength of the password. * * Eg: $this->characterStrength(3, array("lowercase", "uppercase", "digits", "punctuation")) @@ -57,25 +98,122 @@ class PasswordValidator * @param array $testNames The names of the tests to perform * @return $this */ - public function characterStrength($minScore, $testNames) + public function characterStrength($minScore, $testNames = null) { - $this->minScore = $minScore; - $this->testNames = $testNames; - return $this; + + Deprecation::notice( + '5.0', + 'Use ->setMinTestScore($score) and ->setTextNames($names) instead.' + ); + return $this->setMinTestScore($minScore) + ->setTestNames($testNames); } /** + * @deprecated 5.0 * Check a number of previous passwords that the user has used, and don't let them change to that. * * @param int $count * @return $this */ public function checkHistoricalPasswords($count) + { + Deprecation::notice('5.0', 'Use ->setHistoricCount($value) instead.'); + return $this->setHistoricCount($count); + } + + /** + * @return integer + */ + public function getMinLength() + { + if ($this->minLength !== null) { + return $this->minLength; + } + return $this->config()->get('min_length'); + } + + /** + * @param $minLength + * @return $this + */ + public function setMinLength($minLength) + { + $this->minLength = $minLength; + return $this; + } + + /** + * @return integer + */ + public function getMinTestScore() + { + if ($this->minScore !== null) { + return $this->minScore; + } + return $this->config()->get('min_test_score'); + } + + /** + * @param $minScore + * @return $this + */ + public function setMinTestScore($minScore) + { + $this->minScore = $minScore; + return $this; + } + + /** + * @return array + */ + public function getTestNames() + { + if ($this->testNames !== null) { + return $this->testNames; + } + return array_keys(array_filter($this->getTests())); + } + + /** + * @param $testNames + * @return $this + */ + public function setTestNames($testNames) + { + $this->testNames = $testNames; + return $this; + } + + /** + * @return integer + */ + public function getHistoricCount() + { + if ($this->historicalPasswordCount !== null) { + return $this->historicalPasswordCount; + } + return $this->config()->get('historic_count'); + } + + /** + * @param $count + * @return $this + */ + public function setHistoricCount($count) { $this->historicalPasswordCount = $count; return $this; } + /** + * @return array + */ + public function getTests() + { + return $this->config()->get('character_strength_tests'); + } + /** * @param String $password * @param Member $member @@ -85,69 +223,66 @@ class PasswordValidator { $valid = ValidationResult::create(); - if ($this->minLength) { - if (strlen($password) < $this->minLength) { - $valid->addError( - _t( - 'SilverStripe\\Security\\PasswordValidator.TOOSHORT', - 'Password is too short, it must be {minimum} or more characters long', - ['minimum' => $this->minLength] - ), - 'bad', - 'TOO_SHORT' - ); - } + $minLength = $this->getMinLength(); + if ($minLength && strlen($password) < $minLength) { + $error = _t( + 'SilverStripe\\Security\\PasswordValidator.TOOSHORT', + 'Password is too short, it must be {minimum} or more characters long', + ['minimum' => $this->minLength] + ); + + $valid->addError($error, 'bad', 'TOO_SHORT'); } - if ($this->minScore) { - $score = 0; - $missedTests = array(); - foreach ($this->testNames as $name) { - if (preg_match(self::config()->character_strength_tests[$name], $password)) { - $score++; - } else { - $missedTests[] = _t( - 'SilverStripe\\Security\\PasswordValidator.STRENGTHTEST' . strtoupper($name), - $name, - 'The user needs to add this to their password for more complexity' - ); + $minTestScore = $this->getMinTestScore(); + if ($minTestScore) { + $missedTests = []; + $testNames = $this->getTestNames(); + $tests = $this->getTests(); + + foreach ($testNames as $name) { + if (preg_match($tests[$name], $password)) { + continue; } + $missedTests[] = _t( + 'SilverStripe\\Security\\PasswordValidator.STRENGTHTEST' . strtoupper($name), + $name, + 'The user needs to add this to their password for more complexity' + ); } - if ($score < $this->minScore) { - $valid->addError( - _t( - 'SilverStripe\\Security\\PasswordValidator.LOWCHARSTRENGTH', - 'Please increase password strength by adding some of the following characters: {chars}', - ['chars' => implode(', ', $missedTests)] - ), - 'bad', - 'LOW_CHARACTER_STRENGTH' + $score = count($this->testNames) - count($missedTests); + if ($score < $minTestScore) { + $error = _t( + 'SilverStripe\\Security\\PasswordValidator.LOWCHARSTRENGTH', + 'Please increase password strength by adding some of the following characters: {chars}', + ['chars' => implode(', ', $missedTests)] ); + $valid->addError($error, 'bad', 'LOW_CHARACTER_STRENGTH'); } } - if ($this->historicalPasswordCount) { + $historicCount = $this->getHistoricCount(); + if ($historicCount) { $previousPasswords = MemberPassword::get() ->where(array('"MemberPassword"."MemberID"' => $member->ID)) ->sort('"Created" DESC, "ID" DESC') - ->limit($this->historicalPasswordCount); + ->limit($historicCount); /** @var MemberPassword $previousPassword */ foreach ($previousPasswords as $previousPassword) { if ($previousPassword->checkPassword($password)) { - $valid->addError( - _t( - 'SilverStripe\\Security\\PasswordValidator.PREVPASSWORD', - 'You\'ve already used that password in the past, please choose a new password' - ), - 'bad', - 'PREVIOUS_PASSWORD' + $error = _t( + 'SilverStripe\\Security\\PasswordValidator.PREVPASSWORD', + 'You\'ve already used that password in the past, please choose a new password' ); + $valid->addError($error, 'bad', 'PREVIOUS_PASSWORD'); break; } } } + $this->extend('updateValidatePassword', $password, $member, $valid, $this); + return $valid; } } diff --git a/tests/php/Security/MemberTest.php b/tests/php/Security/MemberTest.php index 8476380ca..bc56fd6f6 100644 --- a/tests/php/Security/MemberTest.php +++ b/tests/php/Security/MemberTest.php @@ -259,8 +259,8 @@ class MemberTest extends FunctionalTest public function testValidatePassword() { /** - * @var Member $member -*/ + * @var Member $member + */ $member = $this->objFromFixture(Member::class, 'test'); $this->assertNotNull($member);