Make WordPress Core

Opened 12 years ago

Last modified 3 months ago

#16293 reopened defect (bug)

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

Reported by: petemall's profile PeteMall Owned by:
Milestone: 6.3 Priority: normal
Severity: normal Version:
Component: Users Keywords: has-patch needs-refresh needs-testing needs-unit-tests
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 12 years ago.
Test, please: don't have Wordpress Multisite. But, should fix the problem.
16293.diff (3.9 KB) - added by garyc40 12 years ago.
allow deleting super admins
16293.2.diff (631 bytes) - added by Ipstenu 11 years ago.
Editing users.php to check for SuperAdmin in a non-hardcodey way.
16293.3.diff (596 bytes) - added by JustinSainton 11 years ago.
16293.4.diff (520 bytes) - added by jeremyfelt 9 years ago.

Download all attachments as: .zip

Change History (32)

#1 @PeteMall
12 years ago

  • Description modified (diff)

#2 @SergeyBiryukov
12 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.

@Moskjis
12 years ago

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

@garyc40
12 years ago

allow deleting super admins

#3 @garyc40
12 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.

#4 @georgestephanis
11 years ago

  • Keywords needs-refresh added; has-patch removed

#5 @wonderboymusic
11 years 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.

#6 @SergeyBiryukov
11 years 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

@Ipstenu
11 years ago

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

#7 @Ipstenu
11 years ago

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

#8 @wpmuguru
11 years 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

#9 @JustinSainton
11 years ago

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

Should be good now?

#10 @MadtownLems
10 years 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.

#11 @SergeyBiryukov
10 years ago

  • Keywords 3.7-early added

#13 @wonderboymusic
10 years ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

#14 in reply to: ↑ 12 @nacin
10 years 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.

#15 @jeremyfelt
9 years 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

@jeremyfelt
9 years ago

#16 @jeremyfelt
9 years 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.

#17 @jeremyfelt
9 years ago

#20440 was marked as a duplicate.

#18 @jeremyfelt
9 years ago

  • Component changed from Multisite to Users
  • Focuses multisite added

#19 @jeremyfelt
9 years 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. :)


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


9 years ago

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


9 years ago

#22 @nacin
9 years 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.

#23 @nacin
9 years 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.

#24 @swissspidy
8 years ago

  • Keywords needs-refresh added; commit removed

#25 @webgirl
6 years ago

  • Version 3.1 deleted

On a multi-site set-up hosted on Cloudways a user was created as 0 in database after upgrading. The user with id 0 cannot be deleted but shows added in the database.

Wordpress 4.7.1
Buddypress 2.7.4
bbpress 2.5.12

Memcached
Apache
PHP FPM
Varnish
Nginx

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


3 months ago

#27 @audrasjb
3 months ago

  • Keywords needs-testing needs-unit-tests added
  • Milestone changed from Future Release to 6.3

As per today's old ticket triage session:

The latest patch should still be good to go: comment:19

There were some concerns in comment:23, but without any further details, let's go ahead with the patch unless a closer investigation reveals any other concerns.

Moving for 6.3 consideration.
Thanks @SergeyBiryukov @hellofromTonya for the triage discussion.

Note: See TracTickets for help on using tickets.