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.

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