Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
This very simple patch makes it so that users other than uid 1 cannot edit edit uid 1. You may want to have several administrators able to futz with user accounts, but the admin should be protected from a cranky user.
(This was suggested by a comment in another thread, but I felt it should be a separate patch.)
Comment | File | Size | Author |
---|---|---|---|
#12 | no-edit-admin-39636-3.patch | 986 bytes | macgirvin |
#10 | no_edit_admin-39636-2.patch | 899 bytes | RobLoach |
no_edit_admin.patch | 993 bytes | Crell | |
Comments
Comment #1
Dries CreditAttribution: Dries commentedI don't think this is necessary.
Comment #2
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedThis is not only not neccessary it also doesn't make much sense. There might be multiple users which have all permissions on a site. A usefull way to prevent tampering with these accounts would be to only allow people who have some permission to edit all these accounts.
Comment #3
ahoeben CreditAttribution: ahoeben commentedThe point is that if you let anyone who has user edit privileges edit user 1, they can change the password for user 1, log in as user 1 and gain unrestricted access to the site.
No single privilege should give any user the means to gain unrestricted access.
Comment #4
Crell CreditAttribution: Crell commentedAgreed.
@killes: This isn't as much intended to prevent an arbitrary user from taking over an arbitrary user as it is to ensure that UID 1 remains the site owner's "trump card". Yes, other people can self-escalate, but only UID 1 is UID 1, and only UID 1 can change UID 1.
Comment #5
Crell CreditAttribution: Crell commentedI like hunmonk's patch in this thread better: http://drupal.org/node/46149 :-)
Comment #6
gaele CreditAttribution: gaele commentedhttp://drupal.org/node/46149 is about deletion of user #1. This issue is about editing.
When I first encountered this I was stupefied this is part of core. As has been said before the administer users permission allows anyone to change the password of user #1, effectively allowing "root access" to the site. In my opinion this renders the administer users permission pretty useless. Without any mention of this "feature" in the handbooks it's a security hole.
So, what to do? Block editing of user #1? Require supplying the current password when changing it? (Shouldn't we do this for all users?)
Comment #7
jrabeemer CreditAttribution: jrabeemer commentedGaele, speaking from a security standpoint, that (supplying current pw) would be a good security feature. But I think that deserves its own bug report/feature request.
Comment #8
chx CreditAttribution: chx commentedComment #9
RobLoachSubscribing... Although I want the moderators to be able to edit users, I don't want to give them root access.
Comment #10
RobLoachThis is a very important issue, as anyone with the "administer users" permission can take super user access (user/1). The attached patch makes it so that users with "administer users" cannot edit user/1. Only user/1 can edit the super user account.
Comment #12
macgirvin CreditAttribution: macgirvin commentedPatch works just fine - updated to chase HEAD. For many people, the issue is a critical, top priority security issue which shouldn't have remained in the queue for two days, let alone almost 2 1/2 years.
Comment #13
catchI'd like to see less special-casing of user/1 rather than more.
What about something like "administer $role users" permissions (a bit like content types)? That'd allow for proper delegation of user administration, and protect 'admin users' alongside user/1.
Comment #14
ahoeben CreditAttribution: ahoeben commentedI think we need both; noone should have the ability to break into the super-user account by resetting its name/password; the super-user account is much too powerful for that. More granular user administration would be nice to have.
Comment #15
catchPlenty of sites have admin roles where they can do everything user/1 can (except running update.php which I think is hardcoded to user/1) - and the patch as it stands doesn't offer protection to those roles. Giving someone the php filter permission allows them to do anything they like in your database too - but I don't think we should special case that module to be only enabled by user/1 or similar.
Comment #16
Crell CreditAttribution: Crell commented+1 to catch's suggestion in #13, as more fine-grained permissions are always a good thing. That still doesn't solve the problem of uid 1 still being editable, though, since uid 1 may still be in a role. If we ever introduce a "wheel" group for admins, then I'd give them all special treatment, not remove special treatment.
Comment #17
catchWell if you had an admin role, and put user/1 in that admin role - then you'd have to give users the "administer admin users" permission in order for user/1 to be editable going by #13. I'd be in favour of an admin role with user/1 being placed into it as part of the default install profile, or something similar, in fact it'd be quite nice to get rid of the whole idea of user/1 if there's a clean way to do this.
Various combinations of administer input formats, administer permissions and administer modules can let you escalate yourself to a situation where 'DELETE FROM {users};' is just a couple of googles away for any moderately competent malicious user. In fact that'd be easier than escalating yourself to user/1.
So if we're going to start special casing permissions then this'll need to be done in a lot more places, otherwise it's gives a false sense of security. And if we do that, then http://drupal.org/node/248598 needs some more consideration.
Comment #18
macgirvin CreditAttribution: macgirvin commentedA very common configuration is to allow 'staff' members of an organization permission to manage the daily operations of a site, whilst leaving the critical settings (which could mess things up completely) protected. In this regard, we mostly have the ability now to set things up in a manner that works and prevents somebody with 'some' privileges from blowing the site away completely. There's rarely an organizational need to delegate the administer filters or even the administer modules permissions once the site is set up and running smoothly.
However, administering users is one of the primary tasks we (in this case technical operations and IT personnel) wish to delegate to non-technical admin staff - and this doesn't work very well in the current model because it's basically all or nothing. I'm personally ambivalent about whether this is based on user/1; but it's fairly trivial to make this single level of delegation work and seems like the easiest solution short of a general permissions overhaul that would probably be overkill for the vast majority of sites.
Comment #19
gaele CreditAttribution: gaele commentedRelated: #251370: Require current password before changing it
Comment #20
David_Rothstein CreditAttribution: David_Rothstein commentedThis patch seems like it may go a bit too far. Not being able to edit the email address and password is one thing, but if I'm not mistaken, this would also prevent much more benign things (like signature and picture) from being edited too. Is that really the desired behavior?
In the long run, however, I agree that breaking "administer users" into more fine-grained permissions is the real solution here...
Comment #21
timoratd CreditAttribution: timoratd commentedsubscribe.
What a lifesaver! now I can open 'administer users' permission to PR guys, it was a nightmare to add new hired to the system every week by myself.
Comment #22
Dave ReidSee also #46149: Prevent account cancellation for uid 1.
Comment #24
rhimes CreditAttribution: rhimes commented+million for this patch
just discovered this thread & IMO should be a security update - thanks all!
now a question - I've a dist. profile which creates a "psuedo-user/1" with almost all permissions which needs the security/protection this patch affords a "real" #1 - how do I add that (code syntax) for user/3 - which my "psuedo #1" will always be?
Thanks in advance
Comment #25
RobLoachSo the general conception is that we should split the "administer users" role into "administer %role users"? If this is the case:
Comment #26
Crell CreditAttribution: Crell commentedRelevant related discussion: #381584: Hierarchical Permissions System
Comment #27
sunAmen to that. Would have greatly simplified node/8 (canceling own user account).
The same way we need to special case uid 1 to prevent its deletion (via UI), we need to special case editing its password. However, with regard to aforementioned, badly needed check for the current password, we want to unconditionally expose the "current password" field for uid 1.
Comment #28
newbuntu CreditAttribution: newbuntu commentedFor my application, I want to have "super admin" and "regular admin" (I created a role for that)
I want the "regular admin" to have most permissions like the "super admin" with a few exceptions, such as administer modules and php filter access.
However, if I grant "administer users" to a "regular admin" or anybody, then he can change his own permissions to anything he wants. The problem is "administer users" permission is an all-or-nothing option.
I think the permission control should use a policy somewhat like this: a user CANNOT grant more permissions to anyone (include himself) than what his "superior" has granted him.
I understand "superior" implies a hiarchical chain. If we don't want to have a full hirachy, we should have at least two levels: Super Admin and everyone else - except "super admin", no one should be able to grant a permission to someone, if the former doesn't have that permission in the first place.
Comment #29
rhimes CreditAttribution: rhimes commented@ newbuntu -
I've done the same - hence #24 question & BTW see fixed below
Your "regular admin" can't change his or anyone else's perms without you granting him "administer permissions" even if you do grant him "administer users" - it's currently NOT "all or nothing".
However, if you want the "regular admin" (or others) TO be able to grant others "some" permissions, you can do that by creating roles (like your "regular admin" role), assign those roles perms, and use Role_Assign module and allow specific roles assignable (or not) by those w/ "administer users" AND "assign roles" perms.
#24 question fixed (afford same protection given uid1 to "almost"1 [#3 in my case]) w/ changing user.module / function user_edit_access from:
to:
Comment #30
ahoeben CreditAttribution: ahoeben commented> Your "regular admin" can't change his or anyone else's perms without
> you granting him "administer permissions" even if you do grant him
> "administer users" - it's currently NOT "all or nothing".
AFAIK, it is all or nothing; anyone with "administer users" permission can change the password for user 1. And once you have access to the user 1 account, you have access to everything.
Comment #31
rhimes CreditAttribution: rhimes commented@ahoeben - I think we're both restating the basic issue but in different ways
In #29 I'm telling newbuntu that u1 edit protection (by hack supplied [which includes "psuedo-1" protection for his case & mine] or other patches et al which effect same) makes "administer users" NOT "all or nothing", because "administer users" does not include "administer permissions".
Comment #32
newbuntu CreditAttribution: newbuntu commented@rhimes, #31
I misspoke when I said "administer users" was all-or-nothing. I meant "administer permissions" was all-or-nothing. I came up with a simple idea that I believe can fundamentally alter how drupal manages permission control. Please take a look at http://drupal.org/node/491732. Feedback will be appreciated!
Coupled with this patch, I believe my proposed change creates a quasi-hierarchical permission management scheme for drupal, where super admin (uid=1) can grant "administer permissions" easily to "regular admin" without worrying about losing total control.
Comment #33
sunThanks for taking the time to report this issue.
However, marking as duplicate of #86299: Add "current password" field to "change password form".
You can follow up on that issue to track its status instead. If any information from this issue is missing in the other issue, please make sure you provide it over there.
Comment #34
David_Rothstein CreditAttribution: David_Rothstein commentedI don't think this is actually a duplicate. See my comment at http://drupal.org/node/86299#comment-2201598 for why.
Comment #35
sourcesoft CreditAttribution: sourcesoft commented#12: no-edit-admin-39636-3.patch queued for re-testing.
Comment #36
droplet CreditAttribution: droplet commentedComment #37
klausipatch does not apply
Comment #38
lpalgarvio CreditAttribution: lpalgarvio commented+1