From f9f4d186243f8cb8be0958e2e658771e3d821478 Mon Sep 17 00:00:00 2001 From: Mike Rylander Date: Fri, 2 May 2014 17:00:43 -0400 Subject: [PATCH] LP#1254918: Allow skiping of user-object perms Previous to this commit, permissions in Evergreen check a cominbation of: * user-object permissions (does the user have a direct permission mapping to the object in question) * user-context permissions (does the user have the permission at the object's context location, whose field is defined in the IDL) * user-global permission (lacking a context location, does the user have the permission globally (at the top of the org tree) and therefore can apply the action to all objects of this typ) In practice, there are almost no user-object permissions. When retrieving just on object from the database, the cost of this check is negligable to the point that we can completely ignore it. However, when retrieving a large set of objects, such as the list of all funds in a large, consortial environment, the cost to check the user-object permission adds up to a noticable amount of time. To address this, we add a new construct to the IDL instructing the PCRUD infrastructure to skip user-object permission checking in those cases where the design and use of the system makes user-specific object permissions needless or superfluous. This is embodied in a new XML attribute on the element: ignore_object_perms. When set to "true", pcrud will skip all user-object permission checks, resulting in faster time-to-first-result. Additionally, we add a new "owning_user" attribute on the element of the section. This new attribute specifies the field containing the actor.usr.id of the user that "owns" the object. This allows PCRUD to test ownership of an object directly, and if the requesting user and owning user are the same, the action is allowed. Finaly, when "global_required" is "true" for the permission check, and there is no "owning_user" attribute defined for the class in the IDL, we skip the above-mentioned user-object permission check. When "global_required" is "false" or there is an "owning_user" attribute, we check for user permissions. In all cases, the "ignore_object_perms" attribute is honored, and in its presence we skip non-owner user-object permissions. The net result is an immediate increase in speed for retrieval of objects in the presence of the "global_required" attribute, and a mechanism to increase the speed of specific cases of context-aware retrival by the use of "ignore_object_perms". We use this new mechanism to speed the retrieval of fund objects in the ACQ interfaces that draw available-fund dropdowns. Signed-off-by: Mike Rylander Signed-off-by: Bill Erickson Signed-off-by: Ben Shum --- Open-ILS/examples/fm_IDL.xml | 2 +- Open-ILS/examples/permacrud.xsd | 5 + Open-ILS/src/c-apps/oils_idl-core.c | 8 ++ Open-ILS/src/c-apps/oils_sql.c | 187 ++++++++++++++++++++++++++-- 4 files changed, 188 insertions(+), 14 deletions(-) diff --git a/Open-ILS/examples/fm_IDL.xml b/Open-ILS/examples/fm_IDL.xml index f318da84d5..e574b29e77 100644 --- a/Open-ILS/examples/fm_IDL.xml +++ b/Open-ILS/examples/fm_IDL.xml @@ -7676,7 +7676,7 @@ SELECT usr, - + diff --git a/Open-ILS/examples/permacrud.xsd b/Open-ILS/examples/permacrud.xsd index 451ac4b8f2..22750b74f1 100644 --- a/Open-ILS/examples/permacrud.xsd +++ b/Open-ILS/examples/permacrud.xsd @@ -47,6 +47,7 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA + @@ -58,6 +59,7 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA + @@ -69,6 +71,7 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA + @@ -80,6 +83,7 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA + @@ -100,6 +104,7 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA + diff --git a/Open-ILS/src/c-apps/oils_idl-core.c b/Open-ILS/src/c-apps/oils_idl-core.c index 5d432ab3f1..707991e501 100644 --- a/Open-ILS/src/c-apps/oils_idl-core.c +++ b/Open-ILS/src/c-apps/oils_idl-core.c @@ -282,6 +282,7 @@ osrfHash* oilsIDLInit( const char* idl_filename ) { { create : { permission : [ x, y, z ], global_required : "true", -- anything else, or missing, is false + ignore_object_perms : "true", -- anything else, or missing, is false local_context : [ f1, f2 ], foreign_context : { class1 : { fkey : local_class_key, field : class1_field, context : [ a, b, c ] }, ...} }, @@ -298,6 +299,7 @@ osrfHash* oilsIDLInit( const char* idl_filename ) { osrfHash* pcrud = osrfNewHash(); osrfHashSet( class_def_hash, pcrud, "permacrud" ); xmlNodePtr _l = _cur->children; + char * ignore_object_perms = (char*) xmlGetProp(_cur, BAD_CAST "ignore_object_perms"); while(_l) { if (strcmp( (char*)_l->name, "actions" )) { @@ -325,6 +327,9 @@ osrfHash* oilsIDLInit( const char* idl_filename ) { osrfHash* action_def_hash = osrfNewHash(); osrfHashSet( pcrud, action_def_hash, action_name ); + // Set the class-wide ignore_object_perms flag + osrfHashSet( action_def_hash, ignore_object_perms, "ignore_object_perms"); + // Tokenize permission attribute into an osrfStringArray prop_str = (char*) xmlGetProp(_a, BAD_CAST "permission"); if( prop_str ) @@ -334,6 +339,9 @@ osrfHash* oilsIDLInit( const char* idl_filename ) { osrfHashSet( action_def_hash, map, "permission"); xmlFree( prop_str ); + osrfHashSet( action_def_hash, + (char*)xmlGetNoNsProp(_a, BAD_CAST "owning_user"), "owning_user"); + osrfHashSet( action_def_hash, (char*)xmlGetNoNsProp(_a, BAD_CAST "global_required"), "global_required"); diff --git a/Open-ILS/src/c-apps/oils_sql.c b/Open-ILS/src/c-apps/oils_sql.c index dac5d8c9d7..6d3d25cc9f 100644 --- a/Open-ILS/src/c-apps/oils_sql.c +++ b/Open-ILS/src/c-apps/oils_sql.c @@ -1561,6 +1561,12 @@ static int verifyObjectPCRUD ( osrfMethodContext* ctx, osrfHash *class, const js // for storing lists of integers, so we fake it with a list of strings.) osrfStringArray* context_org_array = osrfNewStringArray( 1 ); + const char* context_org = NULL; + const char* pkey = NULL; + jsonObject *param = NULL; + const char* perm = NULL; + int OK = 0; + int i = 0; int err = 0; const char* pkey_value = NULL; if( str_is_true( osrfHashGet(pcrud, "global_required") ) ) { @@ -1595,8 +1601,8 @@ static int verifyObjectPCRUD ( osrfMethodContext* ctx, osrfHash *class, const js osrfLogDebug( OSRF_LOG_MARK, "global-level permissions not required, " "fetching context org ids" ); - const char* pkey = osrfHashGet( class, "primarykey" ); - jsonObject *param = NULL; + + pkey = osrfHashGet( class, "primarykey" ); if( !pkey ) { // There is no primary key, so we can't do a fresh lookup. Use the row @@ -1627,6 +1633,8 @@ static int verifyObjectPCRUD ( osrfMethodContext* ctx, osrfHash *class, const js param = jsonObjectExtractIndex( _list, 0 ); jsonObjectFree( _list ); + + fetch = 0; } if( !param ) { @@ -1842,20 +1850,166 @@ static int verifyObjectPCRUD ( osrfMethodContext* ctx, osrfHash *class, const js osrfHashIteratorFree( class_itr ); } } - - jsonObjectFree( param ); } - const char* context_org = NULL; - const char* perm = NULL; - int OK = 0; + // If there is an owning_user attached to the action, we allow that user and users with + // object perms on the object. CREATE can't use this. We only do this when there is no + // context org for this action, and when we're not ignoring object perms. + if ( + *method_type != 'c' && + !str_is_true( osrfHashGet(pcrud, "ignore_object_perms") ) && // Always honor + context_org_array->size == 0 + ) { + char* owning_user_field = osrfHashGet( pcrud, "owning_user" ); + if (owning_user_field) { + + if (!param) { // We didn't get it during the context lookup + pkey = osrfHashGet( class, "primarykey" ); + + if( !pkey ) { + // There is no primary key, so we can't do a fresh lookup. Use the row + // image that we already have. If it doesn't have everything we need, too bad. + fetch = 0; + param = jsonObjectClone( obj ); + osrfLogDebug( OSRF_LOG_MARK, "No primary key; using clone of object" ); + } else if( obj->classname ) { + pkey_value = oilsFMGetStringConst( obj, pkey ); + if( !fetch ) + param = jsonObjectClone( obj ); + osrfLogDebug( OSRF_LOG_MARK, "Object supplied, using primary key value of %s", + pkey_value ); + } else { + pkey_value = jsonObjectGetString( obj ); + fetch = 1; + osrfLogDebug( OSRF_LOG_MARK, "Object not supplied, using primary key value " + "of %s and retrieving from the database", pkey_value ); + } + + if( fetch ) { + // Fetch the row so that we can look at the foreign key(s) + osrfHashSet((osrfHash*) ctx->session->userData, "1", "inside_verify"); + jsonObject* _tmp_params = single_hash( pkey, pkey_value ); + jsonObject* _list = doFieldmapperSearch( ctx, class, _tmp_params, NULL, &err ); + jsonObjectFree( _tmp_params ); + osrfHashSet((osrfHash*) ctx->session->userData, "0", "inside_verify"); + + param = jsonObjectExtractIndex( _list, 0 ); + jsonObjectFree( _list ); + } + } + + if( !param ) { + // The row doesn't exist. Complain, and deny access. + osrfLogDebug( OSRF_LOG_MARK, + "Object not found in the database with primary key %s of %s", + pkey, pkey_value ); + + growing_buffer* msg = buffer_init( 128 ); + buffer_fadd( + msg, + "%s: no object found with primary key %s of %s", + modulename, + pkey, + pkey_value + ); + + char* m = buffer_release( msg ); + osrfAppSessionStatus( + ctx->session, + OSRF_STATUS_INTERNALSERVERERROR, + "osrfMethodException", + ctx->request, + m + ); + + free( m ); + return 0; + } else { + + int ownerid = atoi( oilsFMGetStringConst( param, owning_user_field ) ); + + // Allow the owner to do whatever + if (ownerid == userid) + OK = 1; + + i = 0; + while( !OK && (perm = osrfStringArrayGetString(permission, i++)) ) { + dbi_result result; + + osrfLogDebug( + OSRF_LOG_MARK, + "Checking object permission [%s] for user %d " + "on object %s (class %s)", + perm, + userid, + pkey_value, + osrfHashGet( class, "classname" ) + ); + + result = dbi_conn_queryf( + writehandle, + "SELECT permission.usr_has_object_perm(%d, '%s', '%s', '%s') AS has_perm;", + userid, + perm, + osrfHashGet( class, "classname" ), + pkey_value + ); + + if( result ) { + osrfLogDebug( + OSRF_LOG_MARK, + "Received a result for object permission [%s] " + "for user %d on object %s (class %s)", + perm, + userid, + pkey_value, + osrfHashGet( class, "classname" ) + ); + + if( dbi_result_first_row( result )) { + jsonObject* return_val = oilsMakeJSONFromResult( result ); + const char* has_perm = jsonObjectGetString( + jsonObjectGetKeyConst( return_val, "has_perm" )); + + osrfLogDebug( + OSRF_LOG_MARK, + "Status of object permission [%s] for user %d " + "on object %s (class %s) is %s", + perm, + userid, + pkey_value, + osrfHashGet(class, "classname"), + has_perm + ); + + if( *has_perm == 't' ) + OK = 1; + jsonObjectFree( return_val ); + } + + dbi_result_free( result ); + if( OK ) + break; + } else { + const char* msg; + int errnum = dbi_conn_error( writehandle, &msg ); + osrfLogWarning( OSRF_LOG_MARK, + "Unable to call check object permissions: %d, %s", + errnum, msg ? msg : "(No description available)" ); + if( !oilsIsDBConnected( writehandle )) + osrfAppSessionPanic( ctx->session ); + } + } + } + } + } // For every combination of permission and context org unit: call a stored procedure // to determine if the user has this permission in the context of this org unit. // If the answer is yes at any point, then we're done, and the user has permission. // In other words permissions are additive. - int i = 0; - while( (perm = osrfStringArrayGetString(permission, i++)) ) { + i = 0; + while( !OK && (perm = osrfStringArrayGetString(permission, i++)) ) { dbi_result result; osrfStringArray* pcache = NULL; @@ -1905,7 +2059,14 @@ static int verifyObjectPCRUD ( osrfMethodContext* ctx, osrfHash *class, const js } } - if( pkey_value ) { + if( + pkey_value && + !str_is_true( osrfHashGet(pcrud, "ignore_object_perms") ) && // Always honor + ( + !str_is_true( osrfHashGet(pcrud, "global_required") ) || + osrfHashGet(pcrud, "owning_user") + ) + ) { osrfLogDebug( OSRF_LOG_MARK, "Checking object permission [%s] for user %d " @@ -4231,7 +4392,7 @@ char* SELECT ( // Look up the field in the IDL const char* col_name = jsonObjectGetString( selfield ); - osrfHash* field_def; + osrfHash* field_def = NULL; if (!osrfStringArrayContains( osrfHashGet( @@ -4319,7 +4480,7 @@ char* SELECT ( jsonObjectGetKeyConst( selfield, "column" ) ); // Get the field definition from the IDL - osrfHash* field_def; + osrfHash* field_def = NULL; if (!osrfStringArrayContains( osrfHashGet( osrfHashGet( class_field_set, col_name ), @@ -5046,7 +5207,7 @@ static char* buildOrderByFromArray( osrfMethodContext* ctx, const jsonObject* or const char* field = jsonObjectGetString( jsonObjectGetKeyConst( order_spec, "field" )); - jsonObject* compare_to = jsonObjectGetKeyConst( order_spec, "compare" ); + jsonObject* compare_to = jsonObjectGetKey( order_spec, "compare" ); if( !field || !class_alias ) { osrfLogError( OSRF_LOG_MARK, -- 2.43.2