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

diff --git a/includes/Title.php b/includes/Title.php
index 997063b..c151f4a 100644 (file)
--- 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 11b9c01..cb5e1f8 100644 (file)
--- a/tests/phpunit/includes/TitlePermissionTest.php
+++ b/tests/phpunit/includes/TitlePermissionTest.php
@@ -1,5 +1,6 @@
 <?php
 
+use MediaWiki\Block\Restriction\PageRestriction;
 use MediaWiki\MediaWikiServices;
 
 /**
@@ -892,7 +893,7 @@ class TitlePermissionTest extends MediaWikiLangTestCase {
                        'wgEmailAuthentication' => 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 ) );
        }
 }