Merge "Add password policy setting `suggestChangeOnLogin`"
authorjenkins-bot <jenkins-bot@gerrit.wikimedia.org>
Tue, 12 Mar 2019 16:35:00 +0000 (16:35 +0000)
committerGerrit Code Review <gerrit@wikimedia.org>
Tue, 12 Mar 2019 16:35:00 +0000 (16:35 +0000)
includes/DefaultSettings.php
includes/auth/AbstractPasswordPrimaryAuthenticationProvider.php
includes/auth/LocalPasswordPrimaryAuthenticationProvider.php
includes/password/UserPasswordPolicy.php
includes/user/User.php
languages/i18n/en.json
languages/i18n/qqq.json
tests/phpunit/includes/auth/AbstractPasswordPrimaryAuthenticationProviderTest.php
tests/phpunit/includes/auth/LocalPasswordPrimaryAuthenticationProviderTest.php
tests/phpunit/includes/password/UserPasswordPolicyTest.php
tests/phpunit/structure/PasswordPolicyStructureTest.php

index 7d0f108..4ec82ed 100644 (file)
@@ -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' => <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
@@ -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' => [
index b6fedcd..cd9eac4 100644 (file)
@@ -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 ) {
index e9adb7e..047a811 100644 (file)
@@ -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 );
index c61c795..79a5539 100644 (file)
@@ -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;
        }
 
index f84a6ff..d6de0aa 100644 (file)
@@ -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
index d2d77f9..82fc7f6 100644 (file)
        "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"
 }
index d48bd94..3245afe 100644 (file)
        "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"
 }
index e67d405..49601cb 100644 (file)
@@ -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' );
index e1b25a1..6d831f6 100644 (file)
@@ -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' );
        }
-
 }
index 80881ad..ec7bafc 100644 (file)
@@ -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
+                       ],
                ];
        }
 
index b263762..60ce575 100644 (file)
@@ -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 ];
                }