Skip to content
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

Detach the model_class and obj from utilization #12

Open
rodrigondec opened this issue Sep 3, 2020 · 3 comments · May be fixed by #15
Open

Detach the model_class and obj from utilization #12

rodrigondec opened this issue Sep 3, 2020 · 3 comments · May be fixed by #15
Assignees

Comments

@rodrigondec
Copy link

rodrigondec commented Sep 3, 2020

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
Copy link
Member

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.

@RignonNoel RignonNoel self-assigned this Sep 21, 2020
@arianesc arianesc linked a pull request Oct 7, 2020 that will close this issue
@rodrigondec
Copy link
Author

@RignonNoel we opened the PR #15 🎉

@rodrigondec
Copy link
Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants