Add user_unnamed_ip variable

After temporary accounts are enabled, filters that rely on an ip
in the `user_name` will fail (eg. `ip_in_range` and `ip_in_ranges`).
To keep these filters working:

- Expose the IP through another variable, `user_unnamed_ip`, that can be
  used instead of `user_name`.
- The variable is scoped to only reveal the IPs of temporary accounts
  and un-logged in users.
- Wikis that don't have temporary accounts enabled will be able to see
  this variable but it won't provide information that `user_name`
  wasn't already providing
- Introduce the concept of transforming variable values before writing
  to the blob store and after retrieval, as IPs need to be deleted from
  the logs eventually and can't be stored as-is in the amend-only blob
  store

Bug: T357772
Change-Id: I8c11e06ccb9e78b9a991e033fe43f5dded8f7bb2

i18n/en.json

@@ -386,6 +386,7 @@
 	"abusefilter-edit-builder-vars-movedto-last-edit-age": "Time since last move destination page edit in seconds ($1)",
 	"abusefilter-edit-builder-vars-user-editcount": "Edit count of the user ($1)",
 	"abusefilter-edit-builder-vars-user-age": "Age of the user account ($1)",
+	"abusefilter-edit-builder-vars-user-unnamed-ip": "IP of the user account (for logged-out users and temporary accounts only) ($1)",
 	"abusefilter-edit-builder-vars-user-name": "Name of the user account ($1)",
 	"abusefilter-edit-builder-vars-user-type": "Type of the user account ($1)",
 	"abusefilter-edit-builder-vars-user-groups": "Groups (including implicit) the user is in ($1)",

i18n/qqq.json

@@ -431,6 +431,7 @@
 	"abusefilter-edit-builder-vars-movedto-last-edit-age": "Paraphrase: The number of seconds since the last revision was made on the destination of the page that is to be moved. Returns zero when page is non-existent. Abuse filter syntax option in a dropdown from the group {{msg-mw|abusefilter-edit-builder-group-vars}}.\n\nParameters:\n* $1 - text that will be inserted into the filter code editor",
 	"abusefilter-edit-builder-vars-user-editcount": "Abuse filter syntax option in a dropdown from the group {{msg-mw|abusefilter-edit-builder-group-vars}}.\n\nParameters:\n* $1 - text that will be inserted into the filter code editor",
 	"abusefilter-edit-builder-vars-user-age": "Abuse filter syntax option in a dropdown from the group {{msg-mw|abusefilter-edit-builder-group-vars}}.\n\nParameters:\n* $1 - text that will be inserted into the filter code editor",
+	"abusefilter-edit-builder-vars-user-unnamed-ip": "Abuse filter syntax option in a dropdown from the group {{msg-mw|abusefilter-edit-builder-group-vars}}.\n\nParameters:\n* $1 - text that will be inserted into the filter code editor",
 	"abusefilter-edit-builder-vars-user-name": "Abuse filter syntax option in a dropdown from the group {{msg-mw|abusefilter-edit-builder-group-vars}}.\n\nParameters:\n* $1 - text that will be inserted into the filter code editor",
 	"abusefilter-edit-builder-vars-user-type": "Abuse filter syntax option in a dropdown from the group {{msg-mw|abusefilter-edit-builder-group-vars}}.\n\nParameters:\n* $1 - text that will be inserted into the filter code editor",
 	"abusefilter-edit-builder-vars-user-groups": "Parameters:\n* $1 - text that will be inserted into the filter code editor\n\nSee also:\n* {{msg-mw|Abusefilter-edit-builder-vars-global-user-groups}}",

includes/Api/CheckMatch.php

@@ -120,7 +120,7 @@ class CheckMatch extends ApiBase {
 				$this->dieWithError( 'apierror-permissiondenied-generic', 'deletedabuselog' );
 			}
 
-			$vars = $this->afVariablesBlobStore->loadVarDump( $row->afl_var_dump );
+			$vars = $this->afVariablesBlobStore->loadVarDump( $row );
 		}
 		if ( $vars === null ) {
 			// @codeCoverageIgnoreStart

includes/Api/QueryAbuseLog.php

@@ -165,6 +165,7 @@ class QueryAbuseLog extends ApiQueryBase {
 		$this->addFields( 'afl_deleted' );
 		$this->addFields( 'afl_filter_id' );
 		$this->addFields( 'afl_global' );
+		$this->addFields( 'afl_ip' );
 		$this->addFieldsIf( 'afl_id', $fld_ids );
 		$this->addFieldsIf( 'afl_user_text', $fld_user );
 		$this->addFieldsIf( [ 'afl_namespace', 'afl_title' ], $fld_title );
@@ -319,7 +320,7 @@ class QueryAbuseLog extends ApiQueryBase {
 			if ( $fld_details ) {
 				$entry['details'] = [];
 				if ( $canSeeDetails ) {
-					$vars = $this->afVariablesBlobStore->loadVarDump( $row->afl_var_dump );
+					$vars = $this->afVariablesBlobStore->loadVarDump( $row );
 					$varManager = $this->afVariablesManager;
 					$entry['details'] = $varManager->exportAllVars( $vars );
 				}

includes/KeywordsManager.php

@@ -202,6 +202,8 @@ class KeywordsManager {
 			'user_editcount' => 'user-editcount',
 			// Generates abusefilter-edit-builder-vars-user-age
 			'user_age' => 'user-age',
+			// Generates abusefilter-edit-builder-vars-user-unnamed-ip
+			'user_unnamed_ip' => 'user-unnamed-ip',
 			// Generates abusefilter-edit-builder-vars-user-name
 			'user_name' => 'user-name',
 			// Generates abusefilter-edit-builder-vars-user-type

includes/Pager/AbuseLogPager.php

@@ -347,6 +347,7 @@ class AbuseLogPager extends ReverseChronologicalPager {
 				'afl_global',
 				'afl_filter_id',
 				'afl_user',
+				'afl_ip',
 				'afl_user_text',
 				'afl_action',
 				'afl_actions',

includes/Special/SpecialAbuseLog.php

@@ -758,7 +758,8 @@ class SpecialAbuseLog extends AbuseFilterSpecialPage {
 		$output .= Xml::tags( 'p', null, $pager->doFormatRow( $row, false ) );
 
 		// Load data
-		$vars = $this->varBlobStore->loadVarDump( $row->afl_var_dump );
+		$vars = $this->varBlobStore->loadVarDump( $row );
+
 		$out->addJsConfigVars( 'wgAbuseFilterVariables', $this->varManager->dumpAllVars( $vars, true ) );
 		$out->addModuleStyles( 'mediawiki.interface.helpers.styles' );
 

includes/VariableGenerator/VariableGenerator.php

@@ -88,6 +88,15 @@ class VariableGenerator {
 
 		$this->vars->setVar( 'user_name', $user->getName() );
 
+		$this->vars->setLazyLoadVar(
+			'user_unnamed_ip',
+			'user-unnamed-ip',
+			[
+				'user' => $user,
+				'rc' => $rc,
+			]
+		);
+
 		$this->vars->setLazyLoadVar(
 			'user_type',
 			'user-type',

includes/Variables/LazyVariableComputer.php

@@ -345,6 +345,27 @@ class LazyVariableComputer {
 				$title = $parameters['title'];
 				$result = $this->restrictionStore->getRestrictions( $title, $action );
 				break;
+			case 'user-unnamed-ip':
+				$user = $parameters['user'];
+				$result = null;
+
+				// Don't return an IP for past events (eg. revisions, logs)
+				// This could leak IPs to users who don't have IP viewing rights
+				if ( !$parameters['rc'] &&
+					// Reveal IPs for:
+					// - temporary accounts: temporary account names will replace the IP in the `user_name`
+					//   variable. This variable restores this access.
+					// - logged-out users: This supports the transition to the use of temporary accounts
+					//   so that filter maintainers on pre-transition wikis can migrate `user_name` to `user_unnamed_ip`
+					//   where necessary and see no disruption on transition.
+					//
+					// This variable should only ever be exposed for these use cases and shouldn't be extended
+					// to registered accounts, as that would leak account PII to users without the right to see
+					// that information
+					( $this->userIdentityUtils->isTemp( $user ) || IPUtils::isIPAddress( $user->getName() ) ) ) {
+					$result = $user->getRequest()->getIP();
+				}
+				break;
 			case 'user-type':
 				/** @var UserIdentity $userIdentity */
 				$userIdentity = $parameters['user-identity'];

includes/Variables/VariablesBlobStore.php

@@ -3,9 +3,11 @@
 namespace MediaWiki\Extension\AbuseFilter\Variables;
 
 use FormatJson;
+use InvalidArgumentException;
 use MediaWiki\Storage\BlobAccessException;
 use MediaWiki\Storage\BlobStore;
 use MediaWiki\Storage\BlobStoreFactory;
+use stdClass;
 
 /**
  * This service is used to store and load var dumps to a BlobStore
@@ -56,6 +58,14 @@ class VariablesBlobStore {
 		// as those are needed for the diff view on top of the abuse log pages
 		$vars = $this->varManager->dumpAllVars( $varsHolder, [ 'old_wikitext', 'new_wikitext' ] );
 
+		// if user_unnamed_ip exists it can't be saved, as var dump blobs are stored in an append-only
+		// database and stored IPs eventually need to be cleared.
+		// Set the value to something safe here, as by now it's been used in the filter and if
+		// logs later need it, it can be reconstructed from afl_ip.
+		if ( isset( $vars[ 'user_unnamed_ip' ] ) && $vars[ 'user_unnamed_ip' ] ) {
+			$vars[ 'user_unnamed_ip' ] = true;
+		}
+
 		// Vars is an array with native PHP data types (non-objects) now
 		$text = FormatJson::encode( $vars );
 
@@ -72,13 +82,22 @@ class VariablesBlobStore {
 	/**
 	 * Retrieve a var dump from a BlobStore.
 	 *
-	 * @param string $address
+	 * The entire $row is passed through but only the following columns are actually required:
+	 * - afl_var_dump: the main variable store to load
+	 * - afl_ip: the IP value to use if necessary
+	 *
+	 * @param stdClass $row
 	 *
 	 * @return VariableHolder
 	 */
-	public function loadVarDump( string $address ): VariableHolder {
+	public function loadVarDump( stdClass $row ): VariableHolder {
+		if ( !isset( $row->afl_var_dump ) || !isset( $row->afl_ip ) ) {
+			throw new InvalidArgumentException( 'Both afl_var_dump and afl_ip must be set' );
+		}
+
 		try {
-			$blob = $this->blobStore->getBlob( $address );
+			$varDump = $row->afl_var_dump;
+			$blob = $this->blobStore->getBlob( $varDump );
 		} catch ( BlobAccessException $ex ) {
 			return new VariableHolder;
 		}
@@ -86,6 +105,17 @@ class VariablesBlobStore {
 		$vars = FormatJson::decode( $blob, true );
 		$obj = VariableHolder::newFromArray( $vars );
 		$this->varManager->translateDeprecatedVars( $obj );
+
+		// If user_unnamed_ip was set when afl_var_dump was saved, it was saved as a visibility boolean
+		// and needs to be translated back into an IP
+		// user_unnamed_ip uses afl_ip instead of saving the value because afl_ip gets purged and the blob
+		// that contains user_unnamed_ip can't be modified
+		if (
+			$this->varManager->getVar( $obj, 'user_unnamed_ip', $this->varManager::GET_LAX )->toNative()
+		) {
+			$obj->setVar( 'user_unnamed_ip', $row->afl_ip );
+		}
+
 		return $obj;
 	}
 }

includes/View/AbuseFilterViewExamine.php

@@ -248,6 +248,7 @@ class AbuseFilterViewExamine extends AbuseFilterView {
 		$row = $dbr->newSelectQueryBuilder()
 			->select( [
 				'afl_deleted',
+				'afl_ip',
 				'afl_var_dump',
 				'afl_rev_id',
 				'afl_filter_id',
@@ -287,7 +288,8 @@ class AbuseFilterViewExamine extends AbuseFilterView {
 			return;
 		}
 
-		$vars = $this->varBlobStore->loadVarDump( $row->afl_var_dump );
+		$vars = $this->varBlobStore->loadVarDump( $row );
+
 		$out->addJsConfigVars( [
 			'wgAbuseFilterVariables' => $this->varManager->dumpAllVars( $vars, true ),
 			'abuseFilterExamine' => [ 'type' => 'log', 'id' => $logid ]

includes/View/AbuseFilterViewRevert.php

@@ -305,7 +305,7 @@ class AbuseFilterViewRevert extends AbuseFilterView {
 			$actions = explode( ',', $row->afl_actions );
 			$currentReversibleActions = array_intersect( $actions, $reversibleActions );
 			if ( count( $currentReversibleActions ) ) {
-				$vars = $this->varBlobStore->loadVarDump( $row->afl_var_dump );
+				$vars = $this->varBlobStore->loadVarDump( $row );
 				try {
 					// The variable is not lazy-loaded
 					$accountName = $vars->getComputedVariable( 'accountname' )->toNative();

tests/phpunit/AbuseFilterConsequencesTest.php

@@ -1215,14 +1215,14 @@ class AbuseFilterConsequencesTest extends MediaWikiIntegrationTestCase {
 		$this->doAction( $actionParams );
 
 		// We just take a dump from a single filters, as they're all identical for the same action
-		$dumpID = $this->getDb()->newSelectQueryBuilder()
-			->select( 'afl_var_dump' )
+		$row = $this->getDb()->newSelectQueryBuilder()
+			->select( 'afl_var_dump, afl_ip' )
 			->from( 'abuse_filter_log' )
 			->orderBy( 'afl_timestamp', SelectQueryBuilder::SORT_DESC )
 			->caller( __METHOD__ )
-			->fetchField();
+			->fetchRow();
 
-		$vars = AbuseFilterServices::getVariablesBlobStore()->loadVarDump( $dumpID )->getVars();
+		$vars = AbuseFilterServices::getVariablesBlobStore()->loadVarDump( $row )->getVars();
 
 		$interestingVars = array_intersect_key( $vars, array_fill_keys( $usedVars, true ) );
 

tests/phpunit/unit/VariableGenerator/VariableGeneratorTest.php

@@ -48,7 +48,8 @@ class VariableGeneratorTest extends MediaWikiUnitTestCase {
 			'user_groups',
 			'user_rights',
 			'user_blocked',
-			'user_age'
+			'user_age',
+			'user_unnamed_ip',
 		];
 
 		$variableHolder = new VariableHolder();

tests/phpunit/unit/Variables/LazyVariableComputerTest.php

@@ -14,6 +14,7 @@ use MediaWiki\Extension\AbuseFilter\Variables\LazyVariableComputer;
 use MediaWiki\Extension\AbuseFilter\Variables\VariableHolder;
 use MediaWiki\Permissions\PermissionManager;
 use MediaWiki\Permissions\RestrictionStore;
+use MediaWiki\Request\WebRequest;
 use MediaWiki\Revision\RevisionLookup;
 use MediaWiki\Revision\RevisionRecord;
 use MediaWiki\Revision\RevisionStore;
@@ -150,7 +151,7 @@ class LazyVariableComputerTest extends MediaWikiUnitTestCase {
 		$getUserVar = static function ( $user, $method ): LazyLoadedVariable {
 			return new LazyLoadedVariable(
 				$method,
-				[ 'user' => $user, 'user-identity' => $user ]
+				[ 'user' => $user, 'user-identity' => $user, 'rc' => null ]
 			);
 		};
 
@@ -195,6 +196,30 @@ class LazyVariableComputerTest extends MediaWikiUnitTestCase {
 		$var = $getUserVar( $user, 'user-type' );
 		yield 'user_type for unregistered username' => [ $var, 'unknown' ];
 
+		$request = $this->createMock( WebRequest::class );
+		$request->method( 'getIP' )->willReturn( '127.0.0.1' );
+		$user = $this->createMock( User::class );
+		$user->method( 'getRequest' )->willReturn( $request );
+		$user->method( 'getName' )->willReturn( '127.0.0.1' );
+		$var = $getUserVar( $user, 'user-unnamed-ip' );
+		yield 'user_unnamed_ip for an anonymous user' => [ $var, '127.0.0.1' ];
+
+		$user = $this->createMock( User::class );
+		$user->method( 'getName' )->willReturn( 'Test User' );
+		$var = $getUserVar( $user, 'user-unnamed-ip' );
+		yield 'user_unnamed_ip for a user' => [ $var, null ];
+
+		$mockUserIdentityUtils = $this->createMock( UserIdentityUtils::class );
+		$mockUserIdentityUtils->method( 'isTemp' )->with( $user )->willReturn( true );
+		$request = $this->createMock( WebRequest::class );
+		$request->method( 'getIP' )->willReturn( '127.0.0.1' );
+		$user = $this->createMock( User::class );
+		$user->method( 'getRequest' )->willReturn( $request );
+		$var = $getUserVar( $user, 'user-unnamed-ip' );
+		yield 'user_unnamed_ip for a temp user' => [
+			$var, '127.0.0.1', [ 'UserIdentityUtils' => $mockUserIdentityUtils ]
+		];
+
 		$user = $this->createMock( User::class );
 		$groups = [ '*', 'group1', 'group2' ];
 		$userGroupManager = $this->createMock( UserGroupManager::class );

tests/phpunit/unit/Variables/VariablesBlobStoreTest.php

@@ -3,6 +3,7 @@
 namespace MediaWiki\Extension\AbuseFilter\Tests\Unit\Variables;
 
 use FormatJson;
+use InvalidArgumentException;
 use MediaWiki\Extension\AbuseFilter\KeywordsManager;
 use MediaWiki\Extension\AbuseFilter\Parser\AFPData;
 use MediaWiki\Extension\AbuseFilter\Variables\VariableHolder;
@@ -12,6 +13,7 @@ use MediaWiki\Storage\BlobAccessException;
 use MediaWiki\Storage\BlobStore;
 use MediaWiki\Storage\BlobStoreFactory;
 use MediaWikiUnitTestCase;
+use stdClass;
 use Wikimedia\TestingAccessWrapper;
 
 /**
@@ -67,17 +69,86 @@ class VariablesBlobStoreTest extends MediaWikiUnitTestCase {
 		$blob = FormatJson::encode( $vars );
 		$blobStore = $this->createMock( BlobStore::class );
 		$blobStore->expects( $this->once() )->method( 'getBlob' )->willReturn( $blob );
+
+		$row = new stdClass;
+		$row->afl_var_dump = $blob;
+		$row->afl_ip = '';
 		$varBlobStore = $this->getStore( null, $blobStore );
-		$loadedVars = $varBlobStore->loadVarDump( 'foo' )->getVars();
+		$loadedVars = $varBlobStore->loadVarDump( $row )->getVars();
 		$this->assertArrayHasKey( 'foo-variable', $loadedVars );
 		$this->assertSame( 42, $loadedVars['foo-variable']->toNative() );
 	}
 
+	/**
+	 * @dataProvider provideLoadVarDumpVarTransformation
+	 */
+	public function testLoadVarDumpVarTransformation( $data, $expected ) {
+		$vars = [
+			'user_unnamed_ip' => $data[ 'user_unnamed_ip' ]
+		];
+
+		$manager = $this->createMock( VariablesManager::class );
+		$manager->method( 'getVar' )->willReturn( AFPData::newFromPHPVar( $data['user_unnamed_ip'] ) );
+		$blob = FormatJson::encode( $vars );
+		$blobStore = $this->createMock( BlobStore::class );
+		$blobStore->expects( $this->once() )->method( 'getBlob' )->willReturn( $blob );
+		$row = new stdClass;
+		$row->afl_var_dump = $blob;
+		$row->afl_ip = $data[ 'afl_ip' ];
+		$varBlobStore = new VariablesBlobStore(
+			$manager,
+			$this->createMock( BlobStoreFactory::class ),
+			$blobStore,
+			null
+		);
+		$loadedVars = $varBlobStore->loadVarDump( $row )->getVars();
+		$this->assertArrayHasKey( 'user_unnamed_ip', $loadedVars );
+		$this->assertSame( $expected, $loadedVars[ 'user_unnamed_ip' ]->toNative() );
+	}
+
+	/**
+	 * Data provider for testLoadVarDumpVarTransformation
+	 *
+	 * @return array
+	 */
+	public static function provideLoadVarDumpVarTransformation() {
+		return [
+			'ip visible, ip available' => [
+				[
+					'user_unnamed_ip' => true,
+					'afl_ip' => '1.2.3.4',
+				],
+				'1.2.3.4'
+			],
+			'ip visible, ip cleared' => [
+				[
+					'user_unnamed_ip' => true,
+					'afl_ip' => '',
+				],
+				''
+			],
+			'ip not visible, ip available' => [
+				[
+					'user_unnamed_ip' => null,
+					'afl_ip' => '1.2.3.4',
+				],
+				null
+			]
+		];
+	}
+
 	public function testLoadVarDump_fail() {
 		$blobStore = $this->createMock( BlobStore::class );
 		$blobStore->expects( $this->once() )->method( 'getBlob' )->willThrowException( new BlobAccessException );
 		$varBlobStore = $this->getStore( null, $blobStore );
-		$this->assertCount( 0, $varBlobStore->loadVarDump( 'foo' )->getVars() );
+		$row = new stdClass;
+		$row->afl_var_dump = '';
+		$row->afl_ip = '';
+		$this->assertCount( 0, $varBlobStore->loadVarDump( $row )->getVars() );
+
+		$row = new stdClass;
+		$this->expectException( InvalidArgumentException::class );
+		$varBlobStore->loadVarDump( $row )->getVars();
 	}
 
 	private function getBlobStore(): BlobStore {
@@ -117,7 +188,10 @@ class VariablesBlobStoreTest extends MediaWikiUnitTestCase {
 
 		$storeID = $varBlobStore->storeVarDump( VariableHolder::newFromArray( $toStore ) );
 		$this->assertIsString( $storeID );
-		$loadedVars = $varBlobStore->loadVarDump( $storeID )->getVars();
+		$row = new stdClass;
+		$row->afl_var_dump = $storeID;
+		$row->afl_ip = '';
+		$loadedVars = $varBlobStore->loadVarDump( $row )->getVars();
 		$nativeLoadedVars = array_map( static function ( AFPData $el ) {
 			return $el->toNative();
 		}, $loadedVars );
@@ -197,7 +271,15 @@ class VariablesBlobStoreTest extends MediaWikiUnitTestCase {
 					'action' => 'createaccount',
 					'accountname' => 'XXX'
 				]
-			]
+			],
+			'User IP' => [
+				[
+					'user_unnamed_ip' => '1.2.3.4'
+				],
+				[
+					'user_unnamed_ip' => true
+				]
+			],
 		];
 	}
 }