When you set a gallery to be "private", node_gallery_access allows the owner of the gallery, admin, and anyone with the 'access protected contents' or 'edit protected contents' permissions to access said gallery.

However, when you protect a gallery with a password, the only person that doesn't get prompted for a password to view the gallery is the owner of the gallery itself. This seems incorrect to me. Shouldn't password protected galleries honor the same rules as private galleries?

I'm attaching a patch that modifies the behavior of node_gallery_access to allow that.

In the interest of backwards compatibility, I included a checkbox on the node_gallery_access setup page that says 'Allow admin and users with view/edit permissions access without a password'. When checked, it kicks in the new behavior, when unchecked it retains the previous behavior. Note if you check the box as admin in one browser, and are testing in another browser, you will likely need to logout/login on the second browser to test.

CommentFileSizeAuthor
#22 Issue_612122.patch12.06 KBjustintime
#17 nga-1.patch11.33 KBdbeall
#12 nga.patch9.87 KBjustintime
node_gallery_access_admin_nopassword.patch4.1 KBjustintime

Comments

dddave’s picture

dddave commented

However, when you protect a gallery with a password, the only person that doesn't get prompted for a password to view the gallery is the owner of the gallery itself. This seems incorrect to me. Shouldn't password protected galleries honor the same rules as private galleries?

Should they really?

I am not sure I understand the problem correctly. You want to allow any user with "access.." permission to be able to access any password protected gallery without entering the password? I understand the motivation but I am not sure this patch is the way to go (didn't apply it yet).
Basically what you want seems to be right: Every admin (=role with access protected content) should be able to access any protected content wether it is set to private or protected with a password. There has to be such a permission to prevent users from creating galleries unaccessible to admins. If only UID1 can check such content this would be a big problem.
BUT I can easily imagine configurations/sites where you have various admins but you don't want them to be able to access any password protected gallery (e.g: Large sites with lots of different users). Often admins are privileged users who are helping out (think of moderators in forums); do I really want to make such helpers almighty gallery admins?

I think the permissions should be more fine-grained, i.e. there should be a permission that allows admins to access any protected content and another one allowing only to access private content. If users with the "access private content" permission want access to password protected galleries they have to know the password.

Is my understanding of the problem correct?
Do we need another permission?

justintime’s picture

justintime commented
Status: Needs review » Needs work

I wrote the patch because I was assuming that if private meant "private except for users with permissions", then I thought password should mean "password except for users with permissions". That wasn't the case, and it wasn't documented anywhere, so I figured it was a bug.

For the record, my use case is where one or more users with the photographer role are creating galleries. Some galleries are public, some are meant to be accessed by one user (photographer sets the owner and marks it as private), and some are meant to be editable by one user, but viewable by anyone with the correct password. Since the photographer may be managing 10 or more galleries at one time, asking the photographer for the unique password to each one would be a usability nightmare.

However, I *really* like the idea of adding a "view password protected galleries without a password" permission. It's a different solution to my problem, but it still gets the job done. Most importantly, it makes sense without needing a lot of documentation.

Does anyone have any other use cases or potential pitfalls? I'll set this back to "needs work", and if no one objects, I'll see if I can get that implemented. I'll likely change the wording of the existing permission to be "access private content" instead of "access protected content", then add two new permissions, "access password protected galleries without a password" and "edit password protected galleries without a password".

dddave’s picture

dddave commented

The more I think about it, the more I believe in this additional permission solution.
First of all, if no additional permission is implemented your patch (assuming it works) has to be included. Password protected galleries only accessible with the password or by UID1 is an administrative nightmare.

But as I wrote in my first post: The larger sites are which use this module the more people need to be admins. More diverse sites may have different kinds of admins and making all of them to "super"admins for node gallery seems to be a "big" step.

So +1 (again) for the two new permission and +1 for the wording changes.

Edit: WTF!! Just fooling around (as UID1) and set the gallery of another user to password. Now the UID1 has to enter the password!? If a user sets the gallery to "password" UID1 can enter without a problem. So not really a problem but perhaps a sign of some bogus logic.

edit2: Something very strange is going on here. I am able to protect galleries (as UIDxy) with a password which even UID1 has to enter. I am working with different browsers so this shouldn't be some caching quirks. I'll undertake some additional testing. This is really akward.

dddave’s picture

dddave commented
Component: Code » Access Contrib
justintime’s picture

justintime commented
Component: Access Contrib » Code

dddave - there is no check in the code at all for UID1 at any point in time. It only checks to see if you are the owner. If you are UID1, and UID1 owns the gallery, then you can view it. Otherwise, UID1 is treated (improperly, IMHO), as any other user.

This would obviously be another bug fixed by our proposed patch.

dddave’s picture

dddave commented
Priority: Normal » Critical

WTH!? I am pretty sure I tested various times if UID1 can enter password protected galleries. Am I suffering from Alzheimer's here?

If UID1 cannot enter any gallery at any time this is a BIG and critical problem. Think of childporn galleries hidden even from UID1. ARGH!

justintime’s picture

justintime commented
Component: Code » Access Contrib
Priority: Critical » Normal

Don't worry, I get selective Alzheimer's too ;-)

Go here: http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/node_galler...

Search for "function node_gallery_access_check". It's laid out pretty clearly right there:

- If my uid matches the owners uid, allow access
- If I have access or edit protected contents, allow access
- If I have previously entered the password correctly in this session, allow access
- Otherwise, deny access.

No mention of $user->uid == 1 anywhere in the entire module.

dddave’s picture

dddave commented

Ok, to sum it up:

- introduce two new permissions: "access password protected galleries without a password" and "edit password protected galleries without a password"

- rename "access protected content" to "access private content"

- let UID1 access every gallery

kmonty’s picture

kmonty
He/Him
Location Berlin, DE + SF, CA
commented

$user->id 1 should definitely have the ability to access everything. That is definitely a bug.

I like all the proposed changes. If you can update your existing patch to reflect them, I'd be greatly appreciative!

justintime’s picture

justintime commented

Sure thing - give me some time and I'll post a patch.

kmonty’s picture

kmonty
He/Him
Location Berlin, DE + SF, CA
commented

Not completed related, but this is more unexpected permissions problems: #516068: Users with the permission to "administer nodes" are not capable of uploading/editing images

justintime’s picture

justintime commented
Status: Needs work » Needs review
StatusFileSize
new nga.patch9.87 KB

Wow - I'm fried, but I got a lot done.

First, #8 above is implemented.

Also, #11 above which links to #516068: Users with the permission to "administer nodes" are not capable of uploading/editing images is halfway implemented. The other half is in node_gallery core, which I'm not too familiar with yet.

Finally, I also included support for custom priorites - #480304: Node Gallery Access conflicts with Content Access module.

The patch is against CVS from about 10 minutes ago. I've been not enjoying git lately, so if something's broken, let me know. I did about 20 minutes of testing, and it worked great for me.

After installing the patch, make sure you rebuild all permissions. Enjoy!

dbeall’s picture

dbeall commented

looks like a big patch.
I will have a run through later..
I have not been enjoying hooking up the sewer system here at the lake either. But I have it almost done.
It wouldn't be so bad, but the county government is complete corruption here, they all belong in prison.

kmonty’s picture

kmonty
He/Him
Location Berlin, DE + SF, CA
commented

I still need to test this.

There is also this potentially related issue: #572224: Private Galleries Appear in Gallery List

kmonty’s picture

kmonty
He/Him
Location Berlin, DE + SF, CA
commented

At some point, we're going to have to write some views filters. I just created a ticket #617760: Add Views Filters to Node Gallery Access that allow users to filter out protected gallerys/images from views

dddave’s picture

dddave commented

Whoa! I am afraid I don't have the time to test this beast today but thanks anyway.

dbeall’s picture

dbeall commented
StatusFileSize
new nga-1.patch11.33 KB

I am just now testing, before this goes out I added some minor text t('string')changes.
nga.patch(amended)

justintime’s picture

justintime commented

Bah, just before signing off, I may have found a bug in my patch. Can someone else try this if they have time?
- Create a password protected gallery, owned by a "normal" user
- Assign images to it
- Verify UID1 can view the gallery and it's images without a password
- Sign in as the owner
- Gallery is viewable, but none of the images are.

I'm swamped this weekend with the kids and grandparents. I'll look more into it on Sunday evening hopefully.

justintime’s picture

justintime commented

Nevermind, my photos were owned by uid1 instead of the gallery owner.

justintime’s picture

justintime commented

It just hit me, this is probably a bug, isn't it? I mean, NGA's whole purpose in life is to apply access permissions assigned at the gallery level to the image level, right? So it shouldn't matter who owns the image, as long as I own the gallery, I should be able to view it. Can anyone confirm or deny that statement?

dbeall’s picture

dbeall commented

It makes perfect sense to me.
If a person#1 that owns something(image), lends it to person#2, then person #2 has assumed access and control over the item until the original owner takes it back.

So in effect, the owner of the image(person#1) is trusting it's use to (person#2) to do as he sees fit.

If person#1 only wanted person#2 to view his images, he would leave them in his own gallery and give person#2 the password.

justintime’s picture

justintime commented
StatusFileSize
new Issue_612122.patch12.06 KB

Just a few lines of code to node_gallery_access_node_access_records was needed to fix the owner issue. The module doesn't care who owns the image, only the parent gallery. The attached patch contains the patch from #12, #17, and this little bugfix.

dddave’s picture

dddave commented

Did some testing:

+ Two new permissions (enter/edit password gals without password)
+ Renamed permissions for private galleries
+ UID1 rules

(+) Access Control Module priority (did not test the functionality but noticed the functionality)

halfway done: Administer nodes should also allow to edit/upload images (going to post my comments at #516068: Users with the permission to "administer nodes" are not capable of uploading/editing images

Any other functionality/changes I forgot?

kmonty’s picture

kmonty
He/Him
Location Berlin, DE + SF, CA
commented

#516068: Users with the permission to "administer nodes" are not capable of uploading/editing images is an unrelated issue (although it definitely needs to be solved). If this patch is working as expected, I'll apply it. (I've tested it some and seems solid to me, but some extra eyes is always a good thing).

justintime’s picture

justintime commented

Just commenting that I've been running the patch on my development copy now since I wrote the patch, and haven't found any other bugs.

dddave’s picture

dddave commented

Same here. This looks really good!

kmonty’s picture

kmonty
He/Him
Location Berlin, DE + SF, CA
commented
Status: Needs review » Patch (to be ported)

Yeah, it looks solid.

kmonty’s picture

kmonty
He/Him
Location Berlin, DE + SF, CA
commented
Status: Patch (to be ported) » Fixed

Committed

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.