Action::checkCanExecute should only block an Action if the user is sitewide blocked

The method over-enforces partial blocks by preventing users from performing
the action on unrelated pages.

Bug: T209284
Change-Id: I4ee0e7c0188d491cf8fc0bbbbf7e492cdf309f45

diff --git a/includes/actions/Action.php b/includes/actions/Action.php
index e5233f0..f288a5c 100644 (file)
--- a/includes/actions/Action.php
+++ b/includes/actions/Action.php
@@ -314,9 +314,14 @@ abstract class Action implements MessageLocalizer {
                        }
                }
 
-               if ( $this->requiresUnblock() && $user->isBlocked() ) {
+               // If the action requires an unblock, explicitly check the user's block.
+               if ( $this->requiresUnblock() && $user->isBlockedFrom( $this->getTitle() ) ) {
                        $block = $user->getBlock();
-                       throw new UserBlockedError( $block );
+                       if ( $block ) {
+                               throw new UserBlockedError( $block );
+                       }
+
+                       throw new PermissionsError( $this->getName(), [ 'badaccess-group0' ] );
                }
 
                // This should be checked at the end so that the user won't think the

diff --git a/tests/phpunit/includes/actions/ActionTest.php b/tests/phpunit/includes/actions/ActionTest.php
index 9c8b957..d80d627 100644 (file)
--- a/tests/phpunit/includes/actions/ActionTest.php
+++ b/tests/phpunit/includes/actions/ActionTest.php
@@ -1,5 +1,7 @@
 <?php
 
+use MediaWiki\Block\Restriction\PageRestriction;
+
 /**
  * @covers Action
  *
@@ -22,6 +24,8 @@ class ActionTest extends MediaWikiTestCase {
                        'edit' => true,
                        'revisiondelete' => SpecialPageAction::class,
                        'dummy' => true,
+                       'access' => 'ControlledAccessDummyAction',
+                       'unblock' => 'RequiresUnblockDummyAction',
                        'string' => 'NamedDummyAction',
                        'declared' => 'NonExistingClassName',
                        'callable' => [ $this, 'dummyActionCallback' ],
@@ -183,6 +187,53 @@ class ActionTest extends MediaWikiTestCase {
                return new CalledDummyAction( $context->getWikiPage(), $context );
        }
 
+       public function testCanExecute() {
+               $user = $this->getTestUser()->getUser();
+               $user->mRights = [ 'access' ];
+               $action = Action::factory( 'access', $this->getPage(), $this->getContext() );
+               $this->assertNull( $action->canExecute( $user ) );
+       }
+
+       public function testCanExecuteNoRight() {
+               $user = $this->getTestUser()->getUser();
+               $user->mRights = [];
+               $action = Action::factory( 'access', $this->getPage(), $this->getContext() );
+
+               try {
+                       $action->canExecute( $user );
+               } catch ( Exception $e ) {
+                       $this->assertInstanceOf( PermissionsError::class, $e );
+               }
+       }
+
+       public function testCanExecuteRequiresUnblock() {
+               $user = $this->getTestUser()->getUser();
+               $user->mRights = [];
+
+               $page = $this->getExistingTestPage();
+               $action = Action::factory( 'unblock', $page, $this->getContext() );
+
+               $block = new Block( [
+                       'address' => $user,
+                       'by' => $this->getTestSysop()->getUser()->getId(),
+                       'expiry' => 'infinity',
+                       'sitewide' => false,
+               ] );
+               $block->setRestrictions( [
+                       new PageRestriction( 0, $page->getTitle()->getArticleID() ),
+               ] );
+
+               $block->insert();
+
+               try {
+                       $action->canExecute( $user );
+               } catch ( Exception $e ) {
+                       $this->assertInstanceOf( UserBlockedError::class, $e );
+               }
+
+               $block->delete();
+       }
+
 }
 
 class DummyAction extends Action {
@@ -196,6 +247,10 @@ class DummyAction extends Action {
 
        public function execute() {
        }
+
+       public function canExecute( User $user ) {
+               return $this->checkCanExecute( $user );
+       }
 }
 
 class NamedDummyAction extends DummyAction {
@@ -206,3 +261,15 @@ class CalledDummyAction extends DummyAction {
 
 class InstantiatedDummyAction extends DummyAction {
 }
+
+class ControlledAccessDummyAction extends DummyAction {
+       public function getRestriction() {
+               return 'access';
+       }
+}
+
+class RequiresUnblockDummyAction extends DummyAction {
+       public function requiresUnblock() {
+               return true;
+       }
+}