Thijs/wiki.techinc.nl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Add force option to password policy

Browse source 
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
This commit is contained in:
Gergő Tisza 2019-01-01 22:17:30 -08:00
parent 1d47891cc3
commit f15ecc60cd
No known key found for this signature in database
GPG key ID: C34FEC97E6257F96
9 changed files with 200 additions and 96 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

13
includes/DefaultSettings.php
View file

@ -4454,13 +4454,20 @@ $wgCentralIdLookupProvider = 'local';
* Password policy for the wiki.
* Structured as
* [
* 'policies' => [ <group> => [ <policy> => <value>, ... ], ... ],
* 'policies' => [ <group> => [ <policy> => <settings>, ... ], ... ],
* 'checks' => [ <policy> => <callback>, ... ],
* ]
* where <group> is a user group, <policy> is a password policy name
* (arbitrary string) defined in the 'checks' part, <callback> is the
* PHP callable implementing the policy check, <value> is a number,
* boolean or null that gets passed to the callback.
* PHP callable implementing the policy check, <settings> 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' => <value> ], simply <value> 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

5
includes/auth/AbstractPasswordPrimaryAuthenticationProvider.php
View file

@ -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,
];
}

53
includes/password/UserPasswordPolicy.php
View file

@ -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;

24
includes/user/User.php
View file

@ -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;

1
languages/i18n/en.json
View file

@ -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.",

1
languages/i18n/qqq.json
View file

@ -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.",

2
tests/phpunit/includes/auth/AbstractPasswordPrimaryAuthenticationProviderTest.php
View file

@ -88,7 +88,7 @@ class AbstractPasswordPrimaryAuthenticationProviderTest extends \MediaWikiTestCa
public function testCheckPasswordValidity() {
$uppCalled = 0;
$uppStatus = \Status::newGood();
$uppStatus = \Status::newGood( [] );
$this->setMwGlobals( [
'wgPasswordPolicy' => [
'policies' => [

10
tests/phpunit/includes/auth/LocalPasswordPrimaryAuthenticationProviderTest.php
View file

@ -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() {

187
tests/phpunit/includes/password/UserPasswordPolicyTest.php
View file

@ -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
],
];
}