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

Changes from 2 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
6 changes: 6 additions & 0 deletions classes/services/PKPSubmissionService.inc.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@YvesLepidus YvesLepidus Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

$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;
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()) {
Expand Down