Page MenuHomePhabricator

PermissionManager::isBlockedFrom() can return true even if the user does not have a block
Closed, ResolvedPublic

Description

Problem
Calling PermissionManager::isBlockedFrom() implies that the user 1) has a block and 2) the block applies to the Title given. However, this is not necessarily the case.

UserIsBlockedFrom allows an extension to modify the result of PermissionManager::isBlockedFrom(). Unfortunately, this method hook is called regardless if the user has a block or not.

Likewise, UserIsHidden allows an extension to modify the "hidden" status of a user without actually having a block attached to the user.

Proposed Solution
Since PermissionManager::isBlockedFrom() is mainly called from PermissionManager::checkUserBlock() and that method does not call the aforementioned method unless the user has a block, it is safe (in the majority of instances) to only call this hook if the user has a block, thereby guaranteeing that a true result also guarantees that the user has a block.

The UserIsHidden hook doesn't actually make sense anymore. Extensions have the ability to add blocks to users using GetBlockedStatus. Therefore, the former hook should be deprecated in favor of the later. During the deprecation period, a SystemBlock should be generated when the result of the hook is true.

Details

Related Gerrit Patches:

Event Timeline

Restricted Application added subscribers: MGChecker, Aklapper. · View Herald TranscriptJul 24 2019, 11:31 PM

Change 525305 had a related patch set uploaded (by Dbarratt; owner: Tchanders):
[mediawiki/core@master] WIP example of SystemBlock for hidden global account

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

Tchanders moved this task from Untriaged to The Letter Song on the Anti-Harassment board.
Tchanders moved this task from Ready to In Progress on the Anti-Harassment (The Letter Song) board.

@dmaza & @Mooeypoo Today @Tchanders and I were talking about this task, and we thought that perhaps it would be better to deprecate the GetBlockedStatus hook (See: T229035). Basically, while GetBlockedStatus technically works it's not ideal. Primarily because it allows an extension to mutate the entire User object rather than just the block. Also, it conflates adding blocks with altering (changing or removing blocks).

I'm wondering if it would be better to add one (or ideally two?) new hooks to BlockManager::getUserBlock(). The first would allow an extension to add blocks to the list of blocks before a potential CompositeBlock is generated and a second that would take the resulting block (either single or composite) and allow an extension to modify (or destroy) it. This would prevent an extension from mutating the User object.

Change 525305 merged by jenkins-bot:
[mediawiki/core@master] Ensure block hooks keep user state consistent with realistic blocks

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

dom_walden added a subscriber: dom_walden.

Regression (testing changes made to AbstractBlock and DatabaseBlock):

  • Can create a hidden block, gets applied to user correctly when editing an article.

PermissionManager::isBlockedFrom:

  • That block information is displayed about $user when accessing User:$user or User_talk:$user (Article::showMissingArticle)
  • Editing pages

BlockManager::getUserBlock

  • Already tested incidentally above (e.g. editing while blocked).

Other testing of this change has been done (incidentally) as part of T229692 and T228950.

dbarratt closed this task as Resolved.Sep 5 2019, 2:47 PM

Change 535230 had a related patch set uploaded (by Tchanders; owner: Tchanders):
[mediawiki/core@master] Deprecate the UserIsHidden hook

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

Change 535230 merged by jenkins-bot:
[mediawiki/core@master] Deprecate the UserIsHidden hook

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