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

YvesLepidus · 2024-12-12T16:05:11Z

…on from editing metadata in the submission. Signed-off-by: yves <[email protected]>

asmecher

Thanks, @YvesLepidus! I made a few suggestions for change.

asmecher · 2024-12-12T17:15:30Z

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);


Every case where canEditPublication is called, the $submission object is already available -- so I'd suggest changing calling code to just pass the submission object instead of $submission->getId(). That would save the additional (expensive) fetch of the submission object from the DB.

Hi, @asmecher ! 😃
I've adjusted the canEditPublication method and updated the calls in lib-pkp. I've also done the PR to include the method call update in OMP and OPS.

@YvesLepidus I think you're missing a PR for OJS too, no?

I couldn't find a call to the canEditPublication method in the OJS 3.3.0 source code. 🤔
I only found it in the main branch, using Repository.

asmecher · 2024-12-12T17:19:23Z

classes/services/PKPSubmissionService.inc.php

+                $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;


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.

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.

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?

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.

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

… a Submission object instead of a submission identifier and updates calls to this method. Signed-off-by: yves <[email protected]>

…tadata editing restriction only to these cases. Signed-off-by: yves <[email protected]>

…sive role of author/reader Signed-off-by: yves <[email protected]>

asmecher

Thanks, @YvesLepidus, just another couple of comments!

When you've gone through a set of review comments, don't forget to tag me on the issue to let me know; otherwise I won't know when you've finished adjusting things and have it ready for a second look.

classes/services/PKPSubmissionService.inc.php

…est as a reference.

…e from author/reader.

asmecher · 2025-01-28T01:00:02Z

classes/services/PKPSubmissionService.inc.php

@@ -176,10 +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();


@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.

@asmecher I reversed it in this commit.

asmecher

@YvesLepidus, just one more individual comment, thanks! Once that's addressed, could you forward-port this to stable-3_4_0 and main?

…ifier (it was previously excluded).

YvesLepidus · 2025-01-28T20:39:27Z

@YvesLepidus, just one more individual comment, thanks! Once that's addressed, could you forward-port this to stable-3_4_0 and main?

Ok @asmecher, I'll adapt it for these branchs.

asmecher · 2025-01-31T15:37:43Z

classes/services/PKPSubmissionService.inc.php

@@ -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'));


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.

asmecher · 2025-01-31T15:38:08Z

classes/services/PKPSubmissionService.inc.php

@@ -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'));


Same as here

asmecher · 2025-01-31T15:38:26Z

classes/services/PKPSubmissionService.inc.php

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


This change should be removed.

YvesLepidus added 2 commits December 11, 2024 18:21

pkp#10486 adds validation to prevent an author of a rejected submissi…

7e5959d

…on from editing metadata in the submission. Signed-off-by: yves <[email protected]>

pkp#10486 renames the variable $isAuthorOfTheSubmission to $userIsAuthor

5df4624

Signed-off-by: yves <[email protected]>

YvesLepidus mentioned this pull request Dec 12, 2024

Prevent authors from editing submission metadata and galleys after a submisson is rejected #10486

Open

asmecher requested changes Dec 12, 2024

View reviewed changes

pkp#10486 changes parameter of canEditPublication method to receive…

4a08436

… a Submission object instead of a submission identifier and updates calls to this method. Signed-off-by: yves <[email protected]>

YvesLepidus added 2 commits December 13, 2024 17:05

pkp#10486 adds a check if the user is not an manager, applying the me…

e49e800

…tadata editing restriction only to these cases. Signed-off-by: yves <[email protected]>

pkp#10486 The restriction is only applied if the author has the exclu…

2819c8e

…sive role of author/reader Signed-off-by: yves <[email protected]>

asmecher requested changes Jan 23, 2025

View reviewed changes

classes/services/PKPSubmissionService.inc.php Outdated Show resolved Hide resolved

classes/services/PKPSubmissionService.inc.php Outdated Show resolved Hide resolved

YvesLepidus force-pushed the i10486 branch from 8c9910d to 2819c8e Compare January 27, 2025 14:59

YvesLepidus added 2 commits January 27, 2025 11:07

pkp#10486 changes the retrieval of the Context object to not use Requ…

1c03478

…est as a reference.

pkp#10486 removes the true return when the author has a different rol…

bc5e628

…e from author/reader.

asmecher reviewed Jan 28, 2025

View reviewed changes

asmecher requested changes Jan 28, 2025

View reviewed changes

pkp#10486 adds code that optimizes the retrieval of the Context ident…

2757b51

…ifier (it was previously excluded).

asmecher reviewed Jan 31, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

YvesLepidus commented Dec 12, 2024

asmecher left a comment

asmecher Dec 12, 2024

YvesLepidus Dec 12, 2024

asmecher Dec 13, 2024

YvesLepidus Dec 13, 2024 •

edited

Loading

asmecher Dec 12, 2024

YvesLepidus Dec 13, 2024

YvesLepidus Dec 13, 2024

YvesLepidus Dec 18, 2024

YvesLepidus Dec 18, 2024

asmecher left a comment

asmecher Jan 28, 2025

YvesLepidus Jan 28, 2025

asmecher left a comment

YvesLepidus commented Jan 28, 2025

asmecher Jan 31, 2025

asmecher Jan 31, 2025

asmecher Jan 31, 2025

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

Are you sure you want to change the base?

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

Conversation

YvesLepidus commented Dec 12, 2024

asmecher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YvesLepidus Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asmecher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asmecher left a comment

Choose a reason for hiding this comment

YvesLepidus commented Jan 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YvesLepidus Dec 13, 2024 •

edited

Loading