diff --git a/includes/ActionSpecifier.php b/includes/ActionSpecifier.php index 098a826ec..83080bc92 100644 --- a/includes/ActionSpecifier.php +++ b/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 */ diff --git a/includes/ChangeTags/ChangeTagger.php b/includes/ChangeTags/ChangeTagger.php index 3049d204a..0f314907c 100644 --- a/includes/ChangeTags/ChangeTagger.php +++ b/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 ); } /** diff --git a/includes/Consequences/Consequence/RangeBlock.php b/includes/Consequences/Consequence/RangeBlock.php index 0af300bae..c15b02a76 100644 --- a/includes/Consequences/Consequence/RangeBlock.php +++ b/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( diff --git a/includes/Consequences/Consequence/Throttle.php b/includes/Consequences/Consequence/Throttle.php index 115bfd7f5..5bda032cb 100644 --- a/includes/Consequences/Consequence/Throttle.php +++ b/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; diff --git a/includes/Consequences/ConsequencesFactory.php b/includes/Consequences/ConsequencesFactory.php index 119845016..78c9964a5 100644 --- a/includes/Consequences/ConsequencesFactory.php +++ b/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' ) ); diff --git a/includes/FilterRunner.php b/includes/FilterRunner.php index 4176f71b9..f0c0f9541 100644 --- a/includes/FilterRunner.php +++ b/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' ) ) { diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index 7a908e013..fca5936bc 100644 --- a/includes/ServiceWiring.php +++ b/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 { diff --git a/includes/View/AbuseFilterViewRevert.php b/includes/View/AbuseFilterViewRevert.php index 149cf7484..ff64ce9dc 100644 --- a/includes/View/AbuseFilterViewRevert.php +++ b/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 diff --git a/tests/phpunit/ConsequenceGetMessageTestTrait.php b/tests/phpunit/ConsequenceGetMessageTestTrait.php index 9b3c9f5aa..a6ecc526d 100644 --- a/tests/phpunit/ConsequenceGetMessageTestTrait.php +++ b/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 ) ); diff --git a/tests/phpunit/unit/ActionSpecifierTest.php b/tests/phpunit/unit/ActionSpecifierTest.php index 54c075d68..1150dce32 100644 --- a/tests/phpunit/unit/ActionSpecifierTest.php +++ b/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 ); } diff --git a/tests/phpunit/unit/ChangeTags/ChangeTaggerTest.php b/tests/phpunit/unit/ChangeTags/ChangeTaggerTest.php index 378490b47..35afd3115 100644 --- a/tests/phpunit/unit/ChangeTags/ChangeTaggerTest.php +++ b/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' => [ diff --git a/tests/phpunit/unit/Consequences/Consequence/RangeBlockTest.php b/tests/phpunit/unit/Consequences/Consequence/RangeBlockTest.php index 9bb9cd76d..e85de3c79 100644 --- a/tests/phpunit/unit/Consequences/Consequence/RangeBlockTest.php +++ b/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' ); } diff --git a/tests/phpunit/unit/Consequences/Consequence/ThrottleTest.php b/tests/phpunit/unit/Consequences/Consequence/ThrottleTest.php index 2091d19cb..269c7221c 100644 --- a/tests/phpunit/unit/Consequences/Consequence/ThrottleTest.php +++ b/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 ); diff --git a/tests/phpunit/unit/Consequences/Consequence/WarnTest.php b/tests/phpunit/unit/Consequences/Consequence/WarnTest.php index 882dbe1b3..510febf70 100644 --- a/tests/phpunit/unit/Consequences/Consequence/WarnTest.php +++ b/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 ) ); diff --git a/tests/phpunit/unit/Consequences/ConsequencesExecutorTest.php b/tests/phpunit/unit/Consequences/ConsequencesExecutorTest.php index 2d368925e..3189b04ca 100644 --- a/tests/phpunit/unit/Consequences/ConsequencesExecutorTest.php +++ b/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 diff --git a/tests/phpunit/unit/Consequences/ConsequencesFactoryTest.php b/tests/phpunit/unit/Consequences/ConsequencesFactoryTest.php index ab5966f2e..e27c48ce8 100644 --- a/tests/phpunit/unit/Consequences/ConsequencesFactoryTest.php +++ b/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 ) ); } diff --git a/tests/phpunit/unit/Consequences/ParametersTest.php b/tests/phpunit/unit/Consequences/ParametersTest.php index ee4a143f1..2398f7919 100644 --- a/tests/phpunit/unit/Consequences/ParametersTest.php +++ b/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' ); diff --git a/tests/phpunit/unit/FilterRunnerTest.php b/tests/phpunit/unit/FilterRunnerTest.php index a06bffb47..f20330956 100644 --- a/tests/phpunit/unit/FilterRunnerTest.php +++ b/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