entity bundle is not passed to entity_access() for 'create' op

I'll take a look at this when I get a chance.

I can verify this as well. I can create with user1 just fine. Normal users can create in web interface, but not with services.

My un-educated guess as to the why:
I've noticed that when creating nodes with clean controller and user1, the author is not set (ie, the node says the author is anon). I have to specify the author, this seems like a bug. If I create nodes with normal services, the user is set to the logged in user (this is the correct way I think).

My guess is this has something to do w/ it. :)

Oh, I should note, this is with "session authentication". I don't know if this is the case w/ oauth or basic http....

Ahh, I think I figured this out. This is because entity_access does not provide a generic bundle create op access check.

I swiped some code from http://drupal.org/node/1856978#comment-6956976 and made a patch. Works for clean controller, it should work w/ the "dirty" controller as well (if one is clean... isn't the other dirty :P ) but I didn't test it.

I don't think that making a special case of node is a good thing here.

However, we could do what I already do in Entity Operations: overload entity_access() to take a bundle name in the 'create' op, same as node_access().

> entity_access($op, $entity_type, $entity = NULL, $account = NULL)

That way, if your custom entity's access callback knows what to do with that, it can handle it. If it doesn't, it should have no business looking at the $entity parameter in a 'create' op anyway :) Provided we document the trick we're pulling, of course...

I'm not sure I get how that works, maybe its too late :).

Heh, I know the feeling :)

entity_access() just hands over to your entity type's 'access callback'.

I don't know what ECK does for that, but if it's a custom entity, you can do what you want. Currently, on a 'create' op, you get a NULL for $entity. So access callbacks will currently ignore that parameter on $op 'create'.

What I'm suggesting is that we extend this by overloading the $entity parameter, the same way that node_access() overloads the $node param.

Then your own access callback can take this into account.

I see your point. Maybe multiple and single need to be separated. Use case:

load all a webforms submissions
loop through the submissions to get the webform_data type
send an email to someone using token data from the submissions.

Alright, this is the way I got it to work. See my attached patch.

Yup, that's along the lines of that I meant.

Though --

+++ b/plugins/services_entity_resource.inc
@@ -15,8 +15,12 @@ class ServicesEntityResourceController implements ServicesResourceControllerInte
+      //type must be supplied or we can't check for access
+      if(!isset($args[1]['type'])){
+        services_error('Entity "type" is required', 406);
+      }

That's a bit harsh! Not all entity types will be using this in their access callback. Least of all the ones with no bundles!

I had a similar problem, I was using Services Entity API with the Entity Registration module.

When trying to create a registration, I got an error that the access callback of the registration entity needs a full entity object.

This problem is actually solved with the simple path I attached.
With this patch the bundle gets also passed to the access callback.

I just realized that my patch doesn't work properly, sorry.

Made a small change, now it works.

> I got an error that the access callback of the registration entity needs a full entity object.

That's non-standard. It's useful, but not how other Entity API modules do it. Look at profile2, for instance.

Looking at https://drupal.org/node/1780646 it seems like the approach in #14 is being taken by entity_api, so perhaps it is becoming the new standard? The patch candidate is in #136 of that thread, or: https://drupal.org/files/entity-entity_access-1780646-136.patch

> Looking at https://drupal.org/node/1780646 it seems like the approach in #14 is being taken by entity_api, so perhaps it is becoming the new standard?

That would definitely be a good thing -- would solve a lot of problems!

I suppose adding the parameter to entity_access() won't have any effect on entity types whose access callback doesn't support it, so it's fine to do it.

I think it would be a good idea to add a comment to the call to entity_access() though, to explain what we're doing and why.

Also, here's another small tweak that should be made:

+++ b/plugins/services_entity_resource.inc
@@ -15,8 +15,9 @@ class ServicesEntityResourceController implements ServicesResourceControllerInte
       list($entity_type) = $args;
+      $entity = entity_create($entity_type, $args[1]);

Instead of $args[1], $entity should be picked out with the list() operator.

Made those changes to #14.

Realized that this requires slightly different handling for the clean controller.

This issue is quite similar to another one, isn't it? I've been getting them confused :/

Isn't there a thing on Entity module to now pass a full entity to entity_access() even on create? How would that change affect this?

Yes the Entity API issue is #1780646: entity_access() fails to check node type specific create access. The latest patch candidate there (#191) passes a full entity on create, and that's the approach taken here as well. I suppose we could wait for that to settle, though it hasn't received much maintainer input lately. But without this fix, it's not possible to create nodes without 'bypass node access' permissions. This fix works even without the Entity API patch, but it generates notices.

1. +++ b/plugins/services_entity_resource.inc
   @@ -14,9 +14,11 @@ class ServicesEntityResourceController implements ServicesResourceControllerInte
   +      // Create an entity from the data and pass this to entity_access. This allows us
   
   +++ b/plugins/services_entity_resource_clean.inc
   @@ -11,16 +11,23 @@
   +      // Create a new wrapper from the entity so we can call its access() method.
   

   These comments lines need wrapping.

2. +++ b/plugins/services_entity_resource_clean.inc
   @@ -216,4 +223,28 @@ class ServicesEntityResourceControllerClean extends ServicesEntityResourceContro
   +  protected function createWrapperFromValues($entity_type, array &$values) {
   

   Is it because of the property names the clean controller uses that we need this? As in, instead of 'field_foo', our incoming data is just 'foo'?

   I do think the clean controller is more trouble than it's worth... and the removal of 'field' I think is just asking for a clash.

More critically, I think we have to postpone on #1780646: entity_access() fails to check node type specific create access (and a release of Entity API with that change).

Otherwise, the node access callback that Entity API adds will crash with our change, AFAICT. We'll be passing in a $node to entity_access(), and the following code will try to load the $node we've passed in and fail:

/**
 * Access callback for the node entity.
 *
 * This function does not implement hook_node_access(), thus it may not be
 * called entity_metadata_node_access().
 */
function entity_metadata_no_hook_node_access($op, $node = NULL, $account = NULL) {
  if (isset($node)) {
    // If a non-default revision is given, incorporate revision access.
    $default_revision = node_load($node->nid);
    if ($node->vid != $default_revision->vid) {
      return _node_revision_access($node, $op);
    }
    else {
      return node_access($op, $node, $account);
    }
  }

I disagree about the clean controller. I think it takes better advantage of the entity metadata, both for access and validation checks, and as a general description of the properties of an entity. Though I do agree that stripping the "field_" is a nicety that isn't worth it. I'd be more than happy to remove that, but it might affect others using the controller.

Specifically, transform_values() also takes care of decoding "resource references" (the way the clean controller handles referenced entities).

Regarding postponement, the patch actually works with the current version of entity api -- it just throws a few notices. Eventually by the time our skeleton node gets passed to node_node_access(), we have:

/**
 * Implements hook_node_access().
 */
function node_node_access($node, $op, $account) {
  $type = is_string($node) ? $node : $node->type;
  ...

If you still want to postpone, that's fine - but as it is it's not possible to create a node with the clean controller unless you have "bypass node access" permission, which is not a happy state of affairs.

> Though I do agree that stripping the "field_" is a nicety that isn't worth it. I'd be more than happy to remove that, but it might affect others using the controller.

Well we're still in alpha, so we'd be fine to make that sort of change. I'll open a new issue for that.

> If you still want to postpone, that's fine - but as it is it's not possible to create a node with the clean controller unless you have "bypass node access" permission, which is not a happy state of affairs.

Ah, I didn't realize that. Sorry, I keep flitting in and out of this issue whenever I have spare time :(

Would it be possible to commit this patch in two halves:

1. Fix the clean controller so it works
2. When the Entity API patch lands and is released, commit the change to the regular controller

Actually, I think it will fail in the regular controller as well. See the attached test.

OK how about this workaround - a special case for nodes until the Entity API bug is fixed.

#26: 1865102-26-services_entity-access-bundle.patch queued for re-testing.

Automated tests are broken -- see #2109167: Testbot not downloading all dependencies.

#26: 1865102-26-services_entity-access-bundle.patch queued for re-testing.

Rerolled to apply to HEAD.

Is there anything still holding this up?

@joachim - would like to commit this - let me know if you have any objections.

Just a few minor things:

1. +++ b/plugins/services_entity_resource.inc
   @@ -14,9 +14,16 @@ class ServicesEntityResourceController implements ServicesResourceControllerInte
   +      // Create an entity from the data and pass this to entity_access. This
   

   entity_access needs a () on it.

2. +++ b/plugins/services_entity_resource.inc
   @@ -14,9 +14,16 @@ class ServicesEntityResourceController implements ServicesResourceControllerInte
   +      // allows us to check per-bundle creation rights (e.g. in node_access).
   

   This case doesn't cover node_access() due to the workaround above.

3. +++ b/plugins/services_entity_resource_clean.inc
   @@ -11,16 +11,28 @@
   +  public function access($op, $args) {
   +    if ($op == 'create') {
   

   Can you add a comment to explain why 'create' gets special handling?

4. +++ b/plugins/services_entity_resource_clean.inc
   @@ -249,4 +261,28 @@ class ServicesEntityResourceControllerClean extends ServicesEntityResourceContro
   -}
   \ No newline at end of file
   

   Missing terminal newline.

http://drupalcode.org/project/services_entity.git/commit/038618c