Protected Node Denies Private File Access

hmm, let me know if i understood it well :

You have a content type that is NOT protected by the module.
In that content type you have a private file field.

a user can access a node of that content type without typing a password (because the node in NOT protected) but can't access the private file until you give them "bypass password protection" permission ?

What error does the user get? a permission denied?

Thanks

hi,
following patch makes a clean up of some legacy code from the drupal 6 version.
it should be fixing the issue.
please try it and let me know.

Hello,

I test without the patch, and I encounter the issue.

I test with the bypass permission, it worked.

I remove the bypass permission to anonymous user, apply the patch, still got the issue.

I don't read the patch, just applied it.

(all of that without password protecting the node)

what did you notice exactely after removing the permission ?
if it is an access denied to the file then it's just working as dsigned, that's why private files are private :)
if it is sth else then we have a bug to fix.

Oups, sorry, I forgot about that.

But : https://drupal.org/documentation/modules/file
"
Accessing Private Files

Once configured, files stored in the private directory are inaccessible via a direct link; however, if Drupal constructs a link to the file, the file will be accessible to anyone who can see the link.
"

So if I can see the link (anonymous or not), I should have access to the file, no ?

With the patch, even with the bypass password protection, I can't access to the file.

When I disable protected node, it works fine.

I don't know if it's me who doesn't understand what needs to be tested or something else ?

and protected node module is implemnting its own logic to change that behaviour of a basic drupal :)
accessing a pivate file of a protected node should only be allowed if we can access the node after typing its password or we will be redirected to the password form.

The permission of bypassing is only there for nodes, it can't be safely implemented for files too. If we dare doing that we will have the initial bug : only using this module to be able to access all private files ! how a security hole !

i fixed the description of the permission to say its only there for nodes, i agree it needs more documentation and commenting.

What would really be a bug here is the 2 following cases :
1)
in a new drupal, a user can access a private file (cause it has the good permissions...)
enable protected_node + configure it somehow implies that the user can't access the file no more.
2)
in a new drupal, a user can't access a private file (anonymous for example)
enabling proteted node + configure it somehow implies that the user can now access the private file

that would be an overriding of normal behaviour.

let's start by making sure of this ;)

once this is done, we will have to make some decisions concerning real world problems :
1) multiple usage of a file (in multiple protected nodes that have different passwords)
2) mixed usage of a file (in a protected and non protected nodes)
we will have to make some decisions here, but let's go throw it step by step, for now we may want to document more the bypass permission aim and concern + validate the cases above prouving we don't break the default behaviour.

Ok, I understand that when we deal with files (and media) it's a lot harder. :)

I think we have bug in the first case :
Enabling protected node, without protecting any password breaks the default behavior of private file.

About the next concerns : mixed usage (protected and not protected nodes) : I think if the file is used in at least one not protected and published node, it should be accessible.

can you please give some details on the test that didn't work.
thx

- Fresh Drupal install.
- A content type with a file field on private.
- a node of this type, published, with a file
- an anonymous user can access the file as a link to the file is available on the node page (everything is right).
- protected node (with the patch) enabled and permissions on access protected content for all roles.
- the node is not protected
- anonymous user can still access the node, see the link, but get an access denied when clicking on the link.

I try to improve the first patch for this issue,

In the hook_init, I tried to get from where the user access the private file, the problem is in case of multiple usage of the file, the function protected_node_and_attachment is not ready and the rest of the module too.

I want to rewrite properly all these parts because I think the logic of the current code is not elegant, clear and I bet in D7 it's easier, more performant with less code.

I want to remove the legacy of D6 of this module.

But before please someone review my other patches, especially this two ones :

https://drupal.org/comment/8437013#comment-8437013
https://drupal.org/comment/8440251#comment-8440251

If you want I can upload my tests.

I remade the patch of Izus in comment #3. It was no more appliable due to recent commits.

I didn't modify the query, I think it's ok now.

Sorry I don't have more time for protected node this weekend.

Hello,

I just wonder, why Protected node implements hooks about files ? The name of the module is 'protected node', not 'protected file'.

So I think to simplify/clean the module and to remove the bugs, we should remove these hooks.

Therefore, all the questioning about all the cases like 'if a private file is on a node, but also on another node which is protected.....' And those problems should be delegated to the author of the contents who should have thought of the different paths to access a file.

Your opinions ?

I think it is an important feature request for the usage to be really significant.
The D6 version used to support it for private files, I find it interesting to try to do the same but only for private files.
I believe the support for this should be optional, letting admins make the decision to use it or not.
The private file relays on a permission per file field if I remember well, we don't need to alter any of this. but here is what may seem logic to do:

what about this for decision making ?

Nice table :).

I think it is clear that way. We will see in the code what needs to be done.

The third column is useless, no ?

updated the table just to be sure everyone understand it is "&&" condition for the lines.
3 column is normally there by default but let's be sure of it :)

Hello,

I wanted to implement automated tests for this feature. But before implementing the tests. I tried to reproduce the bug. And I didn't manage to reproduce the bug.

I made a content type with the following fields :
- public file
- private file
- public image
- private image

I let you guess how they are configured :)

I got two nodes of this content type.

Password protection on per node.

One node is protected, the other not.

As anonymous user, I can access all the files/images of the unprotected node. Without the password I can't access any of the files/images of the private fields of the protected node passing the URL directly in the browser and I can access any of the files/images in the public fields passing the URL directly in the browser.

So, did the bug has been solved indirectly ?

I do not test with the media module that offer the possibility to reuse the files. Do you want me to test with the media module and to implement automated tests ?

EDIT : I did not apply any patch.

actually, if no issue is ther no more, we should implement tests for the #15 cases (at least 6 cases)
the clean up in #3 and #13 is a good move too i think, mainly it gets rid of some D6 code ;)

Ok, Without the media (or scald, but with scald it is another thing) module, is it possible to reuse a file uploaded with the core widget ?

I ask that to know if I need to run tests with the media and file_entity module in requirements.

i don't think core give this possiility
but you can in the test setup create a file entity (with permanant file status) and reuse it in the tests where ever you need it ;)

Here is a patch that is a beginning of private file testing.

I modified only for per_node features.

The helper functions are based on the core ones.

Tell me if you want the tests about private files organiazed like this or each them in their own .test file ?

hi,
thanks for this
actually it will be goof if we decouple the private file protection from other tests. we will need a new test file to handle only private file protection stuff.
we will then add more tests for it when we will handle multiple(n>1) and mixed(private and public) files attached to a node.
so yes, we definitly need to decouple this test from others
Thanks

Edit ; so one new .test file with all the private file test cases in it, (6 or 7 methods to start i thinks)

Ok,

I was thinking of testing private files for each mode (per node, per type, global). If you prefer only one file for testing in only one mode, that's ok.

I will rework on this when the other patches I submitted today will be merged :)

so here it is, all patches are good and this issue can be focused on.
"now is the time" ;)

I think I will need to rebase the patch. I will do it this evening.

Patch from #22 rebased and the test is in its own file.

adding a new line at the end of the patch file.

hi,
thanks ! here is my review

1. +++ b/tests/protected_node.private_file.test
   @@ -0,0 +1,204 @@
   +    // Ensure the file can be downloaded.
   ...
   +		$this->assertResponse(200, 'Confirmed that the generated URL is correct by downloading the shipped file.', $this->group);
   

   there are some tabulations here for 2 lines

2. +++ b/tests/protected_node.private_file.test
   @@ -0,0 +1,204 @@
   +		$file_uri = $node->private_file['und'][0]['uri'];
   

   there are some tabulations here for 5 lines

3. +++ b/tests/protected_node.private_file.test
   @@ -0,0 +1,204 @@
   +		// Ensure the file cannot be downloaded.
   

   there are some tabulations here for 4 lines

4. +++ b/tests/protected_node.private_file.test
   @@ -0,0 +1,204 @@
   +  function createFileField($name, $type_name, $field_settings = array(), $instance_settings = array(), $widget_settings = array()) {
   

   for the two methods that come from FileFieldTestCase class we should try to use the helper method of the class FileFieldTestCase::createFileField and FileFieldTestCase::attachFileField
   or if there is a reason we dont want to, juste document that we inspired it from file.test

this patch containes the basic tests we need to start
as a plan for what is remaining here, and once this patch is merged:
1) implement test for cases in #15
2) deal with cardinality > 1 for mixed multiple files

do you see anything else we could miss for the plan ?

Thanks

Hi,

Review taken into account.

Note: "assertResponse" does not take into account $this->group, so I removed it.

When this patch will be merged. Is it possible to copy the comments which say the next tests to do in this issue #2277045: Implement tests for protected node and close this one ?

I think it will be good for the moral to have one less issue (at least for mine :) ) and to centralize the tests in the dedicated issue.

Thanks for the review.

ok,
i merged #38
i agree to close this issue and add remaning tasks in the test one, but there is more than just tests ;)
one delocalised to the tests issue, please feel free to mark this one as fixed :)

Ok,

Testing issue issue summary updated.

Hum... I don't clearly understand the "but there is more than just tests" ? Sorry

I am sorry to say this is still an issue when working with webforms. Steps to reproduce:

• Enable protected_node.
• Create a webform with a required file upload component that stores in the private file system.
• View the form. Select a file and press Upload. Submit the form.
• The form validation complains that the file field is required, even though it was provided.

Hello,

Thanks for the steps, I reproduce the error.

What is weird is that my webform of test is not protected.

Just activating protected_node breaks webform behavior.

I will try to fix that.

In hook_file_download I removed the test

if ($file->status) {

because when uploading before submitting, status was at 0 so we do not check if the node was protected or not.

weird !
actually the $file->status stuff seems needed as it checks the file status of laoded files during hook_file_download
https://api.drupal.org/api/drupal/modules!system!system.api.php/function...
and this hook is fire only when downloading private files, it shouldn't be fired when uploading them.

hi again,
i followed the steps in #43 (with the last code base) and couldn't reproduce it.
can you please double check if the issue described hese stills accurate
Thanks

Hello Izus,

Did you press the upload button before submitting the form ?

In #44, I reproduce the error when pressing the upload button. Without that it works.

yes Grimreaper i pressed it and it worked just fine

i tested it locally but I also tested it with simplytest.me (webform+protected_node 7 branch) and nothing noticed !

The steps in #43 produce the same error shown with webform 7.x-4.2 and the latest 7.x-1.x of this module.

Hello,

I tried to work on this issue to close it but I cannot reproduce the bug with a fresh install of Drupal 7.34, the last version of protected node and webform 7.x-4.2.

Did someone can reproduce it ?

Yes, I reproduce the error now.

I don't know why 2 days ago I didnt. Maybe my Drupal cache was not cleared so a hook implementation was not fired.

Hello,

Here is a patch that fix the problem. Should I had automated tests ?

When you press the upload button, the file status is 0, so I add the else.

But when you want to consult the submission data, the file status is 1, so I mage another query to retrieve the nid passing through the submission data.

Thanks for any review.

Hi cilefen and Prizem,
do you confirm this patch solves the issue fot you guys !
Thanks

#56 fixes it for me in manual testing.

Thansk for the test and the patch
here is my code review :

+++ b/protected_node.module
@@ -723,6 +739,11 @@ function protected_node_file_download($uri) {
+      return array();

This will affect all non published files.

do we really need to return an array here ? (doesn't the -1 returned do the trick or is this a really special case for webform ? if it's the case we can maybe wrapp it with a module_exists to not influence the behaviour of other non published files)

Hi Izus,

Ok, I wrapped with a check on the uri if it is a webform submission or not. Because we need this array returned.

I don't know why with by example, a private file upload on a node form, it is okay without that, but not with webform.

Hi,
Thanks all for the patch and testing
this is now merged.
++