WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#12109 closed defect (bug) (fixed)

map_meta_cap doesnt work for multisite superadmins

Reported by: dd32 Owned by:
Milestone: 3.0 Priority: normal
Severity: normal Version:
Component: Role/Capability Keywords: dev-feedback
Focuses: multisite Cc:

Description

I noticed that Users -> Add New was being shown for Multisite when the Add New page was disabled on the MS options page.

The current logic is that map_meta_cap will remove the create_users capability for when this option is disabled.

However, Thats not always the case, Take the 'create_users' value for example, It'll exist for super admin. you see, Theres a slight issue in that map_meta_cap is never run for Multisite super admins, super admins have ALL caps, even 'cap_doesnt_exist'

IMO, this is something that should probably be removed, super admins should only have caps if their role has the cap. I believe this is currently a work around due to the lack of a "Super admin" role as such.

Code in question: http://core.trac.wordpress.org/browser/trunk/wp-includes/capabilities.php#L725

719	        function has_cap( $cap ) {
720	                if ( is_numeric( $cap ) ) {
721	                        _deprecated_argument( __FUNCTION__, '2.0', __('Usage of user levels by plugins and themes is deprecated. Use roles and capabilities instead.') );
722	                        $cap = $this->translate_level_to_cap( $cap );
723	                }
724	
725	                // Multisite super admin has all caps by definition.
726	                if ( is_multisite() && is_super_admin() )
727	                        return true;

Attachments (1)

12109.diff (1.1 KB) - added by dd32 4 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 in reply to: ↑ description wpmuguru4 years ago

Replying to dd32:

However, Thats not always the case, Take the 'create_users' value for example, It'll exist for super admin. you see, Theres a slight issue in that map_meta_cap is never run for Multisite super admins, super admins have ALL caps, even 'cap_doesnt_exist'

IMO, this is something that should probably be removed, super admins should only have caps if their role has the cap. I believe this is currently a work around due to the lack of a "Super admin" role as such.

Caps are stored by blog/site not by network (i.e. caps can be changed on individual blogs). Therefore, the super admin role does not rely on those caps. As it applies to this instance, the super admin would have to be added as an admin on every site/blog to maintain the super admin role.

comment:2 wpmuguru4 years ago

I'd like to leave this one open so the issue can be taken into consideration if the user_role system is revamped.

comment:3 wpmuguru4 years ago

Related #10201.

dd324 years ago

comment:4 dd324 years ago

  • Keywords has-patch dev-feedback added

attachment 12109.diff added

  • Runs map_meta_cap() for ALL users.
  • In the event that a super admin is the current user, Returns true for all caps, Unless specifically denied by map_meta_cap.
    • Untested in a multi-site/multi-user MS environment, but see no reason why it wouldnt work

comment:5 nacin4 years ago

I like this patch. Super admins don't have consistent caps in an MS environment as wpmuguru said, in the patch we're granting all anyway unless map_meta_cap specifically denies them. This makes a lot of sense. +1.

comment:6 wpmuguru4 years ago

Patch okay by me.

comment:7 wpmuguru4 years ago

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

(In [13270]) use map_meta_cap for multisite superadmins, props dd32, fixes #12109

comment:8 wpmuguru4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I found several instances where the code flops back and forth between is_super_admin() & current_user_can( 'manage_network*' ).

Re-opening and will patch later tonight.

comment:9 wpmuguru4 years ago

(In [14003]) more manage_network_* caps, see #12109

comment:10 wpmuguru4 years ago

(In [14011]) add permission checks to grant/revoke_super_admin(), see #12109, related #12460

comment:11 nacin4 years ago

  • Keywords has-patch removed

I am very confused by [14011]. These checks are already made here:

	if ( is_multisite() && !IS_PROFILE_PAGE && current_user_can( 'manage_network_options' ) )
		empty( $_POST['super_admin'] ) ? revoke_super_admin( $user_id ) : grant_super_admin( $user_id );

comment:12 wpmuguru4 years ago

That didn't apply the permission checks if the function calls were in themes or plugins.

comment:13 wpmuguru4 years ago

(In [14033]) use meta caps in [14032], see #12109

comment:14 nacin4 years ago

Whenever these functions are used in core, they need to be paired with proper capability checks, and they are.

Here are various functions that do not have any capability checks: wp_delete_post(), wp_delete_user(), wp_revoke_user(), wp_insert_user(), not to mention the entire capabilities API.

Not providing a full API is a huge problem, and it rears its head all too often. It happens in MU code all over the place. It stifles innovation and it prevents us from making changes in the future without breaking every plugin that had to go it alone previously.

If we decide to change the storage schema, or offer an alternative such as #12815, anyone not using the API will have their code broken. In many cases, we are providing an API so we can detach plugins from the schema underneath, enabling it to evolve and be flexible.

Any plugin can run update_site_option(), any plugin can modify the DB value, any plugin can attach hooks to the _site_option() API, and any plugin should be able to avoid all of those and use an abstracted API without penalty.

It boils down to this: Plugins and themes can do anything. They're just going to avoid the API when we pretend they can't or when we attempt to restrict them.

I am reverting [14011] and adding in transaction actions.

comment:15 nacin4 years ago

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

(In [14040]) Revert [14011]. Add some actions. fixes #12109, see #12460.

Note: See TracTickets for help on using tickets.