Make WordPress Core

Opened 10 years ago

Last modified 2 years ago

#29213 assigned enhancement

Introduce capability for access to nav-menus.php

Reported by: westonruter's profile westonruter Owned by: johnbillion's profile johnbillion
Milestone: Future Release Priority: normal
Severity: normal Version: 3.0
Component: Menus Keywords: dev-feedback needs-unit-tests needs-patch granular-capabilities
Focuses: administration Cc:

Description (last modified by westonruter)

Management of the nav menus currently requires edit_theme_options capability, a capability associated with administrators which grants the power to make many wide sweeping changes. There should be a discrete capability edit_nav_menus just for managing menus, one that is inherited for anyone who has edit_theme_options by default. This was done for Customizer access in #28605 with the introduction of a customize capability.

Originally brought up in #14386.
For introducing a manage_widgets capability, see #31020.

Change History (32)

#1 @celloexpressions
10 years ago

+1

Like with customize, it should be a meta capability.

@jesin
10 years ago

#2 @jesin
10 years ago

I created patch 29213.diff as a starting point based on changesheet [29170] and added edit_nav_menus capability through the map_meta_cap filter. However accessing wp-admin/nav-menus.php displayed the following error:

You do not have sufficient permissions to access this page.

This was because user_can_access_admin_page() was returning false. Digging deeper I found that $_wp_menu_nopriv contained the following value:

$_wp_menu_nopriv["nav-menus.php"] = true;

To test how customize worked I used the same filter and printed $_wp_menu_nopriv. This is what I saw:

$_wp_menu_nopriv["customize.php?return=%2Fwp-admin%2Fcustomize.php"] = true;

But wp-admin/customize.php was accessible, so I narrowed down to the user_can_access_admin_page() function's following lines:

if ( isset( $_wp_menu_nopriv[$pagenow] ) )
    return false;

The $pagenow variable only contains the filename without query strings so when customize was used this line checked for isset( $_wp_menu_nopriv["customize.php"] ) which obviously caused this if loop to be skipped with user_can_access_admin_page() returning true.

Now the question is why did customize.php and nav-menus.php end up in the $_wp_menu_nopriv list when appropriate capabilities were being used.

The answer is in lines 93 - 117 of file wp-admin/includes/menu.php. Specifically the following lines:

if ( $new_parent != $old_parent ) {
    $_wp_real_parent_file[$old_parent] = $new_parent;
    $menu[$id][2] = $new_parent;

This piece of code replaces the parent menu's filename with the filename of its child menu. This results in the following values:

Array
(
	[0] => Appearance
	[1] => edit_theme_options
	[2] => nav-menus.php
	[3] =>
	[4] => menu-top menu-icon-appearance
	[5] => menu-appearance
	[6] => dashicons-admin-appearance
)

Notice how this menu got its parent's capability requirement. This causes nav-menus.php (or customize.php?return=%2Fwp-admin%2Fcustomize.php) to be added to the $_wp_menu_nopriv list.

So when filenames are being replaced we should also replace their capability requirement.

#3 @westonruter
10 years ago

  • Milestone changed from Awaiting Review to Future Release

#4 @westonruter
9 years ago

  • Milestone changed from Future Release to 4.2

We'll be working to get this finalized for 4.2, along with related patches to add fine-grained caps for managing Widgets.

#5 @ocean90
9 years ago

  • Milestone changed from 4.2 to Future Release

Let's wait for a patch first.

#6 @westonruter
9 years ago

There is a patch :-)

#7 @johnbillion
9 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Future Release to 4.2
  • Owner set to westonruter
  • Status changed from new to assigned

#8 @celloexpressions
9 years ago

I'm not sure about the name edit_nav_menus. Menu items are post objects (menus are taxonomies), and as such there are several related capabilities that could exist at a more fine-grained level. I think for the tasks of creating, editing, and deleting menus, manage_nav_menus would be appropriate as all-encompassing of being able to do stuff with menus.

#9 @westonruter
9 years ago

Good call. For accessing the admin UI, the manage_nav_menus makes sense I think. And then for users who can edit_theme_options could then also by default have the edit_nav_menus, delete_nav_menu, etc capabilities.

#10 @westonruter
9 years ago

  • Keywords dev-feedback added
  • Owner changed from westonruter to ocean90
  • Status changed from assigned to reviewing

In 29213.2.diff:

  • Use manage_menus cap instead of edit_nav_menus. The use of “menus” makes sense instead of “nav_menus” because the theme support is called “menus”. Though at the same time, the post type is called “nav_menu_item” and the taxonomy is called “nav_menu”. I am not partial to either capability. I think celloexpressions should have the last word :-)
  • Bubble-up submenu admin page capability in addition to location; replace label if no children left. This is based on the great research by jesin. If the user lacks the capability for the original menu parent, then the capability and location of the first sub-menu item is assigned to the parent. If there is only one submenu item anyway, then the label is also copied up to the parent item.
  • Show the “Menus” link in the admin bar if the user can manage_menus. Also, the change here also allows other menu items (specifically “Customize”) to appear in the admin bar if they have the required capability. Previously, the entire menu would short-circuit if the user lacked edit_theme_options.

Note that the changes to allow the Menus link to appear in the admin menu and the admin bar were also initially done in the Customizer for #28605. However, it was decided on IRC that Core support for displaying the Customize links would be removed and it would have to be manually added in by a plugin that also granted the user the customize capability. We'll need to decide if that decision remains.

#11 @westonruter
9 years ago

By the way, here is a sample plugin which grants access to manage_menus for a user who can edit_posts:

<?php
function allow_users_who_can_edit_posts_to_customize( $caps, $cap, $user_id ) {
        $required_cap = 'edit_posts';
        if ( 'manage_menus' === $cap && user_can( $user_id, $required_cap ) ) {
                $caps = array( $required_cap );
        }
        return $caps;
}
add_filter( 'map_meta_cap', 'allow_users_who_can_edit_posts_to_customize', 10, 3 );

#12 @westonruter
9 years ago

  • Description modified (diff)

#13 @westonruter
9 years ago

In 29213.3.diff: If there are no menus, check manage_menus cap in WP_Nav_Menu widget, and if the user lacks the cap, don't show a link to nav-menus.php

#14 @westonruter
9 years ago

In 29213.4.diff: Use manage_menus cap check instead of edit_theme_options in menu WP Ajax requests

#15 @westonruter
9 years ago

In 29213.5.diff: Use manage_menus capability for Customizer section and settings for Navigation

#16 @westonruter
9 years ago

  • Keywords 2nd-opinion added

This change is causing a problem:

Bubble-up submenu admin page capability in addition to location; replace label if no children left. This is based on the great research by jesin. If the user lacks the capability for the original menu parent, then the capability and location of the first sub-menu item is assigned to the parent. If there is only one submenu item anyway, then the label is also copied up to the parent item.

The legacy "Link Categories" menu to appear as a top-level admin menu item when the current user can manage_categories but cannot manage_links. In the beautiful wp-admin/menus.php we have:

<?php
// ...
$menu[15] = array( __('Links'), 'manage_links', 'link-manager.php', '', 'menu-top menu-icon-links', 'menu-links', 'dashicons-admin-links' );
        $submenu['link-manager.php'][5] = array( _x('All Links', 'admin menu'), 'manage_links', 'link-manager.php' );
        /* translators: add new links */
        $submenu['link-manager.php'][10] = array( _x('Add New', 'link'), 'manage_links', 'link-add.php' );
        $submenu['link-manager.php'][15] = array( __('Link Categories'), 'manage_categories', 'edit-tags.php?taxonomy=link_category' );

As described above, the behavior is to move a submenu item to the parent if there is only one submenu item. In this case, the parent item was also inaccessible. The quick fix here could be to wrap this entire block in a if ( get_option( 'link_manager_enabled' ) ) check. However, I'm afraid that there could be other instances where submenu items inadvertently bubble up to the top admin menu, due to the brittleness of wp-admin/menus.php.

For the admin menu, at least, we may need to go the route of the customize cap and just require that plugins manually add admin menu items for the managing menus.

The links in the admin bar, however, could be a different story since it is not brittle.

Thoughts?

This ticket was mentioned in Slack in #core by drew. View the logs.


9 years ago

This ticket was mentioned in Slack in #core by drew. View the logs.


9 years ago

This ticket was mentioned in Slack in #core by drew. View the logs.


9 years ago

#20 @DrewAPicture
9 years ago

  • Milestone changed from 4.2 to Future Release

Citing discussion in Slack with @westonruter and later, @ocean90, punting this to a future release along with #31020.

#21 @westonruter
8 years ago

  • Owner changed from ocean90 to westonruter
  • Status changed from reviewing to accepted

This ticket was mentioned in Slack in #core by sergey. View the logs.


8 years ago

#23 @johnbillion
7 years ago

  • Keywords early added
  • Owner changed from westonruter to johnbillion
  • Status changed from accepted to assigned

#24 @johnbillion
7 years ago

  • Keywords needs-unit-tests added; 2nd-opinion removed

#25 @westonruter
7 years ago

  • Keywords needs-patch added; has-patch removed

Patch needs to be updated to use capability inWP_Customize_Nav_Menus.

#26 @flixos90
7 years ago

  • Keywords granular-capabilities added

All of these are part of a larger goal.

#27 @johnbillion
2 years ago

  • Keywords early removed
Note: See TracTickets for help on using tickets.