Add: "View any unpublished [entity_type]" permission

Also in need of this. Is this patch suitable for prod?

Hey together

In my opinion the patch is not doing the right thing. If the user has the permission "View any unpublished node" for example, the condition, that gets added, filters out all unpublished nodes that are owned by the user viewing the data, which is wrong.

$unpublished_conditions->addCondition($uid_key, $account->id(), '<>');

In my opinion, if the user has the permission to view any unpublished entites of a specific type, neither the published condition nor the unpublished condition(s) need to be added here, since the user is able to see all entites no matter if they are published or unpublished.

What do others think about that?

For now i created a new patch, that just removes the line, that filters out all entities owned by the user.
But as said above, it would be maybe better to not add those conditions at all. But cannot see, if this has other implications then.

1. +++ b/src/EntityAccessControlHandler.php
   @@ -33,13 +33,9 @@ protected function checkEntityOwnerPermissions(EntityInterface $entity, $operati
   -        if ($account->id() != $entity->getOwnerId()) {
   -          // There's no permission for viewing other user's unpublished entity.
   -          return AccessResult::neutral()->cachePerUser();
   -        }
   -
            $permissions = [
              "view own unpublished {$entity->getEntityTypeId()}",
   +          "view any unpublished {$entity->getEntityTypeId()}",
            ];
   

   The entity ownership check is required to add the "view own unpublished ..." permission.

2. +++ b/src/EntityAccessControlHandler.php
   @@ -33,13 +33,9 @@ protected function checkEntityOwnerPermissions(EntityInterface $entity, $operati
            $result = AccessResult::allowedIfHasPermissions($account, $permissions)->cachePerUser();
   

   Here we have to add the "OR" rule to the conjunction, if not, only users with both permissions granted will have access.

3. +++ b/src/QueryAccess/QueryAccessHandlerBase.php
   @@ -148,7 +148,12 @@ public function buildConditions($operation, AccountInterface $account) {
   -        $unpublished_conditions->addCondition($uid_key, $account->id());
   

   This is required to properly check the "view own ..." permission.

+++ b/src/QueryAccess/QueryAccessHandlerBase.php
@@ -152,6 +153,12 @@ abstract class QueryAccessHandlerBase implements EntityHandlerInterface, QueryAc
+      if ($has_owner && $account->hasPermission("view any unpublished $entity_type_id")) {
+        $unpublished_conditions = new ConditionGroup('AND');
+        $unpublished_conditions->addCacheContexts(['user']);

The cache context isn't the whole user entity but just user.permissions

I'm also fixing tests by adding the new view any ... permission to the expected permissions in data provider.

Updating test as we now use user and user.permissions cache contexts when user is the owner of content

Fixing typo in last patch

Tests are there

Attaching patch with updated test for new permission.

1. +++ b/src/EntityPermissionProviderBase.php
   @@ -70,6 +70,13 @@ class EntityPermissionProviderBase implements EntityPermissionProviderInterface,
   +    if ($has_owner && $entity_type->entityClassImplements(EntityPublishedInterface::class)) {
   +      $permissions["view any unpublished {$entity_type_id}"] = [
   +        'title' => $this->t('View any unpublished @type', [
   +          '@type' => $plural_label,
   +        ]),
   +      ];
   +    }
   

   seems like this could be put in the existing condition, but I'm not sure if this should be exposed unconditionally for every entity type, we don't really have a system to allow entity types to control that. but maybe we should.

2. +++ b/src/QueryAccess/QueryAccessHandlerBase.php
   @@ -153,6 +154,12 @@ abstract class QueryAccessHandlerBase implements EntityHandlerInterface, QueryAc
   +      if ($has_owner && $account->hasPermission("view any unpublished $entity_type_id")) {
   +        $unpublished_conditions = new ConditionGroup('AND');
   +        $unpublished_conditions->addCacheContexts(['user.permissions']);
   +        $unpublished_conditions->addCondition($published_key, '0');
   +      }
   

   the result of all this is then a condition that says (status = 0 OR status = 1). That's pretty pointless.

   Instead, what we can do is add an extra condition to "if ($operation == 'view' && $has_published) {" that the user does not have the view any unpublished permission and if he does, we can simply skip all that logic.

we don't really have a system to allow entity types to control that. but maybe we should.

@berdir can you add more details? What do you think it is missing exactly? I think this is something that "we should have" even if we do not have at this moment.

Thanks for the patch in #22, this was exactly what I was looking for.

what I mean is that every entity type that uses the permission provider will get that new permission automatically, even if it is not useful, so with this and other extensions in the future, I'm wondering if we need more fine grained configuration to control what permissions you get.

Well, based on the current task that I am working on, I thought that "view any" would not be needed, then the client figured out that they need a new "viewer" role that can only view entities, also for content moderation the "view any" permission is needed. (Stay tuned, I am going to submit an issue/patch for that module too and reference it here ;) )

So I do not think that fine grained configuration is needed for this, if someone does not need it, it does not use it.

The UncacheableEntityAccessControlHandler also required some tweaking, since this is what I am using mostly in my code. Suggestions from #24 are still not included.

...and the issue+patch for content moderation. As I saw there are quite some open issues related to how CM handles access checking.

Thanks for the patch, I needed this for a project and #27 worked fine for me.

Thanks.

#27 looks good for me.

Thanks! #27 was what I need.

#27 FTW! Thanks mxr576

#27 works here too.

Thanks!

Unassigned.

This looks good. I don't think we need a flag for opting out this permission. It makes sense that someone could view any, edit any, but not delete any. Site builders should just ignore this permission, otherwise.

+++ b/src/QueryAccess/QueryAccessHandlerBase.php
@@ -154,6 +155,12 @@ abstract class QueryAccessHandlerBase implements EntityHandlerInterface, QueryAc
+      if ($has_owner && $account->hasPermission("view any unpublished $entity_type_id")) {
+        $unpublished_conditions = new ConditionGroup('AND');
+        $unpublished_conditions->addCacheContexts(['user.permissions']);
+        $unpublished_conditions->addCondition($published_key, '0');
+      }
+

Per #24, we should optimize the condition which sets the $published_key to 1.

Keeping at NW despite the +1 due to query adjustments.

Hello mglaman, could you please explain what you mean by optimizing the condition for setting "$published_key" to 1?

I'm assuming that what @mglaman means, is that we should optimize the condition that already exists a little bit above, so that we fetch both published and unpublished product when the current user has the view any unpublished product permission.

(For reference, this is the current condition).

        // Restrict the existing conditions to published entities only.
        $published_conditions = new ConditionGroup('AND');
        $published_conditions->addCacheContexts(['user.permissions']);
        $published_conditions->addCondition($entity_conditions);
        $published_conditions->addCondition($published_key, '1');

So when I think of it... It basically means that we can skip the condition alltogether in this case?

So perhaps the existing code can be refactored like this?

        if (!$account->hasPermission("view any unpublished $entity_type_id")) {
          // Restrict the existing conditions to published entities only.
          $published_conditions = new ConditionGroup('AND');
          $published_conditions->addCacheContexts(['user.permissions']);
          $published_conditions->addCondition($entity_conditions);
          $published_conditions->addCondition($published_key, '1');
        }

Oh but there's also this:

      if ($has_owner && $account->hasPermission("view own unpublished $entity_type_id")) {
        $unpublished_conditions = new ConditionGroup('AND');
        $unpublished_conditions->addCacheContexts(['user']);
        $unpublished_conditions->addCondition($owner_key, $account->id());
        $unpublished_conditions->addCondition($published_key, '0');
      }

There's probably room for optimization for sure, but the current patch should also work.. It just seems that we can probably do better, and even just skip adding the status condition in some cases.

Thanks, @mxr576 for your effort, but in some cases, the entity doesn't have an owner especially when the permission "view any unpublished entity_type"
so in this patch I removed the owner check :)

so that we fetch both published and unpublished product when the current user has the view any unpublished product permission.

Yes, I agreed w/ @Berdir that the query conditions could be optimized based on earlier logic checks.

if ($has_owner && $account->hasPermission("view any unpublished $entity_type_id")) {

If the user has the view any permisison, why do we care about $has_owner? This seems like a candidate for if/elseif/else

if ($account->hasPermission("view any unpublished $entity_type_id")) {

} elseif ($has_owner && $account->hasPermission("view own unpublished $entity_type_id") {

} else { 
  // published logic
}

Opened the issue fork and added a commit based on #27, implementing some of the optimization mglaman suggested in #39.

Leaving in "needs work" because I feel it can be optimized further.

Also we might want to add "view any unpubliched $bundle $type" permissions as well.

Created patch from the MR

Hi! Could someone from the maintainers review the latest patch and leave feedback what's need to be done else e.g. https://www.drupal.org/project/entity/issues/3023527#comment-15157931 or it can be merged as is? Thanks!

#42 works as expected

core: 10.2.3
entity: 8.x-1.4

Hi
I am also tested, and #42 wroks!

The patch from #42 works fine.

update patch to changes in 1.5 version

Nice patch! #47 worked over here.
Thanks community ☺️

It doesn't work on core 10.2.7. Any help pls?

re-roll #47 and remove $has_owner check from the permission "view any unpublished entity_type"

I'm going to see if I can refresh this and take a look if the feedback has been addressed.

I'm hiding the patches so we can make sure the work stays in the same MR the patches seem to have diverged from the MR so I created a new one starting with patch in 50.

Ok I hope it is ok to still mark this, I only updated test variables to the ones they were clearly meant to be.

I went through it and it looks right.

I manually tested it too.
The cache contexts look like they were updated properly
I did change the variables in the test because they were named wrong.

Tests pass and the test only job fails.

The patch looks good at first glance, haven't manually tested it, but just noting that I'd like Commerce to leverage the change for its Product entity (See #3262938: View any unpublished commerce_product).

Unfortunately I think this needs to be addressed in core, @berdir mentioned this is minimally maintained and this is not a security issue. I've been meaning to find the corresponding core issue but I haven't had the time yet.

Attached is the patch file for MR 39
In case someone needs a static patch

Not exactly the way this is supposed to be managed but there was a release yesterday bringing this module to 1.6.0 and it looks like this work has been committed there.

No, this is definitely not in the release. It might conflict with that though, I didn't test that.

update patch to changes in 1.6 version

Please update the mr

Hi @berdir, I saw you RTBCED this, any reason not to go ahead and commit this? (I don't have commit access myself).

The plain diff no longer applied correctly. I merged 8.x-1.x into 3023527-view-any-unpublished to resolve this.

New patch attached, which is the same as the plain diff.

Updated for entity 8.x-1.x branch changes.