From a5b68cf46ddbbbbe31f6a207703440009347d04d Mon Sep 17 00:00:00 2001 From: Dreamy Jazz Date: Wed, 31 Jan 2024 17:50:53 +0000 Subject: [PATCH] Don't attempt to steal or create the FilterUser in CheckUserHandler Why: * When CheckUser asks the AbuseFilter extension for modifications to rows inserted into the CheckUser tables, the AbuseFilter extension attempts to get the Filter user via User::newSystemUser * User::newSystemUser can deadlock if multiple requests to create the system user are being made at once. * The CheckUserHander does not need to create the abuse filter system and instead only needs to know if a given $user is the equal to the FilterUser. * As such the FilterUser service needs to provide a way to check if a given $user is equal without creating the FilterUser. What: * Add FilterUser::isUserSameAs which returns a boolean value indicating whether the Abuse Filter system user is the equal to a given UserIdentity in the same way that UserIdentity::equals is implemented. * Refactor ::getUser to get the username for the filter user in a separate method, so that the ::isUserSameAs method can also use this method. Name this new method ::getFilterUserName. * Add a test for the FilterUser service to ensure consistent test coverage * Convert the @covers and @coversDefaultClass annotations to be a @covers for the class. This is because PHPUnit recommends this in https://docs.phpunit.de/en/9.6/annotations.html#appendixes-annotations-covers-tables-annotations Bug: T356275 Bug: T346967 Change-Id: I8a101781bb47612deabb0f2a06a398ac13e860e6 --- includes/FilterUser.php | 70 ++++++++++++------- includes/Hooks/Handlers/CheckUserHandler.php | 6 +- includes/ServiceWiring.php | 1 + tests/phpunit/AbuseFilterFilterUserTest.php | 46 +++++++----- .../Hooks/CheckUserHandlerTest.php | 6 +- 5 files changed, 83 insertions(+), 46 deletions(-) diff --git a/includes/FilterUser.php b/includes/FilterUser.php index 028421be0..6033edd57 100644 --- a/includes/FilterUser.php +++ b/includes/FilterUser.php @@ -6,31 +6,33 @@ use MediaWiki\Permissions\Authority; use MediaWiki\User\User; use MediaWiki\User\UserGroupManager; use MediaWiki\User\UserIdentity; +use MediaWiki\User\UserNameUtils; use MessageLocalizer; use Psr\Log\LoggerInterface; class FilterUser { public const SERVICE_NAME = 'AbuseFilterFilterUser'; - /** @var MessageLocalizer */ - private $messageLocalizer; - /** @var UserGroupManager */ - private $userGroupManager; - /** @var LoggerInterface */ - private $logger; + private MessageLocalizer $messageLocalizer; + private UserGroupManager $userGroupManager; + private UserNameUtils $userNameUtils; + private LoggerInterface $logger; /** * @param MessageLocalizer $messageLocalizer * @param UserGroupManager $userGroupManager + * @param UserNameUtils $userNameUtils * @param LoggerInterface $logger */ public function __construct( MessageLocalizer $messageLocalizer, UserGroupManager $userGroupManager, + UserNameUtils $userNameUtils, LoggerInterface $logger ) { $this->messageLocalizer = $messageLocalizer; $this->userGroupManager = $userGroupManager; + $this->userNameUtils = $userNameUtils; $this->logger = $logger; } @@ -48,30 +50,23 @@ class FilterUser { return $this->getUser(); } + /** + * Compares the given $user to see if they are the same as the FilterUser. + * + * @return bool + */ + public function isSameUserAs( UserIdentity $user ): bool { + // Checking the usernames are equal is enough, as this is what is done by + // User::equals and UserIdentityValue::equals. + return $user->getName() === $this->getFilterUserName(); + } + /** * @todo Stop using the User class when User::newSystemUser is refactored. * @return User */ private function getUser(): User { - $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. - // Don't use the database in case the English onwiki message is broken, T284364 - $defaultName = $this->messageLocalizer->msg( 'abusefilter-blocker' ) - ->inLanguage( 'en' ) - ->useDatabase( false ) - ->text(); - $user = User::newSystemUser( $defaultName, [ 'steal' => true ] ); - } + $user = User::newSystemUser( $this->getFilterUserName(), [ 'steal' => true ] ); '@phan-var User $user'; // Promote user to 'sysop' so it doesn't look @@ -82,4 +77,29 @@ class FilterUser { return $user; } + + /** + * Gets the username for the FilterUser. + * + * @return string + */ + private function getFilterUserName(): string { + $username = $this->messageLocalizer->msg( 'abusefilter-blocker' )->inContentLanguage()->text(); + if ( !$this->userNameUtils->getCanonical( $username ) ) { + // 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. + // Don't use the database in case the English onwiki message is broken, T284364 + $username = $this->messageLocalizer->msg( 'abusefilter-blocker' ) + ->inLanguage( 'en' ) + ->useDatabase( false ) + ->text(); + } + return $username; + } } diff --git a/includes/Hooks/Handlers/CheckUserHandler.php b/includes/Hooks/Handlers/CheckUserHandler.php index 280e6e1bc..8c59f9bfd 100644 --- a/includes/Hooks/Handlers/CheckUserHandler.php +++ b/includes/Hooks/Handlers/CheckUserHandler.php @@ -45,7 +45,7 @@ class CheckUserHandler implements ) { if ( $this->userIdentityUtils->isNamed( $user ) && - $this->filterUser->getUserIdentity()->getId() == $user->getId() + $this->filterUser->isSameUserAs( $user ) ) { $ip = '127.0.0.1'; $xff = false; @@ -64,7 +64,7 @@ class CheckUserHandler implements ) { if ( $this->userIdentityUtils->isNamed( $user ) && - $this->filterUser->getUserIdentity()->getId() == $user->getId() + $this->filterUser->isSameUserAs( $user ) ) { $ip = '127.0.0.1'; $xff = false; @@ -83,7 +83,7 @@ class CheckUserHandler implements ) { if ( $this->userIdentityUtils->isNamed( $user ) && - $this->filterUser->getUserIdentity()->getId() == $user->getId() + $this->filterUser->isSameUserAs( $user ) ) { $ip = '127.0.0.1'; $xff = false; diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index 82197719c..79301cc2f 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -110,6 +110,7 @@ return [ // TODO We need a proper MessageLocalizer, see T247127 RequestContext::getMain(), $services->getUserGroupManager(), + $services->getUserNameUtils(), LoggerFactory::getInstance( 'AbuseFilter' ) ); }, diff --git a/tests/phpunit/AbuseFilterFilterUserTest.php b/tests/phpunit/AbuseFilterFilterUserTest.php index 325822d82..7fac6cc1c 100644 --- a/tests/phpunit/AbuseFilterFilterUserTest.php +++ b/tests/phpunit/AbuseFilterFilterUserTest.php @@ -1,29 +1,27 @@ createMock( MessageLocalizer::class ); $ml->method( 'msg' )->willReturn( $this->getMockMessage( $name ) ); - $ugm = MediaWikiServices::getInstance()->getUserGroupManager(); - $filterUser = new FilterUser( $ml, $ugm, new NullLogger() ); + $ugm = $this->getServiceContainer()->getUserGroupManager(); + $filterUser = new FilterUser( + $ml, + $ugm, + $this->getServiceContainer()->getUserNameUtils(), + new NullLogger() + ); $actual = $filterUser->getUserIdentity(); $this->assertSame( $name, $actual->getName(), 'name' ); @@ -31,10 +29,6 @@ class AbuseFilterFilterUserTest extends MediaWikiIntegrationTestCase { $this->assertTrue( $filterUser->getAuthority()->isAllowed( 'block' ) ); } - /** - * @covers ::getUser - * @covers ::getUserIdentity - */ public function testGetUser_invalidName() { $name = 'Foobar filter user'; $ml = $this->createMock( MessageLocalizer::class ); @@ -42,10 +36,15 @@ class AbuseFilterFilterUserTest extends MediaWikiIntegrationTestCase { $msg->method( 'inContentLanguage' )->willReturn( $this->getMockMessage( '' ) ); $msg->method( 'inLanguage' )->willReturn( $this->getMockMessage( $name ) ); $ml->method( 'msg' )->willReturn( $msg ); - $ugm = MediaWikiServices::getInstance()->getUserGroupManager(); + $ugm = $this->getServiceContainer()->getUserGroupManager(); $logger = new TestLogger(); $logger->setCollect( true ); - $filterUser = new FilterUser( $ml, $ugm, $logger ); + $filterUser = new FilterUser( + $ml, + $ugm, + $this->getServiceContainer()->getUserNameUtils(), + $logger + ); $actual = $filterUser->getUserIdentity(); $this->assertSame( $name, $actual->getName(), 'name' ); @@ -58,4 +57,19 @@ class AbuseFilterFilterUserTest extends MediaWikiIntegrationTestCase { } $this->assertTrue( $found, 'Invalid name not logged' ); } + + public function testIsUserSameAs() { + $ml = $this->createMock( MessageLocalizer::class ); + $ml->method( 'msg' )->willReturn( $this->getMockMessage( 'AbuseFilter blocker user' ) ); + $ugm = $this->getServiceContainer()->getUserGroupManager(); + $filterUser = new FilterUser( + $ml, + $ugm, + $this->getServiceContainer()->getUserNameUtils(), + new NullLogger() + ); + + $this->assertTrue( $filterUser->isSameUserAs( $filterUser->getUserIdentity() ) ); + $this->assertFalse( $filterUser->isSameUserAs( $this->getTestUser()->getUser() ) ); + } } diff --git a/tests/phpunit/integration/Hooks/CheckUserHandlerTest.php b/tests/phpunit/integration/Hooks/CheckUserHandlerTest.php index b14f80d81..aab3339ab 100644 --- a/tests/phpunit/integration/Hooks/CheckUserHandlerTest.php +++ b/tests/phpunit/integration/Hooks/CheckUserHandlerTest.php @@ -21,8 +21,10 @@ class CheckUserHandlerTest extends MediaWikiIntegrationTestCase { private function getCheckUserHandler(): CheckUserHandler { $filterUser = $this->createMock( FilterUser::class ); - $filterUser->method( 'getUserIdentity' ) - ->willReturn( new UserIdentityValue( 1, 'Abuse filter' ) ); + $filterUser->method( 'isSameUserAs' ) + ->willReturnCallback( static function ( $user ) { + return $user->getName() === 'Abuse filter'; + } ); $userIdentityUtils = $this->createMock( UserIdentityUtils::class ); $userIdentityUtils->method( 'isNamed' ) ->willReturnCallback( static function ( $name ) {