See the parent task for more context.
- Ensure that a user is made aware that the filter they are about to save will be protected, if it contains a protected variable
Description
Description
Details
Details
Customize query in gerrit
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| In Progress | Niharika | T324492 Temporary accounts - MVP | |||
| Open | None | T326816 Update features for temporary accounts | |||
| Open | Tchanders | T326869 Update TSP-owned products that may be affected by IP Masking | |||
| Resolved | lbowmaker | T262321 IP Masking | |||
| Resolved | tstarling | T300263 [IP Masking] Create temporary account on first edit | |||
| Open | None | T307060 [Epic] Temporary account AbuseFilter support | |||
| Open | STran | T357772 Investigate: How will `ip_in_range` and `ip_in_ranges` function when temporary accounts are enabled | |||
| Open | STran | T363906 Ensure filters that use PII-sensitive variables are protected | |||
| Resolved | STran | T364485 Alert a filter editor that a filter must be protected if it is saved with a protected variable |
Event Timeline
Comment Actions
Change #1029162 had a related patch set uploaded (by Tchanders; author: Tchanders):
[mediawiki/extensions/AbuseFilter@master] Ensure that changes to af_hidden are correctly set in afh_changed_fields
Comment Actions
Change #1026560 had a related patch set uploaded (by STran; author: Tchanders):
[mediawiki/extensions/AbuseFilter@master] Allow variables to be restricted by user right
Comment Actions
Change #1029162 abandoned by STran:
[mediawiki/extensions/AbuseFilter@master] Display changes to filter privacy levels on history and diff pages
Reason:
This work was merged into 1032420 (utility functions) and 1026560 (UI/X updates)
Comment Actions
As discussed with @STran - UX copy is as follows:
| Error message: | Your filter was not saved because it uses protected variables: user_ip. Please hide details of this filter from users who cannot see protected variables. |
| Checkbox: | Hide details of this filter from users who cannot see protected variables. |
| Checkbox helper text: | This action is permanent and cannot be undone. |
| Disabled checkbox: | Details of this filter are hidden from users who cannot see protected variables. This cannot be undone. |
Comment Actions
Some conversation around this happened in one of the AbuseFilter check-ins and we talked through some clarifications we thought would improve the UX:
- If you're not using a protected variable in an unprotected, you shouldn't need to check the box or see it. We should hide the checkbox if no protected variable is used and the filter isn't protected yet. As there is no js parser and we certainly shouldn't implement one for this, we were thinking of doing a simple string comparison check, which could possibly lead to false positives (eg. someone happens to have a username that includes a protected variable). Should copy reflect that uncertainty?
- The copy for the checkbox implies that you have a choice in whether or not to protect the filter (You can't - the filter will set its own protections). Should this messaging be more explicit that it's an acknowledgement?
- It's unclear what a protected variable is if you don't already know something about why this is happening (temporary accounts, protecting ips, etc) and what variables specifically are considered protected until you trip an error. Can we link to documentation about protected variables in the message? Or list out the protected variables?
Comment Actions
@STran Thanks for the update.
If you're not using a protected variable in an unprotected, you shouldn't need to check the box or see it. We should hide the checkbox if no protected variable is used and the filter isn't protected yet. As there is no js parser and we certainly shouldn't implement one for this, we were thinking of doing a simple string comparison check, which could possibly lead to false positives (eg. someone happens to have a username that includes a protected variable). Should copy reflect that uncertainty?
I'm not sure how to address this issue via the copy (without overcomplicating the error message?) Open to suggestions!
The copy for the checkbox implies that you have a choice in whether or not to protect the filter (You can't - the filter will set its own protections). Should this messaging be more explicit that it's an acknowledgement?
Yes, I suggest we change the copy in the error message and checkbox so that it is acting like an acknowledgement of understanding for the user (see mockup).
It's unclear what a protected variable is if you don't already know something about why this is happening (temporary accounts, protecting ips, etc) and what variables specifically are considered protected until you trip an error. Can we link to documentation about protected variables in the message? Or list out the protected variables?
Good point! I agree let's link to documentation in the error message and checkbox.
Updated copy:
| Error message: | Your filter was not saved because it uses [protected variables]: user_ip. Please acknowledge that you understand details of this filter will be hidden from users who cannot see protected variables. |
| Checkbox: | I understand that details of this filter will be hidden from users who cannot see [protected variables] |
| Checkbox helper text: | This action is permanent and cannot be undone |
| Disabled checkbox: | Details of this filter are hidden from users who cannot see protected variables |
Comment Actions
Change #1038808 had a related patch set uploaded (by STran; author: STran):
[mediawiki/extensions/AbuseFilter@master] [WIP] Implement 'protected' filter acknowledgement checkbox
Comment Actions
Change #1038808 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Implement 'protected' filter acknowledgement checkbox
Comment Actions
Change #1039250 had a related patch set uploaded (by STran; author: STran):
[mediawiki/extensions/AbuseFilter@master] Clarify protected status in filter checkboxes
Comment Actions
I'm not sure how to address this issue via the copy (without overcomplicating the error message?) Open to suggestions!
If this is overly complicated, would it be better to leave it as-is and if see if it causes confusion/fix it then?
Comment Actions
Yes I think that makes most sense. Let's monitor it and if it becomes an issue we can revisit.
Comment Actions
Change #1039591 had a related patch set uploaded (by Tchanders; author: Tchanders):
[mediawiki/extensions/AbuseFilter@master] Hide the checkbox for protecting a filter from users without the right
Comment Actions
Change #1039250 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Clarify protected status in filter checkboxes
Comment Actions
Change #1039591 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Hide the checkbox for protecting a filter from users without the right
Comment Actions
@STran If I use a filter rule like:
action="edit" & user_unnamed_ip="1.2.3.4"
I am not alerted about using a protected variables, and can save it without having to check I understand that details of this filter will be hidden...
Indeed, I was able to save the filter even if my user did not have rights to see protected variables.
I haven't experimented with this much, but I guess it is because user_unnamed_ip is not the first condition. I notice af_hidden is set to 0 for these filters.
Comment Actions
Change #1041174 had a related patch set uploaded (by STran; author: STran):
[mediawiki/extensions/AbuseFilter@master] Force full evaluation of filter in FilterEvaluator->getUsedVars()
Comment Actions
Change #1041174 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Force full evaluation of filter in FilterEvaluator->getUsedVars()
Comment Actions
When trying to save a filter with user_unnamed_ip, I see the warning:
Your filter was not saved because it uses the following protected variables: user_unnamed_ip. Please acknowledge that you understand details of this filter will be hidden from users who cannot see protected variables.
This worked even with a complicated AbuseFilter expression (I concatenated all the files from AbuseFilter's parserTests, inserting user_unnamed_ip at various points).
Test environment: local docker Abuse Filter – (92e1335) 13:45, 13 June 2024.