WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 3 months ago

#39205 closed enhancement (fixed)

Replace is_super_admin() check with current_user_can( 'upgrade_database' )

Reported by: dhanendran Owned by: flixos90
Milestone: 4.8 Priority: normal
Severity: normal Version:
Component: Role/Capability Keywords: has-patch needs-dev-note has-unit-tests commit
Focuses: multisite Cc:

Description

This is part of the #37616 task. There is an is_super_admin() check which needs to be replaced with current_user_can( 'upgrade_database' ) in wp-admin/includes/ms.php (line 776)

Attachments (3)

39205.patch (396 bytes) - added by dhanendran 7 months ago.
39205.diff (2.1 KB) - added by flixos90 5 months ago.
39205.2.diff (2.7 KB) - added by flixos90 3 months ago.

Download all attachments as: .zip

Change History (16)

@dhanendran
7 months ago

#1 @flixos90
7 months ago

  • Focuses administration removed
  • Keywords has-patch needs-dev-note added
  • Milestone changed from Awaiting Review to 4.8
  • Owner set to flixos90
  • Status changed from new to assigned
  • Type changed from defect (bug) to enhancement

Thanks for the patch @dhanendran, looks good!

We'll need to have some discussion on how this new capability should be handled: Should we not handle it at all (meaning only the super admin has that capability) or should we add a clause in map_meta_cap() to grant that capability to regular admins on non-multisite setups as well? Is there another place in core where this capability could be used?

We definitely need to document the new capability in a dev note and add it to the unit tests array in Tests_User_Capabilities::_getMultiSiteMetaCaps().

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


7 months ago

#3 @johnjamesjacoby
7 months ago

Should we add a clause in map_meta_cap() to grant that capability to regular admins on non-multisite setups as well?

This is what I recommend we work towards. It's the most backwards compatible, and provides a filterable funnel for all database upgrade checks.

Is there another place in core where this capability could be used?

  • Maybe wp_should_upgrade_global_tables()?
  • Maybewp_upgrade()?
  • Maybe some additional context could/should be passed in, so plugins can come to their own conclusions

#4 @johnbillion
7 months ago

  • Keywords needs-unit-tests added

#5 @flixos90
7 months ago

@johnjamesjacoby: I like the idea of having this capability check before upgrade routines. I think we should only check for this when upgrades happen via the UI though as I'm not quite sure about the consequences if we put such a check into functions like wp_upgrade() directly (for example, how does WP-CLI handle this?).

My approach would be to put current_user_can( 'upgrade_database' ) into wp-admin/network/upgrade.php, it could replace the existing manage_network check in order to be more precise. We might also be able to insert the same check into wp-admin/upgrade.php - the latter would depend on our map_meta_cap() approach for the function though: Currently everyone can access the wp-admin/upgrade.php screen and perform a database upgrade as far as I can see. We can either change that (probably not though), or we could say that on a non-multisite setup the capability maps to exist which everyone has. It could look like this (in map_meta_cap()):

case 'upgrade_database':
    if ( is_multisite() ) {
        $caps[] = 'manage_network';
    } else {
        $caps[] = 'exist';
    }

The only thing that would technically change is that in a multisite setup only the network administrator could access the wp-admin/upgrade.php page while currently everyone can. I don't really think the current (pretty much open) implementation makes sense, but I might be missing something.

Anyways, if we do anything with that capability beyond the existing patch (and unit tests), we should handle this in a separate ticket.

@flixos90
5 months ago

#6 @flixos90
5 months ago

  • Keywords has-unit-tests 2nd-opinion added; needs-unit-tests removed

As the general update routines in WordPress are publicly accessible (see #34200), let's only work in network upgrade scope here. I uploaded 39205.diff which I think is good to go, except for one major question: Since we're only dealing with upgrading the network, should we rather be calling the capability upgrade_network instead of the general upgrade_database?

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


5 months ago

This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.


5 months ago

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


3 months ago

#10 @johnjamesjacoby
3 months ago

Checking upgrade_network and passing in the network ID is an interesting idea. I like it.

@flixos90
3 months ago

#11 @flixos90
3 months ago

  • Keywords 2nd-opinion removed

39205.2.diff uses a more specific upgrade_network capability, as mentioned above. That capability is also used for the menu page in the network administration panel, which was missed in the previous patch.

@johnjamesjacoby I like the idea of passing an ID to it, however we should follow these efforts in a separate ticket in the future where we can investigate more granular network capabilities in general. It should not be a problem to adjust the existing capabilities in a backward-compatible way, since we can always pre-fill with the current network if none is passed to it. Feel free to open one (it would fit well into this list: https://core.trac.wordpress.org/query?status=accepted&status=assigned&status=new&status=reopened&status=reviewing&id=29213%2C31020%2C33240%2C38652%2C39083%2C39084%2C39156%2C39677&col=id&col=summary&col=status&col=owner&col=priority&col=milestone&col=version&order=priority)!

#12 @johnbillion
3 months ago

  • Keywords commit added

I reviewed and tested this yesterday and it all looks good.

At WordCamp Torino we briefly discussed the idea of naming the cap update_network but soon started down a rabbit hole. upgrade_network is good.

#13 @flixos90
3 months ago

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

In 40404:

Multisite: Introduce an upgrade_network capability.

Prior to this change, a mix of is_super_admin() calls and manage_network capability checks was used to determine whether the current user could upgrade the network. With this changeset a dedicated capability is introduced that allows more granular handling.

Props dhanendran for the original patch.
Fixes #39205. See #37616.

Note: See TracTickets for help on using tickets.