This a patch to Drupal core to implement public/private file handling. At this time it isn't entirely complete and needs plenty of bug fixing. This code will be rapidly maturing over the next couple of weeks and I'll be posting some testing documentation in a couple of days. A description of changes that implement this are below.

First, lots of changes in file.inc to cope with having two file paths. This involved changes to file_create_url and file_create_path to return correct paths and file_set_private for database manipulation and moving files around. There were also function changes to file_move and file_copy to support some of the things going on. The last big change was file_debug, which will presumably be removed but has made life easier for debugging. A list of changed funtions is below.

file.inc

  file_create_url
  _file_create_url
  file_create_path
  _file_create_path
  file_set_private
  file_directory_private_path
  file_debug

Changes in upload module are related to updating forms to have a public/private values for files and adding file_set_private to upload_save. A list of affected functions is below.

upload.module

  upload_save
  _upload_prepare
  upload_get_private
  theme_upload_form_current
  _upload_form

There have been mostly cosmetic changes in system.module. One new database column is added to the files table, a private path variable is added and private/public file preferences.

system.module / system.schema

I have added mostly a new file, private.php, to avoid a full drupal bootstrap when piping files through Drupal. For now this is mostly a skeleton but really only needs minimal code.

Known Issues
* JS uploads are broken (this doesn't appear to be from changes in this patch).
So for testing please turn JS off for the moment.
* Nodes can only have one file attached, a bug is dropping data from other files.
* When the private directory is created it isn't protected with a proper .htaccess file.
* The process of uploading and then setting a file to private is a bit hackish and could
use some refactoring.
* An update function needs to be created for the one schema change.

Comments

Status:Active» Needs work

For some reason the majority of your patch is devoted to removing the update module's files. Perhaps you need to CVS up and re-roll?

what's the purpose of private.php? is this a work in progress?

per the coding standards there should be a space after the comma:

+  $csspath = _file_create_path('css',FILE_DOWNLOADS_PUBLIC);

you do it a few other places too.

What's the purpose for changing these constants?

-define('FILE_DOWNLOADS_PUBLIC', 1);
-define('FILE_DOWNLOADS_PRIVATE', 2);
+define('FILE_DOWNLOADS_PUBLIC', 0);
+define('FILE_DOWNLOADS_PRIVATE', 1);

If they're being used in a different context with a different value you should change the name so it doesn't screw people up.

The indenting on the PHPDoc is incorrect and there shouldn't be two spaces between function and the function name:

+
+/**
+ *  Create download path to a file
+ *
+ *  @return A string containing the path to the desired file
+ *  @param &$file A file object.
+ *
+ */
+
+function  file_create_url(&$file) {

The PHPdoc for this doesn't really explain how it's used... how is it different than file_create_url()?

/**
  * Create the download path to a file.
  *
  * @param $path A string containing the path of the file to generate URL for.
+ * @param $private An integer indicated whether the file is public (0) or private (1).
  * @return A string containing a URL that can be used to download the file.
  */
-function file_create_url($path) {
+function _file_create_url($path, $private = 0) {

Actually, why are you creating two separate functions for this? It seems simple enough that it could be done in one. That actually goes for all of these functions. Why not just fix file_create_path() rather than adding a wrapper?

You're not documenting the parameter you're adding to file_copy() and file_move().

file_private_downloads() and upload_get_private() are both only called once. You'd probably be better just calling variable_get() directly.

You should be using the constant rather than the value:

-      $picture = file_create_url($account->picture);
+      $picture = _file_create_url($account->picture,0);

As you note file_set_private($file) is weird. I think the correct way to do this would be to add a new status constant:

define('FILE_STATUS_PRIVATE', 2);

And then use bitwise operators to test and set the status ($status & FILE_STATUS_PRIVATE to test and $status | FILE_STATUS_PRIVATE to set) then use file_set_status() to update the database.

Sorry, just read:

I have added mostly a new file, private.php, to avoid a full drupal bootstrap when piping files through Drupal. For now this is mostly a skeleton but really only needs minimal code.

so ignore that part of my feedback.

StatusFileSize
new76.07 KB

Alright, I've re-rolled the patch and it seems to have gotten rid of all that nasty update.module stuff. I fixed style errors where I found them and I removed the two functions that were just kind of wrapping up variable_get.

In regards to the constant change, this was to make it so the public/private values were in line with the private field in the database being either true or false. It seemed like a reasonable change to make since a sitewide public/private files setting isn't being used anymore and furthermore was little used before this patch started using the constant values.

Having file_create_url and _file_create_url was at the suggestion of dopry. file_create_url is using the file object interface which isn't being used in some parts of the code and would be difficult to. However it makes life easier in file.inc and upload.module by just passing $file around rather than accessing all of its different members depending on the function. I'm not sure if this is the best way to do things, but I think having both at least for some transitional period might make sense.

I'll look into some methods to clean up file_set_private a bit. I think the only really messy case is with uploading, but that's the important one.

Also, it seems that patches in core have fixed the problem with attaching more than one file to a node. So that's working now!

Looking a lot better but there's still some issues.

You're still editing MAINTAINERS.txt, profiles/default/default.profile and the change to includes/bootstrap.inc is probably unneeded. On that note do you really mean to be editing includes/language.inc, misc/collapse.js, misc/jquery.js, modules/search/search.module, modules/system/system.admin.inc, themes/garland/style.css.

I've found it very helpful to review the patch file before I post it. Often this lets me catch small changes that I didn't mean to make.

The PHPDoc is incorrect, see the Doxygen formatting conventions:

+/**
+ * Create download path to a file.  Unlike _file_create_url
+ * this takes a file object as input rather than a path.
+ *
+ * @return A string containing the path to the desired file
+ * @param &$file A file object.
+ *
+ */
+function file_create_url(&$file) {

This seems wonky, you should either use a Boolean or point people towards the constants you define:

+ * @param $private An integer indicating whether the file should be copied to Drupal's
+ *   public or private file path.
+ *   Zero - The public file path should be used.
+ *   Non-zero - The private file path should be used.

The previous comment goes for:

+  return array('url' => _file_create_url($file,0), 'struct');

missing space between parameters:

+  $dest = _file_create_path($dest,$private);

I think this part of the patch should actually be submitted as a separate patch because it looks like it'd fix an issue I've run into before but never tracked down a solution to:

@@ -787,7 +877,8 @@ function file_set_status(&$file, $status
  * @param $headers An array of http headers to send along with file.
  */
function file_transfer($source, $headers) {
-  ob_end_clean();
+
+  @ob_end_clean(); // Supress error messages because there may be no buffer!
   foreach ($headers as $header) {
     // To prevent HTTP header injection, we delete new lines that are

This is indented incorrectly:

+  $form['settings_general']['upload_private_default'] = array(
+  '#type' => 'radios',
+  '#title' => t('Upload access default'),
+  '#default_value' => variable_get('upload_private_default', FILE_DOWNLOADS_PUBLIC),
+  '#options' => array(FILE_DOWNLOADS_PUBLIC => t('Public - files are available using HTTP directly.'), FILE_DOWNLOADS_PRIVATE => t('Private - files are transferred by Drupal.')),
+  '#description' => t('This determines whether file uploads will be publicly or privately accessible by default')
+  );

You don't need the trailing space in the parameters:

+  $file = _file_create_path($file, FILE_DOWNLOADS_PRIVATE );

You're adding a new blank lines to the beginning of upload_nodeapi() and upload_save()

StatusFileSize
new27.51 KB

I've rolled a new version of the patch fixing problems that you noted. The patch is much more clean now. I've also done some testing and there are still some issues.

1. JS Uploads still aren't working
2. Files with duplicate names aren't working.

I think the JS issue may be form API related. I'll do some more testing and try and get a good backtrace.

On the upside of things, nodes can have more than one file attached with mixed public/private status on the files!

Status:Needs work» Needs review

subscribe .. i posted once on this topic - not sure if it is helpful at all - http://www.tejasa.com/node/113

@moshe

I like the idea, but I think that a bootstrap is necessary, because checking tokens would still mean that the file is publicly accessible. I.E. users could bypass Drupal altogether and talk directly to the webserver, which will not check for tokens.

I think a good option for performance (if not necessarily user friendliness or ease) would be interfacing with apache (or whatever the webserver happens to be) and do .htaccess checking. Unfortunately, doing that would be extremely complicated.

Right now the plan is a compromise, a new file has been added (private.php) which will bootstrap DRUPAL_BOOTSTRAP_SESSION to avoid a full bootstrap and files will be piped through that.

@CitizenKane - it seems like you assume that the file will live within the web root but thats not how private files works today, nor did i imagine it that way in my blog post.

StatusFileSize
new29.37 KB

@moshe
If I'm not mistaken if it isn't in the web root then apache can't touch it without some help from PHP, which means bootstrapping.

@everyone
I've rolled a new version of the patch, this has fixed some style problems and also has rough beginnings for private.php. Currently, things seem to be working pretty well but issues may be encountered. For instance, duplicate files can be placed on the same node, but if they are private the system URL has a bug in which a page not found will be returned. Any testing, feedback or comments are appreciated.

"If I'm not mistaken if it isn't in the web root then apache can't touch it without some help from PHP, which means bootstrapping."

Certainly PHP must get involved. A bootstrap is not necessarily required. One can just read a file from anywhere in the filesystem and serve it. thats how file_transfer() works. private files today are stored outside of web root.

moshe, if it's private and you're checking permissions that means bootstrapping (at least the database) to check permissions... right?

@drewish - in that post i linked to, i proposed a temporary url which is checked for correctness. correctness could be a hash on the session cookie so no, drupal would need to bootstrap at all. see that post. i was a bit brief but hopefully this clicks for you.

perhaps a hash of the filename and session cookie... otherwise they'd be able to view any private file.

@drewish - yes, good idea.

StatusFileSize
new30.51 KB

I have the lastest patch attached. This includes writing an .htaccess file in the private-files directory that restricts outside access. Additionally the patch has been updated to stay current with mainline changes.

StatusFileSize
new34.6 KB

I've updated the patch to stay current with head. The only thing still lacking is a working private.php. This patch needs some review and testing.

subscribe

Version:6.x-dev» 7.x-dev

This is a *huge* feature patch, and we are focusing on Drupal 6 stabilization, so make sure you get this in when Drupal 7 development starts.

I know code freeze is over, but if this patch is stable... i really waiting long time for public and private file handing... the current "non" choice is really a big big security problem on one side and on the other side i cannot use color module, css aggregate and compress and so on and so on. That's all really bad... code freeze or not - this would be a GREAT feature and i hope we can rethink if this could get in or not... waiting another year for this small feature could be a sounds like a mistake.

hass, we've all got features we'd love to squeeze into D6 but once you start down that slippery slope we'd never get a release shipped. you just need to remember start working on this as soon as the D7 code base is opened up.

Sorry for the very late reply to all this, we're aiming to get this in D7. There are code changes all over the place so it would be somewhat difficult to guarantee stability without lots of code testing. I'll be getting on top of this an getting things cleaned up when I have some more time.

subscribing...

This is interesting... here's a thought:

One doesn't actually need an option for public or private handling. No, there should be only one way files are handled, and hence the public/private option should be removed.

However, files that have any kind of protection do need catering for. That is, files should be split into two groups:

  • Those that are open to the public
  • Those that are only available to certain roles, or groups, or otherwise protected (taxonomy_access_control, etc, etc, etc).

Let's assume for a moment that both groups are accessed by the same URL, and that is under the system/files... path.

When a file is created, a check is made to see which group it fits into (public, or some kind of protection).

If the file is public, it is placed in directly in the system/files directory. Yes - this is new, this directory will have to be under the web root, and writable by the web server.

If the file has protection, it is placed in the 'File system path' directory, which must reside outside of the web root - in reality this should be renamed 'Protected file system path' perhaps.

Now, when a request for something in system/files... comes it, if the file is public it will be found by the web server and served up directly. Easy, very fast, no bootstrap or anything required.

If no real file on this path exists then processing as now (Drupal 5.x) will take over. If a protected file with name exists it will be returned only if the user has permission using the current checks and PHP read/output code. If no such protected file exists then the 404 handler is used as now.

I would assume that most files are public, with only a small number being protected (well, it's like this on our sites). This being the case, most file accesses would be handled directly by the web server with no PHP processing required - a great performance improvement.

I can perhaps see a problem if one introduces protection for a massive set of files that have to be moved from public to protected (or visa-versa) and the two directories are not on the same file system (hence movement would be slow). Maybe this is a flaw, but surely can be overcome.

Is this a good idea, or is there a better approach?

Obviously this wouldn't work, but I want to make sure the D7 issue queue is nice and clean when necessary. Upon testing, this patch failed:

$ patch -p0 < publicprivatefiles_0.patch
patching file private.php
patching file includes/common.inc
Hunk #1 succeeded at 1768 (offset 144 lines).
Hunk #2 succeeded at 1895 (offset 227 lines).
Hunk #3 succeeded at 2240 (offset 378 lines).
Hunk #4 succeeded at 2263 (offset 154 lines).
patching file includes/file.inc
Hunk #7 FAILED at 639.
Hunk #8 succeeded at 872 (offset 5 lines).
Hunk #9 succeeded at 905 (offset 5 lines).
Hunk #10 succeeded at 937 (offset 5 lines).
Hunk #11 succeeded at 1066 (offset 7 lines).
Hunk #12 FAILED at 1091.
2 out of 12 hunks FAILED -- saving rejects to file includes/file.inc.rej
patching file includes/locale.inc
Hunk #1 FAILED at 1629.
Hunk #2 FAILED at 2100.
Hunk #3 FAILED at 2155.
3 out of 3 hunks FAILED -- saving rejects to file includes/locale.inc.rej
patching file modules/blogapi/blogapi.module
Hunk #1 succeeded at 374 (offset 7 lines).
patching file modules/locale/locale.install
Hunk #1 succeeded at 213 (offset 87 lines).
patching file modules/system/system.admin.inc
Hunk #1 succeeded at 1379 (offset 113 lines).
Hunk #2 FAILED at 1399.
1 out of 2 hunks FAILED -- saving rejects to file modules/system/system.admin.inc.rej
patching file modules/system/system.install
Hunk #1 succeeded at 2018 with fuzz 1 (offset -1696 lines).
patching file modules/system/system.module
Hunk #1 succeeded at 718 (offset 128 lines).
Hunk #2 succeeded at 731 with fuzz 2 (offset 128 lines).
can't find file to patch at input line 580
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|
|=== modified file 'modules/system/system.schema'
|--- modules/system/system.schema 2007-09-11 19:14:34 +0000
|+++ modules/system/system.schema 2007-09-26 03:04:08 +0000
--------------------------
File to patch:

Status:Needs review» Needs work

Someone able to re-roll... so we get this in... :-)

I hope there is someone to reroll this (if not CitizenKane himself). It'd be a travesty for him to have wasted all that work.

I do not know how to reroll things. If someone would like to teach me, I'll be on IRC tomorrow afternoon ;). Sorry for complaining and not being able to do it myself, I just want to make sure we don't waste this patch.

i'm looking to help get this in too. but i'm not sure what to do.

(subscribe)

subscribing

Subscribe

subscribing

Sorry everyone. Taking five classes right now plus work to afford rent, so I haven't had time to get this patch synced up with head. I have a bazaar branch that people can work on if anyone is interested. I will soon have time to get this patch synced up and finished, and at the very least I will make sure that there is someone to do it if for some reason I can't.

at raintonr in #24: how do multisite setups fit in your picture?

(subscribing too)

Well, surely multi-site file handling can be done in the same way as 'public' file handling is right now. I didn't look into how that is done with multi-site but the same principle will surely apply.

StatusFileSize
new27.56 KB

Alright, patch has been re-rolled and can be applied against head. I need to do testing to verify that things are still working, but otherwise things should be just fine. It has a lot of tweaks and cleanups and the base has been put in for transferring private files. This should be hitting completed status within a week or two. If anyone wants to play with anything the bzr branch is at http://drupal.codeincarnate.com.

Status:Needs work» Needs review

Shoudn't this CNR?

Status:Needs review» Needs work

1. You shoudn't surround %d with single quotes in query's
2. Only add *one* blank between sentences (review all strings) and here is a typo (priavte) where priavte files will be stored.  This directory has to exist
3. t('Default Download method') should be written as t('Default download method')
4. Code style should be 'Content-Type: '. $file->filemime as I know.

This is all from code review... not tested.

Re: point 4 in #39, the string concatenation operator should always have spaces around it, which is missing in a lot of various areas in this patch.

@Susurrus: No, use coder module.

Wrong: 'Content-Type: ' . $file->filemime

Correct: 'Content-Type: '. $file->filemime
Correct: $test . $file->filemime

@hass: The coder module isn't up to date. Please see the coding standards.

There seem to be a code style change in D7... ok.

Will update the problems soon, there are also some bugs in the code that I need to fix, just trying to keep it up to date with HEAD was goal #1.

A rerolled patch, public and private files are pretty much working now. Including the ability to switch a file from being public to being private and vice versa. Big thing left is private.php which is mostly implemented, big thing is to get working key generation.

Hmm, patch didn't didn't seem to get attached. Trying this again.

Question:

All this discussion centers around FILE security for different user classes.

How about page security? (certain pages only available to particular user classes?) Is anything done on this?

Re: #48. Have a look at Taxonomy Access Control module. Or using OG to protect certain pages (put in non-public group).

Currently doing some code review with dopry, will post improved patch soon.

@CitizenKane: Are you working on the improved patch?

@hass: yep, the patch is essentially feature complete and has been since the end of July, I'll get it synced up to head and get it here soon (hopefully tomorrow)

?

subscribe

@CitizenKane: "tomorrow" is now over one month ago... any way to get this important feature forward?

Note that some people are using private files because they can't or wont allow a writable directory inside the web server root. I suggest adding a setting that - if specified - overrides the FILE_DOWNLOADS_PUBLIC argument supplied to _file_create_path() and _file_create_url() and makes these act as if FILE_DOWNLOADS_PRIVATE was supplied instead. I.e. if this setting is enabled, _file_create_url($file, FILE_DOWNLOADS_PUBLIC) returns a private URL.

It's true that some parts of core currently don't work with private files (e.g. #250451: locale.module adds wrong js path and #146611: Allow JS/CSS aggregation in private mode), but this is often caused by improper use of file_create_url() and is easily fixed by generating proper URLs and supplying a proper download hook.

However - this feature is here much more important than a workaround somewhere else as it would also fix the all other issues. I don't understand what the problem is with a public writeable directory. If you don't like to add your normal files there, put them in private part, but CSS/JS compressed files are saved in public to circumvent bootstrap performance degradation.

Subscribe

Any updates, since the patch is supposed to be (almost) ready?

StatusFileSize
new27.75 KB

Doing cleanup of file.inc and upload.module to make getting this in more possible. The hook_file patch that drewish landed has really changed things and has added some complications. There is a non-working patch attached just for people to see where it is at. I've been going through everything and found issues in file.inc and upload.module. More work will be forthcoming.

subscribing

Subscribing.

subscribing

Subscribing.

@CitizenKane: Are you working on this feature?

Tracking

subscribing

Marked #146611: Allow JS/CSS aggregation in private mode "Wont Fix" to try and get all the effort focussed in one area to get this resolved!

subscribing

subscribing

@CitizenKane: We mailed some weeks ago and I asked you if you are working on this case. You said yes, but I do not see any progress. If you don't actively plan to work on this issue, please unassign it and provide us the latest code and all information we need to start working on it. Would also be great if you are able to sum up the current status in all details.

We are only ~2 months ago from code freeze and I'd really like to see this feature in D7. If you cannot work on it give us all information you have (for e.g. name the blockers) so we can work on it and get it IN core in 2009, please. THX.

Title:SoC 2007: Public/Private File HandlingPublic/Private File Handling
StatusFileSize
new26.01 KB

yes, lets get this going again.

attached patch is an update based on #60 that i just got to a point that it applies to HEAD and doesn't kill a fresh install.

lets move forward with that, unless/unitl CitizenKane posts something better.

This one needs work...:-) the patch removes some things changed in D7 (for e.g. drupal_chmod) and other lines change a switch to if/else, but I do not see the need for this change and like switch more. Need to spend more time to understand the ideas first... would be great to have an idea about the implementation and than start working on the code... I will try your patch... maybe we see how this should looks like UI wise.

Assigned:CitizenKane» Unassigned

@hass: yes, discussion would be good. i really just updated the patch to give CitizenKane a clear signal that we're not going to wait any more - i'm not set on anything in the current patch at all. i'll try to put up a summary tomorrow (unless you beat me too it :-)), then we can discuss some more before we get to a real patch.

I'd started to reply to this a while back after some conversations at the Denver Open Media Camp but the message got lost in a browser crash. I'll try to reconstruct since there seems to be some new interest in this issue.

We need three separate directories: public (web accessible), private (outside the webroot or protected by .htaccess), temp (best if it's outside the webroot or protected by .htaccess). For new installs this is straighforward but for upgrades it presents some complications. We'd need to create a new directory for the opposite of their current file mode e.g if they've got private file transfers enabled we need to create public files directory.

The public/private status should be stored in the $file->status bit flags.

file_create_path() and file_create_url() should really take $file objects... that or we need to go more object oriented and move those functions into member funtions e.g. $file->getUrl() and $file->getPath(). I think I'd actually be more into the later since it would allow us to have separate classes for Public/Private/Temp files that could hide the different logic.

I'm using IIS for testing and on page admin/settings/file-system I get:

The directory  does not exist.
Notice: Undefined index: DOCUMENT_ROOT in system_check_private_path() (line 1144 of C:\Inetpub\wwwroot\drupal7\modules\system
\system.module).

Assigned:Unassigned» beejeebus

drewish: thanks for the reply. i plan to hack on this at the boston drupalcamp this weekend, so any further thoughts are welcome.

justinrandell, well i'm planning on being there too so we can discuss it further. i'm also starting to believe that this really needs to be done along side #227232: Support PHP stream wrappers since it gives us a nice OOP way of isolating the code for the various filesystem modes.

drewish, cool, i'll see you there and review the stream wrappers issue.

subscribe

subscribe

make sure to watch #499156: CDN integration: allow file URLs to be rewritten by hook_file_url_alter() as you'd have to reimplement file_create_url with that in mind once it goes in. also, as has been pointed out, the stream wrapper patch already implements this in another way. perhaps that functionality can be pulled out from there and implemented here in that fashion, as a dependent patch? not sure it's worth the effort or is even feasible though; just an initial brainstorm.

subscribing

#227232: Support PHP stream wrappers went in, so we should be able to get this done easier now.

#517814: File API Stream Wrapper Conversion is another follow up patch.

subscribing

Status:Needs work» Closed (duplicate)

marking this as a duplicate of #517814: File API Stream Wrapper Conversion

In #84 we have been told it's a follow up and not a duplicate?

and in #86 i'm telling you it's a duplicate. it was fixed by #517814: File API Stream Wrapper Conversion. grab a copy of HEAD and give it a whirl.

for those looking for workarounds for private download support:

according to issue 146611#comment-1182361 at #146611: Allow JS/CSS aggregation in private mode D7 is expected to ship with public/private by file functionality, so creating a new folder for D6 was denied

nevertheless, making a public folder available to support features unavailable for private download method is a very affordable cost

so I'll be maintaining such as a path for D6 at #181003: private download method and dynamically generated CSS and JS