Currently, entity access controllers get separate methods for each $operation (view, delete, update, create). We originally did that in order to give developers a way to easily split up the code as some entities currently have a horrible amount of if/else and switches in their access callbacks due to the entire thing being forced into a single function callback. Now that we got entity access controllers we got that separation naturally but for some reason decided to force developers to use these namespaced methods via the interface (viewAccess, deleteAccess, etc.). Not only is that somewhat weird from an interface perspective and also considering the way it is currently invoked (we mostly map plain ->access() methods to ->{$operation}Access()).

Now, after converting a few of the core entity access callbacks to those new controllers it becomes clear that this step was not very well thought through and that the interface should probably just come with a plain ->access() method itself. We got the code separation already anyways due to us using controllers for that now. If a controller is complex enough it can very well still use separate methods, but we should not enforce it.

This issue should:

  • Simplify EntityAccessControllerInterface to only require a plain ->access() method with the $operation as an argument.
  • Convert all existing implementations to that new pattern and simplify wherever possible.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

theduke’s picture

Status: Needs work » Needs review
FileSize
23.33 KB

After looking at the AccessControllerInterface implementations,
I fully agree that one access() method is sufficient.

Attaching a patch.
I might have missed some dynamic invocations of *Acces(), but should have found most.
We'll see if some tests fail.

Also, I am not sure if it's possible that some other operation than view, update, create or delete are checked.
I decided to return false for unknown operations. Let me know if this is unnecessary.

I checked all affected files with drupalcs, and it reports no errors (well, none affecting my edits).

Status: Active » Needs work

The last submitted patch, drupal-accesscontrollerdx-1947892-1.patch, failed testing.

theduke’s picture

Whoops, missed one access() call that was causing infinite recursion...

Status: Needs review » Needs work

The last submitted patch, drupal-accesscontrollerdx-1947892-1.patch, failed testing.

theduke’s picture

Status: Needs work » Needs review
FileSize
24.03 KB

Why is $GLOBALS['user'] not a instance of the User class but stdClass??

Anyway, this should fix the failures.
Also made UserAccessController::viewAccess protected.

Berdir’s picture

global user is more like a session object not a user entity, we need to fix that (not by making it an entity but something else).

Thanks for working on this!

Berdir’s picture

+++ b/core/lib/Drupal/Core/Entity/Entity.phpundefined
@@ -312,7 +311,7 @@ public function getTranslationLanguages($include_default = TRUE) {
-          foreach (array_filter($this->$field_name) as $langcode => $value)  {
+          foreach (array_filter($this->$field_name) as $langcode => $value) {

Careful with fixing unrelated things like this.

The problem is that there might be another patch that fixes this as well (either only does or another patch that fixes it too) and then those patches will unnecessarily conflict.

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
@@ -88,7 +52,7 @@ public function deleteAccess(EntityInterface $entity, $langcode = LANGUAGE_DEFAU
-  protected function access(EntityInterface $entity, $operation, $langcode = LANGUAGE_DEFAULT, User $account = NULL) {
+  protected function computeAccess(EntityInterface $entity, $operation, $langcode = LANGUAGE_DEFAULT, User $account = NULL) {

checkAccess() maybe?

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockAccessController.phpundefined
@@ -17,31 +17,20 @@
+  public function access(EntityInterface $entity, $operation, $langcode = LANGUAGE_DEFAULT, User $account = NULL) {
+    if ($operation === 'view') {
+      $flag = TRUE;
+    }
+    elseif (in_array($operation, array('create', 'update', 'delete'))) {
+      $flag = user_access('administer blocks', $account);
+    }
+    else {
+      $flag = FALSE;
+    }
+
+    return $flag;

I prefer early returns over such variables. Then it would just be if view return TRUE, elseif ... return user access else return FALSE.

So either that or at least rename the variable to $access, I think that would make more sense.

While we are at it, we should probably rename this to check/computeAccess because then we get the static cache functionality from the base class for free.

+++ b/core/modules/node/lib/Drupal/node/NodeAccessController.phpundefined
@@ -18,27 +18,9 @@
-  public function viewAccess(EntityInterface $node, $langcode = LANGUAGE_DEFAULT, User $account = NULL) {
...
-    // If no modules implement hook_node_grants(), the default behavior is to
-    // allow all users to view published nodes, so reflect that here.
-    $status = $node instanceof EntityNG ? $node->getTranslation($langcode, FALSE)->status->value : $node->status;
-    return $this->setCache($status, $node, 'view', $langcode, $account);

Not sure if we can keep this, the reason for this is the fallback at the end, which allows access if it's public by default. So we need to either keep method as access() or move the logic at the bottom to somewhere else.

+++ b/core/modules/node/lib/Drupal/node/NodeAccessController.phpundefined
@@ -47,7 +29,7 @@ protected function access(EntityInterface $node, $operation, $langcode = LANGUAG
-    if (($access = parent::access($node, $operation, $langcode, $account)) !== NULL) {
+    if (($access = parent::computeAccess($node, $operation, $langcode, $account)) !== NULL) {

We have now at least three different ways to implement the check access method.

- Ignore what parent does (custom block)
- Fallback to parent if not defined specifically (block)
- Use parent implementation (the access hook) if something is returned. (node)
I think we should unify all implementations to they way node works, the hook isn't very useful and works differently otherwise.

We could maybe even move the hook invocation into the default access method() and only call checkAccess() if nobody overrides it with the hook?

Status: Needs review » Needs work

The last submitted patch, drupal-accesscontrollerdx-1947892-1.patch, failed testing.

theduke’s picture

Status: Needs work » Needs review

I also noticed another problem:

--- a/core/modules/block/lib/Drupal/block/BlockAccessController.php
+++ b/core/modules/block/lib/Drupal/block/BlockAccessController.php
@@ -17,10 +17,15 @@
 class BlockAccessController extends EntityAccessController {
 
   /**
-   * Overrides \Drupal\Core\Entity\EntityAccessController::viewAccess().
+   * Overrides \Drupal\Core\Entity\EntityAccessController::computeAccess().
    */
-  public function viewAccess(EntityInterface $entity, $langcode = LANGUAGE_DEFAULT, User $account = NULL) {
-    return $entity->getPlugin()->access();
+  public function computeAccess(EntityInterface $entity, $operation, $langcode = LANGUAGE_DEFAULT, User $account = NULL) {
+    if ($operation === 'view') {
+      return $entity->getPlugin()->access();
+    }

For view operations, the passed in $account is not taken into account, but always the global user.
BlockInterface::access() does not allow an account to be passed in.

I will open a ticket for extending the interface to allow just that.
See #1951386: Extend BlockPluginInterface::access to allow passing in an account

theduke’s picture

theduke’s picture

Issue tags: -Novice
FileSize
26.72 KB

I addressed most of your feedback.

All extending controllers now use the caching of EntityAccessController.

The base access() calls the entity_access hook, and then $this->checkAccess if no result is returned.
So you just need to override checkAccess with the specific checking.

NodeAccessController does implement access() too though, to ensure that "bypass node access" permission always takes precendence.
I also (hopefully) fixed the logic and order in NodeAccessController.

And I don't really think this is a trivial issue, so removing the novice tag.
-----------

This probable is not the right place to discuss this, but:
#1862750: Implement entity access API for nodes has some discussion on hook_entity_access() and hook_entitytype_access().
Right now, hook_entitytype_access() is invoked.
Should we add hook_entity_access() too?

Berdir’s picture

Thanks, I really like where this is going.

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
@@ -22,55 +22,49 @@ class EntityAccessController implements EntityAccessControllerInterface {
+   * This method provides result caching to prevent redundant computation.
+   * To utilize this, extending classes should not override access(),
+   * but instead implement the checkAccess() method as below.
...
+   * This method is supposed to be overwritten by extending classes that
+   * do their own custom access checking.

@@ -88,22 +82,8 @@ public function deleteAccess(EntityInterface $entity, $langcode = LANGUAGE_DEFAU
+  protected function checkAccess(EntityInterface $entity, $operation, $langcode = LANGUAGE_DEFAULT, User $account = NULL) {
+    return NULL;

I thought about making checkAccess() abstract but that's a bit tricky because then you can't use it directly anymore. But that doesn't really make sense because if nobody implements the hook then it doesn't return anything.

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
@@ -22,55 +22,49 @@ class EntityAccessController implements EntityAccessControllerInterface {
+    $access = module_invoke_all($entity->entityType() . '_access', $entity, $operation, $account, $langcode);

About the hook, I'm not sure, is there a use case to grant access to all entity types and not a specfic one?

The problem with that is often that we need to decide which one is called first but this should not be a problem here, we could just call both, merge $access together and then check. But I think we should discuss that in a separate issue.

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
@@ -22,55 +22,49 @@ class EntityAccessController implements EntityAccessControllerInterface {
+    if (in_array(FALSE, $access, TRUE)) {
+      return FALSE;
...
+    elseif (in_array(TRUE, $access, TRUE)) {
+      return TRUE;

You're skipping the cache set here.

We also need to introduce constants instead of TRUE/FALSE as we have for Node right now and then use those there but that should be a separate issue I think, to keep this one reviewable.

+++ b/core/lib/Drupal/Core/Entity/EntityAccessControllerInterface.phpundefined
@@ -16,10 +16,13 @@
-   * Checks 'view' access for a given entity or entity translation.
+   * Check access to an operation on a given entity or entity translation.

CheckS was correct here.

+++ b/core/lib/Drupal/Core/Entity/EntityAccessControllerInterface.phpundefined
@@ -16,10 +16,13 @@
+   *   The operation acces should be checked for.
+   *   Usually one of "view", "create", "update" or "delete".

typo in "acces". Also, I'd suggest to start with the second sentence on the same line and use as much as possible of the 80 characters. It is easier to read IMHO.

+++ b/core/modules/block/lib/Drupal/block/BlockAccessController.phpundefined
@@ -17,10 +17,15 @@
+    else {
+      return parent::checkAccess($entity, $operation, $langcode, $account);

The parent does nothing, so this is kinda pointless, I'd just remove the else and only return something if you know what to do.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermAccessController.phpundefined
@@ -19,31 +19,32 @@
+  public function checkAccess(EntityInterface $entity, $operation, $langcode = LANGUAGE_DEFAULT, User $account = NULL) {
+    switch ($operation) {
+      case 'view':
+        $flag = user_access('access content', $account);

This one should also be updated to either return immediatly (I *think* we usually do that but don't have strong preference) or use $access as the variable.

+++ b/core/modules/user/lib/Drupal/user/UserAccessController.phpundefined
@@ -17,14 +17,51 @@
+      $account = user_load($GLOBALS['user']->uid);

I'm working on a patch to clean up the User type hint mess :)

Status: Needs review » Needs work

The last submitted patch, drupal-accesscontrollerdx-1947892-11.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
7.95 KB
50.71 KB

Fixed the test fails and the stuff pointed out in #12.

Status: Needs review » Needs work

The last submitted patch, drupal-accesscontrollerdx-1947892-14.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
28.79 KB

Messed up the re-roll. Interdiff is good.

Status: Needs review » Needs work
Issue tags: -Entity Access, -Entity Field API

The last submitted patch, drupal-accesscontrollerdx-1947892-16.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +Entity Access, +Entity Field API
andypost’s picture

Methods vs switch() - looks like switch() requires a less code to write

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
@@ -22,55 +22,51 @@ class EntityAccessController implements EntityAccessControllerInterface {
+      $account = user_load($GLOBALS['user']->uid);

+++ b/core/modules/node/lib/Drupal/node/NodeAccessController.phpundefined
@@ -60,12 +43,13 @@ protected function access(EntityInterface $node, $operation, $langcode = LANGUAG
+      $account = user_load($GLOBALS['user']->uid);

+++ b/core/modules/user/lib/Drupal/user/UserAccessController.phpundefined
@@ -17,14 +17,51 @@
+      $account = user_load($GLOBALS['user']->uid);

entity_load('user', ...)

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityAccessTest.phpundefined
@@ -89,6 +89,10 @@ function testEntityAccess() {
   function testEntityAccessDefaultController() {
+    // The implementation requires that the global user id can be loaded.
+    global $user;
+    $user = $this->createUser(array('uid' => 2));

Please clarify why uid=2

Berdir’s picture

Doesn't matter if it's user_load() or entity_load(), both loads an entity that is defined in entity.module. Using entity_load() just paints it in a different color :) #1825332: Introduce an AccountInterface to represent the current user is the place to solve this.

It's uid 2 because uid 1 has no permission restructions. This is copied from another test method, it's just required now because the structure is now different and we do a load for all calls by default.

fubhy’s picture

Normally I'd say "let's please use entity_load() in favor of entity-specific wrappers" but this is really just temporary and won't be necessary once we can rely on the $account argument.

Other than that this looks good already.

Here are a few minor things that I found. Mostly nitpicks.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockAccessController.phpundefined
@@ -17,31 +17,18 @@
+  public function checkAccess(EntityInterface $entity, $operation, $langcode = LANGUAGE_DEFAULT, User $account = NULL) {
+    if ($operation === 'view') {
+      return TRUE;
+    }
+    elseif (in_array($operation, array('create', 'update', 'delete'))) {
+      return user_access('administer blocks', $account);
+    }
+    else {
+      return NULL;
+    }

+++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestAccessController.phpundefined
@@ -17,34 +17,21 @@
+  public function checkAccess(EntityInterface $entity, $operation, $langcode = LANGUAGE_DEFAULT, User $account = NULL) {
+    if ($operation === 'view') {
+      if ($langcode != LANGUAGE_DEFAULT) {
+        return user_access('view test entity translations', $account);
+      }
+      return user_access('view test entity', $account);
+    }
+    elseif (in_array($operation, array('create', 'update', 'delete'))) {
+      return user_access('administer entity_test content', $account);
+    }
+    else {
+      return NULL;
     }

That last 'else' statement is redundant in both places.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermAccessController.phpundefined
@@ -19,31 +19,26 @@
+    switch ($operation) {
+      case 'view':
+        return user_access('access content', $account);
+        break;
+
+      case 'create':
+        return user_access('administer taxonomy', $account);
+        break;
+
+      case 'update':
+        return user_access("update terms in {$entity->bundle()}", $account) || user_access('administer taxonomy', $account);
+        break;
+
+      case 'delete':
+        return user_access("delete terms in {$entity->bundle()}", $account) || user_access('administer taxonomy', $account);
+        break;
+    }

+++ b/core/modules/user/lib/Drupal/user/UserAccessController.phpundefined
@@ -17,14 +17,51 @@
+
+    switch ($operation) {
+      case 'view':
+        $access = $this->viewAccess($entity, $langcode, $account);
+        break;
+
+      case 'create':
+        $access = user_access('administer users', $account);
+        break;
+
+      case 'update':
+        // Users can always edit their own account. Users with the 'administer
+        // users' permission can edit any account except the anonymous account.
+        $access = (($account->uid == $entity->uid) || user_access('administer users', $account)) && $entity->uid > 0;
+        break;
+
+      case 'delete':
+        // Users with 'cancel account' permission can cancel their own account,
+        // users with 'administer users' permission can cancel any account
+        // except the anonymous account.
+        $access = ((($account->uid == $entity->uid) && user_access('cancel account', $account)) || user_access('administer users', $account)) && $entity->uid > 0;
+        break;
+
+      default:
+        $access = NULL;
+        break;
     }
 

These 'breaks' are redundant given the return statemens above them.

+++ b/core/modules/user/lib/Drupal/user/UserAccessController.phpundefined
@@ -17,14 +17,51 @@
+      case 'view':
+        $access = $this->viewAccess($entity, $langcode, $account);
+        break;
+
+      case 'create':
+        $access = user_access('administer users', $account);
+        break;
+
+      case 'update':
+        // Users can always edit their own account. Users with the 'administer
+        // users' permission can edit any account except the anonymous account.
+        $access = (($account->uid == $entity->uid) || user_access('administer users', $account)) && $entity->uid > 0;
+        break;
+
+      case 'delete':
+        // Users with 'cancel account' permission can cancel their own account,
+        // users with 'administer users' permission can cancel any account
+        // except the anonymous account.
+        $access = ((($account->uid == $entity->uid) && user_access('cancel account', $account)) || user_access('administer users', $account)) && $entity->uid > 0;
+        break;
+
+      default:
+        $access = NULL;
+        break;
     }
 
+    return $access;
+  }

This could also be simplified by moving the return statements right into the switch statement instead of first populating $account and then returning it at the end of the method.

Berdir’s picture

Re-roll, simplified those conditions a bit.

Also trying to remove the default arguments of those protected helper functions, allows to remove some user_loads() but might be a problem if one of those user_load()'s returns NULL, but that shouldn't be happening...

andypost’s picture

In case of different operations the result would NULL so better return predictable TRUE/FALSE if no condition for operation provided

+++ b/core/modules/block/lib/Drupal/block/BlockAccessController.phpundefined
@@ -17,10 +17,12 @@
+  public function checkAccess(EntityInterface $entity, $operation, $langcode, User $account) {
+    if ($operation === 'view') {
+      return $entity->getPlugin()->access();

+++ b/core/modules/node/lib/Drupal/node/NodeAccessController.phpundefined
@@ -75,31 +54,35 @@ protected function access(EntityInterface $node, $operation, $langcode = LANGUAG
+    if ($operation === 'view') {
+      return $status;

+++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestAccessController.phpundefined
@@ -17,34 +17,18 @@
+    if ($operation === 'view') {
+      if ($langcode != LANGUAGE_DEFAULT) {
+        return user_access('view test entity translations', $account);
+      }
+      return user_access('view test entity', $account);
+    }
+    elseif (in_array($operation, array('create', 'update', 'delete'))) {
+      return user_access('administer entity_test content', $account);

suppose this require return FALSE after condition

Berdir’s picture

No. FALSE/NULL are different. NULL means I have nothing to say if access should be granted or not and I'll let others decide. FALSE means access is denied, no matter what others say.

While that isn't important in some cases as this is the last check, node for example is different as the node access controller falls back to node grants if this method returns FALSE.

Berdir’s picture

Berdir’s picture

Re-rolled, used @inheritdoc, made that method protected everywhere. Converted the new comment access controller, removed the interface there as it's no longer of any use.

andypost’s picture

Patch looks nice improvement just minor:

+++ b/core/modules/comment/lib/Drupal/comment/CommentAccessController.phpundefined
@@ -16,45 +16,33 @@
+        return user_access('access comments', $account);
+        break;
...
+        return user_access('post comments', $account);
+        break;
...
+        return ($account->uid && $account->uid == $entity->uid->value && $entity->status->value == COMMENT_PUBLISHED && user_access('edit own comments', $account)) || user_access('administer comments', $account);
+        break;
...
+        return user_access('administer comments', $account);
+        break;
...
+        return user_access('administer comments', $account);
+        break;

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermAccessController.phpundefined
@@ -19,31 +19,26 @@
+        return user_access('access content', $account);
+        break;
...
+        return user_access('administer taxonomy', $account);
+        break;
...
+        return user_access("edit terms in {$entity->bundle()}", $account) || user_access('administer taxonomy', $account);
+        break;
...
+        return user_access("delete terms in {$entity->bundle()}", $account) || user_access('administer taxonomy', $account);
+        break;

+++ b/core/modules/user/lib/Drupal/user/UserAccessController.phpundefined
@@ -17,18 +17,43 @@
+        return $this->viewAccess($entity, $langcode, $account);
+        break;
...
+        return user_access('administer users', $account);
+        break;
...
+        return (($account->uid == $entity->uid) || user_access('administer users', $account)) && $entity->uid > 0;
+        break;
...
+        return ((($account->uid == $entity->uid) && user_access('cancel account', $account)) || user_access('administer users', $account)) && $entity->uid > 0;
+        break;

drop this "break"

Berdir’s picture

Yes, I wasn't sure about that, do we have any coding standards for that? @fubhy said the same, @dww in IRC said that he personally likes them, so I'm not sure :)

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

The coding standard implies but does not specify that there should always be a break;

I always have break; in case I refactor out the return statements later. I'd say leave them in.

I like this patch a lot, EntityAccessControllerInterface was very confusing when I first read it.

theduke’s picture

@Berdir: thanks for continuing this, sadly I was too busy to finish things up.

fubhy’s picture

Yah, it's not that bad I guess. I can live with the 'break's. So RTBC+1 from me too.

andypost’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal-accesscontrollerdx-1947892-26.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Entity Access, +Entity Field API

The last submitted patch, drupal-accesscontrollerdx-1947892-26.patch, failed testing.

andypost’s picture

patch needs re-roll

Berdir’s picture

Status: Needs work » Needs review
FileSize
968 bytes
37.45 KB

The patch still applied but we got a new access controller that needed to be updated.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

Berdir’s picture

alexpott’s picture

Title: Improve DX with EntityAccessControllerInterface » Change notice: Improve DX with EntityAccessControllerInterface
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active

Committed 47308d2 and pushed to 8.x. Thanks!

I guess we need to update https://drupal.org/node/1862420

Berdir’s picture

Status: Active » Needs review
alexpott’s picture

Title: Change notice: Improve DX with EntityAccessControllerInterface » Improve DX with EntityAccessControllerInterface
Priority: Critical » Normal
Status: Needs review » Fixed

Looks good

andypost’s picture

Added example of usage for $operation - I think this Change Notice requires it

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