Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Section Editor assigned as an Author (only) can access discussions that they are not participants in #3666

Closed
carzamora opened this issue May 5, 2018 · 32 comments
Labels
Bug:1:Low A bug that does not have a severe consequence or affects a small number of users.

Comments

@carzamora
Copy link
Contributor

I have a General Editor role, is like a Section Editor role with this permissions:
editorissue

First problem: A General Editor has permission for edit or delete discussions even when the discussion is not open by him/her, and can edit the content of the message.

You need to remember a Editor can submit a manuscript (in my case General Editors have these roles: General Editor, Author, Reviewer, Reader).

Second problem: When a General Editor sends a submission as an Author, he/she is able to view, delete and edit discussion even when they are not assigned to this discussion.

If you need more info I am happy to help you!

@NateWr
Copy link
Contributor

NateWr commented May 8, 2018

Are you using 3.1.1? We introduced significant permissions changes in #3130 so that permissions are now determined by the user's assigned role in the submission.

If you have a Section Editor who is making a submission, they should not be assigned an editorial role in that submission. They should only be assigned an author role. This should prevent them from seeing submission details that an author would not normally be able to see.

@carzamora
Copy link
Contributor Author

Yes, I am using 3.1.1! I know about some of this changes and was weird when I found these problems... I thought maybe this was not seen before.

Of course, in this submission, this editor only has Author role. It's possible that issue just happens with past submission, I mean submissions submitted before updated to 3.1.1?

Please don't close this issue yet; I need to try with another submission to verify, and check if this happened in news submissions, thanks @NateWr

@NateWr NateWr added the Bug:1:Low A bug that does not have a severe consequence or affects a small number of users. label May 9, 2018
@NateWr
Copy link
Contributor

NateWr commented May 9, 2018

I was able to confirm this. A Section Editor assigned to a submission as an Author (only) is able to see discussions that they are not participants in.

@NateWr NateWr changed the title [OJS] It is necessary to restrict the permissions of the Section Editor Section Editor assigned as an Author (only) can see discussions that they are not participants in May 9, 2018
@carzamora
Copy link
Contributor Author

carzamora commented May 9, 2018

But only able to see? in my case, I detect this user can see, edit or delete the discussions (basically have permission as Editor more than Author)
image

@asmecher asmecher added this to the OJS/OMP 3.1.1-1 milestone May 9, 2018
@NateWr NateWr changed the title Section Editor assigned as an Author (only) can see discussions that they are not participants in Section Editor assigned as an Author (only) can access discussions that they are not participants in May 10, 2018
@NateWr
Copy link
Contributor

NateWr commented May 14, 2018

According to QueriesAccessHelper::getCanListAll(), managers and subeditors are allowed to see all discussions. I'm wondering if this is too permissive.

Imagine a scenario where an Editor (JM) and Section Editor (SE) are both assigned to a submission. As an Author, I can open a discussion and only assign myself and the Editor (JM). At that point, I think I would have a reasonable expectation that the Section Editor (SE) would not have access to that discussion. However, it appears that they would.

Does anyone have thoughts about whether it's important that a Section Editor (SE) can see every discussion? @asmecher did you have any particular use-cases in mind? Are there any objections to limiting this to the Editor (JM) role?

@asmecher
Copy link
Member

Long term, our policy for section editors is that they have the same rights as full editors, but only when they've been assigned to the submission. That's recently gotten eroded somewhat, e.g. with the option for section editors to recommend only rather than recording decisions. I have no objection to a change that requires SEs to be assigned in order to list/see discussions but I think some closer-to-the-users feedback might be needed, e.g. @jmacgreg and @stranack.

@NateWr
Copy link
Contributor

NateWr commented May 17, 2018

I have no objection to a change that requires SEs to be assigned in order to list/see discussions

Can I just clarify, do you mean assigned to the submission? So a SE should be able to see any discussion in a submission to which they're assigned, regardless of whether they're a participant?

I'm a little nervous about how this deviates from the kind of email-like behavior a user might expect, leading users to think a conversation is limited to some participants when it's not. Perhaps we should design the participants list with any assigned JM/SE pre-selected as a participant and unable to be removed?

@asmecher
Copy link
Member

No, I meant to the discussion, not the submission. (As things currently stand, they get that permission by being assigned to the submission.) Just so I'm clear, you're proposing that full editors/managers should be able to create discussions that don't include SEs, and that in that case SE's would not be able to view/participate in the discussion, correct?

@NateWr
Copy link
Contributor

NateWr commented May 18, 2018

Just so I'm clear, you're proposing that full editors/managers should be able to create discussions that don't include SEs, and that in that case SE's would not be able to view/participate in the discussion, correct?

I'm proposing that anyone assigned to the submission should be able to create a discussion and choose the participants. Only discussion participants would be able to access a discussion, except Journal Managers, who can access any discussion.

(I also think there's a strong case for not giving Journal Managers this surveillance capability, but I appreciate that might impact on the day-to-day work of a lot of journals.)

@jmacgreg
Copy link
Contributor

100% agreed with Nate's comment above. This is how I would expect this feature to work.

@NateWr
Copy link
Contributor

NateWr commented May 18, 2018

@jmacgreg can I clarify: do you support the exception which allows Journal Managers to see any discussion?

@jmacgreg
Copy link
Contributor

Sorry - I support having any assigned participant being able to start a discussion; and only discussion participants being able to see that discussion. I don't have much of an opinion on whether JMs should be able to see everything or not - I'd want to check with some production journals first (and can do so if you like).

NateWr added a commit to NateWr/pkp-lib that referenced this issue May 18, 2018
This commit takes submission assignments into account when determining
whether a manager or subeditor can access a discussion. Only managers are
allowed to access discussions they are not participants in, and only when
they don't have a lower-level assignment on the submission
@ajnyga
Copy link
Collaborator

ajnyga commented May 18, 2018

Just raising the case of open reviews.
There the editor (journal editor or section editor) will probably want to see what the author and the reviewer are discussing.

@asmecher
Copy link
Member

Agreed re: what you've described for section editors, Nate. I think we'd probably have to do quite a bit of a review and some work if we were to limit full editors, though (not to say it's impossible).

NateWr added a commit to NateWr/pkp-lib that referenced this issue May 18, 2018
This commit takes submission assignments into account when determining
whether a manager or subeditor can access a discussion. Only managers are
allowed to access discussions they are not participants in, and only when
they don't have a lower-level assignment on the submission
@NateWr
Copy link
Contributor

NateWr commented May 18, 2018

PR:
#3712

Tests only:
pkp/ojs#1974

@NateWr
Copy link
Contributor

NateWr commented May 21, 2018

All checks have passed. @bozana sorry to pile on, but since Alec is away at a conference can you code review this one too?

NateWr added a commit to NateWr/pkp-lib that referenced this issue May 21, 2018
@asmecher
Copy link
Member

(Thanks for covering for me, @NateWr and @bozana! I'm trying to keep the backlog from growing too badly over this one.)

NateWr added a commit to NateWr/pkp-lib that referenced this issue May 22, 2018
This commit takes submission assignments into account when determining
whether a manager or subeditor can access a discussion. Only managers are
allowed to access discussions they are not participants in, and only when
they don't have a lower-level assignment on the submission
NateWr added a commit to NateWr/pkp-lib that referenced this issue May 22, 2018
NateWr added a commit to NateWr/pkp-lib that referenced this issue May 23, 2018
NateWr added a commit to NateWr/pkp-lib that referenced this issue May 23, 2018
@NateWr
Copy link
Contributor

NateWr commented May 23, 2018

Thanks @bozana. Just that one outstanding comment, others address. Tests re-running.

NateWr added a commit that referenced this issue May 24, 2018
#3666 Prevent dual-assigned editors froma ccessing discussions
NateWr added a commit to NateWr/pkp-lib that referenced this issue May 24, 2018
NateWr added a commit to NateWr/pkp-lib that referenced this issue May 24, 2018
NateWr added a commit to NateWr/pkp-lib that referenced this issue May 24, 2018
NateWr added a commit that referenced this issue May 24, 2018
#3666 Prevent dual-assigned editors from accessing discuss…
NateWr added a commit that referenced this issue May 24, 2018
#3666 Prevent dual-assigned editors from accessing discuss…
@NateWr
Copy link
Contributor

NateWr commented May 24, 2018

All merged to master, ojs-stable-3_1_1 and omp-stable-3_1_1.

@NateWr NateWr closed this as completed May 24, 2018
ppv1979 pushed a commit to ppv1979/pkp-lib that referenced this issue May 25, 2018
@ajnyga
Copy link
Collaborator

ajnyga commented Jul 12, 2018

Section editors can not see "comments for the editor" anymore, see https://forum.pkp.sfu.ca/t/ojs-3-1-1-2-comment-for-the-editor-not-showing/46426/6

This is a problem for journals that mainly use section editors instead of actual editors. Especially if submissions are assigned automatically.

@bozana
Copy link
Collaborator

bozana commented Jul 12, 2018

@NateWr, would then be the solution to treat Pre-Review Discussions differently/extra?

@ajnyga
Copy link
Collaborator

ajnyga commented Jul 12, 2018

Or assign all existing pre-review discussions automatically to the assigned section editor?

@bozana
Copy link
Collaborator

bozana commented Jul 12, 2018

Yes, this would then be needed every time an editor is assigned/unassigned?

@ajnyga
Copy link
Collaborator

ajnyga commented Jul 12, 2018

Yeah I think so? Of course if there would be a way of adding a "type" to some discussions then you could target those instead of all discussions in a particular stage.

I mean something like

DISCUSSION_TYPE_PRIVATE
DISCUSSION_TYPE_PUBLIC (not a good name, but basically a type of discussion that becomes visible for all stage participants automatically)

At the moment only the "comments of the editor" would use the second type, but we would then have a ready solution to handle similar cases later if they appear.

Just a thought

@NateWr
Copy link
Contributor

NateWr commented Jul 12, 2018

I'd like to see this as part of the assigning workflow. I think it makes sense to generalise this feature. When assigning a participant, the user should see a list of discussions with checkboxes to select which ones the new participant should be assigned to.

Anyone want to file that?

@ajnyga
Copy link
Collaborator

ajnyga commented Jul 12, 2018

How about the automatic assignments. I mean situations where a section editor is responsible for a particular section and receives those submissions based on the section's settings?

@NateWr
Copy link
Contributor

NateWr commented Jul 12, 2018

Wherever we're assigning users to the comments-to-the-editor discussion, we should make sure automatically assigned section editors are included.

@ajnyga
Copy link
Collaborator

ajnyga commented Jul 12, 2018

Yes but the scenario I am thinking is that:

  • you have section A with a responsible section editor B
  • someone submits to section A and the submission is automatically assigned to the section editor B
  • section editor B opens the submission, but does not see the comments to the editor, because she is not participant in the discussion

In these cases the actual editor who can see the comments may not even open the whole submission, because there is already a section editor taking care of it.

@NateWr
Copy link
Contributor

NateWr commented Jul 12, 2018

Section editor B should be assigned to this discussion when the discussion is created.

@ajnyga
Copy link
Collaborator

ajnyga commented Jul 12, 2018

But the discussion is created before the automatic assignment happens. It is created during the submission process in the step 1.

@carzamora
Copy link
Contributor Author

I think @ajnyga approach is a few more simple for Editors because do not affect the current process, they not need to think what they need to do, keeping the process like they know now

@NateWr
Copy link
Contributor

NateWr commented Jul 13, 2018

But the discussion is created before the automatic assignment happens. It is created during the submission process in the step 1.

Then we can update the discussion participants when the submission is completed.

It'd be good to capture any further discussion in its own issue, so I've opened one here: #3910

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug:1:Low A bug that does not have a severe consequence or affects a small number of users.
Projects
None yet
Development

No branches or pull requests

6 participants