Problem/Motivation

Similar to #2856363: Path alias changes for draft revisions immediately leak into live site.

Steps to reproduce

  1. Enable book module and configure it for article content type.
  2. Create a node and publish it.
  3. Create a new draft revision of the node, adding it to a book outline from the node form.
  4. Note the node immediately appears in the book outline.The same will be true to changes/removals from book outlines when editing nodes as draft.

There are really two bugs here:

1. Data loss/integrity due to side effects from saving drafts.

2. Access bypass on sites that have more complex workflows restricting access to publish actions, since this allows people who wouldn't be able to publish a draft to affect the published site.

Proposed resolution

Either:

1. Prevent changes to book outlines when saving revisions as drafts.

2. Add revision support to book module.

(or the first as a stop-gap and the second as a follow-up task).

A further consideration once we get to workspaces is admin/structure/book - if you're in a workspace you'd expect that to only make changes in the workspace, at the moment it'll change things on the live site.

Remaining tasks

User interface changes

API changes

Data model changes

Comments

catch created an issue. See original summary.

catch’s picture

Issue summary: View changes
mxr576’s picture

I think this is not a newly discovered bug just no one created an issue for this yet, rather everybody tried to find an alternative solution and get rid of book module.
I had also realised that this is just an another drawback of book module's a few months ago. After that I tried the impossible on a D7 project by combining Revisioing, Entity menu links and Book modules. (Not speaking of OG Book module.) TL;DR. I gave up after a while, because I saw how much work would it be to create a proper and working solution. D7's Book module depends too much on menu system which makes things really really complicated.
I also tried to find an another solution, like Tree module, but it is in highly unstable state so I end up with a custom soution.

To be honest, I was really disappointed when I saw that Book module is ported to D8 without any improvements. I had no doubt that it has the same limitations as in D7 and because of that using it an a project has more drawbacks than benefits in 2o17.
That is why I get thrilled when I saw PreivousNext's new module a few weeks ago, called Entity Hierarchy which leverages nested sets to make hierarchy generation faster by using entity reference fields. (Similarly to Tree module) I knew this could be a perfect replacement of Book module.
What a coincidence, I've just started to enhance this module today to become a perdect replacement of Book.

https://www.drupal.org/node/2862096

cilefen’s picture

Issue tags: +Triaged D8 critical

@alexpott, @catch, @cottser, @xjm and I considered this issue and agree this is suitably a critical priority bug for the reasons stated in the issue summary: data integrity, and an access bypass.

pwolanin’s picture

The non-support for revisions in book has been evident for a very long time and I'm pretty sure I've closed issues in the past as "works as designed".

The only critical piece here would seem to be blocking a non-published revision from adding to or changing the book hierarchy.

I don't think it's possible or sensible to add revision support for elements of a hierarchy at the individual node level, but it would be more feasible at the level of a workspace.

If you think it's possible to integrate with individual node revisions, please explain the data model to me and how that would work as revisions are published or reverted. The taxonomy tree is represented by {taxonomy_term_hierarchy} which is not revision aware either.

Note my proposed (not accepted) core conversation was to be about some of this: https://events.drupal.org/baltimore2017/sessions/better-content-hierarch...

cilefen’s picture

The only critical piece here would seem to be blocking a non-published revision from adding to or changing the book hierarchy.

Indeed. The priority is a reaction to that, not necessarily a blessing of the proposed solution.

amateescu’s picture

Status: Active » Needs review
FileSize
1.06 KB
1.68 KB

Ok, so let's implement the first proposed resolution from the issue summary and leave the revision support part for a followup.

The last submitted patch, 7: 2858431-test-only.patch, failed testing.

pwolanin’s picture

+++ b/core/modules/book/src/BookManager.php
@@ -249,6 +249,12 @@ public function updateOutline(NodeInterface $node) {
+    if ($node->isNewRevision() && !$node->isDefaultRevision()) {

Do we even need to check if it's a new revision? If it's not the default revision we should ignore it.

amateescu’s picture

That's true. I got it right in the comment but I forgot to also update the code :)

amateescu’s picture

Issue tags: +DevDaysSeville
catch’s picture

On #5 I think we should open follow-ups to discuss that. If it's unfixable (or we choose not to fix it), then it means book module won't have proper integration with content moderation or full site preview via workspaces. I definitely think we should do the quick fix here since it's a (potential) access bypass and UX issue, as well as lack of the feature.

+++ b/core/modules/book/src/BookManager.php
@@ -249,6 +249,12 @@ public function updateOutline(NodeInterface $node) {
 
+    // Prevent changes to the book outline if the node being saved is not the
+    // default revision.
+    if (!$node->isDefaultRevision()) {
+      return FALSE;
+    }
+

So this prevents the change, but we need some feedback for the person trying to make the change.

pwolanin’s picture

Status: Needs review » Needs work

So needs work to at least set a message?

Probably needs a test too?

amateescu’s picture

Status: Needs work » Needs review
FileSize
7.16 KB

Finally got around to writing a nice UI test for this. No interdiff because the initial patch was quite simple :)

amateescu’s picture

Oops, I forgot a small change that I made to the existing BookTest class.

The last submitted patch, 14: 2858431-14.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 15: 2858431-15.patch, failed testing.

timmillwood’s picture

Issue tags: +Needs screenshots
+++ b/core/modules/book/src/BookManager.php
@@ -249,6 +249,13 @@ public function updateOutline(NodeInterface $node) {
+      drupal_set_message($this->t('The book outline could not be changed because a non-default revision (for example: a new draft revision) of the content has been created.'), 'error');

This seems a bit wordy, but not sure I have a better suggestion. Think this issue needs screenshots, then usability review?

amateescu’s picture

Status: Needs work » Needs review
Issue tags: -Needs screenshots
FileSize
21.71 KB
16.7 KB

I forgot that a child test class also executes the methods from the parent one if it's not abstract, so we have to extract the common needs in a trait.

Here's a screenshot as well, which shows that after a new draft is saved the user is taken to the admin/content listing with an error message displayed. I don't have any better suggestion for the wording of that message though..

amateescu’s picture

Forgot to attach the screenshot :/

Status: Needs review » Needs work

The last submitted patch, 19: 2858431-19.patch, failed testing.

jhodgdon’s picture

Possibly better message text:

This is not currently the default revision, so you cannot change the book outline.

or something similar?

catch’s picture

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

catch’s picture

Issue tags: +Needs usability review

Tagging for usability review, we might need a meta on ways to talk about published vs. draft revisions.

jhodgdon’s picture

Yeah, I'm not sure what the current way is to talk about revisions. But I think using the word "published" could be confusing in the case where the default revision isn't in a "published" state (i.e. the node isn't published at this time at all)?

So maybe:

You can only change the book outline for the default revision of this content.

amateescu’s picture

Status: Needs work » Needs review
FileSize
22 KB
3.02 KB

@jhodgdon, when someone clicks 'Save and create new draft' I think it is easier for them to connect the 'new draft' part to something that's not published, while 'default revision' doesn't really mean anything to the casual user. So I went with @catch's proposed text for now...

jhodgdon’s picture

That makes some sense... But what happens if the node is not published at all -- can there be a default revision, or does the concept even apply if none of the revisions is published? What does the UI say in that case? And can you edit the non-default revision?

And in that case, can you even add the unpublished node to a book outline? ... I think the answer to that last question is yes -- at least on drupal.org in D7 when we used to use the Book module for documentation nodes, we could definitely put an unpublished node in a book outline.

jhodgdon’s picture

OK, answering my own questions:
- Started up simplytest.me with 8.4.x, Standard install profile
- Turned on Book module
- Edited Book page content type to be unpublished by default, create new revision by default when editing.
- Created a Book page content item, made it be a top-level book, saved as Unpublished. [button says "Save as unpublished"]
- Created a second book page, made it also a top-level book, also unpublished.
- Created a third book page. Initially added it to book 1, saved as unpublished.
- Edited the third book page, changed body. [button says "Save and keep unpublished"]

OK, I don't see any ability to edit revisions here. The issue summary doesn't say anything about additional modules to turn on. I guess maybe the issue summary needs an update maybe?

Anyway... On the theory that this bug only applies to editing drafts, I looked in Experimental and turned on Content Moderation (which also turned on Workflows). I then went to Content types, and added the default Editorial Workflow to the Book page content type.

Now when I edit node 3, the button says "Save and Create New Draft" (note wonky non-standard capitalization!). However, I still do not see a way to edit anything but the latest revision. It looks like I can edit that revision, and publish it, and revert it. I don't see anything special happening with Workflows / Content Moderation...

So.... Not sure what is going on here. How do you actually go edit a non-default revision anyway? I'm logged in as user 1, so it should not be a permissions problem...

Also, I'm not sure the UI of the Book module should be changed to match the UI of the default configuration from an experimental module (Workflows and/or Content Moderation).

amateescu’s picture

@jhodgdon, yes, the issue summary doesn't mention that you need to enable Content Moderation in order to create new drafts (forward revisions).

So.... Not sure what is going on here. How do you actually go edit a non-default revision anyway?

When you have CM enabled and click the 'Save and Create New Draft', that will create a non-default (forward) revision. Then when you edit the node edit, you will be editing a non-default revision :)

Edit:

Also, I'm not sure the UI of the Book module should be changed to match the UI of the default configuration from an experimental module (Workflows and/or Content Moderation).

Not sure what you mean by this. Can you expand your concern a bit?

jhodgdon’s picture

This patch is for the Book module.

The UI of "Save and Create New Draft" and "Restore to Draft" is apparently from the default configuration that is supplied for the "Editorial workflow" in the experimental Content Moderation module:

http://cgit.drupalcode.org/drupal/tree/core/modules/content_moderation/c...

People can easily create their own workflows using the Workflows UI -- this is just the default workflow that is supplied when you install the Content Moderation module. I do not think that this experimental module would pass Usability review yet either -- for one thing, these buttons do not use the default capitalization like the rest of Drupal core (should be "Save and create new draft" and "Restore to draft").

But I guess that the issue summary says you have published one node and are creating a new draft, so I guess that my comment above doesn't apply, and the UI saying "published" is probably appropriate... you can go ahead and ignore my last few comments. :)

catch’s picture

@jhodgdon while it's theoretically possible to have forward revisions when the node itself is unpublished, it's not likely to happen with content_moderation via the UI.

pwolanin’s picture

+++ b/core/modules/book/tests/src/Kernel/BookForwardRevisionTest.php
@@ -0,0 +1,79 @@
+   * Tests the book_system_info_alter() method.

This code comment does not look correct?

amateescu’s picture

Status: Needs review » Needs work

The last submitted patch, 33: 2858431-33.patch, failed testing.

yoroy’s picture

For the message, what about a mashup of what @jhodgdon and @catch suggested:

This is not the default revision. You can only change the book outline for the <em>published</em> version of this content.

Maybe using "default revision" together with "published version" helps to understand a bit better what's going on when you run into this. I opened #2875154: Clarify and document wording around drafts, revisions, published & friends.

(FWIW, I agree with "only" fixing the critical leakage here.)

amateescu’s picture

Status: Needs work » Needs review
Issue tags: -Needs issue summary update
FileSize
22.11 KB
2.24 KB

Updated the message with @yoroy's suggestion.

dawehner’s picture

+++ b/core/modules/book/src/BookManager.php
@@ -249,6 +249,13 @@ public function updateOutline(NodeInterface $node) {
 
+    // Prevent changes to the book outline if the node being saved is not the
+    // default revision.
+    if (!$node->isDefaultRevision()) {
+      drupal_set_message($this->t('This is not the default revision. You can only change the book outline for the <em>published</em> version of this content.'), 'error');
+      return FALSE;
+    }
+

I'm quite confused that we use drupal_set_message in an API service. Are we sure there is no better way to handle this?

amateescu’s picture

We could take out the drupal_set_message() call from the API function and put it in the various form submit handlers that are calling \Drupal\book\BookManager::updateOutline(). Do you think that would be preferable?

dawehner’s picture

Yeah I would totally say so :) I guess we should not call the method in the first place, when we don't intend to update it.

amateescu’s picture

I just tried to do #38 but, alas, we can't because the book module relies only on API functions (actually, hooks) to update the outline when a node is inserted or updated: book_node_insert() and book_node_update()

So if we take out the drupal_set_message() call from \Drupal\book\BookManager::updateOutline(), there's no other way to inform the user that the book changes were not applied!

This problem circles back to the fact that Content Moderation decides to make a revision non-default way too late, so I opened #2883868: Content Moderation decides to set a new revision as the default one way too late in the entity update process to discuss and fix that. Until that issue is fixed, I'm afraid I don't see any way around #37.

vijaycs85’s picture

Patch in #36 still applies cleanly. Here is some review comments.

  1. +++ b/core/modules/book/src/BookManager.php
    @@ -249,6 +249,13 @@ public function updateOutline(NodeInterface $node) {
    +      drupal_set_message($this->t('This is not the default revision. You can only change the book outline for the <em>published</em> version of this content.'), 'error');
    

    Probably a todo or @see to reflect the discussions in #38 and #40

  2. +++ b/core/modules/book/src/BookManager.php
    --- /dev/null
    +++ b/core/modules/book/tests/src/Functional/BookContentModerationTest.php
    
    +++ b/core/modules/book/tests/src/Functional/BookContentModerationTest.php
    @@ -0,0 +1,83 @@
    +  public static $modules = ['book', 'block', 'book_test', 'content_moderation'];
    

    Are we sure the test should be in book module instead of content_moderation? This is the first test class enables content_moderation outside content moderation (experimental) module in core.

  3. +++ b/core/modules/book/tests/src/Functional/BookTest.php
    @@ -15,6 +13,8 @@
    +  use BookTestTrait;
    

    Nice clean up!

vijaycs85’s picture

Issue summary: View changes
SKAUGHT’s picture

Status: Needs review » Needs work
amateescu’s picture

Status: Needs work » Needs review
FileSize
22.21 KB
723 bytes

Re #41:

1. You're right, we should leave a @todo there :)
2. Yup, why not. The tests for book integration with Views are also in the book module.
3. :D

timmillwood’s picture

+++ b/core/modules/book/src/BookManager.php
@@ -249,6 +249,15 @@ public function updateOutline(NodeInterface $node) {
+      drupal_set_message($this->t('This is not the default revision. You can only change the book outline for the <em>published</em> version of this content.'), 'error');

This message is thrown even if the outline hasn't changed.

Also, I'm not sure about the wording. It first mentions about not being the "default version" then talks about only being able change the "published version". Then with Content Moderation they can't go back and edit the published version, they can only edit the latest. So I think we need to say, publish to update the outline. Although that's not strictly true, they just need to change to a moderation state which updates the default revision, which might not publish the entity, or even be the published state.