From bf28dbce0e5084340cda84365e4ffe65f020124c Mon Sep 17 00:00:00 2001 From: STran Date: Thu, 23 May 2024 07:49:17 -0700 Subject: [PATCH] Allow variables to be restricted by user right Some exposed variables (eg. `user_ip`) used in filters are sensitive and need to only be available to restricted groups of users. Back-end changes: - Add `AbuseFilterProtectedVariables` which defines what variables are protected by the new right `abusefilter-access-protected-vars` - Add the concept of a `protected` variable, the use of which will denote the entire filter as protected via a flag on `af_hidden` New UX features: - Display changes to the protected status of filters on history and diff pages - Check for protected variables and the right to see them in filter validation and don't allow a filter to be saved if it uses a variable that the user doesn't have access to - Check for the right to view protected variables before allowing access and edits to existing filters that use them Bug: T364465 Bug: T363906 Change-Id: I828bbb4015e87040f69a8e10c7888273c4f24dd3 --- extension.json | 9 +- i18n/en.json | 9 +- i18n/qqq.json | 9 +- includes/AbuseFilterPermissionManager.php | 78 +++++++++++++++- includes/Api/QueryAbuseFilters.php | 28 +++++- includes/Api/QueryAbuseLog.php | 9 +- includes/Filter/AbstractFilter.php | 7 ++ includes/Filter/Flags.php | 22 ++++- includes/Filter/MutableFilter.php | 7 ++ includes/FilterLookup.php | 5 +- includes/FilterStore.php | 29 +++++- includes/FilterUtils.php | 10 ++ includes/FilterValidator.php | 45 ++++++++- includes/Pager/AbuseFilterHistoryPager.php | 33 +++++-- includes/Pager/AbuseFilterPager.php | 3 + includes/Pager/AbuseLogPager.php | 4 +- includes/Pager/GlobalAbuseFilterPager.php | 4 +- includes/Parser/FilterEvaluator.php | 22 +++++ includes/ServiceWiring.php | 12 ++- includes/Special/SpecialAbuseLog.php | 4 +- includes/SpecsFormatter.php | 1 + includes/View/AbuseFilterViewDiff.php | 7 ++ includes/View/AbuseFilterViewEdit.php | 6 ++ includes/View/AbuseFilterViewExamine.php | 4 +- includes/View/AbuseFilterViewHistory.php | 8 +- tests/phpunit/integration/FilterStoreTest.php | 34 ++++++- .../integration/FilterValidatorTest.php | 3 +- .../unit/AbuseFilterPermissionManagerTest.php | 91 ++++++++++++++++++- .../unit/Filter/AbstractFilterTest.php | 3 +- tests/phpunit/unit/Filter/FlagsTest.php | 6 +- .../phpunit/unit/Filter/MutableFilterTest.php | 1 + tests/phpunit/unit/FilterLookupTest.php | 2 +- tests/phpunit/unit/FilterValidatorTest.php | 89 +++++++++++++++++- tests/phpunit/unit/Parser/ParserTest.php | 11 +++ tests/phpunit/unit/SpecsFormatterTest.php | 6 +- 35 files changed, 576 insertions(+), 45 deletions(-) diff --git a/extension.json b/extension.json index c2526c563..5afe33c03 100644 --- a/extension.json +++ b/extension.json @@ -29,7 +29,8 @@ "abusefilter-hide-log", "abusefilter-modify-global", "abusefilter-modify-blocked-external-domains", - "abusefilter-bypass-blocked-external-domains" + "abusefilter-bypass-blocked-external-domains", + "abusefilter-access-protected-vars" ], "GroupPermissions": { "*": { @@ -575,6 +576,12 @@ "AbuseFilterEnableBlockedExternalDomain": { "value": false, "description": "Temporary config value to disable Special:BlockedExternalDomains" + }, + "AbuseFilterProtectedVariables": { + "value": [ + "user_unnamed_ip" + ], + "description": "Array of variables that are be considered protected (limited access) and require the abusefilter-access-protected-vars right to use/view." } }, "load_composer_autoloader": true, diff --git a/i18n/en.json b/i18n/en.json index 25354fc0a..cd14d2396 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -43,6 +43,7 @@ "action-abusefilter-revert": "revert all changes by a given abuse filter", "action-abusefilter-view-private": "view abuse filters marked as private", "action-abusefilter-log-private": "view logs of abuse filters marked as private", + "action-abusefilter-log-protected": "view logs of abuse filters marked as protected", "action-abusefilter-hide-log": "hide entries in the abuse log", "action-abusefilter-hidden-log": "view hidden abuse log entries", "action-abusefilter-modify-global": "create or modify global abuse filters", @@ -143,6 +144,7 @@ "abusefilter-list-lastmodified": "Last modified", "abusefilter-list-group": "Filter group", "abusefilter-hidden": "Private", + "abusefilter-protected": "Protected", "abusefilter-unhidden": "Public", "abusefilter-enabled": "Enabled", "abusefilter-deleted": "Deleted", @@ -264,6 +266,7 @@ "abusefilter-block-user": "block registered users", "abusefilter-block-talk": "talk page blocked", "abusefilter-edit-denied": "You may not view details of this filter, because it is hidden from public view.", + "abusefilter-edit-denied-protected-vars": "You may not view details of this filter because it uses protected variables and is hidden from public view.", "abusefilter-edit-main": "Filter parameters", "abusefilter-edit-done-subtitle": "Filter edited", "abusefilter-edit-done": "[[Special:AbuseFilter/history/$1/diff/prev/$2|Your changes]] to [[Special:AbuseFilter/$1|filter $3]] have been saved.", @@ -271,6 +274,7 @@ "abusefilter-edit-missingfields": "The following fields are required and must be filled: $1", "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-viewhistory": "View this filter's history", "abusefilter-edit-history": "History:", "abusefilter-edit-check": "Check syntax", @@ -439,6 +443,7 @@ "abusefilter-history": "Change history for Abuse Filter #$1", "abusefilter-history-foruser": "Changes by $1", "abusefilter-history-hidden": "Hidden", + "abusefilter-history-protected": "Protected", "abusefilter-history-enabled": "Enabled", "abusefilter-history-global": "Global", "abusefilter-history-timestamp": "Time", @@ -613,5 +618,7 @@ "action-abusefilter-modify-blocked-external-domains": "create or modify what external domains are blocked from being linked", "right-abusefilter-bypass-blocked-external-domains": "Bypass blocked external domains", "action-abusefilter-bypass-blocked-external-domains": "bypass blocked external domain", - "abusefilter-blocked-domains-cannot-edit-directly": "Create or modify what external domains are blocked from being linked must be done through [[Special:BlockedExternalDomains|the special page]]." + "abusefilter-blocked-domains-cannot-edit-directly": "Create or modify what external domains are blocked from being linked must be done through [[Special:BlockedExternalDomains|the special page]].", + "right-abusefilter-access-protected-vars": "View and create filters that use protected variables", + "action-abusefilter-access-protected-vars": "view and create filters that use protected variables" } diff --git a/i18n/qqq.json b/i18n/qqq.json index 723177d24..220b85447 100644 --- a/i18n/qqq.json +++ b/i18n/qqq.json @@ -88,6 +88,7 @@ "action-abusefilter-revert": "{{doc-action|abusefilter-revert}}", "action-abusefilter-view-private": "{{doc-action|abusefilter-view-private}}", "action-abusefilter-log-private": "{{doc-action|abusefilter-log-private}}", + "action-abusefilter-log-protected": "{{doc-action|abusefilter-access-protected-vars}}", "action-abusefilter-hide-log": "{{doc-action|abusefilter-hide-log}}", "action-abusefilter-hidden-log": "{{doc-action|abusefilter-hidden-log}}", "action-abusefilter-modify-global": "{{doc-action|abusefilter-modify-global}}", @@ -188,6 +189,7 @@ "abusefilter-list-lastmodified": "Column header in abuse filter overview for the last modified timestamp for a filter.\n{{Identical|Last modified}}", "abusefilter-list-group": "The filter group the edit filter is in.", "abusefilter-hidden": "Abuse filter status.\n{{Identical|Private}}", + "abusefilter-protected": "Abuse filter status.\n{{Identical|Protected}}", "abusefilter-unhidden": "Abuse filter status.\n{{Identical|Public}}", "abusefilter-enabled": "Abuse filter status.\n{{Identical|Enabled}}", "abusefilter-deleted": "Abuse filter status.\n{{Identical|Deleted}}", @@ -309,6 +311,7 @@ "abusefilter-block-user": "Specification for type of block 'Block' is a verb.", "abusefilter-block-talk": "Specification for type of block. See {{msg-mw|Blocklist-nousertalk}}", "abusefilter-edit-denied": "Text used when a user has to access to filter details.", + "abusefilter-edit-denied-protected-vars": "Text used when a user is denied access to filter details because they don't have the viewing right.", "abusefilter-edit-main": "Fieldset legend for edit filter form.", "abusefilter-edit-done-subtitle": "Page subtitle when as filter was edited and saved.", "abusefilter-edit-done": "Text displayed to a user after editing a filter. Parameters:\n* $1 - a filter ID\n* $2 - the ID of the change itself\n* $3 - localized filter ID", @@ -316,6 +319,7 @@ "abusefilter-edit-missingfields": "Message to warn a user that a filter could not be edited for a given reason. Parameters:\n* $1 is a list of missing fields.", "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-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}}.", @@ -484,6 +488,7 @@ "abusefilter-history": "Used as page title.\n\n\"Change history\" is the \"history of changes\"\n\nParameters:\n* $1 - filter ID\n\nIf the filter ID is not specified, {{msg-mw|Abusefilter-filter-log}} will be used.", "abusefilter-history-foruser": "Parameters:\n* $1 - a link to the changing user's page\n* $2 - (Optional) the plain text username", "abusefilter-history-hidden": "{{Identical|Hidden}}", + "abusefilter-history-protected": "{{Identical|Protected}}", "abusefilter-history-enabled": "{{Identical|Enabled}}", "abusefilter-history-global": "{{Identical|Global}}", "abusefilter-history-timestamp": "Used as table column header in history page of a filter.\n\nTranslate \"Time\" as \"Timestamp\" (time and date).\n{{Identical|Time}}", @@ -658,5 +663,7 @@ "action-abusefilter-modify-blocked-external-domains": "{{doc-action|abusefilter-modify-blocked-external-domains}}", "right-abusefilter-bypass-blocked-external-domains": "{{doc-right|abusefilter-bypass-blocked-external-domains}}", "action-abusefilter-bypass-blocked-external-domains": "{{doc-action|abusefilter-bypass-blocked-external-domains}}", - "abusefilter-blocked-domains-cannot-edit-directly": "Error message shown when someone tries to edit the list of blocked domains directly and bypass the Special page." + "abusefilter-blocked-domains-cannot-edit-directly": "Error message shown when someone tries to edit the list of blocked domains directly and bypass the Special page.", + "right-abusefilter-access-protected-vars": "{{doc-right|abusefilter-access-protected-vars}}", + "action-abusefilter-access-protected-vars": "{{doc-action|abusefilter-access-protected-vars}}" } diff --git a/includes/AbuseFilterPermissionManager.php b/includes/AbuseFilterPermissionManager.php index af4300e9c..85f73e96b 100644 --- a/includes/AbuseFilterPermissionManager.php +++ b/includes/AbuseFilterPermissionManager.php @@ -2,6 +2,7 @@ namespace MediaWiki\Extension\AbuseFilter; +use MediaWiki\Config\ServiceOptions; use MediaWiki\Extension\AbuseFilter\Filter\AbstractFilter; use MediaWiki\Permissions\Authority; @@ -12,6 +13,24 @@ use MediaWiki\Permissions\Authority; class AbuseFilterPermissionManager { public const SERVICE_NAME = 'AbuseFilterPermissionManager'; + public const CONSTRUCTOR_OPTIONS = [ + 'AbuseFilterProtectedVariables', + ]; + + /** + * @var string[] Protected variables defined in config via AbuseFilterProtectedVariables + */ + private $protectedVariables; + + /** + * @param ServiceOptions $options + */ + public function __construct( + ServiceOptions $options + ) { + $this->protectedVariables = $options->get( 'AbuseFilterProtectedVariables' ); + } + /** * @param Authority $performer * @return bool @@ -71,6 +90,44 @@ class AbuseFilterPermissionManager { ); } + /** + * @param Authority $performer + * @return bool + */ + public function canViewProtectedVariables( Authority $performer ) { + $block = $performer->getBlock(); + return ( + !( $block && $block->isSitewide() ) && + $performer->isAllowed( 'abusefilter-access-protected-vars' ) + ); + } + + /** + * 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 + * + * @param Authority $performer + * @param string[] $usedVariables + * @return string[]|bool + */ + public function shouldProtectFilter( Authority $performer, $usedVariables ) { + $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; + } + } + } + /** * @param Authority $performer * @return bool @@ -111,9 +168,24 @@ class AbuseFilterPermissionManager { * @return bool */ public function canSeeLogDetailsForFilter( Authority $performer, int $privacyLevel ): bool { - if ( FilterUtils::isHidden( $privacyLevel ) ) { - return $this->canSeeLogDetails( $performer ) - && $this->canViewPrivateFiltersLogs( $performer ); + if ( $privacyLevel ) { + $res = true; + if ( FilterUtils::isHidden( $privacyLevel ) ) { + if ( + !$this->canSeeLogDetails( $performer ) || + !$this->canViewPrivateFiltersLogs( $performer ) + ) { + $res = false; + } + } + + if ( FilterUtils::isProtected( $privacyLevel ) ) { + if ( !$this->canViewProtectedVariables( $performer ) ) { + $res = false; + } + } + + return $res; } return $this->canSeeLogDetails( $performer ); diff --git a/includes/Api/QueryAbuseFilters.php b/includes/Api/QueryAbuseFilters.php index 490c789a8..b293cd90c 100644 --- a/includes/Api/QueryAbuseFilters.php +++ b/includes/Api/QueryAbuseFilters.php @@ -76,6 +76,7 @@ class QueryAbuseFilters extends ApiQueryBase { $fld_time = isset( $prop['lastedittime'] ); $fld_status = isset( $prop['status'] ); $fld_private = isset( $prop['private'] ); + $fld_protected = isset( $prop['protected'] ); $result = $this->getResult(); @@ -125,11 +126,20 @@ class QueryAbuseFilters extends ApiQueryBase { $this->getDb()->bitAnd( 'af_hidden', Flags::FILTER_HIDDEN ) . ' != 0', isset( $show['private'] ) ); + $this->addWhereIf( + $this->getDb()->bitAnd( 'af_hidden', Flags::FILTER_USES_PROTECTED_VARS ) . ' != 0', + isset( $show['!protected'] ) + ); + $this->addWhereIf( + $this->getDb()->bitAnd( 'af_hidden', Flags::FILTER_USES_PROTECTED_VARS ) . ' = 0', + isset( $show['!protected'] ) + ); } $res = $this->select( __METHOD__ ); $showhidden = $this->afPermManager->canViewPrivateFilters( $this->getAuthority() ); + $showProtected = $this->afPermManager->canViewProtectedVariables( $this->getAuthority() ); $count = 0; foreach ( $res as $row ) { @@ -146,7 +156,11 @@ class QueryAbuseFilters extends ApiQueryBase { if ( $fld_desc ) { $entry['description'] = $row->af_public_comments; } - if ( $fld_pattern && ( !FilterUtils::isHidden( $row->af_hidden ) || $showhidden ) ) { + if ( + $fld_pattern && + ( !FilterUtils::isHidden( $row->af_hidden ) || $showhidden ) && + ( !FilterUtils::isProtected( $row->af_hidden ) || $showProtected ) + ) { $entry['pattern'] = $row->af_pattern; } if ( $fld_actions ) { @@ -155,7 +169,11 @@ class QueryAbuseFilters extends ApiQueryBase { if ( $fld_hits ) { $entry['hits'] = intval( $row->af_hit_count ); } - if ( $fld_comments && ( !FilterUtils::isHidden( $row->af_hidden ) || $showhidden ) ) { + if ( + $fld_comments && + ( !FilterUtils::isHidden( $row->af_hidden ) || $showhidden ) && + ( !FilterUtils::isProtected( $row->af_hidden ) || $showProtected ) + ) { $entry['comments'] = $row->af_comments; } if ( $fld_user ) { @@ -168,6 +186,9 @@ class QueryAbuseFilters extends ApiQueryBase { if ( $fld_private && FilterUtils::isHidden( $row->af_hidden ) ) { $entry['private'] = ''; } + if ( $fld_protected && FilterUtils::isProtected( $row->af_hidden ) ) { + $entry['protected'] = ''; + } if ( $fld_status ) { if ( $row->af_enabled ) { $entry['enabled'] = ''; @@ -216,6 +237,8 @@ class QueryAbuseFilters extends ApiQueryBase { '!deleted', 'private', '!private', + 'protected', + '!protected', ], ], 'limit' => [ @@ -238,6 +261,7 @@ class QueryAbuseFilters extends ApiQueryBase { 'lastedittime', 'status', 'private', + 'protected', ], ParamValidator::PARAM_ISMULTI => true ] diff --git a/includes/Api/QueryAbuseLog.php b/includes/Api/QueryAbuseLog.php index 4c33a97e7..4bbc3f270 100644 --- a/includes/Api/QueryAbuseLog.php +++ b/includes/Api/QueryAbuseLog.php @@ -144,8 +144,8 @@ class QueryAbuseLog extends ApiQueryBase { try { $privacyLevel = $lookup->getFilter( $filterID, $global )->getPrivacyLevel(); } catch ( CentralDBNotAvailableException $_ ) { - // Conservatively assume it's hidden, like in SpecialAbuseLog - $privacyLevel = Flags::FILTER_HIDDEN; + // Conservatively assume it's hidden and protected, like in SpecialAbuseLog + $privacyLevel = Flags::FILTER_HIDDEN & Flags::FILTER_USES_PROTECTED_VARS; } catch ( FilterNotFoundException $_ ) { $privacyLevel = Flags::FILTER_PUBLIC; $foundInvalid = true; @@ -155,6 +155,11 @@ class QueryAbuseLog extends ApiQueryBase { [ 'apierror-permissiondenied', $this->msg( 'action-abusefilter-log-private' ) ] ); } + if ( Flags::FILTER_USES_PROTECTED_VARS & $privacyLevel ) { + $this->dieWithError( + [ 'apierror-permissiondenied', $this->msg( 'action-abusefilter-log-protected' ) ] + ); + } } } diff --git a/includes/Filter/AbstractFilter.php b/includes/Filter/AbstractFilter.php index 7d325a9d8..0b3dadfe2 100644 --- a/includes/Filter/AbstractFilter.php +++ b/includes/Filter/AbstractFilter.php @@ -116,6 +116,13 @@ class AbstractFilter { return $this->flags->getHidden(); } + /** + * @return bool + */ + public function isProtected(): bool { + return $this->flags->getProtected(); + } + /** * @return int */ diff --git a/includes/Filter/Flags.php b/includes/Filter/Flags.php index 2c2200024..2edf1d8b4 100644 --- a/includes/Filter/Flags.php +++ b/includes/Filter/Flags.php @@ -12,6 +12,8 @@ class Flags { private $deleted; /** @var bool */ private $hidden; + /** @var bool */ + private $protected; /** @var int */ private $privacyLevel; /** @var bool */ @@ -19,6 +21,7 @@ class Flags { public const FILTER_PUBLIC = 0b00; public const FILTER_HIDDEN = 0b01; + public const FILTER_USES_PROTECTED_VARS = 0b10; /** * @param bool $enabled @@ -30,6 +33,7 @@ class Flags { $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->global = $global; } @@ -77,9 +81,25 @@ class Flags { $this->updatePrivacyLevel(); } + /** + * @return bool + */ + public function getProtected(): bool { + return $this->protected; + } + + /** + * @param bool $protected + */ + public function setProtected( bool $protected ): void { + $this->protected = $protected; + $this->updatePrivacyLevel(); + } + private function updatePrivacyLevel() { $hidden = $this->hidden ? self::FILTER_HIDDEN : 0; - $this->privacyLevel = $hidden; + $protected = $this->protected ? self::FILTER_USES_PROTECTED_VARS : 0; + $this->privacyLevel = $hidden | $protected; } /** diff --git a/includes/Filter/MutableFilter.php b/includes/Filter/MutableFilter.php index aac892eb9..d3b5252db 100644 --- a/includes/Filter/MutableFilter.php +++ b/includes/Filter/MutableFilter.php @@ -116,6 +116,13 @@ class MutableFilter extends Filter { $this->flags->setHidden( $hidden ); } + /** + * @param bool $protected + */ + public function setProtected( bool $protected ): void { + $this->flags->setProtected( $protected ); + } + /** * @param bool $global */ diff --git a/includes/FilterLookup.php b/includes/FilterLookup.php index 12297dadf..a47da3b55 100644 --- a/includes/FilterLookup.php +++ b/includes/FilterLookup.php @@ -391,7 +391,10 @@ class FilterLookup implements IDBAccessObject { $hidden = in_array( 'hidden', $flags, true ) ? Flags::FILTER_HIDDEN : 0; - return $hidden; + $protected = in_array( 'protected', $flags, true ) ? + Flags::FILTER_USES_PROTECTED_VARS : + 0; + return $hidden | $protected; } /** diff --git a/includes/FilterStore.php b/includes/FilterStore.php index 3bd1d4625..657e49cf6 100644 --- a/includes/FilterStore.php +++ b/includes/FilterStore.php @@ -6,6 +6,8 @@ use ManualLogEntry; 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; @@ -104,6 +106,12 @@ 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 ) { @@ -113,7 +121,7 @@ class FilterStore { // Everything went fine, so let's save the filter $wasGlobal = $originalFilter->isGlobal(); [ $newID, $historyID ] = $this->doSaveFilter( - $performer->getUser(), $newFilter, $differences, $filterId, $wasGlobal ); + $performer->getUser(), $newFilter, $originalFilter, $differences, $filterId, $wasGlobal ); return Status::newGood( [ $newID, $historyID ] ); } @@ -122,6 +130,7 @@ class FilterStore { * * @param UserIdentity $userIdentity * @param Filter $newFilter + * @param Filter $originalFilter * @param array $differences * @param int|null $filterId * @param bool $wasGlobal @@ -130,12 +139,13 @@ class FilterStore { private function doSaveFilter( UserIdentity $userIdentity, Filter $newFilter, + Filter $originalFilter, array $differences, ?int $filterId, bool $wasGlobal ): array { $dbw = $this->lbFactory->getPrimaryDatabase(); - $newRow = $this->filterToDatabaseRow( $newFilter ); + $newRow = $this->filterToDatabaseRow( $newFilter, $originalFilter ); // Set last modifier. $newRow['af_timestamp'] = $dbw->timestamp(); @@ -208,6 +218,9 @@ class FilterStore { if ( FilterUtils::isHidden( $newRow['af_hidden'] ) ) { $flags[] = 'hidden'; } + if ( FilterUtils::isProtected( $newRow['af_hidden'] ) ) { + $flags[] = 'protected'; + } if ( $newRow['af_enabled'] ) { $flags[] = 'enabled'; } @@ -280,8 +293,16 @@ class FilterStore { * @param Filter $filter * @return array */ - private function filterToDatabaseRow( Filter $filter ): array { + private function filterToDatabaseRow( Filter $filter, Filter $originalFilter ): array { // T67807: integer 1's & 0's might be better understood than booleans + + // If the filter is already protected, it must remain protected even if + // the current filter doesn't use a protected variable anymore + $privacyLevel = $filter->getPrivacyLevel(); + if ( $originalFilter->isProtected() ) { + $privacyLevel |= Flags::FILTER_USES_PROTECTED_VARS; + } + return [ 'af_id' => $filter->getID(), 'af_pattern' => $filter->getRules(), @@ -291,7 +312,7 @@ class FilterStore { 'af_actions' => implode( ',', $filter->getActionsNames() ), 'af_enabled' => (int)$filter->isEnabled(), 'af_deleted' => (int)$filter->isDeleted(), - 'af_hidden' => $filter->getPrivacyLevel(), + 'af_hidden' => $privacyLevel, 'af_global' => (int)$filter->isGlobal(), 'af_timestamp' => $filter->getTimestamp(), 'af_hit_count' => $filter->getHitCount(), diff --git a/includes/FilterUtils.php b/includes/FilterUtils.php index 4b5c9e4d2..ff2c94fde 100644 --- a/includes/FilterUtils.php +++ b/includes/FilterUtils.php @@ -17,4 +17,14 @@ class FilterUtils { public static function isHidden( int $privacyLevel ) { return (bool)( Flags::FILTER_HIDDEN & $privacyLevel ); } + + /** + * Given a bitmask, return if the protected flag is set + * + * @param int $privacyLevel + * @return bool + */ + public static function isProtected( int $privacyLevel ) { + return (bool)( Flags::FILTER_USES_PROTECTED_VARS & $privacyLevel ); + } } diff --git a/includes/FilterValidator.php b/includes/FilterValidator.php index cf8283ec3..b8d3f60c2 100644 --- a/includes/FilterValidator.php +++ b/includes/FilterValidator.php @@ -19,7 +19,8 @@ class FilterValidator { public const CONSTRUCTOR_OPTIONS = [ 'AbuseFilterValidGroups', - 'AbuseFilterActionRestrictions' + 'AbuseFilterActionRestrictions', + 'AbuseFilterProtectedVariables', ]; /** @var ChangeTagValidator */ @@ -37,6 +38,11 @@ class FilterValidator { /** @var string[] */ private $validGroups; + /** + * @var string[] Protected variables defined in config via AbuseFilterProtectedVariables + */ + private $protectedVariables; + /** * @param ChangeTagValidator $changeTagValidator * @param RuleCheckerFactory $ruleCheckerFactory @@ -54,6 +60,7 @@ class FilterValidator { $this->permManager = $permManager; $this->restrictedActions = array_keys( array_filter( $options->get( 'AbuseFilterActionRestrictions' ) ) ); $this->validGroups = $options->get( 'AbuseFilterValidGroups' ); + $this->protectedVariables = $options->get( 'AbuseFilterProtectedVariables' ); } /** @@ -102,6 +109,11 @@ class FilterValidator { } } + $protectedVarsPermissionStatus = $this->checkCanViewProtectedVariables( $performer, $newFilter ); + if ( !$protectedVarsPermissionStatus->isGood() ) { + return $protectedVarsPermissionStatus; + } + $globalPermStatus = $this->checkGlobalFilterEditPermission( $performer, $newFilter, $originalFilter ); if ( !$globalPermStatus->isGood() ) { return $globalPermStatus; @@ -345,6 +357,37 @@ class FilterValidator { return $ret; } + /** + * @param Authority $performer + * @param AbstractFilter $filter + * @return Status + */ + public function checkCanViewProtectedVariables( Authority $performer, AbstractFilter $filter ): Status { + $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 ) ); + } + 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 diff --git a/includes/Pager/AbuseFilterHistoryPager.php b/includes/Pager/AbuseFilterHistoryPager.php index cae9b954e..492ff2958 100644 --- a/includes/Pager/AbuseFilterHistoryPager.php +++ b/includes/Pager/AbuseFilterHistoryPager.php @@ -41,6 +41,9 @@ class AbuseFilterHistoryPager extends TablePager { /** @var bool */ private $canViewPrivateFilters; + /** @var bool */ + private $canViewProtectedVars; + /** * @param IContextSource $context * @param LinkRenderer $linkRenderer @@ -50,6 +53,7 @@ class AbuseFilterHistoryPager extends TablePager { * @param ?int $filter * @param ?string $user User name * @param bool $canViewPrivateFilters + * @param bool $canViewProtectedVars */ public function __construct( IContextSource $context, @@ -59,7 +63,8 @@ class AbuseFilterHistoryPager extends TablePager { SpecsFormatter $specsFormatter, ?int $filter, ?string $user, - bool $canViewPrivateFilters = false + bool $canViewPrivateFilters = false, + bool $canViewProtectedVars = false ) { // needed by parent's constructor call $this->filter = $filter; @@ -69,6 +74,7 @@ class AbuseFilterHistoryPager extends TablePager { $this->specsFormatter = $specsFormatter; $this->user = $user; $this->canViewPrivateFilters = $canViewPrivateFilters; + $this->canViewProtectedVars = $canViewProtectedVars; $this->mDefaultDirection = true; } @@ -157,17 +163,27 @@ class AbuseFilterHistoryPager extends TablePager { break; case 'afh_id': // Set a link to a diff with the previous version if this isn't the first edit to the filter. - // Like in AbuseFilterViewDiff, don't show it if the user cannot see private filters and any - // of the versions is hidden. + // Like in AbuseFilterViewDiff, don't show it if: + // - the user cannot see private filters and any of the versions is hidden + // - the user cannot see protected variables and any of the versions is protected $formatted = ''; if ( $this->filterLookup->getFirstFilterVersionID( $row->afh_filter ) !== (int)$value ) { // @todo Should we also hide actions? $prevFilter = $this->filterLookup->getClosestVersion( $row->afh_id, $row->afh_filter, FilterLookup::DIR_PREV ); - if ( $this->canViewPrivateFilters || + if ( + ( $this->canViewPrivateFilters || + ( + !in_array( 'hidden', explode( ',', $row->afh_flags ) ) && + !$prevFilter->isHidden() + ) + ) && ( - !in_array( 'hidden', explode( ',', $row->afh_flags ) ) && - !$prevFilter->isHidden() + $this->canViewProtectedVars || + ( + !in_array( 'protected', explode( ',', $row->afh_flags ) ) && + !$prevFilter->isProtected() + ) ) ) { $title = SpecialAbuseFilter::getTitleForSubpage( @@ -233,6 +249,11 @@ class AbuseFilterHistoryPager extends TablePager { $info['conds'][] = $this->mDb->bitAnd( 'af_hidden', Flags::FILTER_HIDDEN ) . ' = 0'; } + if ( !$this->canViewProtectedVars ) { + // Hide data the user can't see. + $info['conds'][] = 'af_hidden & ' . Flags::FILTER_USES_PROTECTED_VARS . ' = 0'; + } + return $info; } diff --git a/includes/Pager/AbuseFilterPager.php b/includes/Pager/AbuseFilterPager.php index 3c6e2fe43..5c1c9f6c3 100644 --- a/includes/Pager/AbuseFilterPager.php +++ b/includes/Pager/AbuseFilterPager.php @@ -291,6 +291,9 @@ class AbuseFilterPager extends TablePager { if ( FilterUtils::isHidden( (int)$value ) ) { $flagMsgs[] = $this->msg( 'abusefilter-hidden' )->parse(); } + if ( FilterUtils::isProtected( (int)$value ) ) { + $flagMsgs[] = $this->msg( 'abusefilter-protected' )->parse(); + } if ( !$flagMsgs ) { return $this->msg( 'abusefilter-unhidden' )->parse(); } diff --git a/includes/Pager/AbuseLogPager.php b/includes/Pager/AbuseLogPager.php index 583150a68..a453a06df 100644 --- a/includes/Pager/AbuseLogPager.php +++ b/includes/Pager/AbuseLogPager.php @@ -183,9 +183,9 @@ class AbuseLogPager extends ReverseChronologicalPager { $privacyLevel = $filterObj->getPrivacyLevel(); } catch ( CentralDBNotAvailableException $_ ) { $escaped_comments = $this->msg( 'abusefilter-log-description-not-available' )->escaped(); - // either hide all filters, including not hidden, or show all, including hidden + // either hide all filters, including not hidden/protected, or show all, including hidden/protected // we choose the former - $privacyLevel = Flags::FILTER_HIDDEN; + $privacyLevel = Flags::FILTER_HIDDEN & Flags::FILTER_USES_PROTECTED_VARS; } } else { $escaped_comments = Sanitizer::escapeHtmlAllowEntities( diff --git a/includes/Pager/GlobalAbuseFilterPager.php b/includes/Pager/GlobalAbuseFilterPager.php index 470cb5a89..b877da5fe 100644 --- a/includes/Pager/GlobalAbuseFilterPager.php +++ b/includes/Pager/GlobalAbuseFilterPager.php @@ -64,8 +64,8 @@ class GlobalAbuseFilterPager extends AbuseFilterPager { return $lang->commaList( $statuses ); case 'af_hit_count': - // If the rule is hidden, don't show it, even to privileged local admins - if ( FilterUtils::isHidden( $row->af_hidden ) ) { + // If the rule is hidden or protected, don't show it, even to privileged local admins + if ( FilterUtils::isHidden( $row->af_hidden ) || FilterUtils::isProtected( $row->af_hidden ) ) { return ''; } return $this->msg( 'abusefilter-hitcount' )->numParams( $value )->parse(); diff --git a/includes/Parser/FilterEvaluator.php b/includes/Parser/FilterEvaluator.php index 4ab2c05f8..a7724caba 100644 --- a/includes/Parser/FilterEvaluator.php +++ b/includes/Parser/FilterEvaluator.php @@ -189,6 +189,11 @@ class FilterEvaluator { */ private $equivset; + /** + * @var array AFPToken::TID values found during node evaluation + */ + private $usedVars = []; + /** * Create a new instance * @@ -287,6 +292,7 @@ class FilterEvaluator { $this->mAllowShort = true; $this->mFilter = null; $this->warnings = []; + $this->usedVars = []; } /** @@ -438,6 +444,20 @@ class FilterEvaluator { return $ret; } + /** + * Parse a filter and return the variables used. + * All variables are AFPToken::TID and are found during the node stepthrough in evaluation + * and saved to self::usedVars to be returned to the caller in this function. + * + * @param string $filter + * @return array + */ + public function getUsedVars( $filter ) { + $this->resetState(); + $this->evaluateExpression( $filter ); + return array_unique( $this->usedVars ); + } + /** * Evaluate the value of the specified AST node. * @@ -812,6 +832,8 @@ class FilterEvaluator { // allowing it to result in DUNDEFINED. $allowMissingVariables = !$this->varExists( $var ) || $this->allowMissingVariables; + array_push( $this->usedVars, $var ); + // It's a built-in, non-disabled variable (either set or unset), or a set custom variable $flags = $allowMissingVariables ? VariablesManager::GET_LAX diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index 36b04cef4..37ff8cb9f 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -71,8 +71,13 @@ return [ ); }, PermManager::SERVICE_NAME => static function ( MediaWikiServices $services ): PermManager { - // No longer has any dependencies - return new PermManager(); + return new PermManager( + new ServiceOptions( + PermManager::CONSTRUCTOR_OPTIONS, + $services->getMainConfig(), + [ 'AbuseFilterProtectedVariables' => [] ] + ) + ); }, ChangeTagger::SERVICE_NAME => static function ( MediaWikiServices $services ): ChangeTagger { return new ChangeTagger( @@ -166,7 +171,8 @@ return [ $services->get( PermManager::SERVICE_NAME ), new ServiceOptions( FilterValidator::CONSTRUCTOR_OPTIONS, - $services->getMainConfig() + $services->getMainConfig(), + [ 'AbuseFilterProtectedVariables' => [] ] ) ); }, diff --git a/includes/Special/SpecialAbuseLog.php b/includes/Special/SpecialAbuseLog.php index 6142c3775..cd56f01c2 100644 --- a/includes/Special/SpecialAbuseLog.php +++ b/includes/Special/SpecialAbuseLog.php @@ -723,8 +723,8 @@ class SpecialAbuseLog extends AbuseFilterSpecialPage { $privacyLevel = AbuseFilterServices::getFilterLookup()->getFilter( $filterID, $global ) ->getPrivacyLevel(); } catch ( CentralDBNotAvailableException $_ ) { - // Conservatively assume that it's hidden, like in formatRow - $privacyLevel = Flags::FILTER_HIDDEN; + // Conservatively assume that it's hidden and protected, like in formatRow + $privacyLevel = Flags::FILTER_HIDDEN & Flags::FILTER_USES_PROTECTED_VARS; } } else { $privacyLevel = $row->af_hidden; diff --git a/includes/SpecsFormatter.php b/includes/SpecsFormatter.php index 1befb287d..770143e4e 100644 --- a/includes/SpecsFormatter.php +++ b/includes/SpecsFormatter.php @@ -147,6 +147,7 @@ class SpecsFormatter { 'enabled' => $filter->isEnabled(), 'deleted' => $filter->isDeleted(), 'hidden' => $filter->isHidden(), + 'protected' => $filter->isProtected(), 'global' => $filter->isGlobal() ] ); $flagsDisplay = []; diff --git a/includes/View/AbuseFilterViewDiff.php b/includes/View/AbuseFilterViewDiff.php index f328f77a7..55d186d0b 100644 --- a/includes/View/AbuseFilterViewDiff.php +++ b/includes/View/AbuseFilterViewDiff.php @@ -157,6 +157,13 @@ class AbuseFilterViewDiff extends AbuseFilterView { return false; } + if ( !$this->afPermManager->canViewProtectedVariables( $this->getAuthority() ) && + ( $this->oldVersion->isProtected() || $this->newVersion->isProtected() ) + ) { + $this->getOutput()->addWikiMsg( 'abusefilter-history-error-protected' ); + return false; + } + try { $this->nextHistoryId = $this->filterLookup->getClosestVersion( $this->newVersion->getHistoryID(), diff --git a/includes/View/AbuseFilterViewEdit.php b/includes/View/AbuseFilterViewEdit.php index 90809c627..3a9b7df7d 100644 --- a/includes/View/AbuseFilterViewEdit.php +++ b/includes/View/AbuseFilterViewEdit.php @@ -299,6 +299,12 @@ class AbuseFilterViewEdit extends AbuseFilterView { return; } + // Filters that use protected variables should always be hidden from public view + if ( $filterObj->isProtected() && !$this->afPermManager->canViewProtectedVariables( $user ) ) { + $out->addHTML( $this->msg( 'abusefilter-edit-denied-protected-vars' )->escaped() ); + return; + } + if ( $isCreatingNewFilter ) { $title = $this->msg( 'abusefilter-add' ); } elseif ( $this->afPermManager->canEditFilter( $user, $filterObj ) ) { diff --git a/includes/View/AbuseFilterViewExamine.php b/includes/View/AbuseFilterViewExamine.php index c3320260d..b5caac75c 100644 --- a/includes/View/AbuseFilterViewExamine.php +++ b/includes/View/AbuseFilterViewExamine.php @@ -268,8 +268,8 @@ 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, like in SpecialAbuseLog - $privacyLevel = Flags::FILTER_HIDDEN; + // Conservatively assume that it's hidden and protected, like in SpecialAbuseLog + $privacyLevel = Flags::FILTER_HIDDEN & Flags::FILTER_USES_PROTECTED_VARS; } if ( !$this->afPermManager->canSeeLogDetailsForFilter( $performer, $privacyLevel ) ) { $out->addWikiMsg( 'abusefilter-log-cannot-see-details' ); diff --git a/includes/View/AbuseFilterViewHistory.php b/includes/View/AbuseFilterViewHistory.php index eede7850c..c20b5a8e1 100644 --- a/includes/View/AbuseFilterViewHistory.php +++ b/includes/View/AbuseFilterViewHistory.php @@ -71,6 +71,7 @@ class AbuseFilterViewHistory extends AbuseFilterView { $out->enableOOUI(); $filter = $this->getRequest()->getIntOrNull( 'filter' ) ?: $this->filter; $canViewPrivate = $this->afPermManager->canViewPrivateFilters( $this->getAuthority() ); + $canViewProtectedVars = $this->afPermManager->canViewProtectedVariables( $this->getAuthority() ); if ( $filter ) { try { @@ -82,6 +83,10 @@ class AbuseFilterViewHistory extends AbuseFilterView { $out->addWikiMsg( 'abusefilter-history-error-hidden' ); return; } + if ( isset( $filterObj ) && $filterObj->isProtected() && !$canViewProtectedVars ) { + $out->addWikiMsg( 'abusefilter-history-error-hidden' ); + return; + } } if ( $filter ) { @@ -162,7 +167,8 @@ class AbuseFilterViewHistory extends AbuseFilterView { $this->specsFormatter, $filter, $user, - $canViewPrivate + $canViewPrivate, + $canViewProtectedVars ); $out->addParserOutputContent( $pager->getFullOutput() ); diff --git a/tests/phpunit/integration/FilterStoreTest.php b/tests/phpunit/integration/FilterStoreTest.php index a5b4a6e39..67de88d82 100644 --- a/tests/phpunit/integration/FilterStoreTest.php +++ b/tests/phpunit/integration/FilterStoreTest.php @@ -44,10 +44,11 @@ class FilterStoreTest extends MediaWikiIntegrationTestCase { $row = self::DEFAULT_VALUES; $row['timestamp'] = $this->db->timestamp( $row['timestamp'] ); $filter = $this->getFilterFromSpecs( [ 'id' => $id ] + $row ); + $oldFilter = MutableFilter::newDefault(); // Use some black magic to bypass checks /** @var FilterStore $filterStore */ $filterStore = TestingAccessWrapper::newFromObject( AbuseFilterServices::getFilterStore() ); - $row = $filterStore->filterToDatabaseRow( $filter ); + $row = $filterStore->filterToDatabaseRow( $filter, $oldFilter ); $row += AbuseFilterServices::getActorMigration()->getInsertValues( $this->db, 'af_user', @@ -172,4 +173,35 @@ class FilterStoreTest extends MediaWikiIntegrationTestCase { $this->assertStatusGood( $status ); $this->assertFalse( $status->getValue(), 'Status value should be false' ); } + + public function testSaveFilter_usesProtectedVars() { + $row = [ + 'id' => '2', + 'rules' => "ip_in_range( user_unnamed_ip, '1.2.3.4' )", + 'name' => 'Mock filter with protected variable used' + ]; + + $origFilter = MutableFilter::newDefault(); + $newFilter = $this->getFilterFromSpecs( $row ); + + // Try to save filter without right to use protected variables + $user = $this->getTestUser()->getUser(); + $status = AbuseFilterServices::getFilterStore()->saveFilter( $user, $row['id'], $newFilter, $origFilter ); + $expectedError = 'abusefilter-edit-protected-variable'; + $this->assertStatusWarning( $expectedError, $status ); + + // Save filter with right, with 'protected' flag enabled + $this->overrideUserPermissions( $user, [ 'abusefilter-access-protected-vars', 'abusefilter-modify' ] ); + $row = [ + '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, + ]; + $newFilter = $this->getFilterFromSpecs( $row ); + $status = AbuseFilterServices::getFilterStore()->saveFilter( + $user, $row['id'], $newFilter, $origFilter + ); + $this->assertStatusGood( $status ); + } } diff --git a/tests/phpunit/integration/FilterValidatorTest.php b/tests/phpunit/integration/FilterValidatorTest.php index f6b6420d9..bd147dc34 100644 --- a/tests/phpunit/integration/FilterValidatorTest.php +++ b/tests/phpunit/integration/FilterValidatorTest.php @@ -31,7 +31,8 @@ class FilterValidatorTest extends MediaWikiIntegrationTestCase { FilterValidator::CONSTRUCTOR_OPTIONS, [ 'AbuseFilterActionRestrictions' => [], - 'AbuseFilterValidGroups' => [ 'default' ] + 'AbuseFilterValidGroups' => [ 'default' ], + 'AbuseFilterProtectedVariables' => [], ] ) ); diff --git a/tests/phpunit/unit/AbuseFilterPermissionManagerTest.php b/tests/phpunit/unit/AbuseFilterPermissionManagerTest.php index 5f845a90e..302dccf28 100644 --- a/tests/phpunit/unit/AbuseFilterPermissionManagerTest.php +++ b/tests/phpunit/unit/AbuseFilterPermissionManagerTest.php @@ -4,6 +4,7 @@ namespace MediaWiki\Extension\AbuseFilter\Tests\Unit; use Generator; use MediaWiki\Block\DatabaseBlock; +use MediaWiki\Config\ServiceOptions; use MediaWiki\Extension\AbuseFilter\AbuseFilterPermissionManager; use MediaWiki\Extension\AbuseFilter\Filter\AbstractFilter; use MediaWiki\Extension\AbuseFilter\Filter\Flags; @@ -18,8 +19,14 @@ class AbuseFilterPermissionManagerTest extends MediaWikiUnitTestCase { use MockAuthorityTrait; private function getPermMan(): AbuseFilterPermissionManager { - // No longer has any dependencies - return new AbuseFilterPermissionManager(); + return new AbuseFilterPermissionManager( + new ServiceOptions( + AbuseFilterPermissionManager::CONSTRUCTOR_OPTIONS, + [ + 'AbuseFilterProtectedVariables' => [ 'user_unnamed_ip' ] + ] + ) + ); } public function provideCanEdit(): Generator { @@ -166,14 +173,94 @@ class AbuseFilterPermissionManagerTest extends MediaWikiUnitTestCase { public static function provideCanSeeLogDetailsForFilter(): Generator { $details = [ 0 => 'abusefilter-log-detail' ]; $private = [ 1 => 'abusefilter-log-private' ]; + $protected = [ 2 => 'abusefilter-access-protected-vars' ]; yield 'filter hidden, not privileged' => [ Flags::FILTER_HIDDEN, [], false ]; yield 'filter hidden, details only' => [ Flags::FILTER_HIDDEN, $details, false ]; yield 'filter hidden, private logs only' => [ Flags::FILTER_HIDDEN, $private, false ]; yield 'filter hidden, details and private logs' => [ Flags::FILTER_HIDDEN, $details + $private, true ]; + yield 'filter protected, not privileged' => [ Flags::FILTER_USES_PROTECTED_VARS, [], false ]; + yield 'filter protected, privileged' => [ Flags::FILTER_USES_PROTECTED_VARS, $protected, true ]; + yield 'filter hidden and protected, details and private only' => [ + Flags::FILTER_HIDDEN | Flags::FILTER_USES_PROTECTED_VARS, $details + $private, false + ]; + yield 'filter hidden and protected, protected only' => [ + Flags::FILTER_HIDDEN | Flags::FILTER_USES_PROTECTED_VARS, $protected, false + ]; yield 'filter visible, not privileged' => [ Flags::FILTER_PUBLIC, [], false ]; yield 'filter visible, privileged' => [ Flags::FILTER_PUBLIC, $details, true ]; } + public function provideCanViewProtectedVariables(): Generator { + $block = $this->createMock( DatabaseBlock::class ); + $block->method( 'isSiteWide' )->willReturn( true ); + yield 'not privileged, blocked' => [ $block, [], false ]; + yield 'not privileged, not blocked' => [ null, [], false ]; + yield 'has right, blocked' => [ $block, [ 'abusefilter-access-protected-vars' ], false ]; + yield 'has right, not blocked' => [ null, [ 'abusefilter-access-protected-vars' ], true ]; + } + + /** + * @dataProvider provideCanViewProtectedVariables + */ + public function testCanViewProtectedVariables( ?DatabaseBlock $block, array $rights, bool $expected ) { + if ( $block !== null ) { + $performer = $this->mockUserAuthorityWithBlock( + $this->mockRegisteredUltimateAuthority()->getUser(), + $block, + $rights + ); + } else { + $performer = $this->mockRegisteredAuthorityWithPermissions( $rights ); + } + + $this->assertSame( + $expected, + $this->getPermMan()->canViewProtectedVariables( $performer ) + ); + } + + public static function provideShouldProtectFilter(): Generator { + yield 'cannot view, protected vars' => [ + [ + 'rights' => [], + 'usedVars' => [ 'user_unnamed_ip' ] + ], + [ 'user_unnamed_ip' ] + ]; + yield 'cannot view, no protected vars' => [ + [ + '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 + */ + public function testShouldProtectFilter( array $data, $expected ) { + $performer = $this->mockRegisteredAuthorityWithPermissions( $data[ 'rights' ] ); + $this->assertSame( + $expected, + $this->getPermMan()->shouldProtectFilter( $performer, $data[ 'usedVars' ] ) + ); + } + /** * @param int $privacyLevel * @param array $rights diff --git a/tests/phpunit/unit/Filter/AbstractFilterTest.php b/tests/phpunit/unit/Filter/AbstractFilterTest.php index 3ba98507b..e7305b590 100644 --- a/tests/phpunit/unit/Filter/AbstractFilterTest.php +++ b/tests/phpunit/unit/Filter/AbstractFilterTest.php @@ -47,7 +47,7 @@ class AbstractFilterTest extends MediaWikiUnitTestCase { $group = 'group'; $enabled = true; $deleted = false; - $privacyLevel = Flags::FILTER_HIDDEN; + $privacyLevel = Flags::FILTER_HIDDEN | Flags::FILTER_USES_PROTECTED_VARS; $global = false; $filter = new AbstractFilter( new Specs( $rules, $comments, $name, $actionsNames, $group ), @@ -63,6 +63,7 @@ class AbstractFilterTest extends MediaWikiUnitTestCase { $this->assertSame( $enabled, $filter->isEnabled(), 'enabled' ); $this->assertSame( $deleted, $filter->isDeleted(), 'deleted' ); $this->assertSame( true, $filter->isHidden(), 'hidden' ); + $this->assertSame( true, $filter->isProtected(), 'uses protected vars' ); $this->assertSame( $privacyLevel, $filter->getPrivacyLevel(), 'privacy level' ); $this->assertSame( $global, $filter->isGlobal(), 'global' ); } diff --git a/tests/phpunit/unit/Filter/FlagsTest.php b/tests/phpunit/unit/Filter/FlagsTest.php index 3ee751f46..c2e6f1a75 100644 --- a/tests/phpunit/unit/Filter/FlagsTest.php +++ b/tests/phpunit/unit/Filter/FlagsTest.php @@ -14,13 +14,14 @@ class FlagsTest extends MediaWikiUnitTestCase { public function testGetters() { $enabled = true; $deleted = false; - $privacyLevel = Flags::FILTER_HIDDEN; + $privacyLevel = Flags::FILTER_HIDDEN | Flags::FILTER_USES_PROTECTED_VARS; $global = false; $flags = new Flags( $enabled, $deleted, $privacyLevel, $global ); $this->assertSame( $enabled, $flags->getEnabled(), 'enabled' ); $this->assertSame( $deleted, $flags->getDeleted(), 'deleted' ); $this->assertSame( true, $flags->getHidden(), 'hidden' ); + $this->assertSame( true, $flags->getProtected(), 'protected variables' ); $this->assertSame( $privacyLevel, $flags->getPrivacyLevel(), 'privacy level' ); $this->assertSame( $global, $flags->getGlobal(), 'global' ); } @@ -32,7 +33,7 @@ class FlagsTest extends MediaWikiUnitTestCase { * @dataProvider provideSetters */ public function testSetters( $value, string $setter, string $getter ) { - $flags = new Flags( true, true, Flags::FILTER_HIDDEN, true ); + $flags = new Flags( true, true, Flags::FILTER_HIDDEN | Flags::FILTER_USES_PROTECTED_VARS, true ); $flags->$setter( $value ); $this->assertSame( $value, $flags->$getter() ); @@ -46,6 +47,7 @@ class FlagsTest extends MediaWikiUnitTestCase { 'enabled' => [ true, 'setEnabled', 'getEnabled' ], 'deleted' => [ false, 'setDeleted', 'getDeleted' ], 'hidden' => [ true, 'setHidden', 'getHidden' ], + 'protected' => [ true, 'setProtected', 'getProtected' ], 'global' => [ false, 'setGlobal', 'getGlobal' ], ]; } diff --git a/tests/phpunit/unit/Filter/MutableFilterTest.php b/tests/phpunit/unit/Filter/MutableFilterTest.php index 9c625263f..95b99c881 100644 --- a/tests/phpunit/unit/Filter/MutableFilterTest.php +++ b/tests/phpunit/unit/Filter/MutableFilterTest.php @@ -49,6 +49,7 @@ class MutableFilterTest extends MediaWikiUnitTestCase { 'enabled' => [ true, 'setEnabled', 'isEnabled' ], 'deleted' => [ false, 'setDeleted', 'isDeleted' ], 'hidden' => [ true, 'setHidden', 'isHidden' ], + 'protected' => [ true, 'setProtected', 'isProtected' ], 'global' => [ false, 'setGlobal', 'isGlobal' ], 'actions' => [ [ 'foo' => [] ], 'setActions', 'getActions' ], 'user ID' => [ 163, 'setUserID', 'getUserID' ], diff --git a/tests/phpunit/unit/FilterLookupTest.php b/tests/phpunit/unit/FilterLookupTest.php index ac5bcb2df..15764b9e4 100644 --- a/tests/phpunit/unit/FilterLookupTest.php +++ b/tests/phpunit/unit/FilterLookupTest.php @@ -168,7 +168,7 @@ class FilterLookupTest extends MediaWikiUnitTestCase { foreach ( $filters as $filter ) { $flags = []; - foreach ( [ 'enabled', 'deleted', 'hidden', 'global' ] as $flag ) { + foreach ( [ 'enabled', 'deleted', 'hidden', 'protected', 'global' ] as $flag ) { $method = 'is' . ucfirst( $flag ); if ( $filter->$method() ) { $flags[] = $flag; diff --git a/tests/phpunit/unit/FilterValidatorTest.php b/tests/phpunit/unit/FilterValidatorTest.php index d34aa79ed..9cd701166 100644 --- a/tests/phpunit/unit/FilterValidatorTest.php +++ b/tests/phpunit/unit/FilterValidatorTest.php @@ -16,6 +16,7 @@ use MediaWiki\Extension\AbuseFilter\Parser\ParserStatus; use MediaWiki\Extension\AbuseFilter\Parser\RuleCheckerFactory; use MediaWiki\Permissions\Authority; use MediaWiki\Status\Status; +use MediaWiki\Tests\Unit\Permissions\MockAuthorityTrait; use MediaWikiUnitTestCase; use PHPUnit\Framework\MockObject\MockObject; @@ -26,6 +27,7 @@ use PHPUnit\Framework\MockObject\MockObject; * @covers \MediaWiki\Extension\AbuseFilter\FilterValidator */ class FilterValidatorTest extends MediaWikiUnitTestCase { + use MockAuthorityTrait; /** * @param AbuseFilterPermissionManager|null $permissionManager @@ -45,6 +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', [] ] + ] ); } $checkerFactory = $this->createMock( RuleCheckerFactory::class ); $checkerFactory->method( 'newRuleChecker' )->willReturn( $ruleChecker ); @@ -60,7 +66,8 @@ class FilterValidatorTest extends MediaWikiUnitTestCase { FilterValidator::CONSTRUCTOR_OPTIONS, [ 'AbuseFilterActionRestrictions' => array_fill_keys( $restrictions, true ), - 'AbuseFilterValidGroups' => $validFilterGroups + 'AbuseFilterValidGroups' => $validFilterGroups, + 'AbuseFilterProtectedVariables' => [ 'user_unnamed_ip' ], ] ) ); @@ -312,6 +319,86 @@ 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 ) + ); + + $filterProtectedVarsFalse = $this->createMock( AbstractFilter::class ); + $filterProtectedVarsFalse->method( 'getRules' )->willReturn( 'user_name' ); + $this->assertFalse( + $this->getFilterValidator()->usesProtectedVars( $filterProtectedVarsFalse ) + ); + } + + /** + * @dataProvider provideCheckCanViewProtectedVariables + */ + public function testCheckCanViewProtectedVariables( $data ) { + $performer = $this->mockRegisteredAuthorityWithPermissions( $data[ 'rights' ] ); + $permManager = $this->createMock( AbuseFilterPermissionManager::class ); + $permManager->method( 'shouldProtectFilter' )->willReturn( false ); + $filter = $this->createMock( AbstractFilter::class ); + $filter->method( 'getRules' )->willReturn( $data[ 'rules' ] ); + $this->assertStatusGood( $this->getFilterValidator( $permManager ) + ->checkCanViewProtectedVariables( $performer, $filter ) + ); + } + + /** + * @dataProvider provideCheckCanViewProtectedVariablesError + */ + public function testCheckCanViewProtectedVariablesError( $data ) { + $performer = $this->mockRegisteredAuthorityWithPermissions( $data[ 'rights' ] ); + $permManager = $this->createMock( AbuseFilterPermissionManager::class ); + $permManager->method( 'shouldProtectFilter' )->willReturn( [ 'user_unnamed_ip' ] ); + $filter = $this->createMock( AbstractFilter::class ); + $filter->method( 'getRules' )->willReturn( $data[ 'rules' ] ); + $this->assertStatusMessageParams( + 'abusefilter-edit-protected-variable', + $this->getFilterValidator( $permManager )->checkCanViewProtectedVariables( $performer, $filter ) + ); + } + + public static function provideCheckCanViewProtectedVariables(): array { + return [ + 'cannot view, no protected vars' => [ + [ + 'rights' => [], + 'rules' => 'user_name' + ], + 0 + ], + 'can view, protected vars' => [ + [ + 'rights' => [ 'abusefilter-access-protected-vars' ], + 'rules' => 'user_unnamed_ip' + ], + 0 + ], + 'can view, no protected vars' => [ + [ + 'rights' => [ 'abusefilter-access-protected-vars' ], + 'rules' => 'user_name' + ], + 0 + ] + ]; + } + + public static function provideCheckCanViewProtectedVariablesError(): array { + return [ + 'cannot view, protected vars' => [ + [ + 'rights' => [], + 'rules' => 'user_unnamed_ip' + ] + ], + ]; + } + public function testCheckAllTags_noTags() { $this->assertStatusMessageParams( 'tags-create-no-name', $this->getFilterValidator()->checkAllTags( [] ) ); } diff --git a/tests/phpunit/unit/Parser/ParserTest.php b/tests/phpunit/unit/Parser/ParserTest.php index d07876502..1adaf5cbb 100644 --- a/tests/phpunit/unit/Parser/ParserTest.php +++ b/tests/phpunit/unit/Parser/ParserTest.php @@ -1162,4 +1162,15 @@ class ParserTest extends ParserTestCase { $wrapper = TestingAccessWrapper::newFromObject( $parser ); $this->assertSame( $vars, $wrapper->mVariables ); } + + /** + * @covers \MediaWiki\Extension\AbuseFilter\Parser\FilterEvaluator + */ + public function testGetUsedVars() { + $parser = $this->getParser(); + /** @var FilterEvaluator $wrapper */ + $wrapper = TestingAccessWrapper::newFromObject( $parser ); + $this->assertSame( [ 'user_name' ], $wrapper->getUsedVars( 'ip_in_range( user_name, "1.2.3.4" )' ) ); + $this->assertSame( [ 'user_name' ], $wrapper->getUsedVars( 'ip_in_range( USER_NAME, "1.2.3.4" )' ) ); + } } diff --git a/tests/phpunit/unit/SpecsFormatterTest.php b/tests/phpunit/unit/SpecsFormatterTest.php index a5b803fed..cd8630dc0 100644 --- a/tests/phpunit/unit/SpecsFormatterTest.php +++ b/tests/phpunit/unit/SpecsFormatterTest.php @@ -160,7 +160,11 @@ class SpecsFormatterTest extends MediaWikiUnitTestCase { $multiple = MutableFilter::newDefault(); $multiple->setHidden( true ); - yield 'multiple' => [ $multiple, 'abusefilter-history-enabled,abusefilter-history-hidden' ]; + $multiple->setProtected( true ); + yield 'multiple' => [ + $multiple, + 'abusefilter-history-enabled,abusefilter-history-hidden,abusefilter-history-protected' + ]; } /**