Error reporting as non-superuser

[adrianbj]

Short description of the issue

I am actually not sure if this is a bug or intentional, but when debug mode is on and a non-superuser clicks on the "new" page list action and there is no allowed page to be created, they get the full stack trace:

ProcessWire: ProcessPageAdd: No templates allowed for adding new pages here.
DEBUG MODE BACKTRACE ($config->debug == true):

/wire/core/Wire.php(380): ProcessWire\ProcessPageAdd->___buildForm()

etc
etc

Expected behavior

I guess I expected the debug mode messages are only visible to superusers. Maybe I have come across this before, but I still think it's unfriendly.

I know debug mode should be off for live sites, but sometimes with dev sites which are password protected it's nice to leave it on. I don't think non-superusers should ever see the full error. Or are you of the opinion that perhaps sometimes we need to debug something while logged in as a non-superuser and that's the reason for this?

Reality is that debug mode doesn't have much relevance with Tracy installed, but I would still that to confirm that this behavior is intended.

Thanks!

Actual behavior

Full stack trace is shown to non-superusers.

[adrianbj]

PS - I'd actually like to see "new" removed from the list of actions when there is no allowed templates for child pages.

[netcarver]

@adrianbj Hi Adrian, I've been trying to reproduce this on a 3.0.84 local install but am having no joy. My home page template is setup to allow children and I've not defined any allowed templates. I have $config->debug = true; in my config file. Clicking on the "New" option in home-page's options in the page tree doesn't seem to generate any stack trace when logged in as a non-superuser. I'm running with the Reno theme.

Perhaps I'm missing something.

[adrianbj]

Thanks @netcarver for looking into this. Here's a screencast showing the issue and the settings for the home template and the basic-page template. You'll notice that when I start I am logged in as abc123 which is a user with page edit permission. Then I switch to my ajones user to show the various template settings.

[teppokoivula]

Or are you of the opinion that perhaps sometimes we need to debug something while logged in as a non-superuser and that's the reason for this?

+1 for this point of view. In my opinion debug mode should only ever be used for debugging, so seeing more verbose errors (including stack traces) is expected, and it should definitely apply to all users :)

[adrianbj]

@teppokoivula - you make a good argument :)

Perhaps my biggest issue then in this particular situation is that the New page list action button shouldn't show up in the first place.

[Toutouwai]

Perhaps my biggest issue then in this particular situation is that the New page list action button shouldn't show up in the first place.

@adrianbj, what is the restriction that should be preventing the user from seeing the "New" button for the Home page? You show the settings for the basic-page template in the screencast but I've never seen the "New" button appear wrongly for a user that does not have the add permission for a template.

Could it be that there is another template that the user does have add permission for that is allowed to exist under home, but the "Can this template be used for new pages" setting is "No" or "One (no more allowed)"?

Because if that is the case in your example there does seem to be an oversight in PagePermissions::addableTemplate that can cause the error if no new pages are allowed to be created for a template. If you add the following after line 848 does it fix the issue?

if(!$template) continue; // childTemplates may contain IDs for templates since deleted
if($template->noParents == 1) continue;
if($template->noParents == -1 && $template->getNumPages() != 0) continue;

Edit: added check to make sure template exists

[adrianbj]

Nice one @Toutouwai - that solves one of the scenarios if I am not the superuser, but if I am superuser, then this https://github.com/processwire/processwire/blob/839d9325cccb44e8bd4931047c8173a7990356e4/wire/modules/PagePermissions.module#L789-L791 prevents your new rules from ever coming into play. Maybe this is intentional, but I am not sure why the superuser should see the "new" button if they get an error when trying to click it.

The other situation where the "new" button shows but shouldn't (and results in an error) is related to this issue: #802

If you define a single childtemplate as valid for children of the template and then delete that template, you get an error because the parent template's childTemplates setting still includes this invalid template. This comes from this line: https://github.com/processwire/processwire/blob/839d9325cccb44e8bd4931047c8173a7990356e4/wire/modules/PagePermissions.module#L810

But I guess since the childTemplates array won't get updated when a template is deleted this is maybe expected behavior?

[Toutouwai]

Maybe this is intentional, but I am not sure why the superuser should see the "new" button if they get an error when trying to click it.

I agree it would be better for superusers not to see the New button in those circumstances. Seems like the simplest thing would be to use the modified addableTemplate() for superusers and non-superusers alike.

Here are some changes that seem to resolve the remaining issues you mentioned (not many lines changed but including the entirety of both methods here for clarity)...

/**
 * Can the current user add child pages to this page?
 *
 * Optionally specify the page to be added as the first argument for additional access checking.
 * i.e. if($page->addable($somePage))
 * 
 * @param HookEvent $event
 *
 */
public function addable($event) {

    /** @var Page $page */
    $page = $event->object; 
    $user = $this->wire('user'); 
    $addable = false; 
    $addPage = null;
    $_ADDABLE = false; // if we really mean it (as in, do not perform secondary checks)

    if($page->template->noChildren) {
        $addable = false; 

    } else if(in_array($page->id, $this->wire('config')->usersPageIDs) && $user->hasPermission('user-admin')) {
        // users with user-admin access adding a page to users: add access is assumed
        // rather than us having a separate 'users' template where access is defined
        $addable = true; 
        $_ADDABLE = true;

    } else if($user->hasPermission('page-add', $page)) {
        // user has page-add permission, now we need to check that they have access
        // on the templates in this context
        $addable = $this->addableTemplate($page, $user);
    }

    if($user->isSuperuser()) $_ADDABLE = true;

    // check if a $page is provided as the first argument for additional access checking
    if($addable) {
        $addPage = $event->arguments(0);
        if(!$addPage || !$addPage instanceof Page || !$addPage->id) $addPage = null;
        if($addPage && $addPage->template && $page->template) {
            if(count($page->template->childTemplates) && !in_array($addPage->template->id, $page->template->childTemplates)) {
                $addable = false;
            }
        }
    }

    // check additional permissions if in multi-language environment
    if($addable && !$_ADDABLE && $addPage && $this->wire('languages')) {
        if(!$this->hasPageEditLangDefault($user, $addPage) || !$this->hasPageEditLangNone($user, $addPage)) {
            // if user can't edit default language, or can't edit non-multi-language fields, then deny add access
            $addable = false;
        }
    }

    $event->return = $addable;
}

/**
 * Checks that a parent is addable within the context of it's template (i.e. has page-create for the template)
 *
 * When this function is called, it has already been determined that the user has page-add permission.
 * So this is just narrowing down to make sure they have access on a template. 
 * 
 * @param Page $page
 * @param User $user
 * @return bool
 *
 */
protected function addableTemplate(Page $page, User $user) {

    $has = false; 

    if(count($page->template->childTemplates)) {

        // page's template defines specific templates for children
        // see if the user has access to one of them 

        foreach($page->template->childTemplates as $id) {
            $template = $this->wire('templates')->get($id);
            if(!$template) continue; // childTemplates may contain IDs for templates since deleted
            if($template->noParents == 1) continue;
            if($template->noParents == -1 && $template->getNumPages() != 0) continue;
            if(!$template->useRoles) $template = $page->getAccessTemplate('edit');
            if($template && $user->hasPermission('page-create', $template)) $has = true;           
            if($has) break;
        }
        
    } else if(in_array($page->id, $this->wire('config')->usersPageIDs) && $user->hasPermission('user-admin')) {
        // user-admin permission implies create access to the 'user' template
        $has = true; 

    } else {
        // page's template does not specify templates for children
        // so check to see if they have edit access to ANY template that can be used

        foreach($this->wire('templates') as $template) { 
            // if($template->noParents) continue; 
            if($template->parentTemplates && !in_array($page->template->id, $template->parentTemplates)) continue; 
            // if($template->flags & Template::flagSystem) continue;
            //$has = $user->hasPermission('page-edit', $template); 
            $has = $user->hasPermission('page-create', $template); 
            if($has) break;
        }
    }

    return $has; 
}

There are some redundant parts of addableTemplate() when it comes to superusers. Perhaps to make it a little more efficient there could be some $user->isSuperuser() check added so only the $page->template->childTemplates part runs for them.

[netcarver]

@ryancramerdesign Bumping this for your consideration.

[ryancramerdesign]

@netcarver This behavior is not unexpected as the PageList gives more weight to speed than to accuracy in situations like this. Particularly when it comes to superuser, we try and skip over as much overhead as possible. In this case just to determine things like if the user might be able to add a page. But when it comes to actually adding a page, it doesn't need to be fast since it's only working with one page, so it can be slower and more verbose and determine for sure whether or not a user is allowed to perform the action, before actually doing it.