Bug 18755 - Allow empty password fields in Patron Info requests
Summary: Allow empty password fields in Patron Info requests
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Colin Campbell
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 18943
  Show dependency treegraph
 
Reported: 2017-06-08 11:27 UTC by Colin Campbell
Modified: 2017-07-31 12:00 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Some SIP devices expect an empty password field in a patron info request to be accepted as OK by the server. Since patch for bug 16610 was applied this is not the case. This reinstates the old behaviour for sip logins with the parameter allow_empty_passwords="1"


Attachments
Patch enabling the option (2.59 KB, patch)
2017-06-08 12:18 UTC, Colin Campbell
Details | Diff | Splinter Review
Bug 18755 Allow empty passwords in Patron Info to return OK (2.64 KB, patch)
2017-07-01 00:22 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 18755: Allow empty passwords in Patron Info to return OK (2.74 KB, patch)
2017-07-05 11:12 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 Colin Campbell 2017-06-08 11:27:22 UTC
Bug 16610 changed the behaviour of patron info requests and responses - previously if the password field was empty in the request, we returned password ok. Many sip clients assume that passing an empty pwd field means "I'm not validating a password - I just want to know patron is valid" and are failing because of the change in behaviour.

As both scenarios are valid I suggest adding a parameter to the sip config to allow empty password fields to be treated as previously. To my knowledge this is only an issue in the patron info response.
Comment 1 Colin Campbell 2017-06-08 12:18:57 UTC
Created attachment 64101 [details] [review]
Patch enabling the option

To test send a patron info request with the patron password field (AD) containing no characters. Default returns CQ = N and 'Invalid password' in the screen message. Alter config file to add allow_empty_passwords="1" to the appropriate login parameters in accounts. Restart the sipserver to reread the config and repeat the request CQ is now returned as "Y" and the screen message does not report the password as invalid
Comment 2 M. Tompsett 2017-07-01 00:22:04 UTC
I figured the test added should fail under master, so:
git bz apply 18755
git checkout origin/master -- C4/SIP/Sip/MsgType.pm
prove t/db_dependent/SIP/Message.t
-- fails as expected
git reset --hard origin/master
git bz apply 18755
prove t/db_dependent/SIP/Message.t
-- passes as expected.

This change is fully tested by the test that is added, so I'm going to sign off.
Comment 3 M. Tompsett 2017-07-01 00:22:57 UTC
Created attachment 64762 [details] [review]
Bug 18755 Allow empty passwords in Patron Info to return OK

With this patch a parameter 'allow_empty_passwords="1" can be added to a
login in the SIP configuration file to allow the behaviour as was normal
before the patch for bug 16610 was applied. Some sip clients rely on
this behaviour sending an empty password field when they wish to
validate to user but do not have the password.
If a password is supplied it will be validated

A test has been added to Message.t to confirm this behaviour

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 4 Marcel de Rooy 2017-07-03 09:59:55 UTC
Colin: What about Patron Status now?

If you provide no AD field, Patron Status responds with a CQN (without Invalid
password). Should it return a CQ?
Same for empty AD; receive CQN. Should the new option change behavior here? 

Note:
Patron Info without AD does not return a CQ. (Should it?)
And an empty AD with allow_e_p returns a CQY.
Comment 5 Colin Campbell 2017-07-04 15:24:08 UTC
(In reply to Marcel de Rooy from comment #4)
> Colin: What about Patron Status now?
> 
> If you provide no AD field, Patron Status responds with a CQN (without
> Invalid
> password). Should it return a CQ?
> Same for empty AD; receive CQN. Should the new option change behavior here? 
> 
> Note:
> Patron Info without AD does not return a CQ. (Should it?)
> And an empty AD with allow_e_p returns a CQY.

Patron status is a bit odd the CQ is like patron info in being an optional field, but unlike patron info the AD password field is a required field so I assume that a missing AD should provoke an invalid password in return. basically the behaviour has remained unchanged. In practice I cant recall seeing any kind of unit that uses patron status, I think when sip 2 was released patron info effectively superceded it. So I think we should leave functionality as is, just in case anything relies on the current behaviour. ( I tested on a very old sip software before any of the patches affecting patron info to confirm that behaviour hadnt been inadvertantly changed by other patches)
Comment 6 Marcel de Rooy 2017-07-05 11:12:33 UTC
Created attachment 64797 [details] [review]
Bug 18755: Allow empty passwords in Patron Info to return OK

With this patch a parameter 'allow_empty_passwords="1" can be added to a
login in the SIP configuration file to allow the behaviour as was normal
before the patch for bug 16610 was applied. Some sip clients rely on
this behaviour sending an empty password field when they wish to
validate to user but do not have the password.
If a password is supplied it will be validated

A test has been added to Message.t to confirm this behaviour

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2017-07-05 11:13:27 UTC
(In reply to Colin Campbell from comment #5)
> (In reply to Marcel de Rooy from comment #4)
> Patron status is a bit odd the CQ is like patron info in being an optional
> field, but unlike patron info the AD password field is a required field so I
> assume that a missing AD should provoke an invalid password in return.
> basically the behaviour has remained unchanged. In practice I cant recall
> seeing any kind of unit that uses patron status, I think when sip 2 was
> released patron info effectively superceded it. So I think we should leave
> functionality as is, just in case anything relies on the current behaviour.
> ( I tested on a very old sip software before any of the patches affecting
> patron info to confirm that behaviour hadnt been inadvertantly changed by
> other patches)

Clear enough. Thanks.
Comment 8 Katrin Fischer 2017-07-06 06:05:40 UTC
> With this patch a parameter 'allow_empty_passwords="1" can be added...

Can we document these kinds of parameters somewhere? Maybe in the example file? I am worried that these kind of features will remain mostly unknown if there are not more clues.
Comment 9 Colin Campbell 2017-07-06 08:02:51 UTC
(In reply to Katrin Fischer from comment #8)
> > With this patch a parameter 'allow_empty_passwords="1" can be added...
> 
> Can we document these kinds of parameters somewhere? Maybe in the example
> file? I am worried that these kind of features will remain mostly unknown if
> there are not more clues.

I wonder if we need to rethink the way SIP configuration is held. I'm not sure that adding options to an xml file is very accessible to many users.
Comment 10 Katrin Fischer 2017-07-06 09:03:03 UTC
I agree, but that's probably a bigger issue. I am not sure how granular it is, but I think possibly it should be as granular as per 'sip2 using unit'?
Having a commented example file might be quicker to achieve for now and helpful for moving things later.
Comment 11 Jonathan Druart 2017-07-06 18:50:49 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 12 Fridolin SOMERS 2017-07-12 09:19:48 UTC
Pushed to 17.05.x, will be in 17.05.02
Comment 13 Katrin Fischer 2017-07-16 11:47:53 UTC
I've filed bug 18943 about the missing documentation.

This patch has been pushed to 16.11.x and will be in 16.11.10.
Comment 14 Mason James 2017-07-31 12:00:55 UTC
Pushed to 16.05.x, for 16.05.15 release