bug 36102 / bug 34478 added the csrf token to opac-auth for patron login, however, it isn't used for SCO/SCI logins
Created attachment 163356 [details] [review] Bug 36349: Make sure CSRF token is included for all login scenarios To test: 1 - In KTD visit: http://localhost:8080/cgi-bin/koha/sci/sci-main.pl 2 - Everything should be set for auto self check user etc, just login as a patron If not (or not using KTD) setup a self check user, enable SCO and SCI, set self check patron system preferences, then login with patron 3 - 403 Error 4 - Repeat with sco: http://localhost:8080/cgi-bin/koha/sco/sco-main.pl 5 - Apply patch, restart all 6 - Try again, both should be successful
For the SCO log in (step 3), I have to log in three times - I'm assuming this is not what is expected: 1. Normal log in 2. SCO log in 3. Normal log on --> Then get to check out items In addition, when using KTD I needed to enable the SelfCheckInModule system preference for step 1. Tested using both Firefox and Chromium.
We have a problem here. We are removing the credential (login_userid and login_password from CGI) in get_template_and_user if not a POST (and not op ne 'cud-login'). I am failing at finding a correct fix. Should we actually fix the AutoSelfCheckAllowed behavior in sco-main.pl and deal with the credentials retrieved from the prefs in the controller?
Created attachment 163386 [details] [review] Bug 36349: Fix AutoSelfCheckAllowed This feels terribly wrong...
And logout is broken as well, you cannot finish the user session.
Created attachment 163514 [details] [review] Bug 36349: Fix AutoSelfCheckAllowed Move the check to C4::Auth. Yes, it's not nice, I didn't expect to add exception to this code, and it's adding more ugly code to get_template_and_user, but... Suggestions welcome!
Still not happy with this new patch, but it seems to restore the feature.
Created attachment 163552 [details] [review] Bug 36349: Make sure CSRF token is included for all login scenarios To test: 1 - In KTD visit: http://localhost:8080/cgi-bin/koha/sci/sci-main.pl 2 - Everything should be set for auto self check user etc, just login as a patron If not (or not using KTD) setup a self check user, enable SCO and SCI, set self check patron system preferences, then login with patron 3 - 403 Error 4 - Repeat with sco: http://localhost:8080/cgi-bin/koha/sco/sco-main.pl 5 - Apply patch, restart all 6 - Try again, both should be successful Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 163553 [details] [review] Bug 36349: Fix AutoSelfCheckAllowed Move the check to C4::Auth. Yes, it's not nice, I didn't expect to add exception to this code, and it's adding more ugly code to get_template_and_user, but... Suggestions welcome! Signed-off-by: Owen Leonard <oleonard@myacpl.org>
This works, but it doesn't cover the SCI too - separate bug or want to update the patch?
Changes to Auth need tests.
(In reply to Nick Clemens from comment #10) > This works, but it doesn't cover the SCI too - separate bug or want to > update the patch? What's broken with SCI?
(In reply to Marcel de Rooy from comment #11) > Changes to Auth need tests. Yes, but first I would like to make sure there is not a better solution, because it smells!
(In reply to Jonathan Druart from comment #12) > (In reply to Nick Clemens from comment #10) > > This works, but it doesn't cover the SCI too - separate bug or want to > > update the patch? > > What's broken with SCI? Login fails too with No CSRF token passed for POST http://localhost:8080/opac/sci/sci-main.pl
I see what is confusing: When logged in the OPAC already (or in staff if it's the same domain), SCI seems to have no issue because it uses the current session.
(In reply to Victor Grousset/tuxayo from comment #14) > (In reply to Jonathan Druart from comment #12) > > (In reply to Nick Clemens from comment #10) > > > This works, but it doesn't cover the SCI too - separate bug or want to > > > update the patch? > > > > What's broken with SCI? > > Login fails too with > No CSRF token passed for POST http://localhost:8080/opac/sci/sci-main.pl With the patches?
oops, I though the confusion was about whether or not both SCI and SCO were affected by the bug. Turns out patches indeed addresses both even if touching opac/sci/sci-main.pl wasn't needed. --- Anyway, found this while testing: 1. Open both http://localhost:8080/cgi-bin/koha/sci/sci-main.pl and http://localhost:8080/cgi-bin/koha/sco/sco-main.pl 2. log in and log out from SCI 3. go back to the SCO tab and try to log in 4. "The form submission failed (Wrong CSRF token). Try to come back, refresh the page, then try again." That would likely only affect testing scenarios depending on detailed testing habits about when opening stuff. So not much impact likely. I don't know if the cause of that could still have a wider relevance.
What you describe is "expected". At least other login forms are affected by this and should be reported on its own bug (ie. not only sci/sco).
(In reply to Jonathan Druart from comment #18) > What you describe is "expected". At least other login forms are affected by > this and should be reported on its own bug (ie. not only sci/sco). Ok, no deeper issue, great. What is the next step? Calling for help on koha-devel to find a cleaner approach? Or going with the current one? In that case I can sign-off. Found nothing else weird after messing around with the forms.
Sorry for neglecting this one so long. I'm taking a deeper look at the issue today...
With the 1st patch, we've got manual log in fixed for /cgi-bin/koha/sco/sco-main.pl If it's the first session, I can manually log into /cgi-bin/koha/sci/sci-main.pl (However, if I log into /cgi-bin/koha/sco/sco-main.pl and then log into /cgi-bin/koha/sci/sci-main.pl it doesn't work, as I'm presented with the message "Access denied. Sorry, the system doesn't think you have permission to access this page." It looks like it's because it's doing the "kick_out" since we have $is_sco_user. That's a current system thing so nothing to worry about.) -- The 2nd patch fixes AutoSelfCheckAllowed, but I can see what Jonathan means about it being ugly. Something to keep in mind is that we're re-authenticating on every SCO page load, but my bug 34478 commit 8b69d0b4d3e72171d9f2d51234f345405a433c4f actually makes the AutoSelfCheck session persistent, so once we have that authenticated session, we don't really need to re-authenticate. One option could be to move the AutoSelfCheck auth process into "checkauth". Another option could be to allow sco-main.pl to do a sort of "preauth" and create its own authenticated session before get_template_and_user() if necessary. (At the moment, this is sort of what we're trying to do by force stuffing the AutoSelfCheck credentials on every load of sco-main.pl.) However, this option would need some tweaks to things like get_template_and_user() and checkauth() to be able to accept a sessionID passed in (instead of just looking at the incoming session cookie). So I think we could do something a bit smarter when handling our sessions. -- But... both of those options would take more work. Jonathan has provided something that already appears to work, and we're getting closer to our May release deadline, so I think perhaps we shouldn't make the perfect the enemy of the good.
For completeness, we should remove the AutoSelfCheck stuff from opac/sco/printslip.pl as well, since it's covered by the second patch.
I need to switch tasks, but let me know what you think. Otherwise, I can QA stamp this tomorrow...
Created attachment 164411 [details] [review] Bug 36349: Remove passing CGI params from sco/printslip.pl
Created attachment 164412 [details] [review] Bug 36349: Add tests
Good to go now if nothing better is found.
Created attachment 164444 [details] [review] Bug 36349: Make sure CSRF token is included for all login scenarios To test: 1 - In KTD visit: http://localhost:8080/cgi-bin/koha/sci/sci-main.pl 2 - Everything should be set for auto self check user etc, just login as a patron If not (or not using KTD) setup a self check user, enable SCO and SCI, set self check patron system preferences, then login with patron 3 - 403 Error 4 - Repeat with sco: http://localhost:8080/cgi-bin/koha/sco/sco-main.pl 5 - Apply patch, restart all 6 - Try again, both should be successful Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 164445 [details] [review] Bug 36349: Fix AutoSelfCheckAllowed Move the check to C4::Auth. Yes, it's not nice, I didn't expect to add exception to this code, and it's adding more ugly code to get_template_and_user, but... Suggestions welcome! Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 164446 [details] [review] Bug 36349: Remove passing CGI params from sco/printslip.pl Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 164447 [details] [review] Bug 36349: Add tests Signed-off-by: David Cook <dcook@prosentient.com.au>
I think this was only in Failed QA because missing test, so with it added I think we can skip back to Signed Off, and I'll add Passed QA. -- It might not be the most elegant solution, but it's the most workable one, and I think workable is what we need right now.
Pushed for 24.05! Well done everyone, thank you!
Depends on Bug 34478 not in 23.11.x