Move "Administer OG menus" to OG roles/permissions

subscribing

Interesting, will look into it.

Can you provide an example use case where this would be useful?

This will be postponed for the next branch as it will break existing setups when upgrading.

Attached is a patch that implements this, without changing the actual permissions levels. This doesn't do as much as suggested in #3. It just exposes the existing "administer og menu" permission through the Organic Groups custom permissions interface. This allows you to designate a non-group-admin role that can manage the menu. It works in my setup. One question I had though is in og_menu_access() you are checking for:

if (node_access('update', $node)) {

A non-group-admin is not going to be able to do that and it prevents that user from even seeing the Menus tabs on the group. So, I replaced with it with:

if (user_access('administer og menu') || og_user_access_by_entity('administer og menu', 'node', $node->nid)) {

That more explicitly checks for og menu admin permission, but there may be something I'm missing here. Why did you have it checking for node update access before?

This patch is again og_menu-7.x-2.x-dev.

The patch in comment 5 applies cleanly and provides the functionality as defined in the initial ticket. Also as far as I can tell it addresses the functionality described in comment 3 too. I think I might be missing something on the functionality described in 3 though.

I cannot tell how this would break existing sites.

1. Previously the global permission was available
2. After patching the existing global permission is still available but it is also available at a "group permission level" eg. /admin/config/group/permissions

Steps for setup:

1. Apply patch
2. Give "Administer OG menus" permission to roles you want to be able to edit *any* og menu via admin/people/permissions
3. give 'Administer OG menus' to 'Administrator Member' (should this be default?) via 'admin/config/group/permissions' and any other og roles you want to be able to edit the og menu for specific groups

As long as no og roles are given the 'Administer OG Menus' permission I cannot think of a way this would break existing installs.

I don't see why this patch can't be committed now. Setting to "needs review" for further input but this is RTBC IMO.

In #5

suggestion to replace

if (node_access('update', $node)) {

to

if (user_access('administer og menu') || og_user_access_by_entity('administer og menu', 'node', $node->nid)) {

makes no sense, if you scroll up in the code you'll see that snippet is already part of an if statement which begins with

  elseif (user_access('administer og menu')) {

tbh, I don't know exactly why or how the node_access check got there.

I took a bit of time to more closely review og_menu_access, and found a few things.

Several issues are addressed in this re-rolled patch;

1. og_menu_menu(): parameters to og_menu_edit_item_form incorrect when adding a new link
2. og_menu_access()
   • if no node object return FALSE in all cases, this was previously nested deeper, but if we don't have a node we shouldn't be doing anything with a menu or menu links.
   • adjust access per rv0's comment in 7, we want to execute that branch of logic if the user has global permissions or rights to this particular group menu
   • adjust logic to return TRUE for all other non new menu cases, we aren't concerned about group membership since that is now governed by global permission and og level permission.
3. og_menu.pages.inc: fixed exposed issue with clobbered menu variable

I think it is worth adjusting the entries for og_menu_menu() to always pass the $op parameter to og_menu_access(). As it stands right now we only ever get NULL or 'new-menu'. I think the resulting code would be much more easy to read if we switched to always expect the op parameter and branch the access control logic accordingly. In the attached patch we punt and return TRUE in the event that $op=NULL and the user has access to global 'administer og menu' or og level 'administer og menu'.

re-roll of patch in 8 to account for recent commit.

Patch reviewed and pushed to latest dev.

Thanks