The project page states:

If the user-being-edited has multiple roles, the user-doing-the-editing must have permission to edit ALL of the user-being-edited's roles.

However the "XX and other roles" permission for just one of the roles in question is sufficient to get access. These permissions seem rather dangerous. For example, suppose I have an admin user, with admin role. I then give the admin user a second role 'slightly-trusted' (perhaps as part of a bulk update of roles). This would immediately open up full admin access to anyone who with "Edit slightly-trusted user and other roles".

I would say the purpose of this module is to provide an easy and safe way for developers to create subadmins with restricted edit access. These permissions seem to break this.

  • Casual users coming to the module for the first time might get mixed up and set "XX and other roles" with they really meant just "XX".
  • Even experienced users can get caught out as to prove that the "XX and other roles" permissions are safe requires examining the roles of every single user in the site. What's more any future change of role can break the safety.

Or in a nutshell, I claim that adding a low-permission role to an already admin user should not be something that can totally break your site's security.

As the new maintainer, my view is that the only safe solution is to abandon the "and other roles" idea entirely as insecure. I will listen carefully to your views, but please bear in mind it would be most useful to convince me that there isn't a security problem, rather than just saying that you really want this feature :-).

Comments

adamps’s picture

Title: "XX and other roles" permission can give access when it shouldn't » "XX and other roles" permission can give admin access by accident
Issue summary: View changes
adamps’s picture

Issue summary: View changes
adamps’s picture

Issue summary: View changes
adamps’s picture

Assigned: Unassigned » adamps
Status: Active » Needs review
Issue tags: +beta1
adamps’s picture

Issue tags: -beta1 +beta2
ptsimard’s picture

Totally agree with dropping those permissions. Our system has many roles of variable access level and I found them very dangerous. At first they tripped me up. I made the decision to never use these and hide them in our system.

I totally approve this suggestion.

  • AdamPS committed 34b1116 on 7.x-2.x
    Batch of changes for beta2 release
    
    There are too many interlocking...
adamps’s picture

Version: 7.x-1.0-beta1 » 7.x-2.0-beta1
Status: Needs review » Closed (fixed)

Fix now available in latest release