Bug 16694 - Limit SIP2 auth by patron attribute
Summary: Limit SIP2 auth by patron attribute
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Magnus Enger
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-06-08 13:20 UTC by Magnus Enger
Modified: 2020-11-26 18:30 UTC (History)
7 users (show)

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


Attachments
Bug 16694 - Limit SIP2 auth by patron attribute (3.74 KB, patch)
2016-08-26 10:41 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 16694 - Limit SIP2 auth by patron attribute (3.75 KB, patch)
2016-10-06 08:38 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 16694 - Limit SIP2 auth by patron attribute (3.91 KB, patch)
2016-10-13 20:46 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 16694 - Limit SIP2 auth by patron attribute (4.26 KB, patch)
2016-10-17 11:30 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 16694 - Limit SIP2 auth by patron attribute (4.19 KB, patch)
2017-09-05 12:14 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 18137: (QA followup) Make sure the session exists and is expired on expiration tests (1.38 KB, patch)
2017-09-05 12:18 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 18137: (QA followup) Make sure the session exists and is expired on expiration tests (1.38 KB, patch)
2017-09-05 12:19 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 16694 - Limit SIP2 auth by patron attribute (4.19 KB, patch)
2017-09-05 12:19 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 16694 - Add tests to t/db_dependent/SIP/Message.t (4.03 KB, patch)
2017-09-05 12:19 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 16694 - Add tests to t/db_dependent/SIP/Message.t (4.11 KB, patch)
2017-10-11 13:47 UTC, Owen Leonard
Details | Diff | Splinter Review
Patch for C4/SIP/Sip/MsgType.pm (1.63 KB, patch)
2018-10-17 12:21 UTC, Mark Alexander
Details | Diff | Splinter Review
Relevant code from patron validator plugin (1.09 KB, text/plain)
2018-10-17 12:24 UTC, Mark Alexander
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Magnus Enger 2016-06-08 13:20:06 UTC
In Scandinavia "more open" (Swedish: "Meröppet", Norwegian: "Meråpent") is a hot issue at the moment. What this means is that patrons can use their card and password/PIN to lock themselves into the library even when staff is not around. Typically, this means that patrons can get access to the library building from 7 am to 11 pm, even if the library is only staffed from 8 am to 5 pm. 

Typically, SIP2 is used to communnicate between the "more open"-system and the ILS, specifically to authenticate the user with a username and password/PIN. 

Also, you want the ability to turn "more open" access on and off for patrons. An extended patron attribute with possible values of NULL/empty, 0 or 1 should serve well for this. 

SIP2 does not have a field for saying "this is a moreopen login", so we need a way to identify a SIP2 user as a moreopen user. My plan is to add another attribute to the login element in SIPConfig.xml to achieve this:

<login id="koha"   password="koha"  delimiter="|" error-detect="enabled" institution="kohalibrary" patron-attribute="MEROPPET" />

"MEROPPET" is then the name of an extended patron attribute which can be set to NULL or 0 (patron should not have access to moreopen) or 1 (patron should have access).

This should be flexible enough to allow other, similar schemes to use some other patron attribute.
Comment 1 Benjamin Rokseth 2016-08-26 10:41:32 UTC Comment hidden (obsolete)
Comment 2 Benjamin Rokseth 2016-10-06 08:38:57 UTC Comment hidden (obsolete)
Comment 3 Magnus Enger 2016-10-11 15:21:16 UTC Comment hidden (obsolete)
Comment 4 Magnus Enger 2016-10-11 15:24:41 UTC Comment hidden (obsolete)
Comment 5 Benjamin Rokseth 2016-10-13 20:46:43 UTC Comment hidden (obsolete)
Comment 6 Benjamin Rokseth 2016-10-13 20:50:36 UTC Comment hidden (obsolete)
Comment 7 Magnus Enger 2016-10-17 11:30:36 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2017-01-27 10:01:34 UTC
Would be nice to validate this small change with a small test in e.g. t/db_dependent/SIP/Message.t
We already have a test there for patron info.
Should not be big deal :)
Comment 9 Magnus Enger 2017-04-20 18:39:04 UTC
Benjamin: Feel like writing that test?
Comment 10 Benjamin Rokseth 2017-09-05 12:14:33 UTC Comment hidden (obsolete)
Comment 11 Benjamin Rokseth 2017-09-05 12:15:35 UTC
rebased against master.

Ignoring failing test 'Checkin V2' as it has no bearing on this bug and should be resolved in another bug
Comment 12 Benjamin Rokseth 2017-09-05 12:18:05 UTC Comment hidden (obsolete)
Comment 13 Benjamin Rokseth 2017-09-05 12:19:01 UTC Comment hidden (obsolete)
Comment 14 Benjamin Rokseth 2017-09-05 12:19:41 UTC
Created attachment 66848 [details] [review]
Bug 16694 - Limit SIP2 auth by patron attribute

The main use case of this bug is to use patron attributes to grant special
privileges, e.g. to open a door to an unmanned library.

This patch adds an extra check against patron attributes if login account
in SIPconfig.xml has a key validate_patron_attribute set to some patron attribute.

If a patron information request is sent (63), and patron has proper rights in the
given attribute: (a value of 1/true or some authorised value mapping to 1)
The user will be allowed access (in SIP: charge and/or renewal ok).
Otherwise denied.

Please note that this is specific to the SIP login account, so self checkout
machines can be handled differently than e.g. a door card terminal.

To test:
0) you need to debug using telnet or the koha provided sip_client
1) add validate_patron_attribute="testattribute" to some login account in SIPconfig.xml
2) add a patron attribute "testattribute"
3) edit some patron and set "testattribute" to "1"
4) do a sip login with the given login account from SIPconfig.xml
5) do a patron information request (63) on the patron
6) observe that no charge or renewal denied is given in the response (64  )
7) try all or any of the following:
 - set patron attribute to anything but "1"
 - delete the patron attribute
 - map the patron attribute to an authorized list, e.g. (YES_NO) and
   set it to a value that doesn't map to "1", e.g. "No".
8) do a patron information request (63) again
9) observe that charge and renewal is now denied in the SIP response (64YY)
10) thank yourself if noone else does and grab a coffee

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Took me a while to remember I was on a gitified setup and needed to do
sudo cp C4/SIP/Sip/MsgType.pm /usr/share/koha/lib/C4/SIP/Sip/MsgType.pm
before I could test properly. Works as expected. I have a Swedish customer
running a similar hack in production, so looking forward to getting this
into Koha proper.
Comment 15 Benjamin Rokseth 2017-09-05 12:19:54 UTC
Created attachment 66849 [details] [review]
Bug 16694 - Add tests to t/db_dependent/SIP/Message.t
Comment 16 Marcel de Rooy 2017-09-05 12:22:56 UTC
(In reply to Benjamin Rokseth from comment #11)
> rebased against master.
> 
> Ignoring failing test 'Checkin V2' as it has no bearing on this bug and
> should be resolved in another bug

The test passes with me btw. Please copy your test results in a comment.
Comment 17 Benjamin Rokseth 2017-09-05 12:24:13 UTC
needs another signoff on added tests
Comment 18 Benjamin Rokseth 2017-09-05 12:31:16 UTC
(In reply to Marcel de Rooy from comment #16)
> (In reply to Benjamin Rokseth from comment #11)
> > rebased against master.
> > 
> > Ignoring failing test 'Checkin V2' as it has no bearing on this bug and
> > should be resolved in another bug
> 
> The test passes with me btw. Please copy your test results in a comment.

Good. May be I have corrupted my DB somehow from testing various tests.
Seems there are problems on creating items

  resensitize(): no item found in object to resensitize at 
  C4/SIP/ILS/Transaction/Checkin.pm line 127.
    
    #   Failed test 'Check screen msg'
    #   at ./t/db_dependent/SIP/Message.t line 448.
    #          got: ''
    #     expected: '1'
  new ILS::Item(Kckdb) : No item 'Kckdb'. at C4/SIP/ILS/Item.pm line 80.
Comment 19 Owen Leonard 2017-10-11 13:47:40 UTC
Created attachment 67956 [details] [review]
Bug 16694 - Add tests to t/db_dependent/SIP/Message.t

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Test passes with no errors
Comment 20 Marcel de Rooy 2017-10-13 08:05:29 UTC
QA: Looking here now
Comment 21 Marcel de Rooy 2017-10-13 08:31:44 UTC
QA Comment:
Thanks for adding the test.
I am not yet fully comfortable with the code added to MsgType.pm. I would rather have the validation in Patron.pm and just calling $patron->validate_or_something from MsgType.

Also the current code needs additional checking.
If a patron is already debarred etc, so charge_ok is already 0, we should not set charge_ok to 1 only because the validation attribute is present.

Another point (no blocker, but adding to discussion): Is the test attr==1 not too simple? Could we allow something like validate_patron="opendoor=Y" or even two expr like validate_patron="opendoor=Y;categorycode=X" etc.? Could be a future extension..

Copying Coling: Do you have additional feedback ?

Changing status
Comment 22 Mark Alexander 2018-10-16 23:38:53 UTC
Our library needs a similar feature, but our use case is more complicated.

We use two different services that provide streamed movies and ebooks, respectively.  So our patron validation code would look at the client IP address on the SIP2 "patron info" or "patron status" request to determine which of the two services made the request.  Then it would use the appropriate criteria for that service to validate the patron, perhaps using a different patron attribute for each service.

Right now I've hacked the SIP2 server for this purpose, but it might make more sense for the validation code to be placed in a plugin, to allow it to be customized for each library's special requirements.  I'll try working on a patch for this.
Comment 23 Mark Alexander 2018-10-17 12:21:06 UTC
Created attachment 80702 [details] [review]
Patch for C4/SIP/Sip/MsgType.pm

Attached is patch to C4/SIP/Sip/MsgType.pm that uses a plugin to
validate patrons.  It requires a plugin_class attribute in the account
section of SIPconfig.xml; in my test it looked like this:

  <accounts>
      <login id="kanopy" password="**password**" delimiter="|"
        error-detect="enabled"
        institution="RPL"
        plugin_class="Koha::Plugin::Com::Bloovis::Sip2patron" />
  </accounts>

I will also attach the relevant code from the plugin separately.

I have no idea if this is the right approach but it's enough to get our library working with Kanopy.
Comment 24 Mark Alexander 2018-10-17 12:24:12 UTC
Created attachment 80703 [details]
Relevant code from patron validator plugin

This is my test plugin for validating SIP2 patrons, with the boilerplate code stripped out.
Comment 25 Colin Campbell 2018-10-22 13:17:27 UTC
patron should not be being set to undef in MsgType - this could result in a failed connection. Patron is still supposed to be returned but patron status flagged
Comment 26 Colin Campbell 2018-10-22 13:28:23 UTC
I find it a bit concerning that the "more open" functionality is relying on as insecure and abusable protocol as sip2. I trust that some additional security is added to the connection but it does suggest that things are completely open rather than more open
Comment 27 Magnus Enger 2018-10-22 13:30:33 UTC
(In reply to Colin Campbell from comment #26)
> I find it a bit concerning that the "more open" functionality is relying on
> as insecure and abusable protocol as sip2. I trust that some additional
> security is added to the connection but it does suggest that things are
> completely open rather than more open

That decision is in the hands of the companies providing the physical "more open" machinery.
Comment 28 Mark Alexander 2018-10-22 23:51:33 UTC
(In reply to Colin Campbell from comment #25)
> patron should not be being set to undef in MsgType - this could result in a
> failed connection. Patron is still supposed to be returned but patron status
> flagged

I am confused by this.  I see that find_patron does sometimes return undef, and that is tested in build_patron_status, handle_block_patron, handle_patron_info, and handle_patron_enable.  In those functions, undef seems to indicate an invalid patron.  So I thought it would be safe to emulate that behavior.
Comment 29 Magnus Enger 2020-11-19 14:19:27 UTC
Hm, I think I agree that a plugin might make sense here. But can we agree on how to signal that the patron should be denied access? The vendors of "more open" that I have worked with expect a SIP2 response that has us using this to set the proper status:

$patron->{'charge_ok'}

Could we pass $patron to the plugin and let the plugin do what it needs to it, before returning it? So instead of this (as suggested by Mark):

$patron = undef unless ( check_patron ( $patron, $server ) );

we could do this? 

$patron = check_patron ( $patron, $server );

And the plugin could return undef or just a changed $patron. 

I would also suggest a more detailed name for the config-parameter: 

  <accounts>
      <login id="kanopy" password="**password**" delimiter="|"
        error-detect="enabled"
        institution="RPL"
        plugin_class="Koha::Plugin::Com::Bloovis::Sip2patron" />
  </accounts>

Maybe something like patron_validation_plugin_class? There could be other plugins that do other things in the future.
Comment 30 Mark Alexander 2020-11-26 18:30:31 UTC
(In reply to Magnus Enger from comment #29)
> Could we pass $patron to the plugin and let the plugin do what it needs to
> it, before returning it? So instead of this (as suggested by Mark):
> 
> $patron = undef unless ( check_patron ( $patron, $server ) );
> 
> we could do this? 
> 
> $patron = check_patron ( $patron, $server );
> 
> And the plugin could return undef or just a changed $patron. 
> 
> I would also suggest a more detailed name for the config-parameter: 
> 
>   <accounts>
>       <login id="kanopy" password="**password**" delimiter="|"
>         error-detect="enabled"
>         institution="RPL"
>         plugin_class="Koha::Plugin::Com::Bloovis::Sip2patron" />
>   </accounts>
> 
> Maybe something like patron_validation_plugin_class? There could be other
> plugins that do other things in the future.

I like these suggestions. I have made these changes to my plugin and MsgType.pm on my test setup, and they work as expected.