Bonfire
Bonfire copied to clipboard
lonnieezell
Managing Role Permissions
The Role Permissions management is not working as was originally intended so I think now is a good time to gather requirements for this and to make the necessary changes.
The intention was to have a permission for each Role which would allow editing the permissions for that Role eg an Editor role could edit the permissions for a SubEditor role.
Giving the Editor access to manage the permissions to the SubEditor also requires that they get access to edit/delete any role. Not Good.
Here are some suggested requirements/changes:
- Site permissions can only be assigned by the Admin
- Define a list of core permissions which can only be assigned by the Admin
- Rearrange the permission requirement - working on this now
- Allow a Role (Role A) with access to assign permissions to another role (Role B) to only be able to assign permissions to Role B that Role A already has. (This does make sense to me now @hailwood :-) )
I am about to submit code which will improve the permission checking in the roles module.
I'm marking this issue as 0.7 because I think the changes I'm making now fix some of the main issues for now.
//cc @lonnieezell //cc @svizion //cc @hailwood //cc @mwhitneysdsu - What do you think?
You know, I haven't looked at the permissions in depth since you revised them that last time, but I know I ran into some issues when I updated a project. I fixed it by hand in the DB at that point, but at some point, seeing an overall listing of what permission scheme is being checked for where might be handy.
Otherwise, this sounds pretty good. I really like the 4th part, also, except that if an admin accidentally unchecks the wrong permission... at that point they're pretty much screwed, unless we have some other way around that. I know that I've had times when the mouse battery is going out, or the arm is bumped by the family cat, etc, where I click something that I didn't mean to.
@lonnieezell Yeah, The admin needs to be a special role, The idea of an admin role is that they have 100% control over the system right so you would never take a permission away from an admin role? so having the ability to do so seems a wee bit like bloat to me. and not being able to works well as a safe guard, as they can never get into a position where no one can assign a particular permission.
@seandowney With regards to the list of core permissions, isn't that what the site permissions already are?
@hailwood By "core" I mean some of the "Bonfire." permissions for the core modules for example Roles, Permissions Users. So I'm proposing the Admin would only be able to assign access to those permissions for the likes of those core modules. We could just say all modules under the Settings and Developer contexts.
It sounds pretty good honestly. I don't really think users that have a permission should be able to remove it and if user A doesn't have a permission he shouldn't be able to assign it to someone else.
On the core permissions, I personally think it should be Admin and Developer just because I have to maintain a lot of site's that I give the owner Admin status but they don't really know what the permissions so I normally remove the settings and developer context's from the Admin role so they can't mess up things to badly. But that's just my personal feelings on it, if we used a set ID for the Admin role I could always just rename the 2 roles.
This sounds good to me as well. The permissions is one part of the system that drew me to Bonfire in the first place, but which I still have not had time to explore in depth.
I think with the "core" permissions idea, i.e. only having certain users able to assign those permissions, it would be best for it to be a permission level assigned to Admin by default, but which we could assign to other roles (i.e. Developer as @svizion mentions above). I can definitely see many use cases where the Admin is not the best person in which to install the highest abilities, even though it's the best role in which to install those privileges by default.
Yeah the tricky part is how to know which roles should have those permissions. I hate using IDs but Admin should always be role id 1.
If you can suggest a good way to implement what you need please post it here. The great thing with the roles is that they can be renamed, as @svizion said, which can give someone the illusion that they are "Admin" ;-)
yea I haven't really come up with a better method then using the id either honestly, that's kinda why I was thinking on just renaming the Role names around would be easy and work for most people.
Could always set a permission that is essentially 'Is admin'. But have the permission not show up to anyone who is not an admin? Allows us to easily define which roles are admins, and the fake admins won't know any difference?
@hailwood that's a fairly good idea on managing it, I know I have a ton of modules out there with ids hard-coded into them for Developer's ID == 6 kinda stuff, at least with your method you could easily add more roles to it
But then what does the setting actually mean?
If you can define what the requirements would be then we can work from there.
maybe this is a good time to take a fresh look at permissions in general? Seems like we're kinda doing that anyway.
I know that the permissions we have are necessary in many ways, but is there a way to clean them up and minimize the amount needed and still get the same thing happening?
Another idea I've toyed with is adding 'keys' that can be assigned to any user for more fine-grained access, like for Beta Keys, or sub-accounts under a main account. Haven't thought about that too much, but it is one place where we fall short. And not sure I like the idea of a full-fledged ACL due to complexity... but the sheer number of permissions we're getting is making things complex anyway.
Any thoughts?
If I understand correctly, the setting would essentially replace the current check against the admin role_id. Basically, if you have that setting, you would have the ability to change all of the permissions, regardless of what other permissions you have set on that role.
On the topic of cleaning up permissions, I would like to add multiple roles to a user, so I could apply roles as groups of permissions, rather than having to manage more roles with overlapping permissions. For instance, all of our faculty will have one set of permissions, but each division will also have a set of permissions specific to them, so currently I need to setup a role for faculty within each division, staff within each division, administrators (not site admins) within each division, etc. So with (for example) 5 divisions and the 3 groups mentioned, we would currently need 15 roles.
If I instead could apply a role for the division and a role for the group, I would have 8 roles to handle the same group of users.
Some other means of grouping permissions would be equally welcome. I'm currently working on a rather large module which I'm building by combining a number of smaller modules I generated with the Module Builder, and my Permissions Matrix on my development box looks like an abused dartboard.
@lonnieezell @mwhitneysdsu Yes I think there is definitely merit in looking at the permissions again.
The aim should be to make it robust enough to be useful, but at the same time uncomplicated and extendable. Not much to ask eh !!
Assigning multiple roles to users is probably the easiest way to do this and then on login combine the permissions of all roles of that user.
What do you think?
//cc @jfox015 Do you think that would satisfy the requirement for #473 ?
@seandowney Robust, Uncomplicated, Extendable... Something in my head is screaming the old "pick 2..." mantra, but I don't think that'll be necessary.
In the end I think this discussion has brought about a number of good ideas, and hopefully many of us have found ways to handle our own issues whether or not the permissions system changes in any significant way. I'm going to have to spend a bit more time looking at the existing system before I can guess at what might meet those requirements.
@seandowney @mwhitneysdsu @hailwood I'm all for simplicity. I guess for my scenario I could use roles to assure a user is in a certain enhanced class and then run a proprietary test to confirm membership in a certain group (specific league in my case). Again, I have to ask if a more meta based groups type of assignment/filtering/permissioning warrants any discussion or adds undue/unwanted complexity.
@jfox015 I'm not sure what you mean by the meta based solution.
Could you explain it a bit more?
@seandowney Sure. I was thinking of something along the lines of the groups that Ion Auth 2 uses.
Roles to me defines what type of actions the user can take within and applications.Membership within the Developer role gives you access to perform certain type of actions.
Groups defines more of a content based membership definition. It could be used to restrict access to either view or contribute to content areas. For instance, if you are a member of a "PHP Coders" group, you can post messages to the group with a form. If you are not, you have read only access.
@jfox015 Would this replace the permission system as it stands now or be an addition to it?
Could you give examples of the data in the Roles and Groups and how they would be checked for access to the different areas?
Sorry for these questions - I even wrote a custom permission system with actions and access levels for a bespoke system but I can't think at the moment !
@seandowney On a side note, Instead of using a hard coded value for the Admin ID == 1 what about using a constant to define what ID you want to give that permission to, for some reason I have a older bonfire site and my Admin is ID 5 and Developer is 6. Just something I noticed when I was updating a very old Bonfire site
@svizion yeah that could happen. Using a constant is better and we should do that. Someone could still mess with the roles and then they would have to remember/know to update the constant. That would be their issue for messing with the Admin roles :-)
Yea I'm thinking a Constant would be way better then Hard Coding the Role number in. In the Last CMS I created I added a simple helper method to check it but that's probably over kill checking a constant would be faster and less code needed, if your good with it, I'll add one in and replace the checks that use it, I was thinking just ADMIN_ID would be a good easy to understand constant name for it
Support for ADMIN_ID to be an array as well could be useful? Actually, can constants be arrays?
Oh and on note, to me it sounds like a one role, many groups would be the way to go?
Clearly there would be more overhead than a constant defined in the same file that uses it, but maybe a config entry that accepts an array of IDs.
@seandowney So Config entry with a in_array kinda thing for the Role Checking instead of hard coding == 1 or what nots?
If we are going to do that would it be better to have a setting on each role for Admin or not?
We would need a db query to get them but they could be cached for ages and cache removed if a role changes - added, updated or deleted.
What do ye think?
@lonnieezell @jfox015 @hailwood @svizion @mwhitneysdsu Any further thoughts on this?
I think the first time I read it I didn't really understand the question. I think I like the idea of setting a flag on each role, I'm just not sure how that impacts performance. At the very least I'm guessing you could pull the list of flag values from the database in one query and cache it as a bitmask or something along those lines.