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 still is making sure that authenticated users can only access to their own unpublished node when they have "view own unpublished contnet" 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...

UPDATE Well, actually, probably... all permission based filtering that happened in runtime in \Drupal\node\NodeAccessControlHandler::checkAccess() probably should become a query time filtering, not just for Views... #fixme

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 Views 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

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 #3452449: Grant query level access to own/any unpublished nodes 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.

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

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.

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

\o/

Thanks for your collaboration @smustgrave!

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.

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.

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.

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.

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.

There are no merge conflicts atm but this branch is behind HEAD with 100+ commits, so I am rebasing to ensure whoever reviews the latest changes can review an up to date state.

#goForTenDotFourDotZero

The MR still applies.

Removing the tag, since this has not been merged before 10.4.0 got released. :-(

The MR was still mergeable, but merged 11.x to keep the MR up to date.

Thanks @longwave for the suggestions and @prabha1997 applying them. I have also bumped versions on related CR-s.

Should be ready to be merged, right? :)

There was no conflict, so moving back to RTBC.

❯ git fetch origin
remote: Enumerating objects: 7322, done.
remote: Counting objects: 100% (5502/5502), done.
remote: Compressing objects: 100% (1877/1877), done.
remote: Total 2830 (delta 1874), reused 1628 (delta 854), pack-reused 0 (from 0)
Receiving objects: 100% (2830/2830), 719.19 KiB | 10.27 MiB/s, done.
Resolving deltas: 100% (1874/1874), completed with 976 local objects.
From https://git.drupalcode.org/project/drupal
   018087b1ff7..42de5b1df41  10.4.x     -> origin/10.4.x
   0cf4a33c149..60a21fa9287  10.5.x     -> origin/10.5.x
   d3b4dce5a03..ff743d3ce22  11.1.x     -> origin/11.1.x
   6dd918c9360..93ec7a93b73  11.x       -> origin/11.x
 * [new tag]                 10.4.4     -> 10.4.4
 * [new tag]                 11.1.4     -> 11.1.4
❯ git merge origin/11.x
Auto-merging core/modules/node/src/NodeViewsData.php
Merge made by the 'ort' strategy.

That makes sense. If this change is introduced in a new minor version, couldn’t we instead provide an update hook to remove the unnecessary status filter from affected Views? Or would that only be feasible in a new Drupal major release? (The idea came when I checked code examples for the requirement checks and found all these config update implementations in \Drupal\views\ViewsConfigUpdater.)

If the plan is to introduce this in 10.4.x and 11.1.x (which I’d love to see happen after all these iterations), then these versions could include the requirements warning, while the subsequent minor versions could handle the removal of redundant filters via an update hook.

Needs review, and feedback welcomed on the wording.

("Please" was removed from the message since the screenshot is taken.)

Awesome! Thanks for bearing with me!

So this is not eligible anymore for backporting to 10.5?

Hopefully having this in 11.x means we get some contrib tests against it and they might reveal things we've not considered or otherwise.

We will see, I am only excepting contrib test failures in case they were asserted on something that they should not or not valid anymore.