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.
Created attachment 116243 [details] [review] 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.
Created attachment 116245 [details] [review] 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.
Created attachment 116246 [details] [review] 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.
Should this still be marked as NEW and not Needs Signoff?
Oops!
Still intending to test this one Kyle?
Created attachment 120266 [details] [review] 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>
Created attachment 120267 [details] [review] 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>
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?
(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.
(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.
Created attachment 120796 [details] [review] 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>
Created attachment 120797 [details] [review] 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>
Created attachment 120798 [details] [review] 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
Created attachment 120799 [details] [review] 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
`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 :)
Created attachment 120829 [details] [review] 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>
Created attachment 120830 [details] [review] 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>
Small code changes, covered by tests, moving to PQA
Pushed to master for 21.11, thanks to everybody involved!
Pushed to 21.05.x for 21.05.02
Pushed to 20.11.x for 20.11.09
Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed.