Choice to store E-Mail attachments in private folder

Can you attach your patch? That would make it more likely that this feature will get implemented... :)

Thanks, that helps. This will happen eventually, probably before 1.3 is released.

As for development tips, I suppose you could start here:
http://drupal.org/contributors-guide

Personally I find it's easiest to just look at how existing modules do this. For example, if you wanted to turn the above into a real patch you could look at this previous commit where an admin option was added to the support module:
http://drupal.org/cvs?commit=264936

And this subsequent patch where per-client overrides were added to the global option:
http://drupal.org/cvs?commit=265280

And finally this patch which fixed a bug from the previous patch:
http://drupal.org/cvs?commit=272968

From the first CVS link, the admin option was easily added here:
http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/support/sup...

Finally, documentation on how to create a patch:
http://drupal.org/patch/create

Looking for this patch, subscribing.

Is there a need to keep files public or should they always be stored private? Perhaps this could be controlled with a simple checkbox in the settings area. One could also use the path for the content type to further organize the file structure when the file is being saved.

Would be glad to help with patching and testing, have you reworked davidv's suggestion Jeremy?

I'm trying to implement this on the 1.3 release version of the module but it refuses to save the file in the private folder.

Watchdog log says it is in private but in the db and in the node listing the path is not the private one.

Has there been any changes in the latest version that might affect this?

After deepscanning the Private Upload module i found a pieace of code which related to a "bug" in Drupal 6 or just the lack of knowledge of the FormAPI. The author had to set a private flag like I've implemented below to make it work. Also added rename feature in code to make it clear what's going on.

$attachment->private = TRUE; // This is the flag that has to be set
$attachment->filepath = file_save_data($attachment->attachment, file_create_path( variable_get('private_upload_path', 'private')) .'/'. utf8_encode($attachment->filename), FILE_EXISTS_RENAME);

I will make an admin setup for this and post code for it later this week.

Ok, i've set it up like this.

In support.admin.inc I've added a variable connected to a checkbox in the support_admin_settings form starting on line 354.

$form['user']['support_store_files_privately'] = array(
    '#title' => t('Private storage of attachments'),
    '#type' => 'checkbox',
    '#default_value' => variable_get('support_store_files_privately', FALSE),
    '#description' => t('Check this box if you would like to store attachments privately.'),
  );

In support.module I've added the following to check whether the feature is enabled or not starting on line 1148.

if(variable_get('support_store_files_privately', FALSE) == TRUE) {
  $attachment->private = TRUE;
  $attachment->filepath = file_save_data($attachment->attachment, file_create_path( variable_get('private_upload_path', 'private')) .'/'. utf8_encode($attachment->filename), FILE_EXISTS_RENAME);
}
else {
  $attachment->filepath = file_save_data($attachment->attachment, file_directory_path() .'/'. utf8_encode($attachment->filename)); 
}

This looks good to me. The only thing left to do is to also provide per-client configuration for this.

I was looking on doing it a per client basis from the beginning but changed my mind as I'm new to Drupal module Development.

I tried to use the same 'variable_get()' method in the form for the client settings and then using 'variable_set()' in the submit handler for the function. This would not work though as the variable wasn't client specific of course.

Would there be another way of implementing a variable this way or would it have to be written to the database? I could keep on it if you point me in the right direction.

See comment #3 in this issue for more details on creating a per-client patch. I hope to have time soon (2-3 weeks) to merge in this and other fixes/features. I will implement the necessary per-client functionality at that point if you've not already contributed a patch.

Ok, sorry for not working correctly on the patches. I've never added a patch before but have now set myself into it a little bit and hopefully I've come up with something useful.

I noticed that you have removed some code from the support_admin_settings() function where I had added the functionality of this issue in the dev version so I just added the checkbox option to the bottom of the form for now.

I haven't added the per client functionality yet but will look into this coming week. I do attach the patch for the changes I've done so far though just to make sure I got the patching part right :P

Okey, I have worked out a new patch now which also integrates the possibility to override the global setting in the private area.

Also added code in the install file to make room for an integer to be saved that keeps track of the selected option.

EDIT: Argh, this patch is useless, something went wrong, sorry for spamming :P

This should do it, ignore the last one.

I performed some white space cleanup (removed a bunch of tabs that your patch added to the support module and added spaces after logical operators to match the Drupal coding standards), and then read through the patch. My updated version is attached.

How is this setting affected by the core "File system" configuration options found at "admin/settings/file-system"? Why don't we simply support this configuration?

The actual code level implementation looks fine, but I need to understand the above before I'm able to commit. Thanks!

The reason I chose not to use that setting is to because I wanted to be able to control the individual Clients. Could this be done in another way using the file system settings?

Another reason is that I've experienced several issues when using the Private setting in the file-system prefs. This is most likely because my knowledge about it is limited. I have however, talked to many others with problems if using the Private setting.

Basically I just kept building on what the initial poster was talking about and did what came to mind. If you know of a better way of doing this I'd be more than glad to hear about it :]

Tested your latest patch and works fine as far as I can tell.

I second this or some other solution to this security hole .. I just had 3 ebay and paypal spoof emails sent through our help desk which in turn a hosted fake registration page through drupal

I want to merge this, but I feel like the global setting is wrong, that we should instead by respecting Drupal's public/private setting as global, and then if anything allow a per-client override of this global setting.