pkp/pkp-lib#3666 Prevent dual-assigned editors froma ccessing discussions #3712
Conversation
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
…in discussions they're not assigned to
| // Managers have all access to all submissions. | ||
| $fileAccessPolicy->addPolicy(new RoleBasedHandlerOperationPolicy($request, ROLE_ID_MANAGER, $roleAssignments[ROLE_ID_MANAGER])); | ||
| // Managers can access all submission files as long as the manager has not | ||
| // has not been assigned to a lesser role in the stage. |
There was a problem hiding this comment.
minor typo: has not (in the first comment line) and has not (in the second comment line)
| @@ -184,9 +211,11 @@ function buildFileAccessPolicy($request, $args, $roleAssignments, $mode, $fileId | |||
| $subEditorFileAccessPolicy = new PolicySet(COMBINING_DENY_OVERRIDES); | |||
| $subEditorFileAccessPolicy->addPolicy(new RoleBasedHandlerOperationPolicy($request, ROLE_ID_SUB_EDITOR, $roleAssignments[ROLE_ID_SUB_EDITOR])); | |||
|
|
|||
| // 2) ... but only if they have been assigned to the requested submission. | |||
| // 2) ... but only if they have been assigned as a subeditor to the requested submission. | |||
| import('lib.pkp.classes.security.authorization.internal.UserAccessibleWorkflowStageRequiredPolicy'); | |||
| $subEditorFileAccessPolicy->addPolicy(new UserAccessibleWorkflowStageRequiredPolicy($request)); | |||
There was a problem hiding this comment.
It's from earlier, but I am wondering at the moment why only this policy and not the whole WorkflowStageAccessPolicy here 🤔
There was a problem hiding this comment.
I don't think it's necessary here. Having looked at it, it seems redundant. We only call the UserAccessibleWorkflowStageRequiredPolicy here so that we have the ASSOC_TYPE_ACCESSIBLE_WORKFLOW_STAGES in the authorized context objects. The AssignedStageRoleHandlerOperationPolicy should provide the only check we need.
I think we could add WorkflowStageAccessPolicy as well without causing any harm, but I am a bit nervous. The WorkflowStageAccessPolicy relies on the overall roles in a context, not the assigned role in the submission, so I'm not 100% that it is providing a useful check here.
It would also lead to extra database reads. (SubmissionRequiredPolicy will be called twice.)
🤷♂️
| $query = $queryDao->getById($queryId); | ||
| if ($query && $query->getHeadNote()->getUserId() == $this->_user->getId()) return true; | ||
| // Assistants, authors and reviewers are allowed, if they created the query | ||
| if ($this->hasStageRole($query->getStageId(), array(ROLE_ID_ASSISTANT, ROLE_ID_AUTHOR, ROLE_ID_REVIEWER))) { |
There was a problem hiding this comment.
should a SUB_EDITOR be able to edit if he/she created the query?
There was a problem hiding this comment.
Ooh, good catch. Looks like before subeditors were allowed even if they didn't create the query. I'll add that back in.
| // as long as they have Manager-level access to the workflow stage | ||
| $accessibleWorkflowStages = $this->getAuthorizedContextObject(ASSOC_TYPE_ACCESSIBLE_WORKFLOW_STAGES); | ||
| $managerAssignments = array_intersect(array(ROLE_ID_MANAGER), $accessibleWorkflowStages[$query->getStageId()]); | ||
| if (!empty($managerAssignments)) return AUTHORIZATION_PERMIT; |
There was a problem hiding this comment.
@NateWr , I had a question for this part as well, but I cannot find it any more -- apparently I haven't added it at all :-P Sorry :-( I was wondering if we should maybe use here the new AssignedStageRoleHandlerOperationPolicy :-)
There was a problem hiding this comment.
Can you call another ...Policy inside of a ...Policy::effect()? How does that work?
There was a problem hiding this comment.
Hmmm... Yes, you are right... I've just seen that some policies add some other policies, but they usually do that in the constructor and have no effect function... -- I am not sure how this works... Only the PKPSiteAccessPolicy uses both: constructor with additional policies and effect function... Hmmm...
OK, we can leave it as it is...
No description provided.