Possible to bypass holds placement limits via direct API calls

[Bill Erickson]

The holds placement permit and creation steps are handled by two different API calls:

open-ils.circ.title_hold.is_possible
open-ils.circ.holds.create[.override]

This means Evergreen clients have the ability to bypass the "permit" step and go straight to the holds placement step, ignoring any limits in place with the hold matrix. This problem was brought to my attention because there is apparently an "unauthorized" IOS app that some patrons are using to place holds and it's letting them place as many holds as they want without limit.

As to why/how, I believe the "is_possible" call started out as a way to answer the question, "are there any targets for this hold / should I bother placing it?". Over time, it evolved into the primary location to check for permissibility. It's time to rectify that.

My suggestion is to deprecate both of the above methods and move clients toward using open-ils.circ.holds.test_and_create[.batch]. It combines the permission check with the holds placement. Fortunately, there are few interfaces that actually call the open-ils.circ.holds.create[.override] methods, since the catalog is the only place holds are placed. TPAC is already using open-ils.circ.holds.test_and_create.batch We just need to teach JSPAC how to do the same and thoroughly document/announce the change so that 3rd-party applications can start using the correct API.

[Jason Etheridge (phasefx)]

JFYI, holds can also be created with place_hold.xul (e.g. Request
Items in Copy Buckets), but it's already using
open-ils.circ.holds.test_and_create.batch

[Thomas Berezansky (tsbere)]

For note, I believe that even the test and create batch method suffers from this.

If you call the override variant *first* the title hold is possible checks don't run.

I have plans on changing how a lot of that works. They include making the non-batch create function go away as a call of its own....but I have reasons to want the title hold is possible check to continue to exist. "Could this hold be placed, and if not, why not?" type interfaces could be very useful to allow staff to test without placing actual holds, for example.

[Jason Stephenson (jstephenson)]

The changes that I am planning for https://bugs.launchpad.net/evergreen/+bug/1076062 will most likely address this issue. I'm still working on a complete description of the problems in that bug and the plans to address them via code. I'm going to send that to the dev list when I've completed the description. I won't mention this bug in that email since it is a private, security issue and since fixing this one is actually incidental to fixing 1076062.

[Jason Stephenson (jstephenson)]

I set the priority to medium I don't think this is that big of an issue as far as security issues go. It does not warrant a special security fix notice in my opinion.

[Jason Stephenson (jstephenson)]

Not really sure if my branch for bug 1076062 addresses this at all.

I didn't test for it, so we'll consider this bug still open and separate from that one.

[Kathy Lussier (klussier)]

I don't know if this is the same issue, but we are currently seeing a problem where the kenstir Android app is placing holds without first going through the permit test. I've looked through the code, but I'm unfamiliar with it and didn't see which call is being used. I've posted a comment to the Android app's LP bug, but for the long-term, I think we need to prevent these calls from occurring, because the next person who builds an app could use them.

The system where this occurred is on 2.8.x. I know a lot of jspac code was removed in 2.9. Is it possible it also removed the ability to make those calls, making this a non-issue?

[Kathy Lussier (klussier)]

After a bit of digging, I've found that the Android app is indeed using open-ils.circ.holds.create call.

I performed a grep on the code, aside from Holds.pm, I found that open-ils.circ.holds.create and open-ils.circ.holds.create.override are only used internally in Open-ILS/web/opac/common/js/config.js

 var CREATE_HOLD = "open-ils.circ:open-ils.circ.holds.create";
 var CREATE_HOLD_OVERRIDE = "open-ils.circ:open-ils.circ.holds.create.override";

Grepping CREATE_HOLD turns up ilsperm.CREATE_HOLD in some of the dtd files, but it doesn't look like the call is being used internally now that the jspac files have been removed.

I found the same to be true for open-ils.circ.title_hold.is_possible.

I would like to follow through with Bill's original suggestion to 1) deprecate these methods and then 2)entirely remove them from the code so that third-party apps can no longer use them. Actually, I wouldn't mind jumping to #2 right away, but I'm guessing clients using these API's need some forewarning.

What needs to be done to deprecate these methods for 2.10. Is it primarily a documentation/PR change? If so, I'm happy to do the legwork on this. I also could working on removing these calls from Holds.pm, but I probably will need a little assistance/direction in doing so.

[Jason Stephenson (jstephenson)]

Well, you're supposed to run open-ils.circ.title_hold.is_possible before doing open-ils.circ.holds.create, but that said using open-ils.circ.holds.test_and_create.batch is better.

I would be for deprecating open-ils.circ.holds.create in 2.10, and removing it in the next release.

A non-batch version of open-ils.circ.holds.test_and_create.batch might be good to add when you only want to do 1 hold at a time. The batch on the end may throw some developers who don't realize that a list with one entry is still a batch.

I think open-ils.circ.title_hold.is_possible should definitely stay. There are times I've used that method to find out if holds can be placed or not without actually creating the hold.

[Jason Stephenson (jstephenson)]

BTW, I removed the 2.3 and 2.4 series targets because there is no way it's getting fixed there. :)

[Bill Erickson (berick)]

For added fun, the code we want to encourage using (open-ils.circ.holds.test_and_create[.batch]) calls the code we want to deprecate (open-ils.circ.holds.create[.override]) internally. (Code re-use, FTW). If we deprecate open-ils.circ.holds.create[.override], we'll need to retain its code in a non-published API / utility function.

[Jeff Davis (jdavis-sitka)]

First crack at a fix:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/jeffdavis/lp1017990-deprecate-hold-create

This logs a warning about the deprecated API calls, moves the bulk of the create_hold method to a separate implementation method, and makes the open-ils.circ.holds.test_and_create.batch[.override] calls use the implementation method directly.

[Jeff Davis (jdavis-sitka)]

Fix still backports cleanly to master. Adding a pullrequest and targeting to current releases. We might want to revisit this bug during the next dev meeting, it's been awhile.

[Jeff Davis (jdavis-sitka)]

That is, the fix can be cherry-picked to master without any merge conflicts. "Backport" isn't really the right word. :)

[Jason Stephenson (jstephenson)]

With the branch from comment #11 installed and rebased on master, placing holds from the staff client is broken.

1. When placing a hold from the Holds tab of the Patron record, the patron's barcode is not autofilled in.

2. When placing a hold for a patron from the web staff client, and internal server error is generated:

Aug 1 10:48:48 buster apache2[12525]: [perl:warn] [pid 12525] [client ::1:54680
] 0 : HASH(0x5611cb097310), referer: https://buster.sigio.com/eg/opac/place_hold
?detail_record_view=0;locg=1;pubdate=is;hold_target=96;query=vivaldi;hold_source
_page=%2Feg%2Fopac%2Fresults%3Fbool%3Dand%3Bbool%3Dand%3Bbool%3Dand%3Bqtype%3Dke
yword%3Bqtype%3Dtitle%3Bqtype%3Dauthor%3Bcontains%3Dcontains%3Bcontains%3Dcontai
ns%3Bcontains%3Dcontains%3Bquery%3Dvivaldi%3Bquery%3D%3Bquery%3D%3B_adv%3D1%3Bde
tail_record_view%3D0%3Blocg%3D1%3Bpubdate%3Dis;hold_type=T
Aug 1 10:48:48 buster apache2[12525]: [perl:error] [pid 12525] [client ::1:5468
0] egweb: Context Loader error: Can't locate object method "content" via package
 "OpenSRF::DomainObject::oilsMethodException" at /usr/local/share/perl/5.28.1/Op
enILS/WWW/EGCatLoader/Account.pm line 1411.\n, referer: https://buster.sigio.com
/eg/opac/place_hold?detail_record_view=0;locg=1;pubdate=is;hold_target=96;query=
vivaldi;hold_source_page=%2Feg%2Fopac%2Fresults%3Fbool%3Dand%3Bbool%3Dand%3Bbool
%3Dand%3Bqtype%3Dkeyword%3Bqtype%3Dtitle%3Bqtype%3Dauthor%3Bcontains%3Dcontains%
3Bcontains%3Dcontains%3Bcontains%3Dcontains%3Bquery%3Dvivaldi%3Bquery%3D%3Bquery
%3D%3B_adv%3D1%3Bdetail_record_view%3D0%3Blocg%3D1%3Bpubdate%3Dis;hold_type=T
Aug 1 10:48:48 buster root: ::1 - - [01/Aug/2019:10:48:48 -0400] "POST /eg/opac/place_hold?detail_record_view=0;locg=1;pubdate=is;hold_target=96;query=vivaldi;hold_source_page=%2Feg%2Fopac%2Fresults%3Fbool%3Dand%3Bbool%3Dand%3Bbool%3Dand%3Bqtype%3Dkeyword%3Bqtype%3Dtitle%3Bqtype%3Dauthor%3Bcontains%3Dcontains%3Bcontains%3Dcontains%3Bcontains%3Dcontains%3Bquery%3Dvivaldi%3Bquery%3D%3Bquery%3D%3B_adv%3D1%3Bdetail_record_view%3D0%3Blocg%3D1%3Bpubdate%3Dis;hold_type=T HTTP/1.0" 500 1086

There is no corresponding error in osrfsys.log.

Going to view a patron's holds in the OPAC leads to a 500 Internal Server Error:

Aug 1 10:56:54 buster apache2[12498]: [perl:error] [pid 12498] [client ::1:54762] egweb: Context Loader error: Exception: OpenSRF::EX::ERROR 2019-08-01T10:56:54 OpenILS::WWW::EGWeb /usr/local/share/perl/5.28.1/OpenILS/WWW/EGWeb.pm:185 System ERROR: Exception: OpenSRF::DomainObject::oilsMethodException 2019-08-01T10:56:54 OpenSRF::AppRequest /usr/local/share/perl/5.28.1/OpenSRF/AppSession.pm:1159 <404> Method [open-ils.circ.holds.id_list.retrieve.authoritative] not found for OpenILS::Application::Circ\n\n, referer: https://buster.sigio.com/eg/opac/myopac/main
Aug 1 10:56:54 buster root: ::1 - - [01/Aug/2019:10:56:54 -0400] "GET /eg/opac/myopac/holds HTTP/1.0" 500 1086

There is a corresponding error in the osrfsys.log:

[2019-08-01 10:56:54] open-ils.circ [INFO:12420:Application.pm:159:15646708191249887] CALL: open-ils.circ open-ils.circ.holds.id_list.retrieve.authoritative e3c6b07e71a59057f8c654961f6790de, 71, 0
[2019-08-01 10:56:54] open-ils.circ [INFO:12420:Tran...

[Jason Stephenson (jstephenson)]

I have set this to Won't Fix for all but Evergreen 3.4, since the goal seems to be deprecation and removal of the open-ils.circ.holds.create method. Such a change should not be backported.

I have also removed the pullrequest tag since the branch does not work for me.

I have tested again with master and everything works without this patch.

[Chris Sharp (chrissharp123)]

I just went down the rabbithole on this issue. Working branch here: https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/csharp/lp1017990-deprecate-hold-create - it removes the open-ils.circ.holds.create[.override] and batch methods and calls the create_hold subroutine directly. It passes the live Perl tests. I got ambitious here, so criticism is welcome!

[Ken Cox (kenstir)]

PR looks good to me! The Hemlock app has not called open-ils.circ.holds.create in about 5 years but this looks like a good cleanup.

[Galen Charlton (gmc)]

I can confirm neither Aspen's nor VuFind's Evergreen drivers invoke any of the deprecated APIs.

However, a check of customer logs reminded me that the Communico mobile app is still using open-ils.circ.holds.create. That said, I know that OCLC is making some changes to the app, so I'll continue to lobby them to switch to open-ils.circ.holds.test_and_create.batch.

[Jeff Davis (jdavis-sitka)]

As of today, Chris's fix still applies cleanly to main. Are we ready to commit it? If not, do we want to announce deprecation of the old API in 3.13 and schedule actual removal for a future release?

[Jason Boyer (jboyer)]

I've got a signoff / followup for Chris's branch here: https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/jboyer/lp1017990_stop_trying_to_make_batch_happen / working/user/jboyer/lp1017990_stop_trying_to_make_batch_happen

It keeps his changes so that nothing *in* Evergreen should call the deprecated functions, while returning the old functions and marking them deprecated in the perldoc while logging a "DEPRECATION WARNING: BLAH BLAH" whenever they're called.

Note to the committer that puts this in main / a release branch: There's a X.XX tag in the release notes that should be replaced with the version this is released in.