Prevent account cancellation for uid 1

Is it a feature request ... or a bug? Because UID 1 is critical enough to be deleted by mistake, which is something stupid, but possible.

Related issue: http://drupal.org/node/48147

I would most certainly consider this a bug. I have deleted uid #1 on accident on my test site and the only way to recover is to create a new entry in the users table with a uid of #1. There should be no way to delete user #1.

Another problem comes with user managment ('administer users') delegation, each user with this permission can delete UID 1.

It's a bit dangerous...

Which is why UID 1 should be kept as "special", as it's the site owner's trump card account. It shouldn't be deletable, and people with permission "administer users" should still not be able to edit it. If you can't edit it, you can't delete it. :-)

See: http://drupal.org/node/39636

While a separate level to also protect other "administer users" users, as killes hints at in that thread, might be useful as well, walling off UID 1 from the rest of the userbase is a good thing, IMHO.

Why don't we launch a poll to see what Drupal users prefer?

:insert rolling eyes smilie here:

guess what? w/ the current situation, uid 0 can also be deleted by anyone w/ admin users perms--very not good. i really think we should prevent any deletion of uid 0 and 1 by anybody, including uid 1, when it comes to the user interface. seriously, if somebody is knowledgeable enough to intelligently run w/o either one of those, then they are perfectly capable of figuring out a non-UI method of accomplishing it. so let's protect the rest of the folks from seriously screwing up their system, eh? :)

attached patch solves this pretty well, i think.

oops. THIS patch nicely solves the problem...

I don't really get why people see this as a problem. If people want to shoot themselves into the foot, let them.

it can be debated that user 1 deleting is desired for power users -- but users who know enough to operate a drupal site w/o uid 1 can hack user module, delete via database or whatever. but for those who was not here yesterday and even for some of them this patch is needed.

killes: the point is that they _don't_ want to shoot themselves in the foot. why should we penalize people for not having knowledge at your level? it's unnecessary.

I've marked http://drupal.org/node/39636 as a duplicate of this, since I like hunmonk's version (block UID 0 and 1 instead of just 1) better than mine.

+1 on this. It's a trivial fix that provides a good safety net for non-expert admins *and* provides better security (UID 1 is a safe "Trump card" account). Let's get this committed.

IMO, this isn't a problem. In all the years, this hasn't been a real problem.

That's that.

http://drupal.org/node/49101

Here's a few ideas, you can take them or leave them.

1) When deleting user 1, instead of deleting the account, it actually resets it. Set the username back to admin, password to random characters, remove all profile fields (if the module is enabled), remove all previous posts, etc.

2) Go ahead and delete user number 1, but if no other user has administrative rights, either give an option of giving another user the admin rights, or let user 2 default to admin role. I don't really think defaulting to user 2 would work, but I just thought I'd throw it out there.

3) Do not allow user 1 to delete himself. This would make sure that another user has admin access, or at least user admin access in order to give himself an administrative role.

Having had a stressful day trying to solve why a number of nodes were showing me 403 errors it turned out they were owned by uid 0 and my users table had magically lost its uid 0.

I can't believe how easy it could be to delete uid 0 by a site admin.

Even if uid 1 is not protected for reasons in previous comments, surely uid 0 should be saved from any possible deletion?

budda, I ended up coding a small contrib to add this level of protection:
http://forums.phpmix.org/viewtopic.php?t=1008

Since both users (0 and 1) are critical (and/or not easy to recover), I think something ought to be done in core itself, though.

Does anyone have the sql to insert a the user 1 ?

This was brought up again by the Portland UI-SIG's review of Drupal: http://drupal.org/node/126143

Reseting to active since it hasn't been reviewed in a year, but feel free to won't fix it again.

+1 from me for UID 1 at least. It is indeed a special case for Drupal, and there's no point in trying to deny that. As Drupal becomes less developer-centric these types of usability reforms will become more helpful to Drupal admins.

I'm still +1 on this. It clearly is a problem for some people. In the standard use case, not having uid 1 or uid 0 is an extremely bad idea, and recovering from it is hard. If you're running an edge case-site where for some reason you don't need those, then you should know what you're doing (read: know how to do it manually in the admin). I'd much rather require people building an edge case abnormal site to have to go out of their way to delete one or two records in the database than leave open the possibility for any "personal blogger" site to be hosed by clicking the wrong button by accident, and the more potential admins you have on a site the bigger the risk of that happening.

this does what you want:

http://drupal.org/project/userprotect

not saying that we shouldn't bake it into core -- just pointing out that the feature you wish is easily available today...

Related to: http://drupal.org/node/75289

Lost user #1
How do I re-add a deleted user 1?
Accidentally deleted admin account
Help! I deleted the "super Admin!"
help please FAST, I deleted user 1, how can i recover
URGENT: I deleted user 1 by mistake

This is clearly a problem.

And note that four of those are nodes are in the 100,000s, which means that these people have had a nice, friendly installer and may well have never had to mess with SQL before. So when they're told that the way to fix the problem is do an INSERT INTO user... that is extremely problematic.

hunmonk: are you still planning to work on this, or can I yoink it from you?

@webchick: i wrote http://drupal.org/project/userprotect as my solution to the problem. i've had my fight on this one going into core -- feel free to take up the cause... :)

I started evaluating Drupal today, and the one of the reasons Drupal drew my attention is the ACL funtionality.
I was very disappointed to see that a user with "administer users" rights is able to change the superuser credentials!!!

On a larger site it is very common that there are administrators who manage users. Of course the should NEVER be able to change the superuser info!

Keep up the good work!

Joep

Here is an updated version for d6 that also protects the user admin page.

I think we'll be able to fix this more elegantly once we have a callback per-button. We'd need less code so I'm tempted to put this patch on hold until the FAPI3 patch lands.

Marked http://drupal.org/node/153118 as a duplicate. FAPI 3 has long since landed. What's the status of this issue?

Looked at the latest patch.

- Seems like only protection for user #1 remained. I cannot find out where could someone remove user #0, was that a possibility in previous versions and not in Drupal 6?

I am positive that we should protect user #1 from being deleted. We might be able to do it more extensibly with a per-button callback, but contributed modules can already form_alter the delete button out of the UI, if need be, so there a callback would not help. I don't really see what Dries envisioned here.

Let's get this to FAPI 3 before going any farther

I've long thought that there should be an option to run a strictly role-based security model, where there is no superuser (think Windows security). However, simply deleting User 1 is a fairly blunt-force-trauma way of going about it.

Why not create a setting in settings.php called $super_user_id and use that in core instead of the literal 1? In this implementation, we can delete-protect whatever user it refers to. The occasional power admin can still create a rootless site by setting $super_user_id to null. We can then be certain of the site owner's intent.

Also, if a site owner somehow lost control of the site (to root password compromise, etc), they could conceivably recover by creating a regular user and setting $super_user_id to their new uid.

It may sound stupid, by why physically delete users at all? I think additional binary flag in the users table (or status field can be re-used) showing if a user has been deleted. That would make easy user recovery for any account. Core API should filter out deleted accounts, so they are not visible from the front-end.

+1 -- I almost accidentally deleted user 1 due to the re-arranged form buttons throughout core.

Updated hunmonk's patch, merged with ChrisKennedy's changes.

Subscribing and like to say, I cannot delete "Administrator" in Windows, too. Additional one of my users deleted the user 1 by mistake today, too. Recreating is painful and impossible by someone not experienced and you must know how the Drupal DB is working, sometimes setting back sequences, etc.

Why did you set it back to 6? It needs to go into 7.x before going into 6.x.

subscribe

I assume patch #36 was only meant for Drupal 6?
It still applied to the latest HEAD, but doesn't seem to be working correctly.
On user/1/edit the 'Delete' button has disappeared, but user/1/delete is still accessible and working (admin user gets deleted).
Also, trying to delete user 1 on the admin/user/user page shows two few 'notice' errors.

Any chance this gets updated for the latest HEAD?

@xiv
I'm going to have to agree - to an extent. Though that should be discussed in a separate issue.

Subscribing.

I'm 99.99% sure this patch needs a re-roll. And by speaking of D7, most probably a test, too.

Would it be any easier to just change user_delete()?

I'm +1 for this. I guess I don't care where the check is made (I'll let the more knowledgable folks debate that). As long as Drupal has these built-in dependencies on UID 1, we shouldn't be able to delete it.

@hass - #37 is too true. You can rename the Windows Administrator account to whatever you want, but it always uses the well-known SID and can't be deleted.

#43 could also be edited to not allow a user to delete their own account:

No, if a user cannot be deleted then there should not be an option to do this in the first place. That's like the question: "Do you see 'edit' links on my issue follow-ups?" (I guess not, unless you are one of our rocking webmasters)

#45 is a different issue, because compared to uid 1, you could easily re-create yourself a new user. This issue is about deleting user root on Linux, deleting user Administrator on Windows, and deleting the first user in Drupal. Only the latter is made possible too easy and would have to be re-created by a developer, as outlined in earlier follow-ups.

What about adding an 'global_admin' flag in the users table?

Then, instead of checking for user-uid == 1, a new core function could be added so you could do user_is_global_admin().

This one could look something like this:

Then, user_delete could look something like this:

Finally, we would need a method to assign Global users, maybe a flag in the user add/edit form available only to already existing global administrators. Validation here would have to check the site cannot be left without global administrators, maybe checking that one cannot remove such a flag from him/herself.

Folks, please stop hi-jacking this issue. Half-backed support for something that does not exist yet does not help anyone. There is uid 1 only. And this issue is about deleting uid 1 only.

FWIW, see #64861: META-roles, user registration handling and SPAM registration, #182023: Add a third default role to core for handling Administrative duties.

I'm sorry, sun. I missed those issues. Drupal is getting bigger as time passes...

I guess, this particular issue might depend on how those proposals evolve.

Since this is issue is very well scoped (simply don't allow user 1 to be deleted, ever) I would not like to hold it up for other possible initiatives such as the not making user 1 special, which may or may never get solved. If that issue ever gets a patch that gets to a committable state, then it can always modify the behaviour of deletion to go against a role instead of a specific user, or whatever.

Let's stay on-topic here please, folks.

1 remaining question: Does this require a test? If so, what would we want to test?

Yeah, we should add a test as well. Seems like something that might easily break down the road if we don't document how it's intended to work.

Testing user_delete seems like a good thing to do, as well as the various points throughout the interface where this can be done (user/1/delete, admin/user/user, ..?).

I've to admit - I don't know how to write tests. Since this seems the only missing thing to get this in - who has the fu to write those tests?

The patch in #51 doesn't work as intended. It causes a "Notice: Undefined variable: form_values in user_admin_account_validate()".

If, after applying this patch, one attempts to block the superuser account via the form on admin/user/user, one finds that one is successful. It's tantamount to the same thing as deleting the superuser account, since no one would be able to login to this account. People who complain about accidentally deleting their superuser account are doing so because they're ostensibly trying to login to that account, and cannot.

What I recommend is that we remove the checkbox for the superuser account on admin/user/user, and then remove the delete button from user/1/edit. If we do it this way, we won't need any tests nor will we need another function in core or even another drupal_set_message. Plus, this method makes it easier to grok from a UI / DX perspective.

See what you think.

Snippet for Drupal 6.

The last submitted patch failed testing.

#55 implemented as an addon module: http://drupal.org/project/protect_critical_users

Hope that helps until this is properly addressed in core.

#54 isn't safe because you can still use an url...

Safe? Mmmm? The idea is to not hand people the keys to the wrecking ball crane. If you are smart enough to be able to handcraft a url that deletes the very account you're logged in as, be our guest.

No, what I'm more concerned with is why the patch in #54 fails testing for invalid syntax, but still runs and works fine. I canna figure it out, Captain. Help, somebody?

try this patch, it even includes a test.

I'll take a look at this more later... but, looking at the patch this simply won't work. Here's why...

<?code>
function user_delete($edit, $uid) {
+ // User #1 can't be deleted
+ if ($account->uid == 1) {
+ drupal_set_message(t('Failed to delete user: This user is the superuser or root administrator, and cannot be deleted.'), 'error');
+ return;
+ }
+
$account = user_load(array('uid' => $uid));

You check $account for the id before $account contains the user information. It might simply be easier to do something like:

fixed and re-rolled against head ;)

Did a more detailed review.... I look forward to this getting into D7.

The 4 tests should be their own test methods. If there is common setup between them it should be done in the setUp method.

The method name TestRootAdminNoDelete should start with a lowercase t. So, it should be testRootAdminNoDelete.

I'm not a fan of the message 'Failed to delete user: This user is the superuser or root administrator, and cannot be deleted.'. It doesn't specify what user can't be deleted. Maybe something like 'Failed to delete user 1: This user is the superuser or root administrator, and cannot be deleted.'

The note on user_delete would read better as "@note User 1 cannot be deleted."

Why is the checkbox for user 1 removed from the form in user_admin_account? I understand stopping the delete operation. But, with this for other operations are permitted and can be extended beyond the core provided ones. The checkbox should remain for those.

undeletable is not a word. The comment on the user_profile_form should read something like:
+ // User 1 cannot be deleted so we don't display the delete button.

subscribing

intergrated comments, but adapted testcase differently.

The last submitted patch failed testing.

at least the bot works now...

why do i always miss the most glaring errors...
thanks Heine!

and i always forget... something...

The last submitted patch failed testing.

48 passes, 0 fails, and 0 exceptions on the testcase in the patch...

did some cruft to allow logging in as user #1 in simpletest (thanks for that go to Dave Reid).

Not sure in D7, but in D6 it is possible to do user/0/delete too. It was not possible in D5.

similar issue, I may fix that in a followup issue, let's get this one in core first.

Is there a reason you used the code below rather than the setUp method?

Are there any other examples of this usage in core? Commonality is a big deal to the core maintainers.

I coudn't use the setUp method, without completely duplicating the test code.

the idea behind this code is, that the same tests get run, and they should get the same result, either when logged in as user #1 or as another admin user.

I also isolated the root-admin login in it's own function, as that did gave a poblem when using drupalUserLogin() with user #1

If you have a better Idea, please suggest.

reroll against HEAD, improved tests. full review please?

included testcase: 48 passes, 0 fails, and 0 exceptions

Patch still applies cleanly, code looks good to me, bot is happy and it's a nice feature, I don't see a reason why this shouldn't go in.

The patch has a number of code style issues, including comments that don't end in periods.

Please replace "@note" with "Please note that", append a dot to the sentence, and a blank PHPDoc line after it.

Missing trailing dot.

This message sounds strange and uses terms we do not use in Drupal. Replace with something like "The first user cannot be deleted."

As this is a inline comment, it should be technically correct. Technically correct is that uid 1 *can* be deleted, but *should not* be deleted.

Double space in here.

See above, replace with "uid 1" or "first user".

Missing trailing punctuation.

First letter should capitalized.

Also, please replace all abbreviations like "don't" with "do not".

Maybe the checkbox to delete accounts from the admin section could also be removed for uid 1?

@#81,
I'm going with mfer's comment:

Why is the checkbox for user 1 removed from the form in user_admin_account? I understand stopping the delete operation. But, with this for other operations are permitted and can be extended beyond the core provided ones. The checkbox should remain for those.

@sun,
I like you :D (no really, I like good scrutinizing!)
I will continue to work on this after christmas :D

hmm... I see. Then, maybe it could be added a validation step, otherwise there would be an error but some users deleted and some other don't, because the operation will fail when try to user_delete(1).

In an attempt to get this friggin' thing in, so I'll never have to look at my DB again...

Attached patch addresses #79 and #80 and a couple other similar bugs in the comments.

I don't know, what still has to be done codewise, but it looks good to me (which means absolutely nothing, but hey...)

@markus_petrux in #84: I disagree! The purpose of this patch is to prevent people from deleting uid 1. Period. Possible edge-case usability problems are probably valid, but belong in another issue once this is in.

PS: I'm a Windows user, so I can only hope I didn't break anything line-ending wise.

doh, this patch doesn't apply at all anymore, thanks to the node/8 (Let users cancel their accounts) code that went in.

user_delete() is no more, now its user_cancel(), _user_cancel(), and user_cancel_access()

the tests need to be rewritten as well. There's no more "Delete" button or action or operation, its all called cancel now.

Note that I already had two small bits in the patch for 8, which would have incorporated this issue. I had to left them out to not fall in scope creep. Implementing this is a no-brainer now.

So, Sun, are you going to post those tidbits as a patch for this issue? If so, great, I'll wait. Otherwise, I'll forge ahead with a re-roll.

Better title.

Senpai takes it on from here :)

Sorry, erm, really sorry - wrong logic in both changes.

Yeah, I was kinda wondering about that when I read the #89 patch. :) I'll write a test, and then perform some RTBC patch testing on this issue just as soon as I can get #375931: Simpletests are broken when cvs_deploy is enabled figured out.

Resubmitted last patch for testing. Any updates?

I rerolled the original patch to account for fuzz & offset, then added a Simpletest that checks to see if uid1 can delete their own account. It runs successfully. Since there's already a test that checks to see if the user attempting an account deletion has the proper perms to accomplish such a task, I feel that this extra little test will serve to protect the uid1 account in the future, even from it's owner.

Patch Testing Instructions
To test this patch, apply it and then visit the user/1/edit page with any logged in account. Whether you're user #1 or #42, you should never see a "Cancel Account" button at the bottom of the user/1/edit page.
Then, go to the admin/user/user page and select the checkbox for user #1 and attempt to cancel that account. You should be prevented from doing so, and receive a warning message.
If you successfully complete these two steps, the patch will be ready to go.

Woops! I forgot that I was supposed to add a chunkett of code from a way earlier patch to sun's patch in order to remove that pesky uid1 checkbox from the admin/user/user page. Here's try number 2.

Patch Testing Instructions
To test this patch, apply it and then visit the user/1/edit page with any logged in account. Whether you're user #1 or #42, you should never see a "Cancel Account" button at the bottom of the user/1/edit page.
Then, go to the admin/user/user page and be sure there's no checkbox for user #1 and no way to attempt to cancel that account.
If you successfully complete these two steps, the patch will be ready to go.

The last submitted patch failed testing.

Hmm, I don't get it. The patch in #93 passes, but when I add one array_pop() + a line-line test for same, it fails? I can't see what the problem is here. Someone else wanna give this a set of eyes?

array_pop *returns* the altered array.

Yup, it sure does, and that's what I'm after here. I want to remove the first element of the array, and then return the remaining results so that $form '#options' => $accounts can make use of it. Here's what the admin/user/user page would look like after patch #94:

It works perfectly too. Any other user can be blocked, unblocked, or canceled except the uid1 account. No, macgirvin, what I can't figure out is why the Simpletests are now failing, whereas they didn't before?

[Update] There are only 10 small tests that are failing now, and the first of them is the UserCancelTestCase->testMassUserCancelByAdmin() function, on line 489 of user.test. The failure is a "Failed to set field accounts[3] to 1". Here's the relevant section of the test:

Now, how would I rewrite this so that it didn't try and cancel 100% of the user accounts on a Drupal site?

Figured it out. There was some left over Simpletest code that was attempting to cancel the admin_user while the admin_user was canceling normal user accounts. Ungood. That feature needs to be written into it's own test so that nobody can mistakenly delete their own account.

I've also added an if (array_key_exists) around the array_pop() for good measure down the road. No telling when something might change and we'd be popping the wrong user account out of that array.

Side note: I noticed that the classes UserAdminTestCase and UserCancelTestCase are remarkably similar. Maybe combine them in another issue?

The last submitted patch failed testing.

I think this patch has gotten a little off-track.

• There's no need to do anything with the admin/user/user page. Those checkboxes allow batch user operations to be applied, and its quite possible to want to do something with uid1 that's not a delete
• in user_multiple_cancel_confirm_submit() we prevent uid1 from being deleted, but why not inform the user of this fact when presenting this form in the first place (user_multiple_cancel_confirm())? This is a good opportunity to either remove uid1 from the list of accounts to be deleted, or at least set a message saying it will not be deleted, and perhaps explain the proper way to do that.
• There is still some way to delete uid1 if you really really want to, right?
• There's no checking for uid1 in user_cancel(), _user_cancel(), or user_cancel_access() - these are the functions that actually do the work. It would be a lot safer to add additional checks here. Current checks at the form level are good for UI, but it's too easy for a module to re-expose some way to delete uid1.

just my $0.02, looking forward to more comments!

I agree with Karl on uid1's checkbox needing to be on the admin page for use by other batch operations, so i'll remove the array_pop. it sucked anyway due to the way that array is being built randomly.

As for the user_multiple_cancel_confirm dialog, I think that's a great idea and will implement it forthwith.

There's still some way to delete uid1, right? No, there isn't. Core should not provide any means of deleing the principle user and crippling a site. On the other hand, we're not putting these safety checks into the worker functions themselves because, as is the manner of contrib, if someone wants to write a module that deletes uid1 and replaces it with a covey of little green dragons, they should be able to. We're just not going to allow core to harm itself accidentally.

@Senpai - I was going to argue that there should be a way to delete uid1, just no accidentally. But honestly, I can't think of a single time I've ever wanted to do that. So I'm fine with removing that ability.

gah, double post

I second that. People that actually have a use-case of deleting uid1 will run a run a rather professional site and, hence, also have the know-how to jump in their DB and just do it there. Therefore, to my mind, "keeping Drupal flexible for developers", shouldn't be an argument against this approach.

Just bumping this - it's really close to going in, so let's give it the final push!

Here's a slightly different take on things that doesn't remove the uid1's checkbox, but it still protects the uid1 account from being canceled during a bulk delete operation (the rest of the ways to delete uid1 have been covered earlier in this issue and are still included in this patch).

I went this route at kscheirer's suggestion that some third-party modules might want to perform bulk operations on the entire userbase. Ok, that's cool by me. So we'll just prevent Drupal from submitting a request for cancellation of uid1, even if the admin had selected uid1 along with some other users during the cancellation operation. Check this video out:

http://blip.tv/file/2328552/

If this is acceptable behavior, we can go even one step further and use JS to disable uid1's checkbox should the administrator choose 'Cancel the selected user accounts' from the dropdown menu. Somebody please tell me you like this approach before I code the additional jQuery (and fix the weirded Simpletests)?

Karma +1 for Senpai for showing off with video.

also, the jQuery probaly shoudn't be part of this patch, let's try to keep the kittens alive ;)

No obvious complaints from this side, but like to see one more reviewer to review this before turning this sucker RTBC.

Ok, that's one thumbs up from Alexander, and one more from the AutoBot. I'll fix the Simpletests so they actually do something useful and then re-roll.

I don't want to hijack this thread so perhaps any responses to this question and how these two issues might conflict or benefit from a little cooperation if seen as mutually beneficial might best be handled in this issue:

#497500: Create an administrator account in the default install profile, separate from user 1

I would like to ditch all of the special casing surrounding the arbitrary "user 1" now that we have an "administrative user" role looking to be provided by default. We can then treat all "administrative users" with the role equally. Under such a scenario we could prevent the last user with this role form being deleted/disabled in the same way we are doing above. Here are two options I see:

1. Administrative user role is provided by default on install. It is hardcoded to pass all user_access() checks just like user 1 was and the checkboxes in permissions are disabled. The name can be changed but it cannot be deleted.

2. Administrative user role is provided by default on install. Its permissions, set by the permissions page are respected like normal roles excepting the "administer permissions" one which is disabled. New permissions are automatically enabled as they are now. The name can be changed but it cannot be deleted.

This solves the "roles provide permissions not users, except for user 1" confusion many new users have and allows multiple administrators which makes the site more secure because passwords dont have to be shared, etc. I think of it as a SUDOERS role. It also gets rid fo the special casing surrounding the otherwise arbitrarily important user 1.

All followups to Michael's comment #110 above should take place in that issue, not this one. #497500: Create an administrator account in the default install profile, separate from user 1

I cannot finish the Simpletests for this patch until #515040: Simpletest throws exception on a fresh install gets fixed. The underlying patch from #107, however, works fine and is only being held up from RTBC because of a really quirky and as yet unsolvable core bug that affects Simpletest, and thus this patch (if in fact this patch even needs a Simpletest?).

The last submitted patch failed testing.

Tagging.

Rerolling to keep up with head.

This patch incorporates all the previous changes, adds a working test that should prevent uid1 from being able to delete uid1, and re-factors some minor stuff in user.test's UserCancelTestCase. Lets see if the AutoBot likes it.

I did a fresh update of Drupal HEAD and and new database install. Every time I try and run this test, the batch gets aborted, and I'm kicked back to the front page with "No active batch."

Any idea how to resolve that?

Also, the comment    * This should never be possible, for obvious reasons. - the reasons may be obvious to contributors of this thread, but either remove the comment or give the reasons :)

in the if ($edit['accounts'] && array_key_exists(1, $edit['accounts'])) { ... } block I think the warning to the user is unclear. 'You attempted to cancel the Fred account which might have broken your site; that part of the request has been disallowed.' I think it's clearer to indicate why Fred's account is so important: it is user id 1.

hmm... do we have (handbook) documentation about user #1???

In that case, we should refer to it, if not, is should be created in another issue ;)

The last submitted patch failed testing.

I didn't read all the comments here but apparently there is still the possibility that a user with the "administer user" permissions granted can DELETE the UID1 and also EDIT the UID1 which is more or less the same.. This user with granted permissions can either damage or take over the site.

In how many cases do you really know who is in charge of administering users? And in how many cases do you as a developer need to judge on who is allowed to be registered and who's not? Most possibly you don't deal with that task but the administrator of the community (maybe moderator) is in charge of this. I mean, as a web developer you know how to deal with it but in community sites people are swapping positions a lot. So the one who was in charge of doing something leaves a community (moves, gets married or has babies.. whatever reason) and passes on the key to the next one.. you don't have control over that. So in order to protect your hard work of setting up this CMS for a specific community, customer, whatever I'd say it IS a necessity to allow for a certain level of UID1 protection in core.

Anyway, if it doesn't get implemented, then each developer has to find his own best practice of protection for this through modification of the system, patches or the use of certain modules.

As of today I'm using the userprotect module in D6.13 which is pretty lightweight and solves this issue. I tried markus petrux's protect_critical_users but that still allowed to edit the UID1 which means they can still take over the steering wheel of the website! That's fine if they are competent enough to do that but in many cases you want to stay responsible for the site unless you specifically agree that you won't be anymore.

And how much code would it take to get it in core?

Rerolling to keep up with HEAD. Also revised some comments and changed the output of the "uid1 deletion attempt" text.

The last submitted patch failed testing.

Someone renamed admin/user/user to admin/people. Rerolling accordingly.

The last submitted patch failed testing.

Here's an updated patch that should pass testing.

no need to remove spaces here, otherwise looking nice.

This review is powered by Dreditor.

@alexanderpas in #125: I don't see the line you're talking about with removed spaces. I did however find a drupalLogout() that was left over from earlier troubleshooting, so here's a reroll that still passes all tests. No functional code was changed since patch #124

It looks like this patch changes no APIs, so can be revisited post-code-freeze. I'm in favour, obviously. ;P I think Dries may want to have a final yes/no though.

Also, although this is really stupid behaviour, it's not a critical bug.

webchick: read your own comment from about two years ago (#24)

It is not only stupid behaviour, it is behaviour effectively breaks your ability to update (or even access your website in the worst case), without the ability to undo it without going in the database.

extreme case:
Install drupal => delete admin user => your drupal installation is now the digital version of a brick.

Webchick, the reason this should be committed today, rather than later, is that there's a "select all" checkbox at the top of the admin/people form, and a way to cancel all accounts with a single button push. If someone gets it into their heads that they need to erase all their users, or if they mistakenly choose this, or whatever, it kills the site, and you and I have to spend billable hours helping them fix it.

On second thought, don't commit this one. Ever. I think I'm starting a new Drupal Help service... ;)

Oh, and one more thing. This patch contains tests for code that has already been committed without it's tests, and probably shouldn't have been. Can we get Dries to look it over if necessary and then get it committed? Please?

Senpai, to put things in perspective, here are examples of the kind of massive patches that Dries and I are juggling which *have* to be resolved before code freeze:

#516138: Move CCK HEAD in core
#544418: Integrate CTools AJAX framework with Drupal to extend (and replace) existing ahah framework
#282122: D7UX: "Save draft" and "Publish" buttons on node forms
#113614: Add centralized token/placeholder substitution to core
#367595: Translatable fields
#497118: Remove the registry (for functions)
#517688: Initial D7UX admin overlay
#473268: D7UX: Put edit links on everything

So, to put it bluntly, no. :P I am not going to hassle Dries to look at this. I've no argument that it's a totally dumb behaviour, but it's also been like that forever and there's no reason it can't wait until other more pressing issues are fixed first.

The last submitted patch failed testing.

I agree, this needs to be fixed before Drupal 7 is released. This gives us a few more months, the same amount of time before our support phones will start to ring. ;-)

As the author of node/8 I absolutely concur that this is mission critical.

The patch needs to be re-rolled and also needs some clean-ups though.

Sorry. Something went completely wrong here. I'm not sure what exactly has been discussed since comment #92, but the most recent patch that was marked as RTBC contained a lot of test-shuffling-around for no good reason.

Attached patch contains the reverse-engineered essentials of the last patch.

Also has a pretty compelling filename that will hopefully make Dries to love this patch more. 8)

The last submitted patch failed testing.

This patch is nearly 4 years old!!!!!!!!!!!!!!!!!!!!!!!!!!!! LOL,

You all have a crazy kind of patience to work like this........keep it up!!

- Y.

@fizk Thanks for that extremely helpful comment!!!!!!!!!!!!!!!!!!!!!!!!!!! Not all of us knew that when someone points out how old an issue is, it automatically gets it fixed! Now we do! None of this stupid 'do-ocracy' crap that the entire Drupal community is based around. It's not like there's an existing module to help all those poor helpless souls that aren't using Drupal 13 yet since this will NEVAR get fixed!!!!!!!!!!!1

</sarcasm></LOL>

Re-rolled against HEAD. Updated and fixed the tests. Looks ready to fly for me. Let's see what the bot thinks.

It looks like uid 1 can still cancel its own account?

needs something like:

(pardon the wierd linebreaks. It was my attempt to make the logic easier to read.

Thanks, fixed in here.

It works as expected, I am unable to cancel the account (when trying to cancel one account or when trying to cancel more than one) and I do not see the "Cancel" button when editing the uid1.

One thing I noted, it is not much informative when I try to do it through the users list, maybe we could set a flash message right before the "continue;" ? And since this action requires a confirmation, we could even do it on the confirmation page and remove the uid1 from the list of selected accounts to "cancel" (and if there are none left, redirect the user to the users list after setting a flash message telling what happened). I think that is pretty much what the video in #107 shows, or maybe this is just not possible as code freeze is past us ... in that case we can just prevent it and set a flash message before the "continue;".

Also, even with this patch, it is possible to block the uid1 account (select uid1 in the list, and choose "Block the selected users", although it doesn't seem to have any effect except to show it in the list of users as "blocked". Maybe we could also prevent this and also set a message when the uid1 tries to block himself ?

Hello, I was wondering if this will be for Drupal 6 as well?
I just tried logging in as admin (uid1) today and it didn't work.
Checked the database and uid1 was deleted..I don't know what happened.

Core feature additions are not backported. However, the userprotect module is available for Drupal 6 and offers similar functionality, plus additional configuration.

Added a message (warning/error depending on amount of selected users).

Note that the typo in "canceled" is already fixed in this patch.

Everything works as expected. Great job sun, it's more "user-friendly" now :)

Regarding my previous comment. It is still possible to "block" the admin user, but it has not effect. For me it is not important, but if anyone feels that is it really wrong (to be able to do something that has no effect, at least as far as I can see), they should create another issue only for this I think (something like "Prevent blocking the uid1 account... which has no effect anyways" ;)).

@TheRec: Do you think we can turn this issue to RTBC? As you said, your other concern could be discussed in another issue once that deletion of user 1 will be fixed.

xmacinfo: When at least a second contributor (mayne you did it already ?) will have reviewed and tested it (as required by the review process), it will be possible to mark it RTBC ;)

Took some time to read and test the patch. IMO, it solves the problem nicely. RTBC.

I would like to answer the initial question:
Why allow deleting user 1?
for the same reason why disabling root login on linux servers is a good thing.. As I know in D7 you can do everything without user 1, so deleting user 1 should not be a problem..

you still can delete user 1 by removing it from the database, thereby proving you have enough access to put it back...

similar, you can remove a normal user from ubuntu with the gui, however, you can't remove root using the same gui.

Re-test of drupal.user-cancel-uid1.146.patch from comment #146 was requested by webchick.

Seems like if we want to make this tight, http://api.drupal.org/api/function/user_cancel/7 shouls also disallow cancellation of uid 1. Also, why is that function calling drupal_set_mssage?! How about throwing an exception?

The agreed on compromise here was to only remove the possibility from the UI (forms), not from API functions. For any reason, some (or one?) people would like to retain the ability to even kill uid 1 programmatically. Hence, this patch only disallows canceling uid 1 from the user account and user mass-operations forms. That's fine for me and we can revisit whether to harden this protection in D8.

Those issues you raised about user_cancel() belong into a separate issue. :)

Re-test of drupal.user-cancel-uid1.146.patch from comment #146 was requested by pwolanin.

making sure tests still pass.

kind of seems like this bit should be added to a validation handler instead of the form builder? The code right before it prevents $accounts[1] from being set in the form builder.

Thanks for the in-depth reviews, pwolanin! This additional check + redirect + message was added to the form builder already, because after filtering away uid 1, there can be zero accounts left, so the confirmation form would be senseless.

The last submitted patch, drupal.user-cancel-uid1.146.patch, failed testing.

Re-test of drupal.user-cancel-uid1.146.patch from comment #146 was requested by sun.

Wow! I'll be so happy to see this finally get committed!

@Pasqualle: "Why allow deleting user 1?" It's also because if you install a Drupal site, and can then delete the only user account accidentally via the site's admin control panel and therefore prevent anyone from over logging in again, it looks to the rest of the world like "Those stupid Drupal programmers can't code. Who would market a hand grenade with a pin that could fall out accidentally?"

That's why. :)

Alright, alright. ;-) Committed to CVS HEAD.

<happyface />

Automatically closed -- issue fixed for 2 weeks with no activity.