These functions are totally messed up, for several reasons

  1. The function node_gallery_get_gallery_list() wrongly uses node_gallery_user_access()
    It passes gallery type as an argument. If you look at node_gallery_user_access() function you will see it accepts gallery node objects, not type strings.
  2. The function node_gallery_get_gallery_list() tries to check access using node_gallery_user_access() function but without user argument which is pointless
  3. Both node_gallery_get_gallery_list() and node_gallery_user_access() ignore node access system. We can't assume that editable nodes === user-owned (authored) nodes because it ignores cases where gallery is read-only, and cases where user can edit other user's nodes. Drupal has access authority - node access system, and the module must use db_rewrite_sql() and node_access() functions to check access.
    Ignoring node access is also dangerous because we could disclose titles of private galleries which user couldn't even view

Comments

crea’s picture

crea commented
Title: node_gallery_get_gallery_list() and node_gallery_user_access() are totally messed up » node_gallery_get_gallery_list() function wrongly uses node_gallery_user_access()
Priority: Major » Normal

Updated summary

crea’s picture

crea commented
Issue summary: View changes

Updated summary

crea’s picture

crea commented
Issue summary: View changes

Updated summary

crea’s picture

crea commented
Issue summary: View changes

Added note about private gallery title discosing

crea’s picture

crea commented

Marked #1242852: node_gallery_get_gallery_list() doesn't respect node access as a duplicate of this issue.

crea’s picture

crea commented
Title: node_gallery_get_gallery_list() function wrongly uses node_gallery_user_access() » node_gallery_get_gallery_list() is totally messed up
crea’s picture

crea commented
Title: node_gallery_get_gallery_list() is totally messed up » node_gallery_get_gallery_list() and node_gallery_user_access() are totally messed up

Changing to major

crea’s picture

crea commented
Priority: Normal » Major
scroogie’s picture

scroogie commented
Title: node_gallery_get_gallery_list() function wrongly uses node_gallery_user_access() » node_gallery_get_gallery_list() and node_gallery_user_access() are totally messed up
Priority: Normal » Major

1. We should indeed refactor that a bit.

2. It's not pointless at all. If $account is not passed, it is NULL. When user_access gets passed NULL, it uses the logged in user. Please see http://api.drupal.org/api/drupal/modules--user--user.module/function/use...

3. It depends on how you define the edit permission. Is the user allowed to add pictures to a gallery if he is allowed to edit it? I think so.

crea’s picture

crea commented

@scroogie
On 2 - it's pointless to use logged in user, because the node_gallery_get_gallery_list() function has $uid argument which can be different from the current user
On 3 - You are missing the point. Yes the logics "user allowed to add pictures to a gallery if he is allowed to edit it" works, but we can't assume user can edit the node just because he's an author. And we shouldn't limit the gallery list just to galleries user owns anyway - that doesn't account for shared galleries.

If you are questioning if the user does need edit access to a gallery to move images into it, I think he does. There's node access system that we can use. If we ignore node access system, we need to reinvent the wheel with own access control system which I think is a waste of time and overcomplication of it all.
There's also Node Gallery Access module which uses node access system already. I think there's no other choice than to use node access system.

scroogie’s picture

scroogie commented

Thanks for telling me "that I'm missing the point".

However, the function first checks if the user can edit the gallery using node_gallery_user_access. It's assumed that the user can move images between his own galleries, which is the use case where the function is used.

This is debatable, as I said before, but neither "messed up", nor "missing the point".

The function is used in node_gallery_change_gallery_action_form() in node_gallery.actions.inc. I hope Justin will chime in and explain the usage there. That will make clear if we can use db_rewrite_sql or not. Perhaps we will just split the two uses up.

crea’s picture

crea commented

It's public API function which doesn't do what it promises. I understand that it was done with some assumptions in mind, but you don't design software like that, if you want other developers to use your code.
When someone will want to use this function, he will look at the function doc which says

@param $uid
    (optional) if specified, the list returned will only contain galleries the user can modify.
crea’s picture

crea commented

Separating moving images into a gallery from the gallery edit permission is quite interesting subject. But I think it's better to discuss that in a separate issue.

scroogie’s picture

scroogie commented

So perhaps we should change the PHPDoc.

You can't really seperate this from discussing the permissions, it's essentially the same thing. If we changed the permission logic so users can only upload images to galleries they can edit, then we will have the problem that there can never be pictures in galleries they created but are not allowed to edit. Otherwise we will have to change the fundamental workflow and image creation logic (currently its gallery first, images then).
I assume that this is the reason that somone (IIRC I wasn't yet a co-maintainer back then, I don't even know if Justin was) decided to use the authorship as the decisive criterion.

So to sum up what needs doing:
- we should use db_rewrite_sql to additionally respect access modules
- we should discuss why we need the $uid parameter in node_gallery_get_gallery_list()
- we should discuss and take a decision on the general permission interpretation

crea’s picture

crea commented

Yes, I agree with your points

thatjustin’s picture

thatjustin commented

I'm debugging an issue right now that I think may be related to this.

I have Workflow along with Workflow Access and Node Gallery installed. When workflow access grants "edit posts in this state," no matter what I try, I can't get node gallery to allow "sort images" or "manage images" to that user. However, if I explicitly grant "edit own node_gallery_image content" in /admin/user/permissions or if I enable content access and grant "edit own content" to the image type, then I can "sort images" or "manage images" works (for that user).

What I am wondering is if that is because of what is being discussed here. It seems to me that node gallery is ignoring the edit rights that workflow access is granting.

Thanks.

semanthis’s picture

semanthis commented

I'm not shure if I got something wrong. I'm dealing with the bug "odd bug here where an anonymous user with privs to view "Manage Images" can see all galleries in the dropdown." (as written in the comment in node_gallery.actions.inc). As soon as I give users the right to manage galleries, I have all galleries listed in the select, meanwhile they only have the node-permission to only edit their own galleries. This seems to be closely related to issue of this thread. Is there any advice how I could deal with that bug?

zengenuity’s picture

zengenuity commented
Status: Active » Closed (won't fix)

Issue queue cleanup. No more changes to 6.x version.

zengenuity’s picture

zengenuity commented
Issue summary: View changes

better english