Implement 'protected' filter acknowledgement checkbox

- Add a basic checkbox on the filter edit page that must be checked if a
  filter uses a protected variable to ensure that the user is aware that
  their filter will also become protected

Bug: T364485
Change-Id: I7c7652f7d1a81223229b839ff7eee5da4af74c8a

i18n/en.json

@@ -212,6 +212,7 @@
 	"abusefilter-edit-enabled": "Enable this filter",
 	"abusefilter-edit-deleted": "Mark as deleted",
 	"abusefilter-edit-hidden": "Hide details of this filter from public view",
+	"abusefilter-edit-protected": "Hide details of this filter from users who can not see protected variables",
 	"abusefilter-edit-global": "Global filter",
 	"abusefilter-edit-rules": "Conditions:",
 	"abusefilter-edit-field-conditions": "conditions",
@@ -275,6 +276,7 @@
 	"abusefilter-edit-deleting-enabled": "You cannot mark an active filter as deleted.",
 	"abusefilter-edit-restricted": "You cannot edit this filter, because it contains one or more restricted actions.\nPlease ask a user with permission to add restricted actions to make the change for you.",
 	"abusefilter-edit-protected-variable": "You cannot save this filter because you don't have permission to use the following variables: $1",
+	"abusefilter-edit-protected-variable-not-protected": "You must protect this filter in order to save it because it uses the following variables: $1",
 	"abusefilter-edit-viewhistory": "View this filter's history",
 	"abusefilter-edit-history": "History:",
 	"abusefilter-edit-check": "Check syntax",

i18n/qqq.json

@@ -257,6 +257,7 @@
 	"abusefilter-edit-enabled": "Checkbox label for a filter flag.",
 	"abusefilter-edit-deleted": "Checkbox label for a filter flag.",
 	"abusefilter-edit-hidden": "Checkbox label for a filter flag.",
+	"abusefilter-edit-protected": "Checkbox label for a filter flag.",
 	"abusefilter-edit-global": "Checkbox label for a filter flag.",
 	"abusefilter-edit-rules": "Field label for filter rules.\n{{Identical|Condition}}",
 	"abusefilter-edit-field-conditions": "Description for filter rules, to be used in error message.\n{{Identical|Condition}}",
@@ -320,6 +321,7 @@
 	"abusefilter-edit-deleting-enabled": "Message to warn a user that an active filter can't be marked as deleted",
 	"abusefilter-edit-restricted": "Message to warn a user that a filter could not be edited for a given reason.",
 	"abusefilter-edit-protected-variable": "Message to warn a user that a filter could not be edited for a given reason. Parameters:\n* $1 is the variable that raised the error.",
+	"abusefilter-edit-protected-variable-not-protected": "Message to warn a user that a filter could not be edited for a given reason.",
 	"abusefilter-edit-viewhistory": "Link description for link that leads to a revision overview for a filter.",
 	"abusefilter-edit-history": "Field label for {{msg-mw|abusefilter-edit-viewhistory}}.\n{{Identical|History}}",
 	"abusefilter-edit-check": "Button text for checking abuse filter syntax.\n\nUsed in {{msg-mw|Abusefilter-test-syntaxerr}}.",

includes/FilterStore.php

@@ -7,7 +7,6 @@ use MediaWiki\Extension\AbuseFilter\ChangeTags\ChangeTagsManager;
 use MediaWiki\Extension\AbuseFilter\Consequences\ConsequencesRegistry;
 use MediaWiki\Extension\AbuseFilter\Filter\Filter;
 use MediaWiki\Extension\AbuseFilter\Filter\Flags;
-use MediaWiki\Extension\AbuseFilter\Filter\MutableFilter;
 use MediaWiki\Extension\AbuseFilter\Special\SpecialAbuseFilter;
 use MediaWiki\Permissions\Authority;
 use MediaWiki\Status\Status;
@@ -106,12 +105,6 @@ class FilterStore {
 			return $validationStatus;
 		}
 
-		// Filter would have thrown an error if the user didn't have permission to use protected variables
-		// We only need to check if the filter is using protected variables and force-set the flag to protected
-		// TODO: T364485 will use a checkbox to set the protected flag. Remove this when that's implemented.
-		$newFilter = MutableFilter::newFromParentFilter( $newFilter );
-		$newFilter->setProtected( $this->filterValidator->usesProtectedVars( $newFilter ) );
-
 		// Check for non-changes
 		$differences = $this->filterCompare->compareVersions( $newFilter, $originalFilter );
 		if ( !$differences ) {

includes/FilterValidator.php

@@ -114,6 +114,11 @@ class FilterValidator {
 			return $protectedVarsPermissionStatus;
 		}
 
+		$protectedVarsStatus = $this->checkProtectedVariables( $newFilter );
+		if ( !$protectedVarsStatus->isGood() ) {
+			return $protectedVarsStatus;
+		}
+
 		$globalPermStatus = $this->checkGlobalFilterEditPermission( $performer, $newFilter, $originalFilter );
 		if ( !$globalPermStatus->isGood() ) {
 			return $globalPermStatus;
@@ -357,6 +362,29 @@ class FilterValidator {
 		return $ret;
 	}
 
+	/**
+	 * @param AbstractFilter $filter
+	 * @return Status
+	 */
+	public function checkProtectedVariables( AbstractFilter $filter ): Status {
+		$ret = Status::newGood();
+		$ruleChecker = $this->ruleCheckerFactory->newRuleChecker();
+		$usedVariables = (array)$ruleChecker->getUsedVars( $filter->getRules() );
+		$usedProtectedVariables = array_intersect( $usedVariables, $this->protectedVariables );
+
+		if (
+			count( $usedProtectedVariables ) > 0 &&
+			!$filter->isProtected()
+		) {
+			$ret->error(
+				'abusefilter-edit-protected-variable-not-protected',
+				Message::listParam( $usedProtectedVariables )
+			);
+		}
+
+		return $ret;
+	}
+
 	/**
 	 * @param Authority $performer
 	 * @param AbstractFilter $filter
@@ -373,21 +401,6 @@ class FilterValidator {
 		return $ret;
 	}
 
-	/**
-	 * TODO: Remove this function when T364485 is implemented, as it makes this function obselete
-	 * This is a stop-gap function used to check if a filter should be marked as protected
-	 *
-	 * @param AbstractFilter $filter
-	 * @return bool
-	 */
-	public function usesProtectedVars( AbstractFilter $filter ): bool {
-		$ruleChecker = $this->ruleCheckerFactory->newRuleChecker();
-		$usedVariables = (array)$ruleChecker->getUsedVars( $filter->getRules() );
-		$usedProtectedVariables = array_intersect( $usedVariables, $this->protectedVariables );
-
-		return count( $usedProtectedVariables ) > 0;
-	}
-
 	/**
 	 * @param AbstractFilter $filter
 	 * @return Status

includes/View/AbuseFilterViewEdit.php

@@ -417,7 +417,7 @@ class AbuseFilterViewEdit extends AbuseFilterView {
 			] );
 
 		// Build checkboxes
-		$checkboxes = [ 'hidden', 'enabled', 'deleted' ];
+		$checkboxes = [ 'hidden', 'enabled', 'protected', 'deleted' ];
 		$flags = '';
 
 		if ( $this->getConfig()->get( 'AbuseFilterIsCentral' ) ) {
@@ -452,9 +452,10 @@ class AbuseFilterViewEdit extends AbuseFilterView {
 			// * abusefilter-edit-enabled
 			// * abusefilter-edit-deleted
 			// * abusefilter-edit-hidden
+			// * abusefilter-edit-protected
 			// * abusefilter-edit-global
 			$message = "abusefilter-edit-$checkboxId";
-			// isEnabled(), isDeleted(), isHidden(), isGlobal()
+			// isEnabled(), isDeleted(), isHidden(), isProtected(), isGlobal()
 			$method = 'is' . ucfirst( $checkboxId );
 			$postVar = 'wpFilter' . ucfirst( $checkboxId );
 
@@ -1256,6 +1257,7 @@ class AbuseFilterViewEdit extends AbuseFilterView {
 		$newFilter->setDeleted( $request->getCheck( 'wpFilterDeleted' ) );
 		$newFilter->setEnabled( $request->getCheck( 'wpFilterEnabled' ) );
 		$newFilter->setHidden( $request->getCheck( 'wpFilterHidden' ) );
+		$newFilter->setProtected( $request->getCheck( 'wpFilterProtected' ) );
 		$newFilter->setGlobal( $request->getCheck( 'wpFilterGlobal' )
 			&& $this->getConfig()->get( 'AbuseFilterIsCentral' ) );
 

tests/phpunit/integration/FilterStoreTest.php

@@ -190,8 +190,13 @@ class FilterStoreTest extends MediaWikiIntegrationTestCase {
 		$expectedError = 'abusefilter-edit-protected-variable';
 		$this->assertStatusWarning( $expectedError, $status );
 
-		// Save filter with right, with 'protected' flag enabled
+		// Add right and try to save filter without setting the 'protected' flag
 		$this->overrideUserPermissions( $user, [ 'abusefilter-access-protected-vars', 'abusefilter-modify' ] );
+		$status = AbuseFilterServices::getFilterStore()->saveFilter( $user, $row['id'], $newFilter, $origFilter );
+		$expectedError = 'abusefilter-edit-protected-variable-not-protected';
+		$this->assertStatusWarning( $expectedError, $status );
+
+		// Save filter with right, with 'protected' flag enabled
 		$row = [
 			'id' => '3',
 			'rules' => "ip_in_range( user_unnamed_ip, '1.2.3.4' )",

tests/phpunit/unit/FilterValidatorTest.php

@@ -319,17 +319,22 @@ class FilterValidatorTest extends MediaWikiUnitTestCase {
 			[ $unrestricted, $restricted, $restrictions, $canModifyRestrictedPM, null ];
 	}
 
-	public function testUsesProtectedVars() {
-		$filterProtectedVarsTrue = $this->createMock( AbstractFilter::class );
-		$filterProtectedVarsTrue->method( 'getRules' )->willReturn( 'user_unnamed_ip' );
-		$this->assertTrue(
-			$this->getFilterValidator()->usesProtectedVars( $filterProtectedVarsTrue )
+	public function testCheckProtectedVariablesGood() {
+		$filter = $this->createMock( AbstractFilter::class );
+		$filter->method( 'getRules' )->willReturn( 'user_unnamed_ip' );
+		$filter->method( 'isProtected' )->willReturn( true );
+		$this->assertStatusGood(
+			$this->getFilterValidator()->checkProtectedVariables( $filter )
 		);
+	}
 
-		$filterProtectedVarsFalse = $this->createMock( AbstractFilter::class );
-		$filterProtectedVarsFalse->method( 'getRules' )->willReturn( 'user_name' );
-		$this->assertFalse(
-			$this->getFilterValidator()->usesProtectedVars( $filterProtectedVarsFalse )
+	public function testCheckProtectedVariablesError() {
+		$filter = $this->createMock( AbstractFilter::class );
+		$filter->method( 'getRules' )->willReturn( 'user_unnamed_ip' );
+		$filter->method( 'isProtected' )->willReturn( false );
+		$this->assertStatusMessageParams(
+			'abusefilter-edit-protected-variable-not-protected',
+			$this->getFilterValidator()->checkProtectedVariables( $filter )
 		);
 	}