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.
A user that can delegate one or more roles can change the password of any users, including administrators, e.g. user 1.
Proposed fix:
give an access denied error when this user tries to edit an account that has 'administer users' permission (as users for which this module is intended don't have the 'administer users' permission themselves, they should not be able to edit users who do).
patch will follow in the next comment.
Comment | File | Size | Author |
---|---|---|---|
#16 | 1156414-prevent-editing-of-certain-users-16.patch | 3.52 KB | geodaniel |
Comments
Comment #1
Willem van den Ende CreditAttribution: Willem van den Ende commentedThis is a 4 line patch (including comment) that denies access to accounts that have 'administer permissions' permission. This makes it possible to have multiple users with admin permissions that can't be modified by others.
Comment #2
Andrew Schulman CreditAttribution: Andrew Schulman commentedI'm sorry, I'm probably being dense, but could you please explain how that can happen? How, by assigning a role to a user or removing one, can I ultimately change any user's password?
The patch seems clear enough, and might be a good idea. I just need to understand the flaw that it addresses. We might also want to write a test to be sure that the flaw is addressed.
Comment #3
nico.knaepen CreditAttribution: nico.knaepen commentedsubscribing
Comment #4
drasgardian CreditAttribution: drasgardian commentedI think I'm hitting a similar scenario to Willem here.
I need users with the role of 'site admin' to be able to create users and assign them the role 'researcher', but not any other role.
I have given site admins permission to administer users and have used role delegation to allow them to assign the researcher role.
But now, from the 'administer users' permission, site admins can change the password of user 1.
Willem's patch prevents against this.
The underlying issue here is not necessarily role_delegation's responsibility to fix, it's more related to #39636: Security: The "administer users" permission exposes user/1. That said, plenty of people might still benefit from Willem's patch.
Comment #5
drasgardian CreditAttribution: drasgardian commentedAs there is no patch for Drupal 7, I ended up using the userprotect module instead.
Comment #6
LeoPx CreditAttribution: LeoPx commentedFor Drupal 7, you can add this function in role_delegation.module :
Comment #7
solotandem CreditAttribution: solotandem commented@Andrew Schulman, responding to your questions in #2 (which you may have answered but are not documented on this issue):
If a user has the "Administer users" permission, then he will be able to enter a URL of "user/%/edit" or click the "Edit" link on any user (from the user list page) and change their account data, including the password. Without this permission, this module will allow him to only be able to edit certain roles for a user (based on the role delegation permissions assigned to him) at "user/%/roles."
The attached patch prevents user A from editing user B if any of the following apply:
- user B is user 1 and user A is not
- user B has a role user A can not assign.
This patch adds the first condition above to the code snippet posted by Leo-Px in #6.
Comment #8
sanguis CreditAttribution: sanguis commented#7 works great!
Comment #9
Kirschbaum CreditAttribution: Kirschbaum commented#7 works great for me as well. It would be nice to get this into the next stable release.
Comment #10
jason.fisher CreditAttribution: jason.fisher commentedCan we get this committed?
Comment #11
Benji Mauer CreditAttribution: Benji Mauer commented#7 works as advertised. Seems like a critical issue that should get patched as soon as possible.
Comment #12
Benji Mauer CreditAttribution: Benji Mauer commentedMy only recommendation from a UI standpoint would be to also hide the edit links for users with roles that a user can't assign in the "People" tab, but I'm not able to write a patch for it.
Comment #13
geodaniel CreditAttribution: geodaniel commentedI've attached an updated patch which does a few more things to improve this patch:
Comment #14
klonos...still RTBC. I gave this a spin at simplytest.me
1. Created a "high user" role.
2. Created a "low user" role.
3. Created a user "High" (
user/2
).4. Created a user "Low" (
user/3
).5. Gave both the "Assign high user role" as well as the "Assign low user role" permissions to user High.
6. Gave only the "Assign low user role" permissions to user Low.
7. Gave both High and Low users the "View user profiles" permission.
Verified that the patch at #13 works the same as the one at #7:
- When logged in as user "High", I could view and edit both user profiles and could assign both "high user" and "low user" roles ("Edit" tab was available in both profiles and access allowed in both
/user/2/edit
as well as/user/3/edit
).- When logged in as user "Low", I could view both profiles but only edit the Low user profile (the "Edit" tab was not available for user "High" and got access denied for
/user/2/edit
).8. Gave both Hing and Low users the "Administer users" permission (so they can access
/admin/people
).- When logged in as user "High", I could view all user profiles listed under
/admin/people
, and the "edit" operation link was available for users "High" and "Low" - not for the admin.- When logged in as user "Low", I could view all user profiles listed under
/admin/people
but had an edit link available only for user "Low" itself - not for user "High" or admin.One could do a more extensive test using devel_generate to generate a few extra users, but I hope that covers it all.
Comment #15
geodaniel CreditAttribution: geodaniel commentedThanks for the detailed testing. Do you think we should also remove the Roles tab from the profile pages of these higher privileged users when the editing user doesn't have the 'Administer users' permission?
Comment #16
geodaniel CreditAttribution: geodaniel commentedThis updated patch adds a check to the role_delegation_access() function to hide the roles tab as well.
Comment #17
klonosGimme a few hours and I'll retest the scenario in #14 (adjusting it to include the change from #15/#16). This time I'll use devel_generate to create more users so that I can test the "Low" user by logging in with a different low-user account and the "High" user by logging in with a different high-user account (the way I tested now we couldn't be sure if the tabs, the action links and the access denied pages worked as they did simply because these users were viewing/editing their own account).
Comment #18
NWOM CreditAttribution: NWOM commented#16 still works great! Thank you!
Comment #19
JeroenTIt’s been a while since the last comments in this issue. If this is still a problem feel free to reopen and provide patches.
Comment #20
NWOM CreditAttribution: NWOM commented#16 absolutely works, and would potentially just need a reroll.
Comment #21
NWOM CreditAttribution: NWOM commentedComment #22
JeroenTImo this should not be part of role_delegation as @drasgardian stated in #5 the user_protect module should be used. This module is more flexible and has more configuration options.
For the /user/%user/roles page an extra check can be added when the user_protect module is enabled.
Comment #23
JeroenTI provided a patch in the userprotect issue queue: #1984520: "User Role Delegation" module overrides User Protect settings, but I would like the opposite behavior which fixes the integration with userprotect. I will close this issue in favor of that.
Comment #24
rutiolmaModule userprotect is not super flexible.
For example, if I have a role just to manage users (Role A) and another role of editors (Role B) I may want that all users within role A get able to manage users in role B, but that won't be possible without allowing role B users to manage role B users.
This introduces a big limitation that didn't exist just with Role Delegation.
I agree with the fact that access checks should be done elsewhere but, in this case, it makes sense to block the access to roles not assigned for delegation.