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 8 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 all commits
Commits
Show all changes
8 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
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
40 changes: 26 additions & 14 deletions classes/services/PKPSubmissionService.inc.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ public function getProperties($submission, $props, $args = null) {

// Retrieve the submission's context for properties that require it
if (array_intersect(['_href', 'urlAuthorWorkflow', 'urlEditorialWorkflow'], $props)) {
$submissionContext = $request->getContext();
Copy link
Member

Choose a reason for hiding this comment

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

@YvesLepidus, these lines (and further down) were an optimization: if the request's context specifies the right ID, then we use it; otherwise fetch the context from the database. I think those were safe and reliable and shouldn't be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@asmecher I reversed it in this commit.

$submissionContext = $request->getContext();
if (!$submissionContext || $submissionContext->getId() != $submission->getData('contextId')) {
$submissionContext = Services::get('context')->get($submission->getData('contextId'));
}
Expand Down Expand Up @@ -251,7 +251,7 @@ function($publication) use ($args, $submission, $submissionContext) {
}
}

$values = Services::get('schema')->addMissingMultilingualValues(SCHEMA_SUBMISSION, $values, $request->getContext()->getSupportedSubmissionLocales());
$values = Services::get('schema')->addMissingMultilingualValues(SCHEMA_SUBMISSION, $values, $submissionContext);

\HookRegistry::call('Submission::getProperties::values', array(&$values, $submission, $props, $args));

Expand Down Expand Up @@ -317,7 +317,7 @@ public function getPropertyReviewAssignments($submission) {

$request = \Application::get()->getRequest();
$currentUser = $request->getUser();
$context = $request->getContext();
$context = Services::get('context')->get($submission->getData('contextId'));
Copy link
Member

Choose a reason for hiding this comment

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

Sorry to keep sending you back on this issue -- but this will potentially add a lot of overhead, as it'll mean fetching the object repeatedly if called on a list of submissions. I would suggest using the same optimization you see elsewhere:

if (!$context || $context->getId() != $submission->getData('contextId')) {
    $context = Services::get('context')->get($submission->getData('contextId'));
}

This will probably be faster, but still safe for batch jobs.

$dateFormatShort = $context->getLocalizedDateFormatShort();
$due = is_null($reviewAssignment->getDateDue()) ? null : strftime($dateFormatShort, strtotime($reviewAssignment->getDateDue()));
$responseDue = is_null($reviewAssignment->getDateResponseDue()) ? null : strftime($dateFormatShort, strtotime($reviewAssignment->getDateResponseDue()));
Expand Down Expand Up @@ -396,7 +396,7 @@ public function getPropertyStages($submission, $stageIds = null) {
}

$currentUser = \Application::get()->getRequest()->getUser();
$context = \Application::get()->getRequest()->getContext();
$context = Services::get('context')->get($submission->getData('contextId'));
Copy link
Member

Choose a reason for hiding this comment

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

Same as here

$contextId = $context ? $context->getId() : CONTEXT_ID_NONE;

$stages = array();
Expand Down Expand Up @@ -550,11 +550,7 @@ public function getWorkflowUrlByUserRoles($submission, $userId = null) {
return false;
}

$submissionContext = $request->getContext();

if (!$submissionContext || $submissionContext->getId() != $submission->getData('contextId')) {
$submissionContext = Services::get('context')->get($submission->getData('contextId'));
}
$submissionContext = Services::get('context')->get($submission->getData('contextId'));
Copy link
Member

Choose a reason for hiding this comment

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

This change should be removed.


$dispatcher = $request->getDispatcher();

Expand Down Expand Up @@ -788,22 +784,38 @@ 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 = $submission->getData('contextId');
$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) {
$userIsOnlyAuthorOrReader = true;
$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) {
$userIsOnlyAuthorOrReader = false;
break;
}
}
if ($userIsOnlyAuthorOrReader) {
return false;
}
}
// 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