403 error if you use VBO in group related views

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

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.

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?

Graber → closed merge request !83

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 provide(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.