WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 3 weeks ago

#16293 reopened defect (bug)

In multisite installs, users with id 1 can't be deleted

Reported by: PeteMall Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.1
Component: Users Keywords: has-patch commit
Focuses: multisite Cc:

Description (last modified by nacin)

You can't delete a user with user id of 1.
See: source:trunk/wp-admin/network/edit.php@17326#L359

Attachments (5)

edit.patch (577 bytes) - added by Moskjis 3 years ago.
Test, please: don't have Wordpress Multisite. But, should fix the problem.
16293.diff (3.9 KB) - added by garyc40 3 years ago.
allow deleting super admins
16293.2.diff (631 bytes) - added by Ipstenu 18 months ago.
Editing users.php to check for SuperAdmin in a non-hardcodey way.
16293.3.diff (596 bytes) - added by JustinSainton 18 months ago.
16293.4.diff (520 bytes) - added by jeremyfelt 4 months ago.

Download all attachments as: .zip

Change History (28)

comment:1 PeteMall3 years ago

  • Description modified (diff)

comment:2 SergeyBiryukov3 years ago

  • Keywords needs-patch added

hughestm in #16300:

The problem is that it's looking for hard coded values to not delete. It should check to see if you have another user set as super_admin, and if so, then allow you to delete this account.

Moskjis3 years ago

Test, please: don't have Wordpress Multisite. But, should fix the problem.

garyc403 years ago

allow deleting super admins

comment:3 garyc403 years ago

  • Keywords has-patch added; needs-patch removed

hughestm's approach won't work, because deleting super admins is currently not allowed.

What we need to do is change:

if ( $id != '0' && $id != '1' ) {

to

if ( ! is_super_admin( $id ) ) {

Then hughestm can change site option admin_email to something else, demote user with id 0 or 1 to a normal user, and delete him as usual.

This is not very straight forward process, so maybe it's better to allow deletion of super admins. The patch I attached does this. It really needs a sanity check.

comment:4 georgestephanis22 months ago

  • Keywords needs-refresh added; has-patch removed

comment:5 wonderboymusic18 months ago

  • Keywords 3.2-early dev-feedback needs-refresh removed
  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from new to closed
  • Type changed from defect (bug) to enhancement

Can't delete right away, but you can after pressing edit and revoking super admin privileges, which I think is fine.

comment:6 SergeyBiryukov18 months ago

  • Keywords needs-refresh added
  • Milestone set to Future Release
  • Resolution wontfix deleted
  • Status changed from closed to reopened
  • Type changed from enhancement to defect (bug)

Replying to wonderboymusic:

Can't delete right away, but you can after pressing edit and revoking super admin privileges, which I think is fine.

The Delete link appears after revoking super admin privileges, but it does nothing.

The hard coded user IDs that cannot be deleted are still there:
http://core.trac.wordpress.org/browser/tags/3.4.2/wp-admin/network/users.php#L99

Ipstenu18 months ago

Editing users.php to check for SuperAdmin in a non-hardcodey way.

comment:7 Ipstenu18 months ago

Took garyc40's patch and applied it to network/users.php (didn't blow up my localhost)

comment:8 wpmuguru18 months ago

The issue with 16293.2.diff is that some networks will only have one super admin. The last condition should be removed

&& count( get_super_admins() ) > 1

JustinSainton18 months ago

comment:9 JustinSainton18 months ago

  • Keywords has-patch dev-feedback added; needs-refresh removed

Should be good now?

comment:10 MadtownLems12 months ago

  • Cc jason.lemahieu@… added

I've also confirmed that this neither blows up my 3.5.1 install (or anything else), and that it allows me to successfully delete a de-super-admin'd user with ID of 1, prompting to reassign posts as expected.

comment:11 SergeyBiryukov12 months ago

  • Keywords 3.7-early added

comment:13 wonderboymusic9 months ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

comment:14 in reply to: ↑ 12 nacin7 months ago

  • Milestone changed from 3.7 to Future Release

Replying to SergeyBiryukov:

A brief discussion in IRC: https://irclogs.wordpress.org/chanlog.php?channel=wordpress-dev&day=2013-04-25&sort=asc#m601642

Concerns there not yet addressed.

comment:15 jeremyfelt4 months ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from Future Release to 3.9

Looking at 16293.3.diff:

  • We don't need to check is_super_admin( $id ) as this is handled (with an error message) later in confirm_delete_users()
  • Less sure on the current user check. It depends on what a user with manage_network_users capabilities who is not a super admin is able to do.
  • Checking admin_user_id may make sense in that it should have already been updated to another ID through another process if the intent is to delete the original admin. This seems more appropriate than the admin_email check, but should be investigated some more.

Most likely, if additional restrictions are added for the deletion of users, it should be inside confirm_delete_users() so that the messaging is easy to display as with other errors.

I do think we can work to get this into the next cycle if we clarify a couple things here.

Related: #20440

jeremyfelt4 months ago

comment:16 jeremyfelt4 months ago

Additional info:

  • If the admin_email has not been changed, you are unable to revoke super admin privileges on a user. This could remove the requirement to check admin_user_id or admin_email as that can be assumed as already changed.
  • admin_email can be changed to another user while leaving admin_id as is. admin_id does not prevent a super admin from being demoted.
  • If we remove the check for $id != 0, it is unlikely though possible that you end up with this screen, so we should leave that check there.

16293.4.diff removes only the $id != 1 check.

I *think* the only thing up in the air with this patch is the behavior of the manage_network_users capability.

comment:17 jeremyfelt3 months ago

#20440 was marked as a duplicate.

comment:18 jeremyfelt3 months ago

  • Component changed from Multisite to Users
  • Focuses multisite added

comment:19 jeremyfelt4 weeks ago

  • Keywords has-patch commit added; dev-feedback 3.7-early needs-patch removed

Looking through and testing 16293.4.diff again.

Requirements for deleting a user from the network:

  • Current user can manage_network_users
  • Current user has delete_user cap
  • User being deleted is not a super admin
  • The email of the user being deleted is not currently set as the network admin email

If these are in order, a user with an ID of 1 can be deleted using bulk actions. A user with an ID of 1 cannot be deleted using the delete row action link even though the delete row action link appears. There are no restrictions on a user ID of 2, contrary to the ticket title.

16293.4.diff does the trick as all the existing checks take care of normal concerns when deleting users from the network.

If the manage_network_users capability is assigned to user ID 1, they are no longer a super admin, and the delete_user meta capability is mapped to the user—they can delete themselves from the network after reassigning content to another user. This is the case with any user other than ID 1 as well. The experience is actually not that bad if you're expecting it. :)


comment:20 ircbot3 weeks ago

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.

comment:21 ircbot3 weeks ago

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.

comment:22 nacin3 weeks ago

  • Description modified (diff)
  • Summary changed from In multisite installs, users with id 1 or 2 can't be deleted to In multisite installs, users with id 1 can't be deleted

This harks back to http://mu.trac.wordpress.org/changeset/297 (wow secure very). It's also always been just user ID 1, never 2.

This used to be the same way for sites, too, but it got switched to $current_site->blog_id at some point.

comment:23 nacin3 weeks ago

  • Milestone changed from 3.9 to Future Release

I'm seeing some oddities here while poking around. Would some hardening of our checks here before making any changes.

Note: See TracTickets for help on using tickets.