From 7e5959da7e3a0ec19d981cc267ed1874704c27be Mon Sep 17 00:00:00 2001 From: yves Date: Wed, 11 Dec 2024 18:21:13 -0400 Subject: [PATCH 1/5] pkp/pkp-lib#10486 adds validation to prevent an author of a rejected submission from editing metadata in the submission. Signed-off-by: yves --- classes/services/PKPSubmissionService.inc.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/classes/services/PKPSubmissionService.inc.php b/classes/services/PKPSubmissionService.inc.php index b8fb44dad65..0f5b2828263 100644 --- a/classes/services/PKPSubmissionService.inc.php +++ b/classes/services/PKPSubmissionService.inc.php @@ -795,6 +795,12 @@ public function delete($submission) { public function canEditPublication($submissionId, $userId) { $stageAssignmentDao = DAORegistry::getDAO('StageAssignmentDAO'); /* @var $stageAssignmentDao StageAssignmentDAO */ $stageAssignments = $stageAssignmentDao->getBySubmissionAndUserIdAndStageId($submissionId, $userId, null)->toArray(); + $submission = $this->get($submissionId); + $isAuthorOfTheSubmission = !empty($stageAssignmentDao->getBySubmissionAndRoleId($submissionId, ROLE_ID_AUTHOR, null, $userId)->toArray()); + // If the submission is declined and the current user is an author of the submission + if ($submission->getStatus() == STATUS_DECLINED && $isAuthorOfTheSubmission) { + return false; + } // Check for permission from stage assignments foreach ($stageAssignments as $stageAssignment) { if ($stageAssignment->getCanChangeMetadata()) { From 5df4624c9f7fa64da0b8595b0cde79ce90bbc311 Mon Sep 17 00:00:00 2001 From: yves Date: Thu, 12 Dec 2024 11:42:10 -0400 Subject: [PATCH 2/5] pkp/pkp-lib#10486 renames the variable $isAuthorOfTheSubmission to $userIsAuthor Signed-off-by: yves --- classes/services/PKPSubmissionService.inc.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/classes/services/PKPSubmissionService.inc.php b/classes/services/PKPSubmissionService.inc.php index 0f5b2828263..307b260d400 100644 --- a/classes/services/PKPSubmissionService.inc.php +++ b/classes/services/PKPSubmissionService.inc.php @@ -796,11 +796,11 @@ public function canEditPublication($submissionId, $userId) { $stageAssignmentDao = DAORegistry::getDAO('StageAssignmentDAO'); /* @var $stageAssignmentDao StageAssignmentDAO */ $stageAssignments = $stageAssignmentDao->getBySubmissionAndUserIdAndStageId($submissionId, $userId, null)->toArray(); $submission = $this->get($submissionId); - $isAuthorOfTheSubmission = !empty($stageAssignmentDao->getBySubmissionAndRoleId($submissionId, ROLE_ID_AUTHOR, null, $userId)->toArray()); - // If the submission is declined and the current user is an author of the submission - if ($submission->getStatus() == STATUS_DECLINED && $isAuthorOfTheSubmission) { - return false; - } + $userIsAuthor = !empty($stageAssignmentDao->getBySubmissionAndRoleId($submissionId, ROLE_ID_AUTHOR, null, $userId)->toArray()); + // If the submission is declined and the current user is an author of the submission + if ($submission->getStatus() == STATUS_DECLINED && $userIsAuthor) { + return false; + } // Check for permission from stage assignments foreach ($stageAssignments as $stageAssignment) { if ($stageAssignment->getCanChangeMetadata()) { From 4a084361365b6d8fd0875846f03d3ca102688c7e Mon Sep 17 00:00:00 2001 From: yves Date: Thu, 12 Dec 2024 17:06:50 -0400 Subject: [PATCH 3/5] pkp/pkp-lib#10486 changes parameter of `canEditPublication` method to receive a Submission object instead of a submission identifier and updates calls to this method. Signed-off-by: yves --- api/v1/submissions/PKPSubmissionHandler.inc.php | 2 +- .../internal/PublicationCanBeEditedPolicy.inc.php | 2 +- classes/services/PKPSubmissionService.inc.php | 9 ++++----- controllers/grid/users/author/AuthorGridHandler.inc.php | 2 +- pages/authorDashboard/PKPAuthorDashboardHandler.inc.php | 2 +- pages/workflow/PKPWorkflowHandler.inc.php | 2 +- 6 files changed, 9 insertions(+), 10 deletions(-) diff --git a/api/v1/submissions/PKPSubmissionHandler.inc.php b/api/v1/submissions/PKPSubmissionHandler.inc.php index fd30c36aa7e..7d3c7b40c17 100644 --- a/api/v1/submissions/PKPSubmissionHandler.inc.php +++ b/api/v1/submissions/PKPSubmissionHandler.inc.php @@ -698,7 +698,7 @@ public function editPublication($slimRequest, $response, $args) { // Prevent users from editing publications if they do not have permission. Except for admins. $userRoles = $this->getAuthorizedContextObject(ASSOC_TYPE_USER_ROLES); - if (!in_array(ROLE_ID_SITE_ADMIN, $userRoles) && !Services::get('submission')->canEditPublication($submission->getId(), $currentUser->getId())) { + if (!in_array(ROLE_ID_SITE_ADMIN, $userRoles) && !Services::get('submission')->canEditPublication($submission, $currentUser->getId())) { return $response->withStatus(403)->withJsonError('api.submissions.403.userCantEdit'); } diff --git a/classes/security/authorization/internal/PublicationCanBeEditedPolicy.inc.php b/classes/security/authorization/internal/PublicationCanBeEditedPolicy.inc.php index 1060c87b0da..d283fe69a61 100644 --- a/classes/security/authorization/internal/PublicationCanBeEditedPolicy.inc.php +++ b/classes/security/authorization/internal/PublicationCanBeEditedPolicy.inc.php @@ -37,7 +37,7 @@ public function effect() // Prevent users from editing publications if they do not have permission. Except for admins. $userRoles = $this->getAuthorizedContextObject(ASSOC_TYPE_USER_ROLES); - if (in_array(ROLE_ID_SITE_ADMIN, $userRoles) || Services::get('submission')->canEditPublication($submission->getId(), $this->_currentUser->getId())) { + if (in_array(ROLE_ID_SITE_ADMIN, $userRoles) || Services::get('submission')->canEditPublication($submission, $this->_currentUser->getId())) { return AUTHORIZATION_PERMIT; } diff --git a/classes/services/PKPSubmissionService.inc.php b/classes/services/PKPSubmissionService.inc.php index 307b260d400..668f315642b 100644 --- a/classes/services/PKPSubmissionService.inc.php +++ b/classes/services/PKPSubmissionService.inc.php @@ -788,15 +788,14 @@ public function delete($submission) { /** * Check if a user can edit a publications metadata * - * @param int $submissionId + * @param Submission $submission * @param int $userId * @return boolean */ - public function canEditPublication($submissionId, $userId) { + public function canEditPublication($submission, $userId) { $stageAssignmentDao = DAORegistry::getDAO('StageAssignmentDAO'); /* @var $stageAssignmentDao StageAssignmentDAO */ - $stageAssignments = $stageAssignmentDao->getBySubmissionAndUserIdAndStageId($submissionId, $userId, null)->toArray(); - $submission = $this->get($submissionId); - $userIsAuthor = !empty($stageAssignmentDao->getBySubmissionAndRoleId($submissionId, ROLE_ID_AUTHOR, null, $userId)->toArray()); + $stageAssignments = $stageAssignmentDao->getBySubmissionAndUserIdAndStageId($submission->getId(), $userId, null)->toArray(); + $userIsAuthor = !empty($stageAssignmentDao->getBySubmissionAndRoleId($submission->getId(), ROLE_ID_AUTHOR, null, $userId)->toArray()); // If the submission is declined and the current user is an author of the submission if ($submission->getStatus() == STATUS_DECLINED && $userIsAuthor) { return false; diff --git a/controllers/grid/users/author/AuthorGridHandler.inc.php b/controllers/grid/users/author/AuthorGridHandler.inc.php index bfe928f2d87..390ea7e1ccb 100644 --- a/controllers/grid/users/author/AuthorGridHandler.inc.php +++ b/controllers/grid/users/author/AuthorGridHandler.inc.php @@ -256,7 +256,7 @@ function canAdminister($user) { if ($submission->getDateSubmitted() == null) return true; // The user may not be allowed to edit the metadata - if (Services::get('submission')->canEditPublication($submission->getId(), $user->getId())) { + if (Services::get('submission')->canEditPublication($submission, $user->getId())) { return true; } diff --git a/pages/authorDashboard/PKPAuthorDashboardHandler.inc.php b/pages/authorDashboard/PKPAuthorDashboardHandler.inc.php index b75696f3186..59a3d3230b0 100644 --- a/pages/authorDashboard/PKPAuthorDashboardHandler.inc.php +++ b/pages/authorDashboard/PKPAuthorDashboardHandler.inc.php @@ -287,7 +287,7 @@ function setupTemplate($request) { // Check if current author can edit metadata $userRoles = $this->getAuthorizedContextObject(ASSOC_TYPE_USER_ROLES); $canEditPublication = true; - if (!in_array(ROLE_ID_SITE_ADMIN, $userRoles) && !Services::get('submission')->canEditPublication($submission->getId(), $user->getId())) { + if (!in_array(ROLE_ID_SITE_ADMIN, $userRoles) && !Services::get('submission')->canEditPublication($submission, $user->getId())) { $canEditPublication = false; } diff --git a/pages/workflow/PKPWorkflowHandler.inc.php b/pages/workflow/PKPWorkflowHandler.inc.php index 526a675af6a..c551ddd7930 100644 --- a/pages/workflow/PKPWorkflowHandler.inc.php +++ b/pages/workflow/PKPWorkflowHandler.inc.php @@ -145,7 +145,7 @@ function index($args, $request) { $currentStageId = $submission->getStageId(); $accessibleWorkflowStages = $this->getAuthorizedContextObject(ASSOC_TYPE_ACCESSIBLE_WORKFLOW_STAGES); $canAccessPublication = false; // View title, metadata, etc. - $canEditPublication = Services::get('submission')->canEditPublication($submission->getId(), $request->getUser()->getId()); + $canEditPublication = Services::get('submission')->canEditPublication($submission, $request->getUser()->getId()); $canAccessProduction = false; // Access to galleys and issue entry $canPublish = false; // Ability to publish, unpublish and create versions $canAccessEditorialHistory = false; // Access to activity log From e49e800c7d3189aaf97680d724de8f67ebd3cf33 Mon Sep 17 00:00:00 2001 From: yves Date: Fri, 13 Dec 2024 17:05:15 -0400 Subject: [PATCH 4/5] pkp/pkp-lib#10486 adds a check if the user is not an manager, applying the metadata editing restriction only to these cases. Signed-off-by: yves --- classes/services/PKPSubmissionService.inc.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/classes/services/PKPSubmissionService.inc.php b/classes/services/PKPSubmissionService.inc.php index 668f315642b..acdf791d651 100644 --- a/classes/services/PKPSubmissionService.inc.php +++ b/classes/services/PKPSubmissionService.inc.php @@ -793,11 +793,12 @@ public function delete($submission) { * @return boolean */ public function canEditPublication($submission, $userId) { + $contextId = Application::get()->getRequest()->getContext()->getId(); $stageAssignmentDao = DAORegistry::getDAO('StageAssignmentDAO'); /* @var $stageAssignmentDao StageAssignmentDAO */ $stageAssignments = $stageAssignmentDao->getBySubmissionAndUserIdAndStageId($submission->getId(), $userId, null)->toArray(); $userIsAuthor = !empty($stageAssignmentDao->getBySubmissionAndRoleId($submission->getId(), ROLE_ID_AUTHOR, null, $userId)->toArray()); - // If the submission is declined and the current user is an author of the submission - if ($submission->getStatus() == STATUS_DECLINED && $userIsAuthor) { + // If the user is an author of a declined submission and user can't edit anyway ie. is not manager + if ($submission->getStatus() == STATUS_DECLINED && $userIsAuthor && !$this->_canUserAccessUnassignedSubmissions($contextId, $userId)) { return false; } // Check for permission from stage assignments @@ -807,8 +808,7 @@ public function canEditPublication($submission, $userId) { } } // If user has no stage assigments, check if user can edit anyway ie. is manager - $context = Application::get()->getRequest()->getContext(); - if (count($stageAssignments) == 0 && $this->_canUserAccessUnassignedSubmissions($context->getId(), $userId)) { + if (count($stageAssignments) == 0 && $this->_canUserAccessUnassignedSubmissions($contextId, $userId)) { return true; } // Else deny access From 2819c8ed645cbfcd4e54df42e987fd059bc5fb55 Mon Sep 17 00:00:00 2001 From: yves Date: Wed, 18 Dec 2024 17:57:48 -0400 Subject: [PATCH 5/5] pkp/pkp-lib#10486 The restriction is only applied if the author has the exclusive role of author/reader Signed-off-by: yves --- classes/services/PKPSubmissionService.inc.php | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/classes/services/PKPSubmissionService.inc.php b/classes/services/PKPSubmissionService.inc.php index acdf791d651..9f93c0a1931 100644 --- a/classes/services/PKPSubmissionService.inc.php +++ b/classes/services/PKPSubmissionService.inc.php @@ -797,8 +797,15 @@ public function canEditPublication($submission, $userId) { $stageAssignmentDao = DAORegistry::getDAO('StageAssignmentDAO'); /* @var $stageAssignmentDao StageAssignmentDAO */ $stageAssignments = $stageAssignmentDao->getBySubmissionAndUserIdAndStageId($submission->getId(), $userId, null)->toArray(); $userIsAuthor = !empty($stageAssignmentDao->getBySubmissionAndRoleId($submission->getId(), ROLE_ID_AUTHOR, null, $userId)->toArray()); - // If the user is an author of a declined submission and user can't edit anyway ie. is not manager - if ($submission->getStatus() == STATUS_DECLINED && $userIsAuthor && !$this->_canUserAccessUnassignedSubmissions($contextId, $userId)) { + // If the submission is rejected and the user's only role is an author + if ($submission->getStatus() == STATUS_DECLINED && $userIsAuthor) { + $roleDao = DAORegistry::getDAO('RoleDAO'); /* @var $roleDao RoleDAO */ + $roles = $roleDao->getByUserId($userId, $contextId); + foreach ($roles as $role) { + if ($role->getRoleId() != ROLE_ID_AUTHOR && $role->getRoleId() != ROLE_ID_READER) { + return true; + } + } return false; } // Check for permission from stage assignments