Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restrict media privacy until a referencing page is published #46

Open
wants to merge 62 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
62 commits
Select commit Hold shift + click to select a range
6fb93bd
Implement simplified private_media app
ababic Nov 29, 2024
9f912b9
Add to image and document models
ababic Nov 29, 2024
8aad840
Enable custom image serve view
ababic Nov 29, 2024
b30bd3b
Fix unrelated linter complain on user model
ababic Nov 29, 2024
4dd76f3
Purge serve and file URLs when the privacy of an object changes
ababic Dec 1, 2024
12a1480
Rejig manager methods to avoid a separate update query for 'file_perm…
ababic Dec 2, 2024
97215de
Revise signal handlers to ignore references from draft pages when dec…
ababic Dec 2, 2024
ea7466c
Placate linters
ababic Dec 2, 2024
592807f
Add tests
ababic Dec 2, 2024
1c34a50
Use the warning log level for ACL setting issues
ababic Dec 3, 2024
fb73cb3
Add docs
ababic Dec 3, 2024
92613b3
Revise model docstrings to account for changes
ababic Dec 3, 2024
c73cd07
Revise test docstrings in test_signal_handlers.py
ababic Dec 3, 2024
04e31c2
Tweak docs based on feedback
ababic Dec 3, 2024
ab6a27d
Use more recognisable issue name in pylint: disable statement
ababic Dec 3, 2024
7f7cb4f
Add a docstring to DummyPrivacySettingFileSystemStorage
ababic Dec 3, 2024
19dde02
Don't cache ServeImage view responses, and update PrivateImageMixin.g…
ababic Dec 3, 2024
f7c46e0
Update signal handlers to only fetch objects that are public or priva…
ababic Dec 3, 2024
6fc3c98
Add tests for PrivacySettingS3Storage
ababic Dec 3, 2024
5919d83
Be consistant in checking mocks across test_make_public and test_make…
ababic Dec 3, 2024
22007bb
Use mock_aws instead
ababic Dec 3, 2024
b3fa22e
Add note to docs about image serve view responses not being cached
ababic Dec 4, 2024
8b19623
Use 'info' to log unsupported file permission setting attempts
ababic Dec 5, 2024
e1ecdb3
Fix moto extras in pyproject.toml
ababic Dec 10, 2024
0621789
Regenerate lock
ababic Dec 10, 2024
585f90f
Don't double up on publish/unpublish signals for pages
ababic Dec 10, 2024
52267f2
Utilize assertIsSubclass in tests
ababic Dec 10, 2024
10fdba7
Log ACL set/put errors as exceptions
ababic Dec 10, 2024
f67ac65
Test serve views with IS_EXTERNAL_ENV=True
ababic Dec 10, 2024
8037f3d
Fixup for 10fdba7
ababic Dec 10, 2024
f0c5d04
Revert "Utilize assertIsSubclass in tests"
ababic Dec 10, 2024
aa86f2d
Fix storage tests
ababic Dec 10, 2024
439d5a1
Use InMemoryStorage for DummyPrivacySettingFileSystemStorage
ababic Dec 10, 2024
0a31a10
Re-run only specific (read-only) tests with IS_EXTERNAL_ENV=True
ababic Dec 10, 2024
a2748a6
Update unpublish_media_on_unpublish() to use a subset of 'referencing…
ababic Dec 10, 2024
f13bb20
Revise the docs
ababic Dec 10, 2024
22e3f8a
Reference integration tests in docs
ababic Dec 10, 2024
bef76b4
Fix typos
ababic Dec 10, 2024
a42a9c5
Fix another docs typo
ababic Dec 10, 2024
c16b886
One last round of typo fixes
ababic Dec 10, 2024
fe260b8
Add notes on how private and public media items behave
ababic Dec 10, 2024
593af11
Remove excess whitespace
ababic Dec 10, 2024
3a66cd0
Merge branch 'main' into feature/private-media
ababic Dec 11, 2024
ce09c50
PRIVATE_MEDIA_PERMISSION_SETTING_MAX_WORKERS -> PRIVATE_MEDIA_BULK_UP…
ababic Dec 13, 2024
16a05fb
Raise an exception when the active storage backends does not implemen…
ababic Dec 13, 2024
60fdb15
Rename PrivacySettingS3Storage to AccessControlledS3Storage and imple…
ababic Dec 13, 2024
2f5dd30
Update tests to account for logging location and storage class name c…
ababic Dec 13, 2024
dee1193
intended_privacy -> privacy
ababic Dec 13, 2024
50f131d
Improve readability in PrivateMediaMixin.save()
ababic Dec 13, 2024
f3af123
Raise storage exceptions outside of threadpool excecution
ababic Dec 13, 2024
b581823
Placate linters
ababic Dec 13, 2024
7f0de81
Return early to reduce indentation in retry_file_permission_set_attem…
ababic Dec 13, 2024
5198943
Use abstract Collection type in update_file_permissions() annotation
ababic Dec 13, 2024
89a670f
Placate mypy
ababic Dec 13, 2024
d5f2ecc
Fix bulk_operations not setting file permissions when a generator is …
ababic Dec 13, 2024
faefcac
Simplify logging in AccessControlLoggingFileSystemStorage and use alt…
ababic Dec 13, 2024
0bf2898
Update docs to reflect logging format changes
ababic Dec 13, 2024
c196dee
Revert: Don't cache ServeImage view responses, and update PrivateImag…
ababic Dec 13, 2024
e164276
Simplify PrivateImageMixin.get_privacy_controlled_serve_urls()
ababic Dec 13, 2024
aaea1e9
Merge branch 'main' into feature/private-media
ababic Dec 13, 2024
cfe3672
Merge branch 'main' into feature/private-media
ababic Dec 13, 2024
6284741
Fix rendition purge urls test
ababic Dec 13, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# Generated by Django 5.1.3 on 2024-11-29 16:19

import django.utils.timezone
from django.db import migrations, models


class Migration(migrations.Migration):
dependencies = [
("documents", "0002_alter_customdocument_file_size"),
]

operations = [
migrations.AddField(
model_name="customdocument",
name="_privacy",
field=models.CharField(default="public", max_length=10),
),
migrations.AddField(
model_name="customdocument",
name="file_permissions_last_set",
field=models.DateTimeField(editable=False, null=True),
),
migrations.AddField(
model_name="customdocument",
name="privacy_last_changed",
field=models.DateTimeField(default=django.utils.timezone.now, editable=False),
),
]
17 changes: 17 additions & 0 deletions cms/documents/migrations/0004_alter_customdocument__privacy.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# Generated by Django 5.1.3 on 2024-11-29 16:20
zerolab marked this conversation as resolved.
Show resolved Hide resolved

from django.db import migrations, models


class Migration(migrations.Migration):
dependencies = [
("documents", "0003_customdocument__privacy_and_more"),
]

operations = [
migrations.AlterField(
model_name="customdocument",
name="_privacy",
field=models.CharField(default="private", max_length=10),
),
]
12 changes: 8 additions & 4 deletions cms/documents/models.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
from wagtail.documents.models import AbstractDocument
from wagtail.documents.models import Document as WagtailDocument
from collections.abc import Sequence
from typing import ClassVar

from wagtail.documents.models import AbstractDocument, Document

class CustomDocument(AbstractDocument):
from cms.private_media.models import PrivateDocumentMixin


class CustomDocument(PrivateDocumentMixin, AbstractDocument):
"""Custom Wagtail document class.

Using a custom class from the beginning allows us to add
any customisations we may need.
"""

admin_form_fields = WagtailDocument.admin_form_fields
admin_form_fields: ClassVar[Sequence[str]] = Document.admin_form_fields
28 changes: 28 additions & 0 deletions cms/images/migrations/0003_customimage__privacy_and_more.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# Generated by Django 5.1.3 on 2024-11-29 16:19

import django.utils.timezone
from django.db import migrations, models


class Migration(migrations.Migration):
dependencies = [
("images", "0002_customimage_description"),
]

operations = [
migrations.AddField(
model_name="customimage",
name="_privacy",
field=models.CharField(default="public", max_length=10),
),
migrations.AddField(
model_name="customimage",
name="file_permissions_last_set",
field=models.DateTimeField(editable=False, null=True),
),
migrations.AddField(
model_name="customimage",
name="privacy_last_changed",
field=models.DateTimeField(default=django.utils.timezone.now, editable=False),
),
]
17 changes: 17 additions & 0 deletions cms/images/migrations/0004_alter_customimage__privacy.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# Generated by Django 5.1.3 on 2024-11-29 16:20

from django.db import migrations, models


class Migration(migrations.Migration):
dependencies = [
("images", "0003_customimage__privacy_and_more"),
]

operations = [
migrations.AlterField(
model_name="customimage",
name="_privacy",
field=models.CharField(default="private", max_length=10),
),
]
17 changes: 12 additions & 5 deletions cms/images/models.py
Original file line number Diff line number Diff line change
@@ -1,21 +1,28 @@
from collections.abc import Sequence
from typing import ClassVar

from django.db import models
from wagtail.images.models import AbstractImage, AbstractRendition, Image
from wagtail.images.models import AbstractImage, Image

from cms.private_media.models import AbstractPrivateRendition, PrivateImageMixin


class CustomImage(AbstractImage):
class CustomImage(PrivateImageMixin, AbstractImage): # type: ignore[django-manager-missing]
"""Custom Wagtail image class.

Using a custom class from the beginning allows us to add
any customisations we may need.
"""

admin_form_fields = Image.admin_form_fields
admin_form_fields: ClassVar[Sequence[str]] = Image.admin_form_fields


class Rendition(AbstractRendition):
class Rendition(AbstractPrivateRendition):
"""Our custom rendition class."""

image = models.ForeignKey["CustomImage"]("CustomImage", related_name="renditions", on_delete=models.CASCADE)
image = models.ForeignKey( # type: ignore[var-annotated]
"CustomImage", related_name="renditions", on_delete=models.CASCADE
)

class Meta:
unique_together = (("image", "filter_spec", "focal_point_key"),)
Empty file added cms/private_media/__init__.py
Empty file.
11 changes: 11 additions & 0 deletions cms/private_media/apps.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
from django.apps import AppConfig


class PrivateMediaConfig(AppConfig):
default_auto_field = "django.db.models.AutoField"
name = "cms.private_media"

def ready(self) -> None:
from .signal_handlers import register_signal_handlers # pylint: disable=import-outside-toplevel

register_signal_handlers()
57 changes: 57 additions & 0 deletions cms/private_media/bulk_operations.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
import concurrent.futures
import logging
from collections.abc import Callable, Iterable
from typing import TYPE_CHECKING

from django.conf import settings

from cms.private_media.constants import Privacy

if TYPE_CHECKING:
from django.db.models.fields.files import FieldFile


logger = logging.getLogger(__name__)


def bulk_set_file_permissions(files: Iterable["FieldFile"], intended_privacy: Privacy) -> dict["FieldFile", bool]:
"""Set file permissions for an iterable of FieldFile objects, using the
make_private() or make_public() methods of the storage backend.

Uses a thread pool to set the file permissions in parallel where possible.

Args:
files: An iterable of FieldFile objects to set permissions for
intended_privacy: The intended privacy status for the supplied files

Returns:
dict[FieldFile, bool]: A mapping of files to a boolean indicating whether
the permission setting request was successful
"""
results: dict[FieldFile, bool] = {}

def set_file_permission_and_report(file: "FieldFile") -> None:
storage = file.storage
handler: Callable[[FieldFile], bool] | None
if intended_privacy is Privacy.PRIVATE:
handler = getattr(storage, "make_private", None)
elif intended_privacy is Privacy.PUBLIC:
handler = getattr(storage, "make_public", None)
RealOrangeOne marked this conversation as resolved.
Show resolved Hide resolved

if handler is None:
Copy link
Contributor

@MebinAbraham MebinAbraham Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For private media, I would like to enforce the fact that the storage backend must have these handlers. For local dev, the handles can "log out the action". I would rather not have it pass silently if storage doesn't support it; I would like to take the approach of the storage backend must implement it.

Copy link
Author

@ababic ababic Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Funnily enough, that's how I had it originally, but I changed my mind at some point. Should be very easy to restore.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved in 16a05fb

logger.info(
"%s does not support setting of individual file permissions to %s, so skipping for: %s.",
storage.__class__.__name__,
intended_privacy,
file.name,
)
results[file] = True
else:
results[file] = handler(file)

with concurrent.futures.ThreadPoolExecutor(
max_workers=int(settings.PRIVATE_MEDIA_PERMISSION_SETTING_MAX_WORKERS)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, could be something shorter like: MEDIA_PERMISSION_MAX_WORKERS

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to stick with the PRIVATE_MEDIA_ prefix, as it makes it easier to mentally map settings to apps in the project. But, I do find the current name a little confusing, so have changed to PRIVATE_MEDIA_BULK_UPDATE_MAX_WORKERS in ce09c50.

Whilst it's still a bit lengthy, I think clarity is more important when it comes to settings.

) as executor:
executor.map(set_file_permission_and_report, files)

return results
7 changes: 7 additions & 0 deletions cms/private_media/constants.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
from django.db.models import TextChoices
from django.utils.translation import gettext_lazy as _


class Privacy(TextChoices):
PRIVATE = "private", _("Private")
PUBLIC = "public", _("Public")
Empty file.
Empty file.
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
from typing import TYPE_CHECKING, Any

from django.core.management.base import BaseCommand
from django.db.models import F

from cms.private_media.constants import Privacy
from cms.private_media.models.images import PrivateImageMixin
from cms.private_media.utils import get_private_media_models

if TYPE_CHECKING:
from cms.private_media.models import PrivateMediaMixin


class Command(BaseCommand):
def add_arguments(self, parser: Any) -> None:
parser.add_argument(
"--dry-run",
action="store_true",
dest="dry_run",
default=False,
help="Dry run -- don't change anything.",
)

def handle(self, *args: Any, **options: Any) -> None:
self.dry_run = options["dry_run"] # pylint: disable=attribute-defined-outside-init
Copy link
Contributor

@MebinAbraham MebinAbraham Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, add as class attr like the publish_bundles.py command does instead of the pylint disable?. The key point is consistency.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been resolved

if self.dry_run:
self.stdout.write("This is a dry run.")

for model in get_private_media_models():
queryset = model.objects.filter(file_permissions_last_set__lt=F("privacy_last_changed"))
if not self.dry_run and issubclass(model, PrivateImageMixin):
queryset = queryset.prefetch_related("renditions")
permissions_outdated = list(queryset)
self.stdout.write(f"{len(permissions_outdated)} {model.__name__} instances have outdated file permissions.")
if permissions_outdated:
make_private = []
make_public = []
for obj in permissions_outdated:
if obj.privacy is Privacy.PRIVATE:
make_private.append(obj)
elif obj.privacy is Privacy.PUBLIC:
make_public.append(obj)

self.update_file_permissions(model, make_private, Privacy.PRIVATE)
self.update_file_permissions(model, make_public, Privacy.PUBLIC)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: I think it is easier to read if we added some newlines and exit early to reduce indentation. It feels a bit crowded.

        for model in get_private_media_models():
            queryset = model.objects.filter(file_permissions_last_set__lt=F("privacy_last_changed"))
            if not self.dry_run and issubclass(model, PrivateImageMixin):
                queryset = queryset.prefetch_related("renditions")
            
            permissions_outdated = list(queryset)
            self.stdout.write(f"{len(permissions_outdated)} {model.__name__} instances have outdated file permissions.")
            
            if not permissions_outdated:
                return None
            
            make_private = []
            make_public = []
            for obj in permissions_outdated:
                if obj.privacy is Privacy.PRIVATE:
                    make_private.append(obj)
                elif obj.privacy is Privacy.PUBLIC:
                    make_public.append(obj)

            self.update_file_permissions(model, make_private, Privacy.PRIVATE)
            self.update_file_permissions(model, make_public, Privacy.PUBLIC)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Funnily enough, that's how I had it originally, but it was before I broke handling off to update_file_permissions, and pylint had a problem with the number of return statements. It should be okay to do that again now that update_file_permissions exists

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved in 7f0de81


def update_file_permissions(
self, model_class: type["PrivateMediaMixin"], items: list["PrivateMediaMixin"], privacy: Privacy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type annotation for args should be abstract unless the method needs it to be a list.

Copy link
Author

@ababic ababic Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely agree where it comes to reusable code, but in the case of management commands (which are self-contained, and only really reusable if you're defining a base class for other commands to inherit), I think being explicit adds value. Less... 'how you might use me' and more 'this is exactly how i'm being used'.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in 5198943

) -> None:
"""Update the file permissions for the provided items to reflect the provided privacy status."""
plural = model_class._meta.verbose_name_plural
if self.dry_run:
self.stdout.write(f"Would update file permissions for {len(items)} {privacy} {plural}.")
else:
updated_count = 0
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():
updated_count += 1

self.stdout.write(f"File permissions successfully updated for {updated_count} {privacy} {plural}.")
Loading
Loading