Summary:	ASTERISK-25917: [patch]app_voicemail: passwordlocation=spooldir only works if you manually add secret.conf yourself
Reporter:	Jonathan R. Rose (JonathanRose)	Labels:
Date Opened:	2016-04-12 17:18:49	Date Closed:	2016-05-04 08:42:38
Priority:	Major	Regression?	Yes
Status:	Closed/Complete	Components:	Applications/app_voicemail
Versions:	11.22.0-rc1 13.8.0-rc1	Frequency of Occurrence	Constant
Related Issues:
Environment:		Attachments:	( 0) asterisk_write_new_config.diff
Description:	Long time no patch. passwordlocation=spooldir is a general section option in voicemail.conf that makes changed voicemail passwords be stored in a secret.conf file within the individual mailbox folders rather than writing the change directly to voicemail.conf I tracked this feature back to 1.8 which is the first version where it was implemented. It doesn't work in the final version of 1.8 or in any version since because ast_config_text_file_save won't write new files. It exits early on account of failing an access check against the file that doesn't exist yet. I've written a very simple patch against Asterisk 11 that just adds a check for when the file doesn't exist and skips the check against read and write access check if that's the case. I'll be attaching the patch to the issue, and hopefully my new contributor agreement will be authorized soon.
Comments:	By: Asterisk Team (asteriskteam) 2016-04-12 17:18:50.353-0500 Thanks for creating a report! The issue has entered the triage process. That means the issue will wait in this status until a Bug Marshal has an opportunity to review the issue. Once the issue has been reviewed you will receive comments regarding the next steps towards resolution. A good first step is for you to review the [Asterisk Issue Guidelines|https://wiki.asterisk.org/wiki/display/AST/Asterisk+Issue+Guidelines] if you haven't already. The guidelines detail what is expected from an Asterisk issue report. Then, if you are submitting a patch, please review the [Patch Contribution Process|https://wiki.asterisk.org/wiki/display/AST/Patch+Contribution+Process]. By: Jonathan R. Rose (JonathanRose) 2016-04-12 17:23:56.534-0500 Oh, I should probably add... I tested the patch a couple of different ways. 1. If the directory the config is being written to doesn't exist, it will still fail. The way it fails is slightly different since it's due to being unable to create the file instead of not having 'access'. Seems like expected behavior to me. 2. If the directory exists and the file doesn't exist then everything is fine. The file will be created fresh and the write should proceed as normal. By: Rusty Newton (rnewton) 2016-04-13 09:39:44.306-0500 Hey Jonathan! Good to see you! I'll open this issue up. By: Jonathan R. Rose (JonathanRose) 2016-04-19 12:28:23.089-0500 Oh hey, some more information on this issue... these access checks aren't in 11.6-cert13, so this is actually a regression. At the same time, I assume this change was made for a reason, so there's a good chance that my patch isn't the best answer to this problem. It might actually be more prudent to add the ability to write new files as an option to the config.h function being used here. Since it turned out to be a regression, I've bumped the severity to major. By: Jonathan R. Rose (JonathanRose) 2016-04-19 12:58:08.752-0500 Regression was introduced by commit 2fbdeafb19b52165f778f238589d34d60d693081 Author: George Joseph <george.joseph@fairview5.com> Date:   Wed Sep 10 16:01:44 2014 +0000    config: bug: fix truncation of included config files on permissions error        ast_config_text_file_save() currently truncates include files as they    are processed.  If a subsequent include file or the main config file has    a permissions error that prevents writing, earlier include files are left    truncated resulting in a frantic search for backups.        This patch causes ast_config_text_file_save to check for write access    on all files before it truncates any of them.        Will be applied 1.8 > trunk.        Tested by: George Joseph    Review: https://reviewboard.asterisk.org/r/3986/    ........        Merged revisions 422900 from http://svn.asterisk.org/svn/asterisk/branches/1.8            git-svn-id: https://origsvn.digium.com/svn/asterisk/branches/11@422903 65c4cc65-6c06-0410-ace0-fbb531ad65f3 By: Jonathan R. Rose (JonathanRose) 2016-04-19 15:31:08.096-0500 Thanks George. Let me know if you need any additional information. By: Jonathan R. Rose (JonathanRose) 2016-05-02 12:40:54.306-0500 I notice the issue is still open, so I'm going to go ahead and give a quick break down of what has happened so that whoever ends up seeing it through knows the situation... https://gerrit.asterisk.org/#/q/topic:ASTERISK-25917 George Joseph had a patch up on Gerrit that was accepted and committed to 11, 13, and master. George's patch also includes an internal test that verifies config file saving behavior. I've confirmed that this patch fixes the issue. I also have a patch up on Gerrit for the Asterisk testsuite. It's a test that uses a somewhat older voicemail testing system and specifically verifies that the password will be saved to the right file on a fresh system when using passwordlocation=spooldir. Asterisk sends a specific test event for when the password is saved using spooldir, so that event is checked along with the contents of the saved secret.conf file that goes in the spool directory. That patch has not yet been reviewed. Once that has been either accepted or rejected, this issue will be resolved.