[Bug 28785] New: Code in C4::Auth::checkauth is copy pasted
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 Bug ID: 28785 Summary: Code in C4::Auth::checkauth is copy pasted Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: ASSIGNED Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: jonathan.druart+koha@gmail.com Reporter: jonathan.druart+koha@gmail.com QA Contact: testopia@bugs.koha-community.org The code to check cookie authentication is duplicated in several places inside C4::Auth: * check_cookie_auth * checkauth * check_api_auth The subroutine check_cookie_auth must be called from checkauth and check_api_auth to avoid code duplication. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |28786 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28786 [Bug 28786] Two-factor authentication for staff client - TOTP -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 --- Comment #1 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 123314 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123314&action=edit Bug 28785: Centralize cookie auth check in check_cookie_auth This code is duplicated in 3 different places, we must call check_cookie_auth instead. It makes check_cookie_auth returns a 'restricted' when SessionRestrictionByIP is set and the IP changed. It also returns a third parameters contained the old and new IP, to fill the "info" hash in checkauth but apparently the oldip and newip variables are not even used from the template. We may want to remove it completely. No change is expected with this patch, the different authentication methods should still work as before. Test plan: Log in the staff and OPAC interfaces, logout. Log in and call script that call the 3 different subroutines modified by this patch. For instance you can list checkouts (that is using check_cookie_auth) and display a patron's image (using check_api_auth). QA with good knowledge of the C4::Auth module and the different authentication methods is required. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 --- Comment #2 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 123315 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123315&action=edit Bug 28785: Adjust check_cookie_auth calls The previous patch makes check_cookie_auth return the session instead of $sessionID, so we are adjusting the different calls to prevent confusion. However they are mainly used to check the authentication status and don't care about this second variable. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 --- Comment #3 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 123316 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123316&action=edit Bug 28785: Add skip_version_check for checkauth We could have this patch but we also could decide to skip it. The idea is to avoid 2 checks of the version when we are coming from checkauth. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 --- Comment #4 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 123317 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123317&action=edit Bug 28785: Don't send SessionRestrictionByIP to template It's not used. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 --- Comment #5 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 123319 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123319&action=edit Bug 28785: Add missing POD for check_cookie_auth -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #123314|0 |1 is obsolete| | Attachment #123315|0 |1 is obsolete| | Attachment #123316|0 |1 is obsolete| | Attachment #123317|0 |1 is obsolete| | Attachment #123319|0 |1 is obsolete| | --- Comment #6 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 123320 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123320&action=edit Bug 28785: Centralize cookie auth check in check_cookie_auth This code is duplicated in 3 different places, we must call check_cookie_auth instead. It makes check_cookie_auth returns a 'restricted' when SessionRestrictionByIP is set and the IP changed. It also returns a third parameters contained the old and new IP, to fill the "info" hash in checkauth but apparently the oldip and newip variables are not even used from the template. We may want to remove it completely. No change is expected with this patch, the different authentication methods should still work as before. Test plan: Log in the staff and OPAC interfaces, logout. Log in and call script that call the 3 different subroutines modified by this patch. For instance you can list checkouts (that is using check_cookie_auth) and display a patron's image (using check_api_auth). QA with good knowledge of the C4::Auth module and the different authentication methods is required. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 --- Comment #7 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 123321 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123321&action=edit Bug 28785: Adjust check_cookie_auth calls The previous patch makes check_cookie_auth return the session instead of $sessionID, so we are adjusting the different calls to prevent confusion. However they are mainly used to check the authentication status and don't care about this second variable. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 --- Comment #8 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 123322 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123322&action=edit Bug 28785: Add skip_version_check for checkauth We could have this patch but we also could decide to skip it. The idea is to avoid 2 checks of the version when we are coming from checkauth. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 --- Comment #9 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 123323 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123323&action=edit Bug 28785: Don't send SessionRestrictionByIP to template It's not used. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the| |Sponsored-by: Orex Digital release notes| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #123320|0 |1 is obsolete| | Attachment #123321|0 |1 is obsolete| | Attachment #123322|0 |1 is obsolete| | Attachment #123323|0 |1 is obsolete| | --- Comment #10 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 123392 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123392&action=edit Bug 28785: Centralize cookie auth check in check_cookie_auth This code is duplicated in 3 different places, we must call check_cookie_auth instead. It makes check_cookie_auth returns a 'restricted' when SessionRestrictionByIP is set and the IP changed. It also returns a third parameters contained the old and new IP, to fill the "info" hash in checkauth but apparently the oldip and newip variables are not even used from the template. We may want to remove it completely. No change is expected with this patch, the different authentication methods should still work as before. Test plan: Log in the staff and OPAC interfaces, logout. Log in and call script that call the 3 different subroutines modified by this patch. For instance you can list checkouts (that is using check_cookie_auth) and display a patron's image (using check_api_auth). QA with good knowledge of the C4::Auth module and the different authentication methods is required. Signed-off-by: Owen Leonard <oleonard@myacpl.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 --- Comment #11 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 123393 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123393&action=edit Bug 28785: Adjust check_cookie_auth calls The previous patch makes check_cookie_auth return the session instead of $sessionID, so we are adjusting the different calls to prevent confusion. However they are mainly used to check the authentication status and don't care about this second variable. Signed-off-by: Owen Leonard <oleonard@myacpl.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 --- Comment #12 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 123394 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123394&action=edit Bug 28785: Add skip_version_check for checkauth We could have this patch but we also could decide to skip it. The idea is to avoid 2 checks of the version when we are coming from checkauth. Signed-off-by: Owen Leonard <oleonard@myacpl.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 --- Comment #13 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 123395 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123395&action=edit Bug 28785: Don't send SessionRestrictionByIP to template It's not used. Signed-off-by: Owen Leonard <oleonard@myacpl.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 --- Comment #14 from David Cook <dcook@prosentient.com.au> --- Comment on attachment 123393 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123393 Bug 28785: Adjust check_cookie_auth calls Review of attachment 123393: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=28785&attachment=123393) ----------------------------------------------------------------- Why do this patch instead of having check_cookie_auth return the sessionID instead of the session? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 --- Comment #15 from David Cook <dcook@prosentient.com.au> --- Comment on attachment 123393 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123393 Bug 28785: Adjust check_cookie_auth calls Review of attachment 123393: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=28785&attachment=123393) ----------------------------------------------------------------- Why do this patch instead of having check_cookie_auth return the sessionID instead of the session? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #16 from David Cook <dcook@prosentient.com.au> --- Excited to see this change overall. At some point, I would love to rewrite checkauth... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #123392|0 |1 is obsolete| | --- Comment #17 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 124776 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=124776&action=edit Bug 28785: Centralize cookie auth check in check_cookie_auth This code is duplicated in 3 different places, we must call check_cookie_auth instead. It makes check_cookie_auth returns a 'restricted' when SessionRestrictionByIP is set and the IP changed. It also returns a third parameters contained the old and new IP, to fill the "info" hash in checkauth but apparently the oldip and newip variables are not even used from the template. We may want to remove it completely. No change is expected with this patch, the different authentication methods should still work as before. Test plan: Log in the staff and OPAC interfaces, logout. Log in and call script that call the 3 different subroutines modified by this patch. For instance you can list checkouts (that is using check_cookie_auth) and display a patron's image (using check_api_auth). QA with good knowledge of the C4::Auth module and the different authentication methods is required. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #123393|0 |1 is obsolete| | --- Comment #18 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 124777 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=124777&action=edit Bug 28785: Adjust check_cookie_auth calls The previous patch makes check_cookie_auth return the session instead of $sessionID, so we are adjusting the different calls to prevent confusion. However they are mainly used to check the authentication status and don't care about this second variable. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #123394|0 |1 is obsolete| | --- Comment #19 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 124778 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=124778&action=edit Bug 28785: Add skip_version_check for checkauth We could have this patch but we also could decide to skip it. The idea is to avoid 2 checks of the version when we are coming from checkauth. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #123395|0 |1 is obsolete| | --- Comment #20 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 124779 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=124779&action=edit Bug 28785: Don't send SessionRestrictionByIP to template It's not used. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #21 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Always takes a while to wrap my brain around Auth again.. All seems solid in testing and there's certainly a code improvement here. Tests pass, QA script happy, QA person happy. Passing QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |21.11.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 --- Comment #22 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Pushed to master for 21.11, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |RESOLVED CC| |kyle@bywatersolutions.com Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 --- Comment #23 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Did this patch set (inadvertently) have the side-effect of no longer allowing to return to previous page when hitting an unauthorized one ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 --- Comment #24 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Marcel de Rooy from comment #23)
Did this patch set (inadvertently) have the side-effect of no longer allowing to return to previous page when hitting an unauthorized one ?
Possible in 20.11, not on master (before 29914 and friends). It would be my first suspect. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28785 --- Comment #25 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #6)
QA with good knowledge of the C4::Auth module and the different authentication methods is required.
Certainly :) -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org