rodrigondec commented 4 years ago

I'm willing to open PR!

Resume

I'm not using Django/DRF in a traditional way. I'm using a Domain Driven Design architecture.

https://phalt.github.io/post/django-api-domains/ https://github.com/rodrigondec/drf-api-domain

My ViewSet doesn't have access to my Model classes (my serializers included!).

To use this lib I'm going to overwrite the methods DRYPermissions.has_permission and DRYPermissions.has_object_permission to use my service instead of model_class and obj respectively.

My code

from rest_framework import permissions
from dry_rest_permissions.generics import DRYPermissions

from ..rest_framework.viewsets import ServiceGenericViewSet

class ServiceDRYPermissions(DRYPermissions):
    """
    Customização para o DRYPermissions utilizar a nossa camada service,
    e não o model!
    """

    def has_permission(self, request, view: ServiceGenericViewSet):
        """
        Overrides the standard function and figures out methods to call for global permissions.
        """
        if not self.global_permissions:
            return True

        assert view.service is not None, (
                "global_permissions set to true without a service "
                "set on the view for '%s'" % view.__class__.__name__
        )

        service = view.get_service()

        action_method_name = None
        if hasattr(view, 'action'):
            action = self._get_action(view.action)
            action_method_name = "has_{action}_permission".format(action=action)
            # If the specific action permission exists then use it, otherwise use general.
            if hasattr(service, action_method_name):
                return getattr(service, action_method_name)(request=request)

        if request.method in permissions.SAFE_METHODS:
            assert hasattr(service, 'has_read_permission'), \
                self._get_error_message(service, 'has_read_permission', action_method_name)
            return service.has_read_permission(request=request)
        else:
            assert hasattr(service, 'has_write_permission'), \
                self._get_error_message(service, 'has_write_permission', action_method_name)
            return service.has_write_permission(request=request)

    def has_object_permission(self, request, view: ServiceGenericViewSet, obj):
        """
        Overrides the standard function and figures out methods to call for object permissions.
        """
        if not self.object_permissions:
            return True

        service = view.get_service()

        action_method_name = None
        if hasattr(view, 'action'):
            action = self._get_action(view.action)
            action_method_name = "has_object_{action}_permission".format(action=action)
            # If the specific action permission exists then use it, otherwise use general.
            if hasattr(service, action_method_name):
                return getattr(service, action_method_name)(request=request, obj=obj)

        if request.method in permissions.SAFE_METHODS:
            assert hasattr(service, 'has_object_read_permission'), \
                self._get_error_message(service, 'has_object_read_permission', action_method_name)
            return service.has_object_read_permission(request=request, obj=obj)
        else:
            assert hasattr(service, 'has_object_write_permission'), \
                self._get_error_message(service, 'has_object_write_permission', action_method_name)
            return service.has_object_write_permission(request=request, obj=obj)

Proposition

Instead of doing

serializer_class = view.get_serializer_class()

assert serializer_class.Meta.model is not None, (
            "global_permissions set to true without a model "
            "set on the serializer for '%s'" % view.__class__.__name__
        )

model_class = serializer_class.Meta.model

I suggest to create a function to get the 'target' for the permission loookup!

Suggestion of overwritable helper function

class DRYPermissions(permissions.BasePermission):
    def _get_permission_target(self,view, obj=None):
        if obj:
            return obj

        serializer_class = view.get_serializer_class()

        assert serializer_class.Meta.model is not None, (
                "global_permissions set to true without a model "
                "set on the serializer for '%s'" % view.__class__.__name__
            )

        model_class = serializer_class.Meta.model
        return model_class

This way I could overwrite only the target without duplicating the whole lib!

Another improvement possible is to use **kwargs!

This way anyone who uses this lib may 'ignore' the arguments passed if they don't want it.

permission call

target = self._get_permission_target(view, obj)

if hasattr(target, action_method_name):
    return getattr(target, action_method_name)(request=request)

obj permission call

target = self._get_permission_target(view, obj)

if hasattr(target, action_method_name):
    return getattr(target, action_method_name)(request=request, obj=obj)

RignonNoel commented 4 years ago

Hi @rodrigondec!

Thanks for all the details and links, it's a really interessting concept and integration. Honestly i do not see why to refuse a PR on this subject, it seems to be to be a really good enhancement to allow new architecture.

If you do so, please think about:

updating the documentation to make a little example with your architecture
update test to be sure we keep it working in the future since other contributor will maybe not know your kind of architecture

I will make sure to follow this thread and to help you if you need help with your PR, we will be able to make a release as soon as it will be merged to not let you wait after it.

rodrigondec commented 3 years ago

@RignonNoel we opened the PR #15 :tada: