Page MenuHomePhabricator

Do not block page moves with AbuseFilter on LocalRenameJob page moves
Closed, ResolvedPublic

Description

When an user account is renamed, their pages are usually moved to the new name. Some wikis such as thwiki among others (see filter 17) have set filters to avoid users moving their user pages. This causes, on renames, that page moves needed to be performed get blocked by the abuse filter. Is there any way to exempt the page moves triggered by [Global]RenameUser from AbuseFilter? Thank you.

Details

Related Gerrit Patches:
mediawiki/extensions/CentralAuth : masterMake `onAbuseFilterShouldFilterAction` static
mediawiki/extensions/CentralAuth : masterTell AbuseFilter not to filter moves during a rename
mediawiki/extensions/AbuseFilter : masterTemporarily exclude Jobs from TitleMove hook

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Daimona added a subscriber: Daimona.EditedDec 16 2018, 1:36 PM

This reminds me of T204881. Basically, the root problem here is that we don't have a way to exclude certain actions from filtering - some sort of T69936, but related to the code instead of the users. I cannot tell right now if it'll be difficult to implement such feature, but for sure I can say that it's worth spending some time thinking about it, instead of quickly providing a flaky solution. The stopgap here is tuning the offending filters, for instance by checking user_name, or global_user_groups.

UPDATE: Well, actually the user-based solution could be fine for this. If so, this is duplicate of T69936. Otherwise, we could add a hook to AbuseFilter (either before checking filters or before taking consequences), which CentralAuth could use to bypass the filter. Let's hear some thoughts.

1997kB added a comment.EditedDec 16 2018, 3:15 PM

This is strange but it looks like all these filters (1, 2, 3, 4, 5, 6) started triggering after this rename (T209488), before that there are no such logs. And also these filters were not updated from long time.

@1997kB That's not strange: it's because we recently changed the hook used for filtering move actions. Before, we used to run filters when checking user's permissions, and that part is skipped by the Rename Job. Now, we run filters right before the move is executed, and this includes the Job's case. The change went live with 1.33-wmf.8 last week, so that's why it didn't happen before.

So...we had fixed this back in T69875: GlobalRename: Page moves are being blocked by local abusefilters.

@1997kB That's not strange: it's because we recently changed the hook used for filtering move actions. Before, we used to run filters when checking user's permissions, and that part is skipped by the Rename Job. Now, we run filters right before the move is executed, and this includes the Job's case. The change went live with 1.33-wmf.8 last week, so that's why it didn't happen before.

This seems wrong. It was very intentional that AbuseFilter ran during the user permission check, because it actually depends upon the specific user. I think the rationale in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/AbuseFilter/+/472668 is wrong, specifically "We're not really checking permissions" given that many filters do in fact check permissions.

Ah I didn't know this already happened in the past.
As for the checking permission thing, I'm not convinced. To me, checking permissions means deciding whether the user is allowed to move the page based on their rights, if they're blocked etc., not checking filters which could also make use of such things. It's also true that filtering depends on the user performing the action, and that many filters do check permissions (but not as their only check, I hope), but still I don't think this can be considered "checking permissions". Also, note that the main reason which lead to the patch is in the associated task. Sure, it could have been resolved in other ways, but I wanted to kill two birds with a stone; two birds and a half if we take into account the confusing message we used to show users when they hit the filter.
That being said, I still think that the proper solution is creating a way to bypass abuse filters; maybe, both a code-based way (e.g. a new AbuseFilter hook) and a user-based way.

Until it's fixed, should we stop renaming people who have accounts on these wikis as it's happening with every rename there and we have alot of broken page moves.

@1997kB You may ask for that filters to be changed by local filter managers. It should be enough to check global_user_groups. A proper fix is building a way to bypass AF, and we should plan it well. For sure, nothing will change for these two weeks, since there won't be deployments (and the patch needs to be written and reviewed).

@Daimona Unfortunatelly global renamer is a local user group at Meta-Wiki so global_user_groups would only work for stewards.

Ah! Anyway there are other options: check summary, check the username, ...

1997kB added a comment.Jan 2 2019, 2:32 AM

So we gonna go to each wiki and fix them or is there any one way to fix for all them?

@1997kB How many offending filters are there? A temporary solution is to fix them, yeah. The long-term solution is T69936.

@Daimona : There are 7 of them triggering rn. If you want to fix, I have mentioned some of them in my message above. @Lymantria : This one on zhwiki is fixed (1). But there could be more, yesterday I found one on metawiki (2) triggering for a renamer whois isn't much active there, so it depends upon local edits of renamer.

@1997kB Unfortunately I cannot fix them, since a global abusefilter manager group doesn't exist. I hope to get to work on T69936 soon.

As for Meta I did https://meta.wikimedia.org/wiki/Special:AbuseFilter/history/87/diff/prev/2053 which I hope excludes global renamers/stewards from it. Although Adavyd's case is not very common as global renamers usually have more than 20 edits on Meta before being appointed :) Hope that such fix is okay, otherwise please let me know.

As for the filters on other wikis, if you let us know what 'exactly' needs to be done we could discuss if we can do it ourselves. We ain't supposed to modify local abusefilters. Note that global renamers are a local user group on Meta, so excluding via user_rights/global_user_group is not an option for wikis other than Meta.

Since this is meant to be a temporary workaround, anything that works is fine. For instance, checking the summary against the default summary for renames (quick but easy to bypass), or checking that the user name belongs to the list of global renamers manually (longer but safer).
I'm trying to figure out if there's an acceptable workaround for the moment, as the "abusefilter-exempt" thingy is tricky.

Change 482408 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Temporary exclude Jobs from TitleMove hook

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

Daimona moved this task from Backlog to Next on the User-Daimona board.

@Daimona @1997kB This abuse filter log https://nl.wikipedia.org/wiki/Speciaal:Filterlogboek/1426739 suggests that there is no edit summary ("bewerkingssamenvatting") that can be used to exclude from the filter. I am reluctant to list a couple of names of global renamers, which will be hard to keep up to date. I therefore have deactived the filter at nlwiki for now.

That's weird, because the Job seems to use centralauth-rename-movelog as summary, without making it possible to change it. Anyway the hack above should solve the problem, at least for now.

1997kB added a comment.Jan 6 2019, 2:44 AM

@Lymantria In the log you mentioned, I tried to move it manually, so there's no summary.

@1997kB Okay, I included the standard part of the summary in the filter. BTW, if you maurally move, you can pass by the warning by just confirming your move (filter at nlwiki is just warning, not preventing).

1997kB added a comment.Jan 6 2019, 6:58 PM

@Daimona Just tested a rename and looks like hack you did isn't working.

@1997kB Eh, I submitted the patch, but it still needs someone to review and merge it (and amend if necessary) :-)

Daimona moved this task from Next to Under review on the User-Daimona board.Jan 7 2019, 11:20 AM

Until it's fixed I think we should stop renaming people. It's literally triggering everywhere and these (1, 2) are few latest triggers I could found and it's only for me, there are 70 other renamers who might be triggering somewhere.

@1997kB The patch above needs some love, I'll try to add some reviewers on gerrit. Making the global renamer group a real global group, so that it could be read via global_user_groups would also be useful for this specific case.

Daimona triaged this task as High priority.Jan 16 2019, 5:45 PM
Ejs-80 added a subscriber: Ejs-80.Jan 26 2019, 10:38 AM
Nihlus added a subscriber: Nihlus.Feb 22 2019, 3:24 PM
Base added a subscriber: Base.Mar 9 2019, 12:43 AM
1997kB added a comment.EditedMar 24 2019, 1:00 PM

@Daimona Should we close this? That patch has been merged and looks like filters are no more blocking page moves during rename.
edit: My bad I have clicked earlier gerrit link.

https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/AbuseFilter/+/482408/ isn't merged yet. But if AF is no longer blocking page moves during renames, maybe something else was merged that made this superfluous -or- we ain't triggering those filters anymore.

@1997kB: @MarcoAurelio is correct. If what you said is actually true, it'd be very interesting. Could you please try again?

Mistakenly checked that earlier gerrit link and thought it's no more happening, but it is not yet fixed. Checked some filter and they still triggering, although I haven't triggered on recent rename having account on 735 wikis. Sorry for unnecessary ping.

Daimona claimed this task.Mar 25 2019, 7:28 PM
DannyS712 added a comment.EditedJul 21 2019, 8:13 PM

@Daimona a potential implementation that doesn't require any code changes:

  1. Convert the global-renamer group to be a global user group
  2. Restrict the wikiset for the global-renamer group to just metawiki
  3. Exempt users in the global-renamer group from the abuse filters

Even if a wiki isn't part of a global group's wikiset, abusefilter still has access to that global group - see https://en.wikipedia.org/wiki/Special:AbuseFilter/examine/1171433212, where a global sysop moves a page on enwiki. In the global_user_groups field, global-sysop is included despite not applying to enwiki.

@Daimona a potential implementation that doesn't require any code changes:

  1. Convert the global-renamer group to be a global user group
  2. Restrict the wikiset for the global-renamer group to just metawiki
  3. Exempt users in the global-renamer group from the abuse filters

Even if a wiki isn't part of a global group's wikiset, abusefilter still has access to that global group - see https://en.wikipedia.org/wiki/Special:AbuseFilter/examine/1171433212, where a global sysop moves a page on enwiki. In the global_user_groups field, global-sysop is included despite not applying to enwiki.

Yeah, making it a global group has already been taken into account. However, that's not the best solution. IMHO, it's better to just exclude jobs altogether, like the patch above does for move actions.
Ideally, I believe we should provide a hook like AbuseFilterShouldFilterAction or smth like that, and other extensions should use it to tell whether the current action should be skipped.
This way it'd also work for non-move actions, without having to blindly allow all jobs.

Change 482408 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Temporarily exclude Jobs from TitleMove hook

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

OK, so I sent a patch for T229252. With that in place, all we need is a patch in CentralAuth, adding a handler for the new hook, where we should determine whether the current action is a global rename. I'm not familiar with CA, so I don't know how that should be done, but we can try to add hook params if needed. Just, please, avoid lame stuff like checking the summary because that'd be exploitable.

Change 529917 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/CentralAuth@master] Tell AbuseFilter not to filter moves during a rename

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

Change 482408 abandoned by Daimona Eaytoy:
Temporarily exclude Jobs from TitleMove hook

Reason:
I'd like to move on with I00ac93d32343d5a0d429f87347994f3bc46ce3fc at this point

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

Krinkle added a subscriber: Krinkle.

Tagging CPT because I'm asking a question about PermissionManager in code review at https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/CentralAuth/+/529917/.

@Krinkle We've fallen a little behind on CR, we're hoping to clear down our queue this sprint, apologies for the delay.

Change 529917 merged by jenkins-bot:
[mediawiki/extensions/CentralAuth@master] Tell AbuseFilter not to filter moves during a rename

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

Daimona closed this task as Resolved.Sep 14 2019, 5:18 PM
Daimona removed a project: Patch-For-Review.
Daimona moved this task from Under review to Done on the User-Daimona board.

Change 536756 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/CentralAuth@master] Make onAbuseFilterShouldFilterAction static

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

Change 536756 merged by jenkins-bot:
[mediawiki/extensions/CentralAuth@master] Make onAbuseFilterShouldFilterAction static

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