Default action behaviors in getAccessMask()

You don't need an isset() check when you have an empty() check. Pushed a fix, thanks.

This change has caused some problems:
#2544082: Node updates being lost on return from action call
#2527646: Custom Action Stopped Working
#2542120: 'Pass ids as arguments to a page' fails with "Skipped XXXX due to insufficient permissions." since update to 3.3

The fundamental problem is that setting your behavior to "changes_property" has 2 effects:

1. Determines the access check that VBO will run
2. Causes an entity_save() to automatically be run against your entities in ViewsBulkOperationsAction::execute()

I think it's unreasonable to group those 2 things into a single key. You may want the access check without the entity_save, and you may want the entity_save without the access check.

I don't know how possible it is to re-architect things since VBO borrows an api from the actions module, but ideally those things would be separated.

As a stopgap, I think we should fall back to one of 2 possible behaviors:

1. Revert this change completely. Allow an action with no behavior specified to pass the access check unconditionoally. Existing code was written against this assumption. Is that truly "wrong" behavior of the API? Maybe it is, I'm just not sufficiently familiar.
2. If no behavior is specified still fall back to returning the VBO_ACCESS_OP_UPDATE access mask but don't actually set $this->operationInfo['behavior'] on the action object so that the action is not later forced into an entity_save in the execute method (which can be fatal for some complex revisioning tasks).

Patches supporting each option (1 and 2) are attached.

Patch for option 1 should actually be this, since $this->operationInfo['behavior'] will always be set, if only to an empty array.

I agree with #5. Using "behavior" to determine action is incorrect because the purpose of behavior is to determine additional actions to run such as entity save operations.

+1 for option #1 is a decent short-term patch to apply on top of VBO, but I am not sure if it is releasable because at this point VBO has introduced two user bases: 3.2 and 3.3 users, and we should not break compatibility with the latter.

Another option for the long term would be to check for an additional "access" key in action info similar to entity api and entity info. This would be something like

• Undefined: use entity access as a fallback to appease the site builder security theater.
• array('type' => 'behavior'): use behavior for 3.3 backwards-compatibility.
• array('type' => 'permission', 'permission' => 'access content'): use a permission.
• array('type' => 'callback', 'callback' => CALLABLE): allow someone to override or define custom access.

That would allow the use case of defining a custom action with very specific non-entity-related access.

+1 for mradcliffe's suggestion in #7.

Actually...looking at this a bit more closely, I think we should do something a little different than #7.

The current code returns a bitmask that's used to determine what types of operations are being done on the entity in question so that entity_access can be called. We absolutely need to enforce entity_access properly and doing so means knowing what types of operations will be run against the entity.

I think we can maybe take a cue from hook_menu and define the following 2 new keys:

access callback
A callable. By default this will be 'entity_access'.

access arguments
An array of arguments to pass to the callable. By default this array will be populated with values mapped from the behavior key. This will retain backwards-compatibility.

Ugh. This is all a bit of a mess. The assumption that entity_access would be the way to determine access is quite baked into _views_bulk_operations_entity_access(). I'm trying to unravel how best to move the API forward without breaking things for existing users. I tend to think that getAccessMask() should be deprecated in order to give more flexibility.

And why does _views_bulk_operations_entity_access() even exist? Why do we not have an ViewsBulkOperationsBaseOperation::entity_access() method so that operation classes can define their own entity_access logic?

Here's a first stab. I'm not doing exactly what I described in #9. Instead I've added 3 possible keys (all optional):

entity operations
An array of operations that will be executed against the entity. e.g. array('update', 'view'). If this is specified then entity_access() will be called for each of those operations for each entity. This is effectively allowing you to specify the access checks that will be run against your entity and not have that tied to whatever you've set in the 'behavior' key.

access callback
A callable that will be called with call_user_func_array(). If this is set then it takes complete precedence over 'entity operations' and 'behavior' for access purposes. This callable's first 3 arguments should receive $entity_type, $entity, and $account. The rest of its arguments will be populated with whatever's set in 'access arguments'.

access arguments
An array of additional optional arguments passed to 'access callback' as described above.

If none of the above keys are populated, then it will fall back to the existing behavior of using the 'behavior' key to determine access.

I've implemented all of this by adding a new method to ViewsBulkOperationsBaseOperation called entity_access. This should give us more flexibility.

I'm not sure this is a great approach, overall. It feels messy. I could be convinced to simplify things by dropping the 'access callback' and 'access arguments' keys entirely since the new approach would allow folks who desperately want more advanced capabilities to just extend ViewsBulkOperationsBaseOperation and write their own implementation of entity_access().

+1 on the general approach used here. A couple small things fixed here. Otherwise, I'd call this RTBC.

'access arguments' => !empty($action['access arguments']) ? $action['access callback'] : array(),

Should that access callback be access arguments?

@drumm -- yep; typo! Good catch.

I manually tested the patch in #14, applying it on top of VBO 3.3, and tested the access callback bit.

- I confirmed that without access callback my custom action with type user was denied for a semi-privileged user for 'changes_property'.
- I confirmed that after adding access callback to the custom action, the action was successful and went through the access callback instead.
- I did not have any actions using entityOperations so I could not manually test that.

I could not apply the patch on top of VBO 3.3, trying:

wget https://www.drupal.org/files/issues/views_bulk_operations-no_default_action-2254871-15.patch
patch < views_bulk_operations-no_default_action-2254871-15.patch

in the module's directory, I got:

can't find file to patch at input line 5
Perhaps you should have used the -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/plugins/operation_types/action.inc b/plugins/operation_types/action.inc
|index 2cdd6a1..c38cb06 100644
|--- a/plugins/operation_types/action.inc
|+++ b/plugins/operation_types/action.inc
--------------------------
File to patch:

The patch was made using git. As such, it can be applied using patch as curl https://www.drupal.org/files/issues/views_bulk_operations-no_default_action-2254871-15.patch | patch -p1 or patch -p1 < views_bulk_operations-no_default_action-2254871-15.patch if the patch has already been downloaded. The former is the way I patched and tested on VBO 3.3.

I see access arguments gets populated in views_bulk_operations_operation_action_list() with this patch. Where do access callback and entity operations get added in? As far as I can tell, they are new APIs introduced by this patch.

Attached is a more simple fix for this regression. This makes ViewsBulkOperationsAction::getAccessMask() read-only, it doesn't modify $this->operationInfo['behavior'].

This approach fixes #2590101: "Report node to Akismet as spam and unpublish" action should properly unpublish in my testing, and #15 does not. And it does not require any API additions.

I prefer the more flexible approach in #15 that adds to the API a way for me to have operations that are not dependent on entity operations. I don't want to give administer users permissions or view user profiles permissions in order to grant access to some random action that requires more complex access requirements than a boolean.

With #15, it also becomes possible for a custom module to alter action info and add in any additional access restrictions or make those complex access restrictions that go beyond entity access whereas it is not possible to do so in #20.

I think it would be a good idea to revise #15 with changes from the patch in #20.

drumm's suggestion in #20 seems appealing because it fixes the immediate problem faced by some existing actions without making them make any changes. Unfortunately, it means splintering the VBO user-base into 3 different APIs rather than just the 2 that currently exist. To be explicit:

Unset behavior property in 3.2:

• Behavior on the object remains unset
• All access checks pass
• No auto-save of the entity

Unset behavior property in 3.3 (and in patch #15):

• Behavior on the object is auto-set to changes_property
• Access check against entity_access('update'...)
• Auto-save the entity

Unset behavior property in patch #20:

• Behavior on the object remains unset
• Access check against entity_access('update'...)
• No auto-save of the entity

I'll be honest: in my mind, 3.2 had it right. I don't agree with the premise of this original issue that there should have been a fallback to changes_property. Is that in the documentation somewhere?

Anyhow...I think we're at a point where we just need explicit guidance from the module maintainers for the desired correct behavior of behavior.

One thing that 3.3, patch 15 and patch 20 are all missing is the ability for an action to avoid access checks altogether (this was possible in 3.2 by having an unset behavior). I think this is valuable. I've updated #15 to allow the bypassing of all access checks if access callback is set to TRUE (just like the menu system). I've attached 2 different versions of that patch: one that's a straight update of #15, and one that incorporates #20.

+1 for 3.2 having correct behaviour. This issue has become and issue for one of my sites with OG where users act on OG relationships - they don't (and shouldn't), need 'update' access on an entity to perform operations group management operations with entities other than themselves.

Thanks for your diligence on this issue azinck, you saved me a bunch of time debugging.

I've came across this issue when trying similar thing than stewart.adam. I have a user that it's only confirming OG pending requests and I don't want to give him editing permission for group.

I patched VBO with azinck's patch in #22 (the second one that incorporates #20 as well).

There is a problem with passing the 'access callback' argument. As I found out it's not added in views_bulk_operations_operation_action_list ('access arguments' is) so we don't have it entity_access. I've added it and it's working.

Another notice - azinck, you said that we should put TRUE to 'access callback' but than the function does

if (is_numeric($callback)) {
  return (bool) $callback;
}

So correct value for bypassing access is 1.

Please check.

Good catch on both of those. Will give you a fixed patch shortly.

FWIW, the reason for my error interpreting the boolean was that I just copied that code from _menu_check_access because I wanted to be consistent with the types of values that it interprets. What I didn't realize was that _menu_router_build() is casting bools to ints for storage so _menu_check_access() is only having to deal with either strings or ints. I just assumed that in the crazy world of PHP type casting that is_numeric(TRUE) must evaluate to true :).

I'm going to only post one patch since #20 will still apply on top of this if you want that functionality.

Oops, sloppy on my part. Also was neglecting to add 'entity operations' to the array.

Let's try this one more time and see if I can avoid screwing it up :) -- #27 accidentally included #20. This one should be clean.

From my point of view the patch now does what it's supposed to.

And about is_numeric() part, I was also looking at it for quite some time when I realized. :)

I can confirm this patch works

#29 does not fix the problem as it affected Drupal.org in #2590101: "Report node to Akismet as spam and unpublish" action should properly unpublish . I suppose Mollom module will have to implement some of this new API to be a complete fix?

Anyhow...I think we're at a point where we just need explicit guidance from the module maintainers for the desired correct behavior of behavior.

Agreed.

@drumm If the maintainers want to move forward with #29 as-is, then Mollom would have to update its implementation of the VBO API.

However, if the maintainers like your approach in #20 it could be applied on top of #29 and Mollom could remain un-changed. It comes down to the maintainers deciding between the options outlined in #22.

So, I'm here because I need VBO to stop validating a operation for a view I've already set access permissions on.

I applied both the patches from comments #29 and #20, and I was able to achieve what I needed with a custom action.

The comment for the added method in base.class.php in the patch in comment #29 is lacking a considerable amount of information. So, I updated it and tweaked a few of the other comments. I also added some white space to comply w/Drupal's Coding Standards. I did not change any of the actual code.

Even though I didn't change the code, I'm still marking it CNR to make sure I didn't accidentally add or remove something important.

@oadaeh, I'm looking at committing one of these approaches ASAP. It doesn't look like #20 is actually in #35, though the interdiff changes look fine. Can you maybe try that again if you are around. My thought is I'll read through and test #29 + #20 as a possible way forward for this issue.

@azinck and @mradcliffe and @drumm did some great work thinking through this problem!

Thank you!

Ok to help push this along a bit, here's the patches from #29 and #20 combined for your review.

Thanks, @joelpittet. I'm not sure if I have much time at the moment, but probably the next step is to confirm the patch works with the various usages.

Sorry for the delay, Joel. I've been quite busy lately, but I'll try to set aside some time today or tomorrow to help out with this.

I went through what I had locally, and I'm not sure what happened, but when creating my patch in #35, I failed to apply the patch for #20 (obviously). However, I have the patch in #20 applied to the code I'm using, so I had some sort of disconnect somewhere.

I updated the branch I was working off of to have the latest upstream changes, applied the patches from #20 and #29, applied the comment changes I made in my patch (but not all the Drupal Coding Standards changes I made earlier), and I've attached the result.

It's fairly similar to what you have, @joelpittet.

Thanks, those coding standards improvements look good. Much appreciated @oadaeh :)

Hi all

Please don't consider me boring but I've made reroll of patch from #40.
Some obvious things such as:
* Change order of conditions in one place (see interdiff).

Also there is one place in the patch that didn't sympathize to me.
I mean this code:

public function entity_access($entity_type, $entity, $account = NULL) {
    ...
    // If the action has specified which operations are being executed against
    // this entity, then check those specifically against entity_access. This
    // is offered as a convenience to avoid having to write a custom access
    // callback for many common use-cases.
    if (!empty($this->operationInfo['entity operations'])) {
      foreach ($this->operationInfo['entity operations'] as $op) {
        return entity_access($op, $entity_type, $entity, $account);
      }
    }
   ...
  }

To be honest I didn't understand this thing. It looks like we have an array of entity operations but we return result of execution entity_access only for the first operation.
How it going works?
If it's correct flow then maybe we should made this property not array?

I made a patch which shows my point.

@sumthief the hunk you are referring to doesn't seem to exist in #40 nor your interdiff.

From #40:

+    // If the action has specified which operations are being executed against
+    // this entity, then check those specifically against entity_access. This
+    // is offered as a convenience to avoid having to write a custom access
+    // callback for many common use-cases.
+    if (!empty($this->operationInfo['entity operations'])) {
+      foreach ($this->operationInfo['entity operations'] as $op) {
+        if (!entity_access($op, $entity_type, $entity, $account)) {
+          return FALSE;
+        }
+        return TRUE;
+      }
+    }

The code just shortcuts to the first failed access to return FALSE early, which seems good to me.
The change you have does the same thing with a new $allowed_entity_operations variable keeping track but still breaking early and returning, I don't see the code path difference, maybe I'm missing something?

The other change in your patch seem like a nice swap, thank you, that will prevent a notice error report:

-    if (in_array('changes_property', $this->operationInfo['behavior']) || empty($this->operationInfo['behavior'])) {
+    if (empty($this->operationInfo['behavior']) || in_array('changes_property', $this->operationInfo['behavior'])) 

@joelpittet,

It doesn't exists in my patch because it was rewritten on this code (with $allowed_entity_operations):

+    // If the action has specified which operations are being executed against
+    // this entity, then check those specifically against entity_access. This
+    // is offered as a convenience to avoid having to write a custom access
+    // callback for many common use-cases.
+    if (!empty($this->operationInfo['entity operations'])) {
+      $allowed_entity_operations = TRUE;
+      foreach ($this->operationInfo['entity operations'] as $op) {
+        if (!entity_access($op, $entity_type, $entity, $account)) {
+          $allowed_entity_operations = FALSE;
+          break;
+        }
+      }

For me tracking is necessary.
Let me explain: for example we have case when we have defined $this->operationInfo['entity operations'] and it contains 2 operations:

// Lets imagine that this command
var_dump($this->operationInfo['entity operations']);
// Will output this:
array(
 0 => 'view',
 1 => 'update',
);

So if we will put this code: what will happen if user have permission to view, but have no permission on update?

foreach ($this->operationInfo['entity operations'] as $op) {
+        if (!entity_access($op, $entity_type, $entity, $account)) {
+          return FALSE;
+        }
+        return TRUE;
+      }

It will check first item only and make a global return from function.
Looks like a possible vulnerability (this function will return TRUE because user have permission on view and it will checked first).

But I could be mistaken and please don't hesitate to correct me.

Yes, I agree with you, sumthief. It's a mistake from my patch. The return true needs to be moved out one level, outside the foreach.

Oh yeah good eye! That return TRUE should have been outside. I should have read the brackets closer on that one. It would be the same code if the return TRUE were outside the loop, but then it would be not shortcut-ed and go through all access TRUE.

Ok let's leave it as it is now:) Can we have a review of #42

That said, I don't agree with #42's style choice.

We don't need $allowed_entity_operations. Just return false immediately if we ever hit false there, and return TRUE if we manage to escape the foreach without returning FALSE.

So it should be:

    if (!empty($this->operationInfo['entity operations'])) {
+      foreach ($this->operationInfo['entity operations'] as $op) {
+        if (!entity_access($op, $entity_type, $entity, $account)) {
+          return FALSE;
+        }
+      }
+      return TRUE;
+    }

@azinck That is really minor, I'd rather concentrate on getting this in and I'll fix that on commit. You've worked a lot on the patches so I do appreciate your code reviews if there is any more changes needed but I'd like to see someone who is using one of the previous patches to chime in with a real life test and a RTBC stamp

Agreed that my nitpick is indeed very minor :).

My biggest concern about this is that we're changing the API again (see my post #22 in this issue). This patch implements option #3. If we commit this we need to communicate really clearly about the change so that other modules can update (yet again).

@azinck, I think I'm with you on that. I'm a bit wary of the Assume edit by default.

#6seems like the right approach in general. Maybe instead of #20 we can merge with #6?

Though since ::entity_access() is called first, you will get this new API... the more I think about this the more I just want to commit #6 and move the other code to a follow-up and write tests for it to ensure the use-cases are covered. There are no test in VBO, so we'd need to start from scratch there and build up some.

Am I thinking about this all wrong? Anybody want to help me write some tests, I can kick start them and turn them on.

@joelpittet I don't think it's a problem to merge #6 and #29. If we were to do that then the API of VBO 3.2 would be restored but enhanced with the additional options added here if folks were to want to use them. Anyone using 'behavior' as they were using it in VBO 3.2 would see the appropriate functionality since ::entity_access() would be called first but none of its checks would be activated and the code flow would fall through to _views_bulk_operations_entity_access() anyway.

If we were to merge #6 and #29 the only thing that would worry me is if there's any contrib code written that relies on 3.3+'s behavior of defaulting an un-specified "behavior" property to "changes_property". In that case you'd have actions receiving an access check in 3.3 that would no longer receive an access check (or an auto-save) once this patch hits.

I wanted to add that for my situation, neither of the proposed options in #20 or #29 worked for me. I was only able to accomplish what I desired by combining them. Otherwise, I would have just +1'ed the one that worked for me.

I don't remember if I specifically tried the patch in #6, but I did spend quite a bit of time reading through and trying and testing the various things listed as potential fixes until I found what worked for me, and because it was different than what was being proposed, I offered my patch.

The patch does not work for me. Only user 1 (admin) can perform mass operations.

Rerolled patch in #42 for VBO most recent version 3.x. Please note it doesn't work in 3.4! Two conflicts resolved by adding !$skip_permission_check to the beginning of if clauses in views_bulk_operations.module lines 1091 and 1189.

Why does #56 say "bojanz committed 14fdbed on 8.x-1.x" when this issue was/is filed for 7.x-3.x and it shows as being in 7.x-3.x? https://cgit.drupalcode.org/views_bulk_operations/commit/?id=14fdbed

@DigitalFrontiersMedia because he applied the same patch to the 8.x-1.x branch as he did to the 7.x-3.x branch in #3.

https://cgit.drupalcode.org/views_bulk_operations/tree/plugins/operation...

Ah. Thanks for clearing that up. I am confused no more. :-)

This is an FYI update. The patch in #54 applies cleanly to the current 7.x-3.x-dev branch, applies with some fuzz to 7.x-3.5, and still addresses the issue (at least for me).

I have the problem that when I want to bulk change "Order status" of Ubercart orders it doesn't change the status anymore. I have VBO 3.3 and Drupal 7.65. When I upgrade to VBO 3.5 (also tried VBO 3.4 first) and I select a few and try to bulk update the order status I get the error "Please select at least one item.". So I think this issue still exists.

iam facing another proplem that when iam with admin account i can execute the bulk opperation and can change the state of the workflow... but if i give the permission to the manager role it do not work .... and the document stay in the same state ..
so is it possible that there are some intersection of other permissions to be given to the role .....

views_bulk_operations-no_default_action-2254871-54.patch doesn't work.

@joelpittet https://www.drupal.org/project/views_bulk_operations/issues/2542120#comm...

Please review the patches proposed in #2254871: Default action behaviors in getAccessMask(), which should solve this issue

How is this 54 patch supposed to work with "Pass ids as arguments to a page" VBO action? I tested this https://www.drupal.org/project/views_bulk_operations/issues/2542120#comm... and it works and probably the https://www.drupal.org/files/issues/vbo_set_behaviors_in_admin_use_multi... would also work.

Could you plz try the "Pass ids as arguments to a page" VBO action with anonymous user and views_bulk_operations-no_default_action-2254871-54.patch?

Thanks