From 29bee071b23832173a55364453d3132a2fe26101 Mon Sep 17 00:00:00 2001 From: Max Semenik Date: Sat, 14 Sep 2019 00:58:42 -0700 Subject: [PATCH 1/1] Optionally require both username and email for password resets Bug: T232694 Change-Id: I70ed25ea4f810bf642fcb3df6f9b2663732b5dcf (cherry picked from commit 1de3611539dbef2df2b9c0a4632aecd066695990) --- includes/user/PasswordReset.php | 53 +- languages/i18n/en.json | 1 + languages/i18n/qqq.json | 1 + .../includes/user/PasswordResetTest.php | 453 ++++++++++++++++-- 4 files changed, 449 insertions(+), 59 deletions(-) diff --git a/includes/user/PasswordReset.php b/includes/user/PasswordReset.php index 8ef1d0d500..be7ea915e4 100644 --- a/includes/user/PasswordReset.php +++ b/includes/user/PasswordReset.php @@ -61,6 +61,7 @@ class PasswordReset implements LoggerAwareInterface { private $permissionCache; public static $constructorOptions = [ + 'AllowRequiringEmailForResets', 'EnableEmail', 'PasswordResetRoutes', ]; @@ -166,12 +167,14 @@ class PasswordReset implements LoggerAwareInterface { . ' is not allowed to reset passwords' ); } + $username = $username ?? ''; + $email = $email ?? ''; + $resetRoutes = $this->config->get( 'PasswordResetRoutes' ) + [ 'username' => false, 'email' => false ]; if ( $resetRoutes['username'] && $username ) { $method = 'username'; - $users = [ User::newFromName( $username ) ]; - $email = null; + $users = [ $this->lookupUser( $username ) ]; } elseif ( $resetRoutes['email'] && $email ) { if ( !Sanitizer::validateEmail( $email ) ) { return StatusValue::newFatal( 'passwordreset-invalidemail' ); @@ -188,12 +191,33 @@ class PasswordReset implements LoggerAwareInterface { $error = []; $data = [ 'Username' => $username, - 'Email' => $email, + // Email gets set to null for backward compatibility + 'Email' => $method === 'email' ? $email : null, ]; if ( !Hooks::run( 'SpecialPasswordResetOnSubmit', [ &$users, $data, &$error ] ) ) { return StatusValue::newFatal( Message::newFromSpecifier( $error ) ); } + $firstUser = $users[0] ?? null; + $requireEmail = $this->config->get( 'AllowRequiringEmailForResets' ) + && $method === 'username' + && $firstUser + && $firstUser->getBoolOption( 'requireemail' ); + if ( $requireEmail ) { + if ( $email === '' ) { + return StatusValue::newFatal( 'passwordreset-username-email-required' ); + } + + if ( !Sanitizer::validateEmail( $email ) ) { + return StatusValue::newFatal( 'passwordreset-invalidemail' ); + } + } + + // Check against the rate limiter + if ( $performingUser->pingLimiter( 'mailpassword' ) ) { + return StatusValue::newFatal( 'actionthrottledtext' ); + } + if ( !$users ) { if ( $method === 'email' ) { // Don't reveal whether or not an email address is in use @@ -203,18 +227,11 @@ class PasswordReset implements LoggerAwareInterface { } } - $firstUser = $users[0]; - if ( !$firstUser instanceof User || !$firstUser->getId() ) { // Don't parse username as wikitext (T67501) return StatusValue::newFatal( wfMessage( 'nosuchuser', wfEscapeWikiText( $username ) ) ); } - // Check against the rate limiter - if ( $performingUser->pingLimiter( 'mailpassword' ) ) { - return StatusValue::newFatal( 'actionthrottledtext' ); - } - // All the users will have the same email address if ( !$firstUser->getEmail() ) { // This won't be reachable from the email route, so safe to expose the username @@ -222,6 +239,11 @@ class PasswordReset implements LoggerAwareInterface { wfEscapeWikiText( $firstUser->getName() ) ) ); } + if ( $requireEmail && $firstUser->getEmail() !== $email ) { + // Pretend everything's fine to avoid disclosure + return StatusValue::newGood(); + } + // We need to have a valid IP address for the hook, but per T20347, we should // send the user's name if they're logged in. $ip = $performingUser->getRequest()->getIP(); @@ -324,4 +346,15 @@ class PasswordReset implements LoggerAwareInterface { } return $users; } + + /** + * User object creation helper for testability + * @codeCoverageIgnore + * + * @param string $username + * @return User|false + */ + protected function lookupUser( $username ) { + return User::newFromName( $username ); + } } diff --git a/languages/i18n/en.json b/languages/i18n/en.json index 54f55674ac..92d1d9cb3a 100644 --- a/languages/i18n/en.json +++ b/languages/i18n/en.json @@ -608,6 +608,7 @@ "passwordreset-ignored": "The password reset was not handled. Maybe no provider was configured?", "passwordreset-invalidemail": "Invalid email address", "passwordreset-nodata": "Neither a username nor an email address was supplied", + "passwordreset-username-email-required": "Both username and email address are required to receive a temporary password via email.", "changeemail": "Change or remove email address", "changeemail-summary": "", "changeemail-header": "Complete this form to change your email address. If you would like to remove the association of any email address from your account, leave the new email address blank when submitting the form.", diff --git a/languages/i18n/qqq.json b/languages/i18n/qqq.json index 4fa60a27ab..dc8c74b513 100644 --- a/languages/i18n/qqq.json +++ b/languages/i18n/qqq.json @@ -818,6 +818,7 @@ "passwordreset-ignored": "Shown when password reset was unsuccessful due to configuration problems.", "passwordreset-invalidemail": "Returned when the email address is syntatically invalid.", "passwordreset-nodata": "Returned when no data was provided.", + "passwordreset-username-email-required": "Used in [[Special:PasswordReset]].\n\nSee also:\n* {{msg-mw|tog-requireemail}}\n* {{msg-mw|prefs-help-requireemail}}", "changeemail": "Title of [[Special:ChangeEmail|special page]]. This page also allows removing the user's email address.", "changeemail-summary": "{{ignored}}", "changeemail-header": "Text of [[Special:ChangeEmail]].", diff --git a/tests/phpunit/includes/user/PasswordResetTest.php b/tests/phpunit/includes/user/PasswordResetTest.php index b5677fa99f..b37a6b45bc 100644 --- a/tests/phpunit/includes/user/PasswordResetTest.php +++ b/tests/phpunit/includes/user/PasswordResetTest.php @@ -1,6 +1,7 @@ $enableEmail, - 'PasswordResetRoutes' => $passwordResetRoutes, - ] ); - - return new ServiceOptions( PasswordReset::$constructorOptions, $hash ); - } + const VALID_IP = '1.2.3.4'; + const VALID_EMAIL = 'foo@bar.baz'; /** * @dataProvider provideIsAllowed @@ -29,7 +24,7 @@ class PasswordResetTest extends MediaWikiTestCase { public function testIsAllowed( $passwordResetRoutes, $enableEmail, $allowsAuthenticationDataChange, $canEditPrivate, $block, $globalBlock, $isAllowed ) { - $config = $this->makeConfig( $enableEmail, $passwordResetRoutes ); + $config = $this->makeConfig( $enableEmail, $passwordResetRoutes, false ); $authManager = $this->getMockBuilder( AuthManager::class )->disableOriginalConstructor() ->getMock(); @@ -199,69 +194,429 @@ class PasswordResetTest extends MediaWikiTestCase { ]; } - public function testExecute_email() { - $config = $this->makeConfig( true, [ 'username' => true, 'email' => true ] ); + /** + * @expectedException \LogicException + */ + public function testExecute_notAllowed() { + $user = $this->getMock( User::class ); + /** @var User $user */ + + $passwordReset = $this->getMockBuilder( PasswordReset::class ) + ->disableOriginalConstructor() + ->setMethods( [ 'isAllowed' ] ) + ->getMock(); + $passwordReset->expects( $this->any() ) + ->method( 'isAllowed' ) + ->with( $user ) + ->willReturn( Status::newFatal( 'somestatuscode' ) ); + /** @var PasswordReset $passwordReset */ + $passwordReset->execute( $user ); + } + + /** + * @dataProvider provideExecute + * @param string|bool $expectedError + * @param ServiceOptions $config + * @param User $performingUser + * @param PermissionManager $permissionManager + * @param AuthManager $authManager + * @param string|null $username + * @param string|null $email + * @param User[] $usersWithEmail + */ + public function testExecute( + $expectedError, + ServiceOptions $config, + User $performingUser, + PermissionManager $permissionManager, + AuthManager $authManager, + $username = '', + $email = '', + array $usersWithEmail = [] + ) { // Unregister the hooks for proper unit testing $this->mergeMwGlobalArrayValue( 'wgHooks', [ 'User::mailPasswordInternal' => [], 'SpecialPasswordResetOnSubmit' => [], ] ); - $authManager = $this->getMockBuilder( AuthManager::class )->disableOriginalConstructor() + $loadBalancer = $this->getMockBuilder( ILoadBalancer::class ) ->getMock(); - $authManager->expects( $this->any() )->method( 'allowsAuthenticationDataChange' ) + + $users = $this->makeUsers(); + + $lookupUser = function ( $username ) use ( $users ) { + return $users[ $username ] ?? false; + }; + + $passwordReset = $this->getMockBuilder( PasswordReset::class ) + ->setMethods( [ 'getUsersByEmail', 'isAllowed', 'lookupUser' ] ) + ->setConstructorArgs( [ + $config, + $authManager, + $permissionManager, + $loadBalancer, + new NullLogger() + ] ) + ->getMock(); + $passwordReset->method( 'getUsersByEmail' )->with( $email ) + ->willReturn( array_map( $lookupUser, $usersWithEmail ) ); + $passwordReset->method( 'isAllowed' ) ->willReturn( Status::newGood() ); - $authManager->expects( $this->exactly( 2 ) )->method( 'changeAuthenticationData' ); + $passwordReset->method( 'lookupUser' ) + ->willReturnCallback( $lookupUser ); - $permissionManager = $this->getMockBuilder( PermissionManager::class ) - ->disableOriginalConstructor() + /** @var PasswordReset $passwordReset */ + $status = $passwordReset->execute( $performingUser, $username, $email ); + $this->assertStatus( $status, $expectedError ); + } + + public function provideExecute() { + $defaultConfig = $this->makeConfig( true, [ 'username' => true, 'email' => true ], false ); + $emailRequiredConfig = $this->makeConfig( true, [ 'username' => true, 'email' => true ], true ); + $performingUser = $this->makePerformingUser( self::VALID_IP, false ); + $throttledUser = $this->makePerformingUser( self::VALID_IP, true ); + $permissionManager = $this->makePermissionManager( $performingUser, true ); + + return [ + 'Invalid email' => [ + 'expectedError' => 'passwordreset-invalidemail', + 'config' => $defaultConfig, + 'performingUser' => $throttledUser, + 'permissionManager' => $permissionManager, + 'authManager' => $this->makeAuthManager(), + 'username' => '', + 'email' => '[invalid email]', + 'usersWithEmail' => [], + ], + 'No username, no email' => [ + 'expectedError' => 'passwordreset-nodata', + 'config' => $defaultConfig, + 'performingUser' => $throttledUser, + 'permissionManager' => $permissionManager, + 'authManager' => $this->makeAuthManager(), + 'username' => '', + 'email' => '', + 'usersWithEmail' => [], + ], + 'Email route not enabled' => [ + 'expectedError' => 'passwordreset-nodata', + 'config' => $this->makeConfig( true, [ 'username' => true ], false ), + 'performingUser' => $throttledUser, + 'permissionManager' => $permissionManager, + 'authManager' => $this->makeAuthManager(), + 'username' => '', + 'email' => self::VALID_EMAIL, + 'usersWithEmail' => [], + ], + 'Username route not enabled' => [ + 'expectedError' => 'passwordreset-nodata', + 'config' => $this->makeConfig( true, [ 'email' => true ], false ), + 'performingUser' => $throttledUser, + 'permissionManager' => $permissionManager, + 'authManager' => $this->makeAuthManager(), + 'username' => 'User1', + 'email' => '', + 'usersWithEmail' => [], + ], + 'No routes enabled' => [ + 'expectedError' => 'passwordreset-nodata', + 'config' => $this->makeConfig( true, [], false ), + 'performingUser' => $throttledUser, + 'permissionManager' => $permissionManager, + 'authManager' => $this->makeAuthManager(), + 'username' => 'User1', + 'email' => self::VALID_EMAIL, + 'usersWithEmail' => [], + ], + 'Email reqiured for resets, but is empty' => [ + 'expectedError' => 'passwordreset-username-email-required', + 'config' => $emailRequiredConfig, + 'performingUser' => $throttledUser, + 'permissionManager' => $permissionManager, + 'authManager' => $this->makeAuthManager(), + 'username' => 'User1', + 'email' => '', + 'usersWithEmail' => [], + ], + 'Email reqiured for resets, is invalid' => [ + 'expectedError' => 'passwordreset-invalidemail', + 'config' => $emailRequiredConfig, + 'performingUser' => $throttledUser, + 'permissionManager' => $permissionManager, + 'authManager' => $this->makeAuthManager(), + 'username' => 'User1', + 'email' => '[invalid email]', + 'usersWithEmail' => [], + ], + 'Throttled' => [ + 'expectedError' => 'actionthrottledtext', + 'config' => $defaultConfig, + 'performingUser' => $throttledUser, + 'permissionManager' => $permissionManager, + 'authManager' => $this->makeAuthManager(), + 'username' => 'User1', + 'email' => '', + 'usersWithEmail' => [], + ], + 'No user by this username' => [ + 'expectedError' => 'nosuchuser', + 'config' => $defaultConfig, + 'performingUser' => $performingUser, + 'permissionManager' => $permissionManager, + 'authManager' => $this->makeAuthManager(), + 'username' => 'Nonexistent user', + 'email' => '', + 'usersWithEmail' => [], + ], + 'If no users with this email found, pretend everything is OK' => [ + 'expectedError' => false, + 'config' => $defaultConfig, + 'performingUser' => $performingUser, + 'permissionManager' => $permissionManager, + 'authManager' => $this->makeAuthManager(), + 'username' => '', + 'email' => 'some@not.found.email', + 'usersWithEmail' => [], + ], + 'No email for the user' => [ + 'expectedError' => 'noemail', + 'config' => $defaultConfig, + 'performingUser' => $performingUser, + 'permissionManager' => $permissionManager, + 'authManager' => $this->makeAuthManager(), + 'username' => 'BadUser', + 'email' => '', + 'usersWithEmail' => [], + ], + 'Email reqiured for resets, no match' => [ + 'expectedError' => false, + 'config' => $emailRequiredConfig, + 'performingUser' => $performingUser, + 'permissionManager' => $permissionManager, + 'authManager' => $this->makeAuthManager(), + 'username' => 'User1', + 'email' => 'some@other.email', + 'usersWithEmail' => [], + ], + "Couldn't determine the performing user's IP" => [ + 'expectedError' => 'badipaddress', + 'config' => $defaultConfig, + 'performingUser' => $this->makePerformingUser( null, false ), + 'permissionManager' => $permissionManager, + 'authManager' => $this->makeAuthManager(), + 'username' => 'User1', + 'email' => '', + 'usersWithEmail' => [], + ], + 'User is allowed, but ignored' => [ + 'expectedError' => 'passwordreset-ignored', + 'config' => $defaultConfig, + 'performingUser' => $performingUser, + 'permissionManager' => $permissionManager, + 'authManager' => $this->makeAuthManager( [ 'User1' ], 0, [ 'User1' ] ), + 'username' => 'User1', + 'email' => '', + 'usersWithEmail' => [], + ], + 'One of users is ignored' => [ + 'expectedError' => 'passwordreset-ignored', + 'config' => $defaultConfig, + 'performingUser' => $performingUser, + 'permissionManager' => $permissionManager, + 'authManager' => $this->makeAuthManager( [ 'User1', 'User2' ], 0, [ 'User2' ] ), + 'username' => '', + 'email' => self::VALID_EMAIL, + 'usersWithEmail' => [ 'User1', 'User2' ], + ], + 'User is rejected' => [ + 'expectedError' => 'rejected by test mock', + 'config' => $defaultConfig, + 'performingUser' => $performingUser, + 'permissionManager' => $permissionManager, + 'authManager' => $this->makeAuthManager(), + 'username' => 'User1', + 'email' => '', + 'usersWithEmail' => [], + ], + 'One of users is rejected' => [ + 'expectedError' => 'rejected by test mock', + 'config' => $defaultConfig, + 'performingUser' => $performingUser, + 'permissionManager' => $permissionManager, + 'authManager' => $this->makeAuthManager( [ 'User1' ] ), + 'username' => '', + 'email' => self::VALID_EMAIL, + 'usersWithEmail' => [ 'User1', 'User2' ], + ], + 'Reset one user via password' => [ + 'expectedError' => false, + 'config' => $defaultConfig, + 'performingUser' => $performingUser, + 'permissionManager' => $permissionManager, + 'authManager' => $this->makeAuthManager( [ 'User1' ], 1 ), + 'username' => 'User1', + 'email' => self::VALID_EMAIL, + // Make sure that only the user specified by username is reset + 'usersWithEmail' => [ 'User1', 'User2' ], + ], + 'Reset one user via email' => [ + 'expectedError' => false, + 'config' => $defaultConfig, + 'performingUser' => $performingUser, + 'permissionManager' => $permissionManager, + 'authManager' => $this->makeAuthManager( [ 'User1' ], 1 ), + 'username' => '', + 'email' => self::VALID_EMAIL, + 'usersWithEmail' => [ 'User1' ], + ], + 'Reset multiple users via email' => [ + 'expectedError' => false, + 'config' => $defaultConfig, + 'performingUser' => $performingUser, + 'permissionManager' => $permissionManager, + 'authManager' => $this->makeAuthManager( [ 'User1', 'User2' ], 2 ), + 'username' => '', + 'email' => self::VALID_EMAIL, + 'usersWithEmail' => [ 'User1', 'User2' ], + ], + "Email is required for resets, user didn't opt in" => [ + 'expectedError' => false, + 'config' => $emailRequiredConfig, + 'performingUser' => $performingUser, + 'permissionManager' => $permissionManager, + 'authManager' => $this->makeAuthManager( [ 'User2' ], 1 ), + 'username' => 'User2', + 'email' => self::VALID_EMAIL, + 'usersWithEmail' => [ 'User2' ], + ], + ]; + } + + private function assertStatus( StatusValue $status, $error = false ) { + if ( $error === false ) { + $this->assertTrue( $status->isGood(), 'Expected status to be good' ); + } else { + $this->assertFalse( $status->isGood(), 'Expected status to not be good' ); + if ( is_string( $error ) ) { + $this->assertNotEmpty( $status->getErrors() ); + $message = $status->getErrors()[0]['message']; + if ( $message instanceof MessageSpecifier ) { + $message = $message->getKey(); + } + $this->assertSame( $error, $message ); + } + } + } + + private function makeConfig( $enableEmail, array $passwordResetRoutes, $emailForResets ) { + $hash = [ + 'AllowRequiringEmailForResets' => $emailForResets, + 'EnableEmail' => $enableEmail, + 'PasswordResetRoutes' => $passwordResetRoutes, + ]; + + return new ServiceOptions( PasswordReset::$constructorOptions, $hash ); + } + + /** + * @param string|null $ip + * @param bool $pingLimited + * @return User + */ + private function makePerformingUser( $ip, $pingLimited ) : User { + $request = $this->getMockBuilder( WebRequest::class ) ->getMock(); - $permissionManager->method( 'userHasRight' )->willReturn( true ); + $request->method( 'getIP' ) + ->willReturn( $ip ); + /** @var WebRequest $request */ - $loadBalancer = $this->getMockBuilder( ILoadBalancer::class ) + $user = $this->getMockBuilder( User::class ) + ->setMethods( [ 'getName', 'pingLimiter', 'getRequest' ] ) ->getMock(); - $request = new FauxRequest(); - $request->setIP( '1.2.3.4' ); - $performingUser = $this->getMockBuilder( User::class )->getMock(); - $performingUser->expects( $this->any() )->method( 'getRequest' )->willReturn( $request ); - $performingUser->expects( $this->any() )->method( 'getName' )->willReturn( 'Performer' ); + $user->method( 'getName' ) + ->willReturn( 'SomeUser' ); + $user->method( 'pingLimiter' ) + ->with( 'mailpassword' ) + ->willReturn( $pingLimited ); + $user->method( 'getRequest' ) + ->willReturn( $request ); + /** @var User $user */ + return $user; + } + + private function makePermissionManager( User $performingUser, $isAllowed ) : PermissionManager { $permissionManager = $this->getMockBuilder( PermissionManager::class ) ->disableOriginalConstructor() ->getMock(); - $permissionManager->expects( $this->once() ) - ->method( 'userHasRight' ) + $permissionManager->method( 'userHasRight' ) ->with( $performingUser, 'editmyprivateinfo' ) - ->willReturn( true ); + ->willReturn( $isAllowed ); - $targetUser1 = $this->getMockBuilder( User::class )->getMock(); - $targetUser2 = $this->getMockBuilder( User::class )->getMock(); - $targetUser1->expects( $this->any() )->method( 'getName' )->willReturn( 'User1' ); - $targetUser2->expects( $this->any() )->method( 'getName' )->willReturn( 'User2' ); - $targetUser1->expects( $this->any() )->method( 'getId' )->willReturn( 1 ); - $targetUser2->expects( $this->any() )->method( 'getId' )->willReturn( 2 ); - $targetUser1->expects( $this->any() )->method( 'getEmail' )->willReturn( 'foo@bar.baz' ); - $targetUser2->expects( $this->any() )->method( 'getEmail' )->willReturn( 'foo@bar.baz' ); + /** @var PermissionManager $permissionManager */ + return $permissionManager; + } - $passwordReset = $this->getMockBuilder( PasswordReset::class ) - ->setMethods( [ 'getUsersByEmail' ] ) - ->setConstructorArgs( [ - $config, - $authManager, - $permissionManager, - $loadBalancer, - new NullLogger() - ] ) + /** + * @param string[] $allowed + * @param int $numUsersToAuth + * @param string[] $ignored + * @return AuthManager + */ + private function makeAuthManager( + array $allowed = [], + $numUsersToAuth = 0, + array $ignored = [] + ) : AuthManager { + $authManager = $this->getMockBuilder( AuthManager::class ) + ->disableOriginalConstructor() ->getMock(); - $passwordReset->expects( $this->any() )->method( 'getUsersByEmail' )->with( 'foo@bar.baz' ) - ->willReturn( [ $targetUser1, $targetUser2 ] ); + $authManager->method( 'allowsAuthenticationDataChange' ) + ->willReturnCallback( + function ( TemporaryPasswordAuthenticationRequest $req ) use ( $allowed, $ignored ) { + $value = in_array( $req->username, $ignored, true ) + ? 'ignored' + : 'okie dokie'; + return in_array( $req->username, $allowed, true ) + ? Status::newGood( $value ) + : Status::newFatal( 'rejected by test mock' ); + } ); + $authManager->expects( $this->exactly( $numUsersToAuth ) ) + ->method( 'changeAuthenticationData' ); + + /** @var AuthManager $authManager */ + return $authManager; + } + + /** + * @return User[] + */ + private function makeUsers() { + $user1 = $this->getMockBuilder( User::class )->getMock(); + $user2 = $this->getMockBuilder( User::class )->getMock(); + $user1->method( 'getName' )->willReturn( 'User1' ); + $user2->method( 'getName' )->willReturn( 'User2' ); + $user1->method( 'getId' )->willReturn( 1 ); + $user2->method( 'getId' )->willReturn( 2 ); + $user1->method( 'getEmail' )->willReturn( self::VALID_EMAIL ); + $user2->method( 'getEmail' )->willReturn( self::VALID_EMAIL ); + + $user1->method( 'getBoolOption' ) + ->with( 'requireemail' ) + ->willReturn( true ); - $status = $passwordReset->isAllowed( $performingUser ); - $this->assertTrue( $status->isGood() ); + $badUser = $this->getMockBuilder( User::class )->getMock(); + $badUser->method( 'getName' )->willReturn( 'BadUser' ); + $badUser->method( 'getId' )->willReturn( 3 ); + $badUser->method( 'getEmail' )->willReturn( null ); - $status = $passwordReset->execute( $performingUser, null, 'foo@bar.baz' ); - $this->assertTrue( $status->isGood() ); + return [ + 'User1' => $user1, + 'User2' => $user2, + 'BadUser' => $badUser, + ]; } } -- 2.20.1