Contrib.social

Incompatible with Entity Type's ::checkAccess() implementations

Open on Drupal.org →
Created on 7 August 2023, over 1 year ago
Updated 26 February 2024, about 1 year ago

Problem/Motivation

This module implements HOOK_entity_access(), which takes precedence over the ContentEntityType's access handler implementation:

 * @ContentEntityType(
 *   [...]
 *     "access" = "Drupal\precima_serviceportal\PrecimaCustomerAccessControlHandler",
[]...

This is implemented in:
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...

// Invoke hook_entity_access() and hook_ENTITY_TYPE_access(). Hook results
  // take precedence over overridden implementations of
  // EntityAccessControlHandler::checkAccess(). Entities that have checks that
  // need to be done before the hook is invoked should do so by overriding
  // this method.
  // We grant access to the entity if both of these conditions are met:
  // - No modules say to deny access.
  // - At least one module says to grant access.

Hopefully we can improve the situation based on:

// Also execute the default access check except when the access result is
  // already forbidden, as in that case, it can not be anything else.

Also https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21... points out:

Be careful when writing generalized access checks shared between routing and entity checks: routing uses the andIf() operator. So returning an isNeutral() does not determine entity access at all but it always ends up denying access while routing.

This is a major issue, as if this module returns access denied, the checkAccess() implementation of the Entity access handler isn't called at all and even privileged users like site builders, with their access defined in these classes have no access on view / edit / delete / ... and it's REALLY hard to find this reason!

If we can't fix this by design, we should perhaps at least document this in the UI, where the "deny" behavior can be selected. Sadly the Drupal core fallback seems to be to allow access, if not explicitly denied?
Still, it might make sense to prefer "neutral"?

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

🐛 Bug report
Status

Active

Version

1.0

Component

Code

Created by

🇩🇪Germany Anybody Porta Westfalica

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Sign in to follow issues

Comments & Activities

  • Issue created by @Anybody
  • Comment over 1 year ago →
    🇩🇪Germany Anybody Porta Westfalica
  • Comment over 1 year ago →
    🇩🇪Germany Anybody Porta Westfalica
  • Comment over 1 year ago →
    🇩🇪Germany Anybody Porta Westfalica

    I think we need a test module with a custom content entity to show the issue. As a mid-level users, the "view / edit / delete" permissions of that custon entity (as generated by drush gen) simply have no effect and you get access denied, until you comment out the hook_entity_access() logic of this module.

  • Comment over 1 year ago →
    🇩🇪Germany Anybody Porta Westfalica

    It's not 100% sure the module has this issue, as I found it at entity_access_by_role_field ( 🐛 Incompatible with Entity Type's ::checkAccess() implementations Active ) but I think it's quite sure as they share a lot of code, also in this area.

    The problem can be resolved by returning "NEUTRAL" (selected in the settings) instead of "FORBIDDEN".

    So I think to mitigate this issue and decide, if it's a bug or intended to work like this, we should add information about the risks of using "FORBIDDEN" to the UI setting?

  • Comment over 1 year ago →
    🇩🇪Germany Anybody Porta Westfalica

    Okay, but in some cases (media entities for example) that indeed leads to access where there should be no access granted -.- So be careful. No general solution.

  • Assigned to Grevil
  • Comment about 1 year ago →
    🇩🇪Germany Anybody Porta Westfalica

    In the first step we need tests to clearly reproduce the situation in code. Assigning @Grevil for this and to take a general look. Read the other issue and the information above carefully first.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024