Private groups aren't private until "rebuild permissions" is run

[indigoxela]

In Drupal, when enabling og_access, one gets prompted to "rebuild permissions" in a second step.

This step does not happen in B, so even private groups are visible to non-members and even anonymous.

Once "rebuild permissions" runs, everything seems to work as expected.

We can either prompt, or we could also run node_access_rebuild(); automatically.

node_access_needs_rebuild(TRUE);

should actually do the trick.

[indigoxela]

Thinking this over ... it's not actually enabling the og_access module, that needs to trigger a node_access rebuild, but when adding an access field (Group visibility) to a group type. Enabling the module does nothing on its own.

Correct me if I'm wrong, though.

@laryn what do you think?

[argiepiano]

Hi @indigoxela. I've done some pretty exhaustive testing and comparison between D7 and Backdrop Organic groups access control. At this stage I have focused ONLY on group access (not group content access), but I think the same applies to both.

Bottom line: rebuilding permissions once (preferably right after enabling og_access) fixes all view access problems of group nodes. This is true even if you create new groups, and even if you add "Group visibility" fields to any group content type later. You don't need to rebuild permissions every time you add such a field to a group type. That initial rebuild sets you up for the future.

The main difference between D7 and B is that D7 nags you constantly with an error message until you rebuild permissions. Backdrop simply does not. There is no way for a Backdrop admin to know that a rebuilt is needed. Furthermore, neither clearing caches or running cron will ever rebuild the permissions table. I'll open an issue in Backdrop's queue for this.

At this stage I have focused ONLY on group access permissions (not group content access). My testing workbench is pretty simple: I use one node as a group, and try to access it (view it) through an authenticated user after enabling og_access and setting the group to private.

In fact D7 and Backdrop present exactly the same access problem if you don't rebuild permissions. To see this you can do the following, both in Backdrop or Drupal:

1. Start without og_access (og_access disabled)
2. Create a group node type, and add a node of that type (at this point you can't control visibility)
3. Enable og_access. Do NOT rebuild permissions (ignore the nagging)
4. Add a "Group visibility" field to the group node type
5. Edit the node and select "Private"
6. Try to view the node with an authenticated user - you'll be able to see it despite the Private setting. This is true for both B and D7
7. Rebuild permissions at admin/reports/status/rebuild
8. Try to access the group again: you'll get Access denied

SO... D7 relied on the admin user to manually rebuild permissions through an annoying error msg. Backdrop does not. What's then the solution? I see three possible paths:

1. Add (re-add) the annoying nagging to Backdrop core so that admins know that permissions need to be rebuilt
2. OR, make rebuilding automatic on cache clearing (not ideal, as rebuilding can take a LONG time if you have 1000s of nodes)
3. OR Add a "local" nagging to OG through hook_init

I believe number 3 is probably the fastest way to get there.

Thoughts?

[argiepiano]

Added an issue in B's queue about creating a rebuild warning for admins https://github.com/backdrop/backdrop-issues/issues/5589

[argiepiano]

One more thought about how to approach this. I've taken a quick look at content_access contrib. In that module, they trigger an automatic rebuild if the number of nodes is below a threshold (1000). See content_access_mass_update(). When the number of nodes is above 1000, they do the same thing as OG does when enabled - they set the flag that a rebuilt is needed. Problem is: that flag does not trigger a warning in B.

I'm thinking that we could do two things (haven't tested this):

1. Use the threshold idea to trigger an automatic rebuild here (upon enabling)
2. Temporarily, while the B issue is addressed, we could show a warning that a manual rebuilt is needed if the number of nodes is above the threshold

[argiepiano]

I have submitted a PR for Backdrop core issue 5589 that shows a "red dot" warning on the admin bar when permissions need to be rebuilt. We could probably get this merged if one or more of you (@indigoxela, @olafgrabienski or @laryn) would do a code review and testing of that PR.

Once that's merged, that red warning will be shown whenever og_access is enabled and permissions need to be rebuilt.

[argiepiano]

@indigoxela, since I know you invested a lot of time into solving this puzzle, I wanted to share a few observations. I think I cracked the problem (which also affects some tests). Perhaps you can help me find a solution.

So, when you first save a group content node that "belongs" to a private group node, access permissions don't work properly, as you observed, even if you had rebuilt permissions when you enabled og_access. This means that the content can be seen by anyone even if the group is set to private, unless you rebuild permissions one more time.

It took a LOT of digging, but I have found the issue. It has to do with the order in which a couple of hooks are invoked when you save a new node.

In D7, the order is:

• hook_node_insert()
• hook_node_access_records() (invoked by node_access_acquire_grants())

Calling hook_node_insert() assures that the function og_group() runs first and creates an entry in the og_membership table, which establishes the new node's "membership" within the group node. Then og_node_access_records() takes care of creating the correct entry in the node_access() table that sets the correct grant id for the node just created.

You can see these two hooks being invoked one after the other in D7's node.module

    module_invoke_all('node_' . $op, $node);
    module_invoke_all('entity_' . $op, $node, 'node');

    // Update the node access table for this node.
    node_access_acquire_grants($node);

---

In Backdrop, as you may have guessed, this order is reversed. You can see this in NodeStorageController::save(). First the method $this->postSave() is invoked, which calls node_access_acquire_grants(). Then the method invokeHook() is called, which calls the insert hook.

So, in B, og_node_access_records() ends up being called before og_group(). The new node doesn't have a membership established before the grant ID is created, and therefore the new node ends up with a grant ID of "0", which means it's viewable by anyone.

---

The most obvious solution is to alter this order by patching B's core. But this may be hard, as postSave() may do other things besides calling node_access_acquire_grants() in potential extensions of the NodeStorageController class (or maybe not?). I can open an issue. What do you think?

I thought of using hook_node_presave(), but that can't be done as we need the nid to run og_group().

Any thoughts?

[argiepiano]

Oh, I just thought of a pretty simple solution. Invoking node_access_acquire_grants() at the end of og_group(). This means that node_access_acquire_grants() would be invoked twice, once before og_group() and once at the end, but it should be a pretty effective compromise if core is not fixed. I haven't tested this.... I'll do so tomorrow.

And another related issue. Entries in the og_membership table are never deleted in B when you delete group content nodes - this happens normally on D7.

[indigoxela]

in B, og_node_access_records() ends up being called before og_group()

Wow, so that explains a lot. Thank you so much for all your work!

given the current lack of interest in core committers in anything that requires work, I don't see this going anywhere. What do you think?

It may appear like that, but I don't think we should give up. Interest is there - I'm pretty sure. It's a complex problem, though, and core committers are humans with day jobs, too. :wink:

Invoking node_access_acquire_grants() at the end of og_group(). This means that node_access_acquire_grants() would be invoked twice, once before og_group() and once at the end

Hm, that's an interesting workaround. Do you see a chance to reorder things in og to get things working? I'll try to have a closer look at the whole process this weekend. Again, thank you so much for all your digging!

[indigoxela]

I wonder if hook_node_access_records could be useful for us.

[argiepiano]

I wonder if hook_node_access_records could be useful for us.

That hook is used in og - that's the hook that is called ahead of time in Backdrop. og_node_access_records() in og_access.module.

[argiepiano]

OK, I have confirmed that simply swapping the order of $this->invokeHook($op, $entity); and $this->postSave($entity, $op == 'update'); in NodeStorageController:save() solves this issue.

[argiepiano]

I submitted a PR for core to swap the order of invocations as described above. I suspect this will raise some eyebrows, and it may actually never make it - after all, this bug seemingly only affects fringe cases like OG.

Given that, I'll toy with an alternative as I describe above. If core ever gets changed, we'll need to come back to OG and change again. I'll try to submit a PR soon.

[argiepiano]

PR #65 submitted. This PR also allows tests in OgAccessTestCase to pass.

[indigoxela]

I submitted a https://github.com/backdrop/backdrop/pull/4049 for core to swap the order of invocations as described above. I suspect this will raise some eyebrows...

Yes, saw it and I think you found a real bug in core. And yes, I raised an eyebrow or two, but not in a dismissive, but rather in a confused way when looking at the existing core code. :wink:

[laryn]

Merged (with a todo to potentially revist the need for this when/if the core issue is addressed). Thanks @argiepiano and @indigoxela!

[olafgrabienski]

In Drupal, when enabling og_access, one gets prompted to "rebuild permissions" in a second step. This step does not happen in B

I've updated my test site including the merge of the last commit, and I noticed something I didn't look at before: In contrast to enabling, when I disable the OG access control sub-module, I get the message "Content permissions have been rebuilt". Is this difference of interest here?