Summary

Specifying access to a section using the 'Groups' does not work correctly for private spaces that inherit Group users.

Running in to this issue after upgrading a site.

Steps to recreate from fresh installation

Create a user: 'test-access'
Create a Group: 'Access group'
Keep the default settings (Group user inheritance set to 'Yes')
Add the 'test-access' user to the 'Access group'

Create a Space: 'Space'
Set the space to be 'Private'
Under 'Inheritance': Parents: Choose the 'Access group'
Set 'Group user inheritance' to 'No'
Save

Create a section inside 'Space' called 'Normal section'
Do not apply any visibility rules, save

Notice that the 'test-access' user can access the 'Space' and see the 'Normal section' - expected behaviour

Now, create a section inside 'Space' called 'Controlled section'
Under the Section visibility: Groups: select 'Access group'
Save the section

Notice that the 'test-access' user cannot access the 'Controlled section', or see it on the space page.

This is not the expected behaviour. By specifying that the 'Access group' can access the section, it has actually denied access to it.

Comments

JKingsnorth’s picture

JKingsnorth CreditAttribution: JKingsnorth commented

This seems to step from recent changes in:
oa_core/includes/oa_core.access.inc
oa_core/includes/oa_core.util.inc

Restoring these files to the version from the 2.22 version of the module seems to fix the problem. So something appears to have broken between the 2.22 and 2.23 versions.

Obviously reverting to these old versions is not the correct solution.

I assume it is related to oa_core_node_grants.

Edits:

Further tracked down to the oa_core_get_groups_by_user function (in oa_core.utils.inc), which returned a lot more results in the previous version.

This is the issue: Previously that function also returned groups that the user was an 'inherited' member of, not just direct membership. The comment on the function says:

REPLACES og_get_groups_by_user() in OG and returns a list of groups including subspaces including via INHERITED membership

However it does not include inherited memberships.

hefox’s picture

hefox CreditAttribution: hefox commented

Can you try the updated patch for https://www.drupal.org/node/2407399 to see if it's changes?

if can run behat tests, the subspace.feature is testing this scenario (cannot reproduce against dev)

JKingsnorth’s picture

JKingsnorth CreditAttribution: JKingsnorth commented

I'll check this tomorrow, thanks hefox, but I'm not sure it's related?

The 'oa_core_get_groups' function in oa_core.utils.inc doesn't seem to be related to OG directly, it makes it's own DB query to check access? http://cgit.drupalcode.org/oa_core/tree/includes/oa_core.util.inc?id=15a...

hefox’s picture

hefox CreditAttribution: hefox commented

oa_core_get_groups_by_user should be getting inherited groups via this code segment:

  if (module_exists('og_subgroups') && !empty($user_groups[$group_type])) {
    //$orig_user_groups = og_subgroups_children_load_multiple($user_groups);
    $user_groups[$group_type] = oa_core_get_groups_by_parent($user_groups[$group_type], array(OA_SPACE_TYPE, OA_GROUP_TYPE), NULL, $include_archived, NULL, TRUE);
  }

It's used to support og_subgroups and performance -- many og functions do node_loads which are slooow (sans entity cache)

JKingsnorth’s picture

JKingsnorth CreditAttribution: JKingsnorth commented
Status: Active » Needs work

Hi hefox.

I've applied the patch from that issue, flushed cache and rebuilt permissions, but the issue is still occurring - the sections are not visible.

So I imagine there is some sort of issue in that function/call that you posted in your comment above.

(However that patch did solve the other issue where people had access to Group pages they were not supposed to).

JKingsnorth’s picture

JKingsnorth CreditAttribution: JKingsnorth commented

Confirmed that the issue is in oa_core_get_groups_by_parent()

It doesn't, in fact, seem to return the 'child' groups. Whereas in OA core 2.22 it did.

I tested this using the following code (taken out of the call in the comment above):

$test = oa_core_get_groups_by_parent(array(74), array(OA_SPACE_TYPE, OA_GROUP_TYPE), NULL, FALSE, NULL, TRUE);
dsm($test);

Where '74' is the ID of a Group that has 7 sub-groups. It returns 8 elements in OA 2.22, and only one (itself) in the current version.

hefox’s picture

hefox CreditAttribution: hefox commented
Status: Needs work » Active

The patch wasn't specially for this issue, so back to active. Needs work needs a patch.

mpotter’s picture

mpotter CreditAttribution: mpotter commented

Have you applied the patch here: https://www.drupal.org/node/2437855 ? I believe calling oa_core_get_groups_by_parent with the array(OA_SPACE_TYPE, OA_GROUP_TYPE) was triggering that other error and without the patch it's going to abort and not return the children.

JKingsnorth’s picture

JKingsnorth CreditAttribution: JKingsnorth commented

Hi mpotter. I've applied the patch from that issue (flushed cache and rebuilt permissions) and the sections are still not appearing.

The test code above still only returns the parent.

hefox’s picture

hefox CreditAttribution: hefox commented

Have you added a debug statment to see why it's not calling oa_core_get_groups_by_parent or what is happening in there that not returning any groups?

JKingsnorth’s picture

JKingsnorth CreditAttribution: JKingsnorth commented

hefox, oa_core_get_groups_by_parent is being called, but it is not returning 'child' groups, only the parent. This can be demonstrated by running the code:

$test = oa_core_get_groups_by_parent(array(74), array(OA_SPACE_TYPE, OA_GROUP_TYPE), NULL, FALSE, NULL, TRUE);
dsm($test);

Where '74' is the node ID of a Group that has one or more child Spaces.

hefox’s picture

hefox CreditAttribution: hefox commented

When I last tested that, I was unable to reproduce that. Can you dig further to see where it's exiting out/unable to find children?

The tests testing this situation are running fine (just re-ran them)

JKingsnorth’s picture

JKingsnorth CreditAttribution: JKingsnorth commented
Issue summary: View changes

Hi again. I've managed to recreate the issue again now. There was one important step missing from the original steps to recreate the error.

The issue emerges when the 'Group' has group user inheritance turned on, but the child space has 'Group user inheritance' off. The expected behaviour is that the Space inherits the users of the Group, but any sub-groups of the Space would not inherit membership.

Instead, setting the 'child' to not have user inheritance also cuts off users being inherited from parents of the group. In this case, the space should still inherit membership from parents, but children of the space should not.

That may not read well, but if you run through the updated process in the original report then you'll see what I mean.

Tested on OA 2.3.2 with the latest dev of oa_core.

hefox’s picture

hefox CreditAttribution: hefox commented
Status: Active » Fixed

yep, it was checking inheritence setting of child instead of parent

JKingsnorth’s picture

JKingsnorth CreditAttribution: JKingsnorth commented

Fixed here, for reference: http://cgit.drupalcode.org/oa_core/commit/?id=1c8646b

Thanks for the fix hefox. Out of interest, why are we doing this in SQL rather than relying on the OG API?

JKingsnorth’s picture

JKingsnorth CreditAttribution: JKingsnorth commented

As a follow up, perhaps the test suite could also look out for this issue?

mpotter’s picture

mpotter CreditAttribution: mpotter commented

Out of interest, why are we doing this in SQL rather than relying on the OG API?

Because the OG API is very slow on sites with a large number of space, especially in OG Subgroups. We have been making a significant number of performance improvements within OA by using direct db queries (using the Drupal database API, not with raw SQL). While we have made several patches for various OG modules, some of these changes are beyond the scope of patches to the OG code since the OG code is written more to use existing APIs rather than being super concerned about performance. In OA2 we care a lot more about performance and have Enterprise client sites running OA2 with tens of thousands of spaces.

JKingsnorth’s picture

JKingsnorth CreditAttribution: JKingsnorth commented

Thanks mpotter. I understand that performance is key. But introducing these custom versions of existing functionality adds to the maintenance overhead for the project, and I think it would also open up more chances for bugs to creep in. I might create a separate issue where this could be discussed in more detail, but over the last year OA has become vastly more complicated. I'm not saying this is a bad thing, and I know Phase2 now has a dedicated distro team to support OA (which is great).

mpotter’s picture

mpotter CreditAttribution: mpotter commented

Totally off topic for this thread, but...

Now that Open Atrium is a real "product" for us, you'll actually see even more in the future beyond "just a distribution". I don't think OA itself has really become "vastly more complicated" in the last year since the core architecture hasn't changed. What you are seeing are more Apps being added and more layers of usability to help site builders and space admins do more and customize more easily without needing to write code. You'll see more and more of that kind of UX layering over the normal Drupal UI in the future.

Once we get the Comments stuff straightened out in the 2.4x release you will likely see less architectural changes and more "front end" changes in the future.

But yes, all of that means more custom code that needs to be maintained, ported to Drupal 8, etc. But that's the task we accepted when we put together the product team to support this fulltime and more than any previous distro from Phase2.

Ultimately, we need OA to meet the needs of our clients, and performance has been a big part of that lately. Sometimes you have to make a call between clean API code that is slow vs more optimized code that performs but takes more effort to support.

The fact that we can load spaces, pages etc in Open Atrium in under 2 seconds on a site with 16,000+ spaces, all with custom panel panes is a testimate to how well Drupal *can* perform with better code. When we were calling OG functions directly the pages were taking 2GB of PHP RAM and loading in about 30 minutes!! That's simply because the entity wrapper API and some of the OG and og_subgroups functions are doing too many entity_loads without thinking of the performance impact.

Basically, OG wasn't really architected to handle what og_subgroups is trying to do. We ultimately might submit this code back to OG, but my guess is that they won't be very willing to completely revamp their code to replace entity wrappers with direct DB queries.

JKingsnorth’s picture

JKingsnorth CreditAttribution: JKingsnorth commented

Thanks again for the further explanation mpotter. It's certainly appreciated, and it's great to see how 'active' the issue queue has been lately in terms of issues being triaged and addressed.

but my guess is that they won't be very willing to completely revamp their code to replace entity wrappers with direct DB queries.

If the OG code is that slow (2secs versus 30minutes) then maybe they won't mind ;]

Anyway, thanks again. Glad the original issue has been fixed.

Referring back to #16 - should this be 'needs work' again so that this scenario can be accounted for in the test suite?

Status: Fixed » Closed (fixed)

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