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

Files: 
CommentFileSizeAuthor
#42 interdiff.txt1.58 KBandypost
#42 access-controllers-2012916-42.patch12.67 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 57,793 pass(es).
[ View ]
#38 interdiff.txt2.08 KBandypost
#38 access-controllers-2012916-38.patch12.42 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 57,767 pass(es).
[ View ]
#36 interdiff.txt470 bytesandypost
#36 access-controllers-2012916-36.patch12.16 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 57,713 pass(es), 20 fail(s), and 24 exception(s).
[ View ]
#34 interdiff.txt4.03 KBandypost
#34 access-controllers-2012916-34.patch12.16 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 55,714 pass(es), 95 fail(s), and 48 exception(s).
[ View ]
#33 interdiff.txt1.24 KBandypost
#33 access-controllers-2012916-33.patch12.54 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 57,314 pass(es).
[ View ]
#28 interdiff.txt3.79 KBandypost
#28 access-controllers-2012916-28.patch12.54 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 58,042 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#27 interdiff.txt765 bytesandypost
#27 access-controllers-2012916-27.patch10.59 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 57,711 pass(es).
[ View ]
#26 interdiff.txt3.03 KBandypost
#26 access-controllers-2012916-26.patch11.34 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 57,657 pass(es).
[ View ]
#22 menu-access-controllers-2012916-22.patch10.39 KBpwieck
PASSED: [[SimpleTest]]: [MySQL] 58,258 pass(es).
[ View ]
#16 access-controllers-2012916-15.patch10.42 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 57,316 pass(es).
[ View ]
#15 menu-access-controllers-2012916-15.patch10.42 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 57,569 pass(es).
[ View ]
#11 account-interface-ups-2012916.patch2.08 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 57,334 pass(es).
[ View ]
#1 access-controllers-2012916-1.patch10.39 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 54,994 pass(es).
[ View ]

Comments

andypost’s picture

Status:Active» Needs review
StatusFileSize
new10.39 KB
PASSED: [[SimpleTest]]: [MySQL] 54,994 pass(es).
[ View ]

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

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

Status:Needs work» Needs review

Test passes locally

amateescu’s picture

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

Status:Reviewed & tested by the community» Fixed

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

andypost’s picture

Status:Fixed» Reviewed & tested by the community

This one was not pushed

catch’s picture

Status:Reviewed & tested by the community» Fixed

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

andypost’s picture

now ok

Berdir’s picture

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
StatusFileSize
new2.08 KB
PASSED: [[SimpleTest]]: [MySQL] 57,334 pass(es).
[ View ]

Not so ok :)

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

catch’s picture

Status:Needs review» Fixed

Whoops and thanks, committed/pushed.

catch’s picture

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

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

Status:Needs work» Needs review
StatusFileSize
new10.42 KB
PASSED: [[SimpleTest]]: [MySQL] 57,569 pass(es).
[ View ]

Combined the two patches.

andypost’s picture

StatusFileSize
new10.42 KB
PASSED: [[SimpleTest]]: [MySQL] 57,316 pass(es).
[ View ]
andypost’s picture

patches are identical

andypost’s picture

andypost’s picture

Status:Needs review» Reviewed & tested by the community

Patches are identical so Retest berdir's one

alexpott’s picture

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

Working on reroll of #15

pwieck’s picture

Status:Needs work» Needs review
StatusFileSize
new10.39 KB
PASSED: [[SimpleTest]]: [MySQL] 58,258 pass(es).
[ View ]

rerolled.

Berdir’s picture

+++ 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

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

Status:Needs review» Needs work
Issue tags:-Needs reroll

Let's do that then :)

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new11.34 KB
PASSED: [[SimpleTest]]: [MySQL] 57,657 pass(es).
[ View ]
new3.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

StatusFileSize
new10.59 KB
PASSED: [[SimpleTest]]: [MySQL] 57,711 pass(es).
[ View ]
new765 bytes

access check for delete now lives in EntityListController

andypost’s picture

StatusFileSize
new12.54 KB
FAILED: [[SimpleTest]]: [MySQL] 58,042 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
new3.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

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

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

Status:Needs work» Needs review
StatusFileSize
new12.54 KB
PASSED: [[SimpleTest]]: [MySQL] 57,314 pass(es).
[ View ]
new1.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

StatusFileSize
new12.16 KB
FAILED: [[SimpleTest]]: [MySQL] 55,714 pass(es), 95 fail(s), and 48 exception(s).
[ View ]
new4.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

Status:Needs work» Needs review
StatusFileSize
new12.16 KB
FAILED: [[SimpleTest]]: [MySQL] 57,713 pass(es), 20 fail(s), and 24 exception(s).
[ View ]
new470 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

Status:Needs work» Needs review
StatusFileSize
new12.42 KB
PASSED: [[SimpleTest]]: [MySQL] 57,767 pass(es).
[ View ]
new2.08 KB

should be fine mostly

dawehner’s picture

@@ -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

Status:Needs review» Reviewed & tested by the community

Looks great to me.

amateescu’s picture

Status:Reviewed & tested by the community» Needs work

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

andypost’s picture

Status:Needs work» Needs review
Issue tags:+CodeSprintCIS
StatusFileSize
new12.67 KB
PASSED: [[SimpleTest]]: [MySQL] 57,793 pass(es).
[ View ]
new1.58 KB

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

amateescu’s picture

Status:Needs review» Reviewed & tested by the community

Thanks!

alexpott’s picture

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.