Auto create CTools access plugins from entity-metadata based on CRUD permissions

Makes sense to me.

Rules features an API for re-using single conditions as well as condition sets, including a way to embed the UI. So if desired, making use of Rules conditions in panels shouldn't be hard, however we'd need to manage to get rules variables from of the ctools context.

Anyway, doing the "compatibility" module sounds like a good start.

There's a token context in CTools if that helps. :)

@Itangalo,
Checkout #1138708: Auto create "content-type" CTools plugins from tokens -- it's a start in this direction of creating a content-type plugin for Entity API's metadata. I think It's very similar to the way we can do access plugins.

Moving this issue to Entity API:

Patch auto creates access plugins from metadata properties that define 'access callback'.

#1138708: Auto create "content-type" CTools plugins from tokens is now committed :). This means all tokens, including the ones declared by entity-metadata are exposed as CTools content-type plugins.

So now this patch will complete it by adding the CTools access plugin.

Sounds good + patch looks basically good.

However, there is another way to specify access information - via the 'setter callback' property. Then, it is assumed the access information is complete, thus if there is no access metadata the API will grant you access to a property based on entity level access. So we could just go and all entity properties.

>+ * Settings form for the 'by entity_bundle' access plugin
I think this comment needs to be updated.

>+ 'title' => t("Entity: metadata"),
>+ 'description' => t('Control access by entity-metadata.'),

I think this description is just going to confuse people. Perhaps we just say "Entity property" and "Control access by entity property" ?

Beside that, I'm not sure about adding CTools integration to the entity API as I'm just slowly diving into ctools, so I'd probably be a bad maintainer for that.

> However, there is another way to specify access information - via the 'setter callback' property

Is there an example for this in entity API?

uhm, sry I meant 'setter permission'. E.g. see the node author property.

Finally got back to it.

+++ b/entity.module
@@ -1208,3 +1208,12 @@ function _entity_info_add_metadata_callbacks(&$entity_info) {
+  if ($module == 'ctools' && !empty($plugin)) {
+    return "plugins/$plugin";
+  }

Is plugins the default name for that? Else, I'd prefer "ctools" as plugins isn't really connected to ctools as term in general.

+++ b/plugins/access/entity.inc
@@ -0,0 +1,92 @@
+  'description' => t('Control access by entity property.'),

I'm not sure this is clear to everyone.

+++ b/plugins/access/entity.inc
@@ -0,0 +1,92 @@
+  $info = entity_get_property_info();
+

Let's also cover bundle-properties? -> use entity_get_all_property_info()

+++ b/plugins/access/entity.inc
@@ -0,0 +1,92 @@
+  if (!$wrapper) {
+    return;
+  }

We should also check whether the property still exists, i.e. use isset($wrapper->property).

+++ b/plugins/access/entity.inc
@@ -0,0 +1,92 @@
+  if (!$wrapper->{$name}->access('view')) {
+    return;
+  }

Let's catch metadata wrapper exceptions caused by that and return FALSE in case of an exception. That would cover the not existing property case too.

+++ b/plugins/access/entity.inc
@@ -0,0 +1,92 @@
+ * Provide a summary description based upon the checked entity_bundle.

bundle?

+++ b/plugins/access/entity.inc
@@ -0,0 +1,92 @@
+  return t('"@s" @name', array('@s' => $context->identifier, '@name' => $plugin['title']));

What is @s supposed to be?

> Is plugins the default name for that?

Yes, this is the convention.

> Let's catch metadata wrapper exceptions caused by that and return FALSE in case of an exception. That would cover the not existing property case too.

Isn't this redundant if we already check before isset($wrapper->property)?

> What is @s supposed to be?

This @s is copied from CTools plugins. The output is in the attached image.

+++ b/entity.module
@@ -1212,3 +1212,12 @@ function _entity_info_add_metadata_callbacks(&$entity_info) {
+  if ($module == 'ctools' && !empty($plugin)) {
+    return "plugins/$plugin";

Still, let's go with "ctools/$plugin". That makes it clear to which module the stuff belongs.

+++ b/includes/entity.property.inc
@@ -44,9 +44,9 @@ function entity_get_property_info($entity_type = NULL) {
- * In contrast to entity_get_property_info(), this function returns info about
- * all properties the entity might have, thus it adds an all properties assigned
- * to entity bundles.
+ * In contrast to entity_get_property_info(), this function returns info ¶
+ * about all properties the entity might have, thus it adds an all ¶
+ * properties assigned to entity bundles.

Why are you adding trailing whitespaces here? But indeed "an all" is wrong, I guess it should be "in all"

+++ b/plugins/access/entity.inc
@@ -0,0 +1,91 @@
+// $Id$

deprecated

+++ b/plugins/access/entity.inc
@@ -0,0 +1,91 @@
+  'title' => t("Entity property"),

Maybe we should call "Entity property access" ?

+++ b/plugins/access/entity.inc
@@ -0,0 +1,91 @@
+function entity_entity_metadata_ctools_access_get_child($plugin, $parent, $child) {

Uhm, again that ugly "entity_entity" prefix. y that?
Also we should implement this function without relying on multiple as this is performance relevant. (Yes, I got some ctools experience.. :D)

+++ b/plugins/access/entity.inc
@@ -0,0 +1,91 @@
+  }
+  ¶
+  // Access check on the property.

Whitespace in empty line.

Isn't this redundant if we already check before isset($wrapper->property)?

Indeed. As we've fixed entity loading now to not throw exceptions, it's fine now without catching it.

+++ b/plugins/access/entity.inc
@@ -0,0 +1,91 @@
+  if (!$wrapper->{$property}->access('view')) {
+    return FALSE;
+  }
+
+  return TRUE;

Why not just return it?

This @s is copied from CTools plugins. The output is in the attached image.

Uhm, that's an ugly, undescriptive replacement. Still, let's go with it so any existing translation for it applies here too.

re white-spaces, Oops, sorry - Aptana doesn't have the trim whitespace feature, and I keep forgetting that :/

> Also we should implement this function without relying on multiple as this is performance relevant. (Yes, I got some ctools experience.. :D)

Can you explain what you mean, as we are defining multiple access under a single access plugin.

Attached patch with other fixes.

ctools uses the 'get child' method to get the plugin-child-info when executing it at run-time. Thus for performance it's not good to load all possible childrens there (via multiple) but only the necessary on. I guess that's the cause for its existence.

It is sometimes necessary to load all, but yes, whenever possible, 'get child' should be implemented correctly to avoid loading them all.

Justed tested #1261044: Add a direct support of entity in ctools page manager - for that having an access plugin for entity_access() would be nice. I guess thats more often needed than property based access? Let's just add it too.

entity_metadata_ctools_access_get_child() now extracts the entity_type and property name from the $child.
Also fixed access callback to return the expected result ;)

Slightly better patch -- using the limit option in explode()

--- /dev/null
+++ b/ctools/access/entity.inc

I'd expect entity_access() integration from a plugin "access/entity.inc". Maybe better call it "entity_property.inc"? So we can use "entity.inc" for entity_access() integration.

It doesn't make much sense to me (from a user perspective) to have rather sophisticated entity-property-access integration, but no simple entity-access integration. So let's add an entity.inc plugin at the same time - that shouldn't be hard to do.

+++ b/ctools/access/entity.inc
@@ -0,0 +1,96 @@
+function entity_metadata_ctools_access_get_child($plugin, $parent, $child) {

Let's just use the entity_ctools namespace, the entity_metadata namespace is pretty much deprecated.

+++ b/ctools/access/entity.inc
@@ -0,0 +1,96 @@
+        $plugins[$parent. ':' . $entity_type . '__' . $name] = $plugin;

Above you are using only one underscore as separator. Also, why not just use a colon as separator again?

+++ b/ctools/access/entity.inc
@@ -0,0 +1,96 @@
+  // This is needed to allow other settings to pass.

hm, which other settings?

• Patch addresses #22.
• Instead of only checking "view" access we now allow selecting view or edit.
• Added entity.inc for entity level check.

I think we are getting closer ;)

Indeed :)

+++ b/ctools/access/entity.inc
@@ -0,0 +1,103 @@
+  // Get all the entity types that have metadata properties.
+  $entity_types = array_keys(entity_get_property_info());

We should just use all entity-types that have an access callback here, see entity_type_supports().

+++ b/ctools/access/entity.inc
@@ -0,0 +1,103 @@
+      'view' => t('View'),
+      'edit' => t('Edit'),

entity_access has more/different operations.

+++ b/ctools/access/entity.inc
@@ -0,0 +1,103 @@
+ * Check for access based on access to entity property.

based on entity_access

+++ b/ctools/access/entity.inc
@@ -0,0 +1,103 @@
+  $entity = clone $context->data;

Why do we need that clone?

+++ b/ctools/access/entity_property.inc
@@ -0,0 +1,120 @@
+  $entity = clone $context->data;

Again, why clone?

> Why do we need that clone?

This is something I've seen in Ctools. I guess it fits some plugins where we might change the entity itself, and we don't want to manipulate the original entity. Anyway, removed.

Other things fixed as-well.

bump :)

Oh, sry for letting this lie so long.

+++ b/ctools/access/entity.inc
@@ -0,0 +1,104 @@
+  $wrapper = entity_metadata_wrapper($entity_type, $entity);
+  // Access check on the entity.
+  return $wrapper->access($conf['operation']);

Let's skip the wrapper here and just use entity_access() as the wrapper isn't really used.

Then, this just checks access for the global user. Shouldn't we allow passing in a user just like the node_access plugin?

+++ b/ctools/access/entity_property.inc
@@ -0,0 +1,120 @@
+  $plugin['description'] = t('Control access of @label property.', array('@label' => $property['label']));

We are not controlling access *of* the property. I'd say "control access by property"? Still, we should clarify it with an example.

+++ b/ctools/access/entity_property.inc
@@ -0,0 +1,120 @@
+  $plugin['required context'] = new ctools_context_required(t(ucfirst($entity_type)), 'entity:' . $entity_type);

Shouldn't we use the entity type label here?

+++ b/ctools/access/entity_property.inc
@@ -0,0 +1,120 @@
+    '#description' => t('The operation (view or edit) to be checked against the property.'),

I don't think we have to list the options again in the description.

Fixed #28

> + $plugin['required context'] = new ctools_context_required(t(ucfirst($entity_type)), 'entity:' . $entity_type);

btw, let it be noted I really hate passing those variables inside a t() :)

+++ b/ctools/access/entity.incundefined
@@ -0,0 +1,102 @@
+function entity_ctools_entity_access_get_child($plugin, $parent, $child) {
+  $plugin_name = $parent. ':' . $child;
+  $plugin = array_merge($plugin, _entity_ctools_entity_prepare_plugin($child, $plugin_name));
+  return $plugin;
+}
+
+function entity_ctools_entity_access_get_children($plugin, $parent) {
+  $plugins = array();
+  foreach (array_keys(entity_get_info()) as $entity_type) {
+    if (entity_type_supports($entity_type, 'access')) {
+      $plugin_name = $parent. ':' . $entity_type;
+      $plugin = _entity_ctools_entity_prepare_plugin($entity_type, $plugin_name);
+      $plugins[$parent. ':' . $entity_type] = $plugin;
+    }
+  }
+
+  return $plugins;

Those functions are missing documentation blocks.

+++ b/ctools/access/entity_property.incundefined
@@ -0,0 +1,126 @@
+/**
+ * Plugin declaration.
+ */

Plugin declaration for what?

+++ b/ctools/access/entity_property.incundefined
@@ -0,0 +1,126 @@
+function entity_ctools_entity_property_access_get_child($plugin, $parent, $child) {
+  list($entity_type, $property_name) = explode(':', $child, 2);
+  $properties = entity_get_all_property_info($entity_type);
+  $property = $properties[$property_name];
+
+  $plugin_name = $parent. ':' . $child;
+  $plugin = array_merge($plugin, _entity_ctools_entity_property_prepare_plugin($entity_type, $property, $property_name, $plugin_name));
+  return $plugin;
+}
+
+function entity_ctools_entity_property_access_get_children($plugin, $parent) {
+  // Get all the entity types that have metadata properties.
+  $entity_types = array_keys(entity_get_property_info());
+
+  $plugins = array();
+  foreach ($entity_types as $entity_type) {
+    // Get all the properties of the entity type.
+    foreach (entity_get_all_property_info($entity_type) as $property_name => $property) {
+      if (!empty($property['access callback']) || !empty($property['setter permission'])) {
+        $plugin_name = $parent. ':' . $entity_type . ':' . $property_name;
+        $plugin = _entity_ctools_entity_property_prepare_plugin($entity_type, $property, $property_name, $plugin_name);
+        // Separate the entity type from the property name.
+        $plugins[$parent. ':' . $entity_type . ':' . $property_name] = $plugin;
+      }
+    }
+  }
+
+  return $plugins;
...
+/**
+ * Form callback.

Same here.

+++ b/ctools/access/entity_property.incundefined
@@ -0,0 +1,126 @@
+/**
+ * Form callback.
+ */

Form callback for what?

In general the code looks okay to me but it lacks documentation and the existing documentation could be improved (a lot of @param and @return declarations are missing). Just a minor additional note: I like to document functions in a "What does it do?" manner instead of a "Do this!" manner (if you know what I mean). That means, instead of having a doc block that reads "Check for access based on access to entity property." I would probably phrase it like this: "Callback function for checking access to the entity based on access to an entity property." (same content but still slightly different). I often use comments like the one that you used there to describe single lines of code though.

According to the coding standards comment should start with a verb ala:

Checks access based on access to an entity property.

> + $plugin['required context'] = new ctools_context_required(t(ucfirst($entity_type)), 'entity:' . $entity_type);

Yep, that's really ugly. I'd say it shuold use the entity type label, but let's follow CTools "style" here in order to go with existing UI.

Last comment 3 years ago...will this issue be completed? Interesting with regard to Rules Panels integration.

Tested #29, it does what it announces. More docstrings (while formally making sense) will not make the code more readable, so setting RTBC. YMMV.

Sry for letting this lie so long. As this still needed / wanted functionality? Seems there were not many people interested into it during the last year? But if so, I'd be happy to add it.

Some quick remarks:

+++ b/ctools/access/entity.inc
@@ -0,0 +1,102 @@
+function entity_ctools_entity_access_get_child($plugin, $parent, $child) {
...
+function entity_ctools_entity_access_get_children($plugin, $parent) {

+++ b/ctools/access/entity_property.inc
@@ -0,0 +1,126 @@
+function entity_ctools_entity_property_access_get_child($plugin, $parent, $child) {
...
+function entity_ctools_entity_property_access_get_children($plugin, $parent) {

Missing function docblocks.

The patch in #29 no longer applied, so here's a reroll. I looked at the code to get a sense of what entity_ctools_entity_property_access_get_child() does, but I'm not entirely sure. As I've more sites to update because of the new Drupal core security release, I'll only made some code style fixes that were obvious enough for me. For the rest I leave the patch as is for now.