Forbid editing of accounts that have higher permission than current user

A user that can delegate one or more roles can change the password of any users, including administrators, e.g. user 1.

I'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.

subscribing

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

As there is no patch for Drupal 7, I ended up using the userprotect module instead.

For Drupal 7, you can add this function in role_delegation.module :

/**
 * Implements hook_form_FORM_ID_alter().
 * @param type $form
 * @param type $form_state
 *
 * You can't edit users that have a role you can't assign
 */
function role_delegation_form_user_profile_form_alter(&$form, $form_state) {
  global $user;
  if (!user_access('assign all roles') && $form['#user']->uid != $user->uid) {
    foreach ($form['account']['roles']['#default_value'] as $role) {
      if ($role != DRUPAL_ANONYMOUS_RID && $role != DRUPAL_AUTHENTICATED_RID) {
        if (!user_access('assign ' . $form['account']['roles']['#options'][$role] . ' role')) {
          drupal_access_denied();
          drupal_exit();
        }
      }
    }
  }
}

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

#7 works great!

#7 works great for me as well. It would be nice to get this into the next stable release.

Can we get this committed?

#7 works as advertised. Seems like a critical issue that should get patched as soon as possible.

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

I've attached an updated patch which does a few more things to improve this patch:

• hides /user/%/edit menu item (tab), preventing the edit tab from showing when user user B has higher permissions
• hides the edit link on admin/people admin screen when user B has higher permissions

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

Thanks 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?

This updated patch adds a check to the role_delegation_access() function to hide the roles tab as well.

Gimme 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).

#16 still works great! Thank you!

It’s been a while since the last comments in this issue. If this is still a problem feel free to reopen and provide patches.

#16 absolutely works, and would potentially just need a reroll.

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

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

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