Merge pull request #4526 from open-formulieren/chore/4492-remove-uplo…

…ad-ids-from-session

Remove storing upload IDs in the session

src/openforms/formio/api/views.py

@@ -16,7 +16,6 @@
 from openforms.submissions.api.renderers import PlainTextErrorRenderer
 from openforms.submissions.attachments import clean_mime_type
 from openforms.submissions.models import TemporaryFileUpload
-from openforms.submissions.utils import add_upload_to_session
 
 from .serializers import TemporaryFileUploadSerializer
 
@@ -73,8 +72,6 @@ def post(self, request, *args, **kwargs):
             content_type=clean_mime_type(file.content_type),
             file_size=file.size,
         )
-        add_upload_to_session(upload, self.request.session)
-
         return Response(
             self.serializer_class(instance=upload, context={"request": request}).data
         )

src/openforms/formio/tests/test_api_fileupload.py

@@ -5,7 +5,6 @@
 from unittest.mock import patch
 
 from django.conf import settings
-from django.core.cache import caches
 from django.core.files.uploadedfile import SimpleUploadedFile
 from django.test import override_settings, tag
 from django.utils.translation import gettext as _
@@ -15,14 +14,11 @@
 from rest_framework import status
 from rest_framework.reverse import reverse
 from rest_framework.test import APITestCase, APITransactionTestCase
-from tenacity import retry, retry_if_exception_type, stop_after_attempt
 
 from openforms.config.models import GlobalConfiguration
 from openforms.submissions.attachments import temporary_upload_from_url
-from openforms.submissions.constants import UPLOADS_SESSION_KEY
 from openforms.submissions.tests.factories import SubmissionFactory
 from openforms.submissions.tests.mixins import SubmissionsMixin
-from openforms.tests.utils import log_flaky
 
 TEST_FILES = Path(__file__).parent.resolve() / "files"
 
@@ -87,13 +83,12 @@ def test_upload_view(self):
 
         # use the convenient helper to check the model instance
         upload = temporary_upload_from_url(body["url"])
+        assert upload is not None
         self.assertEqual(upload.file_name, "my-file.txt")
         self.assertEqual(upload.content_type, "text/plain")
         self.assertEqual(upload.content.read(), b"my content")
         self.assertEqual(upload.file_size, 10)
-
-        # added to session
-        self.assertEqual([str(upload.uuid)], self.client.session[UPLOADS_SESSION_KEY])
+        self.assertEqual(upload.submission, self.submission)
 
     def test_upload_empty(self):
         self._add_submission_to_session(self.submission)
@@ -396,11 +391,6 @@ def test_cannot_connect_to_clamdav(self, m_config):
 )
 class ConcurrentUploadTests(SubmissionsMixin, APITransactionTestCase):
 
-    @retry(
-        stop=stop_after_attempt(3),
-        retry=retry_if_exception_type(AssertionError),
-        reraise=True,
-    )
     @tag("gh-3858")
     def test_concurrent_file_uploads(self):
         submission = SubmissionFactory.from_components(
@@ -439,26 +429,9 @@ def do_upload() -> str:
             futures = [executor.submit(do_upload) for _ in range(0, 2)]
             urls = [future.result() for future in as_completed(futures)]
 
-        uuids = {
-            url.removeprefix("http://testserver/api/v2/submissions/files/")
-            for url in urls
-        }
-
-        session_uuids = set(self.client.session[UPLOADS_SESSION_KEY])
-
-        # Flaky test - provide some debug output
-        if session_uuids != uuids:
-            log_flaky()
-            print("Flaky test, dumping debug output...")
-            print(f"{session_uuids=}")
-            print(f"{uuids=}")
-            print("Session cache entries:")
-            cache = caches["session"]
-            entries = dict(cache._cache).keys()
-            for _key in entries:
-                _, _, key = _key.split(":", 2)
-                print(f"  key: {key}\nvalue: {cache.get(key)}\n")
-            # clear cache for next run
-            cache.clear()
-
-        self.assertEqual(session_uuids, uuids)
+        # check that we can access the detail endpoint for each upload
+        for detail_url in urls:
+            with self.subTest(url=detail_url):
+                response = self.client.get(detail_url)
+
+                self.assertEqual(response.status_code, status.HTTP_200_OK)

src/openforms/submissions/api/mixins.py

@@ -11,11 +11,7 @@
 from ..signals import submission_complete
 from ..tasks import on_post_submission_event
 from ..tokens import submission_status_token_generator
-from ..utils import (
-    persist_user_defined_variables,
-    remove_submission_from_session,
-    remove_submission_uploads_from_session,
-)
+from ..utils import persist_user_defined_variables, remove_submission_from_session
 
 
 class SubmissionCompletionMixin:
@@ -46,7 +42,6 @@ def _complete_submission(self, submission: Submission) -> str:
         logevent.form_submit_success(submission)
 
         remove_submission_from_session(submission, self.request.session)
-        remove_submission_uploads_from_session(submission, self.request.session)
 
         # after committing the database transaction where the submissions completion is
         # stored, start processing the completion.

src/openforms/submissions/api/permissions.py

@@ -6,9 +6,9 @@
 
 from openforms.api.permissions import TimestampedTokenPermission
 
-from ..constants import SUBMISSIONS_SESSION_KEY, UPLOADS_SESSION_KEY
+from ..constants import SUBMISSIONS_SESSION_KEY
 from ..form_logic import check_submission_logic
-from ..models import SubmissionStep
+from ..models import SubmissionStep, TemporaryFileUpload
 from ..tokens import (
     submission_report_token_generator,
     submission_status_token_generator,
@@ -81,30 +81,16 @@ def filter_queryset(self, request: Request, view: APIView, queryset):
         return queryset.filter(uuid__in=active_submissions)
 
 
-class OwnsTemporaryUploadPermission(permissions.BasePermission):
+class OwnsTemporaryUploadPermission(AnyActiveSubmissionPermission):
     """
     Verify the upload is registered in the users session
     """
 
-    def has_permission(self, request: Request, view: APIView) -> bool:
-        active_uploads = request.session.get(UPLOADS_SESSION_KEY)
-        if not active_uploads:
-            return False
-        return True
-
-    def has_object_permission(self, request: Request, view: APIView, obj) -> bool:
-        active_uploads = request.session.get(UPLOADS_SESSION_KEY)
-
-        upload_url_kwarg = view.lookup_url_kwarg or view.lookup_field
-        upload_uuid = view.kwargs[upload_url_kwarg]
-
-        return str(upload_uuid) in active_uploads
-
-    def filter_queryset(self, request: Request, view: APIView, queryset):
-        active_uploads = request.session.get(UPLOADS_SESSION_KEY)
-        if not active_uploads:
-            return queryset.none()
-        return queryset.filter(uuid__in=active_uploads)
+    def has_object_permission(
+        self, request: Request, view: APIView, obj: TemporaryFileUpload
+    ) -> bool:
+        submission_uuid = str(obj.submission.uuid)
+        return owns_submission(request, submission_uuid)
 
 
 class DownloadSubmissionReportPermission(TimestampedTokenPermission):

src/openforms/submissions/api/views.py

@@ -11,7 +11,6 @@
 from openforms.api.serializers import ExceptionSerializer
 
 from ..models import SubmissionReport, TemporaryFileUpload
-from ..utils import remove_upload_from_session
 from .permissions import (
     DownloadSubmissionReportPermission,
     OwnsTemporaryUploadPermission,
@@ -107,6 +106,4 @@ def perform_destroy(self, instance):
         # delete files from disc as well if they had been already
         # saved when trying to access the next form step
         instance.attachments.all().delete()
-
-        remove_upload_from_session(instance, self.request.session)
         instance.delete()

src/openforms/submissions/constants.py

@@ -2,7 +2,6 @@
 from django.utils.translation import gettext_lazy as _
 
 SUBMISSIONS_SESSION_KEY = "form-submissions"
-UPLOADS_SESSION_KEY = "form-uploads"
 
 IMAGE_COMPONENTS = ["signature"]
 

src/openforms/submissions/tests/mixins.py

@@ -3,8 +3,8 @@
 from openforms.middleware import CSRF_TOKEN_HEADER_NAME
 from openforms.utils.tests.cache import clear_caches
 
-from ..constants import SUBMISSIONS_SESSION_KEY, UPLOADS_SESSION_KEY
-from ..models import Submission, TemporaryFileUpload
+from ..constants import SUBMISSIONS_SESSION_KEY
+from ..models import Submission
 
 
 class SubmissionsMixin:
@@ -19,17 +19,9 @@ def _add_submission_to_session(self, submission: Submission):
         session[SUBMISSIONS_SESSION_KEY] = ids
         session.save()
 
-    def _add_upload_to_session(self, upload: TemporaryFileUpload):
-        session = self.client.session
-        ids = session.get(UPLOADS_SESSION_KEY, [])
-        ids += [str(upload.uuid)]
-        session[UPLOADS_SESSION_KEY] = ids
-        session.save()
-
     def _clear_session(self):
         session = self.client.session
         session[SUBMISSIONS_SESSION_KEY] = []
-        session[UPLOADS_SESSION_KEY] = []
         session.save()
 
     def _get_session_submission_uuids(self):

src/openforms/submissions/tests/test_temporary_uploads.py

@@ -17,28 +17,18 @@
     temporary_upload_from_url,
     temporary_upload_uuid_from_url,
 )
-from openforms.submissions.constants import UPLOADS_SESSION_KEY
 from openforms.submissions.models import TemporaryFileUpload
 from openforms.submissions.models.submission_files import SubmissionFileAttachment
 from openforms.submissions.tests.factories import (
-    SubmissionFactory,
     SubmissionFileAttachmentFactory,
     TemporaryFileUploadFactory,
 )
 from openforms.submissions.tests.mixins import SubmissionsMixin
-from openforms.submissions.utils import (
-    add_upload_to_session,
-    append_to_session_list,
-    remove_from_session_list,
-    remove_upload_from_session,
-)
+from openforms.submissions.utils import append_to_session_list, remove_from_session_list
 
 
 @temp_private_root()
 class TemporaryFileUploadTest(SubmissionsMixin, APITestCase):
-    @classmethod
-    def setUpTestData(cls):
-        cls.submission = SubmissionFactory.create()
 
     def tearDown(self):
         self._clear_session()
@@ -100,7 +90,7 @@ def test_filename_properly_escaped(self):
         for filename, escaped in tests:
             with self.subTest(filename=filename, escaped=escaped):
                 upload = TemporaryFileUploadFactory.create(file_name=filename)
-                self._add_upload_to_session(upload)
+                self._add_submission_to_session(upload.submission)
                 url = reverse(
                     "api:submissions:temporary-file", kwargs={"uuid": upload.uuid}
                 )
@@ -115,23 +105,24 @@ def test_filename_properly_escaped(self):
     def test_delete_view_requires_registered_uploads(self):
         upload = TemporaryFileUploadFactory.create()
         url = reverse("api:submissions:temporary-file", kwargs={"uuid": upload.uuid})
-        response = self.client.delete(url)
-        self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN)
 
-        # add it
-        self._add_upload_to_session(upload)
+        with self.subTest("upload submission not in session"):
+            response = self.client.delete(url)
 
-        dict(self.client.session)
+            self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN)
 
-        response = self.client.delete(url)
-        self.assertEqual(response.status_code, status.HTTP_204_NO_CONTENT)
+        with self.subTest("upload submission added to session"):
+            self._add_submission_to_session(upload.submission)
+
+            response = self.client.delete(url)
+
+            self.assertEqual(response.status_code, status.HTTP_204_NO_CONTENT)
 
     def test_delete_view(self):
         upload = TemporaryFileUploadFactory.create()
         path = upload.content.path
         self.assertTrue(os.path.exists(path))
-
-        self._add_upload_to_session(upload)
+        self._add_submission_to_session(upload.submission)
 
         url = reverse("api:submissions:temporary-file", kwargs={"uuid": upload.uuid})
 
@@ -153,12 +144,10 @@ def test_delete_view(self):
         with self.assertRaises(TemporaryFileUpload.DoesNotExist):
             upload.refresh_from_db()
 
-        # 403 (because permission check fails before object is retrieved)
+        # 404 because the object cannot be found anymore
         response = self.client.delete(url)
-        self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN)
 
-        # gone
-        self.assertEqual([], self.client.session[UPLOADS_SESSION_KEY])
+        self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND)
 
     def test_delete_instance_method(self):
         upload = TemporaryFileUploadFactory.create()
@@ -193,12 +182,10 @@ def test_delete_queryset_method(self):
 
     def test_delete_temporary_file_attachement_deletes_the_saved_one_as_well(self):
         upload = TemporaryFileUploadFactory.create()
+        self._add_submission_to_session(upload.submission)
         saved_upload = SubmissionFileAttachmentFactory.create(temporary_file=upload)
         url = reverse("api:submissions:temporary-file", kwargs={"uuid": upload.uuid})
 
-        # add it to the session
-        self._add_upload_to_session(upload)
-
         # make sure the two files exist
         self.assertEqual(TemporaryFileUpload.objects.get(), upload)
         self.assertEqual(SubmissionFileAttachment.objects.get(), saved_upload)
@@ -263,17 +250,3 @@ def test_session_utils(self):
 
         # ignore values never added
         remove_from_session_list(session, "my_key", 3)
-
-    def test_session_upload_utils(self):
-        session = self.client.session
-
-        upload_1 = TemporaryFileUploadFactory.create()
-        upload_2 = TemporaryFileUploadFactory.create()
-        add_upload_to_session(upload_1, session)
-        add_upload_to_session(upload_2, session)
-
-        session_uploads = session[UPLOADS_SESSION_KEY]
-        self.assertEqual([str(upload_1.uuid), str(upload_2.uuid)], session_uploads)
-
-        remove_upload_from_session(upload_1, session)
-        self.assertEqual([str(upload_2.uuid)], session_uploads)

src/openforms/submissions/utils.py

@@ -28,15 +28,10 @@
 from openforms.utils.urls import build_absolute_uri
 from openforms.variables.constants import FormVariableSources
 
-from .constants import SUBMISSIONS_SESSION_KEY, UPLOADS_SESSION_KEY
+from .constants import SUBMISSIONS_SESSION_KEY
 from .exceptions import FormDeactivated, FormMaintenance
 from .form_logic import check_submission_logic
-from .models import (
-    Submission,
-    SubmissionReport,
-    SubmissionValueVariable,
-    TemporaryFileUpload,
-)
+from .models import Submission, SubmissionReport, SubmissionValueVariable
 from .tokens import submission_report_token_generator
 
 logger = logging.getLogger(__name__)
@@ -162,29 +157,6 @@ def remove_submission_from_session(
     remove_from_session_list(session, SUBMISSIONS_SESSION_KEY, str(submission.uuid))
 
 
-def add_upload_to_session(upload: TemporaryFileUpload, session: SessionBase) -> None:
-    """
-    Store the upload UUID in the request session for authorization checks.
-    """
-    append_to_session_list(session, UPLOADS_SESSION_KEY, str(upload.uuid))
-
-
-def remove_upload_from_session(
-    upload: TemporaryFileUpload, session: SessionBase
-) -> None:
-    """
-    Remove the submission UUID from the session if it's present.
-    """
-    remove_from_session_list(session, UPLOADS_SESSION_KEY, str(upload.uuid))
-
-
-def remove_submission_uploads_from_session(
-    submission: Submission, session: SessionBase
-) -> None:
-    for attachment in submission.get_attachments().filter(temporary_file__isnull=False):
-        remove_upload_from_session(attachment.temporary_file, session)
-
-
 def send_confirmation_email(submission: Submission) -> None:
     logevent.confirmation_email_start(submission)