-
-
Notifications
You must be signed in to change notification settings - Fork 75
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
Changes from 2 commits
9665dbc
3e9ad53
2fe5487
c446b0d
b78c4a1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
from swapper import load_model | ||
|
||
Organization = load_model('openwisp_users', 'Organization') | ||
|
@@ -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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we could call this simply |
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) |
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') | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
org1 = self._get_org() | ||
OrganizationUser.objects.create(user=user, organization=org1, is_admin=True) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. Find function here: https://github.com/openwisp/openwisp-users/blob/master/openwisp_users/tests/utils.py#L128 |
||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) |
There was a problem hiding this comment.
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