Fall back for entities that don't explicitly define an access callback

Looks good.

OT: Can we please keep 7.x code in a dedicated branch and avoid the CVS HEAD confusion? Will also help for the git migration: #953916: Proposal: Ditch the ability to have release nodes pointing to HEAD/master

>OT: Also, can we please please please develop in HEAD? (and drop DRUPAL-7--1)
I dislike developing in HEAD. It's not obvious to which release-branch the code belongs nor is the drupal major-version information visible.

@issue:
Is there a real use-case of having access == TRUE? If so, I guess you should still should have to specify that explicitly to prevent people going with that accidentally.
In order to just help people getting started with the code, I suppose we should additionally print an error message or better throw an exception.

Not that for the rules integration, it's optional:

  $result = entity_access('view', $entity_type);
  // If no access callback is given, just grant access for viewing.
  return isset($result) ? $result : TRUE;

>@fago: Yes, I think there is a use-case of "agile development". When Relation module attempted to implement relations as entities, we did not want to care for entity access at all.

Makes sense. You don't have to care, but of course then some stuff has to be disabled. Just granting access might be unexpected and thus lead to security vulnerabilities, thus is a no-go. Entity-access is not UI specific. Still any module relying on entity-access can react on not-existing access-information as suiting, just as shown above for the Rules integration. But modules need to be able to rely on access == TRUE, to be really TRUE.

In order to ease coming up with the UI, we could do an entity_ui_access() wrapper returning TRUE and showing a notice telling the developer access information is missing?

I also ran into this trouble, as entityreference module makes use of this function, while entity API documentation states that "access callback" is optional and provides an entity_access() API callback that counters that. At least documentation should get fixed.

We ran into this a similar issue while implementing Entity Services API - https://drupal.org/project/issues/services_entity

To give the whole picture, we're in the middle of developping an android app that imports data from Drupal. A clean way to do this, seemed to be the Services module - https://drupal.org/project/services . Since we wanted to expose some contrib and some custom entities, we've also implemented Entity Services API

This module provides support for all Entity API entity types to Services. All entity types get a standard resource, similar to what Services provides for code entity types such as nodes.

The problem is, that we could only fetch core entities and after a discussion #2114011: Services Entity API not working with non-core entities we found that this was because both the contrib and our custom entities didn't provide an access callback function.

Adding this to our custom modules isn't a problem, but for the contrib modules it will be. Therefor I think it will be best to have some sort of system in Entity API that provides an access callback fall back in case there is no explicit defined one. sun's patch seems like a good starting point.

The most recent patch includes a lot of unrelated changes. The only relevant part is this and the docs changes:

+  // If the entity does not implement an access callback, then access is
+  // unrestricted.
+  return TRUE;

This would be a big change for existing modules -- one that would have security implications, as sites installing the update might suddenly find that access to their entities is suddenly wide open.

I have to disagree with you, the patch states this:

function entity_access($op, $entity_type, $entity = NULL, $account = NULL) {
  $info = entity_get_info($entity_type);
  if (isset($info['access callback'])) {
    return $info['access callback']($op, $entity, $account, $entity_type);
  }
  // If the entity does not implement an access callback, then access is
  // unrestricted.
  return TRUE;
}

So not all entities are open, but only the ones that have no callback defined. Agreed, that might cause problems too, but then can't we implement a simple check, like whether or not the user is authenticated or something?

I'm in support of that. Currently, to use entity_access() properly you have to do something like:

if (entity_access("xxxx", $entity) !== FALSE) {
  // ...
}

Which looks very stupid. Even Entity Reference doesn't get that right yet.

> So not all entities are open, but only the ones that have no callback defined

Yes, that's what I meant. Sorry for not being clearer.

The situation would be:

1. site developer either installs contrib module or writes custom module which has an entity type and no entity_access.
2. site dev updates Entity API
3. boom! all previously inaccessible entities are now accessible

Admittedly, if you were using Entity API's admin UI you had to have the access callback. If you're using Services Entity, likewise. I am just concerned that people may have made their own systems that expose entities that will now be granting access.

I don't think there's much we can do about this in the code -- but this would need a change record and a prominent mention in the release notes.

Oke, I understand that leads to complications.

So I went investigating the possibilities of some sort of entity exposure wrapper, using hook_entity_info_alter() to provide entities with an access callback in case they miss one. Then I could perform the actual check in the custom module allowing lots of UI settings if desired. But there I discovered your module differs from the Drupal (menu) access callback. (I believe I've seen it in other places as well.)

In Drupal menu access callback I can, according to https://api.drupal.org/api/drupal/includes%21menu.inc/function/_menu_che..., enter a boolean like this:

function hook_menu() {
  $items['example'] = array(
    'title' => 'Example Page',
    'access callback' => TRUE,
    ...
  );
  ...
}

The entity_access function always expects an access callback function, disallowing me to do the access check on another level and altering the boolean value in.

Could you perhaps update the entity_access function to also accept booleans (perhaps in a similar way as the menu access check)?
I believe that won't cause any security vulnerabilities, but it should help me (and hopefully others) to expose custom modules. Also it would improve consistency between this module and the core.

Wait, that probably won't work either since the entity info goes into static cache...

The menu access callback and the entity access callback are very different things. I don't think it's feasibly or desirable to have them work more similarly: they do different things at different times. Menu access deals with paths, so lets you specify your parameters so you can match up with your path. For entity access, you always have an entity, a type, an operation, and a user account.

I think whatever we do here, there is going to be a change in access for entities. There's no way round that. Before, an entity type had no access callback and so entity_access() blocked access to it. Now, it will grant it.

I'm also not sure how much use it would be to provide a standard access model for entities, as there are lots of different requirements for different types of entity. At the very least, you'd want more than one system, to provide for different levels of complexity.

For example, in my own project, I have some entities that just expose view/create/edit/delete permissions, some that do that but per-bundle, and some that do that but via their own hook_ENTITY_TYPE_access() so that other modules can act too (so that I can use custom entities with OG).

My module https://drupal.org/project/entity_operations goes some way towards that in that it defines the permissions for a custom entity type, but it doesn't handle the access.

Here's a reroll.

While I agree with sun's improvements to the wording of the docs for the entity_access() params, I don't feel those fixes are relevant to this issue.

The 5 year old patch in #15 to entity.api.php and entity.module applied cleanly to the latest entity 7.x-1.x-dev, but still needs review.