wikimedia/mediawiki-extensions-AbuseFilter
1
0
Fork 0
mirror of https://gerrit.wikimedia.org/r/mediawiki/extensions/AbuseFilter.git synced 2024-09-23 10:18:22 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fix big problems with normalizeThrottleParameters

Browse source 
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
This commit is contained in:
Daimona Eaytoy 2018-11-28 12:31:40 +01:00
parent d7629efb7c
commit aa280998c0
2 changed files with 74 additions and 28 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

10
includes/AbuseFilter.php
View file

@ -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() .

92
maintenance/normalizeThrottleParameters.php
View file

@ -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 )