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 (file)
--- 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 (file)
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;
+}