Improve Permission denied handling in WordPress by re-suing the get_p…

[seamuslee001]

…ermission_denied function to output message instead of Exception when permission is denied

Overview

This seeks to improve the handling when a user gets a Permission denied error in WordPress

Before

Yellow screen / fatal error caused by Exception being thrown

After

Page content replaced with a message saying you do not have permission

ping @kcristiano @mattwire @haystack thoughts?

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[christianwach]

@seamuslee001 Looks good to me without a r-run. I wonder what happens if this method is called:

1. Multiple times.
2. After the_content filter has run.

I assume:

1. This could be protected against with a static variable in the method that returns without adding the filter if set.
2. Nothing will be shown. Perhaps writing to the PHP log might be useful?

[eileenmcnaughton]

@seamuslee001 @christianwach @kcristiano do we think this is probably ok as is or needs a Civi::Static ?

[kcristiano]

@eileenmcnaughton

I can do an r-run with this, but based on @christianwach I do think we'd need to use Civi::Static to protect against it being called multiple times.

[braders]

I actually think this change provides less flexibility to site developers and so I am not in favor of this being merged.

In CRM_Core_Invoke, (almost) all Civi logic is wrapped in a try/catch. This catch then calls CRM_Core_Error::handleUnhandledException($e), which in turn calls \Civi::dispatcher()->dispatch('hook_civicrm_unhandled_exception', $event) and renders fatal.tpl.

I run a WordPress site which does two things related to errors:

• Uses the hook_civicrm_unhandled_exception event to add extra logging, so that we can see what exceptions are being trigged and on what URLs
• Adds a custom fatal.tpl.

Our custom fatal.tpl is slightly unconvential in that it just calls a custom Smarty function which looks a bit like this:

if (!is_user_logged_in() && !headers_sent())
{
    // Redirect to login
    $session = CRM_Core_Session::singleton();
    $session->getStatus(true);
    $session->setStatus('Please login to view this page', '', 'alert', ['expires' => 0]);

    wp_safe_redirect('/login');
    die();
}

if (!is_admin())
{
    $context = Timber::context();
    $context['fatal'] = $params;
    Timber::render('fatal.twig', $context);
}
else
{
   echo 'Sorry. A non-recoverable error has occurred. Please view the error logs for details';
}

We've taken the assumption that most exceptions go away when logged-in, although I appreciate this might not be sutiable for every site. In our Timber template we render the WordPress header and footer along with customised error text prompting the user to contact the site owner for help. With this pull request, I'm struggling to see how we would be able to customise the error page in this way.

I actually wonder if the correct course of action is to keep the Exception here, which allows for developers to add logging and other custom handling, but to improve the fatal.tpl and/or the CRM_Core_Error::handleUnhandledException method so that Civi provides out-the-box so that it uses the site's header, footer and styling within the error screen when on the front-end.

It's also worth noting that I think permissionDenied() could be called within the WordPress admin area. In those cases the_content won't be called, and so I don't think this pull request considers that.

[christianwach]

@braders Funnily enough I was looking at this over the weekend too - but from a slightly different angle. I was trying - unsuccessfully as it happens - to wrap $this->civi->invoke() (and indeed CRM_Core_Invoke::invoke() itself) in try/catch statements so that the CiviCRM error response could be passed back to the_content or the Shortcode for display on the front-end. I don't see WordPress admin as such a problem since WordPress itself uses the featureless wp_die() page for permissions failures.

I can appreciate the sophistication of your approach but I would be surprised if it is one that the majority of CiviCRM-WordPress folks are able to implement and I so I remain supportive of the general gist of this PR, even if this implementation isn't quite right. I see no reason why your requirements for logging couldn't be satisfied with the addition to the method of e.g.

$e = new \Exception();
CRM_Core_Error::debug_log_message(ts( 'Permission denied.'));
CRM_Core_Error::debug_log_message($e->getTraceAsString());
... etc etc ...

What is, in my view, the incorrect approach is the current out-of-the-box behaviour.

[braders]

@christianwach Just to be clear, I absolutely am not opposed to CiviCRM handling this better out-of-the-box, I just want to ensure that possibilities for customising the error display still exist for those sites which need it.

Something like the following would probable address my concerns (although a more concise name for the new filter could be found!). This ensures a message is still shown in the admin area, and allows sites to fallback to the current functionality (where they could do something similar to what I suggested above, e.g. for sites that need a custom error page, or for themes that are not using the_content().

public function permissionDenied() {
    status_header(403);

    if ($this->canRenderPermissionDeniedInTheContent()) {
        $CiviWordPressClass = civi_wp();
        add_filter('the_content', [$CiviWordPressClass->users, 'get_permission_denied']);
        return;
    } else {
        throw new CRM_Core_Exception(ts('You do not have permission to access this page.'));
    }
}

public function canRenderPermissionDeniedInTheContent() {
    return apply_filters('civicrm/can_render_permission_denied_in_the_content', !is_admin() && !wp_doing_ajax() )
}

I'm not sure I completely get what you mean about the exceptions. Are you suggesting that code gets added to permissionDenied?

[christianwach]

@braders I agree that your approach is more flexible. And TBH I'm still not entirely sure that calling add_filter('the_content') here is the best approach.

[jaapjansma]

@seamuslee001 @BettyDolfing and I are reviewing PR's and we came across this one. It looks like it needs some additional work. Are you able to do this work? If not are you able to explain why it is not needed or we can close this PR.

[eileenmcnaughton]

@seamuslee001 ^^ was 22 days ago - am closing until you want to revisit (if you do)