Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Services makes it possible that with 'administer users' permission, user/0 and user/1 can be deleted, which can break the website.
Comment | File | Size | Author |
---|---|---|---|
#8 | 2032153-delete-admin-user-8.patch | 1.55 KB | ygerasimov |
#6 | 2032153-5.patch | 1.58 KB | Raphael Dürst |
#5 | 2032153-4.patch | 1.61 KB | Raphael Dürst |
#3 | 2032153-0.patch | 550 bytes | Raphael Dürst |
Comments
Comment #1
marcingy CreditAttribution: marcingy commentedComment #2
ygerasimov CreditAttribution: ygerasimov commentedAdd tagging.
Comment #3
Raphael Dürst CreditAttribution: Raphael Dürst commentedVery simple patch, but I think it solves this issue.
Comment #4
ygerasimov CreditAttribution: ygerasimov commentedThank 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?
Comment #5
Raphael Dürst CreditAttribution: Raphael Dürst commentedI 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.
Comment #6
Raphael Dürst CreditAttribution: Raphael Dürst commentedJust found one small thing,
array('@uid' => $uid)
is not necessary anymore.Comment #7
marcingy CreditAttribution: marcingy commentedInstead of
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?
Comment #8
ygerasimov CreditAttribution: ygerasimov commented@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.
Comment #9
marcingy CreditAttribution: marcingy commentedLooks good now.
Comment #10
kylebrowning CreditAttribution: kylebrowning commented