Posted by pamphile on January 10, 2006 at 5:26am
7 followers
| Project: | Image |
| Version: | 6.x-1.x-dev |
| Component: | image.module |
| Category: | feature request |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
Restricting image deletion is not possible.
Admin should be able to restrict users ability to delete images.
Thanks
Marcel
http://www.macminiforums.com/forum/
Comments
#1
we mirror the core drupal permissions which don't separate out deletion and editing. in drupal 6 i believe delete is a separate permission so i'd be willing to change this then.
#2
We should update the permissions on 6 so we still mirror core permissions.
Snip from node.module:
$perms[] = 'create '. $name .' content';$perms[] = 'delete own '. $name .' content';
$perms[] = 'delete any '. $name .' content';
$perms[] = 'edit own '. $name .' content';
$perms[] = 'edit any '. $name .' content';
#3
i created a patch for that, please review.
regards
#4
created a patch to update permission table.
regards
#5
Hi pebosi. Thanks for all the work you're putting in with the patches :)
Could you update this so it uses the same SQL queries as core, as outlined in this comment: http://drupal.org/node/409974#comment-1724240
Also, it's simplest to make one patch with all the changes for an issue, instead of one patch per file.
#6
Combined, updated my patch. Please review
regards
#7
#8
Update update function to use strtr.
regards
#9
Same as the image gallery permission issue regarding preg_replace().
This one though... should ideally come with tests. But well... since it seems that I am the only one who seems to be able to write them currently, and I don't want to hinder progress. Thus, simply adding a @todo comment to the top of the image.test file would be sufficient.
#10
cross-posted.
Anyway. You need to use the array() syntax for strtr(), because passing 2 string arguments to strtr() means that each single character in the first string will be replaced with the corresponding character of the second string. ;)
Please also note that image.test contains the permissions as well - which makes me worry: Did you grep the Image module code-base for potential other instances of those permissions?
#11
Updated to change the permissions also in other files.
#12
oh my... I've fixed those Windows line-endings in .test files. Can you re-roll?
#13
here it is.
#14
Updated patch to use switch instead of if...else in hook_access
regards
#15
Marked #541466: Add delete permissions, standardize existing edit permissions as duplicate, which contains an equally nice patch.
#16
Tested, all works great.
Committing:
#44057 by pebosi: Changed permissions in Image module to match those in core.
Thanks again for all your work pebosi :)
BTW, that pattern you've got for updating perms is very neat. You should contribute it to a PHP snippets page in the documentation, or maybe even as a helper function in Core:
while ($role = db_fetch_object($result)) {$renamed_permission = strtr($role->perm, array('edit images' => 'edit any images'));
if ($renamed_permission != $role->perm) {
$ret[] = update_sql("UPDATE {permission} SET perm = '$renamed_permission' WHERE rid = $role->rid");
}
}
#17
I'm sorry, but the patch as committed has just broken node access!
hook_access() must return NULL if it doesn't have a definitive answer, and then node access will kick in. The "improvement" in #14 results in image_access() always returning TRUE or FALSE.
We've gone through this very same problem with Forum, Poll, and Blog in D6 core (committed in #153998-99: Access checks returning NULL is ugly.).
#13 is correct. You must revert #14 and apply #13!
#18
+++ image.install 26 Jun 2009 12:26:03 -0000@@ -334,3 +334,17 @@ function image_update_6102() {
+ $renamed_permission = strtr($role->perm, array('edit images' => 'edit any images'));
+++ image.module 26 Jun 2009 12:31:53 -0000
@@ -81,24 +81,23 @@ function image_node_info() {
- return array('create images', 'view original images', 'edit own images', 'edit images');
+ return array('create images', 'delete own images', 'delete any images', 'edit own images', 'edit any images', 'view original images');
We need to replace a bit more here:
'edit images' => 'edit any images, delete any images'
'edit own images' => 'edit own images, delete own images'
(Note that I'm currently not sure whether there really is a space after the comma in the permissions strings.)
Additionally, I want to kindly ask for sorting the permissions in hook_perm() by
- administer*
- view*
- create*
- edit*
- delete*
(For people [like me] who have core patch applied that removes the asort() for permissions)
+++ image.module 26 Jun 2009 12:31:53 -0000@@ -81,24 +81,23 @@ function image_node_info() {
+ case 'create':
+ return user_access('create images', $account);
+ break;
+ case 'update':
+ return (user_access('edit any images', $account) || user_access('edit own images', $account) && ($account->uid == $node->uid));
+ break;
+ case 'delete':
+ return (user_access('delete any images', $account) || user_access('delete own images', $account) && ($account->uid == $node->uid));
+ break;
In addition to what salvis said, there should be blank lines between case blocks.
23 days to code freeze. Better review yourself.
#19
@salvis:
> hook_access() must return NULL if it doesn't have a definitive answer
It was never returning NULL before, was it?
http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/image/image...
#20
If the function ends with no return statement, then its return value is NULL.
Still not sure about the comma between stored permissions.
Fixed module update function name.
Fixed should return NULL instead of FALSE.
Fixed trailing white-space in image_access().
#21
So you're saying we should return TRUE if the permission applies, but NOT return FALSE if it doesn't?
I don't understand why:
http://api.drupal.org/api/function/hook_access
TRUE if the operation is to be allowed; FALSE if the operation is to be denied; NULL to not override the settings in the node_access table, or access control modules.
If the user doesn't have, say 'edit own images', why should we not deny access?
#22
@sun: I confess I am bemused by node_access, even looking at the node_example module and the documentation.
I'd say just commit the fix now, so the dev release isn't broken.
#23
And my database has rows like this in {permission}:
access comments, post comments, post comments without approval, create images, access content
So that part's ok.
I'm going to trust that sun knows that "P || Q && R" means the right thing in PHP. :/
> If the function ends with no return statement, then its return value is NULL.
That's not the bit I was confused about. It's why we ought to return NULL in the first place. I take it then that permissions like "edit own NODE" are only meant to be positive, that is, if they are absent from a user then other means may still grant access? It would be helpful if the documentation actually explained this conceptually...
Committed sun's patch so we don't have a broken dev.
#44057 by sun: Fixed earlier patch for permissions in Image module.
#24
Read the node_access() function:
<?php
function node_access($op, $node, $account = NULL) {
...
if (user_access('administer nodes', $account)) {
return TRUE;
}
if (!user_access('access content', $account)) {
return FALSE;
}
// Can't use node_invoke(), because the access hook takes the $op parameter
// before the $node parameter.
$module = node_get_types('module', $node);
if ($module == 'node') {
$module = 'node_content'; // Avoid function name collisions.
}
$access = module_invoke($module, 'access', $op, $node, $account);
if (!is_null($access)) {
return $access;
}
// If the module did not override the access rights, use those set in the
// node_access table.
...
?>
If the user has administer nodes, they get access, but absence of administer nodes does NOT deny access, of course. Only access content has the power of denial.
Just take a step backwards and think about what is reasonable behavior.
We can only get past
<?phpif (!is_null($access)) {
return $access;
}
?>
and on to checking the {node_access} table, if hook_access() returns NULL.
Yes. See the attached screenshot for how Image Gallery Access explains the permissions.
Thanks for fixing this quickly!
#25
The delete permissions were still missing...
#26
Thanks for the explanation.
Still makes my brain hurt :(
So is it the case that any permission that involves an existing node (so not 'create') should return TRUE/NULL rather than TRUE/FALSE?
hook_access docs do give that in the example, but with things that cause brain pain I find idiot-proof instructions are needed.
#27
http://api.drupal.org/api/function/hook_access states:
So. This can be understood as a half-baked ACL:
- TRUE explicitly allows access, without taking node access grants (e.g. from OG or Domain Access) into account. Mainly used for edit/delete permissions.
- FALSE explicitly denies access, without taking node access grants (e.g. from OG or Domain Access) into account. Mainly used for edit/delete permissions.
- NULL defers access check to node access grants (e.g. OG or Domain Access). Mainly used for view permissions.
#28
That also means that
- NULL == FALSE when considering edit/delete permissions. (However, most node access modules only deal with view access btw)
Committed attached patch to speed up this access check (doing the id comparison first).
#29
sp3boy flagged a problem -- the above patches broke our tests with the changed permissions!
The patch at #411568: derivative paths should be taken from the corresponding original file, and not from the default fixes the problem, but I am going to commit part of it here under this issue for a clean commit log and issue history.
Committing the attached.
#30
yay, thanks! :)
#31
Automatically closed -- issue fixed for 2 weeks with no activity.