From c9925325528f6251483709018f6ba4e0055bf603 Mon Sep 17 00:00:00 2001 From: Ehsan200 Date: Sat, 10 Jun 2023 14:35:19 +0330 Subject: [PATCH 1/7] feat: Add PermissionCacheMixin and cache permission methods results --- rest_framework/permissions.py | 44 ++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 16 deletions(-) diff --git a/rest_framework/permissions.py b/rest_framework/permissions.py index 8fb4569cb1..6a2d11b376 100644 --- a/rest_framework/permissions.py +++ b/rest_framework/permissions.py @@ -1,6 +1,8 @@ """ Provides a set of pluggable permission policies. """ +from functools import lru_cache + from django.http import Http404 from rest_framework import exceptions @@ -8,6 +10,16 @@ SAFE_METHODS = ('GET', 'HEAD', 'OPTIONS') +class PermissionCacheMixin: + @lru_cache + def has_permission_value(self, request, view): + return self.has_permission(request, view) + + @lru_cache + def has_object_permission_value(self, request, view, obj): + return self.has_object_permission(request, view, obj) + + class OperationHolderMixin: def __and__(self, other): return OperandHolder(AND, self, other) @@ -55,61 +67,61 @@ def __eq__(self, other): ) -class AND: +class AND(PermissionCacheMixin): def __init__(self, op1, op2): self.op1 = op1 self.op2 = op2 def has_permission(self, request, view): return ( - self.op1.has_permission(request, view) and - self.op2.has_permission(request, view) + self.op1.has_permission_value(request, view) and + self.op2.has_permission_value(request, view) ) def has_object_permission(self, request, view, obj): return ( - self.op1.has_object_permission(request, view, obj) and - self.op2.has_object_permission(request, view, obj) + self.op1.has_object_permission_value(request, view, obj) and + self.op2.has_object_permission_value(request, view, obj) ) -class OR: +class OR(PermissionCacheMixin): def __init__(self, op1, op2): self.op1 = op1 self.op2 = op2 def has_permission(self, request, view): return ( - self.op1.has_permission(request, view) or - self.op2.has_permission(request, view) + self.op1.has_permission_value(request, view) or + self.op2.has_permission_value(request, view) ) def has_object_permission(self, request, view, obj): return ( - self.op1.has_permission(request, view) - and self.op1.has_object_permission(request, view, obj) + self.op1.has_permission_value(request, view) + and self.op1.has_object_permission_value(request, view, obj) ) or ( - self.op2.has_permission(request, view) - and self.op2.has_object_permission(request, view, obj) + self.op2.has_permission_value(request, view) + and self.op2.has_object_permission_value(request, view, obj) ) -class NOT: +class NOT(PermissionCacheMixin): def __init__(self, op1): self.op1 = op1 def has_permission(self, request, view): - return not self.op1.has_permission(request, view) + return not self.op1.has_permission_value(request, view) def has_object_permission(self, request, view, obj): - return not self.op1.has_object_permission(request, view, obj) + return not self.op1.has_object_permission_value(request, view, obj) class BasePermissionMetaclass(OperationHolderMixin, type): pass -class BasePermission(metaclass=BasePermissionMetaclass): +class BasePermission(PermissionCacheMixin, metaclass=BasePermissionMetaclass): """ A base class from which all permission classes should inherit. """ From 80ce4a2cb83ee9e93c559ef402c737ca967c19ae Mon Sep 17 00:00:00 2001 From: Ehsan200 Date: Sat, 10 Jun 2023 14:36:50 +0330 Subject: [PATCH 2/7] feat: Use new methods in all views and use cached permissions in each view instance --- rest_framework/views.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/rest_framework/views.py b/rest_framework/views.py index 4c30029fdc..c72033547b 100644 --- a/rest_framework/views.py +++ b/rest_framework/views.py @@ -1,6 +1,8 @@ """ Provides an APIView class that is the base of all views in REST framework. """ +from functools import cached_property + from django.conf import settings from django.core.exceptions import PermissionDenied from django.db import connections, models @@ -277,6 +279,10 @@ def get_permissions(self): """ return [permission() for permission in self.permission_classes] + @cached_property + def cached_permissions(self): + return self.get_permissions() + def get_throttles(self): """ Instantiates and returns the list of throttles that this view uses. @@ -328,8 +334,8 @@ def check_permissions(self, request): Check if the request should be permitted. Raises an appropriate exception if the request is not permitted. """ - for permission in self.get_permissions(): - if not permission.has_permission(request, self): + for permission in self.cached_permissions: + if not permission.has_permission_value(request, self): self.permission_denied( request, message=getattr(permission, 'message', None), @@ -341,8 +347,8 @@ def check_object_permissions(self, request, obj): Check if the request should be permitted for a given object. Raises an appropriate exception if the request is not permitted. """ - for permission in self.get_permissions(): - if not permission.has_object_permission(request, self, obj): + for permission in self.cached_permissions: + if not permission.has_object_permission_value(request, self, obj): self.permission_denied( request, message=getattr(permission, 'message', None), From 175f9118e9fd3d1810214ab4e55a838a142048af Mon Sep 17 00:00:00 2001 From: Ehsan200 Date: Sat, 10 Jun 2023 14:37:09 +0330 Subject: [PATCH 3/7] test: Add new tests for cached permissions --- tests/test_permissions.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/test_permissions.py b/tests/test_permissions.py index 428480dc7e..f02a75b553 100644 --- a/tests/test_permissions.py +++ b/tests/test_permissions.py @@ -735,3 +735,31 @@ def has_object_permission(self, request, view, obj): composed_perm = (IsAuthenticatedUserOwner | permissions.IsAdminUser) hasperm = composed_perm().has_object_permission(request, None, None) assert hasperm is False + + +class PermissionsCacheTests(TestCase): + + class IsAuthenticatedUserOwnerWithCounter(permissions.IsAuthenticated): + + def __init__(self): + self.call_counter = 0 + + def has_permission(self, request, view): + self.call_counter += 1 + return True + + def test_composed_perm_permissions(self): + request = factory.get('/1', format='json') + request.user = AnonymousUser() + + composed_perm = (self.IsAuthenticatedUserOwnerWithCounter | permissions.IsAdminUser) + composed_perm_instance = composed_perm() + # in OR composed permissions has_object_permission will call has_permission too. + # we must ensure that this method (has_permission) is called once + has_permission_value = composed_perm_instance.has_permission(request, None) + has_object_permission_value = composed_perm_instance.has_object_permission(request, None, None) + + self.assertTrue(has_permission_value) + self.assertTrue(has_object_permission_value) + + self.assertEqual(composed_perm_instance.op1.call_counter, 1) From 23db9b61289dde4a107c111d4bd01d2a850b1f3c Mon Sep 17 00:00:00 2001 From: Ehsan200 Date: Sat, 10 Jun 2023 15:09:57 +0330 Subject: [PATCH 4/7] fix: Remove functools.cached_property and use django.cached_property --- rest_framework/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rest_framework/views.py b/rest_framework/views.py index c72033547b..ecce544196 100644 --- a/rest_framework/views.py +++ b/rest_framework/views.py @@ -1,7 +1,6 @@ """ Provides an APIView class that is the base of all views in REST framework. """ -from functools import cached_property from django.conf import settings from django.core.exceptions import PermissionDenied @@ -10,6 +9,7 @@ from django.http.response import HttpResponseBase from django.utils.cache import cc_delim_re, patch_vary_headers from django.utils.encoding import smart_str +from django.utils.functional import cached_property from django.views.decorators.csrf import csrf_exempt from django.views.generic import View From d87ad2366bc49f547a8de9406900e425ae64e2c3 Mon Sep 17 00:00:00 2001 From: Ehsan200 Date: Sat, 10 Jun 2023 15:15:49 +0330 Subject: [PATCH 5/7] fix: Call lru_cache decorator in PermissionCacheMixin for all python versions --- rest_framework/permissions.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rest_framework/permissions.py b/rest_framework/permissions.py index 6a2d11b376..456b7c8e12 100644 --- a/rest_framework/permissions.py +++ b/rest_framework/permissions.py @@ -11,11 +11,11 @@ class PermissionCacheMixin: - @lru_cache + @lru_cache() def has_permission_value(self, request, view): return self.has_permission(request, view) - @lru_cache + @lru_cache() def has_object_permission_value(self, request, view, obj): return self.has_object_permission(request, view, obj) From 2f230c2e85e27a7813b92faabab39b1391053fa0 Mon Sep 17 00:00:00 2001 From: Ehsan200 Date: Wed, 12 Jul 2023 14:31:50 +0330 Subject: [PATCH 6/7] fix: Use custom cache for better memory usage --- rest_framework/permissions.py | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/rest_framework/permissions.py b/rest_framework/permissions.py index 456b7c8e12..10bbef87fc 100644 --- a/rest_framework/permissions.py +++ b/rest_framework/permissions.py @@ -1,8 +1,6 @@ """ Provides a set of pluggable permission policies. """ -from functools import lru_cache - from django.http import Http404 from rest_framework import exceptions @@ -11,13 +9,20 @@ class PermissionCacheMixin: - @lru_cache() + def __init__(self): + self._cache = {} + def has_permission_value(self, request, view): - return self.has_permission(request, view) + key = (request, view) + if key not in self._cache: + self._cache[key] = self.has_permission(request, view) + return self._cache[key] - @lru_cache() def has_object_permission_value(self, request, view, obj): - return self.has_object_permission(request, view, obj) + key = (request, view, obj) + if key not in self._cache: + self._cache[key] = self.has_object_permission(request, view, obj) + return self._cache[key] class OperationHolderMixin: @@ -69,6 +74,7 @@ def __eq__(self, other): class AND(PermissionCacheMixin): def __init__(self, op1, op2): + super().__init__() self.op1 = op1 self.op2 = op2 @@ -87,6 +93,7 @@ def has_object_permission(self, request, view, obj): class OR(PermissionCacheMixin): def __init__(self, op1, op2): + super().__init__() self.op1 = op1 self.op2 = op2 @@ -108,6 +115,7 @@ def has_object_permission(self, request, view, obj): class NOT(PermissionCacheMixin): def __init__(self, op1): + super().__init__() self.op1 = op1 def has_permission(self, request, view): From 751d69a3ab0f6f1bb7ab3f0e60d8279018b41e79 Mon Sep 17 00:00:00 2001 From: Ehsan200 Date: Wed, 12 Jul 2023 14:32:34 +0330 Subject: [PATCH 7/7] fix: Call parent constructor for new Permission in test --- tests/test_permissions.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_permissions.py b/tests/test_permissions.py index f02a75b553..81a6d6c15d 100644 --- a/tests/test_permissions.py +++ b/tests/test_permissions.py @@ -742,6 +742,7 @@ class PermissionsCacheTests(TestCase): class IsAuthenticatedUserOwnerWithCounter(permissions.IsAuthenticated): def __init__(self): + super().__init__() self.call_counter = 0 def has_permission(self, request, view):