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.

CommentFileSizeAuthor
#146 cancel-uid1-multiple.png3.75 KBsun
#146 cancel-uid1-only.png3.93 KBsun
#146 drupal.user-cancel-uid1.146.patch6.01 KBsun
#142 drupal.user-cancel-uid1.142.patch4.4 KBsun
#139 drupal.user-cancel-uid1.139.patch4.4 KBsun
#135 i-will-cancel-uid1-on-drupal-org-after-upgrade-to-d7.patch4.39 KBsun
#126 46149-126_prevent_cancel_uid1.patch20.25 KBSenpai
#124 46149-124_prevent_cancel_uid1.patch21.6 KBcwgordon7
#122 46149-122_prevent_cancel_uid1.patch20.71 KBSenpai
#120 46149-120_prevent_cancel_uid1.patch19.85 KBSenpai
#114 46149-114_prevent_cancel_uid1.patch19.91 KBSenpai
#107 46149_prevent_cancel_uid1_2.patch12.95 KBSenpai
#99 46149_prevent_cancel_uid1_1.patch8.74 KBSenpai
#94 46149_prevent_cancel_uid1_0.patch5 KBSenpai
#93 46149_prevent_cancel_uid1.patch3.93 KBSenpai
#90 drupal.cancel-uid1.patch1.7 KBsun
#89 drupal.cancel-1.patch1.79 KBsun
#85 one_patch_zero_fails_0.patch6.06 KBtstoeckler
#76 one_patch_zero_fails.patch6.27 KBalexanderpas
#71 one_patch_zero_fails.patch5.39 KBalexanderpas
#68 one_for_the_bot.patch4.77 KBalexanderpas
#66 one_for_the_show.patch4.82 KBalexanderpas
#62 one_patch_to_rule_them_all.patch4.41 KBalexanderpas
#60 Where_is_your_one.patch4.44 KBalexanderpas
#54 user_remove_superuser_delete_buttons.patch1.68 KBSenpai
#51 drupal.delete-uid1.patch3.11 KBsun
#36 drupal6.delete-uid1.patch3.14 KBsun
#35 drupal6.delete-uid1.patch834 bytessun
#27 disallow_uid1_delete.patch2.46 KBChrisKennedy
#8 user_delete_4.patch1.33 KBhunmonk
#7 user_delete_3.patch1.07 KBhunmonk
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

markus_petrux’s picture

Category: feature » bug

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.

markus_petrux’s picture

Steve Dondley’s picture

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.

dlr’s picture

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

It's a bit dangerous...

Crell’s picture

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.

markus_petrux’s picture

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

:insert rolling eyes smilie here:

hunmonk’s picture

Assigned: Unassigned » hunmonk
FileSize
1.07 KB

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.

hunmonk’s picture

FileSize
1.33 KB

oops. THIS patch nicely solves the problem...

killes@www.drop.org’s picture

Priority: Critical » Normal
Status: Active » Needs review

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

chx’s picture

Priority: Normal » Critical
Status: Needs review » Reviewed & tested by the community

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.

chx’s picture

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.

Crell’s picture

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.

Dries’s picture

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

killes@www.drop.org’s picture

Status: Reviewed & tested by the community » Closed (won't fix)

That's that.

markus_petrux’s picture

bradlis7’s picture

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.

budda’s picture

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?

markus_petrux’s picture

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.

Aurian Noreinor’s picture

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

ChrisKennedy’s picture

Title: Why allow deleting user 1? » Disallow deletion of user 1
Version: x.y.z » 6.x-dev
Category: bug » feature
Priority: Critical » Normal
Status: Closed (won't fix) » Active

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.

Crell’s picture

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.

hunmonk’s picture

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

webernet’s picture

webchick’s picture

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?

hunmonk’s picture

Assigned: hunmonk » Unassigned

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

joep.hendrix’s picture

Version: 7.x-dev » 6.x-dev
Status: Needs work » Active

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

ChrisKennedy’s picture

Status: Active » Needs review
FileSize
2.46 KB

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

Dries’s picture

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.

Crell’s picture

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

Gábor Hojtsy’s picture

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.

dmitrig01’s picture

Status: Needs review » Needs work

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

catch’s picture

Version: 6.x-dev » 7.x-dev
ewlloyd’s picture

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.

xiv’s picture

Version: 6.x-dev » 7.x-dev
Status: Active » Needs work

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.

sun’s picture

Version: 7.x-dev » 6.0
Category: feature » bug
Status: Needs work » Needs review
FileSize
834 bytes

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

sun’s picture

FileSize
3.14 KB

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

hass’s picture

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.

Freso’s picture

Version: 6.0 » 7.x-dev

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

macgirvin’s picture

subscribe

sdecabooter’s picture

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?

cridenour’s picture

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

Subscribing.

sun’s picture

Status: Needs review » Needs work

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

Dave Reid’s picture

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

function user_delete($edit, $uid) {
  if ($uid == 1) {
    drupal_set_message(t('Superuser #1 cannot be deleted.'), 'error');
    return;
  }
  $account = user_load(array('uid' => $uid));
  sess_destroy_uid($uid);
  _user_mail_notify('status_deleted', $account);
  module_invoke_all('user', 'delete', $edit, $account);
  db_query('DELETE FROM {users} WHERE uid = %d', $uid);
  db_query('DELETE FROM {users_roles} WHERE uid = %d', $uid);
  db_query('DELETE FROM {authmap} WHERE uid = %d', $uid);
  $variables = array('%name' => $account->name, '%email' => '<' . $account->mail . '>');
  watchdog('user', 'Deleted user: %name %email.', $variables, WATCHDOG_NOTICE);
}

ScoutBaker’s picture

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.

Dave Reid’s picture

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

function user_delete($edit, $uid) {
  global $user;
  if ($uid == 1) {
    drupal_set_message(t('Superuser #1 cannot be deleted.'), 'error');
    return;
  }
  elseif (isset($user->uid) && $user->uid == $uid) {
    drupal_set_message(t('You cannot delete your own account.'), 'error');
    return;
  }
  ...
sun’s picture

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.

markus_petrux’s picture

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:

/**
 * Check if the given user is global site administrator.
 *
 * @param $account
 *   (optional) The account to check, if not given use currently logged in user.
 *
 * @return
 *   Boolean TRUE if the current user is global site administrator.
 */
function user_is_global_admin($account = NULL) {
  global $user;

  if (is_null($account)) {
    $account = $user;
  }

  return ($account->global_admin == 1);
}

Then, user_delete could look something like this:

function user_delete($edit, $uid) {
  global $user;
  $account = user_load(array('uid' => $uid));
  if (user_is_global_admin($account)) {
    drupal_set_message(t('Global administrators cannot be deleted.'), 'error');
    return;
  }
  elseif (isset($user->uid) && $user->uid == $uid) {
    drupal_set_message(t('You cannot delete your own account.'), 'error');
    return;
  }
  ...

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.

sun’s picture

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.

markus_petrux’s picture

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.

webchick’s picture

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.

sun’s picture

Status: Needs work » Needs review
FileSize
3.11 KB

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

webchick’s picture

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

sun’s picture

Status: Needs review » Needs work

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?

Senpai’s picture

Status: Needs work » Needs review
FileSize
1.68 KB

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.

Re: Users | Senpai's HEAD sandbox
Re: admin | Senpai's HEAD sandbox
markus_petrux’s picture

Snippet for Drupal 6.

/**
 * Save this into protect_critical_uids.module
 */

/**
 * Implementation of hook_init().
 */
function protect_critical_uids_init() {
  // Catch user/*/delete requests.
  if (preg_match('#^user/([0-9]+)/delete#', $_GET['q'], $matches)) {
    global $user;
    $uid = (int)$matches[1];
    if ($uid == 0 || $uid == 1) {
      drupal_set_message(t('Oops! User #@uid cannot be deleted!', array('@uid' => $uid)), 'error');
      drupal_goto($uid == 0 ? '<front>' : 'user/'. $uid .'/edit');
    }
    else if ($uid == $user->uid) {
      drupal_set_message(t('Oops! You cannot delete your own account!'), 'error');
      drupal_goto('user/'. $uid .'/edit');
    }
  }
  // Catch delete requests from user administration.
  if (strpos($_GET['q'], 'admin/user/user') === 0) {
    if (!empty($_POST['accounts']) && isset($_POST['operation']) && ($_POST['operation'] == 'delete')) {
      if (isset($_POST['accounts'][1])) {
        unset($_POST['accounts'][1]);
        if (empty($_POST['accounts'])) {
          drupal_set_message(t('Oops! User #1 cannot be deleted!'), 'error');
          drupal_goto('admin/user/user');
        }
      }
    }
  }
}

/**
 * Implementation of hook_form_alter().
 */
function protect_critical_uids_form_alter(&$form, $form_state, $form_id) {
  // Remove delete button from user 1 edit form.
  if ($form_id == 'user_profile_form') {
    if (isset($form['delete']) && isset($form['_account']['#value']->uid) && $form['_account']['#value']->uid == 1) {
      unset($form['delete']);
    }
  }
}
Anonymous’s picture

Status: Needs review » Needs work

The last submitted patch failed testing.

markus_petrux’s picture

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

Hope that helps until this is properly addressed in core.

meba’s picture

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

Senpai’s picture

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?

alexanderpas’s picture

Assigned: Unassigned » alexanderpas
Status: Needs work » Needs review
FileSize
4.44 KB

try this patch, it even includes a test.

mfer’s picture

Status: Needs review » Needs work

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:

  if ($uid == 1) {
    drupal_set_message(t('Failed to delete user: This user is the superuser or root administrator, and cannot be deleted.'), 'error');
    return;
  }
alexanderpas’s picture

Status: Needs work » Needs review
FileSize
4.41 KB

fixed and re-rolled against head ;)

mfer’s picture

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.

mfer’s picture

Status: Needs review » Needs work
babbage’s picture

subscribing

alexanderpas’s picture

Status: Needs work » Needs review
FileSize
4.82 KB

intergrated comments, but adapted testcase differently.

Status: Needs review » Needs work

The last submitted patch failed testing.

alexanderpas’s picture

FileSize
4.77 KB

at least the bot works now...

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

alexanderpas’s picture

Status: Needs work » Needs review

and i always forget... something...

Status: Needs review » Needs work

The last submitted patch failed testing.

alexanderpas’s picture

Status: Needs work » Needs review
FileSize
5.39 KB

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

markus_petrux’s picture

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

alexanderpas’s picture

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

mfer’s picture

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

+  function testDeleteRootAdminSelf() {
+    // login as root administrator.
+    $this->rootAdminLogin();
+
+    $this->deleteButtonMissing();
+    $this->directLinkDisabled();
+    $this->massDeleteHandling();
+  }

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

alexanderpas’s picture

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.

alexanderpas’s picture

FileSize
6.27 KB

reroll against HEAD, improved tests. full review please?

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

swentel’s picture

Status: Needs review » Reviewed & tested by the community

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.

keith.smith’s picture

Status: Reviewed & tested by the community » Needs work

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

sun’s picture

+ * @note User #1 cannot be deleted

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

+  // Don't allow deletion of user #1

Missing trailing dot.

+    drupal_set_message(t('Failed to delete user 1: This user is the superuser or root administrator, and cannot be deleted.'), 'error');

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

-  if (user_access('administer users')) {
+  // User 1 cannot be deleted so we don't display the delete button.
+  if (user_access('administer users') && $account->uid != 1) {

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.

+class UserRootAdminNoDeleteTestCase  extends DrupalWebTestCase {

Double space in here.

+   * Don't allow the root administrator to delete itself.
+   * Don't allow deletion of the root administrator by another administrator

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

+   * Don't allow the root administrator to delete itself.
+    // Sort the list, so that the root administrator is in the list
+    // Select the root administrator for deletion
+   * Pepare the root administrator for login

Missing trailing punctuation.

+    // generate a username and password for the root administrator
+    // add the username and the hashed password to the database.
+    // load the complete user from the database.

First letter should capitalized.

sun’s picture

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

markus_petrux’s picture

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

alexanderpas’s picture

@#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

markus_petrux’s picture

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

function user_admin_account_validate($form, &$form_state) {
  $form_state['values']['accounts'] = array_filter($form_state['values']['accounts']);
  if (count($form_state['values']['accounts']) == 0) {
    form_set_error('', t('No users selected.'));
  }
  // Check that user 1 has not been requested for deletion.
  else if ($form_state['values']['operation'] == 'delete' && isset($form_state['values']['accounts'][1])) {
    form_set_error('', t('The first user cannot be deleted.'));
  }
}
alexanderpas’s picture

Assigned: alexanderpas » Unassigned
tstoeckler’s picture

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.

kscheirer’s picture

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.

sun’s picture

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.

Senpai’s picture

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.

sun’s picture

Title: Disallow deletion of user 1 » Prevent account cancellation for uid 1
Status: Needs work » Needs review
FileSize
1.79 KB

Better title.

Senpai takes it on from here :)

sun’s picture

FileSize
1.7 KB

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

Senpai’s picture

Assigned: Unassigned » Senpai

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.

sun’s picture

Resubmitted last patch for testing. Any updates?

Senpai’s picture

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.

Senpai’s picture

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.

Status: Needs review » Needs work

The last submitted patch failed testing.

Senpai’s picture

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?

macgirvin’s picture

array_pop *returns* the altered array.

Senpai’s picture

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:

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

Senpai’s picture

Status: Needs work » Needs review
FileSize
8.74 KB

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?

Status: Needs review » Needs work

The last submitted patch failed testing.

kscheirer’s picture

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!

Senpai’s picture

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.

kscheirer’s picture

@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.

kscheirer’s picture

gah, double post

tstoeckler’s picture

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.

brianV’s picture

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

Senpai’s picture

Status: Needs work » Needs review
FileSize
12.95 KB

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

alexanderpas’s picture

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.

Senpai’s picture

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.

michaelfavia’s picture

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.

Senpai’s picture

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

Status: Needs review » Needs work

The last submitted patch failed testing.

amc’s picture

Issue tags: +admin user

Tagging.

Senpai’s picture

Status: Needs work » Needs review
FileSize
19.91 KB

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.

kscheirer’s picture

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.

alexanderpas’s picture

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

Status: Needs review » Needs work

The last submitted patch failed testing.

BassPlaya’s picture

Version: 7.x-dev » 6.13

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?

hass’s picture

Version: 6.13 » 7.x-dev
Senpai’s picture

Status: Needs work » Needs review
FileSize
19.85 KB

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

Status: Needs review » Needs work

The last submitted patch failed testing.

Senpai’s picture

Status: Needs work » Needs review
FileSize
20.71 KB

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

Status: Needs review » Needs work

The last submitted patch failed testing.

cwgordon7’s picture

Status: Needs work » Needs review
FileSize
21.6 KB

Here's an updated patch that should pass testing.

alexanderpas’s picture

+++ modules/user/user.test	16 Aug 2009 15:13:49 -0000
@@ -127,7 +126,7 @@ class UserValidationTestCase extends Dru
+      'ᚠᛇᚻ᛫ᛒᛦᚦ'         => array('Valid UTF8 username', 'assertNull'), // runes

no need to remove spaces here, otherwise looking nice.

This review is powered by Dreditor.

Senpai’s picture

Priority: Normal » Critical
Status: Needs review » Reviewed & tested by the community
FileSize
20.25 KB

@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

webchick’s picture

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.

webchick’s picture

Priority: Critical » Normal

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

alexanderpas’s picture

Priority: Normal » Critical

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.

Senpai’s picture

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?

webchick’s picture

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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

xmacinfo’s picture

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

sun’s picture

Issue tags: -admin user +API clean-up

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.

sun’s picture

Status: Needs work » Needs review
FileSize
4.39 KB

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)

Status: Needs review » Needs work

The last submitted patch failed testing.

fizk’s picture

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

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

- Y.

Dave Reid’s picture

@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>

sun’s picture

Assigned: Senpai » sun
Status: Needs work » Needs review
FileSize
4.4 KB

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

markus_petrux’s picture

+      '#access' => ($account->uid == $user->uid && user_access('cancel account')) || (user_access('administer users') && $account->uid > 1),

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

brianV’s picture

Status: Needs review » Needs work

needs something like:

'#access' => (
  $account->uid > 1 && (
    ($account->uid == $user->uid && user_access('cancel account')) || (user_access('administer users'))
  )
),

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

sun’s picture

Status: Needs work » Needs review
FileSize
4.4 KB

Thanks, fixed in here.

TheRec’s picture

Status: Needs review » Needs work

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 ?

Bilmar’s picture

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.

Crell’s picture

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

sun’s picture

Status: Needs work » Needs review
FileSize
6.01 KB
3.93 KB
3.75 KB

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

cancel-uid1-multiple.png

cancel-uid1-only.png

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

TheRec’s picture

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" ;)).

xmacinfo’s picture

@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.

TheRec’s picture

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

brianV’s picture

Status: Needs review » Reviewed & tested by the community

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

Pasqualle’s picture

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

alexanderpas’s picture

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.

Status: Reviewed & tested by the community » Needs review

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

sun’s picture

Status: Needs review » Reviewed & tested by the community
pwolanin’s picture

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?

sun’s picture

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

Status: Reviewed & tested by the community » Needs review

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

pwolanin’s picture

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.

+  // Output a notice that user 1 cannot be canceled.
+  if (isset($accounts[1])) {
+    $redirect = (count($accounts) == 1);
+    $message = t('The user account %name cannot be cancelled.', array('%name' => $accounts[1]->name));
+    drupal_set_message($message, $redirect ? 'error' : 'warning');
+    // If only user 1 was selected, redirect to the overview.
+    if ($redirect) {
+      drupal_goto('admin/people');
+    }
sun’s picture

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.

Status: Needs review » Needs work
Issue tags: -API clean-up

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

Status: Needs work » Needs review
Issue tags: +API clean-up

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

sun’s picture

Status: Needs review » Reviewed & tested by the community
Senpai’s picture

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

Dries’s picture

Status: Reviewed & tested by the community » Fixed

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

silkscreen’s picture

Senpai’s picture

<happyface />

Status: Fixed » Closed (fixed)

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

lostchord’s picture

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

bryancasler’s picture

Is anything like this in 6.16? Also, what about user 0?

ajaysolutions’s picture

I'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.

Senpai’s picture

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

pratikk’s picture

In 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.

hass’s picture

This is why we make backups.

xmacinfo’s picture

Backups don’t solve every issues. This still needs to be fixed.

hass’s picture

But not for #172