Page MenuHomePhabricator

Restrictions of overlapping blocks should be merged on enforcement
Open, NormalPublic5 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).

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
User::getBlockedStatus() should collect all of the blocks. (we could bail when we find the first sitewide block, but you can have overlapping sitewide blocks that have different restrictions... i.e. one could prevent account creation and another could prevent sending emails)

If it finds more than one block, it should take the first block that is found, and merge any restriction properties (email, user talk, page/namespace restrictions) from the other blocks into it. The block should have a property that, when set, prevents the block from being saved (by throwing an error).

Additionally Block::chooseBlock() will need to be deprecated and replaced with this new restriction merging logic.

Create a follow-on task for deprecation(?) or clean up of old, unused methods?

Event Timeline

dbarratt created this task.Oct 3 2018, 8:04 PM
Restricted Application added subscribers: MGChecker, Aklapper. ยท View Herald TranscriptOct 3 2018, 8:04 PM
dmaza added a subscriber: dmaza.Oct 3 2018, 11:50 PM

Whatever is decided here, multiblock should be kept in mind when designing it considering that something very similar needs to be done if/when multiblocks is implemented

Whatever is decided here, multiblock should be kept in mind when designing it considering that something very similar needs to be done if/when multiblocks is implemented

Sure. I'm not sure it has that much overlap. Though you could say this task ought to proceed any work on multiblocks. If multiblocks existed, instead of a limited number of overlapping blocks, you'd potentially have an unlimited number of overlapping blocks.

However, I'm not convinced that multiblocks is the right solution anyways.

I can't think of a succinct yet comprehensive way to write acceptance criteria for this task, so maybe it's fine to leave the 'Solution' section as-is and write a handful of use cases that illustrate what should happen. For example:

  • If a logged-in user has a partial block and their IP address is sitewide blocked (and disallows edits from logged-in users) that person's session should be sitewide blocked.
  • If a logged-in user has a partial block against Kangaroo and their IP address has a partial block against Kiwi that person should be able to edit all pages except Kangaroo and Kiwi
  • etc...

๐Ÿฆ˜ ๐Ÿฅ

TBolliger set the point value for this task to 8.
  • Might have performance impact
  • Currently there is a sequence that checks for blocks.
  • This is not about multi-blocks, but rather to change
  • When in doubt, err to the most restrictive (sitewide over partial)
  • If a user is double blocked, it doesn't really matter which block to show.

We might have to end up splitting another task out of this that is more robust โ€” the fix for this task will be to enforce a sitewide block over a partial block.

TBolliger renamed this task from Overlapping blocks should be merged on enforcement to If a user/IP combo is partially/sitewide (overlapping) blocked, always enforce the sitewide block.Nov 29 2018, 7:45 PM
TBolliger moved this task from Backlog to spare column on the Anti-Harassment board.

The user could have two overlapping partial blocks... and I think those should be enforced together rather than picking one or the other, so I don't think this issue strictly has to do with sitewide blocks exclusively. However it is possible for the user to get a partial block enforcement, when they also have a sitewide block

The user could have two overlapping partial blocks... and I think those should be enforced together rather than picking one or the other, so I don't think this issue strictly has to do with sitewide blocks exclusively. However it is possible for the user to get a partial block enforcement, when they also have a sitewide block

Agreed, David.

We talked today and the more serious concern is that a sitewide blocked IP address/range would be able to edit if they edited from a username which only has a partial block. This needs to be addressed sooner rather than later.

In estimation we discussed a few options, none of which we felt were ideal. The first step for this will be to decide on a correct/desired implementation, then we'll estimate it and take it into a 2019 sprint.

TBolliger triaged this task as Normal priority.Nov 29 2018, 11:39 PM

@TBolliger is it ok, if the user has multiple overlapping partial blocks, if the block notice does not necessarily give them the correct block?

dbarratt renamed this task from If a user/IP combo is partially/sitewide (overlapping) blocked, always enforce the sitewide block to Restrictions of overlapping blocks should be merged on enforcement.Dec 18 2018, 9:05 PM
TBolliger added a comment.EditedDec 18 2018, 9:09 PM

@TBolliger is it ok, if the user has multiple overlapping partial blocks, if the block notice does not necessarily give them the correct block?

Yes.

It's not 100% ideal but these will be edge cases and the most important functionality is the enforcement.

If this is not ready for estimation, please move back.

dbarratt updated the task description. (Show Details)Dec 18 2018, 9:16 PM
dbarratt updated the task description. (Show Details)
dbarratt updated the task description. (Show Details)Dec 18 2018, 9:23 PM
dbarratt updated the task description. (Show Details)

@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