Entity Access does not check host entity

Hmm, this direction feels kinda wrong to me.

Suddenly the target entity needs to know about the embedding context and even persist it.
As a result, you need to fight already against serialization recursion problems..

I thought we should resolve the permission problem on the field level (by the entity reference revisions field / the paragraphs formatter) and suppress items top down. What is the problem that requires the embedded entity itself to be aware of the parent context?

Hm, i think the problem originates in the fact that Drupal does not offer anything to deal with embedding / recursion.

One assumption we introduced was that direct access is not happening ever. To make sure this applies, we did not offer a link template to edit the view and entity directly.

I don't know what we can do with the private file problem using the core methods.
Should we try to report usage against the host entity?

I will ping Berdir for some better advice about this challenging situation.

Hm, don't have much to add.

Usually, we say that it is not your responsibility to check access of your parent, e.g. it is wrong for field level access checks to check the entity too.

But yes, the private file use case is valid, we have to do something about that. The usage approach might work, we've discussed that before, also with drupal-media/media_entity, which has similar requirements.

Reviewed parts of the patch:

1. +++ b/src/Entity/Paragraph.php
   @@ -303,4 +333,105 @@ class Paragraph extends ContentEntityBase implements ParagraphInterface {
   +            // Little trick to get the revision id.
   +            $query = \Drupal::entityQueryAggregate($host_entity_type)
   +              ->groupby($revision_id_key)
   +              ->condition($host_entity_field_name . '.target_id', $this->id())
   +              ->condition($host_entity_field_name . '.target_revision_id', $this->getRevisionId())
   +              ->allRevisions()
   +              ->range(0, 1);
   +
   +            $host_ids = $query->execute();
   +            if (count($host_ids)) {
   +              $host_id = reset($host_ids);
   

   the revision id is always the array key of the result. I don't think you really want allRevisions() here, you care about the current revision that's using oyur paragraph, no?

2. +++ b/src/ParagraphAccessControlHandler.php
   @@ -25,16 +26,87 @@ class ParagraphAccessControlHandler extends EntityAccessControlHandler
   +    // If parent says forbidden, don't even try anything else.
   +    if (parent::checkAccess($entity, $operation, $account)->isForbidden()) {
   +      return AccessResult::forbidden()->cacheUntilEntityChanges($entity);
   +    }
   +
   +    // If parent says allowed, don't even try anything else.
   +    if (parent::checkAccess($entity, $operation, $account)->isAllowed()) {
   +      return AccessResult::allowed()->cacheUntilEntityChanges($entity);
   +    }
   +
   

   The thing with access result objects is that you should keep using them, not re-create them.

   So I think you want something like

   $result = parent:...
   if (!$result->isNeutral()) {
     return $result->addCacheableDependency($entity);
   

   (cacheUntilEntityCHanges is deprecated).

3. +++ b/src/ParagraphAccessControlHandler.php
   @@ -25,16 +26,87 @@ class ParagraphAccessControlHandler extends EntityAccessControlHandler
   +      // Check access on host entity.
   +      if ($host_entity->access($check_parent_operation, $account)) {
   +        return AccessResult::allowed()->cacheUntilEntityChanges($entity)->cacheUntilEntityChanges($host_entity);
   +      } else {
   +        return AccessResult::forbidden()->cacheUntilEntityChanges($entity)->cacheUntilEntityChanges($host_entity);
   +      }
   

   Same here, you want to use the existing access object, maybe that varies by user or by permission (or moon phase ;)). Just do return $host_entity->access($check_parent_operation, $account, TRUE)->addCacheableDependency($entity);

   I don't think you need a dependency on the host entity, if it depends on a status of that (e.g. published or not), then the host entity access control handler is responsible for adding that.

OK, found time to discuss the situation.

We decided that the proper approach for paragraphs is to declare a paragraph as an embedded entity by concept.
This means that the entity host is immutable and due to the field API design it's needed to resolve the parent relationship from the item. And the access check by paragraphs is needed and thus (with knowing that there is only one parent) simple.

We still want to see integrity management delegated as much as possible to entity reference revisions:
ERR introduces the concept of a composite entity. It will support parent_type/parent_id keys in the annotation of the target entity that define the parent entity type and id field name. ERR will then make sure that the IDs are persisted on creation, and also will delete child items.

This means that a paragraph item is NOT reusable (thus has no label, no management metadata). If ever someone wants to do this, he will add a module with a new / altered widget that could still add an operation "make this paragraph item reusable" that leads to creation of a separate (reusable, value cloned) entity with own metadata and switching the paragraphs bundle to a bundle with a reference to the reusable item. Integrity (deleting the reference) is still well done by ERR... And all of the complexity of a mixed case disappears.

So let's postpone this issue for proper ERR support #2641824: Maintain composite relationship.
And promote to release critical for the security implications of potential information disclosure.

It seems the Composite relationship will help with this.

Discussed with @Berdir , added host check, the patch includes test coverage. Also I am working on top of the issue #2676190: Make paragraphs use ERR composite relationship since that will make the check easier.

Seems to be working locally, maybe the tests will pass after have committed the other issue (?)

Sorry, I should also have uploaded the non combined patch. Here we go.

+++ b/src/ParagraphAccessControlHandler.php
@@ -22,9 +22,15 @@ class ParagraphAccessControlHandler extends EntityAccessControlHandler {
+  protected function checkAccess(EntityInterface $paragraph, $operation, AccountInterface $account) {
...
+    if (!$paragraph->isNew()) {
+      $parent_access = $paragraph->getParentEntity()->access($operation, $account, TRUE);

There is a moment where a paragraph is stored and not yet attached to the parent entity. Thus getParentEntity() will not return the parent.

Your code would fail hard here.

Since this situation is special, it contains risk for many other applications to break things. We should well document it and test cover it.
Actually this situation is initiated by the ERR module and not paragraphs and needs documentation there if not already done.

Uploaded combined and non combined patches, add a check for the parent and also extended tests for preview mode.

Lets see why the combined patch is not working.

Made small changes, added interdiff against patch of comment 23 to make the review easier.

+++ b/src/Tests/ParagraphsAccessTest.php
@@ -0,0 +1,132 @@
+    $image_url = file_create_url('private://privateImage.jpg');

Debugging it I get http://d8.dev/system/files/privateImage.jpg, but form raw data I get /system/files/styles/medium/private/privateImage.jpg?itok=<em>something</em>, so maybe you should hardcode it. Not sure if this could work though.
(I had a similar problem here #2671046: Paragraphs Type Permission should take the publish/unpublish status into account and I've just hardcoded it).

Changed the test, this should work, also again diff against patch #23

Added interdiff against #35

I don't agree that changing that $image_url position could change anything for the test, since you are creating that file URL hardcoding the URI...
Also, please write "\Drupal\paragraphs\Tests\ParagraphsAccessTest"! ;)

Changes made based on comment #55 also changed a bit the generation of the url, let's see what testbot thinks about this.

For the non-combined patch, you haven't defined 'parent_type' and 'parent_id' anywhere, that is why all the tests fail... Added these data, works locally, let's see if there are any other bug for testbot.
Later we will debug deeper the combined patch.

The non-combined patch isn't meant to work, it's just for reviewing. those fields are added in the other issue.

When you have something like that, I suggest you disable testing for those.

Thanks for the feedback, after checking the test fail it seems it is not working, it still finds the image, not sure how to proceed.

Need help for this.

Debugging some I think I found the problem, should be in the access check.

+++ b/src/ParagraphAccessControlHandler.php
@@ -22,9 +22,15 @@ class ParagraphAccessControlHandler extends EntityAccessControlHandler {
+    /** @var \Drupal\paragraphs\Entity\Paragraph $paragraph */
+    if ($paragraph->getParentEntity() != NULL) {
+      $parent_access = $paragraph->getParentEntity()->access($operation, $account, TRUE);
+      return AccessResult::allowedIf($operation != 'view' || $paragraph->status->value)
+        ->andIf($parent_access);
+    }
+    return AccessResult::allowedIf($operation != 'view' || $paragraph->status->value);

Will test more and upload a new patch soon.

It seems there would be a caching problem in core. Added a second image inside the same paragraph, the access check should be fine. Let's see again what test bot thinks about this.

After so many ideas, changes and patches 'the idea' came to us, it seems testbot is using rc4 of ERR which does not implement composite relationship yet. That is the reason why the tests are failing.

Still applies, retesting. :-)

Ah, we need the second patch.

Another christmas present today. Committing!