* Re: [PATCH] BUG10965: only write auth.conf if username/password are set [not found] <5635E50D.2070307@teissler.de> @ 2015-11-01 16:07 ` Michael Tremer 2015-11-02 18:43 ` Timo Eissler 0 siblings, 1 reply; 3+ messages in thread From: Michael Tremer @ 2015-11-01 16:07 UTC (permalink / raw) To: development [-- Attachment #1: Type: text/plain, Size: 6675 bytes --] 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(a)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(a)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(a)teissler.de [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] BUG10965: only write auth.conf if username/password are set 2015-11-01 16:07 ` [PATCH] BUG10965: only write auth.conf if username/password are set Michael Tremer @ 2015-11-02 18:43 ` Timo Eissler 0 siblings, 0 replies; 3+ messages in thread From: Timo Eissler @ 2015-11-02 18:43 UTC (permalink / raw) To: development [-- Attachment #1: Type: text/plain, Size: 6635 bytes --] So i'm gonna add the Tested-by: Timo Eissler <timo.eissler(a)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(a)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(a)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(a)teissler.de -- Timo Eissler Senior Project Engineer / Consultant Am Zuckerberg 54 D-71640 Ludwigsburg Tel.: +49 7141 4094003 Mobil.: +49 151 20650311 Email: timo(a)teissler.de [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 3+ messages in thread
[parent not found: <5635A80F.30003@oab.de>]
* Re: [PATCH] BUG10965: only write auth.conf if username/password are set [not found] <5635A80F.30003@oab.de> @ 2015-11-01 16:07 ` Michael Tremer 0 siblings, 0 replies; 3+ messages in thread From: Michael Tremer @ 2015-11-01 16:07 UTC (permalink / raw) To: development [-- Attachment #1: Type: text/plain, Size: 6214 bytes --] 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(a)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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-11-02 18:43 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <5635E50D.2070307@teissler.de>
2015-11-01 16:07 ` [PATCH] BUG10965: only write auth.conf if username/password are set Michael Tremer
2015-11-02 18:43 ` Timo Eissler
[not found] <5635A80F.30003@oab.de>
2015-11-01 16:07 ` Michael Tremer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox