Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Restricting image deletion is not possible.
Admin should be able to restrict users ability to delete images.
Thanks
Marcel
http://www.macminiforums.com/forum/
Comment | File | Size | Author |
---|---|---|---|
#29 | core-style-content-44057.patch | 1.48 KB | joachim |
#28 | image-HEAD.access.patch | 1.03 KB | sun |
#25 | image_gallery_access-explains-permissions.2.png | 27.37 KB | salvis |
#24 | image_gallery_access-explains-permissions.png | 25.76 KB | salvis |
#20 | image-HEAD.access.patch | 2.72 KB | sun |
Comments
Comment #1
drewish CreditAttribution: drewish commentedwe 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.
Comment #2
joachim CreditAttribution: joachim commentedWe should update the permissions on 6 so we still mirror core permissions.
Snip from node.module:
Comment #3
pebosi CreditAttribution: pebosi commentedi created a patch for that, please review.
regards
Comment #4
pebosi CreditAttribution: pebosi commentedcreated a patch to update permission table.
regards
Comment #5
joachim CreditAttribution: joachim commentedHi 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.
Comment #6
pebosi CreditAttribution: pebosi commentedCombined, updated my patch. Please review
regards
Comment #7
pebosi CreditAttribution: pebosi commentedComment #8
pebosi CreditAttribution: pebosi commentedUpdate update function to use strtr.
regards
Comment #9
sunSame 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.
Comment #10
suncross-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?
Comment #11
pebosi CreditAttribution: pebosi commentedUpdated to change the permissions also in other files.
Comment #12
sunoh my... I've fixed those Windows line-endings in .test files. Can you re-roll?
Comment #13
pebosi CreditAttribution: pebosi commentedhere it is.
Comment #14
pebosi CreditAttribution: pebosi commentedUpdated patch to use switch instead of if...else in hook_access
regards
Comment #15
sunMarked #541466: Add delete permissions, standardize existing edit permissions as duplicate, which contains an equally nice patch.
Comment #16
joachim CreditAttribution: joachim commentedTested, 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:
Comment #17
salvisI'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!
Comment #18
sunWe 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)
In addition to what salvis said, there should be blank lines between case blocks.
23 days to code freeze. Better review yourself.
Comment #19
joachim CreditAttribution: joachim commented@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...
Comment #20
sunIf 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().
Comment #21
joachim CreditAttribution: joachim commentedSo 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?
Comment #22
joachim CreditAttribution: joachim commented@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.
Comment #23
joachim CreditAttribution: joachim commentedAnd 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.
Comment #24
salvisRead the node_access() function:
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
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!
Comment #25
salvisThe delete permissions were still missing...
Comment #26
joachim CreditAttribution: joachim commentedThanks 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.
Comment #27
sunhttp://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.
Comment #28
sunThat 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).
Comment #29
joachim CreditAttribution: joachim commentedsp3boy 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.
Comment #30
sunyay, thanks! :)