Description
Jacek Ablewicz
2014-09-18 09:46:21 UTC
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 |