Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Support for capabilities module #52

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

takkaria
Copy link
Collaborator

This is the work-in-progress that I am doing to integrate the group and user_capability modules.

@cagabi
Copy link
Collaborator

cagabi commented Sep 4, 2018

Ann, this is great!

There is one issue to take into account before doing the merge: there are two permissions systems in place: the capabilities and the original "Administrator, Sub-administrator, Passive member".

In my opinion the capabilities module is the one to keep. This means:

  • We need to get rid of all the permissions checks that happen everywhere. Like replacing all the calls to $this->is_group_admin($groupid, $admin_userid) with a simple check that the user is in the group. For example you can see it in the 'editgroup' method where we first check the capability of the user and then we check that it's an administrator. It could be the case that a user has the capability but is a Sub-administrator
  • In an emonCMS installation with the Group module but without the Capabilities all the members of a group will have the privileges of the current Administrator. Not a problem really but maybe we should add it to the readme.
  • There is one capability missing "Can see it's own group" to fit the purpose of the Passive member (who doesn't even know that belongs to a group).
  • One possible limitation: each user can have only one role. Now a user can be administrator of a group and sub-administrtor of another and passive member of another
  • Adopting the Capabilities module has implications in other modules: Task and Graph (with Group support) which rely in the current permissions system (but this is my problem ;-)

I'll do this bits of work and then do the merge unless you want to add something else

@cagabi
Copy link
Collaborator

cagabi commented Sep 5, 2018

Hi again,

@takkaria. I have started removing the original permissions system in the Group module so that we only use the Capabilities module and a point I make above is coming again to my head so I would like to hear your opinion. Also @beaylott and @matt-carboncoop as you are administrators of our installation you may also have something to say.

The original permission system has now 3 roles (Administrator, sub-administrator and passive member). As it is now, it is not a flexible system to use because the capabilities for each role are handcoded and there is not a neat way to use it in the code. A positive thing for me with it is that the roles are assigned to a user in a group scope meaning that a user can be Administrator of a group and Sub-administrator of another.

The new permissions based on the Capabilities module is super flexible with the nice UI to add/remove capabilities to a role and very nice to use in the code. The limitation I find is that a role is applied in the system scope meaning that if you are granted a capability in one of your roles you have it for all the groups you are in. So if you are Administrator in a group then you cannot be downgraded to Sub-administrator in another.

Now, depending on the use of the installation the scope of the role may or may not be an issue. I f we only plan to have users in one group that's ok with the Capabilities module as it is. But if we wanted, for example, to have Dom as administrator of the Carbon Co-op members group then he would be administrator of the Nobel Grid group as well.

I am trying to think how we could use (or tweak) the Capabilities module to give a user a role that is for a specific group but I can't see how, the capabilities are granted at a system level and that is it 🤔

@takkaria
Copy link
Collaborator Author

takkaria commented Sep 5, 2018

Hi Carlos

Yeah, I'm not sure if it makes sense to totally remove the group roles that are already there. As you say, perhaps this is something that needs a bit more thinking through. Seems like there are two purposes for groups-type functionality:

  1. When you are an administrator and you want a UI to make it easy to administrate different sets of users.
  2. When you want to allow users to share data with other users, without giving them access to everything on the system.

They are quite different things and obviously the capabilities work is more tilted towards number 1. I am not sure if anyone uses groups apart from Carbon Coop - I guess you know @cagabi? Is 2 a significant use case?

I think it's important that there is still group-level access control. For example, there are people who are admins in the Lancaster group who shouldn't have access to other groups' users. Maybe it's enough to say that users only have those permissions over people in groups that they are in, which is (I think?) how it currently is.

Currently the two systems work together OK I think: you can have some admins in a group who can only edit user info (this has been useful at Lancaster) but others who have more abilities. But you know for sure that any user who is down as 'passive member' has no permissions. I think that is a useful feature.

@takkaria
Copy link
Collaborator Author

takkaria commented Sep 5, 2018

I can think of a way to grant specific permissions only in a group: I'd need to add a grant_in column in the user_roles table, which would be a list of group IDs that the role applied to (or * for all).

However, while I think this would be a good solution just for the group module, I don't know if it would make sense more broadly if capabilities were used in more places. I think I would like to expand the capabilities support to other parts of EmonCMS - there was some interest in this on the forum when I posted about it - and see if this would make sense there too. How would you feel about holding off on changing things for a little while?

@cagabi
Copy link
Collaborator

cagabi commented Sep 5, 2018

I can think of a way to grant specific permissions only in a group: I'd need to add a grant_to column in the user_roles table, which would be a list of group IDs that applied to (or * for all).

I am trying to figure out how it would look in the UI and the practical implications to the current roles and cannot see it clear. Anyway the important point is the other one you make:

However, while I think this would be a good solution just for the group module, I don't know if it would make sense more broadly if capabilities were used in more places.

I totally agree with this. The potential of the Capabilities module is great for the whole emonCMS so such a bespoke solution doesn't feel right.

How would you feel about holding off on changing things for a little while?

Yes, better to wait. So we don't do merge it yet but leaving it in the current branch in our installation.

I'll keep thinking on it though. Maybe there is way of using the Capabilities module from inside the Group module to define the capabilities of the current roles while we keep assigning roles inside the groups

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants