Finer control of permission inhertence [#2029123]

Right now the user gains whatever permissions they have in the parent group.

However, that means, for example, they don't have the permission the members of that child group have (if they have been customized to be different than parent group), and that all admins, etc. of parent group will travel down, which is desirable for some use cases but not other.

Proposal is to add control in how permissions are inherited, allowing parent groups to set whether child groups use permissions from parent or use the default member permission in the child group.

Patch depends on #2028055: Add caching for calculating parent/children. Like that, I try avoiding any extra calls/try to be as performant as possible. For example, when checking access, only check it for the user's group (I think it currently checks all parent groups despite user not being part of the group, meaning users gain whatever access non-member has from the parent, including ability to register for group o.o), and exit out as soon as access has been flipped to true.

Comment	File	Size	Author
#9	2029123-og_permission_and_cache-9.patch	46.84 KB	hefox
#8	2029123-og_permission_and_cache-8.patch	46.67 KB	beeradb
#6	2029123-og_permission_and_cache-5.patch	45.79 KB	beeradb
#4	2029123-og_permission_and_cache-4.patch	45.76 KB	beeradb
#3	2029123-og_permission_and_cache-3.patch	45.35 KB	hefox
#2	2029123-og_permission_and_cache-2.patch	45.35 KB	hefox
#1	2029123-og_permission_and_cache-1.patch	45.36 KB	hefox
	og_user_permission_inhertence.patch	8.31 KB	hefox

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

hefox CreditAttribution: hefox commented 27 June 2013 at 04:47

Status:

Needs work

» Needs review

File	Size
2029123-og_permission_and_cache-1.patch	45.36 KB

I was adding tests (the current tests are for 1.x version as far as I can tell, so just scrapped them) and concluded it's much easier to handle one patch than two, so putting the cache issue as postponed and updating the entire patch (permission field and cache) here, though the permission part is a minor part of the patch.

Comment #2

hefox CreditAttribution: hefox commented 27 June 2013 at 23:05

File	Size
2029123-og_permission_and_cache-2.patch	45.35 KB

The oops of copy and pasting membership_type instead of membership hook continues, fixing.

Comment #3

hefox CreditAttribution: hefox commented 1 July 2013 at 01:24

File	Size
2029123-og_permission_and_cache-3.patch	45.35 KB

Fixing a place that didn't get updated

Comment #4

beeradb CreditAttribution: beeradb commented 3 July 2013 at 18:04

File	Size
2029123-og_permission_and_cache-4.patch	45.76 KB

Here's a re-roll, which allows for the $filtered parameter on child groups as well.

Comment #5

hefox CreditAttribution: hefox commented 3 July 2013 at 18:23

Status:

Needs review

» Needs work

+++ b/og_subgroups.common.inc
@@ -312,79 +235,176 @@ function _og_subgroups_get_inherited_users($group_type, $group_id, $states = arr
+function og_subgroups_parents_load($group_type, $group_id, $filter = TRUE, $fetch_all = TRUE, $include_current = FALSE, $reset = FALSE) {
...
+function og_subgroups_children_load_multiple($groups, $fetch_all = TRUE, $filter = TRUE, $include_current = TRUE, $reset = FALSE) {

Different order of $fetch_all vs $filter?

+++ b/og_subgroups.common.inc
@@ -312,79 +235,176 @@ function _og_subgroups_get_inherited_users($group_type, $group_id, $states = arr
+function og_subgroups_children_load($group_type, $group_id, $fetch_all = TRUE, $include_current = FALSE, $reset = FALSE) {
...
+    $groups_all[$cid] = og_subgroups_children_load_multiple(array($group_type => array($group_id)), $fetch_all, $include_current, $reset);

didn't add it to arguments, or update the og_subgroups_childre_load_multiple call

Comment #6

beeradb CreditAttribution: beeradb commented 3 July 2013 at 18:32

Status:

Needs work

» Needs review

File	Size
2029123-og_permission_and_cache-5.patch	45.79 KB

Ack, good catch on both points. Fixed with this one.

Comment #7

hefox CreditAttribution: hefox commented 8 July 2013 at 17:58

Status:

Needs review

» Needs work

Need to only clear cache if updating a parent field vs. content

Comment #8

beeradb CreditAttribution: beeradb commented 10 July 2013 at 03:57

File	Size
2029123-og_permission_and_cache-8.patch	46.67 KB

Here's a re-rolled version.

The previous patch had an issue. Here's a re-roll that includes:

* If the entity_load were to fail within the child_load_multiple or parent_load_multiple functions, the $group_cache key would not be set for that specific $cid, eventually that would trickle down into the og_subgroups_merge_groups function where it would try to merge a NULL value with an array (bad). This new patch addresses that issue by adding a blank array in the event that entity_load is not successful. This also allows it to cache so we aren't calling entity_load on non-existent (or broken) entities all over the place.
* Some minor coding cleanups.

Leaving this as needs work, since the caching work @hefox mentioned still needs to be updated.