From a9911b2582a20d9fe0569439d0d22a8c3135c249 Mon Sep 17 00:00:00 2001 From: Tim Starling Date: Tue, 6 Jun 2017 15:05:22 +1000 Subject: [PATCH] Improve test coverage in includes/password 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 --- includes/password/Pbkdf2Password.php | 7 +- .../includes/password/BcryptPasswordTest.php | 4 + .../password/EncryptedPasswordTest.php | 81 +++++++++++++ .../LayeredParameterizedPasswordTest.php | 8 ++ .../includes/password/MWOldPasswordTest.php | 24 ++++ .../password/MWSaltedPasswordTest.php | 21 ++++ .../includes/password/PasswordFactoryTest.php | 110 ++++++++++++++++++ .../password/PasswordPolicyChecksTest.php | 23 ++++ .../includes/password/PasswordTest.php | 6 +- .../includes/password/PasswordTestCase.php | 25 +++- .../password/Pbkdf2PasswordFallbackTest.php | 29 +++++ .../includes/password/Pbkdf2PasswordTest.php | 5 + .../password/UserPasswordPolicyTest.php | 9 +- 13 files changed, 338 insertions(+), 14 deletions(-) create mode 100644 tests/phpunit/includes/password/EncryptedPasswordTest.php create mode 100644 tests/phpunit/includes/password/MWOldPasswordTest.php create mode 100644 tests/phpunit/includes/password/MWSaltedPasswordTest.php create mode 100644 tests/phpunit/includes/password/PasswordFactoryTest.php create mode 100644 tests/phpunit/includes/password/Pbkdf2PasswordFallbackTest.php diff --git a/includes/password/Pbkdf2Password.php b/includes/password/Pbkdf2Password.php index 6ffada36b4..4a8831e32f 100644 --- a/includes/password/Pbkdf2Password.php +++ b/includes/password/Pbkdf2Password.php @@ -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, diff --git a/tests/phpunit/includes/password/BcryptPasswordTest.php b/tests/phpunit/includes/password/BcryptPasswordTest.php index 8f80362470..9b8e01e772 100644 --- a/tests/phpunit/includes/password/BcryptPasswordTest.php +++ b/tests/phpunit/includes/password/BcryptPasswordTest.php @@ -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 index 0000000000..0c8565372a --- /dev/null +++ b/tests/phpunit/includes/password/EncryptedPasswordTest.php @@ -0,0 +1,81 @@ + [ + '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 ) ); + } +} diff --git a/tests/phpunit/includes/password/LayeredParameterizedPasswordTest.php b/tests/phpunit/includes/password/LayeredParameterizedPasswordTest.php index 773f033abb..cf96d0673c 100644 --- a/tests/phpunit/includes/password/LayeredParameterizedPasswordTest.php +++ b/tests/phpunit/includes/password/LayeredParameterizedPasswordTest.php @@ -1,5 +1,9 @@ [ + '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 index 0000000000..53a6ad138b --- /dev/null +++ b/tests/phpunit/includes/password/MWSaltedPasswordTest.php @@ -0,0 +1,21 @@ + [ + '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 index 0000000000..5d585f3221 --- /dev/null +++ b/tests/phpunit/includes/password/PasswordFactoryTest.php @@ -0,0 +1,110 @@ +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() ); + } +} diff --git a/tests/phpunit/includes/password/PasswordPolicyChecksTest.php b/tests/phpunit/includes/password/PasswordPolicyChecksTest.php index 6357510f4f..7dfb3cf5f1 100644 --- a/tests/phpunit/includes/password/PasswordPolicyChecksTest.php +++ b/tests/phpunit/includes/password/PasswordPolicyChecksTest.php @@ -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() ); + } } diff --git a/tests/phpunit/includes/password/PasswordTest.php b/tests/phpunit/includes/password/PasswordTest.php index e8a4d5c686..d0177d0d4a 100644 --- a/tests/phpunit/includes/password/PasswordTest.php +++ b/tests/phpunit/includes/password/PasswordTest.php @@ -20,10 +20,10 @@ * @file */ +/** + * @covers InvalidPassword + */ class PasswordTest extends MediaWikiTestCase { - /** - * @covers InvalidPassword::equals - */ public function testInvalidUnequalInvalid() { $passwordFactory = new PasswordFactory(); $invalid1 = $passwordFactory->newFromCiphertext( null ); diff --git a/tests/phpunit/includes/password/PasswordTestCase.php b/tests/phpunit/includes/password/PasswordTestCase.php index 9a142cbcca..80b9838d8e 100644 --- a/tests/phpunit/includes/password/PasswordTestCase.php +++ b/tests/phpunit/includes/password/PasswordTestCase.php @@ -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 index 0000000000..605d1905c6 --- /dev/null +++ b/tests/phpunit/includes/password/Pbkdf2PasswordFallbackTest.php @@ -0,0 +1,29 @@ + [ + '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" ], + ]; + } +} diff --git a/tests/phpunit/includes/password/Pbkdf2PasswordTest.php b/tests/phpunit/includes/password/Pbkdf2PasswordTest.php index 07cdab0117..ff069cd94b 100644 --- a/tests/phpunit/includes/password/Pbkdf2PasswordTest.php +++ b/tests/phpunit/includes/password/Pbkdf2PasswordTest.php @@ -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, ] ]; } diff --git a/tests/phpunit/includes/password/UserPasswordPolicyTest.php b/tests/phpunit/includes/password/UserPasswordPolicyTest.php index 8a69b5ce1e..3bd82b4ac8 100644 --- a/tests/phpunit/includes/password/UserPasswordPolicyTest.php +++ b/tests/phpunit/includes/password/UserPasswordPolicyTest.php @@ -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( -- 2.20.1