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.

[Toutouwai]

The negatives of this issue in my view are:

• The text "The process returned no content" shown on the Add New screen is meaningless to a non-superuser and likely a source of confusion.
• The Page List is a less useful source of information than it could be for seeing which pages will allow new children to be added.
• Both superusers and non-superusers have some time wasted when they try and fail to add a new page.

I think these are not insignificant negatives, so if the reason for potentially not fixing them is based on performance it would be good to test to what the actual performance impact is in a typical or even worst case scenario.

For the first fix I suggested above (which fixes this issue and #802 but only for non-superusers), I used Debug::timer() around ProcessPageList::render() when 101 pages are listed (so a reasonably large number of pages). The results in PW 3.0.127, averaged over five renders...

Non-superuser without the fix: 0.1842
Non-superuser with the fix: 0.1840

So fractionally faster with the fix for some reason, but the difference is so small we can say essentially no change.

For the second fix (which solves both issues for superusers and non-superusers) the results were...

Superuser without the fix: 0.0853
Superuser with the fix: 0.0851

Again fractionally faster with the fix but essentially no change.

So there doesn't seem to be any significant performance impact to applying a fix for these issues.

[adrianbj]

This behavior is not unexpected as the PageList gives more weight to speed than to accuracy in situations like this.

Maybe this is an ok compromise for superusers because we can accept idiosyncrasies like this, but clients see these sorts of things and get a bad impression. Same goes for error messages seen by non-superusers - this is an area that is really important - clients need non-techie friendly messages that mean something and don't look like something is broken.

[ryancramerdesign]

@Toutouwai That doesn't make sense that calling a potentially heavy method call would be faster than skipping it, so I think the measurement is either incorrect (like due to involvement of cache), or underneath the measurable threshold. But your results seem to indicate it's worth investigating further, so I'm curious what's actually different in the big block of code you posted? I'm not clear on that. Is there some way to better highlight what changes you are suggesting? Thanks.

[adrianbj]

I'm curious what's actually different in the big block of code you posted?
http://www.mergely.com/IFFPT6ia/?ws=1

EDIT: old link doesn't work anymore, the new one is https://editor.mergely.com/nR8G0I7d?ws=1&wl=1 (matjazpotocnik)

[Toutouwai]

I'm curious what's actually different in the big block of code you posted?

Other ways besides what @adrianbj suggested:

• The compare features built into PhpStorm: https://www.jetbrains.com/help/phpstorm/comparing-files-and-folders.html
• If you're using Github Desktop then a diff is automatically shown for any uncommitted changes you make to the local copy of the repo.

[matjazpotocnik]

@adrianbj @Toutouwai is this still an issue? I didn't test myself, but I assume it's not fixed. There are some changes in the addable() method in the latest dev version of PW 3.0.220 (check for superuser), but no change in addableTemplate() method... https://editor.mergely.com/nR8G0I7d?ws=1&wl=1