Summary:	ASTERISK-05774: [patch] make voicemail files group writable
Reporter:	Tzafrir Cohen (tzafrir)	Labels:
Date Opened:	2005-12-04 11:34:32.000-0600	Date Closed:	2006-01-13 15:07:04.000-0600
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Applications/app_voicemail
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) vm_fix_perms_10.diff.txt ( 1) vm_nofix_perms4.dpatch
Description:	we needed to be able to write to the voicemail files from an apache in the group 'asterisk'. It turns out, however, that app_voicemail creates files and directory with hardwiered non-group-readable permissions. There is basically one place where files are created. However there are many places where directories are created. Normally with a series of mkdir-s (which seem quite replicated), but ometimes even using 'mkdir -p'. Thus this patch mainly cleans up the directory creation in app_voicemail.c . It also sets aside #define-s for the modes of file creations and directory creations. The 1.0 version was actually tested, and this is why it is attached here. The version for 1.2 is known to build. I have not yet tested HEAD. However it seems that the code has not radically improved since.
Comments:	By: Tilghman Lesher (tilghman) 2006-01-05 15:04:50.000-0600 This needs to be configurable via an option in voicemail.conf.  In addition to permissions, the group is also probably something that needs to be configured. By: Tzafrir Cohen (tzafrir) 2006-01-05 15:21:14.000-0600 You can leave things as they are (or #define them to the old value). But more importantly: the patch makes it possible to change in one place by getting rid of crift code. Another note: setting explicit modes is actually unnecessary: you can set them to 777, and have the user set Asterisk's umask at Asterisk startup. By: Tzafrir Cohen (tzafrir) 2006-01-05 15:34:59.000-0600 In short: this patch cleans up rather than adds features. By: Tilghman Lesher (tilghman) 2006-01-05 16:13:51.000-0600 In that case, please re-upload your patch with the current settings in SVN, not your custom settings. By: Tzafrir Cohen (tzafrir) 2006-01-06 14:28:49.000-0600 New version of the patch: now it only cleans up app_voicemail.c . Also note that those are not exactly "custom setings": allowing someone other than the asterisk user to read voicemail is hardly a bug. You wouldn't want to run your httpd as the user asterisk, but rather , maybe add it to the group asterisk. By: Jason Parker (jparker) 2006-01-06 14:29:00.000-0600 Patch removes some ODBC functionality. I noticed this was also in your first (but not second) patch. Also, can you make it voicemail_dir() or voicemailfile(), so they are consistant? By: Tzafrir Cohen (tzafrir) 2006-01-06 14:37:49.000-0600 Could you please be more specific? what odbc functionality? Where exactly in the patch? Note that the second one (_10) is against asterisk 1.0.9, and will not patch on 1.2 . I have only tested that one. I don't used odbc, though. By: Jason Parker (jparker) 2006-01-06 14:49:30.000-0600 All of hunk 3. @@ -1108,20 +1145,6 @@ It still compiles for you, because you haven't defined USE_ODBC_STORAGE. By: Tilghman Lesher (tilghman) 2006-01-06 15:04:32.000-0600 If you also got rid of the useless renames (make_file and make_dir), this patch would be a whole lot smaller, while still cleaning up. By: Tzafrir Cohen (tzafrir) 2006-01-06 15:09:00.000-0600 Offending hunk removed. One other clarification: the patch also renames the static function "make_file" to "voicemail_file". The name "make_file" is confusing as it does not make any file (think mkdir): it only composes a file name. Ditto for "make_dir" renamed "voicemail_dir". By: Tzafrir Cohen (tzafrir) 2006-01-06 15:33:43.000-0600 Hmmm, I wrote my previous comment before reading Corydon76's comment. Recent upload is a version with no renames. By: Tilghman Lesher (tilghman) 2006-01-06 16:04:42.000-0600 Another thing just occurred to me:  previously, you could invoke voicemail on a system that didn't even have the /var/spool/asterisk directory and it would work.  With this patch, you aren't creating /var/spool/asterisk if it doesn't exist, and the whole thing will fail. One approach to solve this might be to use ast_app_separate_args with a delimiter of '/', then reconstruct each part of the path with a for loop and strncat, attempting the mkdir at each progressive path addition.  Remember that the spool directory can be configured to be anywhere, so add a fudge factor into the possible number of components in the path.  10 or 12 should be enough. By: Tzafrir Cohen (tzafrir) 2006-01-06 16:33:50.000-0600 Are you sure that /var/spool/asterisk would have been created in all the cases? Actually from what I see, there is only one system(mkdir -p) that was replced (in the case of leaving a message, I believe). The others were plain mkdir-s that would not have generated the parent /var/spool/asterisk . The code there lacks any check for the result of the mkdir, BTW. So if there is no such directory and you have no permissionsto create one, you'll get strange errors in your log. By: Tilghman Lesher (tilghman) 2006-01-07 07:36:10.000-0600 That's true, but in the case of retrieving messages, if the directory doesn't exist, then there are no messages, so it's not a big deal.  But if the /var/spool/asterisk directory doesn't exist, then nobody can leave a message, with this patch applied.  That WILL cause somebody a problem. By: Tzafrir Cohen (tzafrir) 2006-01-07 07:37:13.000-0600 version number 4: this time I simply removed the hunk that patched the "mkdir -p". So you're left with the old code that generated /var/spool/asterisk/voicemail if it did not exist (and if it had the permissions). And all of that through system(). I don't like it, but it is simpler to get the rest of the patch applied and then worry about this ugliness. By: Matt O'Gorman (mogorman) 2006-01-13 15:05:57.000-0600 vm_nofix is commited as of 8068 in trunk