Add password auth checking to LookupUser service

[Josh Stompro]

Hello, Our state wide ILL resource sharing system asked us if we would like to consider using NCIP for patron authentication instead of using SIP2. Lake Agassiz Regional Library out of Moorhead MN and Minitex in MN, with the mnlink.org resource sharing system.

I think this is done using the LookupUser message which would include an AuthenticationInput entity. One entry for the barcode and then one entry for the password/pin.

Jeff Davis was kind enough to share with me an example of how a custom TADL fork of iNCIPit (NCIP V1) handles it.
https://git.evergreen-ils.org/?p=working/random.git;a=commitdiff;h=11da1d420d7ade35cb5efa37c266a4c76bc3da1b;hp=baf9acde133a0e8f22f699c99e30985130f5d768

It probably needs a config option to specify if all LookupUser request are required to have a pin/password, or if it is just evaluated if it exists in the request.

There is also the AuthenticationPrompt element that seems to be to pass on what fields NCIPServer would want for authentication. That looks like it would be part of the LookupAgencyResponse message. LookupAgency doesn't look like it is currently implemented.

[Jason Stephenson (jstephenson)]

Josh,

NCIP isn't a standard so much as it is a menu of messages that endpoints can use to communicate with each other. The current implementation of the Evergreen driver working Auto-Graphics' SHAREit software. It doesn't implement LookupAgency methods because SHAREit does not implement them.

The design is flexible enough that a new driver could be added for a new vendor and that driver would implement the messages that the other vendor supports.

It is also possible to add one or two simple methods to the existing driver so long as they don't interfere with the current integration with SHAREit. If all that is being added it LookupAgency and LookupAgencyResponse and the other functionality doesn't change, then that might be doable in the current driver.

I have been considering adding an option to require password lookup in the LookupUser messages. I was thinking of a simple Boolean to indicate if passwords are required or not.

[Josh Stompro (u-launchpad-stompro-org)]

Hello Jason, I don't think I need LookupAgency added for my purposes, I was just making note of it in relation to the idea of NCIPServer being able to describe what kind of authentication prompts it wants shown to the end user. I understand that just because it is described in the standard, doesn't mean anyone actually makes use of that.

+1 to a simple boolean config option for the require passwords feature.

Thanks for the feedback as always.
Josh

[Josh Stompro (u-launchpad-stompro-org)]

Would the require passwords setting only be for the lookupUser message?

# Messages for which AuthenticationInput are valid.
use constant AUTHENTICATIONINPUT_MESSAGES => (
    'LookupUser', 'RenewItem', 'CheckOutItem', 'RequestItem',
);

Or would it be for any of the messages that support the AuthenticationInput fields?

I'm trying to imagine how that would work with a mix of Staff initiated actions and patron initiated actions. Staff are not going to have the users password. And would a 3rd party system hold onto the users plain text password after authentication to have it to send when it gets to the point to send a RequestItem or RenewItem message.

Josh

[Josh Stompro (u-launchpad-stompro-org)]

This bug for the SIPServer EG driver has some good examples also, looks like some code from the TADL branch is based off the code Galen submitted

https://bugs.launchpad.net/evergreen/+bug/1526558

There are a few options to control if the auth_proxy gets used or not which may be good to copy.

The sipserver code handles deleting the session after auth since the session never gets passed on, that should be included also.

Josh

[Josh Stompro (u-launchpad-stompro-org)]

I have an untested WIP branch to add this at
https://gitlab.com/LARL/ncipserver/-/commit/5041c9410c08bec1282d9b89a59cb0a0b736cef1

And I retract this sentence
"The sipserver code handles deleting the session after auth since the session never gets passed on, that should be included also."

The TADL code does handle that correctly.

Jason, I'm happy to make any changes you would like to see to make it better. I'll see if I can get it working with a test message next.

Josh

[Josh Stompro (u-launchpad-stompro-org)]

I was able to get this working yesterday afternoon, here is a branch with the commits squashed.
https://gitlab.com/LARL/ncipserver/-/commits/lp2018615-password-check/

Here is a NCIPServer Working branch with the changes added to the current master. I had a few things that depended on other commits that haven't made it in yet.
user/stompro/lp2018615-password-check-master

https://git.evergreen-ils.org/?p=working/NCIPServer.git;a=shortlog;h=refs/heads/user/stompro/lp2018615-password-check-master

I'm going to hold off on the pullrequest tag for now, since I don't have real world testing data yet.

Josh

[Josh Stompro (u-launchpad-stompro-org)]

Hello, I've had some testing against ReShare now, and this seems to work fine so adding pullrequest.
Josh

[Jason Stephenson (jstephenson)]

Josh, thanks for the contribution!

I have been looking at it to day, and it basically works for me with some test Perl code set up to work with the code.

However, I have some questions.

I may have missed it in your previous comments, but is there a reason that you only implemented this for LookupUser? I would think that some of the other messages that support AuthenticationInput could benefit from using it.

While I'm sure this work for Folio, I'm going to contact our vendor to get some new sample messages to see if this will work them out of the box or if it will require some modification.

[Jason Stephenson (jstephenson)]

I am also curious about the placeholders in the sample configuration. Can you explain why they are necessary?

[Jason Stephenson (jstephenson)]

I am pushing some changes, etc., that come up as a result of testing to the following branch:

https://git.evergreen-ils.org/?p=working/NCIPServer.git;a=shortlog;h=refs/heads/user/dyrcona/lp2018615_WIP

I have not yet signed off on Josh's commits.

There also appears to be a minor conflict with main in examples/oils_ncip.xml.example at the moment. I may rebase the working branch an force push it.

[Jason Stephenson (jstephenson)]

Going back to this from the bug description:

"There is also the AuthenticationPrompt element that seems to be to pass on what fields NCIPServer would want for authentication. That looks like it would be part of the LookupAgencyResponse message. LookupAgency doesn't look like it is currently implemented."

My understanding is that NCIP to NCIP authentication is meant to be handled via certificates. I do not know if any vendors actually support this.

[Josh Stompro (u-launchpad-stompro-org)]

"I am also curious about the placeholders in the sample configuration. Can you explain why they are necessary?"

There is an error that pops up when all the entries in a config area are commented out. I'll move that to a different bug report... I think I mainly see it in the <patrons></patrons> section since I didn't enable any of those options.

Shows up as a crash and a message like the following in logs/production.log
[12702] error @0.380047> [hit #1]request to POST / crashed: Can't use string ("If you want to have transits abo"...) as a HASH ref while "strict refs" in use at /home/opensrf/NCIPServer/lib/NCIP/ILS/Evergreen.pm line 3135. in /usr/share/perl5/Dancer/Handler.pm l. 101

[Josh Stompro (u-launchpad-stompro-org)]

"I may have missed it in your previous comments, but is there a reason that you only implemented this for LookupUser? I would think that some of the other messages that support AuthenticationInput could benefit from using it."

Only because that is the only place where I needed the functionality at. ReShare wanted to get rid of needing Sip2 for patron auth, for logging into the Vufind interface when users need to place holds. By adding it to the lookupuser request, NCIP can handle the authentication now.

There are no other user triggered actions now that would need authentication in our usage.

[Jason Stephenson (jstephenson)]

I think my branch has some extra stuff from other branches in it. I'm going to make a new one.

[Jason Stephenson (jstephenson)]

I have a pushed a signed off, rebased branch to user/dyrcona/lp2018615-rebase-signoff: https://git.evergreen-ils.org/?p=working/NCIPServer.git;a=shortlog;h=refs/heads/user/dyrcona/lp2018615-rebase-signoff

Normally, I would have just pushed this to main, but I want to wait a few days because we have an update going on next week, and I want to try the latest features in main without the possible distraction of this one.

Regarding the errors with the XML comments showing up in the hashes: I think that only happens if the entire contents of the XML element are comments. I may be wrong, but if you uncomment at least one of the commented settings, it works.

At any rate, I usually use a stripped down version of the configuration with no comments in production. Perhaps this should be documented in the README? If so, that would be another LP bug in my opinion.

My branch does strip out the placeholder elements. I squashed this into a commit with white space changes.

I'll make a note to revisit this in a week or two.

Thanks again, Josh!

[Josh Stompro (u-launchpad-stompro-org)]

> Regarding the errors with the XML comments showing up in the hashes: I think that only happens if the entire contents of the XML element are comments. I may be wrong, but if you uncomment at least one of the commented settings, it works.

That sounds correct. The placeholders just fix that, so if a user doesn't need any of the options in a section, they don't hit the error. I've run into that several times so I just wanted to try and get a fix in so others don't have to hit that issue.