Split 'administer permissions' into a new administer roles permissions

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

+1 on this. For the rename, "administer access configuration" vs. "administer user roles"?

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

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

- +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"?

+1 great idea.
Certainly useful for larger sites where delegation of tasks has to be well planned out.

Alan

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

Subscribe.

Will review if you reroll.

OK, 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?

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

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

Moving this to D7.

Why mark this as postponed? Marking as cnr because it's probably outdated by now.

Just 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

sub

Tagging.

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

it becomes essential to be able to delegate some tasks

Absolutely, 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?

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

• create, delete and edit roles
• assign permissions to roles
• assign roles to users

and the "Administer users" permission that allows to:

• access the people overview
• edit other user accounts

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:

1. rename "Administer permissions" to "Administer roles and permissions" for clarity
2. create a new "Manage users roles" and replace the call to "Administer permissions" in the user edit form and the people overview bulk actions
3. add an upgrade path that adds the "Manage users roles" to all roles that used to have the "Administer permissions" permissions
4. write upgrade path tests
5. write a change record
6. open a follow-up to merge Role Delegation into the Core

@DuaelFr Sure, if a group of people want this change and can justify it to the committers then I'm not raising any objections.

2) replace the call to "Administer permissions" in the user edit form and the people overview bulk actions

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':

open a follow-up to merge Role Delegation into the Core

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

• What is the problem with Role Delegation being a contrib module that those who want it can install?
• How would you justify a specific implementation of selective roll assignment as the one that everyone must use? Arguably the whole point of the entity access API is to allow different sites to chose different solutions.

Role Delegation and Administer 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.

You have lost me!

• Starting with 8.x-3.x Administer Users by Role allows to choose which roles another role can assign to users.
• In all versions, Administer Users by Role allows to choose which roles another role can edit users for.
• So you just define your safe roles, and the sub-admin/Moderator can manage the users within that set, editing or assigning roles.

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.

Let's give a try to the simplest iteration of this issue.

What a stupid person I am :/

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

• Prior to that issue, you need to change AccountForm.php
• After that issue, you would need to change UserAccessControlHandler.php

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.

Thanks for the pointer. I'll adapt my patch when the other issue will be commited.

The changes in ChangeUserRoleBase.php and RoleAccessControlHandler.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.

The changes in ChangeUserRoleBase.php and RoleAccessControlHandler.php are meant to have a minimal impact on the code to avoid side effects.

To avoid side-effects, don't change them or it will break existing code.

it only relies on the role's edit access for now

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:

• A user with 'administer users' permission and no other can run assign role actions - that's the bug of #2846365: [regression] User roles field access is inconsistent, users with 'administer users' permission can gain full access. Once that bug is fixed you will need to change UserAccessControlHandler (not RoleAccessControlHandler).
• A user with 'administer users roles' can set roles on the account form.

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

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

Listing #2846365: [regression] User roles field access is inconsistent, users with 'administer users' permission can gain full access as related.

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

Reroll the patch for 9.2.x

Re-roll the patch for 9.3.x and fixed CS issues

Tested 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:

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

1. +++ b/core/modules/toolbar/tests/src/Functional/ToolbarAdminMenuTest.php
   @@ -88,6 +88,7 @@ protected function setUp(): void {
          'administer users',
   +      'administer users roles',
          'access user profiles',
   

   This should probably be 'administer user roles'.

2. +++ b/core/modules/user/src/AccountForm.php
   @@ -205,7 +205,7 @@ public function form(array $form, FormStateInterface $form_state) {
          '#default_value' => (!$register ? $account->getRoles() : []),
          '#options' => $roles,
   -      '#access' => $roles && $user->hasPermission('administer permissions'),
   +      '#access' => $roles && $user->hasPermission('administer users 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).

3. +++ b/core/modules/user/src/AccountForm.php
   @@ -214,6 +214,16 @@ public function form(array $form, FormStateInterface $form_state) {
    
   +    // Prevent non-admin users from assigning the admin role to anybody.
   +    // Remove, once https://www.drupal.org/project/drupal/issues/2846365
   +    // is resolved
   +    if (!$user->hasPermission('administer permissions')) {
   +      $form['account']['roles']['administrator'] = [
   +        '#default_value' => FALSE,
   +        '#disabled' => TRUE,
   +      ];
   +    }
   +
   

   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.

Re-titling as well.

Re-roll the patch for 9.5.x and renamed permission from "administer users roles" to "administer user roles".