[GitHub] [madlib] orhankislal commented on a change in pull request #554: DL: Check if the owner of the object table is a superuser

[GitBox <gi...@apache.org>]

orhankislal commented on a change in pull request #554:
URL: https://github.com/apache/madlib/pull/554#discussion_r587272084



##########
File path: src/ports/postgres/modules/utilities/utilities.py_in
##########
@@ -775,6 +775,17 @@ def is_superuser(user):
     return plpy.execute("SELECT rolsuper FROM pg_catalog.pg_roles "\
                         "WHERE rolname = '{0}'".format(user))[0]['rolsuper']
 
+def get_table_owner(schema_table):
+
+    split_table = schema_table.split(".",1)
+    schema = split_table[0]

Review comment:
       There are two aspects at play. The accuracy of the `get_table_owner` utility function and how it is used in `load_custom_function`. 
   
   Let's start with the second point. `load_custom_function` appends the madlib schema to the given parameter no matter what it looks like (madlib_keras_custom_function.py_in, line 71).  When we call the `get_table_owner`, our own prefixed schema name is split and the users' `object_table` argument is tested. This means the user cannot trick us with any schema name because we don't accept any and we don't parse their input, we directly look at the catalog to see if we have an exact match for the table name.
   
   Getting back to the first point, the accuracy of the `get_table_owner` function. If some other future function tries to use this (which is possible since this is a generic utility function, not DL specific) there might be a way to break it since we might not have put the same amount of control on the input. I guess they might have a schema name like "as.df" which will mess up the split. Please note that the existing `get_schema` utility function uses a similar split. 
   
   Considering these two points, I cannot think of an issue for `load_custom_function` unless the dbadmin installed madlib to a schema with `.` in its name. madpack does not allow usage of `.` in the schema name but an admin can rename the `madlib` schema once it is installed. I don't know if madlib even works in this state but I don't think we can account for any arbitrary manipulations of our installation.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org