Page MenuHomePhabricator
Create Task
Maniphest T229692

Use GetUserBlockComplete hook instead of GetBlockedStatus in TorBlock extension
Closed, ResolvedPublic
Actions

Description

The GetBlockedStatus hook will be deprecated as outlined in T229035, and will be replaced by a GetUserBlockComplete, which passes a user's block instead of the user object.

TorBlock should use GetUserBlock, and the conditions for removing a block should not change. The conditions are:

$wgTorDisableAdminBlocks &&
TorExitNodes::isExitNode() &&
$user->mBlock instanceof AbstractBlock &&
$user->mBlock->getType() != DatabaseBlock::TYPE_USER

If the block is a composite block, made from multiple original blocks, the block should be removed if all the original blocks fulfill these criteria.

Details

SubjectRepoBranchLines +/-
Use GetUserBlock hook instead of GetBlockedStatusmediawiki/extensions/TorBlockmaster+29 -13
Customize query in gerrit

Related Objects
Search...

Event Timeline

Tchanders created this task.Aug 2 2019, 7:41 PM
Restricted Application removed a project: Patch-For-Review. · View Herald TranscriptAug 2 2019, 7:41 PM
gerritbot added a comment.Aug 2 2019, 7:43 PM

Change 525665 had a related patch set uploaded (by Tchanders; owner: Tchanders):
[mediawiki/extensions/TorBlock@master] Use GetUserBlockComplete hook instead of GetBlockedStatus

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

gerritbot added a project: Patch-For-Review.Aug 2 2019, 7:43 PM
Tchanders added a comment.Aug 2 2019, 7:47 PM

@dbarratt You can this locally by enabling TorBlock, setting $wgTorDisableAdminBlocks = true and overriding TorExitNodes::isExitNode() to return true/false. It should then remove any non-user blocks, but keep any user blocks or composite blocks that contain user blocks.

JJMC89 moved this task from Backlog to User blocking on the MediaWiki-User-management board.Aug 13 2019, 4:57 PM
Tchanders claimed this task.Aug 21 2019, 4:53 PM
Tchanders added a project: Anti-Harassment.
Tchanders moved this task from Untriaged 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.
dmaza updated the task description. (Show Details)Aug 27 2019, 10:44 PM
Tchanders added a subtask: T228948: PermissionManager::isBlockedFrom() can return true even if the user does not have a block.Aug 28 2019, 11:18 AM
gerritbot added a comment.Aug 29 2019, 10:17 PM

Change 525665 merged by jenkins-bot:
[mediawiki/extensions/TorBlock@master] Use GetUserBlock hook instead of GetBlockedStatus

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

ReleaseTaggerBot added a project: MW-1.34-notes (1.34.0-wmf.21; 2019-09-03).Aug 29 2019, 11:00 PM
Tchanders moved this task from Code Review 🔍 to QA/Testing 🐞 on the Anti-Harassment (The Letter Song) board.Aug 29 2019, 11:05 PM
dom_walden added subscribers: Niharika, dom_walden.EditedSep 2 2019, 3:44 PM

By setting $wgTorDisableAdminBlocks = true (which is default), any IP blocks that coincide with Tor exit nodes will not get applied, including System blocks.

User blocks, or composite blocks containing user blocks, still get applied. This includes cookie blocks.

@Niharika It should be noted that TorBlocks does not block Special:PasswordReset. Perhaps that should be raised as a bug, as otherwise there does not seem to be a way to block anon. Tor users from Special:PasswordReset.

Also, the default of the extensions is to not apply Tor blocks to logged in users (they have the torunblocked right). But, IP blocks applying to those users are still removed. Logged in users using Tor might find themselves unblocked, unless they have blocks applying to their username.

On WMF wikis, I believe $wgTorDisableAdminBlocks = false and only users in the ipblock-exempt group have the torunblocked right, so this won't affect production.

But, it will change the behaviour of default installs.

Tchanders added a comment.Sep 2 2019, 4:13 PM

But, it will change the behaviour of default installs.

@dom_walden It was our intention to keep the conditions for removing a block the same with this patch (so IP blocks are removed because they were removed before). Is there something that we've inadvertently changed?

That said, the one thing that should have changed is that previously composite blocks would never be kept, because they are always type IP, whereas now they will be kept if they contain a user block.

dom_walden added a comment.Sep 2 2019, 4:39 PM
In T229692#5459379, @Tchanders wrote:

But, it will change the behaviour of default installs.

@dom_walden It was our intention to keep the conditions for removing a block the same with this patch (so IP blocks are removed because they were removed before). Is there something that we've inadvertently changed?

Previously, TorBlocks did not remove IP blocks (reading the previous code, I'm not exactly sure what it was meant to do :P). Which is possibly a bug your patch has fixed.

Niharika added a comment.Sep 3 2019, 5:53 PM

Thanks @dom_walden for explaining. I agree that it seems like we have fixed a bug here. I think we should make a note that we found the behavior wasn't as expected and we fixed it with this ticket on the extension talk page (https://www.mediawiki.org/wiki/Extension_talk:TorBlock) so third party users who might be active there can see it.

dom_walden moved this task from QA/Testing 🐞 to Done (2019-20: Q2) on the Anti-Harassment (The Letter Song) board.Sep 4 2019, 3:06 PM
In T229692#5462539, @Niharika wrote:

Thanks @dom_walden for explaining. I agree that it seems like we have fixed a bug here. I think we should make a note that we found the behavior wasn't as expected and we fixed it with this ticket on the extension talk page (https://www.mediawiki.org/wiki/Extension_talk:TorBlock) so third party users who might be active there can see it.

OK. I'll move this along.

Who should update the documentation?

dom_walden mentioned this in T228948: PermissionManager::isBlockedFrom() can return true even if the user does not have a block.Sep 5 2019, 11:34 AM
dbarratt closed subtask T228948: PermissionManager::isBlockedFrom() can return true even if the user does not have a block as Resolved.Sep 5 2019, 2:47 PM
dbarratt closed this task as Resolved.Sep 5 2019, 5:24 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits