From: David Barratt <dbarratt@wikimedia.org>
Date: Mon, 12 Nov 2018 15:13:18 +0000 (-0500)
Subject: Action::checkCanExecute should only block an Action if the user is sitewide blocked
X-Git-Tag: 1.34.0-rc.0~3475^2
X-Git-Url: https://git.heureux-cyclage.org/?a=commitdiff_plain;h=f205e6e9f0b628341711804c8a8a2c34aba9ebc6;p=lhc%2Fweb%2Fwiklou.git

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 e5233f0607..f288a5cb1f 100644
--- 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 9c8b957b27..d80d627b4f 100644
--- 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;
+	}
+}