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

[feature] Implemented view permissions by extending DjangoModelPermissions class #249 #251

Merged
merged 5 commits into from
Jun 1, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
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
32 changes: 31 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
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,31 @@ class IsOrganizationOwner(BaseOrganizationPermission):

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


class ViewDjangoModelPermissions(DjangoModelPermissions):
Copy link
Member

Choose a reason for hiding this comment

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

we could call this simply DjangoModelPermissions

def __init__(self):
self.perms_map = copy.deepcopy(self.perms_map)
self.perms_map['GET'] = ['%(app_label)s.view_%(model_name)s']
atb00ker marked this conversation as resolved.
Show resolved Hide resolved

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

user = request.user
if not user or (not 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 user.has_perms(perms) or user.has_perms(change_perm):
return True
else:
return False
Copy link
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 user.has_perms(perms)
8 changes: 8 additions & 0 deletions tests/testapp/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,11 @@ def _create_shelf(self, **kwargs):
s.full_clean()
s.save()
return s

def _create_template(self, **kwargs):
options = dict(name='test-template')
options.update(kwargs)
t = self.template_model(**options)
t.full_clean()
t.save()
return t
134 changes: 134 additions & 0 deletions tests/testapp/tests/test_permission_classes.py
Original file line number Diff line number Diff line change
@@ -1,14 +1,23 @@
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 swapper import load_model

from openwisp_users.api.throttling import AuthRateThrottle

from ..models import Template
from .mixins import TestMultitenancyMixin

User = get_user_model()
Group = load_model('openwisp_users', 'Group')
OrganizationUser = load_model('openwisp_users', 'OrganizationUser')


class TestPermissionClasses(TestMultitenancyMixin, TestCase):
def setUp(self):
AuthRateThrottle.rate = 0
self.template_model = Template
self.member_url = reverse('test_api_member_view')
self.manager_url = reverse('test_api_manager_view')
self.owner_url = reverse('test_api_owner_view')
Expand Down Expand Up @@ -117,3 +126,128 @@ 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_view_permission_with_operator(self):
user = User.objects.create_user(
username='operator', password='tester', email='operator@test.com'
)
operator_group = Group.objects.filter(name='Operator')
user.groups.set(operator_group)
Copy link
Member

Choose a reason for hiding this comment

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

Just FYI, I think there are functions for common actions. Like _create_operator.
These should be inherited and used! 😄

Copy link
Member Author

@ManishShah120 ManishShah120 May 31, 2021

Choose a reason for hiding this comment

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

Hey, @atb00ker Yes, I tried to use it but with this function, the user gets all the permissions without being in the operator group, and here we only needed testapp.view_template permission.

org1 = self._get_org()
OrganizationUser.objects.create(user=user, organization=org1, is_admin=True)
Copy link
Member

Choose a reason for hiding this comment

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

self.client.force_login(user)
token = self._obtain_auth_token()
auth = dict(HTTP_AUTHORIZATION=f'Bearer {token}')
t1 = self._create_template(organization=org1)
with self.subTest('Get Template List'):
response = self.client.get(reverse('test_template_list'), **auth)
self.assertEqual(response.status_code, 403)
with self.subTest('Get Template Detail'):
response = self.client.get(
reverse('test_template_detail', args=[t1.pk]), **auth
)
self.assertEqual(response.status_code, 403)

def test_view_permission_with_administrator(self):
user = User.objects.create_user(
username='operator', password='tester', email='operator@test.com'
)
administrator_group = Group.objects.get(name='Administrator')
change_perm = Permission.objects.get(codename='change_template')
administrator_group.permissions.add(change_perm)
user.groups.add(administrator_group)
org1 = self._get_org()
OrganizationUser.objects.create(user=user, organization=org1, is_admin=True)
self.client.force_login(user)
token = self._obtain_auth_token()
auth = dict(HTTP_AUTHORIZATION=f'Bearer {token}')
t1 = self._create_template(organization=org1)
with self.subTest('Get Template List'):
response = self.client.get(reverse('test_template_list'), **auth)
self.assertEqual(response.status_code, 200)
with self.subTest('Get Template Detail'):
response = self.client.get(
reverse('test_template_detail', args=[t1.pk]), **auth
)
self.assertEqual(response.status_code, 200)
permissions = administrator_group.permissions.values_list('codename', flat=True)
self.assertFalse('view_template' in permissions)
self.assertTrue('change_template' in permissions)

def test_view_permission_with_operator_having_view_perm(self):
user = User.objects.create_user(
username='operator', password='tester', email='operator@test.com'
)
operator_group = Group.objects.get(name='Operator')
view_perm = Permission.objects.get(codename='view_template')
operator_group.permissions.add(view_perm)
user.groups.add(operator_group)
org1 = self._get_org()
OrganizationUser.objects.create(user=user, organization=org1, is_admin=True)
self.client.force_login(user)
token = self._obtain_auth_token()
auth = dict(HTTP_AUTHORIZATION=f'Bearer {token}')
t1 = self._create_template(organization=org1)
with self.subTest('Get Template List'):
response = self.client.get(reverse('test_template_list'), **auth)
self.assertEqual(response.status_code, 200)
with self.subTest('Get Template Detail'):
response = self.client.get(
reverse('test_template_detail', args=[t1.pk]), **auth
)
self.assertEqual(response.status_code, 200)
with self.subTest('Change Template Detail'):
data = {'name': 'change-template'}
response = self.client.patch(
reverse('test_template_detail', args=[t1.pk]), data, **auth
)
self.assertEqual(response.status_code, 403)
with self.subTest('Delete Template'):
response = self.client.delete(
reverse('test_template_detail', args=[t1.pk]), **auth
)
self.assertEqual(response.status_code, 403)

def test_view_django_model_permission_with_view_perm(self):
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
org1 = self._get_org()
OrganizationUser.objects.create(user=user, organization=org1, is_admin=True)
self.client.force_login(user)
token = self._obtain_auth_token()
auth = dict(HTTP_AUTHORIZATION=f'Bearer {token}')
t1 = self._create_template(organization=org1)
with self.subTest('Get Template List'):
response = self.client.get(reverse('test_template_list'), **auth)
self.assertEqual(response.status_code, 200)
with self.subTest('Get Template Detail'):
response = self.client.get(
reverse('test_template_detail', args=[t1.pk]), **auth
)
self.assertEqual(response.status_code, 200)

def test_view_django_model_permission_with_change_perm(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
org1 = self._get_org()
OrganizationUser.objects.create(user=user, organization=org1, is_admin=True)
self.client.force_login(user)
token = self._obtain_auth_token()
auth = dict(HTTP_AUTHORIZATION=f'Bearer {token}')
t1 = self._create_template(organization=org1)
Copy link
Member

Choose a reason for hiding this comment

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

the first lines of each tests look really similar, please try to make a private helper method which prepares the data needed for the test, so we can make the actual test code shorter and easier to read.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, Please have a look. 👍

with self.subTest('Get Template List'):
response = self.client.get(reverse('test_template_list'), **auth)
self.assertEqual(response.status_code, 200)
with self.subTest('Get Template Detail'):
response = self.client.get(
reverse('test_template_detail', args=[t1.pk]), **auth
)
self.assertEqual(response.status_code, 200)
1 change: 1 addition & 0 deletions tests/testapp/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,4 +48,5 @@
name='test_shelf_list_unauthorized_view',
),
path('template/', views.template_list, name='test_template_list',),
path('template/<int:pk>/', views.template_detail, name='test_template_detail',),
]
23 changes: 21 additions & 2 deletions tests/testapp/views.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
import swapper
from rest_framework.generics import ListAPIView, ListCreateAPIView
from rest_framework.generics import (
ListAPIView,
ListCreateAPIView,
RetrieveUpdateDestroyAPIView,
)
from rest_framework.response import Response
from rest_framework.views import APIView

Expand All @@ -17,6 +21,7 @@
IsOrganizationManager,
IsOrganizationMember,
IsOrganizationOwner,
ViewDjangoModelPermissions,
)

from .models import Book, Config, Shelf, Template
Expand Down Expand Up @@ -169,7 +174,20 @@ def get_queryset(self):
class TemplateListCreateView(ListCreateAPIView):
serializer_class = TemplateSerializer
authentication_classes = (BearerAuthentication,)
permission_classes = (IsOrganizationMember,)
permission_classes = (
IsOrganizationMember,
ViewDjangoModelPermissions,
)
queryset = Template.objects.all()


class TemplateDetailView(FilterByOrganizationManaged, RetrieveUpdateDestroyAPIView):
serializer_class = TemplateSerializer
authentication_classes = (BearerAuthentication,)
permission_classes = (
IsOrganizationMember,
ViewDjangoModelPermissions,
)
queryset = Template.objects.all()


Expand All @@ -188,3 +206,4 @@ class TemplateListCreateView(ListCreateAPIView):
shelf_list_manager_view = ShelfListManagerView.as_view()
shelf_list_owner_view = ShelfListOwnerView.as_view()
template_list = TemplateListCreateView.as_view()
template_detail = TemplateDetailView.as_view()