Merge "Don't attempt to steal or create the FilterUser in CheckUserHandler"

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;
+	}
 }

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;

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' )
 		);
 	},

tests/phpunit/AbuseFilterFilterUserTest.php

@@ -1,29 +1,27 @@
 <?php
 
 use MediaWiki\Extension\AbuseFilter\FilterUser;
-use MediaWiki\MediaWikiServices;
 use Psr\Log\NullLogger;
 
 /**
  * @group Test
  * @group AbuseFilter
  * @group Database
- * @coversDefaultClass \MediaWiki\Extension\AbuseFilter\FilterUser
- * @covers ::__construct()
+ * @covers \MediaWiki\Extension\AbuseFilter\FilterUser
  * @todo Make a unit test once DI is possible for User::newSystemUser
  */
 class AbuseFilterFilterUserTest extends MediaWikiIntegrationTestCase {
-	/**
-	 * @covers ::getUser
-	 * @covers ::getUserIdentity
-	 * @covers ::getAuthority
-	 */
 	public function testGetUser() {
 		$name = 'AbuseFilter blocker user';
 		$ml = $this->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() ) );
+	}
 }

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 ) {