Summary:	ASTERISK-04225: [patch] Add password to the VocemailMain function parameters
Reporter:	ssljivic (ssljivic)	Labels:
Date Opened:	2005-05-19 02:55:34	Date Closed:	2011-06-07 14:04:42
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Applications/app_voicemail
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) vm-main-pass-1.0.3.patch
Description:	Hi, I have updated the VoicemailMain application so that it is possible to pass password of the mailbox as well. The format is now: VoiceMailMain([mailbox][:password][@context][|options]) Hope that this will be accepted. Regards, Stojan Sljivic
Comments:	By: Russell Bryant (russell) 2005-05-19 08:30:17 What is the advantage of using this over the 's' option? By: Brian West (bkw918) 2005-05-19 08:32:27 Was about to say it doesn't look like it has any advantage over s /b By: ssljivic (ssljivic) 2005-05-19 08:46:59 With option 's' there is no password validation. We are working on Asterisk integration with another PBX that does not have voicemail. So when we send the call to the Asterisk for voicemail processing we pass mailbox ID and password that is collected on the proprietary IVR. Without this new feature a security could be endangered by accessing any mailbox (since no password is required). This feature increases security level over the 's' option. Regards, Stojan Sljivic By: Michael Jerris (mikej) 2005-05-19 09:01:45 Please mention that you have a disclaimer next time, I can see from 4285 you do. By: ssljivic (ssljivic) 2005-05-19 09:09:30 Sorry. My disclaimer is on file. Regards, Stojan Sljivic By: Russell Bryant (russell) 2005-05-19 09:13:22 Ok, I think that's reasonable.  However, since this is a new feature, it will not be included in the 1.0 branch.  You will need to provide a patch for CVS HEAD. Also, if you, or anyone else, feels like doing it, it would make sense to add this to VMAuthenticate as well.  It's not absolutely necessary, though. By: ssljivic (ssljivic) 2005-05-19 09:17:15 Hi drumkilla, This patch is for CVS HEAD 05/17/05-11:25:10, not for 1.0. Regards, Stojan Sljivic By: Russell Bryant (russell) 2005-05-19 09:56:29 Ok.  The "1.0" in the patch name had me confused.  I guess you meant version "1.0" of the patch.  :) By: ssljivic (ssljivic) 2005-05-19 10:00:52 Exactly, I'm versioning the patches. The Asterisk version the patch is for is in the bug properties/description. By: Russell Bryant (russell) 2005-05-19 10:10:40 In the following line ... + if (vmu && (vmu->password[0] == '\0' || (vmu->password[0] == '-' && vmu->password[1] == '\0'))) { Why would you have a special case to allow "-" to be treated as a blank password?  Unless there is a dependency on this somewhere else in the code, I recommend only checking for (vmu && vmu->password[0]) By: Kevin P. Fleming (kpfleming) 2005-05-19 13:06:01 ... which would more rightly be ast_strlen_zero() :-) By: Gregory Hinton Nietsky (irroot) 2005-05-19 17:20:53 while we are about it i propose a t [transparent] option foir vmauthenticate that will not wait for nor prompt for the mailbox when mailbox[@context] are supplied the prompt for the password can then be governed by the s option this is currently used by me to authenticate for functions such as a call forward request .... most usefull is it ... see 4340 By: ssljivic (ssljivic) 2005-05-20 02:39:07 Hi drumkila, I haven't added the code: + if (vmu && (vmu->password[0] == '\0' || (vmu->password[0] == '-' && vmu->password[1] == '\0'))) { It was there before my updates, so I didn't want to change anything that I'm not sure what it is for. Regards, Stojan Sljivic By: ssljivic (ssljivic) 2005-05-20 02:42:16 Beside the line: if (vmu && (vmu->password[0] == '\0' || (vmu->password[0] == '-' && vmu->password[1] == '\0'))) { there are several more places in the code where '-' is compared against the password. Regards, Stojan Sljivic By: Michael Jerris (mikej) 2005-05-21 10:39:18 // comment in 4553 block of this patch... Is this tested? By: ssljivic (ssljivic) 2005-05-23 03:36:36 That comment in the block 4553 is a mistake. New patch will be uploaded. By: Russell Bryant (russell) 2005-05-23 18:27:49 Comments on 1.0.1 of your patch ... 1) Go ahead and use ast_copy_string instead of strncpy.  Also, note that with ast_copy_string, you do not need to subtract one from the buffer length 2) Instead of checking (vmu->password[n] == '\0'), please use ast_strlen_zero(vmu->password).  I realize that this was already there, but it might as well be cleaned up while we're at it. 3) In chunk number 6 of your patch, why is there a new block there when it does the exact same thing that comes right after it?  I think your intention may have been to print out a different message to the CLI if an incorrect message was provided via the call to the VoiceMail application. By: ssljivic (ssljivic) 2005-05-26 03:18:48 Hi drumkilla, New updated patch has been uploaded. Regards, Stojan Sljivic By: Russell Bryant (russell) 2005-05-26 08:33:53 Why use a separate parameter to vm_authentice for the password when there is already a password field in 'struct ast_vm_user' ?  You could then just check to see if that field was initialized in vm_authenticate before asking the user to provide a password. By: Clod Patry (junky) 2005-06-20 18:59:01 ssljivic: poke? whatcha think about what drumkilla said? Can we have any answer here? Thanks. By: ssljivic (ssljivic) 2005-06-22 02:26:23 Hi, I have already uploaded new patch (1.0.3) with implemented suggestion from dumkila. Regards By: Russell Bryant (russell) 2005-07-18 10:00:58 I think this is just about ready to go, except for a couple little things. 1)  In the case where you don't specify a password, your code will seg fault when you call ast_copy_string in the last hunk of your patch. 2)  In vm_authenticate, I don't think you need the extra buffer pwd, when later, you just copy it over to the password buffer.  :) By: Michael Jerris (mikej) 2005-07-25 22:13:37 suspended due to 1 week no response to update request.  Re-open when updates are ready.  Thanks!