daisylb / bridgekeeper

Django permissions, but with QuerySets
https://bridgekeeper.leigh.party/
MIT License
42 stars 14 forks source link

Instance checks without instance should return true (maybe) #24

Closed Ochita closed 3 years ago

Ochita commented 3 years ago

Hi! Firstly, I want to thank you for this handy module. It's really great. And secondly, I want to discuss a bit about R, Is and In rules behaviour when instance object not passed. I try to use bridgekeeper with default django modelAdmin, I write a little mixin, inspired by your code and some django-rules code. It provides needed functionallity: filter objects list, filter objects in selects, check permissions.

from django.contrib.auth import get_permission_codename
from bridgekeeper import perms as global_permissions

class BridgeKeeperModelAdminMixin:
    def get_queryset(self, request):
        qs = super().get_queryset(request)
        opts = self.opts
        codename = get_permission_codename('view', opts)
        perm = '%s.%s' % (opts.app_label, codename)
        if not global_permissions.get(perm):
            codename = get_permission_codename('change', opts)
            perm = '%s.%s' % (opts.app_label, codename)
        if global_permissions.get(perm):
            return global_permissions.get(perm).filter(request.user, qs)
        else:
            return qs

    def formfield_for_foreignkey(self, db_field, request, **kwargs):
        model_field = super().formfield_for_foreignkey(db_field, request, **kwargs)
        # noinspection PyProtectedMember
        model_meta = model_field.queryset.model._meta
        perm = '%s.view_%s' % (model_meta.app_label, model_meta.model_name)
        if not global_permissions.get(perm):
            perm = '%s.change_%s' % (model_meta.app_label, model_meta.model_name)
        if global_permissions.get(perm):
            model_field.queryset = global_permissions.get(perm).filter(request.user, model_field.queryset)
        return model_field

    def has_view_permission(self, request, obj=None):
        opts = self.opts
        if not obj:
            codename = get_permission_codename('possible_view', opts)
        else:
            codename = get_permission_codename('view', opts)
        perm = '%s.%s' % (opts.app_label, codename)
        if global_permissions.get(perm):
            return request.user.has_perm(perm, obj)
        else:
            return self.has_change_permission(request, obj)

    def has_change_permission(self, request, obj=None):
        opts = self.opts
        if not obj:
            codename = get_permission_codename('possible_change', opts)
        else:
            codename = get_permission_codename('change', opts)
        return request.user.has_perm('%s.%s' % (opts.app_label, codename), obj)

    def has_delete_permission(self, request, obj=None):
        opts = self.opts
        if not obj:
            codename = get_permission_codename('possible_delete', opts)
        else:
            codename = get_permission_codename('delete', opts)
        return request.user.has_perm('%s.%s' % (opts.app_label, codename), obj)

But django admin calls has_*_permission without object for checking what links and icons show to user on first page. And if i had the rule

perms['access_test.view_user'] = is_authenticated & (R(company=lambda user: user.company) | is_super_company)

it returns false without object for non supercompany user and didn't show Users link on page. So i need to make additional condition and additional rules for checking possibility for actions

perms['access_test.possible_view_user'] = is_authenticated

This case works fine. But maybe R conditions should return True without instanse and there is no need for addtitional rules? Because you can do anything with nothing, it's didn't break the law)) User possibly had some objects to view.

What do you think? Does this break something? I don't know if it used somwhere else without instance.

philipstarkey commented 3 years ago

I know this is a bit late (and also I'm not the project maintainer....so take what I say with a grain of salt!) but I think that you can't allow check(user, None) to return True or else it would result in global permission checks using user.has_perm() to return True when no instance is provided. That's certainly not what you want as it deviates from expected Django behaviour and could let users access resources throughout your application code when they shouldn't.

I think what you would want to do for your admin mixin is something similar to how bridgekeeper handles the rest_framework integration here. That is your has_*_permission() methods, when called without an object, should use the is_possible_for() method of bridgekeeper rules. Then the global (without object) level checks return True/False depending on whether it's possible for the user to have access to instances, and then the more specific object level checks are done as usual, without you having to define additional rules like you did in your example.

Hope that makes sense and maybe gives some ideas to you or other people coming across this.

Ochita commented 3 years ago

Unfortunately , this was used in prototype which already closed. But as I know, You absolutely right. There was problems in other part of system with this empty checks, so it used bridgekeeper without any changes. I'm not shure, did they find out is_possible_for() method or use my version with semiduplicated rules, cause I left this project almost immidiatelly. But it's great answer, thank you. I think I will try to use this approach if I would work with same task in future.

daisylb commented 3 years ago

Hey, sorry this flew under my radar! @philipstarkey is right, is_possible_for is what you want to use here.