Core permissions on group entities are leaking to group content
Closed this issue · 3 comments
Use case:
- I have a "Community" group type which is intended to host posts from the group members, similar to a Facebook group.
- I have two group content types:
- "Post" nodes which are posted by the group members.
- "Page" nodes which contain basic information about the community.
- I have an "Editor" role which is allowed to edit and delete the "Page" nodes (which is an OG permission), and also the "Community" node itself (which is a core permission), but is not allowed to edit and delete the "Post" nodes.
Expected behavior:
- When I am logged in as an Editor and visit a "Post" node, then I should not have access to edit or delete the node.
Actual behavior:
- I have access to delete the node.
This is caused in this section of OgAccess::userAccessEntity():
// Check if the operation matches a group level operation such as
// 'subscribe without approval'.
$user_access = $this->userAccess($group, $operation, $user);
if ($user_access->isAllowed()) {
return $user_access->addCacheTags($cache_tags);
}
$forbidden->inheritCacheability($user_access);
This doesn't check that the operation passed on to $this->userAccess() is actually a group level operation (such as subscribe without approval) and not a core entity operation (such as 'view', 'update', 'delete'). At the moment all permissions that a user has on the group will fall through to group content.
Also it seems unnecessary to call $this->userAccess() to check for group level operations on group content. These permissions are typically things like manage members, subscribe etc which don't apply to group content. The call to $this->userAccess() earlier in the method should be sufficient.
This is probably a security issue since it gives unintended access to do basic entity operations on all group content entities if the user has this permission on the group entity. This will also break existing code which is relying on this behavior but it is clearly incorrect. If a role should have a certain access on group content they should be assigned the appropriate GroupContentOperationPermission.
It looks like this second call to $this->userAccess() is intended to:
- check if the user is UID 1, this user has all permissions on all group content.
- check if the user has the "administer group" permission, which gives access to do everything on group content.
- check if the
group_manager_full_accessflag is set in the OG configuration and the user is the owner, this would give full access. - reuse the
hook_og_user_access_alter()alter hook, but this doesn't seem right since this will pass the group object to the hook implementations, while we are in fact checking access on group content.
OK it seems that this can only be replicated if the alter hook is implemented. Giving a user access to edit or delete a group in hook_og_user_access_alter() has the unintended side effect that it will also give these permissions to all group content. This happens because the alter hook is fired both for the group and the group content.