Updated: Comment #10

Problem/Motivation

In a D7 module, I could define a permission with hook_permission() and then I could define access on menu items etc. with a simple user_access('permission_name') or in some cases, just putting something like access => 'permission_name' in a hook_menu() array item.

Now we come to D8. I'm looking at this nice page on how to create a config entity:
https://drupal.org/node/1809494

So let's say I have a 'administer foo_bars' permission, and I want to say that to do any operations on this config entity, you have to have this permission. Which would be the normal thing for config entities, I think.

In order to do this, it appears that I have to create a whole EntityAccess class just to override the checkAccess() method and tell it to return user_access('permission_name').

It seems like it would be a whole lot easier if I could just put this permission name in my annotation, and the default class for entity access would take over and do this for me? This doesn't seem like it would be too hard to do.

Proposed resolution

  • Create a simple access controller for entities.
  • Make it so that if you add annotation to the entity type giving an administrative permission name, the default access controller will do a simple user_access('the permission you gave') to check permissions for all operations.

Remaining tasks

  • Decide on the annotation key. It should probably be something like admin_permission, since it will be similar to "administer nodes" in its effect (allow full CRUD).
  • Add the new annotation key to EntityType and document it.
  • Create the simple access controller class.
  • Refactor existing entity types to use the new system.

Assuming the last step (refactoring) is done, we probably do not need to have any explicit tests added for this, because many existing tests will implicitly be using the system.

User interface changes

None.

API changes

We'll have a simpler way to create entity types, and fewer access classes needed, especially for config entity types.

#2085429: Provide a simple list controller for config entities
#2089757: Auto generate routing entries for entities based on an 'admin_path' and 'admin_permission' annotation

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Title: D8Dx: Why should I need a whole access class just to do a permission? » D8Dx: Why should I need a whole access class just to do a simple permission check?
Issue tags: +DX (Developer Experience)

forgot tag

dawehner’s picture

Status: Active » Needs review
FileSize
10.26 KB

In general the entity access controller is just partly about routing but determine access in many more places like for example checking the access when you render a single entity in a sidebar.

This patch adds a new functionality to the base entity access controller and converts some of the existing entities to it.

Status: Needs review » Needs work

The last submitted patch, entity_access-2105557-2.patch, failed testing.

larowlan’s picture

larowlan’s picture

There is a base access controller that uses a permission annotation key in that issue

Berdir’s picture

Component: documentation » entity system

@larowlan: True, but I think this deserves it's own, separate issue.

+1 on the concept for adding such a class, not sure if we want to support only a single permission or multiple ones. I'm fine with this getting in for just single-permission use cases, but we might want to think about naming it in a way that does't get in the way for adding more. admin_permission might be fine as it indicates that it allows everything. We could then add a view_permission easily that only allows view access.

Also note that quite a few config entities have a few special cases, like not allowing some operations under some conditions. We might be able to standardize more there, e.g. not allow to edit/delete locked entities once we have a common concept for them.

larowlan’s picture

Lets make sure we keep this consistent with #2089757: Auto generate routing entries for entities based on an 'admin_path' and 'admin_permission' annotation as both need the concept of an admin/access permission.

Berdir’s picture

Title: D8Dx: Why should I need a whole access class just to do a simple permission check? » Add an admin_permission to EntityType annotations to provide simple default access handling

Changing the title to something that explains what we want to do.

Berdir’s picture

Issue tags: +Entity Field API

Moar tags.

jhodgdon’s picture

I love the concept of #2, with the caveat of #6. I will update/create the issue summary accordingly, since it sounds like we're probably all in agreement about this (at least for now; the beauty of issue summaries is that they can be edited). :)

Berdir’s picture

Status: Needs work » Needs review
FileSize
9.67 KB
13.46 KB

Re-roll, inject the entity info instead of calling out to the entity manager, no idea why we're not doing this yet but do it for createInstance.

This also fixes the create implementation that referenced a non existing $entity object.

webchick’s picture

Priority: Normal » Major

Yay, didn't see this before I filed #2107137: Fix the DX for declaring custom access checkers, which has an example (with its 24 lines of code, 4 "use" statements, 1 class, and 1 method with 4 totally obscure arguments) of why this should be major.

Berdir’s picture

Status: Needs review » Needs work

The last submitted patch, entity_access-2105557-13.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.3 KB
28.56 KB

Language and View access controllers used the wrong method.

Status: Needs review » Needs work

The last submitted patch, entity_access-2105557-14.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.17 KB
28.75 KB

Some small details have been missing.

damiankloip’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/EntityAccessController.php
    @@ -44,9 +51,12 @@ class EntityAccessController implements EntityAccessControllerInterface {
    +  public function __construct($entity_type, array $entity_info) {
    

    Nice, we want to be doing this everywhere anyway, atm we are doing it half of the time.

  2. +++ b/core/modules/language/lib/Drupal/language/LanguageAccessController.php
    @@ -17,21 +17,14 @@ class LanguageAccessController extends EntityAccessController {
    +        return !$entity->locked && parent::checkAccess($entity, $operation, $langcode, $account);
    

    I guess language entity doesn;t have a locked() method. That would be nice IMO. Obv. nothing to do with this issue.

Basically I think this patch looks great already. Let's just see how the bot gets on.

Berdir’s picture

As discussed, needs documentation of the new annotation key and specifying the default isn't necessary, that's in EntityType.

@#18-2: There's an issue to introduce an interface for the concept of locked entities but the problem is that locked means different things. some can be edited but not deleted, others can't be edited or deleted, ... but make something the default and those that differ need to check it themself

jhodgdon’s picture

Yeah for all the

--- a/core/modules/action/lib/Drupal/action/ActionAccessController.php
+++ /dev/null

blocks in this patch!!! :)

I have a couple of questions about this patch:

a) In some of the entity classes' annotation sections, for types that I think are using the new default controller, we have:

+++ b/core/modules/image/lib/Drupal/image/Entity/ImageStyle.php
@@ -33,8 +33,8 @@
  *     },
  *     "storage" = "Drupal\Core\Config\Entity\ConfigStorageController",
  *     "list" = "Drupal\image\ImageStyleListController",
- *     "access" = "Drupal\image\ImageStyleAccessController"
  *   },
+ *   admin_permission = "administer image styles",

(the access controller is being removed from the list of controllers)

And in some of them we have:

--- a/core/modules/block/custom_block/lib/Drupal/custom_block/Entity/CustomBlockType.php
+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Entity/CustomBlockType.php
@@ -22,7 +22,7 @@
  *   module = "custom_block",
  *   controllers = {
  *     "storage" = "Drupal\Core\Config\Entity\ConfigStorageController",
- *     "access" = "Drupal\custom_block\CustomBlockTypeAccessController",
+ *     "access" = "Drupal\Core\Entity\EntityAccessController",
  *     "form" = {
  *       "default" = "Drupal\custom_block\CustomBlockTypeFormController",
  *       "add" = "Drupal\custom_block\CustomBlockTypeFormController",
@@ -31,6 +31,7 @@
  *     },
  *     "list" = "Drupal\custom_block\CustomBlockTypeListController"
  *   },
+ *   admin_permission = "administer blocks",

(the controller that was there before is replaced by the default EntityAccessController).

This seems inconsistent. Any reason for it? Do classes that want to use the default access controller need to declare this in their annotation or not?

b) As mentioned above... we need to add the 'admin_permission' item to \Drupal\Core\Entity\Annotation\EntityType. I suggest:

  /**
   * The name of the default administrative permission.
   *
   * The default \Drupal\Core\Entity\EntityAccessController class checks this
   * permission for all operations in its checkAccess() method. Entities with
   * more complex permissions can extend this class to do their own access
   * checks.
   *
   * @var string (optional)
   */
   public $admin_permission;
dawehner’s picture

Yeah this was my fault ... the default value is stored in the annotation so it does not have to be set.

damiankloip’s picture

(the access controller is being removed from the list of controllers)

The default will always be Drupal\Core\Entity\EntityAccessController. So I think in reply to the consistency question, they should all be removed if they use the default.. :)

damiankloip’s picture

Oops, x-posted. Already taken care of!

jhodgdon’s picture

This looks good to me! I used grep and verified that with the latest patch, nothing is declaring 'access' to be EntityAccessController in its annotation. Also I verified that the default access controller was already documented in the EntityType annotation class.

+1 for RTBC assuming that the tests all pass again, and I expect they will.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

I love it!

webchick’s picture

33 files changed, 68 insertions, 274 deletions.

<3 <3 <3 :D

This looks effing awesome!!

Will let it sit overnight just in case someone else has anything to say about it.

catch’s picture

Title: Add an admin_permission to EntityType annotations to provide simple default access handling » Change notice: Add an admin_permission to EntityType annotations to provide simple default access handling
Category: bug » task
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Looks great. Committed/pushed to 8.x!

Needs a change notice.

dawehner’s picture

Status: Active » Needs review
jhodgdon’s picture

That looks pretty good dawehner... I think it would be even clearer if you showed the EntityType annotation in the "with controller" situation too (showing the controller annotation)?

tim.plunkett’s picture

Title: Change notice: Add an admin_permission to EntityType annotations to provide simple default access handling » Add an admin_permission to EntityType annotations to provide simple default access handling
Category: task » bug
Status: Needs review » Fixed
Issue tags: -Needs change record
jhodgdon’s picture

Thanks!

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

Anonymous’s picture

Issue summary: View changes

create summary

arnoldbird’s picture

Issue summary: View changes

This is an old issue, but does anyone know if the admin_permission key is documented?

It is not documented here: https://www.drupal.org/docs/8/api/entity-api/structure-of-an-entity-anno...
Nor here: https://www.drupal.org/docs/8/api/entity-api/creating-a-content-entity-t...

At...
https://www.drupal.org/docs/8/api/configuration-api/creating-a-configura...
...I see this language...

The admin_permission key automatically allows all access for users with that permission. In case more logic is required, a custom access controller can be specified.

...but what does "access" mean in this context? I'm finding that if a user does not have the perm that's set in the admin_permissions key, they can't view the admin listing of that type of entity even if they have view permissions. However, they can still add and edit entities if they have add/edit perms, even if they don't have the perm in the admin_permissions key. So the admin_permissions key seems to prevent users from accessing the admin listing, but does not necessarily affect add/edit operations.

I see that in some cases, the admin_permissions key affects whether an item appears in a menu or not. Should that be? I suppose in some cases Drupal will supply a link in a menu if the user can access the listing, but removes the link when they can't. So it may be by some indirect mechanism that admin_permissions affects menu links.

In the above discussion, it looks like there was talk of making admin_permission an optional key, but it appears to be required at this point. If I remove it from any of my entity definitions and clear cache, I get...

<em class="placeholder">InvalidArgumentException</em>: Routing requirement for &quot;_permission&quot; must be a string.

I find myself wishing there were one fewer way to control access, and the appearance of menu items, in Drupal. There are so many permutations, it seems like it would be difficult to debug if there is a problem.

Thank you.

Berdir’s picture

The admin_permission feature is only implemented inside the entity access control handler. It allows access to anything that is controlled by access control handlers: the create, view, update, delete operations.

There is currently no general concept a list/overview access operation, which means it has nothing to do with route access to a entity list except that both cases can use the same permission string but it can also be something entirely different.

I think there is an issue somewhere to have a more generic handling of list permissions but it's tricky because just like create, it's not tied to a specific entity, so it would need a new, separate API/method and it's not so easy to add something completely new to the existing access control handlers.

Currently it's up to each entity type to implement something useful, e.g. node and now also terms have specific permissions that allow overview access that doesn't imply full update/delete access.

arnoldbird’s picture

@Berdir

Thank you for the pointers. I created a new issue to work out some documentation for this key: https://www.drupal.org/project/drupal/issues/2938889

I will record my findings there.

arnoldbird’s picture

@Berdir

The admin_permission feature is only implemented inside the entity access control handler.

I'm not sure this is true. Note that the class EntityType has a method, getAdminPermission, that returns the value of the admin_permission key. And when I generated a module and entity with Drupal console, the sample code includes a call to getAdminPermission. It doesn't appear that this getAdminPermission method is used often, but it's just a matter of time before one module developer assumes that they can call this method on another module developer's entity and get a predictable result.

It allows access to anything that is controlled by access control handlers: the create, view, update, delete operations.

That's not true, I'm afraid. It allows access to whatever path is set in the collection key. It has no impact on CRUD operations.

@jhodgdon

So let's say I have a 'administer foo_bars' permission, and I want to say that to do any operations on this config entity, you have to have this permission. Which would be the normal thing for config entities, I think.

This is not how the admin_permission key works, as of now. So it appears the feature that was developed does not address jodgdon's original issue. The admin_permission key does not control access to CRUD operations -- only to the collection path.

In order to do this, it appears that I have to create a whole EntityAccess class just to override the checkAccess() method and tell it to return user_access('permission_name').

Creating a new EntityAccess class is, sorry to say, an easier work flow than what we now have, I believe.

It seems like it would be a whole lot easier if I could just put this permission name in my annotation, and the default class for entity access would take over and do this for me? This doesn't seem like it would be too hard to do."

I'm 4 years late commenting on this, but I think the admin_permission annotation makes development more difficult. Development is always more difficult when there are more places in code and UI to control access. An ideal system has as few places as possible to control access.

At the moment, the best solution for working around the admin_permission key is to misuse the collection key, if I'm not mistaken. But I have not yet found the place in the core where the admin_permission key impacts access to the collection path. The best solution may end up being to set the collection key to the path that serves as the landing page for the entity's admin functions (e.g. misuse the key) or to set a dummy path for the collection to try to ensure that neither the admin_permission key nor the collection key will have any impact on my application now or in the future.

I'm sure once I get clarity on all this, it won't seem as complicated.

arnoldbird’s picture

Berdir’s picture

Please move that comment to your new issue. It doesn't make sense discuss this in an issue that was closed 4 years ago. The only reason I saw it is because I'm relying a lot on e-mail notifications.

arnoldbird’s picture

Thanks @Berdir. Going forward I'll only comment in the new issue.