Bug 29264 - SIP config allows use of non-branchcode institution ids causes workers to die without responding
Summary: SIP config allows use of non-branchcode institution ids causes workers to die...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: Kyle M Hall (khall)
QA Contact: Joonas Kylmälä
URL:
Keywords:
Depends on:
Blocks: 29564
  Show dependency tree / graph
 
Reported: 2021-10-18 12:28 UTC by Kyle M Hall (khall)
Modified: 2022-06-06 20:31 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This adds a warning to the logs where a SIP login uses an institution id that is *not* a valid library code. If a SIP login uses an institution with an id that doesn't match a valid branchcode, everything will appear to work, but the SIP worker will die anywhere that Koha gets the branch from the userenv and assumes it is valid. The repercussions of this are that actions such as the checkout message simply die and do not return a response message to the requestor.
Version(s) released in:
21.11.00,21.05.05,20.11.12,20.05.18
Circulation function:

Attachments
Bug 29264: SIP config allows use of non-branchcode institution ids causes workers to die without responding (2.90 KB, patch)
2021-10-18 12:32 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 29264: SIP config allows use of non-branchcode institution ids causes workers to die without responding (2.95 KB, patch)
2021-11-08 08:02 UTC, David Nind 		Details | Diff | Splinter Review
Bug 29264: SIP config allows use of non-branchcode institution ids causes workers to die without responding (3.09 KB, patch)
2021-11-13 13:22 UTC, Joonas Kylmälä 		Details | Diff | Splinter Review
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall (khall) 2021-10-18 12:28:00 UTC 
If is entirely possible to create an SIP institution whose ID does not match a valid branchcode in Koha's SIP config. In fact, Koha's example SIP config contains an example of this ( kohalibrary / kohalibrary2 ).

If a SIP login uses an institution with an id that doesn't match a valid branchcode, everything will appear to work, but the SIP worker will die anywhere that Koha gets the branch from the userenv and assumes it is valid.

The repercussions of this are that actions such as the checkout message simply die and do not return a response message to the requestor.

At the very least, we should output a warning to the SIP log.

I think we should strongly consider disallowing institution ids in the SIP config that do not match valid branchcodes. In this scenario, attempting to start the SIP server should result in a error message with the SIP server exiting immediately.
Comment 1 Kyle M Hall (khall) 2021-10-18 12:32:06 UTC 
Created attachment 126432 [details] [review]
Bug 29264: SIP config allows use of non-branchcode institution ids causes workers to die without responding

If is entirely possible to create an SIP institution whose ID does not match a valid branchcode in Koha's SIP config. In fact, Koha's example SIP config contains an example of this ( kohalibrary / kohalibrary2 ).

If a SIP login uses an institution with an id that doesn't match a valid branchcode, everything will appear to work, but the SIP worker will die anywhere that Koha gets the branch from the userenv and assumes it is valid.

The repercussions of this are that actions such as the checkout message simply die and do not return a response message to the requestor.

At the very least, we should output a warning to the SIP log.

I think we should strongly consider disallowing institution ids in the SIP config that do not match valid branchcodes. In this scenario, attempting to start the SIP server should result in a error message with the SIP server exiting immediately.

Test Plan:
1) Apply this patch
2) Make a sip login that uses an instution whose id is *not* a valid branchcode
3) Start the SIP server
4) Check sip.log, you should see a warning similar to the following:
[2021/10/18 12:18:29] [2068079] [ERROR] ERROR: Institution kohalibrary does does not match a branchcode. This can cause unexpected behavior. C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (220)
Comment 2 David Nind 2021-11-08 08:02:53 UTC 
Created attachment 127425 [details] [review]
Bug 29264: SIP config allows use of non-branchcode institution ids causes workers to die without responding

If is entirely possible to create an SIP institution whose ID does not match a valid branchcode in Koha's SIP config. In fact, Koha's example SIP config contains an example of this ( kohalibrary / kohalibrary2 ).

If a SIP login uses an institution with an id that doesn't match a valid branchcode, everything will appear to work, but the SIP worker will die anywhere that Koha gets the branch from the userenv and assumes it is valid.

The repercussions of this are that actions such as the checkout message simply die and do not return a response message to the requestor.

At the very least, we should output a warning to the SIP log.

I think we should strongly consider disallowing institution ids in the SIP config that do not match valid branchcodes. In this scenario, attempting to start the SIP server should result in a error message with the SIP server exiting immediately.

Test Plan:
1) Apply this patch
2) Make a sip login that uses an instution whose id is *not* a valid branchcode
3) Start the SIP server
4) Check sip.log, you should see a warning similar to the following:
[2021/10/18 12:18:29] [2068079] [ERROR] ERROR: Institution kohalibrary does does not match a branchcode. This can cause unexpected behavior. C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (220)

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 David Nind 2021-11-08 08:10:15 UTC 
Test notes:

- koha-testing-docker has institutions already setup that have invalid branch codes (see /etc/koha/sites/kohadev/SIPconfig.xml - MAIN, kohalibrary and kohalibrary2).

- Before the patch is applied there are no errors in /var/log/koha/kohadev/sip.log after restarting everything (or koha-sip --stop kohadev and koha-sip --start kohadev)

- After the patch is applied and everything restarted, the error messages as per step 4 of the test plan appear in the sip log file.
Comment 4 Joonas Kylmälä 2021-11-13 13:22:13 UTC 
Created attachment 127606 [details] [review]
Bug 29264: SIP config allows use of non-branchcode institution ids causes workers to die without responding

If is entirely possible to create an SIP institution whose ID does not match a valid branchcode in Koha's SIP config. In fact, Koha's example SIP config contains an example of this ( kohalibrary / kohalibrary2 ).

If a SIP login uses an institution with an id that doesn't match a valid branchcode, everything will appear to work, but the SIP worker will die anywhere that Koha gets the branch from the userenv and assumes it is valid.

The repercussions of this are that actions such as the checkout message simply die and do not return a response message to the requestor.

At the very least, we should output a warning to the SIP log.

I think we should strongly consider disallowing institution ids in the SIP config that do not match valid branchcodes. In this scenario, attempting to start the SIP server should result in a error message with the SIP server exiting immediately.

Test Plan:
1) Apply this patch
2) Make a sip login that uses an instution whose id is *not* a valid branchcode
3) Start the SIP server
4) Check sip.log, you should see a warning similar to the following:
[2021/10/18 12:18:29] [2068079] [ERROR] ERROR: Institution kohalibrary does does not match a branchcode. This can cause unexpected behavior. C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (220)

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 5 Jonathan Druart 2021-11-15 11:15:38 UTC 
Shouldn't we then adjust the SIP config file?
Comment 6 Jonathan Druart 2021-11-15 11:39:56 UTC 
Pushed to master for 21.11, thanks to everybody involved!
Comment 7 Kyle M Hall (khall) 2021-11-19 15:50:35 UTC 
Pushed to 21.05.x for 21.05.05
Comment 8 Fridolin Somers 2021-11-19 21:21:34 UTC 
Pushed to 20.11.x for 20.11.12
Comment 9 Victor Grousset/tuxayo 2021-11-21 21:11:51 UTC 
Backported: Pushed to 20.05.x branch for 20.05.18
Comment 10 Wainui Witika-Park 2021-11-23 00:25:48 UTC 
Not backporting to 19.11.x - LTS version will just backport security patches