Services makes it possible that with 'administer users' permission, user/0 and user/1 can be deleted, which can break the website.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marcingy’s picture

Priority: Major » Normal
ygerasimov’s picture

Issue tags: +Novice

Add tagging.

Raphael Dürst’s picture

Status: Active » Needs review
FileSize
550 bytes

Very simple patch, but I think it solves this issue.

ygerasimov’s picture

Status: Needs review » Needs work

Thank you very much for the patch. Could you please also add a test for this situation to ensure that we get error back in case we try to delete those users?

Raphael Dürst’s picture

Status: Needs work » Needs review
FileSize
1.61 KB

I think, it was not really possible to delete user 0 before. It returns a 404 status.
But I added tests for both deleting user 0 & 1.

I didn't actually write any tests before this one, so I hope I did this right.

Raphael Dürst’s picture

FileSize
1.58 KB

Just found one small thing, array('@uid' => $uid) is not necessary anymore.

marcingy’s picture

Instead of

function _user_resource_delete($uid) {
   $account = user_load($uid);
-  if (empty($account)) {
+  if ($uid == 1) {
+    return services_error(t('The admin user cannot be deleted.'), 406);
+  }
+  elseif (empty($account)) {
     return services_error(t('There is no user with ID @uid.', array('@uid' => $uid)), 404);
   }

Why not only load the account if uid is not equal to 1? Otherwise looks good.

-------

On a total side note and out of scope doing a full user load for purposes of checking if an account exits seems over kill, we could just do a simple efq query or direct MySQL query? Yuriy thoughts?

ygerasimov’s picture

@Raphael Dürst thanks very much. Tests look good.

I have checked rfc for response codes (http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html) and think returning 403 is more appropriate in case user tried to delete admin user.

I have also moved check about the user before it gets loaded. See attached patch.

@marcingy I agree that we can optimize not loading full user. But this is micro optimization that I do not much concerned. If you like please commit substituting user_load() with efq.

marcingy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good now.

kylebrowning’s picture

Status: Reviewed & tested by the community » Fixed

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