Field level access for EntityNG

That patch at least gets the access system working again so we don't regress when converting to EntityNG. If we don't get a different patch here quickly, we should proceed with naive Klausi.

#4 works for me, I guess.

At some point, when either "Field types as plugins" or "Field API to CMI" gets in, $field (Field API) is going to be an object.
At this point, I was planning to remove field_access($field, $op) with $field->access($op).

Would that still be doable in that scenario ?

Good to see EntityNG getting an implementation of field level access control.

Yeah, we already discussed this briefly on IRC.

The problem I see with hook_field_ng_access() is that it's not a field API hook but an entity (field) api hook, and so should be prefixed, documented and tested in the entity api. I'd suggest introducing hook_entity_field_access_alter() and implementing it by using field_access() from field module. It's alter as the main access code lives in the class. Once we've unified field API and entity field API we can think about removing hook_field_access() or not.

+++ b/core/modules/field/field.api.php
@@ -2200,5 +2202,31 @@ function hook_field_access($op, $field, $entity_type, $entity, $account) {
+function hook_field_ng_access($operation, Field $field, User $account) {

Hook-docu must use fully-qualified class names. Also, the docu does not really clarify access results to be FALSE as soon as a single module returns FALSE.

I think we could need yched's input on whether #10 works out, so assigning to him for that.

Just to make sure I got #10 right, here's my understanding, please correct me if I'm wrong:

The immediate plan would be:
- Core/Entity/Field/Type/Field::access() invokes hook_entity_field_access_alter($op, $field)
- field.module's implementation of the hook does

if ($field is a Field API field) { // like, $field instanceof OldFieldAPIField ? don't know the actual class name, if there's one
  return field_access($op, $the_field_definition_struct) // invokes the current hook_field_access()
}

The "future" plan would be:
- field_access() and hook_field_access() go away
- access is determined by $field->access($op), which invokes hook_entity_field_access()

Yes, I guess that works for me.

ad #12 - exactly, just the following:

access is determined by $field->access($op), which invokes hook_entity_field_access()

I'd only invoke hook_entity_field_access_alter(). So a pre-defined field, let's say user e-mail address can implement it's own custom access() method in the class, while modules may alter the default. Makes sense?

+++ b/core/includes/entity.api.php
@@ -392,3 +392,33 @@ function hook_entity_field_info_alter(&$info, $entity_type) {
+ * @todo: This hook will replace hook_field_access() after all field access

Well, I agree but I guess this will be influenced on how the field API and entiy field API unification works out in detail. So we'll have to figure it out in a follow-up - potentially we still want to have a field API specific hook exposing field API interals. Thus, I'd suggest to remove the todo and just create a follow-up issue for now.

+++ b/core/includes/entity.api.php
@@ -392,3 +392,33 @@ function hook_entity_field_info_alter(&$info, $entity_type) {
+ * @param array $context
+ *   Context array on the performed operation with the following keys:
+ *   - access: the original access flag from the default implementation
+ *     (boolean).
+ *   - operation: either "view" or "edit" (string).
+ *   - field: the field object (\Drupal\Core\Entity\Field\Type\Field).
+ *   - account: the user account to check access for

Why have a $context array and not just three separate parameters here?
Regarding field, I'd suggest descrbing it with as "entity field object" to be more clear it isn't a field API field array.

Also, we start with upper-case after the colon.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/FieldAccessTest.php
@@ -0,0 +1,49 @@
+ */
+class FieldAccessTest extends WebTestBase {
+

This should be a DrupalUnitTestCase. (Possibly existing ones also but that's another issue).

I've converted most of the existing entity tests to DUBT in #1893108: Convert most entity tests to DrupalUnitTestBase, that also adds a base test class that would simplify the set-up for you.

Please review :)

code looks good. fago's comments have been addressed.

+++ b/core/includes/entity.api.php
@@ -409,3 +409,30 @@ function hook_entity_field_info_alter(&$info, $entity_type) {
+ * This hook is invoked from Field::access() to let modules alter access to

This should have the fully namespaced class name I think, else developers probably won't find the class.

Because drupal_alter() does not support more than 2 context variables.

ouch. Still, the hook signature is very ugly like that and worse than what we had before, so I think we should try to find a better solution. Maybe better just use module_implements ourselves to implement it?

+++ b/core/lib/Drupal/Core/Entity/Field/Type/Field.php
@@ -290,6 +290,23 @@ public function __clone() {
   public function access($operation = 'view', User $account = NULL) {
-    // TODO: Implement access() method. Use item access.
+    global $user;
+    if (!isset($account)) {
+      $account = $user;
+    }
+    // Grant access per default.
+    $access = TRUE;
+    // We do not use drupal_alter() here because it only allows two context
+    // parameters which would make the signature of the hook very ugly.
+    foreach (module_implements('entity_field_access_alter') as $module) {

#10 says "It's alter as the main access code lives in the class.", but the above code neither implements a default nor adds a @todo to do so. So I don't understand why this is an 'alter' hook instead of a regular access hook, similar to hook_file_download_access(), where modules can return TRUE, FALSE, or NULL.

@effulgentsia: That's a nice example, because I am wondering if we still need that hook now that we have a generic entity access API. See #1327224-25: Access denied to taxonomy term image. The main reason for adding it was that there is no such thing and all entities need to implement it.

#10 says "It's alter as the main access code lives in the class.", but the above code neither implements a default nor adds a @todo to do so. So I don't understand why this is an 'alter' hook instead of a regular access hook, similar to hook_file_download_access(), where modules can return TRUE, FALSE, or NULL.

Well, that's the default, but I suppose other would work differently. E.g., I suppose the user e-mail address should not be accessible by default, or user-name has a permission for 'edit'. In that case we'll have a proper default which you need to alter in case you want to grant access?

@berdir: I think this case is different as we want field access module ala field permission to be able to chime in.

+++ b/core/lib/Drupal/Core/Entity/Field/Type/Field.php
@@ -290,6 +290,23 @@ public function __clone() {
+      // If a hook implementation altered $access to FALSE we can return
+      // immediately.
+      if ($access === FALSE) {
+        return FALSE;

I'd not expect that to happen in an alter hook. Why not just pass it through? If the code decides to alter a FALSE back to TRUE, just let it?

@fago: Swapping out the Field class sounds interesting, can you point me to an example where we do that already, in entity_test for example?

It's not done anywhere in core any-more I think. You can specify your own via 'list class', as you do with 'class'.

fago argued that the default implementation lives in the access() method of the class, therefore it is an alter hook. In this most generic case the default implementation is just $access = TRUE;

I still disagree with it being an alter hook. It doesn't follow our patterns elsewhere. hook_node_access(), hook_block_access(), hook_file_download_access(), AccessCheckInterface::access(), are all examples of a hook (or method) that returns TRUE, FALSE, or NULL (except for hook_node_access() which uses constants needlessly for legacy reasons), and the caller decides how to combine the results (generally, deny if one denies, allow if one allows and none deny, and implement some default if all return NULL). In some cases, this is also combined by the caller (e.g., node_access() and BlockBase::access()) implementing its own checks in addition to the hook.

Why do we want to deviate from that pattern here?

Note that what I'm questioning here isn't just the appending of "alter" to the hook name, but also the behavior of a later running hook implementation to simply override the result, disallowing the caller from deciding how to combine independent checks.

ok, looks like we have two kinds of access hooks here:

a) ones with custom access + a hook
b) ones without default access and just a hook

hook_node_access(), hook_block_access(), hook_file_download_access() seem to be of type a), while hook_field_access() and I think AccessCheckInterface::access() is b) (not sure here?).

However, all three using of type a) do not allow altering the default, e.g. if node-access detects the permission "bypass node access" or the lack of "access content" it immediately exits - there is no alteration involved here. So a module can never hook into that. The same for blocks - if path visibility says no no module can alter that.

So our implementations of type a) are not really defaults which modules may override, they are pre-defined custom access implementation you cannot override. However, in our case we specifically need the ability to override - so those patterns don't fit.

The pattern used by AccessCheckInterface::access() or hook_field_access() looks actually good to me, however they miss the important part of having a pre-defined $default_access. That's actually something we do not have a pattern for yet.
We could handle our default-access just as any other hook implementation also, but that would result in a weird behaviour if you return FALSE as no one could opt-into access then. It means, you'd have to return NULL instead of FALSE there unless you really mean that no-one should be able to override.

I actually stumbled over that when implementing hook_field_access() in contrib also - once you return FALSE no module can change that any more. I must say that relying on alter-hook-ordering isn't better in this situation either.

hook_file_download_access() is actually an interesting example as it has an access + an access alter hook. So it's as we'd have a consistent pattern here, on the contrary :D But I agree we should try to become consistent here, so ++effulgentsia for aiming for it here.

hook_node_access is class B, as of Drupal 7. (I wrote it that way specifically.) AccessCheckInterface followed that pattern deliberately. I cannot speak for hook_block_access or hook_file_download_access().

I think that's actually a much better model to follow in most cases. The lack of a "default" state is not an issue. Or, rather, it's up to the caller to decide if it should default-allow or default-deny. Both of the examples above are default-deny, but I could see field access making sense as default-allow. That's a much better model than an alter hook.

Node access has some custom access (access content, administer nodes) which you cannot override, thus I'd qualify it as class a)?

Anyway, we need more than just a default-grant or default-deny possibility - as we might have default access logic in certain fields/properties, e.g. consider the e-mail address field which comes with default edit permissions.

So what about that:
- add a defaultAccess() method which by default grants access and can be overridden by e.g. the e-mail class
- invoke hook_entity_field_access() using the usual pattern of returning TRUE, FALSE or NULL and deny access as soon as one module returns FALSE. defaultAccess() would be handled just as another module.
- invoke hook_entity_field_access_alter() to allow altering the result such that modules could really enforce granting access even if another module already returned FALSE (otherwise there would be no way back).

I'm against having hook_entity_field_access() + hook_entity_field_access_alter(), we should only have one hook. It would be an API WTF if we have two hooks that do very similar things.

I don't see this as a WTF, it's the usual pattern of hook + alter hook and what we already do for file download access.

The alter hook in the current state provides a maximum of flexibility, modules can even change access back from FALSE to TRUE if they want.

Yes, but for that relying on module order is really bad. There should be two clear steps, 1. provide access or 2. enforce/alter access if needed.

Thus, I still think the suggestion of #35 is good and should account for the concerns others had with the alter-only hook?

I agree with #37. Furthermore, I think the alter hook should be modeled on hook_file_download_access_alter(), where it's not given a single $access result to alter, but rather an array keyed by module (plus a key for the default, e.g., _default). The reason being that a module implementing this alter hook shouldn't normally be saying, "I know for sure that access should be allowed regardless of what *any* other module says", but rather be saying, "I want to override the default access logic, or module X's access logic".

Since the alter hook in this case would be very rarely needed, it can also be a real drupal_alter() passed a $context.

Note, if we do want to allow modules to implement a "allow access regardless of what other modules say" logic (e.g., what node.module does for people with "bypass node access" permission), then we can also introduce a separate hook, e.g. hook_entity_field_bypass_access() that we could run first, but I don't know if we have a legitimate use case for that. Without this hook, modules would need to implement hook_entity_field_access_alter() and override every item in the array to TRUE, which is okay, but implies something different: it implies "the other modules determined their access checks incorrectly" rather than "the other modules might be right, but this user is special and deserves to access the field regardless".

To me having the alter would suffices, I don't think it implies that the module are incorrect - they are correct but my module's voice should be stronger - thus I alter them.

I like the idea of passing on return statements by module, this would also help e.g. devel module to provide some access-introspection for fields.

Since the alter hook in this case would be very rarely needed, it can also be a real drupal_alter() passed a $context.

Agreed

Looks great. Thanks.

Thanks klausi, the new patch looks great! I agree this is RTBC.

I'd have one suggestion but this can happen as a follow-up, such it does not hold on things:

+++ b/core/lib/Drupal/Core/Entity/Field/Type/Field.php
@@ -289,6 +289,48 @@ public function __clone() {
+    // Invoke hook and collect grants/denies for field access. Our default
+    // access flag is masked under the umbrella of the system module.

This should be documented for the alter hook.

+++ b/core/includes/entity.api.phpundefined
@@ -512,3 +512,49 @@ function hook_entity_field_info_alter(&$info, $entity_type) {
+/**
+ * Alters the default access behaviour for a given field.
+ *
+ * @param array $grants
+ *   An array of grants gathered by hook_entity_field_access(). The array is
+ *   keyed by the module that defines the field's access control; the values are
+ *   grant responses for each module (Boolean or NULL).
+ * @param array $context
+ *   Context array on the performed operation with the following keys:
+ *   - operation: The operation to be performed (string).
+ *   - field: The entity field object (\Drupal\Core\Entity\Field\Type\Field).
+ *   - account: The user account to check access for
+ *     (Drupal\user\Plugin\Core\Entity\User).
+ */
+function hook_entity_field_access_alter(array &$grants, array $context) {
+  $field = $context['field'];
+  if ($field->getName() == 'field_of_interest' && $grants['node'] === FALSE) {
+    // Override node module's restriction to no opinion.
+    $grants['node'] = NULL;
+  }

Why would you do this and not just have your own access method returning true?

+++ b/core/lib/Drupal/Core/Entity/Field/Type/Field.phpundefined
@@ -289,6 +289,48 @@ public function __clone() {
+    // Invoke hook and collect grants/denies for field access. Our default
+    // access flag is masked under the umbrella of the system module.
+    $grants = array('system' => $access);
+    foreach (module_implements('entity_field_access') as $module) {
+      $grants = array_merge($grants, array($module => module_invoke($module, 'entity_field_access', $operation, $this, $account)));

I don't see any reason to add a default access flag under system module. Either entity module could implement the hook, or make the array key 'default'. Referencing system module from the class isn't something we do anywhere else (except where core functionality has been baked into system module for years and never factored out).

+++ b/core/lib/Drupal/Core/Entity/Field/Type/Field.phpundefined
@@ -289,6 +289,48 @@ public function __clone() {
+    // All grants are NULL and have no opinion - deny access in that case.
+    return FALSE;
+  }
+
+  /**
+   * Contains the default access logic of this field.
+   *
+   * See \Drupal\Core\TypedData\AccessibleInterface::access() for the parameter
+   * doucmentation.
+   *
+   * @return bool
+   *   TRUE if access to this field is allowed per default, FALSE otherwise.
+   */
+  public function defaultAccess($operation = 'view', User $account = NULL) {
+    // Grant access per default.
+    return TRUE;

The only way you can get to all grants being NULL, is if someone alters the defaultAccess? Why not actually default to TRUE and skip having the default grant at all.

If there are good reasons for this that were discussed in the issue, they should at a minimum be added to the comments.

"default" is not a valid module name and could conflict with contrib. We could use "drupal", but I thought a real module name makes more sense.

Maybe we could just use an invalid-module name like ":default" ?

Yep, we want the possibility to define custom default access in the providing class - so it's properly encapsulated.

An invalid module name makes sense, but I would personally prefer to use an underscore, like Field API does for option widgets (e.g. "_none"); so "_default". I realize that this is not really invalid, but let's be serious.. who's gonna name a project this way?

thanks klausi, patch looks good. I think this is read to fly, thus setting back to RTBC. We can still bikeshed on the best ":default" key in a follow-up.

Agreed with RTBC.

Hmm OK. :default is a lot better. Still not sure about the _alter() but it's not the first and won't be the last alter we have that looks a bit superfluous, I'll probably end up implementing that in 18 months...

Committed/pushed to 8.x, will need a change notice.

Sorry rolled this back.

Drupal core - 8.x fail: http://qa.drupal.org/8.x-status

Overall Summary
==============================================

FAILED: [[SimpleTest]]: [MySQL] 51,327 pass(es), 12 fail(s), and 20 exception(s).

Individual Environment Summaries
==============================================

-- [[SimpleTest]]: [MySQL] --

* Drupal\block\Tests\Views\DisplayBlockTest (23 pass(es), 1 fail(s), and 0 exception(s))

* Drupal\system\Tests\Entity\FieldAccessTest (2 pass(es), 0 fail(s), and 1 exception(s))

#48: field-ng-access-1883152-48.patch queued for re-testing.

I was going to report the test breakage catch reported in #54 — glad to see #56 is already fixing that — impressive speed! :)

hm, this is quite a work-a-round to do hack around with the global user. Shouldn't we just add the necessary schema for loading a user instead?

Ok, but the comment says it's just about loading. Still, if the global user is not set yet and we have to set it, why do we have to re-store the active id then?

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/FieldAccessTest.php
@@ -15,11 +15,19 @@
+   * Holds the currently active global user ID that gets replaced during the
+   * test.
+   *
+   * @var int

Summary should not exceed 80chars + it's "integer" I think.

typo: hould

Use Use drupal_container()->get('module_handler')->getImplementations($hook). instead of module_implements() as per notice on http://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/functio...

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/FieldAccessTest.php
@@ -43,20 +43,10 @@ protected function setUp() {
+    // The users table is needed for creating dummy user accounts.
+    $this->installSchema('user', array('users'));

You also need $this->installSchema('system', 'sequences'); for that. But you'd get this for free if you extend EntityUnitTestBase, including a nice helper method for creating the user.

Yeah, testing with a user-entity is indeed better. So this is ready and now has unit-test cases! -> Back to RTBC.

#65: field-ng-access-1883152-65.patch queued for re-testing.

Committed/pushed to 8.x, thanks!