Bug 16610 - Regression in SIP2 user password handling
Summary: Regression in SIP2 user password handling
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 14507
Blocks:
  Show dependency treegraph
 
Reported: 2016-05-27 12:38 UTC by Kyle M Hall
Modified: 2019-06-27 09:24 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 16610 - Regression in SIP2 user password handling (1.53 KB, patch)
2016-05-27 12:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16610 - Regression in SIP2 user password handling (1.59 KB, patch)
2016-05-27 13:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16610: [QA Follow-up] Add a test (3.19 KB, patch)
2016-06-01 16:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16610 - Regression in SIP2 user password handling (1.68 KB, patch)
2016-06-01 16:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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