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

Assigned To

Authored By

	Tchanders
	Jul 1 2019, 3:39 PM

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.)

Details

Subject	Repo	Branch	Lines +/-
Pass the target as a string to the reason Message for autoblocks	mediawiki/core	wmf/1.35.0-wmf.3	+1 -1
Pass the target as a string to the reason Message for autoblocks	mediawiki/core	master	+1 -1
Store block reasons as CommentStoreComments in block classes	mediawiki/core	master	+167 -81

Customize query in gerrit

Related Objects

Mentioned In: T234406: Pass the relevant and necessary context information to UserBlockedError
T227901: PHP warning on attempting to set a block cookie after CentralAuth auto login
T227892: [BUG] InvalidArgumentException "Blocker must be a local user or a name that cannot be a local user"
T227174: Improve the consistency of block error notices
T226777: BlockManager->trackBlockWithCookie triggers "Recursion detected in RequestContext::getLanguage" warning
T227005: Don't require that the blocker be a User for a SystemBlock
Mentioned Here: T236286: Uncaught Error: Widget not found when editing filters
T227174: Improve the consistency of block error notices
rMWc35f3851e907: Localisation updates from https://translatewiki.net.
T180050: PHP Warning: Recursion detected in RequestContext
T226777: BlockManager->trackBlockWithCookie triggers "Recursion detected in RequestContext::getLanguage" warning
T227005: Don't require that the blocker be a User for a SystemBlock

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

dbarratt added a project: MediaWiki-User-management.Jul 1 2019, 3:50 PM

dbarratt moved this task from Backlog to User blocking on the MediaWiki-User-management board.

Tchanders mentioned this in T227005: Don't require that the blocker be a User for a SystemBlock.Jul 1 2019, 4:37 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:

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.
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.
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.

Tchanders mentioned this in T226777: BlockManager->trackBlockWithCookie triggers "Recursion detected in RequestContext::getLanguage" warning.Jul 3 2019, 5:46 AM

Tchanders mentioned this in T227174: Improve the consistency of block error notices.Jul 3 2019, 8:51 AM

Niharika moved this task from Untriaged to Triage/To be Estimated on the Anti-Harassment board.Jul 11 2019, 6:34 PM

Tchanders mentioned this in T227892: [BUG] InvalidArgumentException "Blocker must be a local user or a name that cannot be a local user".Jul 12 2019, 5:30 PM

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

gerritbot added a project: Patch-For-Review.Sep 22 2019, 9:32 PM

Tchanders claimed this task.Sep 23 2019, 9:41 AM

Tchanders moved this task from Triage/To be Estimated to The Letter Song on the Anti-Harassment board.

Tchanders edited projects, added Anti-Harassment (The Letter Song); removed Anti-Harassment.

Tchanders moved this task from Ready 🎬 (ONLY IF YOU HAVE NO MORE CODE TO REVIEW) to Code Review 🔍 on the Anti-Harassment (The Letter Song) board.

Anomie added a project: Platform Team Workboards (Clinic Duty Team).Sep 23 2019, 2:56 PM

Anomie moved this task from Inbox to External Code Review In Progress on the Platform Team Workboards (Clinic Duty Team) board.

Tchanders mentioned this in T227901: PHP warning on attempting to set a block cookie after CentralAuth auto login.Oct 8 2019, 8:41 PM

dbarratt moved this task from Code Review 🔍 to QA/Testing 🐞 on the Anti-Harassment (The Letter Song) board.Oct 18 2019, 9:34 PM

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

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

ReleaseTaggerBot added a project: MW-1.35-notes (1.35.0-wmf.3; 2019-10-22).Oct 18 2019, 11:00 PM

Maintenance_bot removed a project: Patch-For-Review.Oct 18 2019, 11:10 PM

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.

dbarratt moved this task from QA/Testing 🐞 to In Progress 💪 on the Anti-Harassment (The Letter Song) board.Oct 19 2019, 7:16 PM

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.

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();

In T227007#5588988, @Reedy wrote:

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 moved this task from In Progress 💪 to QA/Testing 🐞 on the Anti-Harassment (The Letter Song) board.Oct 20 2019, 10:42 AM

@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:&quot;mId&quot;;i:3;s:5:&quot;mName&quot;;s:3:&quot;Eve&quot;;s:9:&quot;mRealName&quot;;s:0:&quot;&quot;;s:6:&quot;mEmail&quot;;s:13:&quot;eve@localhost&quot;;s:8:&quot;mTouched&quot;;s:14:&quot;20191009132845&quot;;s:19:&quot;mEmailAuthenticated&quot;;s:14:&quot;20191007155126&quot;;s:14:&quot;mOptionsLoaded&quot;;b:0;s:5:&quot;mFrom&quot;;s:2:&quot;id&quot;;s:10:&quot;mBlockedby&quot;;i:-1;s:9:&quot;mHideName&quot;;N;s:8:&quot;mOptions&quot;;N;s:6:&quot;mBlock&quot;;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

gerritbot added a project: Patch-For-Review.Oct 22 2019, 3:53 PM

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

Maintenance_bot removed a project: Patch-For-Review.Oct 22 2019, 6:10 PM

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

gerritbot added a project: Patch-For-Review.Oct 22 2019, 6:53 PM

ReleaseTaggerBot edited projects, added MW-1.35-notes (1.35.0-wmf.4; 2019-10-29); removed MW-1.35-notes (1.35.0-wmf.3; 2019-10-22).Oct 22 2019, 10:00 PM

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

Maintenance_bot removed a project: Patch-For-Review.Oct 23 2019, 12:10 AM

ReleaseTaggerBot edited projects, added MW-1.35-notes (1.35.0-wmf.3; 2019-10-22); removed MW-1.35-notes (1.35.0-wmf.4; 2019-10-29).Oct 23 2019, 3:00 AM

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.

In T227007#5594595, @dom_walden wrote:

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]]"]} |
+------------+--------------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------+--------------------------------------------------------------------------------------------------+

In T227007#5598774, @dom_walden wrote:

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).

In T227007#5598814, @Daimona wrote:

In T227007#5598774, @dom_walden wrote:

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).

abusefilter_noaction.png (937×1 px, 24 KB)

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.

dom_walden mentioned this in T234406: Pass the relevant and necessary context information to UserBlockedError.Oct 23 2019, 2:05 PM

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

In T227007#5598873, @dom_walden wrote:

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

Ah, that's a recent regression, T236286.

Reasons and blockers for SystemBlocks should not be stored as translated messagesClosed, ResolvedPublicActions

Description

Details

Related Objects

Event Timeline

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