From: Gergő Tisza Date: Wed, 2 Jan 2019 06:17:30 +0000 (-0800) Subject: Add force option to password policy X-Git-Tag: 1.34.0-rc.0~3133^2 X-Git-Url: http://git.cyclocoop.org/%28?a=commitdiff_plain;h=f15ecc60cd944105e15d378d32249cfafc8e1eec;p=lhc%2Fweb%2Fwiklou.git Add force option to password policy Adds a way to set an array of options for a password policy. Currently there is one option, 'forceChange', which forces the user to change their password (if it fails the given check) before logging in. Bug: T118774 Change-Id: I28c31fc4eae08c3ac44eff3a05f5e785ce4b9e01 --- diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php index f7c3fce124..9a42419d86 100644 --- a/includes/DefaultSettings.php +++ b/includes/DefaultSettings.php @@ -4454,13 +4454,20 @@ $wgCentralIdLookupProvider = 'local'; * Password policy for the wiki. * Structured as * [ - * 'policies' => [ => [ => , ... ], ... ], + * 'policies' => [ => [ => , ... ], ... ], * 'checks' => [ => , ... ], * ] * where is a user group, is a password policy name * (arbitrary string) defined in the 'checks' part, is the - * PHP callable implementing the policy check, is a number, - * boolean or null that gets passed to the callback. + * PHP callable implementing the policy check, is an array + * of options with the following keys: + * - value: (number, boolean or null) the value to pass to the callback + * - forceChange: (bool, default false) if the password is invalid, do + * not let the user log in without changing the password + * As a shorthand for [ 'value' => ], simply can be written. + * When multiple password policies are defined for a user, the settings + * arrays are merged, and for fields which are set in both arrays, the + * larger value (as understood by PHP's 'max' method) is taken. * * A user's effective policy is the superset of all policy statements * from the policies for the groups where the user is a member. If more diff --git a/includes/auth/AbstractPasswordPrimaryAuthenticationProvider.php b/includes/auth/AbstractPasswordPrimaryAuthenticationProvider.php index 4096f1971a..b6fedcd583 100644 --- a/includes/auth/AbstractPasswordPrimaryAuthenticationProvider.php +++ b/includes/auth/AbstractPasswordPrimaryAuthenticationProvider.php @@ -121,9 +121,10 @@ abstract class AbstractPasswordPrimaryAuthenticationProvider $reset = $this->getPasswordResetData( $username, $data ); if ( !$reset && $this->config->get( 'InvalidPasswordReset' ) && !$status->isGood() ) { + $hard = $status->getValue()['forceChange'] ?? false; $reset = (object)[ - 'msg' => $status->getMessage( 'resetpass-validity-soft' ), - 'hard' => false, + 'msg' => $status->getMessage( $hard ? 'resetpass-validity' : 'resetpass-validity-soft' ), + 'hard' => $hard, ]; } diff --git a/includes/password/UserPasswordPolicy.php b/includes/password/UserPasswordPolicy.php index 0c52354f1d..9eb921dcfa 100644 --- a/includes/password/UserPasswordPolicy.php +++ b/includes/password/UserPasswordPolicy.php @@ -43,7 +43,7 @@ class UserPasswordPolicy { /** * @param array $policies * @param array $checks mapping statement to its checking function. Checking functions are - * called with the policy value for this user, the user object, and the password to check. + * called with the policy value for this user, the user object, and the password to check. */ public function __construct( array $policies, array $checks ) { if ( !isset( $policies['default'] ) ) { @@ -68,7 +68,9 @@ class UserPasswordPolicy { * @param User $user who's policy we are checking * @param string $password the password to check * @return Status error to indicate the password didn't meet the policy, or fatal to - * indicate the user shouldn't be allowed to login. + * indicate the user shouldn't be allowed to login. The status value will be an array, + * potentially with the following keys: + * - forceChange: do not allow the user to login without changing the password if invalid. */ public function checkUserPassword( User $user, $password ) { $effectivePolicy = $this->getPoliciesForUser( $user ); @@ -88,7 +90,9 @@ class UserPasswordPolicy { * @param string $password the password to check * @param array $groups list of groups to which we assume the user belongs * @return Status error to indicate the password didn't meet the policy, or fatal to - * indicate the user shouldn't be allowed to login. + * indicate the user shouldn't be allowed to login. The status value will be an array, + * potentially with the following keys: + * - forceChange: do not allow the user to login without changing the password if invalid. */ public function checkUserPasswordForGroups( User $user, $password, array $groups ) { $effectivePolicy = self::getPoliciesForGroups( @@ -112,19 +116,34 @@ class UserPasswordPolicy { * @return Status */ private function checkPolicies( User $user, $password, $policies, $policyCheckFunctions ) { - $status = Status::newGood(); - foreach ( $policies as $policy => $value ) { + $status = Status::newGood( [] ); + $forceChange = false; + foreach ( $policies as $policy => $settings ) { if ( !isset( $policyCheckFunctions[$policy] ) ) { throw new DomainException( "Invalid password policy config. No check defined for '$policy'." ); } - $status->merge( - call_user_func( - $policyCheckFunctions[$policy], - $value, - $user, - $password - ) + if ( !is_array( $settings ) ) { + // legacy format + $settings = [ 'value' => $settings ]; + } + if ( !array_key_exists( 'value', $settings ) ) { + throw new DomainException( "Invalid password policy config. No value defined for '$policy'." ); + } + $value = $settings['value']; + /** @var StatusValue $policyStatus */ + $policyStatus = call_user_func( + $policyCheckFunctions[$policy], + $value, + $user, + $password ); + if ( !$policyStatus->isGood() && !empty( $settings['forceChange'] ) ) { + $forceChange = true; + } + $status->merge( $policyStatus ); + } + if ( $status->isOK() && $forceChange ) { + $status->value['forceChange'] = true; } return $status; } @@ -174,6 +193,7 @@ class UserPasswordPolicy { /** * Utility function to get a policy that is the most restrictive of $p1 and $p2. For * simplicity, we setup the policy values so the maximum value is always more restrictive. + * It is also used recursively to merge settings within the same policy. * @param array $p1 * @param array $p2 * @return array containing the more restrictive values of $p1 and $p2 @@ -186,8 +206,15 @@ class UserPasswordPolicy { $ret[$key] = $p2[$key]; } elseif ( !isset( $p2[$key] ) ) { $ret[$key] = $p1[$key]; - } else { + } elseif ( !is_array( $p1[$key] ) && !is_array( $p2[$key] ) ) { $ret[$key] = max( $p1[$key], $p2[$key] ); + } else { + if ( !is_array( $p1[$key] ) ) { + $p1[$key] = [ 'value' => $p1[$key] ]; + } elseif ( !is_array( $p2[$key] ) ) { + $p2[$key] = [ 'value' => $p2[$key] ]; + } + $ret[$key] = self::maxOfPolicies( $p1[$key], $p2[$key] ); } } return $ret; diff --git a/includes/user/User.php b/includes/user/User.php index ec75f052e5..0fe2db3f79 100644 --- a/includes/user/User.php +++ b/includes/user/User.php @@ -1179,15 +1179,17 @@ class User implements IDBAccessObject, UserIdentity { /** * Check if this is a valid password for this user * - * Create a Status object based on the password's validity. - * The Status should be set to fatal if the user should not - * be allowed to log in, and should have any errors that - * would block changing the password. - * - * If the return value of this is not OK, the password - * should not be checked. If the return value is not Good, - * the password can be checked, but the user should not be - * able to set their password to this. + * Returns a Status object with a set of messages describing + * problems with the password. If the return status is fatal, + * the action should be refused and the password should not be + * checked at all (this is mainly meant for DoS mitigation). + * If the return value is OK but not good, the password can be checked, + * but the user should not be able to set their password to this. + * The value of the returned Status object will be an array which + * can have the following fields: + * - forceChange (bool): if set to true, the user should not be + * allowed to log with this password unless they change it during + * the login process (see ResetPasswordSecondaryAuthenticationProvider). * * @param string $password Desired password * @return Status @@ -1201,7 +1203,7 @@ class User implements IDBAccessObject, UserIdentity { $wgPasswordPolicy['checks'] ); - $status = Status::newGood(); + $status = Status::newGood( [] ); $result = false; // init $result to false for the internal checks if ( !Hooks::run( 'isValidPassword', [ $password, &$result, $this ] ) ) { @@ -1210,7 +1212,7 @@ class User implements IDBAccessObject, UserIdentity { } if ( $result === false ) { - $status->merge( $upp->checkUserPassword( $this, $password ) ); + $status->merge( $upp->checkUserPassword( $this, $password ), true ); return $status; } elseif ( $result === true ) { return $status; diff --git a/languages/i18n/en.json b/languages/i18n/en.json index 62d7d7b771..a2acb637d6 100644 --- a/languages/i18n/en.json +++ b/languages/i18n/en.json @@ -581,6 +581,7 @@ "resetpass-abort-generic": "Password change has been aborted by an extension.", "resetpass-expired": "Your password has expired. Please set a new password to log in.", "resetpass-expired-soft": "Your password has expired and needs to be changed. Please choose a new password now, or click \"{{int:authprovider-resetpass-skip-label}}\" to change it later.", + "resetpass-validity": "Your password is not valid: $1\n\nPlease set a new password to log in.", "resetpass-validity-soft": "Your password is not valid: $1\n\nPlease choose a new password now, or click \"{{int:authprovider-resetpass-skip-label}}\" to change it later.", "passwordreset": "Reset password", "passwordreset-text-one": "Complete this form to receive a temporary password via email.", diff --git a/languages/i18n/qqq.json b/languages/i18n/qqq.json index 715ab59817..cca9f312fa 100644 --- a/languages/i18n/qqq.json +++ b/languages/i18n/qqq.json @@ -784,6 +784,7 @@ "resetpass-abort-generic": "Generic error message shown on [[Special:ChangePassword]] when an extension aborts a password change from a hook.", "resetpass-expired": "Generic error message shown on [[Special:ChangePassword]] when a user's password is expired", "resetpass-expired-soft": "Generic warning message shown on [[Special:ChangePassword]] when a user needs to reset their password, but they are not prevented from logging in at this time", + "resetpass-validity": "Warning message shown on [[Special:ChangePassword]] when a user needs to reset their password, because their password is not valid.\n\nParameters:\n* $1 - error message", "resetpass-validity-soft": "Warning message shown on [[Special:ChangePassword]] when a user needs to reset their password, because their password is not valid.\n\nRefers to {{msg-mw|authprovider-resetpass-skip-label}}.\n\nParameters:\n* $1 - error message", "passwordreset": "Title of [[Special:PasswordReset]].\n{{Identical|Reset password}}", "passwordreset-text-one": "Text on [[Special:PasswordReset]] that appears when there is only one way of resetting the password.\n\n{{msg-mw|Passwordreset-text-many}} will be used, when there are multiple ways of resetting the password.", diff --git a/tests/phpunit/includes/auth/AbstractPasswordPrimaryAuthenticationProviderTest.php b/tests/phpunit/includes/auth/AbstractPasswordPrimaryAuthenticationProviderTest.php index 5f1b9fe96e..e67d405448 100644 --- a/tests/phpunit/includes/auth/AbstractPasswordPrimaryAuthenticationProviderTest.php +++ b/tests/phpunit/includes/auth/AbstractPasswordPrimaryAuthenticationProviderTest.php @@ -88,7 +88,7 @@ class AbstractPasswordPrimaryAuthenticationProviderTest extends \MediaWikiTestCa public function testCheckPasswordValidity() { $uppCalled = 0; - $uppStatus = \Status::newGood(); + $uppStatus = \Status::newGood( [] ); $this->setMwGlobals( [ 'wgPasswordPolicy' => [ 'policies' => [ diff --git a/tests/phpunit/includes/auth/LocalPasswordPrimaryAuthenticationProviderTest.php b/tests/phpunit/includes/auth/LocalPasswordPrimaryAuthenticationProviderTest.php index b7e86c80a8..e1b25a1ebd 100644 --- a/tests/phpunit/includes/auth/LocalPasswordPrimaryAuthenticationProviderTest.php +++ b/tests/phpunit/includes/auth/LocalPasswordPrimaryAuthenticationProviderTest.php @@ -174,6 +174,16 @@ class LocalPasswordPrimaryAuthenticationProviderTest extends \MediaWikiTestCase $this->assertNotNull( $ret ); $this->assertSame( 'resetpass-validity-soft', $ret->msg->getKey() ); $this->assertFalse( $ret->hard ); + + $this->manager->removeAuthenticationSessionData( null ); + $row->user_password_expires = null; + $status = \Status::newGood( [ 'forceChange' => true ] ); + $status->error( 'testing' ); + $providerPriv->setPasswordResetFlag( $userName, $status, $row ); + $ret = $this->manager->getAuthenticationSessionData( 'reset-pass' ); + $this->assertNotNull( $ret ); + $this->assertSame( 'resetpass-validity', $ret->msg->getKey() ); + $this->assertTrue( $ret->hard ); } public function testAuthentication() { diff --git a/tests/phpunit/includes/password/UserPasswordPolicyTest.php b/tests/phpunit/includes/password/UserPasswordPolicyTest.php index 78175fac8f..80881ad5ec 100644 --- a/tests/phpunit/includes/password/UserPasswordPolicyTest.php +++ b/tests/phpunit/includes/password/UserPasswordPolicyTest.php @@ -30,7 +30,7 @@ class UserPasswordPolicyTest extends MediaWikiTestCase { protected $policies = [ 'checkuser' => [ - 'MinimalPasswordLength' => 10, + 'MinimalPasswordLength' => [ 'value' => 10, 'forceChange' => true ], 'MinimumPasswordLengthToLogin' => 6, 'PasswordCannotMatchUsername' => true, ], @@ -44,6 +44,8 @@ class UserPasswordPolicyTest extends MediaWikiTestCase { 'MinimumPasswordLengthToLogin' => 1, 'PasswordCannotMatchBlacklist' => true, 'MaximalPasswordLength' => 4096, + // test null handling + 'PasswordCannotMatchUsername' => null, ], ]; @@ -62,15 +64,24 @@ class UserPasswordPolicyTest extends MediaWikiTestCase { public function testGetPoliciesForUser() { $upp = $this->getUserPasswordPolicy(); - $user = User::newFromName( 'TestUserPolicy' ); - $user->addToDatabase(); - $user->addGroup( 'sysop' ); - + $user = $this->getTestUser( [ 'sysop' ] )->getUser(); $this->assertArrayEquals( [ 'MinimalPasswordLength' => 8, 'MinimumPasswordLengthToLogin' => 1, - 'PasswordCannotMatchUsername' => 1, + 'PasswordCannotMatchUsername' => true, + 'PasswordCannotMatchBlacklist' => true, + 'MaximalPasswordLength' => 4096, + ], + $upp->getPoliciesForUser( $user ) + ); + + $user = $this->getTestUser( [ 'sysop', 'checkuser' ] )->getUser(); + $this->assertArrayEquals( + [ + 'MinimalPasswordLength' => [ 'value' => 10, 'forceChange' => true ], + 'MinimumPasswordLengthToLogin' => 6, + 'PasswordCannotMatchUsername' => true, 'PasswordCannotMatchBlacklist' => true, 'MaximalPasswordLength' => 4096, ], @@ -87,7 +98,7 @@ class UserPasswordPolicyTest extends MediaWikiTestCase { $this->assertArrayEquals( [ - 'MinimalPasswordLength' => 10, + 'MinimalPasswordLength' => [ 'value' => 10, 'forceChange' => true ], 'MinimumPasswordLengthToLogin' => 6, 'PasswordCannotMatchUsername' => true, 'PasswordCannotMatchBlacklist' => true, @@ -100,108 +111,96 @@ class UserPasswordPolicyTest extends MediaWikiTestCase { /** * @dataProvider provideCheckUserPassword */ - public function testCheckUserPassword( $username, $groups, $password, $valid, $ok, $msg ) { + public function testCheckUserPassword( $groups, $password, StatusValue $expectedStatus ) { $upp = $this->getUserPasswordPolicy(); - - $user = User::newFromName( $username ); - $user->addToDatabase(); - foreach ( $groups as $group ) { - $user->addGroup( $group ); - } + $user = $this->getTestUser( $groups )->getUser(); $status = $upp->checkUserPassword( $user, $password ); - $this->assertSame( $valid, $status->isGood(), $msg . ' - password valid' ); - $this->assertSame( $ok, $status->isOK(), $msg . ' - can login' ); + $this->assertSame( $expectedStatus->isGood(), $status->isGood(), 'password valid' ); + $this->assertSame( $expectedStatus->isOK(), $status->isOK(), 'can login' ); + $this->assertSame( $expectedStatus->getValue(), $status->getValue(), 'flags' ); } public function provideCheckUserPassword() { + $success = Status::newGood( [] ); + $warning = Status::newGood( [] ); + $forceChange = Status::newGood( [ 'forceChange' => true ] ); + $fatal = Status::newGood( [] ); + // the message does not matter, we only test for state and value + $warning->warning( 'invalid-password' ); + $forceChange->warning( 'invalid-password' ); + $warning->warning( 'invalid-password' ); + $fatal->fatal( 'invalid-password' ); return [ - [ - 'PassPolicyUser', + 'No groups, default policy, password too short to login' => [ [], '', - false, - false, - 'No groups, default policy, password too short to login' + $fatal, ], - [ - 'PassPolicyUser', + 'Default policy, short password' => [ [ 'user' ], 'aaa', - false, - true, - 'Default policy, short password' + $warning, ], - [ - 'PassPolicyUser', + 'Sysop with good password' => [ [ 'sysop' ], 'abcdabcdabcd', - true, - true, - 'Sysop with good password' + $success, ], - [ - 'PassPolicyUser', + 'Sysop with short password' => [ [ 'sysop' ], 'abcd', - false, - true, - 'Sysop with short password' + $warning, ], - [ - 'PassPolicyUser', + 'Checkuser with short password' => [ [ 'sysop', 'checkuser' ], 'abcdabcd', - false, - true, - 'Checkuser with short password' + $forceChange, ], - [ - 'PassPolicyUser', + 'Checkuser with too short password to login' => [ [ 'sysop', 'checkuser' ], 'abcd', - false, - false, - 'Checkuser with too short password to login' - ], - [ - 'Useruser', - [ 'user' ], - 'Passpass', - false, - true, - 'Username & password on blacklist' + $fatal, ], ]; } + public function testCheckUserPassword_blacklist() { + $upp = $this->getUserPasswordPolicy(); + $user = User::newFromName( 'Useruser' ); + $user->addToDatabase(); + + $status = $upp->checkUserPassword( $user, 'Passpass' ); + $this->assertFalse( $status->isGood(), 'password invalid' ); + $this->assertTrue( $status->isOK(), 'can login' ); + } + /** * @dataProvider provideMaxOfPolicies */ - public function testMaxOfPolicies( $p1, $p2, $max, $msg ) { + public function testMaxOfPolicies( $p1, $p2, $max ) { $this->assertArrayEquals( $max, - UserPasswordPolicy::maxOfPolicies( $p1, $p2 ), - $msg + UserPasswordPolicy::maxOfPolicies( $p1, $p2 ) ); } public function provideMaxOfPolicies() { return [ - [ + 'Basic max in p1' => [ [ 'MinimalPasswordLength' => 8 ], // p1 [ 'MinimalPasswordLength' => 2 ], // p2 [ 'MinimalPasswordLength' => 8 ], // max - 'Basic max in p1' ], - [ + 'Basic max in p2' => [ [ 'MinimalPasswordLength' => 2 ], // p1 [ 'MinimalPasswordLength' => 8 ], // p2 [ 'MinimalPasswordLength' => 8 ], // max - 'Basic max in p2' ], - [ - [ 'MinimalPasswordLength' => 8 ], // p1 + 'Missing items in p1' => [ + [ + 'MinimalPasswordLength' => 8, + ], // p1 [ 'MinimalPasswordLength' => 2, 'PasswordCannotMatchUsername' => 1, @@ -210,9 +209,8 @@ class UserPasswordPolicyTest extends MediaWikiTestCase { 'MinimalPasswordLength' => 8, 'PasswordCannotMatchUsername' => 1, ], // max - 'Missing items in p1' ], - [ + 'Missing items in p2' => [ [ 'MinimalPasswordLength' => 8, 'PasswordCannotMatchUsername' => 1, @@ -224,7 +222,64 @@ class UserPasswordPolicyTest extends MediaWikiTestCase { 'MinimalPasswordLength' => 8, 'PasswordCannotMatchUsername' => 1, ], // max - 'Missing items in p2' + ], + 'complex value in p1' => [ + [ + 'MinimalPasswordLength' => [ + 'value' => 8, + 'foo' => 1, + ], + ], // p1 + [ + 'MinimalPasswordLength' => 2, + ], // p2 + [ + 'MinimalPasswordLength' => [ + 'value' => 8, + 'foo' => 1, + ], + ], // max + ], + 'complex value in p2' => [ + [ + 'MinimalPasswordLength' => 8, + ], // p1 + [ + 'MinimalPasswordLength' => [ + 'value' => 2, + 'foo' => 1, + ], + ], // p2 + [ + 'MinimalPasswordLength' => [ + 'value' => 8, + 'foo' => 1, + ], + ], // max + ], + 'complex value in both p1 and p2' => [ + [ + 'MinimalPasswordLength' => [ + 'value' => 8, + 'foo' => 1, + 'baz' => false, + ], + ], // p1 + [ + 'MinimalPasswordLength' => [ + 'value' => 2, + 'bar' => 2, + 'baz' => true, + ], + ], // p2 + [ + 'MinimalPasswordLength' => [ + 'value' => 8, + 'foo' => 1, + 'bar' => 2, + 'baz' => true, + ], + ], // max ], ]; }