Problem/Motivation

The hook_entity_access documentation does not really tell us anything that the function header / return type (AccessResultInterface) doesn't.

Most importantly, it does does not document the use of orIf. Since February, the routing system uses andIf solely and not being aware of the difference can lead to a security issue when a shared access check returns a Neutral result instead of a Forbidden because the two make no difference when routing but they do when entity access checking. So this is a security weakness at best -- can a documentation issue be critical? If it can be, this one needs to be.

Proposed resolution

Document it! I already put a note on https://www.drupal.org/node/2122195/revisions/view/8436387/8470706 to denote the difference.

Remaining tasks

Get dawehner to review this and commit it ASAP.

Either here or a followup should discuss what are the possible values of $operation.

CommentFileSizeAuthor
#7 2490074_7.patch1.43 KBchx
#5 2490074_5.patch1.44 KBchx
disgrace.patch1.25 KBchx

Comments

chx’s picture

chx commented
Status: Active » Needs review
chx’s picture

chx commented
Issue summary: View changes
chx’s picture

chx commented
Issue summary: View changes
chx’s picture

chx commented
Issue summary: View changes
chx’s picture

chx commented
StatusFileSize
new 2490074_5.patch1.44 KB
dawehner’s picture

dawehner
Primary language German
commented 
  1. +++ b/core/modules/system/entity.api.php
@@ -522,7 +522,14 @@
+ *    The access result. The final result is calculated by using
+ *    \Drupal\Core\Access\AccessResultInterface::orIf() on the result of every
+ *    hook_entity_access(), hook_ENTITY_TYPE_access() implementation and
+ *    result of the entity specific checkAccess() method in the entity access
+ *    control handler. 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.

    <3 

  2. +++ b/core/modules/system/entity.api.php
@@ -548,7 +555,7 @@ function hook_entity_access(\Drupal\Core\Entity\EntityInterface $entity, $operat
+ *    The access result. hook_entity_access() has detailed documentation.ag ser

    Something odd happened here at the end of the sentence!

chx’s picture

chx commented
StatusFileSize
new 2490074_7.patch1.43 KB

Something odd -- looks like i started to type "ag service_tags" before switching to console.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Ah yeah, I assumed something like that.

xjm’s picture

xjm
she/her
Primary language English
commented
Category: Bug report » Task
Priority: Major » Normal
Status: Reviewed & tested by the community » Fixed

Thanks @chx. This is a great improvement to these docs, and of course it's important for people implementing this hook to understand WTF is going on. ;)

This issue only changes documentation, so per https://www.drupal.org/core/beta-changes, this can be completed any time during the Drupal 8 beta phase. Committed and pushed to 8.0.x.

I tweaked the punctuation slightly on commit:

- *    hook_entity_access(), hook_ENTITY_TYPE_access() implementation and
- *    result of the entity specific checkAccess() method in the entity access
+ *    hook_entity_access() and hook_ENTITY_TYPE_access() implementation, and the
+ *    result of the entity-specific checkAccess() method in the entity access

  • xjm committed 02f6ab2 on 8.0.x 
    Issue #2490074 by chx, dawehner: hook_entity_access documentation is...

Status: Fixed » Closed (fixed)

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