fix: Folder permission cache update sometimes raised TypeError (#1539)

* test: add failing test case `test_update_folder_permission_cache_overwrites_existing_cache_value`

* fix: TypeError in `filer.cache.update_folder_permission_cache`

* Update filer/cache.py

Ensure it is a set

Co-authored-by: Fabian Braun <[email protected]>

---------

Co-authored-by: Peter Wischer <[email protected]>
Co-authored-by: Peter Wischer <[email protected]>

Author: fsbraun

filer/cache.py

@@ -29,7 +29,7 @@ def get_folder_perm_cache_key(user: UserModel, permission: str) -> str:
     return f"filer:perm:{permission}"
 
 
-def get_folder_permission_cache(user: UserModel, permission: str) -> typing.Optional[dict]:
+def get_folder_permission_cache(user: UserModel, permission: str) -> typing.Optional[typing.Set[int]]:
     """
     Retrieves the cached folder permissions for a given user and permission.
 
@@ -43,7 +43,7 @@ def get_folder_permission_cache(user: UserModel, permission: str) -> typing.Opti
     permission (str): The permission for which the permissions are being retrieved.
 
     Returns:
-    dict or None: The permissions for the user, or None if no cache value exists.
+    set or None: The permissions for the user, or None if no cache value exists.
     """
     cache_value = cache.get(get_folder_perm_cache_key(user, permission))
     if cache_value:
@@ -71,7 +71,7 @@ def clear_folder_permission_cache(user: UserModel, permission: typing.Optional[s
         cache.delete(get_folder_perm_cache_key(user, permission))
 
 
-def update_folder_permission_cache(user: UserModel, permission: str, id_list: typing.List[int]) -> None:
+def update_folder_permission_cache(user: UserModel, permission: str, id_list: typing.Set[int]) -> None:
     """
     Updates the cached folder permissions for a given user and permission.
 
@@ -84,8 +84,8 @@ def update_folder_permission_cache(user: UserModel, permission: str, id_list: ty
         The `user` can be an instance of the default `django.contrib.auth.models.User`
         or any custom user model specified by `AUTH_USER_MODEL` in the settings.
     permission (str): The permission to update.
-    id_list (list): The list of IDs to set as the new permissions.
+    id_list (set): The set of IDs to set as the new permissions.
     """
-    perms = get_folder_permission_cache(user, permission) or {}
-    perms[user.pk] = id_list
+    perms = cache.get(get_folder_perm_cache_key(user, permission)) or {}
+    perms[user.pk] = set(id_list)
     cache.set(get_folder_perm_cache_key(user, permission), perms)

tests/test_permission_cache.py

@@ -11,7 +11,7 @@ class PermissionCacheTests(TestCase):
     def setUp(self):
         self.user = User.objects.create_user(username='testuser', password='12345')
         self.permission = 'can_read'
-        self.id_list = [1, 5, 8]
+        self.id_list = {1, 5, 8}
 
     def tearDown(self):
         clear_folder_permission_cache(self.user)
@@ -45,3 +45,9 @@ def test_update_folder_permission_cache_updates_permissions_for_user_and_permiss
         update_folder_permission_cache(self.user, self.permission, self.id_list)
         permissions = get_folder_permission_cache(self.user, self.permission)
         self.assertEqual(permissions, self.id_list)
+
+    def test_update_folder_permission_cache_overwrites_existing_cache_value(self):
+        update_folder_permission_cache(self.user, self.permission, {2})
+        update_folder_permission_cache(self.user, self.permission, self.id_list)
+        permissions = get_folder_permission_cache(self.user, self.permission)
+        self.assertEqual(permissions, self.id_list)

tests/test_permissions.py

@@ -3,6 +3,7 @@
 from django.conf import settings
 from django.contrib.admin import helpers
 from django.contrib.auth.models import Group, Permission
+from django.core.cache import cache
 from django.core.files import File as DjangoFile
 from django.test.testcases import TestCase
 from django.urls import reverse
@@ -163,6 +164,7 @@ def test_combined_groups(self):
             # We have to invalidate cache
             delattr(self.folder, 'permission_cache')
             delattr(self.folder_perm, 'permission_cache')
+            cache.clear()
 
             self.assertEqual(self.folder.has_read_permission(request1), True)
             self.assertEqual(self.folder.has_read_permission(request2), True)
@@ -172,6 +174,7 @@ def test_combined_groups(self):
             self.assertEqual(self.folder_perm.pretty_logical_path, "/test_folder2")
         finally:
             filer_settings.FILER_ENABLE_PERMISSIONS = old_setting
+            cache.clear()
 
     def test_overlapped_groups_deny1(self):
         # Tests overlapped groups with explicit deny
@@ -216,6 +219,7 @@ def test_overlapped_groups_deny1(self):
 
         finally:
             filer_settings.FILER_ENABLE_PERMISSIONS = old_setting
+            cache.clear()
 
     def test_overlapped_groups_deny2(self):
         # Tests overlapped groups with explicit deny
@@ -261,6 +265,7 @@ def test_overlapped_groups_deny2(self):
 
         finally:
             filer_settings.FILER_ENABLE_PERMISSIONS = old_setting
+            cache.clear()
 
     def test_overlapped_groups1(self):
         # Tests overlapped groups without explicit deny
@@ -305,6 +310,7 @@ def test_overlapped_groups1(self):
 
         finally:
             filer_settings.FILER_ENABLE_PERMISSIONS = old_setting
+            cache.clear()
 
     def test_overlapped_groups2(self):
         # Tests overlapped groups without explicit deny
@@ -350,6 +356,7 @@ def test_overlapped_groups2(self):
 
         finally:
             filer_settings.FILER_ENABLE_PERMISSIONS = old_setting
+            cache.clear()
 
     def test_folder_who_owner(self):
         perm = FolderPermission.objects.create(