Let's add an API for checking access to entity objects, e.g. like the entity_access() function of the d7 contrib entity API module. Furthermore, the API should provide means for checking property/field access based upon #1346214: [meta] Unified Entity Field API, while incorporating entity access.

Note, that this issue is supposed to implement access checks on already loaded entity objects and/or checking general access, but not about applying access checks on queries. Let's handle that in another issue, e.g. #777578: Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter().

Problem / Motivation

For implementing web services an easy way to check entity access is necessary, such that inaccessible properties can be filtered out and access to inaccessible entities is denied. Of course, that API would be useful in many other situations as well, i.e. for pretty much every module that wants to generally support entities and incorporate access checks.

Implementation plan

Related issues

#1810320: Remove EntityTranslationControllerInterface::getAccess() once have entity access is postponed on this issue.

Follow-up issues

#1825332: Introduce an AccountInterface to represent the current user

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fago’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Issue tags: +Entity system, +API addition
Leeteq’s picture

Related discussion in #1704168: Move node access functions to separate module (closed as duplicate in favor of this issue)

larowlan’s picture

Issue tags: +#pnx-sprint

tagging

larowlan’s picture

Issue tags: +Platform Initiative
fubhy’s picture

Did anything happen during that sprint @larowlan? Otherwise I am going to start working on this.

larowlan’s picture

Fubhy, go for it - pnx-sprint is a weekly mini-sprint.

YesCT’s picture

YesCT’s picture

Issue summary: View changes

Updated issue summary.

Wim Leers’s picture

Spark's Edit module D8 port is also blocked on this.

(I tried duplicating entity_access() just for Edit for now, but core doesn't yet implement the necessary access callbacks and I didn't feel like duplicating all that. Hence, I currently am skipping permissions checking until this is ready.)

Wim Leers’s picture

Issue tags: +Spark

Tagging.

fubhy’s picture

FileSize
17.69 KB

Okay. So the first step for me would be to implement access checks for loaded entity objects.

  • Each operation is a separate method to stop the current switch() craziness in our existing implementations.
  • The method name on the controller equals the operation name.
  • The interface requires 'view', 'create', 'update', 'delete' to be set on each access controller.
  • Custom operations can be added easily. The operation is simply mapped to the method name.
  • Entity types that don't implement an access controller are completely unprotected.
  • A controller can extend EntityAccessBase to return FALSE for all unsupported operations through __call(). => Unlike in case of a non-existent controller [in which case all access checks return TRUE] a 'protected' entity type should return FALSE for all unknown operations. I am not sure if __call() should be part of the interface though. I just thought it could be handy to support fallbacks for entity types that are unaware of certain operations that some other entity types might use. Could come in handy for modules that work with entities in a generic fashion.

So... regarding "any" access ("general" access): I was not entirely sure how we should implement that or if it is even part of this issue. AnyAccess checks work in such a custom way across core and contrib (does the langcode matter? does the bundle matter? etc.) that I wasn't really sure how to reflect that in the controller.

I started with something like this:

function entity_access($entity_type, $operation = 'view', $bundle = NULL, $langcode = NULL, \Drupal\user\User $user = NULL) {
  if (!$controller = entity_access_controller($entity_type)) {
    return TRUE;
  }
  return $controller->{"{$operation}Any"}($bundle, $langcode, $user);
}

But fago had some objections. So I am leaving this open for discussion for now.

fubhy’s picture

Status: Active » Needs review
Damien Tournoud’s picture

I would avoid calling the method $operation directly. I would recommend ${operation}Access, so that we can safely add methods to the access controllers later on.

fago’s picture

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -251,8 +251,17 @@ public function getIterator() {
+      // If no access controller is defined the entity is fully accessible by
+      // every user.
+      return TRUE;

Let's better return NULL in this case. For a lot of module TRUE is a good default, but not for all. With NULL, modules can decide upon the default theirselves.

+++ b/core/lib/Drupal/Core/Entity/EntityAccessBase.php
@@ -0,0 +1,42 @@
+abstract class EntityAccessBase implements EntityAccessControllerInterface {

I'd call that "AccessControllerBase". With namespaces we don'T necessaryil need the Entity prefix any more, see DataBaseStorageController.

+++ b/core/lib/Drupal/Core/Entity/EntityAccessBase.php
@@ -0,0 +1,42 @@
+   * Restricts access for all unknown operations.

Should describe how it canbe used to support unknown operations.

+++ b/core/lib/Drupal/Core/Entity/EntityAccessControllerInterface.php
@@ -0,0 +1,94 @@
+   * @param string $langcode
+   *   The language code for which to check access for.

Can we have that default to the default entity language? As default value, we can use the LANGUAGE_DEFAULT constant.

+++ b/core/lib/Drupal/Core/Entity/EntityAccessControllerInterface.php
@@ -0,0 +1,94 @@
+   *   (optional) The user form whom to check 'view' access for the given

typo

+++ b/core/lib/Drupal/Core/Entity/EntityAccessControllerInterface.php
@@ -0,0 +1,94 @@
+   *   entity. Defaults to the current user.

Should say, that if it's NULL the current user is used.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityAccessTest.php
@@ -0,0 +1,71 @@
+    // Check that unsupported operations return FALSE through __call().
+    $this->assertEntityAccess(array(
+      'foo' => FALSE,
+      'bar' => FALSE,
+    ), $entity);

Calling it with an unsupported operation? I'd expect an InvalidArgumentException - not a valid return value.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTranslationAccessTest.php
@@ -0,0 +1,56 @@
+class EntityTranslationAccessTest extends EntityAccessTestBase  {
+
+  public static function getInfo() {
+    return array(
+      'name' => 'Entity translation access',
+      'description' => 'Tests access for entity translations.',
+      'group' => 'Entity API',

I'm not sure we really need a separate test case for that. Why not just move it over to entity access tests and have everything in one place?

+++ b/core/modules/system/tests/modules/entity_access_test/entity_access_test.module
@@ -0,0 +1,34 @@
+/**
+ * Implements hook_permission().
+ */
+function entity_access_test_permission() {
+  return array(
+    'view test entity' => array(
+      'title' => t('View test entities'),
+    ),
+    'view test entity translations' => array(

What about the idea of having that in the access controller?

E.g. have a method

hook_permission() and invoke it from system.module for all access controllers. Then, an access controller can define and test it's own access permissions.

+++ b/core/modules/system/tests/modules/entity_test/entity_test.module
@@ -13,6 +13,7 @@ function entity_test_entity_info() {
+    'access controller class' => 'Drupal\entity_test\EntityTestAccessController',

Misses docs.

I am not sure if __call() should be part of the interface though.

I don't think so. Probably it even shouldn't be part of the base controller. For defining support for additional $ops one might want to implement a proper method anyway. Imo, it suffices to document at the access controller interface how custom operations can be supported.

I would avoid calling the method $operation directly. I would recommend ${operation}Access, so that we can safely add methods to the access controllers later on.

Indeed.

So... regarding "any" access ("general" access): I was not entirely sure how we should implement that or if it is even part of this issue. AnyAccess checks work in such a custom way across core and contrib (does the langcode matter? does the bundle matter? etc.) that I wasn't really sure how to reflect that in the controller.

Yep. Before introducing lots of different combinations, I think we'd have to research what's really used somewhere, that's definitely a follow-up though. I could see us already adding general All access, but I'm fine with postponing that as well.

fubhy’s picture

FileSize
15.61 KB

Let's better return NULL in this case. For a lot of module TRUE is a good default, but not for all. With NULL, modules can decide upon the default theirselves.

Okay, agreed.

I'd call that "AccessControllerBase". With namespaces we don'T necessaryil need the Entity prefix any more, see DataBaseStorageController.

Entirely removed for now.

Should describe how it canbe used to support unknown operations.

Same.

Can we have that default to the default entity language? As default value, we can use the LANGUAGE_DEFAULT constant.

Agreed.

Should say, that if it's NULL the current user is used.

Hmm... It does?! :)

I'm not sure we really need a separate test case for that. Why not just move it over to entity access tests and have everything in one place?

Yes... I had much more stuff there before I removed the "general" access bits. And yes... let's keep them separated for now. Without them this patch is actually really simple and small.

What about the idea of having that in the access controller?

I am not sure. In many cases entity access does not have explicit entity-related permissions but instead uses permissions that e.g. other modules provide or one that is not limited to the entity. Also, that would mean that we would have to use system module for it which I really don't like :(.

I would avoid calling the method $operation directly. I would recommend ${operation}Access, so that we can safely add methods to the access controllers later on.

Right... done.

klausi’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -251,8 +251,17 @@ public function getIterator() {
+    if (!$controller = entity_access_controller($this->entityType)) {
+      // If no access controller is defined the entity is fully accessible by
+      // every user.
+      return TRUE;
+    }

IMO, this should default to FALSE if there is no access controller. I cannot think of an entity type that would allow view and edit for arbitrary users. So TRUE is not really a sensible default.

fubhy’s picture

Status: Needs work » Needs review

Hmm... Not sure about that. I think that really is a question of how we define it. Let's see what others think about that.

fubhy’s picture

FileSize
15.04 KB

Okay... I am on board now with defaulting to FALSE. That indeed is a much more sensible default. Thanks klausi. I also did some further cleanup (still had some leftovers from the more complex implementation left). This patch now basically just defines the interface and how it talks to entities and entity translations. Everything else should be postponed to follow-ups as this issue currently blocks quite a few others.

fago’s picture

Let's better return NULL in this case. For a lot of module TRUE is a good default, but not for all. With NULL, modules can decide upon the default theirselves.

I thought you agreed to having it default to NULL? :-) As said, the reasonable default depends on the use case - so let modules treat the case as they want.

fubhy’s picture

FileSize
14.92 KB

lol... misunderstanding there :). Okay.. another re-roll then:

klausi’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -251,8 +251,16 @@ public function getIterator() {
+    if (!$controller = entity_access_controller($this->entityType)) {
+      // No access controller is defined.
+      return NULL;
+    }

That violates the contract of AccessibleInterface::access(). TRUE or FALSE, what's it gonna be?

pounard’s picture

Deny all and allow on a per use case basis as a default for all access operations sounds security wise. I'd recommend FALSE here. But, this is a special case since there is no access controller: it's matter of do we decide there will always be an access controller? Case in which we need to enforce a default access controller as fallback and not treat this case that should not happen. Not having an access controller sounds like a pragmatic error that should throw an exception, thus leaving the code in a consistent state where we cannot not use an access control for entities. What do you think?

sun’s picture

"No" is the answer you get for all security questions by default.

AFAIK, there is no entity in core that has absolutely zero access conditions, at minimum there's a user permission. The edge-case entity that really has none can implement a custom NullAccessController.

pounard’s picture

I think we agree, so this:

function entity_access_controller($entity_type) {
  $controllers = &drupal_static(__FUNCTION__, array());
  if (!isset($controllers[$entity_type])) {
    $type_info = entity_get_info($entity_type);
    if (isset($type_info['access controller class']) && $class = $type_info['access controller class']) {
      $controllers[$entity_type] = new $class($entity_type);
    }
    else {
      $controllers[$entity_type] = FALSE;
    }
  }
  return $controllers[$entity_type];
}
[...]
  public function access($operation = 'view', \Drupal\user\User $user = NULL) {
    if (!$controller = entity_access_controller($this->entityType)) {
      // No access controller is defined.
      return NULL;
    }
    if ($operation == 'edit') {
      // Map the 'edit' operation to 'update'.
      $operation = 'update';
    }
    return $controller->{"{$operation}Access"}($this, LANGUAGE_DEFAULT, $user);
  }

Should become this:

function entity_access_controller($entity_type) {
  $controllers = &drupal_static(__FUNCTION__, array());
  if (!isset($controllers[$entity_type])) {
    $type_info = entity_get_info($entity_type);
    if (isset($type_info['access controller class']) && $class = $type_info['access controller class']) {
      $controllers[$entity_type] = new $class($entity_type);
    }
    else {
      $controllers[$entity_type] = new NullEntityAccessController($entity_type);
    }
  }
  return $controllers[$entity_type];
}
[...]
  public function access($operation = 'view', \Drupal\user\User $user = NULL) {
    if ($operation == 'edit') {
      // Map the 'edit' operation to 'update'.
      $operation = 'update';
    }
    return entity_access_controller($this->entityType)
      ->{"{$operation}Access"}($this, LANGUAGE_DEFAULT, $user);
  }
fubhy’s picture

Status: Needs work » Needs review
FileSize
16.41 KB

Okay. Adding a defualt controller as suggested by @pounard. This might look like it is redundant because it simply returns FALSE all the time but at the same time it gives us consistency for the return value of ->access and we don't have to implement custom behaviors there everywhere we use entity_access_controller() somewhere. Plus, custom entities only have to implement the methjods that they want to support in a custom way.

fubhy’s picture

FileSize
16.75 KB

Moving the default controller definition to entity_get_info().

Also @see #1821662: Register entity controllers with the DIC.

Lars Toomre’s picture

Status: Needs review » Needs work
+++ b/core/includes/entity.incundefined
@@ -340,6 +341,27 @@ function entity_page_label(EntityInterface $entity, $langcode = NULL) {
+ *
+ * @return Drupal\Core\Entity\EntityAccessControllerInterface
+ *   An entity access controller instance.

Small nit if this gets re-rolled... This should have an initial slash '\' according to #1487760: [policy, no patch] Decide on documentation standards for namespaced items.

Lars Toomre’s picture

Status: Needs work » Needs review

Whoops... Did not mean to change status.

fago’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.php
@@ -0,0 +1,44 @@
+ * Default entity access controller implementation.

Should mention that it defaults to access being FALSE.

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.php
@@ -0,0 +1,44 @@
+class EntityAccessController implements EntityAccessControllerInterface {

class should tell what it is: e.g. call it
DefaultAccessController ? or better NoAccessController ?

+++ b/core/lib/Drupal/Core/Entity/EntityAccessControllerInterface.php
@@ -0,0 +1,82 @@
+   *   (optional) The user for whom to check 'view' access for. Defaults to
+   *   the current user.

Nitpicking, but it defaults to NULL, what means the current user.
"+ * (optional) The user for whom to check 'view' access for or NULL to check access for the current user. Defaults to NULL.

sun’s picture

Turning it into the default access controller makes sense. I'd imagine that it could even be the base controller for all others, so anything that isn't overridden defaults to FALSE.

That said, I see CRUD in there, but reality is CRUDL. I think it would be wise to add a listAccess() method right from the start.

fubhy’s picture

@sun I book that under "AnyAccess" methods (I would not call it list access, I think that term rather describes "query access"). Those need custom arguments that are different to those used by our single "loaded" object access checks. Just look at node_access for example. We would also need an optional $bundle argument there. And I think we can find quite a few more exceptions. In the end we will have to define an interface that would be able to solve most of those custom use-cases but it would take a considerable amount of bikeshedding time to figure that out. Considering that this issue currently blocks others and the patch is really just a super simple foundation for further entity access issues we should wrap this up first before we discuss any other features that we need to cover.

That said, in the end we will not only need something like CRUDL (LIST) we basically need AnyAccess check capabilities for CREATE, UPDATE, DELETE, etc. - We might even want to discuss if we want a global access check that would out-rule all others (accessByPass() or something). But, again, I think we should do that stuff in a follow-up.

pounard’s picture

L is not an entity permission, but the aggregation of successive R operations over a set of entities. I don't think it makes sense to integrate it into the access controller, this check belongs at query time, when listing.

fago’s picture

I don't think it makes sense to integrate it into the access controller, this check belongs at query time, when listing.

I could see us making an exception for "all-access", but generally - yes.

But, again, I think we should do that stuff in a follow-up.

Agreed!

fubhy’s picture

FileSize
16.88 KB

Apart from renaming the default controller (all the other controllers / base controllers are prefixed with Entity too currently and I don't want to make the first step) I believe I did everything that was asked for.

Lars Toomre’s picture

Status: Needs review » Needs work

Here are some comments from reading through the patch in #33.

+++ b/core/lib/Drupal/Core/Entity/EntityAccessControllerInterface.phpundefined
@@ -0,0 +1,82 @@
+   *
+   * @param EntityInterface $entity
+   *   The entity to check 'view' access for.
+   * @param string $langcode

This needs to be a fully qualified path to EntityInterface. Same below.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityAccessTest.phpundefined
@@ -0,0 +1,121 @@
+
+use Drupal\simpletest\WebTestBase;
+use Drupal\Core\Language\Language;
+use Drupal\Core\TypedData\AccessibleInterface;
+use Drupal\user\User;

Can these be sorted alphabetically?

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityAccessTest.phpundefined
@@ -0,0 +1,121 @@
+    foreach ($ops as $op => $result) {
+      $msg = t("Entity access returns @result with operation '@op'.", array('@result' => isset($result) ? 'null' : ($result ? 'true' : 'false'), '@op' => $op));
+      $this->assertEqual($result, $object->access($op, $user), $msg);

Can we change $msg to $message? Also, there is not need to use t() here. Use format_string() instead.

Also, for readability, it would help to breakout the array elements on separate lines.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityAccessTest.phpundefined
@@ -0,0 +1,121 @@
+    // The custom user is not allowed to view test entities.
+    $user = $this->drupalCreateUser();
+    $this->assertEntityAccess(array(

It would be helpful not reuse the $user variable here. Perhaps call this $custom_user instead?

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityAccessTest.phpundefined
@@ -0,0 +1,121 @@
+    variable_set('entity_test_translation', TRUE);

Why not use the state() system instead of variable_set()?

Lars Toomre’s picture

Status: Needs work » Needs review

Did not mean to change status...

fubhy’s picture

FileSize
17.02 KB

Next try.

Edit:

Can we change $msg to $message? Also, there is not need to use t() here. Use format_string() instead.

Oh, good catch. That was copied over from the NodeAccess tests.

Can these be sorted alphabetically?

I don't think we have an actual standard for that yet: #1624564: Coding standards for "use" statements
But sure, I can reorganize those.

It would be helpful not reuse the $user variable here. Perhaps call this $custom_user instead?

Okay

Why not use the state() system instead of variable_set()?

This is part of the entity_test module and the way that the translation tests trigger the stuff they inject in their mock modules. I am just using it too!

Lars Toomre’s picture

Thanks @fubhy! I presume that there is an issue somewhere to convert those variable_set()'s. We should add a note to that one so that they are aware that we are using the variable here too.

Status: Needs review » Needs work
Issue tags: -Entity system, -API addition, -Platform Initiative, -#pnx-sprint, -Spark

The last submitted patch, 1696660-36.patch, failed testing.

fubhy’s picture

Status: Needs work » Needs review
Issue tags: +Entity system, +API addition, +Platform Initiative, +#pnx-sprint, +Spark

#36: 1696660-36.patch queued for re-testing.

fago’s picture

FileSize
18.26 KB

Technically, the patch looks good, however it misses docs on the possible values for $op in the accessibleinterface as well as on the entityinterface level. Also, the $user docs got only updated for viewAccess. I fixed that and added some docs for $op.

Note, that we cannot redefine access() on the EntityInterface as this results in a fatal (at least on my install), maybe it's working with php 5.3.10. See related comment: http://drupal.org/node/1800122#comment-6630376

Updated patch attached.

sun’s picture

This looks good to me. Only a small, but important nit:

-  public function access(\Drupal\user\User $account = NULL) {
...
+  public function access($operation = 'view', \Drupal\user\User $user = NULL) {

Everywhere in this patch:

The user account should always be in an $account variable, not $user, so as to ensure that it never clashes with global $user.

fubhy’s picture

FileSize
18.34 KB

Okay. This should be good to go then!

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

This patch looks fine to me. It makes a good, basic step towards a more sophisticated entity access API.

There's a lot of work ahead there, so I think it makes sense to move forward here and to hash out more granular details in the follow-up issues.

A unified entity access API is very badly needed. It should have been in D7 already, and the fact that it wasn't made the entity system considerably harder to work with. Therefore, fixing this gap is critically important for D8 from my perspective.

(At some point in the process, I want and need to make sure that special use-cases of modules like Mollom are taken care of, which act on all entity types, and also, which sometimes need to figure out "reverse access permissions" which seem to be foreseen as bypass permissions in here. But yeah, laters.)

plach’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -250,9 +250,17 @@ public function getIterator() {
+    if ($operation == 'edit') {
+      // Map the 'edit' operation to 'update'.
+      $operation = 'update';
+    }

+++ b/core/lib/Drupal/Core/Entity/Field/Type/EntityTranslation.php
@@ -233,8 +233,12 @@ public function isEmpty() {
+    if ($operation == 'edit') {
+      // Map the 'edit' operation to 'update'.
+      $operation = 'update';
+    }

Eeew. We need at very least a @todo about fixing the AccessibleInterface and remove this hack. This is really bad DX.

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.php
@@ -0,0 +1,47 @@
+use Drupal\user\User;

+++ b/core/lib/Drupal/Core/Entity/EntityAccessControllerInterface.php
@@ -0,0 +1,82 @@
+  public function viewAccess(EntityInterface $entity, $langcode = LANGUAGE_DEFAULT, \Drupal\user\User $account = NULL);
...
+  public function createAccess(EntityInterface $entity, $langcode = LANGUAGE_DEFAULT, \Drupal\user\User $account = NULL);
...
+   */
+  public function updateAccess(EntityInterface $entity, $langcode = LANGUAGE_DEFAULT, \Drupal\user\User $account = NULL);
...
+  public function deleteAccess(EntityInterface $entity, $langcode = LANGUAGE_DEFAULT, \Drupal\user\User $account = NULL);
+}
...
+++ b/core/lib/Drupal/Core/Entity/Field/Type/Field.php
@@ -297,7 +297,7 @@ public function __clone() {
+  public function access($operation = 'view', User $account = NULL) {
...
+++ b/core/lib/Drupal/Core/TypedData/AccessibleInterface.php
@@ -15,12 +15,18 @@
-  public function access(\Drupal\user\User $account = NULL);
+  public function access($operation = 'view', \Drupal\user\User $account = NULL);

These are violating the requirement stating that code in the Drupal\Core namespace may not depend on code provided by a module. I am afraid we need a Drupal\Core\AccountInterface (or somesuch) that Drupal\user\User implements.

Also, why aren't we using the use statement here?

fago’s picture

+ if ($operation == 'edit') {
+ // Map the 'edit' operation to 'update'.
+ $operation = 'update';
+ }

Eeew. We need at very least a @todo about fixing the AccessibleInterface and remove this hack. This is really bad DX.

Well, having different $ops is not something invented here, we already use 'view' and 'edit' for field access while we use 'create', 'update', 'delete' and 'view' for entity access. I'm not sure it's such a bad hack - it's just mapping between our existing ops. I agree that's unfortunate, but how it is now. We could discuss unifying $ops in a separate issue, but how create/update/delete would apply to a field is questionable.

These are violating the requirement stating that code in the Drupal\Core namespace may not depend on code provided by a module. I am afraid we need a Drupal\Core\AccountInterface (or somesuch) that Drupal\user\User implements.

True, but that's not invented by this patch either. I'm not sure what's the best way to handle this, but I think we should care about that in its own issue.

plach’s picture

I agree that's unfortunate, but how it is now. We could discuss unifying $ops in a separate issue, but how create/update/delete would apply to a field is questionable.

We can totally defer this, that's why I suggested a @todo. IMHO the pain point is only that IIUC we are using 'edit' and 'update' to indicate the same op, albeit in different contexts.

True, but that's not invented by this patch either. I'm not sure what's the best way to handle this, but I think we should care about that in its own issue.

If we remove those FQCN, what about a @todo above the related use statement?

fubhy’s picture

Status: Needs work » Needs review
FileSize
2.8 KB
18.41 KB

We can totally defer this, that's why I suggested a @todo. IMHO the pain point is only that IIUC we are using 'edit' and 'update' to indicate the same op, albeit in different contexts.

'Update' is a type of 'edit' operation with the current meaning of 'edit' (for fields, for example, 'edit' covers 'create', 'update' as well as 'delete' operations).

I added the @todo's to both interfaces (EntityAccessControllerInterface and AccessibleInterface).

But yes, that user module dependency was not introduced here and should be fixed somewhere else too. I am opening a follow-up for that.

Edit: Oh, that interdiff is in reverse. But you get the idea... :)

fubhy’s picture

fubhy’s picture

fubhy’s picture

Status: Needs review » Reviewed & tested by the community

So... It's green, we got the @todo's in place and the follow-up issue have been created. Resetting this to RTBC. Let's proceed in the follow-ups.

Will write the change notice once it's committed.

sun’s picture

plach’s picture

@fubhy:

Thanks!

fago’s picture

I agree this is good to go.

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Entity system, +API addition, +Platform Initiative, +Entity Access, +#pnx-sprint, +Spark

The last submitted patch, 1696660-47.patch, failed testing.

fubhy’s picture

Status: Needs work » Needs review
FileSize
19.66 KB

Re-roll. Also removed entity_access_test mock module in favor and put that stuff into entity_test directly.

Stalski’s picture

Status: Needs review » Reviewed & tested by the community

Looking good to me

catch’s picture

Status: Reviewed & tested by the community » Needs work

Looks fine to me although I haven't done a full review yet.

It's a simple enough patch but that's a good thing in this case. Couple of questions?

+++ b/core/lib/Drupal/Core/Entity/Entity.phpundefined
@@ -250,9 +250,17 @@ public function getIterator() {
+    if ($operation == 'edit') {
+      // Map the 'edit' operation to 'update'.
+      $operation = 'update';

Why? Can't we just change the calling code to use 'update'? Also it's a bit odd with the $op argument, why not four methods like the access interface?

fubhy’s picture

Why? Can't we just change the calling code to use 'update'? Also it's a bit odd with the $op argument, why not four methods like the access interface?

We need a single entry point. This implements the AccessibleInterface which excepts 'edit' and 'view' as $op. We simply extend that but since entity access checks have more granular permissions we have to do this mapping.

fago’s picture

Status: Needs work » Reviewed & tested by the community

Why? Can't we just change the calling code to use 'update'?

It can. This is just to be consistent with the AccessibleInterface, which defines 'view' and 'edit' as used for fields. Imho the docs are unnecessary complicated becaues of that, so a little wrapper as suggested in #1825346: Introduce an access wrapper function for use in menu system access callbacks, etc. could help that as well as redefining the access() method in the entity interface, once we required 5.3.10 (see #40)
Should we include the entity_access() wrapper with this patch to ease things right now?

Also it's a bit odd with the $op argument, why not four methods like the access interface?

Maybe, but it might be even useful to have $op in some cases - e.g. if your general edit page just has a $op that differentaties between create and update. Also I think this goes more inline which we had before as node_access() (and contrib entity_access() had it that way, so I'd prefer keeping it this way right now.

Setting back to RTBC for getting committer feedback.

chx’s picture

We have an existing node access system and I have proposed elevating it to entity access in #1819726: Move node access code into a pluggable class

Edit: this is off topic, I misunderstood the intent of this patch. The next comment fixes the misunderstanding by retitleing the issue.

chx’s picture

Title: Add an entity access API » Add an entity access API for single entity access
Status: Reviewed & tested by the community » Needs work

{"{$operation}Access"} you must be kidding me. This is unreadable. Please use separate variables.

fubhy’s picture

Status: Needs work » Needs review
FileSize
19.7 KB
chx’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work
Issue tags: -Entity system, -API addition, -Platform Initiative, -Entity Access, -#pnx-sprint, -Spark

The last submitted patch, 1696660-63.patch, failed testing.

agentrickard’s picture

Status: Needs work » Needs review

#63: 1696660-63.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1696660-63.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review

#63: 1696660-63.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Entity system, +API addition, +Platform Initiative, +Entity Access, +#pnx-sprint, +Spark

The last submitted patch, 1696660-63.patch, failed testing.

fubhy’s picture

Status: Needs work » Needs review
FileSize
19.73 KB

Status: Needs review » Needs work
Issue tags: -Entity system, -API addition, -Platform Initiative, -Entity Access, -#pnx-sprint, -Spark

The last submitted patch, 1696660-70.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review
Issue tags: +Entity system, +API addition, +Platform Initiative, +Entity Access, +#pnx-sprint, +Spark

#70: 1696660-70.patch queued for re-testing.

webflo’s picture

Fixed the ViewsUI:access() signature.

fubhy’s picture

Status: Needs review » Reviewed & tested by the community

Okay. I have no clue what's up with the ViewsUI.php and __call() but that's not part of this issue.

catch’s picture

Status: Reviewed & tested by the community » Needs work

@fago I think the entity_access() wrapper (with a note it's especially for access callbacks) would be good yeah. Quick re-roll with that?

Berdir’s picture

We've kept and renamed entity_label() to entity_page_label() for that purpose. Not sure if entity_page_access() makes sense, though.

Also, access callbacks will be replaced with those access listeners I think, that one might be able to directly call ->access() we just need to tell him how to get the entity...

fubhy’s picture

Status: Needs work » Reviewed & tested by the community

I am not sure if that makes sense really. I am currently hacking on a follow-up patch to the {slug} upcasting patch from @katbailey (#1798214: Upcast request arguments/attributes to full objects) (slightly modified though) and also on the _access one from here: #1793520: Add access control mechanism for new router system. We are really beyond the constraints of having to have stupid callbacks that we can invoke from the menu system. It's capable of much than that now. I am a big fan of the access check compiler pass introduced in that issue which would allow us to do '_entity_access' directly on the on the route instead of having a stupid callback. Can't we just skip entity_access for now (because, really... where are we going to use that once hook_menu() is really dead?!). I am going to post a follow-up patch to #1793520: Add access control mechanism for new router system (which currently adds _access and _permission) which would add _entity_access or something. But let's get this in first, it's really not blocked on that if you ask me.

catch’s picture

So if we don't have the separate callback, how can we get rid of this then?

+    if ($operation == 'edit') {
+      // Map the 'edit' operation to 'update'.
+      $operation = 'update';
+    }
fubhy’s picture

Stop! It's fago time! (McHammer playing in the background).

catch’s picture

Status: Reviewed & tested by the community » Needs review
fago’s picture

#73: 1696660-73.patch queued for re-testing.

fago’s picture

ad #77: If we don't need the callback anymore, the better. +1

webchick’s picture

Priority: Normal » Major

Seems like there are quite a few things out there that would like to leverage this feature if it existed, so escalating to a "major" feature.

fago’s picture

#73: 1696660-73.patch queued for re-testing.

fubhy’s picture

FileSize
19.19 KB

After discussing this with fago and catch on IRC we agreed that it would be much cleaner to extend the AccessibleInterface to support more granular $operations and move the mapping into the (not yet implemented) Field/Property access method. So with this patch the AccessibleInterface supports 'view', 'create', 'update', 'delete' instead of just 'view' and 'edit'. The mapping to 'edit' has consequently been removed.

fago’s picture

Status: Needs review » Reviewed & tested by the community

I like this. Changes look good. Back to RTBC given tests are green.

hefox’s picture

Sorry, haven't been quite following this issue, but Just going to mention that I've struggled with the limitation of node_access ops: even in d6 it's been a solid reliable way to check for access checks, but the ops is a limitation when trying to build custom access logic.

For example, premium access which has the logic that users can see a teaser version of a node but not the full content.

Also, when looking access when looking into #849602: Update 'username' theme template to use 'view label' operation., there was different types of access like there is both 'cancel' and 'delete' account with seperate logic.

so tl;dr: limited ops, limited possibilities.

fubhy’s picture

You can always add custom $ops. The interface just defines which $ops HAVE to be supported. You can add more.

hefox’s picture

Oh! Cool =)

To clarify, can someone add a new op to an already defined entity (e.g. could premium add a 'view_teaser' to node entity) or would the entity be in complete control of what ops it implements (e.g. user entity have 'cancel' and 'delete' ops)?

Based on quick look at the patch, node entity is not implementing this yet?

I'm not too familair with how the entity system works (stuck in d6 land).

fubhy’s picture

Hmm... To be honest: That is a pretty good question. This would not work easily as of now (you would have to replace the entity access controller for nodes in order to make that happen). However, that does not work anymore once there are multiple modules that want to add stuff. We could probably implement something along the lines of how form controllers currently work. You got a default one but you can add additional, custom form controllers.

fubhy’s picture

Oh, but let's move that discussion to a follow-up... I opened #1842896: Consider adding support for custom $operations for single entity access - Let's discuss a possible implementation for that there.

sun’s picture

I think that question rather maps to #1839516: Introduce entity operation providers

But I agree it is a very good question. Over there, I sorta arrived at the conclusion that each entity operation should probably be a plugin, so any module can add further operations or alter the existing. I almost wonder whether the entity access API shouldn't also be based on plugins, and wonder even further, whether the access API shouldn't be completely bound to entity operations instead of being a standalone thing...?

fubhy’s picture

Yeah. I think I agree with that idea. Let's get this committed anyways because the implementation should still pretty much look the same. However, I am very interested in pushing this further and would like to start writing some code for that other issue asap (unless you already started with that).

amateescu’s picture

Re #92 I know I'm offtopic but I have to say this: Wow, you do wonder a lot! =))

xjm’s picture

Assigned: Unassigned » xjm

Nifty.

whether the access API shouldn't be completely bound to entity operations instead of being a standalone thing

I think this is potentially an excellent idea (as a followup).

Questions:

  1. +++ b/core/lib/Drupal/Core/Entity/EntityAccessControllerInterface.phpundefined
    @@ -0,0 +1,85 @@
    +// @todo Don't depend on module level code.
    +use Drupal\user\Plugin\Core\Entity\User;
    

    What does this mean? Is there a followup issue?

  2. +++ b/core/lib/Drupal/Core/Entity/Field/Type/EntityTranslation.phpundefined
    @@ -233,8 +233,9 @@ public function isEmpty() {
    +  public function access($operation = 'view', \Drupal\user\Plugin\Core\Entity\User $account = NULL) {
    +    $method = $operation . 'Access';
    +    return entity_access_controller($this->parent->entityType())->$method($this->parent, $this->langcode, $account);
    

    How will this interact with what we're doing over at #1658846: Add language support to node access grants and records? Presumably we'll eventually convert Node to implement this API?

  3. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityAccessTest.phpundefined
    @@ -0,0 +1,133 @@
    +    // Remove the access controller definition from the test entity.
    +    variable_set('entity_test_default_access_controller', TRUE);
    ...
    +    // Enable translations for the test entity type.
    +    variable_set('entity_test_translation', TRUE);
    
    +++ b/core/modules/system/tests/modules/entity_test/entity_test.moduleundefined
    @@ -16,6 +16,10 @@ function entity_test_entity_info_alter(&$info) {
    +  // Optionally unset the access controller to test the fallback.
    +  if (variable_get('entity_test_default_access_controller')) {
    

    Shouldn't we be using either config or state for this rather than introducing another variable?

  4. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityAccessTest.phpundefined
    @@ -0,0 +1,133 @@
    +    $this->assertTrue(is_a($controller, '\Drupal\Core\Entity\EntityAccessController'), 'The default entity controller is used for the entity_test entity type.');
    

    is_a() is deprecated in favor of instanceof: http://us1.php.net/instanceof#example-131

And various doc formatting things:

  1. +++ b/core/lib/Drupal/Core/Entity/EntityAccessControllerInterface.phpundefined
    @@ -0,0 +1,85 @@
    +   * @param string $langcode
    +   *   (optional) The language code for which to check access for. Defaults to
    +   *   LANGUAGE_DEFAULT.
    +   * @param \Drupal\user\Plugin\Core\Entity\User $account
    +   *   (optional) The user for whom to check access for, or NULL to check access
    

    Minor issue: The word "for" is repeated twice in documentation of these parameters for each method.

  2. +++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
    @@ -0,0 +1,47 @@
    + * Base class for entity access controllers.
    
    +++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestAccessController.phpundefined
    @@ -0,0 +1,49 @@
    + * Access controller for the test entity.
    

    Verbify!

  3. +++ b/core/includes/entity.incundefined
    @@ -305,6 +305,27 @@ function entity_page_label(EntityInterface $entity, $langcode = NULL) {
    + *
    + * @see hook_entity_info()
    

    The @see should be at the end of the docblock. Also, it would be better to link to EntityManager instead now.

  4. +++ b/core/includes/entity.incundefined
    @@ -305,6 +305,27 @@ function entity_page_label(EntityInterface $entity, $langcode = NULL) {
    + * @return Drupal\Core\Entity\EntityAccessControllerInterface
    

    Missing the initial slash.

  5. +++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
    @@ -0,0 +1,47 @@
    + * Definition of Drupal\Core\Entity\EntityAccessController.
    
    +++ b/core/lib/Drupal/Core/Entity/EntityAccessControllerInterface.phpundefined
    @@ -0,0 +1,85 @@
    + * Definition of Drupal\Core\Entity\EntityAccessControllerInterface.
    

    These should be switched to "Contains" rather than "Definition of".

I'll reroll to clean up the nitpicky things.

fubhy’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityAccessControllerInterface.phpundefined
@@ -0,0 +1,85 @@
+// @todo Don't depend on module level code.
+use Drupal\user\Plugin\Core\Entity\User;
What does this mean? Is there a followup issue?

We are currently relying on a User entity there. Eventually we should move to a AccountInterface or something like that in Drupal\lib\Core so we don't reference module level code (user module -> user entity) in there.

Thanks for the detailed review.

xjm’s picture

Assigned: xjm » Unassigned
FileSize
9.2 KB
19.2 KB

We are currently relying on a User entity there. Eventually we should move to a AccountInterface or something like that in Drupal\lib\Core so we don't reference module level code (user module -> user entity) in there.

Alrighty, so followup issue? (= ?)

Attached fixes the rest of stuff.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, entity-1696660-97.patch, failed testing.

xjm’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
19.2 KB
+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityAccessTest.phpundefined
@@ -103,7 +103,7 @@ function testEntityAccessDefaultController() {
-    variable_set('entity_test_translation', TRUE);
+    state()->set('entity_test_translation', TRUE);

Whoops, didn't mean to change that. Reverted that one change here.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, entity-1696660-98.patch, failed testing.

fubhy’s picture

Status: Needs work » Reviewed & tested by the community

@xjm that already exists over here:
#1825332: Introduce an AccountInterface to represent the current user (@see comment #48 and before in this issue)

xjm’s picture

*sigh* Sorry for the noise.

fubhy’s picture

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityAccessTest.phpundefined
@@ -0,0 +1,133 @@
+    $this->assertTrue($controller instanceof '\Drupal\Core\Entity\EntityAccessController', 'The default entity controller is used for the entity_test entity type.');

That is still a string (for instanceof to work it has to be an actual object name)

fubhy’s picture

Issue summary: View changes

Updated issue summary. added info that #1810320: Remove EntityTranslationControllerInterface::getAccess() once have entity access is postponed on this issue.

xjm’s picture

That is still a string (for instanceof to work it has to be an actual object name)

Yeah, that's what #102 fixes.

fubhy’s picture

Yeah, sorry... x-post :/

Status: Reviewed & tested by the community » Needs work

The last submitted patch, entity-1696660-100.patch, failed testing.

Berdir’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
@@ -0,0 +1,47 @@
+/**
+ * @file
+ * Contains Drupal\Core\Entity\EntityAccessController.

Note: The latest iteration of #1487760: [policy, no patch] Decide on documentation standards for namespaced items defined that the namespace is no longer necessary here.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityAccessTest.phpundefined
@@ -0,0 +1,133 @@
+namespace Drupal\system\Tests\Entity;
...
+    // Check that the default access controller is used for entities that don't
+    // have a specific access controller defined.
+    $controller = entity_access_controller('entity_test');
+    $this->assertTrue($controller instanceof Drupal\Core\Entity\EntityAccessController, 'The default entity controller is used for the entity_test entity type.');

I think this is missing a \, that's why the tests are failing.

xjm’s picture

Note: The latest iteration of #1487760: [policy, no patch] Decide on documentation standards for namespaced items defined that the namespace is no longer necessary here.

Er... no, I don't think so. I'm fairly sure this is probably the most important place to include the full namespace. :) It only states that the full namespace does not need to be included inline in documentation. The file docblock has its own specific rule earlier in 1354. See:
http://drupal.org/node/1354#files
http://drupal.org/coding-standards/docs#namespaces

xjm’s picture

Oh, I see more deliberation by all of 2 people was added 3 days ago. I'll respond over there.

xjm’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1 KB
19.2 KB

Thanks @Berdir.

fubhy’s picture

FileSize
19.17 KB

Sorry, another re-roll.

+++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestAccessController.phpundefined
@@ -0,0 +1,49 @@
+   * Implements EntityAccessInterface::delete().

It's EntityAccessControllerInterface :/

webchick’s picture

Category: feature » task
Priority: Major » Critical
Status: Reviewed & tested by the community » Active

Awesome! This looks really good, and has sign off by all the right people.

We are over thresholds right now, but we were under over the weekend but I was at an event and unable to commit patches. So I'm cashing a raincheck. :)

Committed and pushed to 8.x. Thanks!

This will need a change notice, methinks.

effulgentsia’s picture

Title: Add an entity access API for single entity access » Change notice: Add an entity access API for single entity access
dawehner’s picture

Started with one on http://drupal.org/node/1862420

fago’s picture

Title: Change notice: Add an entity access API for single entity access » Add an entity access API for single entity access
Status: Active » Fixed

Thanks, I've added a pointer to the EntityAccessControllerInterface. Else, I think that's good, thus marking fixed.

Berdir’s picture

Do we need some follow-ups here to complete this? See for example #1807776-41: Support both simple and editorial workflows for translating entities, according to that many access checks are not yet correctly implemented (which also indicates that they aren't used/covered by tests).

Maybe an issue for each entity type to convert all access checks to the new API, ensure test coverage and also make sure it's working correctly. Not sure if there already is an issue to remove at least the generic access stuff from the translation handler in favor of this, if not, that needs an issue too.

fago’s picture

Wim Leers’s picture

larowlan’s picture

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

Anonymous’s picture

Issue summary: View changes

Added http://drupal.org/node/1825332 as a follow-up issue.