search

pkp / pkp-lib

The library used by PKP's applications OJS, OMP and OPS, open source software for scholarly publishing.
https://pkp.sfu.ca
GNU General Public License v3.0
297 stars 443 forks source link

Publishing / posting blocked for users with both Section editor assignment and a Journal Manager role #6875

Closed ajnyga closed 1 year ago

ajnyga commented 3 years ago

A shared problem in all applications 3.3.0 but most visible in OPS. This was reported by @alexxxmendonca and their team (feel free to add any additional information that might be missing here).

In 3.3.0 a Journal editor / Server manager can be blocked from publishing a new submission if she has a stage assigment as a Section editor / Moderator. Because of the Manager role she can still see the Publish / Post button in the UI but the API will return a user.authorization.accessibleWorkflowStage error. This is because the Section Editor stage assigment does not allow the user to publish.

This has to work differently in OPS where we definitely want to allow Moderators the ability to Post new submission. That is their main functionality in the software. I think this was possible in 3.2 but has been removed somewhere along the way.

So fix number 1 would be: allow OPS moderators to post.

We also need to reconsider how this works in OJS. Many journals use the Section editor role for Journal editors as well, because of the automatic assignments functionality (see https://github.com/pkp/pkp-lib/issues/4457). This will probably lead to a lot of situations where the Publishing functionality is shown for the Journal editor but blocked because of the Section editor assignment. This will definitely cause a lot of confusion when journals start to upgrade to 3.3. in bigger numbers.

Fix number 2 would be: in OJS allow the Journal Manager role override the "can the user publish" check that is being applied here.

OR do we expect that Section Editors in OJS can also Publish? I think in 3.1. only editors could do that?

ping @NateWr @asmecher

ajnyga commented 3 years ago

(ps. in the long run it would be maybe good if the code would refer to Context Managers and Section Managers instead of journal specific lingo)

ajnyga commented 3 years ago

I think this does not work as expected: https://github.com/pkp/pkp-lib/blob/main/classes/security/authorization/StageRolePolicy.inc.php#L62

When I click Publish / Post in the UI that condition is never true, because ASSOC_TYPE_ACCESSIBLE_WORKFLOW_STAGES does not seem to be set at all. $userAccessibleStages is an empty array.

ajnyga commented 3 years ago

I can fix that by doing this:

  if (in_array($routeName, $this->requiresProductionStageAccess)) {
      import('lib.pkp.classes.security.authorization.internal.UserAccessibleWorkflowStageRequiredPolicy');
      $this->addPolicy(new UserAccessibleWorkflowStageRequiredPolicy($request));
      import('lib.pkp.classes.security.authorization.StageRolePolicy');
      $this->addPolicy(new StageRolePolicy($this->productionStageAccessRoles, WORKFLOW_STAGE_ID_PRODUCTION, false));
  }

Here: https://github.com/pkp/pkp-lib/blob/main/api/v1/submissions/PKPSubmissionHandler.inc.php#L173-L176

Basically setting the missing ASSOC_TYPE_ACCESSIBLE_WORKFLOW_STAGES constant. Probably there are other ways of solving this as well.

NateWr commented 3 years ago

Thanks @ajnyga, I've been able to reproduce the problem that you describe.

Subeditors (section editors, series editors, moderators) should be allowed to publish as long as their user group is assigned to the production stage (yes by default). It looks like the problem lies in SubmissionAccessPolicy, which is gets the accessible workflow stages for subeditors but not for managers. There seems to be a problem when there are dual roles, although looking at the code it should apply both policies.

NateWr commented 3 years ago

Just a quick note: the fix in this comment will not work because it prevents unassigned managers from publishing.

ajnyga commented 3 years ago

OJS Authorization Policies ❤️

ajnyga commented 3 years ago

Does https://github.com/pkp/pkp-lib/blob/main/classes/security/authorization/StageRolePolicy.inc.php#L82 require the empty check? But I am maybe looking at the wrong place for the solution anyway. I got a bit confused first because I had the "can only recommend" setting on for the section editor assigment I first tested this with and that hides the Publish button altogether. Makes sense that by default they can Publish, no problem there.

edit: ah ok, I see SubmissionAccessPolicy is called in the API but as you said, but it does not set the constant properly.

NateWr commented 3 years ago

Does https://github.com/pkp/pkp-lib/blob/main/classes/security/authorization/StageRolePolicy.inc.php#L82 require the empty check?

Yes. A manager may be assigned to a submission in a less-than-manager role, so we always apply authorization based on assigned role OR if no role let managers do things.

I've spent some time on this today with no luck. You're right that the problem lies in the fact that ASSOC_TYPE_ACCESSIBLE_WORKFLOW_STAGES has not been set when StageRolePolicy is called. I suspect that SubmissionAccessPolicy is exiting after the first successful policy, which is the manager's policy, and so UserAccessibleWorkflowStageRequiredPolicy (added for subeditors) is never called. But adding UserAccessibleWorkflowStageRequiredPolicy to the manager's policy didn't seem to work.

ajnyga commented 3 years ago

We have started to encounter this problem with several journals after upgrading to 3.2.1. Is there any potential solution for this?

asmecher commented 2 years ago

I think there are two problems:

  1. A code-level problem, where ASSOC_TYPE_ACCESSIBLE_WORKFLOW_STAGES is not available in the authorized context list. As you found, @ajnyga, this is happening because StageRolePolicy depends on UserAccessibleWorkflowStageRequiredPolicy but that policy has not been brought into the policy list in api/v1/submissions/PKPSubmissionHandler.inc.php. I agree with the part of your proposal that adds UserAccessibleWorkflowStageRequiredPolicy, but @NateWr mentions a follow-up problem which I think can be solved as follows:

    diff --git a/classes/security/authorization/SubmissionAccessPolicy.inc.php b/classes/security/authorization/SubmissionAccessPolicy.inc.php
index ee562a821..0302cd7f2 100644
--- a/classes/security/authorization/SubmissionAccessPolicy.inc.php
+++ b/classes/security/authorization/SubmissionAccessPolicy.inc.php
@@ -43,8 +43,13 @@ class SubmissionAccessPolicy extends ContextPolicy {
               // Managerial role
               //
               if (isset($roleAssignments[ROLE_ID_MANAGER])) {
+                       $managerSubmissionAccessPolicy = new PolicySet(COMBINING_DENY_OVERRIDES);
+                       import('lib.pkp.classes.security.authorization.internal.UserAccessibleWorkflowStageRequiredPolicy');
+                       $managerSubmissionAccessPolicy->addPolicy(new UserAccessibleWorkflowStageRequiredPolicy($request));
+
                       // Managers have access to all submissions.
-                       $submissionAccessPolicy->addPolicy(new RoleBasedHandlerOperationPolicy($request, ROLE_ID_MANAGER, $roleAssignments[ROLE_ID_MANAGER]));
+                       $managerSubmissionAccessPolicy->addPolicy(new RoleBasedHandlerOperationPolicy($request, ROLE_ID_MANAGER, $roleAssignments[ROLE_ID_MANAGER]));
+                       $submissionAccessPolicy->addPolicy($managerSubmissionAccessPolicy);
               }

               //

    (@NateWr, I think this is what you were working towards -- but I suspect the missing piece was the additional policy set with COMBINING_DENY_OVERRIDES, which forces all component policies to agree on a GRANT, rather than picking the first and bailing.)

  2. A business-level problem relating to automatic assignments: because automatic sub editor assignments are always made using the sub editor role, and that role is intentionally of limited functionality, automatic assignments can implicitly limit an editor's ability to work with a submission. I can think of several solutions:

    • Recommend against automatic section editor assignments for editor accounts (not a great option but anyway)
    • At the time of assignment, look at the user's "best" role and assign that rather than always using a sub editor (prone to lots of ambiguity!)
    • At the time of making the decision, look at the editor's "best" role (likewise)
    • Make the automatic assignments configurable so that a role can be specified.

    That last solution is obviously bigger in scope but given the amount of confusion about automatic assignments I think it makes sense to solve it all together. See also: https://github.com/pkp/pkp-lib/issues/7103, https://github.com/pkp/pkp-lib/issues/4457 -- especially Nate's comment:

    At the risk of blowing this up into something bigger than it needs to be: one of the things I'd like to see is the ability to configure automatic assignments during the editorial workflow. It would be nice if journals could configure, for example, that a Layout Editor is automatically assigned when a submission reaches the Production stage. This would help automate the hand-off, especially to managers/assistants who often need to intervene at some step of the process.

    Let's continue that discussion over there but I'm 100% in favour. Without that, automatic assignments are going to be a continued source of confusion and inflexibility.

asmecher commented 2 years ago

@ajnyga, just a reminder that this one is assigned to you; it likely won't make 3.3.0-9, FWIW.

ajnyga commented 2 years ago

@asmecher did some testing with this.

Using COMBINING_DENY_OVERRIDES like you suggested does not seem to work. I will look closer what is happening there in a couple of days...

Basically the outcome is:

ajnyga commented 2 years ago

So I could not figure out how to make that example work. However, I have an alternative solution here that seems to work with all the above scenarios. Do you see a problem with that?

https://github.com/pkp/pkp-lib/compare/main...ajnyga:f6875?expand=1

ajnyga commented 2 years ago

Basically in StageRolePolicy, if userAccessibleStages is empty AND the user is a manager, you actually already have a check there for cases where managers may have a stage assigment but no accessible stages. The code currently checks if there is a manager assignment and I just added also a check for sub_editor assignments. https://github.com/pkp/pkp-lib/blob/main/classes/security/authorization/StageRolePolicy.php#L106-L108

This seems like a safe change, because you can not even access that loop if you are not a manager or a site admin.

ajnyga commented 2 years ago

ping @asmecher since you probably know the authorization stuff the best

ajnyga commented 2 years ago

ping @asmecher one of our journals bumped again to this issue, did you check the solution linked above? If it seems safe, would like to fix it in our server.

ajnyga commented 2 years ago

Note that this probably can also happen if a journal manager is assigned to a submission using an assistant role, like layout editor. You see the same behaviour: buttons for publishing are visible, but the use of the actions is blocked by the policies.

asmecher commented 2 years ago

@ajnyga, sorry for the wait; your proposed change is low-risk as you've noted, and it looks like you've already agreed with @NateWr about the workflow implications. I'm OK to go ahead with https://github.com/pkp/pkp-lib/commit/b1d0f13be698a740e1bd44aeff742ccefe53e0ec.

ajnyga commented 2 years ago

what do you think about assistant roles? The thing is that I am not 100% what the purpose of the loop is where I am suggesting the changes to. I mean what is the original purpose of it.

asmecher commented 2 years ago

Not sure if this is answering your question -- but assistant roles are the catch-all for copyeditors, layout editors, proofreaders, etc. They are press/journal/server "insiders" (i.e. they can't self-register, are expected to be a select few, and can be considered to have a paid/professional relationship with the journal/server/press) but may also be contract roles, i.e. their work may be provided by an external business entity. [edited: yep, this was not helpful :) -AS]

ajnyga commented 1 year ago

@asmecher @NateWr

I am a bit worried with changing code that to me has an unknown purpose.

I mean these lines: https://github.com/pkp/pkp-lib/blob/main/classes/security/authorization/StageRolePolicy.php#L88-L112

What we are doing now is that since ASSOC_TYPE_ACCESSIBLE_WORKFLOW_STAGES is not set here https://github.com/pkp/pkp-lib/blob/main/classes/security/authorization/StageRolePolicy.php#L68

We are basically running $stageAssignmentDao->getBySubmissionAndUserIdAndStageId() again here https://github.com/pkp/pkp-lib/blob/main/classes/security/authorization/StageRolePolicy.php#L96

Which is just doing the same what happens when you set ASSOC_TYPE_ACCESSIBLE_WORKFLOW_STAGES https://github.com/pkp/pkp-lib/blob/d0cd5cc32f94906c5bcac0a59e7ea2b6a03889d9/classes/user/Repository.php#L199

My question is that what is the situation described here? https://github.com/pkp/pkp-lib/blob/main/classes/security/authorization/StageRolePolicy.php#L93-L94

        // Managers may have a stage assignment but no $userAccessibleStages, so they will
        // not be caught by the earlier code that checks stage assignments.

I am not sure when that happens? In this case it happens just because ASSOC_TYPE_ACCESSIBLE_WORKFLOW_STAGES is not set but this is clearly not the original purpose of that code?

Also, what does this exception mean: https://github.com/pkp/pkp-lib/blob/main/classes/security/authorization/StageRolePolicy.php#L90-L92 Why do we permit access when allowRecommendOnly is true?

ajnyga commented 1 year ago

However, this would fix the situation where a manager is assigned as a section editor or as an assistant. Meaning that for example when a manager is assigned as a layout editor, they can publish as usual. This is also how the UI works, it shows the buttons by overriding based on the manager role instead of the stage assigment role.

I also changed the commenting a bit

        // Managers may have a stage assignment but $userAccessibleStages is not set so they will
        // not be caught by the earlier code that checks stage assignments.
        if (empty($userAccessibleStages) && count(array_intersect([Role::ROLE_ID_MANAGER, Role::ROLE_ID_SITE_ADMIN], $this->getAuthorizedContextObject(ASSOC_TYPE_USER_ROLES)))) {

            if ($this->_allowRecommendOnly) {
                return AuthorizationPolicy::AUTHORIZATION_PERMIT;
            }
            // If manager has a stage assignment as an manager, section editor or assistant, allow access with manager permissions
            $stageAssignmentDao = DAORegistry::getDAO('StageAssignmentDAO'); /** @var StageAssignmentDAO $stageAssignmentDao */
            $result = $stageAssignmentDao->getBySubmissionAndUserIdAndStageId(
                $this->getAuthorizedContextObject(ASSOC_TYPE_SUBMISSION)->getId(),
                Application::get()->getRequest()->getUser()->getId(),
                $this->_stageId
            );
            $userGroupDao = DAORegistry::getDAO('UserGroupDAO'); /** @var UserGroupDAO $userGroupDao */
            $noResults = true;
            while ($stageAssignment = $result->next()) {
                $noResults = false;
                $userGroup = $userGroupDao->getById($stageAssignment->getUserGroupId());
                if (in_array($userGroup->getRoleId(), [Role::ROLE_ID_MANAGER, Role::ROLE_ID_SUB_EDITOR, Role::ROLE_ID_ASSISTANT]) && !$stageAssignment->getRecommendOnly()) {
                    return AuthorizationPolicy::AUTHORIZATION_PERMIT;
                }
            }

            // A manager is granted access when they are not assigned in any other role
            if ($noResults) {
                return AuthorizationPolicy::AUTHORIZATION_PERMIT;
            }

        }
asmecher commented 1 year ago

Hi all -- I'm deferring this to 3.3.0-13 as I think pressure to get it solved will probably lead us to breaking either OJS or OPS, which will be an unpleasant surprise for a minor release. I'm going to move ahead with the 3.3.0-12 release, and we can come back to this under a little less time pressure and figure out both an expedient solution to the original OPS problem, and a longer term solution for all apps.

ajnyga commented 1 year ago

Ok, just want to note that we encounter this in OJS as well.

NateWr commented 1 year ago

I've read through this whole issue and I'm not confident that we will find a suitable change to the authorization policies that does not have some side effect. Am I right in thinking that there are three problems?

  1. In OPS, moderators (ROLE_ID_SUBEDITOR) can not post preprints. Is this true? It doesn't seem right, since Scielo is making heavy use of moderators right now.

  2. In OJS, journal managers assigned as section editors (ROLE_ID_SUBEDITOR) without access to the production stage can not publish submissions. Can someone describe the use case here more clearly? Usually, section editors have access to the production stage so I'm curious what use case you're running into regularly @ajnyga.

  3. In OJS, journals want to configure automated editorial assignments, so people who should be journal managers are assigned as section editors, limiting their permissions. This is a solved problem in 3.4, since it will be possible to configure automated editorial assignments in any editorial role (including assistants).

alexxxmendonca commented 1 year ago

_1. In OPS, moderators (ROLE_ID_SUBEDITOR) can not post preprints. Is this true? It doesn't seem right, since Scielo is making heavy use of moderators right now._

Yes, this is true. We've been dealing with a manual "workaround" in order to post preprints: a) We have to remove ourselves from the submission as Moderator b) We have to assign ourselves as Server Administrator on the same submission

If we don't do this, we cannot post the preprint. We've been doing this for almost 2 years. It's a bit frustrating.

NateWr commented 1 year ago

I tested this in OPS (stable-3_3_0) and when logged in as a moderator I was able to post a preprint. @alexxxmendonca or @ajnyga can you provide reproduction steps for this problem which include any configuration settings that are necessary to reproduce it?

alexxxmendonca commented 1 year ago

@NateWr does your user include both moderator and server administrator roles?

The main SciELO user account we use has multiple roles: image

For reference, the roles we have are: image

It's my understanding that this problem is caused by different permissions, causing conflicts.

The reproduction steps would be:

  1. Have a user with the same roles as the image above.
  2. Have that same user as the default Moderator for all sections/series (so that new submissions would automatically be assigned to that user).
  3. Submit a preprint using a different author account.
  4. Check is the user is automatically assigned to the new submission as Moderator.
  5. Post the preprint.
NateWr commented 1 year ago

Thanks, I was able to reproduce this by assigning a moderator who also has a manager role in the context.

NateWr commented 1 year ago

I think I may have found a simple solution to this. As @ajnyga figured out, the root of the problem is that when StageRolePolicy is called, the UserAccessibleWorkflowStageRequiredPolicy has not been called and therefore $this->getAuthorizedContextObject(ASSOC_TYPE_ACCESSIBLE_WORKFLOW_STAGES) returns null. This happens because the PolicySet in SubmissionAccessPolicy stops as soon as the first policy passes, which is the managerial override.

To fix this, I moved the check for managers to the bottom of the PolicySet in SubmissionAccessPolicy. This PolicySet uses COMBINING_PERMIT_OVERRIDES, so it will stop as soon as one policy returns true. By moving it to the end, we ensure that other policies are checked first. That means UserAccessibleWorkflowStageRequiredPolicy will be called in every case, except when the user has no stage assignment.

@asmecher can you double-check my logic here? Am I right in thinking it shouldn't cause any side effects to move the manager check lower in the PolicySet? If this sounds correct to you, I'll do some more thorough testing across multiple apps and permission configurations before merging.

PR:

Tests:

asmecher commented 1 year ago

Our underlying policy implementation seems to be based on XACML, and unfortunately I don't know much about that standard.

I don't think moving the manager check lower in the PolicySet will have negative side effects, but only because they have been coded so far (I think) so that the order doesn't matter. Starting to introduce ordering adds a whole new layer to an already byzantine structure, so while it might be a good expedient fix for stable-3_3_0, I'd like to keep thinking for main.

How about separating the creation of ASSOC_TYPE_ACCESSIBLE_WORKFLOW_STAGES from the enforcement of conditions on it? We already have a bit of a hierarchy:

If UserAccessibleWorkflowStageRequiredPolicy got split into two classes, one creating ASSOC_TYPE_ACCESSIBLE_WORKFLOW_STAGES and another implementing checks on it, then the first could be added to the manager check to make the dependency explicit. (main also has a case for ROLE_ID_SITE_ADMIN that presumably would need this too.) The other couple of classes could extend the appropriate base class, I suppose.

NateWr commented 1 year ago

Did you imagine this for 3.3 and 3.4? Or a change to implement early in the 3.5 dev cycle? Personally, I don't feel comfortable making this kind of change to the policies this late in the 3.4 dev cycle. When I worked on the dual-role access permissions in #3130 I got burned by the SubmissionAccessPolicy in half a dozen unexpected ways.

asmecher commented 1 year ago

Fair enough; do you want to test a bit further with the rearrangement of policies for stable-3_3_0 and 3.4 main, and file some of this detail for work in 3.5? (Watch out for the admin condition in main -- presumably it'll require the same treatment.)

NateWr commented 1 year ago

Thanks, I've done some testing today and not found any side effects. I've prepared PRs for the minimal change and filed an issue with the more comprehensive refactor at https://github.com/pkp/pkp-lib/issues/8557.

NateWr commented 1 year ago

Merged to main and stable-3_3_0. This fix will be included in 3.3.0-14 and later. Thanks everyone for helping solve a tricky one! :tada: