Groups and roles by permissions

[idimopoulos]

Hi! It is me again!
I was thinking that we could populate the helper methods a bit more since I have extra use cases that at least for us happen quite often.
In our case, we have a case where we need to query for all groups a user can share some content in. To do this, we need to query all groups that the user has a certain permission in.
By looking into the og code, it seems that two things are missing from this:

• Retrieve all roles with a given permission.
• Retrieve all groups with a given role.

Obviously, a role id also include the entity type id and the bundle so my thoughts are:

• Provide a method to the OgRoleManager service that can load all roles with a given permission. This method can optionally filter by entity type id and bundle.
• Provide a method to the MembershipManager that loads all memberships given some roles and then retrieve the groups. For this method I am using the MembershipManager::getMemberships method so cache is already being taken care by existing methods.

I am attaching the two methods, the interface definitions and some tests related to them.

Please, let me know if these are two specific to be included in og so that I can include them explicitly in our project instead.

[idimopoulos]

@pfrenssen I have fixed all of your remarks and extended the tests to test a few more cases.
I have implemented the $require_all flag, sorry for not naming it $require_all_permissions but I am using it in 2 different places so I though we could keep it simpler to remember the usage by having the same name of the variable. Very small silly thought but it makes a bit of sense to me.

Speaking of which, I took your idea of having a "require all" flag for permissions and extended the other API methods in this PR, getUserGroupsIdsByRoles and getUserGroupsByRoles to also have such a flag. Now you can request groups of the user where he has all of the requested roles.
This could be useful in searches where let's say you want the groups where the user is the author and the publisher or something similar.
Tests for the roles were also extended accordingly.

I did not use a provider for the tests because I wanted to have a test for the default values in the methods as well.

[pfrenssen]

It's getting there! I still have some remarks about the flags that are being added and some nitpicks.

I also made some additional comments while trying to understand what the test was doing. I have committed these in the branch.

[pfrenssen]

I think the $require_all flag should default to TRUE.

Roles in Drupal are additive and tailored to a specific set of actions that the user needs to perform in the site. In corporate sites a user typically has many roles. For example a HR manager might have access see payroll information in specific departments, and a business analyst might see production processes. If you are making a page that correlates payroll with business processes you would restrict it to people that have both roles.

The other case makes less sense. What are the use cases (except from a permission lookup) where you would need to get the groups where a user has one role, or another role, or maybe a third one?

Also the name $require_all is ambiguous. Both the roles and the states are arrays that filter on multiple values, so it is not clear to which this is referring. Let's call it $require_all_roles. And put it as the last parameter so the parameter ordering is consistent with the rest of the class.

[idimopoulos]

The problem with that is that it is almost unavoidable to have conflicts between roles. Even with the current state, you might want to say, I want all users that can see node with ID 3.
This can be, the author (which does not matter in this example), UID 1 (also does not matter), users that can access content, users that can bypass content access, users that have the administer content permission.
So that would mean that we are requesting either an author, a moderator, an owner of the group, or a viewer.
I think most cases will derive anyway from the full PR which will be a scenario like "I want all users with either of those permissions". This will escalate to "I want all users with either of these roles that have one of the requested permissions". And frankly, most cases will be "Give me all users whose roles have this permission". So I still believe that having it default to false is still better. But if you insist, we can go on with your solution.

I agree with the renaming due to ambiguity.

[pfrenssen]

I understand your point, but view it from this angle: in Drupal users are related to roles, not directly to permissions. User roles are intended to solve business use cases for people in certain positions. A role represents something like "The people in the HR department should be able to update the data of the employees". It is not just an intermediate storage for permissions.

Your argumentation is revolving around permissions and not roles. This is completely understandable as your main focus point right now is to solve your immediate use case, but you should considering the wider use cases of people consuming the OG API. This method will return all the group IDs that correspond to passed in roles. It has nothing to do with permissions. This method is getGroupIdsByRoles() not getGroupIdsByPermissions().

If you imagine the use cases for retrieving the groups that are associated with a number of passed in roles then it makes sense that the most common and safer option would be to default to requiring all. If a developer wants to use this method to display sensitive data to users who have multiple roles in the organisation, then having this default to FALSE makes it a security risk. If the developer forgets to toggle the flag then the sensitive information is exposed to all users who have at least one of the roles. If we default to TRUE on the other hand and the developer wants to show information to users that have one or more of the passed in roles, then it is not a risk: if the developer forgets to toggle the flag then the info will be limited to only the users that have all the passed in roles.

[pfrenssen]

Same here, let's rename the flag, make it default to TRUE and move it to the last position in the argument list.

[pfrenssen]

Let's default to TRUE here as well, this is more secure. If a developer forgets to set this flag at least it won't suddenly expose sensitive data to people who only have 1 permission out of an expected set of permissions.

[idimopoulos]

Please, check my comment in the previous similar remark. Again, if you insist, we will go with default to true. However, it is more of a common use case than a security issue. How many times you request a user that has more than one specific permission? Even the access handler has one operation when checking.
In general, when you are requesting users based on permissions, the common case would be that you would want to request all users that can perform a single action, so either "access content" or "administer content" would not be a security leak when you are requesting users that can view the content.

[pfrenssen]

Yeah passing only a single permission in would also be a valid approach for this method. That is also safe. Multiple permissions which default to AND is also safe. However the proposed way of requesting multiple permissions which default to OR is NOT safe.

I will try to explain what I mean with the security risk using your own example: requesting roles by passing in both access content and administer content:

Use case 1: OR

We need to show a page with sensitive information to users that have either the access content OR the administer content permissions. This means we should set the flag $require_all = FALSE.

What happens if the default is TRUE and we forget to set the flag? The sensitive content is not shown to users which only have one of the permissions. The page is only shown to people that have both. This is an annoying bug but not a security risk: the sensitive content is not exposed unintentionally to unprivileged users.

Use case 2: AND

We need to show a page with sensitive information to users that have both the access content AND the administer content permissions. This means we should set the flag $require_all = TRUE.

What happens if the default is FALSE and we forget to set the flag? The sensitive content is now exposed unintentionally to the users that have only one of the permissions. This is a security issue.

[pfrenssen]

This is looking close to completion. I just have some nitpicks and I insist on changing the flags to default to TRUE so that our public API is safe by default.

I will keep this assigned to me to make these small changes, then this is ready for merging after tests are green.

[pfrenssen]

I have switched the polarity of the flags as explained above, moved the test to the right class, and did an overhaul of the documentation. This is looking good now to me. Thanks a lot for this helpful addition!

The test on codeclimate has failed but this is still in experimental stage. All tests on Travis have succeeded.