From 4b39919c473e165071cf32198e534e4ef1e3576c Mon Sep 17 00:00:00 2001 From: Dayllan Maza Date: Wed, 20 Feb 2019 01:02:33 -0500 Subject: [PATCH] Add password policy setting `suggestChangeOnLogin` Password policy checks that fail and have `suggestChangeOnLogin` set to true will prompt for a password change on login. Below are some rules that apply to this setting in different scenarios: - If only one policy fails and has `suggestChangeOnLogin = false`, a password change will not be requested - If more than one policy fails and one or more have `suggestChangeOnLogin` set to true`, a password change will be requested - If `forceChange` is present in any of the failing policies, `suggestChangeOnLogin` value will be ignored and password change will be enforced - if $wgInvalidPasswordReset is set to false `suggestChangeOnLogin` is ignored IMPORTANT** Before this patch, suggesting a password change was the default behavior (depending on $wgInvalidPasswordReset), which means that the necessary changes to $wgPasswordPolicy need to be in place before this patch is merged and gets to production. Bug: T211621 Change-Id: I7a4a0a06273fa4e8bd0da3dac54cf5a1b78bb3fd --- includes/DefaultSettings.php | 13 ++-- ...tPasswordPrimaryAuthenticationProvider.php | 11 ++-- ...lPasswordPrimaryAuthenticationProvider.php | 7 ++ includes/password/UserPasswordPolicy.php | 24 +++++-- includes/user/User.php | 2 + languages/i18n/en.json | 1 + languages/i18n/qqq.json | 1 + ...swordPrimaryAuthenticationProviderTest.php | 2 +- ...swordPrimaryAuthenticationProviderTest.php | 14 +++- .../password/UserPasswordPolicyTest.php | 64 ++++++++++++++++--- .../structure/PasswordPolicyStructureTest.php | 7 +- 11 files changed, 120 insertions(+), 26 deletions(-) diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php index fb31249426..68bf1038a4 100644 --- a/includes/DefaultSettings.php +++ b/includes/DefaultSettings.php @@ -4445,6 +4445,11 @@ $wgCentralIdLookupProvider = 'local'; * - 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 + * - suggestChangeOnLogin: (bool, default false) if true and the password is + * invalid, suggest a password change if logging in. If all the failing policies + * that apply to the user have this set to false, the password change + * screen will not be shown. 'forceChange' takes precedence over + * 'suggestChangeOnLogin' if they are both present. * 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 @@ -4514,10 +4519,10 @@ $wgPasswordPolicy = [ 'PasswordNotInLargeBlacklist' => true, ], 'default' => [ - 'MinimalPasswordLength' => 1, - 'PasswordCannotMatchUsername' => true, - 'PasswordCannotMatchBlacklist' => true, - 'MaximalPasswordLength' => 4096, + 'MinimalPasswordLength' => [ 'value' => 1, 'suggestChangeOnLogin' => true ], + 'PasswordCannotMatchUsername' => [ 'value' => true, 'suggestChangeOnLogin' => true ], + 'PasswordCannotMatchBlacklist' => [ 'value' => true, 'suggestChangeOnLogin' => true ], + 'MaximalPasswordLength' => [ 'value' => 4096, 'suggestChangeOnLogin' => true ], ], ], 'checks' => [ diff --git a/includes/auth/AbstractPasswordPrimaryAuthenticationProvider.php b/includes/auth/AbstractPasswordPrimaryAuthenticationProvider.php index b6fedcd583..cd9eac4b02 100644 --- a/includes/auth/AbstractPasswordPrimaryAuthenticationProvider.php +++ b/includes/auth/AbstractPasswordPrimaryAuthenticationProvider.php @@ -122,10 +122,13 @@ abstract class AbstractPasswordPrimaryAuthenticationProvider if ( !$reset && $this->config->get( 'InvalidPasswordReset' ) && !$status->isGood() ) { $hard = $status->getValue()['forceChange'] ?? false; - $reset = (object)[ - 'msg' => $status->getMessage( $hard ? 'resetpass-validity' : 'resetpass-validity-soft' ), - 'hard' => $hard, - ]; + + if ( $hard || !empty( $status->getValue()['suggestChangeOnLogin'] ) ) { + $reset = (object)[ + 'msg' => $status->getMessage( $hard ? 'resetpass-validity' : 'resetpass-validity-soft' ), + 'hard' => $hard, + ]; + } } if ( $reset ) { diff --git a/includes/auth/LocalPasswordPrimaryAuthenticationProvider.php b/includes/auth/LocalPasswordPrimaryAuthenticationProvider.php index e9adb7ee32..047a8117b2 100644 --- a/includes/auth/LocalPasswordPrimaryAuthenticationProvider.php +++ b/includes/auth/LocalPasswordPrimaryAuthenticationProvider.php @@ -46,6 +46,13 @@ class LocalPasswordPrimaryAuthenticationProvider $this->loginOnly = !empty( $params['loginOnly'] ); } + /** + * Check if the password has expired and needs a reset + * + * @param string $username + * @param \stdClass $row A row from the user table + * @return \stdClass|null + */ protected function getPasswordResetData( $username, $row ) { $now = wfTimestamp(); $expiration = wfTimestampOrNull( TS_UNIX, $row->user_password_expires ); diff --git a/includes/password/UserPasswordPolicy.php b/includes/password/UserPasswordPolicy.php index c61c795fcb..79a553984b 100644 --- a/includes/password/UserPasswordPolicy.php +++ b/includes/password/UserPasswordPolicy.php @@ -71,6 +71,7 @@ class UserPasswordPolicy { * 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. + * - suggestChangeOnLogin: prompt for a password change on login if the password is invalid. */ public function checkUserPassword( User $user, $password ) { $effectivePolicy = $this->getPoliciesForUser( $user ); @@ -93,6 +94,7 @@ class UserPasswordPolicy { * 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. + * - suggestChangeOnLogin: prompt for a password change on login if the password is invalid. */ public function checkUserPasswordForGroups( User $user, $password, array $groups ) { $effectivePolicy = self::getPoliciesForGroups( @@ -118,6 +120,7 @@ class UserPasswordPolicy { private function checkPolicies( User $user, $password, $policies, $policyCheckFunctions ) { $status = Status::newGood( [] ); $forceChange = false; + $suggestChangeOnLogin = false; foreach ( $policies as $policy => $settings ) { if ( !isset( $policyCheckFunctions[$policy] ) ) { throw new DomainException( "Invalid password policy config. No check defined for '$policy'." ); @@ -137,14 +140,27 @@ class UserPasswordPolicy { $user, $password ); - if ( !$policyStatus->isGood() && !empty( $settings['forceChange'] ) ) { - $forceChange = true; + + if ( !$policyStatus->isGood() ) { + if ( !empty( $settings['forceChange'] ) ) { + $forceChange = true; + } + + if ( !empty( $settings['suggestChangeOnLogin'] ) ) { + $suggestChangeOnLogin = true; + } } $status->merge( $policyStatus ); } - if ( $status->isOK() && $forceChange ) { - $status->value['forceChange'] = true; + + if ( $status->isOK() ) { + if ( $forceChange ) { + $status->value['forceChange'] = true; + } elseif ( $suggestChangeOnLogin ) { + $status->value['suggestChangeOnLogin'] = true; + } } + return $status; } diff --git a/includes/user/User.php b/includes/user/User.php index 904a934308..42c84b11d1 100644 --- a/includes/user/User.php +++ b/includes/user/User.php @@ -1191,6 +1191,8 @@ class User implements IDBAccessObject, UserIdentity { * - 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). + * - suggestChangeOnLogin (bool): if set to true, the user should be prompted for + * a password change on login. * * @param string $password Desired password * @return Status diff --git a/languages/i18n/en.json b/languages/i18n/en.json index 8700cecb29..d247b54f28 100644 --- a/languages/i18n/en.json +++ b/languages/i18n/en.json @@ -4183,6 +4183,7 @@ "passwordpolicies-policy-passwordcannotbepopular": "Password cannot be {{PLURAL:$1|the popular password|in the list of $1 popular passwords}}", "passwordpolicies-policy-passwordnotinlargeblacklist": "Password cannot be in the list of 100,000 most commonly used passwords.", "passwordpolicies-policyflag-forcechange": "must change on login", + "passwordpolicies-policyflag-suggestchangeonlogin": "suggest change on login", "easydeflate-invaliddeflate": "Content provided is not properly deflated", "unprotected-js": "For security reasons JavaScript cannot be loaded from unprotected pages. Please only create javascript in the MediaWiki: namespace or as a User subpage" } diff --git a/languages/i18n/qqq.json b/languages/i18n/qqq.json index fab3ddcb30..f3c111a1a4 100644 --- a/languages/i18n/qqq.json +++ b/languages/i18n/qqq.json @@ -4389,6 +4389,7 @@ "passwordpolicies-policy-passwordcannotbepopular": "Password policy that enforces that a password is not in a list of $1 number of \"popular\" passwords. $1 - number of popular passwords the password will be checked against", "passwordpolicies-policy-passwordnotinlargeblacklist": "Password policy that enforces that a password is not in a list of 100,000 number of \"popular\" passwords.", "passwordpolicies-policyflag-forcechange": "Password policy flag that enforces changing invalid passwords on login.", + "passwordpolicies-policyflag-suggestchangeonlogin": "Password policy flag that suggests changing invalid passwords on login.", "easydeflate-invaliddeflate": "Error message if the content passed to easydeflate was not deflated (compressed) properly", "unprotected-js": "Error message shown when trying to load javascript via action=raw that is not protected" } diff --git a/tests/phpunit/includes/auth/AbstractPasswordPrimaryAuthenticationProviderTest.php b/tests/phpunit/includes/auth/AbstractPasswordPrimaryAuthenticationProviderTest.php index e67d405448..49601cb786 100644 --- a/tests/phpunit/includes/auth/AbstractPasswordPrimaryAuthenticationProviderTest.php +++ b/tests/phpunit/includes/auth/AbstractPasswordPrimaryAuthenticationProviderTest.php @@ -142,7 +142,7 @@ class AbstractPasswordPrimaryAuthenticationProviderTest extends \MediaWikiTestCa $this->assertNull( $manager->getAuthenticationSessionData( 'reset-pass' ) ); $manager->removeAuthenticationSessionData( null ); - $status = \Status::newGood(); + $status = \Status::newGood( [ 'suggestChangeOnLogin' => true ] ); $status->error( 'testing' ); $providerPriv->setPasswordResetFlag( 'Foo', $status ); $ret = $manager->getAuthenticationSessionData( 'reset-pass' ); diff --git a/tests/phpunit/includes/auth/LocalPasswordPrimaryAuthenticationProviderTest.php b/tests/phpunit/includes/auth/LocalPasswordPrimaryAuthenticationProviderTest.php index e1b25a1ebd..6d831f6a0a 100644 --- a/tests/phpunit/includes/auth/LocalPasswordPrimaryAuthenticationProviderTest.php +++ b/tests/phpunit/includes/auth/LocalPasswordPrimaryAuthenticationProviderTest.php @@ -38,11 +38,11 @@ class LocalPasswordPrimaryAuthenticationProviderTest extends \MediaWikiTestCase $this->manager = new AuthManager( new \FauxRequest(), $config ); } $this->validity = \Status::newGood(); - $provider = $this->getMockBuilder( LocalPasswordPrimaryAuthenticationProvider::class ) ->setMethods( [ 'checkPasswordValidity' ] ) ->setConstructorArgs( [ [ 'loginOnly' => $loginOnly ] ] ) ->getMock(); + $provider->expects( $this->any() )->method( 'checkPasswordValidity' ) ->will( $this->returnCallback( function () { return $this->validity; @@ -167,7 +167,7 @@ class LocalPasswordPrimaryAuthenticationProviderTest extends \MediaWikiTestCase $this->manager->removeAuthenticationSessionData( null ); $row->user_password_expires = null; - $status = \Status::newGood(); + $status = \Status::newGood( [ 'suggestChangeOnLogin' => true ] ); $status->error( 'testing' ); $providerPriv->setPasswordResetFlag( $userName, $status, $row ); $ret = $this->manager->getAuthenticationSessionData( 'reset-pass' ); @@ -184,6 +184,14 @@ class LocalPasswordPrimaryAuthenticationProviderTest extends \MediaWikiTestCase $this->assertNotNull( $ret ); $this->assertSame( 'resetpass-validity', $ret->msg->getKey() ); $this->assertTrue( $ret->hard ); + + $this->manager->removeAuthenticationSessionData( null ); + $row->user_password_expires = null; + $status = \Status::newGood( [ 'suggestChangeOnLogin' => false, ] ); + $status->error( 'testing' ); + $providerPriv->setPasswordResetFlag( $userName, $status, $row ); + $ret = $this->manager->getAuthenticationSessionData( 'reset-pass' ); + $this->assertNull( $ret ); } public function testAuthentication() { @@ -275,6 +283,7 @@ class LocalPasswordPrimaryAuthenticationProviderTest extends \MediaWikiTestCase // Successful auth with reset $this->manager->removeAuthenticationSessionData( null ); + $this->validity = \Status::newGood( [ 'suggestChangeOnLogin' => true ] ); $this->validity->error( 'arbitrary-warning' ); $this->assertEquals( AuthenticationResponse::newPass( $userName ), @@ -664,5 +673,4 @@ class LocalPasswordPrimaryAuthenticationProviderTest extends \MediaWikiTestCase $ret = $provider->beginPrimaryAuthentication( $reqs ); $this->assertEquals( AuthenticationResponse::PASS, $ret->status, 'new password is set' ); } - } diff --git a/tests/phpunit/includes/password/UserPasswordPolicyTest.php b/tests/phpunit/includes/password/UserPasswordPolicyTest.php index 80881ad5ec..ec7bafcc1e 100644 --- a/tests/phpunit/includes/password/UserPasswordPolicyTest.php +++ b/tests/phpunit/includes/password/UserPasswordPolicyTest.php @@ -35,10 +35,18 @@ class UserPasswordPolicyTest extends MediaWikiTestCase { 'PasswordCannotMatchUsername' => true, ], 'sysop' => [ - 'MinimalPasswordLength' => 8, + 'MinimalPasswordLength' => [ 'value' => 8, 'suggestChangeOnLogin' => true ], 'MinimumPasswordLengthToLogin' => 1, 'PasswordCannotMatchUsername' => true, ], + 'bureaucrat' => [ + 'MinimalPasswordLength' => [ + 'value' => 6, + 'suggestChangeOnLogin' => false, + 'forceChange' => true, + ], + 'PasswordCannotMatchUsername' => true, + ], 'default' => [ 'MinimalPasswordLength' => 4, 'MinimumPasswordLengthToLogin' => 1, @@ -67,7 +75,7 @@ class UserPasswordPolicyTest extends MediaWikiTestCase { $user = $this->getTestUser( [ 'sysop' ] )->getUser(); $this->assertArrayEquals( [ - 'MinimalPasswordLength' => 8, + 'MinimalPasswordLength' => [ 'value' => 8, 'suggestChangeOnLogin' => true ], 'MinimumPasswordLengthToLogin' => 1, 'PasswordCannotMatchUsername' => true, 'PasswordCannotMatchBlacklist' => true, @@ -79,7 +87,11 @@ class UserPasswordPolicyTest extends MediaWikiTestCase { $user = $this->getTestUser( [ 'sysop', 'checkuser' ] )->getUser(); $this->assertArrayEquals( [ - 'MinimalPasswordLength' => [ 'value' => 10, 'forceChange' => true ], + 'MinimalPasswordLength' => [ + 'value' => 10, + 'forceChange' => true, + 'suggestChangeOnLogin' => true + ], 'MinimumPasswordLengthToLogin' => 6, 'PasswordCannotMatchUsername' => true, 'PasswordCannotMatchBlacklist' => true, @@ -92,13 +104,17 @@ class UserPasswordPolicyTest extends MediaWikiTestCase { public function testGetPoliciesForGroups() { $effective = UserPasswordPolicy::getPoliciesForGroups( $this->policies, - [ 'user', 'checkuser' ], + [ 'user', 'checkuser', 'sysop' ], $this->policies['default'] ); $this->assertArrayEquals( [ - 'MinimalPasswordLength' => [ 'value' => 10, 'forceChange' => true ], + 'MinimalPasswordLength' => [ + 'value' => 10, + 'forceChange' => true, + 'suggestChangeOnLogin' => true + ], 'MinimumPasswordLengthToLogin' => 6, 'PasswordCannotMatchUsername' => true, 'PasswordCannotMatchBlacklist' => true, @@ -125,12 +141,16 @@ class UserPasswordPolicyTest extends MediaWikiTestCase { $success = Status::newGood( [] ); $warning = Status::newGood( [] ); $forceChange = Status::newGood( [ 'forceChange' => true ] ); + $suggestChangeOnLogin = Status::newGood( [ 'suggestChangeOnLogin' => true ] ); $fatal = Status::newGood( [] ); + // the message does not matter, we only test for state and value $warning->warning( 'invalid-password' ); $forceChange->warning( 'invalid-password' ); + $suggestChangeOnLogin->warning( 'invalid-password' ); $warning->warning( 'invalid-password' ); $fatal->fatal( 'invalid-password' ); + return [ 'No groups, default policy, password too short to login' => [ [], @@ -147,16 +167,21 @@ class UserPasswordPolicyTest extends MediaWikiTestCase { 'abcdabcdabcd', $success, ], - 'Sysop with short password' => [ + 'Sysop with short password and suggestChangeOnLogin set to true' => [ [ 'sysop' ], 'abcd', - $warning, + $suggestChangeOnLogin, ], 'Checkuser with short password' => [ - [ 'sysop', 'checkuser' ], + [ 'checkuser' ], 'abcdabcd', $forceChange, ], + 'Bureaucrat bad password with forceChange true, suggestChangeOnLogin false' => [ + [ 'bureaucrat' ], + 'short', + $forceChange, + ], 'Checkuser with too short password to login' => [ [ 'sysop', 'checkuser' ], 'abcd', @@ -281,6 +306,29 @@ class UserPasswordPolicyTest extends MediaWikiTestCase { ], ], // max ], + 'complex value in both p1 and p2 #2' => [ + [ + 'MinimalPasswordLength' => [ + 'value' => 8, + 'foo' => 1, + 'baz' => false, + ], + ], // p1 + [ + 'MinimalPasswordLength' => [ + 'value' => 2, + 'bar' => true + ], + ], // p2 + [ + 'MinimalPasswordLength' => [ + 'value' => 8, + 'foo' => 1, + 'bar' => true, + 'baz' => false, + ], + ], // max + ], ]; } diff --git a/tests/phpunit/structure/PasswordPolicyStructureTest.php b/tests/phpunit/structure/PasswordPolicyStructureTest.php index b263762513..60ce575e4c 100644 --- a/tests/phpunit/structure/PasswordPolicyStructureTest.php +++ b/tests/phpunit/structure/PasswordPolicyStructureTest.php @@ -18,14 +18,17 @@ class PasswordPolicyStructureTest extends MediaWikiTestCase { // This won't actually find all flags, just the ones in use. Can't really be helped, // other than adding the core flags here. - $flags = [ 'forceChange' ]; + $flags = [ 'forceChange', 'suggestChangeOnLogin' ]; foreach ( $wgPasswordPolicy['policies'] as $group => $checks ) { foreach ( $checks as $check => $settings ) { if ( is_array( $settings ) ) { - $flags = array_merge( $flags, array_diff( $settings, [ 'value' ] ) ); + $flags = array_unique( + array_merge( $flags, array_diff( array_keys( $settings ), [ 'value' ] ) ) + ); } } } + foreach ( $flags as $flag ) { yield [ $flag ]; } -- 2.20.1