BUG10965: only write auth.conf if username/password are set

  auth.conf was always written, even if no username/password provided.
In this case only the ip or Hostname of the mailserver was written into
auth.conf. Now the file is only filled if username/password are filled.

Signed-off-by: Alexander Marx <alexander.marx@ipfire.org>
---
 html/cgi-bin/mail.cgi | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
  

Hi,

I think there is a problem with this patch.

On Sat, 2015-10-31 at 07:34 +0100, Alexander Marx wrote:
> auth.conf was always written, even if no username/password provided.
> In this case only the ip or Hostname of the mailserver was written
> into
> auth.conf. Now the file is only filled if username/password are
> filled.
> 
> Signed-off-by: Alexander Marx <alexander.marx@ipfire.org>
> ---
>  html/cgi-bin/mail.cgi | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/html/cgi-bin/mail.cgi b/html/cgi-bin/mail.cgi
> index be663a6..f5be104 100755
> --- a/html/cgi-bin/mail.cgi
> +++ b/html/cgi-bin/mail.cgi
> @@ -110,9 +110,12 @@ if ($cgiparams{'ACTION'} eq
> "$Lang::tr{'save'}"){ #SaveButton on configsite
>  		$mail{'SENDER'} 		=
> $cgiparams{'txt_mailsender'};
>  		$mail{'RECIPIENT'}		=
> $cgiparams{'txt_recipient'};
>  
> -		$auth{'AUTHNAME'}		=
> $cgiparams{'txt_mailuser'};
> -		$auth{'AUTHPASS'}		=
> $cgiparams{'txt_mailpass'};
> -		$auth{'AUTHHOST'}		=
> $cgiparams{'txt_mailserver'};
> +		if ($cgiparams{'txt_mailuser'} &&
> $cgiparams{'txt_mailpass'}) {
> +			$auth{'AUTHNAME'}		=
> $cgiparams{'txt_mailuser'};
> +			$auth{'AUTHPASS'}		=
> $cgiparams{'txt_mailpass'};
> +			$auth{'AUTHHOST'}		=
> $cgiparams{'txt_mailserver'};
> +			print TXT1
> "$auth{'AUTHNAME'}|$auth{'AUTHHOST'}:$auth{'AUTHPASS'}\n";
> +		}

My email client made a right mess out of this code above, but maybe you
check with the original one... So if the user wants to clear the
username and password and then hits save I guess these changes won't be
saved. They should though.

Can you confirm this? I think the solution should keep the first three
lines that are now in the if-clause before that and also clear
auth.conf when the configuration was cleared.

>  
>  		$dma{'SMARTHOST'}		=
> $cgiparams{'txt_mailserver'};
>  		$dma{'PORT'}			=
> $cgiparams{'txt_mailport'};
> @@ -129,7 +132,7 @@ if ($cgiparams{'ACTION'} eq "$Lang::tr{'save'}"){
> #SaveButton on configsite
>  			print TXT "$k $v\n";
>  		}
>  		close TXT;
> -		print TXT1
> "$auth{'AUTHNAME'}|$auth{'AUTHHOST'}:$auth{'AUTHPASS'}\n";
> +		close TXT1;
>  		close TXT2;

On a side note: It would probably have helped a bit to name the file
handles better. Like AUTHFILE or so instead of numbering them.

>  
>  	}else{

-Michael
  

> Hi,
>
> I think there is a problem with this patch.
>
> On Sat, 2015-10-31 at 07:34 +0100, Alexander Marx wrote:
>> auth.conf was always written, even if no username/password provided.
>> In this case only the ip or Hostname of the mailserver was written
>> into
>> auth.conf. Now the file is only filled if username/password are
>> filled.
>>
>> Signed-off-by: Alexander Marx <alexander.marx@ipfire.org>
>> ---
>>   html/cgi-bin/mail.cgi | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/html/cgi-bin/mail.cgi b/html/cgi-bin/mail.cgi
>> index be663a6..f5be104 100755
>> --- a/html/cgi-bin/mail.cgi
>> +++ b/html/cgi-bin/mail.cgi
>> @@ -110,9 +110,12 @@ if ($cgiparams{'ACTION'} eq
>> "$Lang::tr{'save'}"){ #SaveButton on configsite
>>   		$mail{'SENDER'} 		=
>> $cgiparams{'txt_mailsender'};
>>   		$mail{'RECIPIENT'}		=
>> $cgiparams{'txt_recipient'};
>>   
>> -		$auth{'AUTHNAME'}		=
>> $cgiparams{'txt_mailuser'};
>> -		$auth{'AUTHPASS'}		=
>> $cgiparams{'txt_mailpass'};
>> -		$auth{'AUTHHOST'}		=
>> $cgiparams{'txt_mailserver'};
>> +		if ($cgiparams{'txt_mailuser'} &&
>> $cgiparams{'txt_mailpass'}) {
>> +			$auth{'AUTHNAME'}		=
>> $cgiparams{'txt_mailuser'};
>> +			$auth{'AUTHPASS'}		=
>> $cgiparams{'txt_mailpass'};
>> +			$auth{'AUTHHOST'}		=
>> $cgiparams{'txt_mailserver'};
>> +			print TXT1
>> "$auth{'AUTHNAME'}|$auth{'AUTHHOST'}:$auth{'AUTHPASS'}\n";
>> +		}
> My email client made a right mess out of this code above, but maybe you
> check with the original one... So if the user wants to clear the
> username and password and then hits save I guess these changes won't be
> saved. They should though.
>
> Can you confirm this? I think the solution should keep the first three
> lines that are now in the if-clause before that and also clear
> auth.conf when the configuration was cleared.

Unfortunately not. With the flow of the code you should see, that 
auth.conf is always opened for writing (overwrite mode). If a username 
and password is set, the file is written due to the if clause.
Otherwise the file is just closed wich leads to an empty file.

In my tests, the system did exactly that. I tested a config without 
user/pass, and clicked "save". Then auth.conf is empty. Next step was to 
fill username/password and click "save" again. Final step: remove 
username/pass and again save file -> empty auth.conf.

Maybe someone else is able to test this?! Timo seems to be a good 
tester, he found the bug ;-)

But i will double check and give feedback if i was wrong, but this can 
take 1-2 days
>
>>   
>>   		$dma{'SMARTHOST'}		=
>> $cgiparams{'txt_mailserver'};
>>   		$dma{'PORT'}			=
>> $cgiparams{'txt_mailport'};
>> @@ -129,7 +132,7 @@ if ($cgiparams{'ACTION'} eq "$Lang::tr{'save'}"){
>> #SaveButton on configsite
>>   			print TXT "$k $v\n";
>>   		}
>>   		close TXT;
>> -		print TXT1
>> "$auth{'AUTHNAME'}|$auth{'AUTHHOST'}:$auth{'AUTHPASS'}\n";
>> +		close TXT1;
>>   		close TXT2;
> On a side note: It would probably have helped a bit to name the file
> handles better. Like AUTHFILE or so instead of numbering them.
>
>>   
>>   	}else{
> -Michael
  

On Sat, 2015-10-31 at 20:21 +0100, Alexander Marx wrote:
> > Hi,
> > 
> > I think there is a problem with this patch.
> > 
> > On Sat, 2015-10-31 at 07:34 +0100, Alexander Marx wrote:
> > > auth.conf was always written, even if no username/password
> > > provided.
> > > In this case only the ip or Hostname of the mailserver was
> > > written
> > > into
> > > auth.conf. Now the file is only filled if username/password are
> > > filled.
> > > 
> > > Signed-off-by: Alexander Marx <alexander.marx@ipfire.org>
> > > ---
> > >   html/cgi-bin/mail.cgi | 11 +++++++----
> > >   1 file changed, 7 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/html/cgi-bin/mail.cgi b/html/cgi-bin/mail.cgi
> > > index be663a6..f5be104 100755
> > > --- a/html/cgi-bin/mail.cgi
> > > +++ b/html/cgi-bin/mail.cgi
> > > @@ -110,9 +110,12 @@ if ($cgiparams{'ACTION'} eq
> > > "$Lang::tr{'save'}"){ #SaveButton on configsite
> > >   		$mail{'SENDER'} 		=
> > > $cgiparams{'txt_mailsender'};
> > >   		$mail{'RECIPIENT'}		=
> > > $cgiparams{'txt_recipient'};
> > >   
> > > -		$auth{'AUTHNAME'}		=
> > > $cgiparams{'txt_mailuser'};
> > > -		$auth{'AUTHPASS'}		=
> > > $cgiparams{'txt_mailpass'};
> > > -		$auth{'AUTHHOST'}		=
> > > $cgiparams{'txt_mailserver'};
> > > +		if ($cgiparams{'txt_mailuser'} &&
> > > $cgiparams{'txt_mailpass'}) {
> > > +			$auth{'AUTHNAME'}		=
> > > $cgiparams{'txt_mailuser'};
> > > +			$auth{'AUTHPASS'}		=
> > > $cgiparams{'txt_mailpass'};
> > > +			$auth{'AUTHHOST'}		=
> > > $cgiparams{'txt_mailserver'};
> > > +			print TXT1
> > > "$auth{'AUTHNAME'}|$auth{'AUTHHOST'}:$auth{'AUTHPASS'}\n";
> > > +		}
> > My email client made a right mess out of this code above, but maybe
> > you
> > check with the original one... So if the user wants to clear the
> > username and password and then hits save I guess these changes
> > won't be
> > saved. They should though.
> > 
> > Can you confirm this? I think the solution should keep the first
> > three
> > lines that are now in the if-clause before that and also clear
> > auth.conf when the configuration was cleared.
> 
> Unfortunately not. With the flow of the code you should see, that 
> auth.conf is always opened for writing (overwrite mode). If a
> username 
> and password is set, the file is written due to the if clause.
> Otherwise the file is just closed wich leads to an empty file.

OK.

> In my tests, the system did exactly that. I tested a config without 
> user/pass, and clicked "save". Then auth.conf is empty. Next step was
> to 
> fill username/password and click "save" again. Final step: remove 
> username/pass and again save file -> empty auth.conf.

I don't know what perl is then actually checking when you have a
statement like this:

  if ($cgiparams{'ABC'}) {
     ...
  }

Does it check for a non-empty string? Obviously not. Does it check if
that value is defined? Apparently not.

I would like to see more readable code if that is possible though. This
is nothing that is connected to this patch alone, it seems to be a
general problem with our perl code here. It works, but it is not clear
to *everyone* why.

> 
> Maybe someone else is able to test this?! Timo seems to be a good 
> tester, he found the bug ;-)
> 
> But i will double check and give feedback if i was wrong, but this
> can 
> take 1-2 days
> > 
> > >   
> > >   		$dma{'SMARTHOST'}		=
> > > $cgiparams{'txt_mailserver'};
> > >   		$dma{'PORT'}			=
> > > $cgiparams{'txt_mailport'};
> > > @@ -129,7 +132,7 @@ if ($cgiparams{'ACTION'} eq
> > > "$Lang::tr{'save'}"){
> > > #SaveButton on configsite
> > >   			print TXT "$k $v\n";
> > >   		}
> > >   		close TXT;
> > > -		print TXT1
> > > "$auth{'AUTHNAME'}|$auth{'AUTHHOST'}:$auth{'AUTHPASS'}\n";
> > > +		close TXT1;
> > >   		close TXT2;
> > On a side note: It would probably have helped a bit to name the
> > file
> > handles better. Like AUTHFILE or so instead of numbering them.
> > 
> > >   
> > >   	}else{
> > -Michael
> 

-Michael
  

> On Sat, 2015-10-31 at 20:21 +0100, Alexander Marx wrote:
>>> Hi,
>>>
>>> I think there is a problem with this patch.
>>>
>>> On Sat, 2015-10-31 at 07:34 +0100, Alexander Marx wrote:
>>>> auth.conf was always written, even if no username/password
>>>> provided.
>>>> In this case only the ip or Hostname of the mailserver was
>>>> written
>>>> into
>>>> auth.conf. Now the file is only filled if username/password are
>>>> filled.
>>>>
>>>> Signed-off-by: Alexander Marx <alexander.marx@ipfire.org>
>>>> ---
>>>>    html/cgi-bin/mail.cgi | 11 +++++++----
>>>>    1 file changed, 7 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/html/cgi-bin/mail.cgi b/html/cgi-bin/mail.cgi
>>>> index be663a6..f5be104 100755
>>>> --- a/html/cgi-bin/mail.cgi
>>>> +++ b/html/cgi-bin/mail.cgi
>>>> @@ -110,9 +110,12 @@ if ($cgiparams{'ACTION'} eq
>>>> "$Lang::tr{'save'}"){ #SaveButton on configsite
>>>>    		$mail{'SENDER'} 		=
>>>> $cgiparams{'txt_mailsender'};
>>>>    		$mail{'RECIPIENT'}		=
>>>> $cgiparams{'txt_recipient'};
>>>>    
>>>> -		$auth{'AUTHNAME'}		=
>>>> $cgiparams{'txt_mailuser'};
>>>> -		$auth{'AUTHPASS'}		=
>>>> $cgiparams{'txt_mailpass'};
>>>> -		$auth{'AUTHHOST'}		=
>>>> $cgiparams{'txt_mailserver'};
>>>> +		if ($cgiparams{'txt_mailuser'} &&
>>>> $cgiparams{'txt_mailpass'}) {
>>>> +			$auth{'AUTHNAME'}		=
>>>> $cgiparams{'txt_mailuser'};
>>>> +			$auth{'AUTHPASS'}		=
>>>> $cgiparams{'txt_mailpass'};
>>>> +			$auth{'AUTHHOST'}		=
>>>> $cgiparams{'txt_mailserver'};
>>>> +			print TXT1
>>>> "$auth{'AUTHNAME'}|$auth{'AUTHHOST'}:$auth{'AUTHPASS'}\n";
>>>> +		}
>>> My email client made a right mess out of this code above, but maybe
>>> you
>>> check with the original one... So if the user wants to clear the
>>> username and password and then hits save I guess these changes
>>> won't be
>>> saved. They should though.
>>>
>>> Can you confirm this? I think the solution should keep the first
>>> three
>>> lines that are now in the if-clause before that and also clear
>>> auth.conf when the configuration was cleared.
>> Unfortunately not. With the flow of the code you should see, that
>> auth.conf is always opened for writing (overwrite mode). If a
>> username
>> and password is set, the file is written due to the if clause.
>> Otherwise the file is just closed wich leads to an empty file.
> OK.
>
>> In my tests, the system did exactly that. I tested a config without
>> user/pass, and clicked "save". Then auth.conf is empty. Next step was
>> to
>> fill username/password and click "save" again. Final step: remove
>> username/pass and again save file -> empty auth.conf.
> I don't know what perl is then actually checking when you have a
> statement like this:
>
>    if ($cgiparams{'ABC'}) {
>       ...
>    }
>
> Does it check for a non-empty string? Obviously not. Does it check if
> that value is defined? Apparently not.
>
> I would like to see more readable code if that is possible though. This
> is nothing that is connected to this patch alone, it seems to be a
> general problem with our perl code here. It works, but it is not clear
> to *everyone* why.
OK i agree with this. Will try to be more exact in future.
 From http://www.perlmonks.org/?node_id=598879 you will see, that there 
are several ways of perl ;-) to check if a variable contains something 
useful. Here is a statement:

Perl tries to make things as easy as possible for you and this is a good 
example.

In most cases, what you actually want is as simple as:

if ($value) { # $value contains something useful } else { # $value is 
"empty" }
[download] 
<http://www.perlmonks.org/?abspart=1;displaytype=displaycode;node_id=598956;part=1>

(Note that the logic is reversed from your original example)

What we're doing here is checking the "truth" of $value. $value will be 
"false" if it contains any of the following values:

  * "undef" (i.e. if it hasn't been given a value)
  * The number 0
  * The string "0"
  * The empty string ("")

Any other value in $value will evaluate to "true".

This is all you need in most cases. There are a couple of "corner cases" 
that you sometimes have to consider:

  * Sometimes 0 is an acceptable value. You account for that by using if
    ($value or $value == 0).
  * Sonetimes you might want to test that $value contains nothing but
    whitespace characters. The easiest way to do that is by checking for
    the presence of non-whitespace characters with if ($value =~ /\S/).


>> Maybe someone else is able to test this?! Timo seems to be a good
>> tester, he found the bug ;-)
>>
>> But i will double check and give feedback if i was wrong, but this
>> can
>> take 1-2 days
>>>>    
>>>>    		$dma{'SMARTHOST'}		=
>>>> $cgiparams{'txt_mailserver'};
>>>>    		$dma{'PORT'}			=
>>>> $cgiparams{'txt_mailport'};
>>>> @@ -129,7 +132,7 @@ if ($cgiparams{'ACTION'} eq
>>>> "$Lang::tr{'save'}"){
>>>> #SaveButton on configsite
>>>>    			print TXT "$k $v\n";
>>>>    		}
>>>>    		close TXT;
>>>> -		print TXT1
>>>> "$auth{'AUTHNAME'}|$auth{'AUTHHOST'}:$auth{'AUTHPASS'}\n";
>>>> +		close TXT1;
>>>>    		close TXT2;
>>> On a side note: It would probably have helped a bit to name the
>>> file
>>> handles better. Like AUTHFILE or so instead of numbering them.
>>>
>>>>    
>>>>    	}else{
>>> -Michael
> -Michael
  

Hi,

sorry for the late answer...

Atleast for me this patch works perfectly.

Reviewed-by: Timo Eissler <timo.eissler@ipfire.org>

Thank you Alex!

Am 01.11.2015 um 06:50 schrieb Alexander Marx:
>
>> On Sat, 2015-10-31 at 20:21 +0100, Alexander Marx wrote:
>>>> Hi,
>>>>
>>>> I think there is a problem with this patch.
>>>>
>>>> On Sat, 2015-10-31 at 07:34 +0100, Alexander Marx wrote:
>>>>> auth.conf was always written, even if no username/password
>>>>> provided.
>>>>> In this case only the ip or Hostname of the mailserver was
>>>>> written
>>>>> into
>>>>> auth.conf. Now the file is only filled if username/password are
>>>>> filled.
>>>>>
>>>>> Signed-off-by: Alexander Marx <alexander.marx@ipfire.org>
>>>>> ---
>>>>>   html/cgi-bin/mail.cgi | 11 +++++++----
>>>>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/html/cgi-bin/mail.cgi b/html/cgi-bin/mail.cgi
>>>>> index be663a6..f5be104 100755
>>>>> --- a/html/cgi-bin/mail.cgi
>>>>> +++ b/html/cgi-bin/mail.cgi
>>>>> @@ -110,9 +110,12 @@ if ($cgiparams{'ACTION'} eq
>>>>> "$Lang::tr{'save'}"){ #SaveButton on configsite
>>>>>   		$mail{'SENDER'} 		=
>>>>> $cgiparams{'txt_mailsender'};
>>>>>   		$mail{'RECIPIENT'}		=
>>>>> $cgiparams{'txt_recipient'};
>>>>>   
>>>>> -		$auth{'AUTHNAME'}		=
>>>>> $cgiparams{'txt_mailuser'};
>>>>> -		$auth{'AUTHPASS'}		=
>>>>> $cgiparams{'txt_mailpass'};
>>>>> -		$auth{'AUTHHOST'}		=
>>>>> $cgiparams{'txt_mailserver'};
>>>>> +		if ($cgiparams{'txt_mailuser'} &&
>>>>> $cgiparams{'txt_mailpass'}) {
>>>>> +			$auth{'AUTHNAME'}		=
>>>>> $cgiparams{'txt_mailuser'};
>>>>> +			$auth{'AUTHPASS'}		=
>>>>> $cgiparams{'txt_mailpass'};
>>>>> +			$auth{'AUTHHOST'}		=
>>>>> $cgiparams{'txt_mailserver'};
>>>>> +			print TXT1
>>>>> "$auth{'AUTHNAME'}|$auth{'AUTHHOST'}:$auth{'AUTHPASS'}\n";
>>>>> +		}
>>>> My email client made a right mess out of this code above, but maybe
>>>> you
>>>> check with the original one... So if the user wants to clear the
>>>> username and password and then hits save I guess these changes
>>>> won't be
>>>> saved. They should though.
>>>>
>>>> Can you confirm this? I think the solution should keep the first
>>>> three
>>>> lines that are now in the if-clause before that and also clear
>>>> auth.conf when the configuration was cleared.
>>> Unfortunately not. With the flow of the code you should see, that 
>>> auth.conf is always opened for writing (overwrite mode). If a
>>> username 
>>> and password is set, the file is written due to the if clause.
>>> Otherwise the file is just closed wich leads to an empty file.
>> OK.
>>
>>> In my tests, the system did exactly that. I tested a config without 
>>> user/pass, and clicked "save". Then auth.conf is empty. Next step was
>>> to 
>>> fill username/password and click "save" again. Final step: remove 
>>> username/pass and again save file -> empty auth.conf.
>> I don't know what perl is then actually checking when you have a
>> statement like this:
>>
>>   if ($cgiparams{'ABC'}) {
>>      ...
>>   }
>>
>> Does it check for a non-empty string? Obviously not. Does it check if
>> that value is defined? Apparently not.
>>
>> I would like to see more readable code if that is possible though. This
>> is nothing that is connected to this patch alone, it seems to be a
>> general problem with our perl code here. It works, but it is not clear
>> to *everyone* why.
> OK i agree with this. Will try to be more exact in future.
> From http://www.perlmonks.org/?node_id=598879 you will see, that there
> are several ways of perl ;-) to check if a variable contains something
> useful. Here is a statement:
>
> Perl tries to make things as easy as possible for you and this is a
> good example.
>
> In most cases, what you actually want is as simple as:
>
> if ($value) { # $value contains something useful } else { # $value is
> "empty" }
> [download]
> <http://www.perlmonks.org/?abspart=1;displaytype=displaycode;node_id=598956;part=1>
>
> (Note that the logic is reversed from your original example)
>
> What we're doing here is checking the "truth" of $value. $value will
> be "false" if it contains any of the following values:
>
>   * "undef" (i.e. if it hasn't been given a value)
>   * The number 0
>   * The string "0"
>   * The empty string ("")
>
> Any other value in $value will evaluate to "true".
>
> This is all you need in most cases. There are a couple of "corner
> cases" that you sometimes have to consider:
>
>   * Sometimes 0 is an acceptable value. You account for that by using
>     if ($value or $value == 0).
>   * Sonetimes you might want to test that $value contains nothing but
>     whitespace characters. The easiest way to do that is by checking
>     for the presence of non-whitespace characters with if ($value =~
>     /\S/).
>
>
>>> Maybe someone else is able to test this?! Timo seems to be a good 
>>> tester, he found the bug ;-)
>>>
>>> But i will double check and give feedback if i was wrong, but this
>>> can 
>>> take 1-2 days
>>>>>   
>>>>>   		$dma{'SMARTHOST'}		=
>>>>> $cgiparams{'txt_mailserver'};
>>>>>   		$dma{'PORT'}			=
>>>>> $cgiparams{'txt_mailport'};
>>>>> @@ -129,7 +132,7 @@ if ($cgiparams{'ACTION'} eq
>>>>> "$Lang::tr{'save'}"){
>>>>> #SaveButton on configsite
>>>>>   			print TXT "$k $v\n";
>>>>>   		}
>>>>>   		close TXT;
>>>>> -		print TXT1
>>>>> "$auth{'AUTHNAME'}|$auth{'AUTHHOST'}:$auth{'AUTHPASS'}\n";
>>>>> +		close TXT1;
>>>>>   		close TXT2;
>>>> On a side note: It would probably have helped a bit to name the
>>>> file
>>>> handles better. Like AUTHFILE or so instead of numbering them.
>>>>
>>>>>   
>>>>>   	}else{
>>>> -Michael
>> -Michael
>
  

On Sun, 2015-11-01 at 06:50 +0100, Alexander Marx wrote:
> 
> > On Sat, 2015-10-31 at 20:21 +0100, Alexander Marx wrote:
> > > > Hi,
> > > > 
> > > > I think there is a problem with this patch.
> > > > 
> > > > On Sat, 2015-10-31 at 07:34 +0100, Alexander Marx wrote:
> > > > > auth.conf was always written, even if no username/password
> > > > > provided.
> > > > > In this case only the ip or Hostname of the mailserver was
> > > > > written
> > > > > into
> > > > > auth.conf. Now the file is only filled if username/password
> > > > > are
> > > > > filled.
> > > > > 
> > > > > Signed-off-by: Alexander Marx <alexander.marx@ipfire.org>
> > > > > ---
> > > > >   html/cgi-bin/mail.cgi | 11 +++++++----
> > > > >   1 file changed, 7 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/html/cgi-bin/mail.cgi b/html/cgi-bin/mail.cgi
> > > > > index be663a6..f5be104 100755
> > > > > --- a/html/cgi-bin/mail.cgi
> > > > > +++ b/html/cgi-bin/mail.cgi
> > > > > @@ -110,9 +110,12 @@ if ($cgiparams{'ACTION'} eq
> > > > > "$Lang::tr{'save'}"){ #SaveButton on configsite
> > > > >   		$mail{'SENDER'} 		=
> > > > > $cgiparams{'txt_mailsender'};
> > > > >   		$mail{'RECIPIENT'}		=
> > > > > $cgiparams{'txt_recipient'};
> > > > >   
> > > > > -		$auth{'AUTHNAME'}		=
> > > > > $cgiparams{'txt_mailuser'};
> > > > > -		$auth{'AUTHPASS'}		=
> > > > > $cgiparams{'txt_mailpass'};
> > > > > -		$auth{'AUTHHOST'}		=
> > > > > $cgiparams{'txt_mailserver'};
> > > > > +		if ($cgiparams{'txt_mailuser'} &&
> > > > > $cgiparams{'txt_mailpass'}) {
> > > > > +			$auth{'AUTHNAME'}		=
> > > > > $cgiparams{'txt_mailuser'};
> > > > > +			$auth{'AUTHPASS'}		=
> > > > > $cgiparams{'txt_mailpass'};
> > > > > +			$auth{'AUTHHOST'}		=
> > > > > $cgiparams{'txt_mailserver'};
> > > > > +			print TXT1
> > > > > "$auth{'AUTHNAME'}|$auth{'AUTHHOST'}:$auth{'AUTHPASS'}\n";
> > > > > +		}
> > > > My email client made a right mess out of this code above, but
> > > > maybe
> > > > you
> > > > check with the original one... So if the user wants to clear
> > > > the
> > > > username and password and then hits save I guess these changes
> > > > won't be
> > > > saved. They should though.
> > > > 
> > > > Can you confirm this? I think the solution should keep the
> > > > first
> > > > three
> > > > lines that are now in the if-clause before that and also clear
> > > > auth.conf when the configuration was cleared.
> > > Unfortunately not. With the flow of the code you should see, that
> > > auth.conf is always opened for writing (overwrite mode). If a
> > > username 
> > > and password is set, the file is written due to the if clause.
> > > Otherwise the file is just closed wich leads to an empty file.
> > OK.
> > 
> > > In my tests, the system did exactly that. I tested a config
> > > without 
> > > user/pass, and clicked "save". Then auth.conf is empty. Next step
> > > was
> > > to 
> > > fill username/password and click "save" again. Final step: remove
> > > username/pass and again save file -> empty auth.conf.
> > I don't know what perl is then actually checking when you have a
> > statement like this:
> > 
> >   if ($cgiparams{'ABC'}) {
> >      ...
> >   }
> > 
> > Does it check for a non-empty string? Obviously not. Does it check
> > if
> > that value is defined? Apparently not.
> > 
> > I would like to see more readable code if that is possible though.
> > This
> > is nothing that is connected to this patch alone, it seems to be a
> > general problem with our perl code here. It works, but it is not
> > clear
> > to *everyone* why.
> OK i agree with this. Will try to be more exact in future.

This is not necessarily an issue with your patch. It is also a bit
related to perl - or that perl-programmers don't *actually* know what
their code does.

> From http://www.perlmonks.org/?node_id=598879 you will see, that
> there are several ways of perl ;-) to check if a variable contains
> something useful. Here is a statement:
> 
> Perl tries to make things as easy as possible for you and this is a
> good example.
> In most cases, what you actually want is as simple as:
> if ($value) {
>   # $value contains something useful
> } else {
>   # $value is "empty"
> }
> [download]
> (Note that the logic is reversed from your original example)
> What we're doing here is checking the "truth" of $value. $value will
> be "false" if it contains any of the following values:
> "undef" (i.e. if it hasn't been given a value)
> The number 0
> The string "0"

That at least should be treated as a valid string. It is unlikely that
this is a username or password, but still this is something that needs
to be handled.

> The empty string ("")

If that was true, you won't be able to clear the form as I mentioned
before.

So I guess this must be changed then.

> Any other value in $value will evaluate to "true".
> This is all you need in most cases. There are a couple of "corner
cases" that you sometimes have to consider:
> Sometimes 0 is an acceptable value. You account for that by using if
> ($value or $value == 0).
> Sonetimes you might want to test that $value contains nothing but
> whitespace characters. The easiest way to do that is by checking for
> the presence of non-whitespace characters with if ($value =~ /\S/).
> 
> > > Maybe someone else is able to test this?! Timo seems to be a good
> > > tester, he found the bug ;-)
> > > 
> > > But i will double check and give feedback if i was wrong, but
> > > this
> > > can 
> > > take 1-2 days
> > > > >   
> > > > >   		$dma{'SMARTHOST'}		=
> > > > > $cgiparams{'txt_mailserver'};
> > > > >   		$dma{'PORT'}			=
> > > > > $cgiparams{'txt_mailport'};
> > > > > @@ -129,7 +132,7 @@ if ($cgiparams{'ACTION'} eq
> > > > > "$Lang::tr{'save'}"){
> > > > > #SaveButton on configsite
> > > > >   			print TXT "$k $v\n";
> > > > >   		}
> > > > >   		close TXT;
> > > > > -		print TXT1
> > > > > "$auth{'AUTHNAME'}|$auth{'AUTHHOST'}:$auth{'AUTHPASS'}\n";
> > > > > +		close TXT1;
> > > > >   		close TXT2;
> > > > On a side note: It would probably have helped a bit to name the
> > > > file
> > > > handles better. Like AUTHFILE or so instead of numbering them.
> > > > 
> > > > >   
> > > > >   	}else{
> > > > -Michael
> > -Michael

-Michael
  

On Sun, 2015-11-01 at 11:10 +0100, Timo Eissler wrote:
> Hi,
> 
> sorry for the late answer...
> 
> Atleast for me this patch works perfectly.


> Reviewed-by: Timo Eissler <timo.eissler@ipfire.org>

You should use the Tested-by: tag than instead of Reviewed-by: because
you didn't review the code.

> 
> Thank you Alex!

-Michael

> 
> Am 01.11.2015 um 06:50 schrieb Alexander Marx:
> >  
> > > On Sat, 2015-10-31 at 20:21 +0100, Alexander Marx wrote:
> > > > > Hi,
> > > > > 
> > > > > I think there is a problem with this patch.
> > > > > 
> > > > > On Sat, 2015-10-31 at 07:34 +0100, Alexander Marx wrote:
> > > > > > auth.conf was always written, even if no username/password
> > > > > > provided.
> > > > > > In this case only the ip or Hostname of the mailserver was
> > > > > > written
> > > > > > into
> > > > > > auth.conf. Now the file is only filled if username/password
> > > > > > are
> > > > > > filled.
> > > > > > 
> > > > > > Signed-off-by: Alexander Marx <alexander.marx@ipfire.org>
> > > > > > ---
> > > > > >   html/cgi-bin/mail.cgi | 11 +++++++----
> > > > > >   1 file changed, 7 insertions(+), 4 deletions(-)
> > > > > > 
> > > > > > diff --git a/html/cgi-bin/mail.cgi b/html/cgi-bin/mail.cgi
> > > > > > index be663a6..f5be104 100755
> > > > > > --- a/html/cgi-bin/mail.cgi
> > > > > > +++ b/html/cgi-bin/mail.cgi
> > > > > > @@ -110,9 +110,12 @@ if ($cgiparams{'ACTION'} eq
> > > > > > "$Lang::tr{'save'}"){ #SaveButton on configsite
> > > > > >   		$mail{'SENDER'} 		=
> > > > > > $cgiparams{'txt_mailsender'};
> > > > > >   		$mail{'RECIPIENT'}		=
> > > > > > $cgiparams{'txt_recipient'};
> > > > > >   
> > > > > > -		$auth{'AUTHNAME'}		=
> > > > > > $cgiparams{'txt_mailuser'};
> > > > > > -		$auth{'AUTHPASS'}		=
> > > > > > $cgiparams{'txt_mailpass'};
> > > > > > -		$auth{'AUTHHOST'}		=
> > > > > > $cgiparams{'txt_mailserver'};
> > > > > > +		if ($cgiparams{'txt_mailuser'} &&
> > > > > > $cgiparams{'txt_mailpass'}) {
> > > > > > +			$auth{'AUTHNAME'}		=
> > > > > > $cgiparams{'txt_mailuser'};
> > > > > > +			$auth{'AUTHPASS'}		=
> > > > > > $cgiparams{'txt_mailpass'};
> > > > > > +			$auth{'AUTHHOST'}		=
> > > > > > $cgiparams{'txt_mailserver'};
> > > > > > +			print TXT1
> > > > > > "$auth{'AUTHNAME'}|$auth{'AUTHHOST'}:$auth{'AUTHPASS'}\n";
> > > > > > +		}
> > > > > My email client made a right mess out of this code above, but
> > > > > maybe
> > > > > you
> > > > > check with the original one... So if the user wants to clear
> > > > > the
> > > > > username and password and then hits save I guess these
> > > > > changes
> > > > > won't be
> > > > > saved. They should though.
> > > > > 
> > > > > Can you confirm this? I think the solution should keep the
> > > > > first
> > > > > three
> > > > > lines that are now in the if-clause before that and also
> > > > > clear
> > > > > auth.conf when the configuration was cleared.
> > > > Unfortunately not. With the flow of the code you should see,
> > > > that 
> > > > auth.conf is always opened for writing (overwrite mode). If a
> > > > username 
> > > > and password is set, the file is written due to the if clause.
> > > > Otherwise the file is just closed wich leads to an empty file.
> > > OK.
> > > 
> > > > In my tests, the system did exactly that. I tested a config
> > > > without 
> > > > user/pass, and clicked "save". Then auth.conf is empty. Next
> > > > step was
> > > > to 
> > > > fill username/password and click "save" again. Final step:
> > > > remove 
> > > > username/pass and again save file -> empty auth.conf.
> > > I don't know what perl is then actually checking when you have a
> > > statement like this:
> > > 
> > >   if ($cgiparams{'ABC'}) {
> > >      ...
> > >   }
> > > 
> > > Does it check for a non-empty string? Obviously not. Does it
> > > check if
> > > that value is defined? Apparently not.
> > > 
> > > I would like to see more readable code if that is possible
> > > though. This
> > > is nothing that is connected to this patch alone, it seems to be
> > > a
> > > general problem with our perl code here. It works, but it is not
> > > clear
> > > to *everyone* why.
> > OK i agree with this. Will try to be more exact in future.
> > From http://www.perlmonks.org/?node_id=598879 you will see, that
> > there are several ways of perl ;-) to check if a variable contains
> > something useful. Here is a statement:
> > 
> > Perl tries to make things as easy as possible for you and this is a
> > good example.
> > In most cases, what you actually want is as simple as:
> > if ($value) {
> >   # $value contains something useful
> > } else {
> >   # $value is "empty"
> > }
> > [download]
> > (Note that the logic is reversed from your original example)
> > What we're doing here is checking the "truth" of $value. $value
> > will be "false" if it contains any of the following values:
> > "undef" (i.e. if it hasn't been given a value)
> > The number 0
> > The string "0"
> > The empty string ("")
> > Any other value in $value will evaluate to "true".
> > This is all you need in most cases. There are a couple of "corner
> > cases" that you sometimes have to consider:
> > Sometimes 0 is an acceptable value. You account for that by using
> > if ($value or $value == 0).
> > Sonetimes you might want to test that $value contains nothing but
> > whitespace characters. The easiest way to do that is by checking
> > for the presence of non-whitespace characters with if ($value =~
> > /\S/).
> > 
> > > > Maybe someone else is able to test this?! Timo seems to be a
> > > > good 
> > > > tester, he found the bug ;-)
> > > > 
> > > > But i will double check and give feedback if i was wrong, but
> > > > this
> > > > can 
> > > > take 1-2 days
> > > > > >   
> > > > > >   		$dma{'SMARTHOST'}		=
> > > > > > $cgiparams{'txt_mailserver'};
> > > > > >   		$dma{'PORT'}			=
> > > > > > $cgiparams{'txt_mailport'};
> > > > > > @@ -129,7 +132,7 @@ if ($cgiparams{'ACTION'} eq
> > > > > > "$Lang::tr{'save'}"){
> > > > > > #SaveButton on configsite
> > > > > >   			print TXT "$k $v\n";
> > > > > >   		}
> > > > > >   		close TXT;
> > > > > > -		print TXT1
> > > > > > "$auth{'AUTHNAME'}|$auth{'AUTHHOST'}:$auth{'AUTHPASS'}\n";
> > > > > > +		close TXT1;
> > > > > >   		close TXT2;
> > > > > On a side note: It would probably have helped a bit to name
> > > > > the
> > > > > file
> > > > > handles better. Like AUTHFILE or so instead of numbering
> > > > > them.
> > > > > 
> > > > > >   
> > > > > >   	}else{
> > > > > -Michael
> > > -Michael
> -- 
> Timo Eissler
> Senior Project Engineer / Consultant
> 
> Am Zuckerberg 54
> D-71640 Ludwigsburg
> 
> Tel.: +49 7141 4094003
> Mobil.: +49 151 20650311
> Email: timo@teissler.de
  

So i'm gonna add the

Tested-by: Timo Eissler <timo.eissler@ipfire.org>

Tag also, as i'm reviewed the code and tested it.

Am 01.11.2015 um 17:07 schrieb Michael Tremer:
> On Sun, 2015-11-01 at 11:10 +0100, Timo Eissler wrote:
>> Hi,
>>
>> sorry for the late answer...
>>
>> Atleast for me this patch works perfectly.
>
>> Reviewed-by: Timo Eissler <timo.eissler@ipfire.org>
> You should use the Tested-by: tag than instead of Reviewed-by: because
> you didn't review the code.
>
>> Thank you Alex!
> -Michael
>
>> Am 01.11.2015 um 06:50 schrieb Alexander Marx:
>>>  
>>>> On Sat, 2015-10-31 at 20:21 +0100, Alexander Marx wrote:
>>>>>> Hi,
>>>>>>
>>>>>> I think there is a problem with this patch.
>>>>>>
>>>>>> On Sat, 2015-10-31 at 07:34 +0100, Alexander Marx wrote:
>>>>>>> auth.conf was always written, even if no username/password
>>>>>>> provided.
>>>>>>> In this case only the ip or Hostname of the mailserver was
>>>>>>> written
>>>>>>> into
>>>>>>> auth.conf. Now the file is only filled if username/password
>>>>>>> are
>>>>>>> filled.
>>>>>>>
>>>>>>> Signed-off-by: Alexander Marx <alexander.marx@ipfire.org>
>>>>>>> ---
>>>>>>>   html/cgi-bin/mail.cgi | 11 +++++++----
>>>>>>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/html/cgi-bin/mail.cgi b/html/cgi-bin/mail.cgi
>>>>>>> index be663a6..f5be104 100755
>>>>>>> --- a/html/cgi-bin/mail.cgi
>>>>>>> +++ b/html/cgi-bin/mail.cgi
>>>>>>> @@ -110,9 +110,12 @@ if ($cgiparams{'ACTION'} eq
>>>>>>> "$Lang::tr{'save'}"){ #SaveButton on configsite
>>>>>>>   		$mail{'SENDER'} 		=
>>>>>>> $cgiparams{'txt_mailsender'};
>>>>>>>   		$mail{'RECIPIENT'}		=
>>>>>>> $cgiparams{'txt_recipient'};
>>>>>>>   
>>>>>>> -		$auth{'AUTHNAME'}		=
>>>>>>> $cgiparams{'txt_mailuser'};
>>>>>>> -		$auth{'AUTHPASS'}		=
>>>>>>> $cgiparams{'txt_mailpass'};
>>>>>>> -		$auth{'AUTHHOST'}		=
>>>>>>> $cgiparams{'txt_mailserver'};
>>>>>>> +		if ($cgiparams{'txt_mailuser'} &&
>>>>>>> $cgiparams{'txt_mailpass'}) {
>>>>>>> +			$auth{'AUTHNAME'}		=
>>>>>>> $cgiparams{'txt_mailuser'};
>>>>>>> +			$auth{'AUTHPASS'}		=
>>>>>>> $cgiparams{'txt_mailpass'};
>>>>>>> +			$auth{'AUTHHOST'}		=
>>>>>>> $cgiparams{'txt_mailserver'};
>>>>>>> +			print TXT1
>>>>>>> "$auth{'AUTHNAME'}|$auth{'AUTHHOST'}:$auth{'AUTHPASS'}\n";
>>>>>>> +		}
>>>>>> My email client made a right mess out of this code above, but
>>>>>> maybe
>>>>>> you
>>>>>> check with the original one... So if the user wants to clear
>>>>>> the
>>>>>> username and password and then hits save I guess these
>>>>>> changes
>>>>>> won't be
>>>>>> saved. They should though.
>>>>>>
>>>>>> Can you confirm this? I think the solution should keep the
>>>>>> first
>>>>>> three
>>>>>> lines that are now in the if-clause before that and also
>>>>>> clear
>>>>>> auth.conf when the configuration was cleared.
>>>>> Unfortunately not. With the flow of the code you should see,
>>>>> that 
>>>>> auth.conf is always opened for writing (overwrite mode). If a
>>>>> username 
>>>>> and password is set, the file is written due to the if clause.
>>>>> Otherwise the file is just closed wich leads to an empty file.
>>>> OK.
>>>>
>>>>> In my tests, the system did exactly that. I tested a config
>>>>> without 
>>>>> user/pass, and clicked "save". Then auth.conf is empty. Next
>>>>> step was
>>>>> to 
>>>>> fill username/password and click "save" again. Final step:
>>>>> remove 
>>>>> username/pass and again save file -> empty auth.conf.
>>>> I don't know what perl is then actually checking when you have a
>>>> statement like this:
>>>>
>>>>   if ($cgiparams{'ABC'}) {
>>>>      ...
>>>>   }
>>>>
>>>> Does it check for a non-empty string? Obviously not. Does it
>>>> check if
>>>> that value is defined? Apparently not.
>>>>
>>>> I would like to see more readable code if that is possible
>>>> though. This
>>>> is nothing that is connected to this patch alone, it seems to be
>>>> a
>>>> general problem with our perl code here. It works, but it is not
>>>> clear
>>>> to *everyone* why.
>>> OK i agree with this. Will try to be more exact in future.
>>> From http://www.perlmonks.org/?node_id=598879 you will see, that
>>> there are several ways of perl ;-) to check if a variable contains
>>> something useful. Here is a statement:
>>>
>>> Perl tries to make things as easy as possible for you and this is a
>>> good example.
>>> In most cases, what you actually want is as simple as:
>>> if ($value) {
>>>   # $value contains something useful
>>> } else {
>>>   # $value is "empty"
>>> }
>>> [download]
>>> (Note that the logic is reversed from your original example)
>>> What we're doing here is checking the "truth" of $value. $value
>>> will be "false" if it contains any of the following values:
>>> "undef" (i.e. if it hasn't been given a value)
>>> The number 0
>>> The string "0"
>>> The empty string ("")
>>> Any other value in $value will evaluate to "true".
>>> This is all you need in most cases. There are a couple of "corner
>>> cases" that you sometimes have to consider:
>>> Sometimes 0 is an acceptable value. You account for that by using
>>> if ($value or $value == 0).
>>> Sonetimes you might want to test that $value contains nothing but
>>> whitespace characters. The easiest way to do that is by checking
>>> for the presence of non-whitespace characters with if ($value =~
>>> /\S/).
>>>
>>>>> Maybe someone else is able to test this?! Timo seems to be a
>>>>> good 
>>>>> tester, he found the bug ;-)
>>>>>
>>>>> But i will double check and give feedback if i was wrong, but
>>>>> this
>>>>> can 
>>>>> take 1-2 days
>>>>>>>   
>>>>>>>   		$dma{'SMARTHOST'}		=
>>>>>>> $cgiparams{'txt_mailserver'};
>>>>>>>   		$dma{'PORT'}			=
>>>>>>> $cgiparams{'txt_mailport'};
>>>>>>> @@ -129,7 +132,7 @@ if ($cgiparams{'ACTION'} eq
>>>>>>> "$Lang::tr{'save'}"){
>>>>>>> #SaveButton on configsite
>>>>>>>   			print TXT "$k $v\n";
>>>>>>>   		}
>>>>>>>   		close TXT;
>>>>>>> -		print TXT1
>>>>>>> "$auth{'AUTHNAME'}|$auth{'AUTHHOST'}:$auth{'AUTHPASS'}\n";
>>>>>>> +		close TXT1;
>>>>>>>   		close TXT2;
>>>>>> On a side note: It would probably have helped a bit to name
>>>>>> the
>>>>>> file
>>>>>> handles better. Like AUTHFILE or so instead of numbering
>>>>>> them.
>>>>>>
>>>>>>>   
>>>>>>>   	}else{
>>>>>> -Michael
>>>> -Michael
>> -- 
>> Timo Eissler
>> Senior Project Engineer / Consultant
>>
>> Am Zuckerberg 54
>> D-71640 Ludwigsburg
>>
>> Tel.: +49 7141 4094003
>> Mobil.: +49 151 20650311
>> Email: timo@teissler.de