Site Health: Make site health page access be filterable

[Clorith]

There are scenarios where the install_plugins capability is removed, even though the user has access (for example if the site runs behind version control), but you'd still want access to health check and debug information, especially as plugins and themes may extend this data as well.

Making the capability filterable would help with access in these situations.

[ocean90]

#47027 was marked as a duplicate.

[spacedmonkey]

I have uploaded a patch 47027.diff.

This adds a new capability that can easily be filtered.

[birgire]

In 46957.diff​ I noticed a minor typo in the function's name:

function wp_maybe_grant_site_heath_caps( $allcaps ) {

that should instead be:

function wp_maybe_grant_site_health_caps( $allcaps ) {

i.e. health instead of heath.

Same for this line:

add_filter( 'user_has_cap', 'wp_maybe_grant_site_heath_caps', 1 );

[birgire]

Another note is the wording of the site_health capability suggestion in 46957.diff​:

if ( ! current_user_can( 'site_health' ) ) {

It feels like it's missing the verb part, like to view_site_health or to access_site_health or something similar.

Like we have with the existing install_plugins capability.

[spacedmonkey]

Thanks for the feedback @birgire I have updated the patched based on that feedback.

[eizzumdm]

I wondered why I did not see Site Health on my QA and production environments. In our Pantheon site, we can only write to our dev environment. Then files are moved upstream with version control. Depending on the install_plugins capability for this feature is definitely a bug.

[TimothyBlynJacobs]

#47204 was marked as a duplicate.

[peterwilsoncc]

As the proposed patch is to emulate a primitive capability, it would be good if it was plural to follow the common patterns in WP Core: primitives are plural, meta caps are singular.

It would also allow for meta caps to be added at a later date for individual tests/groups, for example:

current_user_can( 'view_health_statuses' ); // primative
current_user_can( 'view_health_status', 'security' ); // meta

[palmiak]

Based on @peterwilsoncc advice to change cap name to plural. Also I added fix to ajax-actions.php which was missing.

[joehoyle]

As it happens I wanted to do exactly this, thanks for the patch @palmiak!

+1 on the patch, looks good to me. I'm not really sure of the historical reasons for not adding the cap to the roles, but instead mapping it to install_plugins, I guess that's mostly for compatibility with custom roles, and means no migration of role caps are needed.

[palmiak]

@joehoyle thanks - yet most of work was done by @spacedmonkey . I just added one missing thing from Ajax and change cap names :)

[Clorith]

I think we should be able to land this with 5.2.1, I'm not sure if the view_* prefix is the most appropriate though, since you can likely do more with this capability in the future.

I'm not well versed in meta capabilities like @peterwilsoncc speak of, but what I anticipate will happen in a future iteration is that you can click a button in the action section to have tasks completed for you, so then it's more than just viewing.

[peterwilsoncc]

I was unsure of the verb too, so it would be good to brainstorm a few before committing. My thoughts are:

• review
• check
• access

It would be good to get some others' ideas before updating the patch.

In terms of the noun, I missed the work "health" when commenting above. I also think it could do with some work:

• site_health_status(es)
• site_health_report(s)
• site_heath_log(s)

I anticipate will happen in a future iteration is that you can click a button in the action section to have tasks completed for you

As this may require disk access, I think a separate capability for actioning would be warranted. This would potentially be a meta capability.

Finally, I am wondering if install_plugins is the correct permission to mirror when setting up the faux capability. Were the roles to be updated, the capability would be added to administrator so that may be a better capability to check for.

[Clorith]

Punting to 5.2.2, as more input is needed before we decide on a direction here.

[palmiak]

What would you say about changing that capability to manage_options as a quick fix? This would allow bedrock and pantheon users to see SiteHealth.

I provided such fix in #47027

[Clorith]

We actually moved away from manage_options intentionally as it isn't restrictive enough in various scenarios.

[spacedmonkey]

@Clorith why not activate plug-in capability? This means the most logical and normally make the users as somewhere technical. Install plugins is far to limiting.

I still stand by my original patch with the capability of view_site_heath. If others want more granular capabilities that is another ticket. @peterwilsoncc

I want to get this committed ASAP to hit 5.2.2, we already missed 5.2.1

[peterwilsoncc]

Replying to spacedmonkey:

I still stand by my original patch with the capability of view_site_heath. If others want more granular capabilities that is another ticket. @peterwilsoncc

I want to get this committed ASAP to hit 5.2.2, we already missed 5.2.1

I'm not suggesting granular meta capabilities be added on this ticket.

I am insisting that the (faux) primitive capability follow the WP convention of being plural (similar to activate_plugins, edit_posts) so the future ticket adding meta capabilities can follow the WP convention of being singular (similar to activate_plugin, edit_post).

Not following this convention will add complexity to maintaining the Roles and Capabilities component and it's already complex enough.

46957.3.diff​ is the closest, once a decision is made on the faux primative's name and the real primitive to map it to; I'll commit the refresh in a second. I've marked this blessed to reflect this.

@Clorith I've set you as the owner pending the final couple of decisions:

• faux capabilities name
• existing capability to map to

[Clorith]

As we've not seen anything more movement, we'll make the cut now.

We'll be mapping this to the existing install_plugins capability as the safest approach, this ensures that those that can act on issues are the ones to see the health statuses (this is particularly true for multisite setups, where the network admin is the only one with this capability by default). This is also what is currently used in core by the Site Health module).

As for capability name, let's indeed stick with the WP conventions (as there are multiple checks, this makes sense), and use view_site_health_checks sounds good here, as it's descriptive of what you are doing. This is essentially what was proposed in comment:9 but I wanted to avoid the term status/statuses due to how pluralization of that works in English.

[peterwilsoncc]

@Clorith

Thanks for making a call.

I haven't got time to create it but once the patch is refreshed it can be committed.

The refreshed patch will require:

• update to include the new capability name
• capability added to the data providers _getSingleSitePrimitiveCaps() and _getMultiSitePrimitiveCaps() in tests/phpunit/tests/user/capabilities.php

[palmiak]

I think this should wrap everything up.

[peterwilsoncc]

Thanks, much appreciated.

[spacedmonkey]

@palmiak I noticed an issue with 46957.4.diff.

There are 4 references to view_health_statuses_checks that should be replaced with view_site_health_checks.

[palmiak]

That's what happen when you create a patch having fever. I'll try to fix it later. Thanks @spacedmonkey .

[audrasjb]

Moving this one to 5.3 cince there is a new function/filter.

Feel free to take it back to 5.2.2 if needed.

[peterwilsoncc]

I discussed this with @Clorith in Slack, we'd both like to put it in the minor.

I can commit this over the weekend (I’ll refresh the patch if needs be). Probably Sunday US time.

The unit tests needed are adding a string to data providers and the new filter is well established code used in the function above it.

[peterwilsoncc]

In 46957.5.diff​:

• Converts final four checks to view_site_health_checks
• Single site install: Ensures only administrators have above capability
• Multisite install: Ensures only super-admins have above capability
• Makes coding standards changes to capability test file required for formatting test to pass

That final point adds a lot of white space changes, to view the patch without white space ​view the pull request on GitHub with white space changes suppressed.

​Witness the tests passing on Travis CI.

Next steps:

I had to make rather more changes to the refresh than I expected to get the tests passing. So could someone on this ticket/the site health project please do the following within the next 24 hours so I can commit it:

• confirm the patch behaves as expected on a Multisite install
• confirm the patch behaves as expected on a Single site install
• add the commit keyword and ping me on the ticket if you give it the thumbs up.

I have done the first two but code reviews and logic checks are some of my favourite things.

[spacedmonkey]

@peterwilsoncc why did you change it so that only superadmin have access to this functionality? The capability check is much more flexible and easy to manage. As a maintainer of multisite, I can tell you we are trying to get rid of check to super admin where possible.

Worth nothing, in multisite, the only users that do have install plugins capability by default are super admins. So the check is pointless anyway. If you have given the install plugins capability to a role, they should see the health check screen, the same as single site.

Otherwise this patch seems fine.

[peterwilsoncc]

@spacedmonkey install_plugins is in the capability array for admins in a default Multisite setup so without the super admin check, the tests fail.

​https://travis-ci.com/WordPress/wordpress-develop/jobs/206558757

I’m not sure what the preferred approach is for Multisite (one reason for the request for review), are you able to provide some insight?

[spacedmonkey]

@peterwilsoncc The preferred approach is not a breaking change for existing functionality, is any user with install_plugins capability should be able to see health check. If it forces them to also be super admin as well, it makes it not filterable in multisite, making this patch useless for multisite users.

[spacedmonkey]

For the record, it seems like install plugins is not a capabilitiy of administators in multistite.
​https://github.com/WordPress/wordpress-develop/blob/825686e300da493c5209b7139429a65ce527411f/tests/phpunit/tests/user/capabilities.php#L172

[spacedmonkey]

I have tested 46957.5.diff in multisite and it works as I would expect it.

The install_plugins capability in multisite is odd, but patch works around that with the multisite check.

[peterwilsoncc]

Replying to spacedmonkey:

The install_plugins capability in multisite is odd, but patch works around that with the multisite check.

Yeah, it is. Meta caps do some strange things that mean a user with a capability may not have permission to use it.

Thanks for the review I’ll get on to this later today.

[peterwilsoncc]

Backport patch with menu.php conflict resolved.

[peterwilsoncc]

In 45507:

Site health: Introduce view_site_health_checks capability.

Introduces the faux primitive capability view_site_health_checks available to single site admins and multisite super-admin to view the site health page within the admin.

The capability is mapped to the install_plugins capability without being dependent on the file system being writable. This fixes a bug where the feature couldn't be used by sites unable to write to the file system or managed through version control.

The capability is granted on the user_has_cap filter.

Props birgire, Clorith, palmiak, peterwilsoncc, spacedmonkey.
Fixes #46957.

[peterwilsoncc]

Reopening for back porting to 5.2.

[peterwilsoncc]

In 45508:

Site health: Introduce view_site_health_checks capability.

Introduces the faux primitive capability view_site_health_checks available to single site admins and multisite super-admin to view the site health page within the admin.

The capability is mapped to the install_plugins capability without being dependent on the file system being writable. This fixes a bug where the feature couldn't be used by sites unable to write to the file system or managed through version control.

The capability is granted on the user_has_cap filter.

Props birgire, Clorith, palmiak, peterwilsoncc, spacedmonkey.
Merges [45507] to the 5.2 branch.
Fixes #46957 for 5.2.2.