From ea7687a7fd937fc68f5981f7182d5483d62b0e63 Mon Sep 17 00:00:00 2001
From: csteipp <csteipp@wikimedia.org>
Date: Tue, 11 Mar 2014 18:47:29 -0700
Subject: [PATCH] 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
---
 includes/User.php                   | 51 ++++++++++++++++++++++++-----
 tests/phpunit/includes/UserTest.php | 29 ++++++++++++++++
 2 files changed, 72 insertions(+), 8 deletions(-)

diff --git a/includes/User.php b/includes/User.php
index 6d9f3724ee..9b47acf9a5 100644
--- 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 2ddf05a3b9..e6af1264fc 100644
--- 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' ) );
+	}
 }
-- 
2.20.1