Problem/Motivation

Currently menu module registering services to check access for menu and menu link entities deletion
This custom access should be moved to corresponding access controllers

Proposed resolution

Implement

  • MenuLinkAccessController
  • MenuAccessController

to encapsulate specific access for delete operations and extent them in #1842850: Untangle menu_link access control from _menu_link_translate() and friends for _menu_link_translate()

While menu entity lives in system module it's access controlled should stay there

API changes

No

CommentFileSizeAuthor
#42 interdiff.txt1.58 KBandypost
#42 access-controllers-2012916-42.patch12.67 KBandypost
#38 interdiff.txt2.08 KBandypost
#38 access-controllers-2012916-38.patch12.42 KBandypost
#36 interdiff.txt470 bytesandypost
#36 access-controllers-2012916-36.patch12.16 KBandypost
#34 interdiff.txt4.03 KBandypost
#34 access-controllers-2012916-34.patch12.16 KBandypost
#33 interdiff.txt1.24 KBandypost
#33 access-controllers-2012916-33.patch12.54 KBandypost
#28 interdiff.txt3.79 KBandypost
#28 access-controllers-2012916-28.patch12.54 KBandypost
#27 interdiff.txt765 bytesandypost
#27 access-controllers-2012916-27.patch10.59 KBandypost
#26 interdiff.txt3.03 KBandypost
#26 access-controllers-2012916-26.patch11.34 KBandypost
#22 menu-access-controllers-2012916-22.patch10.39 KBpwieck
#16 access-controllers-2012916-15.patch10.42 KBandypost
#15 menu-access-controllers-2012916-15.patch10.42 KBBerdir
#11 account-interface-ups-2012916.patch2.08 KBBerdir
#1 access-controllers-2012916-1.patch10.39 KBandypost
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Active » Needs review
FileSize
access-controllers-2012916-1.patch10.39 KB

Re-roll of #1842850-8: Untangle menu_link access control from _menu_link_translate() and friends

Status: Needs review » Needs work
Issue tags: -Entity Access, -Entity Field API

The last submitted patch, access-controllers-2012916-1.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Needs review

#1: access-controllers-2012916-1.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Entity Access, +Entity Field API

The last submitted patch, access-controllers-2012916-1.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Needs review

Test passes locally

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Status: Needs review » Reviewed & tested by the community

Looks good to me as a first step. We can deal with gutting _menu_link_translate() in the other issue.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Looks good to me, following the other issue. Committed/pushed to 8.x.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Fixed » Reviewed & tested by the community

This one was not pushed

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

git errored out on me with access denied... Now pushed.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

now ok

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Title: Implement access controller for the menu and menu link entity » HEAD BROKEN: Implement access controller for the menu and menu link entity
Status: Fixed » Needs review
FileSize
account-interface-ups-2012916.patch2.08 KB

Not so ok :)

Wasn't retested and conflicted with AccountInterface. This should fix it, but it's untested.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Fixed

Whoops and thanks, committed/pushed.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Fixed » Needs work

Actually reverted both the follow-up and the original patch, let's start back from this morning...

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Title: HEAD BROKEN: Implement access controller for the menu and menu link entity » Implement access controller for the menu and menu link entity
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
menu-access-controllers-2012916-15.patch10.42 KB

Combined the two patches.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
FileSize
access-controllers-2012916-15.patch10.42 KB

There was collision with #1825332: Introduce an AccountInterface to represent the current user

EDIT sorry, x-post

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

patches are identical

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

#15: menu-access-controllers-2012916-15.patch queued for re-testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs review » Reviewed & tested by the community

Patches are identical so Retest berdir's one

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll...

curl https://drupal.org/files/access-controllers-2012916-15.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 10669  100 10669    0     0   7739      0  0:00:01  0:00:01 --:--:--  9126
error: patch failed: core/modules/menu/menu.routing.yml:17
error: core/modules/menu/menu.routing.yml: patch does not apply
pwieck’s picture

pwieck CreditAttribution: pwieck commented

Working on reroll of #15

pwieck’s picture

pwieck CreditAttribution: pwieck commented
Status: Needs work » Needs review
FileSize
menu-access-controllers-2012916-22.patch10.39 KB

rerolled.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented 
+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkAccessController.phpundefined
@@ -0,0 +1,33 @@
+    $access = user_access('administer menu', $account);
+    if ($access && $operation == 'delete') {
+      // Only items created by the menu module can be deleted.
+      return $entity->module == 'menu';

Hm, so we have code for menu.module in the required menu_link.module.

We introduced a generic hook_$type_access() in the access controller base class, wondering if menu.module could implement this through that hook. Although I'm not quite sure how, maybe disallow delete by default and then menu.module allows to delete it's own menu links?

amateescu’s picture

amateescu CreditAttribution: amateescu commented

maybe disallow delete by default and then menu.module allows to delete it's own menu links?

Yes, I think that's a good plan.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Needs work
Issue tags: -Needs reroll

Let's do that then :)

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Needs review
FileSize
access-controllers-2012916-26.patch11.34 KB
interdiff.txt3.03 KB

New patch, reverted this questionable place to previous implementation!
... and removed deprecated user_access() calls

#24 Not sure it possible because 'administer menus' permission is defined in menu module which depends on menu_link

+++ /dev/nullundefined
@@ -1,37 +0,0 @@
-class DeleteLinkAccessCheck implements AccessCheckInterface {
...
-  public function access(Route $route, Request $request) {
-    if (user_access('administer menu') && $menu_link = $request->attributes->get('menu_link')) {
-      // Links defined via hook_menu may not be deleted. Updated items are an
-      // exception, as they can be broken.
-      return $menu_link->module !== 'system' || $menu_link->updated;

so it's a conversion

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
FileSize
access-controllers-2012916-27.patch10.59 KB
interdiff.txt765 bytes

access check for delete now lives in EntityListController

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
FileSize
access-controllers-2012916-28.patch12.54 KB
interdiff.txt3.79 KB

Digging deeper I found that _menu_overview_tree_form() uses different permissions. So introduced them and cleaned-up useless code.

Status: Needs review » Needs work
Issue tags: -Entity Access, -Entity Field API

The last submitted patch, access-controllers-2012916-28.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Needs review
Issue tags: +Entity Access, +Entity Field API

#28: access-controllers-2012916-28.patch queued for re-testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

This looks like it overlaps with the RTBC #1984702: Convert menu.module's page callbacks to Controllers

Status: Needs review » Needs work

The last submitted patch, access-controllers-2012916-28.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Needs review
FileSize
access-controllers-2012916-33.patch12.54 KB
interdiff.txt1.24 KB

Broken test should pass now

@tim.plunkett I think it's ok, I'll re-roll if this one goes first

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
FileSize
access-controllers-2012916-34.patch12.16 KB
interdiff.txt4.03 KB

Merge after #1984702: Convert menu.module's page callbacks to Controllers

And a bit more security fixes

Status: Needs review » Needs work

The last submitted patch, access-controllers-2012916-34.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Needs review
FileSize
access-controllers-2012916-36.patch12.16 KB
interdiff.txt470 bytes

we need stop to mix edit and update

Status: Needs review » Needs work

The last submitted patch, access-controllers-2012916-36.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Needs review
FileSize
access-controllers-2012916-38.patch12.42 KB
interdiff.txt2.08 KB

should be fine mostly

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
@@ -0,0 +1,48 @@
+ * @see \Drupal\menu_link\Plugin\Core\Entity\MenuLink

Potentially we should also typehint the MenuLinkInterface

@@ -0,0 +1,34 @@
+    if ($operation == 'delete') {
...
+      $system_menus = menu_list_system_menus();
+      return !isset($system_menus[$entity->id()]);

Just wondering why we do not check for 'administer menu' as well.

@@ -0,0 +1,34 @@
+    return user_access('administer menu', $account);

This should be $account->hasPermission.

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Status: Needs review » Reviewed & tested by the community

Looks great to me.

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Status: Reviewed & tested by the community » Needs work

Except that I didn't catch that user_access() call.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Needs review
Issue tags: +CodeSprintCIS
FileSize
access-controllers-2012916-42.patch12.67 KB
interdiff.txt1.58 KB

MenuAccessController should live in system module where the entity defined, so moved current implementation with clean-up

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Status: Needs review » Reviewed & tested by the community

Thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 972406d and pushed to 8.x. Thanks!

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