AFAICT, only 'administer nodes' permission allows deletion of book pages.

This patch allows users with 'maintain books' or 'edit own book pages' permission to delete book pages. The logic doesn't check if the node has updates pending. Should it?

CommentFileSizeAuthor
#24 delete_own_book_pages_0.patch657 bytescoldfly
delete_own_book_pages.patch626 bytesezheidtmann

Comments

ezheidtmann’s picture

ezheidtmann commented

It's a patch, duh.

rivena’s picture

rivena commented

If maintain books is a permission that overlaps other permissions, it should say so. For example, maintain books (create, edit, and delete all book pages).

While I agree that someone who has been given maintainer status should be able to delete pages, I wonder if 'delete own pages' should be made into a seperate permission. I could see a situation where I would not want someone to be able to delete their own pages.

Do these permissions affect things that are not book pages but have been put into books?

Anisa.
It's Golden Week!

ezheidtmann’s picture

ezheidtmann commented

I think it's clear that "maintain books" implies the ability to edit and delete book pages.

No other node module provides a "delete own " permission, so it would be inconsistent for book.module to provide it. Can you provide some examples to justify the inconsistency?

These permissions do not affect nodes which aren't book pages but are in a book. Those nodes are controlled by their respective type-specific permissions, but only users with "maintain books" permission can add or remove those nodes from books. I believe this is the correct behavior.

rivena’s picture

rivena commented

Hmm... But, you just said that until this patch, it *didn't* include the delete pages permission. Right? So, now you are changing it's behavior, but leaving the wording the same. How will people know? Or does it not matter, because you presume they didn't really know it didn't include delete in the first place?

I don't know that I can defend my idea of having a seperate delete own pages permission against a standard of consistency. If all the other edit own permissions include the delete pages permission, then I have no quibbles. :)

Except it would be more userfriendly if I knew what the permissions meant before I granted them to someone, without having to look at the code. But I do realize that is completely out of the scope of the patch. :)

Where do all these little changes *go*, anyway? The changelog just says things like... refractored the search module.

Anisa.

ezheidtmann’s picture

ezheidtmann commented

I believe that "maintain books" permission implies the ability to delete book pages, so this patch fixes that bug.

Other node modules include deletion in the "edit own" permission. For example, page.module:

  if ($op == 'update' || $op == 'delete') {
    if (user_access('edit own pages') && ($user->uid == $node->uid)) {
      return TRUE;
    }
  }

I agree that it would be better if permissions were clearer about what they implied. There was a discussion started by killes a while ago:

One problem with Drupal is the way it handles user permissions. The
permissions are attached to the user object but nobody really knows what
they do. To find out what a particular permission allows you to do, you
often need to have a look at the code.

I don't know if it ever lead to any code.

I think the changelog is meant to list the changes since the previous version without too much technical detail. If you want to see every last change, take a look at the cvs messages.

rivena’s picture

rivena commented

I've been looking at the book module permissions recently, and did a bit of testing. I wanted to know what was the minimum permissions needed to create a new book. I assumed based on this discussion, and a similar discussion on the drupal support list that this was 'maintain books' and 'administer nodes'. I ran into a problem, then added 'access nodes' to this.

So now my user has no other permissions other than 'maintain books', 'administer nodes' and 'access nodes'. You'd think this is more than sufficient to do something simple like create a new book, but in fact, with just these permissions, you do not have access to create a book page. You can edit other pages, and move them into a new book.

Once you have create pages permission, you appear to finally have all the permissions needed, edit own pages is not needed.

So, since, the point of this patch was that not having delete permission was odd, might I suggest adding create permission to maintain books? ;p

Anisa.

Bèr Kessels’s picture

Bèr Kessels commented

I would say this is a "by design".
Maybe we need to rename the two book permissions into:
"create books" and "edit own book pages".
None of the other nodes can be deleted by the roles with "edit own Foo" permissions, nor should the books.

ezheidtmann’s picture

ezheidtmann commented

Thanks for your review, Ber.

story.module allows users with "edit own stories" permission to delete stories.

  if ($op == 'update' || $op == 'delete') {
    if (user_access('edit own stories') && ($user->uid == $node->uid)) {
      return TRUE;
    }
  }

Therefore it is not inconsistent for the same behavior to apply to book nodes.

If "maintain books" really means "create books", then the name should be changed and existing code in book.module needs to be updated. However, I think "maintain books" should allow more than just that. In my situation, I want to be able to give someone the permissions to create books and to create, edit, and delete all book pages. With my patch, "maintain books" and "create book pages" does that.

Bèr Kessels’s picture

Bèr Kessels commented

What i see from story, that is specific to story.module too.

We should really settle for a consistent behaviour for *all* nodes, one that is "enforced" from node.module. These things should really not be decided per node-type and per-module. Thats a recipe for a mess.

Eric Scouten’s picture

Eric Scouten commented

I agree that the current system is a mess.

I disagree that there should be one consistent policy enforced by node.module. Sites may have quite legitimate reasons to have different policies for different node types.

Bèr Kessels’s picture

Bèr Kessels commented

What i was aiming at is to let node module take care of the following:
* edit $node_type
* administer $node_type
* delete $node_type (but would that not better be in 'administer';: users who can administer can delete)
* create $node_type

That way the modules need not cre about this. ut they /can/ take care of it, if they wish, and add more rules and permissions.

Bèr Kessels’s picture

Bèr Kessels commented

oh, and -forgot- :
* view $node_type

Eric Scouten’s picture

Eric Scouten commented

Thanks for the clarification, Bèr. I was afraid that you meant that there would be a single policy applied to all node types, not a policy per node type.

Now I think we're in complete agreement on this subject.

puregin’s picture

puregin commented

I think it's important to keep a clean separation between content and structure (e.g.,
what's inside of the node, versus how the node is associated with other nodes)

I think that there will likely be other explicit structures in Drupal over time, so these concerns affect more than just book module.

Deleting a node from a book is potentially a problem, since this operation affects the structure
of a book.

We also need to think about other interactions with structure - for example, what happens
if someone unpublished a node in a book? Should all children also be unpublished? Should the node contents be hidden, while children remain available in navigation?

Bèr Kessels’s picture

Bèr Kessels commented

puregrin. I think you are gettnig OT here, Thought it is a very valid (and problematic) issue, It really belongs in a new thread.

puregin’s picture

puregin commented

What I was saying is that whether a user should be able to delete a node from a structure should really be enforced by the module which is responsible for the structure.

For example, imagining a simple linear list structure - let's say for a "wish list" - composed of nodes owned by a bunch of different users. In this example, a user could delete his own node with no problem.

For the book module, a users node may have children authored by other users. Letting an arbitrary user delete his own node would be, I think, a mistake, since the children would then be orphaned.

I suppose one could imagine other modules which rely on a hierarchy, which might have a different policy (e.g., automatically attach children to the parent of the deleted node, if such a node exists). Perhaps eventually two 'tree-based' structure modules might inherit from a tree structure module, each enforcing separate policies regarding node operations.

The point is, that one should not have to modify node module when a new structure is defined. Node module should not have to know about how composites of nodes work.

boris mann’s picture

boris mann commented

puregin: I was talking about this today, so I created a task to remind us to build a generalized node relationship module/schema.

Crell’s picture

Crell commented

I think it's clear that "maintain books" implies the ability to edit and delete book pages.

I'm afraid I have to disagree with clydefrog on this one. The words "administer" and "maintain" and "manage" don't give any firm indication of what they mean, in and of themselves. They could vary depending on the context. Some nodes are deleteable in the fist place, some aren't. In that case, "maintain" means two different things, as would anything else you put in there unqualified.

This isn't just a problem with the books module, but in general. It is not intuitively obvious what "administer nodes" or "administer X" means. That's a problem, especially when, as I said, it can vary from one node type to another.

I see, generally, the following types of operations on nodes:

- Create
- Change Ownership
- View Anyone's
- View Own
- Edit Anyone's
- Edit Own
- Delete Anyone's
- Delete Own

Which of those is covered by "administer" or "maintain"? And what if I want to allow someone to, say, edit their own nodes, but not delete them? Or allow a given class of administrator to edit anyone's nodes but not delete any but their own?

I'd actually be much in favor of more fine-grained control over nodes access rights, both for the greater power it offers and for the clearer UI, system-wide. Whether that's best done via a central node API system or via a "recommended permission breakdown" I don't know, and will leave that to those who have spent more time with core than I have. :-) I do believe it's a good idea, however, both for greater flexibility and greater learnability.

noid’s picture

noid commented

Just to clarify things -- based on tests I did with the book module --

(the following hold true for regular users with no "administer nodes" permission)

1) the "edit own book pages" does not give the user the option to delete his own pages.

and

2) "maintain books" doesn't do anything.

This is the state of things right now, right?

If this is so, may I suggest that the initial fix be simple in the meantime which should include:

1) "edit own book pages" means the user can also delete his own pages.

2) "maintain books" means the user can edit (but not delete) pages created by other users.

********************

I've also noticed that "create book pages" (or "edit own book pages" and "maintain books" for that matter) doesn't allow the user to create a top-level page -- there's no option for it in the pulldown. Can this be fixed so that "create book pages" allow the user to create a top-level page (that is start his own book)?

noid’s picture

noid commented

My bad. "Maintain books" works after all.

Tested the permission again and found out that a user with the aforementioned access can edit some of the nodes after all, and these nodes were created after a certain date. So I looked into the modules I installed after that date, and, to cut to the chase, found out that the culprit was TinyMCE. Well, indirectly, that is. :)

Turned out I was using a new customized input format -- per advice of TinyMCE install instructions -- and the older nodes that were created with the default filtered HTML format couldn't be edited. So all I have to do now is change the input format of these older nodes to the new input format I set for TinyMCE. :)

At any rate, perhaps someone can figure out why the new format made the book module act that way. :)

ezheidtmann’s picture

ezheidtmann commented

Your site behaved in that way because the users with "maintain books" permission did not have permission to use the input format defined for those nodes. This is a general feature of Drupal, not specific to the book module.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Closed (works as designed)

please open a new issue if there is a real problem here. this issue is now clutterred.

coldfly’s picture

coldfly commented
Version: x.y.z » 5.2
Status: Closed (works as designed) » Reviewed & tested by the community

I wonder why this patch is not include in the latest release

coldfly’s picture

coldfly commented
StatusFileSize
new delete_own_book_pages_0.patch657 bytes

for version 5.2
something should be modified in the patch

drumm’s picture

drumm
he/him
Location NY, US
commented
Status: Reviewed & tested by the community » Closed (works as designed)

An upgrade on a stable branch should generally not hand out more permissions. I will reconsider if someone else reviews this, do not set your own patches to 'ready to commit.' Another option is to add a new permission for delete, but that is not too good for a stable branch either.

As Moshe said in #22, further discussion is best in a fresh issue, not here.

guillaumeduveau’s picture

guillaumeduveau
Location Toulouse
commented

Recently I have installed Drupal for a scientific team that has to edit a document and someone just asked me to let him delete his own pages. So in my opinion a user with 'edit own book pages' should be able to delete his own pages and I'd agree to say it's a bug.

Thanks for your patch, it works well with version 5.3.

The Fiddler’s picture

The Fiddler commented

I am developing an open source project, using the Book module as a collaborative documentation effort. The issue with not being able to delete book pages just came up, so this functionality is indeed welcome.

Will try this patch in 5.3. Any possible security problems?

Justin Freeman’s picture

Justin Freeman commented

I think this is one of those features which needs to be revisited in newer versions of Drupal (D7?). The current delete permission is counter-intuitive. I solved this problem in the current D5 release by using this module, http://drupal.org/project/content_access

But it would be better if there was an explicit 'delete book pages' permission or a 'manage books' permission.