From: hmonroy Date: Wed, 15 Apr 2020 20:40:44 +0000 (-0700) Subject: [SECURITY] Password Reset Updates X-Git-Tag: 1.34.2~5 X-Git-Url: http://git.cyclocoop.org/?p=lhc%2Fweb%2Fwiklou.git;a=commitdiff_plain;h=11c82a8660fa01f4407c8551b77ec58006b087e8 [SECURITY] Password Reset Updates * Include throttle message in password reset success * Update password reset success message to include throttle message. * Remove password reset invalid email message * Show general message when an invalid email is submitted. * Note: squashing two related master commits for security backports. Related Change-Ids: * Ia247034ec9a93689218c619d391a666c6b92991a * I98a35af26930f3d66308065e271e9617fdbf5076 Bug: T249730 Change-Id: Ie329cc927742ed8637ff6479f63adc79b56a14f8 --- diff --git a/includes/user/PasswordReset.php b/includes/user/PasswordReset.php index 5491b6c98f..1ce12c640a 100644 --- a/includes/user/PasswordReset.php +++ b/includes/user/PasswordReset.php @@ -166,6 +166,12 @@ class PasswordReset implements LoggerAwareInterface { . ' is not allowed to reset passwords' ); } + // Check against the rate limiter. If the $wgRateLimit is reached, we want to pretend + // that the request was good to avoid displaying an error message. + if ( $performingUser->pingLimiter( 'mailpassword' ) ) { + return StatusValue::newGood(); + } + $username = $username ?? ''; $email = $email ?? ''; @@ -176,11 +182,21 @@ class PasswordReset implements LoggerAwareInterface { $users = [ $this->lookupUser( $username ) ]; } elseif ( $resetRoutes['email'] && $email ) { if ( !Sanitizer::validateEmail( $email ) ) { - return StatusValue::newFatal( 'passwordreset-invalidemail' ); + // Only email was supplied but not valid: pretend everything's fine. + return StatusValue::newGood(); } + // Only email was provided $method = 'email'; $users = $this->getUsersByEmail( $email ); $username = null; + // Remove users whose preference 'requireemail' is on since username was not submitted + if ( $this->config->get( 'AllowRequiringEmailForResets' ) ) { + foreach ( $users as $index => $user ) { + if ( $user->getBoolOption( 'requireemail' ) ) { + unset( $users[$index] ); + } + } + } } else { // The user didn't supply any data return StatusValue::newFatal( 'passwordreset-nodata' ); @@ -193,53 +209,59 @@ class PasswordReset implements LoggerAwareInterface { // Email gets set to null for backward compatibility 'Email' => $method === 'email' ? $email : null, ]; + + // Recreate the $users array with its values so that we reset the numeric keys since + // the key '0' might have been unset from $users array. 'SpecialPasswordResetOnSubmit' + // hook assumes that index '0' is defined if $users is not empty. + $users = array_values( $users ); + if ( !Hooks::run( 'SpecialPasswordResetOnSubmit', [ &$users, $data, &$error ] ) ) { return StatusValue::newFatal( Message::newFromSpecifier( $error ) ); } - $firstUser = $users[0] ?? null; + // Get the first element in $users by using `reset` function just in case $users is changed + // in 'SpecialPasswordResetOnSubmit' hook. + $firstUser = reset( $users ) ?? 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 ( $requireEmail && ( $email === '' || !Sanitizer::validateEmail( $email ) ) ) { + // Email is required, and not supplied or not valid: pretend everything's fine. + return StatusValue::newGood(); } if ( !$users ) { if ( $method === 'email' ) { // Don't reveal whether or not an email address is in use - return StatusValue::newGood( [] ); + return StatusValue::newGood(); } else { return StatusValue::newFatal( 'noname' ); } } + // If the username is not valid, tell the user. + if ( $username && !User::getCanonicalName( $username ) ) { + return StatusValue::newFatal( 'noname' ); + } + + // If the username doesn't exist, don't tell the user. + // This is not to avoid disclosure, as this information is available elsewhere, + // but it simplifies the password reset UX. T238961. if ( !$firstUser instanceof User || !$firstUser->getId() ) { - // Don't parse username as wikitext (T67501) - return StatusValue::newFatal( wfMessage( 'nosuchuser', wfEscapeWikiText( $username ) ) ); + return StatusValue::newGood(); } - // All the users will have the same email address + // The user doesn't have an email address, but pretend everything's fine to avoid + // disclosing this fact. Note that all the users will have the same email address (or none), + // so there's no need to check more than the first. if ( !$firstUser->getEmail() ) { - // This won't be reachable from the email route, so safe to expose the username - return StatusValue::newFatal( wfMessage( 'noemail', - wfEscapeWikiText( $firstUser->getName() ) ) ); + return StatusValue::newGood(); } + // Email is required but the email doesn't match: pretend everything's fine. if ( $requireEmail && $firstUser->getEmail() !== $email ) { - // Pretend everything's fine to avoid disclosure return StatusValue::newGood(); } @@ -259,7 +281,15 @@ class PasswordReset implements LoggerAwareInterface { $req->username = $user->getName(); $req->mailpassword = true; $req->caller = $performingUser->getName(); + $status = $this->authManager->allowsAuthenticationDataChange( $req, true ); + // If status is good and the value is 'throttled-mailpassword', we want to pretend + // that the request was good to avoid displaying an error message and disclose + // if a reset password was previously sent. + if ( $status->isGood() && $status->getValue() === 'throttled-mailpassword' ) { + return StatusValue::newGood(); + } + if ( $status->isGood() && $status->getValue() !== 'ignored' ) { $reqs[] = $req; } elseif ( $result->isGood() ) { diff --git a/languages/i18n/en.json b/languages/i18n/en.json index 92d1d9cb3a..7c4eeb692b 100644 --- a/languages/i18n/en.json +++ b/languages/i18n/en.json @@ -603,10 +603,13 @@ "passwordreset-emailelement": "Username:\n$1\n\nTemporary password:\n$2", "passwordreset-emailsentemail": "If this email address is associated with your account, then a password reset email will be sent.", "passwordreset-emailsentusername": "If there is an email address associated with this username, then a password reset email will be sent.", + "passwordreset-success": "You have a requested a password reset.", + "passwordreset-success-details-generic": "If the information submitted is valid, a password reset email will be sent. If you haven't received an email, we recommend that you visit the [[mw:Special:MyLanguage/Help:Reset_password|reset password help page]] or try again later. You can only request a limited number of password resets within a short period of time. Only one password reset email will be sent per valid account every {{PLURAL:$1|hour|$1 hours}} in order to prevent abuse.", + "passwordreset-success-info": "The details you submitted are: $1", + "passwordreset-emailtext-require-email": "However, if you did not generate this request and want to prevent unsolicited\nemails, you may want to update your email options at\n$1.\nYou can require both username and email address to generate password reset\nemails. This may reduce the number of such incidents.", "passwordreset-nocaller": "A caller must be provided", "passwordreset-nosuchcaller": "Caller does not exist: $1", "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", diff --git a/languages/i18n/qqq.json b/languages/i18n/qqq.json index dc8c74b513..d21857cac3 100644 --- a/languages/i18n/qqq.json +++ b/languages/i18n/qqq.json @@ -816,7 +816,6 @@ "passwordreset-nocaller": "Shown when a password reset was requested but the process failed due to an internal error related to missing details about the origin (caller) of the password reset request.", "passwordreset-nosuchcaller": "Shown when a password reset was requested but the username of the caller could not be resolved to a user. This is an internal error.\n\nParameters:\n* $1 - username of the caller", "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.", diff --git a/tests/phpunit/includes/user/PasswordResetTest.php b/tests/phpunit/includes/user/PasswordResetTest.php index 351ef540bd..d16244b2f3 100644 --- a/tests/phpunit/includes/user/PasswordResetTest.php +++ b/tests/phpunit/includes/user/PasswordResetTest.php @@ -2,8 +2,8 @@ use MediaWiki\Auth\AuthManager; use MediaWiki\Auth\TemporaryPasswordAuthenticationRequest; -use MediaWiki\Block\DatabaseBlock; use MediaWiki\Block\CompositeBlock; +use MediaWiki\Block\DatabaseBlock; use MediaWiki\Block\SystemBlock; use MediaWiki\Config\ServiceOptions; use MediaWiki\Permissions\PermissionManager; @@ -43,7 +43,7 @@ class PasswordResetTest extends MediaWikiTestCase { ->with( $user, 'editmyprivateinfo' ) ->willReturn( $canEditPrivate ); - $loadBalancer = $this->getMockBuilder( ILoadBalancer::class )->getMock(); + $loadBalancer = $this->createMock( ILoadBalancer::class ); $passwordReset = new PasswordReset( $config, @@ -194,11 +194,8 @@ class PasswordResetTest extends MediaWikiTestCase { ]; } - /** - * @expectedException \LogicException - */ public function testExecute_notAllowed() { - $user = $this->getMock( User::class ); + $user = $this->createMock( User::class ); /** @var User $user */ $passwordReset = $this->getMockBuilder( PasswordReset::class ) @@ -211,6 +208,7 @@ class PasswordResetTest extends MediaWikiTestCase { ->willReturn( Status::newFatal( 'somestatuscode' ) ); /** @var PasswordReset $passwordReset */ + $this->expectException( \LogicException::class ); $passwordReset->execute( $user ); } @@ -242,8 +240,7 @@ class PasswordResetTest extends MediaWikiTestCase { 'SpecialPasswordResetOnSubmit' => [], ] ); - $loadBalancer = $this->getMockBuilder( ILoadBalancer::class ) - ->getMock(); + $loadBalancer = $this->createMock( ILoadBalancer::class ); $users = $this->makeUsers(); @@ -281,12 +278,32 @@ class PasswordResetTest extends MediaWikiTestCase { $permissionManager = $this->makePermissionManager( $performingUser, true ); return [ - 'Invalid email' => [ - 'expectedError' => 'passwordreset-invalidemail', + 'Throttled, pretend everything is ok' => [ + 'expectedError' => false, 'config' => $defaultConfig, 'performingUser' => $throttledUser, 'permissionManager' => $permissionManager, 'authManager' => $this->makeAuthManager(), + 'username' => 'User1', + 'email' => '', + 'usersWithEmail' => [], + ], + 'Throttled, email required for resets, is invalid, pretend everything is ok' => [ + 'expectedError' => false, + 'config' => $emailRequiredConfig, + 'performingUser' => $throttledUser, + 'permissionManager' => $permissionManager, + 'authManager' => $this->makeAuthManager(), + 'username' => 'User1', + 'email' => '[invalid email]', + 'usersWithEmail' => [], + ], + 'Invalid email, pretend everything is OK' => [ + 'expectedError' => false, + 'config' => $defaultConfig, + 'performingUser' => $performingUser, + 'permissionManager' => $permissionManager, + 'authManager' => $this->makeAuthManager(), 'username' => '', 'email' => '[invalid email]', 'usersWithEmail' => [], @@ -294,7 +311,7 @@ class PasswordResetTest extends MediaWikiTestCase { 'No username, no email' => [ 'expectedError' => 'passwordreset-nodata', 'config' => $defaultConfig, - 'performingUser' => $throttledUser, + 'performingUser' => $performingUser, 'permissionManager' => $permissionManager, 'authManager' => $this->makeAuthManager(), 'username' => '', @@ -304,7 +321,7 @@ class PasswordResetTest extends MediaWikiTestCase { 'Email route not enabled' => [ 'expectedError' => 'passwordreset-nodata', 'config' => $this->makeConfig( true, [ 'username' => true ], false ), - 'performingUser' => $throttledUser, + 'performingUser' => $performingUser, 'permissionManager' => $permissionManager, 'authManager' => $this->makeAuthManager(), 'username' => '', @@ -314,7 +331,7 @@ class PasswordResetTest extends MediaWikiTestCase { 'Username route not enabled' => [ 'expectedError' => 'passwordreset-nodata', 'config' => $this->makeConfig( true, [ 'email' => true ], false ), - 'performingUser' => $throttledUser, + 'performingUser' => $performingUser, 'permissionManager' => $permissionManager, 'authManager' => $this->makeAuthManager(), 'username' => 'User1', @@ -324,45 +341,45 @@ class PasswordResetTest extends MediaWikiTestCase { 'No routes enabled' => [ 'expectedError' => 'passwordreset-nodata', 'config' => $this->makeConfig( true, [], false ), - 'performingUser' => $throttledUser, + 'performingUser' => $performingUser, 'permissionManager' => $permissionManager, 'authManager' => $this->makeAuthManager(), 'username' => 'User1', 'email' => self::VALID_EMAIL, 'usersWithEmail' => [], ], - 'Email reqiured for resets, but is empty' => [ - 'expectedError' => 'passwordreset-username-email-required', + 'Email required for resets but is empty, pretend everything is OK' => [ + 'expectedError' => false, 'config' => $emailRequiredConfig, - 'performingUser' => $throttledUser, + 'performingUser' => $performingUser, 'permissionManager' => $permissionManager, 'authManager' => $this->makeAuthManager(), 'username' => 'User1', 'email' => '', 'usersWithEmail' => [], ], - 'Email reqiured for resets, is invalid' => [ - 'expectedError' => 'passwordreset-invalidemail', + 'Email required for resets but is invalid, pretend everything is OK' => [ + 'expectedError' => false, 'config' => $emailRequiredConfig, - 'performingUser' => $throttledUser, + 'performingUser' => $performingUser, 'permissionManager' => $permissionManager, 'authManager' => $this->makeAuthManager(), 'username' => 'User1', 'email' => '[invalid email]', 'usersWithEmail' => [], ], - 'Throttled' => [ - 'expectedError' => 'actionthrottledtext', + 'Password email already sent within 24 hours, pretend everything is ok' => [ + 'expectedError' => false, 'config' => $defaultConfig, - 'performingUser' => $throttledUser, + 'performingUser' => $performingUser, 'permissionManager' => $permissionManager, - 'authManager' => $this->makeAuthManager(), + 'authManager' => $this->makeAuthManager( [ 'User1' ], 0, [], [ 'User1' ] ), 'username' => 'User1', 'email' => '', - 'usersWithEmail' => [], + 'usersWithEmail' => [ 'User1' ], ], - 'No user by this username' => [ - 'expectedError' => 'nosuchuser', + 'No user by this username, pretend everything is OK' => [ + 'expectedError' => false, 'config' => $defaultConfig, 'performingUser' => $performingUser, 'permissionManager' => $permissionManager, @@ -371,6 +388,16 @@ class PasswordResetTest extends MediaWikiTestCase { 'email' => '', 'usersWithEmail' => [], ], + 'Username is not valid' => [ + 'expectedError' => 'noname', + 'config' => $defaultConfig, + 'performingUser' => $performingUser, + 'permissionManager' => $permissionManager, + 'authManager' => $this->makeAuthManager(), + 'username' => 'Invalid|username', + 'email' => '', + 'usersWithEmail' => [], + ], 'If no users with this email found, pretend everything is OK' => [ 'expectedError' => false, 'config' => $defaultConfig, @@ -381,8 +408,8 @@ class PasswordResetTest extends MediaWikiTestCase { 'email' => 'some@not.found.email', 'usersWithEmail' => [], ], - 'No email for the user' => [ - 'expectedError' => 'noemail', + 'No email for the user, pretend everything is OK' => [ + 'expectedError' => false, 'config' => $defaultConfig, 'performingUser' => $performingUser, 'permissionManager' => $permissionManager, @@ -391,7 +418,7 @@ class PasswordResetTest extends MediaWikiTestCase { 'email' => '', 'usersWithEmail' => [], ], - 'Email reqiured for resets, no match' => [ + 'Email required for resets, no match' => [ 'expectedError' => false, 'config' => $emailRequiredConfig, 'performingUser' => $performingUser, @@ -492,6 +519,16 @@ class PasswordResetTest extends MediaWikiTestCase { 'email' => self::VALID_EMAIL, 'usersWithEmail' => [ 'User2' ], ], + 'Reset three users via email that did not opt in, multiple users with same email' => [ + 'expectedError' => false, + 'config' => $emailRequiredConfig, + 'performingUser' => $performingUser, + 'permissionManager' => $permissionManager, + 'authManager' => $this->makeAuthManager( [ 'User2', 'User3', 'User4' ], 3, [ 'User1' ] ), + 'username' => '', + 'email' => self::VALID_EMAIL, + 'usersWithEmail' => [ 'User1', 'User2', 'User3', 'User4' ], + ], ]; } @@ -562,25 +599,37 @@ class PasswordResetTest extends MediaWikiTestCase { } /** - * @param string[] $allowed - * @param int $numUsersToAuth - * @param string[] $ignored + * @param string[] $allowed Usernames that are allowed to send password reset email + * by AuthManager's allowsAuthenticationDataChange method. + * @param int $numUsersToAuth Number of users that will receive email + * @param string[] $ignored Usernames that are allowed but ignored by AuthManager's + * allowsAuthenticationDataChange method and will not receive password reset email. + * @param string[] $mailThrottledLimited Usernames that have already + * received the password reset email within a given time, and AuthManager + * changeAuthenticationData method will mark them as 'throttled-mailpassword.' * @return AuthManager */ private function makeAuthManager( array $allowed = [], $numUsersToAuth = 0, - array $ignored = [] + array $ignored = [], + array $mailThrottledLimited = [] ) : AuthManager { $authManager = $this->getMockBuilder( AuthManager::class ) ->disableOriginalConstructor() ->getMock(); $authManager->method( 'allowsAuthenticationDataChange' ) ->willReturnCallback( - function ( TemporaryPasswordAuthenticationRequest $req ) use ( $allowed, $ignored ) { + function ( TemporaryPasswordAuthenticationRequest $req ) + use ( $allowed, $ignored, $mailThrottledLimited ) { + if ( in_array( $req->username, $mailThrottledLimited, true ) ) { + return Status::newGood( 'throttled-mailpassword' ); + } + $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' ); @@ -600,12 +649,20 @@ class PasswordResetTest extends MediaWikiTestCase { private function makeUsers() { $user1 = $this->getMockBuilder( User::class )->getMock(); $user2 = $this->getMockBuilder( User::class )->getMock(); + $user3 = $this->getMockBuilder( User::class )->getMock(); + $user4 = $this->getMockBuilder( User::class )->getMock(); $user1->method( 'getName' )->willReturn( 'User1' ); $user2->method( 'getName' )->willReturn( 'User2' ); + $user3->method( 'getName' )->willReturn( 'User3' ); + $user4->method( 'getName' )->willReturn( 'User4' ); $user1->method( 'getId' )->willReturn( 1 ); $user2->method( 'getId' )->willReturn( 2 ); + $user3->method( 'getId' )->willReturn( 3 ); + $user4->method( 'getId' )->willReturn( 4 ); $user1->method( 'getEmail' )->willReturn( self::VALID_EMAIL ); $user2->method( 'getEmail' )->willReturn( self::VALID_EMAIL ); + $user3->method( 'getEmail' )->willReturn( self::VALID_EMAIL ); + $user4->method( 'getEmail' )->willReturn( self::VALID_EMAIL ); $user1->method( 'getBoolOption' ) ->with( 'requireemail' ) @@ -613,12 +670,14 @@ class PasswordResetTest extends MediaWikiTestCase { $badUser = $this->getMockBuilder( User::class )->getMock(); $badUser->method( 'getName' )->willReturn( 'BadUser' ); - $badUser->method( 'getId' )->willReturn( 3 ); + $badUser->method( 'getId' )->willReturn( 5 ); $badUser->method( 'getEmail' )->willReturn( null ); return [ 'User1' => $user1, 'User2' => $user2, + 'User3' => $user3, + 'User4' => $user4, 'BadUser' => $badUser, ]; }