Page MenuHomePhabricator

Restrictions of overlapping blocks should be merged on enforcement
Closed, ResolvedPublic5 Story Points

Description

Problem
It is possible to have overlapping blocks. For instance you can have a block on your user as well as a hard block on an IP address (that also applies to your user). Likewise, you can have an IP Range block that could apply to your IP address and (possibly) your user (if it's a hard block).

BlockManager::getUserBlock() (formerly User::getBlockedStatus()) picks a block to use and that is what gets used. This becomes a problem when overlapping blocks have different configuration.

For instance, if a user is partially blocked from Saturn and IP address (hard block) is partially blocked from Mars, The user should be blocked from both Saturn and Mars. Likewise, if either of these blocks are sitewide blocks, the user should be sitewide blocked.

Likewise, if a user is IP blocked and has an overlapping IP Range block, the user should have all of the restrictions of both blocks.

Solution
BlockManager::getUserBlock() should collect all of the blocks. If it finds more than one block, it should merge any restriction properties (email, user talk, page/namespace restrictions) from the blocks together. The merged block should not be able to be saved.

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

@TBolliger I created a separate issue for the block notices T212326.

TBolliger added a subscriber: Tchanders.

@Tchanders' investigation results can be found in T211238#4922950

TBolliger set the point value for this task to 5.
aezell updated the task description. (Show Details)Feb 7 2019, 6:13 PM

Change 492729 had a related patch set uploaded (by Tchanders; owner: Tchanders):
[mediawiki/core@master] WIP Merge restrictions of multiple blocks on enforcement

https://gerrit.wikimedia.org/r/492729

Tchanders added a comment.EditedMar 12 2019, 12:10 PM

The proposal is to change User::getBlockedStatus to return a merged block combining the features of all blocks that apply to the user. This method is called from many places, and in the following ways:

  • To check whether or not the user has a block - true or false. This use case won't be affected
  • To build an error message, explaining why someone is blocked. In this case, the message will become more vague if someone is blocked by multiple blocks, but we will solve that in T212326
  • In SpecialBlock::checkUnblockSelf, to examine the parameters of a block specifically to the user. We should be careful not to disrupt this, so should have some mechanism to return the user block if there are multiple blocks.
  • In PasswordReset::isBlocked, to decide whether to ignore a block, which relies on knowing the system block type. We need to be able to detect if a merged block is from multiple ignorable system blocks. (See T218905)
  • In User::trackBlockWithCookie, to decide whether to track the block with a cookie. Blocks are tracked if the target is an IP or an IP range. We need to be able to recover the types of the original blocks from a merged block. (See T218905)
  • A few other places, where error messages are created in a different way depending on the block type (e.g. AuthManager::checkAccountCreatePermissions)

Change 497667 had a related patch set uploaded (by Tchanders; owner: Tchanders):
[mediawiki/core@master] Separate Block into AbstractBlock, Block and SystemBlock

https://gerrit.wikimedia.org/r/497667

Change 497668 had a related patch set uploaded (by Tchanders; owner: Tchanders):
[mediawiki/core@master] Add MultipleBlock class for enforcing multiple blocks

https://gerrit.wikimedia.org/r/497668

@Tchanders I cherry-picked the two changesets in T206163#5110429 and T206163#5110430 on my local environment, and I have noticed that anonymous only blocks seem to get applied to users who are logged in. Not sure why.

I thought it might be due to multiple blocks, but the same thing happens if there is only one block.

Could you check if you are seeing this on your own local environment?

@dom_walden Hmm yes, looks like it's coming from T206163#5110430 (doesn't happen when I check out T206163#5110429). I'll investigate, thanks for spotting this.

Change 492729 abandoned by Tchanders:
WIP Merge restrictions of multiple blocks on enforcement

Reason:
We decided to introduce a MultipleBlock type to handle cases of overlapping blocks, instead of complicating the Blocks system further. See I088401105a

https://gerrit.wikimedia.org/r/492729

@Tchanders is this ready for review? if so, which patch?

Tchanders updated the task description. (Show Details)

Change 512447 had a related patch set uploaded (by Tchanders; owner: Tchanders):
[mediawiki/core@master] Decouple Block::newFromTarget from Block::newLoad

https://gerrit.wikimedia.org/r/512447

dom_walden added a comment.EditedMay 29 2019, 7:57 AM

@Tchanders If there is an anonymous IP block and I am logged in on that IP, when I go to Special:CreateAccount I get a fatal error:

Error from line 1379 of /var/www/html/T206163/includes/user/User.php: Call to a member function getBlockForCookie() on null

I have not seen this happen for any other kind of block I have tested (so far). It doesn't seem to do it for any other action, either.

Attempting to unblock an auto block (the IP that was autoblocked, not the username that was blocked):

Error from line 365 of /var/www/html/T206163/includes/Block.php: Call to a member function getType() on string
#0 /var/www/html/T206163/includes/Block.php(1143): Block::chooseMostSpecificBlock(Block)
#1 /var/www/html/T206163/includes/specials/SpecialUnblock.php(183): Block::newFromTarget(string)
#2 /var/www/html/T206163/includes/specials/SpecialUnblock.php(166): SpecialUnblock::processUnblock(array, RequestContext)
#3 /var/www/html/T206163/includes/htmlform/HTMLForm.php(675): SpecialUnblock::processUIUnblock(array, OOUIHTMLForm)
#4 /var/www/html/T206163/includes/htmlform/HTMLForm.php(567): HTMLForm->trySubmit()
#5 /var/www/html/T206163/includes/htmlform/HTMLForm.php(582): HTMLForm->tryAuthorizedSubmit()
#6 /var/www/html/T206163/includes/specials/SpecialUnblock.php(68): HTMLForm->show()
#7 /var/www/html/T206163/includes/specialpage/SpecialPage.php(570): SpecialUnblock->execute(NULL)
#8 /var/www/html/T206163/includes/specialpage/SpecialPageFactory.php(575): SpecialPage->run(NULL)
#9 /var/www/html/T206163/includes/MediaWiki.php(288): MediaWiki\Special\SpecialPageFactory->executePath(Title, RequestContext)
#10 /var/www/html/T206163/includes/MediaWiki.php(865): MediaWiki->performRequest()
#11 /var/www/html/T206163/includes/MediaWiki.php(515): MediaWiki->main()
#12 /var/www/html/T206163/index.php(42): MediaWiki->run()

My guess:
newFromTarget() gets the list of blocks it passes to chooseMostSpecificBlock() from newListFromTarget(), but the latter does not return a list (as far as I can see).

@dom_walden Thanks for reporting these - they should be fixed now.

Tchanders updated the task description. (Show Details)Jun 4 2019, 8:01 PM
dom_walden added a comment.EditedJun 5 2019, 1:06 PM

An anonymous IP block appears to block a logged in user from Special:CreateAccount. Not from any other action (that I have seen so far).

EDIT: Actually, this bug also exists on latest pull of master... I'll investigate.

@dom_walden It turns out that is intentional, according to: https://github.com/wikimedia/mediawiki/blob/master/includes/user/User.php#L4377 - I can't find it documented anywhere else.

It's quite odd that a system that allows you (1) to choose whether a block prevents account creation and (2) to choose whether an IP block applies to logged-in users, magically doesn't allow you to let IP blocks allow logged-in users to create accounts. The need for this functionality should really trigger a rethink of the system.

For now, the least we can do is document it at https://www.mediawiki.org/wiki/Help:Blocking_users so people can have a hope of understanding how a block works without reading the code.

JJMC89 added a subscriber: JJMC89.Jun 5 2019, 4:26 PM

An anonymous IP block appears to block a logged in user from Special:CreateAccount. Not from any other action (that I have seen so far).
EDIT: Actually, this bug also exists on latest pull of master... I'll investigate.

@dom_walden It turns out that is intentional, according to: https://github.com/wikimedia/mediawiki/blob/master/includes/user/User.php#L4377 - I can't find it documented anywhere else.
It's quite odd that a system that allows you (1) to choose whether a block prevents account creation and (2) to choose whether an IP block applies to logged-in users, magically doesn't allow you to let IP blocks allow logged-in users to create accounts. The need for this functionality should really trigger a rethink of the system.

See my comment at T189362#5232305 related to the issue in master.

(I had a quick check that it does allow a logged-in user to create an account if the block allows account creation)

@Mooeypoo @dmaza @dbarratt As discussed, MultipleBlock might be too easily confused with the concept of storing multiple blocks against exactly the same target; whereas this work is about enforcing (but not storing) multiple blocks against different targets that all apply to a given user.

What name do we prefer? So far the suggestion is CompositeBlock which sounds good to me.

Change 512447 merged by jenkins-bot:
[mediawiki/core@master] Decouple DatabaseBlock::newFromTarget from DatabaseBlock::newLoad

https://gerrit.wikimedia.org/r/512447

@Mooeypoo @dmaza @dbarratt As discussed, MultipleBlock might be too easily confused with the concept of storing multiple blocks against exactly the same target; whereas this work is about enforcing (but not storing) multiple blocks against different targets that all apply to a given user.

OTOH, if we did expand the scope of multiple blocks... would we use that class?

What name do we prefer? So far the suggestion is CompositeBlock which sounds good to me.

I think CompositeBlock sounds great to me, and describes what the class does a lot better than the other options. It sounds like it's taking however-many and boils it down to a composite result. Perfect, imo.

OTOH, if we did expand the scope of multiple blocks... would we use that class?

That's a good question, but even if we do, I think something like "CompositeBlock" is better and more understandable.

So, even if we did have an expanded scope where you could store several DB blocks for one target, the class in question still takes whatever-relevant (one, two, or ten?) and boils it down to the relevant restrictions -- that sounds to me, behaviorally, to still be "composite" rather than "multiple".

That said, I also think it's probably a bit of a bikeshedding here, and I acknowledge I'm ESL :P so... I don't know if I am making English sense to anyone but my preference :D

dmaza added a comment.Jun 5 2019, 9:47 PM

+1 for CompositeBlock. IMO the name fits/describes how the object behaves (https://en.wikipedia.org/wiki/Composite_pattern)

I don't have a preference. :)

Change 497668 merged by jenkins-bot:
[mediawiki/core@master] Add CompositeBlock class for enforcing multiple blocks

https://gerrit.wikimedia.org/r/497668

This comment was removed by dom_walden.

Sorry, wrong task.

Generating random combinations of blocks including:
+ Username, IP and IP ranges, autoblocks
+ System blocks (e.g. $wgProxyList, $wgDnsBlacklistUrls, $wgApplyIpBlocksToXff, Tor)
+ Global blocks
+ Blocks set from cookies
and testing different actions anonymously and while logged in (e.g. editing, account creation, sending email).

In all circumstances, the actions were blocked or allowed as appropriate for the blocks that applied. (Apart from preexisting bugs or some cases mentioned above.)

Also, paying attention to which blocks cookies get set from.

Testing locally and on beta (various versions as this was done over a couple of weeks).

dbarratt closed this task as Resolved.Jun 18 2019, 4:11 PM

Thanks @dom_walden! I know testing blocks isn't easy. You've been doing an amazing job and I can't even imagine how! :)