Page MenuHomePhabricator

Rate limit or restrict access to comment removal
Closed, DeclinedPublic

Description

The desired functionality of a non-shell-based interface to delete comments, was described in T158 ("Phabricator should let admins delete comments in Maniphest"), and filed/implemented upstream at https://secure.phabricator.com/T4909 (originally titled "Possibility to delete comments in Maniphest")
That upstream task has many useful comments, from @Qgil and @Aklapper, including "no stats, but I guess it happens once or twice a month that I (or other WMF Bugzilla admins) need to hide a comment or hide/delete an attachment."
The ability to delete comments via the web GUI was added, but with a very broad default.

Currently, all users can delete their own comments, via the "Remove Comment" item in the dropdown menu.

There are concerns with this situation:

  • "Deletion" does not even really work, because updates are instantly emailed to various addresses and the bug mailing list, and also the first few hundred characters are sent to IRC, so it's a false promise.
    • It distances contributors from the expectations of "permanence" which they should have before hitting submit on a wiki-edit or email-send. Some users might be more inclined to write emotional/angry comments, if they believe that the comment can potentially be deleted later (assuming nobody "quotes" it in the meantime).
  • It enables easy/thorough infosuicide,(1) which hurts the community permanently. (or creates a lot of work for maintainers to undo the damage). Details at https://secure.phabricator.com/T4909#57227
  • It enables anyone to (partially/locally) cover up history. (This can create suspicion. "What did she/he write?!").

The only potential benefit, is allowing users to immediately(/partially/locally) cover up mistakes, if they accidentally "saved" very private information. (However, this same accident is possible in email, without any way to undo it at all. Netizens should be used to it.)

The possible solutions are

  1. Rate limit any comment removal. E.g. 1 per day. (That might be a good temporary solution, if a group-based access is too hard to do soon.)
  2. Time limit any comment removal. E.g. Only within 1 minute or 1 hour after saving. (Discussed below, as another good temporary solution)
  3. Usergroup limit to comment removal. (This is the desired feature, with assignable groups (eg. Phab-Admins) as the only users who are able to delete content from local visibility.)

Related Objects

Event Timeline

Quiddity created this task.Nov 9 2014, 7:17 AM
Quiddity updated the task description. (Show Details)
Quiddity raised the priority of this task from to Needs Triage.
Quiddity added a project: Phabricator.
Quiddity changed Security from none to None.
Quiddity added subscribers: Quiddity, Qgil, Aklapper.
Krenair added a subscriber: Krenair.Nov 9 2014, 8:08 PM
Qgil triaged this task as High priority.Nov 9 2014, 9:04 PM

@Quiddity, I share your concerns, as you can see in the thread upstream. This is a change that needs to be done upstream. If you are interested in a short-term reply from the maintainers (hopefully together with an upstream fix), then let's agree on a short term plan first.

Knowing how the Phabricator maintainers see administrators, I bet that proposing the restriction to the feature to admins will not lead to any short term agreement.

I would take the work from epriestley here:

Would you be comfortable with a rate limit instead of restricting this power to administrators? Or limiting user removal to the first hour after the comment is posted, or some similar rule?

Would this make sense:

  • Any user can delete their own comments within an hour after posting them. (knowing that it will too late for email/IRC notifications)
  • Administrators can delete any comment always.

A Time-limit restriction (to 1 hour, or even better to 1 minute, after the initial save), does sound reasonable, as a short-term solution. (I'll add that option to the main description above)
I would suggest this short-term solution is urgent to do before the migration, to prevent infosuicide. (And/or, I think you database wranglers would need to commit to undoing any such damage, if it occurred.)

I'm not familiar enough with bugzilla-admin norms or needs, to comment on the end-goal desired feature, in detail. I'll leave that for others to discuss.

Quiddity updated the task description. (Show Details)Nov 9 2014, 9:25 PM
Quiddity updated the task description. (Show Details)
Qgil added a comment.Nov 9 2014, 9:48 PM

One minute???

How much damage can you cause by deleting your comments made within an hour? Sure, you might destroy some context in a hot, ongoing discussion. Is that bad? In my experience in this and on other projects, this actually sounds like a good feature.

I'm not familiar enough with bugzilla-admin norms or needs, to comment on the end-goal desired feature, in detail.

Bugzilla only: As a member of the Bugzilla admin group (or member of security group) you can mark comments and attachments as private which will hide them from everybody not being a member of one of these two groups. I have seen other Bugzilla admins deleting attachments in the past. You cannot delete comments (except for using SQL on the Bugzilla DB).

Qgil lowered the priority of this task from High to Normal.Nov 13 2014, 9:24 AM

Upstream says that they are not going to rush for this, we have our hands full, and, well, even if an incident would happen the content removed will be accessible in the database.

revi added a subscriber: revi.Nov 24 2014, 3:43 PM
Qgil added a comment.Dec 19 2014, 9:03 AM

A couple of ideas:

  • If the "This comment was removed by" text is indexed (and I'm not sure about this), a search query could be created and watched, so at least we would have an idea of the dimension of the problem, and we would have better chances of detecting abuse or innocent actions from users unaware of this discussion. Blocked by T75743: Can't search an exact phrase in Phabricator
  • On the lines of preventing abuse, maybe upstream would be happy about the creation of a Herald rule to be triggered when a comment is deleted. Then we could set that rule to send an email.

<tl;dr>: Getting a list of users and how many comments they have removed.

In T1185#935819, @Qgil wrote:

A couple of ideas:

  • If the "This comment was removed by" text is indexed (and I'm not sure about this), a search query could be created and watched, so at least we would have an idea of the dimension of the problem

As long as our search cannot search for the entire string (results would show all tasks that have this four words, "by" is probably too short), one could use SQL.
Comments in tasks that get "removed" (hidden from display) are indexed on a database level by setting the value "2" for isDeleted.
You can get a list of all users who have ever removed a comment, and the number of removed comments per user:

SELECT u1.userName, SUM(u2.isDeleted) / 2 AS "Comments removed" 
FROM phabricator_user.user u1 
JOIN phabricator_maniphest.maniphest_transaction_comment u2 
ON u1.phid = u2.authorPHID WHERE u2.isDeleted = 2 
GROUP BY u1.username 
HAVING SUM(u2.isDeleted) > 0;

Note:

  • numbers refer to acting. Users can remove their comments. Administrators can also remove other user's comments. When an admin removes the comment of another user, that action is counted for the admin user in the query above.
  • query not tested for performance. The JOIN should probably be a LEFT or RIGHT one?
  • not sure the HAVING SUM(u2.isDeleted) > 0 condition is actually needed (but cannot hurt either)
  • this is a query across two databases, in case this ever ends up in some script (cf. T1003#852987) that requires access grants.
Aklapper lowered the priority of this task from Normal to Lowest.Oct 9 2015, 8:11 AM

Six months later: Do we have actual examples of harmful behavior?
I'm not aware, and I don't see how this is "normal" priority.

I think we can decline this, what do you think?

Aklapper renamed this task from Restrict access to comment removal to Rate limit or restrict access to comment removal.May 23 2016, 5:36 PM
Restricted Application added a subscriber: TerraCodes. · View Herald TranscriptMay 23 2016, 5:36 PM

[Adjusted task summary to be in sync with upstream]

Jay8g added a subscriber: Jay8g.May 24 2016, 4:52 AM

Perhaps a "hide" feature (like Flow) would be a better substitute, allowing people to hide (their?) comments from the default view while still allowing people to look in the comment history to see what had been written...