Let's say you have an "administrator" role (super-admin role) and a "sub-administrator" role. The "sub-administrator" role should be allowed to assign any role except for "administrator". Well, give them the ability to assign roles at all, you need to also give them the "administer users" permission. But this permission allows them to edit/delete all users (except user 1).... so while they wouldn't be able to remove the "administrator" role from other users, they could easily just edit their username or password or delete their account instead.

At the very least, I think a prominent warning should be displayed about this, both on the module info page and the README.

Comments

bkosborne created an issue. See original summary.

salvis’s picture

Title: Add warning that users with "administer users" permissions can still manage users that have a more priviledged role » Users with "administer users" permissions can manage users that have a more privileged role
Component: Documentation » Code
Category: Feature request » Bug report
Priority: Major » Critical

That is a regression from D7 that needs to be fixed!

Thank you for reporting this!

salvis’s picture

Assigned: Unassigned » salvis
jenlampton’s picture

It sounds like this is the problem...

give them the ability to assign roles at all, you need to also give them the "administer users" permission

If you were willing to give them permission to "administer users" then there is no need for this module. The whole point of the permission to assign roles is so that the "administer users" permission does not need to be granted.

salvis’s picture

Well, no. We do want to give 'administer users' to off-load some work of the main administrator to deputy administrators. The purpose of RA is to keep the deputy administrators from assigning themselves (or someone else) roles that the main administrator wants to keep for himself.

If the main administrator is user 1, then there's no problem, because FA protects user 1. However, if we have other administrators with more power than the RA-restricted deputy administrators, then those are not protected.

I have an almost working solution for this by comparing the sets of permissions, but then I found that
a) this may be too restrictive and should be optional and
b) this may not be restrictive enough, because there may be other privileges connected to roles than just permissions

That's where I got side-tracked by other projects and haven't been able to pursue this anymore...

There are two bigger questions here:
1. Should we limit the use case for RA to just user 1 and one level of deputy admins or can we find a strategy that covers more complicated setups? In the light of b), having more than one level of admins (besides user 1) may not be practical without an extensive configuration machinery.
2. Do we need to make RA absolutely bullet-proof, i.e. do we expect to appoint malicious deputy admins that may try to hack even at the risk of ultimately being discovered (like deleting or hijacking other accounts, which will not go unnoticed)?

tame4tex’s picture

Personally, I feel contrib modules become problematic when they become too "multi-purpose". For me, I see RoleAssign as a module to restrict what roles administrators can assign, not necessarily a module that restricts what users an administrator can edit.

We use https://www.drupal.org/project/userprotect in conjunction with RoleAssign to meet the needs you are discussing. Why not just recommend UserProtect or (if possible) add it as a dependency and dynamically create a userprotect rule based on what roles are chosen.

It would be great to see an alpha3 version soon and I would be more than happy if it just came with a strong recommendation on the module info page to use UserProtect.

salvis’s picture

Thank you for your comment and the recommendation of userprotect, tame4tex!

Between a) and b) in $5 above I think making userprotect a dependency would be going too far, but adding a specific recommendation is a good idea. In the end it boils down to every site evaluating their specific set-up and determining what kind of protection they need.

tame4tex’s picture

Sounds like a great plan! And thank you for your efforts on a great and very useful module!

alexverb’s picture

Is this issue still a blocker for a stable release? https://www.drupal.org/project/roleassign/issues/2953329#comment-12584329
I can't really tell from the last few comments if this is desired behavior or not. The project description also explains:

This module solves this dilemma by introducing the Assign roles permission. While editing a user's account information, a user with this permission will be able to select roles for the user from a set of available roles. Roles available are configured by users with the Administer permissions permission.

Key words: While editing a user's account information,

Doesn't this imply that the administer users permission should be granted to allow the assignment of roles?

developerchris’s picture

salvis’s picture

@DeveloperChris: Thank you for illustrating your comment with screenshots. This is very helpful! However, this has nothing to do with the current thread.

This effects D7 so I am assuming its the same in D8

No, please don't assume. Please remove your comment (edit the comment and remove the text and pictures) and open a new issue with this. Answering your post here will cause a lot of confusion.

salvis’s picture

@DeveloperChris: Thank you for removing your off-topic comment. As I wrote in #11 above, please feel free to start your own topic if the issue is still relevant.

salvis’s picture

@alexverb:

Doesn't this imply that the administer users permission should be granted to allow the assignment of roles?

Indeed, yes. If the deputy administrator cannot assign roles (by administering users) there's no point in trying to limit the roles that they can assign.

salvis’s picture

Component: Code » Documentation
Assigned: salvis » Unassigned
Priority: Critical » Major
Status: Active » Needs review

I've added a first shot for the "Please note!" section to the front page.

Let me know what you think — feedback is welcome!

cedewey’s picture

Status: Needs review » Reviewed & tested by the community

Hi salvis,

The important note you added to the project page reads well to me. Thanks!

joseph.olstad’s picture

Status: Reviewed & tested by the community » Fixed

This was resolved 3 years ago

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.