Allow the book outline functionality on non-book-enabled node types to be hidden from users with the "Administer book outlines" permission

Where do I need to place this patch in order to work?

I don't know what to do with a patch to make it work.

This makes "Allowed book outline types:" actually work! IMHO should be added to core as a fix. Thank you netw3rker!

works perfectly for me. thank you kindly!! :)

Subscribing.

In addition to what netw3rker explained there is another effect to this issue.

Under Content Management > Books > Settings there is this line of text....

"Select content types which users with the add content to books permission will be allowed to add to the book hierarchy. Users with the administer book outlines permission can add all content types."

With the way the book module is today, I don't see a way to enable a role to be able to edit an outline of a given book AND allow them to add only a certain content type to that book.

Since the "administer book outlines" permission will give them the ability to add all content types and they cannot edit outlines without this permission it's an all or nothing for outline editing. Unless I'm missing something obvious?

-Tim

@netw3rker
Thank you!
It worked so far. (Version 6.15)
I hope this patch finds its way into core.

I also thought that should be an obvious behaviour, only to show the outline fieldset on content types which are assigned to books.

I think the status to be set to 'needs review' to let the Testing Bot trigger an automated review of the patch, and report the results.

I guess the patch netw3rker attached was not created from the root directory. Trying again..

Ok, I must admit I have not got patches to pass testing either. As per the guidelines on creating patches - http://drupal.org/patch/create - we have to create it on the latest dev release. I see this issue is also on 7.x Dev snapshot which I downloaded.

I will try to use two different patch files for book.module and book.admin.inc.

Should this fail, may I request someone to help with the patching please?

Thanks

Attaching the patches for Drupal 6 (based on 6.17 release).

I like this one, doesn't seem to be committed yet, is it going to be??

Greetings Bas

Yes, I will be installing this too.

Is there a chance it to be committed into core or is there a code freeze ?

Ciao

subscribe

Ship it! Works great on 6.19.

Subscribing. I just implemented a helper module to effectively do what this patch does, with the exception of simply forcing the administer outlines perm to respect the available types permission.

Subscribe

Patches have code style issues.

bugs have to be fixed in HEAD first.

This patch basically removes a feature, however distracting that feature is, basically this is about removing an unintended feature. Not sure that is a good idea in a stable version (6.x or 7.x alike).

Discussing with netw3rker over email. A possible way forward would be to link the ability to put any content into books to the 'administer nodes' permission instead.

I am not sure I agree with that statement.
Why would you define a feature the possibility to maintain the outline of a content type that is not supposed to have an outline ?
I call this a bug.

Well, I see this as a property of the "administer...." permission. You can set workflow bits for node types on the content type page, but if you have "administer nodes", you can override all that anytime. Same for "administer book outlines", you get more powers. I think that was the thinking when this was implemented. Anyway, generally I agree it looks like a bug, but I can also see how people kind of expect this with an "administer ..." permission.

@Gabor - I think the present situation makes that permission much less useful than it should be.

I'd favor changing the check about moving all content into books to 'administer nodes' since that is already accepted as having maximal control over content.

Oops, I didn't see this thread before, so I've accidentally submitted a duplicate issue a few days back, where there's a patch as well:
http://drupal.org/node/1196576

I don't see why this is still under discussion. It's terribly confusing when you see an Outline tab under nodes that can't/shouldn't be able to be in an outline. #12 works great for me.

#12 works great for me too...

I know this is a very old issue, but I still need this in my installation. The old patches don't work with drush make, so I created a new patch, I also did a bit of clean up so the descriptions are in a t() and such.

I had to change my previous patch. The problem was that the node form alter now uses _book_outline_access, which expected a node object (with a nid), however no nid has been set when we create a node. So I altered the access check a little bit so it takes this into account by passing the node type as a string on node creation. Then node_access can handle the rest.

+++ b/modules/book/book.module
@@ -201,7 +201,11 @@ function book_menu() {
+  $node = isset($node->nid) ? $node : $node->type;
+  return (user_access('administer book outlines') && ((in_array($type, $types)) || (!$restrict))) && node_access('view', $node);

This does not work when creating a new node. "Notice: Trying to get property of non-object" notices are triggered as $node passed to node_access() is not an object.

Attached patch fixes this.

EDIT: and to reproduce this some spesific conditions are required. User must not have 'bypass node access' permission. User must have 'access content' permission. And finally hook_node_access implementations must not deny or allow access.

This works as described and fixes confusing issue.

Patch attached is just a re-roll plus minor code standard fix, so all credits to original authors.

The same behavior occurs in Drupal 8 so this would need to go in Drupal 8 first.

Also, the current behavior is a documented feature - I can see the rationale for wanting to change it, but am not sure yet-another-setting-in-the-admin-UI is a great way to do it. Splitting out a new permission along the lines of "add any content to books" (as discussed in #30 above) sounds like a better solution - yes, there are some backwards compatibility concerns with that, but relatively minor (there probably isn't too much contrib/custom code out there that interfaces with this part of the Book module, and mostly the concern would be broken links).

Meanwhile, a brief review of the patch:

1. What happens if non-book content is already in a book? The changes to _book_outline_access() in this patch would make some of the screens for editing it inaccessible (some via broken links in the admin UI, and some via missing links that should be there).
2. book_help() also documents the current behavior, so maybe it needs to be changed here also?

the update is working but no need to make user has "administer book outline" permission

My team mates and I have been using this little patch for almost a year now on a project. I'll just share it too.

Yeah nice! but still needs tests.

IMO add a new permission is the best approach, as commented in #39.

Added a new permission 'add any content to books', so users with this perm can use the book outline form in all node types, and other users only in allowed book types as expected.

Adding tests.

Sorry for the noise, I will try to minimize changes.

Added a hook_update to grant the new permission to all roles with 'administer book outlines' for backward compatibility.

All green ready for review :)

Thank you for fixing this issue.
After applying this patch I am getting the following error after saving an exisiting node (content type is not an book enabled one):

"You can only change the book outline for the published version of this content."

Is someone having an idea?

#49 gets us most of the way there. We also need to control access to the 'entity.node.book_outline_form' route. This route is for path '/node/{id}/outline'.

Attached patch also clarifies some of the comments/language. Tests are updated (but may fail on the first run).

Reworked tests.

Reworked tests.

Few coding standard issues resolved & added an interdiff.

Attached patch incorporates #67. It also includes additional code standard fixes and comments clean up.

The primary change between #49 and the current patch is that now access to a node’s book settings (both the ‘outline’ tab and the vertical tab on the ‘edit’ page) are controlled by the new BookNodeOutlineAccessCheck access plugin.

@JD_1, re #52 - I made several attempts to reproduce the behavior you described. I believe you're getting caught by BookOutlineConstraintValidator::validate(), but I don't know why. Are you able to provide steps to reproduce?

I will try it with your last patch tomorrow and give you some feedback. Maybe it is related to the issue/fix for the i18n support for books...

Ok: I did some additional tests with the latest patch and here are my results:

For book-enabled content types everything seems to be working fine
If I try to use other content types I still get the error (see attached file)

To get the error I did the following:
1) Apply Patch
2) Run database update
3) Clear cache
4) Uncheck the permission for to add non-book-related content in the permissions
5) Activate the default content moderation worklow for the default content type "basic page"
6) Logon with the user who did not have the permission (point 4)
7) Create a basic page as "draft"
8) Publish this draft/basic page
=> Until this point everything seems to be working
9) Create a new draft
=> Error

I checked the access function within the BookNodeOutlineAccessCheck and get the following return for:

a) a book enabled type:
$this->currentUser->hasPermission('add any content to books') = 1
$this->currentUser->hasPermission('add content to books') = 1
!empty($node->book['bid']) = 1
!$node->isNew()) = 1
book_type_is_allowed($node->getType()) = 1

b) a not enabled book type:
$this->currentUser->hasPermission('add any content to books') = 0
$this->currentUser->hasPermission('add content to books') = 1
!empty($node->book['bid']) = 0
!$node->isNew()) = 1
book_type_is_allowed($node->getType()) = 0

BR

@JD_1 - I'm able to reproduce the bug you described without the patch being offered by this issue.

Steps to reproduce:

1. Install Drupal 8.8 with 'Standard' install profile
2. Enable the following modules:
   • Book
   • Content Moderation
   • Workflows
3. Create a 'Test' role with the following permissions:
4. Book
   • [No permissions granted]
5. Content Moderation
   • Editorial workflow: Use Create New Draft transition.
   • Editorial workflow: Use Publish transition.
   • View any unpublished content
   • View the latest version
6. Node
   • Access the Content overview page
   • Administer content
   • Bypass content access control
7. System
   • Use the administration pages and help
   • View the administration theme
8. Toolbar
   • Use the toolbar
9. Workflows
   • Administer workflows
10. Add the' Basic page' content type to the 'Editorial' content moderation workflow
11. Create a test user with role 'Test'
12. Login with test user
13. Add a 'Basic page' node with workflow state of 'Published'
14. Create a new revision and save with a workflow state of 'Draft'
15. Observe validation error: You can only change the book outline for the published version of this content.

You'll need to open a new issue against core for the Book module. I'd mark it as Major since it renders content moderation unusable with no workaround.

@Chris-burge

Trank you so much for your quick Response and your test!

Just one question:
If this is a core bug-what is this patch fixing. Or the other way around: what can I test to see that this fix is working.
I thought this is exactly what this patch is trying to do.

Or is the major reason/difference to your system the activated content moderation module?

This issue isn't seeking to fix a bug; rather, it's a feature request. Currently, if someone has the 'administer book outlines' permission, then that user can also add non-book content to book outlines. This issue seeks to move that capability to an 'add any content to books' permission. In the event a non-book node is already in a book outline, then the node's book configuration is exposed to users without the permission.

Steps to test:

1. Create a test user/role with the the 'administer content' permission, along with the following book permissions: 'create new books', 'add content to books', 'administer book outlines'.
2. Create two 'Basic page' nodes as an administrator.
3. Add node/1 to a book outline.
4. Login as the test user
5. On node/1, observe the test user sees the 'Book outline' vertical tab and the 'Outline' tab on node/1/edit
6. On node/2, observe the test user does not see the 'Book outline' vertical tab or the 'Outline' tab on node/2/edit
7. Add the 'add any content to books' permission to the test user
8. On node/2, observe the test user now sees the 'Book outline' vertical tab and the 'Outline' tab on a 'Basic page' node

1. The titles in book.permissions.yml like Add non-book content to outlines shouldn't have periods after them.
2. Update to book_help() mentioned in #39. I tried to write something but got tongue tied trying to explain exactly what 'Administer book outlines' does now.
3. On /admin/structure/book/settings the description "Users with the 'Administer book outlines' permission can add all content types" needs to be updated. I believe the specified permission just needs to change to 'Add non-book content to outlines'.

@ericras - Good catches. All points have been addressed in the attached patch.

I also modified BookNodeOutlineAccessCheck::access(). Previously, the 'Add non-book content to outlines' permission implicitly granted the 'Add content and child pages to books and manage their hierarchies' permission, too. I have changed this behavior. Now the 'Add non-book content to outlines' permission is only considered if the role already has the 'Add content and child pages to books and manage their hierarchies' permission. Such is now now stated in the permission description for 'Add non-book content to outlines'.

Attached patch has corrected Git paths.

Just tested against 8.9.x and 9.1.x. The test failure for 8.9.x can be disregarded. The failure of Drupal\Tests\ComposerIntegrationTest::testComposerLockHash is unrelated.

I've rerolled the patch provided by @chris-burge almost a year ago as it was no longer applying to 8.9.x due to other commits to core/modules/book/tests/src/Functional/BookTest.php. Patch and interdiff (I think I did that right) attached.

Patch is updated for 9.1x

Interdiff (not sure if helpful) also attached.

Updated patch to fix 'failed to apply'

Patch applied cleanly and new book permission worked great.

I took a very brief look at the code and spotted this;

+++ b/core/modules/book/tests/src/Functional/BookTest.php
@@ -363,12 +365,26 @@ class BookTest extends BrowserTestBase {
-    $expected_nids = [$book->id(), $nodes[0]->id(), $nodes[1]->id(), $nodes[2]->id(), $nodes[3]->id(), $nodes[6]->id(), $nodes[4]->id()];
...
-    $expected_nids = [$book->id(), $nodes[0]->id(), $nodes[1]->id(), $nodes[2]->id(), $nodes[4]->id()];

Both of these changes appear to be out of scope.

Reroll the patch for 9.2.x and fixed as suggested in #89.

Patch #90 worked for me on 9.2.5

The issue summary doesn't correspond to the fix in the most recent patch.

I think the "Performance" tag may be justifiable here, and if so, makes me think that this could be bumped from a feature request to a task.

The following xhprof diff is from a real-world site, showing the difference on a node edit form which is not enabled as a book content type. Before is without the patch, After is with the patch from #90. I'm testing as a user that does not have the newly added permission in this scenario.

This could probably use more profiling with a more "stock" setup for sure.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

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

Added patch against #90 in version 10.1.

Hiding patch #99 as it's incomplete. You omitted the /core/modules/book/src/Access/BookNodeOutlineAccessCheck.php class

@Deshna Chauhan Please stop uploading incomplete and broken re-roll patches without an interdiff.

@BramDriesen Try to addressed your point add all file in reroll for d10.

@Nitin shrivastava Thanks, but your patch is still missing the /core/modules/book/src/Access/BookNodeOutlineAccessCheck.php class. And from a quick glance you're also missing a newline in the first code block.

I suggest to ignore what was done in #99 and start fresh from the patch in #90.

Hiding your patch and interdiff as well for that reason.

For the next person coming in. Start from #90 and provide a patch+interdiff based of that patch.

@Deshna Chauhan and @Nitin shrivastava I am removing credit for the unhelpful patches per How is credit granted for Drupal core issues.

Addressed the Comment #102
Attached patch against Drupal 10.1.x
Attached reroll patch

@pooja saraah That looks a lot better! Thanks :)

Needs work still justified for #94

Re-Roll The Patch #104 Due To Custom CMD Failed.

Still needs issue summary update from #94

This extension is being deprecated, see #3376070: [Meta] Tasks to deprecate Book module. It will be removed from core and moved to a contrib project, #3376101: [11.x] [Meta] Tasks to remove Book.

This is now Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.

This issue may be re-opened if it can be considered critical, If unsure, re-open the issue and ask in a comment.