"Edit group" permission applies more broadly than expected [#2555689]

To reproduce:

Create 2 group content types, call them A and B.
Configure so that B can be posted in A.
Configure OG sitewide permissions such that the only permission given to a "member" user of group type A is "Edit group". Authenticated users should also not have any extra core permissions.
Create an "A" node, and a "B" node that should be a member of group "A"
Create an authenticated user, and join then to the "A" group as a regular member
Log in as the authenticated user
Attempt to edit the group B node

Observe that the authenticated user IS able to edit the group B node, despite:

Not being a member of group B
Not having any "Edit any B content" permission (via group A permissions, or sitewide)
Not having any "Administer ..." permission (via group A permissions, or sitewide)

The terminology used in the "Edit group" permission (and the description "Edit the group") is clearly singular, and I think misleading - I doubt anyone would expect that this would allow users with this permission to edit all group content posted to the group that simply happens to be a group type itself.

There are a couple of solutions:

We could simply change the permission name/description to make it's behaviour more clear.
We could alter the behaviour to only check the users "Edit group" permission on the group they are trying to edit, so in the case above they would still be able to edit A, but would not be able to edit B (since they are not a member).

I would argue the second case fits more with how existing users would expect this permission to behave, and is also a more logical arrangement, since there is already a distinct "Edit any B content" permission available for group A, which allows a finer level of control - no actual functionality is lost making "Edit group" apply only to the group it is set on.

Comment	File	Size	Author
#2	2555689.patch	654 bytes	Owen Barton
#2

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

22 August 2015 at 00:42

Owen Barton created an issue. See original summary.

Comment #2

Owen Barton CreditAttribution: Owen Barton at CivicActions commented 25 August 2015 at 00:16

Status:

Active

» Needs review

File	Size
2555689.patch	654 bytes

Adding a simple patch for approach 2. Not sure if tests need updating or not.

Comment #3

simon_j_nichols CreditAttribution: simon_j_nichols commented 25 August 2015 at 09:03

Edit Group is a very powerful permission for sure, but I think this is how it is architected.

Your can basically "manage the group" and that includes all content.

In your example B is a member of Group A so it is "managed" by anyone who has this permission.

I would be wary of a simple fix, OG is a complex beast.

Comment #4

simon_j_nichols CreditAttribution: simon_j_nichols commented 25 August 2015 at 10:45

bit more explanation.... if you check the code in og_user_access_entity there is a check that the entity is a group type andthen a call to og_user_access - this should fail in your example .... but the code then goes on with the comment

      // An entity can be a group and group content in the same time. The group
      // didn't return TRUE, but the user still might have access to the
      // permission in group content context.

It then retrieves the groups that entity is a member of and checks those using og_user_access. So I think my original comment is correct. If the entity is group content, it checks the permission of the group type to determine the users rights.

Comment #5

Owen Barton CreditAttribution: Owen Barton at CivicActions commented 27 August 2015 at 00:58

Your can basically "manage the group" and that includes all content.

My understanding (based on tests, as well as reading the code) is that this statement isn't correct. The permission that provides the privileges described in the above statement is actually "Administer Group", which indeed lets you manage the group and edit all content associated with that group.

The "Edit Group" currently lets you edit the group node itself, as well as any content posted to the group that itself happens to be a group. The "Edit group" permission does not let you edit any other content in the group. This behaviour is in conflict with the permission name and it's description ("Edit this group") which are both clearly singular, and I am pretty sure this behaviour would be a surprise to most/all site managers that post groups into groups.

The og_user_access_entity comment you list is indeed the source of the current behaviour. In my view this function is behaving correctly because in the normal case it is checking a permission which should apply to the group and the group content - e.g. an "Edit any X content" permission is clearly in this category. In the case of "Edit group", however it does not make sense to allow edit access for group content, since the scope of the permission is clearly *the* group (not groups that happen to be content of the group). Hence, my proposed patch leaves og_user_access_entity behaviour alone and just the changes the caller to use og_user_access so as to check on the group only (not on group content).

Comment #6

simon_j_nichols CreditAttribution: simon_j_nichols commented 27 August 2015 at 10:55

Interesting. I will get back into the code and check that out.

I may not be able to get back to you for a few days, but I will try and validate your diagnosis and fix and that might lend some weight to your proposed fix.

Simon

Comment #7

simon_j_nichols CreditAttribution: simon_j_nichols commented 28 August 2015 at 11:01

Ok I had some time last night to look at this a bit more .... short version is I believe you are correct

Slightly longer version for other wider review purposes

og_node_access() checks it is being requested to check for permission to "update" a group type node
if that is so it calls og_user_access_entity() to enact the check
og_user_access_entity() checks that the node is a group and if so, that you can access it - this fails for B in your example
og_user_access_entity() then checks that the node is also a content node - which B is, so we proceed.
og_user_access_entity() then gets the group type nodes for which B is content and checks for "update" access on them - A returns positive.
og_user_access_entity() returns positive and you so can edit B

other content of A is not affected as og_node_access() does not call og_user_access_entity() for non-group type nodes, so the bug is confined to calls on a group type node which is also a group content type of another group type

fix looks sensible as well and of course passes the tests