Update image permission assignment through algorithm jobs (#4386)

Avoids the use of the slow `update_viewer_groups_permissions` method by
assigning the `view_image` permissions directly. The downside of this is
that the permissions will need to be kept in sync in both `signals.py`
and `_get_expected_job_viewer_groups` but we have good test coverage for
this.

Closes
DIAGNijmegen/rse-grand-challenge-admin#663

Author: jmsmkn

app/grandchallenge/algorithms/signals.py

@@ -1,14 +1,14 @@
-from django.db.models.signals import m2m_changed
+from django.db.models.signals import m2m_changed, pre_delete
 from django.dispatch import receiver
 from guardian.shortcuts import assign_perm, remove_perm
 
 from grandchallenge.algorithms.models import Job
-from grandchallenge.components.models import ComponentInterfaceValue
+from grandchallenge.cases.models import Image
 
 
 @receiver(m2m_changed, sender=Job.inputs.through)
 @receiver(m2m_changed, sender=Job.outputs.through)
-def update_input_image_permissions(
+def update_view_image_permissions_on_job_io_change(  # noqa:C901
     sender, instance, action, reverse, model, pk_set, **_
 ):
     """
@@ -21,56 +21,67 @@ def update_input_image_permissions(
         return
 
     if sender._meta.label_lower == "algorithms.job_inputs":
-        forward_lookup = "inputs"
         reverse_lookup = "algorithms_jobs_as_input"
     elif sender._meta.label_lower == "algorithms.job_outputs":
-        forward_lookup = "outputs"
         reverse_lookup = "algorithms_jobs_as_output"
     else:
         raise RuntimeError("m2m is only valid for Job inputs and outputs.")
 
     if reverse:
-        component_interface_values = ComponentInterfaceValue.objects.filter(
-            pk=instance.pk, image__isnull=False
-        )
+        images = Image.objects.filter(componentinterfacevalue__pk=instance.pk)
         if pk_set is None:
             # When using a _clear action, pk_set is None
             # https://docs.djangoproject.com/en/2.2/ref/signals/#m2m-changed
             jobs = getattr(instance, reverse_lookup).all()
         else:
             jobs = model.objects.filter(pk__in=pk_set)
+
+        jobs = jobs.prefetch_related("viewer_groups").only("viewer_groups")
     else:
         jobs = [instance]
+
         if pk_set is None:
             # When using a _clear action, pk_set is None
             # https://docs.djangoproject.com/en/2.2/ref/signals/#m2m-changed
-            component_interface_values = getattr(
-                instance, forward_lookup
-            ).filter(image__isnull=False)
+            images = Image.objects.filter(
+                **{f"componentinterfacevalue__{reverse_lookup}__in": jobs}
+            )
         else:
-            component_interface_values = model.objects.filter(
-                pk__in=pk_set, image__isnull=False
+            images = Image.objects.filter(
+                componentinterfacevalue__pk__in=pk_set
             )
 
-    _update_image_permissions(
-        jobs=jobs,
-        component_interface_values=component_interface_values,
-        exclude_jobs=action == "pre_clear",
-    )
+    images = images.distinct()
 
+    if action == "post_add":
+        for job in jobs:
+            for group in job.viewer_groups.all():
+                assign_perm("view_image", group, images)
+
+    elif action in {"post_remove", "pre_clear"}:
+        exclude_jobs = jobs if action == "pre_clear" else None
+
+        for image in images:
+            # We cannot remove image permissions directly as the groups
+            # may have permissions through another object
+            image.update_viewer_groups_permissions(exclude_jobs=exclude_jobs)
+
+    else:
+        raise NotImplementedError
 
-def _update_image_permissions(
-    *, jobs, component_interface_values, exclude_jobs: bool
-):
-    for civ in component_interface_values:
-        # image__isnull=False is used above so we know that civ.image exists
-        civ.image.update_viewer_groups_permissions(
-            exclude_jobs=jobs if exclude_jobs else None
-        )
+
+def _get_images_for_jobs(*, jobs):
+    input_images = Image.objects.filter(
+        componentinterfacevalue__algorithms_jobs_as_input__in=jobs
+    )
+    output_images = Image.objects.filter(
+        componentinterfacevalue__algorithms_jobs_as_output__in=jobs
+    )
+    return input_images.union(output_images)
 
 
 @receiver(m2m_changed, sender=Job.viewer_groups.through)
-def update_group_permissions(
+def update_permissions_on_viewer_groups_change(  # noqa:C901
     *_, instance, action, reverse, model, pk_set, **__
 ):
     if action not in ["post_add", "post_remove", "pre_clear"]:
@@ -90,23 +101,34 @@ def update_group_permissions(
         else:
             groups = model.objects.filter(pk__in=pk_set)
 
-    operation = assign_perm if "add" in action else remove_perm
+    images = _get_images_for_jobs(jobs=jobs)
 
-    for job in jobs:
+    if action == "post_add":
         for group in groups:
-            operation("view_job", group, job)
+            assign_perm("view_job", group, jobs)
+            assign_perm("view_image", group, images)
 
-    queryset = ComponentInterfaceValue.objects.filter(
-        image__isnull=False
-    ).select_related("image")
+    elif action in {"post_remove", "pre_clear"}:
+        for group in groups:
+            for job in jobs:
+                remove_perm("view_job", group, job)
 
-    input_civs = queryset.filter(algorithms_jobs_as_input__in=jobs)
-    output_civs = queryset.filter(algorithms_jobs_as_output__in=jobs)
+        exclude_jobs = jobs if action == "pre_clear" else None
 
-    component_interface_values = {*input_civs, *output_civs}
+        for image in images:
+            # We cannot remove image permissions directly as the groups
+            # may have permissions through another object
+            image.update_viewer_groups_permissions(exclude_jobs=exclude_jobs)
 
-    _update_image_permissions(
-        jobs=jobs,
-        component_interface_values=component_interface_values,
-        exclude_jobs=action == "pre_clear",
-    )
+    else:
+        raise NotImplementedError
+
+
+@receiver(pre_delete, sender=Job)
+def update_view_image_permissions_on_job_deletion(*_, instance: Job, **__):
+    jobs = [instance]
+
+    for image in _get_images_for_jobs(jobs=jobs):
+        # We cannot remove image permissions directly as the groups
+        # may have permissions through another object
+        image.update_viewer_groups_permissions(exclude_jobs=jobs)

app/grandchallenge/core/admin.py

@@ -2,6 +2,8 @@
 from django.contrib.flatpages.admin import FlatPageAdmin
 from django.contrib.flatpages.forms import FlatpageForm
 from django.contrib.flatpages.models import FlatPage
+from django_celery_results.admin import TaskResultAdmin
+from django_celery_results.models import TaskResult
 
 from grandchallenge.core.widgets import MarkdownEditorAdminWidget
 
@@ -29,3 +31,23 @@ class GroupObjectPermissionAdmin(admin.ModelAdmin):
 
 admin.site.unregister(FlatPage)
 admin.site.register(FlatPage, MarkdownFlatPageAdmin)
+
+
+class TaskResultAdminWithDuration(TaskResultAdmin):
+    list_display = (
+        "task_id",
+        "periodic_task_name",
+        "task_name",
+        "date_done",
+        "get_duration",
+        "status",
+        "worker",
+    )
+
+    @admin.display(description="Duration")
+    def get_duration(self, obj):
+        return obj.date_done - obj.date_started
+
+
+admin.site.unregister(TaskResult)
+admin.site.register(TaskResult, TaskResultAdminWithDuration)

app/grandchallenge/evaluation/tasks.py

@@ -1,7 +1,6 @@
 import uuid
 from datetime import timedelta
 
-from billiard.exceptions import SoftTimeLimitExceeded
 from celery.utils.log import get_task_logger
 from django.conf import settings
 from django.core.exceptions import ValidationError
@@ -202,12 +201,7 @@ def prepare_and_execute_evaluation(*, evaluation_pk):
 
 
 @acks_late_micro_short_task(
-    retry_on=(
-        TooManyJobsScheduled,
-        LockNotAcquiredException,
-        # TODO remove SoftTimeLimitExceeded, temporary workaround for https://github.com/DIAGNijmegen/rse-grand-challenge-admin/issues/663
-        SoftTimeLimitExceeded,
-    )
+    retry_on=(TooManyJobsScheduled, LockNotAcquiredException)
 )
 @transaction.atomic
 def create_algorithm_jobs_for_evaluation(*, evaluation_pk, first_run):

app/tests/algorithms_tests/test_signals.py

@@ -1,9 +1,11 @@
 import pytest
+from django.contrib.auth.models import Group
 from guardian.shortcuts import get_perms
 
 from tests.algorithms_tests.factories import AlgorithmJobFactory
 from tests.algorithms_tests.utils import TwoAlgorithms
 from tests.components_tests.factories import ComponentInterfaceValueFactory
+from tests.evaluation_tests.test_permissions import get_groups_with_set_perms
 from tests.factories import GroupFactory, ImageFactory, UserFactory
 from tests.utils import get_view_for_user
 
@@ -287,3 +289,24 @@ def test_group_clearing(self, reverse):
             assert "view_job" not in get_perms(group, job)
             assert "view_image" not in get_perms(group, civ_in.image)
             assert "view_image" not in get_perms(group, civ_out.image)
+
+
+@pytest.mark.django_db
+def test_permissions_removed_on_job_deletion(settings):
+    job = AlgorithmJobFactory(time_limit=60, public=True)
+    image = ImageFactory()
+
+    job.inputs.add(ComponentInterfaceValueFactory(image=image))
+
+    reg_and_anon = Group.objects.get(
+        name=settings.REGISTERED_AND_ANON_USERS_GROUP_NAME
+    )
+
+    assert get_groups_with_set_perms(image) == {
+        reg_and_anon: {"view_image"},
+        job.viewers: {"view_image"},
+    }
+
+    job.delete()
+
+    assert get_groups_with_set_perms(image) == {}