Page MenuHomePhabricator

Add global blocks into CompositeBlocks rather than treating them separately
Closed, ResolvedPublic3 Estimated Story Points

Description

Background

User::getGlobalBlock is used to determine whether a user is blocked globally. This is an unusual case of MediaWiki core knowing about functionality defined in an extension (GlobalBlocking).

The purpose of checking for global blocks specifically seems to be to ensure that the global block information is shown instead of local block information, if both global and local blocks exist. Now that we have CompositeBlock, it is possible to show information about more than one block.

Add the global block via the hook GetUserBlock instead of checking for global blocks separately.

Acceptance criteria
  • GlobalBlocking has a handler for the GetUserBlock hook, which adds any block it finds. If there are already blocks, the global block is added to a CompositeBlock. Otherwise, a GlobalBlock is added.
Notes
  • GlobalBlocking can check blocks via other hooks. These should be unaffected by this work, and may be addressed later.

Related Objects

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

Event Timeline

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

Hmm, I've been looking into this, and I have a fairly functional method by which to do this (need to make some updates still to the GlobalBlock class as it does not define all the block options it needs to), however I've encountered a new problem. As of T227174, it is no longer possible for block objects to define their own error message key and parameters, as both of these are effectively hard-coded into BlockErrorFormatter for the existing block classes in MediaWiki core. This means that the error message you see when blocked by a global block (if this change is implemented) will no longer make as much sense.

Additionally, a change would probably need to be made to CompositeBlock to allow for non-MediaWiki core blocks to define their own contexts, as otherwise it may not be possible to inform the user that they may be subject to a global block.

Thanks for looking into this @TheVoidwalker. Haven't given this much thought yet, but would it help to add the global block-specific message into the block reason instead?

Thanks for looking into this @TheVoidwalker. Haven't given this much thought yet, but would it help to add the global block-specific message into the block reason instead?

Ah, thanks for the suggestion, that's exactly what I've wound up doing.

Change 655801 had a related patch set uploaded (by Voidwalker; owner: Voidwalker):
[mediawiki/extensions/GlobalBlocking@master] Apply GlobalBlocks through GetUserBlock hook

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

I filed T315644: Global blocks should be treated as a subtype of blocks before finding this bug; it has a more comprehensive list of all the ways in which GlobalBlock does not fit in the normal block system.

Tchanders set the point value for this task to 3.Aug 30 2022, 5:42 PM

Thanks for looking into this @TheVoidwalker. Haven't given this much thought yet, but would it help to add the global block-specific message into the block reason instead?

Ah, thanks for the suggestion, that's exactly what I've wound up doing.

Looking at this again, I think this would be a more robust way: T317308: GlobalBlocking should specify block error message key. I've made that a subtask of this, so we can handle the error reporting separately, and focus on using getUserBlock in this task.

I think this needs to be blocked on T317190: GlobalBlock should extend AbstractBlock instead of DatabaseBlock, otherwise callers of User::getBlock() etc. would receive block objects which behave in very unexpected ways.

I don't really have the time to work on this. Feel free to take over for now, otherwise I might have more availability at some undefined later point in time.

Change 843967 had a related patch set uploaded (by Tchanders; author: Tchanders):

[mediawiki/extensions/GlobalBlocking@master] Handle GetUserBlock hook, instead of checking for global blocks separately

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

Change 655801 abandoned by Voidwalker:

[mediawiki/extensions/GlobalBlocking@master] Apply GlobalBlocks through GetUserBlock hook

Reason:

In favor of 843967

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

Testing notes

Create a DBblock on the user,

  • Then globallyBlock the IP they are using.(make sure that the user is not ipblock-exempt/globalblock-exempt I think this means they are not sysops @Tchanders ?)
  • I then went to Special:Upload(or any special page that the user has access to)
  • I then see the message blockedtext-composite-reason(see screenshot below)

message.png (730×1 px, 220 KB)

Change 843967 merged by jenkins-bot:

[mediawiki/extensions/GlobalBlocking@master] Handle GetUserBlock hook, instead of checking for global blocks separately

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

@Tchanders If I am globally blocked and try to edit, I see two block messages:

  • globalblocking-ipblocked (or wikimedia-globalblocking-ipblocked if you have WikimediaMessages installed)
  • globalblocking-blockedtext-ip

I am seeing this on beta and locally.

two_block_messages.png (439×1 px, 96 KB)

(make sure that the user is not ipblock-exempt/globalblock-exempt I think this means they are not sysops @Tchanders ?)

These exemptions can be given to any group, but they are given by default to sysops.

@Tchanders If I am globally blocked and try to edit, I see two block messages:

  • globalblocking-ipblocked (or wikimedia-globalblocking-ipblocked if you have WikimediaMessages installed)
  • globalblocking-blockedtext-ip

I am seeing this on beta and locally.

two_block_messages.png (439×1 px, 96 KB)

This is because one message is coming from the block check, and the other is coming from a separate permissions check. Before this patch, the block check (for editing) wouldn't check global blocks, so you'd see one message if you only had a global block, but two messages if you had a local and a global block.

It doesn't make sense to see both messages any more though. I've filed a follow-up task: T322941: Remove GlobalBlockingHooks::onGetUserPermissionsErrorsExpensive in favour of GlobalBlockingHooks::onGetUserBlock

I have tested global blocks individually (IP, Range and XFF) and in combination with local/database and system blocks for a sample of actions[1].

For an individual global block, the actions blocked from my sample were the same compared to an older version without these changes[2]. It blocked everything from my sample of actions except email.

When combined with other types of block the global blocks appear to always[3] be included in the composite blocks (according to the IDs in the block message) and the appropriate restrictions applied.

I checked the accuracy of the expiry date, blocker name, IP/range, and ID of the global block (when it is part of a composite block).

As an aside, I have not been able to find a way to search for global blocks by their IDs (unless you have DB access), so perhaps listing their IDs in composite blocks is not that useful. Perhaps a phab task for another time.

Test environment:

Notes
  1. Editing, moving and deleting pages, Special:Upload, Special:PasswordReset, Special:Thanks, Thanks via API, Special:EmailUser, Special:CreateAccount and reverting an image.
  2. GlobalBlocking commit 691844350c4e7fc02c676f62307eaf6c967405d0. MediaWiki core commit b9b1bcc5bca767111f6fe1878d1f4895e265165c.
  3. With one exception when you have an autoblock and a global block and use Special:CreateAccount. I am investigating. Nevertheless, the user is still correctly blocked from account creation in this circumstance.

It doesn't make sense to see both messages any more though. I've filed a follow-up task: T322941: Remove GlobalBlockingHooks::onGetUserPermissionsErrorsExpensive in favour of GlobalBlockingHooks::onGetUserBlock

Thanks.

Change 861421 had a related patch set uploaded (by Tchanders; author: Tchanders):

[mediawiki/extensions/GlobalBlocking@master] Remove GlobalBlockingHooks::onGetUserPermissionsErrorsExpensive

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

Change 861421 had a related patch set uploaded (by Tchanders; author: Tchanders):

[mediawiki/extensions/GlobalBlocking@master] Remove GlobalBlockingHooks::onGetUserPermissionsErrorsExpensive

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

Wrong task tagged - patch has been updated.