Design proposal for adding pluggable permission settings support

[thomasyip]

First, thanks @nitely for the great work on Spirit. I looked into the code of multiple forum projects. I like Spirit most. The UI looks clean, the code is pythonesque, the folder structure makes sense. Thanks a lot.

I have a need to support multiple organizations with the same project. I think the best approach is to implement an object-level permission support.

(The following link gives an introduction on how object level backends can be implemented.
Background: [https://github.com/djangoadvent/djangoadvent-articles/blob/master/1.2/06_object-permissions.rst])

I think it better to make certain changes directly into Spirit's (instead of making it as an plugin or another project). I also implemented a reusable authentication backends: [https://github.com/beedesk/django-trusts], which is probably the most true to Django philosophy authentication backends around. The change I proposed here would work with my backends, but it is not backends specific. And, I also like to support model level permission at the same time (which would work with Django's builtin auth backend).

Here are what I am going to do, if you can make some comments and let me know what you think, it would be awesome.

1. Introduce a new decorator, namely @permission_required. The syntax would be similar with the builtin one, but the actual user_passes_test must be customizable with django.conf.settings (namely, SPIRIT_PERMISSION_PASSES_TEST_BACKEND). It is required because the builtin decorator does not handle object level permission check.
2. Introduce another pair of decorator: @permissible_queryset and @permissible_object . The object takes these parameters: (app_label, module_name, args_to_fieldlookups_dict={}, get_to_fieldlookups_dict={}).
   More than one of these decorators can be used to decorate a view func. The decorators will intercept the request and kwargs to be passed to the view func and bind the object to the request object.
   I believe the separation of @permission_required and @permissible_queryset enables both model-level permission check and object-level check. In fact, I think we would need both. For example, create permission is usually a model-level permission, while delete / update / read is at object-level.
3. Update each models's Meta's: default_permissions = ('add', 'change', 'delete', 'read', 'moderate')
4. Implement a SPIRIT_PERMISSION_PASSES_TEST_BACKEND that basically ignore permissible_queryset and permissible_object, and simply call existing spirit.core.utils.moderator_required and login_required decorator.
5. Go into each view and update each method to add (or replace) the decorators we introduce. This is actually the difficult part. There are about 50 to 60 existing calls when I did a simple egrep. And, I probably need a lot of help from you.

I think once we done that we will able to pass all existing tests, but enable other backends to be added.

The SPIRIT_PERMISSION_PASSES_TEST_BACKEND backend that actually passes those permissible object / queryset into user.has_perm('sprit_topic.can_change') can also be included into spirit. But it might also make sense to be included in the actual backends project.

Does it look reasonable? Is it something you might consider including into Spirit?

[nitely]

I'll try to get this reply short and to the point.

Note: I have not thought about all the require changes to implement such a feature and I have not gone through the codebase to refresh my memory (which would require much more time to actually write an accurate reply), so I could be wrong about some things.

Spirit must support the default built-in backend out the box (with no changes required), there are many folks using auth backends other than the default, I would prefer not to make the integration process more difficult than it is now.

I believe the only model requiring permissions is the category model. Besides the permission check at the view level, some of the queries (ie: get topics, likes, comments, etc for all categories to show them at the profile, topics list, etc) must be modified.

The idea is to support permissions by group (not by single/individual user), using both Django built-in features.

Ultimately you could create groups like MyOrg1, MyOrg2, etc and assing them to the categories you want them to be allowed to read, create topics, reply, etc.

PS. I failed at the "short reply" part

[thomasyip]

RE: Spirit must support the default built-in backend out the box (with no changes required)

When I said SPIRIT_PERMISSION_PASSES_TEST_BACKEND, I did not mean authentication backends. The word backends just mean pluggable code that can be defined in settings. What I mean is that the code run in permission_required() has to be pluggable. My proposal will pass all existing tests, with any authentication backends that currently passing the tests.

RE: only model requiring permissions is the category model

You might need to help me understand why "only model requiring permissions is the category model". I followed the code before I wrote that up. The example I was looking at is topic. Say, I don't have the permission, and I have the topic pk somehow, and try to view the details (views.details).

The view will try to get the details with get_public_or_404, which in turn use do a Topic.get_public_or_404, which filter with queryset.visible().open(), which becomes unremoved().public().open(), which translate to:

(Q(category__parent=None) | Q(category__parent__is_removed=False), category__is_removed=False, is_removed=False).(category__is_private=False, is_closed=False, pk=pk, user=user)

From my understanding of the code, it is not limited by category. Perhaps I missed something.

RE: The idea is to support permissions by group

It was exactly what I want to achieve. Have permission defined by django.contrib.auth.models.Group that is builtin to django.

The @permission_required decorator will be used like this:

    @permission_required('spirit_topic.can_read')
    def detail(request, pk, slug):
        # ...

And, the way to grant permission is by the builtin group mechanism:

def grant_user_group_permssion_to_model(user, group_name):
    # get perm by name
    perm = Permission.objects.get_or_create(codename='can_read', ...)
    group = Group.objects.get(name=group_name)

    # connect them
    user.groups.add(group)
    perm.group_set.add(group)

[nitely]

I just read the django's permission model source code and found this:

Permissions are set globally per type of object, not per specific object instance.

Which makes sence given how the django admin works, I did not think of that. I did't get what you mean by object-level permission until now, sorry 😅

In my previous comment I was not aware of this. So, there is no use for the built-in permissions.

I'd create a CategoryPermission model to hold the permissions (like the ObjectPermission but specific, so no need for ContentType) and a GroupPermission to relate category and group-permissions (many-to-many).

What I mean is that the code run in permission_required() has to be pluggable.

I don't think there is a need for that. And I dare to say it's not possible to have a plugable permission model, because of how the database must be queried (ie: to get all "visible" topics) and how the permissions must be granted/assigned.

I fail to see why would you require this anyway.

The example I was looking at is topic. Say, I don't have the permission, and I have the topic pk somehow, and try to view the details (views.details).

Like this:

topic = Topic.objects.get_public_or_404(pk, request.user)
GroupPermission.objects.get_or_403(permission__category=topic.category, permission__can_view=True, group__in=request.user.groups.all())  # May not even work

There may be a better way to do this, probably by creating a ManyToMany field with through parameter, docs, or using _set (ie: category__group_permision_set__group__user_set=request.user) but I don't quite remember if this kind of crazy queries are possible, then you can do a single lookup. It gives you the idea, though. I'm open for suggestions.

[nitely]

This is related to #67 and #88 btw.

[thomasyip]

This article gives some background on how to implement objects permission on top of django's. Django's api is designed to allow object level, but django does not provide the backend. The author of the article is a django core developer.

https://github.com/djangoadvent/djangoadvent-articles/blob/master/1.2/06_object-permissions.rst

[nitely]

Spirit does not support the django admin interface. So, I don't see the point of implementing a new backend, seems like overengineering to me, it's a neat idea though.

[thomasyip]

Ah... The admin interface uses permission, not the other way around. The auth / permission stand on its own. I saw many occurrence of @login_required() in spirit, which is part of auth / permission in Django. In fact, the @permission_required() decorator is just in the same file as @login_required().

I am not asking Spirit to implement an auth backend. I did one and there are 30 of them out there to choose from.

I brought up the article just as an explanation on why object-level permission api is there in Django already, and you can use it as part of standardized api (even django doesn't ship with a builtin auth backends that support it).

[nitely]

Ah... The admin interface uses permission, not the other way around. The auth / permission stand on its own.

Yes, I read the source code. I was refering to this which seems to be the main purpose of implementing it as a backend instead of just a custom decorator.

[thomasyip]

I think the author just use the admin as an example application for his object permission backend. If you look at the ObjectPermissionMixin, you can see that it is using existing api built into Django, namely, user.has_perm(): docs.djangoproject.com auth models

def has_delete_permission(self, request, obj=None):
        opts = self.opts
        return request.user.has_perm(opts.app_label + '.' + opts.get_delete_permission(), obj)

My proposal is to do it in a decorator, replacing some of the existing @login_required and @moderator_required of Spirit with @permission_required one.

[nitely]

Honestly I don't see the point. You want to implement this feature in a overly generic way (backend, contentType, etc) when it's so much simpler to create a custom decorator which is required anyway. And make it plugable, again, why? permissions would be handled within the Spirit's admin interface.

I don't want to add yet another dependency for such a simple task.

The idea seems neat, but the implementation tho...

[thomasyip]

I am using Spirit as a reusable-app along with other apps and my existing apps in the same project. So, I need a permission system that all other apps also use.

And, I have a need to support multiple organizations in the same app. Each organization would have its own moderators / admins. So, the complexity is necessary for my usage.

I agree that it is too generic if one is always using Spirit standalone and does not support multiple organization. I also aware making something generic takes a lot more work and you would need to make compromises.

By any stretch, if you open to general direction of the proposal, we can work more collaboratively. Otherwise, I will just concentrate on my own fork, and not testing it against other backends (which probably save me a bunch of work too). But then, my end solution would be divert with yours over time, and I will have harder and harder time merging your new changes. And, I wouldn't able to contribute other fixes / enhancement back.

So, that's my trade offs.

[nitely]

I am using Spirit as a reusable-app along with other apps and my existing apps in the same project. So, I need a permission system that all other apps also use.

Oh, I see.

And, I have a need to support multiple organizations in the same app. Each organization would have its own moderators / admins. So, the complexity is necessary for my usage.

You can share the groups. You could create Org1 members, Org1 Moderators and Org1 Admins groups. But why do you want to share the permission system? Couldn't you just use the Spirit's admin ui to assign the forum permissions and your own admin ui (or django admin) for everything else?

I think you will find you won't be able to share the permission system anyway. You will need permissions such as can_view, can_comment, can_create_topic, etc (attached to Category model) and those are specific to Spirit. I was thinking of ObjectPermission which has the permissions fields harcoded, but you could make it dynamic (ie: FK in other model and a CharField).

[thomasyip]

The admin for other organization is going to be my customer who is not a developer. They will not be using Django admin interface, that's for sure. It is a lot of confusion for them to use two interfaces. So, if I don't modify Spirit, I would create some new views that modify Spirit model and modify other permission in the same POST request, among other code I need to write to handle two permission systems.

I feel the immediate work of implementing this proposal is about the same, and would be less work down the road if I just have one system. Spirit code looks pretty clean, I need the proposed decorator for my other views, which is something I need to develop anyway. The real work of this proposal is to add @permission_required('spirit_topic.can_read'), which is completely generic and @permissible_object() decorators to about 50 to 60 places. The first pass might be simply go check whether user has the permission on the category. Something like that:

# topic / comment

@permissible_object('spirit_comment', 'topic__category', args_to_fieldlookups_dict={'pk': 'pk'})
@permission_required('spirit_category.can_change')
def update(request, pk):
    # ...

Also, one of the "app" I use is tastypie, which expose "models" to RESTful interface. It has a way to piggy back on Django's builtin permission. I can use another non-Django permission for Spirit's model. But then, it is something to develop if I am not using a single system.

Permission such as can_view, can_comment, can_create_topic. Since Django 1.7 (if I remember correctly, you can define them in models:

class Category(models.Model):
    class Meta:
        default_permissions = ('change', 'delete', 'create', 'comment', 'create_topic')

With this line, migration will create those permissions for you. So, while django's permission is a bit of a compromise, it does make something handy.

[nitely]

Let's see if get it right. If Spirit implements this feature on top of django's Permission model and backend.has_perm(...) you would be able to hook your own permission system? Is that the only thing required?

How would filter the allowed categories and topics within the topic's list view?

[thomasyip]

Great question. I didn't think about it before you ask.

I think it would requires an additional method in SPIRIT_PERMISSION_PASSES_TEST_BACKEND. And, spirit code would use that method to get the filter on.

So, the default backends, to be included, would look like that:

class SpiritPermissionBackend(AbstractSpiritPermissionBackend):
   def perm_test_func(user, perm_code, obj_or_list):
        # ... similar to `django.contrib.auth.decorator#user_passes_test`'s test_func
        return user.has_perm(perm_code)  # skip obj as default backend does not handle obj

    def filter_permitted_queryset(perm_code, user, manager_or_query_set):
        # perm_code is in the format of 'spirit_category.can_change'
        perm = Permission.objects.get_by_natural_keys( ... )

        return manager_or_query_set.filter(
            topic__category__group__permission=perm, 
            topic__category__group__user=user
        )

And, my backend would look like that:

class MySpiritPermissionBackend(AbstractSpiritPermissionBackend):
    def perm_test_func(user, obj_or_list):
        return user.has_perm(perm_code, obj_or_list)

    def filter_permitted_queryset(perm, user, manager_or_query_set):
        # ...
        return my_auth_backend.get_permissible(perm, manager_or_query_set, user)

[nitely]

Seems reasonable, let me sleep on it.

What's the purpose of perm_test_func?

To filter comments, likes (profile view), categories (topic's list) and future features, would have to add a prefix though:

def filter_permitted_queryset(self, perm, manager_or_query_set, prefix_lookup=''):
        # ...
        return manager_or_query_set.filter(**{
            '%(prefix_lookup)scategory__group__permission' % {'prefix_lookup': prefix_lookup}: perm,
            '%(prefix_lookup)scategory__group__user' % {'prefix_lookup': prefix_lookup}: user_obj
        })

I think this is too noisy and too implicit:

# topic / comment

@permission_required('spirit_category.can_change')
@permissible_object('spirit_comment', 'topic__category', args_to_fieldlookups_dict={'pk': 'pk'})
def update(request, pk):
    # ...

What about:

# topic.premissions.py

def test_permission(perm, obj):
    assert isinstance(obj, Category), 'test_permission requires a Category as obj param'

    if not request.user.has_perm(perm, obj):
        raise ForbiddenError

# everyone can update so can_view_topic is enough.

def can_view_topic(request, pk, *args, **kwargs):
    test_permission('spirit_category.can_view_topic', get_or_404(Topic, pk=pk).category)


def can_remove_topic(request, pk, *args, **kwargs):
    test_permission('spirit_category.can_moderate', get_or_404(Topic, pk=pk).category)


def can_create_topic(request, category_id, *args, **kwargs):
    test_permission('spirit_category.can_create_topic', get_or_404(Category, pk=category_id))


# topic.views.py

@access_required(func=can_create_topic)  # Do not name it *permission_required* it will be extrange for other devs since Django already has a decorator with that name.
def create(request, pk):
    # ...

It's more explicit.

[thomasyip]

What's the purpose of perm_test_func?

The problem of Django builtin @permission_required decorators is that it has no way to pass objects to has_perm(). So, we need our own. I would just use exactly the same function argument as the builtin one. But, we need one.

Here is django's code:

# file: django.contrib.auth.decorators.py 

def user_passes_test(test_func, login_url=None, redirect_field_name=REDIRECT_FIELD_NAME):
    # see source file: https://github.com/django/django/blob/master/django/contrib/auth/decorators.py#L12

def permission_required(perm, login_url=None, raise_exception=False):
    def check_perms(user):
        # ...
        # First check if the user has the permission (even anon users)
        if user.has_perms(perms):
            return True
        # ...
    return user_passes_test(check_perms, login_url=login_url)

So, we would need an alternative implementation.

def permission_required(perm, login_url=None, raise_exception=False):
    def check_perms(user):
        # ...
        if spirit_perm_backends.has_permission(request, perm)
            return True
        # ...

    return user_passes_test(check_perms, login_url=login_url)

I am hoping that one day, Django would have a better @permission_required decorator. When the day come, we would change the import from spirit.utils.decorators import permission_required to django.contrib.auth.decorators import permission_required and call it a day, and stop maintaining our own decorators.

[nitely]

But according to this you can use has_perm(perm, obj) directly, as shown in the last part of my previous reply.

[thomasyip]

Interesting point. Yes, if we don't like to use decorator, the has_perm(s) method is there already. (there is a small catch, the builtin backends will always return False if you pass in any object. So, we would need a setting somewhere saying don't pass obj into has_perm() calls, by default.)

I like decorator for permission, as it makes it declarative and quite readable. But, I am not dead-set on using decorator.

[nitely]

uhm, the custom backend will return True if an obj is passed (and passes the test), if not then it fallbacks to the default backend. It maintains the default behaviour.

We want to do something like request.user.has_perm(perm, get_or_404(Topic, pk=pk)), this would work fine.

The decorator is the way to go, IMHO. Although, doing the check within the view will save a db query.

[thomasyip]

Awesome. I think we aren't differ that much in direction.

I am coding the decorators a bit, perhaps I can have a PR (which would not be ready to merge at all, so don't worry when you see it) in a day, if I am not discovering too many thing that I didn't think of.

[nitely]

Great!

There is one other thing, non-moderators should not be able to remove topics, comments, etc or any of the other moderator actions. What I mean is these should not be individual permissions. There should be a can_moderate permission that allows to do all the actions.

The reason is regular users should not ever be able to remove topics, comments, etc. Spirit is a very opinionated forum about some things. That's how currently works, though.

I reflected it in the above (edited) reply.

def can_remove_topic(request, pk, *args, **kwargs):
    test_permission('spirit_category.can_moderate', get_or_404(Topic, pk=pk).category)

[thomasyip]

I am thinking of finishing the decorators implementation and use it on a couple of place as examples. I will keep in mind on can_moderate in case it is the examples.

Not letting user delete make sense, in general.

[nitely]

Seems reasonable, let me sleep on it.

So, I've been thinking a lot on this, I don't think "organizations" is a use case I want to support. At least not my concept of what an organization is (with their own administrator and moderators).

This is what I want in a forum:

• Assign global administrators (as it is now)
• Assign global moderators (as it is now)

I think I don't want moderators groups. Moderators that are able to moderate only the categories they have access to. I may change my mind about this one.

I certainly don't want administrators groups. Administrators that are able to create categories, assign permissions, create categories, edit users, etc only under their group is something I don't plan to support, I believe you won't find that in any forum out there, because it's something very specific to organizations.

[thomasyip]

Thanks for taking the time and effort to think thru it. You raised a lot of great questions in this discussion and I am very thankful you are taking the time to discuss it with me.

I am not asking Spirit to support the way I structure the permissions. I am asking to have a pluggable way to define permission. On the other hand, it means we define permission in decorators (which is the bulk of the work). The pluggable permission could enable someone to host multiple different forums on the same project, for example (both stackoverflow.com and superuser.com, for example), among other scenario that we might not have thought of. I think the question is whether you want add the complexity to allow for people to use Spirit in an unintended way at their own risk. It is your judgement call on balancing the integrity and the reach of the project.

single administrator

It probably makes sense, for my use case, to have only a single Spirit administrator (developers and DBA in my company). In other part of the site, where they manage documents, they might have "admin" role for each company. But, I have not flushed out the idea to the point I can say for sure.

moderators groups

In my scenario, I would have a "public" group, which includes basically all users of the system. For each company, there is a "organization" group to include all their users. One might be the "admin" for the document part of the site. User and organization are many to many relationship. And, the "admin" group of other part of the site (document management, for example) might also be a moderator of Spirit forum.

I agree, it is not a very popular way of using forum. Probably not a very popular way of using Django as well. I say that because group (and per-object) permission in Django isn't very popular.

Anyway, I feel I am half way done on the decorator. I will make a pull request anyway, if you don't mind, such that you can see the direction that I am going at (the PR will be buggy and incomplete), but don't feel obligated that you need to merge it (even when it is completed) if you don't think it fits the direction that Spirit is going.

[nitely]

Thanks for taking the time and effort to think thru it. You raised a lot of great questions in this discussion and I am very thankful you are taking the time to discuss it with me.

NP, I'm always open for discussing features.

I am asking to have a pluggable way to define permission.

I'm not against it, that's fine by me.

It probably makes sense, for my use case, to have only a single Spirit administrator (developers and DBA in my company).

What I'm opposing is having an administrator per group. No one within your organizations will be able to use the Spirit's admin features (assuming you want to integrate part of it to your custom admin section) including creating categories, editing users and so on.

A single global administrator is fine, it's how it works now.

So, the "admin" of other part of the site might also be a moderator of Spirit forum.

This I don't know if I want to support. Moderators per group will be a major engineering effort, be aware.

[thomasyip]

This I don't know if I want to support. Moderators per group will be a major engineering effort, be aware.

I think that it is the responsibility of the permission system (and user to set it up), once we "decorated" the view like this:

@permissible_object('spirit_category', fieldlookups_kwargs={'category_topic_comment_pk': 'pk'})
@permission_required('spirit_category.can_moderate')
def update(self, pk):
    pass

For the out-of-the-box spirit configuration, @permissible_object is not being considered. So, it will returning if request.user has the permission (or is in a group that has the permission).

For object-level permission backends, it will take permissible_object into consideration. And, only pass the test if request.user has the permission and is associated with that particular category (which is an implementation details of the object-level permission system).

For example, with django-trusts backend, I would be doing the following to grant a permission to a specific category. (assuming that the user is already has the django's 'spirit_category.can_moderate' permission.)

def grant_permission_to_user(user, category)
    trust = Trust.objects.get_or_create_settlor_default(settlor=request.user)
    junction = CategoryJunction(trust=trust, category=category)
    junction.save()

[nitely]

You also have to filter the categories/topics/comments/flags/future_features by group permissions within the Spirit's admin (when moderator get access to some of the sections, not implemented yet), moderation actions (like move topic and comments) and future features.

[thomasyip]

Let me make sure I understand you correctly. Are you saying that grant_permission / check_permission need to work with group? Yes, the backends would support that. I was just picking user.user_permissions as an example. auth_group would work similar enough.

https://github.com/django/django/blob/master/django/contrib/auth/models.py#L231

Or are you describing the decorators logic?

[nitely]

Neither. I give you an example: to move a topic you have to pick the category from a list, that list must by filterd by the categories the moderator has access to. Same for everything else I listed above.

[thomasyip]

Then, you need to check permission on both (which won't be part of the PR I am sending soon, but it was something I thought of)

@permissible_object('spirit_category', fieldlookups_kwargs={'category_topic_pk': 'topic_pk'})
@permissible_object('spirit_category', fieldlookups_kwargs={'pk': 'category_pk_moving_to'})
@permission_required('spirit_category.can_moderate')
def update(self, topic_pk, category_pk_moving_to):
    pass

[nitely]

Well, yes. But also you have to filter the list showed in the ui, you will have to do it for regular users anyway (when publishing a topic), it was just an example.

[thomasyip]

Yes, we need:

    def filter_permitted_queryset(self, perm, manager_or_query_set, prefix_lookup='')
       pass

So, it is more than just the decorators.

[nitely]

That's correct. For moderators it's just the categories list/dropdown-menu that needs to be filtered for now (I think). When they get access to the admin interface it will be the flagged comments and removed topics.

[thomasyip]

Okay, I am running out of time and need to context switch to something else (tax and accounting).

But, I am hoping the PR gives you some concrete sense on the decorators approach. I should be able to coming back onto this tomorrow.

[nitely]

No worries. I did a quick read, I'll review it later.

[thomasyip]

Closing it, as it isn't feasible to support more than one object-level backend.