It seems that a user with that permission can access all sorts of funny places such as:

/admin/config/people/accounts

Where they can pretty much destroy the whole site from :/

CommentFileSizeAuthor
#14 remove_administer_users_dependency-1717876-14.patch6.03 KBElin Yordanov
#13 remove_administer_users_dependency-1717876-13.patch6.03 KBElin Yordanov
#11 remove_administer_users_dependency-1717876-7-11.patch1.67 KBElin Yordanov
#9 Bildschirmfoto-15.png37.63 KBfraweg
#7 remove_administer_users_dependency-1717876-7.patch5.21 KBBWPanda
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

7wonders’s picture

7wonders CreditAttribution: 7wonders commented

Only tested on drupal 7.14.

tanc’s picture

tanc
he/him
Primary language English
Location Italy
CreditAttribution: tanc commented

Agreed, this seems like a good way to destroy your site. A user with administer users can also access the 'Manage display' and the 'Manage fields' local tasks which allow them to cause all sorts of trouble.

v1adimir’s picture

v1adimir CreditAttribution: v1adimir commented

Users with 'adminster users' disabled cannot create new users. This behavior is hardcoded in core user.module, 'adminster users' is checked when user_register_form is built.

7wonders’s picture

7wonders CreditAttribution: 7wonders commented

Subuser module gets around this with hook_menu_alter and then its own access callbacks. See below:

/**
 * Implements hook_menu_alter().
 */
function subuser_menu_alter(&$items) {
  $items['admin/people/create']['access callback'] = 'subuser_access_create_callback';
  $items['user/%user']['access callback'] = 'subuser_access_view_callback';
  $items['user/%user/cancel']['access callback'] = 'subuser_access_delete_callback';
  $items['user/%user/cancel/confirm/%/%']['access callback'] = 'subuser_access_delete_callback';
  $items['user/%user/edit']['access callback'] = 'subuser_access_edit_callback';
}

Also, I think if its going to remain with just using the administer users permission it should at least warn people what a can of worms that opens up.

dddbbb’s picture

dddbbb CreditAttribution: dddbbb commented

Agreed. Enabling administer users is far too risky. Checking out http://drupal.org/project/subuser

dddbbb’s picture

dddbbb CreditAttribution: dddbbb commented

Adding the Field UI permissions module to the mix certainly seems to help. At least then you can control which roles can get their grubby mits on 'Manage Fields' and 'Manage Display' for user entities.

http://drupal.org/project/field_ui_permissions

BWPanda’s picture

BWPanda CreditAttribution: BWPanda commented
Status: Active » Needs review
FileSize
remove_administer_users_dependency-1717876-7.patch5.21 KB

Here's a patch that removes the dependency on the 'Administer users' permission (mostly copied from the Subuser module).

I removed all references to the 'administer users' permission from the .test file, however as I have no idea how to run tests, I'm not sure if they still pass or if I've messed them up...

fraweg’s picture

fraweg CreditAttribution: fraweg commented

Hello,

is the actual dev version patched with #7 ?

Best regards
Frank

Edit:

I have test to patch the actual dev version and get this output:

:/$ patch -p1 < remove_administer_users_dependency-1717876-7.patch
patching file administerusersbyrole.info
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file administerusersbyrole.info.rej
patching file administerusersbyrole.module
Hunk #2 succeeded at 191 (offset 6 lines).
patching file administerusersbyrole.test

Did I something wrong? And also the "add user" permission does not work without enabling the "administer user permission"
Have I to do something else?

Mea culpa! After clearing the cache it works. But the "Hunk #1 FAILED at 1." still exist.

fraweg’s picture

fraweg CreditAttribution: fraweg commented
FileSize
Bildschirmfoto-15.png37.63 KB

Ok...now the add role feature seems to work. But when I create the user I have two fields to assign a role to the user as you can see it in the screenshot. Any Idea what to do?

Best regards
Frank

Edit: Sorry...this was an issue with role delegation

nubeli’s picture

nubeli CreditAttribution: nubeli commented

I used patch in #7 on the git commit 9a4ee4a3ed9d8bc9c8f51b1e1257eb8a0aa784c2.

The change does allow the selected role to add and edit users without the broad administer users permission. But it also provides access to the permissions page even though the role doesn't have the permission "administer permissions", which is an even bigger security issue.

If you remove this line from the patch:

$items['admin/people']['access callback'] = 'administerusersbyrole_access_create_callback';

then it removes access to admin/people, admin/people/* except for admin/people/create which is still explicitly listed.

Elin Yordanov’s picture

Elin Yordanov CreditAttribution: Elin Yordanov commented
Title: Enabling adminster users = bad advice!!! » Enabling administer users = bad advice!!!
FileSize
remove_administer_users_dependency-1717876-7-11.patch1.67 KB

Patch #7 works!

However, I found out that the users with 'create users' permission cannot view blocked users. So that, for example, a user with 'create users' permission but without 'administer users' permission can create a user, set his status as blocked (in my case for a peer review for example), and if he wants to view the users account that he just created, he gets access denied message.

I have extended the patch #7, and attached it, but it is not a merged patch, it requires first the patch #7.
If someone can merge these patches (after reviewing) I would be grateful.

Status: Needs review » Needs work

The last submitted patch, remove_administer_users_dependency-1717876-7-11.patch, failed testing.

Elin Yordanov’s picture

Elin Yordanov CreditAttribution: Elin Yordanov commented
Status: Needs work » Needs review
FileSize
remove_administer_users_dependency-1717876-13.patch6.03 KB

Ok, here's the merged patch. This patch contains the patch #7 and enhances it with the solution of the problem described on the comment #11.
----
EDIT: Please ignore this patch, it has a typo. Use the next patch I am going to post.

Elin Yordanov’s picture

Elin Yordanov CreditAttribution: Elin Yordanov commented
FileSize
remove_administer_users_dependency-1717876-14.patch6.03 KB

Here is a revisited version of the previous patch.

edmonkey’s picture

edmonkey CreditAttribution: edmonkey commented

Which commit was this patch #14 for?

Elin Yordanov’s picture

Elin Yordanov CreditAttribution: Elin Yordanov commented

The latest dev version from 2013-May-14. I did also a few other patches in my local installation. If the patch doesn't apply correctly because of the line numbers aren't matching, please try to apply the changes manually and provide a new patch.

prabhatjn’s picture

prabhatjn CreditAttribution: prabhatjn commented

14: remove_administer_users_dependency-1717876-14.patch queued for re-testing.

Ditiwi’s picture

Ditiwi CreditAttribution: Ditiwi commented

14: remove_administer_users_dependency-1717876-14.patch queued for re-testing.

Ditiwi’s picture

Ditiwi CreditAttribution: Ditiwi commented

Good alternative solution: drupal.org/project/user_settings_access

gaele’s picture

gaele CreditAttribution: gaele commented
Title: Enabling administer users = bad advice!!! » Remove dependency on 'Administer users' permission
Issue summary: View changes
gaele’s picture

gaele CreditAttribution: gaele commented
Status: Needs review » Needs work

Patch applies to latest dev. I am able to create but not edit a user.

sinasalek’s picture

sinasalek CreditAttribution: sinasalek commented

This seems related #1776666: Unable to access admin/people even if users have the ability to create new users.

sinasalek’s picture

sinasalek CreditAttribution: sinasalek commented

I had to apply the patch manually but it seems to be working

AdamPS’s picture

AdamPS CreditAttribution: AdamPS commented

Thanks for the patch.

OK, I'm definitely not a expert here - but I've been trying to get this working myself, so I have at least read a lot of the relevant code. My experience is that the module seems to work quite well without 'administer users' permission turned on. I found various problems, some of which this patch addresses:

  • Can't create users (addressed)
  • Can't view blocked users (partially addressed)
  • "Cancel account" button is missing from edit user form
  • In views of users that should have edit or cancel links, the links are missing
  • Can't access the list of users admin/people/ - see #1776666: Unable to access admin/people even if users have the ability to create new users.. I created my own view of users, which has the advantage that you can include the suitable fields. Or that issue has some other possible solutions.

I have some concerns with the patch provided - please tell me why I might be wrong.

1) Altering internals of the permission system seems like potentially bad practice. I agree it's hard to see another option.
$static[$account->uid][$string] = TRUE;
It leaves the permissions cache altered as if the sub-admin user had 'administer users' permission after all. Is there any chance this could be exploitable?? I'm thinking the malicious user could manipulate the form response to reinsert the roles that had been removed or alter the username to an existing user or something like that.

2) This patch fixes "Can't view blocked users " based on 'create users' permission. But what about for a user who has edit permissions for some roles but not 'create users'? It seems like it would be better to allow view of a blocked user precisely when we would be able to edit it.

3) There are some fairly subtle bits of code here - please could we have some comments?
- Comment that _administerusersbyrole_can_view_user is a clone of user_view_access with a small change.
- In administerusersbyrole_access_create_callback refer to the problematic code in user_register_form and explain the solution used.

AdamPS’s picture

AdamPS CreditAttribution: AdamPS commented
Assigned: Unassigned » AdamPS

I intend to fix this as part of #2378869: Meta-issue for Beta 2 release. Please sign up as a follower of that issue and there will shortly be a patch that I would like feedback on.

I have taken the patch from #22 in a modified form - there are quite a few other changes going in too. I have got myself comfortable with issue 1) from my #24, and added a better comment. For 2) I have used a more accurate permission as I suggested. I have fixed the other issues in my bullet list too.

AdamPS’s picture

AdamPS CreditAttribution: AdamPS commented
Status: Needs work » Needs review
AdamPS’s picture

AdamPS CreditAttribution: AdamPS commented
Issue tags: +beta2
AdamPS’s picture

AdamPS CreditAttribution: AdamPS commented

Note that I have deliberately not taken the part of the patch that allowed the subadmin to assign roles. As far as I can see this module is not designed to allow any modification of roles. That's a separate permission, not part of 'administer users' which is what this module deals with. There are other modules that can help provide limited setting of roles.

tregismoreira’s picture

tregismoreira CreditAttribution: tregismoreira commented

#14 works sweet for me! Thanks a lot ;)

  • AdamPS committed 34b1116 on 7.x-2.x 
    Batch of changes for beta2 release

There are too many interlocking...
AdamPS’s picture

AdamPS CreditAttribution: AdamPS commented
Version: 7.x-1.x-dev » 7.x-2.0-beta1
Status: Needs review » Fixed

Fix now available in latest release

AdamPS’s picture

AdamPS CreditAttribution: AdamPS commented
Status: Fixed » Closed (fixed)