From 2e13d58c748974b37e95109de031acb8f4c78180 Mon Sep 17 00:00:00 2001 From: Daimona Eaytoy Date: Tue, 28 Aug 2018 19:22:16 +0200 Subject: [PATCH] Add tests for retrieving RC variables This was also long overdue. Also fix a bug that caused page creations to not be shown when examining past edits (using rc_last_oldid doesn't work for page creations). Bug: T201193 Bug: T262903 Change-Id: I5f7a994add12332c950904146248c5de7c2beee5 --- includes/KeywordsManager.php | 9 + .../VariableGenerator/RCVariableGenerator.php | 3 +- .../AbuseFilterVariableGeneratorDBTest.php | 169 ++++++++++++++++++ .../unit/AbuseFilterKeywordsManagerTest.php | 9 + 4 files changed, 189 insertions(+), 1 deletion(-) diff --git a/includes/KeywordsManager.php b/includes/KeywordsManager.php index d86c9a3fc..51029ba69 100644 --- a/includes/KeywordsManager.php +++ b/includes/KeywordsManager.php @@ -280,4 +280,13 @@ class KeywordsManager { public function getVarsMappings(): array { return $this->getBuilderValues()['vars']; } + + /** + * Get a list of core variables, i.e. variables defined in AbuseFilter (ignores hooks). + * You usually want to use getVarsMappings(), not this one. + * @return string[] + */ + public function getCoreVariables() : array { + return array_keys( self::BUILDER_VALUES['vars'] ); + } } diff --git a/includes/VariableGenerator/RCVariableGenerator.php b/includes/VariableGenerator/RCVariableGenerator.php index 8c4a1d81b..bb1b45866 100644 --- a/includes/VariableGenerator/RCVariableGenerator.php +++ b/includes/VariableGenerator/RCVariableGenerator.php @@ -84,11 +84,12 @@ class RCVariableGenerator extends VariableGenerator { default: return null; } - } elseif ( $this->rc->getAttribute( 'rc_last_oldid' ) ) { + } elseif ( $this->rc->getAttribute( 'rc_this_oldid' ) ) { // It's an edit. $this->addEditVarsForRow(); } else { // @todo Ensure this cannot happen, and throw if it does + wfLogWarning( 'Cannot understand the given recentchanges row!' ); return null; } diff --git a/tests/phpunit/AbuseFilterVariableGeneratorDBTest.php b/tests/phpunit/AbuseFilterVariableGeneratorDBTest.php index 7c27c61f8..2bf72bf86 100644 --- a/tests/phpunit/AbuseFilterVariableGeneratorDBTest.php +++ b/tests/phpunit/AbuseFilterVariableGeneratorDBTest.php @@ -1,6 +1,9 @@ assertSame( $expected, $actual, "Prefix: $prefix" ); } } + + /** + * Check all methods used to retrieve variables from an RC row + * + * @param string $type Type of the action the row refers to + * @param string $action Same as the 'action' variable + * @covers \MediaWiki\Extension\AbuseFilter\VariableGenerator\RCVariableGenerator + * @covers AbuseFilterVariableHolder + * @dataProvider provideRCRowTypes + */ + public function testGetVarsFromRCRow( string $type, string $action ) { + $timestamp = '1514700000'; + MWTimestamp::setFakeTime( $timestamp ); + $user = $this->getMutableTestUser()->getUser(); + $title = Title::newFromText( 'AbuseFilter testing page' ); + $page = $type === 'create' ? WikiPage::factory( $title ) : $this->getExistingTestPage( $title ); + $page->clear(); + + $summary = 'Abuse Filter summary for RC tests'; + $expectedValues = [ + 'user_name' => $user->getName(), + 'action' => $action, + 'summary' => $summary + ]; + + switch ( $type ) { + case 'create': + $expectedValues['old_wikitext'] = ''; + // Fallthrough + case 'edit': + $newText = 'Some new text for testing RC vars.'; + $this->editPage( $title->getText(), $newText, $summary, $title->getNamespace(), $user ); + + $expectedValues += [ + 'page_id' => $page->getId(), + 'page_namespace' => $title->getNamespace(), + 'page_title' => $title->getText(), + 'page_prefixedtitle' => $title->getPrefixedText(), + 'timestamp' => $timestamp + ]; + break; + case 'move': + $newTitle = Title::newFromText( 'Another AbuseFilter testing page' ); + $mpf = MediaWikiServices::getInstance()->getMovePageFactory(); + $mp = $mpf->newMovePage( $title, $newTitle ); + $mp->move( $user, $summary, false ); + $newID = WikiPage::factory( $newTitle )->getId(); + + $expectedValues += [ + 'moved_from_id' => $page->getId(), + 'moved_from_namespace' => $title->getNamespace(), + 'moved_from_title' => $title->getText(), + 'moved_from_prefixedtitle' => $title->getPrefixedText(), + 'moved_to_id' => $newID, + 'moved_to_namespace' => $newTitle->getNamespace(), + 'moved_to_title' => $newTitle->getText(), + 'moved_to_prefixedtitle' => $newTitle->getPrefixedText(), + 'timestamp' => $timestamp + ]; + break; + case 'delete': + $page->doDeleteArticleReal( $summary, $user ); + $expectedValues += [ + 'page_id' => $page->getId(), + 'page_namespace' => $title->getNamespace(), + 'page_title' => $title->getText(), + 'page_prefixedtitle' => $title->getPrefixedText(), + 'timestamp' => $timestamp + ]; + break; + case 'newusers': + // FIXME AuthManager doesn't expose the method for doing this. And mocking AuthManager + // and everything here seems overkill. + $accountName = 'AbuseFilter dummy user'; + $subType = $action === 'createaccount' ? 'create2' : 'autocreate'; + $logEntry = new \ManualLogEntry( 'newusers', $subType ); + $logEntry->setPerformer( $user ); + $logEntry->setTarget( Title::makeTitle( NS_USER, $accountName ) ); + $logEntry->setComment( 'Fooobarcomment' ); + $logEntry->setParameters( [ + '4::userid' => $user->getId(), + ] ); + $logid = $logEntry->insert(); + $logEntry->publish( $logid ); + + $expectedValues = [ + 'action' => $action, + 'accountname' => $accountName, + 'user_name' => $user->getName(), + 'timestamp' => $timestamp + ]; + break; + case 'upload': + // TODO Same problem as AuthManager, but worse because here we'd really have to + // upload a file (addUploadVars immediately tries reading the file) + $this->markTestIncomplete( 'Implement "upload" case!' ); + break; + default: + throw new LogicException( "Type $type not recognized!" ); + } + + if ( $type === 'edit' ) { + $where = [ 'rc_source' => 'mw.edit' ]; + } elseif ( $type === 'create' ) { + $where = [ 'rc_source' => 'mw.new' ]; + } else { + $where = [ 'rc_log_type' => $type ]; + } + $rcQuery = RecentChange::getQueryInfo(); + $row = $this->db->selectRow( + $rcQuery['tables'], + $rcQuery['fields'], + $where, + __METHOD__, + [ 'ORDER BY rc_id DESC' ], + $rcQuery['joins'] + ); + + $rc = RecentChange::newFromRow( $row ); + $varGenerator = new RCVariableGenerator( + new AbuseFilterVariableHolder(), + $rc, + $this->getTestSysop()->getUser() + ); + $actual = $varGenerator->getVars()->getVars(); + + // Convert PHP variables to AFPData + $expected = array_map( [ 'AFPData', 'newFromPHPVar' ], $expectedValues ); + + // Remove lazy variables (covered in other tests) and variables coming + // from other extensions (may not be generated, depending on the test environment) + $coreVariables = AbuseFilterServices::getKeywordsManager()->getCoreVariables(); + foreach ( $actual as $var => $value ) { + if ( !in_array( $var, $coreVariables, true ) || $value instanceof AFComputedVariable ) { + unset( $actual[ $var ] ); + } + } + + // Not assertSame because we're comparing different AFPData objects + $this->assertEquals( $expected, $actual ); + } + + /** + * Data provider for testGetVarsFromRCRow + * @return array + */ + public function provideRCRowTypes() { + return [ + 'edit' => [ 'edit', 'edit' ], + 'create' => [ 'create', 'edit' ], + 'move' => [ 'move', 'move' ], + 'delete' => [ 'delete', 'delete' ], + 'createaccount' => [ 'newusers', 'createaccount' ], + 'autocreateaccount' => [ 'newusers', 'autocreateaccount' ], + 'upload' => [ 'upload', 'upload' ], + ]; + } } diff --git a/tests/phpunit/unit/AbuseFilterKeywordsManagerTest.php b/tests/phpunit/unit/AbuseFilterKeywordsManagerTest.php index cff5be8ca..5cb2861bc 100644 --- a/tests/phpunit/unit/AbuseFilterKeywordsManagerTest.php +++ b/tests/phpunit/unit/AbuseFilterKeywordsManagerTest.php @@ -195,4 +195,13 @@ class AbuseFilterKeywordsManagerTest extends MediaWikiUnitTestCase { $this->assertContainsOnly( 'string', $actual, true ); $this->assertContainsOnly( 'string', array_keys( $actual ), true ); } + + /** + * @covers \MediaWiki\Extension\AbuseFilter\KeywordsManager::getCoreVariables + */ + public function testGetCoreVariables() { + $actual = $this->getKeywordsManager()->getCoreVariables(); + $this->assertIsArray( $actual ); + $this->assertContainsOnly( 'string', $actual, true ); + } }