WatchedItemStore: Enforce a maximum watchlist expiry duration

Introduces $wgWatchlistExpiryMaxDuration which is used instead of given
expiry if the given exceeds it. This is done in the storage layer. The
reasoning is to control the size of the watchlist_expiry table. Hence,
the max duration does not apply to indefinite expiries (since that would
mean now row in watchlist_expiry).

The frontend is responsible for disallowing expiries greater than the
max, if it choses to do so.

APIs should now pass in $wgWatchlistExpiryMaxDuration as the PARAM_MAX
setting for the 'expiry' type. They should also set PARAM_USE_MAX so
that the maximum value is used if it is exceeded.

Other APIs that watch pages will be updated in separate patches
(see T248512 and T248514).

Bug: T249672
Change-Id: I811c444c36c1da1470f2d6e185404b6121a263eb
This commit is contained in:
MusikAnimal 2020-04-20 18:22:27 -04:00
parent cd6c64b4f4
commit 0694cc02f1
14 changed files with 218 additions and 32 deletions

View file

@ -60,6 +60,8 @@ For notes on 1.34.x and older releases, see HISTORY.
setting, but all other aspects of the expiry feature are controlled by it.
* $wgWatchlistPurgeRate — Chance of expired watchlist items being purged on any
page edit. Only has effect if $wgWatchlistExpiry is true.
* $wgWatchlistExpiryMaxDuration — Maximum definite relative duration for
watchlist expiries. Only has effect if $wgWatchlistExpiry is true.
* $wgImgAuthPath can be used to override the path prefix used when handling
img_auth.php requests. (T235357)
* $wgAllowedCorsHeaders — list of headers which can be used in a cross-site API
@ -297,7 +299,8 @@ For notes on 1.34.x and older releases, see HISTORY.
module does not work for pages that do not already exist.
* If $wgWatchlistExpiry is true, the following API changes are made:
- action=watch accepts a new 'expiry' parameter analagous to the expiry
accepted by action=userrights, action=block, etc.
accepted by action=userrights, action=block, etc., except it must be no
greater than $wgWatchlistExpiryMaxDuration, or an infinity value.
- action=query&list=watchlistraw returns pages' watchlist expiry dates.
* (T249526) action=login will now return Failed rather than NeedToken on
session loss.

View file

@ -9486,6 +9486,22 @@ $wgWatchlistExpiry = false;
*/
$wgWatchlistPurgeRate = 0.1;
/**
* Relative maximum duration for watchlist expiries, as accepted by strtotime().
* This relates to finite watchlist expiries only. Pages can be watched indefinitely
* regardless of what this is set to.
*
* This is used to ensure the watchlist_expiry table doesn't grow to be too big.
*
* Only has effect if $wgWatchlistExpiry is true.
*
* Set to null to allow expiries of any duration.
*
* @since 1.35
* @var string|null
*/
$wgWatchlistExpiryMaxDuration = '6 months';
/**
* For really cool vim folding this needs to be at the end:
* vim: foldmarker=@{,@} foldmethod=marker

View file

@ -1179,15 +1179,15 @@ return [
'WatchedItemStore' => function ( MediaWikiServices $services ) : WatchedItemStore {
$store = new WatchedItemStore(
new ServiceOptions( WatchedItemStore::CONSTRUCTOR_OPTIONS,
$services->getMainConfig() ),
$services->getDBLoadBalancerFactory(),
JobQueueGroup::singleton(),
$services->getMainObjectStash(),
new HashBagOStuff( [ 'maxKeys' => 100 ] ),
$services->getReadOnlyMode(),
$services->getMainConfig()->get( 'UpdateRowsPerQuery' ),
$services->getNamespaceInfo(),
$services->getRevisionLookup(),
$services->getMainConfig()->get( 'WatchlistExpiry' )
$services->getRevisionLookup()
);
$store->setStatsdDataFactory( $services->getStatsdDataFactory() );

View file

@ -21,6 +21,7 @@
*/
use Wikimedia\ParamValidator\ParamValidator;
use Wikimedia\ParamValidator\TypeDef\ExpiryDef;
/**
* API module to allow users to watch a page
@ -33,10 +34,14 @@ class ApiWatch extends ApiBase {
/** @var bool Whether watchlist expiries are enabled. */
private $expiryEnabled;
/** @var string Relative maximum expiry. */
private $maxDuration;
public function __construct( ApiMain $mainModule, $moduleName, $modulePrefix = '' ) {
parent::__construct( $mainModule, $moduleName, $modulePrefix );
$this->expiryEnabled = $this->getConfig()->get( 'WatchlistExpiry' );
$this->maxDuration = $this->getConfig()->get( 'WatchlistExpiryMaxDuration' );
}
public function execute() {
@ -177,6 +182,8 @@ class ApiWatch extends ApiBase {
],
'expiry' => [
ParamValidator::PARAM_TYPE => 'expiry',
ExpiryDef::PARAM_MAX => $this->maxDuration,
ExpiryDef::PARAM_USE_MAX => true,
],
'unwatch' => false,
'continue' => [

View file

@ -501,7 +501,7 @@ class ParamValidator {
}
/**
* Fetch and valiate a parameter value using a settings array
* Fetch and validate a parameter value using a settings array
*
* @param string $name Parameter name
* @param array|mixed $settings Default value or an array of settings
@ -551,7 +551,7 @@ class ParamValidator {
}
/**
* Valiate a parameter value using a settings array
* Validate a parameter value using a settings array
*
* @param string $name Parameter name
* @param null|mixed $value Parameter value

View file

@ -2,6 +2,7 @@
namespace Wikimedia\ParamValidator\TypeDef;
use Wikimedia\Message\DataMessageValue;
use Wikimedia\Message\MessageValue;
use Wikimedia\ParamValidator\ParamValidator;
use Wikimedia\ParamValidator\TypeDef;
@ -17,6 +18,19 @@ class ExpiryDef extends TypeDef {
/** @var array Possible values that mean "doesn't expire". */
public const INFINITY_VALS = [ 'infinite', 'indefinite', 'infinity', 'never' ];
/**
* (bool) If truthy, the value given for the PARAM_MAX setting is used if the provided expiry
* exceeds it, and the 'badexpiry-duration' message is shown as a warning.
*
* If false, 'badexpiry-duration' is shown and is fatal.
*/
public const PARAM_USE_MAX = 'param-use-max';
/**
* (int|float) Maximum non-infinity duration.
*/
public const PARAM_MAX = 'param-max';
public function validate( $name, $value, array $settings, array $options ) {
$expiry = self::normalizeExpiry( $value );
@ -28,6 +42,22 @@ class ExpiryDef extends TypeDef {
$this->failure( 'badexpiry-past', $name, $value, $settings, $options );
}
$max = $settings[self::PARAM_MAX] ?? null;
if ( self::expiryExceedsMax( $expiry, $max ) ) {
$dontUseMax = empty( $settings[self::PARAM_USE_MAX] );
// Show warning that expiry exceeds the max, and that the max is being used instead.
$msg = DataMessageValue::new(
$dontUseMax
? 'paramvalidator-badexpiry-duration'
: 'paramvalidator-badexpiry-duration-max',
[ $max ]
);
$this->failure( $msg, $name, $value, $settings, $options, $dontUseMax );
return self::normalizeExpiry( $max );
}
return $expiry;
}
@ -83,4 +113,24 @@ class ExpiryDef extends TypeDef {
return ConvertibleTimestamp::convert( TS_ISO_8601, $unix === 0 ? '00' : $unix );
}
/**
* Given an expiry, test if the normalized value exceeds the given maximum.
*
* @param string|null $expiry
* @param string|null $max Relative maximum duration acceptable by strtotime() (i.e. '6 months')
* @return bool
*/
public static function expiryExceedsMax( ?string $expiry, ?string $max = null ): bool {
// Normalize the max and given expiries to TS_ISO_8601 format.
$expiry = self::normalizeExpiry( $expiry );
$max = self::normalizeExpiry( $max );
if ( !$max || !$expiry || $expiry === 'infinity' ) {
// Either there is no max or given expiry was invalid.
return false;
}
return $expiry > $max;
}
}

View file

@ -10,6 +10,8 @@
"paramvalidator-badbool": "Invalid value \"$2\" for boolean parameter \"$1\". Pass $3 for true, or $5 for false.",
"paramvalidator-badexpiry": "Invalid value \"$2\" for expiry parameter \"$1\".",
"paramvalidator-badexpiry-duration": "Given value \"$2\" for parameter <var>$1</var> exceeds the maximum of \"$3\".",
"paramvalidator-badexpiry-duration-max": "Given value \"$2\" for parameter <var>$1</var> exceeds the maximum of \"$3\". Using maximum instead.",
"paramvalidator-badexpiry-past": "Value \"$2\" for expiry parameter \"$1\" is in the past.",
"paramvalidator-badfloat": "Invalid value \"$2\" for float parameter \"$1\".",
"paramvalidator-badfloat-notfinite": "Value \"$2\" for float parameter \"$1\" is too large or is not a number.",

View file

@ -20,6 +20,8 @@
},
"paramvalidator-badbool": "Error in API parameter validation. Parameters:\n* $1 - Parameter name.\n* $2 - Parameter value.\n* $3 - List of values recognized as \"true\", as a text list.\n* $4 - Count of values in $3.\n* $5 - List of values recognized as \"false\", as a text list. Includes {{msg-mw|paramvalidator-emptystring}}.\n* $6 - Count of values in $5.",
"paramvalidator-badexpiry": "Error in API parameter validation. Parameters:\n* $1 - Parameter name.\n* $2 - Parameter value.",
"paramvalidator-badexpiry-duration": "Error in API parameter validation. Parameters:\n* $1 - Parameter name.\n* $2 - Parameter value.\n* $3 - Maximum permitted value.",
"paramvalidator-badexpiry-duration-max": "Error in API parameter validation. Parameters:\n* $1 - Parameter name.\n* $2 - Parameter value.\n* $3 - Maximum permitted value.",
"paramvalidator-badexpiry-past": "Error in API parameter validation. Parameters:\n* $1 - Parameter name.\n* $2 - Parameter value.",
"paramvalidator-badfloat": "Error in API parameter validation. Parameters:\n* $1 - Parameter name.\n* $2 - Parameter value.",
"paramvalidator-badfloat-notfinite": "Error in API parameter validation. Parameters:\n* $1 - Parameter name.\n* $2 - Parameter value.",

View file

@ -1,6 +1,7 @@
<?php
use Liuggio\StatsdClient\Factory\StatsdDataFactoryInterface;
use MediaWiki\Config\ServiceOptions;
use MediaWiki\Linker\LinkTarget;
use MediaWiki\Revision\RevisionLookup;
use MediaWiki\User\UserIdentity;
@ -22,6 +23,15 @@ use Wikimedia\ScopedCallback;
*/
class WatchedItemStore implements WatchedItemStoreInterface, StatsdAwareInterface {
/**
* @since 1.35
*/
public const CONSTRUCTOR_OPTIONS = [
'UpdateRowsPerQuery',
'WatchlistExpiry',
'WatchlistExpiryMaxDuration',
];
/**
* @var ILBFactory
*/
@ -97,27 +107,35 @@ class WatchedItemStore implements WatchedItemStoreInterface, StatsdAwareInterfac
private $expiryEnabled;
/**
* @var string|null Maximum configured relative expiry.
*/
private $maxExpiryDuration;
/**
* @param ServiceOptions $options
* @param ILBFactory $lbFactory
* @param JobQueueGroup $queueGroup
* @param BagOStuff $stash
* @param HashBagOStuff $cache
* @param ReadOnlyMode $readOnlyMode
* @param int $updateRowsPerQuery
* @param NamespaceInfo $nsInfo
* @param RevisionLookup $revisionLookup
* @param bool $expiryEnabled
*/
public function __construct(
ServiceOptions $options,
ILBFactory $lbFactory,
JobQueueGroup $queueGroup,
BagOStuff $stash,
HashBagOStuff $cache,
ReadOnlyMode $readOnlyMode,
$updateRowsPerQuery,
NamespaceInfo $nsInfo,
RevisionLookup $revisionLookup,
bool $expiryEnabled
RevisionLookup $revisionLookup
) {
$options->assertRequiredOptions( self::CONSTRUCTOR_OPTIONS );
$this->updateRowsPerQuery = $options->get( 'UpdateRowsPerQuery' );
$this->expiryEnabled = $options->get( 'WatchlistExpiry' );
$this->maxExpiryDuration = $options->get( 'WatchlistExpiryMaxDuration' );
$this->lbFactory = $lbFactory;
$this->loadBalancer = $lbFactory->getMainLB();
$this->queueGroup = $queueGroup;
@ -127,10 +145,8 @@ class WatchedItemStore implements WatchedItemStoreInterface, StatsdAwareInterfac
$this->stats = new NullStatsdDataFactory();
$this->deferredUpdatesAddCallableUpdateCallback =
[ DeferredUpdates::class, 'addCallableUpdate' ];
$this->updateRowsPerQuery = $updateRowsPerQuery;
$this->nsInfo = $nsInfo;
$this->revisionLookup = $revisionLookup;
$this->expiryEnabled = $expiryEnabled;
$this->latestUpdateCache = new HashBagOStuff( [ 'maxKeys' => 3 ] );
}
@ -957,7 +973,11 @@ class WatchedItemStore implements WatchedItemStoreInterface, StatsdAwareInterfac
array $rows,
?string $expiry = null
): int {
$expiry = ExpiryDef::normalizeExpiry( $expiry );
if ( ExpiryDef::expiryExceedsMax( $expiry, $this->maxExpiryDuration ) ) {
$expiry = ExpiryDef::normalizeExpiry( $this->maxExpiryDuration );
} else {
$expiry = ExpiryDef::normalizeExpiry( $expiry );
}
if ( !$expiry ) {
// Either expiry was invalid or null (shouldn't change), 0 rows affected.

View file

@ -1,6 +1,7 @@
<?php
use MediaWiki\MediaWikiServices;
use Wikimedia\Timestamp\ConvertibleTimestamp;
/**
* @group API
@ -14,11 +15,21 @@ class ApiWatchTest extends ApiTestCase {
protected function setUp(): void {
parent::setUp();
// Fake current time to be 2019-06-05T19:50:42Z
ConvertibleTimestamp::setFakeTime( 1559764242 );
$this->setMwGlobals( [
'wgWatchlistExpiry' => true,
'wgWatchlistExpiryMaxDuration' => '6 months',
] );
}
protected function tearDown(): void {
parent::tearDown();
ConvertibleTimestamp::setFakeTime( false );
}
protected function getTokens() {
return $this->getTokenList( self::$users['sysop'] );
}
@ -34,8 +45,8 @@ class ApiWatchTest extends ApiTestCase {
$res = $data[0]['watch'][0];
$this->assertSame( 'Talk:Test page', $res['title'] );
$this->assertSame( 1, $res['ns'] );
$this->assertSame( '9999-01-01T00:00:00Z', $res['expiry'] );
$this->assertTrue( $res['watched'] );
$this->assertSame( '2019-12-05T19:50:42Z', $res['expiry'] );
// Re-watch, changing the expiry to indefinite.
$data = $this->doApiRequestWithToken( [
@ -65,17 +76,13 @@ class ApiWatchTest extends ApiTestCase {
// Re-watch, setting an expiry.
$expiry = '2 weeks';
$expectedExpiry = strtotime( $expiry );
$this->doApiRequestWithToken( [
'action' => 'watch',
'titles' => 'UTPage',
'expiry' => $expiry,
], null, $user );
[ $item ] = $store->getWatchedItemsForUser( $user );
$this->assertGreaterThanOrEqual(
$expectedExpiry,
wfTimestamp( TS_UNIX, $item->getExpiry() )
);
$this->assertSame( '20190619195042', $item->getExpiry() );
// Re-watch again, providing no expiry parameter, so expiry should remain unchanged.
$oldExpiry = $item->getExpiry();

View file

@ -33,7 +33,7 @@ class ApiParamValidatorTest extends ApiTestCase {
];
}
public function testKnwonTypes() : void {
public function testKnownTypes() : void {
[ $validator ] = $this->getValidator( new FauxRequest( [] ) );
$this->assertSame(
[

View file

@ -18,9 +18,10 @@ class ExpiryDefTest extends TypeDefTestCase {
* is asserted to cause a ValidationException with the given message.
* @param string $value
* @param string $msg
* @param array $settings
* @return array
*/
private function getValidationAssertion( string $value, string $msg ) {
private function getValidationAssertion( string $value, string $msg, array $settings = [] ) {
return [
$value,
new ValidationException(
@ -28,13 +29,13 @@ class ExpiryDefTest extends TypeDefTestCase {
'expiry',
$value,
[]
)
),
$settings
];
}
/**
* @dataProvider provideValidate
* @inheritDoc
*/
public function testValidate(
$value, $expect, array $settings = [], array $options = [], array $expectConds = []
@ -45,9 +46,16 @@ class ExpiryDefTest extends TypeDefTestCase {
} finally {
ConvertibleTimestamp::setFakeTime( $reset );
}
// Reset the time.
ConvertibleTimestamp::setFakeTime( false );
}
public function provideValidate() {
$settings = [
ExpiryDef::PARAM_MAX => '6 months',
ExpiryDef::PARAM_USE_MAX => true,
];
return [
'Valid infinity' => [ 'indefinite', 'infinity' ],
'Invalid expiry' => $this->getValidationAssertion( 'foobar', 'badexpiry' ),
@ -58,7 +66,8 @@ class ExpiryDefTest extends TypeDefTestCase {
),
'Expiry in past with negative unix time' => $this->getValidationAssertion(
'1969-12-31T23:59:59Z',
'badexpiry-past'
'badexpiry-past',
$settings
),
'Valid expiry' => [
'99990123123456',
@ -68,6 +77,40 @@ class ExpiryDefTest extends TypeDefTestCase {
'1 month',
'2019-07-05T19:50:42Z'
],
'Expiry less than max' => [ '20190701123456', '2019-07-01T12:34:56Z', $settings ],
'Relative expiry less than max' => [ '1 day', '2019-06-06T19:50:42Z', $settings ],
'Infinity less than max' => [ 'indefinite', 'infinity', $settings ],
'Expiry exceeds max' => [
'9999-01-23T12:34:56Z',
'2019-12-05T19:50:42Z',
$settings,
[],
[
[
'code' => 'paramvalidator-badexpiry-duration-max',
'data' => null,
]
],
],
'Relative expiry exceeds max' => [
'10 years',
'2019-12-05T19:50:42Z',
$settings,
[],
[
[
'code' => 'paramvalidator-badexpiry-duration-max',
'data' => null,
]
],
],
'Expiry exceeds max, fatal' => $this->getValidationAssertion(
'9999-01-23T12:34:56Z',
'paramvalidator-badexpiry-duration',
[
ExpiryDef::PARAM_MAX => '6 months',
]
),
];
}
@ -101,4 +144,18 @@ class ExpiryDefTest extends TypeDefTestCase {
];
}
/**
* @covers ExpiryDef::expiryExceedsMax
*/
public function testExpiryExceedsMax() {
$this->assertFalse(
ExpiryDef::expiryExceedsMax( '2020-01-23T12:34:56Z', '6 months' )
);
$this->assertTrue(
ExpiryDef::expiryExceedsMax( '9999-01-23T12:34:56Z', '6 months' )
);
$this->assertFalse(
ExpiryDef::expiryExceedsMax( '2020-01-23T12:34:56Z', null )
);
}
}

View file

@ -19,6 +19,7 @@ class WatchedItemStoreIntegrationTest extends MediaWikiTestCase {
$this->setMwGlobals( [
'wgWatchlistExpiry' => true,
'$wgWatchlistExpiryMaxDuration' => '6 months',
] );
}
@ -122,9 +123,21 @@ class WatchedItemStoreIntegrationTest extends MediaWikiTestCase {
$store = MediaWikiServices::getInstance()->getWatchedItemStore();
$initialUserWatchedItems = $store->countWatchedItems( $user );
$store->addWatch( $user, $title, '20300101000000' );
// Watch for a duration greater than the max ($wgWatchlistExpiryMaxDuration),
// which should get changed to the max.
$expiry = wfTimestamp( TS_MW, strtotime( '10 years' ) );
$store->addWatch( $user, $title, $expiry );
$this->assertLessThanOrEqual(
wfTimestamp( TS_MW, strtotime( '6 months' ) ),
$store->loadWatchedItem( $user, $title )->getExpiry()
);
// Valid expiry that's less than the max.
$expiry = wfTimestamp( TS_MW, strtotime( '1 week' ) );
$store->addWatch( $user, $title, $expiry );
$this->assertSame(
'20300101000000',
$expiry,
$store->loadWatchedItem( $user, $title )->getExpiry()
);
$this->assertEquals( $initialUserWatchedItems + 1, $store->countWatchedItems( $user ) );
@ -133,7 +146,7 @@ class WatchedItemStoreIntegrationTest extends MediaWikiTestCase {
// Invalid expiry, nothing should change.
$store->addWatch( $user, $title, 'invalid expiry' );
$this->assertSame(
'20300101000000',
$expiry,
$store->loadWatchedItem( $user, $title )->getExpiry()
);
$this->assertEquals( $initialUserWatchedItems + 1, $store->countWatchedItems( $user ) );
@ -173,7 +186,9 @@ class WatchedItemStoreIntegrationTest extends MediaWikiTestCase {
$title2 = Title::newFromText( 'WatchedItemStoreIntegrationTestPage1' );
$store = MediaWikiServices::getInstance()->getWatchedItemStore();
$timestamp = '20500101000000';
// Use a relative timestamp in the near future to ensure we don't exceed the max.
// See testWatchAndUnWatchItemWithExpiry() for tests regarding the max duration.
$timestamp = wfTimestamp( TS_MW, strtotime( '1 week' ) );
$store->addWatchBatchForUser( $user, [ $title1, $title2 ], $timestamp );
$this->assertSame(

View file

@ -1,5 +1,6 @@
<?php
use MediaWiki\Config\ServiceOptions;
use MediaWiki\Linker\LinkTarget;
use MediaWiki\Revision\RevisionLookup;
use MediaWiki\Revision\RevisionRecord;
@ -171,20 +172,26 @@ class WatchedItemStoreUnitTest extends MediaWikiTestCase {
* * nsInfo
* * revisionLookup
* * expiryEnabled
* * maxExpiryDuration
* @return WatchedItemStore
*/
private function newWatchedItemStore( array $mocks = [] ) : WatchedItemStore {
$options = new ServiceOptions( WatchedItemStore::CONSTRUCTOR_OPTIONS, [
'UpdateRowsPerQuery' => 1000,
'WatchlistExpiry' => $mocks['expiryEnabled'] ?? true,
'WatchlistExpiryMaxDuration' => $mocks['maxExpiryDuration'] ?? null,
] );
return new WatchedItemStore(
$options,
$mocks['lbFactory'] ??
$this->getMockLBFactory( $mocks['db'] ?? $this->getMockDb() ),
$mocks['queueGroup'] ?? $this->getMockJobQueueGroup(),
new HashBagOStuff(),
$mocks['cache'] ?? $this->getMockCache(),
$mocks['readOnlyMode'] ?? $this->getMockReadOnlyMode(),
1000,
$mocks['nsInfo'] ?? $this->getMockNsInfo(),
$mocks['revisionLookup'] ?? $this->getMockRevisionLookup(),
$mocks['expiryEnabled'] ?? true
$mocks['revisionLookup'] ?? $this->getMockRevisionLookup()
);
}