From 3f7dd25fbfa503d6bf86916bb94586af22d84059 Mon Sep 17 00:00:00 2001 From: Daimona Eaytoy Date: Fri, 27 Nov 2020 15:49:41 +0100 Subject: [PATCH] Create FilterRunnerFactory Next step is splitting the Runner into various subclasses. Change-Id: I766555f31b425cee52fd262c5bfb1c73f3f170d2 --- extension.json | 3 +- includes/AbuseFilter.php | 11 +- includes/AbuseFilterHooks.php | 15 +- .../AbuseFilterPreAuthenticationProvider.php | 4 +- includes/AbuseFilterServices.php | 7 + ...AbuseFilterRunner.php => FilterRunner.php} | 175 ++++++++++-------- includes/FilterRunnerFactory.php | 119 ++++++++++++ includes/ServiceWiring.php | 18 ++ tests/phpunit/AbuseFilterConsequencesTest.php | 8 +- 9 files changed, 268 insertions(+), 92 deletions(-) rename includes/{AbuseFilterRunner.php => FilterRunner.php} (80%) create mode 100644 includes/FilterRunnerFactory.php diff --git a/extension.json b/extension.json index cbe9f3f85..bd7157341 100644 --- a/extension.json +++ b/extension.json @@ -152,7 +152,6 @@ "AbuseFilter": "includes/AbuseFilter.php", "AbuseFilterHooks": "includes/AbuseFilterHooks.php", "AbuseFilterPreAuthenticationProvider": "includes/AbuseFilterPreAuthenticationProvider.php", - "AbuseFilterRunner": "includes/AbuseFilterRunner.php", "AbuseFilterSpecialPage": "includes/special/AbuseFilterSpecialPage.php", "SpecialAbuseLog": "includes/special/SpecialAbuseLog.php", "SpecialAbuseFilter": "includes/special/SpecialAbuseFilter.php", @@ -184,6 +183,8 @@ "MediaWiki\\Extension\\AbuseFilter\\ConsequencesRegistry": "includes/ConsequencesRegistry.php", "MediaWiki\\Extension\\AbuseFilter\\ConsequencesExecutor": "includes/ConsequencesExecutor.php", "MediaWiki\\Extension\\AbuseFilter\\ConsequencesExecutorFactory": "includes/ConsequencesExecutorFactory.php", + "MediaWiki\\Extension\\AbuseFilter\\FilterRunner": "includes/FilterRunner.php", + "MediaWiki\\Extension\\AbuseFilter\\FilterRunnerFactory": "includes/FilterRunnerFactory.php", "AFComputedVariable": "includes/AFComputedVariable.php", "NormalizeThrottleParameters": "maintenance/normalizeThrottleParameters.php", "FixOldLogEntries": "maintenance/fixOldLogEntries.php", diff --git a/includes/AbuseFilter.php b/includes/AbuseFilter.php index 4804a54e0..2d3e7e03a 100644 --- a/includes/AbuseFilter.php +++ b/includes/AbuseFilter.php @@ -36,7 +36,7 @@ class AbuseFilter { * @param string $group The filter's group (as defined in $wgAbuseFilterValidGroups) * @param string $mode 'execute' for edits and logs, 'stash' for cached matches * @return bool[] Map of (integer filter ID => bool) - * @deprecated Since 1.34 See comment on AbuseFilterRunner::checkAllFilters + * @deprecated Since 1.34 See comment on FilterRunner::checkAllFilters */ public static function checkAllFilters( AbuseFilterVariableHolder $vars, @@ -46,8 +46,8 @@ class AbuseFilter { ) { $parser = AbuseFilterServices::getParserFactory()->newParser( $vars ); $user = RequestContext::getMain()->getUser(); - - $runner = new AbuseFilterRunner( $user, $title, $vars, $group ); + $runnerFactory = AbuseFilterServices::getFilterRunnerFactory(); + $runner = $runnerFactory->newRunner( $user, $title, $vars, $group ); $runner->parser = $parser; return $runner->checkAllFilters(); } @@ -58,12 +58,13 @@ class AbuseFilter { * @param string $group The filter's group (as defined in $wgAbuseFilterValidGroups) * @param User $user The user performing the action * @return Status - * @deprecated Since 1.34 Build an AbuseFilterRunner instance and call run() on that. + * @deprecated Since 1.34 Build a FilterRunner instance and call run() on that. */ public static function filterAction( AbuseFilterVariableHolder $vars, Title $title, $group, User $user ) { - $runner = new AbuseFilterRunner( $user, $title, $vars, $group ); + $runnerFactory = AbuseFilterServices::getFilterRunnerFactory(); + $runner = $runnerFactory->newRunner( $user, $title, $vars, $group ); return $runner->run(); } diff --git a/includes/AbuseFilterHooks.php b/includes/AbuseFilterHooks.php index c6d161d4c..fe4eebe6e 100644 --- a/includes/AbuseFilterHooks.php +++ b/includes/AbuseFilterHooks.php @@ -211,7 +211,8 @@ class AbuseFilterHooks { // We don't have to filter the edit return Status::newGood(); } - $runner = new AbuseFilterRunner( $user, $title, $vars, 'default' ); + $runnerFactory = AbuseFilterServices::getFilterRunnerFactory(); + $runner = $runnerFactory->newRunner( $user, $title, $vars, 'default' ); $filterResult = $runner->run(); if ( !$filterResult->isOK() ) { return $filterResult; @@ -357,7 +358,8 @@ class AbuseFilterHooks { $vars = new AbuseFilterVariableHolder(); $builder = new RunVariableGenerator( $vars, $user, $oldTitle ); $vars = $builder->getMoveVars( $newTitle, $reason ); - $runner = new AbuseFilterRunner( $user, $oldTitle, $vars, 'default' ); + $runnerFactory = AbuseFilterServices::getFilterRunnerFactory(); + $runner = $runnerFactory->newRunner( $user, $oldTitle, $vars, 'default' ); $result = $runner->run(); $status->merge( $result ); } @@ -375,7 +377,8 @@ class AbuseFilterHooks { $vars = new AbuseFilterVariableHolder(); $builder = new RunVariableGenerator( $vars, $user, $article->getTitle() ); $vars = $builder->getDeleteVars( $reason ); - $runner = new AbuseFilterRunner( $user, $article->getTitle(), $vars, 'default' ); + $runnerFactory = AbuseFilterServices::getFilterRunnerFactory(); + $runner = $runnerFactory->newRunner( $user, $article->getTitle(), $vars, 'default' ); $filterResult = $runner->run(); $status->merge( $filterResult ); @@ -459,7 +462,8 @@ class AbuseFilterHooks { if ( $vars === null ) { return true; } - $runner = new AbuseFilterRunner( $user, $title, $vars, 'default' ); + $runnerFactory = AbuseFilterServices::getFilterRunnerFactory(); + $runner = $runnerFactory->newRunner( $user, $title, $vars, 'default' ); $filterResult = $runner->run(); if ( !$filterResult->isOK() ) { @@ -536,7 +540,8 @@ class AbuseFilterHooks { if ( !$vars ) { return; } - $runner = new AbuseFilterRunner( $user, $page->getTitle(), $vars, 'default' ); + $runnerFactory = AbuseFilterServices::getFilterRunnerFactory(); + $runner = $runnerFactory->newRunner( $user, $page->getTitle(), $vars, 'default' ); $runner->runForStash(); $totalTime = microtime( true ) - $startTime; MediaWikiServices::getInstance()->getStatsdDataFactory() diff --git a/includes/AbuseFilterPreAuthenticationProvider.php b/includes/AbuseFilterPreAuthenticationProvider.php index 343f5478a..0fe243403 100644 --- a/includes/AbuseFilterPreAuthenticationProvider.php +++ b/includes/AbuseFilterPreAuthenticationProvider.php @@ -2,6 +2,7 @@ use MediaWiki\Auth\AbstractPreAuthenticationProvider; use MediaWiki\Auth\AuthenticationRequest; +use MediaWiki\Extension\AbuseFilter\AbuseFilterServices; use MediaWiki\Extension\AbuseFilter\VariableGenerator\RunVariableGenerator; use MediaWiki\MediaWikiServices; @@ -47,8 +48,9 @@ class AbuseFilterPreAuthenticationProvider extends AbstractPreAuthenticationProv $builder = new RunVariableGenerator( $vars, $creator, $title ); $vars = $builder->getAccountCreationVars( $user, $autocreate ); + $runnerFactory = AbuseFilterServices::getFilterRunnerFactory(); // pass creator in explicitly to prevent recording the current user on autocreation - T135360 - $runner = new AbuseFilterRunner( $creator, $title, $vars, 'default' ); + $runner = $runnerFactory->newRunner( $creator, $title, $vars, 'default' ); $status = $runner->run(); MediaWikiServices::getInstance()->getStatsdDataFactory() diff --git a/includes/AbuseFilterServices.php b/includes/AbuseFilterServices.php index da97252b0..55df5afc6 100644 --- a/includes/AbuseFilterServices.php +++ b/includes/AbuseFilterServices.php @@ -179,4 +179,11 @@ class AbuseFilterServices { public static function getConsequencesExecutorFactory() : ConsequencesExecutorFactory { return MediaWikiServices::getInstance()->getService( ConsequencesExecutorFactory::SERVICE_NAME ); } + + /** + * @return FilterRunnerFactory + */ + public static function getFilterRunnerFactory() : FilterRunnerFactory { + return MediaWikiServices::getInstance()->getService( FilterRunnerFactory::SERVICE_NAME ); + } } diff --git a/includes/AbuseFilterRunner.php b/includes/FilterRunner.php similarity index 80% rename from includes/AbuseFilterRunner.php rename to includes/FilterRunner.php index 4ca0293d6..db2474173 100644 --- a/includes/AbuseFilterRunner.php +++ b/includes/FilterRunner.php @@ -1,23 +1,61 @@ hookRunner = $hookRunner; + $this->filterProfiler = $filterProfiler; + $this->changeTagger = $changeTagger; + $this->filterLookup = $filterLookup; + $this->parserFactory = $parserFactory; + $this->consExecutorFactory = $consExecutorFactory; + $this->abuseLoggerFactory = $abuseLoggerFactory; + $this->watchers = $watchers; + $this->logger = $logger; + $this->statsdDataFactory = $statsdDataFactory; - /** @var AbuseFilterHookRunner */ - private $hookRunner; - - /** @var FilterProfiler */ - private $filterProfiler; - - /** @var ChangeTagger */ - private $changeTagger; - - /** @var FilterLookup */ - private $filterLookup; - - /** @var Watcher[] */ - private $watchers; - - /** - * @param User $user The user who performed the action being filtered - * @param Title $title The title where the action being filtered was performed - * @param AbuseFilterVariableHolder $vars The variables for the current action - * @param string $group The group of filters to check. It must be defined as so in - * $wgAbuseFilterValidGroups, or this will throw. - * @throws InvalidArgumentException - */ - public function __construct( User $user, Title $title, AbuseFilterVariableHolder $vars, $group ) { - global $wgAbuseFilterValidGroups; - if ( !in_array( $group, $wgAbuseFilterValidGroups ) ) { - throw new InvalidArgumentException( '$group must be defined in $wgAbuseFilterValidGroups' ); + if ( !in_array( $group, $validFilterGroups, true ) ) { + throw new InvalidArgumentException( "Group $group is not a valid group" ); } if ( !$vars->varIsSet( 'action' ) ) { throw new InvalidArgumentException( "The 'action' variable is not set." ); @@ -97,18 +144,9 @@ class AbuseFilterRunner { $this->user = $user; $this->title = $title; $this->vars = $vars; - $this->vars->setLogger( LoggerFactory::getInstance( 'AbuseFilter' ) ); + $this->vars->setLogger( $logger ); $this->group = $group; $this->action = $vars->getVar( 'action' )->toString(); - $this->hookRunner = AbuseFilterHookRunner::getRunner(); - $this->filterProfiler = AbuseFilterServices::getFilterProfiler(); - $this->changeTagger = AbuseFilterServices::getChangeTagger(); - $this->filterLookup = AbuseFilterServices::getFilterLookup(); - // TODO Inject, add a hook for custom watchers - $this->watchers = [ - AbuseFilterServices::getUpdateHitCountWatcher(), - AbuseFilterServices::getEmergencyWatcher() - ]; } /** @@ -130,19 +168,11 @@ class AbuseFilterRunner { $this->vars->forFilter = true; $this->vars->setVar( 'timestamp', (int)wfTimestamp( TS_UNIX ) ); - $this->parser = $this->getParser(); - $this->parser->setStatsd( MediaWikiServices::getInstance()->getStatsdDataFactory() ); + $this->parser = $this->parserFactory->newParser( $this->vars ); + $this->parser->setStatsd( $this->statsdDataFactory ); $this->profilingData = []; } - /** - * Shortcut method, so that it can be overridden in mocks. - * @return AbuseFilterParser - */ - protected function getParser() : AbuseFilterParser { - return AbuseFilterServices::getParserFactory()->newParser( $this->vars ); - } - /** * The main entry point of this class. This method runs all filters and takes their consequences. * @@ -151,11 +181,7 @@ class AbuseFilterRunner { * @throws BadMethodCallException If run() was already called on this instance * @return Status Good if no action has been taken, a fatal otherwise. */ - public function run( $allowStash = true ) : Status { - if ( $this->executed ) { - throw new BadMethodCallException( 'run() was already called on this instance.' ); - } - $this->executed = true; + public function run( $allowStash = true ): Status { $this->init(); $skipReasons = []; @@ -163,8 +189,7 @@ class AbuseFilterRunner { $this->vars, $this->title, $this->user, $skipReasons ); if ( !$shouldFilter ) { - $logger = LoggerFactory::getInstance( 'AbuseFilter' ); - $logger->info( + $this->logger->info( 'Skipping action {action}. Reasons provided: {reasons}', [ 'action' => $this->action, 'reasons' => implode( ', ', $skipReasons ) ] ); @@ -223,7 +248,7 @@ class AbuseFilterRunner { return Status::newGood(); } - $executor = AbuseFilterServices::getConsequencesExecutorFactory()->newExecutor( + $executor = $this->consExecutorFactory->newExecutor( $this->user, $this->title, $this->vars @@ -231,11 +256,9 @@ class AbuseFilterRunner { $status = $executor->executeFilterActions( $matchedFilters ); $actionsTaken = $status->getValue(); - $abuseLogger = AbuseFilterServices::getAbuseLoggerFactory()->newLogger( - $this->title, - $this->user, - $this->vars - ); + // Note, it's important that we create an AbuseLogger now, after all lazy-loaded variables + // requested by active filters have been computed + $abuseLogger = $this->abuseLoggerFactory->newLogger( $this->title, $this->user, $this->vars ); [ 'local' => $loggedLocalFilters, 'global' => $loggedGlobalFilters @@ -362,7 +385,7 @@ class AbuseFilterRunner { // Bots do not use edit stashing, so avoid distorting the stats $statsd = $this->user->isBot() ? new NullStatsdDataFactory() - : MediaWikiServices::getInstance()->getStatsdDataFactory(); + : $this->statsdDataFactory; $logger->debug( __METHOD__ . ": cache $type for '{$this->title}' (key $key)." ); $statsd->increment( "abusefilter.check-stash.$type" ); diff --git a/includes/FilterRunnerFactory.php b/includes/FilterRunnerFactory.php new file mode 100644 index 000000000..57ab7c847 --- /dev/null +++ b/includes/FilterRunnerFactory.php @@ -0,0 +1,119 @@ +hookRunner = $hookRunner; + $this->filterProfiler = $filterProfiler; + $this->changeTagger = $changeTagger; + $this->filterLookup = $filterLookup; + $this->parserFactory = $parserFactory; + $this->consExecutorFactory = $consExecutorFactory; + $this->abuseLoggerFactory = $abuseLoggerFactory; + $this->updateHitCountWatcher = $updateHitCountWatcher; + $this->emergencyWatcher = $emergencyWatcher; + $this->logger = $logger; + $this->statsdDataFactory = $statsdDataFactory; + $this->validGroups = $validFilterGroups; + } + + /** + * @param User $user + * @param Title $title + * @param AbuseFilterVariableHolder $vars + * @param string $group + * @return FilterRunner + */ + public function newRunner( + User $user, + Title $title, + AbuseFilterVariableHolder $vars, + string $group + ) : FilterRunner { + // TODO Add a method to this class taking these as params? Add a hook for custom watchers + $watchers = [ $this->updateHitCountWatcher, $this->emergencyWatcher ]; + return new FilterRunner( + $this->hookRunner, + $this->filterProfiler, + $this->changeTagger, + $this->filterLookup, + $this->parserFactory, + $this->consExecutorFactory, + $this->abuseLoggerFactory, + $watchers, + $this->logger, + $this->statsdDataFactory, + $this->validGroups, + $user, + $title, + $vars, + $group + ); + } +} diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index 089477bb4..fa6f84e72 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -10,6 +10,7 @@ use MediaWiki\Extension\AbuseFilter\ChangeTags\ChangeTagger; use MediaWiki\Extension\AbuseFilter\ChangeTags\ChangeTagsManager; use MediaWiki\Extension\AbuseFilter\ChangeTags\ChangeTagValidator; use MediaWiki\Extension\AbuseFilter\ConsequencesExecutor; +use MediaWiki\Extension\AbuseFilter\ConsequencesExecutorFactory; use MediaWiki\Extension\AbuseFilter\ConsequencesExecutorFactory as ConsExecutorFactory; use MediaWiki\Extension\AbuseFilter\ConsequencesFactory; use MediaWiki\Extension\AbuseFilter\ConsequencesLookup; @@ -19,6 +20,7 @@ use MediaWiki\Extension\AbuseFilter\FilterCompare; use MediaWiki\Extension\AbuseFilter\FilterImporter; use MediaWiki\Extension\AbuseFilter\FilterLookup; use MediaWiki\Extension\AbuseFilter\FilterProfiler; +use MediaWiki\Extension\AbuseFilter\FilterRunnerFactory; use MediaWiki\Extension\AbuseFilter\FilterStore; use MediaWiki\Extension\AbuseFilter\FilterUser; use MediaWiki\Extension\AbuseFilter\FilterValidator; @@ -237,6 +239,22 @@ return [ ) ); }, + FilterRunnerFactory::SERVICE_NAME => function ( MediaWikiServices $services ) : FilterRunnerFactory { + return new FilterRunnerFactory( + AbuseFilterHookRunner::getRunner(), + $services->get( FilterProfiler::SERVICE_NAME ), + $services->get( ChangeTagger::SERVICE_NAME ), + $services->get( FilterLookup::SERVICE_NAME ), + $services->get( ParserFactory::SERVICE_NAME ), + $services->get( ConsequencesExecutorFactory::SERVICE_NAME ), + $services->get( AbuseLoggerFactory::SERVICE_NAME ), + $services->get( UpdateHitCountWatcher::SERVICE_NAME ), + $services->get( EmergencyWatcher::SERVICE_NAME ), + LoggerFactory::getInstance( 'AbuseFilter' ), + $services->getStatsdDataFactory(), + $services->getMainConfig()->get( 'AbuseFilterValidGroups' ) + ); + }, ]; // @codeCoverageIgnoreEnd diff --git a/tests/phpunit/AbuseFilterConsequencesTest.php b/tests/phpunit/AbuseFilterConsequencesTest.php index 3beb2379a..6667f4dfd 100644 --- a/tests/phpunit/AbuseFilterConsequencesTest.php +++ b/tests/phpunit/AbuseFilterConsequencesTest.php @@ -39,7 +39,7 @@ use PHPUnit\Framework\MockObject\MockObject; * @group Large * * @covers AbuseFilter - * @covers AbuseFilterRunner + * @covers \MediaWiki\Extension\AbuseFilter\FilterRunner * @covers AbuseFilterHooks * @covers \MediaWiki\Extension\AbuseFilter\VariableGenerator\VariableGenerator * @covers \MediaWiki\Extension\AbuseFilter\VariableGenerator\RunVariableGenerator @@ -1020,7 +1020,7 @@ class AbuseFilterConsequencesTest extends MediaWikiTestCase { * @param array $actionParams Details of the action we need to execute to trigger filters * @covers \MediaWiki\Extension\AbuseFilter\Parser\AbuseFilterParser::getCondCount * @covers \MediaWiki\Extension\AbuseFilter\Parser\AbuseFilterParser::raiseCondCount - * @covers AbuseFilterRunner::checkAllFilters + * @covers \MediaWiki\Extension\AbuseFilter\FilterRunner::checkAllFilters * @dataProvider provideFiltersNoConsequences */ public function testCondsLimit( $createIds, $actionParams ) { @@ -1591,10 +1591,10 @@ class AbuseFilterConsequencesTest extends MediaWikiTestCase { // the given namespace has been localized and thus wouldn't match. $pageName = Title::newFromText( $actionParams['target'] )->getPrefixedText(); foreach ( $loggerMock->getBuffer() as $entry ) { - if ( preg_match( "/AbuseFilterRunner::logCache: cache $type for '$pageName'/", $entry[1] ) ) { + if ( preg_match( "/FilterRunner::logCache: cache $type for '$pageName'/", $entry[1] ) ) { $foundHitOrMiss = true; } - if ( preg_match( "/AbuseFilterRunner::logCache: cache store for '$pageName'/", $entry[1] ) ) { + if ( preg_match( "/FilterRunner::logCache: cache store for '$pageName'/", $entry[1] ) ) { $foundStore = true; } if ( $foundStore && $foundHitOrMiss ) {