Pkp-lib: Reviewers' identities visible to other reviewers in discussion

[amandastevens]

Issue 2976 introduced a discussion grid for reviewers and the ability to create discussions between editors, reviewers, and authors. Participants' identities are hidden automatically from other participants, depending on the review type. The issue says is it's supposed to work as follows:
• In open reviews: all users see each other, including the reviewers, they see all other open reviewers (what do you think?)
• in blind reviews: authors only see the editors; reviewers see the editors; editors see everybody
• in mixed reviews (both blind and open): authors only see editors and open reviewers: blind reviewers only see editors; open reviewers see authors and editors but not the blind reviewers

I tested this on two different OJS 3.1.2 installs and found that reviewers' identities were exposed to other reviewers for both blind and double blind submissions. As an editor I created a discussion topic in the review stage and added the three reviewers to the discussion. The reviewers had all been requested to do a blind review. Then I logged in as one of the reviewers and I could see the names of the other two reviewers in the discussion. I tried this again with a double blind review and the same thing occurred.

Here is a screenshot of the discussion topic when I'm logged in as a reviewer.

[ajnyga]

Just to make sure. This happens if an editors starts a discussion and adds all the reviewers as participants?

The discussion system currently works so that all people added to a discussion can see each other. What blind reviewers can not do is to start that kind of discussions themselves. They can not see each other as suggested participants.

But we already discussed with Alec that some sort of anonymization could be introduced in case an editor starts a discussion like that. I will come up with a pr soon.

[ajnyga]

So looked into this and it is complicated.

Everything works ok as long as the editor does not start a discussion with several blind editors.

If such discussion is created, then it is hard to keep that anonymized. I can easily hide the names from the participants list, but if one of the reviewers answers the message, her name will show up in the "Last reply" and "From" fields. Also I think that it is a bad decision to hide the names because all participants will see each other's answers. So a reviewer might just see the editors name, but the answer ends up to other reviewers as well.

So probably we need a combination of warnings (for the editor when she selects several blind reviewers to a discussion and for the reviewers if they receive a discussion with several blind reviewers and/or authors) and anonymized user names. So instead Firstname Lastname you see something like Reviewer #1234.

I have been talking about this with Alec in Slack, so let's see first what he thinks.

[asmecher]

@amandastevens, would you be OK with a warning before a user is allowed to create a new discussion is created in a way that breaks anonymity? I think a full solution to this will only be possible once we overhaul the email communication system (e.g. permitting OJS to receive email replies from users). That would allow us to provide email-based communication between users that shouldn't know each others' identities, which we can't really offer otherwise.

[NateWr]

I've bumped this up to Critical and assigned it to the 3.1.2-2 milestone. During the sprint in Pittsburgh we saw a case where Blind reviewers were exposed to the Author before any discussions were created. This seems to undermine anonymous review.

This may have been because the Author had other editorial roles in the system, which would mean that this issue doesn't effect very many people. But regardless the create discussions form should show/hide info based on their assigned role in the submission, like other parts of the workflow.

[ajnyga]

Most likely it is a situation where the author is also an editor in the same context.

If current user is editor, all reviewers are considered: https://github.com/pkp/pkp-lib/blob/master/controllers/grid/queries/form/QueryForm.inc.php#L234

If current user is author, only open reviewers are considered: https://github.com/pkp/pkp-lib/blob/master/controllers/grid/queries/form/QueryForm.inc.php#L253

I think the code was written back when we always allowed editors access to anything. So it uses checks like $user->hasRole(array(ROLE_ID_MANAGER), $context->getId()) instead of array_intersect(array(ROLE_ID_MANAGER), $userRoles).

So without testing, I would say that an easy fix would be to add if ($user->hasRole(array(ROLE_ID_SITE_ADMIN), CONTEXT_SITE) || array_intersect(array(ROLE_ID_MANAGER), $userRoles) || array_intersect(array(ROLE_ID_SUB_EDITOR), $userRoles) ) { to https://github.com/pkp/pkp-lib/blob/master/controllers/grid/queries/form/QueryForm.inc.php#L234

[ajnyga]

But, I think that the depending on the outcome of the discussion above, the whole way of creating the list of participants should be considered. There are a lot of different cases that are hard to think of and while the reviewers do not have an actual stage assigment and there are different kinds of reviewers, things get complicated quickly.

[NateWr]

Yes, I think your suggestion above for the array_intersect will resolve the issue.

Can we rename $userRoles to $userRoleAssignments? Just because we use $userRoles elsewhere in the system to identify roles-in-context but here we want roles-in-submission. :+1:

[asmecher]

@NateWr, do you mind if I assign you to this one in order to get your proposed fix merged for 3.1.2-2? If you want to address the critical issue and file the rest separately, that's OK with me.

[NateWr]

Thanks for doing the hard work on this one @ajnyga. I'll backport to stable once the test has passed.

PR:
https://github.com/pkp/pkp-lib/pull/5119

Tests only:
https://github.com/pkp/ojs/pull/2487

[ajnyga]

"hard work"

[NateWr]

Also, I can confirm that the original issue (reviewers seeing other reviewers) only occurs when an editor adds them all to the same discussion. This is less critical, but I'd suggest one of the following:

• [ ] Block discussions that involve more than one reviewer. Return an error that says you can't send a discussion to more than one reviewer.
• [ ] Create a separate discussion for each assigned reviewer. When more than one reviewer is selected, a note should appear in the form to let them know this will happen.

I'm kind of in favor of the first one.

[ajnyga]

The bad thing with option 1 is that it will also block you from creating a new review that involves several open review requests. But I guess that can be taken into account.

[NateWr]

Merged and cherry-picked to stable. I've taken the Critical Issue label off and assigned this to the 3.3 (unconfirmed) milestone.

[dihagan]

I would prefer a warning on this issue, for cases when there is a group of reviewers (say an advisory committee) who need to come to a consensus.

[BagiraHun]

There is a bug in https://github.com/pkp/pkp-lib/blob/master/controllers/grid/queries/form/QueryForm.inc.php#L243

Due type mismatch of the two value, you have to cast them to same integer type (int) or you have to use == operator.

My solution is: https://github.com/BagiraHun/pkp-lib/blob/blind_reviewer_author_patch/controllers/grid/queries/form/QueryForm.inc.php#L243

[asmecher]

@BagiraHun, are you sure that the change you're proposing fixes the issue being described here? Or is it another (but perhaps related) issue? A bit more information about what this resolves would be helpful. Thanks!

[BagiraHun]

@asmecher, we are currently using the latest (v3.2.0.2) version of OJS. Most of our journals's "Default Review Mode" setting has "Double-blind" param. Our workflow tester followed the reviewer's steps in OJS as a Reviewer of the article. At step 3 (Download & Review) she could make a "Review discussion" with the Author as well. Although the Review Mode was Double-blind the Author got the message with the name of the Reviewer.

As a developer, I tried to figured out what cause this problem, and I've found how the list of Participant is handled. I've debugged line-to-line this part of the code, and found that however $reviewAssignment->getReviewerId() == $user->getId() is TRUE, but $reviewAssignment->getReviewerId() === $user->getId() IS not. This source code has similar solution in line 288. So if I changed this, the above discussion participant list has worked as I wanted.

[asmecher]

Thanks, @BagiraHun; I haven't verified the problem locally but it sounds like you've done your homework and relaxing the current exact test is a no-risk proposition. I've committed a change along the lines of what you propose for release with OJS/OMP/OPS 3.2.0-3 and onwards.

[bozana]

Hi all, and maybe above all @ajnyga, this issue is still there i.e. the editor has the possibility to start a review discussion with all participants (author and all reviewers) and in that case all participants see the names of each other in the participant list but also the usernames in the column "From" and "Last reply".
It seems to be a bigger problem to anonymize those names there, right @ajnyga? Would it be easier to allow editor to start a discussion only with one participant? Or as @dihagan suggestion above, to only display a warning for the editor? -- Any easy/easier way to solve it for 3.3...
I would take a look but any hint is very welcome...

[ajnyga]

I think the discussion above agreed (@NateWr ?) that a clear warning would be sufficient here. So if you have anonymized reviewers and authors in the same discussion you would have to confirm that you want to have these participants despite of anonymized review.

The discussion feature is complicated because reviewers are not stage participants and there are a lot of cases to consider.

[NateWr]

I had to remind myself of what's been done here in the past. So the issue before was that an author or reviewer could create a discussion with another author or reviewer. The names appeared in the participant list when creating a new discussion.

That part has been fixed. An author shouldn't be able to create a discussion with a reviewer and vice-versa. But the editor can still create discussions with both an author and a reviewer. And if they do so, the information will be exposed.

I think the easiest and safest approach is to throw a validation error if the editor tries to create a discussion that includes an author and one or more anonymous reviewers.

Suggested error message:

You can not create a discussion that includes the author and an anonymous reviewer, because the reviewer's identity will be revealed.

I think there have been cases where editors have asked for the ability to have a discussion between reviewers/authors. But in my view, we would need to do more work on this to ensure anonymity, because reviewers/authors are likely to end their messages with their name. So if we pursued a feature like this, there'd need to be some step where an editor previewed/edited the message before sharing it with the author.

[ajnyga]

I believe this was a feature that was asked for open reviews. So we should not block it completely.

Edit: I think the original issue was that an author who also had an editor role could create a discussion with blind reviewers and that was fixed above by checking the stage assigments of the current submission.

In open reviews the author should be able to start a discussion with the reviewer and vice versa.

However, currently the editor can start a discussion where both the author and anonymized reviewers are involved. This is of course a selection the editor does, but some kind of warning should be present. Or, as suggested above by Nate, we could throw a validation error if _anonymized reviewers_ and the author are in the discussion but allow discussion with open reviewers and authors.

[NateWr]

The validation error should only be shown for anonymous and double anonymous review assignments.

[bozana]

Thanks @ajnyga and @NateWr! I will take a look...