diff --git a/i18n/en.json b/i18n/en.json index b189bfe61..fc2addd47 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -209,6 +209,7 @@ "abusefilter-throttle-editcount": "edit count", "abusefilter-throttle-site": "whole site", "abusefilter-throttle-page": "page", + "abusefilter-throttle-none": "(none)", "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", diff --git a/i18n/qqq.json b/i18n/qqq.json index effc1ece1..9c05fb55e 100644 --- a/i18n/qqq.json +++ b/i18n/qqq.json @@ -243,6 +243,7 @@ "abusefilter-throttle-editcount": "Throttle option.", "abusefilter-throttle-site": "Throttle option.", "abusefilter-throttle-page": "Throttle option.", + "abusefilter-throttle-none": "Bogus throttle option, means that no options are enabled.", "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}}", diff --git a/includes/AbuseFilter.php b/includes/AbuseFilter.php index 73e3eb578..00d411ffa 100644 --- a/includes/AbuseFilter.php +++ b/includes/AbuseFilter.php @@ -3143,37 +3143,29 @@ class AbuseFilter { array_shift( $parameters ); list( $actions, $time ) = explode( ',', array_shift( $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 { - // 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 ( $parameters 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 = $lang->listToText( $subGroups ); - } else { - $msg = wfMessage( "abusefilter-throttle-$val" ); - $val = $msg->exists() ? $msg->text() : $val; + // 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, abusefilter-throttle-none + foreach ( $parameters 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 = $lang->listToText( $subGroups ); + } else { + $msg = wfMessage( "abusefilter-throttle-$val" ); + $val = $msg->exists() ? $msg->text() : $val; } - unset( $val ); - $groups = $lang->semicolonList( $parameters ); } + unset( $val ); + $groups = $lang->semicolonList( $parameters ); + $displayAction = self::getActionDisplay( $action ) . wfMessage( 'colon-separator' )->escaped() . wfMessage( 'abusefilter-throttle-details' )->params( $actions, $time, $groups )->escaped(); diff --git a/maintenance/normalizeThrottleParameters.php b/maintenance/normalizeThrottleParameters.php index 91a9ae425..aa4cb07de 100644 --- a/maintenance/normalizeThrottleParameters.php +++ b/maintenance/normalizeThrottleParameters.php @@ -11,6 +11,10 @@ * highly unlikely to happen anyway. (T203585) * - If throttle groups are empty (or only contain unknown keywords), ask users to fix every case * by hand. (T203584) + * - Change some edge cases of throttle parameters saved in abuse_filter_history (T215787): + * - parameters = null ==> parameters = [ filterID, "0,0", 'none' ] + * - at least a number missing from parameters[1] ==> insert 0 in place of the missing param + * - empty groups ==> 'none' (special case, uses the message abusefilter-throttle-none) * * @ingroup Maintenance */ @@ -41,7 +45,7 @@ class NormalizeThrottleParameters extends LoggedUpdateMaintenance { return __CLASS__; } - /** @var \Wikimedia\Rdbms\IDatabase $db The master database */ + /** @var \Wikimedia\Rdbms\Database $dbw The master database */ private $dbw; /** @@ -133,12 +137,12 @@ class NormalizeThrottleParameters extends LoggedUpdateMaintenance { } /** - * @see Maintenance::doDBUpdates - * @return bool + * Main logic of parameters normalization + * + * @return int Amount of normalized rows */ - public function doDBUpdates() { + protected function normalizeParameters() { $user = AbuseFilter::getFilterUser(); - $this->dbw = wfGetDB( DB_MASTER ); $dryRun = $this->hasOption( 'dry-run' ); // IDs of filters with invalid rate (count or period) @@ -152,8 +156,6 @@ class NormalizeThrottleParameters extends LoggedUpdateMaintenance { // holds an empty string and (unserialize(afh_actions))['throttle'] is null. $totallyEmpty = []; - $this->beginTransaction( $this->dbw, __METHOD__ ); - // Only select throttle actions $actionRows = $this->dbw->select( 'abuse_filter_action', @@ -320,8 +322,7 @@ class NormalizeThrottleParameters extends LoggedUpdateMaintenance { $touchedIDs = array_merge( $changeActionIDs, $deleteActionIDs ); if ( count( $touchedIDs ) === 0 ) { $this->output( "No throttle parameters to normalize.\n" ); - $this->commitTransaction( $this->dbw, __METHOD__ ); - return !$dryRun; + return 0; } // Create new history rows for every changed filter @@ -396,13 +397,97 @@ class NormalizeThrottleParameters extends LoggedUpdateMaintenance { ); } } + return $affectedActionRows + $historyCount; + } + + /** + * Beautify empty/missing/corrupted parameters in abuse_filter_history + * + * @return int Amount of beautified rows + */ + protected function beautifyHistory() { + $dryRun = $this->hasOption( 'dry-run' ); + + // We need any row containing throttle, but there's no + // need to lock as these rows aren't changed by the actual code. + $likeClause = $this->dbw->buildLike( + $this->dbw->anyString(), + 'throttle', + $this->dbw->anyString() + ); + $histRows = $this->dbw->select( + 'abuse_filter_history', + [ 'afh_id', 'afh_actions', 'afh_filter' ], + [ 'afh_actions ' . $likeClause ], + __METHOD__ + ); + + $beautyIDs = []; + foreach ( $histRows as $row ) { + $acts = unserialize( $row->afh_actions ); + if ( !array_key_exists( 'throttle', $acts ) ) { + // The LIKE clause is very raw, so this could happen + continue; + } + + if ( $acts['throttle'] === null ) { + // Corrupted row, rebuild it (T215787) + $acts['throttle'] = [ $row->afh_filter, '0,0', 'none' ]; + } elseif ( $this->checkThrottleRate( $acts['throttle'][1] ) !== null ) { + // Missing count, make it explicitly 0 + $acts['throttle'][1] = preg_replace( '/^,/', '0,', $acts['throttle'][1] ); + // Missing period, make it explicitly 0 + $acts['throttle'][1] = preg_replace( '/,$/', ',0', $acts['throttle'][1] ); + } elseif ( count( $acts['throttle'] ) === 2 ) { + // Missing groups, make them explicitly "none" (special group) + $acts['throttle'][] = 'none'; + } else { + // Everything's fine! + continue; + } + + $beautyIDs[] = $row->afh_id; + if ( !$dryRun ) { + $this->dbw->update( + 'abuse_filter_history', + [ 'afh_actions' => serialize( $acts ) ], + [ 'afh_id' => $row->afh_id ], + __METHOD__ + ); + } + } + + $changed = count( $beautyIDs ); + if ( $changed ) { + $verb = $dryRun ? 'would beautify' : 'beautified'; + $this->output( + "normalizeThrottleParameter $verb $changed rows in abuse_filter_history" . + " for the following history IDs: " . implode( ', ', $beautyIDs ) . "\n" + ); + } + return $changed; + } + + /** + * @inheritDoc + */ + public function doDBUpdates() { + $dryRun = $this->hasOption( 'dry-run' ); + $this->dbw = wfGetDB( DB_MASTER ); + $this->beginTransaction( $this->dbw, __METHOD__ ); + + $normalized = $this->normalizeParameters(); + $beautified = $this->beautifyHistory(); $this->commitTransaction( $this->dbw, __METHOD__ ); - $changed = $affectedActionRows + $historyCount; + + $changed = $normalized + $beautified; + $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; } }