The Content overview page filters out unpublished nodes when a node access module is enabled

Drupal\Tests\node\Functional\NodeAdminTest::testContentAdminPages
Behat\Mink\Exception\ExpectationException: Link containing href node/3 found.

core/tests/Drupal/Tests/WebAssert.php:587
core/tests/Drupal/Tests/WebAssert.php:445
core/modules/node/tests/src/Functional/NodeAdminTest.php:198

So based on failed tests after making the Status Extra filter to a NOOP, it seems what it handled and probably needs to be handled is making sure that authenticated users can only access to their own unpublished node when node access grants access to them (and by default it only grants access when "view own unpublished content" is granted. This is necessary at least for BC...

Drupal.Tests.node.Functional.Views.StatusExtraTest failed for a good reason, it should become obsolete after this fix.

Interesting. no content moderation test failed... however I expected some failures since other than the Status Extra Views plugin, nothing else run a query time filtering on results displayed on the Content overview page, does it? (Because content_moderation DOES NOT implement hook_node_access_records() just hook_entity_access(). Am I missing something? Or maybe this part is simply not covered with tests...

My colleague → who also took part in the investigation also worth crediting here.

Let's do a review round on the idea...

Let's move this to Views module, because basically this is a View filter plugin... (Plus "Node" has no maintainers... :screaming-cat:)
Also added "Needs subsystem maintainer review" in a hope that maintainers have a better, potentially more performant idea on how to solve this problem.

If you have a node_access implementation on your site, couldn't you just remove the status filter from the View and the correct nodes would be in your View? ie. The nodes that you have access to, independent of the status? Hmm probably only if you do something with (un)published in node_access I guess

Also, this list: '***UNPUBLISHED_NIDS_ACCESS_GRANTED_BY_NODE_ACCESS***' can potentially get HUGE, at how many unpublished nodes will this fail? Depends on max_allowed_packet I guess, but it will fail at some point....

Also, with the logic that was added in #3030477: Views filter "Published status or admin user" not checking "View any unpublished content" permission → and if we now add this, the responsibilities of this filter just seem to be getting too much, they certainly aren't just what the description of that filter says.

Sorry, nothing concrete to add, just general worries. Also, I see the problem, we've run into it in our projects too, just don't see a clean solution at the moment

If you have a node_access implementation on your site, couldn't you just remove the status filter from the View and the correct nodes would be in your View? ie. The nodes that you have access to, independent of the status? Hmm probably only if you do something with (un)published in node_access I guess

Good idea, this simple one haven't came to my mind yet... but you are also right, unpublished ones would only appear if there is a node access module that grants access to them (at the query level). like view_unpublished contrib is enabled.

 * Node access grants apply regardless of the published or unpublished status
 * of the node. Implementations must make sure not to grant access to
 * unpublished nodes if they don't want to change the standard access control
 * behavior. Your module may need to create a separate access realm to handle
 * access to unpublished nodes.

https://github.com/drupal/core/blob/17708a8b8146cc3476a12bda9073376eb591...

My idea in ✨ Grant query level access to own unpublished nodes Active could be related, I do not know the historical reasons why Drupal core tried to leverage node access OOTB.

Also, this list: '***UNPUBLISHED_NIDS_ACCESS_GRANTED_BY_NODE_ACCESS***' can potentially get HUGE, at how many unpublished nodes will this fail? Depends on max_allowed_packet I guess, but it will fail at some point...

I also have this concern, I haven't found anything specific about the potential maximum length of parameters that would break this query... but again, better ideas are welcomed.

, they certainly aren't just what the description of that filter says.

I agree, the description should be changed, the purpose changes as well.

Thanks for raising these, they were valuable inputs.

hmm... there is an https://github.com/drupal/core/blob/11.0.0-beta1/modules/node/src/Plugin... that does something smarter.

Thanks for taking a look @lendude.

Trying to keep up but seems like this is still NW for the concerns expressed.

Question though
1. Does this belong in core or could be better suited for contrib? Generic question for a lot of things I see come through core so thought I'd ask
2. What about a config setting or settings variable that maybe turns it off if performance is a concern?

@smustgrave thanks for the review, I think your questions become irrelevant, since the fix took a complete different direction.

I realized that I did not see the bigger picture... and I realized why all related issues suggested "Disable SQL rewrite" as a "solution".

Since the solution become more simpler, probably we do not need subsystem maintainer review anymore...

Also, let's try to move this back to node module's realm...

Nice! can issue summary be updated for new approach

Will keep an eye out.

I did some adjustments, do you still see any place for improvement?

I also wonder if this would need a CR or not... my current take is on NO.

I think it will as it is changed behavior automatically. Could be simple though with before/after screenshots maybe of the altered results?

Could be simple though with before/after screenshots maybe of the altered results?

I think I maybe could... but the proposed change DOES NOT alter the original query alter anymore, just disables the query alter when a node access module is installed. So in this case, would it make sense capturing queries when it is clear how the behavior was changed?

I get your point on the CR, I'll create one.

I guess the tag is outdated.

Created: https://www.drupal.org/node/3455665 →

That CR is fantastic very nice!

Checking the MR believe the open threads were related to the previous solution which has since pivoted.

Issue summary appears to be inline with everything.

Code wise the change makes sense and I don't see any issue

https://git.drupalcode.org/issue/drupal-3449181/-/jobs/1832340 shows the test coverage.

\o/

Thanks for your collaboration @smustgrave!

I read the IS, and the MR )not a code review). I made 3 suggestions, 2 are wrapping and 1 changes the summary line of the test method. Those changes seems minor and do not require discussion so I have applied those suggestions.

There is a UI change here and a string change, therefore tagging. I have made screenshots and updated the issue summary. Since this is adding a sting, one that is in bold as well, I think there should be feedback from the usability team. I am going to tag for a review as well as I can't comment on the quality of the new string. Now that there are screenshots, I will ping in #ux as well.

Finally, let's have a title update that mentions node grants is involved here. Thanks

Discussed this with xjm and confirmed that because this is change of behavior for permissions/access control that this is not eligible for a patch release. That is this is minor only and needs a release note.

Thanks @quietone for your work on this ticket! It makes perfect sense releasing this in 10.4.0 instead of a patch release for 10.3.x.

I added a release note snippet to the issue description, please review.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

@mxr576, thanks for the snippet. It was very clear, thanks! I tweaked it a bit. There is an issue for the 10.4 beta release notes 📌 10.4.0-beta1 release notes Postponed . You may want to add that snippet to those sometime. I have updated the tags.

Usability review

We discussed this issue at 📌 Drupal Usability Meeting 2024-07-12 Needs work . That issue has a link to a recording of the meeting. For the record, the attendees at the usability meeting were @AaronMcHale, @benjifisher, @rkoller, @shaal, @simohell, and @worldlinemine.

We did not discuss whether this issue is a good idea. It looks to me as though the site builder/administrator could achieve the same effect by removing the 'Published status or admin user' filter. If that is the case, then this issue is making a decision for the site builder, and that is usually a bad idea.

We did discuss the proposed text for the filter. For a site that does not have any modules implementing hook_node_grants(), the additional text is not helpful and may be confusing. For a site that does have such a module, the additional text does not give a clear indication of what the options are.

We recommend adding text only in the second case, and making it more specific. That is, use the same test if (\Drupal::moduleHandler()->hasImplementations('node_grants')) {...}, and do not alter the text if the condition is FALSE. If the condition is TRUE, then do a little more work and give the administrator a list of the relevant modules. We did not discuss the exact wording, but something like

This filter has no effect because the SomeNodeAccess module controls access.

or

This filter has no effect because the SomeNodeAccess and AnotherNodeAccess modules control access.

If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.

Thanks for the UX review!

We did not discuss whether this issue is a good idea. It looks to me as though the site builder/administrator could achieve the same effect by removing the 'Published status or admin user' filter. If that is the case, then this issue is making a decision for the site builder, and that is usually a bad idea.

They could indeed achieve the desired effect, but only if they are fully aware of how the system works behind the scenes. Many site builders and administrators might not realize that even with certain node access modules enabled, "limited editor" roles still would not see unpublished content.

Regarding the point that this issue is making a decision for the site builder, and that this is usually a bad idea, I agree. However, this decision was already made in good faith to enhance security based on previously observed misconfigurations. This issue and the associated merge request aim to roll back those changes, trusting that Drupal users are now more mature and knowledgeable about what they are doing.

Additionally, I believe the numerous recommendations on Stack Overflow and other forums to enable 'administer nodes' and/or 'bypass node access' permissions—even for limited access roles—stem from the current hidden behavior of the admin/content view, which only shows unpublished nodes when one of these permissions were granted (to non-authors).

We did discuss the proposed text for the filter. For a site that does not have any modules implementing hook_node_grants(), the additional text is not helpful and may be confusing. For a site that does have such a module, the additional text does not give a clear indication of what the options are.

I see your point and accept it as valid. However, our UX colleagues usually suggest implementing solutions where end users can still see options, even if they are not currently applicable. This approach helps users know where to find the information when it becomes relevant to them. In this context, if the help text is conditional, site builders might only realize what is happening after they start debugging the issue of "visible unpublished nodes." They won't remember that the Status filter behaves differently unless they have encountered this problem before.

If the condition is TRUE, then do a little more work and give the administrator a list of the relevant modules. We did not discuss the exact wording, but something like

Regarding the suggestion to provide a list of relevant modules if the condition is TRUE, I agree with this approach. However, I'm unsure how to achieve that at the moment without introducing a new public API. The list of modules/themes that implement a hook is protected within ModuleHandler and not accessible to external callers. #fixme \Drupal\Core\Extension\ModuleHandler::getImplementationInfo() is protected.

However, I'm unsure how to achieve that at the moment without introducing a new public API. The list of modules/themes that implement a hook is protected within ModuleHandler and not accessible to external callers. #fixme \Drupal\Core\Extension\ModuleHandler::getImplementationInfo() is protected.

@berdir suggested a working solution for this on Slack: https://git.drupalcode.org/project/pathauto/-/commit/f465b5a7a8b1a83f2ee...

So this is how it looks like the requested change by UX, if everybody likes it then I guess it needs test coverage - which is not that complicated, because in a test we just need to enable node_test (that implements hook_node_grants_alter()), node_access_test (that implements hook_node_grants()) and maybe path_test_node_grants (that also implements hook_node_grants()).

Since the final solution only disables a query alter which has a similar result that anybody could achieve by removing this views filter from a View, I do not see why performance review would be needed. By removing a query alter, the performance should be better :)

   public function query() {
+    if (\Drupal::moduleHandler()->hasImplementations('node_grants')) {
+      return;
+    }

I hope the new issue title is better even if it uses "node access module" instead of "node grants"? #24

Asked the UX team on Slack if we still need the "usability" tag on this issue after #35.

Yes, the Usability tag → remains. The use of the tag is part of the Usability gate → .

Thanks for the title update, it is much better now.

The comment above, #32 made me realize that the class docs for NodeViewsData should be updated to explain what the class will do with this change.

I left both suggestion and questions in the MR. I updated the links for that 'after' screen shots in the issue summary to the images from #35 . I did not review the change record.

Unrelated FunctionalJS failures, let's see if rebase from master helps...
https://git.drupalcode.org/issue/drupal-3449181/-/pipelines/277201/test_...

Unrelated FunctionalJS failures, let's see if rebase from master helps...

It did.

@quietone Thanks for your review, back to you!

I have also updated Unpublished nodes are no longer hidden on Content overview page when a node access module is enabled → CR and created "Published status or admin user" Views filter becomes inactive when a node access module is enabled → CR to ensure granular and precise notification about the change.

Hi I had a question and possibly some feedback for this fix.

We have ran into this issue on a site at our institution. In our situation we have a View Block that uses the the "Content: Published status or admin user" filter and we have the View Unpublished modules installed and enabled. What we have notices is after updating to Drupal 10.3.5 content editors, lost the ability the view the View Block as logged in users, however non authenticated user could see the content. We discover that removing filter and en adding it fixed the issue.

Additionally, we ran another test using the patch provided in this post. The patch did change the UI elements indicating that the filter would not have an effect, however the content did not present. It wasn't until we removed the "Content Published status or admin user" filter and re add it did the content get presented as normal.

My questions:

1. If the patch is to remove the additional Criteria on the queuery, why after apply the patch does the content not show up?
2. Why does the content show up after I remove and re add the filter?

Hiding patches since fixes are in MRs now. That said the MR 8156 appears to have build errors.

Interesting, there were no conflicts. Back to Needs review.

Thanks for your feedback @nathanhealea, I have asked my colleague to try to reproduce the issue you reported. Thanks to the test coverage in the MR I am quite confident that the solution works, but there is always a chance for unknown issues.

❯ git checkout -b '3449181-content_overview_access_fix' --track drupal-3449181/'3449181-content_overview_access_fix'
Branch '3449181-content_overview_access_fix' set up to track remote branch '3449181-content_overview_access_fix' from 'drupal-3449181'.
Switched to a new branch '3449181-content_overview_access_fix'
❯ git fetch origin
remote: Enumerating objects: 2408, done.
remote: Counting objects: 100% (1905/1905), done.
remote: Compressing objects: 100% (743/743), done.
remote: Total 960 (delta 575), reused 516 (delta 178), pack-reused 0 (from 0)
Receiving objects: 100% (960/960), 523.33 KiB | 1.27 MiB/s, done.
Resolving deltas: 100% (575/575), completed with 285 local objects.
From https://git.drupalcode.org/project/drupal
   4e0ce97be7..75ea8e8bf9  10.2.x     -> origin/10.2.x
   72b94a8a0e..b53484abb1  10.3.x     -> origin/10.3.x
   d2ca6921a8..7bbceff8c4  10.4.x     -> origin/10.4.x
   0701817796..70c1b4f59a  11.0.x     -> origin/11.0.x
   49a3c28eaf..2ad947f2f3  11.x       -> origin/11.x
❯ git rebase origin/11.x
Successfully rebased and updated refs/heads/3449181-content_overview_access_fix.

  Failed to clone https://github.com/composer/installers.git via https, ssh p  
  rotocols, aborting.                                                          
                                                                               
  - https://github.com/composer/installers.git                                 
    Cloning into bare repository '/root/.composer/cache/vcs/https---github.co  
  m-composer-installers.git'...                                                
    fatal: unable to access 'https://github.com/composer/installers.git/': Fa  
  iled to connect to github.com port 443 after 130169 ms: Couldn't connect to  
   server                                                                      
                                                                               
  - git@github.com:composer/installers.git                                     
    Cloning into bare repository '/root/.composer/cache/vcs/https---github.co  
  m-composer-installers.git'...                                                
    error: cannot run ssh: No such file or directory                           
    fatal: unable to fork                          

Ahh, so previously the pipeline failed due to transient errors.

Hi @nathanhealea,
I would like to try to reproduce what you described and I would need more context. In the first scenario from which Drupal core version did you perform the update to 10.3.5? Was the patch applied during the update? Which related permissions do content editors have on your site? Is the content which was visible for non-authenticated users but not for content editors published or unpublished? And same question for the second scenario: was the content published or unpublished which disappeared? Did the content disappear for all kind of users?

however the content did not present. It wasn't until we removed the "Content Published status or admin user" filter and re add it did the content get presented as normal.

My questions:

* If the patch is to remove the additional Criteria on the queuery, why after apply the patch does the content not show up?
* Why does the content show up after I remove and re add the filter?

My gut says that this is some kind of stale cache or the lack of cache clear between the patch is being applied and site was visited. But as @morvaim said, some more context could came handy for reproducing this, for example, the source of the View block.

Currently I do not believe that the reported issue is blocking the merge of the MR.