WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 22 months ago

#16860 closed enhancement (fixed)

map_meta_cap use "manage_network_users" instead of is_super_admin for edit_users

Reported by: sboisvert Owned by: jeremyfelt
Milestone: 4.4 Priority: normal
Severity: normal Version:
Component: Users Keywords: has-patch needs-codex
Focuses: administration, multisite Cc:

Description

I find that for a multi-site setup, the ability to edit users is inconsistent. there is a capability called "manage_network_users", but this cap if given to "regular" admin's does not give them the power to edit users.
The current code in map_meta_cap in wp-includes/capabilities.php is:

case 'edit_users':
	// If multisite these caps are allowed only for super admins.
	if ( is_multisite() && !is_super_admin( $user_id ) )
		$caps[] = 'do_not_allow';

removing any flexibility, I would suggest having this based on the "manage_network_users" capability, giving us much more flexibility in terms of assigning power to control user administration.

Attachments (6)

capabilities.diff (687 bytes) - added by daniellandau 2 years ago.
Fixes issue for me
user-new.diff (2.9 KB) - added by daniellandau 2 years ago.
Show the "no confirmation" checkbox when user can manage_network_users
joined.diff (3.9 KB) - added by daniellandau 22 months ago.
a single patch containing all modifications
16860.patch (3.6 KB) - added by chriscct7 22 months ago.
Removed rogue line remove in the media-upload.php file from combined patch (the media-upload.php file was otherwise untouched by the patch)
16860.diff (827 bytes) - added by jeremyfelt 22 months ago.
16860-with-tests.patch (5.2 KB) - added by daniellandau 22 months ago.
Incorporates @jeremyfelt's security improvement and unit tests

Download all attachments as: .zip

Change History (29)

#1 @duck_
6 years ago

Note that in terms of flexibility you can always hook into the map_meta_cap filter.

#2 @chriscct7
3 years ago

  • Focuses administration added
  • Keywords dev-feedback added

I agree with the idea here. Marking for core team to take a look at

#3 @jeremyfelt
3 years ago

  • Milestone changed from Awaiting Review to Future Release

I like the thought of this as well. I would much rather have explicit capabilities than rely on is_super_admin() for many things.

#4 @jeremyfelt
2 years ago

  • Focuses multisite added

This could also be attached to delete_user and delete_users at the same time. create_users already uses the add_new_users network option rather than is_super_admin(), though it may benefit from a manage_network_users capability. Any work here helps to define a path for a sort of network admin role that is not a super admin.

@daniellandau
2 years ago

Fixes issue for me

@daniellandau
2 years ago

Show the "no confirmation" checkbox when user can manage_network_users

#5 @daniellandau
2 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

I need to give administrators in my site access to creating and editing users, but I don't want to give them super admin rights (e.g. I don't want them editing theme functions). I understand the need for a more thorough "network admin role", but for the time being these simple fixes do just the trick for me. If there is some security concern or similar that didn't cross my mind, I'd be happy to revise until something achieving the same effects is in acceptable form.

#6 @jeremyfelt
2 years ago

  • Milestone changed from Future Release to 4.3

We should take a closer look at this for 4.3.

#8 @obenland
2 years ago

@jeremyfelt, have you had a chance to take a closer look?

#9 @obenland
2 years ago

  • Milestone changed from 4.3 to Future Release

#10 @daniellandau
2 years ago

It would really be nice to get this moving forward. The patches are really not that big. It's okay if it doesn't make the 4.3 release, but some indication of wheels turning would be appreciated.

#11 @jeremyfelt
22 months ago

  • Keywords needs-refresh added; dev-feedback needs-testing removed
  • Milestone changed from Future Release to 4.4

Thanks for the patch @daniellandau, I think we can make this happen in 4.4. It looks like the two patches should be combined into one for easy application/testing.

@daniellandau
22 months ago

a single patch containing all modifications

#12 @daniellandau
22 months ago

  • Keywords needs-refresh removed

Thanks for taking a look at this!

I uploaded a single patch containing all modifications.

#13 @sboisvert
22 months ago

Thanks for all the work on this everyone, especially Daniellandau, greatly appreciated.

@chriscct7
22 months ago

Removed rogue line remove in the media-upload.php file from combined patch (the media-upload.php file was otherwise untouched by the patch)

#14 @chriscct7
22 months ago

  • Owner set to chriscct7
  • Status changed from new to assigned

Assignments to a milestone must be owned by someone

Last edited 22 months ago by chriscct7 (previous) (diff)

#15 @chriscct7
22 months ago

  • Keywords needs-docs added
  • Severity changed from minor to normal

This should be documented in the Codex whenever this gets merged in.

#16 @MikeHansenMe
22 months ago

  • Keywords needs-codex added; needs-docs removed

#17 @chriscct7
22 months ago

  • Keywords commit added
  • Status changed from assigned to accepted

Good to commit. Post-commit, someone should update the permissions docs to note the change in case people want to start utilizing manage_network_users

#18 @wonderboymusic
22 months ago

  • Owner changed from chriscct7 to jeremyfelt
  • Status changed from accepted to assigned

#19 @wonderboymusic
22 months ago

  • Status changed from assigned to reviewing

@jeremyfelt
22 months ago

#20 @jeremyfelt
22 months ago

  • Keywords needs-unit-tests added; commit removed

I'll take back some of what I said previously. Let's focus on edit_user/edit_users only for this ticket. We can cover create and delete elsewhere. I want to make sure we focus on what this changeset will be doing.

There's a security concern with the current patch in that any user given the manage_network_users capability can also manage super admins, including their passwords. Once that's changed...ouch.

We can check the specific user ID that is passed into map_meta_cap() and bail if that user ID itself is a super admin. I've done this in 16860.diff, though the conditional is pretty crazy.

I'd like to have unit tests attached to this as well. We'll get this in, but it definitely needs to be vetted quite a bit. I read through the other uses of edit_user and edit_users caps and things seem okay. What other possible consequences does this have?

@daniellandau
22 months ago

Incorporates @jeremyfelt's security improvement and unit tests

#21 @daniellandau
22 months ago

  • Keywords needs-unit-tests removed

I added a unit test (with a couple of assertions).

#22 @jeremyfelt
22 months ago

In 33987:

Multisite: Test edit_user capabilities for multisite administrators

An administrator in multisite can not edit users other than itself.

Props daniellandau for the initial patch.
See #16860.

#23 @jeremyfelt
22 months ago

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

In 33988:

Multisite: Allow users with manage_network_users to edit network users.

Other users in a network can now be given capabilities to manage users without also having global super admin privileges.

  • Users with manage_network_users can not edit super admins.
  • Users with manage_network_users can not promote users to super admin.
  • Uses of is_super_admin() in user-new.php are now updated to manage_network_users.

Props daniellandau, chriscct7.
Fixes #16860.

Note: See TracTickets for help on using tickets.