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

diff --git a/RELEASE-NOTES-1.34 b/RELEASE-NOTES-1.34
index a05f8d1..00e4aad 100644 (file)
--- 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 <https://www.mediawiki.org/wiki/OOUI/Themes> 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 09b2c97..719c60f 100644 (file)
--- 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 248ba14..03c9417 100644 (file)
--- 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 9ad7534..fcc624e 100644 (file)
--- 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 dc0dc79..03043e1 100644 (file)
--- 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 2fd62ee..79286c5 100644 (file)
--- 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'] );