See parent issue #3200985: [meta] Fix undesirable access checking on entity query usages for context and test coverage policy. This issue is major because it blocks #2785449: It's too easy to write entity queries with access checks that must not have them.

The menu_ui_get_menu_link_defaults() provides default parent menu items when editing nodes. It uses entity queries that implicitly checks access for the current user, when it should not.

Issue fork drupal-3204140

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jonathanshaw created an issue. See original summary.

jonathanshaw’s picture

I think this should not be access sensitive, because any access control of menu items would likely be focused on visitors not editors.

MenuLinkContentAccessControlHandler has:

  protected function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
    switch ($operation) {
      case 'view':
        // There is no direct viewing of a menu link, but still for purposes of
        // content_translation we need a generic way to check access.
        return AccessResult::allowedIfHasPermission($account, 'administer menu');

I suggest that under the parent issue's test coverage policy it is acceptable for this not to have test coverage, because it would require unusual customisations to encounter and it's more important to progress the blocked issue.

jonathanshaw’s picture

Status: Active » Needs review
longwave’s picture

Status: Needs review » Reviewed & tested by the community

I think it's unlikely anyone would add access control to individual menu items so I think this is fine.

I did spot another possible but unrelated bug here, the conditions in the two queries are subtly different:

        ->condition('link.uri', 'node/' . $node->id())

vs

        ->condition('link.uri', 'entity:node/' . $node->id())

edit: already open as #2665608: Menu UI searches links by incorrect uri

catch’s picture

Status: Reviewed & tested by the community » Needs review

Hmm I think this should check access because it's showing a list of items in the UI - even though the use case for link access via query alter is very obscure.

jonathanshaw’s picture

Fair enough. TRUE preserves the existing behavior and unblocks the parent issue, so it's harmless to do it here and probably no one will ever care. If someone thinks it should be FALSE and actually cares about, let them open an issue for that.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Thanks I think this is ready to go then.

alexpott’s picture

Version: 9.2.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 1291450a0d to 9.2.x and 8ff460a0d6 to 9.1.x. Thanks!

Backported to 9.1.x because there's no change and keeping things aligned.

  • alexpott committed 1291450 on 9.2.x
    Issue #3204140 by jonathanshaw, catch: EntityQuery accessCheck:...

  • alexpott committed 8ff460a on 9.1.x
    Issue #3204140 by jonathanshaw, catch: EntityQuery accessCheck:...

Status: Fixed » Closed (fixed)

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