diff --git a/cms/private_media/bulk_operations.py b/cms/private_media/bulk_operations.py index 2b10be9b..add4a8cb 100644 --- a/cms/private_media/bulk_operations.py +++ b/cms/private_media/bulk_operations.py @@ -45,12 +45,10 @@ def bulk_set_file_permissions(files: Iterable["FieldFile"], privacy: Privacy) -> def set_file_permission_and_report(file: "FieldFile") -> None: storage = file.storage - handler: Callable[[FieldFile], bool] | None if privacy is Privacy.PRIVATE: - handler = getattr(storage, "make_private", None) + results[file] = storage.make_private(file) elif privacy is Privacy.PUBLIC: - handler = getattr(storage, "make_public", None) - results[file] = handler(file) + results[file] = storage.make_public(file) with concurrent.futures.ThreadPoolExecutor( max_workers=int(settings.PRIVATE_MEDIA_BULK_UPDATE_MAX_WORKERS) diff --git a/cms/private_media/management/commands/retry_file_permission_set_attempts.py b/cms/private_media/management/commands/retry_file_permission_set_attempts.py index e2479775..3734e354 100644 --- a/cms/private_media/management/commands/retry_file_permission_set_attempts.py +++ b/cms/private_media/management/commands/retry_file_permission_set_attempts.py @@ -12,6 +12,8 @@ class Command(BaseCommand): + dry_run: bool + def add_arguments(self, parser: Any) -> None: parser.add_argument( "--dry-run", @@ -22,7 +24,7 @@ def add_arguments(self, parser: Any) -> None: ) def handle(self, *args: Any, **options: Any) -> None: - self.dry_run = options["dry_run"] # pylint: disable=attribute-defined-outside-init + self.dry_run = options["dry_run"] if self.dry_run: self.stdout.write("This is a dry run.") @@ -56,7 +58,7 @@ def update_file_permissions( for item in model_class.objects.bulk_set_file_permissions( # type: ignore[attr-defined] items, privacy, save_changes=True ): - if not item.file_permissions_are_outdated(): + if not item.has_outdated_file_permissions(): updated_count += 1 self.stdout.write(f"File permissions successfully updated for {updated_count} {privacy} {plural}.") diff --git a/cms/private_media/models/images.py b/cms/private_media/models/images.py index c1850c58..2ce011c7 100644 --- a/cms/private_media/models/images.py +++ b/cms/private_media/models/images.py @@ -100,7 +100,7 @@ def url(self) -> str: str: URL for accessing the rendition """ image: PrivateImageMixin = self.image # pylint: disable=no-member - if image.is_public and not image.file_permissions_are_outdated(): + if image.is_public and not image.has_outdated_file_permissions(): file_url: str = self.file.url return file_url return self.serve_url diff --git a/cms/private_media/models/mixins.py b/cms/private_media/models/mixins.py index adf61a0b..6ec00fe4 100644 --- a/cms/private_media/models/mixins.py +++ b/cms/private_media/models/mixins.py @@ -84,16 +84,16 @@ def save(self, *args: Any, set_file_permissions: bool = True, **kwargs: Any) -> super().save(*args, **kwargs) # Trigger file permission updates after-the-fact - if set_file_permissions and self.file_permissions_are_outdated(): + if set_file_permissions and self.has_outdated_file_permissions(): results = bulk_set_file_permissions(self.get_privacy_controlled_files(), self.privacy) # Only update 'file_permissions_last_set' if all updates were successfull - all_updates_successful = all(results.values()) + all_updates_successful = all(results.values()) if all_updates_successful: self.file_permissions_last_set = timezone.now() kwargs.update(force_insert=False, update_fields=["file_permissions_last_set"]) super().save(*args, **kwargs) - def file_permissions_are_outdated(self) -> bool: + def has_outdated_file_permissions(self) -> bool: """Check if the file permissions are outdated relative to privacy changes.""" return self.file_permissions_last_set is None or self.file_permissions_last_set < self.privacy_last_changed diff --git a/cms/private_media/storages.py b/cms/private_media/storages.py index e496fbe4..a6653404 100644 --- a/cms/private_media/storages.py +++ b/cms/private_media/storages.py @@ -2,7 +2,7 @@ from typing import TYPE_CHECKING from botocore.exceptions import ClientError -from django.core.files.storage import FileSystemStorage, InMemoryStorage +from django.core.files.storage import FileSystemStorage from storages.backends.s3 import S3Storage if TYPE_CHECKING: @@ -43,10 +43,9 @@ def _set_file_acl(self, file: "FieldFile", acl_name: str) -> bool: class AccessControlLoggingMixin: - """ - A mixin for storage backends that do not support setting of individual file permissions. - """ - def make_private(self, file: "FieldFile") -> bool: # pylint: disable=unused-argument + """A mixin for storage backends that do not support setting of individual file permissions.""" + + def make_private(self, file: "FieldFile") -> bool: """Pretend to make the provided file private.""" logger.info( "%s does not support setting of individual file permissions to private, so skipping for: %s.", @@ -55,7 +54,7 @@ def make_private(self, file: "FieldFile") -> bool: # pylint: disable=unused-arg ) return True - def make_public(self, file: "FieldFile") -> bool: # pylint: disable=unused-argument + def make_public(self, file: "FieldFile") -> bool: """Pretend to make the provided file public.""" logger.info( "%s does not support setting of individual file permissions to public, so skipping for: %s.", @@ -67,4 +66,5 @@ def make_public(self, file: "FieldFile") -> bool: # pylint: disable=unused-argu class AccessControlLoggingFileSystemStorage(AccessControlLoggingMixin, FileSystemStorage): """A version of Django's `FileSystemStorage` backend for local development and tests, which logs - file-permission-setting requests, and always reports success.""" + file-permission-setting requests, and always reports success. + """ diff --git a/cms/private_media/tests/test_documents.py b/cms/private_media/tests/test_documents.py index 2864cf9f..45390c27 100644 --- a/cms/private_media/tests/test_documents.py +++ b/cms/private_media/tests/test_documents.py @@ -49,14 +49,14 @@ def test_private_document(self): logs.output, [ ( - "INFO:cms.private_media.storages:AccessControlLoggingFileSystemStorage does not support setting of individual " - f"file permissions to private, so skipping for: {document.file.name}." + "INFO:cms.private_media.storages:AccessControlLoggingFileSystemStorage does not support setting " + f"of individual file permissions to private, so skipping for: {document.file.name}." ) ], ) # File permissions should be considered up-to-date - self.assertFalse(document.file_permissions_are_outdated()) + self.assertFalse(document.has_outdated_file_permissions()) # Setting privacy to the same value should not trigger an update to 'privacy_last_changed value_before = document.privacy_last_changed @@ -68,12 +68,12 @@ def test_private_document(self): self.assertGreater(document.privacy_last_changed, value_before) # File permissions should now be considered outdated - self.assertTrue(document.file_permissions_are_outdated()) + self.assertTrue(document.has_outdated_file_permissions()) # Resaving should trigger an update to file permissions and the 'file_permissions_last_set' # timestamp, resolving the issue document.save() - self.assertFalse(document.file_permissions_are_outdated()) + self.assertFalse(document.has_outdated_file_permissions()) def test_public_document(self): """Test the behaviour of public documents: @@ -96,14 +96,14 @@ def test_public_document(self): logs.output, [ ( - "INFO:cms.private_media.storages:AccessControlLoggingFileSystemStorage does not support setting of individual " - f"file permissions to public, so skipping for: {document.file.name}." + "INFO:cms.private_media.storages:AccessControlLoggingFileSystemStorage does not support setting " + f"of individual file permissions to public, so skipping for: {document.file.name}." ) ], ) # File permissions should be considered up-to-date - self.assertFalse(document.file_permissions_are_outdated()) + self.assertFalse(document.has_outdated_file_permissions()) # Setting privacy to the same value should not trigger an update to 'privacy_last_changed value_before = document.privacy_last_changed @@ -115,12 +115,12 @@ def test_public_document(self): self.assertGreater(document.privacy_last_changed, value_before) # File permissions should now be considered outdated - self.assertTrue(document.file_permissions_are_outdated()) + self.assertTrue(document.has_outdated_file_permissions()) # Resaving should trigger an update to file permissions and the 'file_permissions_last_set' # timestamp, resolving the issue document.save() - self.assertFalse(document.file_permissions_are_outdated()) + self.assertFalse(document.has_outdated_file_permissions()) def test_invalid_privacy_value_raises_value_error(self): """Verify that attempting to create a document with an invalid privacy value raises ValueError.""" @@ -170,7 +170,9 @@ def domain_prefixed_url(name: str) -> str: ], ) - @override_settings(STORAGES={"default": {"BACKEND": "cms.private_media.storages.AccessControlLoggingFileSystemStorage"}}) + @override_settings( + STORAGES={"default": {"BACKEND": "cms.private_media.storages.AccessControlLoggingFileSystemStorage"}} + ) def test_file_permission_setting_success(self): """Test successful file permission setting using AccessControlLoggingFileSystemStorage: - Verify permissions are set correctly for both public and private documents. @@ -180,10 +182,12 @@ def test_file_permission_setting_success(self): private_document = DocumentFactory(collection=self.root_collection) public_document = DocumentFactory(_privacy=Privacy.PUBLIC, collection=self.root_collection) - self.assertFalse(private_document.file_permissions_are_outdated()) - self.assertFalse(public_document.file_permissions_are_outdated()) + self.assertFalse(private_document.has_outdated_file_permissions()) + self.assertFalse(public_document.has_outdated_file_permissions()) - @override_settings(STORAGES={"default": {"BACKEND": "cms.private_media.storages.AccessControlLoggingFileSystemStorage"}}) + @override_settings( + STORAGES={"default": {"BACKEND": "cms.private_media.storages.AccessControlLoggingFileSystemStorage"}} + ) @mock.patch( "cms.private_media.storages.AccessControlLoggingFileSystemStorage.make_private", return_value=False, @@ -203,9 +207,9 @@ def test_file_permission_setting_failure(self, mock_make_public, mock_make_priva public_document = DocumentFactory(_privacy=Privacy.PUBLIC, collection=self.root_collection) mock_make_private.assert_called_once_with(private_document.file) - self.assertTrue(private_document.file_permissions_are_outdated()) + self.assertTrue(private_document.has_outdated_file_permissions()) mock_make_public.assert_called_once_with(public_document.file) - self.assertTrue(public_document.file_permissions_are_outdated()) + self.assertTrue(public_document.has_outdated_file_permissions()) @override_settings( @@ -248,7 +252,7 @@ def test_bulk_make_public(self): # Verify all images are now public for obj in self.model.objects.only("_privacy", "file_permissions_last_set", "privacy_last_changed"): self.assertIs(obj.privacy, Privacy.PUBLIC) - self.assertFalse(obj.file_permissions_are_outdated()) + self.assertFalse(obj.has_outdated_file_permissions()) # Another attempt should result in no updates with self.assertNumQueries(1): @@ -271,7 +275,7 @@ def test_bulk_make_private(self): # Verify all images are now private for image in self.model.objects.only("_privacy", "file_permissions_last_set", "privacy_last_changed"): self.assertIs(image.privacy, Privacy.PRIVATE) - self.assertFalse(image.file_permissions_are_outdated()) + self.assertFalse(image.has_outdated_file_permissions()) # Another attempt should result in no updates with self.assertNumQueries(1): @@ -319,6 +323,6 @@ def test_serve_public_document_with_outdated_file_permissions(self): """Test the serve view behaviour for public documents with outdated file permissions.""" self.model.objects.filter(id=self.public_document.id).update(file_permissions_last_set=None) self.public_document.refresh_from_db() - self.assertTrue(self.public_document.file_permissions_are_outdated()) + self.assertTrue(self.public_document.has_outdated_file_permissions()) response = self.client.get(self.public_document.url) self.assertEqual(response.status_code, 200) diff --git a/cms/private_media/tests/test_images.py b/cms/private_media/tests/test_images.py index 483e3a5d..be99ea7f 100644 --- a/cms/private_media/tests/test_images.py +++ b/cms/private_media/tests/test_images.py @@ -54,14 +54,14 @@ def test_private_image(self): logs.output, [ ( - "INFO:cms.private_media.storages:AccessControlLoggingFileSystemStorage does not support setting of individual " - f"file permissions to private, so skipping for: {image.file.name}." + "INFO:cms.private_media.storages:AccessControlLoggingFileSystemStorage does not support setting " + f"of individual file permissions to private, so skipping for: {image.file.name}." ) ], ) # File permissions should be considered up-to-date - self.assertFalse(image.file_permissions_are_outdated()) + self.assertFalse(image.has_outdated_file_permissions()) # Setting privacy to the same value should not trigger an update to 'privacy_last_changed value_before = image.privacy_last_changed @@ -73,12 +73,12 @@ def test_private_image(self): self.assertGreater(image.privacy_last_changed, value_before) # File permissions should now be considered outdated - self.assertTrue(image.file_permissions_are_outdated()) + self.assertTrue(image.has_outdated_file_permissions()) # Resaving should trigger an update to file permissions and the 'file_permissions_last_set' # timestamp, resolving the issue image.save() - self.assertFalse(image.file_permissions_are_outdated()) + self.assertFalse(image.has_outdated_file_permissions()) def test_private_image_renditions(self): """Test that private image renditions use serve URLs instead of direct file URLs.""" @@ -114,14 +114,14 @@ def test_public_image(self): logs.output, [ ( - "INFO:cms.private_media.storages:AccessControlLoggingFileSystemStorage does not support setting of individual " - f"file permissions to public, so skipping for: {image.file.name}." + "INFO:cms.private_media.storages:AccessControlLoggingFileSystemStorage does not support setting " + f"of individual file permissions to public, so skipping for: {image.file.name}." ) ], ) # File permissions should be considered up-to-date - self.assertFalse(image.file_permissions_are_outdated()) + self.assertFalse(image.has_outdated_file_permissions()) # Setting privacy to the same value should not trigger an update to 'privacy_last_changed value_before = image.privacy_last_changed @@ -133,12 +133,12 @@ def test_public_image(self): self.assertGreater(image.privacy_last_changed, value_before) # File permissions should now be considered outdated - self.assertTrue(image.file_permissions_are_outdated()) + self.assertTrue(image.has_outdated_file_permissions()) # Resaving should trigger an update to file permissions and the 'file_permissions_last_set' # timestamp, resolving the issue image.save() - self.assertFalse(image.file_permissions_are_outdated()) + self.assertFalse(image.has_outdated_file_permissions()) def test_public_image_renditions(self): """Test rendition.url behaviour for public image renditions: @@ -157,7 +157,7 @@ def test_public_image_renditions(self): # However, if the file permissions are outdated, rendition.url should return a serve URL with mock.patch( - "cms.private_media.models.PrivateMediaMixin.file_permissions_are_outdated", + "cms.private_media.models.PrivateMediaMixin.has_outdated_file_permissions", return_value=True, ): for rendition in renditions.values(): @@ -210,17 +210,21 @@ def test_invalid_privacy_value_raises_value_error(self): with self.assertRaises(ValueError): ImageFactory(_privacy="invalid", collection=self.root_collection) - @override_settings(STORAGES={"default": {"BACKEND": "cms.private_media.storages.AccessControlLoggingFileSystemStorage"}}) + @override_settings( + STORAGES={"default": {"BACKEND": "cms.private_media.storages.AccessControlLoggingFileSystemStorage"}} + ) def test_file_permission_setting_success(self): """Test successful file permission setting using a storage backend that supports it.""" with self.assertNoLogs("cms.private_media.bulk_operations"): private_image = ImageFactory(collection=self.root_collection) public_image = ImageFactory(_privacy=Privacy.PUBLIC, collection=self.root_collection) - self.assertFalse(private_image.file_permissions_are_outdated()) - self.assertFalse(public_image.file_permissions_are_outdated()) + self.assertFalse(private_image.has_outdated_file_permissions()) + self.assertFalse(public_image.has_outdated_file_permissions()) - @override_settings(STORAGES={"default": {"BACKEND": "cms.private_media.storages.AccessControlLoggingFileSystemStorage"}}) + @override_settings( + STORAGES={"default": {"BACKEND": "cms.private_media.storages.AccessControlLoggingFileSystemStorage"}} + ) @mock.patch( "cms.private_media.storages.AccessControlLoggingFileSystemStorage.make_private", return_value=False, @@ -240,9 +244,9 @@ def test_file_permission_setting_failure(self, mock_make_public, mock_make_priva public_image = ImageFactory(_privacy=Privacy.PUBLIC, collection=self.root_collection) mock_make_private.assert_called_once_with(private_image.file) - self.assertTrue(private_image.file_permissions_are_outdated()) + self.assertTrue(private_image.has_outdated_file_permissions()) mock_make_public.assert_called_once_with(public_image.file) - self.assertTrue(public_image.file_permissions_are_outdated()) + self.assertTrue(public_image.has_outdated_file_permissions()) @override_settings( @@ -289,7 +293,7 @@ def test_bulk_make_public(self): # Verify all images are now public for obj in self.model.objects.only("_privacy", "file_permissions_last_set", "privacy_last_changed"): self.assertIs(obj.privacy, Privacy.PUBLIC) - self.assertFalse(obj.file_permissions_are_outdated()) + self.assertFalse(obj.has_outdated_file_permissions()) # Another attempt should result in no updates with self.assertNumQueries(1): @@ -314,7 +318,7 @@ def test_bulk_make_private(self): # Verify all images are now private for image in self.model.objects.only("_privacy", "file_permissions_last_set", "privacy_last_changed"): self.assertIs(image.privacy, Privacy.PRIVATE) - self.assertFalse(image.file_permissions_are_outdated()) + self.assertFalse(image.has_outdated_file_permissions()) # Another attempt should result in no updates with self.assertNumQueries(1): @@ -372,7 +376,7 @@ def test_serve_public_image_with_outdated_file_permissions(self): """Test the serve view behaviour for public image renditions with outdated file permissions.""" self.model.objects.filter(id=self.public_image.id).update(file_permissions_last_set=None) self.public_image.refresh_from_db() - self.assertTrue(self.public_image.file_permissions_are_outdated()) + self.assertTrue(self.public_image.has_outdated_file_permissions()) for rendition in self.public_image_renditions.values(): response = self.client.get(rendition.serve_url) self.assertEqual(response.status_code, 200) diff --git a/cms/private_media/views.py b/cms/private_media/views.py index 645aef99..9776212a 100644 --- a/cms/private_media/views.py +++ b/cms/private_media/views.py @@ -17,7 +17,7 @@ def serve(self, rendition: "AbstractPrivateRendition") -> "HttpResponse": # If there's no reason (within our control) for the file not to be served by # media infrastructure, redirect - if image.is_public and not image.file_permissions_are_outdated(): + if image.is_public and not image.has_outdated_file_permissions(): return redirect(rendition.file.url) # Block access to private images if the user has insufficient permissions diff --git a/docs/custom-features/media_privacy.md b/docs/custom-features/media_privacy.md index a83aa00b..6d8cdf27 100644 --- a/docs/custom-features/media_privacy.md +++ b/docs/custom-features/media_privacy.md @@ -44,7 +44,7 @@ Whilst file-permission setting requests are quite reliable in S3, they can fail Because a media item's `file_permissions_last_set` timestamp is only updated when all file-permission setting attempts were successful, for media items with outdated file permissions, the `file_permissions_last_set` timestamp will trail behind the `privacy_last_changed` value. -For an individual object, the `file_permissions_are_outdated()` method will return `True` if the `file_permissions_last_set` timestamp is eariler than `privacy_last_changed`. This is used in a few places to vary the behaviour. Specifically, the `href` value for images will continue to point to the media serve view, so that it can still be served to users. The image serve view also checks it before redirecting users to the direct file URL. +For an individual object, the `has_outdated_file_permissions()` method will return `True` if the `file_permissions_last_set` timestamp is eariler than `privacy_last_changed`. This is used in a few places to vary the behaviour. Specifically, the `href` value for images will continue to point to the media serve view, so that it can still be served to users. The image serve view also checks it before redirecting users to the direct file URL. The timestamp values can also be used to identify affected media items in bulk. This is the approach taken by the `retry_file_permission_set_attempts` management command, which runs regularly (every 5 minutes) in hosted environments to help keep file permissions up-to-date. It uses the same bulk-processing logic as the signal handlers, so will retry failed requests, mark any items that are successfully updated, and log any errors that occured.