From 01702d8fb78f50638e585a76a8700e8fde0cc050 Mon Sep 17 00:00:00 2001 From: Fabian Braun Date: Tue, 20 Aug 2024 11:57:09 +0200 Subject: [PATCH] feat: Add cache for permission checks (#1486) --- .github/workflows/test.yml | 2 +- filer/admin/folderadmin.py | 4 + filer/admin/permissionadmin.py | 9 + filer/cache.py | 84 ++++++++++ ...x_together_remove_folder_level_and_more.py | 154 ++++++++++++++++++ filer/migrations/0012_file_mime_type.py | 2 +- filer/models/foldermodels.py | 9 +- tests/requirements/django-main.txt | 1 + tests/settings.py | 1 + tests/test_admin.py | 2 + tests/test_permission_cache.py | 47 ++++++ 11 files changed, 312 insertions(+), 3 deletions(-) create mode 100644 filer/cache.py create mode 100644 filer/migrations/0001_squashed_0016_alter_folder_index_together_remove_folder_level_and_more.py create mode 100644 tests/test_permission_cache.py diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 136b3ef29..caf84495b 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -39,6 +39,6 @@ jobs: run: echo "CUSTOM_IMAGE=custom_image.Image" >> $GITHUB_ENV if: ${{ matrix.custom-image-model }} - name: Run coverage - run: coverage run setup.py test + run: coverage run tests/settings.py - name: Upload Coverage to Codecov uses: codecov/codecov-action@v1 diff --git a/filer/admin/folderadmin.py b/filer/admin/folderadmin.py index 9ed16a2e9..aded1cb1d 100644 --- a/filer/admin/folderadmin.py +++ b/filer/admin/folderadmin.py @@ -28,6 +28,7 @@ from easy_thumbnails.models import Thumbnail from .. import settings +from ..cache import clear_folder_permission_cache from ..models import File, Folder, FolderPermission, FolderRoot, ImagesWithMissingData, UnsortedImages, tools from ..settings import FILER_IMAGE_MODEL, FILER_PAGINATE_BY, TABLE_LIST_TYPE from ..thumbnail_processors import normalize_subject_location @@ -107,6 +108,9 @@ def save_form(self, request, form, change): Given a ModelForm return an unsaved instance. ``change`` is True if the object is being changed, and False if it's being added. """ + if not change: + # New folder invalidates the folder permission cache (or it will not be visible) + clear_folder_permission_cache(request.user) r = form.save(commit=False) parent_id = request.GET.get('parent_id', None) if not parent_id: diff --git a/filer/admin/permissionadmin.py b/filer/admin/permissionadmin.py index eb42d66f6..1e8d75d54 100644 --- a/filer/admin/permissionadmin.py +++ b/filer/admin/permissionadmin.py @@ -2,6 +2,7 @@ from django.utils.translation import gettext_lazy as _ from .. import settings +from ..cache import clear_folder_permission_cache class PermissionAdmin(admin.ModelAdmin): @@ -31,3 +32,11 @@ def get_model_perms(self, request): 'change': enable_permissions, 'delete': enable_permissions, } + + def save_model(self, request, obj, form, change): + clear_folder_permission_cache(request.user) + super().save_model(request, obj, form, change) + + def delete_model(self, request, obj): + clear_folder_permission_cache(request.user) + super().delete_model(request, obj) diff --git a/filer/cache.py b/filer/cache.py new file mode 100644 index 000000000..293366b40 --- /dev/null +++ b/filer/cache.py @@ -0,0 +1,84 @@ +import typing + +from django.core.cache import cache + +from django.contrib.auth import get_user_model + + +User = get_user_model() + + +def get_folder_perm_cache_key(user: User, permission: str) -> str: + """ + Generates a unique cache key for a given user and permission. + + The key is a string in the format "filer:perm:", i.e. it does not + contain the user id. This will be sufficient for most use cases. + + Patch this method to include the user id in the cache key if necessary, e.g., + for far more than 1,000 admin users to make the cached unit require less memory. + + Parameters: + user (User): The user for whom the cache key is being generated. + permission (str): The permission for which the cache key is being generated. + + Returns: + str: The generated cache key. + """ + return f"filer:perm:{permission}" + + +def get_folder_permission_cache(user: User, permission: str) -> typing.Optional[dict]: + """ + Retrieves the cached folder permissions for a given user and permission. + + If the cache value exists, it returns the permissions for the user. + If the cache value does not exist, it returns None. + + Parameters: + user (User): The user for whom the permissions are being retrieved. + 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. + """ + cache_value = cache.get(get_folder_perm_cache_key(user, permission)) + if cache_value: + return cache_value.get(user.pk, None) + return None + + +def clear_folder_permission_cache(user: User, permission: typing.Optional[str] = None) -> None: + """ + Clears the cached folder permissions for a given user. + + If a specific permission is provided, it clears the cache for that permission only. + If no specific permission is provided, it clears the cache for all permissions. + + Parameters: + user (User): The user for whom the permissions are being cleared. + permission (str, optional): The specific permission to clear. Defaults to None. + """ + if permission is None: + for perm in ['can_read', 'can_edit', 'can_add_children']: + cache.delete(get_folder_perm_cache_key(user, perm)) + else: + cache.delete(get_folder_perm_cache_key(user, permission)) + + +def update_folder_permission_cache(user: User, permission: str, id_list: list[int]) -> None: + """ + Updates the cached folder permissions for a given user and permission. + + It first retrieves the current permissions from the cache (or an empty dictionary if none exist). + Then it updates the permissions for the user with the provided list of IDs. + Finally, it sets the updated permissions back into the cache. + + Parameters: + user (User): The user for whom the permissions are being updated. + permission (str): The permission to update. + id_list (list): The list of IDs to set as the new permissions. + """ + perms = get_folder_permission_cache(user, permission) or {} + perms[user.pk] = id_list + cache.set(get_folder_perm_cache_key(user, permission), perms) diff --git a/filer/migrations/0001_squashed_0016_alter_folder_index_together_remove_folder_level_and_more.py b/filer/migrations/0001_squashed_0016_alter_folder_index_together_remove_folder_level_and_more.py new file mode 100644 index 000000000..a3138ac0a --- /dev/null +++ b/filer/migrations/0001_squashed_0016_alter_folder_index_together_remove_folder_level_and_more.py @@ -0,0 +1,154 @@ +# Generated by Django 3.2.25 on 2024-08-19 14:49 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion +import filer.fields.multistorage_file +import filer.models.filemodels +import filer.models.mixins + + +class Migration(migrations.Migration): + + replaces = [('filer', '0001_initial'), ('filer', '0002_auto_20150606_2003'), ('filer', '0003_thumbnailoption'), ('filer', '0004_auto_20160328_1434'), ('filer', '0005_auto_20160623_1425'), ('filer', '0006_auto_20160623_1627'), ('filer', '0007_auto_20161016_1055'), ('filer', '0008_auto_20171117_1313'), ('filer', '0009_auto_20171220_1635'), ('filer', '0010_auto_20180414_2058'), ('filer', '0011_auto_20190418_0137'), ('filer', '0012_file_mime_type'), ('filer', '0013_image_width_height_to_float'), ('filer', '0014_folder_permission_choices'), ('filer', '0015_alter_file_owner_alter_file_polymorphic_ctype_and_more'), ('filer', '0016_alter_folder_index_together_remove_folder_level_and_more')] + + initial = True + + dependencies = [ + ('contenttypes', '0001_initial'), + ('contenttypes', '0002_remove_content_type_name'), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('auth', '0001_initial'), + ] + + operations = [ + migrations.CreateModel( + name='Clipboard', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('user', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='filer_clipboards', to=settings.AUTH_USER_MODEL, verbose_name='user')), + ], + options={ + 'verbose_name': 'clipboard', + 'verbose_name_plural': 'clipboards', + }, + ), + migrations.CreateModel( + name='Folder', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('name', models.CharField(max_length=255, verbose_name='name')), + ('uploaded_at', models.DateTimeField(auto_now_add=True, verbose_name='uploaded at')), + ('created_at', models.DateTimeField(auto_now_add=True, verbose_name='created at')), + ('modified_at', models.DateTimeField(auto_now=True, verbose_name='modified at')), + ('owner', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='filer_owned_folders', to=settings.AUTH_USER_MODEL, verbose_name='owner')), + ('parent', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, related_name='children', to='filer.folder', verbose_name='parent')), + ], + options={ + 'ordering': ('name',), + 'verbose_name': 'Folder', + 'verbose_name_plural': 'Folders', + 'permissions': (('can_use_directory_listing', 'Can use directory listing'),), + 'unique_together': {('parent', 'name')}, + }, + bases=(models.Model, filer.models.mixins.IconsMixin), + ), + migrations.CreateModel( + name='File', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('file', filer.fields.multistorage_file.MultiStorageFileField(blank=True, max_length=255, null=True, upload_to=filer.fields.multistorage_file.generate_filename_multistorage, verbose_name='file')), + ('_file_size', models.BigIntegerField(blank=True, null=True, verbose_name='file size')), + ('sha1', models.CharField(blank=True, default='', max_length=40, verbose_name='sha1')), + ('has_all_mandatory_data', models.BooleanField(default=False, editable=False, verbose_name='has all mandatory data')), + ('original_filename', models.CharField(blank=True, max_length=255, null=True, verbose_name='original filename')), + ('name', models.CharField(blank=True, default='', max_length=255, verbose_name='name')), + ('description', models.TextField(blank=True, null=True, verbose_name='description')), + ('uploaded_at', models.DateTimeField(auto_now_add=True, verbose_name='uploaded at')), + ('modified_at', models.DateTimeField(auto_now=True, verbose_name='modified at')), + ('is_public', models.BooleanField(default=filer.models.filemodels.is_public_default, help_text='Disable any permission checking for this file. File will be publicly accessible to anyone.', verbose_name='Permissions disabled')), + ('folder', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, related_name='all_files', to='filer.folder', verbose_name='folder')), + ('owner', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='owned_%(class)ss', to=settings.AUTH_USER_MODEL, verbose_name='owner')), + ('polymorphic_ctype', models.ForeignKey(editable=False, null=True, on_delete=django.db.models.deletion.CASCADE, related_name='polymorphic_%(app_label)s.%(class)s_set+', to='contenttypes.contenttype')), + ('mime_type', models.CharField(default='application/octet-stream', help_text='MIME type of uploaded content', max_length=255, validators=[filer.models.filemodels.mimetype_validator])), + ], + options={ + 'verbose_name': 'file', + 'verbose_name_plural': 'files', + }, + bases=(models.Model, filer.models.mixins.IconsMixin), + ), + migrations.CreateModel( + name='ClipboardItem', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('clipboard', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='filer.clipboard', verbose_name='clipboard')), + ('file', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='filer.file', verbose_name='file')), + ], + options={ + 'verbose_name': 'clipboard item', + 'verbose_name_plural': 'clipboard items', + }, + ), + migrations.CreateModel( + name='ThumbnailOption', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('name', models.CharField(max_length=100, verbose_name='name')), + ('width', models.IntegerField(help_text='width in pixel.', verbose_name='width')), + ('height', models.IntegerField(help_text='height in pixel.', verbose_name='height')), + ('crop', models.BooleanField(default=True, verbose_name='crop')), + ('upscale', models.BooleanField(default=True, verbose_name='upscale')), + ], + options={ + 'ordering': ('width', 'height'), + 'verbose_name': 'thumbnail option', + 'verbose_name_plural': 'thumbnail options', + }, + ), + migrations.AddField( + model_name='clipboard', + name='files', + field=models.ManyToManyField(related_name='in_clipboards', through='filer.ClipboardItem', to='filer.File', verbose_name='files'), + ), + migrations.CreateModel( + name='FolderPermission', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('type', models.SmallIntegerField(choices=[(0, 'all items'), (1, 'this item only'), (2, 'this item and all children')], default=0, verbose_name='type')), + ('everybody', models.BooleanField(default=False, verbose_name='everybody')), + ('can_edit', models.SmallIntegerField(blank=True, choices=[(None, 'inherit'), (1, 'allow'), (0, 'deny')], default=None, null=True, verbose_name='can edit')), + ('can_read', models.SmallIntegerField(blank=True, choices=[(None, 'inherit'), (1, 'allow'), (0, 'deny')], default=None, null=True, verbose_name='can read')), + ('can_add_children', models.SmallIntegerField(blank=True, choices=[(None, 'inherit'), (1, 'allow'), (0, 'deny')], default=None, null=True, verbose_name='can add children')), + ('folder', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, to='filer.folder', verbose_name='folder')), + ('group', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, related_name='filer_folder_permissions', to='auth.group', verbose_name='group')), + ('user', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='filer_folder_permissions', to=settings.AUTH_USER_MODEL, verbose_name='user')), + ], + options={ + 'verbose_name': 'folder permission', + 'verbose_name_plural': 'folder permissions', + }, + ), + migrations.CreateModel( + name='Image', + fields=[ + ('file_ptr', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, parent_link=True, primary_key=True, related_name='%(app_label)s_%(class)s_file', serialize=False, to='filer.file')), + ('_height', models.FloatField(blank=True, null=True)), + ('_width', models.FloatField(blank=True, null=True)), + ('date_taken', models.DateTimeField(blank=True, editable=False, null=True, verbose_name='date taken')), + ('default_alt_text', models.CharField(blank=True, max_length=255, null=True, verbose_name='default alt text')), + ('default_caption', models.CharField(blank=True, max_length=255, null=True, verbose_name='default caption')), + ('author', models.CharField(blank=True, max_length=255, null=True, verbose_name='author')), + ('must_always_publish_author_credit', models.BooleanField(default=False, verbose_name='must always publish author credit')), + ('must_always_publish_copyright', models.BooleanField(default=False, verbose_name='must always publish copyright')), + ('subject_location', models.CharField(blank=True, default='', max_length=64, verbose_name='subject location')), + ], + options={ + 'swappable': 'FILER_IMAGE_MODEL', + 'verbose_name': 'image', + 'verbose_name_plural': 'images', + 'default_manager_name': 'objects', + }, + bases=('filer.file',), + ), + ] diff --git a/filer/migrations/0012_file_mime_type.py b/filer/migrations/0012_file_mime_type.py index 2d33fb6c0..c680eada7 100644 --- a/filer/migrations/0012_file_mime_type.py +++ b/filer/migrations/0012_file_mime_type.py @@ -28,5 +28,5 @@ class Migration(migrations.Migration): name='mime_type', field=models.CharField(default='application/octet-stream', help_text='MIME type of uploaded content', max_length=255, validators=[filer.models.filemodels.mimetype_validator]), ), - migrations.RunPython(guess_mimetypes, reverse_code=migrations.RunPython.noop), + migrations.RunPython(guess_mimetypes, reverse_code=migrations.RunPython.noop, elidable=True), ] diff --git a/filer/models/foldermodels.py b/filer/models/foldermodels.py index 7b396e8e9..bfe663b1b 100644 --- a/filer/models/foldermodels.py +++ b/filer/models/foldermodels.py @@ -11,6 +11,7 @@ from .. import settings as filer_settings from . import mixins +from ..cache import get_folder_permission_cache, update_folder_permission_cache class FolderPermissionManager(models.Manager): @@ -34,6 +35,10 @@ def get_add_children_id_list(self, user): def __get_id_list(self, user, attr): if user.is_superuser or not filer_settings.FILER_ENABLE_PERMISSIONS: return 'All' + cached_id_list = get_folder_permission_cache(user, attr) + if cached_id_list: + return cached_id_list + allow_list = set() deny_list = set() group_ids = user.groups.all().values_list('id', flat=True) @@ -71,7 +76,9 @@ def __get_id_list(self, user, attr): deny_list.update(perm.folder.get_descendants_ids()) # Deny has precedence over allow - return allow_list - deny_list + id_list = allow_list - deny_list + update_folder_permission_cache(user, attr, id_list) + return id_list class Folder(models.Model, mixins.IconsMixin): diff --git a/tests/requirements/django-main.txt b/tests/requirements/django-main.txt index c35200e8f..2abb6cd9c 100644 --- a/tests/requirements/django-main.txt +++ b/tests/requirements/django-main.txt @@ -1,4 +1,5 @@ -r base.txt git+https://github.com/django/django@main#egg=Django +git+https://github.com/SmileyChris/easy-thumbnails@master#egg=easy-thumbnails django_polymorphic>=3.1 diff --git a/tests/settings.py b/tests/settings.py index 4cf78b2e3..4636d8f06 100644 --- a/tests/settings.py +++ b/tests/settings.py @@ -68,6 +68,7 @@ } } }, + 'THUMBNAIL_DEFAULT_STORAGE_ALIAS': 'default', # for the lack of any other storage defined 'SECRET_KEY': '__secret__', 'DEFAULT_AUTO_FIELD': 'django.db.models.AutoField', } diff --git a/tests/test_admin.py b/tests/test_admin.py index 2ba23b3df..71fdd249f 100644 --- a/tests/test_admin.py +++ b/tests/test_admin.py @@ -1395,6 +1395,8 @@ def test_with_permission_given_to_parent_folder(self): can_edit=FolderPermission.ALLOW, can_read=FolderPermission.ALLOW, can_add_children=FolderPermission.ALLOW) + from filer.cache import clear_folder_permission_cache + clear_folder_permission_cache(self.staff_user) response = self.client.get( reverse('admin:filer-directory_listing', kwargs={'folder_id': self.parent.id})) diff --git a/tests/test_permission_cache.py b/tests/test_permission_cache.py new file mode 100644 index 000000000..e9dd52ab8 --- /dev/null +++ b/tests/test_permission_cache.py @@ -0,0 +1,47 @@ +from django.contrib.auth import get_user_model +from django.core.cache import cache +from django.test import TestCase +from filer.cache import get_folder_perm_cache_key, get_folder_permission_cache, clear_folder_permission_cache, update_folder_permission_cache + + +User = get_user_model() + + +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] + + def tearDown(self): + clear_folder_permission_cache(self.user) + + def test_get_folder_perm_cache_key_generates_unique_key_for_user_and_permission(self): + key = get_folder_perm_cache_key(self.user, self.permission) + self.assertEqual(key, f"filer:perm:{self.permission}") + + def test_get_folder_permission_cache_returns_permissions_for_existing_cache(self): + cache.set(get_folder_perm_cache_key(self.user, self.permission), {self.user.pk: self.id_list}) + permissions = get_folder_permission_cache(self.user, self.permission) + self.assertEqual(permissions, self.id_list) + + def test_get_folder_permission_cache_returns_none_for_non_existing_cache(self): + permissions = get_folder_permission_cache(self.user, self.permission) + self.assertIsNone(permissions) + + def test_clear_folder_permission_cache_clears_all_permissions_for_user(self): + cache.set(get_folder_perm_cache_key(self.user, 'can_read'), {self.user.pk: ['can_read']}) + cache.set(get_folder_perm_cache_key(self.user, 'can_edit'), {self.user.pk: ['can_edit']}) + clear_folder_permission_cache(self.user) + self.assertIsNone(cache.get(get_folder_perm_cache_key(self.user, 'can_read'))) + self.assertIsNone(cache.get(get_folder_perm_cache_key(self.user, 'can_edit'))) + + def test_clear_folder_permission_cache_clears_specific_permission_for_user(self): + cache.set(get_folder_perm_cache_key(self.user, self.permission), {self.user.pk: self.id_list}) + clear_folder_permission_cache(self.user, self.permission) + self.assertIsNone(cache.get(get_folder_perm_cache_key(self.user, self.permission))) + + def test_update_folder_permission_cache_updates_permissions_for_user_and_permission(self): + 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)