Add object permissions based on django-guardian

[jbeezley]

So far this only contains model layer logic on folders (files will be similar). I will work on the rest layer next to give a better sense of how it is used.

[jbeezley]

I'll address minor issues later, I'm pushing this as a proof of concept for the moment. I also have to go through and fix the folder rest tests.

One thing to note about the behavior as implemented is that you get a 404 on requests for objects that you don't have read access to, but a 403 when trying to write to an object that you only have read access. I could see an argument for wanting a 403 in both cases.

Finally, we need to decide on a method for providing public access either by a column on the tree table or by a default group. I'm leaning toward the group method because I'm not sure how to add the condition inside the has_perm query.

[zachmullen]

One thing to note about the behavior as implemented is that you get a 404 on requests for objects that you don't have read access to, but a 403 when trying to write to an object that you only have read access. I could see an argument for wanting a 403 in both cases.

In my experience this is actually the most common behavior, and I'd say the most desirable for security. A 404 doesn't leak information about even the existence of a resource, other than perhaps via side channels.

From a behavioral perspective, it can be really nice for clients if, when read access is denied to an anonymous user, a 401 is returned. However, that subverts the "don't-leak-information-about-resource-existence" principle.

[zachmullen]

Finally, we need to decide on a method for providing public access either by a column on the tree table or by a default group. I'm leaning toward the group method because I'm not sure how to add the condition inside the has_perm query.

A group permission check seems like it requires a JOIN of at least 3 tables. It may be worth considering an optimized path like a public column on the Tree, which could obviate that database query entirely. I guess the decision comes down to how difficult it is to achieve that column check in our own code.

[jbeezley]

A group permission check seems like it requires a JOIN of at least 3 tables. It may be worth considering an optimized path like a public column on the Tree, which could obviate that database query entirely.

I realized I misspoke. We can handle has_perm in python. The issue is with integrating into get_objects_for_user. It returns a filtered queryset, but we would need to find a way to add a special OR condition inside the filter.

[jbeezley]

I'm to the point now where I need to design the rest interface for permissions. I have two different options in mind:

1. Simple, something similar to:

   
   POST /folders/:id/permissions permission=read user_id=1
   DELETE /folders/:id/permissions permission=read user_id=1

POST /folders/:id/permissions permission=write group_id=1 DELETE /folders/:id/permissions permission=read group_id=1

GET /folders/:id/permissions => { read: {users: [1, 4], groups: [1]}, write: {users: [2], groups: []}, admin: {users: [3], groups: []} }

2. Elaborate--Full CRUD subroutes on `/folders/:id/permissions/users` and `/folders/:id/permissions/groups` using drf-nested-routers.

The other complication is what to do about permissions on users and groups themselves.  If we want user information to be private, then it begs the question of what to do about user permissions on a tree that the admin of the tree doesn't have access to.  Option 1 bypasses this for the most part by only returning the id of the users.  We would need to add `/users` and `/groups` routes to look up the actual user/group information in the UI, which could be completed in a follow-up PR.

[jbeezley]

I added an MVP for the permissions api. Individual permissions are serialized like {'id': 1, 'model': 'user', 'permission': 'read'}. It's a little more verbose than what I described before, but it allows the permissions to be returned as a flat list. I'm deferring an api for user group information for a future PR.

There are all sorts of edge cases we could add handling for e.g. preventing explicit permissions for admin users, preventing duplicate permissions for a single user, adding special semantics for public permissions, etc.

[jbeezley]

This is almost ready for a more detailed review. The only major thing I might want to change is swap out id's for group/user names in the permissions API.

[mgrauer]

@jbeezley Can you please update this docs as part of this PR, and add all DKCN specific assumptions to the doc that drove your design?

https://github.com/girder/dkc-next/blob/master/docs/roadmap-authorization.md

[jbeezley]

I added some documentation to the model/permissions layer. I think it addresses the motivations for the code at a high level. The roadmap doc can go into more detail as to policy decisions.

[mgrauer]

@jbeezley what review do you need of this (after resolving merge conflicts) to get this merged? Is it code or comments or both?

[jbeezley]

I guess mostly the comments.

[mgrauer]

@brianhelba @zachmullen could one of you review comments in this PR?