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

ManishShah120 · 2021-05-22T11:54:19Z

Closes #249

…sions class #249 Closes #249

ManishShah120 · 2021-05-22T14:20:21Z

openwisp_users/api/permissions.py

+        if request.method == 'GET':
+            if request.user.has_perms(perms) or request.user.has_perms(change_perm):
+                return True
+            else:
+                return False


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

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! 😄

@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?

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

We should look at existing DRF functions and implement it in a similar way.

I'll also wait for @nemesisdesign check this! 😄

atb00ker

This looks good to me, small comments:

tests/testapp/tests/test_permission_classes.py

openwisp_users/api/permissions.py

atb00ker · 2021-05-22T14:30:16Z

openwisp_users/api/permissions.py

+        if request.method == 'GET':
+            if request.user.has_perms(perms) or request.user.has_perms(change_perm):
+                return True
+            else:
+                return False


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! 😄

ManishShah120 · 2021-05-23T18:11:57Z

Quoting from the issue #249

Tests:

ensure an user of the operator group (not flagged as supersuser) cannot view both endpoints

Added

ensure an user of the administrator group (not flagged as supersuser) can view both endpoints

Added

add the view permission to the operator group, ensure the user now can view both endpoints but cannot change anything (put or delete)

Added

nemesifier

Looks on the right track! I'll need to test it.

But definitely please go ahead with the PR to Django REST Framework, hopefully we can get something similar or better merged there.

openwisp_users/api/permissions.py

okraits

Looks fine. No objections from me.

okraits

Looks fine.

atb00ker

I think this is looking good, small improvement in the tests.

atb00ker · 2021-05-29T11:19:46Z

tests/testapp/tests/test_permission_classes.py

+        user = User.objects.create_user(
+            username='operator', password='tester', email='[email protected]'
+        )
+        operator_group = Group.objects.filter(name='Operator')
+        user.groups.set(operator_group)


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

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.

atb00ker · 2021-05-29T11:20:47Z

tests/testapp/tests/test_permission_classes.py

+        operator_group = Group.objects.filter(name='Operator')
+        user.groups.set(operator_group)
+        org1 = self._get_org()
+        OrganizationUser.objects.create(user=user, organization=org1, is_admin=True)


Same here. Find function here: https://github.com/openwisp/openwisp-users/blob/master/openwisp_users/tests/utils.py#L128

atb00ker

Functional Test:

superuser has access
non-su without view permission & org manager doesn't have access
non-su with view permission & not org manager has access but sees nothing
non-su with view permission & org manager can see temp for their obj. (not shared obj)

nemesifier

Great work @ManishShah120 @atb00ker! 👍

Missing:

Documentation (please add it near to the other sections related to DRF)
Code simplification (see below)

nemesifier · 2021-05-31T17:56:15Z

openwisp_users/api/permissions.py

+            if user.has_perms(perms) or user.has_perms(change_perm):
+                return True
+            else:
+                return False


what about:

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

nemesifier · 2021-05-31T17:58:04Z

tests/testapp/tests/test_permission_classes.py

+        self.client.force_login(user)
+        token = self._obtain_auth_token()
+        auth = dict(HTTP_AUTHORIZATION=f'Bearer {token}')
+        t1 = self._create_template(organization=org1)


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.

Done, Please have a look. 👍

nemesifier · 2021-05-31T17:58:46Z

openwisp_users/api/permissions.py

        return org and (user.is_superuser or user.is_owner(org))
+
+
+class ViewDjangoModelPermissions(DjangoModelPermissions):


we could call this simply DjangoModelPermissions

nemesifier · 2021-05-31T17:58:58Z

openwisp_users/api/permissions.py

@@ -1,5 +1,5 @@
 from django.utils.translation import gettext_lazy as _
-from rest_framework.permissions import BasePermission
+from rest_framework.permissions import BasePermission, DjangoModelPermissions


here you'd import it as BaseDjangoModelPermissions

atb00ker

LGTM! 👍

atb00ker · 2021-06-01T14:39:28Z

tests/testapp/tests/test_permission_classes.py

+        user = self._get_user()
+        operator_group = Group.objects.filter(name='Operator')
+        user.groups.set(operator_group)


Doesn't _get_operator() help here?

No @atb00ker.

nemesifier

👍

[feature] Implemented view permissions by extending DjangoModelPermis…

9665dbc

…sions class #249 Closes #249

ManishShah120 changed the title ~~[feature] Implemented view permissions by extending DjangoModelPermissions class #249~~ [feature] Implemented view permissions by extending DjangoModelPermissions class #249 May 22, 2021

ManishShah120 requested review from atb00ker and nemesifier May 22, 2021 12:05

ManishShah120 self-assigned this May 22, 2021

ManishShah120 commented May 22, 2021

View reviewed changes

atb00ker reviewed May 22, 2021

View reviewed changes

[tests] Updated tests for ViewDjangoModelPermissions class

3e9ad53

ManishShah120 requested a review from okraits May 25, 2021 11:32

nemesifier reviewed May 26, 2021

View reviewed changes

openwisp_users/api/permissions.py Outdated Show resolved Hide resolved

okraits reviewed May 26, 2021

View reviewed changes

[change] Set perms_map attribute

2fe5487

okraits reviewed May 27, 2021

View reviewed changes

atb00ker reviewed May 29, 2021

View reviewed changes

atb00ker previously approved these changes May 31, 2021

View reviewed changes

nemesifier requested changes May 31, 2021

View reviewed changes

[change] Improved code quality

c446b0d

ManishShah120 dismissed atb00ker’s stale review via c446b0d June 1, 2021 11:30

[docs] Added docu for DjangoModelPermissions in DRF section

b78c4a1

atb00ker approved these changes Jun 1, 2021

View reviewed changes

nemesifier mentioned this pull request Jun 1, 2021

Respect can_read_model in DjangoModelPermissions encode/django-rest-framework#6325

Closed

nemesifier approved these changes Jun 1, 2021

View reviewed changes

nemesifier merged commit 70e0ffa into master Jun 1, 2021

nemesifier deleted the issues/249-class-to-implement-view-permission branch June 1, 2021 16:45

nemesifier mentioned this pull request Jun 1, 2021

Fix Respect can_read_model permission in DjangoModelPermissions encode/django-rest-framework#8009

Merged

ManishShah120 mentioned this pull request Jun 23, 2021

[change] Remove redundant code for checking view permissions openwisp/openwisp-network-topology#111

Closed

		return org and (user.is_superuser or user.is_owner(org))


		class ViewDjangoModelPermissions(DjangoModelPermissions):

Uh oh!

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

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

Uh oh!

Conversation

ManishShah120 commented May 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atb00ker May 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atb00ker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

atb00ker May 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ManishShah120 commented May 23, 2021

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

okraits left a comment

Choose a reason for hiding this comment

Uh oh!

okraits left a comment

Choose a reason for hiding this comment

Uh oh!

atb00ker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ManishShah120 May 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atb00ker left a comment

Choose a reason for hiding this comment

Uh oh!

nemesifier left a comment • edited by ManishShah120 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atb00ker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

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

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

atb00ker May 22, 2021 •

edited

Loading

atb00ker May 22, 2021 •

edited

Loading

ManishShah120 May 31, 2021 •

edited

Loading

nemesifier left a comment •

edited by ManishShah120

Loading