Refactor password validity checking

Refactor the password checks to return a status object, so the function
can handle the entire error message, or return multiple error messages.

This patchset aims to keep the functionality identical. A followup
patchset can further improve the functionality. E.g., although
getPasswordValidity stated it could return an array of messages, it
never did so except from the hook, so most callers expect and handle a
single string.

Change-Id: I87644486f5572dc067ebdbacd01fb39c67e5612a

diff --git a/includes/User.php b/includes/User.php
index 6d9f372..9b47acf 100644 (file)
--- a/includes/User.php
+++ b/includes/User.php
@@ -697,6 +697,7 @@ class User {
                return $this->getPasswordValidity( $password ) === true;
        }
 
+
        /**
         * Given unvalidated password input, return error message on failure.
         *
@@ -704,6 +705,33 @@ class User {
         * @return mixed: true on success, string or array of error message on failure
         */
        public function getPasswordValidity( $password ) {
+               $result = $this->checkPasswordValidity( $password );
+               if ( $result->isGood() ) {
+                       return true;
+               } else {
+                       $messages = array();
+                       foreach ( $result->getErrorsByType( 'error' ) as $error ) {
+                               $messages[] = $error['message'];
+                       }
+                       foreach ( $result->getErrorsByType( 'warning' ) as $warning ) {
+                               $messages[] = $warning['message'];
+                       }
+                       if ( count( $messages ) === 1 ) {
+                               return $messages[0];
+                       }
+                       return $messages;
+               }
+       }
+
+       /**
+        * Check if this is a valid password for this user. Status will be good if
+        * the password is valid, or have an array of error messages if not.
+        *
+        * @param string $password Desired password
+        * @return Status
+        * @since 1.23
+        */
+       public function checkPasswordValidity( $password ) {
                global $wgMinimalPasswordLength, $wgContLang;
 
                static $blockedLogins = array(
@@ -711,30 +739,37 @@ class User {
                        'Apitestsysop' => 'testpass', 'Apitestuser' => 'testpass' # r75605
                );
 
+               $status = Status::newGood();
+
                $result = false; //init $result to false for the internal checks
 
                if ( !wfRunHooks( 'isValidPassword', array( $password, &$result, $this ) ) ) {
-                       return $result;
+                       $status->error( $result );
+                       return $status;
                }
 
                if ( $result === false ) {
                        if ( strlen( $password ) < $wgMinimalPasswordLength ) {
-                               return 'passwordtooshort';
+                               $status->error( 'passwordtooshort', $wgMinimalPasswordLength );
+                               return $status;
                        } elseif ( $wgContLang->lc( $password ) == $wgContLang->lc( $this->mName ) ) {
-                               return 'password-name-match';
+                               $status->error( 'password-name-match' );
+                               return $status;
                        } elseif ( isset( $blockedLogins[$this->getName()] ) && $password == $blockedLogins[$this->getName()] ) {
-                               return 'password-login-forbidden';
+                               $status->error( 'password-login-forbidden' );
+                               return $status;
                        } else {
-                               //it seems weird returning true here, but this is because of the
+                               //it seems weird returning a Good status here, but this is because of the
                                //initialization of $result to false above. If the hook is never run or it
                                //doesn't modify $result, then we will likely get down into this if with
                                //a valid password.
-                               return true;
+                               return $status;
                        }
                } elseif ( $result === true ) {
-                       return true;
+                       return $status;
                } else {
-                       return $result; //the isValidPassword hook set a string $result and returned true
+                       $status->error( $result );
+                       return $status; //the isValidPassword hook set a string $result and returned true
                }
        }
 

diff --git a/tests/phpunit/includes/UserTest.php b/tests/phpunit/includes/UserTest.php
index 2ddf05a..e6af126 100644 (file)
--- a/tests/phpunit/includes/UserTest.php
+++ b/tests/phpunit/includes/UserTest.php
@@ -258,4 +258,33 @@ class UserTest extends MediaWikiTestCase {
 
                $wgPasswordExpireGrace = $wgTemp;
        }
+
+       /**
+        * Test password validity checks. There are 3 checks in core,
+        *      - ensure the password meets the minimal length
+        *      - ensure the password is not the same as the username
+        *      - ensure the username/password combo isn't forbidden
+        * @covers User::checkPasswordValidity()
+        * @covers User::getPasswordValidity()
+        * @covers User::isValidPassword()
+        */
+       public function testCheckPasswordValidity() {
+               $this->setMwGlobals( 'wgMinimalPasswordLength', 6 );
+               $user = User::newFromName( 'Useruser' );
+               // Sanity
+               $this->assertTrue( $user->isValidPassword( 'Password1234' ) );
+
+               // Minimum length
+               $this->assertFalse( $user->isValidPassword( 'a' ) );
+               $this->assertFalse( $user->checkPasswordValidity( 'a' )->isGood() );
+               $this->assertEquals( 'passwordtooshort', $user->getPasswordValidity( 'a' ) );
+
+               // Matches username
+               $this->assertFalse( $user->checkPasswordValidity( 'Useruser' )->isGood() );
+               $this->assertEquals( 'password-name-match', $user->getPasswordValidity( 'Useruser' ) );
+
+               // On the forbidden list
+               $this->assertFalse( $user->checkPasswordValidity( 'Passpass' )->isGood() );
+               $this->assertEquals( 'password-login-forbidden', $user->getPasswordValidity( 'Passpass' ) );
+       }
 }