WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#13074 closed defect (bug) (fixed)

Distinguish between user deletion and removal in users.php

Reported by: ryan Owned by:
Milestone: 3.0 Priority: normal
Severity: normal Version:
Component: Users Keywords: has-patch
Focuses: Cc:

Description

users.php makes it possible for a super admin to accidentally delete a user when removing the user from the blog is intended. These two actions should be separated.

Attachments (3)

list-users.diff (5.1 KB) - added by josephscott 4 years ago.
list-users.2.diff (1.4 KB) - added by josephscott 4 years ago.
don't pass user id, don't link username if you can't edit_user
template.patch (535 bytes) - added by layotte 4 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 ryan4 years ago

(In [14178]) Add multisite check on delete. Check promote_user cap. see #13074

comment:3 ryan4 years ago

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

josephscott4 years ago

comment:4 josephscott4 years ago

(In [14189]) New 'list_users' cap to provide more controls over listing users vs. editing
users.

Apply this new cap to the 'Authors & Users' menu item and 'Users' page in
wp-admin.

Bump db version to 14139 to pick up the new cap.

See #13074

comment:5 nacin4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening per IRC discussion. A bit more to do to fully implement list_users.

josephscott4 years ago

don't pass user id, don't link username if you can't edit_user

comment:6 josephscott4 years ago

(In [14191]) - don't pass user id to list_users check

  • only link the username if the edit_user cap check passes

see #13074

layotte4 years ago

comment:7 follow-up: layotte4 years ago

  • Keywords has-patch added

I just noticed that an adminstrator was unable to edit a user...

According to capabilities.php:

case 'edit_user':
	// Allow user to edit itself
	if ( isset( $args[0] ) && $user_id == $args[0] )
		break;
	// Fall through
case 'edit_users':
	// If multisite these caps are allowed only for super admins.
	if ( is_multisite() && !is_super_admin() )
		$caps[] = 'do_not_allow';
	else
		$caps[] = $cap;
	break;

I changed code to link the username if "edit_users" cap check passes... "edit_user" was only allowing edit for self.

I haven't tested the effect of this on MultiSite, FYI.

comment:8 in reply to: ↑ 7 TheDeadMedic4 years ago

@layotte, I was just about to submit a ticket concerning this exact issue.

I believe the line;

$caps[] = $cap;

should in fact be;

$caps[] = 'edit_users';

The problem lies in having skipped the break from the 'edit_user' block, hence falling through to the 'edit_users' block, but then failing the if in the 'edit_users' block, thereby adding a non-existent(?) cap 'edit_user' (note singular) to the $caps array.

comment:9 solarissmoke4 years ago

xref #13137, something has gone wrong that means that current_user_can('edit_user', $user_object->ID) is returning false for super-admins.

comment:10 nacin4 years ago

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

(In [14256]) Explicitly set the capability required in edit_users map_meta_cap branch, so we don't accidentally pass edit_user. props TheDeadMedic. fixes #13074, fixes #13137

Note: See TracTickets for help on using tickets.