Views permissions not respected by TVI

I just tried to replicate this using a vanilla view for TVI with the path taxonomy/term/%, and setting my vocab to this TVI. The minute I change access on the view to an elevated role, or a different permission than published content, my anonymous user gets access denied on the term path. When I set it back, anonymous can see it again.

Despite that, I see where this was introduced.

There was a change a while back that removed use of views_embed_view in favor of an OOP approach:

return views_embed_view($view_name, $view_id, $taxonomy_term->id());

Became:

Views::getView($view_name)->executeDisplay($view_id, $view_arguments);

The one thing views_embed_view does, is check the access after getView. If there is no valid view or the current user does not have access to it, views_embed_view simply does a return. Otherwise, a renderable array is returned.

I likely encountered this and made the change because a controller must return either a Response object or a renderable array. This means that in the event that:

• Somehow, an invalid view was requested
• Or, TVI config contained reference to a view that does not exist anymore
• Current user had no access

Using view_embed_view would return nothing (in my opinion, it should return access denied - I am guessing it does not do this because of non-page type Views displays?). This means I would have to add more code doing return checking before returning from this already complex method, which to me is not desirable.

Before we make changes, could you try to replicate and tell me more about your View config itself so I can attempt to replicate?

Here is a potential solution:

    $view = Views::getView($view_name);

    if (!$view || !$view->access($view_id)) {
      throw new AccessDeniedHttpException();
    }
    
    return Views::getView($view_name)->executeDisplay($view_id, $view_arguments);

This now raises some other concerns. Assume someone is not using a page display to run their TVI output - now the entire page the block is on will show access denied. Perhaps there should be a case for narrowing the field of functionality of TVI to only work with certain display types. Specifically, since we want to respect permissions in Views, then it might make sense to do.

I still can't replicate the original issue, yet.

Oh, yeah, the use statement. I didn't commit or spawn a patch because, for folks using block displays, this will totally hose their page the block is on.

I am not sure I have ever used a contextual filter for a TVI view.

Have you tried to change the access in the view to say, administrator only, and then look at the TVI output as anonymous? I thought for sure I was given access denied.

Okay, I am attaching a patch, but I think that if this is the direction, then it warrants the discussion in #2760871: Simplify TVI.

Should be $view->executeDisplay.... but I am not totally sold on this still.

I just pushed the start of a series of functional tests that test TVI under different scenarios.

http://cgit.drupalcode.org/tvi/tree/tests

Before I go too much further, I'd like to get a consensus on how this particular issue should be resolved.

In order for Views permissions to be respected, it seems to me that you could really only do that by checking the view display plugin and whether or not it is page level. What I mean is, for the Views 'Page' display, you'd want to throw new AccessDeniedHttpException() - because the page output is controlled by Views. That isn't the case for other things, like block displays, where you'd expect the block to just not show without blocking access to the page it is set on.

Perhaps there is a better way of determining that, but I am not seeing how you could support the case of any plugin, and know when to throw access denied, and when to let it pass through. Perhaps I am also thinking about it a little too hard. However, tests should be supplied that go through a variety of configurations of TVI, but access should be respected of Views as well.

Is there a way of determining what to do via ContextAPI or other new Drupal 8 feature?

Any update here?

Here is the result of applying #13 on last dev:

patching file src/Service/TaxonomyViewsIntegratorManager.php
Hunk #1 succeeded at 10 with fuzz 2 (offset -4 lines).
Hunk #2 FAILED at 166.
1 out of 2 hunks FAILED -- saving rejects to file src/Service/TaxonomyViewsIntegratorManager.php.rej

Just to confirm that #13 failed on 8.5

Here is a re-roll of patch #13 that works with the latest TVI dev (at 1.0-beta2).

Hello,
I confirm that #19 apply on last dev!

Looks like #19 doesn't apply on last dev

patching file src/Service/TaxonomyViewsIntegratorManager.php
Hunk #1 succeeded at 9 with fuzz 1 (offset -1 lines).
Hunk #2 FAILED at 168.

Here is the rerolled patch...

kevinquillen Comment I think need return AccessDeniedHttpException
1. TVI override route for term entity.
2. This new controller should return response for all route

I do not think @kevinquillen is activly working on this, and feel free work on issue. Based on comments in #24 - back to needs work. Since permission is important do I bump this to major.

@steinmb, thanks for the review.

I was thinking more about this approach in #3207891: Release 8.x-1.0-rc3 and I think that its important not to break live sites, which are using this module, so we should probably include a permission, and an update hook to grant it by default, that allows access to bypass view access on tvi pages, so that current sites will continue to work regardless of the status. I would default the permission to off on new installs though, since the module should provide security by default.

Otherwise, I think the current approach is good, with the limitation that it needs to support all view displays, at this time, without causing any poor experience for the user (Error messages/WSODs). If we are talking permissions, I think its also important that we verify that term access is enforced as well, TVI shouldn't expose access to the view for a term which the user can't view. I believe that should be covered by core's access control system though.

Here is a quick re-roll of #22 that should apply to the rc3 for anyone using the previous patch.

edit: There are a few issues with this patch and it doesn't address #26 so please ignore it.

Should there be some functional tests for this change to account for the concerns in 26?

Here a correction of #27 which didn't work on 8.x-1.0-rc4

@Soubi, thank you for the patch, works perfectly 🌿

#30 works like a charm.
Thank you

Moving this to the 2.x branch to be reviewed. (Will backport to the 8.x-1.x branch if nessesary)

This is still an issue that has not been fixed?