Miscellaneous minor fixes

- Rename `$hidden` to `$privacyLevel` in Flags::__construct for
  consistency with other places.
- Rename `shouldProtectFilter` and simplify its return value to always
  be an array, since that's how it's currently used. Rename a variable
  that is assigned the return value of this method.
- Add a missing message key to a list of dynamic message keys.
- Rename a property from 'hidden' to 'privacy' in FilterStoreTest for
  consistency. Add a test for removing the protected flag.
- Update old comment referencing `filterHidden`; the method was removed
  in I40b8c8452d9df.
- Use ISQLPlatform::bitAnd() instead of manual SQL in
  AbuseFilterHistoryPager.
- Update mysterious reference to "formatRow" in SpecialAbuseLog.
- Update other references to the very same method in two other places,
  this time credited as "SpecialAbuseLog".
- Add type hints to a few methods; this not only helps with type safety,
  but it also allows PHPUnit to automatically use the proper type in
  mocks.

Change-Id: Ib0167d993b761271c1e5311808435a616b6576fe

includes/AbuseFilterPermissionManager.php

@@ -103,29 +103,20 @@ class AbuseFilterPermissionManager {
 	}
 
 	/**
-	 * Check if the filter should be protected:
-	 * - Return false if it uses no protected variables
-	 * - Return true if it uses protected variables and the performer has view permissions
-	 * - Return an array of used protected variables if the performer doesn't have
-	 *   view permissions
+	 * Check if the filter uses variables that the user is not allowed to use (i.e., variables that are protected, if
+	 * the user can't view protected variables), and return them.
 	 *
 	 * @param Authority $performer
 	 * @param string[] $usedVariables
-	 * @return string[]|bool
+	 * @return string[]
 	 */
-	public function shouldProtectFilter( Authority $performer, $usedVariables ) {
+	public function getForbiddenVariables( Authority $performer, array $usedVariables ): array {
 		$usedProtectedVariables = array_intersect( $usedVariables, $this->protectedVariables );
-		// Protected variables aren't used
-		if ( count( $usedProtectedVariables ) === 0 ) {
-			return false;
-		} else {
-			// Check for permissions if they are
-			if ( $this->canViewProtectedVariables( $performer ) ) {
-				return true;
-			} else {
-				return $usedProtectedVariables;
-			}
+		// All good if protected variables aren't used, or the user can view them.
+		if ( count( $usedProtectedVariables ) === 0 || $this->canViewProtectedVariables( $performer ) ) {
+			return [];
 		}
+		return $usedProtectedVariables;
 	}
 
 	/**

includes/Api/QueryAbuseLog.php

@@ -147,7 +147,7 @@ class QueryAbuseLog extends ApiQueryBase {
 					try {
 						$privacyLevel = $lookup->getFilter( $filterID, $global )->getPrivacyLevel();
 					} catch ( CentralDBNotAvailableException $_ ) {
-						// Conservatively assume it's hidden and protected, like in SpecialAbuseLog
+						// Conservatively assume it's hidden and protected, like in AbuseLogPager::doFormatRow
 						$privacyLevel = Flags::FILTER_HIDDEN & Flags::FILTER_USES_PROTECTED_VARS;
 					} catch ( FilterNotFoundException $_ ) {
 						$privacyLevel = Flags::FILTER_PUBLIC;

includes/Filter/Flags.php

@@ -26,15 +26,15 @@ class Flags {
 	/**
 	 * @param bool $enabled
 	 * @param bool $deleted
-	 * @param int $hidden
+	 * @param int $privacyLevel
 	 * @param bool $global
 	 */
-	public function __construct( bool $enabled, bool $deleted, int $hidden, bool $global ) {
+	public function __construct( bool $enabled, bool $deleted, int $privacyLevel, bool $global ) {
 		$this->enabled = $enabled;
 		$this->deleted = $deleted;
-		$this->hidden = (bool)( self::FILTER_HIDDEN & $hidden );
-		$this->protected = (bool)( self::FILTER_USES_PROTECTED_VARS & $hidden );
-		$this->privacyLevel = $hidden;
+		$this->hidden = (bool)( self::FILTER_HIDDEN & $privacyLevel );
+		$this->protected = (bool)( self::FILTER_USES_PROTECTED_VARS & $privacyLevel );
+		$this->privacyLevel = $privacyLevel;
 		$this->global = $global;
 	}
 

includes/FilterValidator.php

@@ -380,7 +380,7 @@ class FilterValidator {
 		}
 
 		$ruleChecker = $this->ruleCheckerFactory->newRuleChecker();
-		$usedVariables = (array)$ruleChecker->getUsedVars( $filter->getRules() );
+		$usedVariables = $ruleChecker->getUsedVars( $filter->getRules() );
 		$usedProtectedVariables = array_intersect( $usedVariables, $this->protectedVariables );
 
 		if (
@@ -405,9 +405,9 @@ class FilterValidator {
 		$ret = Status::newGood();
 		$ruleChecker = $this->ruleCheckerFactory->newRuleChecker();
 		$usedVars = $ruleChecker->getUsedVars( $filter->getRules() );
-		$missingRights = $this->permManager->shouldProtectFilter( $performer, $usedVars );
-		if ( is_array( $missingRights ) ) {
-			$ret->error( 'abusefilter-edit-protected-variable', Message::listParam( $missingRights ) );
+		$forbiddenVariables = $this->permManager->getForbiddenVariables( $performer, $usedVars );
+		if ( $forbiddenVariables ) {
+			$ret->error( 'abusefilter-edit-protected-variable', Message::listParam( $forbiddenVariables ) );
 		}
 		return $ret;
 	}

includes/Pager/AbuseFilterHistoryPager.php

@@ -246,7 +246,7 @@ class AbuseFilterHistoryPager extends TablePager {
 
 		if ( !$this->canViewProtectedVars ) {
 			// Hide data the user can't see.
-			$info['conds'][] = 'af_hidden & ' . Flags::FILTER_USES_PROTECTED_VARS . ' = 0';
+			$info['conds'][] = $this->mDb->bitAnd( 'af_hidden', Flags::FILTER_USES_PROTECTED_VARS ) . ' = 0';
 		}
 
 		return $info;

includes/Parser/FilterEvaluator.php

@@ -452,7 +452,7 @@ class FilterEvaluator {
 	 * @param string $filter
 	 * @return array
 	 */
-	public function getUsedVars( $filter ) {
+	public function getUsedVars( string $filter ): array {
 		$this->resetState();
 		$cachedAllowShort = $this->mAllowShort;
 		try {

includes/Special/SpecialAbuseLog.php

@@ -745,7 +745,7 @@ class SpecialAbuseLog extends AbuseFilterSpecialPage {
 					$privacyLevel = AbuseFilterServices::getFilterLookup()->getFilter( $filterID, $global )
 						->getPrivacyLevel();
 				} catch ( CentralDBNotAvailableException $_ ) {
-					// Conservatively assume that it's hidden and protected, like in formatRow
+					// Conservatively assume that it's hidden and protected, like in AbuseLogPager::doFormatRow
 					$privacyLevel = Flags::FILTER_HIDDEN & Flags::FILTER_USES_PROTECTED_VARS;
 				}
 			} else {

includes/SpecsFormatter.php

@@ -156,6 +156,7 @@ class SpecsFormatter {
 			// * abusefilter-history-enabled
 			// * abusefilter-history-deleted
 			// * abusefilter-history-hidden
+			// * abusefilter-history-protected
 			// * abusefilter-history-global
 			$flagsDisplay[] = $this->messageLocalizer->msg( "abusefilter-history-$flag" )->escaped();
 		}

includes/View/AbuseFilterViewEdit.php

@@ -287,8 +287,8 @@ class AbuseFilterViewEdit extends AbuseFilterView {
 			$history_id
 		)->parse() );
 
-		// We use filterHidden() to ensure that if a public filter is made private, the public
-		// revision is also hidden.
+		// Grab the current hidden flag from the DB, in case we're editing an older, public revision of a filter that is
+		// currently hidden, so that we can also hide that public revision.
 		if (
 			( $filterObj->isHidden() || (
 				$filter !== null && $this->filterLookup->getFilter( $filter, false )->isHidden() )

includes/View/AbuseFilterViewExamine.php

@@ -268,7 +268,7 @@ class AbuseFilterViewExamine extends AbuseFilterView {
 		try {
 			$privacyLevel = $this->filterLookup->getFilter( $row->afl_filter_id, $row->afl_global )->getPrivacyLevel();
 		} catch ( CentralDBNotAvailableException $_ ) {
-			// Conservatively assume that it's hidden and protected, like in SpecialAbuseLog
+			// Conservatively assume that it's hidden and protected, like in AbuseLogPager::doFormatRow
 			$privacyLevel = Flags::FILTER_HIDDEN & Flags::FILTER_USES_PROTECTED_VARS;
 		}
 		if ( !$this->afPermManager->canSeeLogDetailsForFilter( $performer, $privacyLevel ) ) {

tests/phpunit/integration/FilterStoreTest.php

@@ -28,7 +28,7 @@ class FilterStoreTest extends MediaWikiIntegrationTestCase {
 		'enabled' => 1,
 		'comments' => '',
 		'name' => 'Mock filter',
-		'hidden' => 0,
+		'privacy' => Flags::FILTER_PUBLIC,
 		'hit_count' => 0,
 		'throttled' => 0,
 		'deleted' => 0,
@@ -78,7 +78,7 @@ class FilterStoreTest extends MediaWikiIntegrationTestCase {
 			new Flags(
 				$filterSpecs['enabled'],
 				$filterSpecs['deleted'],
-				$filterSpecs['hidden'],
+				$filterSpecs['privacy'],
 				$filterSpecs['global']
 			),
 			$actions,
@@ -187,7 +187,7 @@ class FilterStoreTest extends MediaWikiIntegrationTestCase {
 			'id' => '3',
 			'rules' => "ip_in_range( user_unnamed_ip, '1.2.3.4' )",
 			'name' => 'Mock filter with protected variable used',
-			'hidden' => Flags::FILTER_USES_PROTECTED_VARS,
+			'privacy' => Flags::FILTER_USES_PROTECTED_VARS,
 		];
 		$newFilter = $this->getFilterFromSpecs( $row );
 		$status = AbuseFilterServices::getFilterStore()->saveFilter(
@@ -195,4 +195,28 @@ class FilterStoreTest extends MediaWikiIntegrationTestCase {
 		);
 		$this->assertStatusGood( $status );
 	}
+
+	public function testSaveFilter__cannotRemoveProtectedFlag() {
+		$row = [
+			'id' => null,
+			'rules' => "ip_in_range( user_unnamed_ip, '1.2.3.4' )",
+			'name' => 'Uses protected variable',
+			'enabled' => true,
+			'privacy' => Flags::FILTER_USES_PROTECTED_VARS,
+		];
+
+		$origFilter = $this->getFilterFromSpecs( $row );
+		$newFilter = MutableFilter::newFromParentFilter( $origFilter );
+		$newFilter->setRules( '1 + 1 === 2' );
+		$newFilter->setProtected( false );
+
+		$status = AbuseFilterServices::getFilterStore()->saveFilter(
+			$this->getTestSysop()->getUser(), null, $newFilter, $origFilter
+		);
+
+		$this->assertStatusGood( $status );
+		$filterID = $status->getValue()[0];
+		$storedFilter = AbuseFilterServices::getFilterLookup()->getFilter( $filterID, false );
+		$this->assertTrue( $storedFilter->isProtected() );
+	}
 }

tests/phpunit/unit/AbuseFilterPermissionManagerTest.php

@@ -219,7 +219,7 @@ class AbuseFilterPermissionManagerTest extends MediaWikiUnitTestCase {
 		);
 	}
 
-	public static function provideShouldProtectFilter(): Generator {
+	public static function provideGetForbiddenVariables(): Generator {
 		yield 'cannot view, protected vars' => [
 			[
 				'rights' => [],
@@ -232,32 +232,32 @@ class AbuseFilterPermissionManagerTest extends MediaWikiUnitTestCase {
 				'rights' => [],
 				'usedVars' => []
 			],
-			false
+			[]
 		];
 		yield 'can view, protected vars' => [
 			[
 				'rights' => [ 'abusefilter-access-protected-vars' ],
 				'usedVars' => [ 'user_unnamed_ip' ]
 			],
-			true
+			[]
 		];
 		yield 'can view, no protected vars' => [
 			[
 				'rights' => [ 'abusefilter-access-protected-vars' ],
 				'usedVars' => []
 			],
-			false
+			[]
 		];
 	}
 
 	/**
-	 * @dataProvider provideShouldProtectFilter
+	 * @dataProvider provideGetForbiddenVariables
 	 */
-	public function testShouldProtectFilter( array $data, $expected ) {
+	public function testGetForbiddenVariables( array $data, $expected ) {
 		$performer = $this->mockRegisteredAuthorityWithPermissions( $data[ 'rights' ] );
 		$this->assertSame(
 			$expected,
-			$this->getPermMan()->shouldProtectFilter( $performer, $data[ 'usedVars' ] )
+			$this->getPermMan()->getForbiddenVariables( $performer, $data[ 'usedVars' ] )
 		);
 	}
 

tests/phpunit/unit/FilterValidatorTest.php

@@ -47,10 +47,10 @@ class FilterValidatorTest extends MediaWikiUnitTestCase {
 			$ruleChecker->method( 'checkSyntax' )->willReturn(
 				new ParserStatus( null, [], 1 )
 			);
-			$ruleChecker->method( 'getUsedVars' )->willReturnMap( [
-				[ 'user_unnamed_ip', [ 'user_unnamed_ip' ] ],
-				[ 'user_name', [] ]
-			] );
+			$ruleChecker->method( 'getUsedVars' )->willReturnCallback( static function ( string $rules ) {
+				preg_match_all( '/user_\w+/i', $rules, $matches, PREG_PATTERN_ORDER );
+				return array_map( 'strtolower', $matches[0] );
+			} );
 		}
 		$checkerFactory = $this->createMock( RuleCheckerFactory::class );
 		$checkerFactory->method( 'newRuleChecker' )->willReturn( $ruleChecker );
@@ -366,7 +366,7 @@ class FilterValidatorTest extends MediaWikiUnitTestCase {
 	public function testCheckCanViewProtectedVariables( $data ) {
 		$performer = $this->mockRegisteredAuthorityWithPermissions( $data[ 'rights' ] );
 		$permManager = $this->createMock( AbuseFilterPermissionManager::class );
-		$permManager->method( 'shouldProtectFilter' )->willReturn( false );
+		$permManager->method( 'getForbiddenVariables' )->willReturn( [] );
 		$filter = $this->createMock( AbstractFilter::class );
 		$filter->method( 'getRules' )->willReturn( $data[ 'rules' ] );
 		$this->assertStatusGood( $this->getFilterValidator( $permManager )
@@ -380,7 +380,7 @@ class FilterValidatorTest extends MediaWikiUnitTestCase {
 	public function testCheckCanViewProtectedVariablesError( $data ) {
 		$performer = $this->mockRegisteredAuthorityWithPermissions( $data[ 'rights' ] );
 		$permManager = $this->createMock( AbuseFilterPermissionManager::class );
-		$permManager->method( 'shouldProtectFilter' )->willReturn( [ 'user_unnamed_ip' ] );
+		$permManager->method( 'getForbiddenVariables' )->willReturn( [ 'user_unnamed_ip' ] );
 		$filter = $this->createMock( AbstractFilter::class );
 		$filter->method( 'getRules' )->willReturn( $data[ 'rules' ] );
 		$this->assertFilterValidatorStatus(