FJNR-inc / dry-rest-permissions

Rules based permissions for the Django Rest Framework
ISC License
75 stars 11 forks source link

[question] Dry rest permissions does not check my object permissions #23

Closed AsifDaum closed 2 years ago

AsifDaum commented 2 years ago

Hi, apologies for the lack of knowledge as I have recently started implementing dry-rest-permissions, but I can't seem to get it to check the has_object_permissions but only the global permissions work for me.

I am fairly new to the permissions system and only recently started coding in django rest framework.

At the moment I am trying to delete a company object by simply having a user call a URL, that URL then gets the current user's active_company and then deletes it only if the current user is the active_company's company_owner.

But what I discovered, is that I somehow can't get has_object_permissions to work anywhere?

If I delete has_write_permission(request), and hit the company_delete URL it gives me the following error:

'<class 'company.models.Company'>' does not have 'has_write_permission' or 'has_company_delete_permission' defined.

This means that it doesn't even look for the has_object_company_delete_permission. And it only checks the global permissions rather than any of the object permissions, what am I possibly doing wrong here?

My model:

class Company(models.Model):
    company_name = models.CharField(max_length=100)
    company_orders = models.IntegerField(blank=True, null=True)
    company_icon = models.ImageField(
        upload_to='media/company_icon', blank=True)
    company_owner = models.ForeignKey(
        User, on_delete=models.SET_NULL, blank=True, null=True)
    company_employees = models.ManyToManyField(
        User, blank=True, null=True, related_name="company_employees")

    def __str__(self):
        return self.company_name

    @staticmethod
    def has_write_permission(request):
        return False

    def has_object_company_delete_permission(self, request):
        return self.company_owner == request.user

My views

class CompanyView(viewsets.ModelViewSet):  # made for viewing details
    permission_classes = (DRYPermissions, )
    queryset = Company.objects.all()
    serializer_class = CompanySerializer

    def create(self, request):
        try:

            company_name = request.data['company_name']
            company_orders = request.data['company_orders']
            company_owner = request.data['company_owner']
            company_owner_obj = User.objects.get(id=company_owner)
            company = Company(company_name=company_name,
                              company_orders=company_orders, company_owner=company_owner_obj)
            company.save()

        except Exception as error:
            response = {
                'error': str(error)
            }
            return Response(response, status=status.HTTP_400_BAD_REQUEST)

        response = {
            'message': 'Company created'
        }

        return Response(response, status=status.HTTP_201_CREATED)

    def company_details(self, request):
        try:
            company_id = request.user.active_company.id

            company = Company.objects.get(id=company_id)

            serialized_data = CompanySerializer(company)

        except Exception as error:
            response = {
                'error': str(error)
            }
            return Response(response)

        return Response(serialized_data.data)

    def company_edit(self, request, **kwargs):
        try:
            company_id = request.user.active_company.id
            company = Company.objects.get(id=company_id)
            serializer = CompanySerializer(
                company, data=request.data, partial=True)
            if serializer.is_valid():
                serializer.save()
        except Exception as error:
            response = {
                'message': str(error)
            }
            return Response(response)
        response = {
            'message': 'Edited Successfully'
        }
        return Response(response)

    def company_delete(self, request):
        try:
            company_id = request.user.active_company.id
            company = Company.objects.filter(id=company_id)
            company.delete()
        except Exception as error:
            response = {
                'message': str(error)
            }
            return Response(response)
        response = {
            'message': 'Deleted Successfully'
        }
        return Response(response)

My urls

urlpatterns = [
    #    Company URLs
    path('company_create/',
         CompanyView.as_view({'post': 'create'}), name='company_create'),  # Create company
    path('company_edit/',
         CompanyView.as_view(), name='company_edit'),  # Edit company details
    path('company_delete/',
         CompanyView.as_view({'delete': 'company_delete'}), name='company_delete'),  # Delete company
    path('company_details/',
         CompanyView.as_view({'get': 'company_details'}), name='company_details'),  # get company details (owner, employees etc)
]

My serializer

class CompanySerializer(serializers.ModelSerializer):
    company_owner = LimitedUserSerializer(read_only=True)

    class Meta:
        model = Company
        fields = ['id', 'company_name', 'company_orders',
                  'company_icon', 'company_owner']
RignonNoel commented 2 years ago

Hi @AsifDaum!

As described in this part of the documentation the Global permissions are always checked first and Object permissions are checked ONLY if global permissions pass.

Documentation Source:

DRY Rest Permissions allows you to define both global and object level permissions.

Global permissions are always checked first and define the ability of a user to take an action on an entire model. For example you can define whether a user has the ability to update any projects from the database.

Object permissions are checked if global permissions pass and define whether a user has the ability to perform a specific action on a single object. These are also known as row level permissions. Note: list and create actions are the only standard actions that are only global. There is no such object level permission call because they are whole table actions.

In this context you have multiple problem actually that you should correct:

Output:

I know that it seems a little bit overkill just to delete an object, but with some experience it allow you to clearly define ans setup permission but also to easily share the generic rules with the Frontend by using DryPermissionsField and DRYGlobalPermissionsField

AsifDaum commented 2 years ago

Hi @RignonNoel,

Thank you so much for your response, and understood, I have implemented the corrections you mentioned which indeed do make a lot of sense.

However I still can't seem to hit the has_object_update_permission(...). company_edit is one of my custom actions and since it is a custom action permission, the documentation states that I should use the detail_route which I presume is now currently known as @action(detail=True)? If I implement this I am still unable to hit the has_object_update_permission(...) and only the global has_update_permission(...) is hit. I have changed the name from company_edit to update and used your example in order to test this.

I have also found out that the has_object_update_permission(...) works if I don't use the custom actions and simply use the provided mixins that come with the standard modelViewset. But there are use cases for me to have customised actions, what could I possibly be doing wrong there that prevents it from accessing the has_object_update_permission(...) on the custom actions?

Thank you,

RignonNoel commented 2 years ago

Hi @AsifDaum,

The problem is that you don't define the custom permission for your custom action viewset:

AsifDaum commented 2 years ago

Hi @RignonNoel

Thank you for your immediate response once again, I have tried exactly what you stated and copied it into the code as well to check it, but it appears that is still not going through to the has_object_company_edit_permission(...). It is only checking the global permissions only has_company_edit_permission(...).

It doesnt hit the unauthorised/forbidden error 403, and when I add the print line it shows that it reaches general.

Could it simply be the way that I .get(...) the company object? Is there a specific way that an object needs to be filtered out before object permission gets applied to it?

I understand that I can simplify the code significantly further by using get_object(...) in that company_edit action, but I want to understand the mechanics of why I am not achieving the desired result and hopefully learn from it. I appreciate your help.

Url:

    path('company_edit/',
         CompanyView.as_view({'patch': 'company_edit'}), name='company_edit'),  # Edit company details

Views

class CompanyView(viewsets.ModelViewSet):  # made for viewing details
    permission_classes = (DRYPermissions,)
    serializer_class = CompanySerializer

    def get_queryset(self):
        return Company.objects.filter(company_employees=self.request.user)

    @action(detail=True, methods=['patch'])
    def company_edit(self, request, **kwargs):
        try:
            company_id = request.user.active_company.id
            company = Company.objects.get(id=company_id)
            serializer = CompanySerializer(
                company, data=request.data, partial=True)
            if serializer.is_valid():
                serializer.save()
        except Exception as error:
            response = {
                'message': str(error)
            }
            return Response(response)
        response = {
            'message': 'Edited Successfully'
        }
        return Response(response)

My model

class Company(models.Model):
    company_name = models.CharField(max_length=100)
    company_orders = models.IntegerField(blank=True, null=True)
    company_icon = models.ImageField(
        upload_to='media/company_icon', blank=True)
    company_owner = models.ForeignKey(
        User, on_delete=models.SET_NULL, blank=True, null=True)
    company_employees = models.ManyToManyField(
        User, blank=True, null=True, related_name="company_employees")

    def __str__(self):
        return self.company_name

    @staticmethod
    def has_read_permission(request):
        return True

    @staticmethod
    @authenticated_users
    def has_company_edit_permission(request):
        print("in global")
        return True

    @authenticated_users
    def has_object_company_edit_permission(self, request):
        print("in object")
        return self.company_owner == request.user
RignonNoel commented 2 years ago

Hi @AsifDaum,

It's strange since what you gave seems good. I even did a complete test of your code on a Django project in order to test your naming convention and the way you use the URLs but everything work well when i run this test i wrote:

    def test_company_edit_as_admin(self):
        self.client.force_authenticate(user=self.staff)

        data = {}

        response = self.client.patch(
            reverse(
                'company_edit',
            ),
            data,
            format='json',
        )

        self.assertEqual(
            response.status_code,
            status.HTTP_200_OK,
            response.content
        )

Can you say me more about how you test your code ? Or is it possible for you to give me access to a demo i can run with your error in order to investigate correctly ?


As you said there are some simplification you can do to have a cleaner code, however all of that should not have any effect on the permission since Dry-Rest-Permission just need some specific things to work correctly:

AsifDaum commented 2 years ago

Hi @RignonNoel,

Unfortunately due to my current lack of experience I have not yet created any tests and believe it or not I am doing it all manually using my frontend, backend admin page and postman. But that will be my next focus after I have sorted this issue out.

There is the challenge that it requires a frontend for it to work due to the user authentication, and I cant exactly remember how to remove the entire authentication system from the backend for it to not require it anymore. So, I am not exactly sure how useful this will be for you regrettably, but here goes: https://github.com/AsifDaum/demo-dry-rest

RignonNoel commented 2 years ago

Hi @AsifDaum,

I see nothing strange on your project, all seems good for me at first look.

I tried to implement a test on your repository, but i hit some problem with the versions of the package since the requirements.txt does not describe the supported version of your app. So i can't have a version running on my computer to help you more.

In this context i can't help you more, sorry :/

AsifDaum commented 2 years ago

Hi @RignonNoel,

I appreciate your help, I did create a test for it just now, the results are unfortunately what I already expected. It only performing the has_global_permission check but not the has_object_permission.

Tests.py

from django.utils import timezone
from oauth2_provider.models import get_application_model, AccessToken
from oauth2_provider.settings import oauth2_settings
from company.views import CompanyView
from company.models import Company
from django.test import TestCase
from rest_framework import status
from users.models import User
from rest_framework.test import APIRequestFactory

Application = get_application_model()

class test_edit_company(TestCase):
    def setUp(self):
        oauth2_settings._SCOPES = [
            "read", "write", "scope1", "scope2", "resource1"]

        self.test_user = User.objects.create_user(
            "test_user", "test@example.com", "123456")

        self.application = Application.objects.create(
            name="Test Application",
            redirect_uris="http://localhost http://example.com http://example.org",
            user=self.test_user,
            client_type=Application.CLIENT_CONFIDENTIAL,
            authorization_grant_type=Application.GRANT_AUTHORIZATION_CODE,
        )

        self.access_token = AccessToken.objects.create(
            user=self.test_user,
            scope="read write",
            expires=timezone.now() + timezone.timedelta(seconds=300),
            token="secret-access-token-key",
            application=self.application
        )
        # read or write as per your choice
        self.access_token.scope = "read"
        self.access_token.save()

        # correct token and correct scope
        self.auth = "Bearer {0}".format(self.access_token.token)

        Company.objects.create(company_name="lion")

    def test_company_edit_as_admin(self):
        factory = APIRequestFactory()

        view = CompanyView.as_view({'patch': 'company_edit'})

        data = {
            "company_name": "test company name"
        }

        request = factory.patch('/company/company_edit/',
                                data,
                                format='json', HTTP_AUTHORIZATION=self.auth
                                )
        response = view(request)
        response.render()

        self.assertEqual(
            response.status_code,
            status.HTTP_200_OK,
            response.content
        )

If my code seems otherwise fine then I would have thought it is one of the packages/modules that may be interfering?

Would you have an idea of it possibly being a conflict between the oAuth2 toolkit and the permissions or would that not make sense?

I also made this django app a couple of months ago so would have thought the versions are the latest, but I will update everything in turn to see if any of that possibly has an effect on it, it would still be weird if it did.

I am still able to use dry-rest-permissions but just not with custom actions regrettably. I might possibly do a fresh install and work my way up the ladder and see what possibly causes this. That is if I am in desperate need for object permissions on custom actions. If I get to do this I will notify the cause in here.

Thanks!

AsifDaum commented 2 years ago

It appears that it does not check for object permissions in any of my custom actions, thus adding the following check in either the get_object() or the custom action itself fixes this.

self.check_object_permissions(self.request, obtainedObject)

An example of what it looks like in my code:

    @action(detail=True, methods=['patch'], pk=None)
    def company_edit(self, request, **kwargs):
        try:
            company_id = request.user.active_company.id
            company = Company.objects.get(id=company_id)
            serializer = CompanySerializer(
                company, data=request.data, partial=True)
            self.check_object_permissions(self.request, company)
            if serializer.is_valid():
                serializer.save()
        except Exception as error:
            response = {
                'message': str(error)
            }
            return Response(response)
        response = {
            'message': 'Edited Successfully'
        }
        return Response(response)
RignonNoel commented 2 years ago

@AsifDaum Oh! You use @action(detail=True) but without specifying any Primary Key!!

If you don't specify any pk DryRestPermission can't know which item you are speaking about and can't execute the object permission! So yes your solution should work since you ask to do it with a specific object, i think it's the good way to go with it!

Is there a specific reason why you don't use @action(detail=False) since it's not a detail call ? It's what misleads my own test