Page MenuHomePhabricator

Block message not shown when a temporary account is blocked on mobile
Closed, ResolvedPublic

Description

Steps to reproduce
  1. Block an IP address
  2. On mobile, try to edit (logged out) from the IP address, with temp accounts enabled

Expected: See a block message
Actual: Progress to the editor; on saving an edit, a block error message is shown

(The same behaviour occurs if you block the temp account directly.)

Notes

At first I thought this was similar to T227167: Block notice is not shown when editing from mobile for composite blocks, in which case blockedtext-tempuser needed to be registered with ApiBase::BLOCK_CODE_MAP. However, doing this in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/999075 didn't solve the problem.

It also happens when a temporary user is partially blocked, in which case they get the same message as a registered user, so it doesn't seem to be related to the message key. (Registered users see the block message correctly.)

Event Timeline

Tchanders renamed this task from Show block message on to Block message not shown when a temporary account is blocked on mobile.

@Tchanders let me know if you need help from the web team. Although it doesn't look like any of the related code here is maintained by us, happy to find you any help you might need to support you on this one!

Thanks @Jdlrobson

I assumed it was to do with the workflow where the temp user notice is shown, which I don't believe we added:

image.png (270×940 px, 27 KB)

We'll can look further into it when we have some time, unless you get there first!

Got it! I hadn't understood this was about MobileFrontend. This code is dual-maintained by web and editing team (it's one of our gray areas of ownership :-)).

We already have a component for BlockMessageDetails at https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/master/src/mobile.editor.overlay/BlockMessageDetails.js so I think if pass block details to createAnonWarning in https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/master/src/mobile.editor.overlay/EditorOverlayBase.js#L576 it should be straightforward to render something here.

My assumption was we opened a BlockMessageDrawer though when we detected someone was blocked and never showed the editor:
https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blame/master/src/mobile.editor.overlay/blockMessageDrawer.js
Perhaps there is something wrong in the logic here?:
https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/0c650ed156e89857143be6d9bf8e45909e2c6eac/src/mobile.init/editor.js#L408

I reproduced this and I'd like to note that this bug seems only to take effect when using the standard editor, and not VisualEditor.

Change #1076033 had a related patch set uploaded (by Amdrel; author: Amdrel):

[mediawiki/core@master] ApiQueryInfo: Return blockinfo for anon users when temp accounts are enabled

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

Attached is a patch for core that fixes this issue. The issue I found was while querying page information and temporary user auto-creation is enabled, a placeholder user is created to test permissions against instead as a temporary user may have the ability to edit a page while an anonymous user cannot. However, blocks are checked against this placeholder as well, and since that placeholder user isn't associated with the request, IP blocks, XFF, and cookie blocks get skipped.

kostajh subscribed.

The bug reported in the task description doesn't seem especially urgent, but the potential misreading of blocks in ApiQueryInfo for new temp accounts potentially has larger impacts, so would be good for us to pick up this task and complete the patch in the next sprint.

Attached is a patch for core that fixes this issue. The issue I found was while querying page information and temporary user auto-creation is enabled, a placeholder user is created to test permissions against instead as a temporary user may have the ability to edit a page while an anonymous user cannot. However, blocks are checked against this placeholder as well, and since that placeholder user isn't associated with the request, IP blocks, XFF, and cookie blocks get skipped.

There's a related task about whether the placeholder is still necessary: T355210: Investigate: Do we still need to use placeholder temp user for edit permission checks?.

A lot has changed since that task was filed, so we should look into it again.

A few ideas to look into:

  • Pass the IP user to addBlockInfoToStatus from ApiQueryInfo
  • In ApiBase::addBlockInfoToStatus ensure that the IP block is checked for a placeholder temp user
  • In BlockManager::getBlock, ensure that a the IP block is checked for a placeholder temp user (or for any temp user?)

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

[mediawiki/core@master] WIP ApiQueryInfo: Check for blocks against the requesting user

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

More about block checking

  • BlockManager::getBlock accepts the request as an optional parameter. If passed, blocks are checked against the request's IP, XFF, cookies, etc.
  • User::getBlock passes the global request to BlockManager if the user it is checking is the global session user.
  • In this task's scenario, ApiQueryInfo makes a temporary account placeholder user and calls User::getBlock on it (via some Authority methods). It's not the global session user, so BlockManager only checks for a block against the user's name (and doesn't find one).

More about temp account placeholder

  • The placeholder is only needed when checking rights in order to allow the following configs both to be set for an autocreate action (see AutoCreateTempUser config docs):
    • $wgGroupPermissions['*']['someAction'] = false;
    • $wgGroupPermissions['temp']['someAction'] = true;
  • We advise against doing this. We advise setting $wgGroupPermissions['*']['someAction'] = true, in which case the placeholder shouldn't be needed (temp accounts docs). But that should be considered in T355210.

Against this context, here are some further thoughts on the approaches suggested above:

A few ideas to look into:

  • Pass the IP user to addBlockInfoToStatus from ApiQueryInfo
  • In ApiBase::addBlockInfoToStatus ensure that the IP block is checked for a placeholder temp user

These are similar ideas, and which place is best is a matter of software architecture.

We could either (1) stop using the placeholder temporary user entirely, or (2) check for blocks against the global user, and check for rights on the temporary account placeholder.

(2) is a quick, safe fix that we could do now and is implemented in https://gerrit.wikimedia.org/r/1114789. We could think further about (1) in T355210.

  • In BlockManager::getBlock, ensure that a the IP block is checked for a placeholder temp user (or for any temp user?)

We shouldn't check the IP block for all temporary users, because there are times when we want to know if the temporary user themselves are blocked. (E.g. if visiting a temporary user's user page, to work out whether to show a "user blocked" message at the top.) This is why User::getBlock works out whether the user being checked is the global session user or not. In practice, we might be safe to check the IP block whenever we have a placeholder temporary account, but adding a special case for this would make BlockManager/User more complicated.

Change #1114789 merged by jenkins-bot:

[mediawiki/core@master] ApiQueryInfo: Check for blocks against the requesting user

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

I've been working on some refactoring of PermissionManager: https://gerrit.wikimedia.org/r/1054957, I got a merge conflict with https://gerrit.wikimedia.org/r/1114789, and I noticed that the refactoring apparently fixes this bug (the test you added still passes when I undo the workaround). I think this is because it would deliver the relevant Block object inside the PermissionStatus object, instead of requiring you to look it up based on the relevant user, which may not be the same as the blocked user. Code review welcome.

Change #1054957 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@master] Generate machine-readable block info in BlockErrorFormatter

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

Change #1054957 merged by jenkins-bot:

[mediawiki/core@master] Generate machine-readable block info in BlockErrorFormatter

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

Change #1076033 abandoned by Bartosz Dziewoński:

[mediawiki/core@master] ApiQueryInfo: Return blockinfo for anon users when temp accounts are enabled

Reason:

Superseded by other changes – https://gerrit.wikimedia.org/r/c/1054957 fixed the same bug, https://gerrit.wikimedia.org/r/c/1114789 adds equivalent tests

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

@matmarex The block messages don't always appear correctly for me on enwiki beta.

When my IP is blocked and I am logged out and not a temporary account. Reason is cut off and I don't know if the "See more" link is in the correct place:

mobile_block_temp_ip.png (404×1 px, 51 KB)

Clicking the "See more" link just shows a blank message:

mobile_block_temp_ip_see_more.png (429×1 px, 39 KB)

Flipping the phone:

mobile_block_temp_ip_flip.png (464×871 px, 50 KB)

@dom_walden That looks to me like a problem with the MobileFrontend interface, rather than the backend changes that @Tchanders and I have made. Does that also happen for normal blocks of registered users? Maybe it should be reported as a separate bug.

@dom_walden That looks to me like a problem with the MobileFrontend interface, rather than the backend changes that @Tchanders and I have made. Does that also happen for normal blocks of registered users? Maybe it should be reported as a separate bug.

I agree - will wait to hear from @dom_walden.

Thanks @matmarex for the much better fix!

@dom_walden That looks to me like a problem with the MobileFrontend interface, rather than the backend changes that @Tchanders and I have made. Does that also happen for normal blocks of registered users? Maybe it should be reported as a separate bug.

Yes, it does. I raised T387119.