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

[PeteMall]

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

[SergeyBiryukov]

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]

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

[garyc40]

allow deleting super admins

[garyc40]

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.

[wonderboymusic]

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

[SergeyBiryukov]

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]

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

[Ipstenu]

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

[wpmuguru]

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

[JustinSainton]

Should be good now?

[MadtownLems]

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.

[SergeyBiryukov]

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

[wonderboymusic]

these are all marked 3.7-early

[nacin]

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.

[jeremyfelt]

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]

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.

[jeremyfelt]

#20440 was marked as a duplicate.

[jeremyfelt]

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. :)

[nacin]

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.

[nacin]

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

[webgirl]

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

[audrasjb]

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.

[oglekler]

@audrasjb it looks like this is fixed already: ​https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-admin/network/users.php#L30C12-L30C12

As the latest patch was comparing the id with a string '0', I'm updating it to use an integer instead. Regarding tests, it seems it would belong to an e2e test instead of a unit test, no? Otherwise, we would need to send a request to the action=delete page, and putting that into a unit test seems a bit out of place.

Trac ticket: https://core.trac.wordpress.org/ticket/16293

[oglekler]

The new patch is very simple but still needs testing. But if it needs Unit test, there is a slim chance that this will be ready before RC1. So, this is the candidate to move into 6.4 milestone.

[oglekler]

Due to lack of Unit tests, I am moving this into 6.4 milestone.

[jeremyfelt]

Regarding tests, it seems it would belong to an e2e test instead of a unit test, no?

Agreed. I've had a hard time thinking through how to test this change with phpunit. This code impacts an admin page handling a POST request, so I think if tests are possible, they'd make more sense in the e2e test environment.

FWIW, I've read through the change and its surrounding code again and I still feel pretty comfortable with my earlier comments on this ticket. :)

[oglekler]

If a user with ID = 1 can be created for testing purposes, patch can be covered with a test 🤔 It needs to be checked.
​https://github.com/WordPress/wordpress-develop/blob/trunk/tests/phpunit/tests/user/wpDeleteUser.php

[oglekler]

Let's check what others are thinking if this needs unit test or not.

[felipeelia]

Hello there! IDK why I didn't receive notifications about the movement in this ticket, sorry for not replying earlier. @oglekler it could be covered with a test, yeah, but it would require a wp_remote_post() call to that page, not a call to a specific function, which (to me) seems weird. I gave a look at the unit test suite and couldn't find a similar example.

I'm happy to write the test if needed (hopefully there is still time to have this in 6.4?)

[oglekler]

@audrasjb can you please provide a hint about unit test? Thank you!

[nicolefurlan]

Thank you @felipeelia! It would be great if you could write a unit test to cover this change.

Bumping this to 6.5 as it doesn't look like it will get resolved this release cycle.

[rajinsharwar]

Punting this Ticket for Future release due to no activity within the cycle. If we have some progress, we can again put this in the milestone.