New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add rules/permissions system #353

Open
nicksellen opened this Issue Aug 29, 2017 · 13 comments

Comments

Projects
None yet
3 participants
@nicksellen
Copy link
Member

nicksellen commented Aug 29, 2017

To implement yunity/karrot-frontend#550 we need a way of defining role permissions, e.g. is this user an admin of the group?

Django includes object-level permissions concept:

Django’s permission framework has a foundation for object permissions, though there is no implementation for it in the core. That means that checking for object permissions will always return False or an empty list (depending on the check performed). An authentication backend will receive the keyword parameters obj and user_obj for each object related authorization method and can return the object level permission as appropriate.
https://docs.djangoproject.com/en/1.11/topics/auth/customizing/#handling-object-permissions

Django Rest Framework supports using Django object-level permissions: http://www.django-rest-framework.org/api-guide/permissions/#djangoobjectpermissions

The standard implementation is https://github.com/django-guardian/django-guardian - this adds some database models to store the permissions, and can be used like (from the readme example):

>>> from django.contrib.auth.models import User, Group
>>> jack = User.objects.create_user('jack', 'jack@example.com', 'topsecretagentjack')
>>> admins = Group.objects.create(name='admins')
>>> jack.has_perm('change_group', admins)
False
>>> from guardian.models import UserObjectPermission
>>> UserObjectPermission.objects.assign_perm('change_group', jack, obj=admins)
<UserObjectPermission: admins | jack | change_group>
>>> jack.has_perm('change_group', admins)
True

An alternative implementation is https://github.com/dfunckt/django-rules - this uses rules/predicates to check permissions - which may in turn use your existing database models to check for permissions gained due to the model relationships (e.g. can edit store? = user.groups.includes(store.group)) without needing to add additional entries to the permissions table as in django-guardian.

I like the django-rules philosophy.

However, we don't actually need anything extra as Django Rest Framework already sets us implement arbitrary logic in the permission classes, so why add anything else? A few reasons:

  • makes the rules/permissions easily reusable across REST API viewsets
  • separates the permissions from the API - we maybe want to check permissions elsewhere in the code (in websocket handlers, in workers, in cron tasks, in management commands, in serializers)
  • buys into the django object-level permissions ecosystem, perhaps there are other tools that can make use of this

Django Rest Framework also mentions some third party permission systems http://www.django-rest-framework.org/api-guide/permissions/#third-party-packages. The most interesting of those to me was https://github.com/dbkaplan/dry-rest-permissions - but I didn't like it's idea to put permission methods that take the request object in the model classes (they should not care about things like requests).

The other thing that would be nice is a way to get the permissions for a given user/object combination as data that can be returned in the API to be used to show/hide different user interface elements.

I did not see anything specific in django-rules for doing this, but as it's actually quite simple I think it should be possible to write one easily (e.g. https://stackoverflow.com/a/31301754).

Permissions and roles are a bit tricky when it comes to listing thing (e.g. give me a list of all groups where I am an admin) and django-rules does not help us here. django-guardian can provide this (see http://django-guardian.readthedocs.io/en/stable/userguide/check.html#get-objects-for-user).

However, if going with django-guardian we are then really tying ourselves to the django-guardian permission database as part of our model, whereas I prefer the approach to define the relationships using our own model structure in such a way it makes sense to query them directly, and using django-rules to provide the django-compliant object-level permissions "view".

So, to get a bit more concrete for the group admins - there is already a table that defines the relationship of a user to a group - thats the join table for user <-> group many to many relationship. However, it's not currently an explicit model, and django is not happy about migrating to use an explicit model on the join table :/ ... but I think it would be worth finding a way.

Once/if that is in place, I would look at storing role names as postgres array-field strings (see https://docs.djangoproject.com/en/1.11/ref/contrib/postgres/fields/#arrayfield).

So a few steps:

  • get feedback on this proposal (primarily from @tiltec)
  • find out how to add through models in migrations, could get messy... (merged!)
  • actually try out django-rules a bit
  • if it all seems good, actually implement admin roles :)
@tiltec

This comment has been minimized.

Copy link
Member

tiltec commented Aug 29, 2017

Thanks for your research!

django-rules implementation steps

I like the django-rules concept of predicates and rules. From what I get, these would be the implementation steps:

  1. Use the default permissions can_add_pickupdate, can_change_pickupdate, can_delete_pickupdate or add custom permissions to the model class if necessary:
    class Meta:
        permissions = (
            ("view_task", "Can see available tasks"),
            ("change_task_status", "Can change the status of tasks"),
            ("close_task", "Can remove a task by setting its status as closed"),
        )
  1. Implement predicates. The user is always the first argument.
@rules.predicate
def is_book_author(user, book):
     return book.author == user
  1. Set permissions
rules.add_perm('books.change_book', is_book_author | is_editor)
  1. Use DRF DjangoObjectPermissions
class ExampleView(APIView):
    permission_classes = (IsAuthenticated, DjangoObjectPermissions)

Questions

  • Can the predicates also be used outside of django-rules, i.e. called directly?
  • What error messages will DRF return when using DjangoObjectPermissions? Currently we have custom permission classes with translatable error messages.

Permission status through API

It would be nice to return the permissions as part of the data, to save us duplicate logic on the frontend:

GET /api/groups/5/

{
"id": 5,
"description": "my group",
"permissions":
  {
    "change_group": true,
    "delete_group": false,
    "add_store": true
  }

As you said, it seems to be quite easy to solve. Either with the method from the Stackoverflow post. Or it could be solved manually with SerializerMethodFields and calling request.user.has_perm for each of the fields.

Or a new endpoint could make sense to keep user-specific stuff out of the group data: /api/groups/5/permissions/


Handle listing by permission

give me a list of all groups where I am an admin

This is tricky with django-rules indeed. If it would become necessary, we could either do horribly inefficient things like filtering the queryset based on has_perm. Or we would need to keep an equivalent set of filter queries around.

The second approach (defining database filters) would open up a completely different way of handling permissions:

  1. define appropriate queries on the model manager
def deletable_by(self, user):
    return self.filter(... some condition)
  1. in the API viewset, filter the queryset depending on the action
def get_queryset(self):
    if self.action == 'destroy':
        return PickupDate.objects.deletable_by(self.request.user)

I have to think more about this method, but it seems to have some obvious drawbacks:

  • needs some additional effort to return telling errors instead of 404: not found
  • viewsets get much more heavy (could be mitigated by convention and mixins)
  • completely bypasses django (object) permissions
@nicksellen

This comment has been minimized.

Copy link
Member

nicksellen commented Aug 29, 2017

I like the django-rules concept of predicates and rules. From what I get, these would be the implementation steps:

My understanding is that step 1 is not required, but would have to find out.

Can the predicates also be used outside of django-rules, i.e. called directly?

Yes! It's one of the selling points. I think you can use them at multiple levels:

  • the predicate is literally just a function: is_book_author(user, book)
  • the django-rules ruleset level (unsure of usage, something with rules.rulesets.test_rule)
  • the django-rules permission level (unsure of usage, something with rules.permissions.has_perm)
  • the django permissions level: user.has_perm('can_modify', book)

What error messages will DRF return when using DjangoObjectPermissions? Currently we have custom permission classes with translatable error messages.

From a quick code reading looks like standard 403, 404, etc.. If you don't have permission you should not be trying (find out what you can do first).

The second approach (defining database filters) would open up a completely different way of handling permissions:

Perhaps this approach could be used with django-rules for the permissions that are easily represented as querysets - can just use the queryset inside a predicate. And/or define querysets next to the predicates that you promise will be equivalent (can_edit_book_predicate, can_edit_book_queryset).

I think limiting permissions to only things that can be represented with a queryset could be unnecessarily limiting, and many object-level permissions don't need to be list queryable in reality.

django-guardian does provide filterable permissions (includes DjangoObjectPermissionsFilter), but then the permission model is limited to user/action/object, and permission/role knowledge kept outside of our normal models.

The way I see the proposal to use rules is that the permissions/roles are stored in our normal models, and django-rules just provides a convenient interface for that information, with nothing stopping us also providing querysets if they are possible and useful.

@tiltec

This comment has been minimized.

Copy link
Member

tiltec commented Aug 31, 2017

What error messages will DRF return when using DjangoObjectPermissions? Currently we have custom permission classes with translatable error messages.

From a quick code reading looks like standard 403, 404, etc.. If you don't have permission you should not be trying (find out what you can do first).

This taps also into another issue: how do we do error handling in the frontend? There is not much currently, mostly forwarding the errors from the backend as plain text. We should open another issue about it and find out if we need custom error messages from the API.

@nicksellen

This comment has been minimized.

Copy link
Member

nicksellen commented Sep 1, 2017

This taps also into another issue: how do we do error handling in the frontend?

I think it is not relevant for this topic though, the user should already know if they can perform an action or not (because the permission info is returned). It's not really an error if they try and do something they are not permitted to, so just returning a status code meaning "no you cannot do this" is fine, there is no information to give to the user that they can usefully act on.

To handle the case that we have made errors in programming the frontend such that lots of invalid permission requests are made, it would seem better to handle this by logging in the backend (maybe can send to sentry?) - and then we get some idea that we are causing these invalid permission paths.

@tiltec

This comment has been minimized.

Copy link
Member

tiltec commented Sep 1, 2017

Backend logging sounds good to me, but may not help frontend devs which are using the dev.foodsaving.world backend.

We could enable Debug = True, but it changes responses in error cases. So it's less suited for beta testing then.
Or all the errors directly go into a new "#it-dev-monitoring" channel, which could create confusion if two devs are working at the same time ("is this my error now?")

Really, some error messages directly from the API would be easier. Maybe we just try out object permissions and see how the errors look like?

@nicksellen

This comment has been minimized.

Copy link
Member

nicksellen commented Sep 1, 2017

I'll keep it in mind :)

@nicksellen

This comment has been minimized.

Copy link
Member

nicksellen commented Sep 1, 2017

Ok, I investigated django-rules.

tl;dr it's nice, but django object permissions is not very well supported in drf - drf permission classes + predicate functions in models should work well

My work is in https://github.com/yunity/foodsaving-backend/tree/add-django-rules - I implemented django rules for the stores.

Comments:

@nicksellen

This comment has been minimized.

Copy link
Member

nicksellen commented Sep 1, 2017

Another approach would be to use django-rules just for the rules bit and not as an implementation of a django permissions backend. But then it's not really adding much. The predicates themselves are just functions. They could be combined into rulesets, but might as well just import the functions directly.

@nicksellen

This comment has been minimized.

Copy link
Member

nicksellen commented Sep 1, 2017

... although then we still don't have a nice way to give the frontend the available permissions for a given object.

Maybe a ruleset per model is an option, something like:

pickup_rules = rules.RuleSet()
pickup_rules.add_rule('edit', is_member & is_allowed)
pickup_rules.add_rule('join', is_member & is_allowed & is_not_full)

Then elsewhere can easily have a function that takes a user, and a pickup, and tells you what they can do to it.

@nicksellen

This comment has been minimized.

Copy link
Member

nicksellen commented Sep 1, 2017

Ok, I hacked together a new approach! https://github.com/yunity/foodsaving-backend/tree/add-rules

Totally gratuitous really. I think using normal drf permissions is actually fine for what we need. I was just enjoying myself implementing this. I think we should probably not use this as I have to access various internal fields in the rules library. Although I do like how it works, but would be better if it could be actually supported by django-rules (ideally by making it officially extendable in such a way).

The main aims are:

  • fully bypass django permissions system (to avoid the crap I described above), connects rules directly to drf permission classes system
  • avoid defining permission names as strings (via imported functions is nicer)
  • allow @detail_route's to define their own permission requirements
  • show the reason permission was denied to use for an error message
  • return the available permissions via the API (I put them under _actions field)

Look in https://github.com/yunity/foodsaving-backend/blob/add-rules/foodsaving/stores/rules.py to see the usage, and https://github.com/yunity/foodsaving-backend/blob/add-rules/foodsaving/utils/ruletools.py to see the implementation.

@tiltec

This comment has been minimized.

Copy link
Member

tiltec commented Sep 2, 2017

Seems to be the perfect solution!

Implementation-wise, could we get rid of django-rules entirely and reimplement the necessary bits? Activity in django-rules seems to be pretty low lately (https://github.com/dfunckt/django-rules/graphs/contributors), so I wouldn't expect them to add more features. But we could ask.

Otherwise, we could fork the django-rules and tailor it to our needs. It wouldn't need to depend on django at all.

@nicksellen

This comment has been minimized.

Copy link
Member

nicksellen commented Sep 2, 2017

Great! Glad you like it.

I'll ask the django-rules person if there is a nicer way to integrate it.

Also, want to explore whether it could/should be set as a base permission class on the viewset, and whether to implement non-object permissions too.

I'd also like to distinguish between 403 permission errors from general error 400, as I described above. I didn't implement that yet.

I guess we could fork it, I would rip out everything that we don't directly use. Would prefer to integrate it though. Low activity for a simple library seems fine, it's basically done!

@nicksellen nicksellen changed the title Add role permissions system to enable group admins Add rules/permissions system to enable group admins Sep 2, 2017

@nicksellen nicksellen changed the title Add rules/permissions system to enable group admins Add rules/permissions system Sep 2, 2017

@escodebar

This comment has been minimized.

Copy link

escodebar commented Nov 16, 2017

Hey Folks!
This could be interesting for you: https://github.com/escodebar/django-rest-framework-rules
Cheers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment