- Grandparent (private)
-- Parent (public [to grandparent]
--- Child (public [to parent])

Child ends up being a totally public group instead of to parent. Child's node access is based on parent but not grandparent, because og_access only checks parents and og_subgroups only removes access to groups that don't have inheritence settings.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hefox’s picture

Status: Active » Needs review
FileSize
4.23 KB
hefox’s picture

Patch was only checking the content access field (which groups may not have) instead of the group access field

JKingsnorth’s picture

Confirmed that #2 fixes the issue - private groups under public groups are no longer publicly available. Patch applied cleanly.

Tested with:

Public Group
^ inherit
Private Group

Previously the Private Group was visible to non-members. Now it is only visible to direct and inherited members, as it is supposed to.

This issue still exists, but is probably specific to the OA distribution: #2445069: Section visibility by group not working correctly

hefox’s picture

This introduced another problem, fixing

hefox’s picture

+        if (!in_array($gids['node'], $gids['node'])) {
+          $gids['node'][] = $node->nid;
         }

I'm not sure how I missed /that/ (in previous patches also)

rooby’s picture

Will this patch help me achieve this structure

-Grandparent group (public) - Group content (private)
-- Parent group (private) - Group content (private)
--- Child group (private) - Group content (private)

Where a member of the grandparent group gets access to parent and child groups under it and a member of the parent group gets access to the child groups under it?

BrightBold’s picture

@rooby - I haven't yet tried the patch above so don't know the answer, but also look at https://www.drupal.org/sandbox/liberatr/2449227 which will hopefully get converted into patch form if it does what people want. It was specifically designed to address some of these content inheritance issues.

rooby’s picture

Thanks for the info. Unfortunately that module doesn't quite cover my requirements.

This patch can pretty much get me what I needed in my above comment but what I didn't mention in my previous comment is that only some roles get public access to the grandparent group.

So for that I'm going to have to get the port of https://www.drupal.org/project/og_access_roles up to scratch (https://www.drupal.org/node/1853494).

Fingers crossed it doesn't need much work to get it compatible with the current version of OG.

After that I'm going to have to patch this module to support that roles based module.

mpotter’s picture

Status: Needs review » Reviewed & tested by the community

We have been using this patch in Open Atrium for a while now. Marking this as RTBC and will commit it soon unless somebody has objections.

mpotter’s picture

Status: Reviewed & tested by the community » Needs work

Spoke a bit too soon. This patch needs to be altered a bit to handle some more general cases. Will post an updated patch with discussion soon.

mpotter’s picture

Status: Needs work » Needs review
FileSize
4.87 KB

OK, here is the new patch for review.

The difference between this and #5 is refactoring the logic for determining if the parent groups are private or not. It adds a function og_subgroups_is_parent_private() which adds an alter hook for this.

The reason for this is that you might have different kinds of OG groups with either custom fields or values to determine if a child group should be forced to be private or not. There are some use-cases where having a private parent group does *not* cause the child to be private. Just checking the group_access field is a good default, but this alter hook allows it to be changed.

A specific example: In Open Atrium, having a private parent *space* should cause the child space to be private. But just having a private parent *group* should NOT cause the child space to be private. We want to be able to inherit membership from private access-groups without making the space itself private.

So, here is an updated patch with this refactor and alter hook:

mpotter’s picture

Here is a minor update with improved comments, fixed docblock, and fixed spelling.

JKingsnorth’s picture

That patch in #12 is included in the Open Atrium distribution and the behaviour I described in #3 is still working as expected. Thanks for the work on fixing this.

Two very minor comments about the patch, but nothing that would block it. I haven't checked the 'grandparent' logic in the original issue description though.

  1. +++ b/og_subgroups.module
    @@ -125,29 +125,101 @@ function og_subgroups_node_grants($account, $op) {
    +      $content_access = !empty($wrapper->{OG_ACCESS_FIELD}) && $wrapper->{OG_ACCESS_FIELD}->value() ? OG_CONTENT_ACCESS_PRIVATE
    +        : (!empty($wrapper->{OG_CONTENT_ACCESS_FIELD}) ? $wrapper->{OG_CONTENT_ACCESS_FIELD}->value() : OG_CONTENT_ACCESS_DEFAULT);
    

    Double ternary operator not so readable? Expand it, or add a comment to explain the logic?

  2. +++ b/og_subgroups.module
    @@ -125,29 +125,101 @@ function og_subgroups_node_grants($account, $op) {
    +      // This is our new logic.
    

    Unnecessary comment line (but who cares ;] )

  • mpotter committed a9d31c7 on 7.x-2.x
    Issue #2407399 by hefox, mpotter: Access only checks one level deep
    
mpotter’s picture

Status: Needs review » Fixed

OK, committed this to a9d31c7.

I removed the comment and split the ternary operation over a few lines to make it more readable.

I also went back and tested the case shown in the OP to verify that it works now.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

markdc’s picture

I don't understand which fields to use in order to achieve this. Documentation?