[drupal-devel] [bug] Users with 'maintain books' or 'edit own book
pages' permission cannot delete book pages
lgarfiel
drupal-devel at drupal.org
Fri Aug 12 00:43:00 UTC 2005
Issue status update for
http://drupal.org/node/21559
Post a follow up:
http://drupal.org/project/comments/add/21559
Project: Drupal
Version: cvs
Component: book.module
Category: bug reports
Priority: normal
Assigned to: Anonymous
Reported by: clydefrog
Updated by: lgarfiel
Status: patch (code needs review)
/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.
lgarfiel
Previous comments:
------------------------------------------------------------------------
Fri, 29 Apr 2005 04:32:19 +0000 : clydefrog
Attachment: http://drupal.org/files/issues/delete_own_book_pages.patch (626 bytes)
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?
------------------------------------------------------------------------
Fri, 29 Apr 2005 14:10:22 +0000 : clydefrog
It's a patch, duh.
------------------------------------------------------------------------
Fri, 29 Apr 2005 14:21:27 +0000 : rivena
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/!
------------------------------------------------------------------------
Sat, 30 Apr 2005 03:43:17 +0000 : clydefrog
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.
------------------------------------------------------------------------
Sat, 30 Apr 2005 11:03:47 +0000 : rivena
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.
------------------------------------------------------------------------
Sat, 30 Apr 2005 16:04:14 +0000 : clydefrog
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:
<?php
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 [1] 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 [2].
[1] http://lists.drupal.org/archives/drupal-devel/2005-04/msg00704.html
[2] http://drupal.org/project/cvs/3060
------------------------------------------------------------------------
Mon, 23 May 2005 17:33:18 +0000 : rivena
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.
------------------------------------------------------------------------
Mon, 08 Aug 2005 10:37:49 +0000 : Bèr Kessels
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.
------------------------------------------------------------------------
Mon, 08 Aug 2005 16:59:30 +0000 : clydefrog
Thanks for your review, Ber.
story.module [3] allows users with "edit own stories" permission to
delete stories.
<?php
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.
[3]
http://cvs.drupal.org/viewcvs/drupal/drupal/modules/story.module?rev=1.167&view=auto
------------------------------------------------------------------------
Mon, 08 Aug 2005 19:16:35 +0000 : Bèr Kessels
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.
------------------------------------------------------------------------
Mon, 08 Aug 2005 19:25:27 +0000 : Eric Scouten
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.
------------------------------------------------------------------------
Mon, 08 Aug 2005 20:35:59 +0000 : Bèr Kessels
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.
------------------------------------------------------------------------
Mon, 08 Aug 2005 20:36:32 +0000 : Bèr Kessels
oh, and -forgot- :
* view $node_type
------------------------------------------------------------------------
Mon, 08 Aug 2005 20:49:31 +0000 : Eric Scouten
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.
------------------------------------------------------------------------
Mon, 08 Aug 2005 20:51:13 +0000 : puregin
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?
------------------------------------------------------------------------
Mon, 08 Aug 2005 21:08:01 +0000 : Bèr Kessels
puregrin. I think you are gettnig OT here, Thought it is a very valid
(and problematic) issue, It really belongs in a new thread.
------------------------------------------------------------------------
Mon, 08 Aug 2005 22:57:34 +0000 : puregin
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.
------------------------------------------------------------------------
Tue, 09 Aug 2005 00:45:07 +0000 : Boris Mann
puregin: I was talking about this today, so I created a task [4] to
remind us to build a generalized node relationship module/schema.
[4] http://drupal.org/node/28480
More information about the drupal-devel
mailing list