Created attachment 22042 [details] [review] Remove $pki_field variable warning
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.
Created attachment 22043 [details] [review] Silence 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+
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.
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. :)
Created attachment 22120 [details] [review] Silence uninitialized value $pki_field warning (and another one)
Created attachment 22125 [details] [review] 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.
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 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.
(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.
(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.
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.
Created attachment 22665 [details] [review] bug_11077: get rid of some warnings
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
Created attachment 22667 [details] [review] Bug 11077 - Correct silent warnings in C4/Auth.pm This patch corrects a noisy ne condition. $userid = $retuserid if ( $retuserid ne ''); became $userid = $retuserid if ( $retuserid ); It also integrates Srdjan Jankovic's patch with Petter Goksøyr Åsen's patch, while correcting the problems found.
Created attachment 22805 [details] [review] [FAILED_QA] Bug 11077: get rid of some warnings
Created attachment 22806 [details] [review] Bug 11077 - Correct silent warnings in C4/Auth.pm This patch 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.
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.
Created attachment 22831 [details] Sample Test Program to confirm behaviour of busted vs current patch vs suggestion
Created attachment 22844 [details] [review] Bug 11077 - Correct silent warnings in C4/Auth.pm This patch 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.
I think we're good now. I apologize, Srdjan I may not have put up my tweaks. :(
No worries mate :)
(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
Created attachment 22854 [details] [review] Bug 11077 - Correct silent warnings in C4/Auth.pm This patch 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. And the 'None' is changed to '', since that behaves properly in the undef case: my $pki_field = C4::Context->preference('AllowPKIAuth') // '';
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 on attachment 23197 [details] [review] [PASSED_QA] Bug 11077: Uninitialized value $pki_field warning First patch of Thomas passed QA
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
Created attachment 23198 [details] [review] [FAILED_QA] bug_11077: get rid of some warnings
Created attachment 23199 [details] [review] Bug 11077 - Correct silent warnings in C4/Auth.pm This patch 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. And the 'None' is changed to '', since that behaves properly in the undef case: my $pki_field = C4::Context->preference('AllowPKIAuth') // '';
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?
Hi Mark, I did not look at the third patch obviously :) Will have a look soon. Thanks.
(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!
Created attachment 23214 [details] [review] Bug 11077 - Correct silent warnings in C4/Auth.pm This patch 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. And the 'None' is changed to '', since that behaves properly in the undef case: my $pki_field = C4::Context->preference('AllowPKIAuth'); if (!defined($pki_field)) { print STDERR "Error: Missing AllowPKIAuth System Preference!\n"; $pki_field = ''; } Because if it was set to 'None' in the undefined case, then 'None' ne 'None' is false, and the if case would not run. However, undef ne 'None' is true and runs with an error log entry. By putting the define check as a separate if clause, a meaningful error message can be given rather than a ubiquitous undef compare check failure message.
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.
(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?
(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?
(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.
Created attachment 23217 [details] [review] Bug 11077 - Fix indents to trace code more easily
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.
Created attachment 23218 [details] [review] Bug 11077 - Correct silent warnings in C4/Auth.pm This patch 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'; }
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. :)
(In reply to M. Tompsett from comment #41) > - Delete the AllowPKIAuth System Preference Oops... and click the login button.
Created attachment 23219 [details] [review] 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'; }
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. :)
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.
Created attachment 23240 [details] [review] 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>
Thanks, Srdjan. Mark, could you please add your signoff too?
Created attachment 23268 [details] [review] [SIGNED OFF] 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>
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.
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>
Passed QA now (see earlier comments and adjustments).
Pushed to master. Thanks, Tomás and Srdjan!
Those patches has been pushed to 3.14.x, will be in 3.14.1
This patch has been pushed to 3.12.x, will be in 3.12.8. Thanks Srdjan and Mark!
*** Bug 9871 has been marked as a duplicate of this bug. ***