WordPress.org

Make WordPress Core

Opened 4 months ago

Closed 3 months ago

Last modified 2 months ago

#46957 closed task (blessed) (fixed)

Site Health: Make site health page access be filterable

Reported by: Clorith Owned by: Clorith
Milestone: 5.2.2 Priority: normal
Severity: normal Version: 5.2
Component: Site Health Keywords: site-health has-patch has-unit-tests commit
Focuses: Cc:

Description

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.

Attachments (6)

46957.diff (2.9 KB) - added by spacedmonkey 4 months ago.
46957.2.diff (2.9 KB) - added by spacedmonkey 4 months ago.
46957.3.diff (10.6 KB) - added by palmiak 4 months ago.
46957.4.diff (13.3 KB) - added by palmiak 3 months ago.
I think this should wrap everything up.
46957.5.diff (24.1 KB) - added by peterwilsoncc 3 months ago.
46957-52.diff (24.0 KB) - added by peterwilsoncc 3 months ago.
Backport patch with menu.php conflict resolved.

Download all attachments as: .zip

Change History (54)

#1 @ocean90
4 months ago

#47027 was marked as a duplicate.

@spacedmonkey
4 months ago

#2 @spacedmonkey
4 months ago

  • Keywords has-patch added; needs-patch removed
  • Version set to trunk

I have uploaded a patch 47027.diff.

This adds a new capability that can easily be filtered.

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


4 months ago

#4 @birgire
4 months ago

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 );
Last edited 4 months ago by birgire (previous) (diff)

#5 @birgire
4 months ago

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.

#6 @spacedmonkey
4 months ago

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

#7 @eizzumdm
4 months ago

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.

Last edited 4 months ago by eizzumdm (previous) (diff)

#8 @TimothyBlynJacobs
4 months ago

#47204 was marked as a duplicate.

#9 @peterwilsoncc
4 months ago

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

#10 @palmiak
4 months ago

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

@palmiak
4 months ago

#11 @joehoyle
4 months ago

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.

#12 @palmiak
4 months ago

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

#13 @Clorith
4 months ago

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.

#14 @peterwilsoncc
3 months ago

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.

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


3 months ago

This ticket was mentioned in Slack in #core by clorith. View the logs.


3 months ago

#17 @Clorith
3 months ago

  • Milestone changed from 5.2.1 to 5.2.2

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

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


3 months ago

#19 @palmiak
3 months ago

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

Last edited 3 months ago by SergeyBiryukov (previous) (diff)

#20 @Clorith
3 months ago

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

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


3 months ago

#22 follow-up: @spacedmonkey
3 months ago

@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

#23 in reply to: ↑ 22 @peterwilsoncc
3 months ago

  • Keywords needs-refresh added
  • Owner set to Clorith
  • Status changed from new to assigned
  • Type changed from enhancement to task (blessed)

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

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


3 months ago

#25 @Clorith
3 months ago

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.

#26 @peterwilsoncc
3 months ago

  • Keywords needs-unit-tests added

@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

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


3 months ago

This ticket was mentioned in Slack in #core by marybaum. View the logs.


3 months ago

@palmiak
3 months ago

I think this should wrap everything up.

#29 @peterwilsoncc
3 months ago

  • Keywords has-unit-tests added; needs-refresh needs-unit-tests removed

Thanks, much appreciated.

#30 @spacedmonkey
3 months ago

@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.

#31 @palmiak
3 months ago

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

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


3 months ago

#33 @audrasjb
3 months ago

  • Milestone changed from 5.2.2 to 5.3

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.

#34 @peterwilsoncc
3 months ago

  • Milestone changed from 5.3 to 5.2.2

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.

This ticket was mentioned in Slack in #core-php by peterwilsoncc. View the logs.


3 months ago

#36 @peterwilsoncc
3 months ago

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.

#37 @spacedmonkey
3 months ago

@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.

#38 @peterwilsoncc
3 months ago

@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?

#39 @spacedmonkey
3 months ago

@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.

#41 follow-up: @spacedmonkey
3 months ago

  • Keywords commit added

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.

#42 in reply to: ↑ 41 @peterwilsoncc
3 months ago

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
3 months ago

Backport patch with menu.php conflict resolved.

#43 @peterwilsoncc
3 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#44 @peterwilsoncc
3 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for back porting to 5.2.

#45 @peterwilsoncc
3 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

This ticket was mentioned in Slack in #hosting-community by spacedmonkey. View the logs.


3 months ago

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


3 months ago

#48 @spacedmonkey
2 months ago

  • Component changed from Administration to Site Health
Note: See TracTickets for help on using tickets.