Deprecate GroupPermission access plugin in favor of an argument_validator so it works out of the box with blocks and modules such as VBO

Hi chmez,

Thanks for your hard work.

I believe instead of using:

+/**
+ * Implements hook_views_plugins_field_alter().
+ */
+function gvbo_views_plugins_field_alter(array &$plugins) {
+  $plugins['views_bulk_operations_bulk_form']['class'] = 'Drupal\gvbo\Plugin\views\field\GroupViewsBulkOperationsBulkForm';
+}

Which would override all views_bulk_operation_bulk_forms

It would be better to write your own implementation of a plugin as such:

/**
 * Implements hook_views_data_alter().
 */
function gvbo_views_data_alter(array &$data) {
  $data['views']['group_views_bulk_operations_bulk_form'] = [
    'title' => t('Group bulk operations for Group Content'),
    'help' => t("Process Group Content returned by the view with Views Bulk Operations' actions."),
    'field' => [
      'id' => 'group_views_bulk_operations_bulk_form',
    ],
  ];
}

And update the Annotation accordingly:

/**
 * Defines the Groups Views Bulk Operations field plugin.
 *
 * @ingroup views_field_handlers
 *
 * @ViewsField("group_views_bulk_operations_bulk_form")
 */
class GroupViewsBulkOperationsBulkForm extends ViewsBulkOperationsBulkForm {

This would make sure this plugin becomes available as a field in the views_ui, giving more flexibility.

Still not perfect, but gave my comment in #6 a shot

Whitespacing issues

Hereby a patch against the latest dev and one which should work with rc4 as well.

Setting issue to "Needs review".

I've attached a new patch file which should solve Error: Class 'Drupal\group\Context\Group' not found in Drupal\group\Context\GroupRouteContext->getGroupFromRoute() (line 74 from the previous patch.

This is a much lighter touch. And seems to serve about the same purpose. No interdiff since this is a radically different approach.

add to #12 support for ajax and all items in view selection.
combining both approaches. #12 was not working for all items in view selection - this is due to

$view_data['redirect_url'] = Url::fromRoute('views_bulk_operations.execute_batch', [
        'view_id' => $view_data['view_id'],
        'display_id' => $view_data['display_id'],
      ]);

in ViewsBulkOperationsBatch::getBatch

adding a class that is hardcoded in vbo js logic.
current views field name fails to pickup vbo js, it searches for

 @ViewsField("views_bulk_operations_bulk_form")

we have

 @ViewsField("group_vbo_bulk_form")

Re-rolled for latest release of VBO using new private storage service.

Here's an even lighter-weight approach that doesn't require a sub-module, a custom field plugin or custom routes.

When Views Bulk Operations redirects to the confirm form, it places some meta-data in the private temp-store.

The temp-store collection is named after the view and display ID, which we have in context here because this is a views plugin, and the access method is called after init - which initializes the view ID and display ID.

Included in this meta-data are the arguments that were originally used to build the view - if one of those uses the group_id plugin, we can take its value and use that to determine the group that was used to generate the view prior to redirecting.

Once we have the group, we can perform the permission check in the exact same fashion as the code that uses the global context provider.

All up this is 16 lines of actual code and 40 odd lines of boilerplate for injecting the services required to work with the temp-store.

I think this lighter touch is much more likely to be accepted by the maintainer, so I'm uploading it here. There's no interdiff because this is a do-over from scratch.

If people would prefer to keep the existing approach, let me know and I'll open a separate issue and link back to it from here - but I think this is ok - we are trying to solve the same problem.

Also needs to work on the AJAX selection route.

I tried the patch in #17 and it does get past the Access Denied on the confirmation screen, but when confirmed it errors out with the message:

An AJAX HTTP error occurred.
HTTP Result Code: 500
Debugging information follows.
Path: /batch?id=27&op=do_nojs&op=do
StatusText: Internal Server Error
ResponseText: The website encountered an unexpected error. Please try again later.Drupal\Component\Plugin\Exception\PluginNotFoundException: The "" entity type does not exist. in Drupal\Core\Entity\EntityTypeManager->getDefinition() (line 150 of core/lib/Drupal/Core/Entity/EntityTypeManager.php).

Might be #3161707: hook_query_TAG_alter does not handle empty MetaData

Yes, that was it. The patch at https://www.drupal.org/project/views_bulk_operations/issues/3163912 fixes it.

Without adding the views_bulk_operations.execute_batch route, this fails to work on actions that aren't configurable.

Can there not be a check on $argument->getPluginId() === 'group_id'?

I have a view that is a list of users, with an action to add them to the current group. So the first argument is a group but it's of type "global".

Could it check the route parameter instead?

Re-rolled patch 12 for VBO 4.x
Would be nice if someone summarised this issue and chose the preferred solution, probably best if it was the maintainer.

Checked #16 and #24 patches. Both work as expected.
I would go with #16 as it does not requre switching field handler.

Well, we might need to address #23.

Patch #22 worked for me with Group 8.x-1.4.

Raised to major because Group related views which using VBO are not working as aspected.

Maybe at some later point or in 2.x we could improve this views access related stuff in general, because #2942657: 403 error for views.ajax route on Group related views (with AJAX enabled) is also related to views access, but used another approach.

Sigh.. This is missing the `entity_target_id` plugin as well, it doesn't have to be `group_id`. Also I'll set those arguments on the view in VBO, will push to 4.1.x soon so this'll not contain any VBO-specific code.

I hope the Group maintainer will take a look then.

Forgot to add the entity type manager.

This now works with latest dev VBO. Will work with stable after the next release.

#31 looks like a good improvement compared to #24, manually tested this and it works well.

Small change - $group_id might not always be sent, found via views UI.

#34 works ok for me.

It's the equal to #34 patch, but without a warning.

https://www.drupal.org/files/issues/2023-03-07/3020883-35.patch

Adjusted Patch:
If group can neither be resolved by path nor by context, it is fetched by views argument if the argument is of plugin "group_id".
Caveat: If there are multiple group arguments, this would only check for the last. In that case the other solutions might have already gotten a valid group.

Hi all, facing the same problem, is there a patch on D10?

Hello, community!
Have the same issue, any updates?

Cross-posting from #group on Drupal Slack:

If you create a MR against 3.3.x and add a test to prove this works, I can accept it. I'm seeing a few potential scenarios in that patch so all of these cases should ideally be proven to work.

Also saw this in a comment above:

Caveat: If there are multiple group arguments, this would only check for the last.

The first loop breaks on first occurence found, the second loop uses the last occurence found. Please make this consistent, probably by making the second loop "break" when something is found.

P.S.: We have some tests starting from GroupViewsKernelTestBase, so you could do the same. Turns out we did have tests for some Views plugins, but not this one yet.

Okay before any work is done, I'm seeing an opportunity to fix this without having unpredictable loop results in Group.

Currently we have:

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
    return new static(
      $configuration,
      $plugin_id,
      $plugin_definition,
      $container->get('group.permissions'),
      $container->get('extension.list.module'),
      $container->get('group.group_route_context') <---- This is bad m'kay?
    );
  }

What if we change the configuration form to allow you to choose a context provider and make it default to group.group_route_context to preserve BC? This way, you can make your own context provider that either gets the group from the view args or from whatever else you can think of. It would also instantly make this work with the Group Sites module.

The only question is whether you can reliably get a view object from a route, i.e.: Answer the question whether the route represents a view and, if so, which view that is. If you can do that, then the solution above would be far superior than the current patch.

You can find an example of a form element asking for a context provider in the Group Sites module: https://git.drupalcode.org/project/group_sites/-/blob/1.0.x/src/Form/Gro...

Moving this to VBO, needs quite some refactoring but It'll be a big step forward for VBO so definitely worth an effort.

This is big unfortunately and will need a (sub) major release. No BC issues expected though.

We need to get back to the primary solution unfortunately - the "great improvement" will make VBO useless on block displays, those dedicated routes were needed.

@Kristiaan, general thoughts: context - based access is something that should be avoided as it creates ways to bypass restrictions - an individual either has access or doesn't and not maybe has and maybe not, so site builders can accidentally leave some doors open and create security issues. Also, it seems bad that group prevents access to routes without group context as usually in cases outside of some module scope neutral would be returned. Can we just return TRUE if no context is available?

That basically ;)

I agree with @graber, Patch #51 "ignores" the group permission when the group is not in context. Modules like VBO has to provide group context on their own if they want to provide group permissions.

Is #51 considered the current solution to this problem? I can confirm it fixes the problem I am having:

When I select (via checkbox) an item on a VBO enabled view, I see 403 in the network tab for

/views-bulk-operations/ajax/my-view-that-uses-parent-group-as-contextual-filter?_wrapper_format=drupal_ajax

I assume because the group id is not present.

I follow the logic of the fix in #51, I just do not have a deep enough understanding of Views, VBO or Group modules to know if this will create unwanted side effects.

Okay so I've given this some more thought and I don't see why a context provider wouldn't work. By default we can use "Group from URL", which behaves 100% like the current code does. With all the same upsides and downsides.

But now you could write your own context provider that defaults to "Group from URL", but falls back to whatever you like. For example, you could first check the route and, in absence of a group entity, see if VBO is installed and try to get the group from the information in the temp store, like @larowlan suggested in #17.

Re #49:

context - based access is something that should be avoided as it creates ways to bypass restrictions

The access wouldn't be based on the context, it would still be based on whether you have the right permission in a Group entity. All that changes is that you have more power to tell Drupal which Group entity you want the permission to be checked against.

- an individual either has access or doesn't and not maybe has and maybe not, so site builders can accidentally leave some doors open and create security issues.

Not sure I can fully follow here. Isn't it always the case that site builders can misconfigure their site if not careful?

Also, it seems bad that group prevents access to routes without group context as usually in cases outside of some module scope neutral would be returned. Can we just return TRUE if no context is available?

Sadly no, because that would actually increase the risk of showing data that someone shouldn't see.

Now I'll admit that in most cases you could argue that, if the route does not contain a Group entity, the view will probably show nothing and then we could allow access. But as demonstrated in the VBO scenario, sometimes the route doesn't know which Group to deal with even though it should. In that case it seems far safer to block anything from happening further than allow perhaps unpredictable code to run. I would imagine that creating a bunch of group relationships without a group to point to can lead to crashes.

Either way, I'll pick this up today and see what I can come up with. I'm very much a fan of a solution which takes guessing completely out of Group and into the responsibility of those who have certainty about their environment. Be it the VBO maintainers, myself (with a VBO support submodule) or developers working on a specific client project with custom Group detection logic.

I think going back to #37, forgetting about everything after and resuming from there may be a good idea ;)

I have plans for Group 4 to rely more on contexts. Group Sites already heavily does and it's been working great there. So I'd like to explore an option more in line with that to make everything work more nicely together in future versions.

The patch from #37 is guesswork. As soon as someone comes along with different needs, we're right back to square one on this discussion. I'd like to fix this in a way that I can tell anyone with slightly different requirements to "just write a context provider" that contains their requirements. Then we can close this once and for all.

Do you want your views to work with Group Sites, which detects the "active group" from domain, path, or whatever you want? Great they will work with that. Do you want them to work with VBO instead? Also possible...

I'm trying to see how this approach is inferior to or less flexible than the patch in #37. Could you please provide more detail? Am I missing something?

Okay so the groundwork seems to be working. Great.

An even better approach would be to allow a list of context providers to be selected that will run in the specified order until a group is found. Then you can reuse "Group from URL" without having to copy its code into "Group from VBO". It would lead to less copy-pasted code all around.

If we switch to that, then I could write a small support module that contains the code from #16 and people could configure their permission plugin to use "Group from URL", followed by "Group from VBO" if they so choose.

I'll try that next. It would fully solve this issue, open up Group's view plugins to work with contexts and make any future request far easier to fulfill "from the outside".

Yes, just.. let's not treat this as VBO-only issue. The context provider should be something like "Group from the current View arguments", otherwise one day we may end up with a similar issue but for a different module that calls View access() method.

Oh yeah, we can provide whatever we want. That "whatever" should be part of this MR, though, so that we can consider the reported issue here fixed.

Either way, attached screenshot shows the progress I've made today. Seems to work rather well, but still need to write a CR and put the proper link in there. Also, tests would be nice.

@graber and I were talking on Slack and I think we can incorporate the patch from #37 into the current MR.

Let's say you have a view where you want one (or a few) context provider(s) to be checked, but on certain pages you want said view to be used inside a block with a hard-coded group ID as the view argument. It would make sense that the view argument were evaluated before the context providers then.

So with that in mind, we should add a checkbox on the config form allowing people to opt into this behavior and explaining that, when checked, a manually provided view argument will always take precedence over the context providers. I'll try and work on that tomorrow.

It also occurred to me that, once this lands, we should also open up a follow-up to allow the same context provider selection in the contextual filter modal. It makes very little sense to be able to choose which context provider to use for the access check, but not the actual views argument.

Okay, so I gave this some more thought and I want to document some findings for posterity:

Q: If we're going to allow the GroupPermission access plugin to use the value from the argument (contextual filter), then why don't we put the new stuff where you choose a context provider inside the views argument? Then we can convert the access plugin to always use the argument instead.

A: Because the group won't always be in the contextual filters. Imagine a scenario where you can publicly view articles, but if you're a member of whichever group the article belongs to, you can also see whatever annotations the editors made in a Views block in the sidebar. Here, the contextual filter would be the node ID, but the GroupPermission plugin would have to be configured with the "view annotations" group permission on the group provided by a custom context provider ArticleParentProvider or whatever.

So for that reason I've chosen to keep the work I did and combine it with the work from #37, as previously explained in #61.

So, even though nothing changes, I'm writing this down in case other people have the same idea and wonder why I didn't offload the GroupPermission plugin's group negotiation to the views argument entirely.

Okay so I did some debugging and found that during an access plugin's lifespan, the view is far from initialized as it saves a lot of resources to not calculate or render a view you don't have access to. But we do need some of the view to be loaded to know which contextual filters were configured.

The latest commits do just that: If, and only if, the GroupPermission plugin is configured to also check the view argument will we load the handlers for the provided display ID and see if any of them use our GroupId ViewsArgument plugin.

This all relies on $this->view->element being populated, which is usually the case when access() is called. But I have a feeling if anything about this approach is fragile, it's that little part. So curious to see what VBO does with $this->view->element, for instance.

Either way, needs an update hook to fix our default views to actually use the group_id plugin ID instead of numeric.

Also, will fix composer in a standalone issue on Monday. Don't need tests to run just yet anyway.

This all relies on $this->view->element being populated, which is usually the case when access() is called. But I have a feeling if anything about this approach is fragile, it's that little part. So curious to see what VBO does with $this->view->element, for instance.

It seems like \Drupal\views\ViewExecutable::$element is only present when "rendering" a view (e.g. on page load). It's not populated at all when just instantiating a view executable to only do things like invoke access plugins (e.g. VBO). Also, the args are stored in different places in both scenarios, unsure how to universally handle this without checking in two places.

When the view is loaded (page load), \Drupal\views\ViewExecutable::$element is populated from \Drupal\views\Plugin\views\display\DisplayPluginBase::buildBasicRenderable as the response for \Drupal\views\Routing\ViewPageController::handle (a render array). By the time execution reaches \Drupal\group\Plugin\views\access\GroupPermission::init, both \Drupal\views\ViewExecutable::$element['#arguments'] and \Drupal\views\ViewExecutable::$element['#display_id'] are present.

However, when checking a box in VBO, the view is initialized from \Drupal\views_bulk_operations\Access\ViewsBulkOperationsAccess::access and uses \Drupal\views\ViewExecutable::setArguments to set the arguments. That method puts the arguments in \Drupal\views\ViewExecutable::$args. It's not rendering the view so neither \Drupal\views\ViewExecutable::$element['#arguments'] or \Drupal\views\ViewExecutable::$element['#display_id'] are populated.

Found out that in the case of a page load \Drupal\views\ViewExecutable::access is invoked before \Drupal\views\ViewExecutable::setDisplay and \Drupal\views\ViewExecutable::setArguments. So you really only have \Drupal\views\ViewExecutable::$element to work with from inside the access plugin.

But in the case of VBO, it's the other way around. \Drupal\views\ViewExecutable is instantiated, \Drupal\views\ViewExecutable::setArguments is invoked, then things like access plugins are invoked after.

So there's some call order difference happening here.

Rerolled Patch #37 to work with Group 3.3.5.

I attempted to use a patch made from the latest merge request. The patch applied successfully but Drupal\group\Plugin\views\access\GroupPermission::access() was still not getting the group context when using Views Bulk Operations. In my case it was lost when you go to the VBO route views_bulk_operations.execute_configurable to configure the bulk operation.

I tested the solution and it works.

For those, who wonder why after applying the patch you still have issues with VBO. You need to introduce your own context provider like GroupRouteContext and very important you have to enable it in the settings of "Group permission" access plugin of the view.

I noticed a few issues:

1) We replace plugin to group_id for group_nodes and for group_members, but hook update is provided only for group_members

2) For the views in active configuration, if we are missing context_providers section, then settings form is never displayed. I believe we need to provide an update hook to set @group.group_route_context:group context provider by default for these views.

      access:
        type: group_permission
        options:
          use_view_argument: true
          context_providers:
            - context_id: '@group.group_route_context:group'
              enabled: true
              weight: -25
          group_permission: 'view group'

I will handle these cases

Ok, sorry for the noise, I have many version of groups currently. I don't see any issues! Everything works as expected

I have merged the latest changes from 3.3.x. Let's bring it in. Should I create another MR for Group 4 ?

Merge request 243 works as advertised. And as mentioned in #71 views need to be updated to include the group context. Plus 1 on getting this in! :)

Just came back to this issue to see if I can commit it, but found out that @fskreuz force pushed over the history. Please don't do that. It messes with my review workflow and makes it far more time consuming.

Right, I've separated out the logic introduced by @fskreuz into a new method that can bail out early. I've gone ahead and force pushed again because I CBA checking if the history wasn't altered and I don't want to have a supply-chain attack on my hands.

For that we need another round of review.

I will add credit for @fskreuz, because their work was valuable and definitely contributed to making this MR more stable. Even though the history no longer shows the commits as theirs due to the double force pushes.

Tests go green as far as I'm concerned.

Group2to3UpdateTest is broken by a core update and I need to figure out why and the new phpstan reports need to be fixed in a dedicated issue.

All green now that Group2to3UpdateTest was fixed in another issue.

I think this looks good, however - only actual context provider on VBO side will fully verify.
Next steps - VBO issue:

1. Additional context provider that takes group from tempstore with minimal test coverage (Kernel)
2. Update hook that runs on all views and sets the context provider where VBO field is present and it's a group view

Thanks for working on this!

Okay so we have a green v4 MR now and an RTBC v3 MR. Going to give this one more final pass when I have more time and attention and then commit it.

I want to make sure I am 200% onboard with the taken approach (even though I wrote most of it) before I commit this and have to maintain it. Group v4 and up will rely heavily on context providers, so this one has to be right.

Okay, think I may have found a fix that works for everything. An "argument_validator" plugin that replaces the current 'access" plugin. You can deny access from an argument validator just the same, but it always has the argument value.

So no more guessing, no more wiring up contexts with fallbacks, just simple:

1. You create a view where you need a group permission to be checked
2. You add a contextual filter for a group argument
3. You add our new validator and select the permission you want
4. You set "Action to take if filter value does not validate" to Display "Access Denied"

The upside is that blocks can choose what value to feed into it using context providers (what I wanted in the first place). Views that do not have a %group part in their URL can then specify a default argument which fetches it from a context provider also. So from now on we can create views that either get a group from the URL or from a context that work both as full views and as blocks. Permission checks happen on the argument and as a bonus, can provide the necessary cacheable metadata.

This is huge to be honest. Although the UX will suffer a bit because we no longer have a regular access plugin we can offer. We could still offer one, but have the form be nothing but a set of instructions on how to achieve this.

The question then becomes, how do we migrate from the old plugin to the new approach in an update hook without messing up people's views? For BC reasons we should keep the old behavior for now, but put a fat warning saying it's deprecated. I can live with this staying until v5 to give people time to adapt.

Will open a new MR with this work Soon™ for comparison. We can keep the old MR in case I forgot something important here.

This is what it looks like with an argument validator.

Feels way more logical, given how the old access plugin always needed something "contextual" to work with anyway.

This is what the old plugin looks like now:

I just tested this with VBO and it works out of the box. So closing the other MRs in favor of this one.

What this could still use is:

1. A few tests to prove that both the old approach (access) and new approach (argument_validator) work
2. A test for moving from old to new to check if form is disabled
3. A hook_requirements to find views with the old approach and flag them
4. A test for the hook_requirement