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" }

(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:

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:


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