Problem/Motivation

Drupal 7 OG Menu provided an administer og menu permission scoped to OG roles, allowing group administrators to manage their own menus without site-wide permissions.

OG Menu 2.x instead relies mainly on global entity permissions for creating, viewing, editing, and deleting OG Menu instances. This means group administrators currently need broad site-wide permissions to manage their group's menu.

Proposed resolution

  • Restore the group-scoped administer og menu permission with the Drupal 7 semantics and rename to disambiguate the global permission to 'administer og menu instances'
  • Use group-aware access checks for OG Menu instance operations.
  • Keep the existing global entity permissions as site-wide overrides.
  • Keep add new links to og menu instance entities as a narrower optional permission.
  • Return appropriate access cacheability metadata.

Remaining tasks

  • Add automated coverage for allowed, denied, cross-group, and global-override access.
  • Confirm a user with administer og menu can create, view, and edit menus only for groups where they hold that permission.

Issue fork og_menu-3616205

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

joelpittet created an issue. See original summary.

joelpittet’s picture

Status: Active » Needs review

While adding coverage for the site-wide permission, I hit a pre-existing access bug that becomes reachable with this change.

OgMenuInstanceAccessControlHandler overrides checkAccess() completely, which drops core's guard that forbids deleting an unsaved entity. This has existed since the handler was added, but no existing permission path granted delete access to a menu instance, so the result remained neutral and the existing test still passed.

Once the global `administer og menu` permission grants delete access, that gap becomes visible and the test fails. The MR restores the guard in the access service:

  if ($operation === 'delete' && $instance->isNew()) {
    return AccessResult::forbidden()->addCacheableDependency($cacheability);
  }

It also adds coverage for an unsaved instance that has a group, which the existing test does not exercise.

I think this is small and closely related enough to keep here, but happy to split it into a follow-up if preferred.

joelpittet’s picture

Issue summary: View changes

Added the rename to the plan in the issue summary 'administer og menu instances'

joelpittet’s picture

StatusFileSize
new142.82 KB
new180.11 KB

We need to cover edit/delete here too:

D7:
D7

D10:
D10

joelpittet’s picture

StatusFileSize
new167.44 KB

Progress this is the result of applying MR:
D10 after fixes

Edit: the Audience field moving up and linking is totally unrelated (if you're wondering), that was just form display config

claudiu.cristea’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me. Feel free to merge when you consider. Thank you

  • joelpittet committed a00f0492 on 2.x
    task: #3616205 Restore group-scoped administer og menu permission
    
    By:...
joelpittet’s picture

Status: Reviewed & tested by the community » Fixed

Thanks again @claudiu.cristea for the review.

I put together a couple of quick roadmaps #3623047: [meta] 2.0.0-beta1 release roadmap and #3623048: [meta] 2.0.0 stable release roadmap

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.