Page MenuHomePhabricator

Global blocks should be treated as a subtype of blocks
Open, Needs TriagePublicSpike

Description

MediaWiki's handling of global blocks is weird in many ways:

  • User::getBlock() does not return global blocks, you need to call User::getGlobalBlock() and there are lots of places in our codebase where this doesn't happen. Same for User::isBlocked() etc. The signatures are different, you can't force a primary DB read for global blocks etc.
  • User::getGlobalBlock() says "Do not use for actual edit permission checks! This is intended for quick UI checks." so even calling both isn't safe (although looking at the code I don't see why it wouldn't be, unless it refers to the lack of READ_LATEST support, which I don't think is realistically important). What should be done instead is unclear. (Use PermissionManager, I guess, but sometimes we specifically want to know whether the user is blocked or not.)
  • Passing a global block to BlockErrorFormatter results in a completely wrong block message. (This is also true for non-global blocks coming from an extension, such as RegexBlock or MediaWiki-extensions-TorBlock.) You either need to figure out which extension owns the block and what methods that extension exposes for block formatting, or use $block->getPermissionsError() which is deprecated and has a different signature with more limited options (you can get around that using DerivativeContext, but ew).
  • I don't think there's a way to get a list of blocks (whether for a specific target or in general). Global blocks are cross-linked from some special pages via the OtherBlockLogLink/OtherAutoblockLogLink hooks. Given that extensions can chose different arbitrary mechanisms for storing blocks, it might not be realistic to do more than that. It would be nice though. Also, it would be nice to cross-link other pages (like block/unblock pages or block list pages) as well. (FWIW there is some ongoing work related to navigation between special pages in T313349: Allow special pages to register navigation links in skins.)
  • User::spreadBlock() and similar methods only apply to core blocks, even though in theory it makes sense for a blocking-related extension to want to handle them.
  • It's unclear what methods a global block is supposed to support. E.g. GlobalBlock is a subclass of DatabaseBlock but calling most DatabaseBlocks would result in crazyness (e.g. GlobalBlock::delete would delete the local block which happens to gave the same ID - granted, that method shouldn't exist in the first place and Block objects should be value objects). Most permission-related methods like GlobalBlock::isCreateAccountBlocked give wrong results.
  • There is no good way to tell if a Block object is a global block if you don't know where it came from. (Should core even have a concept of global blocks, as opposed to just blocks provided by some extension? Not sure. So maybe this isn't a problem.)
  • The Block::TYPE_* nomenclature isn't extensible (or at least isn't documented to be, and it seems hard to coordinate that for integers), even though there is no reason a block would always be based on user or IP. (A bit of an academic problem though, as we don't actually have other block types.)

The ideal solution would be to just merge global blocks into normal blocks:

  • User::getBlock() et al would also check for global blocks. The GetUserBlock and UserIsBlockedGlobally hooks should be merged. (I think there is one situation where a separate global block hook could be useful: when checking whether a user with no local account on the wiki is blocked. This is a very particular use case that's only needed during account creation / autocreation, though; and the current global block hook doesn't really support it.)
  • BlockErrorFormatter should handle non-core blocks via its own set of hooks.
  • GlobalBlock should probably subclass AbstractBlock directly, and share code as needed with DatabaseBlock via a trait/interface. (At a glance the only methods it needs are get/setBlocker, although if T17294: Allow globally blocking of accounts gets done it would probably want to implement autoblocks too.) Also the Block/AbstractBlock separation does not seem useful as it is and should probably be reworked so there's an interface that can actually be used for implementing new block types.
  • Maybe there should be a hook to get all blocks for a given set of filters, and special pages should list global blocks based on that, although this might be (way) more effort than it's worth.

Related Objects

StatusSubtypeAssignedTask
OpenSpikeNone
ResolvedCyndymediawiksim
ResolvedCyndymediawiksim
ResolvedTchanders
ResolvedCyndymediawiksim
ResolvedCyndymediawiksim
ResolvedCyndymediawiksim
Resolved AGueyte
Resolved TThoabala
ResolvedDreamy_Jazz
Resolved AGueyte
ResolvedWMDE-Fisch
Resolved AGueyte
ResolvedCyndymediawiksim
ResolvedCyndymediawiksim
OpenNone
ResolvedTchanders
ResolvedTchanders
Resolved AGueyte
ResolvedSTran
OpenNone

Event Timeline

Tgr updated the task description. (Show Details)
Tchanders subscribed.

@Tgr Thanks for filing this and providing so much detail! Anti-Harassment would like to do some work in this area. We may start by trying to break this up into smaller tasks.

Restricted Application changed the subtype of this task from "Task" to "Spike". · View Herald TranscriptAug 30 2022, 4:15 PM

That would be great, thanks @Tchanders!
FWIW I started some documentation at https://www.mediawiki.org/wiki/Manual:Block_abstraction_layer on how blocks work from an extension's POV.

I've created a few tasks addressing some of the issues from this task's description. Mostly they focus on treating global blocks like other types of blocks, and moving global-blocks-specific behaviour and handling into the GlobalBlocking extension. Also separating GlobalBlock from DatabaseBlock and smaller clean-up work. This is what I think Anti-Harassment can handle with the time we've been given, and also the work which I think is more straightforward to define with technical-only input.

I'll move this to our backlog while we work on the subtasks, but we can keep discussing here or on those. (The tasks I've filed so far don't yet address block lists, User::spreadBlock, block type constants, better defining Block/Abstract block distinction or getting blocks for specific filters.)

FWIW I started some documentation at https://www.mediawiki.org/wiki/Manual:Block_abstraction_layer on how blocks work from an extension's POV.

Thanks! This is really helpful, and we'll try to keep it updated.

Unassigning myself, since I was assigned to create the subtasks for some of this work. (We may yet add more but I'm not working on that right now.)