Bug 11077 - Correct silent warnings in C4/Auth.pm
Summary: Correct silent warnings in C4/Auth.pm
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low trivial (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Marcel de Rooy
URL:
Keywords:
: 9871 (view as bug list)
Depends on:
Blocks: 8817 18275
  Show dependency treegraph
 
Reported: 2013-10-17 22:02 UTC by Tomás Cohen Arazi
Modified: 2017-03-15 17:36 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Remove $pki_field variable warning (1.82 KB, patch)
2013-10-17 22:47 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Silence uninitialized value $pki_field warning (1.97 KB, patch)
2013-10-18 00:50 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Silence uninitialized value $pki_field warning (and another one) (1.97 KB, patch)
2013-10-20 22:07 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11077: Uninitialized value $pki_field warning (2.13 KB, patch)
2013-10-20 22:59 UTC, Petter Goksøyr Åsen
Details | Diff | Splinter Review
bug_11077: get rid of some warnings (3.27 KB, patch)
2013-11-01 08:43 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 11077 - Correct silent warnings in C4/Auth.pm (2.06 KB, patch)
2013-11-01 14:09 UTC, Mark Tompsett
Details | Diff | Splinter Review
[FAILED_QA] Bug 11077: get rid of some warnings (3.28 KB, patch)
2013-11-08 07:49 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 11077 - Correct silent warnings in C4/Auth.pm (1.43 KB, patch)
2013-11-08 07:55 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Sample Test Program to confirm behaviour of busted vs current patch vs suggestion (3.34 KB, text/plain)
2013-11-08 15:47 UTC, Mark Tompsett
Details
Bug 11077 - Correct silent warnings in C4/Auth.pm (3.16 KB, patch)
2013-11-08 17:36 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 11077 - Correct silent warnings in C4/Auth.pm (3.67 KB, patch)
2013-11-11 01:53 UTC, Mark Tompsett
Details | Diff | Splinter Review
[PASSED_QA] Bug 11077: Uninitialized value $pki_field warning (2.23 KB, patch)
2013-11-27 17:01 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[FAILED_QA] bug_11077: get rid of some warnings (3.33 KB, patch)
2013-11-27 17:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11077 - Correct silent warnings in C4/Auth.pm (3.71 KB, patch)
2013-11-27 17:28 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11077 - Correct silent warnings in C4/Auth.pm (4.14 KB, patch)
2013-11-28 14:34 UTC, Mark Tompsett
Details | Diff | Splinter Review
Confirmation of what I said regarding undef ne 'None' (902 bytes, application/x-perl)
2013-11-28 14:37 UTC, Mark Tompsett
Details
Bug 11077 - Fix indents to trace code more easily (3.13 KB, patch)
2013-11-28 18:23 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 11077 - Correct silent warnings in C4/Auth.pm (4.10 KB, patch)
2013-11-28 18:45 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 11077 - Correct silent warnings in C4/Auth.pm (4.92 KB, patch)
2013-11-29 02:45 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 11077 - Correct silent warnings in C4/Auth.pm (4.96 KB, patch)
2013-12-02 21:15 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[SIGNED OFF] Bug 11077 - Correct silent warnings in C4/Auth.pm (5.03 KB, patch)
2013-12-03 14:17 UTC, Mark Tompsett
Details | Diff | Splinter Review
[PASSED_QA] Bug 11077 - Correct silent warnings in C4/Auth.pm (5.11 KB, patch)
2013-12-04 12:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2013-10-17 22:02:15 UTC

    
Comment 1 Tomás Cohen Arazi 2013-10-17 22:47:45 UTC Comment hidden (obsolete)
Comment 2 Mark Tompsett 2013-10-18 00:25:48 UTC
Actually, I was thinking about this annoying problem too, and that's not how I was thinking of solving it.

Around line 774 the $pki_field is defined and set as part of an if statement, but because it can be short-circuited, the variable is sometimes not defined.

I would suggest that the solution is to put:
my $pki_field = C4::Context->preference('AllowPKIAuth') || q{};
before the if statement.
AND
change the if statement to be
... || $pki_field ne 'None' ...

This way $pki_field is defined, and string comparisons shouldn't generate silent errors.

I'm switching this to in discussion.
Comment 3 Tomás Cohen Arazi 2013-10-18 00:50:16 UTC Comment hidden (obsolete)
Comment 4 Mark Tompsett 2013-10-18 02:34:13 UTC
Changed name to reflect the fact that this bug corrects two silent warnings:
[Thu Oct 17 22:32:14 2013] [error] [client 192.168.45.23] [Thu Oct 17 22:32:13 2013] mainpage.pl: Use of uninitialized value in string ne at /home/mtompset/kohaclone/C4/Auth.pm line 685., referer: http://demo.library-admin.kohasystem.ca/
[Thu Oct 17 22:32:14 2013] [error] [client 192.168.45.23] [Thu Oct 17 22:32:14 2013] mainpage.pl: Use of uninitialized value $pki_field in string eq at /home/mtompset/kohaclone/C4/Auth.pm line 795., referer: http://demo.library-admin.kohasystem.ca/
[Thu Oct 17 22:32:14 2013] [error] [client 192.168.45.23] [Thu Oct 17 22:32:14 2013] mainpage.pl: Use of uninitialized value $pki_field in string eq at /home/mtompset/kohaclone/C4/Auth.pm line 795., referer: http://demo.library-admin.kohasystem.ca/

Reproduce:
Go to staff client url
Log in
Check staff client error log file.
Comment 5 Mark Tompsett 2013-10-18 03:26:38 UTC
Test case to check, which makes a difference whether '' or 'None' is used.

Apply patch.
Make sure you aren't doing cas authentication (not normally configured)
DELETE AllowPKIAuth system preference. -- an upgrade gone bad somehow.
Go to staff client.

DO NOT FILL IN ANYTHING.
Click Login
This triggers the modified if statement ('' ne 'None') and the inner else.
This triggers a warning about $retuserid being uninitialized.

Change the line to be 'None'
DO NOT FILL IN ANYTHING.
Click Login
This triggers nothing as far as I can tell, because there is nothing else at the same level.

Hopefully I didn't get them backwards. :)
Comment 6 Tomás Cohen Arazi 2013-10-20 22:07:21 UTC Comment hidden (obsolete)
Comment 7 Petter Goksøyr Åsen 2013-10-20 22:59:08 UTC Comment hidden (obsolete)
Comment 8 Mark Tompsett 2013-10-21 00:59:57 UTC
Comment on attachment 22120 [details] [review]
Silence uninitialized value $pki_field warning (and another one)

Looks like the other signed off patch obsoletes this.
Comment 9 Jonathan Druart 2013-10-24 12:22:05 UTC
Comment on attachment 22125 [details] [review]
Bug 11077: Uninitialized value $pki_field warning

Review of attachment 22125 [details] [review]:
-----------------------------------------------------------------

::: C4/Auth.pm
@@ +754,5 @@
>              -value    => $session->id,
>              -HttpOnly => 1
>          );
> +        $userid = $query->param('userid');
> +        my $pki_field = C4::Context->preference('AllowPKIAuth') // 'None';

I am not sure it is a good idea to suppress the warning caused by an undefined syspref. This syspref should exist in the DB, else something wrong happened when updating the DB. Logs must contain this alert.
Comment 10 Mark Tompsett 2013-10-24 12:34:09 UTC
(In reply to Jonathan Druart from comment #9)
> I am not sure it is a good idea to suppress the warning caused by an
> undefined syspref. This syspref should exist in the DB, else something wrong
> happened when updating the DB. Logs must contain this alert.

If something went wrong updating the DB, should that not be in the logs already? Suppressing while running in a bad state avoids flooding the logs more. Flooded logs don't get read. TL;DR is a new short form I've come to learn: Too Long; Didn't Read. This means the the less flooded log has a better chance of having someone catch the error and correct it. Just my thoughts.
Comment 11 Jonathan Druart 2013-10-24 12:45:15 UTC
(In reply to M. Tompsett from comment #10)
> If something went wrong updating the DB, should that not be in the logs
> already?

I still disagree :) For some reasons, the administrator may have not seen the error when updating the DB. If there is nothing in logs, there is nothing to warn him.
So, I let another QAer to have a look at this patch.
Comment 12 Mark Tompsett 2013-10-24 18:42:04 UTC
I did the following testing to see what the current behaviour is and what the modified behaviour is:
1) I restored a database from 3 months ago.
2) echo > koha-error_log
3) echo > koha-opac-error_log
4) echo > koha-zebradaemon.err
5) ran the staff client upgrade process.
6) logged in.
7) logged out.
8) deleted the AllowPKIAuth system preference.
9) echo > koha-error_log
10) echo > koha-opac-error_log
11) echo > koha-zebradaemon.err
12) clicked the login in button with nothing.

(I chose this case, because between valid, invalid, username only, password only, and completely blank, the completely blank is easiest to test and show the difference)

[Thu Oct 24 14:30:42 2013] [error] [client 192.168.45.23] [Thu Oct 24 14:30:42 2013] mainpage.pl: Use of uninitialized value in string ne at /home/mtompset/kohaclone/C4/Auth.pm line 685., referer: http://demo.library-admin.kohasystem.ca/cgi-bin/koha/mainpage.pl?logout.x=1
[Thu Oct 24 14:30:42 2013] [error] [client 192.168.45.23] [Thu Oct 24 14:30:42 2013] mainpage.pl: Use of uninitialized value $pki_field in string ne at /home/mtompset/kohaclone/C4/Auth.pm line 758., referer: http://demo.library-admin.kohasystem.ca/cgi-bin/koha/mainpage.pl?logout.x=1
[Thu Oct 24 14:30:42 2013] [error] [client 192.168.45.23] [Thu Oct 24 14:30:42 2013] mainpage.pl: Use of uninitialized value $pki_field in string eq at /home/mtompset/kohaclone/C4/Auth.pm line 795., referer: http://demo.library-admin.kohasystem.ca/cgi-bin/koha/mainpage.pl?logout.x=1
[Thu Oct 24 14:30:42 2013] [error] [client 192.168.45.23] [Thu Oct 24 14:30:42 2013] mainpage.pl: Use of uninitialized value $pki_field in string eq at /home/mtompset/kohaclone/C4/Auth.pm line 795., referer: http://demo.library-admin.kohasystem.ca/cgi-bin/koha/mainpage.pl?logout.x=1
[Thu Oct 24 14:30:42 2013] [error] [client 192.168.45.23] [Thu Oct 24 14:30:42 2013] mainpage.pl: Use of uninitialized value $retuserid in string ne at /home/mtompset/kohaclone/C4/Auth.pm line 832., referer: http://demo.library-admin.kohasystem.ca/cgi-bin/koha/mainpage.pl?logout.x=1
[Thu Oct 24 14:30:42 2013] [error] [client 192.168.45.23] File does not exist: /home/mtompset/kohaclone/koha-tmpl/intranet-tmpl/prog/en/css/color.css, referer: http://demo.library-admin.kohasystem.ca/cgi-bin/koha/mainpage.pl
[Thu Oct 24 14:30:43 2013] [error] [client 192.168.45.23] [Thu Oct 24 14:30:43 2013] 404.pl: Use of uninitialized value $lasttime in numeric lt (<) at /home/mtompset/kohaclone/C4/Auth.pm line 709., referer: http://demo.library-admin.kohasystem.ca/cgi-bin/koha/mainpage.pl
[Thu Oct 24 14:30:43 2013] [error] [client 192.168.45.23] [Thu Oct 24 14:30:43 2013] 404.pl: Use of uninitialized value $pki_field in string ne at /home/mtompset/kohaclone/C4/Auth.pm line 758., referer: http://demo.library-admin.kohasystem.ca/cgi-bin/koha/mainpage.pl
[Thu Oct 24 14:30:43 2013] [error] [client 192.168.45.23] [Thu Oct 24 14:30:43 2013] 404.pl: Use of uninitialized value $pki_field in string eq at /home/mtompset/kohaclone/C4/Auth.pm line 795., referer: http://demo.library-admin.kohasystem.ca/cgi-bin/koha/mainpage.pl
[Thu Oct 24 14:30:43 2013] [error] [client 192.168.45.23] [Thu Oct 24 14:30:43 2013] 404.pl: Use of uninitialized value $pki_field in string eq at /home/mtompset/kohaclone/C4/Auth.pm line 795., referer: http://demo.library-admin.kohasystem.ca/cgi-bin/koha/mainpage.pl
[Thu Oct 24 14:30:43 2013] [error] [client 192.168.45.23] [Thu Oct 24 14:30:43 2013] 404.pl: Use of uninitialized value $retuserid in string ne at /home/mtompset/kohaclone/C4/Auth.pm line 832., referer: http://demo.library-admin.kohasystem.ca/cgi-bin/koha/mainpage.pl

Now apply the patch, clear the logs, and click that login button again.


[Thu Oct 24 14:32:54 2013] [error] [client 192.168.45.23] File does not exist: /home/mtompset/kohaclone/koha-tmpl/intranet-tmpl/prog/en/css/color.css, referer: http://demo.library-admin.kohasystem.ca/cgi-bin/koha/mainpage.pl
[Thu Oct 24 14:32:54 2013] [error] [client 192.168.45.23] [Thu Oct 24 14:32:54 2013] 404.pl: Use of uninitialized value $lasttime in numeric lt (<) at /home/mtompset/kohaclone/C4/Auth.pm line 709., referer: http://demo.library-admin.kohasystem.ca/cgi-bin/koha/mainpage.pl

The key is noticing that 832 did not trigger an error! This else is inside the if which we fixed. So, 'None' is wrong, because it is a different behaviour. I changed it to '', and retried.

[Thu Oct 24 14:37:04 2013] [error] [client 192.168.45.23] [Thu Oct 24 14:37:04 2013] mainpage.pl: Use of uninitialized value $retuserid in string ne at /home/mtompset/kohaclone/C4/Auth.pm line 833., referer: http://demo.library-admin.kohasystem.ca/cgi-bin/koha/mainpage.pl
[Thu Oct 24 14:37:04 2013] [error] [client 192.168.45.23] File does not exist: /home/mtompset/kohaclone/koha-tmpl/intranet-tmpl/prog/en/css/color.css, referer: http://demo.library-admin.kohasystem.ca/cgi-bin/koha/mainpage.pl
[Thu Oct 24 14:37:04 2013] [error] [client 192.168.45.23] [Thu Oct 24 14:37:04 2013] 404.pl: Use of uninitialized value $lasttime in numeric lt (<) at /home/mtompset/kohaclone/C4/Auth.pm line 709., referer: http://demo.library-admin.kohasystem.ca/cgi-bin/koha/mainpage.pl
[Thu Oct 24 14:37:04 2013] [error] [client 192.168.45.23] [Thu Oct 24 14:37:04 2013] 404.pl: Use of uninitialized value $retuserid in string ne at /home/mtompset/kohaclone/C4/Auth.pm line 833., referer: http://demo.library-admin.kohasystem.ca/cgi-bin/koha/mainpage.pl

This is correct behaviour, because there is an extra line added to clean up the other silent errors and 833 - 1 = 832, and the error message matches.

So, I was wrong to second guess myself. Sorry, tcohen.

However, this does show that at least 2 other silent errors need correcting before this bug is fully addressed. I'm setting this back to assigned.
Comment 13 Srdjan Jankovic 2013-11-01 08:43:59 UTC Comment hidden (obsolete)
Comment 14 Srdjan Jankovic 2013-11-01 08:47:59 UTC
Tomás, I also had a take on this one (found out for this bug report too late). Please feel free to combine as you find fit. You may take my patch and add your stuff, or take your patch and add $lasttime correction (I believe the rest overlaps).

Cheers,
Srdjan
Comment 15 Mark Tompsett 2013-11-01 14:09:15 UTC Comment hidden (obsolete)
Comment 16 Srdjan Jankovic 2013-11-08 07:49:19 UTC Comment hidden (obsolete)
Comment 17 Srdjan Jankovic 2013-11-08 07:55:26 UTC Comment hidden (obsolete)
Comment 18 Mark Tompsett 2013-11-08 15:45:38 UTC
Srdjan Jankovic, you undid some of my tweaks which fixed the following cases:
q_userid = undef, s_userid is defined and not '' or 0.
OR
s_userid = undef, q_userid is defined and not '' or 0.

That is the following on line 693:
($q_userid && $s_userid && $q_userid ne $s_userid)
will not work exactly like the noisy
($q_userid ne $s_userid)

This is why I put:
    my $q_userid = $query->param('userid') // ''; # (line 653)
and 
    $s_userid = $session->param('id') // ''; # (line 675)

I'm attaching some test code to demonstrate.
I've changed the status to ASSIGNED, since these pieces we have are a bit tangled up and messy.
Comment 19 Mark Tompsett 2013-11-08 15:47:21 UTC Comment hidden (obsolete)
Comment 20 Mark Tompsett 2013-11-08 17:36:30 UTC Comment hidden (obsolete)
Comment 21 Mark Tompsett 2013-11-08 20:57:39 UTC
I think we're good now. I apologize, Srdjan I may not have put up my tweaks. :(
Comment 22 Srdjan Jankovic 2013-11-10 23:09:48 UTC
No worries mate :)
Comment 23 Tomás Cohen Arazi 2013-11-10 23:13:00 UTC
(In reply to Srdjan Jankovic from comment #22)
> No worries mate :)

+1, I thought my signed-by-Petter patch was enough :-D, I welcome anything that makes this warnings away. I'm already focused in other stuff right now :-D
Comment 24 Mark Tompsett 2013-11-11 01:53:17 UTC Comment hidden (obsolete)
Comment 25 Marcel de Rooy 2013-11-27 17:01:16 UTC
Created attachment 23197 [details] [review]
[PASSED_QA] Bug 11077: Uninitialized value $pki_field warning

During login at the Staff interface you get warnings in the logs
regarding an uninitialized value for the $pki_field variable.

To test:
- tail -f /path/to/your-intranet-logs
- Point your browser to your staff login page
- Login
- Three warnings are showed
- Apply the patch
- Log out
- Log in
- No new warnings, and you can still log in.

Thanks
To+

Sponsored-by: Universidad Nacional de Cordoba
Signed-off-by: Petter Goksoyr Asen <boutrosboutrosboutros@gmail.com>

Followed test plan; it works as advertised.
Also works when I deleted AllowPKIAuth system pref.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 26 Marcel de Rooy 2013-11-27 17:03:10 UTC
Comment on attachment 23197 [details] [review]
[PASSED_QA] Bug 11077: Uninitialized value $pki_field warning

First patch of Thomas passed QA
Comment 27 Marcel de Rooy 2013-11-27 17:25:01 UTC
Comment on attachment 22805 [details] [review]
[FAILED_QA] Bug 11077: get rid of some warnings

Second patch of Srdjan contains this change:

-        elsif ( $lasttime < time() - $timeout ) {
+        elsif ( $lasttime && ($lasttime < time() - $timeout) ) {

If lasttime was undefined, it will be interpreted as zero in the first condition and therefore the condition is true. In the second case it will become false. So we have a behavior change. The warning is gone btw :)
Failed QA
Comment 28 Marcel de Rooy 2013-11-27 17:27:40 UTC Comment hidden (obsolete)
Comment 29 Marcel de Rooy 2013-11-27 17:28:42 UTC Comment hidden (obsolete)
Comment 30 Mark Tompsett 2013-11-27 18:51:59 UTC
My patch corrects the problem for which you failed Srdjan Jankovic's patch. You will notice that I changed:
       elsif ( $lasttime && ($lasttime < time() - $timeout) ) {
into
       elsif ( !$lasttime || ($lasttime < time() - $timeout) ) {
which does work.

I did say, "It also integrates Srdjan Jankovic's patch with Petter Goksoyrsen's
patch, while correcting the problems found," by which I meant that if you applied everything including my patch, everything should silence all the bugs and continue to work like before.

Also, my patch depends on his, so should I squash our patches together, and get a single sign off? Should I correct mine after he corrects his? This is a very weird scenario to have with 3 pieces in 3 vastly different states. I'm happy to fix them any which way, but what is the best course of action?
Comment 31 Marcel de Rooy 2013-11-28 07:29:17 UTC
Hi Mark,
I did not look at the third patch obviously :)
Will have a look soon.
Thanks.
Comment 32 Marcel de Rooy 2013-11-28 09:25:24 UTC
(In reply to M. Tompsett from comment #30)
> Also, my patch depends on his, so should I squash our patches together, and
> get a single sign off? Should I correct mine after he corrects his? This is
> a very weird scenario to have with 3 pieces in 3 vastly different states.
> I'm happy to fix them any which way, but what is the best course of action?

I agree with most changes, including the one where s_userid and q_userid are compared.
I think the second patch should not be pushed in its current form, although the third patch resolves the situation.
So I recommend to squash both patches and get a signoff on the new patch.
If you can add a test plan too, go ahead :)

One point remaining: I do not agree with the PKIAuth change in your last patch.
If you do:
+        my $pki_field = C4::Context->preference('AllowPKIAuth') // '';
combined with: || $pki_field ne 'None', what is the result?
An undef in AllowPKIAuth now makes None ne '' so the compare is true. The test should only be true if AllowPKIAuth is a value not equal to None or empty string (undef). So leaving it preference // None was correct.

Thanks for your attention to these details!
Comment 33 Mark Tompsett 2013-11-28 14:34:53 UTC Comment hidden (obsolete)
Comment 34 Mark Tompsett 2013-11-28 14:37:29 UTC Comment hidden (obsolete)
Comment 35 Marcel de Rooy 2013-11-28 14:43:17 UTC
(In reply to M. Tompsett from comment #34)
> Created attachment 23215 [details]
> Confirmation of what I said regarding undef ne 'None'
> 
> I believe this clarifies the problem even better, and this sample program
> demonstrates why I believe '' is the correct value to default in the undef
> case.

Maybe we have some misunderstanding?
Correct me if I am wrong but if PKIAuth is None or undef, the if statement should not be run (so we should make the expr be false). 
Do you agree?
Comment 36 Mark Tompsett 2013-11-28 14:48:29 UTC
(In reply to M. de Rooy from comment #35)
> Maybe we have some misunderstanding?

We may have some cross-talk.


> Correct me if I am wrong but if PKIAuth is None or undef, the if statement
> should not be run (so we should make the expr be false).
> Do you agree?

My sample program demonstrates how it currently works and does not make a judgement one way or another about how it should work. Using '' mimics current behaviour. And though that may not be correct in how it should behave, patches fixing one thing (floody logs) should not cause a behaviour change in another (how it does work). If the issue remains about how it should work (as compared to how it does work), that would be another bug fix in my opinion. Does that clarify where I am coming from?
Comment 37 Marcel de Rooy 2013-11-28 17:52:23 UTC
(In reply to M. Tompsett from comment #36)
> > Correct me if I am wrong but if PKIAuth is None or undef, the if statement
> > should not be run (so we should make the expr be false).
> > Do you agree?
> 
> My sample program demonstrates how it currently works and does not make a
> judgement one way or another about how it should work. Using '' mimics
> current behaviour. And though that may not be correct in how it should
> behave, patches fixing one thing (floody logs) should not cause a behaviour
> change in another (how it does work). If the issue remains about how it
> should work (as compared to how it does work), that would be another bug fix
> in my opinion. Does that clarify where I am coming from?

That makes sense. Actually, we are talking about functionality. But for such a fictive situation (you need to delete your AllowPKIAuth pref and have all other conditions be false), I think we do not need to open a new report. This "behavior change" is so minimal that touching this line and fixing it can be done rightaway.
For the record, the behavior change is actually made in the first patch that has been signed off and passed qa and your third patch changes it back to current behavior with the latent undef-mini-minibug.

But if you scroll through the code, you will see that in both cases the overall effect will be: userid=return=0, auth rejected. So no big deal. Just theory.
Comment 38 Mark Tompsett 2013-11-28 18:23:58 UTC Comment hidden (obsolete)
Comment 39 Mark Tompsett 2013-11-28 18:35:52 UTC
Yes, my patch does change it back. I did as you suggested and looked at the code again. If we set the variable to 'None', it bypasses the if completely. If we set the variable to '', it goes through the elsif check, but runs the else which fail (due to a lack of userid). The return check fails, triggering the else which does nothings, because of the lack of userid. In the end, a lack of login is accomplished either way. Since 'None' was approved via the QA process, I'll change my patch to exclude that tweak.
Comment 40 Mark Tompsett 2013-11-28 18:45:40 UTC Comment hidden (obsolete)
Comment 41 Mark Tompsett 2013-11-28 19:07:56 UTC
Test Plan:
- Go to staff client
- Click the login button without typing anything
- Type just a username without a password and click the login button
- Backup DB
- Delete the AllowPKIAuth System Preference
- Restore DB
Note the amount of noise.
- Apply Patches (All three)
- Go to staff client
- Click the login button without typing anything
- Type just a username without a password and click the login button
- Backup DB
- Delete the AllowPKIAuth System Preference
- Restore DB
The only noise should be the clarified error:
    ERROR: Missing system preference AllowPKIAuth.
This makes the non-sensical flood clearer. :)
Comment 42 Mark Tompsett 2013-11-28 19:25:57 UTC
(In reply to M. Tompsett from comment #41)
> - Delete the AllowPKIAuth System Preference

Oops... and click the login button.
Comment 43 Mark Tompsett 2013-11-29 02:45:47 UTC Comment hidden (obsolete)
Comment 44 Mark Tompsett 2013-11-29 02:50:25 UTC
Confirmed with Srdjan Jankovic that is was okay to squash our patches together. All should be good to go.

Here is the latest test plan:
- Go to staff client
- Click the login button without typing anything
- Type just a username without a password and click the login button
- Backup DB
- Delete the AllowPKIAuth System Preference, and refresh the staff client.
- Restore DB
Note the amount of noise.
- Apply Patches (Both)
- Go to staff client
- Click the login button without typing anything
- Type just a username without a password and click the login button
- Backup DB
- Delete the AllowPKIAuth System Preference, and refresh the staff client.
- Restore DB
The only noise should be the clarified error:
    ERROR: Missing system preference AllowPKIAuth.
This makes the non-sensical flood clearer. :)
Comment 45 Marcel de Rooy 2013-11-29 11:06:59 UTC
Thanks, Mark.
Since the patch still is on the name of Srdjan, I would like to ask him to test the patch again. After that please add your signoff, Mark. Since this looks good to me, QA will not take that much time any more :)
Note that print STDERR could be a warn, and personally I do not think that it is really necessary to warn here but others think so. Fine with me.
Comment 46 Srdjan Jankovic 2013-12-02 21:15:47 UTC Comment hidden (obsolete)
Comment 47 Marcel de Rooy 2013-12-03 12:50:09 UTC
Thanks, Srdjan.
Mark, could you please add your signoff too?
Comment 48 Mark Tompsett 2013-12-03 14:17:33 UTC Comment hidden (obsolete)
Comment 49 Mark Tompsett 2013-12-03 14:20:53 UTC
Re-tested.

- staff client: click login with nothing entered
- click login with just a valid username entered
- click login with proper credentials entered
- log out
- deleted system preference
- click login with proper credentials
- ran qa tool

Looks good to me. Signed off, as you requested Marcel.
Comment 50 Marcel de Rooy 2013-12-04 12:43:44 UTC
Created attachment 23288 [details] [review]
[PASSED_QA] Bug 11077 - Correct silent warnings in C4/Auth.pm

This gets rid of some more warnings.

It also corrects a noisy ne condition.
    $userid = $retuserid if ( $retuserid ne '');
became
    $userid = $retuserid if ( $retuserid );

It also integrates Srdjan Jankovic's patch with Petter Goksoyrsen's
patch, while correcting the problems found.

This includes:
    my $q_userid = $query->param('userid') // '';
along with:
    my $s_userid = '';
and:
    my $s_userid = $session->param('id') // '';
Indentation does not reflect actual scoping.

A missing system preference would have triggered a ubiquitous
undef compare check failure message. This makes the flooding
message more useful, so as to help correct it.
The change to accomplish this was:
        my $pki_field = C4::Context->preference('AllowPKIAuth');
        if (!defined($pki_field)) {
            print STDERR "Error: Missing AllowPKIAuth System Preference!\n";
            $pki_field = 'None';
        }

Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 51 Marcel de Rooy 2013-12-04 12:45:07 UTC
Passed QA now (see earlier comments and adjustments).
Comment 52 Galen Charlton 2013-12-05 15:28:05 UTC
Pushed to master.  Thanks, Tomás and Srdjan!
Comment 53 Fridolin Somers 2013-12-13 15:59:12 UTC
Those patches has been pushed to 3.14.x, will be in 3.14.1
Comment 54 Tomás Cohen Arazi 2013-12-23 15:31:36 UTC
This patch has been pushed to 3.12.x, will be in 3.12.8.

Thanks Srdjan and Mark!
Comment 55 Mark Tompsett 2014-01-06 04:03:42 UTC
*** Bug 9871 has been marked as a duplicate of this bug. ***