From cbea88f818de52385dd55ef71f3e1c3411688590 Mon Sep 17 00:00:00 2001 From: Daimona Eaytoy Date: Sun, 25 Oct 2020 16:30:43 +0100 Subject: [PATCH] Add a service to retrieve the filter user Unfortunately, this isn't using DI completely, because of the User::newSystemUser call. I'm not even sure if we really need to call it or we can just stick to new UserIdentityValue, but leaving like this for now. Also, the types were weakened to UserIdentity, so the transition is going to be easy anyway. Change-Id: I08f8fae0fcc622ff0ac3f86771476d06d1c18549 --- extension.json | 1 + includes/AbuseFilter.php | 30 --------- includes/AbuseFilterRunner.php | 10 ++- includes/AbuseFilterServices.php | 7 +++ includes/BlockAutopromoteStore.php | 11 ++-- includes/FilterUser.php | 67 +++++++++++++++++++++ includes/ServiceWiring.php | 10 +++ includes/Views/AbuseFilterViewRevert.php | 3 +- maintenance/normalizeThrottleParameters.php | 5 +- tests/phpunit/AbuseFilterFilterUserTest.php | 56 +++++++++++++++++ 10 files changed, 161 insertions(+), 39 deletions(-) create mode 100644 includes/FilterUser.php create mode 100644 tests/phpunit/AbuseFilterFilterUserTest.php diff --git a/extension.json b/extension.json index 49b89edaa..0547448d6 100644 --- a/extension.json +++ b/extension.json @@ -169,6 +169,7 @@ "MediaWiki\\Extension\\AbuseFilter\\ChangeTagger": "includes/ChangeTagger.php", "MediaWiki\\Extension\\AbuseFilter\\ChangeTagsManager": "includes/ChangeTagsManager.php", "MediaWiki\\Extension\\AbuseFilter\\BlockAutopromoteStore": "includes/BlockAutopromoteStore.php", + "MediaWiki\\Extension\\AbuseFilter\\FilterUser": "includes/FilterUser.php", "AFComputedVariable": "includes/AFComputedVariable.php", "AFPData": "includes/parser/AFPData.php", "AFPException": "includes/parser/AFPException.php", diff --git a/includes/AbuseFilter.php b/includes/AbuseFilter.php index 4673034d2..c9520ca77 100644 --- a/includes/AbuseFilter.php +++ b/includes/AbuseFilter.php @@ -566,36 +566,6 @@ class AbuseFilter { return $value[$group] ?? $value['default']; } - /** - * @return User - */ - public static function getFilterUser() : User { - $username = wfMessage( 'abusefilter-blocker' )->inContentLanguage()->text(); - $user = User::newSystemUser( $username, [ 'steal' => true ] ); - - if ( !$user ) { - // User name is invalid. Don't throw because this is a system message, easy - // to change and make wrong either by mistake or intentionally to break the site. - wfWarn( - 'The AbuseFilter user\'s name is invalid. Please change it in ' . - 'MediaWiki:abusefilter-blocker' - ); - // Use the default name to avoid breaking other stuff. This should have no harm, - // aside from blocks temporarily attributed to another user. - $defaultName = wfMessage( 'abusefilter-blocker' )->inLanguage( 'en' )->text(); - $user = User::newSystemUser( $defaultName, [ 'steal' => true ] ); - } - '@phan-var User $user'; - - // Promote user to 'sysop' so it doesn't look - // like an unprivileged account is blocking users - if ( !in_array( 'sysop', $user->getGroups() ) ) { - $user->addGroup( 'sysop' ); - } - - return $user; - } - /** * Check whether a filter is allowed to use a tag * diff --git a/includes/AbuseFilterRunner.php b/includes/AbuseFilterRunner.php index 0a7970d09..d58fe0357 100644 --- a/includes/AbuseFilterRunner.php +++ b/includes/AbuseFilterRunner.php @@ -8,6 +8,7 @@ use MediaWiki\Extension\AbuseFilter\VariableGenerator\VariableGenerator; use MediaWiki\Logger\LoggerFactory; use MediaWiki\MediaWikiServices; use MediaWiki\Session\SessionManager; +use MediaWiki\User\UserIdentity; use Wikimedia\IPUtils; use Wikimedia\Rdbms\IDatabase; @@ -74,6 +75,9 @@ class AbuseFilterRunner { /** @var ChangeTagger */ private $changeTagger; + /** @var UserIdentity */ + private $filterUser; + /** * @param User $user The user who performed the action being filtered * @param Title $title The title where the action being filtered was performed @@ -99,6 +103,7 @@ class AbuseFilterRunner { $this->hookRunner = AbuseFilterHookRunner::getRunner(); $this->filterProfiler = AbuseFilterServices::getFilterProfiler(); $this->changeTagger = AbuseFilterServices::getChangeTagger(); + $this->filterUser = AbuseFilterServices::getFilterUser()->getUser(); } /** @@ -811,7 +816,7 @@ class AbuseFilterRunner { // TODO Core should provide a logging method $logEntry = new ManualLogEntry( 'rights', 'rights' ); - $logEntry->setPerformer( AbuseFilter::getFilterUser() ); + $logEntry->setPerformer( $this->filterUser ); $logEntry->setTarget( $this->user->getUserPage() ); $logEntry->setComment( wfMessage( @@ -935,7 +940,6 @@ class AbuseFilterRunner { $preventEditOwnUserTalk ) { $blockUserFactory = MediaWikiServices::getInstance()->getBlockUserFactory(); - $filterUser = AbuseFilter::getFilterUser(); $reason = wfMessage( 'abusefilter-blockreason', $ruleDesc, $ruleNumber @@ -943,7 +947,7 @@ class AbuseFilterRunner { $blockUserFactory->newBlockUser( $target, - $filterUser, + User::newFromIdentity( $this->filterUser ), $expiry, $reason, [ diff --git a/includes/AbuseFilterServices.php b/includes/AbuseFilterServices.php index 0684f28f0..3b51b7508 100644 --- a/includes/AbuseFilterServices.php +++ b/includes/AbuseFilterServices.php @@ -48,4 +48,11 @@ class AbuseFilterServices { public static function getBlockAutopromoteStore() : BlockAutopromoteStore { return MediaWikiServices::getInstance()->getService( BlockAutopromoteStore::SERVICE_NAME ); } + + /** + * @return FilterUser + */ + public static function getFilterUser() : FilterUser { + return MediaWikiServices::getInstance()->getService( FilterUser::SERVICE_NAME ); + } } diff --git a/includes/BlockAutopromoteStore.php b/includes/BlockAutopromoteStore.php index 797fee976..3396c30ae 100644 --- a/includes/BlockAutopromoteStore.php +++ b/includes/BlockAutopromoteStore.php @@ -2,7 +2,6 @@ namespace MediaWiki\Extension\AbuseFilter; -use AbuseFilter; use BagOStuff; use ManualLogEntry; use MediaWiki\User\UserIdentity; @@ -26,13 +25,18 @@ class BlockAutopromoteStore { */ private $logger; + /** @var FilterUser */ + private $filterUser; + /** * @param BagOStuff $store * @param LoggerInterface $logger + * @param FilterUser $filterUser */ - public function __construct( BagOStuff $store, LoggerInterface $logger ) { + public function __construct( BagOStuff $store, LoggerInterface $logger, FilterUser $filterUser ) { $this->store = $store; $this->logger = $logger; + $this->filterUser = $filterUser; } /** @@ -71,8 +75,7 @@ class BlockAutopromoteStore { } $logEntry = new ManualLogEntry( 'rights', 'blockautopromote' ); - $performer = AbuseFilter::getFilterUser(); - $logEntry->setPerformer( $performer ); + $logEntry->setPerformer( $this->filterUser->getUser() ); $logEntry->setTarget( new TitleValue( NS_USER, $target->getName() ) ); $logEntry->setParameters( [ diff --git a/includes/FilterUser.php b/includes/FilterUser.php new file mode 100644 index 000000000..031b770ce --- /dev/null +++ b/includes/FilterUser.php @@ -0,0 +1,67 @@ +messageLocalizer = $messageLocalizer; + $this->userGroupManager = $userGroupManager; + $this->logger = $logger; + } + + /** + * @todo Core should provide an alternative way to create system users, possibly returning just UserIdentity. + * Or can we just use `new UserIdentityValue` here? + * @return UserIdentity + */ + public function getUser() : UserIdentity { + $username = $this->messageLocalizer->msg( 'abusefilter-blocker' )->inContentLanguage()->text(); + $user = User::newSystemUser( $username, [ 'steal' => true ] ); + + if ( !$user ) { + // User name is invalid. Don't throw because this is a system message, easy + // to change and make wrong either by mistake or intentionally to break the site. + $this->logger->warning( + 'The AbuseFilter user\'s name is invalid. Please change it in ' . + 'MediaWiki:abusefilter-blocker' + ); + // Use the default name to avoid breaking other stuff. This should have no harm, + // aside from blocks temporarily attributed to another user. + $defaultName = $this->messageLocalizer->msg( 'abusefilter-blocker' )->inLanguage( 'en' )->text(); + $user = User::newSystemUser( $defaultName, [ 'steal' => true ] ); + } + '@phan-var User $user'; + + // Promote user to 'sysop' so it doesn't look + // like an unprivileged account is blocking users + if ( !in_array( 'sysop', $this->userGroupManager->getUserGroups( $user ) ) ) { + $this->userGroupManager->addUserToGroup( $user, 'sysop' ); + } + + return $user; + } +} diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index da76adabf..fadb9cbe7 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -6,6 +6,7 @@ use MediaWiki\Extension\AbuseFilter\BlockAutopromoteStore; use MediaWiki\Extension\AbuseFilter\ChangeTagger; use MediaWiki\Extension\AbuseFilter\ChangeTagsManager; use MediaWiki\Extension\AbuseFilter\FilterProfiler; +use MediaWiki\Extension\AbuseFilter\FilterUser; use MediaWiki\Extension\AbuseFilter\Hooks\AbuseFilterHookRunner; use MediaWiki\Extension\AbuseFilter\KeywordsManager; use MediaWiki\Logger\LoggerFactory; @@ -50,6 +51,15 @@ return [ BlockAutopromoteStore::SERVICE_NAME => function ( MediaWikiServices $services ): BlockAutopromoteStore { return new BlockAutopromoteStore( ObjectCache::getInstance( 'db-replicated' ), + LoggerFactory::getInstance( 'AbuseFilter' ), + $services->get( FilterUser::SERVICE_NAME ) + ); + }, + FilterUser::SERVICE_NAME => function ( MediaWikiServices $services ): FilterUser { + return new FilterUser( + // TODO We need a proper MessageLocalizer, see T247127 + RequestContext::getMain(), + $services->getUserGroupManager(), LoggerFactory::getInstance( 'AbuseFilter' ) ); }, diff --git a/includes/Views/AbuseFilterViewRevert.php b/includes/Views/AbuseFilterViewRevert.php index c7943b315..bc3c07eec 100644 --- a/includes/Views/AbuseFilterViewRevert.php +++ b/includes/Views/AbuseFilterViewRevert.php @@ -292,7 +292,8 @@ class AbuseFilterViewRevert extends AbuseFilterView { switch ( $action ) { case 'block': $block = DatabaseBlock::newFromTarget( $result['user'] ); - if ( !( $block && $block->getBy() === AbuseFilter::getFilterUser()->getId() ) ) { + $filterUser = AbuseFilterServices::getFilterUser()->getUser(); + if ( !( $block && $block->getBy() === $filterUser->getId() ) ) { // Not blocked by abuse filter return false; } diff --git a/maintenance/normalizeThrottleParameters.php b/maintenance/normalizeThrottleParameters.php index 8fde50376..5e3af7886 100644 --- a/maintenance/normalizeThrottleParameters.php +++ b/maintenance/normalizeThrottleParameters.php @@ -18,6 +18,9 @@ * * @ingroup Maintenance */ + +use MediaWiki\Extension\AbuseFilter\AbuseFilterServices; + if ( getenv( 'MW_INSTALL_PATH' ) ) { $IP = getenv( 'MW_INSTALL_PATH' ); } else { @@ -144,7 +147,7 @@ class NormalizeThrottleParameters extends LoggedUpdateMaintenance { * @return int Amount of normalized rows */ protected function normalizeParameters() { - $user = AbuseFilter::getFilterUser(); + $user = AbuseFilterServices::getFilterUser()->getUser(); $dryRun = $this->hasOption( 'dry-run' ); // IDs of filters with invalid rate (count or period) diff --git a/tests/phpunit/AbuseFilterFilterUserTest.php b/tests/phpunit/AbuseFilterFilterUserTest.php new file mode 100644 index 000000000..c1a7a7837 --- /dev/null +++ b/tests/phpunit/AbuseFilterFilterUserTest.php @@ -0,0 +1,56 @@ +createMock( MessageLocalizer::class ); + $ml->method( 'msg' )->willReturn( $this->getMockMessage( $name ) ); + $ugm = MediaWikiServices::getInstance()->getUserGroupManager(); + $filterUser = new FilterUser( $ml, $ugm, new NullLogger() ); + + $actual = $filterUser->getUser(); + $this->assertSame( $name, $actual->getName(), 'name' ); + $this->assertContains( 'sysop', $ugm->getUserGroups( $actual ), 'sysop' ); + } + + /** + * @covers ::getUser + */ + public function testGetUser_invalidName() { + $name = 'Foobar filter user'; + $ml = $this->createMock( MessageLocalizer::class ); + $msg = $this->createMock( Message::class ); + $msg->method( 'inContentLanguage' )->willReturn( $this->getMockMessage( '' ) ); + $msg->method( 'inLanguage' )->willReturn( $this->getMockMessage( $name ) ); + $ml->method( 'msg' )->willReturn( $msg ); + $ugm = MediaWikiServices::getInstance()->getUserGroupManager(); + $logger = new TestLogger(); + $logger->setCollect( true ); + $filterUser = new FilterUser( $ml, $ugm, $logger ); + + $actual = $filterUser->getUser(); + $this->assertSame( $name, $actual->getName(), 'name' ); + $found = false; + foreach ( $logger->getBuffer() as $msg ) { + if ( strpos( $msg[1], 'MediaWiki:abusefilter-blocker' ) !== false ) { + $found = true; + break; + } + } + $this->assertTrue( $found, 'Invalid name not logged' ); + } +}