Page MenuHomePhabricator

AbuseFilter "global_user_editcount" field can't be used during (auto)createaccount action (Introduce global_account_editcount)
Closed, ResolvedPublicBUG REPORT

Description

When "createaccount" in action is true the variable global_user_editcount seems to be null, while I'd expect it to be zero (for createaccount) and its true value (for autocreateaccount) so, for example, users with swear words in their name in a local language (false friends et al.) would be allowed to create account if they have a sufficient number of edits in other wikis.

Example here, for those who can see it.

Event Timeline

When "createaccount" in action is true the variable global_user_editcount seems to be null

I think you mean it isn't set at all. This confused me at first as I looked into the code (1, 2) and couldn't find where the null would come from.

Assuming that is what you mean, I can see why it happens. AbuseFilter will have the variables computed only if the user exists locally. Which for account creations is mostly the case when someone has an account created for somebody else.

Unfortunately, AbuseFilter is still burdened by the chicken-and-egg problem: processing an account creation that is about to happen, but it hasn't yet.

As a fix, it might be possible to change the hook in CentralAuth from AbuseFilterGenerateUserVarsHook to AbuseFilterAlterVariablesHook even though the opposite is recommended.

When "createaccount" in action is true the variable global_user_editcount seems to be null

I think you mean it isn't set at all.

@matej_suchanek Thanks. I'm sure you're right. I only tested on the linked event post festum, wanting to know what the variables were at the time of account creation. The problem is that I forgot those variables are not saved (AbuseFilter/Rules format does say "most of these variables are always set to false when examinating past edits").

I still think it would be good to have global_user_* available at the time of a new local acc creation. Or the whole case documented a little bit better.

Change 955868 had a related patch set uploaded (by Matěj Suchánek; author: Matěj Suchánek):

[mediawiki/extensions/CentralAuth@master] Set abuse filter variables during (auto)createaccount

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

matej_suchanek changed the task status from Open to In Progress.Sep 10 2023, 2:37 PM
matej_suchanek claimed this task.
Krinkle renamed this task from global_user_editcount is null on (auto)createaccount to AbuseFilter "global_user_editcount" field is null during (auto)createaccount action.Sep 11 2023, 1:54 PM

I may be overthinking this, and I'm ready to change my mind, but it seems to me that these should be new variables. By analogy with the user_* variables, which all refer to the currently logged-in user, and when performing an account creation, you're supposed to use accountname if you want the name of the account being created (per https://www.mediawiki.org/wiki/Extension:AbuseFilter/Rules_format), it would make sense for the global_user_* variables to behave in the same way (and they have behaved this way for 11 years since being added in T22850), and that new variables should be added for the global editcount/groups of the account being created (maybe prefixed with global_account_* or something).

@matmarex, when you're autocreating you're coming in already logged in from another wiki, so there's already a global account with global_user_editcount well defined on other wikis.
When you're creating, all I want is global_user_editcount be 0 instead of undefined.

Though... Now I see what you mean, when action=createaccount and a logged-in user is creating the new acc. I only see that when new users get into some loop and create one acc after another after another. Can't really think of a good use case, but what you're saying makes sense in this rare case.

I can see @matmarex's point: global_user_* vars are available IFF user_* are available, and we should probably not break this. I think this is a good point.

I have already mentioned the example of having an account created for somebody else:

AbuseFilter will have the variables computed only if the user exists locally. Which for account creations is mostly the case when someone has an account created for somebody else.

So in fact we already support these variables during account creation since user_* vars are present, too. If we now added support for these variables during DIY ("do it yourself") account creation, the same variables would carry semantically different meanings in different situations. That could cause confusion, and that's exactly why both accountname and user_name exist.

So I agree with @matmarex that introducing different variables for this is safer. It shouldn't be a problem for filter maintainers because they are already encouraged to check for action, and the case of accountname is well documented.

Change 955868 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@master] Provide abuse filter variables during (auto)createaccount

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

The merged patch should provide a new variable global_account_editcount for this use case. (global_user_editcount will still be null, unless the account is being created by another logged-in user, rather than a logged-out user.)

I'm trying to test this on https://en.wikipedia.beta.wmflabs.org/wiki/Special:AbuseFilter, but it's telling me that the new variables are unrecognized. Am I missing something?

Also, @matej_suchanek could you please update the documentation at https://www.mediawiki.org/wiki/Extension:AbuseFilter/Rules_format as well?

Change 959041 had a related patch set uploaded (by Matěj Suchánek; author: Matěj Suchánek):

[mediawiki/extensions/CentralAuth@master] Let the builder know about the new variables

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

Sorry, I guess I forgot something...

Change 959041 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@master] Let the builder know about the new variables

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

I have been testing with https://en.wikipedia.beta.wmflabs.org/wiki/Special:AbuseFilter/480 and I am not sure if this is working. I've autocreated a bunch of accounts on enwiki.beta after editing with them on dewiki.beta, and all of them have been recorded in the filter log with global_account_editcount=0, for example https://en.wikipedia.beta.wmflabs.org/wiki/Special:AbuseLog/385033.

Could it be the CentralAuthUser::isAttached call? If the account doesn't exist on the wiki, it seems to return false. What could happen if we removed it?

Yeah, so the question is, can we assume that it will be attached? I think that we can, on Wikimedia wikis there should not be any unattached accounts any more, and we don't support third-party users.

Yeah, so the question is, can we assume that it will be attached? I think that we can, on Wikimedia wikis there should not be any unattached accounts any more, and we don't support third-party users.

If it's an auto creation, then that feels like a safe assumption.

Change 960199 had a related patch set uploaded (by Matěj Suchánek; author: Matěj Suchánek):

[mediawiki/extensions/CentralAuth@master] Don't check attached status for abuse filter variables

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

Change 960199 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@master] Don't check attached status during autocreateaccount

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

I tested on the beta cluster again and everything is working as expected now.

matmarex renamed this task from AbuseFilter "global_user_editcount" field is null during (auto)createaccount action to AbuseFilter "global_user_editcount" field can't be used during (auto)createaccount action (Introduce global_account_editcount).Sep 29 2023, 12:40 AM