[Bug 27600] New: renew_all shouldn't perform a password check
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 Bug ID: 27600 Summary: renew_all shouldn't perform a password check Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: SIP2 Assignee: koha-bugs@lists.koha-community.org Reporter: martin.renvoize@ptfs-europe.com QA Contact: testopia@bugs.koha-community.org Authentication should have already taken place at the SIP2 selfcheck machine prior to the call to renew_all and as such should not require re-checking within the call itself. This can cause issues as it's common practice to pass along an empty AD field in the subsequent SIP requests and that defined but empty field can cause an error instead of performing the renewal. We should mimic the code in 'renew' which checks for the patron but does not check for a correct password. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 --- Comment #1 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 116243 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=116243&action=edit Bug 27600: Remove password check from SIP2 renew_all This patch removes the password check code from the renew_all method for SIP2. This is required, as many units send an empty AD field in the SIP message which can cause the transaction to fail.. but the authentication should have already taken place earlier in the session. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 --- Comment #2 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 116245 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=116245&action=edit Bug 27600: Add unit test This patch adds a basic unit test to catch the case where patron_pwd is sent to renew_all as an empty string. Test plan 1/ Run t/db_dependent/SIP/ILS.t and watch it fail 2/ Apply second patch 3/ Re-run the above test and watch it pass. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #116243|0 |1 is obsolete| | --- Comment #3 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 116246 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=116246&action=edit Bug 27600: Remove password check from SIP2 renew_all This patch removes the password check code from the renew_all method for SIP2. This is required, as many units send an empty AD field in the SIP message which can cause the transaction to fail.. but the authentication should have already taken place earlier in the session. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |martin.renvoize@ptfs-europe |ity.org |.com -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |colin.campbell@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=27600 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jon.turner@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=27600 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Trivial patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 --- Comment #4 from Kyle M Hall <kyle@bywatersolutions.com> --- Should this still be marked as NEW and not Needs Signoff? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff --- Comment #5 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Oops! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|renew_all shouldn't perform |SIP2: renew_all shouldn't |a password check |perform a password check -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 --- Comment #6 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Still intending to test this one Kyle? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |joonas.kylmala@helsinki.fi -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 Nick Clemens <nick@bywatersolutions.com> 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=27600 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #116245|0 |1 is obsolete| | --- Comment #7 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 120266 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120266&action=edit Bug 27600: Add unit test This patch adds a basic unit test to catch the case where patron_pwd is sent to renew_all as an empty string. Test plan 1/ Run t/db_dependent/SIP/ILS.t and watch it fail 2/ Apply second patch 3/ Re-run the above test and watch it pass. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #116246|0 |1 is obsolete| | --- Comment #8 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 120267 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120267&action=edit Bug 27600: Remove password check from SIP2 renew_all This patch removes the password check code from the renew_all method for SIP2. This is required, as many units send an empty AD field in the SIP message which can cause the transaction to fail.. but the authentication should have already taken place earlier in the session. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com --- Comment #9 from Nick Clemens <nick@bywatersolutions.com> --- Signed off following test plan, but I have two questions: 1 - add_hold and cancel_hold have the same check - is that also invalid? 2 - There is the config 'allow_empty_password' - should we not remove these checks, but support that config? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=18755 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 --- Comment #10 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- (In reply to Nick Clemens from comment #9)
Signed off following test plan, but I have two questions: 1 - add_hold and cancel_hold have the same check - is that also invalid? 2 - There is the config 'allow_empty_password' - should we not remove these checks, but support that config?
Good questions.. 1. We've only seen the issue presented with the renew_all call, however I did wonder the same myself. I think it might be a slight peculiarity in how the machines at the other end are coded. 2. Hmm, I hadn't really considered allow_empty_password.. I'll take a look, I don't know enough to comment yet. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 --- Comment #11 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to Nick Clemens from comment #9)
Signed off following test plan, but I have two questions: 1 - add_hold and cancel_hold have the same check - is that also invalid? 2 - There is the config 'allow_empty_password' - should we not remove these checks, but support that config?
I've gone back and forth on this multiple times in while reading the SIP spec. I think the key part is that yes, all 3 of these SIP messages have the patron password as part of the spec, but for all of them it is *optional*. In addition, the 3 messages we are looking at don't *do* anything with the patron password. Only patron data and patron info return the "password valid" field. As such my personal opinion the proper thing to do would be to just remove the same checkes from Add Hold and Cancel Hold. Koha's current before is not defined in the spec, not is *any* behavior defined in the spec of patron passwords for these messages. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #120266|0 |1 is obsolete| | --- Comment #12 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 120796 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120796&action=edit Bug 27600: Add unit test This patch adds a basic unit test to catch the case where patron_pwd is sent to renew_all as an empty string. Test plan 1/ Run t/db_dependent/SIP/ILS.t and watch it fail 2/ Apply second patch 3/ Re-run the above test and watch it pass. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 --- Comment #13 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 120797 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120797&action=edit Bug 27600: Remove password check from SIP2 `renew_all` This patch removes the password check code from the renew_all method for SIP2. This is required, as many units send an empty AD field in the SIP message which can cause the transaction to fail.. but the authentication should have already taken place earlier in the session. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 --- Comment #14 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 120798 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120798&action=edit Bug 27600: Remove password check from `cancel_hold` As suggested in the bugzilla comments, the cancel_hold method also doesn't require the password checking code. Test plan 1/ Run t/db_dependent/SIP/ILS.t and watch it pass -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 --- Comment #15 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 120799 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120799&action=edit Bug 27600: Remove password check from `add_hold` As suggested in the bugzilla comments, the add_hold method also doesn't require the password checking code. Test plan 1/ Run t/db_dependent/SIP/ILS.t and watch it pass -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #120267|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 --- Comment #16 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- `allow_empty_password` is a bit of an odd case.. I can see you're thoughts behind adding it to these calls but then I wonder about that being correct as a generalisation. It looks like it was written with a particular request in mind.. the 'patron_info' one... I find that a little odd actually.. that feels like a call that would more likely want to be behind a password so having to 'allow_empty_password' to get these calls working might open the 'patron_info' request to abuse without meaning to. Happy to discus, but as Kyle has looked into the spec, I've implemented his thoughts here :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=28311 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #120798|0 |1 is obsolete| | --- Comment #17 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 120829 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120829&action=edit Bug 27600: Remove password check from `cancel_hold` As suggested in the bugzilla comments, the cancel_hold method also doesn't require the password checking code. Test plan 1/ Run t/db_dependent/SIP/ILS.t and watch it pass Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #120799|0 |1 is obsolete| | --- Comment #18 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 120830 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120830&action=edit Bug 27600: Remove password check from `add_hold` As suggested in the bugzilla comments, the add_hold method also doesn't require the password checking code. Test plan 1/ Run t/db_dependent/SIP/ILS.t and watch it pass Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #19 from Nick Clemens <nick@bywatersolutions.com> --- Small code changes, covered by tests, moving to PQA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 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=27600 --- Comment #20 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=27600 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |minor -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |Pushed to stable Version(s)|21.11.00 |21.11.00,21.05.02 released in| | --- Comment #21 from Kyle M Hall <kyle@bywatersolutions.com> --- Pushed to 21.05.x for 21.05.02 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |Pushed to oldstable Version(s)|21.11.00,21.05.02 |21.11.00,21.05.02,20.11.09 released in| | CC| |fridolin.somers@biblibre.co | |m --- Comment #22 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to 20.11.x for 20.11.09 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=28822 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27600 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED CC| |victor@tuxayo.net Status|Pushed to oldstable |RESOLVED --- Comment #23 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org