Page MenuHomePhabricator

Respect AbuseFilter warnings triggered by imports
Closed, ResolvedPublic1 Estimated Story Points

Description

The FileImporter currently does block the whole import if one of the revisions triggers a AbuseFilter warning. Instead we want to catch these ( potentially multiple ) warnings and show them to the user. Afterwards the user should be able to continue the import.

After the user submitted the form to start the import

  • Detect and collect AbuseFilter warnings when validating the revisions
  • Get back to the preview form and show ( probably at the top )
    • One general warning explaining that some revision(s) the import triggered AbuseFilter
    • The specific warning text(s) of the rules we triggered
  • If the user hits submit again the import will continue and should not be blocked by the AbuseFilter

If the AbuseFilter triggers a block due to a rule ( even if it affects an old revision ) block the whole import and show the warning box for that rule. ( This should be the status quo in that regard )

Event Timeline

I like the current UI where the whole page shows only the error message, and doesn’t distract with things like the image itself or the description. However, there should be a button to go back (if AbuseFilter emits either a warning or entirely blocks the action) and a button to confirm and import in that state anyway (only if AbuseFilter emits just a warning), of course.

The current UI, for anyone who doesn’t know how it looks like:

Lena_WMDE set the point value for this task to 8.Jun 4 2020, 9:26 AM
Lena_WMDE moved this task from Backlog to Tickets ready for pickup on the Move-Files-To-Commons board.

Change 606706 had a related patch set uploaded (by Andrew-WMDE; owner: Andrew-WMDE):
[mediawiki/extensions/FileImporter@master] [WIP][POC] Respect AbuseFilter warnings triggered by imports

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

thiemowmde removed Andrew-WMDE as the assignee of this task.Jun 24 2020, 8:07 AM
thiemowmde added a subscriber: Andrew-WMDE.
awight claimed this task.Jun 29 2020, 9:01 AM

Change 608302 had a related patch set uploaded (by Awight; owner: Awight):
[mediawiki/extensions/FileImporter@master] [WIP] Validation returns Status objects

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/FileImporter/ /608302

awight removed awight as the assignee of this task.Jun 29 2020, 12:11 PM
awight added a subscriber: awight.
awight added a comment.Jul 1 2020, 3:35 PM

Unassigning myself in case anyone wants to take over. I've gone ahead with the ImportOperation Status changes, but haven't done anything with the AbuseFilter integration issue.

From today's discussion:

  • I suggest to not touch the AbuseFilter code. It feels like this would open a rabbit hole. Notably: It appears like AbuseFilter is not made to work with imports, at all. There is nothing that allows AbuseFilter to work with multiple edits at once, and nothing that allows it to work with old revisions. AbuseFilter always assumes edits are solitary events, done by a person, with content that was never checked before. Neither of this is true in case of FileImporter.
  • I suggest to implement what's currently suggested in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/FileImporter/+/606706. That is:
    • Change all code to work with status objects.
    • Merge all warning messages from all validation steps.
    • Make sure the warnings are presented to the user, and the user can decide to ignore them the next time.
    • Ignore the warnings when that happened.
  • I really like the idea of passing a tiny list of filter IDs to decide if a warning was seen before. We discussed if this is a possible attack vector, and no, it's not.
  • A relevant benefit of this approach is, in my opinion, that it does not care at all if the warnings are from the most recent revision, or from older ones. All warnings are collected in a single status object. Even if we later change the process to run AbuseFilter only on the most recent revision, the code dealing with ignorable warnings will be the exact same.
  • Make sure to de-duplicate the warnings! This is critical when AbuseFilter complains about the same issue in multiple revisions.
  • Make sure the UI talks about something like "the file you are going to import contains problematic content somewhere, either in the file description, or in an old revision of the file description". Something like this.

Change 609417 had a related patch set uploaded (by Andrew-WMDE; owner: Andrew-WMDE):
[mediawiki/extensions/FileImporter@master] Adjust FileRevisionFromRemoteUrl validation to only use Status

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

Copy for the generic warning, with specific warnings to be shown below:

An automated filter has identified a potentially unconstructive edit as part of this file's revision history. One or more revisions could have triggered this; each is shown below with its associated warning. Click submit again to ignore any warnings and continue the import.

Link: https://en.wikipedia.org/wiki/Wikipedia:Edit_filter

Note: as discussed with @Andrew-WMDE , this message is only for warnings, not for errors when AbuseFilter blocks an import.

Lena_WMDE changed the point value for this task from 8 to 5.Jul 8 2020, 10:08 AM
Lena_WMDE changed the point value for this task from 5 to 3.

Change 608302 merged by jenkins-bot:
[mediawiki/extensions/FileImporter@master] Validation returns Status objects

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

Change 609417 merged by jenkins-bot:
[mediawiki/extensions/FileImporter@master] Adjust FileRevisionFromRemoteUrl validation to only use Status

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

Change 606706 merged by jenkins-bot:
[mediawiki/extensions/FileImporter@master] Respect AbuseFilter warnings triggered by imports

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

To do: change the styling to "warning message" (yellow) rather than error message: https://design.wikimedia.org/style-guide/components/messages.html

Lena_WMDE changed the point value for this task from 3 to 1.Jul 23 2020, 10:11 AM

OOUI MessageWidget (type: 'warning') should apply to all abuse filter warnings which do not block the user from continuing through the import.

Is it actually possible to use the box message MessageWidget (type: 'warning') for the main/general message, then have all of the specific warnings without the box as MessageWidget (type: 'warning', inline: true)?

Change 615687 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/FileImporter@master] Turn all AbuseFilter warning boxes yellow

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

Change 615687 merged by jenkins-bot:
[mediawiki/extensions/FileImporter@master] Turn all AbuseFilter warning boxes yellow

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

Lena_WMDE closed this task as Resolved.Aug 11 2020, 9:22 AM
Lena_WMDE claimed this task.
Lena_WMDE moved this task from Demo to Done on the WMDE-QWERTY-Sprint-2020-07-22 board.