mirror of
https://gerrit.wikimedia.org/r/mediawiki/extensions/AbuseFilter.git
synced 2024-11-23 21:53:35 +00:00
Fix Status combining MessageSpecifier and parameters array
Constructing a Status like this does not make sense (and I want to deprecate it in I0675e557bb93a1c990fa923c50b9f6ee8a9836c8), because the parameters are ignored by most Status methods: $error = Message::newFromSpecifier( 'abusefilter-blocked-domains-attempted' ); $status = Status::newFatal( $error, 'blockeddomain', 'blockeddomain' ); But it worked here, because FilteredActionsHandler::getApiStatus() used a deprecated method that allowed inspecting them. I feel like this code in BlockedDomainFilter has been added to make the tests pass without thinking about what it actually does, which is to output a bunch of useless incorrect data in API errors. I'm not sure if we can remove that now without breaking API compatibility, so add the useless data in FilteredActionsHandler instead, closer to where it's output. Change-Id: Ic12241bd3029bc1b0e7a0023689a2be35ccd30a8
This commit is contained in:
parent
a5e0851dc0
commit
c458651370
|
@ -12,7 +12,7 @@
|
|||
"license-name": "GPL-2.0-or-later",
|
||||
"type": "antispam",
|
||||
"requires": {
|
||||
"MediaWiki": ">= 1.42.0"
|
||||
"MediaWiki": ">= 1.43.0"
|
||||
},
|
||||
"AvailableRights": [
|
||||
"abusefilter-modify",
|
||||
|
|
|
@ -106,9 +106,7 @@ class BlockedDomainFilter {
|
|||
$blockedDomainsAdded = array_keys( $blockedDomainsAdded );
|
||||
$error = Message::newFromSpecifier( 'abusefilter-blocked-domains-attempted' );
|
||||
$error->params( Message::listParam( $blockedDomainsAdded ) );
|
||||
|
||||
$status = Status::newFatal( $error, 'blockeddomain', 'blockeddomain' );
|
||||
$status->value['blockeddomain'] = [ 'disallow' ];
|
||||
$status->fatal( $error );
|
||||
$this->logFilterHit(
|
||||
$user,
|
||||
$title,
|
||||
|
|
|
@ -94,13 +94,8 @@ class FilteredActionsHandler implements
|
|||
);
|
||||
}
|
||||
|
||||
$filterResult = $this->filterEdit( $context, $user, $content, $summary, $slot );
|
||||
$this->filterEdit( $context, $user, $content, $summary, $slot, $status );
|
||||
|
||||
if ( !$filterResult->isOK() ) {
|
||||
// Produce a useful error message for API edits
|
||||
$filterResultApi = self::getApiStatus( $filterResult );
|
||||
$status->merge( $filterResultApi );
|
||||
}
|
||||
$this->statsDataFactory->timing( 'timing.editAbuseFilter', microtime( true ) - $startTime );
|
||||
|
||||
return $status->isOK();
|
||||
|
@ -114,15 +109,16 @@ class FilteredActionsHandler implements
|
|||
* @param Content $content the new Content generated by the edit
|
||||
* @param string $summary Edit summary for page
|
||||
* @param string $slot slot role for the content
|
||||
* @return Status
|
||||
* @param Status $status
|
||||
*/
|
||||
private function filterEdit(
|
||||
IContextSource $context,
|
||||
User $user,
|
||||
Content $content,
|
||||
string $summary,
|
||||
string $slot = SlotRecord::MAIN
|
||||
): Status {
|
||||
string $slot,
|
||||
Status $status
|
||||
): void {
|
||||
$this->editRevUpdater->clearLastEditPage();
|
||||
|
||||
$title = $context->getTitle();
|
||||
|
@ -130,13 +126,13 @@ class FilteredActionsHandler implements
|
|||
if ( $title === null ) {
|
||||
// T144265: This *should* never happen.
|
||||
$logger->warning( __METHOD__ . ' received a null title.' );
|
||||
return Status::newGood();
|
||||
return;
|
||||
}
|
||||
if ( !$title->canExist() ) {
|
||||
// This also should be handled in EditPage or whoever is calling the hook.
|
||||
$logger->warning( __METHOD__ . ' received a Title that cannot exist.' );
|
||||
// Note that if the title cannot exist, there's no much point in filtering the edit anyway
|
||||
return Status::newGood();
|
||||
return;
|
||||
}
|
||||
|
||||
$page = $context->getWikiPage();
|
||||
|
@ -145,25 +141,42 @@ class FilteredActionsHandler implements
|
|||
$vars = $builder->getEditVars( $content, $summary, $slot, $page );
|
||||
if ( $vars === null ) {
|
||||
// We don't have to filter the edit
|
||||
return Status::newGood();
|
||||
return;
|
||||
}
|
||||
$runner = $this->filterRunnerFactory->newRunner( $user, $title, $vars, 'default' );
|
||||
$filterResult = $runner->run();
|
||||
if ( !$filterResult->isOK() ) {
|
||||
return $filterResult;
|
||||
// Produce a useful error message for API edits
|
||||
$filterResultApi = self::getApiStatus( $filterResult );
|
||||
$status->merge( $filterResultApi );
|
||||
return;
|
||||
}
|
||||
|
||||
$this->editRevUpdater->setLastEditPage( $page );
|
||||
|
||||
if ( $this->permissionManager->userHasRight( $user, 'abusefilter-bypass-blocked-external-domains' ) ) {
|
||||
return Status::newGood();
|
||||
return;
|
||||
}
|
||||
$blockedDomainFilterResult = $this->blockedDomainFilter->filter( $vars, $user, $title );
|
||||
if ( !$blockedDomainFilterResult->isOK() ) {
|
||||
return $blockedDomainFilterResult;
|
||||
// Produce a useful error message for API edits
|
||||
// FIXME: This is not useful, but it was added by accident :(
|
||||
foreach ( $blockedDomainFilterResult->getMessages() as $msg ) {
|
||||
if ( $msg->getKey() === 'abusefilter-blocked-domains-attempted' ) {
|
||||
$blockedDomainFilterResult->replaceMessage(
|
||||
'abusefilter-blocked-domains-attempted',
|
||||
ApiMessage::create( $msg, 'abusefilter-disallowed', [
|
||||
'abusefilter' => [
|
||||
'id' => 'blockeddomain',
|
||||
'description' => 'blockeddomain',
|
||||
'actions' => 'disallow',
|
||||
],
|
||||
] )
|
||||
);
|
||||
}
|
||||
}
|
||||
$status->merge( $blockedDomainFilterResult );
|
||||
}
|
||||
|
||||
return Status::newGood();
|
||||
}
|
||||
|
||||
/**
|
||||
|
@ -174,8 +187,8 @@ class FilteredActionsHandler implements
|
|||
$allActionsTaken = $status->getValue();
|
||||
$statusForApi = Status::newGood();
|
||||
|
||||
foreach ( $status->getErrors() as $error ) {
|
||||
[ $filterDescription, $filter ] = $error['params'];
|
||||
foreach ( $status->getMessages() as $msg ) {
|
||||
[ $filterDescription, $filter ] = $msg->getParams();
|
||||
$actionsTaken = $allActionsTaken[ $filter ];
|
||||
|
||||
$code = ( $actionsTaken === [ 'warn' ] ) ? 'abusefilter-warning' : 'abusefilter-disallowed';
|
||||
|
@ -187,7 +200,7 @@ class FilteredActionsHandler implements
|
|||
],
|
||||
];
|
||||
|
||||
$message = ApiMessage::create( $error, $code, $data );
|
||||
$message = ApiMessage::create( $msg, $code, $data );
|
||||
$statusForApi->fatal( $message );
|
||||
}
|
||||
|
||||
|
@ -298,7 +311,7 @@ class FilteredActionsHandler implements
|
|||
}
|
||||
$blockedDomainFilterResult = $this->blockedDomainFilter->filter( $vars, $user, $title );
|
||||
if ( !$blockedDomainFilterResult->isOK() ) {
|
||||
$error = $blockedDomainFilterResult->getErrors()[0]['message'];
|
||||
$error = $blockedDomainFilterResult->getMessages()[0];
|
||||
return $blockedDomainFilterResult->isOK();
|
||||
}
|
||||
}
|
||||
|
|
Loading…
Reference in a new issue