user-admin-[role] permission gives edit access to users with other roles

Short description of the issue

As I understand it from reading the Roles documentation, roles are intended to be cumulative. A user can have multiple roles, and this means that they can be given limited permissions by having one role and then greater permissions by being given another role.

So I can have a "member" role that is given to front-end users with some special view permissions, and an "editor" role who is an admin user who can edit site content. And I can give the member role to a user with the editor role - they can be both a front-end member and an admin editor.

This is all good, but the user-admin-[role] permission doesn't properly limit edit access to users that have multiple roles. To illustrate...

1. Besides the "member" and "editor" roles mentioned above I create a "membership-admin" role.

2. I install the predefined system permissions: user-admin-all, user-admin-editor, user-admin-member, user-admin-membership-admin.

3. I give the "membership-admin" role these permissions: 2020-03-23_125205 The intention here is that this role has permission to edit users with "member" role and only the "member" role (plus guest). They are not supposed to be able to edit admin users with the "editor" and/or "membership-admin" roles. Incidentally, I don't see why "guest" is a role at all instead of being defined as the absence of all roles - I think things would be clearer that way.

4. I create some users: 2020-03-23_125329 Note that the users with the "editor" and "membership-admin" roles also have the "member" role - this is because they are entitled to the front-end view permissions in addition to their PW admin permissions.

5. The problem is that a user with the "membership-admin" role can now edit other users with the "membership-admin" and/or the "editor" role despite not being given the user-admin-editor or user-admin-membership-admin permissions.

Expected behavior

user-admin-[role] permissions should be conservative by default. If a user has multiple roles then they should only be editable by another user who has all the necessary user-admin-[role] permissions, not just the permission for one of the roles possessed by the user.

Actual behavior

User A with the user-admin-[role] permission can edit any User B with [role], regardless of how privileged User B is in terms of additional roles User B has.

Setup/Environment

ProcessWire version: 3.0.153

@Toutouwai - I just had a quick read - in the middle of other things, but here is a slightly related issue in terms of the guest role: https://github.com/ryancramerdesign/ProcessWire/issues/588

@adrianbj, thanks, I absolutely agree with the comment by @ryancramerdesign:

Guest role probably shouldn't be displayed anywhere, except in the Roles editor and in the Template Access tab. Anywhere else, it means absolutely nothing and is a waste of screen real estate. Guest role is assumed for all users regardless of whether it's assigned or not. There is no reason to ever do a hasRole("guest"). So I'd prefer to move towards storing and showing less useless data rather than more useless data. :) Guest was not always a default assumption like it is now, and that's the only reason you see it anywhere... but we should move towards elimination of guest in 2.6.

It just has to be implemented in the core :-)

@Toutouwai I understand what you are saying, but what you are suggesting isn't how the user-admin-role permissions are intended to work. When you give someone user-admin-role permission, then it means they have the ability to administer any users having role "role", regardless of what their other roles might be (excluding superuser). I think what you are suggesting also could be useful, but you'd likely want to use a hook for that. Maybe something like this?

$wire->addHookAfter('User::editable', function($event) {
  if(!$event->return) return;
  $userToEdit = $event->object;
  $currentUser = $event->wire()->user;
  $permissions = $event->wire()->permissions;
  foreach($userToEdit->roles as $role) {
    if($role->name === 'guest') continue;
    $permission = $permissions->get("user-admin-$role->name");
    if(!"$permission" || !$currentUser->hasPermission($permission)) {
      $event->return = false;
      break;
    }
  }
});

@ryancramerdesign, thanks for the suggestion. But the hook doesn't seem to have any effect. Even if the hook is...

$wire->addHookAfter('User::editable', function($event) {
    $event->return = false;
});

...the users who are listed and editable in ProcessUser is unchanged.

And to be clear: it's not just the editability I want to restrict - if a user isn't editable to another user then I don't want them to appear in the Users lister either (I don't want to leak user data such as email address, or even the very existence of a user, to somebody who shouldn't be able to edit that user).

My experience is that the way that the user-admin-[role] permissions currently works makes the idea of cumulative roles unworkable for any site where users need to be editable by anyone other than a superuser. For sites where a person has more than one role (e.g. they are both a "member" and an "administrator") I have had to abandon the idea that they can have a single user account that is given two roles. Instead I have had to create two user accounts per person, with each account having only one role. This seems to be the only thing that works, but it's really not a good outcome because it means one account cannot have an email address for password recovery or else there would be email address clashes across the two accounts.

Do you think PW could change to operate in the way I expected it would?

user-admin-[role] permissions should be conservative by default. If a user has multiple roles then they should only be editable by another user who has all the necessary user-admin-[role] permissions, not just the permission for one of the roles possessed by the user.

This would make multiple-roles-per-user viable, and anyone who wants the existing behaviour can still achieve it by adding the extra user-admin-[role] permissions to any role that needs them.

@Toutouwai Sorry, I don't think there is a User::editable() hook, I wasn't thinking. It would be a Page::editable() hook where you check either the template or the object type, as you would with any other kind of page:

$wire->addHookAfter('Page::editable', function($event) {
  $page = $event->object;
  if(!$page instanceof User) return;
  $event->return = false;
});

I'm assuming that "administrator" is something different from superuser, since that's a special role outside user-admin permissions. For your access control scenario, I wouldn't suggest having combinations like "member" and "administrator" for the same user because they appear to be opposite user roles. I'm assuming you have this because you want to do something like if($user->hasRole('member')) { /* allow this */ }. While you could add more roles to that statement, what you may really be asking is if the user has permission to go somewhere or do something that both members and administrators have permission to do: if($user->hasPermission('order-pizza') { /* allow this */ }. (Both member and administrator roles have order-pizza permission).

I think it's more common to mix roles for a type of users (like types of front-end users) than it is to mix administrators into your front-end user roles. For instance, all users that register on processwire.com get the login-register role. But there are 4 other roles they can also get depending on the type of user they are. Permission user-admin-login-register applies to all of them. What you are proposing (if I understand correctly) is that it would take 5 different permissions to accomplish what is currently managed with 1.

I know it depends on the case and think I understand what you are suggesting, and how it might be preferable for your own case. But unless I'm missing something, the current permissions work how they are supposed to. If you find a hook that accomplishes what you need and want to share it, I'm open supporting the option as a setting that enables the alternate behavior that better fits your need. Or maybe a checkbox toggle in the permission editor, or something along those lines.

Thanks @ryancramerdesign. I can achieve what I want with a couple of hooks - one being a slightly modified version of what you suggested.

// User is only editable to another user if they have all the relevant user-admin-[role] permissions
$wire->addHookAfter('Page::editable', function($event) {
    if(!$event->return) return;
    $user_to_edit = $event->object;
    if(!$user_to_edit instanceof User) return;
    $user = $event->wire()->user;
    // $user_to_edit is editable when $user has user-admin-all permission
    if($user->hasPermission('user-admin-all')) return;
    // $user must have the user-admin-[role] permission for every role held by $user_to_edit
    foreach($user_to_edit->roles as $role) {
        if($role->name === 'guest') continue;
        if(!$user->hasPermission("user-admin-{$role->name}")) {
            $event->return = false;
            break;
        }
    }
});

// User is only visible in ProcessUser lister to another user if they have all the relevant user-admin-[role] permissions
$wire->addHookBefore('ProcessPageLister::execute', function(HookEvent $event) {
    /* @var ProcessPageLister $lister */
    $lister = $event->object;
    $user = $event->wire()->user;
    // Only for non-superusers
    if($user->isSuperuser()) return;
    // Only for Users lister
    if($lister->parent->id !== 29) return;
    // Determine which roles are not allowed to be edited by this user
    $custom_roles = $event->wire()->roles->find("name!=superuser|guest");
    // Superuser is always non-editable
    $non_editable_roles = [38];
    foreach($custom_roles as $custom_role) {
        if($user->hasPermission('user-admin-all')) continue;
        if($user->hasPermission("user-admin-{$custom_role->name}")) continue;
        $non_editable_roles[] = $custom_role->id;
    }
    // Parse the ProcessUser initSelector into Selectors object
    $selectors = new Selectors($lister->initSelector);
    // Remove any existing Selector objects where the field is "roles"
    $selectors->removeItems($selectors->find('field=roles'));
    // Add to $selectors to exclude non-editable roles
    $selectors->add(new SelectorNotEqual('roles', $non_editable_roles));
    // Set the ProcessUser initSelector
    $lister->initSelector = (string) $selectors;
});

So that solves it for me (edit: something more might be needed for the Users flyout menu when a lot of users are grouped by role), but I still think the guidance given by the Roles documentation...

A user can have multiple roles, each with different permissions. ...you may assign as many roles to a user as you need, and the user will gain the access and permissions assigned to all of the roles they belong to.

...becomes quite impractical as soon as you need to make certain roles editable by non-superusers.

While you could add more roles to that statement, what you may really be asking is if the user has permission to go somewhere or do something that both members and administrators have permission to do: if($user->hasPermission('order-pizza') { / allow this / }. (Both member and administrator roles have order-pizza permission).

The way I think of roles, they're about more than just permissions. If I'm creating a site for an organisation that has a membership, it's not just that members are allowed to view/edit X and not Y, it's that I want to send an email to all members, or I want to print subscription invoices for all members.

I think it's quite typical for an organisation to have a broad membership (role = member) and within that membership some individuals also occupy a more privileged position (role = content_editor, or role = membership_administrator). But it shouldn't be either/or. They should be able to occupy the role of member and membership_adminstrator simultaneously - that's what I take the Roles documentation to be saying.

To give a fictional expansion on the example you gave of the processwire.com site... Suppose there is a role module_author and there are lots of authors and for some reason they're always changing their email addresses and forgetting their passwords. So you create a new role module_author_admin so volunteers can give you a hand to update email addresses and reset passwords. This is a role without any editing access beyond module authors so you're not too worried about giving it to enthusiastic newcomer Jim. But the catch is that there's a very trusted person Charlie who is a module author and also has the highly privileged role homepage_editor. This creates a big risk because if Jim goes rogue he can edit Charlie's account, set his own email and password, and then login and deface the homepage.

But if Charlie therefore can't have both module_author and homepage_editor roles then he has to have two different user accounts. Or maybe more than two accounts if there are many roles with quite granular template/field access. And these accounts can't have the same email address. It gets quite messy.

What you are proposing (if I understand correctly) is that it would take 5 different permissions to accomplish what is currently managed with 1.

True, but it takes 30 seconds to tick some more user-admin-[role] checkboxes and saves all the headaches described above. It also avoids future gotchas where a new extra-privileged role can effectively "leak" to those with lower-level privileges. The way I see it, if I assign a new, more privileged role to a user I am upgrading that user and therefore I want to consciously set who can edit (and thereby potentially seize) their account.

If you find a hook that accomplishes what you need and want to share it, I'm open supporting the option as a setting that enables the alternate behavior that better fits your need.

If the ideas in the hooks above look okay to you maybe that functionality could be activated by a setting in ProcessUser?

I'll admit I haven't fully digested everything @Toutouwai is suggesting here, but I know I've certainly come up against situations where I can't easily get the granularity I am looking for, so I support improvements on this front.

processwire / processwire-issues