From 9feb18149101fb505b8d55beb38c2baa9f848384 Mon Sep 17 00:00:00 2001 From: Max Semenik Date: Mon, 9 Sep 2019 19:49:12 -0700 Subject: [PATCH] Turn PasswordReset into a service My team has plans to work in this area, better make it more testable. Bug: T232694 Change-Id: I200874ec10db69378ada1743b2a7953b3fa01e3e (cherry picked from commit 631f56c5766f582f21dd35eff0376b14692aa145) --- RELEASE-NOTES-1.34 | 1 + includes/MediaWikiServices.php | 9 +++ includes/ServiceWiring.php | 11 ++++ includes/api/ApiResetPassword.php | 7 +-- .../specialpage/LoginSignupSpecialPage.php | 6 +- includes/specials/SpecialPasswordReset.php | 7 +-- includes/user/PasswordReset.php | 60 +++++++++++++------ .../includes/user/PasswordResetTest.php | 44 ++++++++++---- 8 files changed, 100 insertions(+), 45 deletions(-) diff --git a/RELEASE-NOTES-1.34 b/RELEASE-NOTES-1.34 index 48fcbafb97..3f9ae9db0e 100644 --- a/RELEASE-NOTES-1.34 +++ b/RELEASE-NOTES-1.34 @@ -598,6 +598,7 @@ because of Phabricator reports. $wgGroupPermissions['sysop']['blockemail'] = true; * ApiQueryBase::showHiddenUsersAddBlockInfo() is deprecated. Use ApiQueryBlockInfoTrait instead. +* PasswordReset is now a service, its direct instantiation is deprecated. === Other changes in 1.34 === * Added option to specify "Various authors" as author in extension credits using diff --git a/includes/MediaWikiServices.php b/includes/MediaWikiServices.php index 8f4ddf60dc..f1fa8922dd 100644 --- a/includes/MediaWikiServices.php +++ b/includes/MediaWikiServices.php @@ -19,6 +19,7 @@ use MediaWiki\Block\BlockRestrictionStore; use MediaWiki\FileBackend\FSFile\TempFSFileFactory; use MediaWiki\FileBackend\LockManager\LockManagerGroupFactory; use MediaWiki\Http\HttpRequestFactory; +use PasswordReset; use Wikimedia\Message\IMessageFormatterFactory; use MediaWiki\Page\MovePageFactory; use MediaWiki\Permissions\PermissionManager; @@ -818,6 +819,14 @@ class MediaWikiServices extends ServiceContainer { return $this->getService( 'PasswordFactory' ); } + /** + * @since 1.34 + * @return PasswordReset + */ + public function getPasswordReset() : PasswordReset { + return $this->getService( 'PasswordReset' ); + } + /** * @since 1.32 * @return StatsdDataFactoryInterface diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index 83847d80ae..2c1725ed75 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -521,6 +521,17 @@ return [ ); }, + 'PasswordReset' => function ( MediaWikiServices $services ) : PasswordReset { + $options = new ServiceOptions( PasswordReset::$constructorOptions, $services->getMainConfig() ); + return new PasswordReset( + $options, + AuthManager::singleton(), + $services->getPermissionManager(), + $services->getDBLoadBalancer(), + LoggerFactory::getInstance( 'authentication' ) + ); + }, + 'PerDbNameStatsdDataFactory' => function ( MediaWikiServices $services ) : StatsdDataFactoryInterface { $config = $services->getMainConfig(); diff --git a/includes/api/ApiResetPassword.php b/includes/api/ApiResetPassword.php index 6b1c217017..6f13af212a 100644 --- a/includes/api/ApiResetPassword.php +++ b/includes/api/ApiResetPassword.php @@ -20,7 +20,6 @@ * @file */ -use MediaWiki\Auth\AuthManager; use MediaWiki\MediaWikiServices; /** @@ -64,11 +63,7 @@ class ApiResetPassword extends ApiBase { $this->requireOnlyOneParameter( $params, 'user', 'email' ); - $passwordReset = new PasswordReset( - $this->getConfig(), - AuthManager::singleton(), - MediaWikiServices::getInstance()->getPermissionManager() - ); + $passwordReset = MediaWikiServices::getInstance()->getPasswordReset(); $status = $passwordReset->isAllowed( $this->getUser() ); if ( !$status->isOK() ) { diff --git a/includes/specialpage/LoginSignupSpecialPage.php b/includes/specialpage/LoginSignupSpecialPage.php index 8be029aac8..86365c58b5 100644 --- a/includes/specialpage/LoginSignupSpecialPage.php +++ b/includes/specialpage/LoginSignupSpecialPage.php @@ -977,11 +977,7 @@ abstract class LoginSignupSpecialPage extends AuthManagerSpecialPage { } } if ( !$this->isSignup() && $this->showExtraInformation() ) { - $passwordReset = new PasswordReset( - $this->getConfig(), - AuthManager::singleton(), - MediaWikiServices::getInstance()->getPermissionManager() - ); + $passwordReset = MediaWikiServices::getInstance()->getPasswordReset(); if ( $passwordReset->isAllowed( $this->getUser() )->isGood() ) { $fieldDefinitions['passwordReset'] = [ 'type' => 'info', diff --git a/includes/specials/SpecialPasswordReset.php b/includes/specials/SpecialPasswordReset.php index 2ef96ad854..c1d30ee58c 100644 --- a/includes/specials/SpecialPasswordReset.php +++ b/includes/specials/SpecialPasswordReset.php @@ -21,7 +21,6 @@ * @ingroup SpecialPage */ -use MediaWiki\Auth\AuthManager; use MediaWiki\MediaWikiServices; /** @@ -53,11 +52,7 @@ class SpecialPasswordReset extends FormSpecialPage { private function getPasswordReset() { if ( $this->passwordReset === null ) { - $this->passwordReset = new PasswordReset( - $this->getConfig(), - AuthManager::singleton(), - MediaWikiServices::getInstance()->getPermissionManager() - ); + $this->passwordReset = MediaWikiServices::getInstance()->getPasswordReset(); } return $this->passwordReset; } diff --git a/includes/user/PasswordReset.php b/includes/user/PasswordReset.php index 38707dec5b..8ef1d0d500 100644 --- a/includes/user/PasswordReset.php +++ b/includes/user/PasswordReset.php @@ -22,10 +22,14 @@ use MediaWiki\Auth\AuthManager; use MediaWiki\Auth\TemporaryPasswordAuthenticationRequest; +use MediaWiki\Config\ServiceOptions; +use MediaWiki\Logger\LoggerFactory; +use MediaWiki\MediaWikiServices; use MediaWiki\Permissions\PermissionManager; use Psr\Log\LoggerAwareInterface; +use Psr\Log\LoggerAwareTrait; use Psr\Log\LoggerInterface; -use MediaWiki\Logger\LoggerFactory; +use Wikimedia\Rdbms\ILoadBalancer; /** * Helper class for the password reset functionality shared by the web UI and the API. @@ -35,17 +39,19 @@ use MediaWiki\Logger\LoggerFactory; * functionality) to be enabled. */ class PasswordReset implements LoggerAwareInterface { - /** @var Config */ + use LoggerAwareTrait; + + /** @var ServiceOptions|Config */ protected $config; /** @var AuthManager */ protected $authManager; /** @var PermissionManager */ - private $permissionManager; + protected $permissionManager; - /** @var LoggerInterface */ - protected $logger; + /** @var ILoadBalancer */ + protected $loadBalancer; /** * In-process cache for isAllowed lookups, by username. @@ -54,26 +60,44 @@ class PasswordReset implements LoggerAwareInterface { */ private $permissionCache; + public static $constructorOptions = [ + 'EnableEmail', + 'PasswordResetRoutes', + ]; + + /** + * This class is managed by MediaWikiServices, don't instantiate directly. + * + * @param ServiceOptions|Config $config + * @param AuthManager $authManager + * @param PermissionManager $permissionManager + * @param ILoadBalancer|null $loadBalancer + * @param LoggerInterface|null $logger + */ public function __construct( - Config $config, + $config, AuthManager $authManager, - PermissionManager $permissionManager + PermissionManager $permissionManager, + ILoadBalancer $loadBalancer = null, + LoggerInterface $logger = null ) { $this->config = $config; $this->authManager = $authManager; $this->permissionManager = $permissionManager; - $this->permissionCache = new MapCacheLRU( 1 ); - $this->logger = LoggerFactory::getInstance( 'authentication' ); - } - /** - * Set the logger instance to use. - * - * @param LoggerInterface $logger - * @since 1.29 - */ - public function setLogger( LoggerInterface $logger ) { + if ( !$loadBalancer ) { + wfDeprecated( 'Not passing LoadBalancer to ' . __METHOD__, '1.34' ); + $loadBalancer = MediaWikiServices::getInstance()->getDBLoadBalancer(); + } + $this->loadBalancer = $loadBalancer; + + if ( !$logger ) { + wfDeprecated( 'Not passing LoggerInterface to ' . __METHOD__, '1.34' ); + $logger = LoggerFactory::getInstance( 'authentication' ); + } $this->logger = $logger; + + $this->permissionCache = new MapCacheLRU( 1 ); } /** @@ -280,7 +304,7 @@ class PasswordReset implements LoggerAwareInterface { */ protected function getUsersByEmail( $email ) { $userQuery = User::getQueryInfo(); - $res = wfGetDB( DB_REPLICA )->select( + $res = $this->loadBalancer->getConnectionRef( DB_REPLICA )->select( $userQuery['tables'], $userQuery['fields'], [ 'user_email' => $email ], diff --git a/tests/phpunit/includes/user/PasswordResetTest.php b/tests/phpunit/includes/user/PasswordResetTest.php index 4a7aa05b14..b5677fa99f 100644 --- a/tests/phpunit/includes/user/PasswordResetTest.php +++ b/tests/phpunit/includes/user/PasswordResetTest.php @@ -4,23 +4,32 @@ use MediaWiki\Auth\AuthManager; use MediaWiki\Block\DatabaseBlock; use MediaWiki\Block\CompositeBlock; use MediaWiki\Block\SystemBlock; +use MediaWiki\Config\ServiceOptions; use MediaWiki\Permissions\PermissionManager; +use Psr\Log\NullLogger; +use Wikimedia\Rdbms\ILoadBalancer; /** * @covers PasswordReset * @group Database */ class PasswordResetTest extends MediaWikiTestCase { + private function makeConfig( $enableEmail, array $passwordResetRoutes = [] ) { + $hash = new HashConfig( [ + 'EnableEmail' => $enableEmail, + 'PasswordResetRoutes' => $passwordResetRoutes, + ] ); + + return new ServiceOptions( PasswordReset::$constructorOptions, $hash ); + } + /** * @dataProvider provideIsAllowed */ public function testIsAllowed( $passwordResetRoutes, $enableEmail, $allowsAuthenticationDataChange, $canEditPrivate, $block, $globalBlock, $isAllowed ) { - $config = new HashConfig( [ - 'PasswordResetRoutes' => $passwordResetRoutes, - 'EnableEmail' => $enableEmail, - ] ); + $config = $this->makeConfig( $enableEmail, $passwordResetRoutes ); $authManager = $this->getMockBuilder( AuthManager::class )->disableOriginalConstructor() ->getMock(); @@ -39,10 +48,14 @@ class PasswordResetTest extends MediaWikiTestCase { ->with( $user, 'editmyprivateinfo' ) ->willReturn( $canEditPrivate ); + $loadBalancer = $this->getMockBuilder( ILoadBalancer::class )->getMock(); + $passwordReset = new PasswordReset( $config, $authManager, - $permissionManager + $permissionManager, + $loadBalancer, + new NullLogger() ); $this->assertSame( $isAllowed, $passwordReset->isAllowed( $user )->isGood() ); @@ -187,10 +200,7 @@ class PasswordResetTest extends MediaWikiTestCase { } public function testExecute_email() { - $config = new HashConfig( [ - 'PasswordResetRoutes' => [ 'username' => true, 'email' => true ], - 'EnableEmail' => true, - ] ); + $config = $this->makeConfig( true, [ 'username' => true, 'email' => true ] ); // Unregister the hooks for proper unit testing $this->mergeMwGlobalArrayValue( 'wgHooks', [ @@ -204,6 +214,14 @@ class PasswordResetTest extends MediaWikiTestCase { ->willReturn( Status::newGood() ); $authManager->expects( $this->exactly( 2 ) )->method( 'changeAuthenticationData' ); + $permissionManager = $this->getMockBuilder( PermissionManager::class ) + ->disableOriginalConstructor() + ->getMock(); + $permissionManager->method( 'userHasRight' )->willReturn( true ); + + $loadBalancer = $this->getMockBuilder( ILoadBalancer::class ) + ->getMock(); + $request = new FauxRequest(); $request->setIP( '1.2.3.4' ); $performingUser = $this->getMockBuilder( User::class )->getMock(); @@ -228,8 +246,14 @@ class PasswordResetTest extends MediaWikiTestCase { $targetUser2->expects( $this->any() )->method( 'getEmail' )->willReturn( 'foo@bar.baz' ); $passwordReset = $this->getMockBuilder( PasswordReset::class ) - ->setConstructorArgs( [ $config, $authManager, $permissionManager ] ) ->setMethods( [ 'getUsersByEmail' ] ) + ->setConstructorArgs( [ + $config, + $authManager, + $permissionManager, + $loadBalancer, + new NullLogger() + ] ) ->getMock(); $passwordReset->expects( $this->any() )->method( 'getUsersByEmail' )->with( 'foo@bar.baz' ) ->willReturn( [ $targetUser1, $targetUser2 ] ); -- 2.20.1