Access checks returning NULL is ugly.

Tested: applies cleanly and works as advertised. Reviewed: small, logical, safe, and follows code style.

The only possible downside is further mushrooming of the out-of-control user access grid, but that's no reason not to have more fine grained permissions. I agree it's wise to separate edit from delete, but only allowing folks with 'administer nodes' to be able to delete would be worse.

It is a good point to decouple deletion from editing permissions IMHO. But previously it was possible to hand control over the remaining lifecycle of 'own nodes' to users. So if she wants to edit it, fine. If she wants to delete it, fine. Now this breaks that one and the 'own node' editing permission is less powerful. Whether this is good, depends on the actual use case of course. I am not entirely comfortable with adding a 'delete own $type content' on top of what we have already, to make up for the loss, but I don't have a better idea now. Maybe you have.

I really, really need this seperation on my site - user can edit their own comments, but if we give them edit own forum topic permissions they could wipe out 1,200 comments from other users in one go. Since they don't distinguish between topics and replies (nodes and comments) this makes things very confusing for some users.

+1 for having delete dependent on edit permission, that makes plenty of sense.

Suggestions for naming the permission:

delete (requires edit).
delete (depends on edit).

Oh, dear. Please, -1 to 'delete '. $type .' content if edit allowed'. Imagine the support questions.

This is excellent functionality, but Gabor brings up a very good point.

What's wrong with 'delete $type content' and 'delete own $type content' ? Permission bloat? Maybe. But at least it's consistent and clear (although personally, I'd rather those permissions be called 'delete/edit/create all $type content' for ultimate clarity, but I don't want to hijack this patch).

What's wrong with just "delete $type content" but document that edit perms are required? The delete button is only available on the edit tab. Let's keep it that way. Just explain somewhere that you need edit perms on a given node to be able to delete that node. I don't think people will be confused by that. And I can't imagine a use-case where a site needs to configure a role that can delete but not edit content.

dww: Delete links could be / are printed at other places, there is not a delete button only on the edit page. I agree that the use case of only allowing delete without edit is weak, but the use case to allow edit without delete is quite strong. I am not sure that introducing dependencies between permissions is a good idea though. Is there some precedent for this in core? (I am the type who setup up his permissions and forgets about it :)

Otherwise, "delete own $type content" as webchick suggested and I lamented about is better then conditionally restricting "delete" to "own content" if a broader edit permission is not given. That would be very confusing for users IMHO.

I'm coming down on the 'delete $type content' and 'delete own $type content'. It might be extra permissions, but it's very clear and would minimise confusion.

applies with a couple of lines fuzz, tested both with and without delete permission - works as advertised on both.

It's an extra permission, but it's a really important permission and not an area to skimp.

So RTBC.

I sure hope hunmonk's initial jQuery efforts to improve the Sea-O'-Checkboxes UI on admin/user/access is revived, then. This page is already insane, and 2 more perms for each custom node type will make it worse. However, it's already insane, and I agree that a) the fine grained perms are important and b) implicit permission dependencies probably will be more confusing than an extra N rows in this grid.

Still applies with minor fuzz. Code is clean. RTBC.

Committed!

Mind you that this also removes the "Preview changes" button. Correct me if I'm wrong though.

Hello!
Could someone make this work for drupal 5.2? I really need to hide the "delete" button from certain groups, but let them edit the node.
Thank you!

@ark2424 -- Please understand how the issue tracker works here. "Won't fix" means the core maintainers have decided not to fix a given bug or feature request. Moving an issue into that status effectively kills the issue. Furthermore, just because you think it's important doesn't make this critical. Critical bugs are when fundamental functionality stops working, data corruption is possible, etc. This is a bug in the UI, but it's hardly critical -- things still work, even if not as nicely as we'd like. The version should remain 5.x-dev, since a patch that backports this change to the 5.x series will have to be made against the latest code in the DRUPAL-5 branch, not just the specific code that was released as 5.2. And, if there's *ANY* hope at all that this change will make it into the 5.x series, it's by considering this a bug. We never add new features to existing stable releases of Drupal.

So, in summary, every change you made to this issue was inappropriate. ;) But, hopefully this reply will help you learn about how the process works so that you don't make similar mistakes in the future.

Cheers,
-Derek

p.s. @Laurentvw: The "preview changes" button is care of the diff.module. If something doesn't work with that button, please file an issue in the diff.module issue queue. Thanks.

Here is a copy of the patch that should work for the latest version of 5.x. However, this is the first patch I ever made, so I apologize in advance if I did something wrong ;) Thanks!

Yup, that's it, nice work! Code is good, applies cleanly, and I just tested it all on a 5.x test site and everything is working as expected. So, IMHO, this is RTBC. However, I can see how the introduction of new permissions in the middle of a stable series might be frowned upon, in which case moving this back to 6.x-dev/fixed would be understandable. However, it really is a problem that our existing permissions in 5.x aren't fine grained enough to handle this, and it's impossible to override via contrib...

I'd support it going into 5.x we've need it for a while, doesn't break anything, very handy to have.

The only way this would be acceptable for 5.x is if there were a DB update to make the given permissions the exact same after these are added. The delete permissions would need to be given to roles which previously had the corresponding edit permissions.

However, I am not too supportive of adding permissions in a stable branch.

Hello... here is a stab at a new patch that updates the database as drumm suggested. I tested it and it seems to work, but someone with more experience should definitely take a look. I also included a drupal_set_message() that informs the site admin of the new permissions after the upgrade is done. Hopefully that will make the adding of new permissions more palatable. Either way, I think this is a serious bug that needs to get fixed somehow...

Well... anyone?...

I think the old patch doesn't apply any more, but here is a newer version that does.

@David_Rothstein: Thanks, that's a good start. However, there are a few things I'd do differently to try to get this in core:

A) $edit_perms and $edit_delete_perms should be initialized as arrays before you loop over the content types to prevent a PHP warning.

B) All the stuff with the leading and trailing commas isn't necessary. If you just replace 'edit foo content' with 'edit foo content, delete foo content', it doesn't matter where 'edit foo content' lives in the perm string in the {permission} table, the resulting modified string will still be correct. The only reason to do all the extra mojo with the commas is in case some other permission that contains "edit foo content" as a substring is defined somewhere in contrib. I'm not sure if the additional complication in this update is worth protecting against this possibility, but I could certainly be wrong.

C) I'd only UPDATE {permission} if $old_perm != $new_perm.

D) I'd change the wording on the drupal_set_message() so that it doesn't make this sound like a new feature. ;) I'd say something like:

"Drupal now has separate edit and delete permissions. Previously, users who were able to edit content were automatically allowed to delete it. Your database has been updated so your site continues to behave as it did before. However, if you would like to restrict which roles are allowed to delete content, you can modify the permissions at the <a href="@access_url">Access control page</a>."

E) Also, just because that message is generated doesn't mean there's an error updating the DB. So, you shouldn't pass 'error' to drupal_set_message().

Like so... Here's a version without the comma mojo, which is simpler code, but arguably less fool-proof in the face of insane contrib modules.

And here's another that maintains the comma insanity to defend against contribs with funny permission names. Wow, this whole thing would be so much easier if we normalized the {permission} table... http://drupal.org/node/73874

Oh, and all of the above patches make the records in {permission} unnecessarily long if you run this update more than once. :( Again, that'd be *trivial* to get right with a normalized {permission} table, but will double the length of this update if we try to protect against this as things currently stand in D5. I don't think I care, I'm only pointing out the pain we suffer as a result of the existing schema as more evidence that http://drupal.org/node/73874 should be "active", not "won't fix"...

Thanks, @dww, that's much improved! I'm learning...

The attached version fixes a small grammatical mistake in drupal_set_message but otherwise is the exact same as yours.

I agree that the "comma mojo" is a little ugly, but the reason I put it in is exactly as you described. If someone has a custom module that defines a permission "edit story content with reckless abandon", we want to leave that alone -- we don't want to replace it with two separate permissions "edit story content, delete story content with reckless abandon" ;) It does make the code a little uglier, but I think it's better to have ugly code than to potentially break someone's site, so I'm glad you put it back.

I, too, noticed that the patch introduces duplicates into the {permission} table if it's run more than once, but I agree that it's not worth the effort to fix this. First of all, Drupal already tells people not to run the same DB update twice, so they've already been fairly warned that doing so may cause problems, right? Second, I don't think (correct me if I'm wrong) that running it more than once would actually break anyone's site? It seems to me that they would just have some permissions listed twice in the table, but everything would still work, and the next time they changed their permissions it would go back to "normal" anyway...

Upon further thought, here's a version of the patch that restores the 'error' designation for the drupal_set_message. (If my reasoning doesn't sound good, feel free to just go back to the previous version).

I think the drupal_set_message should be labeled an 'error' to the same extent that the problem we're solving is labeled a 'bug'. Specifically, the bug is that currently, Drupal has hidden, implied delete permissions -- when you give someone edit permissions, you're automatically giving them delete permissions too, and Drupal never tells you that's what you're doing. So there are probably people out there whose sites give more permissions to their users than they actually think they're giving (for example, me... that's the exact situation my site was in a few weeks ago when I came across this bug!). The DB update does not fix these people's problems directly; it just gives them a way to fix it. So the point of making it an error message is to say "Hey, your site may not be working like you think it is, but you can fix it if you want."

I think the important thing here is that the drupal_set_message should have visual attention drawn to it -- unfortunately, a regular drupal_set_message on the update.php page looks very ugly and is easy to miss. So maybe there's a way to deal with this using theming or something, but I think calling it an 'error' is probably the simplest way.

Reviewed #30. Tested update 1023 again. I agree that the 'error' is better for visibility, and I agree that it should be flagged as a "hey, watch out for this" style message, instead of just a "by the way: blah, blah, blah" message. It's a little too bad from a strictly-semantic standpoint, since there's not really an *error* with the update, especially since we'll almost always have updated at least 1 role to add the new permissions. But, I guess I can buy the "hey, your site was vulnerable to this potential bug with your permissions you didn't know about, and now you can fix it" motivation for calling this an error message. It's by far the simplest way to draw visual attention to the message, too. Yes, running the update twice adds some cruft to the {permission} table, but that will get cleaned up the next time an admin saves the access control page, so it's not really a big deal. I don't see any more problems in here, so this is RTBC. Thanks, David.

Any comments on what I said in #21: "I am not too supportive of adding permissions in a stable branch"?

Why should the stable branch be getting more permissions?

Because Drupal believes in fine-grained permissions for enhanced security. Currently, many sites want to be able to separate delete from edit, but core is too coarsely grained to allow this. We're only adding new permissions to enable sites to more tightly and securely control access to their content. Some could argue that many sites are now vulnerable to an access bypass vulnerability, since not all site administrators understand that granting edit permission allows delete, too.

However, I'll leave this for someone else to RTBC, since it's a patch I helped write and you already know I support it's inclusion.

I am not sure suddenly losing delete permissions with a point update in Drupal 5 is a good idea after all.

As someone who was bitten by this bug in the first place, I think the argument goes like this:

Drupal relies entirely on the NAME of the permission to describe to the site administrator what the permission does (if there were some kind of "description" field associated with each permission, this bug could easily be fixed without changing anything else, but there isn't so it can't).

That leaves two options for fixing the bug:
(1) Change the name of existing permissions from "edit ___ content" to "edit and delete ___ content" so they more accurately reflect what the permission is.
OR
(2) Separate "edit" and "delete" into separate permissions (what we did with this patch).

Both are about equally complicated, and neither has any serious side effects. (In fact, #1 might have some minor side effects that #2 doesn't -- what if someone has written custom code that relies on the "edit ___ content" permission to exist??)

Given the above, it would seem an awful shame not to go with #2, which opens up a HUGE amount of functionality for free. Gábor, I may be misunderstanding what you're saying, but this patch will not take away anyone's delete permissions -- the database update ensures that all roles who had delete permissions before the patch will continue to have them afterwards. People who have no interest in the extra functionality can safely ignore it and their Drupal 5.x sites will behave exactly as they used to. In that sense, although the patch is technically adding some extra functionality, it does so (a) in the context of fixing a bug, and (b) in a way that doesn't alter the "stable" nature of the Drupal 5.x branch (at least as far as I can tell).

Existing contrib code could just as well depend on calling user_access("edit ...") before doing a delete, so a change in permission meanings might give false security to site admins. (I know it is already called "edit" only, but then there would be a separate delete, but "edit" could still mean delete still).

Hm, good point.

So it sounds like the choice is between:
(1) Having "edit" permissions be misleadingly labeled but consistent across Drupal. That is the current state of affairs.
(2) Having "edit" permissions be accurately labeled and consistent across Drupal core, but potentially inconsistent and very misleading when combined with certain (hypothetical) contrib modules. That is what would happen if the patch is applied.

I guess it's up to the powers that be to decide between these options ;) We can partially address the problem in #2 by changing the "drupal_set_message" in the patch to clearly state that the changes only apply to Drupal core, not anything else, but of course that doesn't really solve the problem, only explains it.

You know, it's been said several times in the above discussion that this functionality can't be accomplished with a contrib module... but is that really true? Has anyone looked at the Nodeaccess module? Unless I'm missing something, that does a perfectly fine job of separating edit and delete permissions. A site admin can disable the node module's edit permissions for a given content type, and then grant edit/delete permissions separately for that content type using Nodeaccess. I only tried this out quickly, but it seems to work fine.

I don't think this changes the fact that this is a bug with Drupal core, but it may be another way for people to get separate edit/delete permissions in Drupal 5 if they don't want to apply/wait for this patch...

I don't think it is a good idea to introduce this patch in a stable branch. It's a feature and features don't get into stable branches.

Gerhard,

This may just be my opinion, but I think it's pretty clear that using the word "edit" to silently imply "delete" is a bug -- with potential to cause real damage if the site administrator does not know about the hidden meaning. Anyone trying to use Drupal to run a wiki could be in for a nasty surprise.

The rest of the "feature vs bug" issue is discussed above. There certainly may be a good way to fix this bug that does not involve adding the new feature, but if so, no one has suggested it yet...

This bug has also been the default behaviour of Drupal since the far reaches of time. Changing it on people all of a sudden in the middle of a random stable release also seems like a rather nasty surprise. :\

Drupal 6 will probably be out within a couple months. I think it's fine to wait until then for the more fine-grained permissions.

I can imagine use cases which allow an editor or admin to delete but not edit content. As an author, I might not want an editor or admin putting words into my mouth by modifying what I wrote, invisibly to the viewing public. I'm sure with a bit of thought, other use cases can be found. POSIX/Unix file permissions work this way, by the way -- and that's a pretty low bar as far as expectations of granularity go.

Changing historic default behavior in the middle of a stable release is generally a bad idea, agreed.

However, let's all admit that Drupal's permission system has also generally been very poor. Relying on modules to call a hook and to interpret permission strings the same way (e.g. 'edit foo' means modify foo but not delete foo, or maybe edit foo means modify foo AND delete foo) is no way to get a reliable and sensible permissions system.

At the persistent object level, probably at the db_query() level, permissions should be enforced all of the time for all operations. Modules can modify permissions and their behaviors, but would no longer need to worry about enforcing them -- inconsistently. This simplifies things and makes Drupal more secure at the same time.

Building a secure and sane permission system requires cooperation and agreement on core code.

So, from the point of view of a drupal newbie, what is the best course of action if you desperately need to have edit/delete permissions separated ?

I was rather surprised that this was not a 'standard' feature, since creation, modification and destruction are part of the life cyle of any object and as such you'd expect the permission system to reflect that, I landed in this thread trying to figure out how to 'enable' that feature but now it seems that it may be a little more involved than that :)

For Drupal 5, I think the best solution is the nodeaccess module. I believe it lets you assign create, read, update, and delete permissions per role by node type or even by node. It is *not* a simple module, but for now I think it's the best option. That will probably be true for Drupal 6 as well at this point.

For Drupal 7, let's get a real access control system into core, since the current one is lame. :-)

This is highly confusing :) The thing I'm trying to achieve is to allow users to append images to an archive attached to a node but not to remove images by others or to edit the other fields in the node. The idea is to have a database of cities and then to allow people to attach their pictures of that city to the node for the city.

What I did was add a cck image field to the city nodes with 'allow multiple' set, this seems to work for the most part after fiddling with the permissions by field module for cck, but if I give someone 'edit' permissions they end up being able to wipe other peoples images as well as the complete record, which was not quite what I meant when I set it up.

Maybe there is a completely different way to achieve the 'attach image to node' effect that does not lead into these permission issues.

I just installed drupal6 and it seems to have the 'delete/edit' permissions on the node level separated, but most of the cck modules have not been ported yet so I can't see if that would solve it or not.

I'll have a look at 'nodeaccess' and see if that has the desired effect, thank you for the pointer!

some feedback on the 'nodeaccess' module. I've installed, and configured it, that went without a hitch. It has fine grained control, but there are two issues with it:

1) (probably the more serious one) it does not seem to work for all content types, I have a 'user defined' (cck) content type called 'city' and there it does not work but with the built in content types the edit tab no longer contains the delete button
2) for every node/role combination there will be an entry in the node_access table using this scheme, EVEN if you do not change the access combinations at all, in other words if every node of a certain content type is set the same then you will still have m roles * n nodes access rules in the node_access table. If it would at least work then that would be something I could probably live with, even though we're talking about 2.5 million nodes and 10 roles, so 25 million entries in node_access.

I assume this is how nodeaccess is supposed to work, and that the per-node permissions are really meant to be interpreted literally but that seems overkill just to get rid of a single 'delete' button... And I can't seem to figure out why it won't remove the delete button for the 'cities' content type but it does work for 'image'.

I'll keep plugging away at it, thanks for the help !

The thing I'm trying to achieve is to allow users to append images to an archive attached to a node but not to remove images by others or to edit the other fields in the node. The idea is to have a database of cities and then to allow people to attach their pictures of that city to the node for the city.

Oh! Then you don't want to be doing it that way at all. :-) Instead, make each image that is uploaded its own node, with an imagefield and a nodereference field that points back to the city node. You can do some massaging in Form API and hook_links() to make the submit form for your image node auto-populate the city reference field. Then just attach a Viewfield to the city node to list all related image nodes. Permissions solved.

We should still separate edit and delete permissions, but not for Drupal 5. :-)

You are correct, that did go in. For D7 I'm thinking something more along the lines that chx has been pondering with node groups and access groups, but that's the subject for another issue.

Since the feature went into D6, and it doesn't look like it's going to go into D5 (and there's a contrib that offers it instead), I am setting this issue back to fixed.

I wash my hands of this. I still think it's a bug that should be back ported, but apparently there's not enough support for that position, so I'll stop pushing.

Agreed, it's a shame there's no good way to get this into 5.x. Oh well.

However, I'm now taking a look at 6.x and noticed the following. The patch that went into 6.x (#10 way up above) did not include a database update. Furthermore, there is no documentation of the new delete permissions in CHANGELOG.txt or at http://drupal.org/node/114774.

This seems like a problem. I tested this and can confirm that updating a site from 5.x to 6.x causes delete permissions to be removed for all roles, with no notice to the site administrator and no guidance for how to restore them. The administrator has to navigate to the permissions page and discover on their own that edit/delete are now separate.

Perhaps it would be a good idea to take the DB update we wrote for 5.x and modify it for 6.x instead?

I'm willing to work on this, but only if it's something that people think will actually go in this time...

David, we discussed this, and decided that it is not a good idea to "widen" the permission set for any role automatically (think about Drupal 5 not having delete permissions, but people might think the edit permissions did not include it). Let administrators decide on who they provide delete permissions with. A notice on the update might be fine as it is used with update status module et al.

thank you Crell, I'll try to set it up that way and see if I can make it work.

This forum is probably drupal's best feature :)

OK, here is a patch that sets a notice about the changes to edit/delete permissions (but does not do any DB update).

Sorry I didn't get to this sooner... I see that a release candidate is out already (congratulations!), but I hope this can still go in. I definitely think it's important to inform the site administrator that the upgrade process is "undoing" some settings they may have been relying on.

We are not using t() in the update process (if you find any, file a bug report please :), so t() should not be used here. We basically cannot make assumptions on the state of translation support, and as these messages come with the new version, and the translations of the new version will only be imported later, they will not be translated in update time anyway.

Also it is not true that edit permissions were removed from *all roles*, as everybody with administer nodes permission can still do deletes just as before.

Should we use url() in there? I guess it'll then point to update.php?q=admin/user/permissions (untested) or cause other problems - as far as I see, update process uses things like base_path() .'?q=admin/user/permissions' instead.

OK, thanks for the tips. Here is a new attempt. I'm worried the text is getting a little long, but at least it should be more accurate this time.

As for t(), it actually looks like it's all over the place in the update process... I'll file a separate bug report.

Finally, I've tested this patch both with and without clean URLs enabled, and url() seems to work fine here. If it does need to be changed for some reason, it's worth pointing out that url() is used other places in the update process too.

looks fine to me.

- Hm, interesting that url() works in update.php, it used to link to update.php?q=dsflkjklj in non-clean URL mode, which is not right.
- What are "core content types"? This part is quite vague. Any content type which was created with the built-in content types interface is a "core content type" right? I am not sure you have a good name for this concept. Maybe this needs some more elaboration there?

Actually, I did a little investigating and it turns out that the "core content types" ambiguity is even worse. As things currently stand...

The following Drupal content types have separate edit/delete permissions in 6.x:
page, story, book, as well as anything defined via the administrative interface (basically any content types associated with the node module)

The following Drupal content types still only have "edit" permissions (with delete implied):
forum, blog

Finally, the poll content type only has edit permissions, but in this case it really just means "edit" (no delete permissions are granted).

That seems like a real mess. What it means is that the first sentence of my patch ("Drupal now has separate edit and delete permissions") isn't even correct -- some of the core modules separate edit/delete but others lump them both together under an "edit" permission. The word "edit" is not being used consistently throughout the core Drupal permissions system.

Anyone have any suggestions for what to do here? I would think (assuming it's not too late) that the sane thing to do would be to patch the forum and blog modules so that they also separate edit and delete permissions.... then everything would be consistent (which would also make it a lot easier to figure out what to write in this update notice).

Who would have thought such a simple patch could cause so much trouble? ;)

Also, about url(), I'm now wondering if it's even a good idea to include a link to the permissions page here. The problem is that the normal upgrade path is supposed to be:
1. disable 5.x versions of contributed modules
2. update to Drupal 6
3. enable 6.x versions of contributed modules

If we tell people to visit the permissions page between steps 2 and 3, I believe what will happen (assuming they save changes to the permissions), is that any permissions they had set for their contributed modules that might be hanging around the {permission} table will get deleted. So when they reenable their contributed modules for 6.x, any permissions settings that they had for these modules will be gone. This could be a real pain for people who use a lot of contributed modules on their site.

Perhaps the better thing to do is to tell people to add the new delete permissions only after they have finished the entire upgrade process...

While it looks a bit too late, I actually think the permission mess you identified makes this one a critical bug to fix (although I am not much into growing the finally shortening critical bugs queue). In the interest of security, we should fix the permission scope/name inconsistencies here ASAP.

Here is a quick patch. I only patched the forum and blog modules... if my understanding of the permissions system is correct, then the poll module is OK since poll_access() neither grants nor denies the ability to delete under any circumstance. Therefore I assume adding delete permissions to that module would count as a feature...

A) Gabor made it pretty clear he thinks that cleaning up the inconsistent mess across all of core is a critical bug for the 6.0 release. Yes, that means we'll have to introduce some new permissions in a few places, but that doesn't make it a "new feature" per se. Therefore, I think this patch should fix poll module, too.

B) It's too bad these are inconsistent with each other:

+  return array('edit own blog', 'delete own blog entries');

I know "delete own blog" might be confusing, but that seems better than this. Better still might be "edit own blog entries", but that'd involve a permission rename, which requires more work (a DB update to do the nasty task of renaming all the perms for existing roles, which is vastly more of a pain in the ass than it would be if the {permissions} table was normalized -- see above for some example patches that do this).

OK, the way I interpreted it was that the "critical" aspect of this issue is that the word "edit" means different things in different places. And the patch I wrote does fix that -- edit should now mean the same thing everywhere (including the poll module). However, I agree that adding delete permissions to the poll module would be good ;)

I also like "edit own blog entries", but if we did that then I think there would also need to be a new "create blog entries" permission. Right now "edit own blog" allows both creating new entries and editing old ones... I assume that's why it's named the way it is? I don't like "delete own blog" because it really sounds to me like it means there's some magic button that lets users delete their entire blog at once... whereas "delete own blog entries" is at least more accurate about what the permission is..

+1 for all blog permissions being "blog entries" (or maybe "blog posts"?).

book module (and maybe locale permissions) has errors due to buggy 5.x - 6.x update. There's a patch to fix the original update, but it'll need to be covered for rc1 - rc2 as well - so maybe merge into here?
http://drupal.org/node/206470

Leaving open for now anyway.

David_Rothstein: yes, I think we should introduce a delete permission in poll for consistency. IMHO these kinds of permission problem issues are important to clean up for security's sake. Users need to have a consistent and clean picture of what permissions mean.

So to clarify:

- do introduce a poll delete permission
- do not rename permissions

This makes us go without update functions (the delete permissions were introduced without update functions so that permissions are not widened but tightened on update instead). It is also quicker to complete, and fits with those possibly running on RC1 already.

A quick untested reroll of the previous patch with the poll module updates already done.

- I converted the directly wrapped if() { if() {}} contructs to swicth() { if() {}} constructs instead which lays itself much better for these kinds of permissions. Now poll and forum access functions look exactly the same (minus content type name).
- Blog still looks ugly...

Blog is so much tempting to clean up, but then we need an update function as well. I attach a second patch, which includes full coverage of blog module permissions and an update function, based on system_update_6039().

Quick look at the complete version: I think this:
$renamed_permission = preg_replace('/(?<=^|,\ )edit\ own\ blog(?=,|$)/', 'create blog entries, edit own blog entries', $renamed_permission);
should be:
$renamed_permission = preg_replace('/(?<=^|,\ )edit\ own\ blog(?=,|$)/', 'create blog entries, edit own blog entries', $role->perm);

Otherwise no time to test now...

I don't have time to test either patch right now but personally I think blog's permissions should be brought into line with all other content types. Given the previous permission renames have gone through smoothly, and we already have one database update from rc1 - rc2, I don't see a problem with this. Leaving them unique for yet another release would be confusing though.

Here comes a roll with http://drupal.org/node/206470 included. These are book and locale permission updates. The book update fixes a bugos permission update in RC1, the locale one was plain missing. We can push these out to book and locale module, but then we need to replicate the same wrapper logic around these, while they are just one and one lines each here.

Feedback / testing welcome.

Fixed the bug noted in #72 by JirkaRybka. Note that this is still untested.

#75 has copy paste errors with $renamed_permission vs. $role->perm again.

OMG! Grepped for use of "edit own blog", so that we can catch all occurrences, and most were easy to fix (they required blog post creation permission), but blogapi module uses it as a generic catch-all permission to create, delete and edit blog posts. So it is used as a 'can have a blog', ie. create blog entries; but it also uses it to accept file uploads, accept taxonomy term associations for nodes, etc. Seemingly without consulting the upload or taxonomy permissions.

In the light of all this, we might be better off with leaving "edit own blog" as is, or at best rename it to "maintain own blog" or something like this to differentiate it from the true "edit" permissions for this release. A true cleanup here would be much more far fetched then what we can comfortably do. The blog module permission fragmentation would have been a brave move on its own to begin with.

Attaching a patch to let you see some of the impact, and the point where I think we should give the blog part up for Drupal 6.

@Gabor: re: the blogAPI vs. blog permission mess, see http://drupal.org/node/54055
Personally, I'd vote in favor of cleaning all of this up and taking our time with it, even if it means delaying the RC another couple of days...

OK, here is a patch merging in the suggestion from #54055 as well (where it is said that blogapi should not rely on blog module permissions at all anyway, as it should be able to work with any content type since Drupal 4.7 :). So introducing the "administer content with blog api" permission. #54055 says "add content through blog api", but blog api uses this permission to maintain all kinds of content (add, delete, edit, categorize, publish, upload files). So "add content" is misleading.

I think this is what we *need* to do here to get over this patch, and more granular blogapi permissions are not a target of this issue, so the latest patch is up for review. Phew.

Ps. we should inform users about the new blogapi permission: (A) we add a blogapi.install with an update function or (B) we include a drupal_set_message() in the update function in the patch if the blogapi module is enabled. (A) is more in line with what we are expected to do =) Not yet in the patch.

Well, I took the last hour to dig blogapi permission handling, and there is no problem with node access handling there, all node operations are all fine (node_access() is always used directly or indirectly), so this fix solves blogapi's node permission problems as well.

Please review!

Tested this by installing modules on 6.x-dev, then on a clean rc-1 to patched 6.x upgrade (with authenticated user having permissions to check they got updated ok). Looks good.

With the missing (brand new) blogapi.install file to provide a message to people upgrading about the new permission. Everything else untouched.

I sat down to test the upgrade path as well, with a fresh Drupal 5.5 install, and gave these permissions:

edit own blog, create book pages, create new books, edit book pages, edit own book pages, outline posts in books, create forum topics, edit own forum topics, administer locales, access content, create page content, create story content, edit own page content, edit own story content, edit page content, edit story content, create polls

Done a test update, and all permissions fall into place as expected:

create blog entries, edit own blog entries, create book content, create new books, edit any book content, edit own book content, administer book outlines, create forum topics, edit own forum topics, administer languages, translate interface, access content, create page content, create story content, edit own page content, edit own story content, edit any page content, edit any story content, create poll content, add content to books

Looks like the user facing notification from patch from #58 got lost in the meantime, while the permission problems appeared in #61, so I included the user notification as well from #58 and added a permission page link to the blogapi note as well. Committed this one.

I just noticed this patch went in and wanted to say thank you!

Hi there,

I have just read most of the 80 something comments above on content permissions with great interest. Suddenly I realized that the permissions for blogs are currently set under the blog access settings, whereas the book permissions are handled, very confusingly by both the node access settings as well as the book access settings. This is foremost confusing, but I suspect it also to be a bug. Just going to do some testing to see what the confusion can lead to...

Cheers

So what I found is that I can set the following access settings for books:

book module

All fine and well so far...

node module

So I propose to move all solely book-related permissions to the book section.

LindenLion

I agree these are a bit inconsistent, however since this is an old issue (that wasn't orignally just about book module permissions), I've opened a new one here: http://drupal.org/node/232294

Just reverting to the original title before going off-topic...

This patch changed course in #71 to suddenly return FALSE instead of NULL in the case where the user doesn't have the required permission. This kills the node_access() half of the node access mechanism, because the node_access() function will never get past

  $access = module_invoke($module, 'access', $op, $node, $account);
  if (!is_null($access)) {
    return $access;
  }

and never look at any node access grants.

Here's the patch to fix it.

The followed patched code doesn't really look right. If $op is 'create', and the user doesn't have access to create blog entries, then, since the switch doesn't break, the code will check both 'update' and 'delete' permissions.

   switch ($op) {
     case 'create':
       // Anonymous users cannot post even if they have the permission. 
      if (user_access('create blog entries', $account) && $account->uid) {
        return TRUE;
      }
     case 'update':
      if (user_access('edit any blog entry', $account) || (user_access('edit own blog entries', $account) && ($node->uid == $account->uid))) {
        return TRUE;
      }
     case 'delete':
      if (user_access('delete any blog entry', $account) || (user_access('delete own blog entries', $account) && ($node->uid == $account->uid))) {
        return TRUE;
      }
   }

Argh, you're right, of course — thanks!

Tested in Drupal 6.2 with PostgreSQL 8.3, seems to work.

To save some lines of code, couldn't you just use:

  return user_access('delete any forum topic', $account) || (user_access('delete own forum topics', $account) && ($account->uid == $node->uid)) ? TRUE : NULL;

.... Instead of ....

      if (user_access('delete any forum topic', $account) || (user_access('delete own forum topics', $account) && ($account->uid == $node->uid))) {
        return TRUE;
      }
      break;

Having this would save you three lines of code for each case you encounter there.

Yes, that is certainly possible, and I like the ternary operator myself, but many are scared of it. I restored the form of the code before #83 was committed.

A clear benefit of using the ternary operator is less pressure to rewrite the cases again and reintroduce the bug, so here's the patch as you suggest.

This is a trivial patch that is badly needed. Without this, these built in content types are unusable with any node access module.

The fact that we are returning NULL instead of FALSE is ugly. I think the _access hook needs to get fixed instead -- at least for Drupal 7. For Drupal 7, this patch needs work.

Yeah, well any if (code) { return TRUE; } is a fine candidate for refactoring to return (code);. No wonder it happened there. Even if documented on http://api.drupal.org/api/function/hook_access/5 it is better to have clean code which implicitly returns NULL when returning NULL is required.

I committed this to Drupal 6 and it needs work for Drupal 7, so awaiting a new issue link for discussion on redoing the NULL return rule. Then can be closed and set back to Drupal 6.

Thanks for committing to D6!

The fact is we want to return three values: YES, NO, and UNDECIDED. Right now we're returning TRUE, FALSE, and NULL. I don't know how to make it prettier.

Why not return constants?

Also, why not have some sort of a thing for node_access_default_access($op, $node) or something instead of having all that duplication of code?

I don't see any benefit in defining constants for such a straight-forward mapping as

TRUE <=> YES
FALSE <=> NO
NULL <=> neither YES nor NO

All node modules would have to be changed to use those constants, whose values would still need to be retained anyway for backward compatibility (we shouldn't unnecessarily mess with node access).

subscribing

Subscribing.

Not critical, and no active patch.

And already being discussed over here: #537862: Convert hook_access() to hook_node_access() for more flexibility

@catch: This is critical. The patch in #71 broke node access for D6. The commit in #99 fixed it for D6, but it remains broken in D7! If #537862: Convert hook_access() to hook_node_access() for more flexibility is committed, that's fine, but if it isn't...