Fix creating non-parameterized hashes in ParameterizedPassword

I noticed MWOldPassword is broken while working on
I7024b287a7. When generating new passwords for it,
a superfluous : is being added to the serialized hash
within the database (and that breaks parsing so that
people can't ever log in).

As this is not really relevant in the real world (as
nobody is hopefully using plain MD5 passwords anymore),
this doesn't need any backward compatibility handling
for the broken hashes.

Change-Id: I753c135a6de39008488bd7462c2bfcda2cbac116

diff --git a/includes/password/MWOldPassword.php b/includes/password/MWOldPassword.php
index 0ba407f..afa5cac 100644 (file)
--- a/includes/password/MWOldPassword.php
+++ b/includes/password/MWOldPassword.php
@@ -38,7 +38,7 @@ class MWOldPassword extends ParameterizedPassword {
        public function crypt( $plaintext ) {
                global $wgPasswordSalt;
 
-               if ( $wgPasswordSalt && count( $this->args ) == 1 ) {
+               if ( $wgPasswordSalt && count( $this->args ) === 1 ) {
                        $this->hash = md5( $this->args[0] . '-' . md5( $plaintext ) );
                } else {
                        $this->args = array();

diff --git a/includes/password/ParameterizedPassword.php b/includes/password/ParameterizedPassword.php
index 4d6e415..187f895 100644 (file)
--- a/includes/password/ParameterizedPassword.php
+++ b/includes/password/ParameterizedPassword.php
@@ -83,10 +83,14 @@ abstract class ParameterizedPassword extends Password {
        }
 
        public function toString() {
-               return
-                       ':' . $this->config['type'] . ':' .
-                       implode( $this->getDelimiter(), array_merge( $this->params, $this->args ) ) .
-                       $this->getDelimiter() . $this->hash;
+               $str = ':' . $this->config['type'] . ':';
+
+               if ( count( $this->params ) || count( $this->args ) ) {
+                       $str .= implode( $this->getDelimiter(), array_merge( $this->params, $this->args ) );
+                       $str .= $this->getDelimiter();
+               }
+
+               return $str . $this->hash;
        }
 
        /**

diff --git a/tests/phpunit/includes/TestUser.php b/tests/phpunit/includes/TestUser.php
index 39822dc..499353e 100644 (file)
--- a/tests/phpunit/includes/TestUser.php
+++ b/tests/phpunit/includes/TestUser.php
@@ -114,8 +114,8 @@ class TestUser {
                $passwordFactory = $this->user->getPasswordFactory();
                $oldDefaultType = $passwordFactory->getDefaultType();
 
-                // B is salted MD5 (thus fast) ... we don't care about security here, this is test only
-               $passwordFactory->setDefaultType( 'B' ); // @TODO: Change this to A once that is fixed: https://gerrit.wikimedia.org/r/167523
+                // A is unsalted MD5 (thus fast) ... we don't care about security here, this is test only
+               $passwordFactory->setDefaultType( 'A' );
                $newPassword = $passwordFactory->newFromPlaintext( $password , $this->user->getPassword() );
 
                $change = false;