WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 3 weeks ago

#39206 closed enhancement (fixed)

Replace `is_super_admin()` with `current_user_can( 'manage_options' )` in wp-admin/network.php

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

Description (last modified by flixos90)

This is part of the #37616 task. There is 1 is_super_admin() check in wp-admin/network.php that should be replaced with current_user_can( 'manage_options' ).

Attachments (3)

39206.patch (470 bytes) - added by ashokkumar24 5 months ago.
39206-new.patch (467 bytes) - added by ashokkumar24 5 months ago.
39206.diff (3.5 KB) - added by flixos90 3 weeks ago.

Download all attachments as: .zip

Change History (14)

@ashokkumar24
5 months ago

#1 @flixos90
5 months ago

  • Description modified (diff)
  • Keywords has-patch needs-refresh added
  • Milestone changed from Awaiting Review to 4.8
  • Owner set to flixos90
  • Status changed from new to reviewing
  • Summary changed from Replace current_user_can( 'manage_options' ) in wp-admin/network.php to Replace `is_super_admin()` with `current_user_can( 'manage_options' )` in wp-admin/network.php
  • Type changed from defect (bug) to enhancement

Thanks for the patch @ashokkumar24! You are using the manage_network capability there although it should be manage_options (see "Ticket 15" in https://core.trac.wordpress.org/ticket/37616#comment:28 for reference) - could you update the patch accordingly?

#2 @ashokkumar24
5 months ago

Hi @flixos90,

I have updated the patch. Please find the attachment.

#3 @flixos90
5 months ago

  • Keywords needs-refresh removed

Thanks @ashokkumar24!

#4 @flixos90
3 months ago

  • Keywords 2nd-opinion needs-refresh added

I noticed that we might need to rethink our actions here. With the current patch, the capability required is always manage_options. That means when a user on an already-setup multisite tries to access wp-admin/network/setup.php, they can do so even if they are not a network administrator. So we need to change the check the following way:

  • If a multisite, the user must have manage_network.
  • If not a multisite, the user must have manage_options.

There's a small room for discussion on how we implement this. I see two possible ways:

  • Just make the clause a little more complex (as in is_multisite() && ! current_user_can( 'manage_network' ) || ! is_multisite() && ! current_user_can( 'manage_options' )).
  • Introduce a meta capability setup_network and handle the above check in map_meta_cap().

I think the latter approach would be cleaner. Just as a reminder, we would need to add the new meta capability to the unit tests as well.

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


3 months ago

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


3 months ago

#7 @johnbillion
2 months ago

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

#8 @johnbillion
8 weeks ago

I'd prefer a setup_network meta capability (handled in map_meta_cap()) which maps to the logic you mentioned.

Needs a patch, and the cap needs adding to Tests_User_Capabilities::_getSingleSiteMetaCaps() and Tests_User_Capabilities::_getMultiSiteMetaCaps().

@flixos90
3 weeks ago

#9 @flixos90
3 weeks ago

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

39206.diff introduces a setup_network cap as discussed. In single site, it falls back to manage_options; in multisite, it falls back to manage_network_options. While I originally proposed manage_network as fallback, manage_network_options is a better choice since that has been previously used as a capability for the menu item in multisite and aligns better with the single site fallback.

#10 @flixos90
3 weeks ago

  • Keywords needs-dev-note added

Adding needs-dev-note since this is a new capability that should be documented in the multisite dev note for 4.8.

#11 @flixos90
3 weeks ago

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

In 40390:

Multisite: Introduce a setup_network capability for setting up multisite.

setup_network is a new meta capability that brings more granular control over the permissions to setup a multisite environment. In a non-multisite environment it falls back to manage_options while in a multisite it falls back to manage_network_options. The introduction of this capability furthermore allows replacing an is_super_admin() check.

Props ashokkumar24 for the original patch.
Fixes #39206. See #37616.

Note: See TracTickets for help on using tickets.