Bypass slow access checks

Created on 7 July 2024, 2 months ago
Updated 14 August 2024, 24 days ago

Problem/Motivation

Checking access to per-bundle Permissions forms, like /admin/structure/comment/manage/comment/permissions, is known to be slow. See πŸ› Don't hide permissions local tasks on bundles when no permissions are defined Needs work for a discussion.

The admin_toolbar_tools module adds these forms to the admin menu. This causes that slow access check to be executed on every page load, when an administrative user is logged in.

The only purpose of the slow access check is to make sure that there are some permissions to manage. Replacing the slow access check with a standard permissions check is not insecure.

For entity types defined by Drupal core, this only matters for two entity-bundle types: comment_type and contact_form.

Proposed resolution

Implement hook_tools_entity_type_alter() in admin_toolbar_tools, replacing the slow access check with a faster one.

Remaining tasks

User interface changes

Per-bundle permissions forms, like /admin/structure/comment/manage/comment/permissions, may be accessible even if they have no permissions to manage. Previously, these pages gave 403 response codes and their links were not shown in menus.

API changes

None

Data model changes

None

πŸ“Œ Task
Status

Postponed: needs info

Version

3.0

Component

Code

Created by

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

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

Merge Requests

Comments & Activities

  • Issue created by @benjifisher
  • Issue was unassigned.
  • Status changed to Needs review 2 months ago
  • πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

    I opened MR 82, implementing the proposed resolution. The code is based on @larowlan's comment #3306434-14: [Config] Warning: Entity view display 'node.article.default': Component 'comment' was disabled because its settings depend on removed dependencies. β†’ .

    It might be a good idea to improve this by making the affected types configurable. Then site administrators who know that there are no permissions to manage could completely remove those forms (unset($route_providers['permissions']); in the hook implementation); those who know there are permissions to manage could access them without the slow check.

  • πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

    FWIW, seems wrong to me to add this hack to this module. Lets focus on improving this in core.

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

    Not sure if this slowness got worse with Drupal 10.3.1 but that is what we have noticed. We had to disable admin_toolbar_tools because it was causing 15-20 second page load times for admin users.

  • πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

    You should look at the referenced core issue and test and report if that's working for you.

  • Status changed to Postponed: needs info about 1 month ago
  • πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

    Marking needs more info because its quite likely this is fixed with the core issue being resolved for D10 and 11.

  • πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

    @todea:

    Not sure if this slowness got worse with Drupal 10.3.1 ...

    I hope you mean from 10.2.? to 10.3.1, not from 10.3.0 to 10.3.1.

    This problem got worse in 10.3 as a side effect of the bug fix πŸ› [regression] Do not bypass route access with 'link to any page' permissions for menu links Fixed . Before that, many of the links in the admin menu were skipping the access check. See this comment: #3306434-66: Fix access checks for bundle permissions to avoid triggering a config validation error β†’ . Also #39 on that issue.

    I would love to know how to reproduce your 15-20 second load times, starting with a fresh install of Drupal.

    Have you tried the change in this issue and/or the change from πŸ› Fix access checks for bundle permissions to avoid triggering a config validation error Fixed ? That issue is fixed in the 10.3.x branch, so it will be part of the next release for 10.3.

    @japerry:

    #3306434 was fixed, but πŸ› Don't hide permissions local tasks on bundles when no permissions are defined Needs work is still open. I will be very happy if the first issue fixes this problem, but @Berdir is pretty sure that we need the second.

    I am not sure whether your comment was referring to #3306434 or an expectation that #3384600 will soon be fixed.

  • πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

    heh, I'm not sure either. ;) I haven't ran into this issue personally yet, so lets let this bake for a while before having to make any specific hacks in this module.

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

    @benjifisher:
    Yes, we went from I believe 10.2.7 straight to 10.3.1

    Not sure if I can replicate the 15-20 second load times w/ something like simplytest.me but it was pretty slow. Back in February we upgraded from Drupal 7 to 10 and it has been a bit of a bumpy road so far. We have ~200 enabled modules, ~299k nodes. Some of the permission related modules we use are: override_node_options, view_unpublished, workbench, workbench_access.

    At the time, disabling admin_toolbar_tools quickly solved the problem and the difference was night & day.

    On our dev server I re-enabled admin_toolbar_tools and I applied patch #5 from 3384600 β†’ and your MR 82 listed above (we needed both). That has fixed the issue on our end, our pages that utilize admin toolbar are back to normal now - much faster.

    Thank you to both you and @berdir for working on these various issues.

  • πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

    @todea:

    On our dev server I re-enabled admin_toolbar_tools and I applied patch #5 from 3384600 and your MR 82 listed above (we needed both).

    That is really surprising. Those two changes should have exactly the same effect. Is it possible that after applying one change, you did not clear caches before testing?

    Drupal 10.3.2 was released last week, and it includes the fix for πŸ› Fix access checks for bundle permissions to avoid triggering a config validation error Fixed . That issue does not prevent the slow access check, but it does prevent the log message each time the check is done, which should help. I would really like to know whether it helps a little or a lot.

    Ideally, I would like to know how long it takes, after clearing caches, to load an admin page with the admin_toolbar_tools module enabled, using the same database and each of these codebases:

    1. Drupal 10.3.1, no patches.
    2. Drupal 10.3.2, no patches.
    3. Drupal 10.3.2, patch from #3384600.
    4. Drupal 10.3.2, patch admin_toolbar_tools from this issue.
    5. Drupal 10.3.2, patches from both (3) and (4).

    The difference between (1) and (2) will tell us how much #3306434 helps, if at all.

    I expect that (3), (4), and (5) will have the same result. If it turns out that you did not clear caches in your earlier test, then I do not really care about (4) and (5).

    If I were doing the test, I would use my local development environment (DDEV). That makes it easy to switch the codebase without having to push the changes to the dev server.

    We are dealing with noticeable differences here, so we do not need to time things to the millisecond. If you just clear caches, start a timer, reload the page, and stop the timer when the page finishes loading, that should tell us what we need to know.

Production build 0.71.5 2024