From b1e4ca70b8e375d52c35b46fb3157fb852948263 Mon Sep 17 00:00:00 2001 From: David Barratt Date: Tue, 6 Nov 2018 11:02:46 -0500 Subject: [PATCH] Title::checkUserBlock should call User::isBlockedFrom for every action Currently, not all actions are processed by User::isBlockedFrom(). This results in users who are partially blocked from specific pages to be blocked from moving and deleting all pages. Bug: T208862 Change-Id: I6312a36911e5b73d773452fefef7ff25b9af08a4 --- includes/Title.php | 30 ++++++++--- .../phpunit/includes/TitlePermissionTest.php | 53 +++++++++++++++++-- 2 files changed, 71 insertions(+), 12 deletions(-) diff --git a/includes/Title.php b/includes/Title.php index 997063b3d1..c151f4a7e5 100644 --- a/includes/Title.php +++ b/includes/Title.php @@ -2688,14 +2688,30 @@ class Title implements LinkTarget { } $useReplica = ( $rigor !== 'secure' ); - if ( ( $action == 'edit' || $action == 'create' ) - && !$user->isBlockedFrom( $this, $useReplica ) - ) { - // Don't block the user from editing their own talk page unless they've been - // explicitly blocked from that too. - } elseif ( $user->isBlocked() && $user->getBlock()->prevents( $action ) !== false ) { + $block = $user->getBlock( $useReplica ); + + // The block may explicitly allow an action (like "read" or "upload"). + if ( $block && $block->prevents( $action ) === false ) { + return $errors; + } + + // Determine if the user is blocked from this action on this page. + try { // @todo FIXME: Pass the relevant context into this function. - $errors[] = $user->getBlock()->getPermissionsError( RequestContext::getMain() ); + $action = Action::factory( $action, WikiPage::factory( $this ), RequestContext::getMain() ); + } catch ( Exception $e ) { + $action = null; + } + + // If no action object is returned, assume that the action requires unblock + // which is the default. + if ( !$action || $action->requiresUnblock() ) { + if ( $user->isBlockedFrom( $this, $useReplica ) ) { + // @todo FIXME: Pass the relevant context into this function. + $errors[] = $block + ? $block->getPermissionsError( RequestContext::getMain() ) + : [ 'badaccess-group0' ]; + } } return $errors; diff --git a/tests/phpunit/includes/TitlePermissionTest.php b/tests/phpunit/includes/TitlePermissionTest.php index 11b9c012f8..cb5e1f8a79 100644 --- a/tests/phpunit/includes/TitlePermissionTest.php +++ b/tests/phpunit/includes/TitlePermissionTest.php @@ -1,5 +1,6 @@ true, ] ); - $this->setUserPerm( [ "createpage", "move" ] ); + $this->setUserPerm( [ 'createpage', 'edit', 'move', 'rollback', 'patrol', 'upload', 'purge' ] ); $this->setTitle( NS_HELP, "test page" ); # $wgEmailConfirmToEdit only applies to 'edit' action @@ -964,11 +965,24 @@ class TitlePermissionTest extends MediaWikiLangTestCase { 'expiry' => 10, 'systemBlock' => 'test', ] ); - $this->assertEquals( [ [ 'systemblockedtext', + + $errors = [ [ 'systemblockedtext', '[[User:Useruser|Useruser]]', 'no reason given', '127.0.0.1', 'Useruser', 'test', '23:00, 31 December 1969', '127.0.8.1', - $wgLang->timeanddate( wfTimestamp( TS_MW, $now ), true ) ] ], + $wgLang->timeanddate( wfTimestamp( TS_MW, $now ), true ) ] ]; + + $this->assertEquals( $errors, + $this->title->getUserPermissionsErrors( 'edit', $this->user ) ); + $this->assertEquals( $errors, $this->title->getUserPermissionsErrors( 'move-target', $this->user ) ); + $this->assertEquals( $errors, + $this->title->getUserPermissionsErrors( 'rollback', $this->user ) ); + $this->assertEquals( $errors, + $this->title->getUserPermissionsErrors( 'patrol', $this->user ) ); + $this->assertEquals( $errors, + $this->title->getUserPermissionsErrors( 'upload', $this->user ) ); + $this->assertEquals( [], + $this->title->getUserPermissionsErrors( 'purge', $this->user ) ); // partial block message test $this->user->mBlockedby = $this->user->getName(); @@ -981,10 +995,39 @@ class TitlePermissionTest extends MediaWikiLangTestCase { 'expiry' => 10, ] ); - $this->assertEquals( [ [ 'blockedtext-partial', + $this->assertEquals( [], + $this->title->getUserPermissionsErrors( 'edit', $this->user ) ); + $this->assertEquals( [], + $this->title->getUserPermissionsErrors( 'move-target', $this->user ) ); + $this->assertEquals( [], + $this->title->getUserPermissionsErrors( 'rollback', $this->user ) ); + $this->assertEquals( [], + $this->title->getUserPermissionsErrors( 'patrol', $this->user ) ); + $this->assertEquals( [], + $this->title->getUserPermissionsErrors( 'upload', $this->user ) ); + $this->assertEquals( [], + $this->title->getUserPermissionsErrors( 'purge', $this->user ) ); + + $this->user->mBlock->setRestrictions( [ + ( new PageRestriction( 0, $this->title->getArticleID() ) )->setTitle( $this->title ), + ] ); + + $errors = [ [ 'blockedtext-partial', '[[User:Useruser|Useruser]]', 'no reason given', '127.0.0.1', 'Useruser', null, '23:00, 31 December 1969', '127.0.8.1', - $wgLang->timeanddate( wfTimestamp( TS_MW, $now ), true ) ] ], + $wgLang->timeanddate( wfTimestamp( TS_MW, $now ), true ) ] ]; + + $this->assertEquals( $errors, + $this->title->getUserPermissionsErrors( 'edit', $this->user ) ); + $this->assertEquals( $errors, $this->title->getUserPermissionsErrors( 'move-target', $this->user ) ); + $this->assertEquals( $errors, + $this->title->getUserPermissionsErrors( 'rollback', $this->user ) ); + $this->assertEquals( $errors, + $this->title->getUserPermissionsErrors( 'patrol', $this->user ) ); + $this->assertEquals( [], + $this->title->getUserPermissionsErrors( 'upload', $this->user ) ); + $this->assertEquals( [], + $this->title->getUserPermissionsErrors( 'purge', $this->user ) ); } } -- 2.20.1