Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
33 changes: 32 additions & 1 deletion openwisp_users/api/permissions.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import copy

from django.utils.translation import gettext_lazy as _
from rest_framework.permissions import BasePermission
from rest_framework.permissions import BasePermission, DjangoModelPermissions
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

here you'd import it as BaseDjangoModelPermissions

from swapper import load_model

Organization = load_model('openwisp_users', 'Organization')
Expand Down Expand Up @@ -66,3 +68,32 @@ class IsOrganizationOwner(BaseOrganizationPermission):

def validate_membership(self, user, org):
return org and (user.is_superuser or user.is_owner(org))


class CustomDjangoModelPermissions(DjangoModelPermissions):
Comment thread
atb00ker marked this conversation as resolved.
Outdated
def __init__(self):
self.perms_map = copy.deepcopy(self.perms_map)
self.perms_map['GET'] = ['%(app_label)s.view_%(model_name)s']
Comment thread
atb00ker marked this conversation as resolved.
Outdated

def has_permission(self, request, view):
# Workaround to ensure DjangoModelPermissions are not applied
# to the root view when using DefaultRouter.
if getattr(view, '_ignore_model_permissions', False):
return True

if not request.user or (
not request.user.is_authenticated and self.authenticated_users_only
):
return False

queryset = self._queryset(view)
perms = self.get_required_permissions(request.method, queryset.model)
change_perm = self.get_required_permissions('PUT', queryset.model)

if request.method == 'GET':
if request.user.has_perms(perms) or request.user.has_perms(change_perm):
return True
else:
return False
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Did not handled the case of super user becasue superuser by default has all the permissions.

Copy link
Copy Markdown
Member

@atb00ker atb00ker May 22, 2021

Choose a reason for hiding this comment

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

I think you can add a check at the top of the function stating:

if user.is_superuser:
    return True

They are allowed to do anything regardless of any permission (this check is there in other has_permission functions too)!

P.S: Not sure if this would be required, please test! 😄

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@atb00ker I am trying to implement this in more generic way, cause am thinking of opening a similar PR in DRF repo once this gets merged here.

By default superuser has all permissions i.e., it can do anything it wants. suppose manually someone removes all the permissions for any specific model for superuser. Then in that case this would not work, since we will return True if it;s a super user, so that case will fail.

What do you think?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To the best of my knowledge, removing permissions from superuser is an anti-pattern, but I understand your point.

  1. We should look at existing DRF functions and implement it in a similar way.
  2. I'll also wait for @nemesisdesign check this! 😄

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

what about:

# user must have either view permission or change permission
return user.has_perms(perms) or user.has_perms(change_perm)


return request.user.has_perms(perms)
30 changes: 30 additions & 0 deletions tests/testapp/tests/test_permission_classes.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
from django.contrib.auth import get_user_model
from django.contrib.auth.models import Permission
from django.test import TestCase
from django.urls import reverse

from openwisp_users.api.throttling import AuthRateThrottle

from .mixins import TestMultitenancyMixin

User = get_user_model()


class TestPermissionClasses(TestMultitenancyMixin, TestCase):
def setUp(self):
Expand Down Expand Up @@ -117,3 +121,29 @@ def test_organization_field_with_errored_parent(self):
with self.assertRaises(AttributeError) as error:
self.client.get(reverse('test_error_field_view'), **auth)
self.assertIn('Organization not found', str(error.exception))

def test_custom_django_model_permission_with_view_permission(self):
Comment thread
atb00ker marked this conversation as resolved.
Outdated
user = User.objects.create_user(
username='operator', password='tester', email='operator@test.com'
)
user_permissions = Permission.objects.filter(codename='view_template')
user.user_permissions.add(*user_permissions)
user.organizations_dict # force caching
self.client.force_login(user)
token = self._obtain_auth_token()
auth = dict(HTTP_AUTHORIZATION=f'Bearer {token}')
response = self.client.get(reverse('test_template_list'), **auth)
self.assertEqual(response.status_code, 200)

def test_custom_django_model_permission_with_change_permission(self):
user = User.objects.create_user(
username='operator', password='tester', email='operator@test.com'
)
user_permissions = Permission.objects.filter(codename='change_template')
user.user_permissions.add(*user_permissions)
user.organizations_dict # force caching
self.client.force_login(user)
token = self._obtain_auth_token()
auth = dict(HTTP_AUTHORIZATION=f'Bearer {token}')
response = self.client.get(reverse('test_template_list'), **auth)
self.assertEqual(response.status_code, 200)
6 changes: 5 additions & 1 deletion tests/testapp/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
)
from openwisp_users.api.permissions import (
BaseOrganizationPermission,
CustomDjangoModelPermissions,
IsOrganizationManager,
IsOrganizationMember,
IsOrganizationOwner,
Expand Down Expand Up @@ -169,7 +170,10 @@ def get_queryset(self):
class TemplateListCreateView(ListCreateAPIView):
serializer_class = TemplateSerializer
authentication_classes = (BearerAuthentication,)
permission_classes = (IsOrganizationMember,)
permission_classes = (
IsOrganizationMember,
CustomDjangoModelPermissions,
)
queryset = Template.objects.all()


Expand Down