encode / django-rest-framework

Web APIs for Django. 🎸
https://www.django-rest-framework.org
Other
28.45k stars 6.84k forks source link

Tutorial - turning on PAGINATE_BY breaks custom permissions #2205

Closed detectedstealth closed 9 years ago

detectedstealth commented 9 years ago

When turning on PAGINATE_BY in settings Adding Pagination

REST_FRAMEWORK = {
    'PAGINATE_BY': 10
}

The custom permissions Object Level Permissions

from rest_framework import permissions

class IsOwnerOrReadOnly(permissions.BasePermission):
    """
    Custom permission to only allow owners of an object to edit it.
    """
    def has_object_permission(self, request, view, obj):
        # Read permissions are allowed to any request,
        # so we'll always allow GET, HEAD, or OPTIONS requests.
        if request.method in permissions.SAFE_METHODS:
            return True
        print(obj)
        # Write permissions are only allowed to the owner of the snippet.
        return obj.owner == request.user

break when trying to view the list of Snippets with the following error:

AttributeError at /snippets/
'Page' object has no attribute 'owner'
Request Method: GET
Request URL:    http://127.0.0.1:8000/snippets/
Django Version: 1.7.1
Exception Type: AttributeError
Exception Value:    
'Page' object has no attribute 'owner'
Exception Location: /Development/Python/django/snippets_tutorial/snippets/permissions.py in has_object_permission, line 14

When disabling the PAGINATE_BY setting you are able to view the Snippets list without any error.

tomchristie commented 9 years ago

Can you double check that this is the 3.0 installed from PyPI, and not the 3.0-beta installed from GitHub?

Same as #2073, which was closed by https://github.com/tomchristie/django-rest-framework/commit/084354d3ebf3949f8c1664d68da4a568da0b10fa

detectedstealth commented 9 years ago

This is version 3.0.0 installed with pip

Bruces-MacBook-Pro:snippets_tutorial brucewade$ workon snippets_tutorial
(snippets_tutorial)Bruces-MacBook-Pro:snippets_tutorial brucewade$ ls
db.sqlite3  manage.py   snippets    tutorial
(snippets_tutorial)Bruces-MacBook-Pro:snippets_tutorial brucewade$ pip freeze > requirements.txt
(snippets_tutorial)Bruces-MacBook-Pro:snippets_tutorial brucewade$ cat requirements.txt 
Django==1.7.1
Pygments==2.0.1
djangorestframework==3.0.0
(snippets_tutorial)Bruces-MacBook-Pro:snippets_tutorial brucewade$
tomchristie commented 9 years ago

Any chance you could include the full traceback?

detectedstealth commented 9 years ago

Also note when not authenticated and viewing the list of snippets there is no errors. Once logging in and trying to view the list is when there is an error.

Environment:
Request Method: GET
Request URL: http://127.0.0.1:8000/snippets/

Django Version: 1.7.1
Python Version: 3.4.2
Installed Applications:
('django.contrib.admin',
 'django.contrib.auth',
 'django.contrib.contenttypes',
 'django.contrib.sessions',
 'django.contrib.messages',
 'django.contrib.staticfiles',
 'rest_framework',
 'snippets')
Installed Middleware:
('django.contrib.sessions.middleware.SessionMiddleware',
 'django.middleware.common.CommonMiddleware',
 'django.middleware.csrf.CsrfViewMiddleware',
 'django.contrib.auth.middleware.AuthenticationMiddleware',
 'django.contrib.auth.middleware.SessionAuthenticationMiddleware',
 'django.contrib.messages.middleware.MessageMiddleware',
 'django.middleware.clickjacking.XFrameOptionsMiddleware')

Traceback:
File "/Users/brucewade/.virtualenvs/snippets_tutorial/lib/python3.4/site-packages/django/core/handlers/base.py" in get_response
  137.                 response = response.render()
File "/Users/brucewade/.virtualenvs/snippets_tutorial/lib/python3.4/site-packages/django/template/response.py" in render
  103.             self.content = self.rendered_content
File "/Users/brucewade/.virtualenvs/snippets_tutorial/lib/python3.4/site-packages/rest_framework/response.py" in rendered_content
  59.         ret = renderer.render(self.data, media_type, context)
File "/Users/brucewade/.virtualenvs/snippets_tutorial/lib/python3.4/site-packages/rest_framework/renderers.py" in render
  715.         context = self.get_context(data, accepted_media_type, renderer_context)
File "/Users/brucewade/.virtualenvs/snippets_tutorial/lib/python3.4/site-packages/rest_framework/renderers.py" in get_context
  665.         raw_data_post_form = self.get_raw_data_form(data, view, 'POST', request)
File "/Users/brucewade/.virtualenvs/snippets_tutorial/lib/python3.4/site-packages/rest_framework/renderers.py" in get_raw_data_form
  595.             if not self.show_form_for_method(view, method, request, instance):
File "/Users/brucewade/.virtualenvs/snippets_tutorial/lib/python3.4/site-packages/rest_framework/renderers.py" in show_form_for_method
  509.                 view.check_object_permissions(request, obj)
File "/Users/brucewade/.virtualenvs/snippets_tutorial/lib/python3.4/site-packages/rest_framework/views.py" in check_object_permissions
  290.             if not permission.has_object_permission(request, self, obj):
File "/Users/brucewade/Development/Python/django/snippets_tutorial/snippets/permissions.py" in has_object_permission
  14.         return obj.owner == request.user

Exception Type: AttributeError at /snippets/
Exception Value: 'Page' object has no attribute 'owner'
xordoquy commented 9 years ago

@detectedstealth I just ran the tutorial and did not had this issue. This might be linked with how you fixed:

owner = serializers.Field(source='owner.username')

which should have caused an exception: "to_representation() must be implemented."

detectedstealth commented 9 years ago

@xordoquy You would be correct with that error if you were supposed to use:

owner = serializers.Field(source='owner.username')

However the tutorials Updating our Serializer uses this:

owner = serializers.ReadOnlyField(source='owner.username')

Notice the ReadOnlyField was supposed to be used not a plain Field for the owner.

tomchristie commented 9 years ago

Sorry I'm a bit lost how these two things are related. If .Field is still being used directly then yes that needs to be fixed in the tutorial. How does that affect this PAGINATE_BY issue that the reporting is seeing?

xordoquy commented 9 years ago

@tomchristie my bad, I copy/pasted the wrong version.

xordoquy commented 9 years ago

@detectedstealth forget my comment anyway, the tutorial has issues with the current master, not the latest release.

detectedstealth commented 9 years ago

I think this is an even bigger issue. When turning off PAGINATE_BY as stated there is no error, which is because

def has_object_permission(self, request, view, obj):

does not get called when PAGINATE_BY is turned off. Then when PAGINATE_BY is turned on the obj param is passed a Page object which causes the error submitted in this reporting.

xordoquy commented 9 years ago

@detectedstealth Please note that the tutorial says to add IsOwnerOrReadOnly to the SnippetDetail while you seem to have it applied to SnippetList. Thing that tip me is in your trace, it shows "Request URL: http://127.0.0.1:8000/snippets/" while IsOwnerOrReadOnly is called.

detectedstealth commented 9 years ago

@xordoquy I think you need to read to the end of the tutorial Refactoring to Use Viewsets where you combined both List and Detail views.

from rest_framework.decorators import detail_route

class SnippetViewSet(viewsets.ModelViewSet):
    """
    This viewset automatically provides `list`, `create`, `retrieve`,
    `update` and `destroy` actions.

    Additionally we also provide an extra `highlight` action.
    """
    queryset = Snippet.objects.all()
    serializer_class = SnippetSerializer
    permission_classes = (permissions.IsAuthenticatedOrReadOnly,
                          IsOwnerOrReadOnly,)

    @detail_route(renderer_classes=[renderers.StaticHTMLRenderer])
    def highlight(self, request, *args, **kwargs):
        snippet = self.get_object()
        return Response(snippet.highlighted)

    def pre_save(self, obj):
        obj.owner = self.request.user
xordoquy commented 9 years ago

Right, this is reproduced here. Using the pagination makes DRF use the pagination serializer which result is sent to the permission object. This leads to inconsistent data sent to the permission class because it'll be either a Page instance in list mode either a Snippet in detail mode. I was tempted to update the tutorial code but I'm not sure whether we should fixed the tutorial or in DRF itself. @tomchristie, thoughts ?

tomchristie commented 9 years ago

in DRF itself.

This.

tomchristie commented 9 years ago

Well pretty ugly, plus no tests at the moment, but this will do for right now. #2089 remains open to improve on it.