Problem/Motivation

1. At some point we check for "administer nodes" permission, which is not OK for a generic access check implementation. We should rely on $entity_type->getAdminPermission() instead.
2. if "list" operation is being checked and we end up passing it on to the entity access unpredictable results can happen (even fatal in my - CRM core - case). "list" doesn't seem to be standard operation and entity access handlers don't handle it in general. We should either pass something else instead of list of communicate clear expectation that we expect access handlers to be able to handle that.

Proposed resolution

1. Rely on admin permission.
2. When "list" operation is checked use "view" when checking access with the entity access handler.

We should update tests to cover this cases.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

slashrsm created an issue. See original summary.

slashrsm’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
2.39 KB

Had 5 mins to look into this. We still need tests. Any feedback on the approach would be awesome.

dawehner’s picture

Thank you @slashrsm
This patch totally makes sense. Here is a test to prove that things actually work.

dawehner’s picture

Issue tags: -Needs tests

  • dawehner committed 473ead6 on 8.x-1.x
    Issue #2729889 by dawehner, slashrsm: \Drupal\entity\Access\...
  • dawehner committed c88386d on 8.x-1.x authored by slashrsm
    Issue #2729889 by dawehner, slashrsm: \Drupal\entity\Access\...
dawehner’s picture

Status: Needs review » Fixed

Well the branch test are broken due to still using php 5.5

Sorry again for not seeing the issue earlier. Thank you @slashrsm!

Status: Fixed » Closed (fixed)

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