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

Files: 
CommentFileSizeAuthor
#8 2032153-delete-admin-user-8.patch1.55 KBygerasimov
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2032153-delete-admin-user-8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#6 2032153-5.patch1.58 KBRaphael Dürst
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2032153-5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#5 2032153-4.patch1.61 KBRaphael Dürst
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2032153-4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#3 2032153-0.patch550 bytesRaphael Dürst
FAILED: [[SimpleTest]]: [MySQL] 1,589 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Comments

marcingy’s picture

Priority:Major» Normal
ygerasimov’s picture

Issue tags:+Novice

Add tagging.

Raphael Dürst’s picture

Status:Active» Needs review
StatusFileSize
new550 bytes
FAILED: [[SimpleTest]]: [MySQL] 1,589 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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
StatusFileSize
new1.61 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2032153-4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

StatusFileSize
new1.58 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2032153-5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

StatusFileSize
new1.55 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2032153-delete-admin-user-8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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