From 7a5508573a3d619c32c5964744d57d004653397f Mon Sep 17 00:00:00 2001 From: Thalia Date: Wed, 24 Jul 2019 16:27:52 +0100 Subject: [PATCH] Ensure block hooks keep user state consistent with realistic blocks Several block-related hooks allow the user to be put into in a state that is inconsistent with blocks that can actually be made: * With UserIsHidden, User::mHideName can be set to true without there being a block * With UserIsBlockedFrom, a user can be blocked from editing a page without there being a block * With GetBlockedStatus, public block properties can be arbitrarily set on a user These problems are mostly theoretical, but mean that it is impossible to make some basic assumptions, e.g. that a user who is blocked from a page must have a block. The hooks are not widely used, and with a few changes we can make them more robust so such assumptions can be made. This patch: * Ensures UserIsBlockedFrom is only called if there is a block. This would be a breaking change if any extensions were using this to block an unblocked user; the intended use case is clearly for extensions to allow user talk page access to blocked users. * Adds a new hook, GetUserBlockComplete, which passes the block for modification. This should be used instead GetBlockedStatus and UserIsHidden, which will be deprecated in the future. * Allows the 'hideName' option to be passed into the AbstractBlock constructor so that suppressing system blocks can be made. Bug: T228948 Bug: T229035 Change-Id: I6f145335abeb16775b08e8c7c751a01f113281e3 --- RELEASE-NOTES-1.34 | 4 ++++ docs/hooks.txt | 7 +++++- includes/Permissions/PermissionManager.php | 25 ++++++++++++---------- includes/block/AbstractBlock.php | 3 +++ includes/block/BlockManager.php | 9 +++++--- includes/block/DatabaseBlock.php | 3 --- 6 files changed, 33 insertions(+), 18 deletions(-) diff --git a/RELEASE-NOTES-1.34 b/RELEASE-NOTES-1.34 index a05f8d18e1..00e4aad680 100644 --- a/RELEASE-NOTES-1.34 +++ b/RELEASE-NOTES-1.34 @@ -98,6 +98,8 @@ For notes on 1.33.x and older releases, see HISTORY. to add fields to Special:Mute. * (T100896) Skin authors can define custom OOUI themes using OOUIThemePaths. See for details. +* (T229035) The GetUserBlock hook was added. Use this instead of + GetBlockedStatus. === External library changes in 1.34 === @@ -347,6 +349,8 @@ because of Phabricator reports. MediaWikiTestCase::resetServices(). * SearchResult protected field $searchEngine is removed and no longer initialized after calling SearchResult::initFromTitle(). +* The UserIsBlockedFrom hook is only called if a block is found first, and + should only be used to unblock a blocked user. * … === Deprecations in 1.34 === diff --git a/docs/hooks.txt b/docs/hooks.txt index 09b2c973c2..719c60f90c 100644 --- a/docs/hooks.txt +++ b/docs/hooks.txt @@ -1726,6 +1726,11 @@ $contentHandler: ContentHandler for which the slot diff renderer is fetched. &$slotDiffRenderer: SlotDiffRenderer to change or replace. $context: IContextSource +'GetUserBlock': Modify the block found by the block manager. This may be a +single block or a composite block made from multiple blocks; the original +blocks can be seen using CompositeBlock::getOriginalBlocks() +&$block: AbstractBlock object + 'getUserPermissionsErrors': Add a permissions error when permissions errors are checked for. Use instead of userCan for most cases. Return false if the user can't do it, and populate $result with the reason in the form of @@ -3700,7 +3705,7 @@ $newUGMs: An associative array (group name => UserGroupMembership object) of the user's current group memberships. 'UserIsBlockedFrom': Check if a user is blocked from a specific page (for -specific block exemptions). +specific block exemptions if a user is already blocked). $user: User in question $title: Title of the page in question &$blocked: Out-param, whether or not the user is blocked from that page. diff --git a/includes/Permissions/PermissionManager.php b/includes/Permissions/PermissionManager.php index 248ba14dc6..03c9417f48 100644 --- a/includes/Permissions/PermissionManager.php +++ b/includes/Permissions/PermissionManager.php @@ -289,7 +289,8 @@ class PermissionManager { } /** - * Check if user is blocked from editing a particular article + * Check if user is blocked from editing a particular article. If the user does not + * have a block, this will return false. * * @param User $user * @param LinkTarget $page Title to check @@ -298,27 +299,29 @@ class PermissionManager { * @return bool */ public function isBlockedFrom( User $user, LinkTarget $page, $fromReplica = false ) { - $blocked = $user->isHidden(); + $block = $user->getBlock( $fromReplica ); + if ( !$block ) { + return false; + } // TODO: remove upon further migration to LinkTarget $title = Title::newFromLinkTarget( $page ); + $blocked = $user->isHidden(); if ( !$blocked ) { - $block = $user->getBlock( $fromReplica ); - if ( $block ) { - // Special handling for a user's own talk page. The block is not aware - // of the user, so this must be done here. - if ( $title->equals( $user->getTalkPage() ) ) { - $blocked = $block->appliesToUsertalk( $title ); - } else { - $blocked = $block->appliesToTitle( $title ); - } + // Special handling for a user's own talk page. The block is not aware + // of the user, so this must be done here. + if ( $title->equals( $user->getTalkPage() ) ) { + $blocked = $block->appliesToUsertalk( $title ); + } else { + $blocked = $block->appliesToTitle( $title ); } } // only for the purpose of the hook. We really don't need this here. $allowUsertalk = $user->isAllowUsertalk(); + // Allow extensions to let a blocked user access a particular page Hooks::run( 'UserIsBlockedFrom', [ $user, $title, &$blocked, &$allowUsertalk ] ); return $blocked; diff --git a/includes/block/AbstractBlock.php b/includes/block/AbstractBlock.php index 9ad753498b..fcc624e6c3 100644 --- a/includes/block/AbstractBlock.php +++ b/includes/block/AbstractBlock.php @@ -96,6 +96,7 @@ abstract class AbstractBlock { * reason string Reason of the block * timestamp string The time at which the block comes into effect * byText string Username of the blocker (for foreign users) + * hideName bool Hide the target user name */ public function __construct( array $options = [] ) { $defaults = [ @@ -104,6 +105,7 @@ abstract class AbstractBlock { 'reason' => '', 'timestamp' => '', 'byText' => '', + 'hideName' => false, ]; $options += $defaults; @@ -120,6 +122,7 @@ abstract class AbstractBlock { $this->setReason( $options['reason'] ); $this->setTimestamp( wfTimestamp( TS_MW, $options['timestamp'] ) ); + $this->setHideName( (bool)$options['hideName'] ); } /** diff --git a/includes/block/BlockManager.php b/includes/block/BlockManager.php index dc0dc79a51..03043e1509 100644 --- a/includes/block/BlockManager.php +++ b/includes/block/BlockManager.php @@ -23,6 +23,7 @@ namespace MediaWiki\Block; use DateTime; use DateTimeZone; use DeferredUpdates; +use Hooks; use IP; use MediaWiki\Config\ServiceOptions; use MediaWiki\Permissions\PermissionManager; @@ -184,6 +185,7 @@ class BlockManager { // Filter out any duplicated blocks, e.g. from the cookie $blocks = $this->getUniqueBlocks( $blocks ); + $block = null; if ( count( $blocks ) > 0 ) { if ( count( $blocks ) === 1 ) { $block = $blocks[ 0 ]; @@ -195,10 +197,11 @@ class BlockManager { 'originalBlocks' => $blocks, ] ); } - return $block; } - return null; + Hooks::run( 'GetUserBlock', [ clone $user, $ip, &$block ] ); + + return $block; } /** @@ -227,7 +230,7 @@ class BlockManager { } } - return array_merge( $systemBlocks, $databaseBlocks ); + return array_values( array_merge( $systemBlocks, $databaseBlocks ) ); } /** diff --git a/includes/block/DatabaseBlock.php b/includes/block/DatabaseBlock.php index 2fd62ee332..79286c5acf 100644 --- a/includes/block/DatabaseBlock.php +++ b/includes/block/DatabaseBlock.php @@ -93,7 +93,6 @@ class DatabaseBlock extends AbstractBlock { * anonOnly bool Only disallow anonymous actions * createAccount bool Disallow creation of new accounts * enableAutoblock bool Enable automatic blocking - * hideName bool Hide the target user name * blockEmail bool Disallow sending emails * allowUsertalk bool Allow the target to edit its own talk page * sitewide bool Disallow editing all pages and all contribution @@ -112,7 +111,6 @@ class DatabaseBlock extends AbstractBlock { 'anonOnly' => false, 'createAccount' => false, 'enableAutoblock' => false, - 'hideName' => false, 'blockEmail' => false, 'allowUsertalk' => false, 'sitewide' => true, @@ -129,7 +127,6 @@ class DatabaseBlock extends AbstractBlock { # Boolean settings $this->mAuto = (bool)$options['auto']; - $this->setHideName( (bool)$options['hideName'] ); $this->isHardblock( !$options['anonOnly'] ); $this->isAutoblocking( (bool)$options['enableAutoblock'] ); $this->isSitewide( (bool)$options['sitewide'] ); -- 2.20.1