From: Jane Sandberg <js7389@princeton.edu>
Date: Mon, 6 Mar 2023 00:57:50 +0000 (-0800)
Subject: LP1808016: improve error handling by open-ils.pcrud
X-Git-Url: http://git.equinoxoli.org/?p=evergreen-equinox.git;a=commitdiff_plain;h=1ef377fd76ef3988db17198c10649b50b7b67e08

LP1808016: improve error handling by open-ils.pcrud

This patch ensures that requests to open-ils.pcrud return
an error code (before the request completion code) when
a permissions or constraint check fails.

To test
-------
[1] Make an invalid request, e.g., by attempting to create a claim
    type whose owner is not set in the Acquisitions Claiming admin
    interface.
[2] Note that the user interface reports that the action succeeds
    (although the new claim type is not actually created).
[3] Apply the patch and repeat step 1. This time, the admin interface
    shoudl report that the creation failed.

Signed-off-by: Jane Sandberg <sandbergja@gmail.com>
Signed-off-by: Terran McCanna <tmccanna@georgialibraries.org>
Signed-off-by: Galen Charlton <gmc@equinoxOLI.org>
---

diff --git a/Open-ILS/src/c-apps/oils_sql.c b/Open-ILS/src/c-apps/oils_sql.c
index 93f2577..d292efb 100644
--- a/Open-ILS/src/c-apps/oils_sql.c
+++ b/Open-ILS/src/c-apps/oils_sql.c
@@ -2365,6 +2365,10 @@ int doCreate( osrfMethodContext* ctx ) {
 	}
 
 	if( !verifyObjectClass( ctx, target )) {
+		osrfAppSessionStatus(ctx->session, OSRF_STATUS_BADREQUEST,
+				"osrfMethodException", ctx->request,
+				"Invalid object or insufficient permissions"
+		);
 		osrfAppRespondComplete( ctx, NULL );
 		return -1;
 	}
@@ -6431,6 +6435,10 @@ int doUpdate( osrfMethodContext* ctx ) {
 		target = jsonObjectGetIndex( ctx->params, 0 );
 
 	if(!verifyObjectClass( ctx, target )) {
+		osrfAppSessionStatus(ctx->session, OSRF_STATUS_BADREQUEST,
+			"osrfMethodException", ctx->request,
+			"Invalid object or insufficient permissions"
+		);
 		osrfAppRespondComplete( ctx, NULL );
 		return -1;
 	}
@@ -6702,6 +6710,10 @@ int doDelete( osrfMethodContext* ctx ) {
 	char* id;
 	if( jsonObjectGetIndex( ctx->params, _obj_pos )->classname ) {
 		if( !verifyObjectClass( ctx, jsonObjectGetIndex( ctx->params, _obj_pos ))) {
+			osrfAppSessionStatus(ctx->session, OSRF_STATUS_BADREQUEST,
+				"osrfMethodException", ctx->request,
+				"Invalid object or insufficient permissions"
+			);
 			osrfAppRespondComplete( ctx, NULL );
 			return -1;
 		}
diff --git a/Open-ILS/src/perlmods/live_t/lp1808016-pcrud-return-error-status.t b/Open-ILS/src/perlmods/live_t/lp1808016-pcrud-return-error-status.t
new file mode 100644
index 0000000..d4b7fb0
--- /dev/null
+++ b/Open-ILS/src/perlmods/live_t/lp1808016-pcrud-return-error-status.t
@@ -0,0 +1,54 @@
+#!perl
+# Yes, this is a Perl test for some C code, but
+# it seemed way easier to write a meaningful
+# integration test here than with libcheck :-D
+
+use strict; use warnings;
+use Test::More tests => 2;
+use OpenILS::Utils::TestUtils;
+use OpenILS::Const qw(:const);
+
+my $script = OpenILS::Utils::TestUtils->new();
+our $apputils = 'OpenILS::Application::AppUtils';
+
+diag('LP108016: Pcrud returns a bad error status before complete');
+
+# Login as a staff with limited permissions (just acq permissions in this case)
+my $credentials = {
+    username => 'br1breid',
+    password => 'barbarar1234',
+    type => 'staff'
+};
+my $authtoken = $script->authenticate($credentials);
+ok(
+    $authtoken,
+    'Logged in'
+) or BAIL_OUT('Must log in');
+
+my $pcrud_ses = $script->session('open-ils.pcrud');
+$pcrud_ses->connect();
+my $xact = $pcrud_ses->request(
+    'open-ils.pcrud.transaction.begin',
+    $script->authtoken
+)->gather(1);
+
+# As this user, try to do something forbidden: create a shelving location
+my $acpl = Fieldmapper::asset::copy_location->new;
+$acpl->owning_lib(1);
+$acpl->name('My bad location');
+my $request = $pcrud_ses->request(
+    'open-ils.pcrud.create.acpl',
+    $script->authtoken,
+    $acpl
+);
+$request->recv();
+
+is(error_code($request), 400, 'We get the expected error code');
+
+sub error_code {
+    my $request_to_check = shift;
+    if ($request_to_check->failed() =~ /<(\d{3})>/ms) {
+        return $1;
+    }
+    return 0;
+}