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.

Sign up for GitHub

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

pkp/pkp-lib#10486 adds validation to prevent an author of a rejected submission from editing metadata in the submission. #10693

Open
wants to merge 9 commits into
base: stable-3_3_0
Choose a base branch
from
Open

pkp/pkp-lib#10486 adds validation to prevent an author of a rejected submission from editing metadata in the submission. #10693

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7e5959d
pkp/pkp-lib#10486 adds validation to prevent an author of a rejected …
YvesLepidus Dec 11, 2024
5df4624
pkp/pkp-lib#10486 renames the variable $isAuthorOfTheSubmission to $u…
YvesLepidus Dec 12, 2024
4a08436
pkp/pkp-lib#10486 changes parameter of `canEditPublication` method to…
YvesLepidus Dec 12, 2024
e49e800
pkp/pkp-lib#10486 adds a check if the user is not an manager, applyin…
YvesLepidus Dec 13, 2024
2819c8e
pkp/pkp-lib#10486 The restriction is only applied if the author has t…
YvesLepidus Dec 18, 2024
1c03478
pkp/pkp-lib#10486 changes the retrieval of the Context object to not …
YvesLepidus Jan 27, 2025
bc5e628
pkp/pkp-lib#10486 removes the true return when the author has a diffe…
YvesLepidus Jan 27, 2025
2757b51
pkp/pkp-lib#10486 adds code that optimizes the retrieval of the Conte…
YvesLepidus Jan 28, 2025
c7402c4
pkp/pkp-lib#10486 fix context retrieval using the conditional that va…
YvesLepidus Feb 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/v1/submissions/PKPSubmissionHandler.inc.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
}

Expand Down
2 changes: 1 addition & 1 deletion classes/security/authorization/internal/PublicationCanBeEditedPolicy.inc.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
22 changes: 17 additions & 5 deletions classes/services/PKPSubmissionService.inc.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -788,22 +788,34 @@ 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) {
$contextId = Application::get()->getRequest()->getContext()->getId();
asmecher marked this conversation as resolved.
Show resolved Hide resolved
$stageAssignmentDao = DAORegistry::getDAO('StageAssignmentDAO'); /* @var $stageAssignmentDao StageAssignmentDAO */
$stageAssignments = $stageAssignmentDao->getBySubmissionAndUserIdAndStageId($submissionId, $userId, null)->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 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;
asmecher marked this conversation as resolved.
Show resolved Hide resolved
}
}
return false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a more complex policy will be required. If someone is an editor, for example, they probably should have permission to edit metadata on declined submissions they also authored. This restriction should probably apply to submissions only when the author is the only stage assignment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a more complex policy will be required. If someone is an editor, for example, they probably should have permission to edit metadata on declined submissions they also authored.

I added a check if the user doesn't have permissions to edit metadata according to the role.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This restriction should probably apply to submissions only when the author is the only stage assignment.

Sorry, I didn't understand exactly what you meant. Is that a necessary validation in this conditional, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't understand exactly what you meant. Is that a necessary validation in this conditional, too?

I discussed it internally with others and understood the point. In the current PR code, there are still issues, such as an author having the role of Moderator in the OPS but not being able to edit the declined submission. I will make the necessary adjustments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've adjusted the restriction to apply only in cases where the author has no role other than author/reader.

}
// Check for permission from stage assignments
foreach ($stageAssignments as $stageAssignment) {
if ($stageAssignment->getCanChangeMetadata()) {
return true;
}
}
// 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
Expand Down
2 changes: 1 addition & 1 deletion controllers/grid/users/author/AuthorGridHandler.inc.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
2 changes: 1 addition & 1 deletion pages/authorDashboard/PKPAuthorDashboardHandler.inc.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
2 changes: 1 addition & 1 deletion pages/workflow/PKPWorkflowHandler.inc.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down