diff --git a/includes/AbuseFilter.php b/includes/AbuseFilter.php index cad362fb9..14355e7c4 100644 --- a/includes/AbuseFilter.php +++ b/includes/AbuseFilter.php @@ -15,12 +15,6 @@ use User; */ class AbuseFilter { - /** - * @var array IDs of logged filters like [ page title => [ 'local' => [ids], 'global' => [ids] ] ]. - * @fixme avoid global state - */ - public static $logIds = []; - /** * @deprecated * @todo Phase out diff --git a/includes/AbuseFilterHooks.php b/includes/AbuseFilterHooks.php index 48e2b16d5..10d4ddfad 100644 --- a/includes/AbuseFilterHooks.php +++ b/includes/AbuseFilterHooks.php @@ -21,14 +21,10 @@ use Status; use Title; use UploadBase; use User; -use WikiMap; use WikiPage; class AbuseFilterHooks { - /** @var WikiPage|null Make sure edit filter & edit save hooks match */ - private static $lastEditPage = null; - /** * Called right after configuration has been loaded. * @codeCoverageIgnore Mainly deprecation warnings and other things that can be tested by running the updater @@ -200,7 +196,8 @@ class AbuseFilterHooks { Content $content, $summary, $slot = SlotRecord::MAIN ) : Status { - self::$lastEditPage = null; + $revUpdater = AbuseFilterServices::getEditRevUpdater(); + $revUpdater->clearLastEditPage(); // @todo is there any real point in passing this in? $text = AbuseFilterServices::getTextExtractor()->contentToString( $content ); @@ -234,7 +231,7 @@ class AbuseFilterHooks { return $filterResult; } - self::$lastEditPage = $page; + $revUpdater->setLastEditPage( $page ); return Status::newGood(); } @@ -281,49 +278,7 @@ class AbuseFilterHooks { int $flags, RevisionRecord $revisionRecord ) { - $curTitle = $wikiPage->getTitle()->getPrefixedText(); - if ( !isset( AbuseFilter::$logIds[ $curTitle ] ) || - $wikiPage !== self::$lastEditPage - ) { - // This isn't the edit AbuseFilter::$logIds was set for - AbuseFilter::$logIds = []; - return; - } - - // Ignore null edit. - $parentRevId = $revisionRecord->getParentId(); - if ( $parentRevId !== null ) { - $parentRev = MediaWikiServices::getInstance() - ->getRevisionLookup() - ->getRevisionById( $parentRevId ); - if ( $parentRev && $revisionRecord->hasSameContent( $parentRev ) ) { - AbuseFilter::$logIds = []; - return; - } - } - - self::$lastEditPage = null; - - $logs = AbuseFilter::$logIds[ $curTitle ]; - if ( $logs[ 'local' ] ) { - // Now actually do our storage - $dbw = wfGetDB( DB_MASTER ); - - $dbw->update( 'abuse_filter_log', - [ 'afl_rev_id' => $revisionRecord->getId() ], - [ 'afl_id' => $logs['local'] ], - __METHOD__ - ); - } - - if ( $logs[ 'global' ] ) { - $fdb = AbuseFilterServices::getCentralDBManager()->getConnection( DB_MASTER ); - $fdb->update( 'abuse_filter_log', - [ 'afl_rev_id' => $revisionRecord->getId() ], - [ 'afl_id' => $logs['global'], 'afl_wiki' => WikiMap::getCurrentWikiDbDomain()->getId() ], - __METHOD__ - ); - } + AbuseFilterServices::getEditRevUpdater()->updateRev( $wikiPage, $revisionRecord ); } /** diff --git a/includes/AbuseFilterServices.php b/includes/AbuseFilterServices.php index f643f5a80..14f65efc2 100644 --- a/includes/AbuseFilterServices.php +++ b/includes/AbuseFilterServices.php @@ -244,4 +244,11 @@ class AbuseFilterServices { public static function getVariableGeneratorFactory() : VariableGeneratorFactory { return MediaWikiServices::getInstance()->getService( VariableGeneratorFactory::SERVICE_NAME ); } + + /** + * @return EditRevUpdater + */ + public static function getEditRevUpdater() : EditRevUpdater { + return MediaWikiServices::getInstance()->getService( EditRevUpdater::SERVICE_NAME ); + } } diff --git a/includes/AbuseLogger.php b/includes/AbuseLogger.php index 44cda1904..68075962d 100644 --- a/includes/AbuseLogger.php +++ b/includes/AbuseLogger.php @@ -40,6 +40,8 @@ class AbuseLogger { private $varBlobStore; /** @var VariablesManager */ private $varManager; + /** @var EditRevUpdater */ + private $editRevUpdater; /** @var ILoadBalancer */ private $loadBalancer; /** @var ServiceOptions */ @@ -54,6 +56,7 @@ class AbuseLogger { * @param FilterLookup $filterLookup * @param VariablesBlobStore $varBlobStore * @param VariablesManager $varManager + * @param EditRevUpdater $editRevUpdater * @param ILoadBalancer $loadBalancer * @param ServiceOptions $options * @param string $wikiID @@ -67,6 +70,7 @@ class AbuseLogger { FilterLookup $filterLookup, VariablesBlobStore $varBlobStore, VariablesManager $varManager, + EditRevUpdater $editRevUpdater, ILoadBalancer $loadBalancer, ServiceOptions $options, string $wikiID, @@ -82,6 +86,7 @@ class AbuseLogger { $this->filterLookup = $filterLookup; $this->varBlobStore = $varBlobStore; $this->varManager = $varManager; + $this->editRevUpdater = $editRevUpdater; $this->loadBalancer = $loadBalancer; $options->assertRequiredOptions( self::CONSTRUCTOR_OPTIONS ); $this->options = $options; @@ -166,10 +171,10 @@ class AbuseLogger { $globalLogIDs = $this->insertGlobalLogEntries( $centralLogRows, $fdb ); } - AbuseFilter::$logIds[ $this->title->getPrefixedText() ] = [ - 'local' => $localLogIDs, - 'global' => $globalLogIDs - ]; + $this->editRevUpdater->setLogIdsForTarget( + $this->title, + [ 'local' => $localLogIDs, 'global' => $globalLogIDs ] + ); return [ 'local' => $loggedLocalFilters, 'global' => $loggedGlobalFilters ]; } @@ -203,7 +208,7 @@ class AbuseLogger { /** * @param array[] $logRows * @param IDatabase $dbw - * @return array + * @return int[] */ private function insertLocalLogEntries( array $logRows, IDatabase $dbw ) : array { global $wgAbuseFilterAflFilterMigrationStage; @@ -275,7 +280,7 @@ class AbuseLogger { /** * @param array[] $centralLogRows * @param IDatabase $fdb - * @return array + * @return int[] */ private function insertGlobalLogEntries( array $centralLogRows, IDatabase $fdb ) : array { $this->varManager->computeDBVars( $this->vars ); diff --git a/includes/AbuseLoggerFactory.php b/includes/AbuseLoggerFactory.php index 3ddc580f9..9157a4048 100644 --- a/includes/AbuseLoggerFactory.php +++ b/includes/AbuseLoggerFactory.php @@ -21,6 +21,8 @@ class AbuseLoggerFactory { private $varBlobStore; /** @var VariablesManager */ private $varManager; + /** @var EditRevUpdater */ + private $editRevUpdater; /** @var ILoadBalancer */ private $loadBalancer; /** @var ServiceOptions */ @@ -35,6 +37,7 @@ class AbuseLoggerFactory { * @param FilterLookup $filterLookup * @param VariablesBlobStore $varBlobStore * @param VariablesManager $varManager + * @param EditRevUpdater $editRevUpdater * @param ILoadBalancer $loadBalancer * @param ServiceOptions $options * @param string $wikiID @@ -45,6 +48,7 @@ class AbuseLoggerFactory { FilterLookup $filterLookup, VariablesBlobStore $varBlobStore, VariablesManager $varManager, + EditRevUpdater $editRevUpdater, ILoadBalancer $loadBalancer, ServiceOptions $options, string $wikiID, @@ -54,6 +58,7 @@ class AbuseLoggerFactory { $this->filterLookup = $filterLookup; $this->varBlobStore = $varBlobStore; $this->varManager = $varManager; + $this->editRevUpdater = $editRevUpdater; $this->loadBalancer = $loadBalancer; $this->options = $options; $this->wikiID = $wikiID; @@ -76,6 +81,7 @@ class AbuseLoggerFactory { $this->filterLookup, $this->varBlobStore, $this->varManager, + $this->editRevUpdater, $this->loadBalancer, $this->options, $this->wikiID, diff --git a/includes/EditRevUpdater.php b/includes/EditRevUpdater.php new file mode 100644 index 000000000..dcb1cf042 --- /dev/null +++ b/includes/EditRevUpdater.php @@ -0,0 +1,138 @@ + [ 'local' => [ids], 'global' => [ids] ] ]. + * @phan-var array + */ + private $logIds = []; + + /** + * @param CentralDBManager $centralDBManager + * @param RevisionLookup $revisionLookup + * @param ILoadBalancer $loadBalancer + * @param string $wikiID + */ + public function __construct( + CentralDBManager $centralDBManager, + RevisionLookup $revisionLookup, + ILoadBalancer $loadBalancer, + string $wikiID + ) { + $this->centralDBManager = $centralDBManager; + $this->revisionLookup = $revisionLookup; + $this->loadBalancer = $loadBalancer; + $this->wikiID = $wikiID; + } + + /** + * Set the WikiPage object used for the ongoing edit + * + * @param WikiPage $page + */ + public function setLastEditPage( WikiPage $page ) : void { + $this->wikiPage = $page; + } + + /** + * Clear the WikiPage object used for the ongoing edit + */ + public function clearLastEditPage() : void { + $this->wikiPage = null; + } + + /** + * @param LinkTarget $target + * @param int[][] $logIds + * @phan-param array{local:int[],global:int[]} $logIds + */ + public function setLogIdsForTarget( LinkTarget $target, array $logIds ) : void { + if ( count( $logIds ) !== 2 || array_diff( array_keys( $logIds ), [ 'local', 'global' ] ) ) { + throw new InvalidArgumentException( 'Wrong keys; got: ' . implode( ', ', array_keys( $logIds ) ) ); + } + $key = $this->getCacheKey( $target ); + $this->logIds[$key] = $logIds; + } + + /** + * @param WikiPage $wikiPage + * @param RevisionRecord $revisionRecord + * @return bool Whether the DB was updated + */ + public function updateRev( WikiPage $wikiPage, RevisionRecord $revisionRecord ) : bool { + $key = $this->getCacheKey( $wikiPage->getTitle() ); + if ( !isset( $this->logIds[ $key ] ) || $wikiPage !== $this->wikiPage ) { + // This isn't the edit $this->logIds was set for + $this->logIds = []; + return false; + } + + // Ignore null edit. + $parentRevId = $revisionRecord->getParentId(); + if ( $parentRevId !== null ) { + $parentRev = $this->revisionLookup->getRevisionById( $parentRevId ); + if ( $parentRev && $revisionRecord->hasSameContent( $parentRev ) ) { + $this->logIds = []; + return false; + } + } + + $this->clearLastEditPage(); + + $ret = false; + $logs = $this->logIds[ $key ]; + if ( $logs[ 'local' ] ) { + $dbw = $this->loadBalancer->getConnectionRef( DB_MASTER ); + $dbw->update( 'abuse_filter_log', + [ 'afl_rev_id' => $revisionRecord->getId() ], + [ 'afl_id' => $logs['local'] ], + __METHOD__ + ); + $ret = true; + } + + if ( $logs[ 'global' ] ) { + $fdb = $this->centralDBManager->getConnection( DB_MASTER ); + $fdb->update( 'abuse_filter_log', + [ 'afl_rev_id' => $revisionRecord->getId() ], + [ 'afl_id' => $logs['global'], 'afl_wiki' => $this->wikiID ], + __METHOD__ + ); + $ret = true; + } + return $ret; + } + + /** + * @param LinkTarget $target + * @return string + */ + private function getCacheKey( LinkTarget $target ) : string { + return $target->getNamespace() . '|' . $target->getText(); + } +} diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index f90f351e2..468d1d751 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -17,6 +17,7 @@ use MediaWiki\Extension\AbuseFilter\Consequences\ConsequencesLookup; use MediaWiki\Extension\AbuseFilter\Consequences\ConsequencesRegistry; use MediaWiki\Extension\AbuseFilter\EchoNotifier; use MediaWiki\Extension\AbuseFilter\EditBoxBuilderFactory; +use MediaWiki\Extension\AbuseFilter\EditRevUpdater; use MediaWiki\Extension\AbuseFilter\FilterCompare; use MediaWiki\Extension\AbuseFilter\FilterImporter; use MediaWiki\Extension\AbuseFilter\FilterLookup; @@ -228,6 +229,7 @@ return [ $services->get( FilterLookup::SERVICE_NAME ), $services->get( VariablesBlobStore::SERVICE_NAME ), $services->get( VariablesManager::SERVICE_NAME ), + $services->get( EditRevUpdater::SERVICE_NAME ), $services->getDBLoadBalancer(), new ServiceOptions( AbuseLogger::CONSTRUCTOR_OPTIONS, @@ -331,6 +333,14 @@ return [ $services->getRepoGroup() ); }, + EditRevUpdater::SERVICE_NAME => function ( MediaWikiServices $services ): EditRevUpdater { + return new EditRevUpdater( + $services->get( CentralDBManager::SERVICE_NAME ), + $services->getRevisionLookup(), + $services->getDBLoadBalancer(), + WikiMap::getCurrentWikiDbDomain()->getId() + ); + }, ]; // @codeCoverageIgnoreEnd diff --git a/tests/phpunit/unit/AbuseLoggerFactoryTest.php b/tests/phpunit/unit/AbuseLoggerFactoryTest.php index b32b83698..4380ada90 100644 --- a/tests/phpunit/unit/AbuseLoggerFactoryTest.php +++ b/tests/phpunit/unit/AbuseLoggerFactoryTest.php @@ -7,6 +7,7 @@ use MediaWiki\Config\ServiceOptions; use MediaWiki\Extension\AbuseFilter\AbuseLogger; use MediaWiki\Extension\AbuseFilter\AbuseLoggerFactory; use MediaWiki\Extension\AbuseFilter\CentralDBManager; +use MediaWiki\Extension\AbuseFilter\EditRevUpdater; use MediaWiki\Extension\AbuseFilter\FilterLookup; use MediaWiki\Extension\AbuseFilter\Variables\VariableHolder; use MediaWiki\Extension\AbuseFilter\Variables\VariablesBlobStore; @@ -32,6 +33,7 @@ class AbuseLoggerFactoryTest extends MediaWikiUnitTestCase { $this->createMock( FilterLookup::class ), $this->createMock( VariablesBlobStore::class ), $this->createMock( VariablesManager::class ), + $this->createMock( EditRevUpdater::class ), $this->createMock( ILoadBalancer::class ), new ServiceOptions( AbuseLogger::CONSTRUCTOR_OPTIONS, diff --git a/tests/phpunit/unit/EditRevUpdaterTest.php b/tests/phpunit/unit/EditRevUpdaterTest.php new file mode 100644 index 000000000..4fc643cae --- /dev/null +++ b/tests/phpunit/unit/EditRevUpdaterTest.php @@ -0,0 +1,207 @@ +assertInstanceOf( + EditRevUpdater::class, + new EditRevUpdater( + $this->createMock( CentralDBManager::class ), + $this->createMock( RevisionLookup::class ), + $this->createMock( ILoadBalancer::class ), + '' + ) + ); + } + + /** + * @param IDatabase|null $localDB + * @param IDatabase|null $centralDB + * @param RevisionLookup|null $revLookup + * @return EditRevUpdater + */ + private function getUpdater( + IDatabase $localDB = null, + IDatabase $centralDB = null, + RevisionLookup $revLookup = null + ) : EditRevUpdater { + $localDB = $localDB ?? $this->createMock( IDatabase::class ); + $lb = $this->createMock( ILoadBalancer::class ); + $lb->method( 'getConnectionRef' )->willReturn( $localDB ); + $centralDB = $centralDB ?? $this->createMock( IDatabase::class ); + $dbManager = $this->createMock( CentralDBManager::class ); + $dbManager->method( 'getConnection' )->willReturn( $centralDB ); + return new EditRevUpdater( + $dbManager, + $revLookup ?? $this->createMock( RevisionLookup::class ), + $lb, + 'fake-wiki-id' + ); + } + + /** + * @param LinkTarget $target + * @return array + */ + private function getPageAndRev( LinkTarget $target ) : array { + $title = Title::newFromLinkTarget( $target ); + // Legacy code. Yay. + $title->mArticleID = 123456; + return [ new WikiPage( $title ), new MutableRevisionRecord( $title ) ]; + } + + /** + * @covers ::updateRev + * @covers ::getCacheKey + */ + public function testUpdateRev_noIDs() { + $titleValue = new TitleValue( NS_PROJECT, 'EditRevUpdater' ); + $this->assertFalse( $this->getUpdater()->updateRev( ...$this->getPageAndRev( $titleValue ) ) ); + } + + /** + * @covers ::setLastEditPage + * @covers ::setLogIdsForTarget + * @covers ::updateRev + * @covers ::getCacheKey + */ + public function testUpdateRev_differentPages() { + $titleValue = new TitleValue( NS_PROJECT, 'EditRevUpdater' ); + $updater = $this->getUpdater(); + $title = Title::makeTitle( NS_HELP, 'Foobar' ); + // Legacy code. Yay. + $title->mArticleID = 123456; + $updater->setLastEditPage( new WikiPage( $title ) ); + $updater->setLogIdsForTarget( $titleValue, [ 'local' => [ 1, 2 ], 'global' => [] ] ); + $this->assertFalse( $updater->updateRev( ...$this->getPageAndRev( $titleValue ) ) ); + } + + /** + * @covers ::setLastEditPage + * @covers ::setLogIdsForTarget + * @covers ::updateRev + * @covers ::getCacheKey + */ + public function testUpdateRev_nullEdit() { + $titleValue = new TitleValue( NS_PROJECT, 'EditRevUpdater' ); + [ $page, $rev ] = $this->getPageAndRev( $titleValue ); + $rev->setParentId( 42 ); + $revLookup = $this->createMock( RevisionLookup::class ); + $revLookup->expects( $this->once() )->method( 'getRevisionById' )->with( 42 )->willReturn( $rev ); + $updater = $this->getUpdater( null, null, $revLookup ); + $updater->setLastEditPage( $page ); + $updater->setLogIdsForTarget( $titleValue, [ 'local' => [ 1 ], 'global' => [ 1 ] ] ); + + $this->assertFalse( $updater->updateRev( $page, $rev ) ); + } + + /** + * @param array $ids + * @covers ::setLastEditPage + * @covers ::setLogIdsForTarget + * @covers ::updateRev + * @covers ::getCacheKey + * @dataProvider provideIDsSuccess + */ + public function testUpdateRev_success( array $ids ) { + $titleValue = new TitleValue( NS_PROJECT, 'EditRevUpdater' ); + [ $page, $rev ] = $this->getPageAndRev( $titleValue ); + $localDB = $this->createMock( IDatabase::class ); + $localDB->expects( $ids['local'] ? $this->once() : $this->never() )->method( 'update' ); + $centralDB = $this->createMock( IDatabase::class ); + $centralDB->expects( $ids['global'] ? $this->once() : $this->never() )->method( 'update' ); + $updater = $this->getUpdater( $localDB, $centralDB ); + $updater->setLastEditPage( $page ); + $updater->setLogIdsForTarget( $titleValue, $ids ); + + $this->assertTrue( $updater->updateRev( $page, $rev ) ); + } + + public function provideIDsSuccess() : array { + return [ + 'local only' => [ [ 'local' => [ 1, 2 ], 'global' => [] ] ], + 'global only' => [ [ 'local' => [], 'global' => [ 1, 2 ] ] ], + 'local and global' => [ [ 'local' => [ 1, 2 ], 'global' => [ 1, 2 ] ] ], + ]; + } + + /** + * @covers ::setLastEditPage + * @covers ::setLogIdsForTarget + * @covers ::updateRev + * @covers ::getCacheKey + */ + public function testUpdateRev_multipleTitles() { + $goodTitleValue = new TitleValue( NS_PROJECT, 'EditRevUpdater' ); + $badTitleValue = new TitleValue( NS_PROJECT, 'These should not be used' ); + $goodIDs = [ 'local' => [ 1, 2 ], 'global' => [] ]; + $badIDs = [ 'local' => [], 'global' => [ 1, 2 ] ]; + [ $page, $rev ] = $this->getPageAndRev( $goodTitleValue ); + $localDB = $this->createMock( IDatabase::class ); + $localDB->expects( $this->once() )->method( 'update' ); + $centralDB = $this->createMock( IDatabase::class ); + $centralDB->expects( $this->never() )->method( 'update' ); + $updater = $this->getUpdater( $localDB, $centralDB ); + $updater->setLastEditPage( $page ); + $updater->setLogIdsForTarget( $goodTitleValue, $goodIDs ); + $updater->setLogIdsForTarget( $badTitleValue, $badIDs ); + + $this->assertTrue( $updater->updateRev( $page, $rev ) ); + } + + /** + * @covers ::clearLastEditPage + * @covers ::updateRev + */ + public function testClearLastEditPage() { + $titleValue = new TitleValue( NS_PROJECT, 'EditRevUpdater-clear' ); + [ $page, $revisionRecord ] = $this->getPageAndRev( $titleValue ); + $updater = $this->getUpdater(); + $updater->setLastEditPage( $page ); + $updater->setLogIdsForTarget( $titleValue, [ 'local' => [ 1, 2 ], 'global' => [] ] ); + $updater->clearLastEditPage(); + $this->assertFalse( $updater->updateRev( $page, $revisionRecord ) ); + } + + /** + * @param array $ids + * @covers ::setLogIdsForTarget + * @dataProvider provideInvalidIDs + */ + public function testSetLogIdsForTarget_invalid( array $ids ) { + $updater = $this->getUpdater(); + $this->expectException( InvalidArgumentException::class ); + $updater->setLogIdsForTarget( new TitleValue( NS_MAIN, 'x' ), $ids ); + } + + public function provideInvalidIDs() : array { + return [ + 'empty' => [ [] ], + 'missing key' => [ [ 'local' => [ 1 ] ] ], + 'extra key' => [ [ 'local' => [ 1 ], 'global' => [ 1 ], 'foo' => [ 1 ] ] ], + ]; + } +}