Skip to content

Use APIView.permission_denied to handle 401 and 403 http errors #175

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Use APIView.permission_denied to handle 401 and 403 http errors #175

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c3e8cad
Use APIView.permission_denied to handle 401 and 403 http errors
hashlash Feb 26, 2023
5edf57c
Fix lint
hashlash Feb 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions rules/contrib/rest_framework.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from django.core.exceptions import ImproperlyConfigured, PermissionDenied
from django.core.exceptions import ImproperlyConfigured


class AutoPermissionViewSetMixin:
Expand Down Expand Up @@ -72,4 +72,4 @@ def initial(self, *args, **kwargs):
# Finally, check permission
perm = self.get_queryset().model.get_perm(perm_type)
if not self.request.user.has_perm(perm, obj):
raise PermissionDenied
self.permission_denied(self.request)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is permission_denied an attribute of DRF's ViewSet? Is this possible to result in an AttributeError?

Also, what are the implications of switching to the new behavior for users that depend on PermissionDenied be raised and is there a way to get the previous behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is permission_denied an attribute of DRF's ViewSet? Is this possible to result in an AttributeError?

permission_denied is defined in APIView, and since ViewSet is a subclass of APIView, the function should always works.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, what are the implications of switching to the new behavior for users that depend on PermissionDenied be raised ...?

The view will return the status code correctly based on whether the request is authenticated (the credentials are valid, but the user doesn't have the correct permission) or not (the credentials is not provided or invalid).

Quoting from DRF docs:

HTTP 401 responses must always include a WWW-Authenticate header, that instructs the client how to authenticate. HTTP 403 responses do not include the WWW-Authenticate header.

The kind of response that will be used depends on the authentication scheme. Although multiple authentication schemes may be in use, only one scheme may be used to determine the type of response. The first authentication class set on the view is used when determining the type of response.

Note that when a request may successfully authenticate, but still be denied permission to perform the request, in which case a 403 Permission Denied response will always be used, regardless of the authentication scheme.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... is there a way to get the previous behavior?

For current implementation, the users needs to override the initial() method and replace the NotAuthenticated exception with PermissionDenied (using try except block).

As an alternative implementation, we could allow users to opt in/out of this change using a Django setting.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The view will return the status code correctly based on whether the request is authenticated (the credentials are valid, but the user doesn't have the correct permission) or not (the credentials is not provided or invalid).

I understand that, what I mean is that this is changing a behavior that existing users may (?) depend on in a way that might cause crashes at runtime so I'm trying to understand the (potential) impact this change may have.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I could only think of a mismatch of HTTP status code checks (most likely on client side) led to unhandled (or incorrect) condition?

Something like this:

if status_code == 200:
    ...
elif status_code == 404:
    ...
elif status_code == 403:
    ...
else:
    return "Unhandled message"

I don't think that would lead to a crash.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @dfunckt, would you prefer to let the user opt-in for this patch for now? I was thinking that you might consider this a breaking change, so enforcing it on the next major release might be more suitable. But still, I think this is a bug, so it should be applied with a patch release (or at least a minor release).

15 changes: 12 additions & 3 deletions tests/testsuite/contrib/test_rest_framework.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
from __future__ import absolute_import

from django.contrib.auth.models import AnonymousUser
from django.contrib.auth.models import AnonymousUser, User
from django.core.exceptions import ImproperlyConfigured
from django.test import TestCase

from rest_framework.authentication import BasicAuthentication
from rest_framework.decorators import action
from rest_framework.response import Response
from rest_framework.serializers import ModelSerializer
from rest_framework.test import APIRequestFactory
from rest_framework.test import APIRequestFactory, force_authenticate
from rest_framework.viewsets import ModelViewSet

import rules # noqa
Expand All @@ -26,6 +27,7 @@ class Meta:
class TestViewSet(AutoPermissionViewSetMixin, ModelViewSet):
queryset = TestModel.objects.all()
serializer_class = TestModelSerializer
authentication_classes = [BasicAuthentication]
permission_type_map = AutoPermissionViewSetMixin.permission_type_map.copy()
permission_type_map["custom_detail"] = "add"
permission_type_map["custom_nodetail"] = "add"
Expand Down Expand Up @@ -58,7 +60,14 @@ def test_predefined_action(self):
self.assertEqual(
self.vs.as_view({"get": "retrieve"})(self.req, pk=1).status_code, 200
)
# Destroy should be forbidden due to missing delete permission
# Unauthenticated destroy should be return 401 due to missing credentials
self.assertEqual(
self.vs.as_view({"get": "destroy"})(self.req, pk=1).status_code, 401
)
# Authenticated with unauthorized destroy should be forbidden due to missing
# delete permission
user = User.objects.create_user(username="user", password="pass")
force_authenticate(self.req, user=user)
self.assertEqual(
self.vs.as_view({"get": "destroy"})(self.req, pk=1).status_code, 403
)
Expand Down