wikimedia/mediawiki-extensions-AbuseFilter
1
0
Fork 0
mirror of https://gerrit.wikimedia.org/r/mediawiki/extensions/AbuseFilter.git synced 2024-11-27 15:30:42 +00:00
Code Issues Packages Projects Releases Wiki Activity

Actually return errors for action=edit API

Browse source 
Setting 'apiHookResult' results in a "successful" response; if we want
to report an error, we need to use ApiMessage. We already were doing
this for action=upload. Now our action=edit API responses will be
consistent with MediaWiki and other extensions, and will be able to
take advantage of errorformat=html.

Since this breaks compatibility anyway, also remove some redundant
backwards-compatibility values from the output.

To avoid user interface regressions in VisualEditor, the changes
I3b9c4fef (in VE) and I106dbd3c (in MediaWiki) should be merged first.

Before:
    {
        "edit": {
            "code": "abusefilter-disallowed",
            "message": {
                "key": "abusefilter-disallowed",
                "params": [ ... ]
            },
            "abusefilter": { ... },
            "info": "Hit AbuseFilter: Test filter disallow",
            "warning": "This action has been automatically identified ...",
            "result": "Failure"
        }
    }

After:
    {
        "errors": [
            {
                "code": "abusefilter-disallowed",
                "data": {
                    "abusefilter": { ... },
                },
                "module": "edit",
                "*": "This action has been automatically identified ..."
            }
        ],
        "*": "See http://localhost:3080/w/api.php for API usage. ..."
    }

For comparison, a 'readonly' error:
    {
        "errors": [
            {
                "code": "readonly",
                "data": {
                    "readonlyreason": "foo bar"
                },
                "module": "main",
                "*": "The wiki is currently in read-only mode."
            }
        ],
        "*": "See http://localhost:3080/w/api.php for API usage. ..."
    }

Bug: T229539
Depends-On: I106dbd3cbdbf7082b1d1f1c1106ece6b19c22a86
Depends-On: I3b9c4fefc0869ef7999c21cef754434febd852ec
Change-Id: I5424de387cbbcc9c85026b8cfeaf01635eee34a0
This commit is contained in:
Bartosz Dziewoński 2019-08-01 03:48:08 +02:00
parent 4c8dac4dc6
commit 82b6f191d4
3 changed files with 38 additions and 54 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

76
includes/AbuseFilterHooks.php
View file

@ -105,11 +105,12 @@ class AbuseFilterHooks {
// @todo is there any real point in passing this in?
$text = AbuseFilter::contentToString( $content );
$filterStatus = self::filterEdit( $context, $content, $text, $status, $summary, $slot );
$filterResult = self::filterEdit( $context, $content, $text, $summary, $slot );
if ( !$filterStatus->isOK() ) {
if ( !$filterResult->isOK() ) {
// Produce a useful error message for API edits
$status->apiHookResult = self::getApiResult( $filterStatus );
$filterResultApi = self::getApiStatus( $filterResult );
$status->merge( $filterResultApi );
}
}
@ -184,13 +185,12 @@ class AbuseFilterHooks {
* @param IContextSource $context the context of the edit
* @param Content $content the new Content generated by the edit
* @param string $text new page content (subject of filtering)
* @param Status $status Error message to return
* @param string $summary Edit summary for page
* @param string $slot slot role for the content
* @return Status
*/
public static function filterEdit( IContextSource $context, Content $content, $text,
Status $status, $summary, $slot = SlotRecord::MAIN
$summary, $slot = SlotRecord::MAIN
) {
self::$lastEditPage = null;
@ -218,8 +218,6 @@ class AbuseFilterHooks {
$runner = new AbuseFilterRunner( $user, $title, $vars, 'default' );
$filterResult = $runner->run();
if ( !$filterResult->isOK() ) {
$status->merge( $filterResult );
return $filterResult;
}
@ -268,45 +266,30 @@ class AbuseFilterHooks {
/**
* @param Status $status Error message details
* @return array API result
* @return Status Status containing the same error messages with extra data for the API
*/
private static function getApiResult( Status $status ) {
global $wgFullyInitialised;
private static function getApiStatus( Status $status ) {
$allActionsTaken = $status->getValue();
$statusForApi = Status::newGood();
$params = $status->getErrorsArray()[0];
$key = array_shift( $params );
foreach ( $status->getErrors() as $error ) {
list( $filterDescription, $filter ) = $error['params'];
$actionsTaken = $allActionsTaken[ $filter ];
$warning = wfMessage( $key )->params( $params );
if ( !$wgFullyInitialised ) {
// This could happen for account autocreation checks
$warning = $warning->inContentLanguage();
$code = ( $actionsTaken === [ 'warn' ] ) ? 'abusefilter-warning' : 'abusefilter-disallowed';
$data = [
'abusefilter' => [
'id' => $filter,
'description' => $filterDescription,
'actions' => $actionsTaken,
],
];
$message = ApiMessage::create( $error, $code, $data );
$statusForApi->fatal( $message );
}
list( $filterDescription, $filter ) = $params;
// The value is a nested structure keyed by filter id, which doesn't make sense when we only
// return the result from one filter. Flatten it to a plain array of actions.
$actionsTaken = array_values( array_unique(
array_merge( ...array_values( $status->getValue() ) )
) );
$code = ( $actionsTaken === [ 'warn' ] ) ? 'abusefilter-warning' : 'abusefilter-disallowed';
ApiResult::setIndexedTagName( $params, 'param' );
return [
'code' => $code,
'message' => [
'key' => $key,
'params' => $params,
],
'abusefilter' => [
'id' => $filter,
'description' => $filterDescription,
'actions' => $actionsTaken,
],
// For backwards-compatibility
'info' => 'Hit AbuseFilter: ' . $filterDescription,
'warning' => $warning->parse(),
];
return $statusForApi;
}
/**
@ -986,13 +969,10 @@ class AbuseFilterHooks {
$filterResult = $runner->run();
if ( !$filterResult->isOK() ) {
$messageAndParams = $filterResult->getErrorsArray()[0];
$apiResult = self::getApiResult( $filterResult );
$error = ApiMessage::create(
$messageAndParams,
$apiResult['code'],
$apiResult
);
// Produce a useful error message for API edits
$filterResultApi = self::getApiStatus( $filterResult );
// @todo Return all errors instead of only the first one
$error = $filterResultApi->getErrors()[0]['message'];
}
return $filterResult->isOK();

12
modules/ve-abusefilter/ve.init.mw.AbuseFilterSaveErrorHandler.js
View file

@ -7,15 +7,17 @@ mw.libs.ve.targetLoader.addPlugin( function () {
ve.init.mw.AbuseFilterSaveErrorHandler.static.name = 'abuseFilter';
ve.init.mw.AbuseFilterSaveErrorHandler.static.matchFunction = function ( data ) {
return !!ve.getProp( data, 'visualeditoredit', 'edit', 'abusefilter' );
return data.errors && data.errors.some( function ( err ) {
return err.code === 'abusefilter-disallowed' || err.code === 'abusefilter-warning';
} );
};
ve.init.mw.AbuseFilterSaveErrorHandler.static.process = function ( data, target ) {
var
$message = $( $.parseHTML( ve.getProp( data, 'visualeditoredit', 'edit', 'warning' ) ) ),
isWarning = ve.getProp( data, 'visualeditoredit', 'edit', 'code' ) !== 'abusefilter-disallowed';
var isWarning = data.errors.every( function ( err ) {
return err.code === 'abusefilter-warning';
} );
// Handle warnings/errors from Extension:AbuseFilter
target.showSaveError( $message, isWarning, isWarning );
target.showSaveError( target.extractErrorMessages( data ), isWarning, isWarning );
// Emit event for tracking. TODO: This is a bad design
target.emit( 'saveErrorAbuseFilter' );
};

4
tests/phpunit/AbuseFilterConsequencesTest.php
View file

@ -752,7 +752,9 @@ class AbuseFilterConsequencesTest extends MediaWikiTestCase {
$actual = [];
foreach ( $errors as $error ) {
$msg = $error['message'];
// We don't use any of the "API" stuff in ApiMessage here, but this is the most
// convenient way to get a Message from a StatusValue error structure.
$msg = ApiMessage::create( $error )->getKey();
if ( strpos( $msg, 'abusefilter' ) !== false ) {
$actual[] = $msg;
}