Page MenuHomePhabricator

Introduce a BlockManager service for getting and combining the blocks that apply to a user/IP
Closed, ResolvedPublic3 Story Points

Description

Introduce a BlockManager service for checking the blocks that apply to a user/IP, to replace the main logic in User::getBlockedStatus and related methods.

The reason we’re doing this now is because checking blocks is becoming more complicated, particularly when multiple blocks apply to a user/IP. Currently, only one block is chosen, but this is not tenable now that we have partial blocks, because a user/IP may be blocked much more leniently than intended, if a partial block is chosen over a sitewide block (T206163).

In the future, the BlockManager could be expanded to incorporate some or all of the factory/utility/cookie-related/database-related methods from Block (see comment below).

Basic plan

Move implementation of block-checking methods from user to the new BlockManager and soft-deprecate said methods in the User class:
User::getBlockedStatus (private)
User::getBlockFromCookieValue (protected)
User::isLocallyBlockedProxy (public - called from UserTest)
User::isDnsBlacklisted (public - called from AuthManager)
User::inDnsBlacklist (public but could be private)

User::getBlockedStatus will call a BlockManager method that queries for all blocks relevant to the user/IP. If multiple are found, a temporary system block will be made that combines the strictest parameters of these. The BlockManager method will return a single block, and User::getBlockStatus will use this block to populate the User properties, as it does currently.

Event Timeline

aezell created this task.Mar 27 2019, 7:26 PM
Restricted Application added subscribers: MGChecker, Aklapper. · View Herald TranscriptMar 27 2019, 7:26 PM
aezell set the point value for this task to 3.Mar 27 2019, 7:27 PM
Niharika edited projects, added Anti-Harassment (Zayin - ז); removed Anti-Harassment.
Niharika triaged this task as Normal priority.

Factories (service):

  • Block::newFromID()
  • Block::newLoad() (this method should take in a target and return a block)
  • Block::initFromRow()
  • Block::newFromRow()
  • Block::newFromTarget()
  • User::getBlockedStatus()

Utilities (service)

  • Block::getIpFragment()
  • Block::isWhitelistedFromAutoblocks()
  • Block::getRangeStart()
  • Block::getRangeEnd()
  • Block::getAutoblockExpiry()
  • Block::purgeExpired()
  • Block::getBlocksForIPList()
  • Block::chooseBlock()
  • Block::parseTarget()
  • Block::getPermissionsError()
  • Block::getBlockErrorParams()
  • User::isLocallyBlockedProxy()

Cookie (another service?)

  • Block::setCookie()
  • Block::clearCookie()
  • Block::getCookieValue()
  • Block::getIdFromCookieValue()
  • User::trackBlockWithCookie()
  • `User::Cookie (another service?)
  • Block::setCookie()
  • Block::clearCookie()
  • Block::getCookieValue()
  • Block::getIdFromCookieValue()
  • User::trackBlockWithCookie()

Database (service?):

  • Block::selectFields()
  • Block::getQueryInfo()
  • Block::getRangeCond()
  • Block::delete()
  • Block::insert()
  • Block::update()
  • Block::getDatabaseArray()
  • Block::getAutoblockUpdateArray()
  • Block::doRetroactiveAutoblock()
  • Block::defaultRetroactiveAutoblock()
  • Block::doAutoblock()
  • Block::deleteIfExpired()
  • Block::updateTimestamp()
  • Block::getRestrictions() (This should probably just be part of the BlockRestriction service, see T219684. Or maybe the restrictions should only be loaded in the factories and not lazy loaded like they are now)
  • Block::findRestriction() (This should be moved the a BlockRestriction service, see T219684. But ideally, it shouldn't make database queries)
  • User::spreadAnyEditBlock()
  • User::spreadBlock()

Value (block)

  • Block::equals() (this does make a database query, maybe it shouldn't and the restrictions should be loaded when the block is loaded)
  • Block::isExpired()
  • Block::isValid()
  • Block::getBy() (this needs to be fixed, Block::getBlocker() can return null)
  • Block::getByName() (this needs to be fixed, Block::getBlocker() can return null)
  • Block::getId()
  • Block::getReason()
  • Block::setReason()
  • Block::getHideName()
  • Block::setHideName()
  • Block::getSystemBlockType()
  • Block::fromMaster()
  • Block::isHardblock()
  • `Block::isAutoblocking()
  • Block::isSitewide()
  • Block::isCreateAccountBlocked()
  • Block::isEmailBlocked()
  • Block::isUsertalkEditAllowed()
  • Block::appliesToRight()
  • Block::prevents()
  • Block::getRedactedName()
  • Block::getType()
  • Block::getTargetAndType()
  • Block::getTarget()
  • Block::getExpiry()
  • Block::setExpiry()
  • Block::getTimestamp()
  • Block::setTimestamp()
  • Block::setTarget()
  • Block::getBlocker()
  • Block::setBlocker()
  • Block::setRestrictions()
  • Block::appliesToUsertalk()
  • Block::appliesToTitle()
  • Block::appliesToNamespace()
  • Block::appliesToPage()

Value (user)

  • User::isBlocked()
  • User::getBlock()
  • User::isBlockedFrom()
  • User::blockedBy()
  • User::blockedFor()
  • User::getBlockId()
  • User::isBlockedGlobally()
  • User::getGlobalBlock() (most of this should be moved into a service since most of it is a factory)
  • User::isBlockedFromCreateAccount()
  • User::isBlockedFromEmailuser()
  • User::isBlockedFromUpload()
Tchanders renamed this task from Design an interface for a BlockManager class(es) to Introduce a BlockManager service for getting and combining the blocks that apply to a user/IP.Apr 2 2019, 8:57 PM
Tchanders updated the task description. (Show Details)

What do we see the as eventual scope for the BlockManager?

If it is for determining/managing the block parameters that apply to a user, then perhaps User::getBlock etc belong in the BlockManager. (On the other hand, methods such as User::isAllowed probably don't, since they are related to permissions.)

Should the scope be expanded to incorporate the other services outlined by @dbarratt? (If not, maybe we should rename it to UserBlockManager.) Should any of these become separate services?

Should the scope be expanded to incorporate the other services outlined by @dbarratt? (If not, maybe we should rename it to UserBlockManager.) Should any of these become separate services?

Limiting the scope seems wise. It seems (from what I can tell) that more smaller independent services are preferred to larger ones.

daniel added a comment.Apr 3 2019, 2:22 PM

I made a similar task last week, which I now closed as a duplicate of this one: T218394. Here is the original task description for reference:

The purpose of this task is to allow application logic to obtain a Block object based on a UserIdentity object (or some other kind of block target) without the need to have a User object. BlockStore is to replace the usage of User::getBlock (and related methods) in PermissionManager (see T208768) and other pplaication logic.

Note that eventually, Block should become a value object (or be replaced by a value object) that has no logic for modifying the database, and no knowledge of the underlying schema. All logic related to the DB schema should be moved to BlockStore. That however is beyond the scope of this ticket. This ticket is a partial refactoring, leaving Block and BlockStore tightly coupled to each other until more functionality is factored out of Block. This fact should be noted as a caveat in the class level documentation of the new class.

For now, the following methods should be moved into the new BlockStore service:

Methods expected to move into the new class:

  • User::getBlockedStatus (private)
  • User::getBlock()
    • User::isBlocked() should probably be deprecated in favor of BLockStore::getBlock(). Same for blockedBy(), blockedFor(), getBLockId(), etc.
    • Note that isBlockedFrom() is currently being moved to PermissionManager, not BlockStore. Maybe that should be re-considered, or the method could be present in both interfaces, with PermissionManager merely delegating.
  • User::getGlobalBlock()
  • User::isDnsBlacklist and isDnsBlacklisted (does this even need to be public?)
  • User::isLocallyBlockedProxy (does this even need to be public?)
  • User::getBlockFromCookieValue is a bot unclear. SHould not be in User, but should it be in BlockStore, or somewhere else? Should the BlockStore know about cookies?
  • Block::newFromTarget, though perhaps we want newFromUser( UserIdentity ) and newFromIP( $ip ) instead, and introduce separate methods for constructing Block objects from other kinds of block targets. If we want to leave this for later, BlockStore::newFromUser and newFromIP( $ip ) can for now call Block::newFromTarget.
  • User::getBlockStatus, and ideally any static method in Block called from there, like Block::getBlocksForIPList and Block::chooseBlock.

    Considerations:
  • BlockStore should bind to UserIdentity instead of User. Any remaining references to the User class should be marked as debt to be removed. References to User should be removed from the BLock class as well, where they can be replaced with UserIdentity.
  • BlockStore should bind to PageIdentity instead of Tilte, once PageIdentity identity becomes available (see T208776). Hints against Title in parameter lists are accatable for now, returning a Title would be unacceptable.
  • Interaction with the HTTP response, in particular to get, set or clear cookies, don't belong in the storage layer. If we can't find a way to keep them out of the new service, perhaps the service is a BlockManager, not a BlockStore. Or we need both in the end.
daniel added a comment.Apr 3 2019, 2:28 PM

I agree with mostly with the plan outlined above. A few comments:

When creating new services and especially new value objects, avoid the Use and Title classes, at least in the public interface. Internal use is still OK, but should be marked with a TODO. Instead of the User class, use UserIdentity. Instead of the Title class, use LinkTarget when only the title (text and namespace) is relevant. If the page ID is needed, you'll have to use Title for now, until we have PageIdentity (T208776).

Also, in my mind, all the stuff in the Value (user) section should also move into a service - perhaps into BlockManager.

In general, I wonder if all these many methods really have to be public. What's the minimum set that could be made public, so application code can still do what it needs to do?

Also, please consider splitting BlockManager and BlockStore. BlockStore would have all the things that touch the database, while BlockManager builds on top of BlockStore and does all the things related to cookies and sessions and such.

Agabi10 added a subscriber: Agabi10.Apr 3 2019, 2:47 PM
dmaza added a subscriber: dmaza.Apr 3 2019, 3:07 PM

Also, please consider splitting BlockManager and BlockStore. BlockStore would have all the things that touch the database, while BlockManager builds on top of BlockStore and does all the things related to cookies and sessions and such.

@daniel Regarding the BlockStore, it was not mentioned that we are also planning on splitting blocks into 3 different classes. AbstractBlock, DatabaseBlock, SystemBlock. For the sake of backwards compatibility a DatabaseBlock will remain as Block for the time being but all the block related database queries will be in DatabaseBlock (still up for discussion).

Here is a WIP of how this would look like https://gerrit.wikimedia.org/r/c/mediawiki/core/+/497667

Ideally, all interaction with blocks should go through BlockManager except perhaps the creation of block objects

Tgr added a subscriber: Tgr.Apr 3 2019, 4:47 PM
dmaza added a comment.Apr 9 2019, 4:30 PM

@daniel are there any name conventions to follow for this "servicification" we are doing? Or any other conventions for that matter?
"Store, Service, Manager", are we using those names interchangeably?

See also: T219684: BlockRestriction should be a service instead of a singleton

daniel added a comment.Apr 9 2019, 8:01 PM

@daniel are there any name conventions to follow for this "servicification" we are doing? Or any other conventions for that matter?
"Store, Service, Manager", are we using those names interchangeably?

See also: T219684: BlockRestriction should be a service instead of a singleton

There are no established rules, but some patterns to be found in the existing services:

  • "Service" itself is rare as a suffix for a service class. Which is good, since it doesn't provide any useful information.
  • A "Lookup" is something that can be used to look up stuff (read: value objects) given some kind of name, id, or address. It will have methods that start with "get" or "load" or "fetch", etc. It's basically a read-only Store, and a Store may well implement a Lookup interface.
  • A "Store" is something that can be used to store things permanently (and then look up later). The things it stores are generally plain value objects. The methods in a Store will include some with names like "save" or "store" or "create" or "put" or "update".
  • A "Manager" provides some kind of application logic on top of other services (usually, a Store), e.g. authentication.
  • There are also "Factories" and "Registries", "Caches" and "Stashes", but these are irrelevant here, and probably obvious.

Change 502820 had a related patch set uploaded (by Tchanders; owner: Tchanders):
[mediawiki/core@master] WIP Introduce a BlockManager service

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

Here's a suggestion for multiple iterations of the BlockManager, broken down in to manageable chunks that can fit around our Block refactoring work (T206163):

  1. Factor out the innards of User::getBlockedStatus, along with the helper methods (this task).
  2. Move over methods from Block that belong in the BlockManager - filed as T221066.
  3. Remove the block-related methods from User - filed as T221067. User::getBlockedStatus will need refactoring at that point. User::trackBlockWithCookie and User::getGlobalBlock interact with these methods, so this is the time to move those.

The BlockStore should be a separate service - T221075

Tchanders updated the task description. (Show Details)Apr 16 2019, 11:58 AM

Here's a suggestion for multiple iterations of the BlockManager, broken down in to manageable chunks that can fit around our Block refactoring work (T206163):

At a glance, that sounds like a good plan!

Thanks for planning this out @Tchanders.

Change 502820 merged by jenkins-bot:
[mediawiki/core@master] Introduce a BlockManager service

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

Niharika moved this task from Ready to Done on the Anti-Harassment (Yōd - י) board.
Niharika moved this task from Done to QA/Testing on the Anti-Harassment (Yōd - י) board.
dom_walden added a subscriber: dom_walden.

The BlockManager gets the block appropriate for the user or IP, including setting from cookies, XFF headers, etc.

I tested that blocks were set appropriately based on which cookies the user had, including both IP ($wgCookieSetOnIpBlock) and auto ($wgCookieSetOnAutoblock) blocks. Where possible I verified that the block returned was correct by looking at the block ID reported in the error message.

I also tested that the cookies were cleared when appropriate (which BlockManager is also responsible for). E.g. when a user with a cookie from an IP logs in, or when a block expires.

I tested blocks were set for IPs in $wgProxyList and those blacklisted by $wgDnsBlacklistUrls.

Briefly tested blocks from XFF headers. I did this more throughly in T222737.

dbarratt closed this task as Resolved.Wed, May 22, 6:06 PM