Exposed term filters should not show term options that the user does not have access to

This is not a bug report, but a feature request.

The fact that you can see unpublished terms as an anon user is a bug in my view and a possible information disclosure. I checked with the security team and they decided not to mark this a security issue, but I still believe this to be something we need to fix ASAP.

So, fix and test for this. It only removes the unpublished terms from the actual exposed filter, not the Views UI. Since there is no 'view unpublished terms' permission, the exact rules about who can see what are a bit fuzzy. But if you can edit Views, being able to see unpublished terms doesn't sound like too much of a problem.

Looks good, just one remark:

+++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
@@ -193,8 +196,13 @@ protected function valueForm(&$form, FormStateInterface $form_state) {
+        if (!\Drupal::currentUser()->hasPermission('administer taxonomy')) {
+          $query->condition('status', 1);
+        }

Should the non-admin user not have the permission "access content"?

@daffie nice catch, that pointed me to an easier solution here then juggling permissions ourselves, just check for view access.

@JannaKha thanks for the review! Much appreciated!

if NSW has child terms (eg Central Coast, Northern Rivers) they will get displayed.

I thought about this and I don't think we should make that decision in Views. If the tree loads the terms and the term says we have access to it, Views should respect that and show it. I totally 100% agree that this can lead to strange results, but it's something the editor can fix (by unpublishing the child terms). If we decide here that they are always hidden, and this is NOT what the editor wants, there would be no way for the editor to fix this.

I like the new approach. Just one nitpick:

+++ b/core/modules/taxonomy/tests/src/Functional/Views/TaxonomyIndexTidUiTest.php
@@ -87,7 +87,7 @@ protected function setUp($import_test_views = TRUE) {
-  public function testFilterUI() {
+  public function ptestFilterUI() {

@@ -134,7 +134,7 @@ public function testFilterUI() {
-  public function testExposedFilter() {
+  public function ptestExposedFilter() {

Why do we rename these 2 tests?

Oops! Debugging stuff so I don't run all the test in the file :D

Removed it.

I am happy now.
The bug has been fixed and a test has been added.
For me it is RTBC.
Thanks everybody!

Small update to the IS to make it more in line with the current fix.

Should this issue be targeted against 8.8.x-dev or 8.9.x-dev?

I checked that the plugin has the correct cache contexts to deal with a user permissions and it appears to

  public function getCacheContexts() {
    $contexts = parent::getCacheContexts();
    // The result potentially depends on term access and so is just cacheable
    // per user.
    // @todo See https://www.drupal.org/node/2352175.
    $contexts[] = 'user';

    return $contexts;
  }

One concern I have is that some sites have a lot of taxonomy terms so are we sure we want to add this to the loop. OTOH as far as I can see there are no additional queries going to be done by \Drupal\taxonomy\TermAccessControlHandler::checkAccess() so maybe this doesn't matter.

Term access by default is less expensive than node access at least when node grants are used, yes. That cache context is already an issue, and we use a patch to remove that because I'd argue that less than 1% of the sites use term access logic that does is not covered by permissions/roles.

That said, the reason this class *already* has the user cache context is because the entity query already ads the taxonomy_term_access tag (which afaik should already be there by default). And just like with nodes, the expectation here is that taxonomy-access modules would implement that query alter hook and add a condition on the term status. So with that argument, this could be closed as a works a designed

Of course, the problem is that core, just like with nodes, doesn't actually implement its own query alter/grants API. Considering that core now does have a term status and access checks, I wonder if a better fix wouldn't be to implement hook_query_taxonomy_term_access_alter(), and add a status = 1 condition if the user doesn't have the administer taxonomy permission. (Which of course could then again break more specific term access modules in D8, but the same could be said of introducing the term status field in the first place).

Of course, the problem is that core, just like with nodes, doesn't actually implement its own query alter/grants API.

#2909307: Consider using the taxonomy term status in taxonomy_build_node_index() seems relevant here.

I think @Berdir's statement that implementing hook_query_taxonomy_term_access_alter() might break "hook_query_taxonomy_term_access_alter" is a strong point in favour of making the change in #9. I think at some point we need to do a major review of entity access and make it consistent but a D8 bug fix is not the place.

Oops - I changed my mind whilst commenting

@JannaKha thanks for having a go at this, but I'm going to roll this back to #9 and set that back to RTBC.

I think the ideas in #18 are good but the scope of that patch is MASSIVE (as indicated by the 45 test fails against the current HEAD). We need to do this in little baby steps or it will never get done (that is just how core development works). Every point you make would require its own issue really.

Let's just keep this focussed on users not being able to view terms they don't have view access to and file follow ups if we want to make this better later.

We need to add the test coverage in #20 back in and we need to add additional test coverage to prove that the changes in #22 are necessary.

Re-upping #9 again, since I still think that this is the way to go here.

As I detailed in #7, I don't think we want the changes in #22 for core, if an editor doesn't want the child terms to be shown, they can unpublish the child terms.

@Lendude: unpublished parent means all children are automatically unpublished even if they are published. That's the logic all my clients are suggesting. I don't agree that the children of unpublished child should be visible to the user with view published content permission.

According to core/modules/taxonomy/src/Plugin/EntityReferenceSelection/TermSelection.php which hides children of unpublished terms, your logic is incorrect.

I'm voting for patch #22.

@VladimirAus the logic that the patch in #9 follows is that the Views module follows whatever the Taxonomy module gives us. If the Taxonomy module decides that child terms of unpublished Terms should not have 'view' access, Views would follow that logic. It is not up to Views to decide what the access logic for Taxonomy terms is.

The fact that core/modules/taxonomy/src/Plugin/EntityReferenceSelection/TermSelection.php has special code for this, is exactly the type of thing I'm trying to avoid here, that this logic is handled separately in different places in core (which will lead to inconsistent application of this logic). It should be handled in one place so that dependant code doesn't have to think about this at all.

So I agree that child terms of unpublished Terms should probably not be visible, but I strongly disagree that Views should have special logic for this. So to me, #9 is the right thing for Views and we would need a separate issue that removes the special logic in \Drupal\taxonomy\Plugin\EntityReferenceSelection\TermSelection and applies that logic to the 'view' access of taxonomy terms in general. That way the responsibility of Term access stays within the Taxonomy module, where it belongs.

But if you really want to push for #22, it needs the test coverage back in and more added for the new logic.

I'm leaving the re-up of #9 RTBC and I'll see if we can get some more eyes on this.

Random unrelated fail.

Another one... unlucky.

+++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
@@ -181,6 +181,9 @@ protected function valueForm(&$form, FormStateInterface $form_state) {
 
         if ($tree) {
           foreach ($tree as $term) {
+            if (!$term->access('view')) {
+              continue;
+            }
             $choice = new \stdClass();
             $choice->option = [$term->id() => str_repeat('-', $term->depth) . \Drupal::service('entity.repository')->getTranslationFromContext($term)->label()];
             $options[] = $choice;
@@ -200,6 +203,9 @@ protected function valueForm(&$form, FormStateInterface $form_state) {

@@ -200,6 +203,9 @@ protected function valueForm(&$form, FormStateInterface $form_state) {
         }
         $terms = Term::loadMultiple($query->execute());
         foreach ($terms as $term) {
+          if (!$term->access('view')) {
+            continue;
+          }
           $options[$term->id()] = \Drupal::service('entity.repository')->getTranslationFromContext($term)->label();
         }
       }

I'm not sure about this change.

If we consider the filter to be a list of terms, then we should be relying on the views configuration or entity query to provide terms with the correct access - i.e. in the specific case mentioned, there should be a filter on published.

With node listings and similar we always rely on published status + node grants, and don't check node view access.

If we can't do that, then we need a comment explaining why we're diverging here.

Not 100% sold on the hardcoded filtering on published, still feels like Views is designing its own taxonomy access check, but ¯\_(ツ)_/¯

Here we go with a check on published.

If we consider the filter to be a list of terms, then we should be relying on the views configuration or entity query to provide terms with the correct access - i.e. in the specific case mentioned, there should be a filter on published.

+1. wonder if it would be a good idea to add a label for published/unpublished in the filter settings instead of trying to hide them. e.g.

diff --git a/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
index 5829681767..30d3bdca7c 100644
--- a/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
+++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
@@ -200,7 +200,7 @@ protected function valueForm(&$form, FormStateInterface $form_state) {
         }
         $terms = Term::loadMultiple($query->execute());
         foreach ($terms as $term) {
-          $options[$term->id()] = \Drupal::service('entity.repository')->getTranslationFromContext($term)->label();
+          $options[$term->id()] = \Drupal::service('entity.repository')->getTranslationFromContext($term)->label() . !$term->isPublished() ? ' (Unpublished)' : NULL;
         }
       }

+++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
@@ -181,6 +181,9 @@ protected function valueForm(&$form, FormStateInterface $form_state) {
+            if (!$term->isPublished()) {

might not work for the use cases where admin view to filter by unpublished terms.

Just throwing my 2c in, this feels like a security issue to me too. I agree with lendude.

Especially when you consider that a precedent was set with https://www.drupal.org/sa-contrib-2020-018 & https://www.drupal.org/sa-contrib-2020-016. They seem similar to me

Heh, I came across this issue and thought it'd be a simple fix, and after reading the comments it's clear there's a lot of good reasons why this is not a simple fix.

I agree with catch in #31. Yes, it feels wrong to have these direct checks for publishing status only (which completely ignores the entity access API), but it's really a temporary bug fix for the real problem described in #14 by Berdir.

If we implemented the direct entity access checks for each term in the loop, while it's not slow NOW, I think it will be slow in the future when #777578: Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter() is implemented, because it could mean an additional DB query for each term in the loop to check entity access grant table for taxonomy term.

So +1 to this solution. I also tested it and it works well.

Filed a follow up for getting a proper fix in place described in #14: #3159905: Taxonomy module should implement hook_query_TAG_alter for taxonomy_access tag

Committed/pushed to 9.1.x and cherry-picked back to 8.9.x, thanks!

I had a nagging feeling about this issue which I couldn't figure out what it is, and just realised after commit...

Should we be checking 'administer taxonomy' before adding the published status check? Then if you have an admin view with a term filter, the patch won't affect you.

Rolled back for now to discuss this, if the answer is 'no' we can recommit as-is.

I think the check should be in line with what the 'view' access check for terms currently does, and that means #44 sounds like a good idea to me.

We had test coverage for this before but no idea why I took that out ¯\_(ツ)_/¯

This adds a check for 'administer taxonomy' to both logic paths and test coverage for that.

Fix and test coverage are good.

You may want to consider removing this todo in TaxonomyIndexTid and closing #2352175: Taxonomy views filters should not have user context:

  public function getCacheContexts() {
    $contexts = parent::getCacheContexts();
    // The result potentially depends on term access and so is just cacheable
    // per user.
    // @todo See https://www.drupal.org/node/2352175.
    $contexts[] = 'user';

    return $contexts;
  }

@jonathanshaw thanks for pointing that one out! I think we should definitely post a comment there if this lands, but I think we need more feedback on it before removing and closing it.

Thanks for #46. Committed/pushed to 9.1.x and cherry-picked back to 8.9.x, thanks!

In the screenshoot i see that the filter was created using a field and maybe it's the only way now but ¿this works with "has taxonomy term" filter? I have 8.9.15 and with has taxonomy term filter the unpublished terms are showing.

thanks

(sorry, I created this comment by mistake and i cannot delete now)

I added follow-up #3250084: Views checks access to taxonomy terms instead of translations.