fix rights on albums and next/previous pictures

sas/api.py

@@ -41,17 +41,12 @@ class PicturesController(ControllerBase):
             cf. https://ae.utbm.fr/user/32663/pictures/)
         """
         user: User = self.context.request.user
-        if not user.was_subscribed and filters.users_identified != {user.id}:
-            # User can view any moderated picture if he/she is subscribed.
-            # If not, he/she can view only the one he/she has been identified on
-            raise PermissionDenied
-        pictures = list(
+        return (
             filters.filter(Picture.objects.viewable_by(user))
             .distinct()
             .order_by("-parent__date", "date")
             .annotate(album=F("parent__name"))
         )
-        return pictures
 
 
 @api_controller("/sas/relation", tags="User identification on SAS pictures")

sas/models.py

@@ -20,6 +20,7 @@ from io import BytesIO
 from django.conf import settings
 from django.core.cache import cache
 from django.db import models
+from django.db.models import Exists, OuterRef
 from django.urls import reverse
 from django.utils import timezone
 from django.utils.translation import gettext_lazy as _
@@ -29,6 +30,36 @@ from core.models import SithFile, User
 from core.utils import exif_auto_rotate, resize_image
 
 
+class SasFile(SithFile):
+    """Proxy model for any file in the SAS.
+
+    May be used to have logic that should be shared by both
+    [Picture][sas.models.Picture] and [Album][sas.models.Album].
+    """
+
+    class Meta:
+        proxy = True
+
+    def can_be_viewed_by(self, user):
+        if user.is_anonymous:
+            return False
+        cache_key = (
+            f"sas:{self._meta.model_name}_viewable_by_{user.id}_in_{self.parent_id}"
+        )
+        viewable: list[int] | None = cache.get(cache_key)
+        if viewable is None:
+            viewable = list(
+                self.__class__.objects.filter(parent_id=self.parent_id)
+                .viewable_by(user)
+                .values_list("pk", flat=True)
+            )
+            cache.set(cache_key, viewable, timeout=10)
+        return self.id in viewable
+
+    def can_be_edited_by(self, user):
+        return user.is_root or user.is_in_group(pk=settings.SITH_GROUP_SAS_ADMIN_ID)
+
+
 class PictureQuerySet(models.QuerySet):
     def viewable_by(self, user: User) -> PictureQuerySet:
         """Filter the pictures that this user can view.
@@ -45,17 +76,10 @@ class PictureQuerySet(models.QuerySet):
 
 class SASPictureManager(models.Manager):
     def get_queryset(self):
-        return PictureQuerySet(self.model, using=self._db).filter(
-            is_in_sas=True, is_folder=False
-        )
+        return super().get_queryset().filter(is_in_sas=True, is_folder=False)
 
 
-class SASAlbumManager(models.Manager):
-    def get_queryset(self):
-        return super().get_queryset().filter(is_in_sas=True, is_folder=True)
-
-
-class Picture(SithFile):
+class Picture(SasFile):
     class Meta:
         proxy = True
 
@@ -68,29 +92,6 @@ class Picture(SithFile):
             (w, h) = im.size
             return (w / h) < 1
 
-    def can_be_edited_by(self, user):
-        perm = cache.get("%d_can_edit_pictures" % (user.id), None)
-        if perm is None:
-            perm = user.is_root or user.is_in_group(pk=settings.SITH_GROUP_SAS_ADMIN_ID)
-
-        cache.set("%d_can_edit_pictures" % (user.id), perm, timeout=4)
-        return perm
-
-    def can_be_viewed_by(self, user: User) -> bool:
-        if user.is_anonymous:
-            return False
-
-        cache_key = f"sas:pictures_viewable_by_{user.id}_in_{self.parent_id}"
-        viewable: list[int] | None = cache.get(cache_key)
-        if viewable is None:
-            viewable = list(
-                Picture.objects.filter(parent_id=self.parent_id)
-                .viewable_by(user)
-                .values_list("pk", flat=True)
-            )
-            cache.set(cache_key, viewable, timeout=10)
-        return self.id in viewable
-
     def get_download_url(self):
         return reverse("sas:download", kwargs={"picture_id": self.id})
 
@@ -142,48 +143,53 @@ class Picture(SithFile):
 
     def get_next(self):
         if self.is_moderated:
-            return (
-                self.parent.children.filter(
+            pictures_qs = self.parent.children.filter(
                 is_moderated=True,
                 asked_for_removal=False,
                 is_folder=False,
                 id__gt=self.id,
             )
-                .order_by("id")
-                .first()
-            )
         else:
-            return (
-                Picture.objects.filter(id__gt=self.id, is_moderated=False)
-                .order_by("id")
-                .first()
-            )
+            pictures_qs = Picture.objects.filter(id__gt=self.id, is_moderated=False)
+        return pictures_qs.order_by("id").first()
 
     def get_previous(self):
         if self.is_moderated:
-            return (
-                self.parent.children.filter(
+            pictures_qs = self.parent.children.filter(
                 is_moderated=True,
                 asked_for_removal=False,
                 is_folder=False,
                 id__lt=self.id,
             )
-                .order_by("id")
-                .last()
-            )
         else:
-            return (
-                Picture.objects.filter(id__lt=self.id, is_moderated=False)
-                .order_by("-id")
-                .first()
+            pictures_qs = Picture.objects.filter(id__lt=self.id, is_moderated=False)
+        return pictures_qs.order_by("-id").first()
+
+
+class AlbumQuerySet(models.QuerySet):
+    def viewable_by(self, user: User) -> PictureQuerySet:
+        """Filter the albums that this user can view.
+
+        Warnings:
+            Calling this queryset method may add several additional requests.
+        """
+        if user.is_root or user.is_in_group(pk=settings.SITH_GROUP_SAS_ADMIN_ID):
+            return self.all()
+        return self.filter(
+            Exists(Picture.objects.filter(parent_id=OuterRef("pk")).viewable_by(user))
         )
 
 
-class Album(SithFile):
+class SASAlbumManager(models.Manager):
+    def get_queryset(self):
+        return super().get_queryset().filter(is_in_sas=True, is_folder=True)
+
+
+class Album(SasFile):
     class Meta:
         proxy = True
 
-    objects = SASAlbumManager()
+    objects = SASAlbumManager.from_queryset(AlbumQuerySet)()
 
     @property
     def children_pictures(self):
@@ -193,15 +199,6 @@ class Album(SithFile):
     def children_albums(self):
         return Album.objects.filter(parent=self)
 
-    def can_be_edited_by(self, user):
-        return user.is_in_group(pk=settings.SITH_GROUP_SAS_ADMIN_ID)
-
-    def can_be_viewed_by(self, user):
-        # file = SithFile.objects.filter(id=self.id).first()
-        return self.can_be_edited_by(user) or (
-            self.is_in_sas and self.is_moderated and user.was_subscribed
-        )  # or user.can_view(file)
-
     def get_absolute_url(self):
         return reverse("sas:album", kwargs={"album_id": self.id})
 

sas/templates/sas/picture.jinja

@@ -4,22 +4,6 @@
   <link rel="stylesheet" href="{{ scss('sas/picture.scss') }}">
 {%- endblock -%}
 
-{% block head %}
-  {{ super() }}
-
-  {% if picture.get_previous() %}
-    <link
-      rel="preload"
-      as="image"
-      href="{{ url("sas:download_compressed", picture_id=picture.get_previous().id) }}"
-    >
-  {% endif %}
-  {% if picture.get_next() %}
-    <link rel="preload" as="image" href="{{ url("sas:download_compressed", picture_id=picture.get_next().id) }}">
-  {% endif %}
-
-{% endblock %}
-
 {% block title %}
   {% trans %}SAS{% endtrans %}
 {% endblock %}
@@ -124,16 +108,16 @@
     <div class="subsection">
       <div class="navigation">
         <div id="prev">
-          {% if picture.get_previous() %}
-            <a href="{{ url( 'sas:picture', picture_id=picture.get_previous().id) }}#pict">
-              <div style="background-image: url('{{ picture.get_previous().as_picture.get_download_thumb_url() }}');"></div>
+          {% if previous_pict %}
+            <a href="{{ url( 'sas:picture', picture_id=previous_pict.id) }}#pict">
+              <div style="background-image: url('{{ previous_pict.get_download_thumb_url() }}');"></div>
             </a>
           {% endif %}
         </div>
         <div id="next">
-          {% if picture.get_next() %}
-            <a href="{{ url( 'sas:picture', picture_id=picture.get_next().id) }}#pict">
-              <div style="background-image: url('{{ picture.get_next().as_picture.get_download_thumb_url() }}');"></div>
+          {% if next_pict %}
+            <a href="{{ url( 'sas:picture', picture_id=next_pict.id) }}#pict">
+              <div style="background-image: url('{{ next_pict.get_download_thumb_url() }}');"></div>
             </a>
           {% endif %}
         </div>

sas/tests/test_api.py

@@ -89,19 +89,33 @@ class TestPictureSearch(TestSas):
         )
         assert [i["id"] for i in res.json()["results"]] == expected
 
-        # trying to access the pictures of someone else
-        res = self.client.get(
-            reverse("api:pictures") + f"?users_identified={self.user_b.id}"
-        )
-        assert res.status_code == 403
-
-        # trying to access the pictures of someone else shouldn't success,
-        # even if mixed with owned pictures
+        # trying to access the pictures of someone else mixed with owned pictures
+        # should return only owned pictures
         res = self.client.get(
             reverse("api:pictures")
             + f"?users_identified={self.user_a.id}&users_identified={self.user_b.id}"
         )
-        assert res.status_code == 403
+        assert res.status_code == 200
+        assert [i["id"] for i in res.json()["results"]] == expected
+
+        # trying to fetch everything should be the same
+        # as fetching its own pictures for a non-subscriber
+        res = self.client.get(reverse("api:pictures"))
+        assert res.status_code == 200
+        assert [i["id"] for i in res.json()["results"]] == expected
+
+        # trying to access the pictures of someone else should return only
+        # the ones where the non-subscribed user is identified too
+        res = self.client.get(
+            reverse("api:pictures") + f"?users_identified={self.user_b.id}"
+        )
+        assert res.status_code == 200
+        expected = list(
+            self.user_b.pictures.intersection(self.user_a.pictures.all())
+            .order_by("-picture__parent__date", "picture__date")
+            .values_list("picture_id", flat=True)
+        )
+        assert [i["id"] for i in res.json()["results"]] == expected
 
 
 class TestPictureRelation(TestSas):

sas/views.py

@@ -119,10 +119,11 @@ class SASMainView(FormView):
 
     def get_context_data(self, **kwargs):
         kwargs = super().get_context_data(**kwargs)
-        kwargs["categories"] = Album.objects.filter(
-            parent__id=settings.SITH_SAS_ROOT_DIR_ID
-        ).order_by("id")
-        kwargs["latest"] = Album.objects.filter(is_moderated=True).order_by("-id")[:5]
+        albums_qs = Album.objects.viewable_by(self.request.user)
+        kwargs["categories"] = list(
+            albums_qs.filter(parent_id=settings.SITH_SAS_ROOT_DIR_ID).order_by("id")
+        )
+        kwargs["latest"] = list(albums_qs.order_by("-id")[:5])
         return kwargs
 
 
@@ -180,7 +181,14 @@ class PictureView(CanViewMixin, DetailView, FormMixin):
 
     def get_context_data(self, **kwargs):
         kwargs = super().get_context_data(**kwargs)
+        pictures_qs = Picture.objects.viewable_by(self.request.user)
         kwargs["form"] = self.form
+        kwargs["next_pict"] = (
+            pictures_qs.filter(id__gt=self.object.id).order_by("id").first()
+        )
+        kwargs["previous_pict"] = (
+            pictures_qs.filter(id__lt=self.object.id).order_by("-id").first()
+        )
         return kwargs
 
     def get_success_url(self):