Merge "Allow partially blocked users to use Thanks"

extension.json

@@ -11,7 +11,7 @@
 	"license-name": "MIT",
 	"type": "other",
 	"requires": {
-		"MediaWiki": ">= 1.31.0",
+		"MediaWiki": ">= 1.34.0",
 		"extensions": {
 			"Echo": "*"
 		}

includes/ApiCoreThank.php

@@ -42,6 +42,8 @@ class ApiCoreThank extends ApiThank {
 				$type = 'rev';
 				$id = $logEntry->getAssociatedRevId();
 			} else {
+				// If there's no associated revision, die if the user is sitewide blocked
+				$this->dieOnSitewideBlockedUser( $user );
 				$excerpt = '';
 				$title = $logEntry->getTarget();
 				$recipient = $this->getUserFromLog( $logEntry );
@@ -52,6 +54,8 @@ class ApiCoreThank extends ApiThank {
 			$revision = $this->getRevisionFromId( $id );
 			$excerpt = EchoDiscussionParser::getEditExcerpt( $revision, $this->getLanguage() );
 			$title = $this->getTitleFromRevision( $revision );
+			$this->dieOnBlockedUser( $user, $title );
+
 			$recipient = $this->getUserFromRevision( $revision );
 			$recipientUsername = $revision->getUser()->getName();
 

includes/ApiFlowThank.php

@@ -47,6 +47,7 @@ class ApiFlowThank extends ApiThank {
 		// Truncate the title text to prevent issues with database storage.
 		$topicTitleText = $this->getLanguage()->truncateForDatabase( $rawTopicTitleText, 200 );
 		$pageTitle = $this->getPageTitleFromRootPost( $rootPost );
+		$this->dieOnBlockedUser( $user, $pageTitle );
 
 		/** @var PostRevision $post */
 		$post = $data['post'];

includes/ApiThank.php

@@ -1,5 +1,7 @@
 <?php
 
+use MediaWiki\MediaWikiServices;
+
 /**
  * Base API module for Thanks
  *
@@ -12,13 +14,43 @@ abstract class ApiThank extends ApiBase {
 			$this->dieWithError( 'thanks-error-notloggedin', 'notloggedin' );
 		} elseif ( $user->pingLimiter( 'thanks-notification' ) ) {
 			$this->dieWithError( [ 'thanks-error-ratelimited', $user->getName() ], 'ratelimited' );
-		} elseif ( $user->isBlocked() ) {
-			$this->dieBlocked( $user->getBlock() );
 		} elseif ( $user->isBlockedGlobally() ) {
 			$this->dieBlocked( $user->getGlobalBlock() );
 		}
 	}
 
+	/**
+	 * Check whether the user is blocked from this title. (This is not the same
+	 * as checking whether they are sitewide blocked, because a sitewide blocked
+	 * user may still be allowed to thank on their own talk page.)
+	 *
+	 * This is separate from dieOnBadUser because we need to know the title.
+	 *
+	 * @param User $user
+	 * @param Title $title
+	 */
+	protected function dieOnBlockedUser( User $user, Title $title ) {
+		$permissionManager = MediaWikiServices::getInstance()->getPermissionManager();
+		if ( $permissionManager->isBlockedFrom( $user, $title ) ) {
+			$this->dieBlocked( $user->getBlock() );
+		}
+	}
+
+	/**
+	 * Check whether the user is sitewide blocked.
+	 *
+	 * This is separate from dieOnBlockedUser because we need to know if the thank
+	 * is related to a revision. (If it is, then use dieOnBlockedUser instead.)
+	 *
+	 * @param User $user
+	 */
+	protected function dieOnSitewideBlockedUser( User $user ) {
+		$block = $user->getBlock();
+		if ( $block && $block->isSitewide() ) {
+			$this->dieBlocked( $block );
+		}
+	}
+
 	protected function dieOnBadRecipient( User $user, User $recipient ) {
 		global $wgThanksSendToBots;
 

includes/ThanksHooks.php

@@ -63,7 +63,7 @@ class ThanksHooks {
 		// prevents thanking diff across multiple revisions)
 		if ( !$user->isAnon()
 			&& $recipientId !== $user->getId()
-			&& !$user->isBlocked()
+			&& !self::isUserBlockedFromTitle( $user, $rev->getTitle() )
 			&& !$user->isBlockedGlobally()
 			&& self::canReceiveThanks( $recipient )
 			&& !$rev->isDeleted( Revision::DELETED_TEXT )
@@ -73,6 +73,18 @@ class ThanksHooks {
 		}
 	}
 
+	/**
+	 * Check whether the user is blocked from the title associated with the revision.
+	 *
+	 * @param User $user
+	 * @param Title $title
+	 * @return bool
+	 */
+	private static function isUserBlockedFromTitle( User $user, Title $title ) {
+		return MediaWikiServices::getInstance()->getPermissionManager()
+			->isBlockedFrom( $user, $title );
+	}
+
 	/**
 	 * Check whether a user is allowed to receive thanks or not
 	 *
@@ -372,7 +384,11 @@ class ThanksHooks {
 		global $wgUser;
 
 		// Don't thank if anonymous or blocked
-		if ( $wgUser->isAnon() || $wgUser->isBlocked() || $wgUser->isBlockedGlobally() ) {
+		if (
+			$wgUser->isAnon()
+			|| self::isUserBlockedFromTitle( $wgUser, $entry->getTarget() )
+			|| $wgUser->isBlockedGlobally()
+		) {
 			return;
 		}
 

tests/phpunit/ApiCoreThankUnitTest.php

@@ -16,13 +16,25 @@ class ApiCoreThankUnitTest extends MediaWikiTestCase {
 		return new ApiCoreThank( new ApiMain(), self::$moduleName );
 	}
 
+	private function createBlock( $options ) {
+		$options = array_merge( [
+			'address' => 'Test user',
+			'by' => 1,
+			'reason' => __METHOD__,
+			'timestamp' => wfTimestamp( TS_MW ),
+			'expiry' => 'infinity',
+		], $options );
+		return new Block( $options );
+	}
+
 	/**
 	 * @dataProvider provideDieOnBadUser
 	 * @covers ApiThank::dieOnBadUser
+	 * @covers ApiThank::dieOnSitewideBlockedUser
 	 */
-	public function testDieOnBadUser( $user, $expectedError ) {
+	public function testDieOnBadUser( $user, $dieMethod, $expectedError ) {
 		$module = $this->getModule();
-		$method = new ReflectionMethod( $module, 'dieOnBadUser' );
+		$method = new ReflectionMethod( $module, $dieMethod );
 		$method->setAccessible( true );
 
 		if ( $expectedError ) {
@@ -42,7 +54,11 @@ class ApiCoreThankUnitTest extends MediaWikiTestCase {
 			->method( 'isAnon' )
 			->will( $this->returnValue( true ) );
 
-		$testCases[ 'anon' ] = [ $mockUser, 'Anonymous users cannot send thanks' ];
+		$testCases[ 'anon' ] = [
+			$mockUser,
+			'dieOnBadUser',
+			'Anonymous users cannot send thanks'
+		];
 
 		$mockUser = $this->getMock( 'User' );
 		$mockUser->expects( $this->once() )
@@ -54,6 +70,7 @@ class ApiCoreThankUnitTest extends MediaWikiTestCase {
 
 		$testCases[ 'ping' ] = [
 			$mockUser,
+			'dieOnBadUser',
 			"You've exceeded your rate limit. Please wait some time and try again"
 		];
 
@@ -65,20 +82,47 @@ class ApiCoreThankUnitTest extends MediaWikiTestCase {
 			->method( 'pingLimiter' )
 			->will( $this->returnValue( false ) );
 		$mockUser->expects( $this->once() )
-			->method( 'isBlocked' )
+			->method( 'isBlockedGlobally' )
 			->will( $this->returnValue( true ) );
 		$mockUser->expects( $this->once() )
-			->method( 'getBlock' )
-			->will( $this->returnValue( new Block( [
-				'address' => 'Test user',
-				'by' => 1,
-				'byText' => 'UTSysop',
-				'reason' => __METHOD__,
-				'timestamp' => wfTimestamp( TS_MW ),
-				'expiry' => 'infinity',
-			] ) ) );
+			->method( 'getGlobalBlock' )
+			->will( $this->returnValue(
+				$this->createBlock( [] )
+			) );
 
-		$testCases[ 'blocked' ] = [ $mockUser, 'You have been blocked from editing' ];
+		$testCases[ 'globally blocked' ] = [
+			$mockUser,
+			'dieOnBadUser',
+			'You have been blocked from editing'
+		];
+
+		$mockUser = $this->getMock( 'User' );
+		$mockUser->expects( $this->once() )
+			->method( 'getBlock' )
+			->will( $this->returnValue(
+				$this->createBlock( [] )
+			) );
+
+		$testCases[ 'sitewide blocked' ] = [
+			$mockUser,
+			'dieOnSitewideBlockedUser',
+			'You have been blocked from editing'
+		];
+
+		$mockUser = $this->getMock( 'User' );
+		$mockUser->expects( $this->once() )
+			->method( 'getBlock' )
+			->will( $this->returnValue(
+				$this->createBlock( [
+					'sitewide' => false
+				] )
+			) );
+
+		$testCases[ 'partial blocked' ] = [
+			$mockUser,
+			'dieOnSitewideBlockedUser',
+			false
+		];
 
 		return $testCases;
 	}