Addons-server: [Reviewer Tools] Anonymising Reviewer Emails

[erosman]

It is a fact the developers are often not happy if their extensions are rejected during a review. While it should not cause any further complications, in reality that is not always the case.
There are instance where the dissatisfied developers resort to targeting the reviewer via other AMO systems as a consequence.
There are even situations where developer blames the addon-reviewer for issues not even related to the review or reviewer.

AMO systems such as reviewer's addon user-reveiw, addon support & addon Abuse Reports are then targeted for harassment and retaliation.

For example:

Harassment of reviewer via user-review system:
https://github.com/mozilla/addons/issues/962
https://github.com/mozilla/addons-server/issues/9686

Harassment of reviewer via add-on support pages:
https://github.com/erosman/support/issues/47
https://github.com/erosman/support/issues/51

Harassment of reviewer via add-on Abuse Reports (since 2015)
https://reviewers.addons.mozilla.org/en-US/reviewers/abuse-reports/firemonkey

Harassment of reviewer via discourse.mozilla.org
(there have been instances in the past)

IMHO, review emails could be generalised and anonymised (similar to auto-approval) in order to avoid such occurrences and to enable reviewers to review add-ons without the fear of becoming a victim of harassment or reprisal from unhappy developers.

@caitmuenster @wagnerand @kewisch

[jvillalobos]

We talked through this today, and decided complete anonymity could be problematic when developers interact with multiple reviewers. It wouldn't be clear when you're talking with a different reviewer or an issue has been escalated. What we decided as a better solution is for reviewers to use a separate alias that is not connected to their developer alias.

This solution would require:

• [ ] A new "Reviewer name" field in the Edit My Profile page. Same rules as display name. Only appears if you have any of the review permissions activated. https://github.com/mozilla/addons-frontend/issues/7806
• [x] If it's not set, the display name becomes the fallback.
• [x] The Recent Activity page on DevHub should use "Reviewer name" in review logs and not link to the user page.
• [x] The Status and Versions page on DevHub should use "Reviewer name" and not link to the user page.
• [x] The "Reviewer" field in all review email should use "Reviewer name".
• [x] In the reviewer tools, the normal display names should be used.

/cc @muffinresearch @diox for thoughts on how much effort this would take.

[erosman]

Sounds good.
So it would be just an alias and no new account needs to be created, correct?

[diox]

The Recent Activity page on DevHub should use "Reviewer name" in review logs.
The Status and Versions page on DevHub should use "Reviewer name".

With the way activity logs work currently, this would use the "Reviewer name" from when the action was made. If it's changed afterwards it wouldn't change the log from past actions. Is that ok?

[erosman]

AFA my request, the email is the real cause for concern. The rest are just internal administration. One suggestion for the internal administration could be to suffix alias, ONLY when displaying data (not on the logs) .eg. username (reviewer alias)

[diox]

Yes, that's typically how we'd handle such a thing, but Activity Logs are a bit weird and the full HTML of the activity is generated right when the activity occurs. Good news though, I've looked a little deeper and the HTML for the add-on review logs do not include the user name - the "by {user}" that is shown is a separate thing, that can be altered at display time. We'll need a little hack to determine which actions this should apply to, but that's doable.

@jvillalobos should be a day or two of work. We should be able to fit that in a milestone this quarter.

[jvillalobos]

If it's changed afterwards it wouldn't change the log from past actions. Is that ok?

That's even better, I think. This way, developers can't map past reviews to new aliases and changing your alias gives you a sort of new identity.

[jvillalobos]

Okay, this isn't large enough to be a project on its own, I think, so I'll flag this as a P2 so we don't lose track of it. It's not that urgent, but it'd be great to get it done before end of April.

[caitmuenster]

Sounds good. In the meantime, I can update the automated messages to not reference specific reviewers.

[diox]

I'll try to take care of it this week, we'll see how it goes since it has a few moving parts.

[diox]

Note for posterity:

• None of the activity log formats we define expose the user directly - only a few restricted to reviewers themselves or admins do.
• The devhub feeds only expose the activity log to_string() and therefore, per previous item, don't expose the user responsible

The emails & devhub pages listing activity logs do include the user, but they do so "manually", that's where a change is needed

[diox]

@jvillalobos I've got a work in progress branch implementing this, but before we move any further on this @eviljeff and I are slightly worried about the added complexity around user names that this represents. This has the potential for new edge cases that would have pretty bad consequences (like accidentally exposing the actual reviewer name somewhere while the reviewer thought he was anonymous).

Is there any chance this could be achieved by simply asking reviewers to have a separate account for their reviewer duties ? It makes its more cumbersome to log in, but would provide a clear separation since their developer account would not have reviewer rights. Only drawback I see with this alternate approach is that it would make it easier for reviewers to approve their own content, but they can already use a separate account if they want to anyway.

[jvillalobos]

I did consider it, but unfortunately we have many reviewers (theme and content in particular) who would have a very difficult time managing multiple AMO/Firefox accounts. We would also need to find a way not lo lose the very long review history of all current reviewers.

If you have any scenarios in mind where the real reviewer username is going to be exposed, I'd be interested in hearing them. However, it sounds like those bad cases end up in a situation no worse than what we have today, and an exposed reviewer can probably solve it by changing their alias again.

[diox]

If you have any scenarios in mind where the real reviewer username is going to be exposed, I'd be interested in hearing them

Well I don't know of any, but the implementation is going to be fairly fragile. An example: just before submitting my pull request I noticed that because I was showing the reviewer alias for every activity log, developers could compare the activity log for a rating with the actual rating and deduce the real account name of the reviewer. I've obviously fixed that, but there might be other instances of this in the future if we're not careful when adding new activity log types, for instance.

However, it sounds like those bad cases end up in a situation no worse than what we have today, and an exposed reviewer can probably solve it by changing their alias again.

Unfortunately I was wrong before (I corrected in another comment) : we don't store the user name in the activity log, only a reference to the user profile, so if they change their alias a malicious developer that has access to an old activity log of that reviewer will know their new alias as well. Changing that is a much longer and difficult task.

[jvillalobos]

so if they change their alias a malicious developer that has access to an old activity log of that reviewer will know their new alias as well.

We can live with that. Bad cases are still rare, so we shouldn't do too much at first.

[wagnerand]

Is there any chance this could be achieved by simply asking reviewers to have a separate account for their reviewer duties ?

I'd prefer that as well. It also improves security/account hardening, because reviewer accounts would be used exclusively for reviewing, not for other developer-related activities.

[diox]

QA: see https://github.com/mozilla/addons-server/issues/11002#issuecomment-476845441

The frontend part to allow users to edit their reviewer name is not done yet, that's https://github.com/mozilla/addons-frontend/issues/7806, but as a workaround for now it should be possible to modify it for a user by using the user admin tool (obviously requires admin permissions).

[diox]

Re-opening because of an issue @willdurand spotted with the way we handle empty values in the API.

[AlexandraMoga]

I've verified this on -dev by using the PATCH account API, which, unlike users admin, also allowed me to test with various reviewer permissions directly.

Results:

• reviewer_name field is present in the account details for users with reviewer permissions
• if not set, reviewer_name falls back to the display_name value (the default value if not set, or the custom value set by the user)
• the recent activity and the manager versions page in dev hub are displaying the reviewer_name (not linked to the profile page) for any reviewer action
• reviewer actions in Rev Tools are still linked to the reviewer's display name
• email notifications from reviewers have a reference to the reviewer_name not the display_name
• non-review activities performed by reviewer accounts (i.e. ratings, submitted addons, collections) will show a reference to the display_name

Some additional notes:

• the reviewer_name field inherits some validations from the 'display_name` (i.e minimum length, the requirement for printable characters etc) but it can be blank (done in #11152)

Questions for @diox

• rating moderators do not have a reviewer_name field linked to their account details => rating moderation activities will still contain a reference to their display name (in the recent activity page for example). Was this group out of scope?
• a small edge case with reviewers that are also developers: if they leave a developer reply to a review action in the manage versions page, the message will display their reviewer alias, not their display name

[diox]

rating moderators do not have a reviewer_name field linked to their account details => rating moderation activities will still contain a reference to their display name (in the recent activity page for example). Was this group out of scope?

It's not a reviewer<->developer interaction so I didn't consider it.

a small edge case with reviewers that are also developers: if they leave a developer reply to a review action in the manage versions page, the message will display their reviewer alias, not their display name

Yeah, developer reply are considered a reviewer action in the existing code I'm relying on for this feature. I had the edge case in mind but I don't think it matters.