Refine menu permissions - add separate one for menu OTF

bumping a version since the main bug has been fixed.

I agree that there could be a separate permission to let users add content to the menu without fully administering the menu.

Simple patch to add a second permission to menu module: permission to access the menu settings fieldset in node forms.

Ok, changes made.

The patch works nicely for me.

I'm on crack. Are you, too?

+++ modules/menu/menu.module	30 Aug 2009 21:51:17 -0000
@@ -41,6 +41,10 @@ function menu_permission() {
+    'access node menu settings' => array(
+      'title' => t('Access node menu settings'),
+      'description' => t('Manage menu links on node forms.'),
+    ),

I'd prefer we didn't use the word 'node' in user-facing text (yes, I know about 'administer nodes' and it can bite me ;))

How about more explicit? "place content into menus" or something?

This review is powered by Dreditor.

Changed the perm description to 'place content into menus', that seems fine. It's hard to succinctly say 'Use the options in the menu settings fieldset which appears on node add/edit forms in order to add, edit or delete that node's menu settings.' but I think this suffices..

+++ modules/menu/menu.module	30 Aug 2009 21:51:17 -0000
@@ -41,6 +41,10 @@ function menu_permission() {
+    'access node menu settings' => array(
+      'title' => t('Access node menu settings'),
+      'description' => t('Place content into menus.'),
+    ),

Sorry, this will teach me to try and review patches at ice cream sprints.

I meant something more like:

+    'place content into menu' => array(

+      'title' => t('Place content into menu'),

+      'description' => t('Access menu controls on content forms to place content into menus.'),

+    ),

We don't use "node" in user-facing text.

I'm tagging "needs usability review" in the hope that someone can come up with something better than the above.

I'm on crack. Are you, too?

This issue slightly overlaps with #38777: Menu OTF needs its own permission

Changed the text to the way webchick put it.

This looks like a handy improvement.

However, could the text be misleading? We're not adding "content", like an image, or intro text into a menu. We're adding links.

Patch #13: "Access menu controls on content forms to place content into menus"

Patch attached: "Access menu controls on content forms to place links to content into menus"

 'place content into menus' => array(
      'title' => t('Link to content in menus'),
      'description' => t('Access menu controls on content forms to place links to content into menus.'),
    ),

Changing to needs review, and adding tag.

I looked over #38777: Menu OTF needs its own permission and this looks like an isolated solution, but sun suggests there is a larger problem. If the larger problem isn't solved this would be an improvement for finer grained menu permissions?

Here's a reroll, with permission description update to 'Add links to content in menus while editing content.'

+++ modules/menu/menu.module	29 Nov 2009 20:55:53 -0000
@@ -48,6 +48,10 @@ function menu_permission() {
+    'place content into menus' => array(
+      'title' => t('Add links to content in menus'),
+      'description' => t('Add links to content in menus while editing content.'),
+    ),
@@ -576,7 +580,7 @@ function menu_form_alter(&$form, $form_s
     $form['menu'] = array(
       '#type' => 'fieldset',
       '#title' => t('Menu settings'),
-      '#access' => user_access('administer menu'),
+      '#access' => user_access('place content into menus') || user_access('administer menu'),

This doesn't really match up. The title says "Add", the internal permission says "place".

According to the code, this permission not only allows you to add links for nodes, but also edit them, and potentially delete them.

I'd suggest to name at least the internal permission more precisely, perhaps "manage links" or "edit links".

I'm on crack. Are you, too?

It's hard to put it concisely. How's this one?

#23: menu.patch queued for re-testing.

I'm fine with that new wording.

Marked #38777: Menu OTF needs its own permission as a duplicate of this issue.

This wording makes sense to me, as a non-native English-speaker. RTBC.

+++ modules/menu/menu.module
@@ -43,7 +43,10 @@ function menu_help($path, $arg) {
+    'edit content menu links' => array(
+      'title' => t('Edit menu links to content while editing content'),
     ),

I don't really care about the human-readable title; but the internal permission name has a completely different meaning.

+++ modules/menu/menu.module
@@ -601,7 +604,7 @@ function menu_form_alter(&$form, $form_state, $form_id) {
-      '#access' => user_access('administer menu'),
+      '#access' => user_Access('edit content menu links') || user_access('administer menu'),

Case error in function name.

Powered by Dreditor.

How about this wording?

--- modules\menu\menu.module
+++ modules\menu\menu.module
@@ -43,7 +43,10 @@
 function menu_permission() {
   return array(
     'administer menu' => array(
-      'title' => t('Administer menus and menu items'),
+      'title' => t('Administer menus and menu links'),
+    ),
+    'edit node menu item' => array(
+      'title' => t('Edit menu links to content while editing content'),
     ),
   );
 }
@@ -601,7 +604,7 @@
     $form['menu'] = array(
       '#type' => 'fieldset',
       '#title' => t('Menu settings'),
-      '#access' => user_access('administer menu'),
+      '#access' => user_access('administer menu') || user_acess('edit node menu item'),
       '#collapsible' => TRUE,
       '#collapsed' => !$link['link_title'],
       '#group' => 'additional_settings',

Alright, trying again.

Is there still a chance of this getting into 7.x? If so, I'll update the patch.

#33: 154471.patch queued for re-testing.

Sounds oke?

Why has this been abandoned? The new permission is really missing from 7.x to make content creation (e.g., Wiki-style pages) for normal users feasible.

Marking this a feature request, since everything works as designed, even if it's risky to let end-users manage menus.

This patch takes a new approach, as discussed with @pwolanin on IRC. Instead of focussing on nodes, we expand the menu system with content-specific permissions for menu links. By separating permissions for config and content, we can ensure that end users can manage links, which are content, without being able to (accidentally) change menu configuration. The new access controller has full test coverage, but not all content usages of the old administer menu permission have been converted yet, and neither have the corresponding tests.

Some issues we need to decide on:

1. Should we convert config permissions in the same patch? It would prevent an awkward in-between state where the "administer menu" permission is used as an undocumented bypass permission for content, but as the primary permission for config.
2. What are static menu link overrides? They seem similar to the menu link storage, but are defined in config as opposed to in the database. MenuLinkContent provides similar behavior, but stores the overrides in entity storage rather than config.
3. About administer menu: We can keep it as a bypass permission, which ensures continuity, but requires complex access checks. We can also deprecate it, which means we keep it around for contributed modules and existing site configurations to use, but we take the permission away from all roles that have it, and replace it with all available new permissions. This requires configuration permissions to be converted in the same patch. A third alternative is to keep the permission for config only, and just add the new content permissions to all roles that have the old permission.
4. MenuLinkContentAccessControlHandler uses the administer menu permission to check "view" access. This was added in #2558905-15: Content translation module - Information disclosure by insufficient access checking, but the reason for this addition is not clear to me, as well as alternative solutions to the problem this check solves.

Thank you for sharing your idea for improving Drupal.

We are working to decide if this proposal meets the Criteria for evaluating proposed changes. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or there is no community support. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

Wanted to bump 1 more time if summary could be cleaned up.