From 963a8ad09c23efe7060fee1362388601412f86f8 Mon Sep 17 00:00:00 2001 From: Dreamy Jazz Date: Tue, 1 Apr 2025 20:35:20 +0100 Subject: ManualLogEntry: Check RecentChanges bot flag before POSTSEND Why: * ManualLogEntry::publish causes an entry to be added to the recentchanges table as well as other publishing actions for the log entry. * This is done in a DeferredUpdate to avoid writes that block the response to the user unnecessarily. * However, doing this means that if we assign a temporary user right during the course of the POST request (such as in the case of an extension wanting to make lots of log entries) this is not seen in the RecentChanges object created at the POSTSEND stage. * Creating the RecentChanges entry at the time of calling ::publish allows temporary user rights, such as 'bot', to take effect and mark the log entries as bot actions. ** We still put the writes to the DB POSTSEND as we only need to create the RecentChanges object to get the bot flag set correctly. What: * Update ManualLogEntry::publish to create the RecentChanges object outside the POSTSEND DeferredUpdate. * Add tests for ManualLogEntry which was previously completely untested (though likely indirectly tested through code which uses this class). ** Adding the tests allows verification that the change performed actually does what we want it to do. Bug: T387659 Change-Id: I80d906bbc1cad180a815477477d9081f9775a197 --- includes/logging/ManualLogEntry.php | 29 +++--- .../includes/logging/ManualLogEntryTest.php | 101 +++++++++++++++++++++ 2 files changed, 117 insertions(+), 13 deletions(-) create mode 100644 tests/phpunit/integration/includes/logging/ManualLogEntryTest.php diff --git a/includes/logging/ManualLogEntry.php b/includes/logging/ManualLogEntry.php index 46a29a9133c0..d2ea485c1072 100644 --- a/includes/logging/ManualLogEntry.php +++ b/includes/logging/ManualLogEntry.php @@ -438,14 +438,17 @@ class ManualLogEntry extends LogEntryBase implements Taggable { $canAddTags = false; } - DeferredUpdates::addCallableUpdate( - function () use ( $newId, $to, $canAddTags ) { - $log = new LogPage( $this->getType() ); - if ( !$log->isRestricted() ) { - ( new HookRunner( MediaWikiServices::getInstance()->getHookContainer() ) ) - ->onManualLogEntryBeforePublish( $this ); - $rc = $this->getRecentChange( $newId ); - + $log = new LogPage( $this->getType() ); + if ( !$log->isRestricted() ) { + // We need to generate a RecentChanges object now so that we can have the rc_bot attribute set based + // on any temporary user rights assigned to the user as part of the creation of this log entry. + // We do not attempt to save it to the DB until POSTSEND to avoid writes blocking a response (T127852). + ( new HookRunner( MediaWikiServices::getInstance()->getHookContainer() ) ) + ->onManualLogEntryBeforePublish( $this ); + $rc = $this->getRecentChange( $newId ); + + DeferredUpdates::addCallableUpdate( + function () use ( $newId, $to, $canAddTags, $rc ) { if ( $to === 'rc' || $to === 'rcandudp' ) { // save RC, passing tags so they are applied there $rc->addTags( $this->getTags() ); @@ -466,11 +469,11 @@ class ManualLogEntry extends LogEntryBase implements Taggable { if ( $to === 'udp' || $to === 'rcandudp' ) { $rc->notifyRCFeeds(); } - } - }, - DeferredUpdates::POSTSEND, - MediaWikiServices::getInstance()->getConnectionProvider()->getPrimaryDatabase() - ); + }, + DeferredUpdates::POSTSEND, + MediaWikiServices::getInstance()->getConnectionProvider()->getPrimaryDatabase() + ); + } } /** diff --git a/tests/phpunit/integration/includes/logging/ManualLogEntryTest.php b/tests/phpunit/integration/includes/logging/ManualLogEntryTest.php new file mode 100644 index 000000000000..80995a63449c --- /dev/null +++ b/tests/phpunit/integration/includes/logging/ManualLogEntryTest.php @@ -0,0 +1,101 @@ +getTestUser()->getUser(); + $target = $this->getExistingTestPage()->getTitle(); + $logParams = [ '4::test' => 'testabc1234', 'testing' => 'testingabc' ]; + + // Give the performer the bot right temporarily to allow testing that ::publish + // (if specified by this test case), so that we can test that ::publish sets the RecentChanges bot flag + // outside any DeferredUpdates to prevent issues such as described in T387659. + if ( $userHasBotRight ) { + $userRightScope = $this->getServiceContainer()->getPermissionManager() + ->addTemporaryUserRights( $performer, 'bot' ); + } + + // Create a log entry and publish it to just RecentChanges + $logEntry = new ManualLogEntry( 'phpunit', 'test' ); + $logEntry->setPerformer( $performer ); + $logEntry->setTarget( $target ); + $logEntry->setComment( 'A very good reason' ); + $logEntry->setTimestamp( '20300405060708' ); + $logEntry->setParameters( $logParams ); + $logEntry->setDeleted( LogPage::DELETED_ACTION ); + $logId = $logEntry->insert(); + $logEntry->publish( $logId, 'rc' ); + ScopedCallback::consume( $userRightScope ); + + // Actually cause the writes to RecentChanges, as they are queued using DeferredUpdates. + DeferredUpdates::doUpdates(); + + // Assert that the log entry exists and that it matches what we provided above. + $this->newSelectQueryBuilder() + ->select( [ + 'log_type', 'log_action', 'log_timestamp', 'log_namespace', 'log_title', 'log_deleted', + 'log_actor', 'comment_text' + ] ) + ->from( 'logging' ) + ->join( 'comment', null, 'log_comment_id=comment_id' ) + ->where( [ 'log_id' => $logId ] ) + ->caller( __METHOD__ ) + ->assertRowValue( [ + 'phpunit', 'test', $this->getDb()->timestamp( '20300405060708' ), $target->getNamespace(), + $target->getDBkey(), LogPage::DELETED_ACTION, $performer->getActorId(), 'A very good reason', + ] ); + $this->assertArrayEquals( + $logParams, + LogEntryBase::extractParams( + $this->newSelectQueryBuilder() + ->select( 'log_params' ) + ->from( 'logging' ) + ->caller( __METHOD__ ) + ->where( [ 'log_id' => $logId ] ) + ->fetchField() + ), + false, + true + ); + + // Assert that the entry was sent to RecentChanges + $actualRecentChangeObject = RecentChange::newFromConds( [ 'rc_logid' => $logId ] ); + $actualRecentChangeTitle = $actualRecentChangeObject->getPage(); + $this->assertNotNull( $actualRecentChangeTitle ); + $this->assertTrue( $target->isSamePageAs( $actualRecentChangeTitle ) ); + $this->assertTrue( $performer->equals( $actualRecentChangeObject->getPerformerIdentity() ) ); + $this->assertArrayContains( + [ + // DB stores booleans as integers and we get back that integer as a string. + 'rc_bot' => (string)intval( $userHasBotRight ), + 'rc_log_type' => 'phpunit', + 'rc_log_action' => 'test', + // The RecentChanges object stores the timestamp as TS_MW, even if DB stores it in a different + // format. + 'rc_timestamp' => '20300405060708', + ], + $actualRecentChangeObject->getAttributes() + ); + } + + public static function provideCreationAndPublishingToRecentChanges() { + return [ + 'User does not have bot right' => [ false ], + 'User temporarily has the bot right' => [ true ], + ]; + } +} -- cgit v1.2.3