1484892 - Modify EditComments extension to let anyone use it conditionally and support inline editing

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Extensions, enhancement)

Description

[Kohei Yoshino [:kohei]]

In Bug 1484477 Comment 6, :dylan mentioned that the extension allows you to edit anyone’s comment. Currently It’s fine because only admins have the privilege, but it’s gonna be a problem if we allow all editbugs members to edit comments. 

Regular users should only be allowed to edit their own posted comments, while admins should be able to edit anyone’s comment with no configuration.

Comment 1

[Kohei Yoshino [:kohei]]

Are there any cases admins have deleted sensitive information by editing comments? The editcomments.html page has a group membership validation probably for that purpose, so currently only admins can view history:

https://github.com/mozilla-bteam/bmo/blob/master/extensions/EditComments/Extension.pm#L73-L79

For transparency, comment edit history should be visible to anyone just like Facebook or GitHub. Probably admins should be able to delete/hide a specific revision?

See also: https://help.github.com/articles/tracking-changes-in-a-comment/

Comment 2

[Kohei Yoshino [:kohei]]

WIP: https://github.com/kyoshino/bmo/compare/bug-1484892-editcomments-priv

Comment 3

[Kohei Yoshino [:kohei]]

(In reply to Kohei Yoshino [:kohei] from comment #2)
> WIP: https://github.com/kyoshino/bmo/compare/bug-1484892-editcomments-priv

Updated to allow admins to hide specific revisions if needed. All existing revisions will also be hidden by default, in case any of these contains sensitive info.

Comment 4

[David Lawrence [:dkl]]

(In reply to Kohei Yoshino [:kohei] from comment #1)
> Are there any cases admins have deleted sensitive information by editing
> comments? The editcomments.html page has a group membership validation
> probably for that purpose, so currently only admins can view history:

Yes we have done that many times. Passwords, API keys, etc. are removed from comments. We would want the certain history items to be invisible for that reason.

dkl

Comment 5

[Kohei Yoshino [:kohei]]

Okay, then per-revision visibility control is certainly needed. Here are screenshots of how it will look like:

Admin: https://screenshots.firefox.com/K2QZYsVlfwvc5haS/bmo-web.vm
Non-admin: https://screenshots.firefox.com/kPFKrKKs78X7ek6F/bmo-web.vm

Comment 6

[David Lawrence [:dkl]]

(In reply to Kohei Yoshino [:kohei] from comment #5)
> Okay, then per-revision visibility control is certainly needed. Here are
> screenshots of how it will look like:
> 
> Admin: https://screenshots.firefox.com/K2QZYsVlfwvc5haS/bmo-web.vm
> Non-admin: https://screenshots.firefox.com/kPFKrKKs78X7ek6F/bmo-web.vm

Looks good to me. I would just implement it the same way we do private comments and only allow those in the insider comments group to see the private revision. What we could do is, when editing a current comment, if the user is an insider, display another checkbox above the textarea called "Hidden" or "Hide Revision" or something similar. 

dkl

Comment 7

[David Lawrence [:dkl]]

The users in the insiders group are already privileged enough to see the hidden comment changes so there is no need to just limit to admins only.

Comment 8

[Kohei Yoshino [:kohei]]

My current implementation is adding a new group:

* (existing) edit_comments_group; default: admin
* (new) edit_own_comments_group bit; default: editbugs

But yeah, $user->is_insider should be able to edit any comments.

* (existing) insidergroup; default: admins and others?
* (existing) edit_comments_group; default: editbugs

Actually my initial implementation was like that. Will update again and send a PR.

Comment 9

[Kohei Yoshino [:kohei]]

Added "Hide This Revision" checkbox above the textarea: https://screenshots.firefox.com/E57oyLFaDxQNisS9/bmo-web.vm

Comment 10

[Kohei Yoshino [:kohei]]

Yes, inline editing... https://screenshots.firefox.com/WqC5RhA0S1KQmNyA/bmo-web.vm

Comment 11

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Kate, do you see any issues here with how we're allowing edits on bugs?

Comment 12

[Kohei Yoshino [:kohei]]

Here’s a summary of the proposed UX:

* Insiders (admins and people in the security group):
    * can edit any comment anytime on the bug page [1]
    * can see all revisions (edit history) of any comment
    * can hide a specific revision in case it contains sensitive info using a checkbox [2]

* Users in the “editbugs” group (employees and active contributors):
    * can edit their own comments anytime on the bug page [1] 
    * can see revisions of any comment, except for revisions hidden by an insider [3]
    * cannot hide any revision, even their own, so they still have to ask admins to hide sensitive info if any

* Users not in the “editbugs” group (new or inactive users):
    * cannot edit any comment, even their own
    * can see revisions of any comment, except for revisions hidden by an insider [3]
    * cannot hide any revision, even their own, so they still have to ask admins to hide sensitive info if any

[1] https://screenshots.firefox.com/WqC5RhA0S1KQmNyA/bmo-web.vm (the Hide This Revision checkbox is only visible to insiders)
[2] https://screenshots.firefox.com/K2QZYsVlfwvc5haS/bmo-web.vm
[3] https://screenshots.firefox.com/kPFKrKKs78X7ek6F/bmo-web.vm

Comment 13

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

FYI, :caitmunster

Comment 14

[Kohei Yoshino [:kohei]]

Ugh, sorry, this was the implementation bug. Redirecting folks to admin Bug 1484477...

Comment 15

[Kohei Yoshino [:kohei]]

Attachment: GitHub Pull Request

Comment 16

[Kohei Yoshino [:kohei]]

Attachment: GitHub Pull Request for schema change only

Comment 17

[Kohei Yoshino [:kohei]]

Merged to master \o/

Comment 18

[Dylan Hardison [:dylan] (he/him)]

Only the schema is going out first, we'll resolve this when the main code lands.

Comment 19

[Kohei Yoshino [:kohei]]

Merged to master, again \o/