From aa280998c0644184504f6df8516e97eca0b17370 Mon Sep 17 00:00:00 2001 From: Daimona Eaytoy Date: Wed, 28 Nov 2018 12:31:40 +0100 Subject: [PATCH] Fix big problems with normalizeThrottleParameters My final testing unveiled 4 problems, see T209565#4780868. Testing again after this patch yields the expected outcome. Update: A fifth problem is that we cannot disable throttling if throttle groups are empty or fully invalid: that case is similar to the one with invalid rate, the throttle limit is never reached and thus throttle just doesn't work. Instead, ask users to fix it by hand. Bug: T203336 Bug: T209565 Change-Id: Id03c9880f60764efc596ac40b8662087fdb30550 --- includes/AbuseFilter.php | 10 +-- maintenance/normalizeThrottleParameters.php | 92 ++++++++++++++++----- 2 files changed, 74 insertions(+), 28 deletions(-) diff --git a/includes/AbuseFilter.php b/includes/AbuseFilter.php index 69f5fabba..1b4fa361e 100644 --- a/includes/AbuseFilter.php +++ b/includes/AbuseFilter.php @@ -1198,7 +1198,7 @@ class AbuseFilter { $log_template = [ 'afl_user' => $user->getId(), 'afl_user_text' => $user->getName(), - 'afl_timestamp' => wfGetDB( DB_REPLICA )->timestamp( wfTimestampNow() ), + 'afl_timestamp' => wfGetDB( DB_REPLICA )->timestamp(), 'afl_namespace' => $title->getNamespace(), 'afl_title' => $title->getDBkey(), 'afl_action' => $action, @@ -2453,7 +2453,7 @@ class AbuseFilter { $newRow = get_object_vars( $newRow ); // Set last modifier. - $newRow['af_timestamp'] = $dbw->timestamp( wfTimestampNow() ); + $newRow['af_timestamp'] = $dbw->timestamp(); $newRow['af_user'] = $user->getId(); $newRow['af_user_text'] = $user->getName(); @@ -3027,13 +3027,11 @@ class AbuseFilter { // 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 ) { + foreach ( $parameters as &$val ) { if ( strpos( $val, ',' ) !== false ) { $subGroups = explode( ',', $val ); foreach ( $subGroups as &$group ) { @@ -3050,7 +3048,7 @@ class AbuseFilter { } } unset( $val ); - $groups = $wgLang->semicolonList( $throttleGroups ); + $groups = $wgLang->semicolonList( $parameters ); } $displayAction = self::getActionDisplay( $action ) . wfMessage( 'colon-separator' )->escaped() . diff --git a/maintenance/normalizeThrottleParameters.php b/maintenance/normalizeThrottleParameters.php index 01c90592e..0836011d2 100644 --- a/maintenance/normalizeThrottleParameters.php +++ b/maintenance/normalizeThrottleParameters.php @@ -9,8 +9,8 @@ * like it would with throttle disabled, we just disable it. Otherwise, since we don't know what * the filter is meant to do, we just ask users to evaluate and fix every case by hand. This is * highly unlikely to happen anyway. (T203585) - * - Disables the throttle action for filters where throttle groups are empty (or only contain - * unknown keywords). Per T203336#4568819, in those case throttle already doesn't work. (T203584) + * - If throttle groups are empty (or only contain unknown keywords), ask users to fix every case + * by hand. (T203584) * * @ingroup Maintenance */ @@ -133,7 +133,7 @@ class NormalizeThrottleParameters extends LoggedUpdateMaintenance { * @return bool */ public function doDBUpdates() { - $user = User::newSystemUser( 'AbuseFilter updater', [ 'steal' => true ] ); + $user = AbuseFilter::getFilterUser(); $this->dbw = wfGetDB( DB_MASTER ); $dryRun = $this->hasOption( 'dry-run' ); @@ -155,16 +155,25 @@ class NormalizeThrottleParameters extends LoggedUpdateMaintenance { $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 + // If the rate is invalid or the groups are empty (or only contain invalid identifiers), + // it means that the throttle limit is never reached. Since we cannot guess what the + // filter should do, nor we want to impose a default, we ask to manually fix the problem. + if ( $rateCheck === 'hand' ) { + $this->fail( + "Throttle count and period for filter $filter are malformed or empty. " . + "Please fix them by hand in the way they're meant to be, then launch the script again." + ); + } elseif ( count( $newGroups ) === 0 ) { + $this->fail( + "Throttle groups are empty for filter $filter. Please add some groups or disable " . + "throttling, then launch the script again." + ); + } + + if ( $rateCheck === 'disable' ) { + // Invalid rate, disable throttle for the filter $deleteActionIDs[] = $actRow->afa_filter; } elseif ( $oldGroups !== $newGroups ) { $newParams = array_merge( array_slice( $params, 0, 2 ), $newGroups ); @@ -177,6 +186,9 @@ class NormalizeThrottleParameters extends LoggedUpdateMaintenance { } } + // Use the same timestamps in abuse_filter and abuse_filter_history, since this is + // what we do in the actual code. + $timestamps = []; $changeActionCount = count( $changeActionIDs ); if ( $changeActionCount ) { if ( $dryRun ) { @@ -191,13 +203,33 @@ class NormalizeThrottleParameters extends LoggedUpdateMaintenance { $newActionRows, __METHOD__ ); + // Touch the abuse_filter table to update the "filter last modified" field + $rows = $this->dbw->select( + 'abuse_filter', + '*', + [ 'af_id' => $changeActionIDs ], + __METHOD__, + [ 'FOR UPDATE' ] + ); + foreach ( $rows as $row ) { + $timestamps[ $row->af_id ] = $this->dbw->timestamp(); + $newRow = [ + 'af_user' => $user->getId(), + 'af_user_text' => $user->getName(), + 'af_timestamp' => $timestamps[ $row->af_id ] + ] + get_object_vars( $row ); + + $this->dbw->replace( + 'abuse_filter', + [ 'af_id' ], + $newRow, + __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( @@ -220,18 +252,19 @@ class NormalizeThrottleParameters extends LoggedUpdateMaintenance { 'abuse_filter', '*', [ 'af_id' => $deleteActionIDs ], - __METHOD__ + __METHOD__, + [ 'FOR UPDATE' ] ); foreach ( $rows as $row ) { $oldActions = explode( ',', $row->af_actions ); $actions = array_diff( $oldActions, [ 'throttle' ] ); - $timestamps[ $row->af_id ] = $this->dbw->timestamp( wfTimestampNow() ); + $timestamps[ $row->af_id ] = $this->dbw->timestamp(); $newRow = [ 'af_user' => $user->getId(), 'af_user_text' => $user->getName(), 'af_timestamp' => $timestamps[ $row->af_id ], 'af_actions' => implode( ',', $actions ), - ] + $row; + ] + get_object_vars( $row ); $this->dbw->replace( 'abuse_filter', @@ -286,24 +319,39 @@ class NormalizeThrottleParameters extends LoggedUpdateMaintenance { $actions = unserialize( $histRow->afh_actions ); $filter = $histRow->afh_filter; $rateCheck = $this->checkThrottleRate( $actions['throttle'][1] ); + list( $oldGroups, $newGroups ) = $this->getNewGroups( $actions['throttle'] ); + + // If the rate is invalid or the groups are empty (or only contain invalid identifiers), + // it means that the throttle limit is never reached. Since we cannot guess what the + // filter should do, nor we want to impose a default, we ask to manually fix the problem. if ( $rateCheck === 'hand' ) { $this->fail( - "Throttle count and period for filter $filter are malformed, probably due to extra commas. " . + "Throttle count and period for filter $filter are malformed or empty. " . "Please fix them by hand in the way they're meant to be, then launch the script again." ); + } elseif ( count( $newGroups ) === 0 ) { + $this->fail( + "Throttle groups are empty for filter $filter. Please add some groups or disable " . + "throttling, then launch the script again." + ); + } + + $timestamp = $timestamps[ $filter ] ?? null; + if ( !$timestamp && !$dryRun ) { + // Sanity check + $this->fail( "The timestamp wasn't saved for filter $filter" ); } - 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_timestamp' => $timestamp, 'afh_changed_fields' => 'actions', ] + get_object_vars( $histRow ); - if ( $rateCheck === 'disable' || count( $newGroups ) === 0 ) { - // Empty throttle groups, disable throttle for the filter + if ( $rateCheck === 'disable' ) { + // Invalid rate, disable throttle for the filter unset( $actions['throttle'] ); $newHistoryRows[] = [ 'afh_actions' => serialize( $actions )