Bug 16610

Summary: Regression in SIP2 user password handling
Product: Koha Reporter: Kyle M Hall <kyle>
Component: SIP2Assignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: major    
Priority: P5 - low CC: brendan, colin.campbell, f.demians, julian.maurice, m.de.rooy, stefan.berndtsson
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18755
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 14507    
Bug Blocks:    
Attachments: Bug 16610 - Regression in SIP2 user password handling
[SIGNED-OFF] Bug 16610 - Regression in SIP2 user password handling
Bug 16610: [QA Follow-up] Add a test
Bug 16610 - Regression in SIP2 user password handling

Description Kyle M Hall 2016-05-27 12:38:27 UTC
Previous to bug 14507, SIP2 only did internal authentication. A change to the way we check empty passwords has caused any empty password to send back a CQ of Y. Previous to that patch set, a CQ of Y would only be sent back of the patron password column was NULL. Now, an empty AD field *always* returns a CQ of Y.
Comment 1 Kyle M Hall 2016-05-27 12:44:35 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2016-05-27 13:48:07 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2016-06-01 16:09:11 UTC
Created attachment 51946 [details] [review]
Bug 16610: [QA Follow-up] Add a test

To verify the original patch, this test shows that before applying
it the Patron Info request did not return CQ==N for an empty
password. Note that the Patron Status did btw.

After applying the original patch, the test passes for patron
info as well as status.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 4 Marcel de Rooy 2016-06-01 16:09:15 UTC
Created attachment 51947 [details] [review]
Bug 16610 - Regression in SIP2 user password handling

Previous to bug 14507, SIP2 only did internal authentication. A change
to the way we check empty passwords has caused any empty password to
send back a CQ of Y. Previous to that patch set, a CQ of Y would only be
sent back of the patron password column was NULL. Now, an empty AD field
*always* returns a CQ of Y.

Test Plan:
1) Send a patron information request with an empty AD field
   Note: You must send the AD field or you won't get back a CQ field
2) Note you get back a CQ of Y
3) Apply this patch
4) Repeat step 1
5) Note you now get back a CQ of N

Signed-off-by: Trent Roby <troby@bclib.info>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 5 Brendan Gallagher 2016-06-03 06:39:39 UTC
Pushed to Master - Should be in the November 2016 release. Thanks!
Comment 6 Frédéric Demians 2016-06-15 07:33:56 UTC
Pushed in 16.05. Will be in 16.05.01.
Comment 7 Julian Maurice 2016-06-16 07:56:34 UTC
Patch pushed to 3.22.x, will be in 3.22.8
Comment 8 Stefan Berndtsson 2017-03-30 12:43:58 UTC
> Previous to bug 14507, SIP2 only did internal authentication. A change
> to the way we check empty passwords has caused any empty password to
> send back a CQ of Y. Previous to that patch set, a CQ of Y would only be
> sent back of the patron password column was NULL. Now, an empty AD field
> *always* returns a CQ of Y.

We recently began testing our selfcheck terminals with Koha. This causes an issue with our setup. The machines we use (Tor3 from Bibliotheca) has an option for disabling pincode/password entry (which is the situation we want). However, this does not remove the AD fields from the transaction, just sends an empty AD. I have no idea how common this behaviour is, but it seems to be similar to the issue mentioned in bug 15124.

We also need the users to have a password field for web access, so we cannot leave that field empty.

The combined consequence of this is that getting a CQN back makes it impossible to use the selfcheck system. Changing the way the selfcheck system creator implements their things is not really something we can do.

This whole thing could be made into a syspref, allowing for empty AD fields.

I'm not sure how to proceed with this, since this is a resolved bug. Should adding a syspref for this be done using a new bug entry?
Comment 9 Colin Campbell 2017-06-08 09:36:19 UTC
Large numbers of units send empty password fields if they do not want the password to be checked. This patch causes them to fail
Comment 10 Colin Campbell 2017-06-08 09:47:09 UTC
As this changes how the sipserver has worked in the past and in a way incompatible with many clients it should really be governed by a config optin in the sip configuration file.
Comment 11 Colin Campbell 2017-06-08 11:30:16 UTC
I have added a bug 18755 to allow the old behaviour on setting an option  in the config of the associated sip account