Bug 27600

Summary: SIP2: renew_all shouldn't perform a password check
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: SIP2Assignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: minor    
Priority: P5 - low CC: colin.campbell, dcook, fridolin.somers, jon.turner, joonas.kylmala, kyle, nick, victor
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18755
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28311
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28822
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.02,20.11.09
Attachments: Bug 27600: Remove password check from SIP2 renew_all
Bug 27600: Add unit test
Bug 27600: Remove password check from SIP2 renew_all
Bug 27600: Add unit test
Bug 27600: Remove password check from SIP2 renew_all
Bug 27600: Add unit test
Bug 27600: Remove password check from SIP2 `renew_all`
Bug 27600: Remove password check from `cancel_hold`
Bug 27600: Remove password check from `add_hold`
Bug 27600: Remove password check from `cancel_hold`
Bug 27600: Remove password check from `add_hold`

Description Martin Renvoize 2021-02-03 10:01:05 UTC
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.
Comment 1 Martin Renvoize 2021-02-03 10:05:42 UTC
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.
Comment 2 Martin Renvoize 2021-02-03 10:18:20 UTC
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.
Comment 3 Martin Renvoize 2021-02-03 10:18:22 UTC
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.
Comment 4 Kyle M Hall 2021-02-03 13:02:10 UTC
Should this still be marked as NEW and not Needs Signoff?
Comment 5 Martin Renvoize 2021-02-03 13:31:15 UTC
Oops!
Comment 6 Martin Renvoize 2021-03-16 14:44:48 UTC
Still intending to test this one Kyle?
Comment 7 Nick Clemens 2021-04-28 11:50:53 UTC
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>
Comment 8 Nick Clemens 2021-04-28 11:50:59 UTC
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>
Comment 9 Nick Clemens 2021-04-28 11:54:12 UTC
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?
Comment 10 Martin Renvoize 2021-05-05 08:18:46 UTC
(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.
Comment 11 Kyle M Hall 2021-05-05 11:40:02 UTC
(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.
Comment 12 Martin Renvoize 2021-05-10 15:34:35 UTC
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>
Comment 13 Martin Renvoize 2021-05-10 15:34:39 UTC
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>
Comment 14 Martin Renvoize 2021-05-10 15:34:43 UTC
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
Comment 15 Martin Renvoize 2021-05-10 15:34:47 UTC
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
Comment 16 Martin Renvoize 2021-05-10 15:43:09 UTC
`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 :)
Comment 17 Nick Clemens 2021-05-11 10:15:12 UTC
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>
Comment 18 Nick Clemens 2021-05-11 10:15:16 UTC
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>
Comment 19 Nick Clemens 2021-05-11 10:15:56 UTC
Small code changes, covered by tests, moving to PQA
Comment 20 Jonathan Druart 2021-06-21 12:03:51 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 21 Kyle M Hall 2021-07-02 13:45:01 UTC
Pushed to 21.05.x for 21.05.02
Comment 22 Fridolin Somers 2021-08-02 20:04:31 UTC
Pushed to 20.11.x for 20.11.09
Comment 23 Victor Grousset/tuxayo 2021-08-12 21:12:30 UTC
Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed.