Views argument validators aren't taken into account on access checks - local task point to 404

Yeah I think argument validators should be able to allow something similar to what \Drupal\views\Plugin\views\access\AccessPluginBase is doing.

Yeah, I just debugged a similar issue on a project, we ended up going with a route subscriber to add additional access checks. This would be a great addition.

This would be a very important addition for us too. Looking fwd to this !

I'm taking a stab at this.

I think this is close to what we need. Unless I'm missing something, a generic solution fell into place quite nicely once I figured out how.

A few notes and things that might require some discussion or confirmation.
I made it only apply when the fail validation behavior is set to "page not found" or "access denied". If someone really wants to display one of the others, i.e., all results a "no results" message or a summary then the menu should probably still appear.

This applies to page and feed displays because Drupal\views\Plugin\views\display\PathPluginBase::getRoute was a good place to put this. If feeds is considered inappropriate then I can move some code to Drupal\views\Plugin\views\display\Page although I'd have to refactor a little to make $argument_map available.

I'm still a little uncertain about what AccessResult is the correct one in the various situations although I think I have it correct mostly by studying what happens on a view with an access restriction. AccessResult::neutral() makes the menu link disappear.

I think AccessResult::allowed() is the appropriate result when we're trying to access the view itself although intuitively AccessResult::neutral() would seem more likely. AccessResult::allowed() appears to work correctly. If the view has a failed access restriction and a validation, it still fails with access denied as intended despite my code returning a AccessResult::allowed();

I will look into writing some tests. I think I need to import a new view configuration to test this or at least add a display to one of the existing test views.

Added a test.

I was feeling my way a little with the test. I'm not sure if it's in the most appropriate place or should / could do something different.

Fixed Some Coding Standards which was in the previous patch.

Interdiff was not created due to an error.

Thanks. Here's an interdiff.

Trying to bring this back to life.

Patch #16 fails to apply to 8.7.x.

I'm starting again with the patches since I didn't do it in the right order a year ago.

This one is the test only which should fail.

Submitting this again with the updated traits.

This should pass.

Thanks @catch. I've addressed all those except the last one.

The route requirement can only be a string so we need the serialization. It looks like Symfony really intends it to be a regex (See Symfony\Component\Routing\Route::sanitizeRequirement) so I am kind of hijacking it for a different reason. But there are other places where it's pipe delimited "serialized" such as in Drupal\rest\Plugin\views\display\collectRoutes::RestExport. I'm not sure if there is an alternative. This is deeper than one dimension so a pipe delimited won't work.

I have changed to using ->setRequirement rather than ->addRequirements which eliminates one array.

Removed an unnecessary comma.

Major as per #2489932-7: View argument validators are not taken into account in access checks.

I just tested this today and it worked well. However, I can consistently get a PHP notice under certain circumstances, and I'm pretty sure it's related to the patch in #28.

I wish I had more time to dig a little deeper, but here's what I have so far.

I'm using two views with argument validation criteria. Both are listing custom entities.

The first one has a path of `/node/%node/slots`, uses a menu tab, and uses the 'Content' validator with a single content type selected. With the patch, everything works as expected. The tab appears on the nodes of the content type chosen in the validator and not on other node types.

The other view has a path of `/user/%user/reservations`, also uses a menu tab, and uses the 'User ID' validator. With the patch, the tab still appears on user pages (e.g. `/user/2`), but on every visit to the account page (`/user/%user`) after a cache rebuild, I see the following PHP notice:

Notice: Undefined index: bundles in Drupal\views\Plugin\views\argument_validator\Entity->validateEntity() (line 202 of core/modules/views/src/Plugin/views/argument_validator/Entity.php).
Drupal\views\Plugin\views\argument_validator\Entity->validateEntity(Object) (Line: 96)
Drupal\user\Plugin\views\argument_validator\User->validateEntity(Object) (Line: 180)
Drupal\views\Plugin\views\argument_validator\Entity->validateArgument('1') (Line: 73)
Drupal\views\Access\ArgumentValidatorAccessCheck->access(Object, Object)
call_user_func_array(Array, Array) (Line: 159)
Drupal\Core\Access\AccessManager->performCheck('access_check.views.argument_validator', Object) (Line: 135)
Drupal\Core\Access\AccessManager->check(Object, Object, NULL, 1) (Line: 92)
Drupal\Core\Access\AccessManager->checkNamedRoute('view.reservations_for_user.page_1', Array, Object, 1) (Line: 327)
Drupal\Core\Menu\LocalTaskManager->getTasksBuild('entity.user.canonical', Object) (Line: 358)
Drupal\Core\Menu\LocalTaskManager->getLocalTasks('entity.user.canonical', 0) (Line: 94)
Drupal\Core\Menu\Plugin\Block\LocalTasksBlock->build() (Line: 203)
Drupal\block\BlockViewBuilder::preRender(Array)
call_user_func('Drupal\block\BlockViewBuilder::preRender', Array) (Line: 378)
Drupal\Core\Render\Renderer->doRender(Array) (Line: 450)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 195)
Drupal\Core\Render\Renderer->render(Array) (Line: 490)
Drupal\Core\Template\TwigExtension->escapeFilter(Object, Array, 'html', NULL, 1) (Line: 83)
__TwigTemplate_491d354868c23a7cdc36790829dd9700797f10bbfee01afc733f74972d1a5ac2->doDisplay(Array, Array) (Line: 432)
Twig_Template->displayWithErrorHandling(Array, Array) (Line: 403)
Twig_Template->display(Array) (Line: 411)
Twig_Template->render(Array) (Line: 64)
twig_render_template('core/modules/system/templates/page.html.twig', Array) (Line: 384)
Drupal\Core\Theme\ThemeManager->render('page', Array) (Line: 437)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 195)
Drupal\Core\Render\Renderer->render(Array) (Line: 490)
Drupal\Core\Template\TwigExtension->escapeFilter(Object, Array, 'html', NULL, 1) (Line: 84)
__TwigTemplate_4a0387b265d067bab2d2de06a8ead777749d03cb5cf7ba23df40bc8d00a80437->doDisplay(Array, Array) (Line: 432)
Twig_Template->displayWithErrorHandling(Array, Array) (Line: 403)
Twig_Template->display(Array) (Line: 411)
Twig_Template->render(Array) (Line: 64)
twig_render_template('core/modules/system/templates/html.html.twig', Array) (Line: 384)
Drupal\Core\Theme\ThemeManager->render('html', Array) (Line: 437)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 195)
Drupal\Core\Render\Renderer->render(Array) (Line: 147)
Drupal\Core\Render\MainContent\HtmlRenderer->Drupal\Core\Render\MainContent\{closure}() (Line: 582)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 148)
Drupal\Core\Render\MainContent\HtmlRenderer->renderResponse(Array, Object, Object) (Line: 90)
Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray(Object, 'kernel.view', Object)
call_user_func(Array, Object, 'kernel.view', Object) (Line: 111)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.view', Object) (Line: 156)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 68)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 99)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 78)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 52)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 666)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

I don't see the error when I revert the patch, which is why I'm guessing it's related. It's interesting that the error is about an undefined index `bundles` and that user entities don't have bundles.

I'll dig around some more, but I'm hoping that this info will help.

Thanks for testing. I've tried to reproduce the error but have not been able to.

Can you describe your second view in detail please.

@jeffam I took another look trying to reproduce your error but was unsuccessful.

I created a custom entity with Drupal Console I think similar to your Reservation. It just has a name and a user reference. I created a view at /user/%user/foo to show the reservations for that user. Then I used the contextual filter validator to validate by role. "Restrict user based on role" is the only validator available when you validate by User ID. The tab appears as expected at /user/2 but not at /user/1.

It that similar to what you were doing?

It seems like somewhere in your view you're validating something by bundle, but what? Does Reservation have bundles? Mine doesn't. It would be nice to see the yml for the view or just a description of it.

I'm at a dead end with this now without more details on how to reproduce the error.

@tetranz - Thanks for looking at this, and sorry for my delayed response. Attached is the view I'm using, although it has some extra stuff that probably isn't relevant. You'll be able to see the `reservee_uid` argument and its validation settings, though.

Thanks @jeffam, I can reproduce the error. I should have a fix in few days.

Unfortunately, this is not quite as simple as I thought it was.

This patch fixes the problem but it is probably not the final solution.

The problem was caused by me not initializing the plugin properly. I was just setting the options which means that it wasn't getting the defaults which include that 'bundles' value.

The proper way to initialize the plugin is to call its ->init() method. That works fine except that it needs the view executable and the display handler. We really don't want to load the view as @catch said in the proposed solution.

I'm not sure what to do. I think we really only need the default options for the plugin but any way of getting at them directly is protected.

Anyway, I'm going to flip this to needs review. Perhaps someone will have a suggestion.

This patch prevents showing taxonomy terms in menu. Steps to reproduce:
1. apply patch
2. create taxonomy term
3. add link to taxonomy term to menu
4. see nothing.

RE: #36. Because this patch triggers on views with 404 or 403 results, a workaround is to change the contextual filter on your taxonomy term view to display contents of "no results found" or any other setting besides "page not found" or "access denied".

Views menu tabs are a key strategy in producing usable UI/UX workflows for complex Drupal architectures with lots of entities referencing each other. This regression from D7 is detrimental to that strategy because the views tabs appear on content types where they should not appear.

The patch in #35 stops applying w/9.3.

Attached is a re-roll that applies for 9.4.x and 9.3.x.

Note: No changes to functionality or a review, just a re-roll.

Somehow uploaded an empty patch in #45.

Here's the proper version.

Great work @tetranz.

The problem described in #35 is tricky. Here's a possible solution:

1. Create an ArgumentValidatorPluginInterface that extends ViewsPluginInterface and contains a method initOptions($options = NULL).
2. Have ArgumentValidatorPluginBase implement that interface, with initOptions simply being a version of init that sets up the options without needing the view and display.
3. When checking access ignore validator plugins that do not implement ArgumentValidatorPluginInterface
4. In ArgumentValidatorPluginBase::construct trigger a deprecation warning if the plugin does not implement ArgumentValidatorPluginInterface

This is a tricky architectural issue that may have relevance beyond this issue; maintainer guidance would be helpful.

Reroll patch #46 for 10.2.x

Also recommend converting to MRs as easier reviews.

Still needs sub-maintainer review but NW for the CC failures.

I just ran into this bug on a new project. I was amazed this was broken. I dug deep into the views code, only to find:

// @todo How do we apply argument validation?

buried in core/modules/views/src/Plugin/views/display/PathPluginBase.php.

Searched the d.o queue and landed here. Thankfully, patch #52 applies cleanly both to 11.x HEAD and my 10.2.5 site. Converted to an MR. Fixed the phpcs and phpstan errors. I haven't addressed #35 / #47, but wanted to get this back into a reviewable state.

Let's smash this bug!

Thanks,
-Derek

Hiding all attached files and starting to save credits.

Well, drat. There are legit test failures in both of these:

core/modules/system/tests/src/Functional/Menu/BreadcrumbTest.php
core/modules/taxonomy/tests/src/Functional/TermTest.php

I'm out of time for today, but I hope to get back to this soon. 😅 If anyone else wants to run with it in the meanwhile, please do. 🙏

Thanks,
-Derek

@dww you may be encountering #35 / #47

Just tidying tags

Reroll patch #52 for 11.2.2
Edit: My bad, uploaded wrong file. See comment below.

Something went wrong with previous comment.
This is the correct reroll patch #52 for 11.2.2

#63 seems to be rerolling #58, which is NR not NR.

The MR doesn't apply anymore on D10.6. I created a patch which will apply on D10.6