Admin page access denied even when access is given to child items

Created on 9 January 2024, 5 months ago
Updated 22 May 2024, 25 days ago

Problem/Motivation

πŸ› Restrict access to empty top level administration pages Fixed introduced a bug where if a user does not have access to the child items of the first item of an admin page, the page gives an access denied, even if the user does have access to some other items. For me the first item was entity.taxonomy_vocabulary.collection, so denying access to that item resulted in the user not being able to access /admin/structure even though they did have access to add/edit menus.

Steps to reproduce

composer create-project drupal/recommended-project drupal.local
cd drupal.local
composer require drush/drush
vendor/bin/drush site:install
vendor/bin/drush user:create Jelle
vendor/bin/drush user:role:add content_editor Jelle
vendor/bin/drush role:perm:add content_editor "access taxonomy overview"

# Login as user 2 and check the toolbar, "Structure" should be there

composer require drupal/scheduler
vendor/bin/drush en scheduler

# Login as user 2 and check the toolbar, "Structure" is gone

Proposed resolution

Only do an early return in \Drupal\system\Access\SystemAdminMenuBlockAccessCheck::hasAccessToChildMenuItems if a child menu item is found where the user does have access to. Don't just check the first one.

Remaining tasks


Fix tests so they fail without the fix, these were broken in the reroll see #39

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

If users have access to a child of an admin page, but not its first child, they will now have access to that admin page.

πŸ› Bug report
Status

Fixed

Version

10.3 ✨

Component
SystemΒ  β†’

Last updated 2 days ago

No maintainer
Created by

πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @Jelle_S
  • πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium
  • Status changed to Needs work 5 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    The linked issue hasn't been committed so instead of a separate issue think this should be closed and that issue moved back to Needs work.

    Will need test coverage either way.

  • πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

    My bad! I linked the wrong issue, the code definitely is already in core. Fixed that part.

  • πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium
  • πŸ‡©πŸ‡ͺGermany luenemann SΓΌdbaden, Germany

    This should be converted to a Merge Request. Change Status to Needs Review if you think it's ready.

    πŸ“Œ Restrict access to empty top level administration pages for overview controller Fixed touches the same function as this patch. Is that issue fixing the same problem?

  • πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

    No, that tackles a different issue. I'll have a look at creating an MR if I find the time today.

  • First commit to issue fork.
  • Status changed to Needs review 5 months ago
  • πŸ‡ΊπŸ‡¦Ukraine taraskorpach Lutsk

    I've created a PR to simplify Jelle_S's life. I'm not sure if the code will work properly, but based on #7, I'm marking the issue as 'Needs Review'.

  • Pipeline finished with Success
    5 months ago
    Total: 614s
    #74861
  • Status changed to Needs work 5 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Thanks but MR should target 11.x

    And will need test coverage

  • πŸ‡ΊπŸ‡¦Ukraine taraskorpach Lutsk

    Should I create a new branch based on the 11.x?

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    You can change the target branch on the current MR, will have to rebase most likely though just FYI.

    11.x is the current development branch. So issues land there first and then backported to other supported branches when committed.

  • πŸ‡ΊπŸ‡¦Ukraine taraskorpach Lutsk
  • Pipeline finished with Success
    5 months ago
    #74887
  • Pipeline finished with Success
    5 months ago
    Total: 671s
    #74888
  • Status changed to Postponed: needs info 5 months ago
  • πŸ‡ΊπŸ‡¦Ukraine taraskorpach Lutsk

    As I mentioned in #10, I'm unsure if the code is correct, so I followed the steps from the summary. Unfortunately, I'm unable to reproduce the issue.

    My user has the 'Administer menus and menu links' permission but lacks the 'Access the taxonomy vocabulary overview page' permission. In this case, I have access to /admin/structure, and the 'Structure' link also appears in the menu.

    I'm marking this as postponed since we need more information to reproduce it.

  • Status changed to Needs work 5 months ago
  • πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

    I've managed to reproduce it on a clean Drupal 10.2.1 install, with the contrib Scheduler module. Unfortunately, in this case, this patch does not seem to work, so there's more going on. Here are the steps to reproduce:

    composer create-project drupal/recommended-project drupal.local
    cd drupal.local
    composer require drush/drush
    vendor/bin/drush site:install
    vendor/bin/drush user:create Jelle
    vendor/bin/drush user:role:add content_editor Jelle
    vendor/bin/drush role:perm:add content_editor "access taxonomy overview"
    
    # Login as user 2 and check the toolbar, "Structure" should be there
    
    composer require drupal/scheduler
    vendor/bin/drush en scheduler
    
    # Login as user 2 and check the toolbar, "Structure" is gone
    
    
  • πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium
  • πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

    It seems to have something to do with this view in scheduler, but I can't figure out what's going wrong:
    https://git.drupalcode.org/project/scheduler/-/blob/2.0.1/config/optiona...

  • πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

    After some further digging:

    The scheduler module adds the view linked above, which creates a menu link item with entity.taxonomy_vocabulary.collection as its parent. So if you follow the steps to reproduce, you create a user that does have access to entity.taxonomy_vocabulary.collection, but does not have access to its child created by the scheduler module: views_view:views.scheduler_scheduled_taxonomy_term.overview

    I'm still trying to wrap my head around the correct implementation details, but basically: once an element in the tree is found that is accessible, and is an actual page, not just an overview of its sublinks, the access check should pass, which it doesn't right now.

  • πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

    @taraskorpach I don't have push access to your MR, but I did find a fix for the problem described above. Not sure how to write a test for this exactly, but in the mean time people experiencing this issue can use the patch attached.

  • πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

    Removed the extra whitespace at the end of the file

  • last update 5 months ago
    Composer error. Unable to continue.
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Environment: PHP 8.1 & MySQL 5.7 updated deps
    last update 5 months ago
    Build Successful
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Environment: PHP 8.1 & MariaDB 10.3.22
    last update 5 months ago
    25,987 pass, 1,824 fail
  • Status changed to Needs review 5 months ago
  • πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

    Still no access to the MR, but here's the above patch with tests included, and one patch with only the tests to prove they fail.

  • Open in Jenkins β†’ Open on Drupal.org β†’
    Environment: PHP 8.1 & MariaDB 10.3.22
    last update 5 months ago
    25,978 pass, 1,861 fail
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Environment: PHP 8.1 & MariaDB 10.3.22
    last update 5 months ago
    Build Successful
  • Pipeline finished with Canceled
    5 months ago
    Total: 498s
    #75609
  • πŸ‡ΊπŸ‡¦Ukraine taraskorpach Lutsk

    I've updated MR according to your patch. Can I grant access to the MR, or does it solely belong to me?

  • Pipeline finished with Success
    5 months ago
    Total: 490s
    #75614
  • πŸ‡§πŸ‡ͺBelgium Jelle_S Antwerp, Belgium

    Sorry, I didn't see the big green "Get push access button"... Thanks @taraskorpach for adding the latest patch to the MR :)

  • Pipeline finished with Success
    5 months ago
    Total: 2182s
    #75633
  • πŸ‡©πŸ‡ͺGermany luenemann SΓΌdbaden, Germany

    Test only job failed with expected message: https://git.drupalcode.org/issue/drupal-3413508/-/jobs/614074#L54

  • Pipeline finished with Success
    5 months ago
    Total: 7685s
    #75658
  • Status changed to RTBC 5 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Hiding patches for clarity.

    Confirmed test-only job (same as #25)

     There was 1 error:
    1) Drupal\Tests\system\Functional\Menu\MenuAccessTest::testSystemAdminMenuBlockAccessCheck
    Behat\Mink\Exception\ExpectationException: Current response status code is 403, but 200 expected.
    /builds/issue/drupal-3413508/vendor/behat/mink/src/WebAssert.php:794
    /builds/issue/drupal-3413508/vendor/behat/mink/src/WebAssert.php:130
    /builds/issue/drupal-3413508/core/modules/system/tests/src/Functional/Menu/MenuAccessTest.php:318
    /builds/issue/drupal-3413508/core/modules/system/tests/src/Functional/Menu/MenuAccessTest.php:336
    /builds/issue/drupal-3413508/core/modules/system/tests/src/Functional/Menu/MenuAccessTest.php:246
    /builds/issue/drupal-3413508/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
    ERRORS!
    Tests: 2, Assertions: 232, Errors: 1.
    

    Issue summary appears to be complete and solution matches change. Believe this is good.

  • πŸ‡¬πŸ‡§United Kingdom malcomio

    I don't think that this is specific to 11.x after the changes for πŸ› Restrict access to empty top level administration pages Fixed - see πŸ› Users with access taxonomy overview permission cannot access admin/structure Needs review , which is on 10.2.x

  • I was also seeing this problem on my site. The latest patch does fix the problem for me (I'm on 10.2.x).

  • πŸ‡§πŸ‡ͺBelgium RandalV

    Also noticed this issue on a 10.2.x site.
    Copied the diff locally, used it as patch with composer -> it works!

  • πŸ‡ΉπŸ‡·Turkey makbay

    I also noticed this on 10.2.x site and the patch fixed it.

  • πŸ‡ΊπŸ‡¦Ukraine kuflievskiy

    Confirm that I also noticed this on the 10.2.2 site and the patch fixed it.

  • πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

    This seriously affects the ability to administer sites without a documented work around so bumping the priority.

    The code is pretty confusing and its not clear to me how it fixes the problem but i don't seen anything broken.

  • πŸ‡«πŸ‡·France flocondetoile Lyon

    Same issue here. Users with no permissions for the first link get an access denied. MR 6100 fix the issue

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Closed as duplicate.

  • πŸ‡ΊπŸ‡ΈUnited States ultimike Florida, USA

    MR6100 works for my use case as well - see https://github.com/drush-ops/drush/issues/5864

    -mike

  • πŸ‡§πŸ‡ͺBelgium flyke

    Our project is on Drupal 10.2.1.
    I was breaking my head about why a content editor cannot see the Structure menu tab, even though he has permissions to edit certain menus and taxonomies and view webform subscriptions.
    I looked through the issie queues of admin_toolbar, admin_toolbar_links_access_filter, gin_toolbar, gin, ... but could not find it.

    The MR6100 applies and the admin -> structure links are again available in the toolbar for the content editor

    BUT

    The user now sees all menus, not only the ones he has access to via permissions.
    Also, its not just listing, he can also edit all the menus he's not supposed to be able to, like the administration menu.
    That should not be possible, security wise. Can anyone confirm ?

  • πŸ‡¦πŸ‡ΊAustralia acbramley

    Marked πŸ› Access denied on system.admin_structure route when Admin Toolbar Extra Tools is enabled Active and πŸ› Missing Top Level Menu Item Closed: duplicate as duplicates of this.

  • Assigned to acbramley
  • Status changed to Needs work 4 months ago
  • πŸ‡¦πŸ‡ΊAustralia acbramley

    This has some huge conflicts with πŸ“Œ Restrict access to empty top level administration pages for overview controller Fixed going to try and resolve them today. Unfortunately both used a 4th child with similar but slightly different access rules.

  • Status changed to Needs review 4 months ago
  • πŸ‡¦πŸ‡ΊAustralia acbramley

    I've done my best to merge in the conflicts, but the tests were quite mind bending to get passing.

    I've also replaced "super" with "grand" in most cases as that seemed to be a large part of the conflicts.

  • Pipeline finished with Success
    4 months ago
    #105480
  • Status changed to Needs work 4 months ago
  • πŸ‡¦πŸ‡ΊAustralia acbramley

    The test only changes passed so something isn't right...

  • πŸ‡¬πŸ‡§United Kingdom aaron.ferris

    MR fixes this issue in my 10.2 install.

  • πŸ‡¦πŸ‡ΊAustralia acbramley

    Yeah it sounds like from the UPDATE in #37 that it was user error.

  • πŸ‡¦πŸ‡ΊAustralia acbramley
  • Issue was unassigned.
  • Status changed to Needs review 3 months ago
  • πŸ‡¦πŸ‡ΊAustralia acbramley

    DOH! Took me far too long to figure out what I'd done wrong... I hadn't put the test menu items back in the merge conflict madness.

    Now that I've done that and fixed up the tests, they are correctly failing without the fix:

    Behat\Mink\Exception\ExpectationException : Routes do not match. 
    Expected routes:
    Accessible routes: menu_test.parent_test, menu_test.child4_test, menu_test.child4_test_overview
    Inaccessible routes: menu_test.child1_test, menu_test.child2_test, menu_test.child3_test_block, menu_test.grand_child1_test, menu_test.grand_child2_test, menu_test.grand_child3_test, menu_test.great_grand_child1_test, menu_test.grand_child4_test
    Actual routes: 
    Accessible routes: menu_test.child4_test, menu_test.child4_test_overview
    Inaccessible routes: menu_test.parent_test, menu_test.child1_test, menu_test.child2_test, menu_test.child3_test_block, menu_test.grand_child1_test, menu_test.grand_child2_test, menu_test.grand_child3_test, menu_test.great_grand_child1_test, menu_test.grand_child4_test
    
  • Pipeline finished with Success
    3 months ago
    #109951
  • πŸ‡¬πŸ‡§United Kingdom Alina Basarabeanu

    With the changes pushed with the last commit, we could not apply the patch to Drupal Core 10.2.3.
    Can you please add a patch file for 10.2.3 version?

  • Status changed to RTBC 3 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Ran test-only feature

    1) Drupal\Tests\system\Functional\Menu\MenuAccessTest::testSystemAdminMenuBlockAccessCheck
    Behat\Mink\Exception\ExpectationException: Routes do not match. 
    Expected routes:
    Accessible routes: menu_test.parent_test, menu_test.child4_test, menu_test.child4_test_overview
    Inaccessible routes: menu_test.child1_test, menu_test.child2_test, menu_test.child3_test_block, menu_test.grand_child1_test, menu_test.grand_child2_test, menu_test.grand_child3_test, menu_test.great_grand_child1_test, menu_test.grand_child4_test
    Actual routes: 
    Accessible routes: menu_test.child4_test, menu_test.child4_test_overview
    Inaccessible routes: menu_test.parent_test, menu_test.child1_test, menu_test.child2_test, menu_test.child3_test_block, menu_test.grand_child1_test, menu_test.grand_child2_test, menu_test.grand_child3_test, menu_test.great_grand_child1_test, menu_test.grand_child4_test
    /builds/issue/drupal-3413508/core/tests/Drupal/Tests/WebAssert.php:580
    /builds/issue/drupal-3413508/core/modules/system/tests/src/Functional/Menu/MenuAccessTest.php:411
    /builds/issue/drupal-3413508/core/modules/system/tests/src/Functional/Menu/MenuAccessTest.php:283
    /builds/issue/drupal-3413508/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
    ERRORS!
    Tests: 2, Assertions: 258, Errors: 1.
    

    Issue summary appears correct.

    Verified the solution does fix the problem described.

  • πŸ‡¦πŸ‡ΊAustralia acbramley

    @Alina Basarabeanu does https://www.drupal.org/files/issues/2024-01-11/3413508-22.patch β†’ still work for you on 10.2?

  • πŸ‡¬πŸ‡§United Kingdom Alina Basarabeanu

    Yes,
    Previously I was using the merge request as was pointing to 10.x branch.
    Thanks

  • πŸ‡ͺπŸ‡ΈSpain Eduardo Morales Alberti Spain, πŸ‡ͺπŸ‡Ί

    Same error here on 10.2.3, the patch https://www.drupal.org/files/issues/2024-01-11/3413508-22.patch β†’ solves our issue.

  • Status changed to Needs work 3 months ago
  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    Added some comments to the MR.

    Also I think we need a change record and a release note. This change might result in users seeing menus items they couldn't before. I think that this means we should tell users about this.

  • Pipeline finished with Failed
    3 months ago
    Total: 1066s
    #121844
  • Status changed to Needs review 3 months ago
  • πŸ‡¦πŸ‡ΊAustralia acbramley

    Brief release note added, feel free to make amendments.

    Random fail on unit tests in the pipeline, have rerun.

  • Pipeline finished with Failed
    3 months ago
    Total: 590s
    #121857
  • Pipeline finished with Success
    3 months ago
    Total: 581s
    #122479
  • Status changed to RTBC 3 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Rebased the MR and that Unit test failure was random.

    Feedback appears to be addressed, release note seems good. Mentions the problem that is solved.

    • catch β†’ committed 54a9077a on 10.3.x
      Issue #3413508 by Jelle_S, taraskorpach, acbramley, smustgrave, alexpott...
    • catch β†’ committed 5274b41a on 11.x
      Issue #3413508 by Jelle_S, taraskorpach, acbramley, smustgrave, alexpott...
  • Status changed to Fixed 3 months ago
  • πŸ‡¬πŸ‡§United Kingdom catch

    Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!

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

  • πŸ‡ΊπŸ‡¦Ukraine khiminrm

    khiminrm β†’ changed the visibility of the branch 3413508-admin-page-access to active.

  • πŸ‡³πŸ‡±Netherlands tarik.cipix

    Patch #22 is still needed on the latest version of Drupal Core 10.2.6. I don't know why this is only cherry picked to 10.3.x? This is a necessary fix for all Drupal sites since it's now only checking the first menu item instead of any menu item.

    Please re-open and add code change to 10.2.x as well, patch can be used in the meantime. (Until a stable version of 10.3.x is released for production)

Production build 0.69.0 2024