* 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
* 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