Page MenuHomePhabricator

Reasons and blockers for SystemBlocks should not be stored as translated messages
Closed, ResolvedPublic

Description

A SystemBlock (also a CompositeBlock) is created temporarily on enforcement, and is not stored in the database. These blocks are created in BlockManager::getUserBlock.

The block reason for this type of block is a message, which is translated in BlockManager::getUserBlock at the point when the block is created. If Block::getUserBlock is called before the user is initialized (e.g. T180050, T226777), the message is translated into the site language rather than the user's language, and a recursion warning is logged.

The block reason could instead be stored as a message key, and translated when it is needed.

(For some types of system block the same applies to the blocker, but the solution is a little more complicated - see T227005.)

Event Timeline

Tchanders created this task.Jul 1 2019, 3:39 PM
Restricted Application added subscribers: MGChecker, Aklapper. · View Herald TranscriptJul 1 2019, 3:39 PM

From T226777#5293474:

  • As a slightly less short-term workaround, one might be able to defer the formatting of the messages separately from the Block objects. For example, by accepting a reason-msg option, which would hold a message key or Message object. This would hide the issue similar to the above, by deferring the look up of the user language and the fetching/formatting of the message until it is displayed, assuming that by that time, the user object is initialised.

It makes a lot of sense to set the message key as a property; the question is then when to translate it? A few possibilities:

  1. Set the $reason property to the message key, and translate it in SystemBlock::getReason and CompositeBlock::getReason. Nothing external to the block classes need change. The block objects would be further entangled with message formatting, and these methods should not be called before the user object is initialized.
  2. Translate the reason at the point an error message is displayed, if the block is a SystemBlock or a CompositeBlock. Callers would need to know to do this check when getting block reasons.
  3. Add a layer of formatting for block details (perhaps a block info formatter service), which should be used on getting block errors, instead of getting block parameters directly from the block. This would translate the reason message where appropriate, and could do other things like formatting expiry times and bidi isolating user names. Callers would need to know to use this instead of getting block params directly.

Change 538448 had a related patch set uploaded (by Tchanders; owner: Tchanders):
[mediawiki/core@master] Allow block reasons to be stored on blocks as message keys

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

Change 538448 merged by jenkins-bot:
[mediawiki/core@master] Store block reasons as CommentStoreComments in block classes

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

Note that I had to temporarily revert the patch above. It made AbstractBlock::mReason private without hard deprecation, and lots of extensions are accessing it directly. I apologize if this could seem an overkill, but better reverted than sorry. It can be re-applied without making the property private if it works for you.

Also, extensions will have to be updated to use getters/setters. I can try to give it a shot tomorrow-ish.

A reminder for something that puzzled me for a couple of minutes: tests where DatabaseBlock::mReason (note: NOT AbstractBlock::mReason) is accessed directly are likely not failing because a dynamic property is created instead. Nevertheless, this is not a good reason to make the property private, given the breaking change.

Reedy added a subscriber: Reedy.Oct 20 2019, 12:13 AM

While I concur it shouldn't have been made private without updating callers first, I don't consider ~7 extensions "lots" ;)

Field
    mReason
Found usages  (12 usages found)
    Value read  (2 usages found)
        mediawiki  (2 usages found)
            extensions/EventBus/includes  (2 usages found)
                EventFactory.php  (2 usages found)
                    EventFactory  (2 usages found)
                        createUserBlockChangeEvent  (2 usages found)
                            827 if ( !is_null( $block->mReason ) ) {
                            828 $attrs['comment'] = $block->mReason;
    Value write  (10 usages found)
        mediawiki  (10 usages found)
            extensions/AbuseFilter/includes  (1 usage found)
                AbuseFilterRunner.php  (1 usage found)
                    AbuseFilterRunner  (1 usage found)
                        doAbuseFilterBlock  (1 usage found)
                            1075 $block->mReason = $reason;
            extensions/BlueSpiceUserManager/src  (1 usage found)
                Extension.php  (1 usage found)
                    Extension  (1 usage found)
                        disableUser  (1 usage found)
                            311 $block->mReason = \wfMessage( 'bs-usermanager-log-user-disabled',
            extensions/CheckUser/includes/specials  (1 usage found)
                SpecialCheckUser.php  (1 usage found)
                    SpecialCheckUser  (1 usage found)
                        doMassUserBlockInternal  (1 usage found)
                            334 $block->mReason = $blockParams['reason'];
            extensions/DisableAccount/maintenance  (2 usages found)
                blockDisabledAccounts.php  (2 usages found)
                    BlockDisabledAccounts  (2 usages found)
                        doBlockAndLog  (2 usages found)
                            98 $block->mReason = $reason;
                            100 $block->mReason = $wgContLang->truncateForDatabase( $reason, 255 );
            extensions/GlobalBlocking/includes/specials  (1 usage found)
                SpecialGlobalBlock.php  (1 usage found)
                    SpecialGlobalBlock  (1 usage found)
                        onSubmit  (1 usage found)
                            236 $block->mReason = $data['Reason'][0];
            extensions/RegexBlock/includes  (4 usages found)
                RegexBlockCore.php  (4 usages found)
                    RegexBlock  (4 usages found)
                        setUserData  (4 usages found)
                            503 $user->getBlock()->mReason = $valid['reason'];
                            509 $user->getBlock()->mReason = wfMessage( 'regexblock-reason-regex', $wgContactLink )->text();
                            512 $user->getBlock()->mReason = wfMessage( 'regexblock-reason-ip', $wgContactLink )->text();
                            515 $user->getBlock()->mReason = wfMessage( 'regexblock-reason-name', $wgContactLink )->text();

While I concur it shouldn't have been made private without updating callers first, I don't consider ~7 extensions "lots" ;)

Everything is relative :) What I meant is, too many extensions to just push patches to any of them right now and hope that they land before the next wmf. branch is cut.

@Tchanders On beta (1.35.0-alpha (c35f385) 06:57, 21 October 2019), attempting to create a block (happens on submit):

[Xa17H6wQBHcAAEKCoswAAAAQ] /wiki/Special:Block Error from line 827 of /srv/mediawiki/php-master/extensions/EventBus/includes/EventFactory.php: Cannot access protected property MediaWiki\Block\DatabaseBlock::$mReason

Backtrace:

#0 /srv/mediawiki/php-master/extensions/EventBus/includes/EventBusHooks.php(396): EventFactory->createUserBlockChangeEvent(string, User, MediaWiki\Block\DatabaseBlock, NULL)
#1 /srv/mediawiki/php-master/includes/Hooks.php(174): EventBusHooks::onBlockIpComplete(MediaWiki\Block\DatabaseBlock, User, NULL)
#2 /srv/mediawiki/php-master/includes/Hooks.php(202): Hooks::callHook(string, array, array, NULL)
#3 /srv/mediawiki/php-master/includes/specials/SpecialBlock.php(1002): Hooks::run(string, array)
#4 /srv/mediawiki/php-master/includes/specials/SpecialBlock.php(1254): SpecialBlock::processForm(array, RequestContext)
#5 /srv/mediawiki/php-master/includes/htmlform/HTMLForm.php(694): SpecialBlock->onSubmit(array, OOUIHTMLForm)
#6 /srv/mediawiki/php-master/includes/htmlform/HTMLForm.php(586): HTMLForm->trySubmit()
#7 /srv/mediawiki/php-master/includes/htmlform/HTMLForm.php(601): HTMLForm->tryAuthorizedSubmit()
#8 /srv/mediawiki/php-master/includes/specialpage/FormSpecialPage.php(187): HTMLForm->show()
#9 /srv/mediawiki/php-master/includes/specialpage/SpecialPage.php(575): FormSpecialPage->execute(NULL)
#10 /srv/mediawiki/php-master/includes/specialpage/SpecialPageFactory.php(607): SpecialPage->run(NULL)
#11 /srv/mediawiki/php-master/includes/MediaWiki.php(298): MediaWiki\Special\SpecialPageFactory->executePath(Title, RequestContext)
#12 /srv/mediawiki/php-master/includes/MediaWiki.php(967): MediaWiki->performRequest()
#13 /srv/mediawiki/php-master/includes/MediaWiki.php(530): MediaWiki->main()
#14 /srv/mediawiki/php-master/index.php(44): MediaWiki->run()
#15 /srv/mediawiki/w/index.php(3): require(string)
#16 {main}

There's already https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/EventBus/+/544332/, I was waiting for CI to finish before giving a final approve.

@Tchanders On beta, with an autoblock like so:

when doing something anonymously at the autoblocked IP address, the block message is like:
Autoblocked because your IP address was recently used by "[[User:[INVALID]|[INVALID]]]". The reason given for [INVALID]'s block is: "Disruptive editing".

Locally, I see a log error:

[Bug58676] Invalid parameter for message "autoblocker": a:12:{s:3:"mId";i:3;s:5:"mName";s:3:"Eve";s:9:"mRealName";s:0:"";s:6:"mEmail";s:13:"eve@localhost";s:8:"mTouched";s:14:"20191009132845";s:19:"mEmailAuthenticated";s:14:"20191007155126";s:14:"mOptionsLoaded";b:0;s:5:"mFrom";s:2:"id";s:10:"mBlockedby";i:-1;s:9:"mHideName";N;s:8:"mOptions";N;s:6:"mBlock";N;}
#0 /var/www/html/core/includes/language/Message.php(1143): Message->extractParam(string, string)
#1 /var/www/html/core/includes/language/Message.php(875): Message->replaceParameters(string, string, string)
#2 /var/www/html/core/includes/language/Message.php(956): Message->toString(string)
#3 /var/www/html/core/includes/block/DatabaseBlock.php(988): Message->plain()
#4 /var/www/html/core/includes/user/User.php(1753): MediaWiki\Block\DatabaseBlock->getReason()
#5 /var/www/html/core/includes/user/User.php(4222): User->getBlockedStatus()
#6 /var/www/html/core/includes/auth/AuthManager.php(1009): User->isBlockedFromCreateAccount()
#7 /var/www/html/core/includes/specials/SpecialCreateAccount.php(69): MediaWiki\Auth\AuthManager->checkAccountCreatePermissions(User)
#8 /var/www/html/core/includes/specialpage/LoginSignupSpecialPage.php(234): SpecialCreateAccount->checkPermissions()
#9 /var/www/html/core/includes/specialpage/SpecialPage.php(575): LoginSignupSpecialPage->execute(NULL)
#10 /var/www/html/core/includes/specialpage/SpecialPageFactory.php(607): SpecialPage->run(NULL)
#11 /var/www/html/core/includes/MediaWiki.php(298): MediaWiki\Special\SpecialPageFactory->executePath(Title, RequestContext)
#12 /var/www/html/core/includes/MediaWiki.php(967): MediaWiki->performRequest()
#13 /var/www/html/core/includes/MediaWiki.php(530): MediaWiki->main()
#14 /var/www/html/core/index.php(44): MediaWiki->run()
#15 {main}

In the comment table in the DB, the comment's row looks like:

+------------+--------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| comment_id | comment_hash | comment_text                                                                                                                                                                                    | comment_data                                                                                                                                                                                                                                                                                                                                  |
+------------+--------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
|      80980 |    -73146175 | [[Wikipedia:Autoblock|Autoblocked]] because your IP address was recently used by "[[User:Drwpb|Drwpb]]". The reason given for Drwpb's block is: "[[WP:Disruptive editing|Disruptive editing]]". | {"_null":true,"_message":["autoblocker",{"mId":16190,"mName":"Drwpb","mRealName":"","mEmail":"domwalden4@gmail.com","mTouched":"20191022093936","mEmailAuthenticated":"20190320184416","mOptionsLoaded":false,"mFrom":"name","mBlockedby":-1,"mHideName":null,"mOptions":null,"mBlock":null},"[[WP:Disruptive editing|Disruptive editing]]"]} |
+------------+--------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+

Change 545311 had a related patch set uploaded (by Tchanders; owner: Tchanders):
[mediawiki/core@master] Pass the target as a string to the reason Message for autoblocks

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

Change 545311 merged by jenkins-bot:
[mediawiki/core@master] Pass the target as a string to the reason Message for autoblocks

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

Change 545373 had a related patch set uploaded (by Tchanders; owner: Tchanders):
[mediawiki/core@wmf/1.35.0-wmf.3] Pass the target as a string to the reason Message for autoblocks

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

Change 545373 merged by jenkins-bot:
[mediawiki/core@wmf/1.35.0-wmf.3] Pass the target as a string to the reason Message for autoblocks

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

We have made a change to how we get and set block reasons. Therefore, I tried to test as many places where blocks get created or displayed, comparing beta with test (before the train, that is) or on my local environment (where I don't have permission on beta).

Testing creating database blocks and that the block reason is stored correctly in the database:

  • Special:Block (including autoblocks; bug in T227007#5594595 now fixed)
  • Creating blocks via API
  • Database blocks created by extensions:
    • CentralAuth
    • GlobalBlocking
    • CheckUser (mass block creation)

Display of block reasons (including creating system and composite blocks, which happens when the user's block is checked):

  • Special:Block (when viewing a preexisting block; also when testing for equality with a preexisting block)
  • Edit, Upload, CreateAccount, EmailUser, PasswordReset, etc.
  • For most of the places changed by https://gerrit.wikimedia.org/r/c/mediawiki/core/+/538452, e.g. action=editchangetags, action=credits, Special:BotPasswords
  • When API editing
  • Viewing current user's block: ?action=query&meta=userinfo&uiprop=blockinfo (tests ApiBlockInfoTrait)
  • Testing if user can be created: ?action=query&list=users&ususers=Drwpb5&usprop=cancreate (tests SecondaryAuthenticationProvider)

For database blocks (including those created by the extensions listed above), autoblocks, composite blocks and system blocks (including those created by the extensions RegexBlock and TorBlock).

This included testing that the block reason is displayed correctly when using a non-default language (for blocks which have translatable reasons, i.e. composite and system blocks).

Also tested updating a preexisting block via Special:Block, and that the block reasons were/were not changed as appropriate in the database.

I tested most of the extensions we updated. Note that I was unable to get the block action working for the AbuseFilter extension. Nor did I systematically test the EventBus extension, although the fact that T227007#5590836 no longer occurs suggests it is fixed.

No longer occurs on test, the block message displays fine.

I no longer see the error on my local environment.

The row in the comment table for the autoblock now looks like:

+------------+--------------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------+--------------------------------------------------------------------------------------------------+
| comment_id | comment_hash | comment_text                                                                                                                                                               | comment_data                                                                                     |
+------------+--------------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------+--------------------------------------------------------------------------------------------------+
|     155634 |    422570251 | Autoblocked because your IP address has been recently used by "[[User:Drwpb|Drwpb]]".
The reason given for Drwpb's block is "[[WP:Disruptive editing|Disruptive editing]]" | {"_null":true,"_message":["autoblocker","Drwpb","[[WP:Disruptive editing|Disruptive editing]]"]} |
+------------+--------------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------+--------------------------------------------------------------------------------------------------+

Note that I was unable to get the block action working for the AbuseFilter extension.

Happy to help! What exactly didn't work?

It should be noted that in block messages now the name of the target of the block and the blocker are surrounded by \u202a and \u202c, which I believe are for bidirectional support (e.g. see https://en.wikipedia.org/wiki/Unicode_control_characters#Bidirectional_text_control and https://en.wikipedia.org/wiki/Bi-directional_text#Table_of_possible_BiDi_character_types).

These are non-printing characters when viewed in HTML, but do appear literally in some API requests. For example, if your IP is blocked and you go to https://test.wikipedia.org/w/api.php?action=query&list=users&ususers=Drwpb5&usprop=cancreate, the output looks something like:

{
    "batchcomplete": "",
    "query": {
        "users": [
            {
                "name": "Drwpb5",
                "missing": "",
                "cancreateerror": [
                    {
                        "message": "cantcreateaccount-text",
                        "params": [
                            "\u202a2001:8B0:7ACF:5D4E:22CF:30FF:FE4E:1D46\u202c",
                            "[[WP:Disruptive editing|Disruptive editing]]",
                            "\u202aDom walden\u202c"
                        ],
                        "code": "cantcreateaccount-text",
                        "type": "error"
                    }
                ]
            }
        ]
    }
}

I think this behaviour was introduced more generally by the BlockErrorFormatter service in T227174 (which I did not notice at the time). I think this task has added it for a couple of new places (including Special:CreateAccount).

Note that I was unable to get the block action working for the AbuseFilter extension.

Happy to help! What exactly didn't work?

Thanks! So, I am attempting to get AbuseFilter to block a user when a particular condition is met.

However, when creating a filter, the actions section is just blank (see below).

My current setup:

$wgAbuseFilterAvailableActions = [ 'block' ];
$wgGroupPermissions['sysop']['abusefilter-modify'] = true;
$wgGroupPermissions['*']['abusefilter-log-detail'] = true;
$wgGroupPermissions['*']['abusefilter-view'] = true;
$wgGroupPermissions['*']['abusefilter-log'] = true;
$wgGroupPermissions['sysop']['abusefilter-privatedetails'] = true;
$wgGroupPermissions['sysop']['abusefilter-modify-restricted'] = true;
$wgGroupPermissions['sysop']['abusefilter-revert'] = true;

My user is a sysop.

dbarratt closed this task as Resolved.Oct 23 2019, 3:14 PM

However, when creating a filter, the actions section is just blank (see below).

Ah, that's a recent regression, T236286.