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 [details] for reference).
Created attachment 31698 [details] [review] 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.
(In reply to Jacek Ablewicz from comment #0) > authorisation/authentification, checkout() would treat this user as s/checkout/checkauth/
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.
With patch the error can no longer be replicated. Switch to "Needs Signoff"
Created attachment 34551 [details] [review] 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>
Martin, could you take a look at this please?
Comment on attachment 34551 [details] [review] Bug 12954 - Questionable logic regarding session handling in C4::Auth::checkauth() Review of attachment 34551 [details] [review]: ----------------------------------------------------------------- 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.
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
Created attachment 34829 [details] [review] [Alternate Patch] 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
Comment on attachment 34829 [details] [review] [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
The test plan needs to include combinations: - username/password - cardnumber/password - bad username/password - username/bad password - cardnumber/bad password
(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.
Comment on attachment 34551 [details] [review] Bug 12954 - Questionable logic regarding session handling in C4::Auth::checkauth() Review of attachment 34551 [details] [review]: ----------------------------------------------------------------- ::: 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.
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
Created attachment 35341 [details] [review] 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>
Created attachment 35412 [details] [review] 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>
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.
Patch pushed to master. Thanks Martin!
Pushed to 3.18.x will be in 3.18.4
Turns out this is really nasty, this bug allows you to bypass the OpacPublic setting. If you have opacpublic switched off, and you try an invalid login, and then hit the opac again, you can now see the opac .. which you shouldn't be able to do. This patch needs to be backported to 3.14.x and 3.16.x. Shifting the search history to the session from a cookie accidentally caused this problem.
(In reply to Chris Cormack from comment #20) > > This patch needs to be backported to 3.14.x and 3.16.x. Indeed. Do we agree that we can backport this bug without backporting its dependancies : Bug 13499 and Bug 13521 ?
(In reply to Fridolin SOMERS from comment #21) > (In reply to Chris Cormack from comment #20) > > > > This patch needs to be backported to 3.14.x and 3.16.x. > > Indeed. > Do we agree that we can backport this bug without backporting its > dependancies : Bug 13499 and Bug 13521 ? 13499 is a perl tidy, and 13521 is fixing two tiny bugs. I don't see why you wouldn't backport 13521.
(In reply to M. Tompsett from comment #22) > > 13499 is a perl tidy, and 13521 is fixing two tiny bugs. > I don't see why you wouldn't backport 13521. Ok, Bug 13521 is very simple, it can be backported indeed
Created attachment 37595 [details] [review] Bug 12954: Failed login should retain anonymous session (3.16.x) 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> Signed-off-by: Mason James <mtj@kohaaloha.com>
hi Fridolin, you might want to pull my 3.16 patch
Pushed to 3.16.x, will be in 3.16.10
Pushed to 3.14.x, will be in 3.14.16