Support Permissions Lock

Hi Jason,

It sounds like your original question doesn't relate to the Role Delegation module. The idea of this module seems to be that you don't assign the "administer permissions" permission. Instead you use the Role Delegation module to say that they can assign certain roles... and the roles that you allow people to assign probably don't include admin.

It sounds like the Role Delegation module is what you need... but you're not actually using it. Or have I missed something in your question?

peace & happiness,
martin

I have users that need the "administer permissions" permission and to assign roles but they must not be able to assign the admin role to themselves or others.

Jason, this is not possible as you've stated it. "administer permissions" allows a user to create roles, assign permissions to roles, and assign roles to users. So once you grant someone "administer permissions", they can do anything on the site, either right away for the operations I just mentioned, or by first granting themselves the necessary permission.

What I think you're asking for is a new permission, say "administer permissions on delegated roles", that would allow a user to set the permissions for any role that he's allowed to delegate. To make this useful you would not grant "administer permissions", since that allows everything. Also, there would have to be a way to ensure that the admin couldn't grant any permission at all to the delegated roles, but only a subset determined by the sysadmin. For example, he might only be able to grant permissions that he himself has: see #491732: Enhance "administer permissions" control mechanism.

I agree that this would be a nice enhancement. It might not even be that hard-- newbuntu seems to have worked out some details at #491732.

BTW, re whether role_delegation is dead: it's not, but see #661622: Is role_delegation still being maintained?.

Well I'm not sure if I'd really call it an oversight. "administer permissions" is known as the sledgehammer permission for the reasons I've mentioned. Role delegation just obeys its traditional meaning: if a user has "administer permissions", then role delegation lets him assign any role.

It would be simple to remove that condition, but it would change the behavior of role delegation in some probably unexpected ways. Maybe a new permission, "administer roles", could be introduced to allow assigning all roles, and no longer allow "administer permissions" to do it.

I'll take a look at Permissions lock, and think about this some more. I see the problem you're describing, and I agree that there should be a solution, but I'm not sure yet what the best solution is. Proposals are welcome.

Andrew.

Maybe a new permission, "administer roles", could be introduced to allow assigning all roles, and no longer allow "administer permissions" to do it.

Sorry, there is of course already an "assign all roles" permission. Maybe really all that's needed is to remove the assumption that "administer permissions" also implies "assign all roles". Again I need to think about that.

@Andrew: You have explained the point every well (thanks!). Role delegation just follows the logic that if a user can administer permissions (thus create and edit roles), then it would be counter-intuitive to prevent that user from assigning some roles.

@jasont28: Your request seems to be a valid one, but I don't feel that those use cases should be handled by Role Delegation, especially if the Permissions Lock module already does the job. I'm very much tempted to set this issue as "won't fix", unless perhaps if Role Delegation happens to not play nice with Permissions Lock.

The Permissions Lock module can limit what roles are available to users with the "administer permissions" permission. It would be nice if Role Delegation honored Permissions Lock's limits.

I can see two possible ways of handling this.

1. Completely remove the tests for 'administer permissions'. The first patch below does this. With this approach, 'administer permissions' no longer grants the ability to assign all roles. The 'assign all roles' permission will have to be granted separately for that.

   The advantage of this approach is that it makes Role delegation automatically work with either Permissions Lock or any other module that restricts the power of 'administer permissions'. The disadvantage is that at sites that aren't using Permissions Lock, 'administer permissions' will no longer be a complete administrative right-- the admin will have to first assign 'assign all roles' to a user, before that user can assign all roles.

2. Change the test for 'administer permissions' to apply only if Permissions Lock isn't present. The second patch below does this. This approach only supports Permissions Lock, so other similar modules would have to be accomodated separately. But it doesn't change the meaning of 'administer permissions' for sites that don't have Permissions Lock, which might be considered an advantage.

Personally I prefer #1. The change isn't really that big for non-Permissions Lock sites, and can be easily documented.

Either way, I think it would be a good idea to provide this feature. There's been a lot of discussion that 'administer permissions' is too broad, and it would be useful to support sites that want to limit it. In fact, I'd like to see Role delegation expand at some point into Role and Permissions Delegation, to provide a complete solution-- but that's for another day. At least this would be a step in that direction.

Thanks for the excellent analysis.
#1 seems a reasonable solution in comparison to a module-specific solution like #2.

I have checked the patches only very quickly (for now). I'm not sure what happens after removing the user_access('administer permissions') check in role_delegation_user_operations(). I'm afraid that this change will cause duplicate add/remove role operations being added (those of core and those of Role Delegation) when the user has the 'administer permissions' permission. Perhaps Permissions Lock does take care of this (I have not tested), but for sites without it, it looks like we'll have duplicates for sure.

This issue hasn't been updated in over a year, therefore I've developed a working patch for this issue since it looks like active work on this module has stalled.

This patch effectively applies the 1st patch mentioned by Andrew (in post #12), but also solves many integration issues with the back-end as raised by David (in post #13). This patch treats "assign all roles" as the 'god' permission when it comes to permissions and roles, whereas Drupal's "administer permissions" is only used to allow access to the permissions page within the admin area.

This patch, when applied, will restrict the user's permissions page to only show the roles the user may delegate, as well as only show the "assign ROLE role" permissions for those roles. The "Permissions Lock" module may then be used to further restrict which permissions the user sees, but is not required for this to work.

dgersting, thanks for posting the patch. Work on this module hasn't stalled, but providing a patch is a good way to get work moving on this particular issue.

The issue is a bit complex to reproduce and test. But I'll look at your patch within the next week and see if it seems to work right.

Subscribe

@dgersting Hi, I tried your patch and it gave me:

warning: in_array() [function.in-array]: Wrong datatype for second argument in /var/www/website.ltd/sites/all/modules/role_delegation/role_delegation.module on line 356

This warnig is when user Tester (is not user 1) with 'administer permissions' permission go to 'admin/user/permissions'. The user has not 'assign all roles' permission. The $removedRoles is NULL

I tried the 1.4 and 6.x-1.x-dev-2011-Jun-17

Edit: If I declare $removedRoles = array(); then the warning is away

thanks M.

Quick and dirty D7 patch (against 7.x.-1.1 version).

I have tried dev and I don't have this problem...

The problem is still present in dev, I've checked the permission "Administer permissions" and on the user account I can assign the role "Administrator" to anyone = not secure!

Patch at #19 did work for me against 7.x-1.1. THKS

It’s been a while since the last comments in this issue. If this is still a problem feel free to reopen and provide patches.

#19 has not been commited yet. Please review.