Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

fubhy’s picture

Assigned: Unassigned » fubhy

Thanks for opening this. Writing a patch now.

fubhy’s picture

Assigned: fubhy » Unassigned
Status: Active » Needs review
FileSize
14.28 KB

I started playing around with this a bit... I was a bit sloppy with writing in-line documentation for the code. I am posting this anyways to get some early reviews as I am a bit unsure about the approach. The create access part makes it kinda ugly if you ask me. The node_access() function is currently a mixture of single and "any" entity access (you don't have to pass a node object). Let's see if this even works...

Status: Needs review » Needs work

The last submitted patch, 1862750-3.patch, failed testing.

fubhy’s picture

Status: Needs work » Needs review
FileSize
14.35 KB

Trying to kill some of the notices.

Status: Needs review » Needs work

The last submitted patch, 1862750-5.patch, failed testing.

Berdir’s picture

+++ b/core/modules/node/lib/Drupal/node/NodeAccessController.phpundefined
@@ -0,0 +1,227 @@
+  /**
+   * Implements EntityAccessControllerInterface::createAccess().
+   */
+  public function createAccess(EntityInterface $node, $langcode = LANGUAGE_DEFAULT, User $account = NULL) {
+    if (($access = $this->getCached($node, 'create', $langcode, $account)) !== NULL) {
+      return $access;
+    }
+
+    if (($access = $this->access($node, 'create', $langcode, $account)) !== NULL) {
+      return $access;
+    }
+
+    $this->setCached(FALSE, $node, 'create', $langcode, $account);
+    return FALSE;

I guess it makes sense to change createAccess() to not get an entity object at all? will require a separate way to acess it, but you never have one if you ask if someone can create one?

dawehner’s picture

Status: Needs work » Needs review
FileSize
882 bytes
14.4 KB

I guess it makes sense to change createAccess() to not get an entity object at all? will require a separate way to acess it, but you never have one if you ask if someone can create one?

Mh, so it would make at least sense to get the bundle as you may want to have create access per bundle.

This small fix should fix a lot of the exceptions.

Status: Needs review » Needs work

The last submitted patch, drupal-1862750-8.patch, failed testing.

Wim Leers’s picture

Issue tags: +in-place editing, +Spark

Tagging as per #1.

fubhy’s picture

Assigned: Unassigned » fubhy

Will take over again from here.

I guess it makes sense to change createAccess() to not get an entity object at all? will require a separate way to acess it, but you never have one if you ask if someone can create one?

Well... It may look weird when you look at it like that, yes... But then again, how can we put createAccess in-line with the rest if not like this? Also, who will make a judgement on what createAccess() needs and what not? So, bundle is pretty obvious then... But in some use-cases we also need the author UID? We can't cover every single possible attribute that might be relevant for checking for access like that. Hence why we went with a full entity object. I don't see any other way for providing us with a good, generic solution (without requiring custom entry points!) for solving this in a way that gives us enough flexibility.

fubhy’s picture

Status: Needs work » Needs review
FileSize
15.31 KB
dawehner’s picture

Reviewed the code under the aspect of ignoring documentation problems.

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
@@ -45,4 +50,41 @@ public function deleteAccess(EntityInterface $entity, $langcode = LANGUAGE_DEFAU
+  protected function cacheId(EntityInterface $entity) {

what about cacheID or better entityCacheID?

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
@@ -45,4 +50,41 @@ public function deleteAccess(EntityInterface $entity, $langcode = LANGUAGE_DEFAU
+  protected function getCached(EntityInterface $entity, $operation, $langcode = LANGUAGE_DEFAULT, User $account = NULL) {

Should we support a way to truncate this cache?

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
@@ -45,4 +50,41 @@ public function deleteAccess(EntityInterface $entity, $langcode = LANGUAGE_DEFAU
+    $account = $account ?: entity_create('user', (array) $GLOBALS['user']);

Let's try to avoid ?: in such relative often used functions.

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
@@ -45,4 +50,41 @@ public function deleteAccess(EntityInterface $entity, $langcode = LANGUAGE_DEFAU
+    if (isset($this->accessCache[$account->id()][$cid][$langcode][$operation])) {
+      return $this->accessCache[$account->id()][$cid][$langcode][$operation];

Should we call $account->id() twice, but $this->cacheId ... not?

+++ b/core/modules/node/lib/Drupal/node/NodeAccessController.phpundefined
@@ -0,0 +1,180 @@
+    if ($status) {
+      return $this->setCached(TRUE, $node, 'view', $langcode, $account);
+    }
...
+    return $this->setCached(FALSE, $node, 'view', $langcode, $account);

What about $this->setCached($status, ...) instead?

+++ b/core/modules/node/lib/Drupal/node/NodeAccessController.phpundefined
@@ -0,0 +1,180 @@
+  public function updateAccess(EntityInterface $node, $langcode = LANGUAGE_DEFAULT, User $account = NULL) {
...
+  public function deleteAccess(EntityInterface $node, $langcode = LANGUAGE_DEFAULT, User $account = NULL) {

These two blocks of code really look similar, maybe we should try to unify them.

+++ b/core/modules/node/lib/Drupal/node/NodeAccessController.phpundefined
@@ -0,0 +1,180 @@
+  protected function grants(EntityInterface $node, $operation, $langcode = LANGUAGE_DEFAULT, User $account = NULL) {

We certainly need explainations in code itself, as that's not a simple logic.

Status: Needs review » Needs work

The last submitted patch, 1862750-12.patch, failed testing.

fubhy’s picture

Reviewed the code under the aspect of ignoring documentation problems.
We certainly need explainations in code itself, as that's not a simple logic.

Yes, totally... I am still playing around with it because I am moving a lot of stuff, splitting it up, unifying it again, etc. ...

Let's try to avoid ?: in such relative often used functions.

I plan to add @todos there all over the place anyways because this whole area of issues (anything that touches the global user basically) gets ugly due to these not-yet-fixed issues:

#1825332: Introduce an AccountInterface to represent the current user
#1549526: Change global $user into $session
#361471: Global $user object should be a complete entity
#1277682: Move responsibility for global $user out of session

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.97 KB
15.42 KB

Replaced those entity_create() calls with a user_load() as entity_create() blows up if global user is a loaded user entity.

In the other issues, we tried to avoid the User/stdClass problem, we might be able to do that here too by postponing the type hint on the internal helper methods until that is cleaned up.

Just want it to get green first, will look at the review later.

dawehner’s picture

For sure some documentation is needed, so this is just some small feedback.

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
@@ -45,4 +52,41 @@ public function deleteAccess(EntityInterface $entity, $langcode = LANGUAGE_DEFAU
+    $account = $account ?: entity_create('user', (array) $GLOBALS['user']);
...
+    $account = $account ?: entity_create('user', (array) $GLOBALS['user']);

Shouldn't these calls then also be changed?

+++ b/core/modules/node/lib/Drupal/node/NodeAccessController.phpundefined
@@ -0,0 +1,180 @@
+      $query = db_select('node_access');

What are the plans to make entity controllers injectable? Seems to be a bit related to the discussion about making plugins injectable.

+++ b/core/modules/node/node.moduleundefined
@@ -2584,113 +2584,31 @@ function node_form_system_themes_admin_form_submit($form, &$form_state) {
+  if (!$node instanceof Node) {

Maybe parenthesis would help the readability a bit.

+++ b/core/modules/node/node.moduleundefined
@@ -2584,113 +2584,31 @@ function node_form_system_themes_admin_form_submit($form, &$form_state) {
+    $type = is_object($node) ? $node->type : $node;

Should bundle() be used here?

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
@@ -45,4 +52,41 @@ public function deleteAccess(EntityInterface $entity, $langcode = LANGUAGE_DEFAU
+    return $entity->isNew() ? $entity->bundle() : $entity->id();

It's kind of odd, as this could cause problems when you have two node add forms on the same page, but hey, this has been the case before as well.

fubhy’s picture

Thanks for reviving this issue @Berdir.

What are the plans to make entity controllers injectable? Seems to be a bit related to the discussion about making plugins injectable.

Yes, we still need to solve that problem. @tim.plunkett already improved the way we are building and retrieving controllers but ultimately we need to find a fully injectable solution. It's funny that you mention that today because I chatted about that with @fago yesterday on the subway :). However, this discussion is going to be slightly different than the one we are currently having in the #1863816: Allow plugins to have services injected because the requirements are different. So, in case of Entities, we build plugin definitions based on the annotations of the entity classes which also contain the definitions for the controllers used by that entity type. However, we probably don't want to inject those into the entity objects, instead we probably want to inject the entity manager into the entity objects (which is kinda weird because that basically means we are injecting the plugin manager into our plugin instances) which in return will manage the controllers. So either the entity manager is the factory for our controllers directly or we move that responsibility to another layer (some kind of proper controller factory) possibly even per controller type... Anyways, this is the wrong place for that discussion.

For sure some documentation is needed, so this is just some small feedback.

I am going to clean this patch up now and add the required documentation.

Berdir’s picture

As discussed with @davereid in IRC, once this is done, we should look at the different access controllers and try to come up with a sane default implementation for entities. Mostly, that will mean moving currently node-specific features into a base implementation I guess. For example, changing hook_node_access() to hook_entity_access() + hook_$entity_access(), like all other $entity hooks now are.

For simple cases, that could even mean a definition of the related permissions in the entity info (e.g. view_access => 'access content') so that cases that are as trivial as VocabularyAccessController don't need need a custom implementation for it.

fago’s picture

+++ b/core/modules/node/lib/Drupal/node/NodeAccessController.php
@@ -53,7 +53,7 @@ public function viewAccess(EntityInterface $node, $langcode = LANGUAGE_DEFAULT,
-    $account = $account ?: entity_create('user', (array) $GLOBALS['user']);

hm, this mis-uses entity_create as factory - but it isn't. I think we should stop doing so and start introducing one.

For now, we could add a interim helper function doing it without entity_create() and add a todo to fix it there?

As discussed with @davereid in IRC, once this is done, we should look at the different access controllers and try to come up with a sane default implementation for entities. Mostly, that will mean moving currently node-specific features into a base implementation I guess. For example, changing hook_node_access() to hook_entity_access() + hook_$entity_access(), like all other $entity hooks now are.

+1. It would be nice if we could have the access controller provide the permission also, such that it's self-contained - defines and checks for the permissions. We'd have to invoke the controllers from the hook somewhere I guess....

fubhy’s picture

Assigned: fubhy » Unassigned
FileSize
19.26 KB

Starting to go for a default EntityAccessController implementation. Invoking a hook there. 100% untested therefore probably dark red, but who knows?

Wim Leers’s picture

#21: :D

Status: Needs review » Needs work

The last submitted patch, 1862750-21.patch, failed testing.

fubhy’s picture

Status: Needs work » Needs review

Next try. There were some minor but obvious hiccups...

Also... We need a better solution for the static caching... What about using a proper static cache bin? We need a way to invalidate cached access checks.

fubhy’s picture

FileSize
19.19 KB

Actually uploading the patch is always a good idea.

Status: Needs review » Needs work

The last submitted patch, 1862750-24.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
652 bytes
19.65 KB

You nicely tricked yourself out ;) Type safe comparison but still return the old constant values doesn't match well...

Status: Needs review » Needs work

The last submitted patch, node-access-controller-1862750-27.patch, failed testing.

amateescu’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.php
@@ -10,39 +10,183 @@
+  protected function cacheId(EntityInterface $entity, $operation, $langcode = LANGUAGE_DEFAULT, User $account = NULL) {
+    // In some cases (e.g. 'create' access checks) the entity has not been saved
+    // yet and therefore does not have an id in which case we use the bundle as
+    // identifier for the cache entry.
+    return $entity->isNew() ? $entity->bundle() : $entity->id();
+  }

This is crazy, why not use the UUID which is always available?

Berdir’s picture

Status: Needs work » Needs review
FileSize
21.06 KB

Changes:

- Re-rolled for NodeNG. Some parts currently look a bit ugly but that's temporary.
- Added resetCache() to the access controller interfaces and implemented it.
- Fixed the translation test failure, that was because the permissions were checked in the wrong order, FALSE wins over TRUE.

No useful interdiff due to the re-roll.

Status: Needs review » Needs work

The last submitted patch, node-access-controller-1862750-30.patch, failed testing.

jthorson’s picture

From testbot apache error logs:

PHP Fatal error: Class Drupal\\user\\UserAccessController contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (Drupal\\Core\\Entity\\EntityAccessControllerInterface::resetCache) in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/user/lib/Drupal/user/UserAccessController.php on line 74

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.67 KB
23.72 KB

Thanks, I thought it would be something simple as that, some access controllers didn't extend from EntityAccessController which makes sense given that it previously didn't do anything.

Status: Needs review » Needs work

The last submitted patch, node-access-controller-1862750-33.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.67 KB
23.77 KB

This should be green.

Berdir’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
@@ -10,39 +10,190 @@
+  protected function access(EntityInterface $entity, $operation, $langcode = LANGUAGE_DEFAULT, User $account = NULL) {
+    $account = $account ?: user_load($GLOBALS['user']->uid);

I'm not sure if I like the current process, we have separate methods on the interface (which are now almost identical) only to redirect it to the same one again? Would it maybe not make sense to invert that somehow?

+++ b/core/modules/node/node.moduleundefined
@@ -56,7 +57,7 @@
-const NODE_ACCESS_ALLOW = 'allow';
+const NODE_ACCESS_ALLOW = TRUE;

@@ -64,7 +65,7 @@
-const NODE_ACCESS_DENY = 'deny';
+const NODE_ACCESS_DENY = FALSE;

Why not make those entity constants? They were introduced in 7.x I think to make it easier to distinguish between TRUE/FALSE/NULL return values because implementations often incorrectly returned FALSE when they should have returned NULL.

fubhy’s picture

Status: Needs review » Reviewed & tested by the community

I'm not sure if I like the current process, we have separate methods on the interface (which are now almost identical) only to redirect it to the same one again? Would it maybe not make sense to invert that somehow?

Yes, I found that particularly tedious too with this patch and some other that I looked at. I will open a follow-up for refactoring the interface. I guess we should simply go with forwarding the ->access() invocations from Entity::access() and TranslatableInterface::access() etc. to EntityAccessController::access() and do the mapping there whenever necessary (if necessary).

Why not make those entity constants? They were introduced in 7.x I think to make it easier to distinguish between TRUE/FALSE/NULL return values because implementations often incorrectly returned FALSE when they should have returned NULL.

Also a task for a follow-up. And yes, we should consider making that a re-usable, entity-generic constant somewhere. +1 on that because I can see how it improves DX. However, that's also a follow-up task.

RTBCing this patch here. Will open the follow-ups asap.

fubhy’s picture

Assigned: Unassigned » fubhy
Status: Reviewed & tested by the community » Needs work

Per discussion on IRC we put UUIDs into the mix here as suggested by @amateescu

fubhy’s picture

FileSize
22.54 KB
3.25 KB
fubhy’s picture

Assigned: fubhy » Unassigned
Status: Needs work » Needs review

Sorry for the spam.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Thank you! Looks much better now :)

fubhy’s picture

I set up two follow-up issues following a quick chat on IRC with @berdir:

#1947892: Improve DX with EntityAccessControllerInterface
#1947880: Replace node_access() by $entity->access()

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work

Thanks for pushing this forward :)

Few nitpicks, one WTF, and a few things that appear to be wrong (despite passing tests).


+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
@@ -10,39 +10,164 @@
+   *   TRUE if access was granted, FALSE if access was denied and NULL if access
+   *   could not be reliably determined.

"not reliably"? I think it's simply "not at all"? :)

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
@@ -10,39 +10,164 @@
+    // @todo Remove this once we can rely on $account.

Link to follow-up issue? Also: shouldn't this @todo also exist elsewhere in the code?

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
@@ -10,39 +10,164 @@
+    // @todo Remove this once we can rely on $account.

Idem.

+++ b/core/modules/node/lib/Drupal/node/NodeAccessController.phpundefined
@@ -0,0 +1,133 @@
+ * Form controller for the node edit forms.

Copy/paste leftover.

+++ b/core/modules/node/lib/Drupal/node/NodeAccessController.phpundefined
@@ -0,0 +1,133 @@
+    $status = (bool) $node instanceof EntityNG ? $node->status : $node->get('status', $langcode)->value;

Shouldn't this be the other way around? Currently if it's an EntityNG node, then it used $node->status, whereas it should be the other way around?

+++ b/core/modules/node/lib/Drupal/node/NodeAccessController.phpundefined
@@ -0,0 +1,133 @@
+    $node_status = isset($node->status) ? $node->status : NULL;
+    $node_uid = isset($node->uid) ? $node->uid : NULL;

Why have "node" in the variable names?

+++ b/core/modules/node/lib/Drupal/node/NodeAccessController.phpundefined
@@ -0,0 +1,133 @@
+      $account = $account ?: user_load($GLOBALS['user']->uid);

WTF?

+++ b/core/modules/node/lib/Drupal/node/NodeAccessController.phpundefined
@@ -0,0 +1,133 @@
+    $status = $node instanceof EntityNG ? $node->status : $node->get('status', $langcode)->value;

As before.

fubhy’s picture

Assigned: Unassigned » fubhy

Link to follow-up issue? Also: shouldn't this @todo also exist elsewhere in the code?

Yes, that @todo should be in about 500 places in core :(.

WTF?

That's related to that :(

Link to follow-up issue? Also: shouldn't this @todo also exist elsewhere in the code?

The Symfony Session issue & a few more that basically circle around the topic of global $user / $session, etc.
#1858196: [meta] Leverage Symfony Session components
#335411: Switch to Symfony2-based session handling
#1549526: Change global $user into $session
#1825332: Introduce an AccountInterface to represent the current user

etc. ... There are many issues that discuss that problem.

Why have "node" in the variable names?

Probably copy&paste from NodeNG conversion :)

fubhy’s picture

Status: Needs work » Needs review
FileSize
4.42 KB
22.75 KB
Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC. Thanks :)

Berdir’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
@@ -86,10 +86,13 @@ public function deleteAccess(EntityInterface $entity, $langcode = LANGUAGE_DEFAU
-   *   could not be reliably determined.
+   *   could not be determined.

I think this description was copied from the interface, where it probably states the same. But that's already in and we can improve that in the follow-up issue.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

This looks good. Committed to 8.x. Thanks.

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

Anonymous’s picture

Issue summary: View changes

Adding follow-ups.