Thijs/wiki.techinc.nl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Don't consult ActionFactory for pages that can't exist

Browse source 
Since 65f89072e3, the Article constructor will throw if a page
can't exist, which means that the Action hierarchy cannot act on pages
that can't exist. So guard the Article construction in
PermissionManager::checkUserBlock(), by assuming that the relevant
Action has nothing to say about such pages.

Bug: T306358
Change-Id: I9fd1324f1df98e8adb9daf4d3e63eb00499bad8e
This commit is contained in:
Tim Starling 2022-04-29 09:34:43 +10:00 committed by James D. Forrester
parent 6eb9aaa43f
commit 28db3e6181
2 changed files with 38 additions and 16 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

35
includes/Permissions/PermissionManager.php
View file

@ -827,23 +827,26 @@ class PermissionManager {
// There is no way to instantiate an action by restriction. However, this
// will get the action where the restriction is the same. This may result
// in actions being blocked that shouldn't be.
// TODO: this drags a ton of dependencies in, would be good to avoid Article
// instantiation and decouple it creating an ActionPermissionChecker interface
// Creating an action will perform several database queries to ensure that
// the action has not been overridden by the content type.
// FIXME: avoid use of RequestContext since it drags in User and Title dependencies
// probably we may use fake context object since it's unlikely that Action uses it
// anyway. It would be nice if we could avoid instantiating the Action at all.
$actionObj = null;
$title = Title::newFromLinkTarget( $page, 'clone' );
$context = RequestContext::getMain();
$actionObj = Action::factory(
$action,
Article::newFromTitle( $title, $context ),
$context
);
// Ensure that the retrieved action matches the restriction.
if ( $actionObj && $actionObj->getRestriction() !== $action ) {
$actionObj = null;
if ( $title->canExist() ) {
// TODO: this drags a ton of dependencies in, would be good to avoid Article
// instantiation and decouple it creating an ActionPermissionChecker interface
// Creating an action will perform several database queries to ensure that
// the action has not been overridden by the content type.
// FIXME: avoid use of RequestContext since it drags in User and Title dependencies
// probably we may use fake context object since it's unlikely that Action uses it
// anyway. It would be nice if we could avoid instantiating the Action at all.
$context = RequestContext::getMain();
$actionObj = Action::factory(
$action,
Article::newFromTitle( $title, $context ),
$context
);
// Ensure that the retrieved action matches the restriction.
if ( $actionObj && $actionObj->getRestriction() !== $action ) {
$actionObj = null;
}
}
// If no action object is returned, assume that the action requires unblock

19
tests/phpunit/includes/Permissions/PermissionManagerTest.php
View file

@ -1359,4 +1359,23 @@ class PermissionManagerTest extends MediaWikiLangTestCase {
$this->assertFalse( $permManager->userCan( 'undelete', $admin, $userJs ) );
$this->assertTrue( $permManager->userCan( 'undelete', $interfaceAdmin, $userJs ) );
}
/**
* Regression test for T306358 -- proper page assertion when checking
* blocked status on a special page
*/
public function testBlockedFromNonProperPage() {
$page = Title::newFromText( 'Special:Blankpage' );
$pm = $this->getServiceContainer()->getPermissionManager();
$user = $this->getMockBuilder( User::class )
->onlyMethods( [ 'getBlock' ] )
->getMock();
$user->method( 'getBlock' )
->willReturn( new DatabaseBlock( [
'address' => '127.0.8.1',
'by' => $this->user,
] ) );
$errors = $pm->getPermissionErrors( 'test', $user, $page );
$this->assertNotEmpty( $errors );
}
}