Page MenuHomePhabricator

Provide a mechanism for other extensions to modify protected variables access requirements
Closed, ResolvedPublic3 Estimated Story Points

Description

Summary

AbuseFilter has a protected protection level that enforces protected variables be only used in protected filters. This feature should be expanded to allow variables to have additional requirements for access, intended to be set by other extensions.

Background

  • AbuseFilter protected variables were added to support the addition of the user_unnamed_ip variable.
    • This variable needed to be not publicly viewable, be only accessible by those who have rights to reveal IP addresses, and also require users to have enabled a preference to view the data
  • Since then we have had a need for other protected variables that do not need these same level of restrictions
  • To allow for this, we need to split the restrictions for the user_unnamed_ip variable off
  • Users who do not meet the additional restrictions for access on a variable should be unable to see the filter in which it is used or the associated logs. In a sense, it should appear as if the filter was "super-protected" by having the variable present.
    • However, It should be possible to remove the variable from a filter to make it back to a normal protected variable state.
    • Logs should remain in this "super protected" state forever if they are associated with a filter that used one of these variables

Technical notes

  • It should be possible to implement per-variable restrictions, but we will need to consider potentially caching the return value of the access methods due to using a hook

Acceptance criteria

  • AbuseFilter exposes a method to add additional restrictions onto protected variable viewing

Details

Related Changes in Gerrit:
SubjectRepoBranchLines +/-
mediawiki/extensions/AbuseFiltermaster+392 -17
mediawiki/extensions/AbuseFiltermaster+129 -50
mediawiki/extensions/AbuseFiltermaster+284 -242
mediawiki/extensions/AbuseFiltermaster+357 -86
mediawiki/extensions/AbuseFiltermaster+168 -68
mediawiki/extensions/AbuseFiltermaster+175 -180
mediawiki/extensions/AbuseFiltermaster+161 -3
mediawiki/extensions/AbuseFiltermaster+347 -20
mediawiki/extensions/AbuseFiltermaster+189 -0
mediawiki/extensions/AbuseFiltermaster+34 -8
mediawiki/extensions/AbuseFiltermaster+82 -0
mediawiki/extensions/AbuseFiltermaster+106 -175
mediawiki/extensions/AbuseFiltermaster+8 -6
mediawiki/extensions/AbuseFiltermaster+70 -0
mediawiki/extensions/AbuseFiltermaster+188 -40
mediawiki/extensions/AbuseFiltermaster+100 -75
mediawiki/extensions/AbuseFiltermaster+105 -42
mediawiki/extensions/AbuseFiltermaster+101 -57
Show related patches Customize query in gerrit

Related Objects

Event Timeline

Having discussed this a bit with Tran, we decided that using a hook would be the easier implementation method that would not be too specific to CheckUser. This would also make AbuseFilter independent of any specific implementation, except that the hook would need to be specific about the protected variables being viewed plus whether the value of the variable is being viewed too.

Dreamy_Jazz changed the point value for this task from 1 to 3.Mar 5 2025, 6:28 PM

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

[mediawiki/extensions/AbuseFilter@master] [Very WIP] Use status for protected variables access methods

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

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

[mediawiki/extensions/AbuseFilter@master] AbuseFilterViewEdit: Test denying access to protected filters

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

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

[mediawiki/extensions/AbuseFilter@master] Expand SpecialAbuseFilterTest for protected variable access

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

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

[mediawiki/extensions/AbuseFilter@master] Expand QueryAbuseLogTest to test denying access to protected filter

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

Change #1125400 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] AbuseFilterViewEdit: Test denying access to protected filters

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

Change #1125401 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] Expand SpecialAbuseFilterTest for protected variable access

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

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

[mediawiki/extensions/AbuseFilter@master] [Very WIP] Accept variables argument to protected access methods

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

Change #1125402 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] Expand QueryAbuseLogTest to test denying access to protected filter

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

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

[mediawiki/extensions/AbuseFilter@master] [Very WIP] Add hooks for protected variable access checker methods

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

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

[mediawiki/extensions/AbuseFilter@master] Replace AbuseFilterServices with DI in CheckMatch API

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

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

[mediawiki/extensions/AbuseFilter@master] [WIP] Make AbuseFilterPermissionManager::canSeeLogDetailsForFilter take Filter

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

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

[mediawiki/extensions/AbuseFilter@master] [Very WIP] Add AbuseFilterLogLookupService

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

Change #1125199 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] Return StatusValue from protected variables permission methods

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

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

[mediawiki/extensions/AbuseFilter@master] [WIP] Create FilterFromSpecsTestTrait

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

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

[mediawiki/extensions/AbuseFilter@master] [WIP] Test SpecialAbuseLog::getPrivateDetailsRow

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

Change #1127104 abandoned by Dreamy Jazz:

[mediawiki/extensions/AbuseFilter@master] [Very WIP] Add AbuseFilterLogLookupService

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

Change #1127079 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] Use dependency injection over AbuseFilterServices in CheckMatch API

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

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

[mediawiki/extensions/AbuseFilter@master] [WIP] Accept variables in AF permission manager protected var methods

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

Change #1127488 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] Create FilterFromSpecsTestTrait

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

Change #1127489 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] Test SpecialAbuseLog::getPrivateDetailsRow

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

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

[mediawiki/extensions/AbuseFilter@master] Make several FilterLookup methods public

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

Change #1127554 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] Make several FilterLookup methods public

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

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

[mediawiki/extensions/AbuseFilter@master] [WIP] Update AbuseFilterView classes to pass Filter to access methods

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

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

[mediawiki/extensions/AbuseFilter@master] [WIP] Expand SpecialAbuseFilterTest to cover to be modified code

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

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

[mediawiki/extensions/AbuseFilter@master] Test QueryAbuseFilters

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

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

[mediawiki/extensions/AbuseFilter@master] [WIP] Expand tests for SpecialAbuseLogTest

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

Change #1127957 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] Test QueryAbuseFilters

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

Change #1127912 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] Expand SpecialAbuseFilterTest to cover to be modified code

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

Change #1129328 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] Expand tests for SpecialAbuseLogTest

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

Change #1127091 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] Take Filter in AbuseFilterPermissionManager::canSeeLogDetailsForFilter

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

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

[mediawiki/extensions/AbuseFilter@master] [Very WIP] Customise protected vars access errors for var specific restrictions

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

Change #1127546 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] Accept variables in AF permission manager protected var methods

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

Change #1127877 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] Update AbuseFilterView classes to pass Filter to access methods

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

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

[mediawiki/extensions/AbuseFilter@master] [WIP] Exclude protected variables from var dumps unless used by filter

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

Change #1125500 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] Provide variables to protected variables access checker methods

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

Dreamy_Jazz updated the task description. (Show Details)
Dreamy_Jazz updated the task description. (Show Details)

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

[mediawiki/extensions/AbuseFilter@master] Hide log detail links on AbuseLog when user cannot see variable

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

Change #1131390 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] Hide log detail links on AbuseLog when user cannot see log details

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

Change #1126554 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] Add hooks for protected variable access checker methods

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

For QA I would suggest doing this locally and checking out the patch https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CheckUser/+/1127984. This is because the functionality cannot be tested on the Beta wikis as CheckUser is not installed there and no functionality changes are made by this ticket until the CheckUser patch is applied.

If needed I can make some suggested steps, but in general I would recommend checking that:

  1. Users with CheckUser IP reveal access and protected filter access can see protected filters and logs that use user_unnamed_ip
  2. Users without IP reveal access cannot see filters that use user_unnamed_ip, even if that user has has access to protected filters

The steps I would write would be quite lengthy as this modifications for this ticket are large, however, they would boil down to applying to all AbuseFilter interfaces the points I've mentioned above.

dom_walden subscribed.

If needed I can make some suggested steps, but in general I would recommend checking that:

  1. Users with CheckUser IP reveal access and protected filter access can see protected filters and logs that use user_unnamed_ip
  2. Users without IP reveal access cannot see filters that use user_unnamed_ip, even if that user has has access to protected filters

I have confirmed this for as many AbuseFilter endpoints as I can find. I have also done it systematically for a large number of AbuseFilter logs on my local. I have tested with and without CheckUser installed. Without CheckUser installed, user_unnamed_ip is still hidden from those without abusefilter-access-protected-vars.