Use ActionSpecifier to load the IP address

To avoid access to the global request context.

Change-Id: I4d97dbe8b693f1fcd5a4e84f2376752d8e954c18

includes/ActionSpecifier.php

@@ -9,7 +9,7 @@ use MediaWiki\User\UserIdentity;
 /**
  * Plain value object that univocally represents an action being filtered
  * @todo Add constants for possible actions?
- * @todo Add the timestamp and IP address
+ * @todo Add the timestamp
  */
 class ActionSpecifier {
 	/** @var string */
@@ -18,6 +18,8 @@ class ActionSpecifier {
 	private $title;
 	/** @var UserIdentity */
 	private $user;
+	/** @var string */
+	private $requestIP;
 	/** @var string|null */
 	private $accountName;
 
@@ -26,15 +28,19 @@ class ActionSpecifier {
 	 * @param LinkTarget $title Where the current action is executed. This is the user page
 	 *   for account creations.
 	 * @param UserIdentity $user
+	 * @param string $requestIP
 	 * @param string|null $accountName Required iff the action is an account creation
 	 */
-	public function __construct( string $action, LinkTarget $title, UserIdentity $user, ?string $accountName ) {
+	public function __construct(
+		string $action, LinkTarget $title, UserIdentity $user, string $requestIP, ?string $accountName
+	) {
 		if ( $accountName === null && strpos( $action, 'createaccount' ) !== false ) {
 			throw new InvalidArgumentException( '$accountName required for account creations' );
 		}
 		$this->action = $action;
 		$this->title = $title;
 		$this->user = $user;
+		$this->requestIP = $requestIP;
 		$this->accountName = $accountName;
 	}
 
@@ -59,6 +65,14 @@ class ActionSpecifier {
 		return $this->user;
 	}
 
+	/**
+	 * @return string
+	 * @note It may be an empty string for less recent changes.
+	 */
+	public function getIP(): string {
+		return $this->requestIP;
+	}
+
 	/**
 	 * @return string|null
 	 */

includes/ChangeTags/ChangeTagger.php

@@ -108,7 +108,14 @@ class ChangeTagger {
 			$recentChange->getAttribute( 'rc_user' ),
 			$recentChange->getAttribute( 'rc_user_text' )
 		);
-		return $this->getActionID( new ActionSpecifier( $action, $title, $user, $user->getName() ) );
+		$specifier = new ActionSpecifier(
+			$action,
+			$title,
+			$user,
+			$recentChange->getAttribute( 'rc_ip' ) ?? '',
+			$user->getName()
+		);
+		return $this->getActionID( $specifier );
 	}
 
 	/**

includes/Consequences/Consequence/RangeBlock.php

@@ -18,8 +18,6 @@ class RangeBlock extends BlockingConsequence {
 	private $rangeBlockSize;
 	/** @var int[] */
 	private $blockCIDRLimit;
-	/** @var string */
-	private $requestIP;
 
 	/**
 	 * @param Parameters $parameters
@@ -30,7 +28,6 @@ class RangeBlock extends BlockingConsequence {
 	 * @param LoggerInterface $logger
 	 * @param array $rangeBlockSize
 	 * @param array $blockCIDRLimit
-	 * @param string $requestIP
 	 */
 	public function __construct(
 		Parameters $parameters,
@@ -40,22 +37,21 @@ class RangeBlock extends BlockingConsequence {
 		MessageLocalizer $messageLocalizer,
 		LoggerInterface $logger,
 		array $rangeBlockSize,
-		array $blockCIDRLimit,
-		string $requestIP
+		array $blockCIDRLimit
 	) {
 		parent::__construct( $parameters, $expiry, $blockUserFactory, $filterUser, $messageLocalizer, $logger );
 		$this->rangeBlockSize = $rangeBlockSize;
 		$this->blockCIDRLimit = $blockCIDRLimit;
-		$this->requestIP = $requestIP;
 	}
 
 	/**
 	 * @inheritDoc
 	 */
 	public function execute(): bool {
-		$type = IPUtils::isIPv6( $this->requestIP ) ? 'IPv6' : 'IPv4';
+		$requestIP = $this->parameters->getActionSpecifier()->getIP();
+		$type = IPUtils::isIPv6( $requestIP ) ? 'IPv6' : 'IPv4';
 		$CIDRsize = max( $this->rangeBlockSize[$type], $this->blockCIDRLimit[$type] );
-		$blockCIDR = $this->requestIP . '/' . $CIDRsize;
+		$blockCIDR = $requestIP . '/' . $CIDRsize;
 
 		$target = IPUtils::sanitizeRange( $blockCIDR );
 		$status = $this->doBlockInternal(

includes/Consequences/Consequence/Throttle.php

@@ -26,8 +26,6 @@ class Throttle extends Consequence implements ConsequencesDisablerConsequence {
 	private $userFactory;
 	/** @var LoggerInterface */
 	private $logger;
-	/** @var string */
-	private $requestIP;
 	/** @var $bool */
 	private $filterIsCentral;
 	/** @var string|null */
@@ -47,7 +45,6 @@ class Throttle extends Consequence implements ConsequencesDisablerConsequence {
 	 * @param UserEditTracker $userEditTracker
 	 * @param UserFactory $userFactory
 	 * @param LoggerInterface $logger
-	 * @param string $requestIP
 	 * @param bool $filterIsCentral
 	 * @param string|null $centralDB
 	 */
@@ -58,7 +55,6 @@ class Throttle extends Consequence implements ConsequencesDisablerConsequence {
 		UserEditTracker $userEditTracker,
 		UserFactory $userFactory,
 		LoggerInterface $logger,
-		string $requestIP,
 		bool $filterIsCentral,
 		?string $centralDB
 	) {
@@ -68,7 +64,6 @@ class Throttle extends Consequence implements ConsequencesDisablerConsequence {
 		$this->userEditTracker = $userEditTracker;
 		$this->userFactory = $userFactory;
 		$this->logger = $logger;
-		$this->requestIP = $requestIP;
 		$this->filterIsCentral = $filterIsCentral;
 		$this->centralDB = $centralDB;
 	}
@@ -173,18 +168,18 @@ class Throttle extends Consequence implements ConsequencesDisablerConsequence {
 	 */
 	private function throttleIdentifier( string $type ): string {
 		$user = $this->parameters->getUser();
-		$title = Title::castFromLinkTarget( $this->parameters->getTarget() );
 		switch ( $type ) {
 			case 'ip':
-				$identifier = $this->requestIP;
+				$identifier = $this->parameters->getActionSpecifier()->getIP();
 				break;
 			case 'user':
 				// NOTE: This is always 0 for anons. Is this good/wanted?
 				$identifier = $user->getId();
 				break;
 			case 'range':
-				$range = IPUtils::isIPv6( $this->requestIP ) ? self::IPV6_RANGE : self::IPV4_RANGE;
-				$identifier = IPUtils::sanitizeRange( "{$this->requestIP}/$range" );
+				$requestIP = $this->parameters->getActionSpecifier()->getIP();
+				$range = IPUtils::isIPv6( $requestIP ) ? self::IPV6_RANGE : self::IPV4_RANGE;
+				$identifier = IPUtils::sanitizeRange( "{$requestIP}/$range" );
 				break;
 			case 'creationdate':
 				// TODO Inject a proper service, not UserFactory, once getRegistration is moved away from User
@@ -199,6 +194,7 @@ class Throttle extends Consequence implements ConsequencesDisablerConsequence {
 				$identifier = 1;
 				break;
 			case 'page':
+				$title = Title::castFromLinkTarget( $this->parameters->getTarget() );
 				// TODO Replace with something from LinkTarget, e.g. namespace + text
 				$identifier = $title->getPrefixedText();
 				break;

includes/Consequences/ConsequencesFactory.php

@@ -75,9 +75,6 @@ class ConsequencesFactory {
 	/** @var UserFactory */
 	private $userFactory;
 
-	/** @var string */
-	private $requestIP;
-
 	/**
 	 * @todo This might drag in unwanted dependencies. The alternative is to use ObjectFactory, but that's harder
 	 *   to understand for humans and static analysis tools, so do that only if the dependencies list starts growing.
@@ -94,7 +91,6 @@ class ConsequencesFactory {
 	 * @param MessageLocalizer $messageLocalizer
 	 * @param UserEditTracker $userEditTracker
 	 * @param UserFactory $userFactory
-	 * @param string $requestIP
 	 */
 	public function __construct(
 		ServiceOptions $options,
@@ -109,8 +105,7 @@ class ConsequencesFactory {
 		Session $session,
 		MessageLocalizer $messageLocalizer,
 		UserEditTracker $userEditTracker,
-		UserFactory $userFactory,
-		string $requestIP
+		UserFactory $userFactory
 	) {
 		$options->assertRequiredOptions( self::CONSTRUCTOR_OPTIONS );
 		$this->options = $options;
@@ -126,7 +121,6 @@ class ConsequencesFactory {
 		$this->messageLocalizer = $messageLocalizer;
 		$this->userEditTracker = $userEditTracker;
 		$this->userFactory = $userFactory;
-		$this->requestIP = $requestIP;
 	}
 
 	// Each class has its factory method for better type inference and static analysis
@@ -166,8 +160,7 @@ class ConsequencesFactory {
 			$this->messageLocalizer,
 			$this->logger,
 			$this->options->get( 'AbuseFilterRangeBlockSize' ),
-			$this->options->get( 'BlockCIDRLimit' ),
-			$this->requestIP
+			$this->options->get( 'BlockCIDRLimit' )
 		);
 	}
 
@@ -203,7 +196,6 @@ class ConsequencesFactory {
 			$this->userEditTracker,
 			$this->userFactory,
 			$this->logger,
-			$this->requestIP,
 			$this->options->get( 'AbuseFilterIsCentral' ),
 			$this->options->get( 'AbuseFilterCentralDB' )
 		);

includes/FilterRunner.php

@@ -237,7 +237,13 @@ class FilterRunner {
 			'accountname',
 			VariablesManager::GET_BC
 		)->toNative();
-		$spec = new ActionSpecifier( $this->action, $this->title, $this->user, $accountname );
+		$spec = new ActionSpecifier(
+			$this->action,
+			$this->title,
+			$this->user,
+			$this->user->getRequest()->getIP(),
+			$accountname
+		);
 
 		// Tag the action if the condition limit was hit
 		if ( $runnerData->getTotalConditions() > $this->options->get( 'AbuseFilterConditionLimit' ) ) {

includes/ServiceWiring.php

@@ -210,8 +210,7 @@ return [
 			// TODO: Use a proper MessageLocalizer once available (T247127)
 			RequestContext::getMain(),
 			$services->getUserEditTracker(),
-			$services->getUserFactory(),
-			RequestContext::getMain()->getRequest()->getIP()
+			$services->getUserFactory()
 		);
 	},
 	EditBoxBuilderFactory::SERVICE_NAME => static function ( MediaWikiServices $services ): EditBoxBuilderFactory {

includes/View/AbuseFilterViewRevert.php

@@ -271,6 +271,7 @@ class AbuseFilterViewRevert extends AbuseFilterView {
 			'afl_id',
 			'afl_user',
 			'afl_user_text',
+			'afl_ip',
 			'afl_action',
 			'afl_actions',
 			'afl_var_dump',
@@ -309,6 +310,7 @@ class AbuseFilterViewRevert extends AbuseFilterView {
 						$row->afl_action,
 						new TitleValue( (int)$row->afl_namespace, $row->afl_title ),
 						$this->userFactory->newFromAnyId( (int)$row->afl_user, $row->afl_user_text ),
+						$row->afl_ip,
 						$accountName
 					),
 					'timestamp' => $row->afl_timestamp

tests/phpunit/ConsequenceGetMessageTestTrait.php

@@ -34,6 +34,7 @@ trait ConsequenceGetMessageTestTrait {
 				'edit',
 				$this->createMock( LinkTarget::class ),
 				$user,
+				'1.2.3.4',
 				null
 			)
 		);
@@ -49,6 +50,7 @@ trait ConsequenceGetMessageTestTrait {
 				'edit',
 				$this->createMock( LinkTarget::class ),
 				$user,
+				'1.2.3.4',
 				null
 			)
 		);

tests/phpunit/unit/ActionSpecifierTest.php

@@ -19,17 +19,20 @@ class ActionSpecifierTest extends MediaWikiUnitTestCase {
 	 * @covers ::getAction
 	 * @covers ::getTitle
 	 * @covers ::getUser
+	 * @covers ::getIP
 	 * @covers ::getAccountName
 	 */
 	public function testGetters() {
 		$action = 'edit';
 		$title = new TitleValue( NS_MAIN, 'Foobar' );
 		$user = new UserIdentityValue( 42, 'John Doe' );
+		$ip = '127.0.0.1';
 		$accountname = 'foobar';
-		$spec = new ActionSpecifier( $action, $title, $user, $accountname );
+		$spec = new ActionSpecifier( $action, $title, $user, $ip, $accountname );
 		$this->assertSame( $action, $spec->getAction(), 'action' );
 		$this->assertSame( $title, $spec->getTitle(), 'title' );
 		$this->assertSame( $user, $spec->getUser(), 'user' );
+		$this->assertSame( $ip, $spec->getIP(), 'IP' );
 		$this->assertSame( $accountname, $spec->getAccountName(), 'accountname' );
 	}
 
@@ -42,6 +45,7 @@ class ActionSpecifierTest extends MediaWikiUnitTestCase {
 			'createaccount',
 			$this->createMock( LinkTarget::class ),
 			$this->createMock( UserIdentity::class ),
+			'127.0.0.1',
 			null
 		);
 	}

tests/phpunit/unit/ChangeTags/ChangeTaggerTest.php

@@ -46,17 +46,19 @@ class ChangeTaggerTest extends MediaWikiUnitTestCase {
 			'rc_namespace' => NS_MAIN,
 			'rc_title' => $titleText,
 			'rc_user' => 42,
-			'rc_user_text' => $userName
+			'rc_user_text' => $userName,
+			'rc_ip' => '127.0.0.1',
 		];
 		$specifierFromArray = static function ( array $specs ): ActionSpecifier {
 			return new ActionSpecifier(
 				$specs['action'],
 				$specs['target'],
 				new UserIdentityValue( 42, $specs['username'] ),
+				$specs['ip'],
 				$specs['accountname'] ?? null
 			);
 		};
-		$baseSpecs = [ 'username' => $userName, 'target' => $title ];
+		$baseSpecs = [ 'username' => $userName, 'target' => $title, 'ip' => '127.0.0.1' ];
 
 		$rcAttribs = [ 'rc_log_type' => null ] + $baseAttribs;
 		yield 'edit' => [

tests/phpunit/unit/Consequences/Consequence/RangeBlockTest.php

@@ -5,8 +5,10 @@ namespace MediaWiki\Extension\AbuseFilter\Tests\Unit\Consequences\Consequence;
 use ConsequenceGetMessageTestTrait;
 use MediaWiki\Block\BlockUser;
 use MediaWiki\Block\BlockUserFactory;
+use MediaWiki\Extension\AbuseFilter\ActionSpecifier;
 use MediaWiki\Extension\AbuseFilter\Consequences\Consequence\RangeBlock;
 use MediaWiki\Extension\AbuseFilter\Consequences\Parameters;
+use MediaWiki\Extension\AbuseFilter\Filter\ExistingFilter;
 use MediaWiki\Extension\AbuseFilter\FilterUser;
 use MediaWikiUnitTestCase;
 use MessageLocalizer;
@@ -87,7 +89,13 @@ class RangeBlockTest extends MediaWikiUnitTestCase {
 	public function testExecute(
 		string $requestIP, array $rangeBlockSize, string $target, bool $result
 	) {
-		$params = $this->provideGetMessageParameters()->current()[0];
+		$specifier = $this->createMock( ActionSpecifier::class );
+		$specifier->method( 'getIP' )->willReturn( $requestIP );
+		$params = new Parameters(
+			$this->createMock( ExistingFilter::class ),
+			false,
+			$specifier
+		);
 		$blockUser = $this->createMock( BlockUser::class );
 		$blockUser->expects( $this->once() )
 			->method( 'placeBlockUnsafe' )
@@ -112,8 +120,7 @@ class RangeBlockTest extends MediaWikiUnitTestCase {
 			$this->getMsgLocalizer(),
 			new NullLogger(),
 			$rangeBlockSize,
-			self::CIDR_LIMIT,
-			$requestIP
+			self::CIDR_LIMIT
 		);
 		$this->assertSame( $result, $rangeBlock->execute() );
 	}
@@ -131,8 +138,7 @@ class RangeBlockTest extends MediaWikiUnitTestCase {
 			$this->getMsgLocalizer(),
 			new NullLogger(),
 			[ 'IPv6' => 24, 'IPv4' => 24 ],
-			self::CIDR_LIMIT,
-			'1.1.1.1'
+			self::CIDR_LIMIT
 		);
 		$this->doTestGetMessage( $rangeBlock, $params, 'abusefilter-blocked-display' );
 	}

tests/phpunit/unit/Consequences/Consequence/ThrottleTest.php

@@ -6,11 +6,15 @@ use BagOStuff;
 use Generator;
 use HashBagOStuff;
 use InvalidArgumentException;
+use MediaWiki\Extension\AbuseFilter\ActionSpecifier;
 use MediaWiki\Extension\AbuseFilter\Consequences\Consequence\Throttle;
 use MediaWiki\Extension\AbuseFilter\Consequences\ConsequenceNotPrecheckedException;
 use MediaWiki\Extension\AbuseFilter\Consequences\Parameters;
+use MediaWiki\Extension\AbuseFilter\Filter\ExistingFilter;
+use MediaWiki\Linker\LinkTarget;
 use MediaWiki\User\UserEditTracker;
 use MediaWiki\User\UserFactory;
+use MediaWiki\User\UserIdentity;
 use MediaWikiUnitTestCase;
 use PHPUnit\Framework\MockObject\MockObject;
 use Psr\Log\NullLogger;
@@ -33,14 +37,18 @@ class ThrottleTest extends MediaWikiUnitTestCase {
 		UserEditTracker $editTracker = null,
 		string $ip = null
 	) {
-		$params = $this->createMock( Parameters::class );
-		$params->method( 'getIsGlobalFilter' )->willReturn( $globalFilter );
-		if ( $user ) {
-			$params->method( 'getUser' )->willReturn( $user );
-		}
-		if ( $title ) {
-			$params->method( 'getTarget' )->willReturn( $title );
-		}
+		$specifier = new ActionSpecifier(
+			'some-action',
+			$title ?? $this->createMock( LinkTarget::class ),
+			$user ?? $this->createMock( UserIdentity::class ),
+			$ip ?? '1.2.3.4',
+			null
+		);
+		$params = new Parameters(
+			$this->createMock( ExistingFilter::class ),
+			$globalFilter,
+			$specifier
+		);
 		return new Throttle(
 			$params,
 			$throttleParams + [ 'groups' => [ 'user' ], 'count' => 3, 'period' => 60, 'id' => 1 ],
@@ -48,7 +56,6 @@ class ThrottleTest extends MediaWikiUnitTestCase {
 			$editTracker ?? $this->createMock( UserEditTracker::class ),
 			$this->createMock( UserFactory::class ),
 			new NullLogger(),
-			$ip ?? '1.2.3.4',
 			false,
 			$globalFilter ? 'foo-db' : null
 		);

tests/phpunit/unit/Consequences/Consequence/WarnTest.php

@@ -40,6 +40,7 @@ class WarnTest extends MediaWikiUnitTestCase {
 				'edit',
 				new TitleValue( NS_HELP, 'Some title' ),
 				new UserIdentityValue( 1, 'Warned user' ),
+				'1.2.3.4',
 				null
 			)
 		);

tests/phpunit/unit/Consequences/ConsequencesExecutorTest.php

@@ -92,6 +92,7 @@ class ConsequencesExecutorTest extends MediaWikiUnitTestCase {
 				'edit',
 				$this->createMock( LinkTarget::class ),
 				$this->createMock( UserIdentity::class ),
+				'1.2.3.4',
 				null
 			),
 			new VariableHolder

tests/phpunit/unit/Consequences/ConsequencesFactoryTest.php

@@ -51,8 +51,7 @@ class ConsequencesFactoryTest extends MediaWikiUnitTestCase {
 			$this->createMock( Session::class ),
 			$this->createMock( MessageLocalizer::class ),
 			$this->createMock( UserEditTracker::class ),
-			$this->createMock( UserFactory::class ),
-			'1.2.3.4'
+			$this->createMock( UserFactory::class )
 		);
 	}
 

tests/phpunit/unit/Consequences/ParametersTest.php

@@ -30,7 +30,7 @@ class ParametersTest extends MediaWikiUnitTestCase {
 		$user = $this->createMock( UserIdentity::class );
 		$target = $this->createMock( LinkTarget::class );
 		$action = 'some-action';
-		$specifier = new ActionSpecifier( $action, $target, $user, null );
+		$specifier = new ActionSpecifier( $action, $target, $user, '1.2.3.4', null );
 		$params = new Parameters( $filter, $global, $specifier );
 
 		$this->assertSame( $filter, $params->getFilter(), 'filter' );

tests/phpunit/unit/FilterRunnerTest.php

@@ -21,6 +21,7 @@ use MediaWikiUnitTestCase;
 use Psr\Log\NullLogger;
 use Title;
 use User;
+use WebRequest;
 
 /**
  * @group Test
@@ -57,6 +58,10 @@ class FilterRunnerTest extends MediaWikiUnitTestCase {
 			$cache = $this->createMock( EditStashCache::class );
 			$cache->method( 'seek' )->willReturn( false );
 		}
+		$request = $this->createMock( WebRequest::class );
+		$request->method( 'getIP' )->willReturn( '127.0.0.1' );
+		$user = $this->createMock( User::class );
+		$user->method( 'getRequest' )->willReturn( $request );
 		return new FilterRunner(
 			new AbuseFilterHookRunner( $this->createHookContainer() ),
 			$this->createMock( FilterProfiler::class ),
@@ -72,7 +77,7 @@ class FilterRunnerTest extends MediaWikiUnitTestCase {
 			$cache,
 			new NullLogger(),
 			$opts,
-			$this->createMock( User::class ),
+			$user,
 			$this->createMock( Title::class ),
 			$vars ?? VariableHolder::newFromArray( [ 'action' => 'edit' ] ),
 			$group