Merge pull request #1056 from ImageMarkup/generalize-access-queries

ImageMarkup · Jan 21, 2025 · a3fb59a · a3fb59a
2 parents 2a1e21f + e723c42
commit a3fb59a
Show file tree

Hide file tree

Showing 2 changed files with 46 additions and 20 deletions.
diff --git a/isic/core/models/image.py b/isic/core/models/image.py
@@ -246,16 +246,30 @@ def view_image_list(user_obj: User, qs: QuerySet[Image] | None = None) -> QueryS
 
         if user_obj.is_staff:
             return qs
-        if user_obj.is_authenticated:
+        elif user_obj.is_authenticated:
+            image_visibility_requirements = Q(public=True) | Q(
+                # this needs list coercion because otherwise it will be a subquery that contains
+                # the user_id, which doesn't allow users with identical privileges to share
+                # the query cache.
+                accession__cohort__contributor_id__in=list(
+                    user_obj.owned_contributors.order_by().values_list("id", flat=True)
+                )
+            )
+
+            if user_obj.imageshare_set.exists():
+                # this is the worst case scenario where we have to put the specific user into the
+                # query, guaranteeing that they won't share the cache with others.
+                # this is also the only portion that demands a left join, forcing the
+                # caller to wrap the query in a distinct() call.
+                image_visibility_requirements |= Q(shares=user_obj)
+
             # Note: permissions here must be also modified in build_elasticsearch_query and
             # LesionPermissions.view_lesion_list.
             return qs.filter(
-                Q(public=True)
-                | Q(accession__cohort__contributor__owners=user_obj)
-                | Q(shares=user_obj)
+                image_visibility_requirements,
             )
-
-        return qs.public()
+        else:
+            return qs.public()
 
     @staticmethod
     def view_image(user_obj: User, obj: Image) -> bool:

diff --git a/isic/ingest/models/lesion.py b/isic/ingest/models/lesion.py
@@ -250,15 +250,31 @@ def to_elasticsearch_document(self, *, body_only: bool = False) -> dict | EsLesi
 class LesionPermissions:
     model = Lesion
     perms = ["view_lesion"]
-    filters = {"view_lesion": "view_lesion_list"}
+    filters = {
+        "view_lesion": "view_lesion_list",
+    }
 
     @staticmethod
     def view_lesion_list(user_obj: User, qs: QuerySet[Lesion] | None = None) -> QuerySet[Lesion]:
         qs = qs if qs is not None else Lesion.objects.all()
 
         if user_obj.is_staff:
             return qs
-        if user_obj.is_authenticated:
+        elif user_obj.is_authenticated:
+            lesion_visibility_requirements = Q(accessions__image__public=True) | Q(
+                # this needs list coercion because otherwise it will be a subquery that contains
+                # the user_id, which doesn't allow users with identical privileges to share
+                # the query cache.
+                cohort__contributor_id__in=list(
+                    user_obj.owned_contributors.order_by().values_list("id", flat=True)
+                )
+            )
+
+            # only add the user share requirement if the user has shares, since it will put
+            # the user_id into the query (making query caching less effective).
+            if user_obj.imageshare_set.exists():
+                lesion_visibility_requirements |= Q(user_share_id=user_obj.id)
+
             return qs.filter(
                 id__in=Lesion.objects.values("id")
                 # if an image doesn't have shares it will return null which is skipped by BoolAnd.
@@ -268,22 +284,18 @@ def view_lesion_list(user_obj: User, qs: QuerySet[Lesion] | None = None) -> Quer
                 .alias(user_share_id=Coalesce(F("accessions__image__shares"), -1))
                 .annotate(
                     # note that these requirements are copied from ImagePermissions.view_image_list
-                    visible=BoolAnd(
-                        Q(accessions__image__public=True)
-                        | Q(cohort__contributor__owners=user_obj)
-                        | Q(user_share_id=user_obj.id)
-                    )
+                    visible=BoolAnd(lesion_visibility_requirements)
                 )
                 .filter(visible=True)
                 .values("id")
             )
-
-        return qs.filter(
-            id__in=Lesion.objects.values("id")
-            .annotate(visible=BoolAnd(Q(accessions__image__public=True)))
-            .filter(visible=True)
-            .values("id")
-        )
+        else:
+            return qs.filter(
+                id__in=Lesion.objects.values("id")
+                .annotate(visible=BoolAnd(Q(accessions__image__public=True)))
+                .filter(visible=True)
+                .values("id")
+            )
 
     @staticmethod
     def view_lesion(user_obj: User, obj: Lesion) -> bool: