Problem/Motivation

When a user who does not have the "administer menu" permission edits and saves a node that has a menu link, the link is moved to the first position in its menu branch. The weights of all other links in that branch are also rewritten.

Users with "administer menu" do not see this, because the tree widget is displayed for them.

Cause:

  • Core's menu_ui sets #access on the menu element to hasPermission('administer menu'). Hidden form elements still keep their default values, so $form_state->getValue('menu') reports the link as enabled, with its current parent.
  • NodeFormSubmitHandler::handleFormSubmit() is added to the node form's submit buttons. It reads prev_sibling and next_sibling from $form_state->getUserInput(). These are only posted when the tree widget was displayed and used.
  • When the user cannot see the menu section, prev_sibling is empty. The handler treats that as "place the link first" (array_unshift()) and renumbers all weights in the branch starting at -50.

Steps to reproduce

  1. Enable the tree widget (use_tree_widget) for a content type, with a menu in available_menus.
  2. Create several nodes with menu links under the same parent, and put the link of one node in the middle or at the end.
  3. Log in as a user who can edit that node but does not have "administer menu".
  4. Edit and save the node without changing anything.
  5. The node's menu link is now first in its branch, and the weights of the other links in the branch are rewritten.

Proposed resolution

Do not reposition links when the user cannot access the menu section. In NodeFormSubmitHandler::handleFormSubmit(), return early if $form['menu']['#access'] is false (an AccessResultInterface value is also handled). The check is in the handler, not in Hooks::nodeFormAlter(), so that access changed by other modules is respected.

Editing a node without access to the menu settings then leaves the link's parent and weight unchanged, as it does in core.

Remaining tasks

  • Review the merge request for 3.x.
  • Run the new functional test (MenuTreeAccessTest) in CI.
  • Backport to 2.x.

User interface changes

None.

API changes

None.

Data model changes

None.

Issue fork menu_tree-3627619

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:

About issue forks

Comments

stockticker created an issue. See original summary.

stockticker opened merge request !16

stockticker opened merge request !17

stockticker’s picture

stockticker commented
Assigned: stockticker » Unassigned
Status: Active » Needs review

Opened two merge requests: !16 against 3.x and !17 against 2.x (backport). The fix skips repositioning in the submit handler when the user can't access the menu settings, so users without "administer menu" no longer move links to the first position. A functional test (MenuTreeAccessTest) is included in both MRs.

Tested manually on a production site running 2.1.1: before the change, an editor saving a page moved its link to first; after the change, the link keeps its position and admins can still reorder links. The new test hasn't been run, because the project doesn't have CI configured. Could CI be enabled for the project? Could you review and tag a release for 2.x and 3.x?

peter törnstrand’s picture

peter törnstrand commented
Status: Needs review » Fixed

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.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.