Deprecate User::isBlocked()

The method User::isBlocked() attempts to answer two questions:
(1) Does the user have a block?
(2) Is the user prevented from performing this action?
The method can answer #1, but it cannot answer #2. Since User::getBlock() can
also answer #1, this method is redundant. The method cannot answer #2 because
there is not enough context in order to answer that question.

If access is being checked against a Title object, all access checks can be
performed with PermissionManager:userCan() which will also check the user's
blocks.

If performing all access checks is not desirable, using
PermissionManager::isBlockedFrom() is also acceptable for only checking if the
user is blocked. This method does *not* determine if the action is allowed,
only that the user's block applies to that Title.

If access is being checked without an existing Title, User::getBlock() can be
used to get the user's block. Then Block::appliesToRight() can be used to
determine if the block applies explicitly to a right (or returns null if
it is unknown or false if explicitly allowed). If the user is creating a new
Title, but the text of the title is not yet known (as in the case of Wikibase),
access should be checked with Block::appliesToNamespace().

Bug: T209004
Change-Id: Ic0ad1b92e957797fee8dcd00bd1092fe69fa58f1

RELEASE-NOTES-1.34

@@ -110,6 +110,9 @@ because of Phabricator reports.
 * The MWNamespace class is deprecated. Use MediaWikiServices::getNamespaceInfo.
 * ExtensionRegistry->load() is deprecated, as it breaks dependency checking.
   Instead, use ->queue().
+* User::isBlocked() is deprecated since it does not tell you if the user is
+  blocked from editing a particular page. Use User::getBlock() or
+  PermissionManager::isBlockedFrom() or PermissionManager::userCan() instead.
 * …
 
 === Other changes in 1.34 ===

includes/Autopromote.php

@@ -198,7 +198,8 @@ class Autopromote {
 			case APCOND_IPINRANGE:
 				return IP::isInRange( $user->getRequest()->getIP(), $cond[1] );
 			case APCOND_BLOCKED:
-				return $user->isBlocked();
+				// @TODO Should partial blocks prevent auto promote?
+				return (bool)$user->getBlock();
 			case APCOND_ISBOT:
 				return in_array( 'bot', User::getGroupPermissions( $user->getGroups() ) );
 			default:

includes/api/ApiBlock.php

@@ -43,13 +43,14 @@ class ApiBlock extends ApiBase {
 		$this->requireOnlyOneParameter( $params, 'user', 'userid' );
 
 		# T17810: blocked admins should have limited access here
-		if ( $user->isBlocked() ) {
+		$block = $user->getBlock();
+		if ( $block ) {
 			$status = SpecialBlock::checkUnblockSelf( $params['user'], $user );
 			if ( $status !== true ) {
 				$this->dieWithError(
 					$status,
 					null,
-					[ 'blockinfo' => ApiQueryUserInfo::getBlockInfo( $user->getBlock() ) ]
+					[ 'blockinfo' => ApiQueryUserInfo::getBlockInfo( $block ) ]
 				);
 			}
 		}

includes/api/ApiQueryUserInfo.php

@@ -126,8 +126,11 @@ class ApiQueryUserInfo extends ApiQueryBase {
 			$vals['anon'] = true;
 		}
 
-		if ( isset( $this->prop['blockinfo'] ) && $user->isBlocked() ) {
-			$vals = array_merge( $vals, self::getBlockInfo( $user->getBlock() ) );
+		if ( isset( $this->prop['blockinfo'] ) ) {
+			$block = $user->getBlock();
+			if ( $block ) {
+				$vals = array_merge( $vals, self::getBlockInfo( $block ) );
+			}
 		}
 
 		if ( isset( $this->prop['hasmsg'] ) ) {

includes/api/ApiRevisionDelete.php

@@ -38,8 +38,10 @@ class ApiRevisionDelete extends ApiBase {
 		$user = $this->getUser();
 		$this->checkUserRightsAny( RevisionDeleter::getRestriction( $params['type'] ) );
 
-		if ( $user->isBlocked() ) {
-			$this->dieBlocked( $user->getBlock() );
+		// @TODO Use PermissionManager::isBlockedFrom() instead.
+		$block = $user->getBlock();
+		if ( $block ) {
+			$this->dieBlocked( $block );
 		}
 
 		if ( !$params['ids'] ) {

includes/api/ApiTag.php

@@ -40,8 +40,10 @@ class ApiTag extends ApiBase {
 		// make sure the user is allowed
 		$this->checkUserRightsAny( 'changetags' );
 
-		if ( $user->isBlocked() ) {
-			$this->dieBlocked( $user->getBlock() );
+		// @TODO Use PermissionManager::isBlockedFrom() instead.
+		$block = $user->getBlock();
+		if ( $block ) {
+			$this->dieBlocked( $block );
 		}
 
 		// Check if user can add tags

includes/api/ApiUnblock.php

@@ -41,13 +41,14 @@ class ApiUnblock extends ApiBase {
 			$this->dieWithError( 'apierror-permissiondenied-unblock', 'permissiondenied' );
 		}
 		# T17810: blocked admins should have limited access here
-		if ( $user->isBlocked() ) {
+		$block = $user->getBlock();
+		if ( $block ) {
 			$status = SpecialBlock::checkUnblockSelf( $params['user'], $user );
 			if ( $status !== true ) {
 				$this->dieWithError(
 					$status,
 					null,
-					[ 'blockinfo' => ApiQueryUserInfo::getBlockInfo( $user->getBlock() ) ]
+					[ 'blockinfo' => ApiQueryUserInfo::getBlockInfo( $block ) ]
 				);
 			}
 		}

includes/api/ApiUserrights.php

@@ -51,8 +51,13 @@ class ApiUserrights extends ApiBase {
 
 		// Deny if the user is blocked and doesn't have the full 'userrights' permission.
 		// This matches what Special:UserRights does for the web UI.
-		if ( $pUser->isBlocked() && !$pUser->isAllowed( 'userrights' ) ) {
-			$this->dieBlocked( $pUser->getBlock() );
+		if ( !$pUser->isAllowed( 'userrights' ) ) {
+			// @TODO Should the user be blocked from changing user rights if they
+			//       are partially blocked?
+			$block = $pUser->getBlock();
+			if ( $block ) {
+				$this->dieBlocked( $block );
+			}
 		}
 
 		$params = $this->extractRequestParams();

includes/auth/CheckBlocksSecondaryAuthenticationProvider.php

@@ -59,9 +59,11 @@ class CheckBlocksSecondaryAuthenticationProvider extends AbstractSecondaryAuthen
 	}
 
 	public function beginSecondaryAuthentication( $user, array $reqs ) {
+		// @TODO Partial blocks should not prevent the user from logging in.
+		//       see: https://phabricator.wikimedia.org/T208895
 		if ( !$this->blockDisablesLogin ) {
 			return AuthenticationResponse::newAbstain();
-		} elseif ( $user->isBlocked() ) {
+		} elseif ( $user->getBlock() ) {
 			return AuthenticationResponse::newFail(
 				new \Message( 'login-userblocked', [ $user->getName() ] )
 			);

includes/changetags/ChangeTags.php

@@ -482,7 +482,9 @@ class ChangeTags {
 		if ( !is_null( $user ) ) {
 			if ( !$user->isAllowed( 'applychangetags' ) ) {
 				return Status::newFatal( 'tags-apply-no-permission' );
-			} elseif ( $user->isBlocked() ) {
+			} elseif ( $user->getBlock() ) {
+				// @TODO Ensure that the block does not apply to the `applychangetags`
+				//       right.
 				return Status::newFatal( 'tags-apply-blocked', $user->getName() );
 			}
 		}
@@ -555,7 +557,9 @@ class ChangeTags {
 		if ( !is_null( $user ) ) {
 			if ( !$user->isAllowed( 'changetags' ) ) {
 				return Status::newFatal( 'tags-update-no-permission' );
-			} elseif ( $user->isBlocked() ) {
+			} elseif ( $user->getBlock() ) {
+				// @TODO Ensure that the block does not apply to the `changetags`
+				//       right.
 				return Status::newFatal( 'tags-update-blocked', $user->getName() );
 			}
 		}
@@ -973,7 +977,9 @@ class ChangeTags {
 		if ( !is_null( $user ) ) {
 			if ( !$user->isAllowed( 'managechangetags' ) ) {
 				return Status::newFatal( 'tags-manage-no-permission' );
-			} elseif ( $user->isBlocked() ) {
+			} elseif ( $user->getBlock() ) {
+				// @TODO Ensure that the block does not apply to the `managechangetags`
+				//       right.
 				return Status::newFatal( 'tags-manage-blocked', $user->getName() );
 			}
 		}
@@ -1045,7 +1051,9 @@ class ChangeTags {
 		if ( !is_null( $user ) ) {
 			if ( !$user->isAllowed( 'managechangetags' ) ) {
 				return Status::newFatal( 'tags-manage-no-permission' );
-			} elseif ( $user->isBlocked() ) {
+			} elseif ( $user->getBlock() ) {
+				// @TODO Ensure that the block does not apply to the `managechangetags`
+				//       right.
 				return Status::newFatal( 'tags-manage-blocked', $user->getName() );
 			}
 		}
@@ -1142,7 +1150,9 @@ class ChangeTags {
 		if ( !is_null( $user ) ) {
 			if ( !$user->isAllowed( 'managechangetags' ) ) {
 				return Status::newFatal( 'tags-manage-no-permission' );
-			} elseif ( $user->isBlocked() ) {
+			} elseif ( $user->getBlock() ) {
+				// @TODO Ensure that the block does not apply to the `managechangetags`
+				//       right.
 				return Status::newFatal( 'tags-manage-blocked', $user->getName() );
 			}
 		}
@@ -1258,7 +1268,9 @@ class ChangeTags {
 		if ( !is_null( $user ) ) {
 			if ( !$user->isAllowed( 'deletechangetags' ) ) {
 				return Status::newFatal( 'tags-delete-no-permission' );
-			} elseif ( $user->isBlocked() ) {
+			} elseif ( $user->getBlock() ) {
+				// @TODO Ensure that the block does not apply to the `deletechangetags`
+				//       right.
 				return Status::newFatal( 'tags-manage-blocked', $user->getName() );
 			}
 		}

includes/mail/EmailNotification.php

@@ -224,7 +224,9 @@ class EmailNotification {
 						&& $watchingUser->isEmailConfirmed()
 						&& $watchingUser->getId() != $userTalkId
 						&& !in_array( $watchingUser->getName(), $wgUsersNotifiedOnAllChanges )
-						&& !( $wgBlockDisablesLogin && $watchingUser->isBlocked() )
+						// @TODO Partial blocks should not prevent the user from logging in.
+						//       see: https://phabricator.wikimedia.org/T208895
+						&& !( $wgBlockDisablesLogin && $watchingUser->getBlock() )
 						&& Hooks::run( 'SendWatchlistEmailNotification', [ $watchingUser, $title, $this ] )
 					) {
 						$this->compose( $watchingUser, self::WATCHLIST );
@@ -262,7 +264,9 @@ class EmailNotification {
 				wfDebug( __METHOD__ . ": user talk page edited, but user does not exist\n" );
 			} elseif ( $targetUser->getId() == $editor->getId() ) {
 				wfDebug( __METHOD__ . ": user edited their own talk page, no notification sent\n" );
-			} elseif ( $wgBlockDisablesLogin && $targetUser->isBlocked() ) {
+			} elseif ( $wgBlockDisablesLogin && $targetUser->getBlock() ) {
+				// @TODO Partial blocks should not prevent the user from logging in.
+				//       see: https://phabricator.wikimedia.org/T208895
 				wfDebug( __METHOD__ . ": talk page owner is blocked and cannot login, no notification sent\n" );
 			} elseif ( $targetUser->getOption( 'enotifusertalkpages' )
 				&& ( !$minorEdit || $targetUser->getOption( 'enotifminoredits' ) )

includes/specials/SpecialBlock.php

@@ -1125,9 +1125,11 @@ class SpecialBlock extends FormSpecialPage {
 		} elseif ( is_string( $target ) ) {
 			$target = User::newFromName( $target );
 		}
-		if ( $performer->isBlocked() ) {
+		if ( $performer->getBlock() ) {
 			if ( $target instanceof User && $target->getId() == $performer->getId() ) {
 				# User is trying to unblock themselves
+				// @TODO Ensure that the block does not apply to the `unblockself`
+				//       right.
 				if ( $performer->isAllowed( 'unblockself' ) ) {
 					return true;
 					# User blocked themselves and is now trying to reverse it

includes/specials/SpecialContributions.php

@@ -385,7 +385,7 @@ class SpecialContributions extends IncludableSpecialPage {
 
 		if ( ( $id !== null ) || ( $id === null && IP::isIPAddress( $username ) ) ) {
 			if ( $sp->getUser()->isAllowed( 'block' ) ) { # Block / Change block / Unblock links
-				if ( $target->isBlocked() && $target->getBlock()->getType() != Block::TYPE_AUTO ) {
+				if ( $target->getBlock() && $target->getBlock()->getType() != Block::TYPE_AUTO ) {
 					$tools['block'] = $linkRenderer->makeKnownLink( # Change block link
 						SpecialPage::getTitleFor( 'Block', $username ),
 						$sp->msg( 'change-blocklink' )->text()

includes/specials/SpecialEditTags.php

@@ -68,8 +68,10 @@ class SpecialEditTags extends UnlistedSpecialPage {
 		$request = $this->getRequest();
 
 		// Check blocks
-		if ( $user->isBlocked() ) {
-			throw new UserBlockedError( $user->getBlock() );
+		// @TODO Use PermissionManager::isBlockedFrom() instead.
+		$block = $user->getBlock();
+		if ( $block ) {
+			throw new UserBlockedError( $block );
 		}
 
 		$this->setHeaders();

includes/specials/SpecialRevisionDelete.php

@@ -123,8 +123,10 @@ class SpecialRevisionDelete extends UnlistedSpecialPage {
 		$user = $this->getUser();
 
 		// Check blocks
-		if ( $user->isBlocked() ) {
-			throw new UserBlockedError( $user->getBlock() );
+		// @TODO Use PermissionManager::isBlockedFrom() instead.
+		$block = $user->getBlock();
+		if ( $block ) {
+			throw new UserBlockedError( $block );
 		}
 
 		$this->setHeaders();

includes/specials/SpecialUserrights.php

@@ -152,8 +152,13 @@ class UserrightsPage extends SpecialPage {
 			* (e.g. they don't have the userrights permission), then don't
 			* allow them to change any user rights.
 			*/
-			if ( $user->isBlocked() && !$user->isAllowed( 'userrights' ) ) {
-				throw new UserBlockedError( $user->getBlock() );
+			if ( !$user->isAllowed( 'userrights' ) ) {
+				// @TODO Should the user be blocked from changing user rights if they
+				//       are partially blocked?
+				$block = $user->getBlock();
+				if ( $block ) {
+					throw new UserBlockedError( $user->getBlock() );
+				}
 			}
 
 			$this->checkReadOnly();

includes/user/User.php

@@ -1372,7 +1372,7 @@ class User implements IDBAccessObject, UserIdentity {
 		$user = $session->getUser();
 		if ( $user->isLoggedIn() ) {
 			$this->loadFromUserObject( $user );
-			if ( $user->isBlocked() ) {
+			if ( $user->getBlock() ) {
 				// If this user is autoblocked, set a cookie to track the Block. This has to be done on
 				// every session load, because an autoblocked editor might not edit again from the same
 				// IP address after being blocked.
@@ -2262,6 +2262,10 @@ class User implements IDBAccessObject, UserIdentity {
 	/**
 	 * Check if user is blocked
 	 *
+	 * @deprecated since 1.34, use User::getBlock() or
+	 *             PermissionManager::isBlockedFrom() or
+	 *             PermissionManager::userCan() instead.
+	 *
 	 * @param bool $fromReplica Whether to check the replica DB instead of
 	 *   the master. Hacked from false due to horrible probs on site.
 	 * @return bool True if blocked, false otherwise
@@ -3563,10 +3567,12 @@ class User implements IDBAccessObject, UserIdentity {
 			// $user->isAllowed(). It is also checked in Title::checkUserBlock()
 			// to give a better error message in the common case.
 			$config = RequestContext::getMain()->getConfig();
+			// @TODO Partial blocks should not prevent the user from logging in.
+			//       see: https://phabricator.wikimedia.org/T208895
 			if (
 				$this->isLoggedIn() &&
 				$config->get( 'BlockDisablesLogin' ) &&
-				$this->isBlocked()
+				$this->getBlock()
 			) {
 				$anon = new User;
 				$this->mRights = array_intersect( $this->mRights, $anon->getRights() );
@@ -4456,7 +4462,7 @@ class User implements IDBAccessObject, UserIdentity {
 	 * @return bool A block was spread
 	 */
 	public function spreadAnyEditBlock() {
-		if ( $this->isLoggedIn() && $this->isBlocked() ) {
+		if ( $this->isLoggedIn() && $this->getBlock() ) {
 			return $this->spreadBlock();
 		}
 

maintenance/importImages.php

@@ -211,7 +211,8 @@ class ImportImages extends Maintenance {
 
 				if ( $checkUserBlock && ( ( $processed % $checkUserBlock ) == 0 ) ) {
 					$user->clearInstanceCache( 'name' ); // reload from DB!
-					if ( $user->isBlocked() ) {
+					// @TODO Use PermissionManager::isBlockedFrom() instead.
+					if ( $user->getBlock() ) {
 						$this->output( $user->getName() . " was blocked! Aborting.\n" );
 						break;
 					}

tests/phpunit/includes/user/UserTest.php

@@ -617,7 +617,7 @@ class UserTest extends MediaWikiTestCase {
 
 		// Confirm that the block has been applied as required.
 		$this->assertTrue( $user1->isLoggedIn() );
-		$this->assertTrue( $user1->isBlocked() );
+		$this->assertInstanceOf( Block::class, $user1->getBlock() );
 		$this->assertEquals( Block::TYPE_USER, $block->getType() );
 		$this->assertTrue( $block->isAutoblocking() );
 		$this->assertGreaterThanOrEqual( 1, $block->getId() );
@@ -638,7 +638,7 @@ class UserTest extends MediaWikiTestCase {
 		$this->assertNotEquals( $user1->getToken(), $user2->getToken() );
 		$this->assertTrue( $user2->isAnon() );
 		$this->assertFalse( $user2->isLoggedIn() );
-		$this->assertTrue( $user2->isBlocked() );
+		$this->assertInstanceOf( Block::class, $user2->getBlock() );
 		// Non-strict type-check.
 		$this->assertEquals( true, $user2->getBlock()->isAutoblocking(), 'Autoblock does not work' );
 		// Can't directly compare the objects because of member type differences.
@@ -654,7 +654,7 @@ class UserTest extends MediaWikiTestCase {
 		$user3 = User::newFromSession( $request3 );
 		$user3->load();
 		$this->assertTrue( $user3->isLoggedIn() );
-		$this->assertTrue( $user3->isBlocked() );
+		$this->assertInstanceOf( Block::class, $user3->getBlock() );
 		$this->assertEquals( true, $user3->getBlock()->isAutoblocking() ); // Non-strict type-check.
 
 		// Clean up.
@@ -694,7 +694,7 @@ class UserTest extends MediaWikiTestCase {
 
 		// 2. Test that the cookie IS NOT present.
 		$this->assertTrue( $user->isLoggedIn() );
-		$this->assertTrue( $user->isBlocked() );
+		$this->assertInstanceOf( Block::class, $user->getBlock() );
 		$this->assertEquals( Block::TYPE_USER, $block->getType() );
 		$this->assertTrue( $block->isAutoblocking() );
 		$this->assertGreaterThanOrEqual( 1, $user->getBlockId() );
@@ -739,7 +739,7 @@ class UserTest extends MediaWikiTestCase {
 
 		// 2. Test the cookie's expiry timestamp.
 		$this->assertTrue( $user1->isLoggedIn() );
-		$this->assertTrue( $user1->isBlocked() );
+		$this->assertInstanceOf( Block::class, $user1->getBlock() );
 		$this->assertEquals( Block::TYPE_USER, $block->getType() );
 		$this->assertTrue( $block->isAutoblocking() );
 		$this->assertGreaterThanOrEqual( 1, $user1->getBlockId() );
@@ -849,7 +849,7 @@ class UserTest extends MediaWikiTestCase {
 		$user2->load();
 		$this->assertTrue( $user2->isAnon() );
 		$this->assertFalse( $user2->isLoggedIn() );
-		$this->assertFalse( $user2->isBlocked() );
+		$this->assertNull( $user2->getBlock() );
 
 		// Clean up.
 		$block->delete();
@@ -885,7 +885,7 @@ class UserTest extends MediaWikiTestCase {
 		$user1 = User::newFromSession( $request1 );
 		$user1->mBlock = $block;
 		$user1->load();
-		$this->assertTrue( $user1->isBlocked() );
+		$this->assertInstanceOf( Block::class, $user1->getBlock() );
 
 		// 2. Create a new request, set the cookie to just the block ID, and the user should
 		// still get blocked when they log in again.
@@ -897,7 +897,7 @@ class UserTest extends MediaWikiTestCase {
 		$this->assertNotEquals( $user1->getToken(), $user2->getToken() );
 		$this->assertTrue( $user2->isAnon() );
 		$this->assertFalse( $user2->isLoggedIn() );
-		$this->assertTrue( $user2->isBlocked() );
+		$this->assertInstanceOf( Block::class, $user2->getBlock() );
 		$this->assertEquals( true, $user2->getBlock()->isAutoblocking() ); // Non-strict type-check.
 
 		// Clean up.
@@ -1459,7 +1459,7 @@ class UserTest extends MediaWikiTestCase {
 		$user = User::newFromSession( $request );
 
 		// logged in users should be inmune to cookie block of type ip/range
-		$this->assertFalse( $user->isBlocked() );
+		$this->assertNull( $user->getBlock() );
 
 		// cookie is being cleared
 		$cookies = $request->response()->getCookies();