Thijs/wiki.techinc.nl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Ensure block hooks keep user state consistent with realistic blocks

Browse source 
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
This commit is contained in:
Thalia 2019-07-24 16:27:52 +01:00
parent d00185cb18
commit 7a5508573a
6 changed files with 33 additions and 18 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
RELEASE-NOTES-1.34
View file

@ -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 ===

7
docs/hooks.txt
View file

@ -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.

25
includes/Permissions/PermissionManager.php
View file

@ -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;

3
includes/block/AbstractBlock.php
View file

@ -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'] );
}
/**

9
includes/block/BlockManager.php
View file

@ -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 ) );
}
/**

3
includes/block/DatabaseBlock.php
View file

@ -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'] );