django-sql-explorer icon indicating copy to clipboard operation
django-sql-explorer copied to clipboard

Permissions Refactor

Open dsanders11 opened this issue 9 years ago • 3 comments

This is a two part suggestion:

  1. Use the Django auth backend for permissions. Permissions are already generated for the models in the app (add/change/delete), and you can add new ones. Specifically a 'view' permission could be added for the Query model, and these permissions could become the default way of handling permission instead of the lambdas. For more complex cases, Django has a tuple of auth backends, allowing for custom backends which can grant permissions (if any backend says yes then it's allowed, unless it raises the PermissionDenied exception). This means users who currently have a complex function for permissions currently can port it to a custom backend like:
class ExplorerPermissionsBackend(object):
    def has_perm(self, user_obj, perm, obj=None):
        # This could be wrapped into a simple base model for users
        if not perm.startswith("explorer."):
            return False

        if user_obj.username == "foobar" and perm == "explorer.change_query":
            return True
        else:
            return False

This would also open up integrating an object-level permissions backend such as django-guardian to grant certain users permissions on individual queries.

  1. The second suggestion dovetails nicely with the first, and that's to allow blacklisting tables for certain users. While it may be OK for super users to dump the whole contents of auth_user, it's a bad idea to leave that exposed to regular staff members. This can somewhat be accomplished with the database connection itself, but it's very coarse-grained.

Use sqlparse to find table names in the query (would need a list of table names, probably from the same method that gets them for the schema list) and check permissions against the auth backends like:

    tables_to_permissions = {}  # table name -> (app_label + ".change_" + model name)

    # Ensure the user has permissions for all tables in the query
    for table_name in query_table_names:
        if not request.user.has_perm(tables_to_permissions[table_name]):
            return  # Permission denied, similar to keyword blacklisting

This would limit users to doing queries on the tables for the models they have the change permission for (unfortunately Django doesn't have a stock 'view' permission).

The code could also first look for a "view_" permission to help open up the possibility of opening up the interface to public consumption. In that situation the custom auth backend in 1 would return True for the "view_" permission for any model which should be queryable by anyone.

dsanders11 avatar Jun 14 '15 07:06 dsanders11

#1 is a terrific idea & bit of feedback.

#2...I'm not as sure. I really don't want to rely on parsing SQL for anything security-related. Explorer does have the 'blacklist' feature, but it's really not something to be relied on. I'm curious why you say the connection approach is coarse-grained. Do you mean that it's clunky to setup up many different connections for different user permissions? Because (at least with postgres) there is a tremendous amount of control in terms of granting different users/connections access to tables, views, functions, etc.

Back to #1 for a second -- I may not be able to work on it for a little while, but I'd be very open to a pull request. Even a PR that laid a little bit of gorundwork would be helpful. I don't know the django permissions system well, but if you could put 1 or 2 pieces in place, I may be able to spare some cycles to implement it across the app. Let me know what you think; I'd love to find a way to make some forward progress.

P.S. Also I owe you a beer if you are ever up in San Francisco! Feel free to get in touch.

chrisclark avatar Jun 27 '15 20:06 chrisclark

I'll try to put something together for 1 (darn GitHub auto-links #1 to an issue, that's annoying) when I get a chance, I'm also fairly bogged down. I've got a decent handle on the permissions framework so I can put together a starting block when I get a chance.

Regarding the coarse-grained connection approach, I meant it's coarse-grained if you don't make a connection-per user. I'm trying to keep things as Django admin panel friendly as possible, so creating new DB connections and tying them to certain users doesn't seem feasible really. However, I totally get your aversion to using SQL parsing for security-related stuff, it's not fool-proof. Of course the blacklisting has it's own problems, such as preventing me from looking up orders shipping to a city named Grant (Michigan or Alabama, take your pick). :-)

I'd love to be able to use the Django permissions framework for this, as right now I don't feel comfortable letting anyone but a superuser have access to SQLExplorer since they get to view everything. While DB connections can limit this there'd still need to be a 'DB connection to Django user' mapping somewhere, and manual creating of the DB connections.

I'll ponder it some more but for the moment in my use case it will stay a toy for superusers.

dsanders11 avatar Jul 05 '15 08:07 dsanders11

@dsanders11 Do you develop any solutions for the second suggestion?

nath24 avatar May 08 '22 14:05 nath24