Problem/Motivation

At the moment all actions are visible to all users that have access to the view. Access check is only performed when an action is executed on an entity.

Proposed resolution

Port the Action Permissions module to Drupal 8 and add support in Views Bulk Operations.

Comments

plov created an issue. See original summary.

plov’s picture

plov’s picture

Status: Active » Needs review
graber’s picture

Status: Needs review » Active

An action has access method 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.

graber’s picture

Title: Access views bulk operations permission » Port Action Permissions to Drupal 8
Issue summary: View changes
adamps’s picture

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.

Completely 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-coded if (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?

graber’s picture

@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:

  • Add permission or access permission optional parameter to action annotation
  • Add an event dispatcher (hooks are fine, but may be eventually deprecated) to the 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 needed
  • Action permissions will use ViewsBulkOperationsActionManager to get all action definitions and create permissions and will have an event subscriber with low weight to add the permission parameter to action definitions.
graber’s picture

Status: Active » Needs work
StatusFileSize
new4.39 KB

This patch prepares VBO API for the actions_permissions module.

adamps’s picture

@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.

  • The patch defines a permission. However D8 has 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?
  • I spotted a typo "Filter out actions that wasn't selected." - should be 'weren't'
  • With the old hooks way there could be an API file that demonstrated how to use the hook. Can we have something similar with events?
graber’s picture

StatusFileSize
new7.61 KB

@AdamPS, thanks for your remarks:

  1. This issue is about Action permissions and what is proposed in the patch will be sufficient for that purpose. However, annotation structure should change to ensure backwards compatibility. If a custom access handler support will be needed by someone, it'll be a subject for a new feature request and with the new structure it'll be easy to implement.
  2. +1
  3. Documentation will need updating, we have the advanced section. action_permissions will have the event subscriber so it'll be a good example as well. I don't know of anything like the D7 module.api.php.

Also extended functional tests to include permission check.

adamps’s picture

Great, 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.

adamps’s picture

Actually 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.

graber’s picture

@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.

graber’s picture

StatusFileSize
new10.06 KB

Included many improvements for the Drupal\views_bulk_operations\Service\ViewsBulkOperationsActionManager class.

  • Graber committed 789ce8f on 8.x-1.x
    Issue #2896410 by Graber, plov, AdamPS: Prepare VBO API for actions...

  • Graber committed bf100c2 on 8.x-1.x
    Issue #2896410 by Graber, plov, AdamPS: Added action_permissions module.
    
graber’s picture

Status: Needs work » Needs review

Actions 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.

adamps’s picture

Status: Needs review » Needs work

Great to see this available, thanks. Comments:

  • If the user is not allowed to run any actions then should not show the vbo-action-form-wrapper. Also hide the checkbox column if possible.
  • I set a custom permission and it worked. However Action Permissions still created a permission - I guess the permission hook was called before the VBO action executed.
  • ActionsPermissionsEventSubscriber: two comments in this file about views - guess they were accidentally block-copied
  • Maybe change ViewsBulkOperationsActionManager::EVENT_NAME to ALTER_ACTIONS_EVENT - clearer and in case the module needs another event later.
  • Would help to document some example code for the event - I have some for pathauto_update_alias that we can use.

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).

graber’s picture

Status: Needs work » Needs review
StatusFileSize
new4.13 KB

Thanks for the review! I couldn't help it and checked all.

  1. True. Included in the patch.
  2. I'll need a way to reproduce this, can you provide steps? I'm using the /tests/views_bulk_operations_test module and the advanced action defines a permission. The permission for this action is not duplicated by actions_permissions in my case.
  3. Agreed, included in the patch.
  4. As above.
  5. Already updated the documentation issue. Coming soon.
  6. Type error: Funny, the file actually missed the correct use statement, 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.
graber’s picture

@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.

adamps’s picture

Great thanks

  1. Looks good. The invisible column still takes space on my website - if you could set it to hidden that would be even better.
  2. I am testing it "for real". I have installed VBO dev on my test site and applied your patch. I have made a custom module with event subscriber code as below.
  3. Thanks
  4. Thanks
  5. Thanks
  6. Now works thanks. (I am on PHP 7.)

namespace Drupal\XXX\EventSubscriber;

use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\EventDispatcher\Event;
use Drupal\views_bulk_operations\Service\ViewsBulkOperationsActionManager;

/**
 * Defines module event subscriber class.
 */
class XXXEventSubscriber implements EventSubscriberInterface {

  /**
   * {@inheritdoc}
   */
  public static function getSubscribedEvents() {
    $events[ViewsBulkOperationsActionManager::ALTER_ACTIONS_EVENT][] = ['alterActions'];
    return $events;
  }

  /**
   * Respond to alter actions request event.
   *
   * @var \Symfony\Component\EventDispatcher\Event $event
   *   The event to respond to.
   */
  public function alterActions(Event $event) {
    $event->definitions['pathauto_update_alias']['requirements']['_permission'] = 'create url aliases';
  }

}

  • Graber committed 0fed90e on 8.x-1.x
    Issue #2896410 by Graber, plov, AdamPS: action_permissions - related...

  • Graber committed a9ff0e3 on 8.x-1.x
    Issue #2896410 by Graber, plov, AdamPS: minor empty field visibility...
graber’s picture

Committed & 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.

adamps’s picture

Excellent, thanks.

  1. Class attribute seems like the right solution to me. All it needs now is CSS as below. I feel that the best place for that would be in the VBO module - what do you think?
  2. Confirmed.
.empty.views-field-views-bulk-operations-bulk-form {
  display: none;
}

  • Graber committed 89e79a4 on 8.x-1.x
    Issue #2896410 by Graber, plov, AdamPS: Added css to hide empty field.
    
graber’s picture

Status: Needs review » Fixed

I think we have this one. Setting to "Fixed".

graber’s picture

Updated credit. Thanks a lot for your help @AdamPS!

adamps’s picture

Great many thanks @Graber, the new release is a big step forwards

Status: Fixed » Closed (fixed)

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