'ProcessPageLister' can't show editable, unpublished pages to users who have `page-edit` but not `page-publish` permission

[michaellenaghan]

Short description of the issue

I'm working on a site that has three roles:

• "author"s can create stories, and only edit their own stories. They can't publish stories.
• "editor"s can edit any stories. They can't publish stories.
• "publisher"s can edit any stories. They can also publish stories.

I tried to create a public page lister bookmark that would list unpublished stories.

The bookmark works for publishers, but not for editors.

Expected behavior

Roles that can edit but not publish pages should be able to list unpublished pages in the lister.

Actual behavior

Roles that can edit but not publish pages are not able to list unpublished pages in the lister.

Optional: Screenshots/Links that demonstrate the issue

Here's an unpublished story:

Both editors and publishers are able to edit the story:

Here's a public Unpublished Stories page lister bookmark:

Publishers can see the unpublished story:

Editors cannot:

Note the message that says "Not all specified templates are editable."

In fact, editors do have page-edit permission:

The problem is that a) the page-publish permission is present in the database; and b) editors don't have page-publish; and c) a temporarily created test page doesn't have Unpublished status.

Look here:

                if(count($parents)) {
                    foreach($templates as $template) {
                        $test = $pages->newPage($template);
                        $test->id = 999; // required (any ID number works)
                        foreach($parents as $parent) {
                            $test->parent = $parent;
                            if($test->editable()) {
                                $numEditable++;
                                break;
                            }
                        }
                    }
                } else {
                    foreach($templates as $template) {
                        $test = $pages->newPage($template);
                        $test->id = 999; // required (any ID number works)
                        if($test->editable()) $numEditable++;
                    }
                }

A test page is created and tested for editability. The editability test ends up here:

        if($this->hasPagePublish) {

            // if user has the page-publish permission here, then we're good
            if($user->hasPermission('page-publish', $page)) return true;

            // if the page is unpublished then we're fine too
            if($page->hasStatus(Page::statusUnpublished)) return true;

            // otherwise user cannot edit this page
            return false;
        }

The check fails because $page->hasStatus(Page::statusUnpublished) returns false.

It returns false because Page::statusUnpublished wasn't added to the temporarily created test page.

Optional: Suggestion for a possible fix

Changing the two instances of this:

    $test = $pages->newPage($template);
    $test->id = 999; // required (any ID number works)

to this:

    $test = $pages->newPage($template);
    $test->id = 999; // required (any ID number works)
    $test->addStatus(Page::statusUnpublished);

fixes the problem.

I don't know enough about ProcessWire to determine whether or not that's the right fix.

It does occur to me, though, that a) that fix would only fix the page lister; and b) the same problem might be hiding somewhere else in the code, because new pages are not automatically unpublished pages.

Steps to reproduce the issue

1. Add the page-publish permission
2. Add one role, "publisher", that can edit and publish a given template
3. Add another role, "editor", that can edit but not publish a given template
4. Create a public page lister bookmark that shows unpublished instances of that template
5. Create an unpublished instance of that template
6. Log in as the publisher, and go to the page lister bookmark
7. You can see the unpublished instance
8. Log in as the editor, and go to the page lister bookmark
9. You can't see the unpublished instance
10. There's a message saying that there are "no editable templates", when in fact the editor can edit instances of that template

Setup/Environment

• ProcessWire 3.0.229
• Lister v0.2.6

[ryancramerdesign]

@michaellenaghan Thanks, I can duplicate that one with the page-publish permission installed. I think this in Lister may be a unique case so adding unpublished status to the test page when page-publish permission is installed seems like a good route. I have pushed an update that does that. Can you confirm it also fixes it there?

[matjazpotocnik]

@michaellenaghan, can you verify the fix and close this issue?

[michaellenaghan]

Sorry; I'll verify the fix on Mon.

[michaellenaghan]

It looks like this is not fixed.

Here's a screenshot from a superuser:

Here's a screenshot from an editor:

I'll see if I can spot the problem in the code.

[michaellenaghan]

OK, I have a partial answer: my current bookmarks are different from my original bookmarks.

My original bookmarks specified a template.

My current bookmarks don't; they're supposed to find all unpublished pages.

This does fix the problem with the original bookmarks.

I'll see if I can figure out where the problem with my current bookmarks is coming from.

[michaellenaghan]

OK, I think I have a partial answer to this too, but I have to think about it a bit more.

            if(count($templates)) {
                    ...
            } else {
                // with no template specified
                // only allow a max include mode of hidden
                // regardless of edit access
                if($includeMode != 'hidden') {
                    if($showIncludeWarnings) {
                        $this->resultNotes[] = $this->_("No templates specified so 'include=hidden' is max allowed include mode");
                    }
                    $includeSelector->value = 'hidden';
                    $changed = true; 
                }
            }

The old problem is in the ... bit; the new problem is in the else bit. I think the comment (with no template specified...) is indicating, indirectly, something like: you can't test permissions unless you know which template(s) you're testing against. (And you can't test for unpublished unless you can test permissions.)

If that's correct, the "why" is not immediately obvious to me. My too-simple mental model is that a) I want to search for unpublished pages; and b) if the publish permission is installed then any page could be unpublished. I haven't worked out why checking for an unpublished status depends on checking for editability.

I'm missing something. As I said, I'll think about it a bit more.

[michaellenaghan]

OK, I'll try to think out loud a bit. Let me know if my thinking is off.

Let's say that permissions are set up so that some users can only edit the pages they themselves created. If there are two such users, the page list will hide their pages from each other until the pages are published. When they are published, each user will only be able to get to the other user's publicly viewable page. In other words: neither user will be able to see the other's in the admin.

I think that the page finder is trying to replicate that behaviour.

There's more, maybe. For example: rather than trying to evaluate criteria for each page, this bit of code is trying to build a single selector that can be applied, in bulk so to speak, against all pages. That's why there's this kind of requirement:

    // if all specified templates are editable, include=unpublished is allowed

I'm not sure that's right!

What I can say, for what it's worth, is that the finder allows you to create reasonable criteria ("show me all unpublished pages") that can't be resolved. The error you get in that case (No templates specified so 'include=hidden' is max allowed include mode) assumes the user knows that there's a connection between "Status" and "include".

Well, not just that.

Here's a suggestion for a possible fix: if $includeSelector has a value, and if $templates is empty, automatically select all editable templates rather than limiting the include criteria. I think that better matches user expectations.

My understanding of ProcessWire is still on the shallow side, so that might be a bad suggestion. :-)

===

In any event, having looked at the code, I can see that I can add a list of templates to the filter criteria to get the behaviour I was looking for. In that sense: yes, this is fixed.

I'll give you a chance to chime in before I close this.