Deprecate User::isBlocked()
authorDavid Barratt <dbarratt@wikimedia.org>
Tue, 23 Apr 2019 17:51:54 +0000 (13:51 -0400)
committerDavid Barratt <dbarratt@wikimedia.org>
Thu, 25 Apr 2019 15:47:44 +0000 (11:47 -0400)
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

19 files changed:
RELEASE-NOTES-1.34
includes/Autopromote.php
includes/api/ApiBlock.php
includes/api/ApiQueryUserInfo.php
includes/api/ApiRevisionDelete.php
includes/api/ApiTag.php
includes/api/ApiUnblock.php
includes/api/ApiUserrights.php
includes/auth/CheckBlocksSecondaryAuthenticationProvider.php
includes/changetags/ChangeTags.php
includes/mail/EmailNotification.php
includes/specials/SpecialBlock.php
includes/specials/SpecialContributions.php
includes/specials/SpecialEditTags.php
includes/specials/SpecialRevisionDelete.php
includes/specials/SpecialUserrights.php
includes/user/User.php
maintenance/importImages.php
tests/phpunit/includes/user/UserTest.php

index e779842..3340225 100644 (file)
@@ -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 ===
index a01465e..02c9d01 100644 (file)
@@ -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:
index 673fc6b..b5d51aa 100644 (file)
@@ -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 ) ]
                                );
                        }
                }
index 59e0524..00d7d84 100644 (file)
@@ -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'] ) ) {
index 6e37774..1ee91c2 100644 (file)
@@ -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'] ) {
index 82cf986..aff0183 100644 (file)
@@ -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
index b748cb3..3aad8f4 100644 (file)
@@ -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 ) ]
                                );
                        }
                }
index e251fe6..acb3da8 100644 (file)
@@ -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();
index 10925b5..3e26097 100644 (file)
@@ -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() ] )
                        );
index 2169a4d..0601397 100644 (file)
@@ -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() );
                        }
                }
index acf2c2e..0b77651 100644 (file)
@@ -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' ) )
index 7d86663..1eb1ad5 100644 (file)
@@ -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
index 08a7fde..99eefdd 100644 (file)
@@ -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()
index 5203807..ed398de 100644 (file)
@@ -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();
index dd6fea7..682bceb 100644 (file)
@@ -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();
index 540754f..d564e5b 100644 (file)
@@ -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();
index 33d216d..44673fc 100644 (file)
@@ -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();
                }
 
index 1728695..f27ea2f 100644 (file)
@@ -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;
                                        }
index f84be3f..e7bedc2 100644 (file)
@@ -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();