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 config entities based on an 'admin_path' and 'admin_permission' annotation

Files: 
CommentFileSizeAuthor
#21 entity_access-2105557-21.patch29.39 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,798 pass(es).
[ View ]
#21 interdiff.txt2.77 KBdawehner
#17 entity_access-2105557-17.patch28.75 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,651 pass(es).
[ View ]
#17 interdiff.txt3.17 KBdawehner
#15 entity_access-2105557-14.patch28.56 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,724 pass(es), 105 fail(s), and 9 exception(s).
[ View ]
#15 entity_access-2105557-14-interdiff.txt2.3 KBBerdir
#13 entity_access-2105557-13.patch27.39 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 57,906 pass(es), 645 fail(s), and 530 exception(s).
[ View ]
#13 entity_access-2105557-13-interdiff.txt14.56 KBBerdir
#11 entity_access-2105557-11.patch13.46 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,097 pass(es), 553 fail(s), and 521 exception(s).
[ View ]
#11 entity_access-2105557-11-interdiff.txt9.67 KBBerdir
#2 entity_access-2105557-2.patch10.26 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 54,620 pass(es), 1,369 fail(s), and 940 exception(s).
[ View ]

Comments

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

Status:Active» Needs review
StatusFileSize
new10.26 KB
FAILED: [[SimpleTest]]: [MySQL] 54,620 pass(es), 1,369 fail(s), and 940 exception(s).
[ View ]

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.

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

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.

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

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.

Issue tags:+Entity Field API

Moar tags.

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). :)

Status:Needs work» Needs review
StatusFileSize
new9.67 KB
new13.46 KB
FAILED: [[SimpleTest]]: [MySQL] 58,097 pass(es), 553 fail(s), and 521 exception(s).
[ View ]

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.

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.

StatusFileSize
new14.56 KB
new27.39 KB
FAILED: [[SimpleTest]]: [MySQL] 57,906 pass(es), 645 fail(s), and 530 exception(s).
[ View ]

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new2.3 KB
new28.56 KB
FAILED: [[SimpleTest]]: [MySQL] 58,724 pass(es), 105 fail(s), and 9 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new3.17 KB
new28.75 KB
PASSED: [[SimpleTest]]: [MySQL] 58,651 pass(es).
[ View ]

Some small details have been missing.

  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.

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

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;

StatusFileSize
new2.77 KB
new29.39 KB
PASSED: [[SimpleTest]]: [MySQL] 58,798 pass(es).
[ View ]

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

(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.. :)

Oops, x-posted. Already taken care of!

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.

Status:Needs review» Reviewed & tested by the community

I love it!

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.

Title:Add an admin_permission to EntityType annotations to provide simple default access handlingChange 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.

Status:Active» Needs review

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)?

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

Thanks!

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

Issue summary:View changes

create summary