[Bug 12954] New: Questionable logic regarding session handling in C4::Auth::checkauth()
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 Bug ID: 12954 Summary: Questionable logic regarding session handling in C4::Auth::checkauth() Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: major Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: gmcharlt@gmail.com Reporter: abl@biblos.pk.edu.pl QA Contact: testopia@bugs.koha-community.org In case of the unsuccessfull log-in attempt (non-existing user ID, bad password, or both), session params are currently (since Bug 10952 got pushed) being set like that in checkauth(): - lasttime & ip params are set, - number, id, cardnumber, firstname etc. are not set, - sessiontype is NOT set to 'anon' So the user session is essentially left in semi-authorised, semi-anonymous state. If then such an user goes directly to any page which does require authorisation/authentification, checkout() would treat this user as authentificated one, and would return prematurely without redirection to the login screen. This also typically (?) leads to get_template_and_user() being bypassed: it does return to the script from which it was called from (but with empty $borrowernumber return value). In case of the (e.g.) opac/opac-user.pl script, observable effect of this bug is an DBI error: opac-user.pl: DBD::mysql::st execute failed: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '' at line 1 at /home/koha/devkohaclone/C4/Letters.pm line 200 (because C4::Letters::getalert() doesn't like to be called with empty $borrowernumber). To replicate: 1/ try to log in with some nonexisting user id or wrong password in OPAC 2/ go directly to the opac/opac-user.pl page (e.g., enter it in the browser address bar, or just click on the "Log in" link); clicking on "Log in" button, when "Login" field is empty would also probably trigger this error 3/ observe DBI error displayed on the screen 4/ you are now in the "deadloop" of sorts (opac/opac-user.pl refuses to display the login screen, no matter how many times you try to reload it); to break the deadloop, one needs to: - remove session cookie from the browser (or cause the session to expire in some other way - closing browser window would be probably enough for that) - remove offending session on the server (from mysql sessions table, ..) - log in with proper credentials using some other page (like opac/opac-main.pl right-side panel), which does not involve opac/opac-user.pl being called without "userid" CGI parameter. Another way to replicate: 1/ try to log in with some nonexisting user id or wrong password in OPAC 2/ go to the staff interface 3/ observe that login screen displayed is different (not the "normal" one, but one with some additional buttons for loging-out etc. + "You do not have permision to access this page" message; see Bug 12951 attachment 31689 for reference). -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 Rafal Kopaczka <rkopaczka@afm.edu.pl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |rkopaczka@afm.edu.pl -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 Jacek Ablewicz <abl@biblos.pk.edu.pl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=12951 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 --- Comment #1 from Jacek Ablewicz <ablewicz@gmail.com> --- Created attachment 31698 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=31698&action=edit Bug 12954 - Questionable logic regarding session handling in C4::Auth::checkauth() Note: This is probably not quite proper patch for this bug (and for sure not the best possible approach for fixing it); uploaded only as reference/proof of concept etc. While it does seem to fix the problem described in Bug 12954 comment #0, it most likely does have some side effects regarding anonymous search history storage and handling. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 Jacek Ablewicz <abl@biblos.pk.edu.pl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=10952 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 --- Comment #2 from Jacek Ablewicz <abl@biblos.pk.edu.pl> --- (In reply to Jacek Ablewicz from comment #0)
authorisation/authentification, checkout() would treat this user as
s/checkout/checkauth/ -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |veron@veron.ch --- Comment #3 from Marc Véron <veron@veron.ch> --- I could replicate the issue: Login to Opac with wrong credentials - username only - password only - wrong username and/or password See page witn message "You entered an incorrect username or password." Hit Button "Login" Result: Software error: Can't use an undefined value as an ARRAY reference at /usr/lib/perl5/DBI.pm line 2054. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff --- Comment #4 from Marc Véron <veron@veron.ch> --- With patch the error can no longer be replicated. Switch to "Needs Signoff" -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #31698|0 |1 is obsolete| | --- Comment #5 from Marc Véron <veron@veron.ch> --- Created attachment 34551 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=34551&action=edit Bug 12954 - Questionable logic regarding session handling in C4::Auth::checkauth() Note: This is probably not quite proper patch for this bug (and for sure not the best possible approach for fixing it); uploaded only as reference/proof of concept etc. While it does seem to fix the problem described in Bug 12954 comment #0, it most likely does have some side effects regarding anonymous search history storage and handling. I could replicate the issue without patch. The issue is no longer replicatable with patch. I sign off because the patch removes a "Software error" on Opac. Signed-off-by: Marc Véron <veron@veron.ch> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Priority|P5 - low |P3 Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de, | |martin.renvoize@ptfs-europe | |.com Component|Architecture, internals, |Browser compatibility |and plumbing | Assignee|gmcharlt@gmail.com |oleonard@myacpl.org -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 --- Comment #6 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Martin, could you take a look at this please? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|oleonard@myacpl.org |abl@biblos.pk.edu.pl -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 --- Comment #7 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Comment on attachment 34551 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=34551 Bug 12954 - Questionable logic regarding session handling in C4::Auth::checkauth() Review of attachment 34551: --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=12954&attachment=34551) ----------------------------------------------------------------- First comments, but nothing really important so far.. Still working my way through the logic for the second part ::: C4/Auth.pm @@ +780,4 @@
} elsif ( !$lasttime || ($lasttime < time() - $timeout) ) { # timed logout + $info{'timed_out'} = 1 if $lasttime;
This feels unrelated to me, as such I'de rather see it in it's own bug rather than confusing the issue here. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 --- Comment #8 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- I think I would prefer to see this done 'properly', i.e. setting the session to anon and ensuring it's tested for to prevent the loop we're finding here. I'll have a play around and see if I can achieve this, unless you want to refine your attempt Jacek -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 --- Comment #9 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 34829 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=34829&action=edit Bug 12954: Failed login should retain anonymous session A failed login should not leave the user in a half logged authenticated state, but rather return them to an anonymouse session as per the pre-login attempt state. To replicate error: 1. Try to log in with some nonexisting user id or wrong password in the OPAC 2. Go directly to /opac-user.pl (e.g., enter it in the browser address bar, or just click on the "Log in" link) 3. Observe a DBI error displayed on the screen 4. You are now in the "deadloop" of sorts (opac/opac-user.pl refuses to display the login screen, no matter how many times you try to reload it); to break the deadloop, one needs to: - remove session cookie from the browser (or cause the session to expire in some other way - closing browser window would be probably enough for that) - remove offending session on the server (from mysql sessions table, ..) - log in with proper credentials using some other page (like opac/opac-main.pl right-side panel), which does not involve opac/opac-user.pl being called without "userid" CGI parameter. To test: 1. Test as above, the DBI error should no longer be present 2. Check that search history works across failed and sucessful login attempts -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #34829|Bug 12954: Failed login |[Alternate Patch] Bug description|should retain anonymous |12954: Failed login should |session |retain anonymous session --- Comment #10 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Comment on attachment 34829 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=34829 [Alternate Patch] Bug 12954: Failed login should retain anonymous session I've submitted an alternate patch, as the original broke cross login attempt anonymous search history. It took a while to get my head around the code, but actually it looks to be a more trivial patch than initially submitted. I've based my patch on the Tidied Auth.pm available in bug 13499 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |13499 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtompset@hotmail.com --- Comment #11 from M. Tompsett <mtompset@hotmail.com> --- The test plan needs to include combinations: - username/password - cardnumber/password - bad username/password - username/bad password - cardnumber/bad password -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 --- Comment #12 from M. Tompsett <mtompset@hotmail.com> --- (In reply to Martin Renvoize from comment #7)
+ $info{'timed_out'} = 1 if $lasttime;
This feels unrelated to me, as such I'de rather see it in it's own bug rather than confusing the issue here.
I agree, please make another bug. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 --- Comment #13 from M. Tompsett <mtompset@hotmail.com> --- Comment on attachment 34551 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=34551 Bug 12954 - Questionable logic regarding session handling in C4::Auth::checkauth() Review of attachment 34551: --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=12954&attachment=34551) ----------------------------------------------------------------- ::: C4/Auth.pm @@ +1060,5 @@
+ if ($session) { + $session->delete(); + $session->flush; + } + C4::Context->_unset_userenv($sessionID) if $sessionID;
I'm not sure moving this outside the if ($userid) check is right. I vaguely recall strangeness with all the 5 test cases that I recommended when I was testing a custom authentication for our organization in terms of setting of userid and return and sessionID. I haven't tried this, but this code change seems wrong to me. I'll possibly attempt later. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |13503 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on|13503 | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 --- Comment #14 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Any chance you could look at my alternative patch too Mark... I would fail qa on the first patch as it broke cross login attempt anonymous search history for me.. which is a regression, I left it there initially so people could see the difference between aproaches adn check whether I'de missed any fringe cases. Test plan: We need to test for the DBI error, and whether search history if maintained accross the login/failed login boundary. Before Patch: Good username / Good password = Login with history maintained Good cardnumber / Good password = Login with history maintained Good username / Bad password = DBI Error Good cardnumber / Bad password = DBI Error Bad username / Good password = DBI Error Bad cardnumber / Good password = DBI Error After Patch: Good username / Good password = Login with history maintained Good cardnumber / Good password = Login with history maintained Good username / Bad password = Bad password error, anon search history maintained Good cardnumber / Bad password = Bad password error, anon search history maintained Bad username / Good password = Bad password error, anon search history maintained Bad cardnumber / Good password = Bad password error, anon search history maintained -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |13521 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply CC| |chris@bigballofwax.co.nz -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #34551|0 |1 is obsolete| | Attachment #34829|0 |1 is obsolete| | --- Comment #15 from Chris Cormack <chris@bigballofwax.co.nz> --- Created attachment 35341 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=35341&action=edit Bug 12954: Failed login should retain anonymous session A failed login should not leave the user in a half logged authenticated state, but rather return them to an anonymouse session as per the pre-login attempt state. To replicate error: 1. Try to log in with some nonexisting user id or wrong password in the OPAC 2. Go directly to /opac-user.pl (e.g., enter it in the browser address bar, or just click on the "Log in" link) 3. Observe a DBI error displayed on the screen 4. You are now in the "deadloop" of sorts (opac/opac-user.pl refuses to display the login screen, no matter how many times you try to reload it); to break the deadloop, one needs to: - remove session cookie from the browser (or cause the session to expire in some other way - closing browser window would be probably enough for that) - remove offending session on the server (from mysql sessions table, ..) - log in with proper credentials using some other page (like opac/opac-main.pl right-side panel), which does not involve opac/opac-user.pl being called without "userid" CGI parameter. To test: 1. Test as above, the DBI error should no longer be present 2. Check that search history works across failed and sucessful login attempts Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #35341|0 |1 is obsolete| | --- Comment #16 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 35412 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=35412&action=edit Bug 12954: Failed login should retain anonymous session A failed login should not leave the user in a half logged authenticated state, but rather return them to an anonymouse session as per the pre-login attempt state. To replicate error: 1. Try to log in with some nonexisting user id or wrong password in the OPAC 2. Go directly to /opac-user.pl (e.g., enter it in the browser address bar, or just click on the "Log in" link) 3. Observe a DBI error displayed on the screen 4. You are now in the "deadloop" of sorts (opac/opac-user.pl refuses to display the login screen, no matter how many times you try to reload it); to break the deadloop, one needs to: - remove session cookie from the browser (or cause the session to expire in some other way - closing browser window would be probably enough for that) - remove offending session on the server (from mysql sessions table, ..) - log in with proper credentials using some other page (like opac/opac-main.pl right-side panel), which does not involve opac/opac-user.pl being called without "userid" CGI parameter. To test: 1. Test as above, the DBI error should no longer be present 2. Check that search history works across failed and sucessful login attempts Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@biblibre.co | |m --- Comment #17 from Jonathan Druart <jonathan.druart@biblibre.com> --- This is a very bad bug! I tried to fix something in opac-user.pl, to avoid the script to continue (a redirect if get_template_and_user does not return a $borrowernumber?), but don't know how to do it correctly. I confirm this patch fixes the issues. Marked as Passed QA. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com --- Comment #18 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Patch pushed to master. Thanks Martin! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable --- Comment #19 from Chris Cormack <chris@bigballofwax.co.nz> --- Pushed to 3.18.x will be in 3.18.4 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au, | |robin@catalyst.net.nz -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12954 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Group|Koha security | CC| |dpavlin@rot13.org Version|unspecified |master Product|Koha security |Koha QA Contact| |testopia@bugs.koha-communit | |y.org Component|Koha |Authentication -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org