Opened 14 years ago
Last modified 8 months 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: | |
Component: | Users | Keywords: | has-patch needs-testing needs-unit-tests dev-feedback |
Focuses: | multisite | Cc: |
Description (last modified by )
You can't delete a user with user id of 1.
See: source:trunk/wp-admin/network/edit.php@17326#L359
Attachments (5)
Change History (47)
#3
@
14 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.
#5
@
12 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
@
12 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
#7
@
12 years ago
Took garyc40's patch and applied it to network/users.php (didn't blow up my localhost)
#8
@
12 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
#10
@
12 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.
#12
follow-up:
↓ 14
@
11 years ago
A brief discussion in IRC: https://irclogs.wordpress.org/chanlog.php?channel=wordpress-dev&day=2013-04-25&sort=asc#m601642
#14
in reply to:
↑ 12
@
11 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
@
11 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 inconfirm_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 theadmin_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
#16
@
11 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 checkadmin_user_id
oradmin_email
as that can be assumed as already changed. admin_email
can be changed to another user while leavingadmin_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.
#19
@
11 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.
11 years ago
This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.
11 years ago
#22
@
11 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
@
11 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.
#25
@
8 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.
20 months ago
#27
@
20 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.
#28
@
16 months ago
@audrasjb it looks like this is fixed already: https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-admin/network/users.php#L30C12-L30C12
This ticket was mentioned in PR #4791 on WordPress/wordpress-develop by @felipeelia.
16 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
@
15 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
@
15 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.
14 months ago
#33
@
14 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.
13 months ago
#35
@
12 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.
12 months ago
#37
@
12 months ago
- Keywords dev-feedback added
Let's check what others are thinking if this needs unit test or not.
#38
@
12 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?)
#40
@
12 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.
hughestm in #16300: