Permissions per role per entity bundle

I am in need of this as well, wonder if this is easily achievable

I added a small utility module that:
1. Alter the clone node route
2. add an access controller
3. Add permission for cloning per bundle.
4. Alter operations for node based on permission.

The module can be incorporated as a submodule or I can possible release it in a sandbox. Patch attaced.
To test:
1. Apply patch:
2. enable eclone_extras

updated patch

I get the following error when attempting to enable Entity Clone Extras:

The website encountered an unexpected error. Please try again later.</br></br><em class="placeholder">ReflectionException</em>: Class Drupal\eclone_extras\EventSubscriber\EntityCloneSubscriber does not exist in <em class="placeholder">ReflectionClass-&gt;__construct()</em> (line <em class="placeholder">30</em> of <em class="placeholder">core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterEventSubscribersPass.php</em>). <pre class="backtrace">Drupal\Core\DependencyInjection\Compiler\RegisterEventSubscribersPass-&gt;process(Object) (Line: 141)
Symfony\Component\DependencyInjection\Compiler\Compiler-&gt;compile(Object) (Line: 788)
Symfony\Component\DependencyInjection\ContainerBuilder-&gt;compile() (Line: 1318)
Drupal\Core\DrupalKernel-&gt;compileContainer() (Line: 892)
Drupal\Core\DrupalKernel-&gt;initializeContainer() (Line: 806)
Drupal\Core\DrupalKernel-&gt;updateModules(Array, Array) (Line: 550)
Drupal\Core\Extension\ModuleInstaller-&gt;updateKernel(Array) (Line: 200)
Drupal\Core\Extension\ModuleInstaller-&gt;install(Array, 1) (Line: 83)
Drupal\Core\ProxyClass\Extension\ModuleInstaller-&gt;install(Array) (Line: 451)
Drupal\system\Form\ModulesListForm-&gt;submitForm(Array, Object)
call_user_func_array(Array, Array) (Line: 111)
Drupal\Core\Form\FormSubmitter-&gt;executeSubmitHandlers(Array, Object) (Line: 51)
Drupal\Core\Form\FormSubmitter-&gt;doSubmitForm(Array, Object) (Line: 589)
Drupal\Core\Form\FormBuilder-&gt;processForm(&#039;system_modules&#039;, Array, Object) (Line: 318)
Drupal\Core\Form\FormBuilder-&gt;buildForm(&#039;system_modules&#039;, Object) (Line: 93)
Drupal\Core\Controller\FormController-&gt;getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber-&gt;Drupal\Core\EventSubscriber\{closure}() (Line: 582)
Drupal\Core\Render\Renderer-&gt;executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber-&gt;wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber-&gt;Drupal\Core\EventSubscriber\{closure}() (Line: 151)
Symfony\Component\HttpKernel\HttpKernel-&gt;handleRaw(Object, 1) (Line: 68)
Symfony\Component\HttpKernel\HttpKernel-&gt;handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session-&gt;handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle-&gt;handle(Object, 1, 1) (Line: 99)
Drupal\page_cache\StackMiddleware\PageCache-&gt;pass(Object, 1, 1) (Line: 78)
Drupal\page_cache\StackMiddleware\PageCache-&gt;handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware-&gt;handle(Object, 1, 1) (Line: 52)
Drupal\Core\StackMiddleware\NegotiationMiddleware-&gt;handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel-&gt;handle(Object, 1, 1) (Line: 665)
Drupal\Core\DrupalKernel-&gt;handle(Object) (Line: 19)
</pre>

If I resubmit the page, it seems to go through.

Afterward, I don't see any obvious way to restrict entity cloning by bundle. My /admin/people/permissions page still only has "Clone all X" type entries. What should I be looking for to restrict by bundle?

OK, that was super weird. I went down the route of reimplementing large chunks of this patch on my own to try to understand it, thought "I'm just reinventing the wheel here" and went back to figure out what was wrong with this patch.

When I tried reinstalling it on a fresh install, I got no errors, and the bundle level permissions showed up properly.

I don't trust my good fortune here. I'm going to try to roll this patch again and ensure it deploys correctly. If so, i'm going to flag this as "reviewed at tested".

For some reason, the original patch refuses to create the EntityCloneSubscriber file on my machine. I've tried creating this revised patch to see if it clears things up...

Aha, now that I've added the file back, I see what you were doing. Most of your Event Subscriber is logging, but it looks like there's debug code in there too. Here's an alternate patch that removes the EntityCloneSubscriber entirely.

Patch #4 adds a module the define permissions for each bundle of and only of the node entity type. You can still have Clone All or take the clone all permission away and give users permission to clone per bundle.
Please add an interdiff to know what changed between the patches so it's easier to review.

Patch #11 works for me.

#11 been working great for me also. (Wait, that's my patch... lol)

The patch seems to have stopped working with the latest version of this module (8.x-1.0-beta2).
I have the permissions set but I can no longer clone.

Update: It does work, but you have to set the permission "clone node entity" as well.

Update 2: But with that permission it is of course possible to clone all nodes of all content types.

I can verify the patch doesn't apply to 1.0-beta2.

However, it does apply to 1.x-dev (51cb014 - as of Feb 22), and appears to still work.

Any ideas what happened here with beta2?

Update - I'm now convinced this might not be working. I'm getting errors of the type:

Notice: Undefined index: is_circular in Drupal\entity_clone\EntityClone\Content\ContentEntityCloneBase->cloneReferencedEntities() (line 154 of modules/contrib/entity_clone/src/EntityClone/Content/ContentEntityCloneBase.php)

Looking into it...

Sorry for all the spammy comments above. I did more testing, I need to reestablish a few facts:

Patch #11 applies against both 1.x-dev and 1.0.0-beta2 without error

My is_circular issue above is a different issue, not related to this patch.

Using both 1.0.0-beta2 AND 1.x-dev, the module no longer functions as expected. Non-admin users lose the ability to clone entity types that they have permission to on 1.0.0-beta1.

Early testing suggests that eclone_extras_entity_operation_alter() isn't even called anymore.

I've verified this was broken by the addition of #2743379: Clone operation shows regardless of permission into 1.0-beta2. Specifically, the change in RouteSubscriber::getEntityCloneRoute():

         ->addRequirements([
-          '_permission' => 'clone ' . $entity_type->id() . ' entity',
+          '_entity_access' => $entity_type_id . '.clone',
         ])

(A note you need to clear your cache for the change to take effect).

I've created a patch to undo this permissions requirement temporarily. Note that this is NOT a proper fix of beta3 - rather it's a short term workaround. eclone_extras permissions needs to be reworked to handle 1.0-beta3's new permission requirements. Which is why I'm leaving this as "Needs Work" for now.

I've had some time to look at this again and reviewed the change that broke this functionality. I noticed a hook_entity_access was added, so I added a hook like that in eclone_extras with the new permissions that are created in the submodule.
It seems to work well for me. I have added a new patch, with this addition:

/**
 * Implements hook_entity_access().
 */
function eclone_extras_entity_access(EntityInterface $entity, $operation, AccountInterface $account) {
  if ($operation === 'clone') {
    return AccessResult::allowedIfHasPermission($account, 'clone ' . $entity->bundle() . ' node entities');
  }
  return AccessResult::neutral();
}

@all it would be great if you guys add interdiff (diff the patches) so we can understand what is changing between the patches. I would like to see an interdiff for patch 4 and 11, 11 and 22. see https://www.drupal.org/documentation/git/interdiff

Also, now I see that patch #4 has some issues. Just looking at the patch file, I fear I may have not generated it properly.

I have made interdiffs between the different patches.

Reviewed the patch from 22, found some coding standard issues which are fixed in #25.

Drive-by review of the interdiff in #25, not the full patch (yet)...

1. diff -u b/eclone_extras/eclone_extras.install b/eclone_extras/eclone_extras.install
   --- b/eclone_extras/eclone_extras.install
   
   --- b/eclone_extras/eclone_extras.install
   +++ b/eclone_extras/eclone_extras.install
   

   I think it's a problem that the submodule uses the "eclone_extras" namespace, not "entity_clone_extras".

2. +++ b/eclone_extras/eclone_extras.module
   @@ -26,21 +26,15 @@
    /**
     * Alter entity operations.
     *
     * @param \Drupal\Core\Entity\EntityInterface $entity
     *   The entity on which the linked operations will be performed.
     *
   - * @return array
   - *   An operations array as returned by
   - *   EntityListBuilderInterface::getOperations().
   - *
     * @see \Drupal\Core\Entity\EntityListBuilderInterface::getOperations()
   - *
     */
   -function eclone_extras_entity_operation_alter(array &$operations, $entity) {
   +function eclone_extras_entity_operation_alter(array &$operations, EntityInterface $entity) {
   

   The operations array is the first argument, passed by reference. So yes, we shouldn't document it as @return, but it should be the first @param documented, not just completely removed.

3. +++ b/eclone_extras/src/Controller/EcloneExtrasAccessController.php
   @@ -8,7 +8,9 @@
   +/**
   + * Class EcloneExtrasAccessController.
   + */
   

   That satisfies the robots, but is totally unhelpful as a class comment. ;)

4. +++ b/eclone_extras/src/Routing/RouteSubscriber.php
   @@ -2,11 +2,13 @@
   +/**
   + * Class RouteSubscriber.
   + */
   

   Same here.

Thanks,
-Derek

Hi. I agree that passed by reference should be documented in @param tag, I only noticed it now.
For the 3,4 it is indeed to please the robots. I did not want to repeat documentation in class doc, since both of them implement only one method other than construct and create, which is very well documented.

I can confirm patch #25 works for me. (using 1.x-dev)

Fixes for comments in #26.

Shoot missed a file. Let's try that again.

Hi. I have took some time to test this and patch from #30 seems to be working as expected. Patch applied successfully, it works as described and there is also no deprecated code. I'm changing status to RTBC.

1. --- /dev/null
   +++ b/entity_clone_extras/entity_clone_extras.info.yml
   

   It would be better to add this submodule in a "modules" folder.

2. +++ b/entity_clone_extras/entity_clone_extras.install
   @@ -0,0 +1,17 @@
   +function entity_clone_extras_install() {
   +  // Set an explicit module weight, to ensure that the form alter hook is
   +  // always called after entity_clone.
   +  $extension_config = \Drupal::configFactory()->get('core.extension');
   +  $weight = $extension_config->get("module.entity_clone");
   +  module_set_weight('entity_clone_extras', $weight + 1);
   +}
   

   This hook is no longer required.
   No hook_form_alter is implemented.

3. +++ b/entity_clone_extras/src/Routing/RouteSubscriber.php
   @@ -0,0 +1,45 @@
   +  public function alterRoutes(RouteBuildEvent $event) {
   +    // Fetch the collection which can be altered.
   +    $collection = $event->getRouteCollection();
   +    if ($route = $collection->get('entity.node.clone_form')) {
   +      $requirements = $route->getRequirements();
   +      // Unset the default requirement for node clone route.
   +      unset($requirements['_permission']);
   +      $requirements['_custom_access'] = '\Drupal\entity_clone_extras\Controller\AccessController::access';
   +      $requirements['node'] = '\d+';
   +      // Add a new requirement.
   +      $route->setRequirements($requirements);
   +      // Add the node to the path requirement to pass it to our access method.
   +    }
   +  }
   

   There is no necessary to alter routes to add a custom access.
   Only Entity operations and hook_entity_access are necessary

Hi,

I removed the uneccesairy files. As suggested the patch still works, but I will put it for review.

Here is the correctly created patch.

@vpeltot

There is no necessary to alter routes to add a custom access.
Only Entity operations and hook_entity_access are necessary

If we don't then, correct me if I am wrong, the clone operation will display regardless of the users permission and the access denied would only happen when a user attempt to clone.

it's been a long time since I first wrote the initial patch so this may have changed.

@ngkoutsaik could you verify the above?

Update: oh I see, this issue may have been addressed in https://www.drupal.org/project/entity_clone/issues/2743379

I tested this with the latest patch applied. I created a user with permission to clone articles. The clone operation does not show on the other content types.

This is on the latest branch.

Adding Drupal 9 compatibility.

I noticed patch #40 has a bug - if a vocabulary machine name is the same as a node type's machine name, and the content type is enabled for cloning, it is possible to clone the terms in the vocabulary too; even though the permission of clone taxonomy term is not enabled.

Steps to reproduce:

1. create new role - "editor"
2. create new user with editor role
3. Create a new content type with machine name "test"
4. create a new vocabulary name with machine name "test" , add a new term "Ice cream"
5. go to permission page (admin/people/permissions)
   1. Check "Clone all Test node entities" in "Entity Clone Extras" section
   2. Enable "Test: Edit any content" permission
   3. Permission "Clone all Taxonomy term entities." is not checked
   4. Make sure the new user have permission to view the term
6. Go to "Ice cream" term page "/taxonomy/term/{tid}"
7. And you will see "Clone" button is there even through the permission of clone terms is not enabled

I've added an additional condition to make sure it only applies to content types not other entity types.

I applied the patch in #41, installed the submodule created by it, and I was able to configure the permissions by node bundle. It works as expected.

Thank you @szhu.

@maursilveira could you mark the issue as "Reviewed & tested by the community"?

Hey Matt, sure thing.

Issue status updated to RTBC.

@maursilveira awesome, thanks!

This is looking good! It just needs some minor documentation fixes. (Please provide an interdiff when fixing this.)

+++ b/modules/entity_clone_extras/entity_clone_extras.info.yml
@@ -0,0 +1,7 @@
+description: 'Allow for bundle level permission for node entity type.'

Better grammar: "Allow for bundle-level permissions for Node entity types"

I don't believe we need a "." at the end of descriptions.

+++ b/modules/entity_clone_extras/entity_clone_extras.module
@@ -0,0 +1,74 @@
+      $output .= '<p>' . t('allow for bundle level permissions for node entity type.') . '</p>';

Same here. Also: missing capitalization.

+++ b/modules/entity_clone_extras/entity_clone_extras.module
@@ -0,0 +1,74 @@
+ * Alter entity operations.

Shouldn't this be "Implements hook_entity_operation_alter()." as per the coding standards? See that coding standard for details.

Sending patch addressing #46. Please review.

The patch in #47 works well in our environment.

+1 patch #47 working.

Patch #47 works with a Drupal 9.3

It seems to me this patch is not working correctly, because the permission for cloning all node entities gets in and makes the bundle specific permission not applicable.

If the permission for all node entities is not set, the bundle specific permission will not apply (permission for all node entities forbits, and then it is forbidden for all node entities regardless of bundle permission).

If the permission for all node entities is set and specific bundle permission is also set, it will be allowed for all node entities and not only the specific bundle (which is correct imho).

I suppose we need to go into entity_clone_entity_access too to make this work.

Proposing patch (i have also included bundle specific permission for media type, since I needed that and removed an old part that is related to an issue already fixed #2743379: Clone operation shows regardless of permission.

Interdiff is to #53

entity_clone_extras module needs dependency to media module now. Added this with this patch.

Interdiff to #53 included

Now we have both a MR and a patch list, can we consolidate on one or the other?

Is the Media dependency necessary? That seems to enable an unnecessary dependency for installs, and I suspect folks with Media entities would already have Media enabled?

As far as I can tell, the MR is not working as expected (see #54). So IMHO the MR is not be considered a fix for this issue. Maybe I am wrong but did somewhat thorough testing, though I am wondering that some people report the changes from MR is working for them. The part that needs to be added to make it work is this:

--- a/entity_clone.module
+++ b/entity_clone.module
@@ -148,7 +148,11 @@ function entity_clone_entity_access(EntityInterface $entity, $operation, Account
   $cache->addCacheContexts(['user.permissions']);
 
   // Deny access if the user cannot clone the entity.
-  $access = AccessResult::forbiddenIf(!$account->hasPermission('clone ' . $entity->getEntityTypeId() . ' entity'));
+  $access = AccessResult::forbiddenIf(
+    !$account->hasPermission('clone ' . $entity->getEntityTypeId() . ' entity')
+    &&
+    !$account->hasPermission('clone ' . $entity->bundle() . ' '. $entity->getEntityTypeId() . ' entities')
+  );
   if ($access->isForbidden()) {
     return $access->addCacheableDependency($cache);
   }

So that is the really necessary difference between MR and patch from #55.

I additionally added the media bundle permission, because I needed it and I do not see why only node bundle permissions should be added, since the module is "all entity". Ideally a generic solution for any entity can be found, but this seems a bit more difficult, so I only put in Media for the moment. I am not sure about the dependency to media. For someone that does not use Media and only want the entity clone extra module for node permission, it is probably annoying. But I have a installation profile and put the entity clone extra module there and then the installation failed saying media is needed. Therefore I thought media dependency would be necessary. I guess it is about this part:

--- a/modules/entity_clone_extras/src/EntityCloneExtrasPermissions.php
+++ b/modules/entity_clone_extras/src/EntityCloneExtrasPermissions.php
@@ -69,6 +69,17 @@ class EntityCloneExtrasPermissions implements ContainerInjectionInterface {
         ]);
     }
 
+    $media_types = $this->entityTypeManager->getStorage('media_type')
+      ->loadMultiple();
+    foreach ($media_types as $key => $type) {
+      // Add a permission to clone each content type.
+      $permissions['clone ' . $key . ' media entities'] = $this
+        ->translationManager
+        ->translate('Clone all <em>@label</em> media entities', [
+          '@label' => $type->label(),
+        ]);
+    }
+

Thinking some more about this, it might probably be better to add a condition around this that looks if Media module is enabled instead of the media dependency. I will gladly change the patch accordingly, but maybe we can have opinion about the difference between MR and patch and the added media feature first?

We need this functionality and the MR did not work in our environment. The patch in #54 does work for us.

@stefan.korn, @ emek can we have MR updated accordingly, then request review.

Thinking some more about this, it might probably be better to add a condition around this that looks if Media module is enabled instead of the media dependency.

I would recommend this route, for those that do not use Media (and I suspect there are folks in that scenario).

Can we get these changes (see think-aloud of next steps in #57 and patch in #54) placed into the MR, and return this to Needs Review?

Please consolidate on one code contribution method (MR or patch) so we can keep this issue moving forward.

Fixed permissions issue as described in #54.
Will add support for media now.

Added support for media entities.
And made it easier to allow to support other entity types.

So I've included the needed changes mentioned in #54

So from now on, the patches list is not needed anymore and the MR can be used.

Seeing some not correct code comments and other issues.
Will take this up

Update module description to reflect the media support
Removed the AccessController file as this is never used and is handled in entity_clone_extras_entity_access

I added a small fix for entity_operation_alter hook, because the clone operation got removed even if user has the "clone entity type entity" permission.

It was the first time I used the new issue fork workflow and I have got it wrong at first I suppose. There is now a "merge commit" that I did not want and need, but the result of the MR should be fine now.

This is realy great work done, here can anyone please review this as second eyes/hands.
Also can we add tests to this is possible?

Once its RTBS will merge followed by release.

I can confirm the latest change is working. It would be awesome to have tests in, but don't have the time for this.
I hope this can be committed and maybe a follow up ticket can be created to add tests?

Merged, created new ticket to add tests for entity_clone_extras submodule.

Hi All,

May I know why Media module needs to be enabled to use this enhancement ? I don't want use media, can I just remove dependency in my case ?

Media module is not needed, dependency was only in an early patch, but in current HEAD https://git.drupalcode.org/project/entity_clone/-/blob/19d7920fb7691c84d... no dependency on media module.