Unpublished books appear in the list of books at /book

Good. This is not critical with a workaround.

Thanks for reporting this issue. We rely on issue reports like this one to resolve bugs and improve Drupal core.

As part of the Bug Smash Initiative, we are triaging Drupal core issues with the priority 'Major'.

I don't think this meets the criteria for major since there is a document workaround of creating a view for this page.

I was able to reproduce this in 9.0.x so it is a bug in book module, nothing to do with a migration. I've also added steps to test issue summary.

Hello, I tested patch #9 but it does not work for me.

I'll create a new patch.

A patch for it and interdiff.

I looked at the code, it makes sense to me. The patch applied cleanly, and had exactly the effect it claimed to. The unpublished book appeared in the list for an anonymous user before the patch, it disappeared from the list after the patch. Just one nitpick. `use Drupal\user\Entity\User;` has been added, and I see no reason for it. Really small change, I'll provide a patch shortly.

Tiny fix to #11 as promised.

Great progress so far!

Not sure about the major/normal, since this is borderline security (users can view unpublished content labels when they don't have the permission to do so) major might be right.

Couple of things:

1. +++ b/core/modules/book/src/BookOutlineStorage.php
   @@ -16,18 +17,30 @@ class BookOutlineStorage implements BookOutlineStorageInterface {
   +  public function __construct(Connection $connection, AccountInterface $currentUser) {
   

   The new argument needs to default to NULL and we need to add a deprecation message about not settings the current user, see #3101738: Exposed term filters should not show term options that the user does not have access to for a similar situation and how to solve this.

2. +++ b/core/modules/book/src/BookOutlineStorage.php
   @@ -16,18 +17,30 @@ class BookOutlineStorage implements BookOutlineStorageInterface {
   +      return $this->connection->query("SELECT DISTINCT(bid) FROM {book} b INNER JOIN {node_field_data} f ON b.nid = f.nid WHERE f.status = 1")->fetchCol();
   

   do we really need to write out all this in query()? Can't we use a select()? (I'm asking, dunno if there is a reason to do it like this)

Also, this needs tests.

in this section:

+    if ($this->currentUser->hasPermission('view any unpublished content')) {

instead of

'view any unpublished content'

you may want

''view own unpublished content'

The permission 'view any unpublished content' is introduced in the content_moderation and workflows modules,
and isn't defined in the book module.

(It may be desirable to use the book module with content_moderation and workflows but you need to make it explicit).
See issue: https://www.drupal.org/project/drupal/issues/2004

As the code in #13 is now written, it prevents anonymous users from seeing unpublished books.
But it also prevents bookAuthors from seeing their own unpublished books.

(See https://www.drupal.org/project/drupal/issues/26552)

Hello @mindbet,

I understand what you mean on comment #16.

What do you think about this?

public function getBooks() {
    $currentUserId = $this->currentUser->id();
    if ($this->currentUser->hasPermission('view any unpublished content')) {
      return $this->connection->query("SELECT DISTINCT(bid) FROM {book}")->fetchCol();
    } 
    elseif ($this->currentUser->hasPermission('view own unpublished content')) {
      return $this->connection->query("SELECT DISTINCT(bid) FROM {book} b INNER JOIN {node_field_data} f ON b.nid = f.nid WHERE f.uid = $currentUserId OR f.status = 1")->fetchCol();
    } 
    else {
      return $this->connection->query("SELECT DISTINCT(bid) FROM {book} b INNER JOIN {node_field_data} f ON b.nid = f.nid WHERE f.status = 1")->fetchCol();
    }
  }

I wrote a new patch with what @Lendude proposed on comment #15 except the second observation. It still needs tests for it.

Cheers, Paulo.

Setting to needs review to kick the testbot into gear, but still needs work for tests

Back to needs work for the tests.

Also, not sure the book module should worry about integrating with the content_moderation module, feels like it should be the other way around.

Here is a test. I added this to the existing testBookListing() to avoid creating yet another Funcational test.

This just adds a test, making no changes to the patch in #17, so the fail patch is the interdiff.

This is a suggested revision for the logic in #17.

Imagine a situation where a top book page is unpublished, but one or more child pages are published.

The select statement:

"SELECT DISTINCT(bid) FROM {book} b INNER JOIN {node_field_data} f ON b.nid = f.nid WHERE f.status = 1"

will return the book id of the child pages.

As a result, the top book page will be included in the book listing even if it shouldn't be shown because of its status.

This revised select statement checks only for top book pages to be included in the listing.

"SELECT DISTINCT(bid) FROM {book} b INNER JOIN {node_field_data} f ON b.nid = f.nid WHERE f.status = 1 AND b.pid = 0"

The select statement for 'view own unpublished content' is similarly revised.

Good one @mindbet,
patch looks good.

I tested it with a book that the top page is unpublished and the child page is published.
The book is not displayed as expected.

Moving to RTBC.

However, since the logic of the patch has changed the test needs to change as well. The test currently unpublishes all nodes in the book and that is not testing the new code.

Changed the test with just the top page unpublished and then with the top page published and one of the children unpublished. I think that covers the cases.

Also the comment needs tweaking.

+++ b/core/modules/book/tests/src/Functional/BookTest.php
@@ -510,6 +510,17 @@ class BookTest extends BrowserTestBase {
+    // Unpublish all the book nodes and Confirm that the created book title

s/Confirm/confirm

I think we're almost there. IMHO the tests should cover what @mindbet pointed on comment #23.

Imagine a situation where a top book page is unpublished, but one or more child pages are published.

I did a patch for it. Please review it.

I agree we need the test but the test is already there. The first test is of the top page unpublished and a book page published, in fact all the other pages are published at that time. The second test is the reverse, the top page published and a child page unpublished.

+++ b/core/modules/book/tests/src/Functional/BookTest.php
@@ -510,6 +510,34 @@ public function testBookListing() {
+    // Unpublish the top book page and publish a page in the book and confirm
+    // that the created book title is not displayed for anonymous.

This is a duplicate of the test that starts on line 515.

Or, have I missed something?

Oh yes, my mistake.
I didn't realize that the book pages are published and the only thing we have to do is set the top book to unpublished.
I attach the patch #25 again.

Thanks @quietone!

@paulocs, thanks!

Needs a reroll after #26552: Allow users with access to unpublished nodes to create unpublished books landed.

Also, can we get a new test-only patch with this so we can see that the new test logic still triggers the bug?

Rerolled patch #28 and added test only patch.

@ravi.shankar, Thanks for the reroll. Can you upload the interdiff, or diff, please. Oh, and it is best to upload the fail/test-only patch first and then the complete patch, that way the failure of the test-only patch won't set the issue to NW.

Uploading test-only patch.

Here attaching re-rolled patch with interdiff.

Yes, that look right now. RTBC +1

+++ b/core/modules/book/src/BookOutlineStorage.php
@@ -16,18 +17,39 @@ class BookOutlineStorage implements BookOutlineStorageInterface {
+    if ($this->currentUser->hasPermission('view any unpublished content')) {
...
+    elseif ($this->currentUser->hasPermission('view own unpublished content')) {
...
+    else {

Sorry, should have spotted this sooner, but we are missing test coverage here. We are setting 3 possible queries, but only testing the third one. The first one might have coverage elsewhere (didn't check), but the middle one has no coverage.
So we need to add coverage for the middle one and maybe to be safe some explicit coverage for the first one

Fixing test coverage that @Lendude pointed on comment #38.

@paulocs nice! Looks good, just nits on the comments and then I think this is ready.

1. +++ b/core/modules/book/tests/src/Functional/BookTest.php
   @@ -561,6 +564,21 @@
   +    // displayed for people who has 'view own unpublished content' permission.
   

   has => have OR people => user (I would do the 'user' one)

2. +++ b/core/modules/book/tests/src/Functional/BookTest.php
   @@ -561,6 +564,21 @@
   +    // Confirm that the created book title is displayed for
   +    // people who has 'view any unpublished content' permission.
   

   has => have OR people => user (I would do the 'user' one)
   and the first line can wrap later, some more words fit on the line before you hit 80 characters

New patch.

I attached the patch two times instead of the interdiff.

Nice, thanks! Looks good to me now

+++ b/core/modules/book/src/BookOutlineStorage.php
@@ -16,18 +17,39 @@ class BookOutlineStorage implements BookOutlineStorageInterface {
+      return $this->connection->query("SELECT DISTINCT(bid) FROM {book} b INNER JOIN {node_field_data} f ON b.nid = f.nid WHERE b.pid = 0 AND (f.uid = $currentUserId OR f.status = 1)")->fetchCol();
...
+      return $this->connection->query("SELECT DISTINCT(bid) FROM {book} b INNER JOIN {node_field_data} f ON b.nid = f.nid WHERE f.status = 1 AND b.pid = 0")->fetchCol();

These queries should use the query builder. I also don't understand why the new queries are limited to books where pid = 0. This is a big change. Also joining like this into node_field_data will break if alternate entity storage is used. Plus this only supports status based access checking - nothing more complex.

Also we're already checking status in \Drupal\book\BookManager::loadBooks() so that's interesting and I ponder if that's even working atm and how it really affects this patch.

I think a better solution here is to change the status check to a view access check instead of the status check. But then again considering all the places getAllBooks is used it might be better to do the access check in \Drupal\book\Controller\BookController::bookRender() and not affect anything else.

Yeah the existing code in BookManager doesn't work as expected because when it does if (isset($nodes[$nid]) && $nodes[$nid]->status) { it expects $nodes[$nid]->status to be a boolean but it's actually a FieldItemList object. Obviously there was 0 test coverage of this and this explains why we probably don't have this issue in D7.

So I tested patch #45 and I think it is a better solution. Not big changes are required and it updates the code that was not working previously.
Moving to RTBC.

We normally try to avoid node access checks for listings - however that relies on the listing query having been rewritten by node grants, and the book tree manager doesn't do anything like that. Since this was wrongly (or more, not-) ported code from 7.x, makes sense to fix unobtrusively and add test coverage, and we could look at converting the query later on.

Committed 989f135 and pushed to 9.1.x. Thanks!

Needs a re-roll for 8.9/9.0

Re-rolled for 9.0.x and 8.9.x.

I tested patch 3040181-45-8.9.x.patch and 3040181-45-9.0.x.patch.from #50.
They look good.

Backporting this seems important due to the fact it is a security improvement.

Committed 45cf678 and pushed to 9.0.x. Thanks!
Committed 652574f and pushed to 8.9.x. Thanks!

This caused a huge performance regression on a site with a few thousand book nodes. The underlying issue that on book node edit forms, all books are loaded in order to fill the book select element options. This was already the case before this patch. Now with this patch we are not only loading thousands of nodes but also doing an access check on each one, which adds about 20 seconds to the load time on a node edit page.

To make matters worse, due to #502430: Allow the book outline functionality on non-book-enabled node types to be hidden from users with the "Administer book outlines" permission this is happening on all node pages, even ones which have nothing to do with books.

Since the book form is provided by the BookManager service which can nicely be overridden, we are able to circumvent this issue without too much hassle. (A regular form alter (which we already doing before) is useless, because it comes after all the loading and access checking has already happened.)

I opened #3187277: Book selection form can load thousands of nodes (turn to autocomplete) for the underlying issue of loading all the nodes in the first place, as I couldn't find an existing issue for that. Since that is the real culprit and this just made things a lot worse, I am not asking for this be rolled back, but wanted to note it in case release managers come to a different conclusion and also in case anyone is suffering from the same performance issues since updating to 8.9.8.