Introduce a service for saving filters

Change-Id: I6b7d16ad7ea1124989ed67c74413979cfd0275c4
This commit is contained in:
Daimona Eaytoy 2020-09-20 00:16:35 +02:00
parent 1ce325efc2
commit 9595bd9da5
9 changed files with 344 additions and 240 deletions

View file

@ -180,6 +180,7 @@
"MediaWiki\\Extension\\AbuseFilter\\FilterCompare": "includes/FilterCompare.php",
"MediaWiki\\Extension\\AbuseFilter\\FilterImporter": "includes/FilterImporter.php",
"MediaWiki\\Extension\\AbuseFilter\\InvalidImportDataException": "includes/InvalidImportDataException.php",
"MediaWiki\\Extension\\AbuseFilter\\FilterStore": "includes/FilterStore.php",
"AFComputedVariable": "includes/AFComputedVariable.php",
"AFPData": "includes/parser/AFPData.php",
"AFPException": "includes/parser/AFPException.php",

View file

@ -1,7 +1,6 @@
<?php
use MediaWiki\Extension\AbuseFilter\AbuseFilterServices;
use MediaWiki\Extension\AbuseFilter\Filter\Filter;
use MediaWiki\Extension\AbuseFilter\Hooks\AbuseFilterHookRunner;
use MediaWiki\Extension\AbuseFilter\VariableGenerator\VariableGenerator;
use MediaWiki\Logger\LoggerFactory;
@ -394,199 +393,6 @@ class AbuseFilter {
return $obj;
}
/**
* Checks whether user input for the filter editing form is valid and if so saves the filter.
* Returns a Status object which can be:
* - Good with [ new_filter_id, history_id ] as value if the filter was successfully saved
* - Good with value = false if everything went fine but the filter is unchanged
* - OK with errors if a validation error occurred
* - Fatal in case of a permission-related error
*
* @param User $user
* @param int|null $filter
* @param Filter $newFilter
* @param Filter $originalFilter
* @param IDatabase $dbw DB_MASTER Where the filter should be saved
* @param Config $config
* @return Status
* @internal
*/
public static function saveFilter(
User $user,
?int $filter,
Filter $newFilter,
Filter $originalFilter,
IDatabase $dbw,
Config $config
) {
$validator = AbuseFilterServices::getFilterValidator();
$validationStatus = $validator->checkAll( $newFilter, $originalFilter, $user );
if ( !$validationStatus->isGood() ) {
return $validationStatus;
}
$wasGlobal = $originalFilter->isGlobal();
// Check for non-changes
$differences = AbuseFilterServices::getFilterCompare()->compareVersions( $newFilter, $originalFilter );
if ( !count( $differences ) ) {
return Status::newGood( false );
}
// Everything went fine, so let's save the filter
list( $new_id, $history_id ) =
self::doSaveFilter( $user, $newFilter, $differences, $filter, $wasGlobal, $dbw, $config );
return Status::newGood( [ $new_id, $history_id ] );
}
/**
* Saves new filter's info to DB
*
* @param User $user
* @param Filter $newFilter
* @param array $differences
* @param int|null $filter
* @param bool $wasGlobal
* @param IDatabase $dbw DB_MASTER where the filter will be saved
* @param Config $config
* @return int[] first element is new ID, second is history ID
*/
private static function doSaveFilter(
User $user,
Filter $newFilter,
$differences,
?int $filter,
$wasGlobal,
IDatabase $dbw,
Config $config
) {
// TODO This code shouldn't be here
$newRow = get_object_vars( $newFilter->toDatabaseRow() );
// Set last modifier.
$newRow['af_timestamp'] = $dbw->timestamp();
$newRow['af_user'] = $user->getId();
$newRow['af_user_text'] = $user->getName();
$dbw->startAtomic( __METHOD__ );
// Insert MAIN row.
$is_new = $filter === null;
$new_id = $filter;
// Preserve the old throttled status (if any) only if disabling the filter.
// TODO: It might make more sense to check what was actually changed
$newRow['af_throttled'] = ( $newRow['af_throttled'] ?? false ) && !$newRow['af_enabled'];
// This is null when creating a new filter, but the DB field is NOT NULL
$newRow['af_hit_count'] = $newRow['af_hit_count'] ?? 0;
$newRow['af_id'] = $new_id;
$dbw->replace( 'abuse_filter', 'af_id', $newRow, __METHOD__ );
if ( $is_new ) {
$new_id = $dbw->insertId();
}
'@phan-var int $new_id';
$availableActions = $config->get( 'AbuseFilterActions' );
$actions = $newFilter->getActions();
$actionsRows = [];
foreach ( array_filter( $availableActions ) as $action => $_ ) {
// Check if it's set
$enabled = isset( $actions[$action] );
if ( $enabled ) {
$parameters = $actions[$action];
if ( $action === 'throttle' && $parameters[0] === null ) {
// FIXME: Do we really need to keep the filter ID inside throttle parameters?
// We'd save space, keep things simpler and avoid this hack. Note: if removing
// it, a maintenance script will be necessary to clean up the table.
$parameters[0] = $new_id;
}
$thisRow = [
'afa_filter' => $new_id,
'afa_consequence' => $action,
'afa_parameters' => implode( "\n", $parameters )
];
$actionsRows[] = $thisRow;
}
}
// Create a history row
$afh_row = [];
foreach ( self::HISTORY_MAPPINGS as $af_col => $afh_col ) {
$afh_row[$afh_col] = $newRow[$af_col];
}
$afh_row['afh_actions'] = serialize( $actions );
$afh_row['afh_changed_fields'] = implode( ',', $differences );
$flags = [];
if ( $newRow['af_hidden'] ) {
$flags[] = 'hidden';
}
if ( $newRow['af_enabled'] ) {
$flags[] = 'enabled';
}
if ( $newRow['af_deleted'] ) {
$flags[] = 'deleted';
}
if ( $newRow['af_global'] ) {
$flags[] = 'global';
}
$afh_row['afh_flags'] = implode( ',', $flags );
$afh_row['afh_filter'] = $new_id;
// Do the update
$dbw->insert( 'abuse_filter_history', $afh_row, __METHOD__ );
$history_id = $dbw->insertId();
if ( $filter !== null ) {
$dbw->delete(
'abuse_filter_action',
[ 'afa_filter' => $filter ],
__METHOD__
);
}
$dbw->insert( 'abuse_filter_action', $actionsRows, __METHOD__ );
$dbw->endAtomic( __METHOD__ );
// Invalidate cache if this was a global rule
if ( $wasGlobal || $newRow['af_global'] ) {
$group = 'default';
if ( isset( $newRow['af_group'] ) && $newRow['af_group'] !== '' ) {
$group = $newRow['af_group'];
}
AbuseFilterServices::getFilterLookup()->purgeGroupWANCache( $group );
}
// Logging
$subtype = $filter === null ? 'create' : 'modify';
$logEntry = new ManualLogEntry( 'abusefilter', $subtype );
$logEntry->setPerformer( $user );
$logEntry->setTarget( SpecialAbuseFilter::getTitleForSubpage( (string)$new_id ) );
$logEntry->setParameters( [
'historyId' => $history_id,
'newId' => $new_id
] );
$logid = $logEntry->insert( $dbw );
$logEntry->publish( $logid );
if ( isset( $actions['tag'] ) ) {
AbuseFilterServices::getChangeTagsManager()->purgeTagCache();
}
AbuseFilterServices::getFilterProfiler()->resetFilterProfile( $new_id );
return [ $new_id, $history_id ];
}
/**
* @param string $action
* @param MessageLocalizer|null $localizer

View file

@ -104,4 +104,11 @@ class AbuseFilterServices {
public static function getFilterImporter() : FilterImporter {
return MediaWikiServices::getInstance()->getService( FilterImporter::SERVICE_NAME );
}
/**
* @return FilterStore
*/
public static function getFilterStore() : FilterStore {
return MediaWikiServices::getInstance()->getService( FilterStore::SERVICE_NAME );
}
}

View file

@ -42,32 +42,6 @@ class Filter extends AbstractFilter {
$this->throttled = $throttled;
}
/**
* TEMPORARY HACK
* @return \stdClass
* @codeCoverageIgnore
*/
public function toDatabaseRow(): \stdClass {
// T67807: integer 1's & 0's might be better understood than booleans
return (object)[
'af_id' => $this->id,
'af_pattern' => $this->specs->getRules(),
'af_public_comments' => $this->specs->getName(),
'af_comments' => $this->specs->getComments(),
'af_group' => $this->specs->getGroup(),
'af_actions' => implode( ',', $this->specs->getActionsNames() ),
'af_enabled' => (int)$this->flags->getEnabled(),
'af_deleted' => (int)$this->flags->getDeleted(),
'af_hidden' => (int)$this->flags->getHidden(),
'af_global' => (int)$this->flags->getGlobal(),
'af_user' => $this->lastEditInfo->getUserID(),
'af_user_text' => $this->lastEditInfo->getUserName(),
'af_timestamp' => $this->lastEditInfo->getTimestamp(),
'af_hit_count' => $this->hitCount,
'af_throttled' => (int)$this->throttled,
];
}
/**
* @return LastEditInfo
*/

271
includes/FilterStore.php Normal file
View file

@ -0,0 +1,271 @@
<?php
namespace MediaWiki\Extension\AbuseFilter;
use AbuseFilter;
use ManualLogEntry;
use MediaWiki\Extension\AbuseFilter\Filter\Filter;
use SpecialAbuseFilter;
use Status;
use stdClass;
use User;
use Wikimedia\Rdbms\ILoadBalancer;
/**
* @internal
*/
class FilterStore {
public const SERVICE_NAME = 'AbuseFilterFilterStore';
/** @var bool[] */
private $afActions;
/** @var ILoadBalancer */
private $loadBalancer;
/** @var FilterProfiler */
private $filterProfiler;
/** @var FilterLookup */
private $filterLookup;
/** @var ChangeTagsManager */
private $tagsManager;
/** @var FilterValidator */
private $filterValidator;
/** @var FilterCompare */
private $filterCompare;
/**
* @param bool[] $afActions
* @param ILoadBalancer $loadBalancer
* @param FilterProfiler $filterProfiler
* @param FilterLookup $filterLookup
* @param ChangeTagsManager $tagsManager
* @param FilterValidator $filterValidator
* @param FilterCompare $filterCompare
*/
public function __construct(
array $afActions,
ILoadBalancer $loadBalancer,
FilterProfiler $filterProfiler,
FilterLookup $filterLookup,
ChangeTagsManager $tagsManager,
FilterValidator $filterValidator,
FilterCompare $filterCompare
) {
$this->afActions = $afActions;
$this->loadBalancer = $loadBalancer;
$this->filterProfiler = $filterProfiler;
$this->filterLookup = $filterLookup;
$this->tagsManager = $tagsManager;
$this->filterValidator = $filterValidator;
$this->filterCompare = $filterCompare;
}
/**
* Checks whether user input for the filter editing form is valid and if so saves the filter.
* Returns a Status object which can be:
* - Good with [ new_filter_id, history_id ] as value if the filter was successfully saved
* - Good with value = false if everything went fine but the filter is unchanged
* - OK with errors if a validation error occurred
* - Fatal in case of a permission-related error
*
* @param User $user
* @param int|null $filter
* @param Filter $newFilter
* @param Filter $originalFilter
* @return Status
*/
public function saveFilter(
User $user,
?int $filter,
Filter $newFilter,
Filter $originalFilter
) : Status {
$validationStatus = $this->filterValidator->checkAll( $newFilter, $originalFilter, $user );
if ( !$validationStatus->isGood() ) {
return $validationStatus;
}
// Check for non-changes
$differences = $this->filterCompare->compareVersions( $newFilter, $originalFilter );
if ( !count( $differences ) ) {
return Status::newGood( false );
}
// Everything went fine, so let's save the filter
$wasGlobal = $originalFilter->isGlobal();
list( $newID, $historyID ) = $this->doSaveFilter( $user, $newFilter, $differences, $filter, $wasGlobal );
return Status::newGood( [ $newID, $historyID ] );
}
/**
* Saves new filter's info to DB
*
* @param User $user
* @param Filter $newFilter
* @param array $differences
* @param int|null $filter
* @param bool $wasGlobal
* @return int[] first element is new ID, second is history ID
*/
private function doSaveFilter(
User $user,
Filter $newFilter,
array $differences,
?int $filter,
bool $wasGlobal
) : array {
$dbw = $this->loadBalancer->getConnectionRef( DB_MASTER );
$newRow = get_object_vars( $this->filterToDatabaseRow( $newFilter ) );
// Set last modifier.
$newRow['af_timestamp'] = $dbw->timestamp();
$newRow['af_user'] = $user->getId();
$newRow['af_user_text'] = $user->getName();
$isNew = $filter === null;
$newID = $filter;
// Preserve the old throttled status (if any) only if disabling the filter.
// TODO: It might make more sense to check what was actually changed
$newRow['af_throttled'] = ( $newRow['af_throttled'] ?? false ) && !$newRow['af_enabled'];
// This is null when creating a new filter, but the DB field is NOT NULL
$newRow['af_hit_count'] = $newRow['af_hit_count'] ?? 0;
$newRow['af_id'] = $newID;
$dbw->startAtomic( __METHOD__ );
$dbw->replace( 'abuse_filter', 'af_id', $newRow, __METHOD__ );
if ( $isNew ) {
$newID = $dbw->insertId();
}
'@phan-var int $newID';
$actions = $newFilter->getActions();
$actionsRows = [];
foreach ( array_filter( $this->afActions ) as $action => $_ ) {
// Check if it's set
$enabled = isset( $actions[$action] );
if ( $enabled ) {
$parameters = $actions[$action];
if ( $action === 'throttle' && $parameters[0] === null ) {
// FIXME: Do we really need to keep the filter ID inside throttle parameters?
// We'd save space, keep things simpler and avoid this hack. Note: if removing
// it, a maintenance script will be necessary to clean up the table.
$parameters[0] = $newID;
}
$thisRow = [
'afa_filter' => $newID,
'afa_consequence' => $action,
'afa_parameters' => implode( "\n", $parameters )
];
$actionsRows[] = $thisRow;
}
}
// Create a history row
$afhRow = [];
foreach ( AbuseFilter::HISTORY_MAPPINGS as $afCol => $afhCol ) {
$afhRow[$afhCol] = $newRow[$afCol];
}
$afhRow['afh_actions'] = serialize( $actions );
$afhRow['afh_changed_fields'] = implode( ',', $differences );
$flags = [];
if ( $newRow['af_hidden'] ) {
$flags[] = 'hidden';
}
if ( $newRow['af_enabled'] ) {
$flags[] = 'enabled';
}
if ( $newRow['af_deleted'] ) {
$flags[] = 'deleted';
}
if ( $newRow['af_global'] ) {
$flags[] = 'global';
}
$afhRow['afh_flags'] = implode( ',', $flags );
$afhRow['afh_filter'] = $newID;
// Do the update
$dbw->insert( 'abuse_filter_history', $afhRow, __METHOD__ );
$historyID = $dbw->insertId();
if ( !$isNew ) {
$dbw->delete(
'abuse_filter_action',
[ 'afa_filter' => $filter ],
__METHOD__
);
}
$dbw->insert( 'abuse_filter_action', $actionsRows, __METHOD__ );
$dbw->endAtomic( __METHOD__ );
// Invalidate cache if this was a global rule
if ( $wasGlobal || $newRow['af_global'] ) {
$group = 'default';
if ( isset( $newRow['af_group'] ) && $newRow['af_group'] !== '' ) {
$group = $newRow['af_group'];
}
$this->filterLookup->purgeGroupWANCache( $group );
}
// Logging
$subtype = $isNew ? 'create' : 'modify';
$logEntry = new ManualLogEntry( 'abusefilter', $subtype );
$logEntry->setPerformer( $user );
$logEntry->setTarget( SpecialAbuseFilter::getTitleForSubpage( (string)$newID ) );
$logEntry->setParameters( [
'historyId' => $historyID,
'newId' => $newID
] );
$logid = $logEntry->insert( $dbw );
$logEntry->publish( $logid );
// Purge the tag list cache so the fetchAllTags hook applies tag changes
if ( isset( $actions['tag'] ) ) {
$this->tagsManager->purgeTagCache();
}
$this->filterProfiler->resetFilterProfile( $newID );
return [ $newID, $historyID ];
}
/**
* @todo Perhaps add validation to ensure no null values remained.
* @param Filter $filter
* @return stdClass
*/
private function filterToDatabaseRow( Filter $filter ) : stdClass {
// T67807: integer 1's & 0's might be better understood than booleans
return (object)[
'af_id' => $filter->getID(),
'af_pattern' => $filter->getRules(),
'af_public_comments' => $filter->getName(),
'af_comments' => $filter->getComments(),
'af_group' => $filter->getGroup(),
'af_actions' => implode( ',', $filter->getActionsNames() ),
'af_enabled' => (int)$filter->isEnabled(),
'af_deleted' => (int)$filter->isDeleted(),
'af_hidden' => (int)$filter->isHidden(),
'af_global' => (int)$filter->isGlobal(),
'af_user' => $filter->getUserID(),
'af_user_text' => $filter->getUserName(),
'af_timestamp' => $filter->getTimestamp(),
'af_hit_count' => $filter->getHitCount(),
'af_throttled' => (int)$filter->isThrottled(),
];
}
}

View file

@ -11,6 +11,7 @@ use MediaWiki\Extension\AbuseFilter\FilterCompare;
use MediaWiki\Extension\AbuseFilter\FilterImporter;
use MediaWiki\Extension\AbuseFilter\FilterLookup;
use MediaWiki\Extension\AbuseFilter\FilterProfiler;
use MediaWiki\Extension\AbuseFilter\FilterStore;
use MediaWiki\Extension\AbuseFilter\FilterUser;
use MediaWiki\Extension\AbuseFilter\FilterValidator;
use MediaWiki\Extension\AbuseFilter\Hooks\AbuseFilterHookRunner;
@ -128,6 +129,17 @@ return [
)
);
},
FilterStore::SERVICE_NAME => function ( MediaWikiServices $services ): FilterStore {
return new FilterStore(
$services->getMainConfig()->get( 'AbuseFilterActions' ),
$services->getDBLoadBalancer(),
$services->get( FilterProfiler::SERVICE_NAME ),
$services->get( FilterLookup::SERVICE_NAME ),
$services->get( ChangeTagsManager::SERVICE_NAME ),
$services->get( FilterValidator::SERVICE_NAME ),
$services->get( FilterCompare::SERVICE_NAME )
);
},
];
// @codeCoverageIgnoreEnd

View file

@ -133,11 +133,8 @@ class AbuseFilterViewEdit extends AbuseFilterView {
return;
}
$dbw = wfGetDB( DB_MASTER );
$status = AbuseFilter::saveFilter(
$user, $filter, $newFilter,
$origFilter, $dbw, $this->getConfig()
);
$filterStore = AbuseFilterServices::getFilterStore();
$status = $filterStore->saveFilter( $user, $filter, $newFilter, $origFilter );
if ( !$status->isGood() ) {
$errors = $status->getErrors();

View file

@ -31,7 +31,7 @@ use MediaWiki\Extension\AbuseFilter\Filter\MutableFilter;
use MediaWiki\Extension\AbuseFilter\Filter\Specs;
use MediaWiki\Extension\AbuseFilter\FilterValidator;
use MediaWiki\Extension\AbuseFilter\Parser\ParserFactory;
use MediaWiki\MediaWikiServices;
use Wikimedia\TestingAccessWrapper;
/**
* @group Test
@ -67,7 +67,13 @@ class AbuseFilterSaveTest extends MediaWikiIntegrationTestCase {
*/
private function createFilter( int $id ) : void {
$filter = $this->getFilterFromSpecs( [ 'id' => $id ] + self::DEFAULT_VALUES );
wfGetDB( DB_MASTER )->insert( 'abuse_filter', get_object_vars( $filter->toDatabaseRow() ) );
// Use some black magic to bypass checks
$filterStore = TestingAccessWrapper::newFromObject( AbuseFilterServices::getFilterStore() );
wfGetDB( DB_MASTER )->insert(
'abuse_filter',
get_object_vars( $filterStore->filterToDatabaseRow( $filter ) ),
__METHOD__
);
}
/**
@ -104,7 +110,7 @@ class AbuseFilterSaveTest extends MediaWikiIntegrationTestCase {
}
/**
* @covers AbuseFilter::saveFilter
* @covers \MediaWiki\Extension\AbuseFilter\FilterStore
* @covers \MediaWiki\Extension\AbuseFilter\FilterValidator
*/
public function testSaveFilter_valid() {
@ -119,9 +125,8 @@ class AbuseFilterSaveTest extends MediaWikiIntegrationTestCase {
$origFilter = MutableFilter::newDefault();
$newFilter = $this->getFilterFromSpecs( $row );
$status = AbuseFilter::saveFilter(
$this->getTestSysop()->getUser(), $row['id'], $newFilter, $origFilter,
wfGetDB( DB_MASTER ), MediaWikiServices::getInstance()->getMainConfig()
$status = AbuseFilterServices::getFilterStore()->saveFilter(
$this->getTestSysop()->getUser(), $row['id'], $newFilter, $origFilter
);
$this->assertTrue( $status->isGood(), "Save failed with status: $status" );
@ -132,7 +137,7 @@ class AbuseFilterSaveTest extends MediaWikiIntegrationTestCase {
}
/**
* @covers AbuseFilter::saveFilter
* @covers \MediaWiki\Extension\AbuseFilter\FilterStore
* @covers \MediaWiki\Extension\AbuseFilter\FilterValidator
*/
public function testSaveFilter_invalid() {
@ -154,10 +159,7 @@ class AbuseFilterSaveTest extends MediaWikiIntegrationTestCase {
$user = $this->getTestUser()->getUser();
// Assign -modify and -modify-global, but not -modify-restricted
$this->overrideUserPermissions( $user, [ 'abusefilter-modify' ] );
$status = AbuseFilter::saveFilter(
$user, $row['id'], $newFilter, $origFilter,
wfGetDB( DB_MASTER ), MediaWikiServices::getInstance()->getMainConfig()
);
$status = AbuseFilterServices::getFilterStore()->saveFilter( $user, $row['id'], $newFilter, $origFilter );
$this->assertFalse( $status->isGood(), 'The filter validation returned a valid status.' );
$actual = $status->getErrors()[0]['message'];
@ -165,7 +167,7 @@ class AbuseFilterSaveTest extends MediaWikiIntegrationTestCase {
}
/**
* @covers AbuseFilter::saveFilter
* @covers \MediaWiki\Extension\AbuseFilter\FilterStore
* @covers \MediaWiki\Extension\AbuseFilter\FilterValidator
*/
public function testSaveFilter_noChange() {
@ -180,9 +182,8 @@ class AbuseFilterSaveTest extends MediaWikiIntegrationTestCase {
$origFilter = AbuseFilterServices::getFilterLookup()->getFilter( $filter, false );
$newFilter = $this->getFilterFromSpecs( $row );
$status = AbuseFilter::saveFilter(
$this->getTestSysop()->getUser(), $filter, $newFilter, $origFilter,
wfGetDB( DB_MASTER ), MediaWikiServices::getInstance()->getMainConfig()
$status = AbuseFilterServices::getFilterStore()->saveFilter(
$this->getTestSysop()->getUser(), $filter, $newFilter, $origFilter
);
$this->assertTrue( $status->isGood(), "Got a non-good status: $status" );

View file

@ -0,0 +1,35 @@
<?php
use MediaWiki\Extension\AbuseFilter\ChangeTagsManager;
use MediaWiki\Extension\AbuseFilter\FilterCompare;
use MediaWiki\Extension\AbuseFilter\FilterLookup;
use MediaWiki\Extension\AbuseFilter\FilterProfiler;
use MediaWiki\Extension\AbuseFilter\FilterStore;
use MediaWiki\Extension\AbuseFilter\FilterValidator;
use Wikimedia\Rdbms\ILoadBalancer;
/**
* @group Test
* @group AbuseFilter
* @coversDefaultClass \MediaWiki\Extension\AbuseFilter\FilterStore
* @todo Expand this. FilterStore is tightly bound to a Database, so it's not easy.
*/
class AbuseFilterFilterStoreTest extends MediaWikiUnitTestCase {
/**
* @covers ::__construct
*/
public function testConstruct() {
$this->assertInstanceOf(
FilterStore::class,
new FilterStore(
[],
$this->createMock( ILoadBalancer::class ),
$this->createMock( FilterProfiler::class ),
$this->createMock( FilterLookup::class ),
$this->createMock( ChangeTagsManager::class ),
$this->createMock( FilterValidator::class ),
$this->createMock( FilterCompare::class )
)
);
}
}