diff --git a/HISTORY.rst b/HISTORY.rst index 6522e0d7da..270b9329dc 100644 --- a/HISTORY.rst +++ b/HISTORY.rst @@ -39,6 +39,7 @@ * Move setting COMMON_TEMPORARY_DIRECTORY to the storage app. The setting is now STORAGE_TEMPORARY_DIRECTORY. * Move file related utilities to the storage app. +* Backport and remove unused code from the permission app. 3.1.11 (2019-04-XX) =================== diff --git a/docs/releases/3.2.rst b/docs/releases/3.2.rst index b6b6ca93c4..f211a9611e 100644 --- a/docs/releases/3.2.rst +++ b/docs/releases/3.2.rst @@ -63,6 +63,7 @@ Other changes * Move setting COMMON_TEMPORARY_DIRECTORY to the storage app. The setting is now STORAGE_TEMPORARY_DIRECTORY. * Move file related utilities to the storage app. +* Backport and remove unused code from the permission app. Removals diff --git a/mayan/apps/acls/managers.py b/mayan/apps/acls/managers.py index 33eaf40b75..4427c21961 100644 --- a/mayan/apps/acls/managers.py +++ b/mayan/apps/acls/managers.py @@ -34,7 +34,7 @@ class AccessControlListManager(models.Manager): try: return Permission.check_permissions( - requester=user, permissions=permissions + permissions=permissions, user=user ) except PermissionDenied: try: @@ -117,7 +117,7 @@ class AccessControlListManager(models.Manager): try: Permission.check_permissions( - requester=user, permissions=(permission,) + permissions=(permission,), user=user ) except PermissionDenied: user_roles = [] diff --git a/mayan/apps/acls/tests/test_actions.py b/mayan/apps/acls/tests/test_actions.py index f638fef451..9a43ed70ba 100644 --- a/mayan/apps/acls/tests/test_actions.py +++ b/mayan/apps/acls/tests/test_actions.py @@ -18,7 +18,7 @@ class ACLActionTestCase(ActionTestCase): 'content_type': ContentType.objects.get_for_model(model=self.document).pk, 'object_id': self.document.pk, 'roles': [self.role.pk], - 'permissions': [permission_document_view.uuid], + 'permissions': [permission_document_view.pk], } ) action.execute(context={'entry_log': self.entry_log}) @@ -40,7 +40,7 @@ class ACLActionTestCase(ActionTestCase): 'content_type': ContentType.objects.get_for_model(model=self.document).pk, 'object_id': self.document.pk, 'roles': [self.role.pk], - 'permissions': [permission_document_view.uuid], + 'permissions': [permission_document_view.pk], } ) action.execute(context={'entry_log': self.entry_log}) diff --git a/mayan/apps/acls/views.py b/mayan/apps/acls/views.py index e32bd6f801..73f51306a7 100644 --- a/mayan/apps/acls/views.py +++ b/mayan/apps/acls/views.py @@ -171,13 +171,12 @@ class ACLPermissionsView(AssignRemoveView): def generate_choices(entries): results = [] + # Sort permissions by their translatable label entries = sorted( - entries, key=lambda x: ( - x.get_volatile_permission().namespace.label, - x.get_volatile_permission().label - ) + entries, key=lambda permission: permission.volatile_permission.label ) + # Group permissions by namespace for namespace, permissions in itertools.groupby(entries, lambda entry: entry.namespace): permission_options = [ (force_text(permission.pk), permission) for permission in permissions diff --git a/mayan/apps/common/mixins.py b/mayan/apps/common/mixins.py index b3a128d9d2..e9c84797ba 100644 --- a/mayan/apps/common/mixins.py +++ b/mayan/apps/common/mixins.py @@ -335,8 +335,7 @@ class ViewPermissionCheckMixin(object): def dispatch(self, request, *args, **kwargs): if self.view_permission: Permission.check_permissions( - requester=self.request.user, - permissions=(self.view_permission,) + permissions=(self.view_permission,), user=self.request.user ) return super( diff --git a/mayan/apps/navigation/classes.py b/mayan/apps/navigation/classes.py index 338e74b50b..0b6c7cf0b1 100644 --- a/mayan/apps/navigation/classes.py +++ b/mayan/apps/navigation/classes.py @@ -366,15 +366,15 @@ class Link(object): if resolved_object: try: AccessControlList.objects.check_access( - permissions=self.permissions, user=request.user, - obj=resolved_object, related=self.permissions_related + obj=resolved_object, permissions=self.permissions, + related=self.permissions_related, user=request.user ) except PermissionDenied: return None else: try: Permission.check_permissions( - requester=request.user, permissions=self.permissions + permissions=self.permissions, user=request.user ) except PermissionDenied: return None diff --git a/mayan/apps/navigation/utils.py b/mayan/apps/navigation/utils.py index 0d67993041..563ffd4af0 100644 --- a/mayan/apps/navigation/utils.py +++ b/mayan/apps/navigation/utils.py @@ -23,8 +23,7 @@ def get_cascade_condition(app_label, model_name, object_permission, view_permiss if view_permission: try: Permission.check_permissions( - requester=context.request.user, - permissions=(view_permission,) + permissions=(view_permission,), user=context.request.user ) except PermissionDenied: pass diff --git a/mayan/apps/permissions/classes.py b/mayan/apps/permissions/classes.py index 3f984ad85a..ef857f862c 100644 --- a/mayan/apps/permissions/classes.py +++ b/mayan/apps/permissions/classes.py @@ -59,7 +59,7 @@ class Permission(object): for namespace, permissions in itertools.groupby(cls.all(), lambda entry: entry.namespace): permission_options = [ - (force_text(permission.uuid), permission) for permission in permissions + (force_text(permission.pk), permission) for permission in permissions ] results.append( (namespace, permission_options) @@ -73,19 +73,19 @@ class Permission(object): ) @classmethod - def check_permissions(cls, requester, permissions): + def check_permissions(cls, permissions, user): try: for permission in permissions: - if permission.stored_permission.requester_has_this(requester): + if permission.stored_permission.user_has_this(user=user): return True except TypeError: # Not a list of permissions, just one - if permissions.stored_permission.requester_has_this(requester): + if permissions.stored_permission.user_has_this(user=user): return True - logger.debug('User "%s" does not have permissions "%s"', - requester, - permissions) + logger.debug( + 'User "%s" does not have permissions "%s"', user, permissions + ) raise PermissionDenied(_('Insufficient permissions.')) @classmethod @@ -95,14 +95,6 @@ class Permission(object): else: return cls._permissions[pk].stored_permission - @classmethod - def get_for_holder(cls, holder): - StoredPermission = apps.get_model( - app_label='permissions', model_name='StoredPermission' - ) - - return StoredPermission.get_for_holder(holder) - @classmethod def invalidate_cache(cls): cls._stored_permissions_cache = {} @@ -116,8 +108,8 @@ class Permission(object): self.namespace = namespace self.name = name self.label = label - self.pk = self.uuid - self.__class__._permissions[self.uuid] = self + self.pk = self.get_pk() + self.__class__._permissions[self.pk] = self def __repr__(self): return self.pk @@ -125,25 +117,22 @@ class Permission(object): def __str__(self): return force_text(self.label) + def get_pk(self): + return '%s.%s' % (self.namespace.name, self.name) + @property def stored_permission(self): - StoredPermission = apps.get_model( - app_label='permissions', model_name='StoredPermission' - ) - try: - return self.__class__._stored_permissions_cache[self.uuid] + return self.__class__._stored_permissions_cache[self.pk] except KeyError: + StoredPermission = apps.get_model( + app_label='permissions', model_name='StoredPermission' + ) + stored_permission, created = StoredPermission.objects.get_or_create( namespace=self.namespace.name, name=self.name, ) - stored_permission.volatile_permission = self - self.__class__._stored_permissions_cache[ - self.uuid - ] = stored_permission - return stored_permission - @property - def uuid(self): - return '%s.%s' % (self.namespace.name, self.name) + self.__class__._stored_permissions_cache[self.pk] = stored_permission + return stored_permission diff --git a/mayan/apps/permissions/managers.py b/mayan/apps/permissions/managers.py index 39cd52756c..3cf9f29e38 100644 --- a/mayan/apps/permissions/managers.py +++ b/mayan/apps/permissions/managers.py @@ -3,7 +3,6 @@ from __future__ import unicode_literals import logging from django.db import models -from django.contrib.contenttypes.models import ContentType logger = logging.getLogger(__name__) @@ -16,9 +15,3 @@ class RoleManager(models.Manager): class StoredPermissionManager(models.Manager): def get_by_natural_key(self, namespace, name): return self.get(namespace=namespace, name=name) - - def get_for_holder(self, holder): - ct = ContentType.objects.get_for_model(holder) - return self.model.objects.filter( - permissionholder__holder_type=ct - ).filter(permissionholder__holder_id=holder.pk) diff --git a/mayan/apps/permissions/models.py b/mayan/apps/permissions/models.py index 186442a6c1..9c6448e425 100644 --- a/mayan/apps/permissions/models.py +++ b/mayan/apps/permissions/models.py @@ -16,6 +16,11 @@ logger = logging.getLogger(__name__) @python_2_unicode_compatible class StoredPermission(models.Model): + """ + This model is the counterpart of the permissions.classes.Permission + class. Allows storing a database counterpart of a permission class. + It is used to store the permissions help by a role or in an ACL. + """ namespace = models.CharField(max_length=64, verbose_name=_('Namespace')) name = models.CharField(max_length=64, verbose_name=_('Name')) @@ -29,22 +34,38 @@ class StoredPermission(models.Model): def __str__(self): try: - return force_text(self.get_volatile_permission()) + return force_text(self.volatile_permission) except KeyError: return self.name - def get_volatile_permission_id(self): + @property + def volatile_permission_id(self): + """ + Return the identifier of the real permission class represented by + this model instance. + """ return '{}.{}'.format(self.namespace, self.name) - def get_volatile_permission(self): + @property + def volatile_permission(self): + """ + Returns the real class of the permission represented by this model + instance. + """ return Permission.get( - pk=self.get_volatile_permission_id(), proxy_only=True + pk=self.volatile_permission_id, proxy_only=True ) def natural_key(self): return (self.namespace, self.name) - def requester_has_this(self, user): + def user_has_this(self, user): + """ + Helper method to check if an user has been granted this permission. + The check is done sequentially over all of the user's groups and + roles. The check is interrupted at the first positive result. + The check always returns True for superusers or staff users. + """ if user.is_superuser or user.is_staff: logger.debug( 'Permission "%s" granted to user "%s" as superuser or staff', @@ -52,24 +73,24 @@ class StoredPermission(models.Model): ) return True - # Request is one of the permission's holders? - for group in user.groups.all(): - for role in group.roles.all(): - if self in role.permissions.all(): - logger.debug( - 'Permission "%s" granted to user "%s" through role "%s"', - self, user, role - ) - return True - - logger.debug( - 'Fallthru: Permission "%s" not granted to user "%s"', self, user - ) - return False + if Role.objects.filter(groups__user=user, permissions=self).exists(): + return True + else: + logger.debug( + 'Fallthru: Permission "%s" not granted to user "%s"', self, user + ) + return False @python_2_unicode_compatible class Role(models.Model): + """ + This model represents a Role. Roles are permission units. They are the + only object to which permissions can be granted. They are themselves + containers too, containing Groups, which are organization units. Roles + are the basic method to grant a permission to a group. Permissions granted + to a group using a role, are granted for the entire system. + """ label = models.CharField( max_length=64, unique=True, verbose_name=_('Label') ) @@ -92,7 +113,7 @@ class Role(models.Model): return self.label def get_absolute_url(self): - return reverse('permissions:role_list') + return reverse(viewname='permissions:role_list') def natural_key(self): return (self.label,) diff --git a/mayan/apps/permissions/serializers.py b/mayan/apps/permissions/serializers.py index 0a8d5e4e95..4753bb3867 100644 --- a/mayan/apps/permissions/serializers.py +++ b/mayan/apps/permissions/serializers.py @@ -20,7 +20,7 @@ class PermissionSerializer(serializers.Serializer): def to_representation(self, instance): if isinstance(instance, StoredPermission): return super(PermissionSerializer, self).to_representation( - instance.get_volatile_permission() + instance.volatile_permission ) else: return super(PermissionSerializer, self).to_representation( diff --git a/mayan/apps/permissions/tests/test_models.py b/mayan/apps/permissions/tests/test_models.py index 5aef04dc79..f654195183 100644 --- a/mayan/apps/permissions/tests/test_models.py +++ b/mayan/apps/permissions/tests/test_models.py @@ -15,7 +15,7 @@ class PermissionTestCase(BaseTestCase): def test_no_permissions(self): with self.assertRaises(PermissionDenied): Permission.check_permissions( - requester=self.user, permissions=(permission_role_view,) + permissions=(permission_role_view,), user=self.user ) def test_with_permissions(self): @@ -25,7 +25,7 @@ class PermissionTestCase(BaseTestCase): try: Permission.check_permissions( - requester=self.user, permissions=(permission_role_view,) + permissions=(permission_role_view,), user=self.user ) except PermissionDenied: self.fail('PermissionDenied exception was not expected.') diff --git a/mayan/apps/permissions/views.py b/mayan/apps/permissions/views.py index 3536bf9a4e..d5e06dc1d8 100644 --- a/mayan/apps/permissions/views.py +++ b/mayan/apps/permissions/views.py @@ -126,13 +126,12 @@ class SetupRolePermissionsView(AssignRemoveView): def generate_choices(entries): results = [] + # Sort permissions by their translatable label entries = sorted( - entries, key=lambda x: ( - x.get_volatile_permission().namespace.label, - x.get_volatile_permission().label - ) + entries, key=lambda permission: permission.volatile_permission.label ) + # Group permissions by namespace for namespace, permissions in itertools.groupby(entries, lambda entry: entry.namespace): permission_options = [ (force_text(permission.pk), permission) for permission in permissions diff --git a/mayan/apps/rest_api/permissions.py b/mayan/apps/rest_api/permissions.py index 88c37cd746..69147c1f86 100644 --- a/mayan/apps/rest_api/permissions.py +++ b/mayan/apps/rest_api/permissions.py @@ -19,7 +19,7 @@ class MayanPermission(BasePermission): if required_permission: try: Permission.check_permissions( - requester=request.user, permissions=required_permission + permissions=required_permission, user=request.user ) except PermissionDenied: return False