Make WordPress Core

Opened 13 years ago

Last modified 6 weeks 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: Future Release Priority: normal
Severity: normal Version:
Component: Users Keywords: has-patch needs-testing needs-unit-tests dev-feedback
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 13 years ago.
Test, please: don't have Wordpress Multisite. But, should fix the problem.
16293.diff (3.9 KB) - added by garyc40 13 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 10 years ago.

Download all attachments as: .zip

Change History (47)

#1 @PeteMall
13 years ago

  • Description modified (diff)

#2 @SergeyBiryukov
13 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
13 years ago

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

@garyc40
13 years ago

allow deleting super admins

#3 @garyc40
13 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
12 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
11 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
11 years ago

  • Keywords 3.7-early added

#13 @wonderboymusic
11 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
10 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
10 years ago

#16 @jeremyfelt
10 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
10 years ago

#20440 was marked as a duplicate.

#18 @jeremyfelt
10 years ago

  • Component changed from Multisite to Users
  • Focuses multisite added

#19 @jeremyfelt
10 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.


10 years ago

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


10 years ago

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

  • Keywords needs-refresh added; commit removed

#25 @webgirl
7 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.


13 months ago

#27 @audrasjb
13 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.

This ticket was mentioned in PR #4791 on WordPress/wordpress-develop by @felipeelia.


9 months ago
#29

  • Keywords needs-refresh removed

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

#30 @oglekler
8 months ago

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.

#31 @oglekler
8 months ago

  • Milestone changed from 6.3 to 6.4

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

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


7 months ago

#33 @jeremyfelt
7 months ago

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

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


6 months ago

#35 @oglekler
5 months ago

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

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


5 months ago

#37 @oglekler
5 months ago

  • Keywords dev-feedback added

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

#38 @felipeelia
5 months ago

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?)

Last edited 5 months ago by felipeelia (previous) (diff)

#39 @oglekler
5 months ago

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

#40 @nicolefurlan
5 months ago

  • Milestone changed from 6.4 to 6.5

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.

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


6 weeks ago

#42 @rajinsharwar
6 weeks ago

  • Milestone changed from 6.5 to Future Release

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.

Note: See TracTickets for help on using tickets.