Not respecting extended group context

+++ og_user_roles.module	24 Oct 2009 15:13:34 -0000
@@ -123,6 +123,10 @@ function og_user_roles_init() {
+  // OG original context fallback

This inline comment needs more explanation about what is done here and why.

+++ og_user_roles.module	24 Oct 2009 15:13:34 -0000
@@ -123,6 +123,10 @@ function og_user_roles_init() {
+  if(!$group_node) {

There should be a space between the if and the opening parenthesis. See http://drupal.org/coding-standards

Also, did you run the tests with this patch applied?

I'm on crack. Are you, too?

I've run into this same limitation. I'm trying to deploy this module on localize.drupal.org to make use of multiple user levels in groups. Currently, users are either admins or members of a group and their permission in the group to submit translations is dependent on this level. However, if you are not running og, the module has way more granular permissions. So our og model currently limits permissions to the og membership levels because we do not (yet) have in-og role controls. og_user_roles seems to be the way to go, but in our case, we actually need permission settings for our own tools based on og membership, so we need to have og_user_roles compatible with custom contexts.

Here is an updated patch. I've run the og_user_role tests, and got 87 passes and no exceptions/fails with this patch.

Manual verification of the patches site also proves that it indeed has an effect on menu items now showing up where a role is granted. :)

Thanks!

Would it be possible to test this advanced context somehow? This patch is touching the core functionality of OGUR - which is privilege escalation - so we better make sure that this does not grant more than we want. ;)

Better title.

@sun: Well, og_init() has this code snippet:

  // Set group context and language if needed.
  if ($group_node = og_determine_context()) {
    og_set_theme($group_node);
    og_set_group_context($group_node);
    // TODOL: is this too late for menu links and such?
    og_set_language($group_node);
  }

So what it does basically, is to use the context determination code which is very similar to OGUR's copy of the same og_determine_context() function. Then if that is determined, it sets that context. I did not find any other place og module would set the context (apart from views handlers).

Now OGUR has a copy of og_determine_context(), so if that gets us a context, it is already set in OGUR. It is only some "third party" code that sets the context if it is different in any way. Such as the views handlers in og_views. So we can write a mock module, which sets a context on an URL we pick and test for that. Better ideas?

Hm. I now visually compared og_determine_context() and og_user_roles_determine_context(). There is no difference aside from a bug that pwolanin fixed (which probably should have been fixed in og.module as well, or better yet, first).

We only implement this fork, because we need to circumvent some statically cached access check results in the menu system (as described in the code docs). But aside from that, og_user_roles does not set a group context and it also shouldn't, because that is the responsibility of og.module. All we do is to use the very same logic as in og_determine_context() to determine the context of the current page, so we can assign additional user roles before the rest of the page logic runs. The immediate next step in the page logic is og_init(), which determines the context on its own and sets the context, if it is valid.

Yes, and that is what we pick up? So any good ideas to have an automated test for this besides my explanation in #6? (I'd really love to deploy this on localize.drupal.org and start to give translation teams more control, especially the German team who are waiting in part on this being deployed to join at all).

ok, I investigated this some more, and perhaps even would be fine to commit without a test.

However, are you sure this patch has an effect at all?

AFAICS, the flow in OG module is this:

1. og_init() tries to determine a group context.
2. Only if a group context could be determined in og_init(), og_set_group_context($group_node) is invoked to set it.
3. If no group context could be determined, then regular page processing goes on without a group context.
4. og_set_group_context() is effectively passed a $group_node in og_init() only.

OGUR only determines the context like OG does, but does not set it.

Now you mentioned og_views, and so I had a look at that.

og_views indeed invokes og_set_group_context() in various views argument and filter handlers with a $group_node.

However, those calls are invoked by the page callback of the current page, i.e. Views. This happens after hook_init().

This patch tries to gather that information during hook_init(), where the page callback is not invoked yet, so no views handlers are invoked yet.

This means that your patch could perhaps only work if og_views module had a weight of -2, so it would run before OGUR (which runs before OG).

Hrmmmm... That's quite an interesting circular problem, which I did not take into consideration yet. :-/

If this patch really has an effect without altering og_views' module weight, then I'd like to know why, and we absolutely have to document that in-code.

Well, my use case does not involve og_views, and my custom og context setting is done based on the path. Ie. there are sections of the site which belong to the same group, but are not og nodes or og_views. All the translation UI for a language is set to belong to that group, so admins will be able to hand out permissions. That is my usecase. I'm hapenning to set group context in l10n_group_init(), which happens to run before OGUR, because of the alphabet. Matter of luck in my case I guess (except I'm using hook_init() to be as early as possible for anybody else respecting group context).

Sorry for letting you wait.

Yes, I'd be fine with committing this patch, but we want to clarify the inline docs to make clear that this is heavily bound to module weights and hook_init() implementations that force a certain group context. OGUR itself alters its weight to -1 upon installation, so anything that wants to set a custom context either needs to have a weight of -2 or needs to be invoked earlier by coincidence (exact filesystem placement respecting modules/, sites/all/modules/, sites/[domain]/modules/, profiles/[profile]/modules/).

@sun: new patch with even more comments, explaining that weights might need to be set.

Thanks for reporting, reviewing, and testing! Committed attached patch.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.