From 707188738300fd4c0c467901d976b5c2e2b5accf Mon Sep 17 00:00:00 2001 From: Dylan F Date: Sun, 19 Jan 2025 03:06:14 +0000 Subject: PermissionManager: Differentiate between cascading protection of file content and file pages This patch reworks RestrictionStore::getCascadeProtectionSourcesInternal to return a third and fourth array: * One for cascading restrictions originating from templatelinks * Another for those originating from imagelinks They are used in PermissionManager::checkCascadingSourcesRestrictions to differentiate cascading protection of file content and file page, but could also be used in the future by action=info and other callers. Bug: T24521 Bug: T62109 Bug: T140010 Change-Id: Ia5863f418538106f4fd657c672298ff6ac835805 (cherry picked from commit 7a4952ef2c5d593fae9419bad39f3e9894f42adf) --- includes/Permissions/PermissionManager.php | 28 +++++-- includes/Permissions/RestrictionStore.php | 90 ++++++++++++++-------- includes/editpage/IntroMessageBuilder.php | 24 +++--- .../includes/Permissions/PermissionManagerTest.php | 55 ++++++++++++- .../includes/Permissions/RestrictionStoreTest.php | 43 ++++++++++- .../includes/Permissions/RestrictionStoreTest.php | 63 +++++++++++++-- 6 files changed, 247 insertions(+), 56 deletions(-) diff --git a/includes/Permissions/PermissionManager.php b/includes/Permissions/PermissionManager.php index 84b3ce6ddd04..d295ee8f0ee9 100644 --- a/includes/Permissions/PermissionManager.php +++ b/includes/Permissions/PermissionManager.php @@ -1130,14 +1130,32 @@ class PermissionManager { ): void { // TODO: remove & rework upon further use of LinkTarget $title = Title::newFromLinkTarget( $page ); + if ( $rigor !== self::RIGOR_QUICK && !$title->isUserConfigPage() ) { - [ $cascadingSources, $restrictions ] = $this->restrictionStore->getCascadeProtectionSources( $title ); + [ $sources, $restrictions, $tlSources, $ilSources ] = $this->restrictionStore + ->getCascadeProtectionSources( $title ); + + // If the file Wikitext isn't transcluded then we + // don't care about edit cascade restrictions for edit action + if ( $action === 'edit' && $page->getNamespace() === NS_FILE && !$tlSources ) { + return; + } + + // For the purposes of cascading protection, edit restrictions should apply to uploads or moves + // Thus remap upload and move to edit + // Unless the file content itself is not transcluded + if ( $ilSources && ( $action === 'upload' || $action === 'move' ) ) { + $restrictedAction = 'edit'; + } else { + $restrictedAction = $action; + } + // Cascading protection depends on more than this page... // Several cascading protected pages may include this page... // Check each cascading level // This is only for protection restrictions, not for all actions - if ( isset( $restrictions[$action] ) ) { - foreach ( $restrictions[$action] as $right ) { + if ( isset( $restrictions[$restrictedAction] ) ) { + foreach ( $restrictions[$restrictedAction] as $right ) { // Backwards compatibility, rewrite sysop -> editprotected if ( $right === 'sysop' ) { $right = 'editprotected'; @@ -1148,10 +1166,10 @@ class PermissionManager { } if ( $right != '' && !$this->userHasAllRights( $user, 'protect', $right ) ) { $wikiPages = ''; - foreach ( $cascadingSources as $pageIdentity ) { + foreach ( $sources as $pageIdentity ) { $wikiPages .= '* [[:' . $this->titleFormatter->getPrefixedText( $pageIdentity ) . "]]\n"; } - $status->fatal( 'cascadeprotected', count( $cascadingSources ), $wikiPages, $action ); + $status->fatal( 'cascadeprotected', count( $sources ), $wikiPages, $action ); } } } diff --git a/includes/Permissions/RestrictionStore.php b/includes/Permissions/RestrictionStore.php index d9d7dc3a156d..4f7cdd6d7998 100644 --- a/includes/Permissions/RestrictionStore.php +++ b/includes/Permissions/RestrictionStore.php @@ -520,9 +520,13 @@ class RestrictionStore { * Cascading protection: Get the source of any cascading restrictions on this page. * * @param PageIdentity $page Must be local - * @return array[] Two elements: First is an array of PageIdentity objects of the pages from - * which cascading restrictions have come, which may be empty. Second is an array like that - * returned by getAllRestrictions(). + * @return array[] Four elements: First is an array of PageIdentity objects combining the + * third and fourth elements of this array, which may be empty. + * Second is an array like that returned by getAllRestrictions(). + * Third is an array of PageIdentity objects of the pages from + * which cascading restrictions have come, orginating via templatelinks, which may be empty. + * Fourth is an array of PageIdentity objects of the pages from + * which cascading restrictions have come, orginating via imagelinks, which may be empty. */ public function getCascadeProtectionSources( PageIdentity $page ): array { $page->assertWiki( PageIdentity::LOCAL ); @@ -542,7 +546,7 @@ class RestrictionStore { PageIdentity $page, bool $shortCircuit = false ) { if ( !$page->canExist() ) { - return $shortCircuit ? false : [ [], [] ]; + return $shortCircuit ? false : [ [], [], [], [] ]; } $cacheEntry = &$this->cache[CacheKeyHelper::getKeyForPage( $page )]; @@ -554,38 +558,56 @@ class RestrictionStore { } $dbr = $this->loadBalancer->getConnection( DB_REPLICA ); - $queryBuilder = $dbr->newSelectQueryBuilder(); - $queryBuilder->select( [ 'pr_expiry' ] ) + + $baseQuery = $dbr->newSelectQueryBuilder() + ->select( $shortCircuit ? [ 'pr_expiry' ] : [ + 'pr_expiry', + 'pr_page', + 'page_namespace', + 'page_title', + 'pr_type', + 'pr_level' + ] ) ->from( 'page_restrictions' ) ->where( [ 'pr_cascade' => 1 ] ); - if ( $page->getNamespace() === NS_FILE ) { - // Files transclusion may receive cascading protection in the future - // see https://phabricator.wikimedia.org/T241453 - $queryBuilder->join( 'imagelinks', null, 'il_from=pr_page' ); - $queryBuilder->andWhere( [ 'il_to' => $page->getDBkey() ] ); - } else { - $queryBuilder->join( 'templatelinks', null, 'tl_from=pr_page' ); - $queryBuilder->andWhere( - $this->linksMigration->getLinksConditions( - 'templatelinks', - TitleValue::newFromPage( $page ) - ) - ); - } - if ( !$shortCircuit ) { - $queryBuilder->fields( [ 'pr_page', 'page_namespace', 'page_title', 'pr_type', 'pr_level' ] ); - $queryBuilder->join( 'page', null, 'page_id=pr_page' ); + $baseQuery->join( 'page', null, 'page_id=pr_page' ); } - $res = $queryBuilder->caller( __METHOD__ )->fetchResultSet(); + $imageQuery = clone $baseQuery; + $imageQuery->join( 'imagelinks', null, 'il_from=pr_page' ) + ->fields( [ + 'type' => $dbr->addQuotes( 'il' ), + ] ) + ->andWhere( [ 'il_to' => $page->getDBkey() ] ); + + $templateQuery = clone $baseQuery; + $templateQuery->join( 'templatelinks', null, 'tl_from=pr_page' ) + ->fields( [ + 'type' => $dbr->addQuotes( 'tl' ), + ] ) + ->andWhere( + $this->linksMigration->getLinksConditions( 'templatelinks', TitleValue::newFromPage( $page ) ) + ); + + if ( $page->getNamespace() === NS_FILE ) { + $unionQuery = $dbr->newUnionQueryBuilder() + ->add( $imageQuery ) + ->add( $templateQuery ) + ->all(); - $sources = []; + $res = $unionQuery->caller( __METHOD__ )->fetchResultSet(); + } else { + $res = $templateQuery->caller( __METHOD__ )->fetchResultSet(); + } + + $tlSources = []; + $ilSources = []; $pageRestrictions = []; $now = wfTimestampNow(); - foreach ( $res as $row ) { + $expiry = $dbr->decodeExpiry( $row->pr_expiry ); if ( $expiry > $now ) { if ( $shortCircuit ) { @@ -593,8 +615,14 @@ class RestrictionStore { return true; } - $sources[$row->pr_page] = new PageIdentityValue( $row->pr_page, - $row->page_namespace, $row->page_title, PageIdentity::LOCAL ); + if ( $row->type === 'il' ) { + $ilSources[$row->pr_page] = new PageIdentityValue( $row->pr_page, + $row->page_namespace, $row->page_title, PageIdentity::LOCAL ); + } elseif ( $row->type === 'tl' ) { + $tlSources[$row->pr_page] = new PageIdentityValue( $row->pr_page, + $row->page_namespace, $row->page_title, PageIdentity::LOCAL ); + } + // Add groups needed for each restriction type if its not already there // Make sure this restriction type still exists @@ -608,14 +636,16 @@ class RestrictionStore { } } + $sources = array_replace( $tlSources, $ilSources ); + $cacheEntry['has_cascading'] = (bool)$sources; + $cacheEntry['cascade_sources'] = [ $sources, $pageRestrictions, $tlSources, $ilSources ]; if ( $shortCircuit ) { return false; } - $cacheEntry['cascade_sources'] = [ $sources, $pageRestrictions ]; - return [ $sources, $pageRestrictions ]; + return [ $sources, $pageRestrictions, $tlSources, $ilSources ]; } /** diff --git a/includes/editpage/IntroMessageBuilder.php b/includes/editpage/IntroMessageBuilder.php index e9a19c3c170d..21eb4ad687eb 100644 --- a/includes/editpage/IntroMessageBuilder.php +++ b/includes/editpage/IntroMessageBuilder.php @@ -637,18 +637,20 @@ class IntroMessageBuilder { } if ( $this->restrictionStore->isCascadeProtected( $page ) ) { # Is this page under cascading protection from some source pages? - $cascadeSources = $this->restrictionStore->getCascadeProtectionSources( $page )[0]; - $htmlList = ''; - # Explain, and list the titles responsible - foreach ( $cascadeSources as $source ) { - $htmlList .= Html::rawElement( 'li', [], $this->linkRenderer->makeLink( $source ) ); + $tlCascadeSources = $this->restrictionStore->getCascadeProtectionSources( $page )[2]; + if ( $tlCascadeSources ) { + $htmlList = ''; + # Explain, and list the titles responsible + foreach ( $tlCascadeSources as $source ) { + $htmlList .= Html::rawElement( 'li', [], $this->linkRenderer->makeLink( $source ) ); + } + $messages->addWithKey( + 'cascadeprotectedwarning', + $localizer->msg( 'cascadeprotectedwarning', count( $tlCascadeSources ) )->parse() . + ( $htmlList ? Html::rawElement( 'ul', [], $htmlList ) : '' ), + Html::warningBox( '$1', 'mw-cascadeprotectedwarning' ) + ); } - $messages->addWithKey( - 'cascadeprotectedwarning', - $localizer->msg( 'cascadeprotectedwarning', count( $cascadeSources ) )->parse() . - ( $htmlList ? Html::rawElement( 'ul', [], $htmlList ) : '' ), - Html::warningBox( '$1', 'mw-cascadeprotectedwarning' ) - ); } if ( !$page->exists() && $this->restrictionStore->getRestrictions( $page, 'create' ) ) { $messages->addWithKey( diff --git a/tests/phpunit/includes/Permissions/PermissionManagerTest.php b/tests/phpunit/includes/Permissions/PermissionManagerTest.php index 17edde7783ab..d72896f8f584 100644 --- a/tests/phpunit/includes/Permissions/PermissionManagerTest.php +++ b/tests/phpunit/includes/Permissions/PermissionManagerTest.php @@ -254,9 +254,15 @@ class PermissionManagerTest extends MediaWikiLangTestCase { [ Title::makeTitle( NS_MAIN, "Bogus" ), Title::makeTitle( NS_MAIN, "UnBogus" ) - ], [ + ], + [ "bogus" => [ 'bogus', "sysop", "protect", "" ], - ] + ], + [ + Title::makeTitle( NS_MAIN, "Bogus" ), + Title::makeTitle( NS_MAIN, "UnBogus" ) + ], + [] ], ] ]; @@ -275,6 +281,51 @@ class PermissionManagerTest extends MediaWikiLangTestCase { ); } + public function testCascadingSourcesRestrictionsForFile() { + $this->setTitle( NS_FILE, 'Test.jpg' ); + $this->overrideUserPermissions( $this->user, [ 'edit', 'move', 'upload', 'movefile', 'createpage' ] ); + + $rs = $this->getServiceContainer()->getRestrictionStore(); + $wrapper = TestingAccessWrapper::newFromObject( $rs ); + $wrapper->cache = [ CacheKeyHelper::getKeyForPage( $this->title ) => [ + 'cascade_sources' => [ + [ + Title::makeTitle( NS_MAIN, 'FileTemplate' ), + Title::makeTitle( NS_MAIN, 'FileUser' ) + ], + [ + 'edit' => [ 'sysop' ], + ], + [ + Title::makeTitle( NS_MAIN, 'FileTemplate' ) + ], + [ + Title::makeTitle( NS_MAIN, 'FileUser' ) + ] + ], + ] ]; + + $permissionManager = $this->getServiceContainer()->getPermissionManager(); + + $this->assertFalse( $permissionManager->userCan( 'upload', $this->user, $this->title ) ); + $this->assertEquals( [ + [ 'cascadeprotected', 2, "* [[:FileTemplate]]\n* [[:FileUser]]\n", 'upload' ] ], + $permissionManager->getPermissionErrors( 'upload', $this->user, $this->title ) + ); + + $this->assertFalse( $permissionManager->userCan( 'move', $this->user, $this->title ) ); + $this->assertEquals( [ + [ 'cascadeprotected', 2, "* [[:FileTemplate]]\n* [[:FileUser]]\n", 'move' ] ], + $permissionManager->getPermissionErrors( 'move', $this->user, $this->title ) + ); + + $this->assertFalse( $permissionManager->userCan( 'edit', $this->user, $this->title ) ); + $this->assertEquals( [ + [ 'cascadeprotected', 2, "* [[:FileTemplate]]\n* [[:FileUser]]\n", 'edit' ] ], + $permissionManager->getPermissionErrors( 'edit', $this->user, $this->title ) + ); + } + /** * @dataProvider provideActionPermissions */ diff --git a/tests/phpunit/integration/includes/Permissions/RestrictionStoreTest.php b/tests/phpunit/integration/includes/Permissions/RestrictionStoreTest.php index d0061e72a8e1..ebde127f2dd2 100644 --- a/tests/phpunit/integration/includes/Permissions/RestrictionStoreTest.php +++ b/tests/phpunit/integration/includes/Permissions/RestrictionStoreTest.php @@ -42,6 +42,10 @@ class RestrictionStoreTest extends MediaWikiIntegrationTestCase { private static $testPageRestrictionSource; /** @var array */ private static $testPageRestrictionCascade; + /** @var array */ + private static $testFileRestrictionSource; + /** @var array */ + private static $testFileTarget; protected function setUp(): void { parent::setUp(); @@ -65,6 +69,12 @@ class RestrictionStoreTest extends MediaWikiIntegrationTestCase { $this->insertPage( 'RestrictionStoreTest_1', '{{RestrictionStoreTestB}}' ); $this->updateRestrictions( self::$testPageRestrictionSource['title'], [ 'edit' => 'sysop' ] ); + + self::$testFileTarget = $this->insertPage( 'File:RestrictionStoreTest.jpg', 'test file' ); + self::$testFileRestrictionSource = + $this->insertPage( 'RestrictionStoreTest_File', '[[File:RestrictionStoreTest.jpg]]' ); + + $this->updateRestrictions( self::$testFileRestrictionSource['title'], [ 'edit' => 'sysop' ], 1 ); } private function newRestrictionStore( array $options = [] ) { @@ -100,22 +110,49 @@ class RestrictionStoreTest extends MediaWikiIntegrationTestCase { $page = self::$testPageRestrictionCascade['title']; $pageSource = self::$testPageRestrictionSource['title']; - [ $sources, $restrictions ] = $this->newRestrictionStore() + [ $sources, $restrictions, $tlSources, $ilSources ] = $this->newRestrictionStore() ->getCascadeProtectionSources( $page ); $this->assertCount( 1, $sources ); + $this->assertCount( 1, $tlSources ); + $this->assertCount( 0, $ilSources ); $this->assertTrue( $pageSource->isSamePageAs( $sources[$pageSource->getId()] ) ); $this->assertArrayEquals( [ 'edit' => [ 'sysop' ] ], $restrictions ); - [ $sources, $restrictions ] = $this->newRestrictionStore() + [ $sources, $restrictions, $tlSources, $ilSources ] = $this->newRestrictionStore() ->getCascadeProtectionSources( $pageSource ); $this->assertCount( 0, $sources ); + $this->assertCount( 0, $tlSources ); + $this->assertCount( 0, $ilSources ); $this->assertCount( 0, $restrictions ); } public function testGetCascadeProtectionSourcesSpecialPage() { - [ $sources, $restrictions ] = $this->newRestrictionStore() + [ $sources, $restrictions, $tlSources, $ilSources ] = $this->newRestrictionStore() ->getCascadeProtectionSources( SpecialPage::getTitleFor( 'Whatlinkshere' ) ); $this->assertCount( 0, $sources ); + $this->assertCount( 0, $tlSources ); + $this->assertCount( 0, $ilSources ); + $this->assertCount( 0, $restrictions ); + } + + public function testGetCascadeProtectionSourcesFile() { + $page = self::$testFileTarget['title']; + $pageSource = self::$testFileRestrictionSource['title']; + + [ $sources, $restrictions, $tlSources, $ilSources ] = $this->newRestrictionStore() + ->getCascadeProtectionSources( $page ); + + $this->assertCount( 1, $sources ); + $this->assertTrue( $pageSource->isSamePageAs( $sources[$pageSource->getId()] ) ); + $this->assertArrayEquals( [ 'edit' => [ 'sysop' ] ], $restrictions ); + $this->assertCount( 1, $ilSources ); + $this->assertCount( 0, $tlSources ); + + [ $sources, $restrictions, $tlSources, $ilSources ] = $this->newRestrictionStore() + ->getCascadeProtectionSources( $pageSource ); + $this->assertCount( 0, $sources ); + $this->assertCount( 0, $tlSources ); + $this->assertCount( 0, $ilSources ); $this->assertCount( 0, $restrictions ); } diff --git a/tests/phpunit/unit/includes/Permissions/RestrictionStoreTest.php b/tests/phpunit/unit/includes/Permissions/RestrictionStoreTest.php index 4312ff05ab6e..cdcfa663de07 100644 --- a/tests/phpunit/unit/includes/Permissions/RestrictionStoreTest.php +++ b/tests/phpunit/unit/includes/Permissions/RestrictionStoreTest.php @@ -30,6 +30,7 @@ use Wikimedia\Rdbms\FakeResultWrapper; use Wikimedia\Rdbms\IDatabase; use Wikimedia\Rdbms\ILoadBalancer; use Wikimedia\Rdbms\SelectQueryBuilder; +use Wikimedia\Rdbms\UnionQueryBuilder; /** * @covers \MediaWiki\Permissions\RestrictionStore @@ -66,7 +67,7 @@ class RestrictionStoreTest extends MediaWikiUnitTestCase { foreach ( $expectedCalls as $index => $calls ) { $dbs[$index] = $this->createNoOpMock( IDatabase::class, - array_merge( array_keys( $calls ), [ 'newSelectQueryBuilder', 'newDeleteQueryBuilder' ] ) + array_merge( array_keys( $calls ), [ 'newSelectQueryBuilder', 'newDeleteQueryBuilder', 'newUnionQueryBuilder' ] ) ); foreach ( $calls as $method => $callback ) { $count = 1; @@ -81,6 +82,11 @@ class RestrictionStoreTest extends MediaWikiUnitTestCase { ->willReturnCallback( static function () use ( $dbs, $index ) { return new SelectQueryBuilder( $dbs[$index] ); } ); + $dbs[$index] + ->method( 'newUnionQueryBuilder' ) + ->willReturnCallback( static function () use ( $dbs, $index ) { + return new UnionQueryBuilder( $dbs[$index] ); + } ); $dbs[$index] ->method( 'newDeleteQueryBuilder' ) ->willReturnCallback( static function () use ( $dbs, $index ) { @@ -964,15 +970,61 @@ class RestrictionStoreTest extends MediaWikiUnitTestCase { static function () { return new FakeResultWrapper( [ (object)[ 'pr_page' => 1, 'page_namespace' => NS_MAIN, 'page_title' => 'test', - 'pr_expiry' => 'infinity', 'pr_type' => 'edit', 'pr_level' => 'Sysop' ] + 'pr_expiry' => 'infinity', 'pr_type' => 'edit', 'pr_level' => 'Sysop', + 'type' => 'tl' ] ] ); - } + }, + 'addQuotes' => [ + static function () { + return 'noop'; + }, + 2 + ] ] ] ] ); $page = PageIdentityValue::localIdentity( 1, NS_MAIN, 'X' ); - [ $sources, $restrictions ] = $obj->getCascadeProtectionSources( $page ); + [ $sources, $restrictions, $tlSources, $ilSources ] = $obj->getCascadeProtectionSources( $page ); $this->assertCount( 1, $sources ); $this->assertArrayHasKey( 'edit', $restrictions ); + $this->assertCount( 0, $ilSources ); + $this->assertCount( 1, $tlSources ); + } + + public function testGetCascadeProtectionSourcesFile() { + $obj = $this->newRestrictionStore( [ 'db' => [ DB_REPLICA => [ + 'addQuotes' => [ + static function () { + return 'noop'; + }, + 2 + ], + 'selectSQLText' => [ + static function () { + return 'noop'; + }, + 2 + ], + 'unionQueries' => static function () { + return 'noop'; + }, + 'query' => static function () { + return new FakeResultWrapper( [ + (object)[ 'pr_page' => 1, 'page_namespace' => NS_MAIN, 'page_title' => 'test1', + 'pr_expiry' => 'infinity', 'pr_type' => 'edit', 'pr_level' => 'Sysop', + 'type' => 'il' ], + (object)[ 'pr_page' => 2, 'page_namespace' => NS_MAIN, 'page_title' => 'test2', + 'pr_expiry' => 'infinity', 'pr_type' => 'edit', 'pr_level' => 'Sysop', + 'type' => 'tl' ] + ] ); + }, + ] ] ] ); + + $page = PageIdentityValue::localIdentity( 1, NS_FILE, 'Image.jpg' ); + [ $sources, $restrictions, $tlSources, $ilSources ] = $obj->getCascadeProtectionSources( $page ); + $this->assertCount( 2, $sources ); + $this->assertCount( 1, $ilSources ); + $this->assertCount( 1, $tlSources ); + $this->assertArrayHasKey( 'edit', $restrictions ); } public function testGetCascadeProtectionSourcesSpecialPage() { @@ -984,9 +1036,10 @@ class RestrictionStoreTest extends MediaWikiUnitTestCase { ] ] ] ] ); $page = $this->makeMockTitle( 'Whatlinkshere', [ 'namespace' => NS_SPECIAL ] ); - [ $sources, $restrictions ] = $obj->getCascadeProtectionSources( $page ); + [ $sources, $restrictions, $ilSources ] = $obj->getCascadeProtectionSources( $page ); $this->assertCount( 0, $sources ); $this->assertCount( 0, $restrictions ); + $this->assertCount( 0, $ilSources ); } public function testShouldNotFetchProtectionSettingsIfActionCannotBeRestricted(): void { -- cgit v1.2.3