Problem/Motivation
On busy websites with many users, it becomes essential to be able to delegate some tasks, for example the management of the user's roles. As it is, you can give the 'administer permissions' to the users who will be in charge of managing the user's roles, but this also allows them to access the Roles and Permissions admin pages!
The RoleAssign module creates this permission, but I believe this feature would be useful in the core, as opposed to having to use an extra module for such a basic feature. (see the desription of the module for more information).
Here is a statement of the access control page.
Permissions also allow trusted users to share the administrative burden of running a busy site.
No doubt this extra permission would extand the flexibility of sharing the administration of busy sites.
Proposed resolution
As suggested in #7 create two new permissions:
- 'administer roles' to give access to admin/people/roles
- 'assign roles' to show the role selector in user edit form (and show the role-related bulk operations on user admin view)
(and keep 'administer permissions' to give access to admin/people/permissions)
Remaining tasks
Task | Novice task? | Contributor instructions | Complete? |
---|---|---|---|
Create a patch | Instructions | ||
Update the issue summary | Instructions | Done in #29 | |
Update the issue summary noting if allowed during the beta | Instructions | ||
Add automated tests | Instructions | ||
Draft a change record for the API changes | Instructions |
User interface changes
Two new permissions
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#72 | split_administer_permissions_151311-72.patch | 3.28 KB | berliner |
| |||
#63 | split_administer_permissions_151311-63.patch | 3.32 KB | berliner |
#61 | split_administer_permissions_151311-61.patch | 3.32 KB | lexbritvin |
#55 | split_administer_permissions_151311-55.patch | 5.72 KB | mmbk |
#51 | split_administer_permissions_151311-51.patch | 5.14 KB | mmbk |
Issue fork drupal-151311
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #1
Dries CreditAttribution: Dries commentedLooks interesting.
My only gripe is that when you ask a random person what the difference is between 'administer access control' and 'assign roles', he probably won't be able to answer that question correctly.
In other words, can we rename 'administer access control' to make it more explanatory? What access does 'administer access control' permission give you after this patch lands? Maybe we can come up with better wording for this permission and further streamline things?
Comment #2
scor CreditAttribution: scor commentedGood point Dries.
'administer access control' gives access to 3 pages:
- admin/user/access (Permissions page, determine access to features by selecting permissions for roles)
- admin/user/roles (List, edit, or add user roles)
- admin/user/rules (List and create rules to disallow usernames, e-mail addresses, and IP addresses)
I also found it in the system.module file, in the system_get_module_admin_tasks function, but it's not used, and there is a comment saying
TODO: this needs to be rewritten for the new menu system.
I can't think of anything better than 'administer access control'. Any other idea considering the 3 pages above?
Comment #3
Crell CreditAttribution: Crell commented+1 on this. For the rename, "administer access configuration" vs. "administer user roles"?
Comment #4
scor CreditAttribution: scor commented"administer user roles" is too specific, its meaning doesn't include the third page:
- admin/user/rules (List and create rules to disallow usernames, e-mail addresses, and IP addresses)
This is typically access administration.
Or else an extra permission called 'administer access rules' could be created for this page.
At the end, the 'administer access control' permission would be split into 3 more explicit permissions: administer access rules, administer user roles, assign roles.
Dries, these 3 permissions are surely more explanatory than 'administer access control'.
Comment #5
Dries CreditAttribution: Dries commentedIf we can't come up with a single good name, it's best to further break up the permissions so that all of them are straight-forward and intuitive.
Comment #6
scor CreditAttribution: scor commentedLet's break up the 'administer access control' into 3 more intuitive permissions:
- administer access rules (the access rules page!)
- administer roles (the access control page with all the permissions/roles, and the add and edit roles pages)
- assign roles (the role section on the user profile pages, and the mass role update on the admin user page)
Attached is a patch implementing this against the lastest HEAD. Do we want this in drupal 6?
Note: make sure to fire menu_rebuild() in order to rebuild the menu_router table and get rid of the former 'administer access control'. Or reinstall drupal after patching to make sure you have a clean set of permissions.
Comment #7
Dries CreditAttribution: Dries commentedI'd like to rename "access control" to "permissions". Also the menu item ...
Given that, we could use the following:
1. administer permissions
2. administer access rules
3. create roles
4. assign roles
Comment #8
scor CreditAttribution: scor commentedstraight forward patch changing the former 'administer access control' permission to one of these 4 new permissions:
1. administer permissions
2. administer access rules
3. create roles
4. assign roles
I don't think we can be more granular and explicit than that.
'create roles' is a bit restrictive since it also allows the edition and deletion of the roles, but if we call it 'administer roles', again it's confusing and people might think it deals with administer permissions or assign roles to users. So I believe 'create role' is good.
The ambiguous menu item 'Access control' has also been rename to 'Permissions'.
Successfully tested on a fresh install.
Comment #9
Crell CreditAttribution: Crell commented- +1 on fully splitting the permissions up to essentially permission per page.
- I'm getting warnings about DOS line endings, although the patch applies successfully anyway.
- It's not immediately apparent what "administer users" means now. I'm not sure if that's a task for this patch or not, but I will mention it.
- This may be a larger question for Drupal 6 in general, but all of these pages are beneath /admin/user, which has the permission "administer site configuration". In the new menu system that means you need that permission as well. I think that's acceptable if and only if that permission gives no access to any "real" pages, just the administrative index pages. I mention it here because I just noticed it while reviewing this patch.
- If "create roles" actually gives edit and delete permissions as well, shouldn't it be "manage roles"?
Comment #10
scor CreditAttribution: scor commented'manage roles' is actually more accurate. +1
This patch implements it.
This is correct, except for the 'assign roles' permission which is used to enable the role update feature on the edit profile page, and the user administration page.
fyi, 'administer users' permission allows to
- view, edit and delete all user accounts
- access the admin user page
- search through the users
- register new users
Comment #11
scor CreditAttribution: scor commentedIs there any chance to have these cool permissions in drupal 6? I think many people will like the flexibility offered by them, especially the 'assign roles' one.
Comment #12
alanburke CreditAttribution: alanburke commented+1 great idea.
Certainly useful for larger sites where delegation of tasks has to be well planned out.
Alan
Comment #13
Crell CreditAttribution: Crell commentedOne hunk fails. It's an easy fix.
However, the admin/user page is still set to "administer site configuration". That means you need that permission in order to get to any of the pages this patch affects, which gives you access to far too many extra pages. I'm not really certain what the best solution to that is. I'm not sure if it has to be handled in this patch directly, either.
Comment #14
scor CreditAttribution: scor commented"administer site configuration": It's the same problem for many other modules (node, contact, statistics, system).
The "assign roles" permission does not require "administer site configuration". (This feature was the original idea of this thread)
The "administer site configuration" permission grants access to the user administration pages. Wouldn't it make more sense to seperate the users from the site configuration? How about having a different permission for the admin/user pages called "access user administration pages"?
Comment #15
forngren CreditAttribution: forngren commentedSubscribe.
Will review if you reroll.
Comment #16
scor CreditAttribution: scor commentedThis patch separates the admin/user pages from the "administer site configuration" perm by using its own "access user administration pages" perm. The only perm required is "access administration pages" (which only grants access to the logs by default). Then you can choose the pages/permissions each role can access.
(patch against the latest HEAD 1.807)
Comment #17
Crell CreditAttribution: Crell commentedOK, I *think* this works correctly. Unfortunately, I just realized I can't test it without doing a full reinstall, because the menu system is currently broken and never rebuilds menu_links. I'm afraid I don't have time to do that right now, as I need to get to bed.
Code looks OK visually, and lacking a better idea I'm OK with the "access user admin pages" permission. Can someone else test this with a fresh install after patching?
Comment #18
forngren CreditAttribution: forngren commentedPatch works like a charm on a clean install. If someone can verify this, I think it's RTBC.
The attached patch is really them same patch with some cruft (i.e. Stripping trailing CRs from patch and eclipse workspace stuff) removed.
Comment #19
Wim Leers- Patch applies cleanly.
- No real code additions, just some "minor" changes, hence no new comments which have to be reviewed.
- Everything works as advertised.
Seems like a RTBC to me!
Comment #20
Dries CreditAttribution: Dries commentedMoving this to D7.
Comment #21
sunComment #22
Wim LeersWhy mark this as postponed? Marking as cnr because it's probably outdated by now.
Comment #23
birdmanx35 CreditAttribution: birdmanx35 commentedJust for the heck of it, this patch came up in the bingo. Tested against head, here's the failure message:
$ patch -p0 < 151311-user-perms.patch
patching file modules/user/user.module
Hunk #1 FAILED at 489.
Hunk #2 FAILED at 843.
Hunk #3 FAILED at 872.
Hunk #4 FAILED at 894.
Hunk #5 FAILED at 1493.
Hunk #6 FAILED at 1655.
Hunk #7 FAILED at 2345.
Hunk #8 FAILED at 2379.
8 out of 8 hunks FAILED -- saving rejects to file modules/user/user.module.rej
Comment #24
scor CreditAttribution: scor commentedwell, not surpised, that's an old patch that will need to be rewritten for D7, and some things like renaming 'access control' to 'permissions' went in D6 already.
Comment #25
sunComment #26
mattyoung CreditAttribution: mattyoung commentedsub
Comment #27
mr.baileysTagging.
Comment #28
DuaelFrWe absolutely need something like this!
Currently, there is only one permission to manage roles and permissions and to assign roles to users.
I think we could split that permissions as said above for 8.0.x then integrate something like Role Delegate in 8.1.x
Beware! I'm not sure this can still be included in 8.0. Do not start working on this before a proper beta evaluation is done.
Comment #29
DuaelFrComment #30
DuaelFrComment #35
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedAbsolutely, but I think a key is to delegate some tasks to a sub-admin without allowing them to gain full admin access.
It seems that any one of the new permissions would still be security critical and allow the user with it to gain any access they wished. For example with "Assign Roles" permission you can just assign yourself any role that you wish. So I'm not sure if this really gives much advantage.
On the other hand, in Drupal 8, the entity access API makes it absolutely possible to grant selective access to manage users with hooks. The module Administer Users by Role does just that - you define a set of safe roles and grant the sub-admin permission to manage users who have only safe roles and assign safe roles. However it won't work until we solve some bugs: #2921112: [META] User access control.
Plus as @DuaelFr points out there is the question of back-compatibility now we are at D8.4. The opportunity for Beta evaluation has passed:-) How would people feel about closing this as "Outdated" and focusing on getting the entity access API fully working?
Comment #36
DuaelFrI do not agree that this should be marked as outdated.
We had a recent example of another permissions-related issue that was commited (#1848686: Add a dedicated permission to access the term overview page (without 'administer taxonomy' permission)).
With Drupal 8.5, we still have the "Administer permissions" permission that allows to:
and the "Administer users" permission that allows to:
There is no need for a permission to edit their own account.
So, if we want someone to be able to assign people to existing roles, we need to give them the ability to create new roles and change their permissions. I feel these two actions does not need the same amount of knowledge and does not imply the same level of security.
I know that the caveat is that if you have an admin role with all permissions enabled, the person that can assign roles can assign this one to themself and gain more privileges. This is, for me, the next step of this issue: integrate Role Delegation in Core.
Finally, my proposal is:
Comment #37
DuaelFrComment #38
AdamPS CreditAttribution: AdamPS at AlbanyWeb commented@DuaelFr Sure, if a group of people want this change and can justify it to the committers then I'm not raising any objections.
See related issue #2846365: [regression] User roles field access is inconsistent, users with 'administer users' permission can gain full access hopefully will be committed soon.
ChangeUserRoleBase::access already calls $object->roles->access
Once that other issue has been fixed, the edit form will also call $object->roles->access
So what is needed here is to change
UserAccessControlHandler.php case 'roles'
:Administer Users by Role is another option and it arguably integrates more closely into core, working with the existing user edit form and existing role-based actions. Existing sites could already have coded yet other implementations of selected roll assignment using the entity access API. I guess my questions here are
Comment #39
DuaelFrRole Delegation
andAdminister Users by Role
are two really different modules. Maybe we should merge both :)Role Delegation allows to choose which roles another role can assign to users. For example, you can have a "Moderator" that can only assign "Trusted user" role to users and an "Administrator" that can assign the "Moderator" role.
Administer Users by Role allows to restrain the user accounts an admin can edit based on their roles. This module seem a bit strange to me as I can only think about an use case where you want to disallow user editing based on their roles. A Moderator should not be able to edit an Administrator but doing so in Drupal's aggregative roles and permissions system is counter intuitive to me.
IMHO, Role Delegation is much easier to integrate and is a more common use case than Administer Users by Role. It could live in contrib but when I see other granular permissions in Core (for nodes or vocabularies for example), I feel that it is something that should be part of the Core too.
Comment #40
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedYou have lost me!
The two modules were more clearly different in D7 but are getting closer now in D8. But RA creates a new role-edit form and AUBR uses the existing core forms/actions. AUBR is broader, it allows editing of users too.
I think if your follow on issue were to say "Core should provide a permission to assign each role" that could be a better way to say it rather than naming the specific module. If these permissions are in core, they need to fully integrate with core - they should affect the core role-assign actions and the core user form.
I guess it will be important to consider potential downsides of including this in core. The approach taken should consider the impact on sites that already have a different approach to the same problem. I wonder if there are sites with 100s or 1000s of roles who would get large numbers of unwanted permissions.
I'm not trying to be negative, I'm just saying think it through carefully, do it the right way, don't create a negative experience for a set of users.
Comment #41
DuaelFrLet's give a try to the simplest iteration of this issue.
Comment #43
DuaelFrWhat a stupid person I am :/
Comment #44
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedAs per #38 I think you need to take a good look at #2846365: [regression] User roles field access is inconsistent, users with 'administer users' permission can gain full access.
ChangeUserRoleBase.php - I don't see why you need to change this file and I think changing it will break existing sites that have a hook to alter access of 'edit'.
RoleAccessControlHandler.php - I don't think you need that file - you are not trying to change access to role entities but rather access to a field on user entities.
Comment #45
DuaelFrThanks for the pointer. I'll adapt my patch when the other issue will be commited.
The changes in
ChangeUserRoleBase.php
andRoleAccessControlHandler.php
are meant to have a minimal impact on the code to avoid side effects. We need to change the way the "Add XXX role to selected users" actions are restricted because it only relies on the role's edit access for now and that assignment and edition are totally unrelated for me.By creating an "assign" operation, we give to contrib modules a much more accurate way to override this specific action. (IMHO, we should also alter the roles checkboxes in the user edit form to use the same behavior but it'd raise more complicated issues)
The question to define whether the assignment access should be carried by the roles themselves or the field is interesting. I'd love to get more opinions on that particular subject.
Comment #47
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedTo avoid side-effects, don't change them or it will break existing code.
The existing code in ChangeUserRoleBase is checking field access for the role field on the user entity. It has got nothing to do with the role entity. Changing a user's role is a change to the user and not any change to the role. Just leave this code, it's fine.
I think you can easily test matters out - remove all of your code changes except AccountForm.php and user.permissions.yml, test what happens. I predict that:
Comment #51
mmbkOMG a twelve years old issue, that imho seems to be quite important...
I followed AdamPS' suggestion from #47, removing changes in the files ChangeUserRoleBase.php and RoleAccessControlHandler.php. Removed the test
/core/modules/user/src/Tests/Views/BulkFormTest.php, which does not exist in actual core anymore and prevented the patch #43 from being applyed.
Furthermore changed to user form to respect https://www.drupal.org/project/drupal/issues/2846365 by disabling the admin-role for non-admin-users.
Locally it's working as I expected, I'm curious what the tests say..
Comment #52
mmbkComment #54
mmbkComment #55
mmbkFixed the tests.
The tests of 'toolbar' module assign some roles to users via the user-form, so their admin-user needs the new permission.
I'm not sure how to handle tests in different modules, so I leave the changes here, as the changes are necessary because of our patch.
Comment #56
eelkeblokListing #2846365: [regression] User roles field access is inconsistent, users with 'administer users' permission can gain full access as related.
Comment #57
eelkeblokNice work, this indeed warrants resurrecting. At the very least the CR needs some tweaking because it still assumes this would have landed in 8.6. Yikes. I am actually not convinced this is the place to fix the admittedly crappy UX that the dropdown in the user overview shows actions you can not actually execute. That seems to be an issue in itself, though.
Comment #61
lexbritvin CreditAttribution: lexbritvin commentedReroll the patch for 9.2.x
Comment #63
berliner CreditAttribution: berliner commentedRe-roll the patch for 9.3.x and fixed CS issues
Comment #66
joshua1234511Tested the patch provided in #63
Appleid the patch on clean installation Drupal v9.5.x (Works cleanly)
Created a new role and user with (administer users roles)
Loged in as the new role user can assign roles to other users other then administrator role
Screencast:
Comment #67
joshua1234511Comment #68
catchThe issue summary doesn't match the patch here, it's not clear to me how this improves the situation compared to now, especially given the patch introduces workarounds for permission expectations.
This should probably be 'administer user roles'.
Without an upgrade path, this will prevent users who could previously administer roles from doing so. Probably need an upgrade path to add the new permission to roles that have 'administer permissions' already. Or if not, some way of notifying admins of the change (for example an update that lists the roles that will no longer be able to do this, without making any changes).
I don't think this is right. #2846365: [regression] User roles field access is inconsistent, users with 'administer users' permission can gain full access doesn't touch the account form at all so it wouldn't allow this to be removed. To me this seems like a flaw in the general approach and why permission and role management is coupled already - to prevent privilege escalation.
Comment #70
catchRe-titling as well.
Comment #72
berliner CreditAttribution: berliner commentedRe-roll the patch for 9.5.x and renamed permission from "administer users roles" to "administer user roles".