Page Menu
Home
Phabricator
Search
Configure Global Search
Log In
Files
F33923245
T260349-2.patch
Lucas_Werkmeister_WMDE (Lucas Werkmeister (WMDE))
Actions
View File
Edit File
Delete File
View Transforms
Subscribe
Authored By
Lucas_Werkmeister_WMDE
Nov 23 2020, 4:41 PM
2020-11-23 16:41:52 (UTC+0)
Size
14 KB
Referenced Files
None
Subscribers
None
T260349-2.patch
View Options
From dab52c3b059b8a8609ec8935f255ec10eb344971 Mon Sep 17 00:00:00 2001
From: Itamar Givon <itamar.givon@wikimedia.de>
Date: Fri, 20 Nov 2020 13:22:17 +0100
Subject: [PATCH] SECURITY: Add onArticleDelete hook handler to
EntityDataPurger
As a followup to the introduction of EntityDataPurger, this hook handler
ensures that all cached urls of a deleted entity and its revisions are
purged after page deletion.
Bug: T260349
Change-Id: Ic270b2814e59c73b964c60533082eb73d46d72c3
---
extension-repo.json | 5 +-
repo/includes/Hooks/EntityDataPurger.php | 111 ++++++++-
.../includes/Hooks/EntityDataPurgerTest.php | 225 +++++++++++++-----
3 files changed, 275 insertions(+), 66 deletions(-)
diff --git a/extension-repo.json b/extension-repo.json
index 32edde2886..b7a6432355 100644
--- a/extension-repo.json
+++ b/extension-repo.json
@@ -479,7 +479,9 @@
"class": "\\Wikibase\\Repo\\Hooks\\EntityDataPurger",
"factory": "\\Wikibase\\Repo\\Hooks\\EntityDataPurger::factory",
"services": [
- "HtmlCacheUpdater"
+ "HtmlCacheUpdater",
+ "DBLoadBalancerFactory",
+ "MainConfig"
]
},
"FederatedPropertiesSpecialPage": {
@@ -530,6 +532,7 @@
"ApiQuery::moduleManager": "\\Wikibase\\Repo\\RepoHooks::onApiQueryModuleManager",
"ArticleDeleteComplete": [
"DeleteDispatcher",
+ "EntityDataPurger",
"\\Wikibase\\Repo\\RepoHooks::onArticleDeleteComplete"
],
"ArticleRevisionVisibilitySet": [
diff --git a/repo/includes/Hooks/EntityDataPurger.php b/repo/includes/Hooks/EntityDataPurger.php
index aaa63ed94a..69eaf82e0c 100644
--- a/repo/includes/Hooks/EntityDataPurger.php
+++ b/repo/includes/Hooks/EntityDataPurger.php
@@ -4,17 +4,21 @@
namespace Wikibase\Repo\Hooks;
+use BatchRowIterator;
+use Config;
use HtmlCacheUpdater;
use MediaWiki\Hook\ArticleRevisionVisibilitySetHook;
+use MediaWiki\Page\Hook\ArticleDeleteCompleteHook;
use Title;
use Wikibase\Lib\Store\EntityIdLookup;
use Wikibase\Repo\LinkedData\EntityDataUriManager;
use Wikibase\Repo\WikibaseRepo;
+use Wikimedia\Rdbms\ILBFactory;
/**
* @license GPL-2.0-or-later
*/
-class EntityDataPurger implements ArticleRevisionVisibilitySetHook {
+class EntityDataPurger implements ArticleRevisionVisibilitySetHook, ArticleDeleteCompleteHook {
/** @var EntityIdLookup */
private $entityIdLookup;
@@ -24,31 +28,49 @@ class EntityDataPurger implements ArticleRevisionVisibilitySetHook {
/** @var HtmlCacheUpdater */
private $htmlCacheUpdater;
+ /**
+ * @var ILBFactory
+ */
+ private $loadBalancerFactory;
+ /**
+ * @var int
+ */
+ private $batchSize;
public function __construct(
EntityIdLookup $entityIdLookup,
EntityDataUriManager $entityDataUriManager,
- HtmlCacheUpdater $htmlCacheUpdater
+ HtmlCacheUpdater $htmlCacheUpdater,
+ ILBFactory $loadBalancerFactory,
+ int $batchSize
) {
$this->entityIdLookup = $entityIdLookup;
$this->entityDataUriManager = $entityDataUriManager;
$this->htmlCacheUpdater = $htmlCacheUpdater;
+ $this->loadBalancerFactory = $loadBalancerFactory;
+ $this->batchSize = $batchSize;
}
- public static function factory( HtmlCacheUpdater $htmlCacheUpdater ): self {
+ public static function factory(
+ HtmlCacheUpdater $htmlCacheUpdater,
+ ILBFactory $loadBalancerFactory,
+ Config $config
+ ): self {
$wikibaseRepo = WikibaseRepo::getDefaultInstance();
return new self(
$wikibaseRepo->getEntityIdLookup(),
$wikibaseRepo->getEntityDataUriManager(),
- $htmlCacheUpdater
+ $htmlCacheUpdater,
+ $loadBalancerFactory,
+ $config->get( 'UpdateRowsPerQuery' )
);
}
/**
- * @param Title $title
- * @param int[] $ids
- * @param int[][] $visibilityChangeMap
- */
+ * @param Title $title
+ * @param int[] $ids
+ * @param int[][] $visibilityChangeMap
+ */
public function onArticleRevisionVisibilitySet( $title, $ids, $visibilityChangeMap ): void {
$entityId = $this->entityIdLookup->getEntityIdForTitle( $title );
if ( !$entityId ) {
@@ -68,4 +90,77 @@ public function onArticleRevisionVisibilitySet( $title, $ids, $visibilityChangeM
}
}
+ /**
+ * @param \WikiPage $wikiPage
+ * @param \User $user
+ * @param string $reason
+ * @param int $id
+ * @param \Content|null $content
+ * @param \ManualLogEntry $logEntry
+ * @param int $archivedRevisionCount
+ * @return bool|void
+ */
+ public function onArticleDeleteComplete(
+ $wikiPage,
+ $user,
+ $reason,
+ $id,
+ $content,
+ $logEntry,
+ $archivedRevisionCount
+ ) {
+ $title = $wikiPage->getTitle();
+ $entityId = $this->entityIdLookup->getEntityIdForTitle( $title );
+ if ( !$entityId ) {
+ return;
+ }
+
+ // This ensures we would delete also EntityData for non revision based urls of an entity.
+ $urls = $this->entityDataUriManager->getPotentiallyCachedUrls( $entityId );
+
+ foreach ( $this->getArchivedRevisionIds( $title, $id ) as $revisionId ) {
+ $urls = array_merge( $urls, $this->entityDataUriManager->getPotentiallyCachedUrls(
+ $entityId,
+ (int)$revisionId
+ ) );
+ }
+
+ if ( $urls !== [] ) {
+ $this->htmlCacheUpdater->purgeUrls( $urls );
+ }
+ }
+
+ private function getArchivedRevisionIds( Title $title, int $id ): iterable {
+ // ensure that we can see the archived rows on the replica
+ $this->loadBalancerFactory->waitForReplication( [
+ 'domain' => $this->loadBalancerFactory->getLocalDomainID(),
+ ] );
+
+ $dbr = $this->loadBalancerFactory
+ ->getMainLB()
+ ->getConnectionRef( DB_REPLICA );
+
+ $iterator = new BatchRowIterator(
+ $dbr,
+ 'archive',
+ [ 'ar_id' ],
+ $this->batchSize
+ );
+
+ $iterator->setFetchColumns( [ 'ar_rev_id' ] );
+ $iterator->addConditions( [
+ 'ar_page_id' => $id,
+ 'ar_title' => $title->getDBkey(),
+ 'ar_namespace' => $title->getNamespace()
+ ] );
+
+ foreach ( $iterator as $batch ) {
+ yield from array_map(
+ function ( $row ) {
+ return $row->ar_rev_id;
+ },
+ $batch
+ );
+ }
+ }
}
diff --git a/repo/tests/phpunit/includes/Hooks/EntityDataPurgerTest.php b/repo/tests/phpunit/includes/Hooks/EntityDataPurgerTest.php
index a58766c619..b5337aa729 100644
--- a/repo/tests/phpunit/includes/Hooks/EntityDataPurgerTest.php
+++ b/repo/tests/phpunit/includes/Hooks/EntityDataPurgerTest.php
@@ -10,8 +10,12 @@
use Wikibase\DataModel\Entity\ItemId;
use Wikibase\Lib\Store\EntityIdLookup;
use Wikibase\Repo\Hooks\EntityDataPurger;
-use Wikibase\Repo\LinkedData\EntityDataRequestHandler;
use Wikibase\Repo\LinkedData\EntityDataUriManager;
+use Wikimedia\Rdbms\FakeResultWrapper;
+use Wikimedia\Rdbms\IDatabase;
+use Wikimedia\Rdbms\ILBFactory;
+use Wikimedia\Rdbms\ILoadBalancer;
+use WikiPage;
/**
* @covers \Wikibase\Repo\Hooks\EntityDataPurger
@@ -21,78 +25,185 @@
* @license GPL-2.0-or-later
*/
class EntityDataPurgerTest extends TestCase {
+ private const UNUSED_ARTICLE_DELETE_ARGS = [ null, '', 0, null, null, 0 ];
- public function testGivenEntityIdLookupReturnsNull_handlerDoesNothing() {
- $title = Title::newFromText( 'Project:About' );
+ private function getEntityIdLookup( Title $title, ?ItemId $entityId ): EntityIdLookup {
$entityIdLookup = $this->createMock( EntityIdLookup::class );
$entityIdLookup->expects( $this->once() )
->method( 'getEntityIdForTitle' )
->with( $title )
- ->willReturn( null );
- $entityDataUriManager = $this->createMock( EntityDataUriManager::class );
- $entityDataUriManager->expects( $this->never() )
- ->method( 'getPotentiallyCachedUrls' );
- $htmlCacheUpdater = $this->createMock( HtmlCacheUpdater::class );
- $htmlCacheUpdater->expects( $this->never() )
- ->method( 'purgeUrls' );
- $purger = new EntityDataPurger( $entityIdLookup, $entityDataUriManager, $htmlCacheUpdater );
+ ->willReturn( $entityId );
- $purger->onArticleRevisionVisibilitySet( $title, [ 1, 2, 3 ], [] );
+ return $entityIdLookup;
}
- public function testGivenEntityIdLookupReturnsId_handlerPurgesCache() {
- $title = Title::newFromText( 'Item:Q1' );
- $entityId = new ItemId( 'Q1' );
- $entityIdLookup = $this->createMock( EntityIdLookup::class );
- $entityIdLookup->expects( $this->once() )
- ->method( 'getEntityIdForTitle' )
- ->with( $title )
- ->willReturn( $entityId );
+ private function getEntityDataUriManager(
+ array $idsToTest,
+ ?ItemId $entityId,
+ bool $baseItemUrl = false
+ ): EntityDataUriManager {
$entityDataUriManager = $this->createMock( EntityDataUriManager::class );
- $entityDataUriManager->expects( $this->once() )
+
+ if ( $entityId === null ) {
+ $entityDataUriManager->expects( $this->never() )
+ ->method( 'getPotentiallyCachedUrls' );
+ return $entityDataUriManager;
+ }
+
+ $calls = count( $idsToTest );
+ $QID = $entityId->getSerialization();
+
+ $arguments = array_map( function ( $revisionId ) use ( $entityId ) {
+ return [ $entityId, $revisionId ];
+ }, $idsToTest );
+
+ $returns = array_map( function ( $revisionId ) use ( $QID ) {
+ $suffix = $QID . '/' . $revisionId;
+ // returns [ 'urlA/Q1/1', 'urlB/Q1/1' ]
+ return [
+ 'urlA/' . $suffix,
+ 'urlB/' . $suffix
+ ];
+ }, $idsToTest );
+
+ if ( $baseItemUrl ) {
+ array_unshift( $arguments, [ $entityId ] );
+ array_unshift( $returns, [
+ 'urlA/' . $QID,
+ 'urlB/' . $QID,
+ ] );
+ $calls += 1;
+ }
+
+ $entityDataUriManager->expects( $this->exactly( $calls ) )
->method( 'getPotentiallyCachedUrls' )
- ->with( $entityId, 1 )
- ->willReturn( [ 'urlA/Q1/1', 'urlB/Q1/1' ] );
- $htmlCacheUpdater = $this->createMock( HtmlCacheUpdater::class );
- $htmlCacheUpdater->expects( $this->once() )
- ->method( 'purgeUrls' )
- ->with( [ 'urlA/Q1/1', 'urlB/Q1/1' ] );
- $purger = new EntityDataPurger( $entityIdLookup, $entityDataUriManager, $htmlCacheUpdater );
+ ->withConsecutive( ...$arguments )
+ ->willReturnOnConsecutiveCalls( ...$returns );
- $purger->onArticleRevisionVisibilitySet( $title, [ 1 ], [] );
+ return $entityDataUriManager;
}
- public function testGivenMultipleRevisions_handlerPurgesCacheOnce() {
- $title = Title::newFromText( 'Item:Q1' );
- $entityId = new ItemId( 'Q1' );
- $entityIdLookup = $this->createMock( EntityIdLookup::class );
- $entityIdLookup->expects( $this->once() )
- ->method( 'getEntityIdForTitle' )
- ->with( $title )
- ->willReturn( $entityId );
- $entityDataUriManager = $this->createMock( EntityDataUriManager::class );
- $entityDataUriManager
- ->method( 'getPotentiallyCachedUrls' )
- ->withConsecutive(
- [ $entityId, 1 ],
- [ $entityId, 2 ],
- [ $entityId, 3 ]
- )
- ->willReturnOnConsecutiveCalls(
- [ 'urlA/Q1/1', 'urlB/Q1/1' ],
- [ 'urlA/Q1/2', 'urlB/Q1/2' ],
- [ 'urlA/Q1/3', 'urlB/Q1/3' ]
- );
+ private function getHtmlCacheUpdater(
+ array $idsToTest,
+ ?ItemId $entityId,
+ bool $baseItemUrl = false
+ ): HtmlCacheUpdater {
$htmlCacheUpdater = $this->createMock( HtmlCacheUpdater::class );
+
+ if ( $entityId === null ) {
+ $htmlCacheUpdater->expects( $this->never() )
+ ->method( 'purgeUrls' );
+ return $htmlCacheUpdater;
+ }
+
+ $QID = $entityId->getSerialization();
+
+ $urls = array_reduce( $idsToTest, function ( $urls, $revisionId ) use ( $QID ) {
+ $suffix = $QID . '/' . $revisionId;
+
+ return array_merge( $urls, [
+ 'urlA/' . $suffix,
+ 'urlB/' . $suffix
+ ] );
+ }, [] );
+
+ if ( $baseItemUrl ) {
+ array_unshift( $urls, 'urlA/' . $QID, 'urlB/' . $QID );
+ }
+
$htmlCacheUpdater->expects( $this->once() )
->method( 'purgeUrls' )
- ->with( [
- 'urlA/Q1/1', 'urlB/Q1/1',
- 'urlA/Q1/2', 'urlB/Q1/2',
- 'urlA/Q1/3', 'urlB/Q1/3',
- ] );
- $purger = new EntityDataPurger( $entityIdLookup, $entityDataUriManager, $htmlCacheUpdater );
+ ->with( $urls );
+
+ return $htmlCacheUpdater;
+ }
+
+ private function getLBFactory( array $returnIds, int $batchSize ): ILBFactory {
+ $mockDB = $this->createMock( IDatabase::class );
+ $mockLB = $this->createMock( ILoadBalancer::class );
+ $mockFactory = $this->createMock( ILBFactory::class );
+
+ $idBatches = array_chunk( $returnIds, $batchSize );
+ $returnBatches = array_map( function ( $batch ) {
+ $rows = array_map( function ( $id ) {
+ return [
+ "ar_id" => 0,
+ "ar_rev_id" => $id
+ ];
+ }, $batch );
+
+ return new FakeResultWrapper( $rows );
+ }, $idBatches );
+
+ // Add empty result to end batch iteration
+ array_push( $returnBatches, new FakeResultWrapper( [] ) );
+
+ $mockDB->method( 'select' )
+ ->willReturnOnConsecutiveCalls( ...$returnBatches );
+
+ $mockLB->method( 'getConnectionRef' )
+ ->willReturn( $mockDB );
+
+ $mockFactory->method( 'getMainLB' )
+ ->willReturn( $mockLB );
+
+ return $mockFactory;
+ }
+
+ private function getEntityDataPurger(
+ Title $title,
+ array $idsToTest,
+ int $batchSize = 5,
+ bool $failedLookup = false,
+ bool $withBaseUri = false
+ ): EntityDataPurger {
+ $entityId = $failedLookup ? null : new ItemId( 'Q1' );
+
+ return new EntityDataPurger(
+ $this->getEntityIdLookup( $title, $entityId ),
+ $this->getEntityDataUriManager( $idsToTest, $entityId, $withBaseUri ),
+ $this->getHtmlCacheUpdater( $idsToTest, $entityId, $withBaseUri ),
+ $this->getLBFactory( $idsToTest, $batchSize ),
+ $batchSize
+ );
+ }
+
+ public function revisionsProvider(): array {
+ // [ array $revisionIds, int $batchSize, bool $failedLookup ]
+ return [
+ 'entity id lookup returns null' => [ [], 5, true ],
+ 'single revision id' => [ [ 1 ] ],
+ 'multiple revision ids' => [ [ 1, 2, 3 ] ],
+ 'batch size smaller than number of ids' => [ [ 1, 2, 3 ], 2 ]
+ ];
+ }
+
+ /**
+ * @dataProvider revisionsProvider
+ */
+ public function testOnArticleRevisionVisibilitySet(
+ array $revisionIds,
+ int $batchSize = 5,
+ bool $failedLookup = false
+ ) {
+ $title = Title::newFromText( 'Test:Title' );
+ $purger = $this->getEntityDataPurger( $title, $revisionIds, $batchSize, $failedLookup );
+ $purger->onArticleRevisionVisibilitySet( $title, $revisionIds, [] );
+ }
- $purger->onArticleRevisionVisibilitySet( $title, [ 1, 2, 3 ], [] );
+ /**
+ * @dataProvider revisionsProvider
+ */
+ public function testOnArticleDeleteComplete(
+ array $revisionIds,
+ int $batchSize = 5,
+ bool $failedLookup = false
+ ) {
+ $title = Title::newFromText( 'Test:Title' );
+ $page = $this->createMock( WikiPage::class );
+ $page->method( 'getTitle' )
+ ->willReturn( $title );
+ $purger = $this->getEntityDataPurger( $title, $revisionIds, $batchSize, $failedLookup, true );
+ $purger->onArticleDeleteComplete( $page, ...self::UNUSED_ARTICLE_DELETE_ARGS );
}
}
--
2.27.0
File Metadata
Details
Attached
Mime Type
text/x-diff
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
8790973
Default Alt Text
T260349-2.patch (14 KB)
Attached To
Mode
T260349: Wikibase does not purge cached Special:EntityData URLs when revisions or entities are deleted
Attached
Detach File
Event Timeline
Log In to Comment