Closed (fixed)
Project:
Views Bulk Operations (VBO)
Version:
8.x-1.x-dev
Component:
Core
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
21 Jul 2017 at 09:44 UTC
Updated:
20 Nov 2017 at 14:19 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
plov commentedComment #3
plov commentedComment #4
graber commentedAn action has
accessmethod that is called before executing it on each entity to check if the current user can execute that action on that particular entity. If not, the action is not executed.As to the form display - if a user has access to see the view, he/she also has access to see the VBO form, as it's a part of this view.
The only thing that could be done here is an additional actions permissions module as in the Drupal 7 version to restrict access to certain actions and not show them on the VBO form.
Comment #5
graber commentedComment #6
adamps commentedCompletely agree, I think that is the feature that would be useful. Users can get confused that there are lots of actions that they don't understand and have no permission to run.
However we could do something more powerful than the D7 version which had a fixed pattern without any hooks. Often I find that a module defines an action and also a permission that governs the action. For example, see pathauto module "Update URL alias of an entity", code here. The action access function simply checks permission "create url aliases". We want to show the action based on that permission. We don't want another permission "run action update URL alias" which the site admin has to try to assign to match "create url aliases".
Here are some ideas that form a plan of increasing complexity that could be added in steps.
1) Create a VBO hook (in the base module) that controls whether to show an action:
hook_display_action(ActionInterface $action, AccountInterface $account = NULL), returns AccessResultInterface. This seems cleaner and more extensible than the D7 hard-codedif (module_exists('actions_permissions'))2) Create a sub-module "Action permissions" that provides an implementation of the above hook by checking against a permission. Default behaviour is to create a new permission to control each action - i.e. identical to D7.
3) Add advanced behaviour to allow use of an existing permission. Ideally there would be a way for module implementers to indicate the existing permission, perhaps extra metadata on the action? "Action permissions" could be aware of the correct mapping for core actions. Could allow user to configure the mapping?
Comment #7
graber commented@AdamPS thanks for the initial concept, thought about it a bit and an idea of modules beeing able to define visibility permissions for actions is a good one.
I'd do it the following way:
ViewsBulkOperationsActionManager::getDefinitions() extendDefinition()or maybe::extendDefinition()(will have to check performance impact) so other modules (including actions_permissions) can set that parameter and also tamper with other parameters if neededComment #8
graber commentedThis patch prepares VBO API for the actions_permissions module.
Comment #9
adamps commented@Graber great thanks, it sounds good. I don't fully understand the new D8 mechanisms, but definitely sounds good to use them. I had a look at your patch, and here are my thoughts in case it helps.
Drupal\Core\Routing\Access\AccessInterface. For example in user.routing.yml requirements can be_permission: 'access administration pages'or_access_user_register" or_user_is_logged_in: 'TRUE'or_entity_access: 'user_role.update'etc. Is there any way you could do the same? I.e. change 'permission' to 'requirements' like routing.yml?Comment #10
graber commented@AdamPS, thanks for your remarks:
Also extended functional tests to include permission check.
Comment #11
adamps commentedGreat, many thanks.
I would appreciate your advice on how to handle an action like "Delete Content". Ideally it would be enabled for users that have at least one of the permissions "XXX: Delete any content" or "XXX: Delete own content". However when my company builds sites, only admins can delete (others unpublish) so it would be possible to simplify.
Would it make sense to write code that automatically sets the Action permissions permission (which I guess would be called something like "Access action delete content") based on other permissions or on role? I guess this code would need to somehow rerun if permissions were saved including the case of a new content type.
Comment #12
adamps commentedActually it's going to work really well for our sites. The permission will default to being on for admins only, which is exactly the correct behaviour. So it seems that you have got a good solution, thanks.
Comment #13
graber commented@AdamPS good to hear it'll work in your case, but if you seek support with individual cases from your work, better use drupal.slack.com, IRC or PMs.
Actions Permissions coming soon.
Comment #14
graber commentedIncluded many improvements for the
Drupal\views_bulk_operations\Service\ViewsBulkOperationsActionManagerclass.Comment #17
graber commentedActions Permissions module is now a part of Views Bulk Operations dev branch. If anyone has a bit time, I'll be gratefull for a review. It'll soon be released in beta1.
Comment #18
adamps commentedGreat to see this available, thanks. Comments:
I tried to test executing the action. Even without the Action Permissions module I was not able to execute actions on the dev branch so it probably isn't related to this patch. I got the following error.
TypeError: Argument 2 passed to Drupal\views_bulk_operations\Controller\ViewsBulkOperationsController::__construct() must be an instance of Drupal\views_bulk_operations\Controller\ViewsBulkOperationsActionProcessor, instance of Drupal\views_bulk_operations\Service\ViewsBulkOperationsActionProcessor given, called in XX/modules/views_bulk_operations/src/Controller/ViewsBulkOperationsController.php on line 48 in Drupal\views_bulk_operations\Controller\ViewsBulkOperationsController->__construct() (line 37 of modules/views_bulk_operations/src/Controller/ViewsBulkOperationsController.php).
Comment #19
graber commentedThanks for the review! I couldn't help it and checked all.
usestatement, but everything worked on my environment and also automated tests that test this functionality didn't finish with errors, on my environment (PHP 5.6) and here on drupal.org (PHP 7) as well. What PHP version are you using? Fixed.Comment #20
graber commented@AdamPS: About 6 - The tests don't cover that actually. also my manual tests lacked coverage of the controller. I'll update the automated tests issue to include a test for that.
Comment #21
adamps commentedGreat thanks
Comment #24
graber commentedCommitted & pushed the changes. Two points to test:
1. I couldn't hide the field completely for rendering, either my knowledge is not sufficient or views API doesn't allow that: If there's a field set to display in the view config, it must be displayed, even empty. To be further researched when time allows. For now I added empty classes to empty table cells and wrappers in case of other style plugins.
2. This was tricky but I think it shuold be solved now.
Comment #25
adamps commentedExcellent, thanks.
Comment #27
graber commentedI think we have this one. Setting to "Fixed".
Comment #28
graber commentedUpdated credit. Thanks a lot for your help @AdamPS!
Comment #29
adamps commentedGreat many thanks @Graber, the new release is a big step forwards