Admin can not save an applied work definition to a page (bad request)

[robbieaverill]

Admin can not save an applied work definition to a page (bad request) (saved but error message)

[robbieaverill]

Cannot reproduce. @sachajudd we might need you to provide a bit more info on how to reproduce this - seems fine as far as I can tell =)

[robbieaverill]

Reproduced:

• Log in as an admin user
• Ensure there's a workflow created
• Choose a page
• Goto Settings > Workflows and select the workflow from the dropdown
• Click Save & Publish on the page

Error: Action "publish" not allowed on controller (Class: SilverStripe\CMS\Controllers\CMSPageSettingsController)

[sachajudd]

@robbieaverill yeah only happens with the Save & Publish button when a Workflow Definition has never been added to a page before. Although Save draft seems to work.

[tractorcow]

My experience:

• Log in as an admin user
• Ensure there's a workflow created
• Choose a page
• Goto Settings > Workflows and select the workflow from the dropdown
• Click Save & Publish on the page <--- Can't do this, only see "apply for approval" button

[tractorcow]

Oh right, if the page doesn't have the workflow, at the start it doesn't have the workflow, which itself hides the publish button. I think I can see a fix. ;)

[tractorcow]

What if:

• only people with XXX Permission can assign a workflow to a page
• people with XXX permission can publish a page even if it has a workflow

[tractorcow]

This hack will fix the initial save.

https://github.com/creative-commoners/silverstripe-advancedworkflow/commit/7a0c3c11f96d364f6053df00b5bf330b65d30f26

A second save however will break again.

[NightJar]

Should it be that an Admin can always publish a page? Although there is something to be said for making an admin approve their own change, at least that makes it explicit that the publish is both intentional and that there is a workflow applied... What are your thoughts @nyeholt ?

[tractorcow]

If an admin can't publish their page, they could manually remove the workflow and then publish, so it's not really secure anyway. If you can do that you may as well allow publish anyway.

[NightJar]

I'd like to make this issue clear, both for my own understanding and for anyone else who reads this thread :) Can you please confirm my thinking is correct @tractorcow ?

The issue stems from the fact that a Save & Publish is executed as two actions in series, first a save, then a publish. The issue manifests because a save applies the workflow, and the subsequent publish fails because that workflow has not been approved?

If my understanding is correct, then simply allowing Admins to be able to publish a page probably isn't an ideal solution either, as it would not apply to any other user than has permission to alter the applied workflow. I'll open a Pull Request for your work around @tractorcow - that at least gets us most of the way there.

[nyeholt]

As far as I can see, the main change between this working between SS3 and SS4 is that WorkflowDefinition now has

    public function canWorkflowPublish($member, $target)
    {
        $publish = $this->extendedCan('canWorkflowPublish', $member, $target);
        if (is_null($publish)) {
            return false;
        }
        return $publish;
    }

which has changed the ultimate result of the WorkflowApplicable::canPublish check to be opinionated (ie, returns false on a null return) about what should happen if there's no specific permission to be found. I'm kind of leaning towards letting this just being a return $publish; here, so that a null return can fall back to be handled by SiteTree::canPublish which would subsequently return true; for ADMIN users. In other words, falling back to the default SS behaviour.

If this behaviour is undesirable and a specific project has a use-case for not allowing ADMIN publishing-at-will, then the 'canWorkflowPublish` hook can be implemented by an extension to explicitly return false regardless of user type, and handle the hack-around capture of this being a Save & Publish event for something that didn't originally have workflow.

[tractorcow]

The issue manifests because a save applies the workflow, and the subsequent publish fails because that workflow has not been approved?

The subsequent publish fails because the first save removes the save button, making it an invalid action.

I'm not sure the suggested fix by @nyeholt will fix it, but @NightJar please test it and let me know if it does.

[tractorcow]

My solution is like this:

public function canWorkflowPublish($member, $target)
    {
        $publish = $this->extendedCan('canWorkflowPublish', $member, $target);
        if (isset($publish)) {
            return $publish;
        }
        return Permission::checkMember($member, 'ADMIN');
    }

[nyeholt]

@tractorcow - what I was getting at was effectively the same as what you're doing, but without the explicit ADMIN permission checking. If you do return $this->extendedCan('canWorkflowPublish', $member, $target); the end result is effectively the same as what you have there; if you return a non-boolean from this method, then SiteTree's canPublish method will skip through to the ADMIN perm check

// Check extension
        $extended = $this->extendedCan('canPublish', $member);
        if ($extended !== null) {
            return $extended;
        }

        if (Permission::checkMember($member, "ADMIN")) {
            return true;
        }

(and running this locally means an admin can save/publish on first application of workflow too) .

[raissanorth]

I feel that leaving the default cascade functionality in place is a good idea, and have gone with @nyeholt 's suggestion (thank you!) in #345

[tractorcow]

@nyeholt actually it'll let you publish by default if you have edit permissions... that could open up publishing to a much larger group which should not have publish rights (in the event a workflow is in place).

The rest of SiteTree::canPublish()

// Check extension
        $extended = $this->extendedCan('canPublish', $member);
        if ($extended !== null) {
            return $extended;
        }

        if (Permission::checkMember($member, "ADMIN")) {
            return true;
        }

        // Default to relying on edit permission
        return $this->canEdit($member);

If there's a workflow in place, all editors can publish, even if the workflow SHOULD disable it.

By a forcible check for admin in canWorkflowPublish, you limit it to admins, but only in the case a workflow exists.

[NightJar]

I'm mildly confused as this is my first foray into this module, but would the action of advancing a workflow not be covered by WorkflowTransition, and whatever permissions that transition step has implemented on it?

[nyeholt]

@tractorcow yep, didn't see how WorkflowApplicable had changed. It might be best if the return false was done at that tier though, to remain consistent with the previous model of workflow perms?

[tractorcow]

You could do the permission check (true / false) at a few levels:

• WorkflowApplicable::canPublish() (check Permission ADMIN if $canPublish is null)
• WorkflowDefinition::canWorkflowPublish() (as I pasted above).

As long as you always return a boolean for canPublish if $definition is not null.