I was about to report this issue, but first tried to search, because I couldn't believe it was possible to delete UID 1. If it is, there should be a reason, isn't it? Well, I've found this issue, which had several fixes, including a check to prevent from deleting UID 1, but it I'm surprised it was removed.
Reading that thread, Dries said:
Also, don't introduce special cases and remove:
+ // Don't allow anyone to edit uid = 1 unless logged in as uid = 1 + if (!($account->uid == 1 && $user->uid != 1)) { + $form['submit'] = array('#type' => 'submit', '#value' => t('Submit'), '#weight' => 30); + } + // Don't allow the deletion of uid = 1 + if (user_access('administer users') && $account->uid != 1) {
Next, m3avrck posted:
Now what is so wrong about the special case of uid=1? That is always a special case in core and it seems to me that, that account shouldn't be able to be deleted and if someone needs to delete it, then I'm sure they are more than capable enough to do that in the database. Right?
and merlinofchaos wrote:
I disagree about not special-casing uid 1. That account should not be deletable. The recovery of that error is painful, and someone asleep at the wheel can do it.
If the system completely removed the dependency on uid 1 I'd be more agreeable, but since uid 1 is already special-cased, I don't see why we can arbitrarily say not to special-case it here.
However, as far as I can see, there was no answer to their comments. It seems everyone was mostly focussed on fixing the rest of the issues related to user deletion, but in my opinion that question remains.
Why allow deleting user 1?
If it should be possible, then maybe adding some kind of additional check (maybe requiring the password, do not allow a user to remove himself, etc.) would be wiser.
Comment | File | Size | Author |
---|---|---|---|
#146 | cancel-uid1-multiple.png | 3.75 KB | sun |
#146 | cancel-uid1-only.png | 3.93 KB | sun |
#146 | drupal.user-cancel-uid1.146.patch | 6.01 KB | sun |
#142 | drupal.user-cancel-uid1.142.patch | 4.4 KB | sun |
#139 | drupal.user-cancel-uid1.139.patch | 4.4 KB | sun |
Comments
Comment #1
markus_petrux CreditAttribution: markus_petrux commentedIs it a feature request ... or a bug? Because UID 1 is critical enough to be deleted by mistake, which is something stupid, but possible.
Comment #2
markus_petrux CreditAttribution: markus_petrux commentedRelated issue: http://drupal.org/node/48147
Comment #3
Steve Dondley CreditAttribution: Steve Dondley commentedI 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.
Comment #4
dlr CreditAttribution: dlr commentedAnother problem comes with user managment ('administer users') delegation, each user with this permission can delete UID 1.
It's a bit dangerous...
Comment #5
Crell CreditAttribution: Crell commentedWhich 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.
Comment #6
markus_petrux CreditAttribution: markus_petrux commentedWhy don't we launch a poll to see what Drupal users prefer?
:insert rolling eyes smilie here:
Comment #7
hunmonk CreditAttribution: hunmonk commentedguess 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.
Comment #8
hunmonk CreditAttribution: hunmonk commentedoops. THIS patch nicely solves the problem...
Comment #9
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedI don't really get why people see this as a problem. If people want to shoot themselves into the foot, let them.
Comment #10
chx CreditAttribution: chx commentedit 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.
Comment #11
chx CreditAttribution: chx commentedkilles: 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.
Comment #12
Crell CreditAttribution: Crell commentedI'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.
Comment #13
Dries CreditAttribution: Dries commentedIMO, this isn't a problem. In all the years, this hasn't been a real problem.
Comment #14
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedThat's that.
Comment #15
markus_petrux CreditAttribution: markus_petrux commentedhttp://drupal.org/node/49101
Comment #16
bradlis7 CreditAttribution: bradlis7 commentedHere'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.
Comment #17
buddaHaving 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?
Comment #18
markus_petrux CreditAttribution: markus_petrux commentedbudda, 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.
Comment #19
Aurian Noreinor CreditAttribution: Aurian Noreinor commentedDoes anyone have the sql to insert a the user 1 ?
Comment #20
ChrisKennedy CreditAttribution: ChrisKennedy commentedThis 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.
Comment #21
Crell CreditAttribution: Crell commentedI'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.
Comment #22
hunmonk CreditAttribution: hunmonk commentedthis 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...
Comment #23
webernet CreditAttribution: webernet commentedRelated to: http://drupal.org/node/75289
Comment #24
webchickLost 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?
Comment #25
hunmonk CreditAttribution: hunmonk commented@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... :)
Comment #26
joep.hendrix CreditAttribution: joep.hendrix commentedI 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
Comment #27
ChrisKennedy CreditAttribution: ChrisKennedy commentedHere is an updated version for d6 that also protects the user admin page.
Comment #28
Dries CreditAttribution: Dries commentedI 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.
Comment #29
Crell CreditAttribution: Crell commentedMarked http://drupal.org/node/153118 as a duplicate. FAPI 3 has long since landed. What's the status of this issue?
Comment #30
Gábor HojtsyLooked 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.
Comment #31
dmitrig01 CreditAttribution: dmitrig01 commentedLet's get this to FAPI 3 before going any farther
Comment #32
catchComment #33
ewlloyd CreditAttribution: ewlloyd commentedI'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.
Comment #34
xiv CreditAttribution: xiv commentedIt 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.
Comment #35
sun+1 -- I almost accidentally deleted user 1 due to the re-arranged form buttons throughout core.
Comment #36
sunUpdated hunmonk's patch, merged with ChrisKennedy's changes.
Comment #37
hass CreditAttribution: hass commentedSubscribing 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.
Comment #38
Freso CreditAttribution: Freso commentedWhy did you set it back to 6? It needs to go into 7.x before going into 6.x.
Comment #39
macgirvin CreditAttribution: macgirvin commentedsubscribe
Comment #40
sdecabooter CreditAttribution: sdecabooter commentedI 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?
Comment #41
cridenour CreditAttribution: cridenour commented@xiv
I'm going to have to agree - to an extent. Though that should be discussed in a separate issue.
Subscribing.
Comment #42
sunI'm 99.99% sure this patch needs a re-roll. And by speaking of D7, most probably a test, too.
Comment #43
Dave ReidWould it be any easier to just change user_delete()?
Comment #44
ScoutBaker CreditAttribution: ScoutBaker commentedI'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.
Comment #45
Dave Reid#43 could also be edited to not allow a user to delete their own account:
Comment #46
sunNo, 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.
Comment #47
markus_petrux CreditAttribution: markus_petrux commentedWhat 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 douser_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.
Comment #48
sunFolks, 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.
Comment #49
markus_petrux CreditAttribution: markus_petrux commentedI'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.
Comment #50
webchickSince 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.
Comment #51
sun1 remaining question: Does this require a test? If so, what would we want to test?
Comment #52
webchickYeah, 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, ..?).
Comment #53
sunI'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?
Comment #54
Senpai CreditAttribution: Senpai commentedThe 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.
Comment #55
markus_petrux CreditAttribution: markus_petrux commentedSnippet for Drupal 6.
Comment #57
markus_petrux CreditAttribution: markus_petrux commented#55 implemented as an addon module: http://drupal.org/project/protect_critical_users
Hope that helps until this is properly addressed in core.
Comment #58
meba CreditAttribution: meba commented#54 isn't safe because you can still use an url...
Comment #59
Senpai CreditAttribution: Senpai commentedSafe? 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?
Comment #60
alexanderpas CreditAttribution: alexanderpas commentedtry this patch, it even includes a test.
Comment #61
mfer CreditAttribution: mfer commentedI'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:
Comment #62
alexanderpas CreditAttribution: alexanderpas commentedfixed and re-rolled against head ;)
Comment #63
mfer CreditAttribution: mfer commentedDid 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.
Comment #64
mfer CreditAttribution: mfer commentedComment #65
babbage CreditAttribution: babbage commentedsubscribing
Comment #66
alexanderpas CreditAttribution: alexanderpas commentedintergrated comments, but adapted testcase differently.
Comment #68
alexanderpas CreditAttribution: alexanderpas commentedat least the bot works now...
why do i always miss the most glaring errors...
thanks Heine!
Comment #69
alexanderpas CreditAttribution: alexanderpas commentedand i always forget... something...
Comment #71
alexanderpas CreditAttribution: alexanderpas commented48 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).
Comment #72
markus_petrux CreditAttribution: markus_petrux commentedNot sure in D7, but in D6 it is possible to do user/0/delete too. It was not possible in D5.
Comment #73
alexanderpas CreditAttribution: alexanderpas commentedsimilar issue, I may fix that in a followup issue, let's get this one in core first.
Comment #74
mfer CreditAttribution: mfer commentedIs 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.
Comment #75
alexanderpas CreditAttribution: alexanderpas commentedI 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.
Comment #76
alexanderpas CreditAttribution: alexanderpas commentedreroll against HEAD, improved tests. full review please?
included testcase: 48 passes, 0 fails, and 0 exceptions
Comment #77
swentel CreditAttribution: swentel commentedPatch 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.
Comment #78
keith.smith CreditAttribution: keith.smith commentedThe patch has a number of code style issues, including comments that don't end in periods.
Comment #79
sunPlease 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.
Comment #80
sunAlso, please replace all abbreviations like "don't" with "do not".
Comment #81
markus_petrux CreditAttribution: markus_petrux commentedMaybe the checkbox to delete accounts from the admin section could also be removed for uid 1?
Comment #82
alexanderpas CreditAttribution: alexanderpas commented@#81,
I'm going with mfer's comment:
@sun,
I like you :D (no really, I like good scrutinizing!)
I will continue to work on this after christmas :D
Comment #83
markus_petrux CreditAttribution: markus_petrux commentedhmm... 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).
Comment #84
alexanderpas CreditAttribution: alexanderpas commentedComment #85
tstoecklerIn 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.
Comment #86
kscheirerdoh, 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.
Comment #87
sunNote 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.
Comment #88
Senpai CreditAttribution: Senpai commentedSo, 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.
Comment #89
sunBetter title.
Senpai takes it on from here :)
Comment #90
sunSorry, erm, really sorry - wrong logic in both changes.
Comment #91
Senpai CreditAttribution: Senpai commentedYeah, 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.
Comment #92
sunResubmitted last patch for testing. Any updates?
Comment #93
Senpai CreditAttribution: Senpai commentedI 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.
Comment #94
Senpai CreditAttribution: Senpai commentedWoops! 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.
Comment #96
Senpai CreditAttribution: Senpai commentedHmm, 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?
Comment #97
macgirvin CreditAttribution: macgirvin commentedarray_pop *returns* the altered array.
Comment #98
Senpai CreditAttribution: Senpai commentedYup, 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:// Cancel user accounts, including own one.
$edit = array();
$edit['operation'] = 'cancel';
foreach ($users as $uid => $account) {
$edit['accounts[' . $uid . ']'] = TRUE;
}
$edit['accounts[' . $admin_user->uid . ']'] = TRUE;
$this->drupalPost('admin/user/user', $edit, t('Update'));
$this->assertText(t('Are you sure you want to cancel these user accounts?'), t('Confirmation form to cancel accounts displayed.'));
$this->assertText(t('When cancelling these accounts'), t('Allows to select account cancellation method.'));
$this->assertText(t('Require e-mail confirmation to cancel account.'), t('Allows to send confirmation mail.'));
$this->assertText(t('Notify user when account is canceled.'), t('Allows to send notification mail.'));
Now, how would I rewrite this so that it didn't try and cancel 100% of the user accounts on a Drupal site?
Comment #99
Senpai CreditAttribution: Senpai commentedFigured 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?
Comment #101
kscheirerI think this patch has gotten a little off-track.
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.just my $0.02, looking forward to more comments!
Comment #102
Senpai CreditAttribution: Senpai commentedI 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.
Comment #103
kscheirer@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.
Comment #104
kscheirergah, double post
Comment #105
tstoecklerI 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.
Comment #106
brianV CreditAttribution: brianV commentedJust bumping this - it's really close to going in, so let's give it the final push!
Comment #107
Senpai CreditAttribution: Senpai commentedHere'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)?
Comment #108
alexanderpas CreditAttribution: alexanderpas commentedKarma +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.
Comment #109
Senpai CreditAttribution: Senpai commentedOk, 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.
Comment #110
michaelfavia CreditAttribution: michaelfavia commentedI 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.
Comment #111
Senpai CreditAttribution: Senpai commentedAll 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?).
Comment #113
amc CreditAttribution: amc commentedTagging.
Comment #114
Senpai CreditAttribution: Senpai commentedRerolling 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.
Comment #115
kscheirerI 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.Comment #116
alexanderpas CreditAttribution: alexanderpas commentedhmm... 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 ;)
Comment #118
BassPlaya CreditAttribution: BassPlaya commentedI 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?
Comment #119
hass CreditAttribution: hass commentedComment #120
Senpai CreditAttribution: Senpai commentedRerolling to keep up with HEAD. Also revised some comments and changed the output of the "uid1 deletion attempt" text.
Comment #122
Senpai CreditAttribution: Senpai commentedSomeone renamed admin/user/user to admin/people. Rerolling accordingly.
Comment #124
cwgordon7 CreditAttribution: cwgordon7 commentedHere's an updated patch that should pass testing.
Comment #125
alexanderpas CreditAttribution: alexanderpas commentedno need to remove spaces here, otherwise looking nice.
This review is powered by Dreditor.
Comment #126
Senpai CreditAttribution: Senpai commented@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
Comment #127
webchickIt 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.
Comment #128
webchickAlso, although this is really stupid behaviour, it's not a critical bug.
Comment #129
alexanderpas CreditAttribution: alexanderpas commentedwebchick: 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.
Comment #130
Senpai CreditAttribution: Senpai commentedWebchick, 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?
Comment #131
webchickSenpai, 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.
Comment #133
xmacinfoI 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. ;-)
Comment #134
sunAs 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.
Comment #135
sunSorry. 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)
Comment #137
fizk CreditAttribution: fizk commentedThis patch is nearly 4 years old!!!!!!!!!!!!!!!!!!!!!!!!!!!! LOL,
You all have a crazy kind of patience to work like this........keep it up!!
- Y.
Comment #138
Dave Reid@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>
Comment #139
sunRe-rolled against HEAD. Updated and fixed the tests. Looks ready to fly for me. Let's see what the bot thinks.
Comment #140
markus_petrux CreditAttribution: markus_petrux commentedIt looks like uid 1 can still cancel its own account?
Comment #141
brianV CreditAttribution: brianV commentedneeds something like:
(pardon the wierd linebreaks. It was my attempt to make the logic easier to read.
Comment #142
sunThanks, fixed in here.
Comment #143
TheRec CreditAttribution: TheRec commentedIt 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 ?
Comment #144
Bilmar CreditAttribution: Bilmar commentedHello, 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.
Comment #145
Crell CreditAttribution: Crell commentedCore feature additions are not backported. However, the userprotect module is available for Drupal 6 and offers similar functionality, plus additional configuration.
Comment #146
sunAdded a message (warning/error depending on amount of selected users).
Note that the typo in "canceled" is already fixed in this patch.
Comment #147
TheRec CreditAttribution: TheRec commentedEverything 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" ;)).
Comment #148
xmacinfo@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.
Comment #149
TheRec CreditAttribution: TheRec commentedxmacinfo: 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 ;)
Comment #150
brianV CreditAttribution: brianV commentedTook some time to read and test the patch. IMO, it solves the problem nicely. RTBC.
Comment #151
PasqualleI 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..
Comment #152
alexanderpas CreditAttribution: alexanderpas commentedyou 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.
Comment #154
sunComment #155
pwolanin CreditAttribution: pwolanin commentedSeems 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?
Comment #156
sunThe 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. :)
Comment #158
pwolanin CreditAttribution: pwolanin commentedmaking 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.
Comment #159
sunThanks 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.
Comment #162
sunComment #163
Senpai CreditAttribution: Senpai commentedWow! 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 ever 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. :)
Comment #164
Dries CreditAttribution: Dries commentedAlright, alright. ;-) Committed to CVS HEAD.
Comment #165
silkscreen CreditAttribution: silkscreen commentedComment #166
Senpai CreditAttribution: Senpai commented<happyface />
Comment #168
lostchord CreditAttribution: lostchord commented@#163
And at 2 in the morning having been on your feet for 18+ hours it is _very_ easy to make such mistakes. Robust systems MUST mitigate the risk of accidental disruption of service. The "dead on his/her feet" administrator use case has to be catered for.
Comment #169
bryancasler CreditAttribution: bryancasler commentedIs anything like this in 6.16? Also, what about user 0?
Comment #170
ajaysolutions CreditAttribution: ajaysolutions commentedI'd like to add to this, that "lesser" admin users can also block user 1 (whereas this account should always be enabled), plus they have the option to change that accounts password too, which could be used to block that account much the same.
Comment #171
Senpai CreditAttribution: Senpai commented@ajaysolutions, It's not always true that uid1 should be enabled. We have several corporate clients who's security policies insist that any super-user account be inaccessible from the web, and so we had to block uid1 on the production servers.
As to the ability of a lesser admin blocking account 1 or changing it's password, if you've given them the ability to "Administer users", then that's what they get to do. :) "You must trust in people, or life becomes impossible."
@animelion: Seen the userprotect module?
Comment #172
pratikk CreditAttribution: pratikk commentedIn response to Killes, sometimes other people shoot you in the foot. My site got hacked and the first thing they did was to delete all users, including UID1. A month later, after recreating users, I'm still trying to re-attribue pages and posts to the right users. Everything defaulted to UID0.
Comment #173
hass CreditAttribution: hass commentedThis is why we make backups.
Comment #174
xmacinfoBackups don’t solve every issues. This still needs to be fixed.
Comment #175
hass CreditAttribution: hass commentedBut not for #172