Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#42 | interdiff.txt | 1.58 KB | andypost |
#42 | access-controllers-2012916-42.patch | 12.67 KB | andypost |
#38 | interdiff.txt | 2.08 KB | andypost |
#38 | access-controllers-2012916-38.patch | 12.42 KB | andypost |
#36 | interdiff.txt | 470 bytes | andypost |
Comments
Comment #1
andypostRe-roll of #1842850-8: Untangle menu_link access control from _menu_link_translate() and friends
Comment #3
andypost#1: access-controllers-2012916-1.patch queued for re-testing.
Comment #5
andypostTest passes locally
Comment #6
amateescu CreditAttribution: amateescu commentedLooks good to me as a first step. We can deal with gutting _menu_link_translate() in the other issue.
Comment #7
catchLooks good to me, following the other issue. Committed/pushed to 8.x.
Comment #8
andypostThis one was not pushed
Comment #9
catchgit errored out on me with access denied... Now pushed.
Comment #10
andypostnow ok
Comment #11
BerdirNot so ok :)
Wasn't retested and conflicted with AccountInterface. This should fix it, but it's untested.
Comment #12
catchWhoops and thanks, committed/pushed.
Comment #13
catchActually reverted both the follow-up and the original patch, let's start back from this morning...
Comment #14
tim.plunkettComment #15
BerdirCombined the two patches.
Comment #16
andypostThere was collision with #1825332: Introduce an AccountInterface to represent the current user
EDIT sorry, x-post
Comment #17
andypostpatches are identical
Comment #18
andypost#15: menu-access-controllers-2012916-15.patch queued for re-testing.
Comment #19
andypostPatches are identical so Retest berdir's one
Comment #20
alexpottNeeds a reroll...
Comment #21
pwieck CreditAttribution: pwieck commentedWorking on reroll of #15
Comment #22
pwieck CreditAttribution: pwieck commentedrerolled.
Comment #23
BerdirHm, 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?
Comment #24
amateescu CreditAttribution: amateescu commentedYes, I think that's a good plan.
Comment #25
BerdirLet's do that then :)
Comment #26
andypostNew 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
so it's a conversion
Comment #27
andypostaccess check for delete now lives in
EntityListController
Comment #28
andypostDigging deeper I found that
_menu_overview_tree_form()
uses different permissions. So introduced them and cleaned-up useless code.Comment #30
andypost#28: access-controllers-2012916-28.patch queued for re-testing.
Comment #31
tim.plunkettThis looks like it overlaps with the RTBC #1984702: Convert menu.module's page callbacks to Controllers
Comment #33
andypostBroken test should pass now
@tim.plunkett I think it's ok, I'll re-roll if this one goes first
Comment #34
andypostMerge after #1984702: Convert menu.module's page callbacks to Controllers
And a bit more security fixes
Comment #36
andypostwe need stop to mix edit and update
Comment #38
andypostshould be fine mostly
Comment #39
dawehnerPotentially we should also typehint the MenuLinkInterface
Just wondering why we do not check for 'administer menu' as well.
This should be $account->hasPermission.
Comment #40
amateescu CreditAttribution: amateescu commentedLooks great to me.
Comment #41
amateescu CreditAttribution: amateescu commentedExcept that I didn't catch that user_access() call.
Comment #42
andypostMenuAccessController should live in system module where the entity defined, so moved current implementation with clean-up
Comment #43
amateescu CreditAttribution: amateescu commentedThanks!
Comment #44
alexpottCommitted 972406d and pushed to 8.x. Thanks!