Improve test coverage in includes/password
authorTim Starling <tstarling@wikimedia.org>
Tue, 6 Jun 2017 05:05:22 +0000 (15:05 +1000)
committerTim Starling <tstarling@wikimedia.org>
Wed, 7 Jun 2017 04:28:11 +0000 (14:28 +1000)
From 21% to 82%.

* Added missing @covers, broadened @covers where appropriate.
* Added tests for some code that lacked them.
* Added a parameter to control the use of hash_pbkdf2() so that the pure
  PHP fallback could be tested. In the non-fallback test, force the use
  of the extension, and mark it skipped if it is not installed.

Bug: T167003
Change-Id: I987e1a89ec343907f4ead7f6192b2d4deb58ac16

13 files changed:
includes/password/Pbkdf2Password.php
tests/phpunit/includes/password/BcryptPasswordTest.php
tests/phpunit/includes/password/EncryptedPasswordTest.php [new file with mode: 0644]
tests/phpunit/includes/password/LayeredParameterizedPasswordTest.php
tests/phpunit/includes/password/MWOldPasswordTest.php [new file with mode: 0644]
tests/phpunit/includes/password/MWSaltedPasswordTest.php [new file with mode: 0644]
tests/phpunit/includes/password/PasswordFactoryTest.php [new file with mode: 0644]
tests/phpunit/includes/password/PasswordPolicyChecksTest.php
tests/phpunit/includes/password/PasswordTest.php
tests/phpunit/includes/password/PasswordTestCase.php
tests/phpunit/includes/password/Pbkdf2PasswordFallbackTest.php [new file with mode: 0644]
tests/phpunit/includes/password/Pbkdf2PasswordTest.php
tests/phpunit/includes/password/UserPasswordPolicyTest.php

index 6ffada3..4a8831e 100644 (file)
@@ -41,12 +41,17 @@ class Pbkdf2Password extends ParameterizedPassword {
                return ':';
        }
 
+       protected function shouldUseHashExtension() {
+               return isset( $this->config['use-hash-extension'] ) ?
+                       $this->config['use-hash-extension'] : function_exists( 'hash_pbkdf2' );
+       }
+
        public function crypt( $password ) {
                if ( count( $this->args ) == 0 ) {
                        $this->args[] = base64_encode( MWCryptRand::generate( 16, true ) );
                }
 
-               if ( function_exists( 'hash_pbkdf2' ) ) {
+               if ( $this->shouldUseHashExtension() ) {
                        $hash = hash_pbkdf2(
                                $this->params['algo'],
                                $password,
index 8f80362..9b8e01e 100644 (file)
@@ -2,6 +2,10 @@
 
 /**
  * @group large
+ * @covers BcryptPassword
+ * @covers ParameterizedPassword
+ * @covers Password
+ * @covers PasswordFactory
  */
 class BcryptPasswordTest extends PasswordTestCase {
        protected function getTypeConfigs() {
diff --git a/tests/phpunit/includes/password/EncryptedPasswordTest.php b/tests/phpunit/includes/password/EncryptedPasswordTest.php
new file mode 100644 (file)
index 0000000..0c85653
--- /dev/null
@@ -0,0 +1,81 @@
+<?php
+
+/**
+ * @covers EncryptedPassword
+ * @covers ParameterizedPassword
+ * @covers Password
+ * @codingStandardsIgnoreStart Generic.Files.LineLength
+ */
+class EncryptedPasswordTest extends PasswordTestCase {
+       protected function getTypeConfigs() {
+               return [
+                       'both' => [
+                               'class' => 'EncryptedPassword',
+                               'underlying' => 'pbkdf2',
+                               'secrets' => [
+                                       md5( 'secret1' ),
+                                       md5( 'secret2' ),
+                               ],
+                               'cipher' => 'aes-256-cbc',
+                       ],
+                       'secret1' => [
+                               'class' => 'EncryptedPassword',
+                               'underlying' => 'pbkdf2',
+                               'secrets' => [
+                                       md5( 'secret1' ),
+                               ],
+                               'cipher' => 'aes-256-cbc',
+                       ],
+                       'secret2' => [
+                               'class' => 'EncryptedPassword',
+                               'underlying' => 'pbkdf2',
+                               'secrets' => [
+                                       md5( 'secret2' ),
+                               ],
+                               'cipher' => 'aes-256-cbc',
+                       ],
+                       'pbkdf2' => [
+                               'class' => 'Pbkdf2Password',
+                               'algo' => 'sha256',
+                               'cost' => '10',
+                               'length' => '64',
+                       ],
+               ];
+       }
+
+       public static function providePasswordTests() {
+               return [
+                       // Encrypted with secret1
+                       [ true, ':both:aes-256-cbc:0:izBpxujqC1YbzpCB3qAzgg==:ZqHnitT1pL4YJqKqFES2KEevZYSy2LtlibW5+IMi4XKOGKGy6sE638BXyBbLQQsBtTSrt+JyzwOayKtwIfRbaQsBridx/O1JwBSai1TkGkOsYMBXnlu2Bu/EquCBj5QpjYh7p3Uq4rpiop1KQlin1BJMwnAa1PovhxjpxnYhlhkM4X5ALoGi3XM0bapN48vt', 'password' ],
+                       [ true, ':secret1:aes-256-cbc:0:izBpxujqC1YbzpCB3qAzgg==:ZqHnitT1pL4YJqKqFES2KEevZYSy2LtlibW5+IMi4XKOGKGy6sE638BXyBbLQQsBtTSrt+JyzwOayKtwIfRbaQsBridx/O1JwBSai1TkGkOsYMBXnlu2Bu/EquCBj5QpjYh7p3Uq4rpiop1KQlin1BJMwnAa1PovhxjpxnYhlhkM4X5ALoGi3XM0bapN48vt', 'password' ],
+                       [ false, ':secret1:aes-256-cbc:0:izBpxujqC1YbzpCB3qAzgg==:ZqHnitT1pL4YJqKqFES2KEevZYSy2LtlibW5+IMi4XKOGKGy6sE638BXyBbLQQsBtTSrt+JyzwOayKtwIfRbaQsBridx/O1JwBSai1TkGkOsYMBXnlu2Bu/EquCBj5QpjYh7p3Uq4rpiop1KQlin1BJMwnAa1PovhxjpxnYhlhkM4X5ALoGi3XM0bapN48vt', 'notpassword' ],
+
+                       // Encrypted with secret2
+                       [ true, ':both:aes-256-cbc:1:m1LCnQVIakfYBNlr9KEgQg==:5yPTctqrzsybdgaMEag18AZYbnL37pAtXVBqmWxkjXbnNmiDH+1bHoL8lsEVTH/sJntC82kNVgE7zeiD8xUVLYF2VUnvB5+sU+aysE45/zwsCu7a22TaischMAOWrsHZ/tIgS/TnZY2d+HNyxgsEeeYf/QoL+FhmqHquK02+4SRbA5lLuj9niYy1r5CoM9cQ', 'password' ],
+                       [ true, ':secret2:aes-256-cbc:0:m1LCnQVIakfYBNlr9KEgQg==:5yPTctqrzsybdgaMEag18AZYbnL37pAtXVBqmWxkjXbnNmiDH+1bHoL8lsEVTH/sJntC82kNVgE7zeiD8xUVLYF2VUnvB5+sU+aysE45/zwsCu7a22TaischMAOWrsHZ/tIgS/TnZY2d+HNyxgsEeeYf/QoL+FhmqHquK02+4SRbA5lLuj9niYy1r5CoM9cQ', 'password' ],
+               ];
+       }
+
+       /**
+        * Wrong encryption key selected
+        * @expectedException PasswordError
+        */
+       public function testDecryptionError() {
+               $hash = ':secret1:aes-256-cbc:0:m1LCnQVIakfYBNlr9KEgQg==:5yPTctqrzsybdgaMEag18AZYbnL37pAtXVBqmWxkjXbnNmiDH+1bHoL8lsEVTH/sJntC82kNVgE7zeiD8xUVLYF2VUnvB5+sU+aysE45/zwsCu7a22TaischMAOWrsHZ/tIgS/TnZY2d+HNyxgsEeeYf/QoL+FhmqHquK02+4SRbA5lLuj9niYy1r5CoM9cQ';
+               $password = $this->passwordFactory->newFromCiphertext( $hash );
+               $password->crypt( 'password' );
+       }
+
+       public function testUpdate() {
+               $hash = ':both:aes-256-cbc:0:izBpxujqC1YbzpCB3qAzgg==:ZqHnitT1pL4YJqKqFES2KEevZYSy2LtlibW5+IMi4XKOGKGy6sE638BXyBbLQQsBtTSrt+JyzwOayKtwIfRbaQsBridx/O1JwBSai1TkGkOsYMBXnlu2Bu/EquCBj5QpjYh7p3Uq4rpiop1KQlin1BJMwnAa1PovhxjpxnYhlhkM4X5ALoGi3XM0bapN48vt';
+               $fromHash = $this->passwordFactory->newFromCiphertext( $hash );
+               $fromPlaintext = $this->passwordFactory->newFromPlaintext( 'password', $fromHash );
+               $this->assertTrue( $fromHash->update() );
+
+               $serialized = $fromHash->toString();
+               $this->assertRegExp( '/^:both:aes-256-cbc:1:/', $serialized );
+               $fromNewHash = $this->passwordFactory->newFromCiphertext( $serialized );
+               $fromPlaintext = $this->passwordFactory->newFromPlaintext( 'password', $fromNewHash );
+               $this->assertTrue( $fromHash->equals( $fromPlaintext ) );
+       }
+}
index 773f033..cf96d06 100644 (file)
@@ -1,5 +1,9 @@
 <?php
 
+/**
+ * @covers LayeredParameterizedPassword
+ * @covers Password
+ */
 class LayeredParameterizedPasswordTest extends PasswordTestCase {
        protected function getTypeConfigs() {
                return [
@@ -26,6 +30,10 @@ class LayeredParameterizedPasswordTest extends PasswordTestCase {
                ];
        }
 
+       protected function getValidTypes() {
+               return [ 'testLargeLayeredFinal' ];
+       }
+
        public static function providePasswordTests() {
                // @codingStandardsIgnoreStart Generic.Files.LineLength.TooLong
                return [
diff --git a/tests/phpunit/includes/password/MWOldPasswordTest.php b/tests/phpunit/includes/password/MWOldPasswordTest.php
new file mode 100644 (file)
index 0000000..51e739c
--- /dev/null
@@ -0,0 +1,24 @@
+<?php
+
+/**
+ * @covers MWOldPassword
+ * @covers ParameterizedPassword
+ * @covers Password
+ */
+class MWOldPasswordTest extends PasswordTestCase {
+       protected function getTypeConfigs() {
+               return [ 'A' => [
+                       'class' => 'MWOldPassword',
+               ] ];
+       }
+
+       public static function providePasswordTests() {
+               return [
+                       [ true, ':A:5f4dcc3b5aa765d61d8327deb882cf99', 'password' ],
+                       // Type-B password with incorrect type name is accepted
+                       [ true, ':A:salt:9842afc7cb949c440c51347ed809362f', 'password' ],
+                       [ false, ':A:d529e941509eb9e9b9cfaeae1fe7ca23', 'password' ],
+                       [ false, ':A:salt:d529e941509eb9e9b9cfaeae1fe7ca23', 'password' ],
+               ];
+       }
+}
diff --git a/tests/phpunit/includes/password/MWSaltedPasswordTest.php b/tests/phpunit/includes/password/MWSaltedPasswordTest.php
new file mode 100644 (file)
index 0000000..53a6ad1
--- /dev/null
@@ -0,0 +1,21 @@
+<?php
+
+/**
+ * @covers MWSaltedPassword
+ * @covers ParameterizedPassword
+ * @covers Password
+ */
+class MWSaltedPasswordTest extends PasswordTestCase {
+       protected function getTypeConfigs() {
+               return [ 'B' => [
+                       'class' => 'MWSaltedPassword',
+               ] ];
+       }
+
+       public static function providePasswordTests() {
+               return [
+                       [ true, ':B:salt:9842afc7cb949c440c51347ed809362f', 'password' ],
+                       [ false, ':B:salt:d529e941509eb9e9b9cfaeae1fe7ca23', 'password' ],
+               ];
+       }
+}
diff --git a/tests/phpunit/includes/password/PasswordFactoryTest.php b/tests/phpunit/includes/password/PasswordFactoryTest.php
new file mode 100644 (file)
index 0000000..5d585f3
--- /dev/null
@@ -0,0 +1,110 @@
+<?php
+
+/**
+ * @covers PasswordFactory
+ */
+class PasswordFactoryTest extends MediaWikiTestCase {
+       public function testRegister() {
+               $pf = new PasswordFactory;
+               $pf->register( 'foo', [ 'class' => 'InvalidPassword' ] );
+               $this->assertArrayHasKey( 'foo', $pf->getTypes() );
+       }
+
+       public function testSetDefaultType() {
+               $pf = new PasswordFactory;
+               $pf->register( '1', [ 'class' => 'InvalidPassword' ] );
+               $pf->register( '2', [ 'class' => 'InvalidPassword' ] );
+               $pf->setDefaultType( '1' );
+               $this->assertSame( '1', $pf->getDefaultType() );
+               $pf->setDefaultType( '2' );
+               $this->assertSame( '2', $pf->getDefaultType() );
+       }
+
+       /**
+        * @expectedException Exception
+        */
+       public function testSetDefaultTypeError() {
+               $pf = new PasswordFactory;
+               $pf->setDefaultType( 'bogus' );
+       }
+
+       public function testInit() {
+               $config = new HashConfig( [
+                       'PasswordConfig' => [
+                               'foo' => [ 'class' => 'InvalidPassword' ],
+                       ],
+                       'PasswordDefault' => 'foo'
+               ] );
+               $pf = new PasswordFactory;
+               $pf->init( $config );
+               $this->assertSame( 'foo', $pf->getDefaultType() );
+               $this->assertArrayHasKey( 'foo', $pf->getTypes() );
+       }
+
+       public function testNewFromCiphertext() {
+               $pf = new PasswordFactory;
+               $pf->register( 'B', [ 'class' => 'MWSaltedPassword' ] );
+               $pw = $pf->newFromCiphertext( ':B:salt:d529e941509eb9e9b9cfaeae1fe7ca23' );
+               $this->assertInstanceOf( MWSaltedPassword::class, $pw );
+       }
+
+       public function provideNewFromCiphertextErrors() {
+               return [ [ 'blah' ], [ ':blah:' ] ];
+       }
+
+       /**
+        * @dataProvider provideNewFromCiphertextErrors
+        * @expectedException PasswordError
+        */
+       public function testNewFromCiphertextErrors( $hash ) {
+               $pf = new PasswordFactory;
+               $pf->register( 'B', [ 'class' => 'MWSaltedPassword' ] );
+               $pf->newFromCiphertext( $hash );
+       }
+
+       public function testNewFromType() {
+               $pf = new PasswordFactory;
+               $pf->register( 'B', [ 'class' => 'MWSaltedPassword' ] );
+               $pw = $pf->newFromType( 'B' );
+               $this->assertInstanceOf( MWSaltedPassword::class, $pw );
+       }
+
+       /**
+        * @expectedException PasswordError
+        */
+       public function testNewFromTypeError() {
+               $pf = new PasswordFactory;
+               $pf->register( 'B', [ 'class' => 'MWSaltedPassword' ] );
+               $pf->newFromType( 'bogus' );
+       }
+
+       public function testNewFromPlaintext() {
+               $pf = new PasswordFactory;
+               $pf->register( 'A', [ 'class' => 'MWOldPassword' ] );
+               $pf->register( 'B', [ 'class' => 'MWSaltedPassword' ] );
+               $pf->setDefaultType( 'A' );
+
+               $this->assertInstanceOf( 'InvalidPassword', $pf->newFromPlaintext( null ) );
+               $this->assertInstanceOf( 'MWOldPassword', $pf->newFromPlaintext( 'password' ) );
+               $this->assertInstanceOf( 'MWSaltedPassword',
+                       $pf->newFromPlaintext( 'password', $pf->newFromType( 'B' ) ) );
+       }
+
+       public function testNeedsUpdate() {
+               $pf = new PasswordFactory;
+               $pf->register( 'A', [ 'class' => 'MWOldPassword' ] );
+               $pf->register( 'B', [ 'class' => 'MWSaltedPassword' ] );
+               $pf->setDefaultType( 'A' );
+
+               $this->assertFalse( $pf->needsUpdate( $pf->newFromType( 'A' ) ) );
+               $this->assertTrue( $pf->needsUpdate( $pf->newFromType( 'B' ) ) );
+       }
+
+       public function testGenerateRandomPasswordString() {
+               $this->assertSame( 13, strlen( PasswordFactory::generateRandomPasswordString( 13 ) ) );
+       }
+
+       public function testNewInvalidPassword() {
+               $this->assertInstanceOf( 'InvalidPassword', PasswordFactory::newInvalidPassword() );
+       }
+}
index 6357510..7dfb3cf 100644 (file)
@@ -133,4 +133,27 @@ class PasswordPolicyChecksTest extends MediaWikiTestCase {
                $this->assertTrue( $statusLong->isOK(), 'Password matches blacklist, not fatal' );
        }
 
+       public static function providePopularBlacklist() {
+               return [
+                       [ false, 'sitename' ],
+                       [ false, 'password' ],
+                       [ false, '12345' ],
+                       [ true, 'hqY98gCZ6qM8s8' ],
+               ];
+       }
+
+       /**
+        * @covers PasswordPolicyChecks::checkPopularPasswordBlacklist
+        * @dataProvider providePopularBlacklist
+        */
+       public function testCheckPopularPasswordBlacklist( $expected, $password ) {
+               global $IP;
+               $this->setMwGlobals( [
+                       'wgSitename' => 'sitename',
+                       'wgPopularPasswordFile' => "$IP/serialized/commonpasswords.cdb"
+               ] );
+               $user = User::newFromName( 'username' );
+               $status = PasswordPolicyChecks::checkPopularPasswordBlacklist( PHP_INT_MAX, $user, $password );
+               $this->assertSame( $expected, $status->isGood() );
+       }
 }
index e8a4d5c..d0177d0 100644 (file)
  * @file
  */
 
+/**
+ * @covers InvalidPassword
+ */
 class PasswordTest extends MediaWikiTestCase {
-       /**
-        * @covers InvalidPassword::equals
-        */
        public function testInvalidUnequalInvalid() {
                $passwordFactory = new PasswordFactory();
                $invalid1 = $passwordFactory->newFromCiphertext( null );
index 9a142cb..80b9838 100644 (file)
@@ -78,8 +78,7 @@ abstract class PasswordTestCase extends MediaWikiTestCase {
 
        /**
         * @dataProvider providePasswordTests
-        * @covers InvalidPassword::equals
-        * @covers InvalidPassword::toString
+        * @covers InvalidPassword
         */
        public function testInvalidUnequalNormal( $shouldMatch, $hash, $password ) {
                $invalid = $this->passwordFactory->newFromCiphertext( null );
@@ -88,4 +87,26 @@ abstract class PasswordTestCase extends MediaWikiTestCase {
                $this->assertFalse( $invalid->equals( $normal ) );
                $this->assertFalse( $normal->equals( $invalid ) );
        }
+
+       protected function getValidTypes() {
+               return array_keys( $this->getTypeConfigs() );
+       }
+
+       public function provideTypes( $type ) {
+               $params = [];
+               foreach ( $this->getValidTypes() as $type ) {
+                       $params[] = [ $type ];
+               }
+               return $params;
+       }
+
+       /**
+        * @dataProvider provideTypes
+        */
+       public function testCrypt( $type ) {
+               $fromType = $this->passwordFactory->newFromType( $type );
+               $fromType->crypt( 'password' );
+               $fromPlaintext = $this->passwordFactory->newFromPlaintext( 'password', $fromType );
+               $this->assertTrue( $fromType->equals( $fromPlaintext ) );
+       }
 }
diff --git a/tests/phpunit/includes/password/Pbkdf2PasswordFallbackTest.php b/tests/phpunit/includes/password/Pbkdf2PasswordFallbackTest.php
new file mode 100644 (file)
index 0000000..605d190
--- /dev/null
@@ -0,0 +1,29 @@
+<?php
+
+
+/**
+ * @group large
+ * @covers Pbkdf2Password
+ */
+class Pbkdf2PasswordFallbackTest extends PasswordTestCase {
+       protected function getTypeConfigs() {
+               return [
+                       'pbkdf2' => [
+                               'class' => 'Pbkdf2Password',
+                               'algo' => 'sha256',
+                               'cost' => '10000',
+                               'length' => '128',
+                               'use-hash-extension' => false,
+                       ],
+               ];
+       }
+
+       public static function providePasswordTests() {
+               return [
+                       [ true, ":pbkdf2:sha1:1:20:c2FsdA==:DGDID5YfDnHzqbUkr2ASBi/gN6Y=", 'password' ],
+                       [ true, ":pbkdf2:sha1:2:20:c2FsdA==:6mwBTcctb4zNHtkqzh1B8NjeiVc=", 'password' ],
+                       [ true, ":pbkdf2:sha1:4096:20:c2FsdA==:SwB5AbdlSJq+rUnZJvch0GWkKcE=", 'password' ],
+                       [ true, ":pbkdf2:sha1:4096:16:c2EAbHQ=:Vvpqp1VICZ3MN9fwNCXgww==", "pass\x00word" ],
+               ];
+       }
+}
index 07cdab0..ff069cd 100644 (file)
@@ -2,6 +2,10 @@
 
 /**
  * @group large
+ * @covers Pbkdf2Password
+ * @covers Password
+ * @covers ParameterizedPassword
+ * @requires function hash_pbkdf2
  */
 class Pbkdf2PasswordTest extends PasswordTestCase {
        protected function getTypeConfigs() {
@@ -10,6 +14,7 @@ class Pbkdf2PasswordTest extends PasswordTestCase {
                        'algo' => 'sha256',
                        'cost' => '10000',
                        'length' => '128',
+                       'use-hash-extension' => true,
                ] ];
        }
 
index 8a69b5c..3bd82b4 100644 (file)
@@ -22,6 +22,7 @@
 
 /**
  * @group Database
+ * @covers UserPasswordPolicy
  */
 class UserPasswordPolicyTest extends MediaWikiTestCase {
 
@@ -56,9 +57,6 @@ class UserPasswordPolicyTest extends MediaWikiTestCase {
                return new UserPasswordPolicy( $this->policies, $this->checks );
        }
 
-       /**
-        * @covers UserPasswordPolicy::getPoliciesForUser
-        */
        public function testGetPoliciesForUser() {
 
                $upp = $this->getUserPasswordPolicy();
@@ -79,9 +77,6 @@ class UserPasswordPolicyTest extends MediaWikiTestCase {
                );
        }
 
-       /**
-        * @covers UserPasswordPolicy::getPoliciesForGroups
-        */
        public function testGetPoliciesForGroups() {
                $effective = UserPasswordPolicy::getPoliciesForGroups(
                        $this->policies,
@@ -103,7 +98,6 @@ class UserPasswordPolicyTest extends MediaWikiTestCase {
 
        /**
         * @dataProvider provideCheckUserPassword
-        * @covers UserPasswordPolicy::checkUserPassword
         */
        public function testCheckUserPassword( $username, $groups, $password, $valid, $ok, $msg ) {
 
@@ -183,7 +177,6 @@ class UserPasswordPolicyTest extends MediaWikiTestCase {
 
        /**
         * @dataProvider provideMaxOfPolicies
-        * @covers UserPasswordPolicy::maxOfPolicies
         */
        public function testMaxOfPolicies( $p1, $p2, $max, $msg ) {
                $this->assertArrayEquals(