You have a bug in your function filebrowser_page_download($fid) when the ZipArchive is created in the Default Temporary Folder :
$target = file_directory_temp() . "/filebrowser_" . _filebrowser_safe_basename($target) . ".zip";
By default, the return value by the function file_directory_temp (http://api.drupal.org/api/function/file_directory_temp/6) is the real path set in php.ini -> upload_tmp_dir OR by default /tmp

Which is incompatible with the Download Manager set to "Direct Download" because the archive is outside the Drupal Root Directory.
And Vice-Versa when the Download Manager set to "Private Download", you got an Denied Access.

In few words :

  1. Set a Temporary directory inside the Drupal Root in admin/settings/file-system if you want to use the Public Download
  2. Let the Drupal's Temporary directory by default (in the php.ini) if you want to use the Private Download

Why do you leave the user to set himself the Public/Private download which could be confusing? and instead use the Drupal setting ?
In this case, insert somewhere:

if (variable_get('file_downloads', FILE_DOWNLOADS_PUBLIC) == FILE_DOWNLOADS_PUBLIC) {
      // Public method
} else {
      // Private Method
}

Comments

Nicolas Georget’s picture

Nicolas Georget commented

Another thing that i figured out in my test: with the option "Allow subdirectory listings.", you can download a Zip archive with some files inside (Files are in the root dir_listing).
Without this option, you got an Denied Access:

function filebrowser_page_download($fid) {
  [...]
  if (strpos($fid, ':')) {
    list($fid, $files_fid) = explode(':', $fid); // <= $fid is ok
    $files_fid = explode(',', $files_fid);       // <= but $files_fid is NULL 
  }
  [...]
  if (!$content || !$node || !_filebrowser_can_download_file($node) || !$target) {
    drupal_access_denied(); // It stoppped here because all the variables are empty
    exit();
  }
Yoran’s picture

Yoran commented

The separation of public/private settings from drupal comes from the fact that most of the time a site is configured to public and also need to have "private access to specific files" through filebrowser.

The second reason why a "download" manager, is that some user of this module have petty customized version of the way of handling files download. As an example, one user make a random simlink to real file in order to both security and apache speed.

As a conclusion, we can imagine to have a "Drupal settings download manager" as third option, letting people who need it to override such settings.

Yoran’s picture

Yoran commented
Status: Active » Postponed (maintainer needs more info)

I fixed the first issue (in dev) but I don't really understand the second one. Can you give me the steps to reproduce this ?

Nicolas Georget’s picture

Nicolas Georget commented

Never mind because i'm not able to reproduce 2nd comment :(
Anyway, i get an empty ZIP archive when i download as ZIP a folder now and the following errors:

warning: filesize(): stat failed for sites/default/files/tmp/filebrowser_test.zip in [...]/sites/all/modules/filebrowser/filebrowser.module on line 912.
warning: fopen(sites/default/files/tmp/filebrowser_test.zip): failed to open stream: No such file or directory in [...]/sites/all/modules/filebrowser/filebrowser.module on line 924.

You get the same ?

The separation of public/private settings from drupal comes from the fact that most of the time a site is configured to public and also need to have "private access to specific files" through filebrowser.
The second reason why a "download" manager, is that some user of this module have petty customized version of the way of handling files download. As an example, one user make a random simlink to real file in order to both security and apache speed.
As a conclusion, we can imagine to have a "Drupal settings download manager" as third option, letting people who need it to override such settings.

I understand why Drupal must handle 2 ways of handling files. But my point was why do you handle it twice. Maybe i say something stupid but let drupal do it variable_get('file_downloads', ...
Or you have an another reason in the filebrowser mechanisms that i don't get it (for sure):

  • public: Direct download (files are served by the web serveur and should be accessible by it)
  • private: Private download (files are served by PHP/Drupal an could be everywhere)

Can you explain a bit what is the difference for your module, technically speaking ?

Yoran’s picture

Yoran commented

No success for me to make zip archive fail. I tried public/private, with or without checking a file, it works fine here. Is your test made in a subfolder ? Are you using linux ?

I didn't make myself clear. My point wasn't about the interest of having 2 methods (private and public) but in the fact that for most installations, you don't want to have the same settings in drupal AND in filebrowser. For example, I have many site that are running under drupal "public" method, but also give access with filebrowser to folders that are not in drupal root. Other example is people that have a mix of filebrowser folders with both public and private methods depending of security issues, root folder position, etc.

My second point was that drupal know only 2 methods (private/public). Filebrowser is designed to handle very customized download methods (using specific hooks) in order to handle very specific situations (for example some people are making random symlinks to use apache for serving "private" files, some other people are using this to check specific file rights against some specific webservices, etc.).

So Drupal public/private settings can be a default for filebrowser (I'll do that soon) but can't be used for all filebrowser nodes.

Nicolas Georget’s picture

Nicolas Georget commented

Is your test made in a subfolder ? Are you using linux ?

Yes for the both questions.

So Drupal public/private settings can be a default for filebrowser (I'll do that soon) but can't be used for all filebrowser nodes.

Ok. understood. Because for me, the only difference was the method used into the function filebrowser_filebrowser_download_manager_process():

  • public Method which is a simple redirection header("Location: " . url(trim($target, '/'), array('absolute' => TRUE)));
  • Private Method that is this time a more complex header functions called to force the download
    header('Content-Description: File Transfer');
header("Cache-Control: public, must-revalidate, max-age=0"); // HTTP/1.1
...

I did not consider the other cases like symlinks, security issues, etc...

Yoran’s picture

Yoran commented

Ok so your problem is "zip selected files from a subfolder leads to an empty zip file", right ? Did you try from root folder ?

Nicolas Georget’s picture

Nicolas Georget commented

Ok. let's say a tree like this:

+ Root Folder (sites/all/files/test)
|
+-- ~/test/filebrowser.install
|
+-- ~/test/filebrowser.module
|
+-- ~/test/filebrowser.pages.inc
|
+-- ~/test/icons
|   |
|   +-- ~/test/icons/ascii.png
|   |
|   +-- ~/test/icons/audio.png
|   |
|   +-- ~/test/icons/binary.png
|
+-- ~/test/robots.txt

When i check the checkbox in front of the folder ~test/icons and choose the action "Download selected items as an ZIP archive (only files)", i get an empty archive....

Aïe, aïe, aïe... Quel con(*) !!!
By copying and pasting the action text "Download selected items as an ZIP archive (only files)" I just noticed the last and the most important part: (only files) !!!
This action only works for files right ?? not for a folder and its content !!

(*) in french /kɔn/, means i'm an idiot, a dummy....

Yoran’s picture

Yoran commented
Status: Postponed (maintainer needs more info) » Closed (won't fix)

Yep, only files in zip :))) We can try to make better for next release if you want => feature request ?