diff --git a/extension.json b/extension.json index d269cfff0..efce9fe05 100644 --- a/extension.json +++ b/extension.json @@ -150,7 +150,8 @@ "ApiAbuseFilterCheckSyntax": "includes/api/ApiAbuseFilterCheckSyntax.php", "ApiAbuseFilterEvalExpression": "includes/api/ApiAbuseFilterEvalExpression.php", "ApiAbuseFilterUnblockAutopromote": "includes/api/ApiAbuseFilterUnblockAutopromote.php", - "ApiAbuseFilterCheckMatch": "includes/api/ApiAbuseFilterCheckMatch.php" + "ApiAbuseFilterCheckMatch": "includes/api/ApiAbuseFilterCheckMatch.php", + "NormalizeThrottleParameters": "maintenance/normalizeThrottleParameters.php" }, "ResourceModules": { "ext.abuseFilter": { @@ -162,6 +163,8 @@ "abusefilter-edit-syntaxok", "abusefilter-edit-syntaxerr", "abusefilter-http-error", + "abusefilter-edit-throttle-placeholder", + "abusefilter-edit-tag-placeholder", "unknown-error" ], "dependencies": [ diff --git a/i18n/en.json b/i18n/en.json index 28b143474..9cd8e22ee 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -201,13 +201,15 @@ "abusefilter-edit-throttle-count": "Number of actions to allow:", "abusefilter-edit-throttle-period": "Period of time (in seconds):", "abusefilter-edit-throttle-groups": "Group throttle by:", - "abusefilter-edit-throttle-ip": "IP address", - "abusefilter-edit-throttle-user": "User account", - "abusefilter-edit-throttle-range": "/16 range", - "abusefilter-edit-throttle-creationdate": "Server time of account creation", - "abusefilter-edit-throttle-editcount": "Edit count", - "abusefilter-edit-throttle-site": "The whole site", - "abusefilter-edit-throttle-page": "Page", + "abusefilter-edit-throttle-hidden-placeholder": "Split with commas to join with AND, and with linebreaks to join with OR", + "abusefilter-edit-throttle-placeholder": "Split with commas to join with AND, and insert one by one to join with OR", + "abusefilter-throttle-ip": "IP address", + "abusefilter-throttle-user": "user account", + "abusefilter-throttle-range": "/16 range", + "abusefilter-throttle-creationdate": "account creation date", + "abusefilter-throttle-editcount": "edit count", + "abusefilter-throttle-site": "whole site", + "abusefilter-throttle-page": "page", "abusefilter-throttle-details": "Allow $1 {{PLURAL:$1|action|actions}} every $2 {{PLURAL:$2|second|seconds}}, group throttle by: $3", "abusefilter-edit-warn-message": "System message to use for warning:", "abusefilter-edit-warn-other": "Other message", @@ -251,6 +253,11 @@ "abusefilter-edit-notallowed": "You are not permitted to create or edit abuse filters", "abusefilter-edit-notallowed-global": "You are not permitted to create or edit global abuse filters", "abusefilter-edit-notallowed-global-custom-msg": "Custom warning messages are not supported for global filters", + "abusefilter-edit-invalid-throttlecount": "The throttle action count must be a positive integer.", + "abusefilter-edit-invalid-throttleperiod": "The throttle period must be a positive integer.", + "abusefilter-edit-empty-throttlegroups": "At least one throttle group must be selected.", + "abusefilter-edit-duplicated-throttlegroups": "Throttle groups cannot have duplicates.", + "abusefilter-edit-invalid-throttlegroups": "The specified throttle groups are not valid.", "abusefilter-edit-builder-select": "Select an option to add it at the cursor", "abusefilter-edit-builder-group-op-arithmetic": "Arithmetic operators", "abusefilter-edit-builder-op-arithmetic-addition": "Addition (+)", diff --git a/i18n/qqq.json b/i18n/qqq.json index 320d0a1be..69ac7e4cd 100644 --- a/i18n/qqq.json +++ b/i18n/qqq.json @@ -234,13 +234,15 @@ "abusefilter-edit-throttle-count": "Field label for entering the number of allowed hits before triggering the filter consequences.", "abusefilter-edit-throttle-period": "Field label for entering a time period in seconds.\n{{Identical|Second}}", "abusefilter-edit-throttle-groups": "Field label for properties to group throttle counts by (for example IP address and username). Throttling is the concept of limiting occurrences of a certain action in a given time frame.", - "abusefilter-edit-throttle-ip": "Checkbox with a throttle group", - "abusefilter-edit-throttle-user": "Checkbox with a throttle group", - "abusefilter-edit-throttle-range": "Checkbox with a throttle group", - "abusefilter-edit-throttle-creationdate": "Checkbox with a throttle group", - "abusefilter-edit-throttle-editcount": "Checkbox with a throttle group", - "abusefilter-edit-throttle-site": "Checkbox with a throttle group", - "abusefilter-edit-throttle-page": "Checkbox with a throttle group", + "abusefilter-edit-throttle-hidden-placeholder": "Label for a textarea where users may insert throttling criteria.", + "abusefilter-edit-throttle-placeholder": "Label for an input field where users may insert throttling criteria.", + "abusefilter-throttle-ip": "Throttle option.", + "abusefilter-throttle-user": "Throttle option.", + "abusefilter-throttle-range": "Throttle option.", + "abusefilter-throttle-creationdate": "Throttle option.", + "abusefilter-throttle-editcount": "Throttle option.", + "abusefilter-throttle-site": "Throttle option.", + "abusefilter-throttle-page": "Throttle option.", "abusefilter-throttle-details": "Description for Special:AbuseFilter/history with a detailed description for throttle action. Parameters:\n* $1 is the number of allowed actions, $2 is the time in seconds to use before resetting the action count, $3 is a list of throttled groups", "abusefilter-edit-warn-message": "Field label for dropdown list with system messages.", "abusefilter-edit-warn-other": "Option in dropdown menu to specify no item from the list should be used.\n\nSee also:\n* {{msg-mw|abusefilter-edit-disallow-other}}", @@ -284,6 +286,11 @@ "abusefilter-edit-notallowed": "Error message when trying to modify a filter while not allowed.", "abusefilter-edit-notallowed-global": "Error message when trying to modify a global filter while not allowed.", "abusefilter-edit-notallowed-global-custom-msg": "Error message when trying to add a custom warning message to a global filter, which is not allowed.", + "abusefilter-edit-invalid-throttlecount": "Error message when trying to provide an invalid action count for \"throttle\" action.", + "abusefilter-edit-invalid-throttleperiod": "Error message when trying to provide an invalid time period for \"throttle\" action.", + "abusefilter-edit-empty-throttlegroups": "Error message when trying to save a filter with \"throttle\" action enabled but no throttle groups selected.", + "abusefilter-edit-duplicated-throttlegroups": "Error message when trying to save a filter with \"throttle\" action enabled and duplicated throttle groups.", + "abusefilter-edit-invalid-throttlegroups": "Error message when trying to save a filter with \"throttle\" action enabled and invalid throttle groups.", "abusefilter-edit-builder-select": "Default value for dropdown menu that allows inserting abuse filter syntax in the filter definition field.", "abusefilter-edit-builder-group-op-arithmetic": "Group entry in dropdown menu.", "abusefilter-edit-builder-op-arithmetic-addition": "Abuse filter syntax option in a dropdown from the group {{msg-mw|abusefilter-edit-builder-group-op-arithmetic}}.", diff --git a/includes/AbuseFilter.php b/includes/AbuseFilter.php index 0d8092c85..6c4efbfcd 100644 --- a/includes/AbuseFilter.php +++ b/includes/AbuseFilter.php @@ -2214,6 +2214,76 @@ class AbuseFilter { return $finalStatus; } + /** + * Validate throttle parameters + * + * @param array $params Throttle parameters + * @return null|string Null on success, a string with the error message on failure + */ + public static function checkThrottleParameters( $params ) { + $throttleRate = explode( ',', $params[1] ); + $throttleCount = $throttleRate[0]; + $throttlePeriod = $throttleRate[1]; + $throttleGroups = array_slice( $params, 2 ); + $validGroups = [ + 'ip', + 'user', + 'range', + 'creationdate', + 'editcount', + 'site', + 'page' + ]; + + $error = null; + if ( preg_match( '/^[1-9][0-9]*$/', $throttleCount ) === 0 ) { + $error = 'abusefilter-edit-invalid-throttlecount'; + } elseif ( preg_match( '/^[1-9][0-9]*$/', $throttlePeriod ) === 0 ) { + $error = 'abusefilter-edit-invalid-throttleperiod'; + } elseif ( !$throttleGroups ) { + $error = 'abusefilter-edit-empty-throttlegroups'; + } else { + $valid = true; + // Groups should be unique in three ways: no direct duplicates like 'user' and 'user', + // no duplicated subgroups, not even shuffled ('ip,user' and 'user,ip') and no duplicates + // within subgroups ('user,ip,user') + $uniqueGroups = []; + $uniqueSubGroups = true; + // Every group should be valid, and subgroups should have valid groups inside + foreach ( $throttleGroups as $group ) { + if ( strpos( $group, ',' ) !== false ) { + $subGroups = explode( ',', $group ); + if ( $subGroups !== array_unique( $subGroups ) ) { + $uniqueSubGroups = false; + break; + } + foreach ( $subGroups as $subGroup ) { + if ( !in_array( $subGroup, $validGroups ) ) { + $valid = false; + break 2; + } + } + sort( $subGroups ); + $uniqueGroups[] = implode( ',', $subGroups ); + } else { + if ( !in_array( $group, $validGroups ) ) { + $valid = false; + break; + } + $uniqueGroups[] = $group; + } + } + + if ( !$valid ) { + $error = 'abusefilter-edit-invalid-throttlegroups'; + } elseif ( !$uniqueSubGroups || $uniqueGroups !== array_unique( $uniqueGroups ) ) { + $error = 'abusefilter-edit-duplicated-throttlegroups'; + } + } + + return $error; + } + /** * Checks whether user input for the filter editing form is valid and if so saves the filter * @@ -2269,6 +2339,15 @@ class AbuseFilter { } } + // If 'throttle' is selected, check its parameters + if ( !empty( $actions['throttle'] ) ) { + $throttleCheck = self::checkThrottleParameters( $actions['throttle']['parameters'] ); + if ( $throttleCheck !== null ) { + $validationStatus->error( $throttleCheck ); + return $validationStatus; + } + } + $differences = self::compareVersions( [ $newRow, $actions ], [ $newRow->mOriginalRow, $newRow->mOriginalActions ] @@ -2915,7 +2994,40 @@ class AbuseFilter { } elseif ( $action === 'throttle' ) { array_shift( $parameters ); list( $actions, $time ) = explode( ',', array_shift( $parameters ) ); - $groups = $wgLang->commaList( $parameters ); + + if ( $parameters === [ '' ] ) { + // Having empty groups won't happen for new filters due to validation upon saving, + // but old entries may have it. We'd better not show a broken message. Also, + // the array has an empty string inside because we haven't been passing an empty array + // as the default when retrieving wpFilterThrottleGroups with getArray (when it was + // a CheckboxMultiselect). + $groups = ''; + } else { + // Old entries may not have unique values. + $throttleGroups = array_unique( $parameters ); + // Join comma-separated groups in a commaList with a final "and", and convert to messages. + // Messages used here: abusefilter-throttle-ip, abusefilter-throttle-user, + // abusefilter-throttle-site, abusefilter-throttle-creationdate, abusefilter-throttle-editcount + // abusefilter-throttle-range, abusefilter-throttle-page + foreach ( $throttleGroups as &$val ) { + if ( strpos( $val, ',' ) !== false ) { + $subGroups = explode( ',', $val ); + foreach ( $subGroups as &$group ) { + $msg = wfMessage( "abusefilter-throttle-$group" ); + // We previously accepted literally everything in this field, so old entries + // may have weird stuff. + $group = $msg->exists() ? $msg->text() : $group; + } + unset( $group ); + $val = $wgLang->listToText( $subGroups ); + } else { + $msg = wfMessage( "abusefilter-throttle-$val" ); + $val = $msg->exists() ? $msg->text() : $val; + } + } + unset( $val ); + $groups = $wgLang->semicolonList( $throttleGroups ); + } $displayAction = self::getActionDisplay( $action ) . wfMessage( 'colon-separator' )->escaped() . wfMessage( 'abusefilter-throttle-details' )->params( $actions, $time, $groups )->escaped(); diff --git a/includes/Views/AbuseFilterViewEdit.php b/includes/Views/AbuseFilterViewEdit.php index 70cd16a15..866a2fdbb 100644 --- a/includes/Views/AbuseFilterViewEdit.php +++ b/includes/Views/AbuseFilterViewEdit.php @@ -566,47 +566,36 @@ class AbuseFilterViewEdit extends AbuseFilterView { 'label' => $this->msg( 'abusefilter-edit-throttle-period' )->text() ] ); - $throttleFields['abusefilter-edit-throttle-groups'] = + + $throttleConfig = [ + 'values' => $throttleGroups, + 'label' => $this->msg( 'abusefilter-edit-throttle-groups' )->parse(), + 'disabled' => $readOnlyAttrib + ]; + $this->getOutput()->addJsConfigVars( 'throttleConfig', $throttleConfig ); + + $hiddenGroups = new OOUI\FieldLayout( - new OOUI\CheckboxMultiselectInputWidget( [ - 'name' => 'wpFilterThrottleGroups[]', - 'value' => $throttleGroups, - 'options' => [ - [ - 'data' => 'ip', - 'label' => $this->msg( 'abusefilter-edit-throttle-ip' )->text() - ], - [ - 'data' => 'user', - 'label' => $this->msg( 'abusefilter-edit-throttle-user' )->text() - ], - [ - 'data' => 'range', - 'label' => $this->msg( 'abusefilter-edit-throttle-range' )->text() - ], - [ - 'data' => 'creationdate', - 'label' => $this->msg( 'abusefilter-edit-throttle-creationdate' )->text() - ], - [ - 'data' => 'editcount', - 'label' => $this->msg( 'abusefilter-edit-throttle-editcount' )->text() - ], - [ - 'data' => 'site', - 'label' => $this->msg( 'abusefilter-edit-throttle-site' )->text() - ], - [ - 'data' => 'page', - 'label' => $this->msg( 'abusefilter-edit-throttle-page' )->text() - ] - ] - ] + $readOnlyAttrib ), + new OOUI\MultilineTextInputWidget( [ + 'name' => 'wpFilterThrottleGroups', + 'value' => implode( "\n", $throttleGroups ), + 'rows' => 5, + 'placeholder' => $this->msg( 'abusefilter-edit-throttle-hidden-placeholder' )->text(), + 'infusable' => true, + 'id' => 'mw-abusefilter-hidden-throttle-field' + ] + $readOnlyAttrib + ), [ - 'label' => $this->msg( 'abusefilter-edit-throttle-groups' )->text(), - 'align' => 'right' + 'label' => new OOUI\HtmlSnippet( + $this->msg( 'abusefilter-edit-throttle-groups' )->parse() + ), + 'align' => 'top', + 'id' => 'mw-abusefilter-hidden-throttle' ] ); + + $throttleFields['abusefilter-edit-throttle-groups'] = $hiddenGroups; + $throttleSettings .= Xml::tags( 'div', @@ -753,10 +742,9 @@ class AbuseFilterViewEdit extends AbuseFilterView { $output .= $checkbox; $tagConfig = [ - 'tagUsed' => $tags, - 'tagLabel' => $this->msg( 'abusefilter-edit-tag-tag' )->parse(), - 'tagPlaceholder' => $this->msg( 'abusefilter-edit-tag-placeholder' )->text(), - 'tagDisabled' => $readOnlyAttrib + 'values' => $tags, + 'label' => $this->msg( 'abusefilter-edit-tag-tag' )->parse(), + 'disabled' => $readOnlyAttrib ]; $this->getOutput()->addJsConfigVars( 'tagConfig', $tagConfig ); @@ -768,7 +756,7 @@ class AbuseFilterViewEdit extends AbuseFilterView { 'rows' => 5, 'placeholder' => $this->msg( 'abusefilter-edit-tag-hidden-placeholder' )->text(), 'infusable' => true, - 'id' => 'mw-abusefilter-hidden-tags-field' + 'id' => 'mw-abusefilter-hidden-tag-field' ] + $readOnlyAttrib ), [ @@ -776,7 +764,7 @@ class AbuseFilterViewEdit extends AbuseFilterView { $this->msg( 'abusefilter-edit-tag-tag' )->parse() ), 'align' => 'top', - 'id' => 'mw-abusefilter-hidden-tags' + 'id' => 'mw-abusefilter-hidden-tag' ] ); $output .= @@ -1192,7 +1180,18 @@ class AbuseFilterViewEdit extends AbuseFilterView { // We need to load the parameters $throttleCount = $request->getIntOrNull( 'wpFilterThrottleCount' ); $throttlePeriod = $request->getIntOrNull( 'wpFilterThrottlePeriod' ); - $throttleGroups = $request->getArray( 'wpFilterThrottleGroups' ); + // First explode with \n, which is the delimiter used in the textarea + $rawGroups = explode( "\n", $request->getText( 'wpFilterThrottleGroups' ) ); + // Trim any space, both as an actual group and inside subgroups + $throttleGroups = []; + foreach ( $rawGroups as $group ) { + if ( strpos( $group, ',' ) !== false ) { + $subGroups = explode( ',', $group ); + $throttleGroups[] = implode( ',', array_map( 'trim', $subGroups ) ); + } elseif ( trim( $group ) !== '' ) { + $throttleGroups[] = trim( $group ); + } + } $parameters[0] = $this->mFilter; $parameters[1] = "$throttleCount,$throttlePeriod"; diff --git a/maintenance/normalizeThrottleParameters.php b/maintenance/normalizeThrottleParameters.php new file mode 100644 index 000000000..01c90592e --- /dev/null +++ b/maintenance/normalizeThrottleParameters.php @@ -0,0 +1,356 @@ +mDescription = 'Normalize AbuseFilter throttle parameters - T203587'; + $this->addOption( 'dry-run', 'Perform a dry run' ); + $this->requireExtension( 'Abuse Filter' ); + } + + /** + * @see Maintenance::getUpdateKey() + * @return string + */ + public function getUpdateKey() { + return __CLASS__; + } + + /** @var \Wikimedia\Rdbms\IDatabase $db The master database */ + private $dbw; + + /** + * Rollback the current transaction and emit a fatal error + * + * @param string $msg The message of the error + */ + protected function fail( $msg ) { + $this->rollbackTransaction( $this->dbw, __METHOD__ ); + $this->fatalError( $msg ); + } + + /** + * Get normalized throttle groups + * + * @param array $params Throttle parameters + * @return array[] The first element is the array of old throttle groups, the second + * is an array of formatted throttle groups + */ + private function getNewGroups( $params ) { + $validGroups = [ + 'ip', + 'user', + 'range', + 'creationdate', + 'editcount', + 'site', + 'page' + ]; + $rawGroups = array_slice( $params, 2 ); + $newGroups = []; + // We use a standard order to check for duplicates. This variable is not used as the actual + // array of groups to avoid silly changes like 'ip,user' => 'user,ip' + $normalizedGroups = []; + // Every group should be valid, and subgroups should have valid groups inside. Only keep + // valid (sub)groups. + foreach ( $rawGroups as $group ) { + if ( strpos( $group, ',' ) !== false ) { + // No duplicates in subgroups + $subGroups = array_unique( explode( ',', $group ) ); + $uniqueGroup = implode( ',', $subGroups ); + $valid = true; + foreach ( $subGroups as $subGroup ) { + if ( !in_array( $subGroup, $validGroups ) ) { + $valid = false; + break; + } + } + sort( $subGroups ); + if ( $valid && !in_array( $subGroups, $normalizedGroups ) ) { + $newGroups[] = $uniqueGroup; + $normalizedGroups[] = $subGroups; + } + } elseif ( in_array( $group, $validGroups ) ) { + $newGroups[] = $group; + $normalizedGroups[] = $group; + } + } + + // Remove duplicates + $newGroups = array_unique( $newGroups ); + + return [ $rawGroups, $newGroups ]; + } + + /** + * Check if throttle rate is malformed, i.e. if it has extra commas + * + * @param string $rate The throttle rate as saved in the DB ("count,period") + * @return string|null String with error type or null if the rate is valid + */ + private function checkThrottleRate( $rate ) { + if ( preg_match( '/^,/', $rate ) === 1 ) { + // The comma was inserted at least in throttle count. This behaves like if + // throttling isn't enabled, so just disable it + return 'disable'; + } elseif ( preg_match( '/^\d+,$/', $rate ) === 1 || preg_match( '/^\d+,\d+$/', $rate ) === 0 ) { + // First condition is for comma only inside throttle period. The behaviour in this case + // is unclear, ask users to fix this by hand. Second condition is for every other case; + // since it's unpredictable what the problem is, we just ask to fix it by hand. + return 'hand'; + } else { + return null; + } + } + + /** + * @see Maintenance::doDBUpdates + * @return bool + */ + public function doDBUpdates() { + $user = User::newSystemUser( 'AbuseFilter updater', [ 'steal' => true ] ); + $this->dbw = wfGetDB( DB_MASTER ); + $dryRun = $this->hasOption( 'dry-run' ); + + $this->beginTransaction( $this->dbw, __METHOD__ ); + + // Only select throttle actions + $actionRows = $this->dbw->select( + 'abuse_filter_action', + [ 'afa_filter', 'afa_parameters' ], + [ 'afa_consequence' => 'throttle' ], + __METHOD__, + [ 'LOCK IN SHARE MODE' ] + ); + + $newActionRows = []; + $deleteActionIDs = []; + $changeActionIDs = []; + foreach ( $actionRows as $actRow ) { + $filter = $actRow->afa_filter; + $params = explode( "\n", $actRow->afa_parameters ); + $rateCheck = $this->checkThrottleRate( $params[1] ); + if ( $rateCheck === 'hand' ) { + $this->fail( + "Throttle count and period for filter $filter are malformed, probably due to extra commas. " . + "Please fix them by hand in the way they're meant to be, then launch the script again." + ); + } + list( $oldGroups, $newGroups ) = $this->getNewGroups( $params ); + + if ( $rateCheck === 'disable' || count( $newGroups ) === 0 ) { + // Empty throttle groups, disable throttle for the filter + $deleteActionIDs[] = $actRow->afa_filter; + } elseif ( $oldGroups !== $newGroups ) { + $newParams = array_merge( array_slice( $params, 0, 2 ), $newGroups ); + $newActionRows[] = [ + 'afa_filter' => $actRow->afa_filter, + 'afa_consequence' => 'throttle', + 'afa_parameters' => implode( "\n", $newParams ) + ]; + $changeActionIDs[] = $actRow->afa_filter; + } + } + + $changeActionCount = count( $changeActionIDs ); + if ( $changeActionCount ) { + if ( $dryRun ) { + $this->output( + "normalizeThrottleParameter has found $changeActionCount rows to change in " . + "abuse_filter_action for the following IDs: " . implode( ', ', $changeActionIDs ) . "\n" + ); + } else { + $this->dbw->replace( + 'abuse_filter_action', + [ [ 'afa_filter', 'afa_consequence' ] ], + $newActionRows, + __METHOD__ + ); + } + } + + $deleteActionCount = count( $deleteActionIDs ); + // Use the same timestamps in abuse_filter and abuse_filter_history, since this is + // what we do in the actual code. + $timestamps = []; + if ( $deleteActionCount ) { + if ( $dryRun ) { + $this->output( + "normalizeThrottleParameter has found $deleteActionCount rows to delete in " . + "abuse_filter_action and update in abuse_filter for the following IDs: " . + implode( ', ', $deleteActionIDs ) . "\n" + ); + } else { + // Delete rows in abuse_filter_action + $this->dbw->delete( + 'abuse_filter_action', + [ + 'afa_consequence' => 'throttle', + 'afa_filter' => $deleteActionIDs + ], + __METHOD__ + ); + // Update abuse_filter. abuse_filter_history done later + $rows = $this->dbw->select( + 'abuse_filter', + '*', + [ 'af_id' => $deleteActionIDs ], + __METHOD__ + ); + foreach ( $rows as $row ) { + $oldActions = explode( ',', $row->af_actions ); + $actions = array_diff( $oldActions, [ 'throttle' ] ); + $timestamps[ $row->af_id ] = $this->dbw->timestamp( wfTimestampNow() ); + $newRow = [ + 'af_user' => $user->getId(), + 'af_user_text' => $user->getName(), + 'af_timestamp' => $timestamps[ $row->af_id ], + 'af_actions' => implode( ',', $actions ), + ] + $row; + + $this->dbw->replace( + 'abuse_filter', + [ 'af_id' ], + $newRow, + __METHOD__ + ); + } + } + } + $affectedActionRows = $changeActionCount + $deleteActionCount; + + $touchedIDs = array_merge( $changeActionIDs, $deleteActionIDs ); + if ( count( $touchedIDs ) === 0 ) { + $this->output( "No throttle parameters to normalize.\n" ); + $this->commitTransaction( $this->dbw, __METHOD__ ); + return !$dryRun; + } + // Create new history rows for every changed filter + $historyIDs = $this->dbw->selectFieldValues( + 'abuse_filter_history', + 'MAX(afh_id)', + [ 'afh_filter' => $touchedIDs ], + __METHOD__, + [ 'GROUP BY' => 'afh_filter', 'LOCK IN SHARE MODE' ] + ); + + $lastHistoryRows = $this->dbw->select( + 'abuse_filter_history', + [ + 'afh_filter', + 'afh_user', + 'afh_user_text', + 'afh_timestamp', + 'afh_pattern', + 'afh_comments', + 'afh_flags', + 'afh_public_comments', + 'afh_actions', + 'afh_deleted', + 'afh_changed_fields', + 'afh_group' + ], + [ 'afh_id' => $historyIDs ], + __METHOD__, + [ 'LOCK IN SHARE MODE' ] + ); + + $newHistoryRows = []; + $changeHistoryFilters = []; + foreach ( $lastHistoryRows as $histRow ) { + $actions = unserialize( $histRow->afh_actions ); + $filter = $histRow->afh_filter; + $rateCheck = $this->checkThrottleRate( $actions['throttle'][1] ); + if ( $rateCheck === 'hand' ) { + $this->fail( + "Throttle count and period for filter $filter are malformed, probably due to extra commas. " . + "Please fix them by hand in the way they're meant to be, then launch the script again." + ); + } + list( $oldGroups, $newGroups ) = $this->getNewGroups( $actions['throttle'] ); + + $fixedRowSection = [ + 'afh_id' => $this->dbw->nextSequenceValue( 'abuse_filter_af_id_seq' ), + 'afh_user' => $user->getId(), + 'afh_user_text' => $user->getName(), + 'afh_timestamp' => $timestamps[ $filter ] ?? $this->dbw->timestamp( wfTimestampNow() ), + 'afh_changed_fields' => 'actions', + ] + get_object_vars( $histRow ); + + if ( $rateCheck === 'disable' || count( $newGroups ) === 0 ) { + // Empty throttle groups, disable throttle for the filter + unset( $actions['throttle'] ); + $newHistoryRows[] = [ + 'afh_actions' => serialize( $actions ) + ] + $fixedRowSection; + $changeHistoryFilters[] = $filter; + } elseif ( $oldGroups !== $newGroups ) { + $actions['throttle'] = array_merge( array_slice( $actions['throttle'], 0, 2 ), $newGroups ); + $newHistoryRows[] = [ + 'afh_actions' => serialize( $actions ) + ] + $fixedRowSection; + $changeHistoryFilters[] = $filter; + } + } + + $historyCount = count( $changeHistoryFilters ); + $sanityCheck = $historyCount === $affectedActionRows; + if ( !$sanityCheck ) { + // Something went wrong. + $this->fail( + "The amount of affected rows isn't equal for abuse_filter_action and abuse_filter history. " . + "Found $affectedActionRows for the former and $historyCount for the latter." + ); + } + if ( count( $newHistoryRows ) ) { + if ( $dryRun ) { + $this->output( + "normalizeThrottleParameter would insert $historyCount rows in abuse_filter_history" . + " for the following filters: " . implode( ', ', $changeHistoryFilters ) . "\n" + ); + } else { + $this->dbw->insert( + 'abuse_filter_history', + $newHistoryRows, + __METHOD__ + ); + } + } + + $this->commitTransaction( $this->dbw, __METHOD__ ); + $changed = $affectedActionRows + $historyCount; + $resultMsg = $dryRun ? + "Throttle parameter normalization would change a total of $changed rows.\n" : + "Throttle parameters successfully normalized. Changed $changed rows.\n"; + $this->output( $resultMsg ); + return !$dryRun; + } +} + +$maintClass = 'NormalizeThrottleParameters'; +require_once RUN_MAINTENANCE_IF_MAIN; diff --git a/modules/ext.abuseFilter.edit.js b/modules/ext.abuseFilter.edit.js index ecf46ee02..930081391 100644 --- a/modules/ext.abuseFilter.edit.js +++ b/modules/ext.abuseFilter.edit.js @@ -358,47 +358,68 @@ } } + /** + * Builds a TagMultiselectWidget, to be used both for throttle groups and change tags + * + * @param {string} action Either 'throttle' or 'tag', will be used to build element IDs + * @param {Array} config The array with configuration passed from PHP code + */ + function buildSelector( action, config ) { + var disabled = config.disabled.length !== 0, + // mw-abusefilter-throttle-parameters, mw-abusefilter-tag-parameters + $container = $( '#mw-abusefilter-' + action + '-parameters' ), + // Character used to separate elements in the textarea. + separator = action === 'throttle' ? '\n' : ',', + selector, field, hiddenField; + + selector = + new OO.ui.TagMultiselectWidget( { + inputPosition: 'outline', + allowArbitrary: true, + allowEditTags: true, + selected: config.values, + // abusefilter-edit-throttle-placeholder, abusefilter-edit-tag-placeholder + placeholder: OO.ui.msg( 'abusefilter-edit-' + action + '-placeholder' ), + disabled: disabled + } ); + field = + new OO.ui.FieldLayout( + selector, + { + label: $( $.parseHTML( config.label ) ), + align: 'top' + } + ); + + // mw-abusefilter-hidden-throttle-field, mw-abusefilter-hidden-tag-field + hiddenField = OO.ui.infuse( $( '#mw-abusefilter-hidden-' + action + '-field' ) ); + selector.on( 'change', function () { + hiddenField.setValue( selector.getValue().join( separator ) ); + } ); + + // mw-abusefilter-hidden-throttle, mw-abusefilter-hidden-tag + $( '#mw-abusefilter-hidden-' + action ).hide(); + $container.append( field.$element ); + } + // On ready initialization $( document ).ready( function () { var basePath, readOnly, $exportBox = $( '#mw-abusefilter-export' ), isFilterEditor = mw.config.get( 'isFilterEditor' ), tagConfig = mw.config.get( 'tagConfig' ), - $tagContainer, tagUsed, tagDisabled, tagSelector, tagField, - tagHiddenField, cbEnabled, cbDeleted; + throttleConfig = mw.config.get( 'throttleConfig' ), + cbEnabled, cbDeleted; if ( isFilterEditor ) { // Configure the actual editing interface if ( tagConfig ) { // Build the tag selector - $tagContainer = $( '#mw-abusefilter-tag-parameters' ); - tagUsed = tagConfig.tagUsed; - tagDisabled = tagConfig.tagDisabled.length !== 0; - // Input field for tags - tagSelector = - new OO.ui.TagMultiselectWidget( { - inputPosition: 'outline', - allowArbitrary: true, - allowEditTags: true, - selected: tagUsed, - placeholder: tagConfig.tagPlaceholder, - disabled: tagDisabled - } ); - tagField = - new OO.ui.FieldLayout( - tagSelector, - { - label: $( $.parseHTML( tagConfig.tagLabel ) ), - align: 'top' - } - ); - tagHiddenField = OO.ui.infuse( $( '#mw-abusefilter-hidden-tags-field' ) ); - tagSelector.on( 'change', function () { - tagHiddenField.setValue( tagSelector.getValue() ); - } ); - - $( '#mw-abusefilter-hidden-tags' ).hide(); - $tagContainer.append( tagField.$element ); + buildSelector( 'tag', tagConfig ); + } + if ( throttleConfig ) { + // Build the throttle groups selector + buildSelector( 'throttle', throttleConfig ); } toggleWarnPreviewButton = OO.ui.infuse( $( '#mw-abusefilter-warn-preview-button' ) ); diff --git a/tests/phpunit/AbuseFilterSaveTest.php b/tests/phpunit/AbuseFilterSaveTest.php index aca95213c..6a5b31212 100644 --- a/tests/phpunit/AbuseFilterSaveTest.php +++ b/tests/phpunit/AbuseFilterSaveTest.php @@ -485,7 +485,112 @@ class AbuseFilterSaveTest extends MediaWikiTestCase { 'needsOtherFilters' => 1 ] ] + ], + [ + [ + 'filterParameters' => [ + 'rules' => '1==1', + 'description' => 'Invalid throttle groups', + 'notes' => 'Throttle... Again', + 'throttleEnabled' => true, + 'throttleCount' => 11, + 'throttlePeriod' => 111, + 'throttleGroups' => 'user\nfoo' + ], + 'testData' => [ + 'doingWhat' => 'Trying to save a filter with invalid throttle groups', + 'expectedResult' => 'an error message saying that throttle groups are invalid', + 'expectedMessage' => 'abusefilter-edit-invalid-throttlegroups', + 'shouldFail' => true, + 'shouldBeSaved' => false, + 'customUserGroup' => '', + 'needsOtherFilters' => false + ] + ] ] ]; } + + /** + * Check that our tag validation is working properly. Note that we only need one test + * for each called function. Consistency within ChangeTags functions should be + * assured by tests in core. The test for canAddTagsAccompanyingChange and canCreateTag + * are missing because they won't actually fail, never. Resolving T173917 would + * greatly improve the situation and could help writing better tests. + * + * @param string $tag The tag to validate + * @param string|null $error The expected error message. Null if validations should pass + * @covers AbuseFilter::isAllowedTag + * @dataProvider provideTags + */ + public function testIsAllowedTag( $tag, $error ) { + $status = AbuseFilter::isAllowedTag( $tag ); + + if ( !$status->isGood() ) { + $actualError = $status->getErrors(); + $actualError = $actualError[0]['message']; + } else { + $actualError = null; + if ( $error !== null ) { + $this->fail( "Tag validation returned a valid status instead of the expected '$error' error." ); + } + } + + $this->assertSame( + $error, + $actualError, + "Expected message '$error', got '$actualError' while validating the tag '$tag'." + ); + } + + /** + * Data provider for testIsAllowedTag + * @return array + */ + public function provideTags() { + return [ + [ 'a|b', 'tags-create-invalid-chars' ], + [ 'mw-undo', 'abusefilter-edit-bad-tags' ], + [ 'abusefilter-condition-limit', 'abusefilter-tag-reserved' ], + [ 'my_tag', null ], + ]; + } + + /** + * Check that throttle parameters validation works fine + * + * @param array $params Throttle parameters + * @param string|null $error The expected error message. Null if validations should pass + * @covers AbuseFilter::checkThrottleParameters + * @dataProvider provideThrottleParameters + */ + public function testCheckThrottleParameters( $params, $error ) { + $result = AbuseFilter::checkThrottleParameters( $params ); + $this->assertSame( $error, $result, 'Throttle parameter validation does not work as expected.' ); + } + + /** + * Data provider for testCheckThrottleParameters + * @return array + */ + public function provideThrottleParameters() { + return [ + [ [ '1', '5,23', 'user', 'ip', 'page,range', 'ip,user', 'range,ip' ], null ], + [ [ '1', '5.3,23', 'user', 'ip' ], 'abusefilter-edit-invalid-throttlecount' ], + [ [ '1', '-3,23', 'user', 'ip' ], 'abusefilter-edit-invalid-throttlecount' ], + [ [ '1', '5,2.3', 'user', 'ip' ], 'abusefilter-edit-invalid-throttleperiod' ], + [ [ '1', '4,-14', 'user', 'ip' ], 'abusefilter-edit-invalid-throttleperiod' ], + [ [ '1', '3,33' ], 'abusefilter-edit-empty-throttlegroups' ], + [ [ '1', '3,33', 'user', 'ip,foo,user' ], 'abusefilter-edit-invalid-throttlegroups' ], + [ [ '1', '3,33', 'foo', 'ip,user' ], 'abusefilter-edit-invalid-throttlegroups' ], + [ [ '1', '3,33', 'foo', 'ip,user,bar' ], 'abusefilter-edit-invalid-throttlegroups' ], + [ [ '1', '3,33', 'user', 'ip,page,user' ], null ], + [ + [ '1', '3,33', 'ip', 'user','user,ip', 'ip,user', 'user,ip,user', 'user', 'ip,ip,user' ], + 'abusefilter-edit-duplicated-throttlegroups' + ], + [ [ '1', '3,33', 'ip,ip,user' ], 'abusefilter-edit-duplicated-throttlegroups' ], + [ [ '1', '3,33', 'user,ip', 'ip,user' ], 'abusefilter-edit-duplicated-throttlegroups' ], + ]; + } } diff --git a/tests/phpunit/AbuseFilterTest.php b/tests/phpunit/AbuseFilterTest.php index e5514a8a0..bb2fa614d 100644 --- a/tests/phpunit/AbuseFilterTest.php +++ b/tests/phpunit/AbuseFilterTest.php @@ -525,51 +525,6 @@ class AbuseFilterTest extends MediaWikiTestCase { ]; } - /** - * Check that our tag validation is working properly. Note that we only need one test - * for each called function. Consistency within ChangeTags functions should be - * assured by tests in core. The test for canAddTagsAccompanyingChange and canCreateTag - * are missing because they won't actually fail, never. Resolving T173917 would - * greatly improve the situation and could help writing better tests. - * - * @param string $tag The tag to validate - * @param string|null $error The expected error message. Null if validations should pass - * @covers AbuseFilter::isAllowedTag - * @dataProvider provideTags - */ - public function testIsAllowedTag( $tag, $error ) { - $status = AbuseFilter::isAllowedTag( $tag ); - - if ( !$status->isGood() ) { - $actualError = $status->getErrors(); - $actualError = $actualError[0]['message']; - } else { - $actualError = null; - if ( $error !== null ) { - $this->fail( "Tag validation returned a valid status instead of the expected '$error' error." ); - } - } - - $this->assertSame( - $error, - $actualError, - "Expected message '$error', got '$actualError' while validating the tag '$tag'." - ); - } - - /** - * Data provider for testIsAllowedTag - * @return array - */ - public function provideTags() { - return [ - [ 'a|b', 'tags-create-invalid-chars' ], - [ 'mw-undo', 'abusefilter-edit-bad-tags' ], - [ 'abusefilter-condition-limit', 'abusefilter-tag-reserved' ], - [ 'my_tag', null ], - ]; - } - /** * Check that version comparing works well *