Page MenuHomePhabricator

CheckUser should impose IP viewing restrictions on AbuseFilter's `unnamed_user_ip` protected variable
Closed, ResolvedPublic

Description

Splitting this off from T387331: Provide a mechanism for other extensions to modify protected variables access requirements in order to clearly track the work.

In a temporary accounts paradigm, CheckUser is the primary authority on IP viewing access. As such, it should also impose this on AbuseFilter where AF exposes IPs (the protected variable, user_unnamed_ip).

(copying reference notes directly from originating task)

From T385687: Investigate: How should the AbuseFilter variable `user_unnamed_ip` be restricted:

Given that we already have a modifiable config, AbuseFilterProtectedVariables that can have variables added to it, I think it wouldn't be unreasonable to also modify it to be able to declare additional restrictions like rights and preferences. eg. [ "user_unnamed_ip" => [ 'rights' => [ 'checkuser-temporary-account' ], 'preferences' => [] ] ] and then respect them anywhere else we use permission checks like canViewProtectedVariableValues.

One thought is that instead of defining additional restrictions through rights and preferences keys, we could define some kind of way to provide a validator class or callback. This would then inform AbuseFilter if the user has access to see the variable.

The reason I suggest this is because the validator callback could be CheckUserPermissionManager::canAccessTemporaryAccountIPAddresses which would then deduplicate the logic for the CheckUser additional restrictions. Furthermore, in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/AbuseFilter/+/1100552 I was defining two callbacks because AbuseFilterPermissionManager has a method for seeing the variable and then another one for seeing the values of the variables. These are defined slightly differently, and I think we could want to do similar for CheckUser

Final mechanism is up to the implementer. The callback seems more in line with work already existing. My only concern is discoverability - it would be nice if this was more exposed as opposed to having to know CU is now involved in the process (assuming AF itself never really knows or cares who is hooking into the mechanism - only that it's being returned a truthy value).

Acceptance Criteria:

  • CU uses this method to protected user_unnamed_ip such that users can only see the value of user_unnamed_ip or filters which contain that variable if they have IP reveal access

Related Objects

Event Timeline

Change #1127984 had a related patch set uploaded (by Dreamy Jazz; author: Dreamy Jazz):

[mediawiki/extensions/CheckUser@master] [Very WIP] Define additional restrictions for AbuseFilter user_unnamed_ip var

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

Change #1130688 had a related patch set uploaded (by Dreamy Jazz; author: Dreamy Jazz):

[mediawiki/extensions/CheckUser@master] Don't pass optional $timestamp parameter in AbuseFilterHandlerTest

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

Change #1127984 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@master] Define additional restrictions for AbuseFilter user_unnamed_ip var

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

Change #1130688 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@master] Don't pass optional $timestamp parameter in AbuseFilterHandlerTest

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