From: David Barratt Date: Tue, 23 Apr 2019 17:51:54 +0000 (-0400) Subject: Deprecate User::isBlocked() X-Git-Tag: 1.34.0-rc.0~1854^2 X-Git-Url: http://git.cyclocoop.org/%22%20.%20generer_url_ecrire%28%22sites_tous%22%29%20.%20%22?a=commitdiff_plain;h=e86a060284e8ade65ca1e358b0b449fd0c0d9559;p=lhc%2Fweb%2Fwiklou.git 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 --- diff --git a/RELEASE-NOTES-1.34 b/RELEASE-NOTES-1.34 index e779842519..3340225628 100644 --- a/RELEASE-NOTES-1.34 +++ b/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 === diff --git a/includes/Autopromote.php b/includes/Autopromote.php index a01465e9ae..02c9d019d5 100644 --- a/includes/Autopromote.php +++ b/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: diff --git a/includes/api/ApiBlock.php b/includes/api/ApiBlock.php index 673fc6b00c..b5d51aae8c 100644 --- a/includes/api/ApiBlock.php +++ b/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 ) ] ); } } diff --git a/includes/api/ApiQueryUserInfo.php b/includes/api/ApiQueryUserInfo.php index 59e0524ad9..00d7d84de8 100644 --- a/includes/api/ApiQueryUserInfo.php +++ b/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'] ) ) { diff --git a/includes/api/ApiRevisionDelete.php b/includes/api/ApiRevisionDelete.php index 6e37774266..1ee91c21f0 100644 --- a/includes/api/ApiRevisionDelete.php +++ b/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'] ) { diff --git a/includes/api/ApiTag.php b/includes/api/ApiTag.php index 82cf986b4e..aff01830e0 100644 --- a/includes/api/ApiTag.php +++ b/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 diff --git a/includes/api/ApiUnblock.php b/includes/api/ApiUnblock.php index b748cb3288..3aad8f4199 100644 --- a/includes/api/ApiUnblock.php +++ b/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 ) ] ); } } diff --git a/includes/api/ApiUserrights.php b/includes/api/ApiUserrights.php index e251fe6943..acb3da8fb1 100644 --- a/includes/api/ApiUserrights.php +++ b/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(); diff --git a/includes/auth/CheckBlocksSecondaryAuthenticationProvider.php b/includes/auth/CheckBlocksSecondaryAuthenticationProvider.php index 10925b50f7..3e260974b6 100644 --- a/includes/auth/CheckBlocksSecondaryAuthenticationProvider.php +++ b/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() ] ) ); diff --git a/includes/changetags/ChangeTags.php b/includes/changetags/ChangeTags.php index 2169a4d72e..06013975fe 100644 --- a/includes/changetags/ChangeTags.php +++ b/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() ); } } diff --git a/includes/mail/EmailNotification.php b/includes/mail/EmailNotification.php index acf2c2e446..0b77651bda 100644 --- a/includes/mail/EmailNotification.php +++ b/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' ) ) diff --git a/includes/specials/SpecialBlock.php b/includes/specials/SpecialBlock.php index 7d86663dfd..1eb1ad5b3a 100644 --- a/includes/specials/SpecialBlock.php +++ b/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 diff --git a/includes/specials/SpecialContributions.php b/includes/specials/SpecialContributions.php index 08a7fde92b..99eefdde93 100644 --- a/includes/specials/SpecialContributions.php +++ b/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() diff --git a/includes/specials/SpecialEditTags.php b/includes/specials/SpecialEditTags.php index 520380763f..ed398deae4 100644 --- a/includes/specials/SpecialEditTags.php +++ b/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(); diff --git a/includes/specials/SpecialRevisionDelete.php b/includes/specials/SpecialRevisionDelete.php index dd6fea7333..682bceb394 100644 --- a/includes/specials/SpecialRevisionDelete.php +++ b/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(); diff --git a/includes/specials/SpecialUserrights.php b/includes/specials/SpecialUserrights.php index 540754f923..d564e5bc3c 100644 --- a/includes/specials/SpecialUserrights.php +++ b/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(); diff --git a/includes/user/User.php b/includes/user/User.php index 33d216d19c..44673fcc44 100644 --- a/includes/user/User.php +++ b/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(); } diff --git a/maintenance/importImages.php b/maintenance/importImages.php index 172869506b..f27ea2feab 100644 --- a/maintenance/importImages.php +++ b/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; } diff --git a/tests/phpunit/includes/user/UserTest.php b/tests/phpunit/includes/user/UserTest.php index f84be3f1c4..e7bedc2d73 100644 --- a/tests/phpunit/includes/user/UserTest.php +++ b/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();