diff --git a/accounts/tests/test_upload.py b/accounts/tests/test_upload.py index d8b6ed9a8..d54ea10ef 100644 --- a/accounts/tests/test_upload.py +++ b/accounts/tests/test_upload.py @@ -31,7 +31,6 @@ from sounds.forms import PackForm from sounds.models import BulkUploadProgress, License, Pack, Sound from tags.models import Tag -from utils.filesystem import File from utils.test_helpers import ( create_test_files, create_user_and_sounds, @@ -99,7 +98,7 @@ def test_select_uploaded_files_to_describe(self): ) self.assertListEqual( sorted( - [os.path.basename(f.full_path) for f in self.client.session[f"{session_key_prefix}-describe_sounds"]] + [os.path.basename(f["full_path"]) for f in self.client.session[f"{session_key_prefix}-describe_sounds"]] ), sorted([filenames[idx] for idx in sounds_to_describe_idx]), ) @@ -122,7 +121,7 @@ def test_select_uploaded_files_to_describe(self): ) self.assertListEqual( sorted( - [os.path.basename(f.full_path) for f in self.client.session[f"{session_key_prefix}-describe_sounds"]] + [os.path.basename(f["full_path"]) for f in self.client.session[f"{session_key_prefix}-describe_sounds"]] ), sorted([filenames[idx] for idx in sounds_to_describe_idx]), ) @@ -159,12 +158,12 @@ def test_describe_selected_files(self): # Set license and pack data in session session = self.client.session session_key_prefix = "304298eb" - session[f"{session_key_prefix}-describe_license"] = License.objects.all()[0] + session[f"{session_key_prefix}-describe_license"] = License.objects.all()[0].id session[f"{session_key_prefix}-describe_pack"] = False session[f"{session_key_prefix}-len_original_describe_sounds"] = 2 session[f"{session_key_prefix}-describe_sounds"] = [ - File(1, filenames[0], os.path.join(user_upload_path, filenames[0]), False), - File(2, filenames[1], os.path.join(user_upload_path, filenames[1]), False), + {"name": filenames[0], "full_path": os.path.join(user_upload_path, filenames[0])}, + {"name": filenames[1], "full_path": os.path.join(user_upload_path, filenames[1])}, ] session.save() @@ -385,3 +384,145 @@ def test_bulk_describe_state_closed(self): self.client.force_login(user) resp = self.client.get(reverse("accounts-bulk-describe", args=[bulk.id])) self.assertContains(resp, "This bulk description process is closed") + + +class SessionJsonSafetyTests(TestCase): + """Regression guard: every value the upload/describe/edit flow writes to request.session + must be JSON-serialisable, so SESSION_SERIALIZER can be flipped to JSONSerializer in a + follow-up PR. If this test goes red, something in the flow is putting a Python object + (Sound/License/Pack/File/...) back into the session.""" + + fixtures = ["licenses", "user_groups", "email_preference_type"] + + def _assert_session_json_safe(self): + from django.contrib.sessions.serializers import JSONSerializer + + serializer = JSONSerializer() + for key, value in self.client.session.items(): + try: + serializer.dumps({key: value}) + except TypeError as exc: + self.fail(f"Session key {key!r} holds a non-JSON-safe value (type={type(value).__name__!r}): {exc}") + + def _assert_session_value(self, prefix, key, expected): + """The key must be present AND equal to `expected`, so that a producer write which + is silently skipped (the key never appears) fails loudly instead of letting the + json-safety sweep above pass over a session that never got the value at all.""" + full_key = f"{prefix}-{key}" + self.assertIn(full_key, self.client.session, f"producer never wrote {full_key!r}") + self.assertEqual(self.client.session[full_key], expected) + + @override_uploads_path_with_temp_directory + def test_describe_flow_session_is_json_safe(self): + filenames = ["file1.wav", "file2.wav"] + user = User.objects.create_user("testuser", email="json@xmpl.com", password="testpass") + self.client.force_login(user) + user_upload_path = settings.UPLOADS_PATH + "/%i/" % user.id + os.makedirs(user_upload_path, exist_ok=True) + create_test_files(filenames, user_upload_path) + + # Step 1: select files from pending_description -> writes describe_sounds, len_original_describe_sounds. + # generate_tree() assigns file ids starting at "file0". + resp = self.client.post( + reverse("accounts-manage-sounds", args=["pending_description"]), + {"describe": "describe", "sound-files": ["file0", "file1"]}, + ) + self.assertEqual(resp.status_code, 302) + session_key_prefix = resp.url.split("session=")[1] + describe_sounds = self.client.session[f"{session_key_prefix}-describe_sounds"] + self.assertTrue( + isinstance(describe_sounds, list) + and all(isinstance(f, dict) and set(f) == {"name", "full_path"} for f in describe_sounds), + f"describe_sounds should be a list of name/full_path dicts, got {describe_sounds!r}", + ) + self._assert_session_value(session_key_prefix, "len_original_describe_sounds", 2) + self._assert_session_json_safe() + + # Step 2: pick a license -> writes describe_license. + # Must be a license that is actually valid in LicenseForm's queryset (the + # describe_license view uses hide_old_license_versions=True). "Attribution" (4.0) is + # in the queryset and survives the 3.0 clean check, so the producer actually runs -- + # using License.objects.all()[0] would (nondeterministically) pick "Sampling+", which + # is not a valid choice, the form would be invalid and the write silently skipped. + attribution = License.objects.get(name="Attribution") + resp = self.client.post( + reverse("accounts-describe-license") + f"?session={session_key_prefix}", + {"license": str(attribution.id)}, + ) + self.assertEqual(resp.status_code, 302) + self._assert_session_value(session_key_prefix, "describe_license", attribution.id) + self._assert_session_json_safe() + + # Step 3: pick "no pack" -> writes describe_pack (= False sentinel) + resp = self.client.post( + reverse("accounts-describe-pack") + f"?session={session_key_prefix}", + {"pack-pack": PackForm.NO_PACK_CHOICE_VALUE, "pack-new_pack": ""}, + ) + self.assertEqual(resp.status_code, 302) + self._assert_session_value(session_key_prefix, "describe_pack", False) + self._assert_session_json_safe() + + def test_describe_pack_id_branches_are_json_safe(self): + # describe_pack writes the …-describe_pack key independently of earlier steps, so we + # exercise each branch against its own fresh session prefix. The two `.id` branches + # (existing pack, new pack) are the ones that previously stored a Pack instance and + # are not reached by the "no pack" path in test_describe_flow_session_is_json_safe. + user = User.objects.create_user("packuser", email="pack@xmpl.com", password="testpass") + self.client.force_login(user) + + # Existing pack -> stores data["pack"].id + existing_pack = Pack.objects.create(user=user, name="Existing pack") + resp = self.client.post( + reverse("accounts-describe-pack") + "?session=packex00", + {"pack-pack": str(existing_pack.id), "pack-new_pack": ""}, + ) + self.assertEqual(resp.status_code, 302) + self._assert_session_value("packex00", "describe_pack", existing_pack.id) + self.assertNotIsInstance(self.client.session["packex00-describe_pack"], bool) + self._assert_session_json_safe() + + # New pack -> creates the pack and stores the new pack.id + resp = self.client.post( + reverse("accounts-describe-pack") + "?session=packnw00", + {"pack-pack": PackForm.NEW_PACK_CHOICE_VALUE, "pack-new_pack": "Brand new pack"}, + ) + self.assertEqual(resp.status_code, 302) + new_pack = Pack.objects.get(user=user, name="Brand new pack") + self._assert_session_value("packnw00", "describe_pack", new_pack.id) + self.assertNotIsInstance(self.client.session["packnw00-describe_pack"], bool) + self._assert_session_json_safe() + + # No pack -> stores the False sentinel (must be the bool False, not an int id) + resp = self.client.post( + reverse("accounts-describe-pack") + "?session=packno00", + {"pack-pack": PackForm.NO_PACK_CHOICE_VALUE, "pack-new_pack": ""}, + ) + self.assertEqual(resp.status_code, 302) + self.assertIs(self.client.session["packno00-describe_pack"], False) + self._assert_session_json_safe() + + def test_edit_flow_session_is_json_safe(self): + user, _packs, sounds = create_user_and_sounds( + num_sounds=2, + num_packs=0, + processing_state="OK", + moderation_state="OK", + ) + self.client.force_login(user) + + # Select sounds for editing from manage_sounds/published -> writes edit_sounds, len_original_edit_sounds + resp = self.client.post( + reverse("accounts-manage-sounds", args=["published"]), + {"edit": "edit", "object-ids": ",".join(str(s.id) for s in sounds)}, + ) + self.assertEqual(resp.status_code, 302) + # The edit redirect appends `&session=` after `next=`, so split defensively. + session_key_prefix = resp.url.split("session=")[1].split("&")[0] + edit_sounds = self.client.session[f"{session_key_prefix}-edit_sounds"] + self.assertTrue( + all(isinstance(i, int) and not isinstance(i, bool) for i in edit_sounds), + f"edit_sounds should be a list of int ids, got {edit_sounds!r}", + ) + self.assertEqual(sorted(edit_sounds), sorted(s.id for s in sounds)) + self._assert_session_value(session_key_prefix, "len_original_edit_sounds", len(sounds)) + self._assert_session_json_safe() diff --git a/accounts/views.py b/accounts/views.py index be659f009..4dd41f5d9 100644 --- a/accounts/views.py +++ b/accounts/views.py @@ -780,9 +780,7 @@ def process_filter_and_sort_options(request, sort_options, tab): session_key_prefix = str(uuid.uuid4())[ 0:8 ] # Use a new so we don't interfere with other active description/editing processes - request.session[f"{session_key_prefix}-edit_sounds"] = ( - sounds # Add the list of sounds to edit in the session object - ) + request.session[f"{session_key_prefix}-edit_sounds"] = [s.id for s in sounds] request.session[f"{session_key_prefix}-len_original_edit_sounds"] = len(sounds) return HttpResponseRedirect( reverse("accounts-edit-sounds") + f"?next={request.path}&session={session_key_prefix}" @@ -930,7 +928,7 @@ def sounds_pending_description_helper(request, file_structure, files): 0:8 ] # Use a new so we don't interfere with other active description/editing processes request.session[f"{session_key_prefix}-describe_sounds"] = [ - files[x] for x in form.cleaned_data["files"] + {"name": files[x].name, "full_path": files[x].full_path} for x in form.cleaned_data["files"] ] request.session[f"{session_key_prefix}-len_original_describe_sounds"] = len( request.session[f"{session_key_prefix}-describe_sounds"] @@ -963,7 +961,7 @@ def describe_license(request): if request.method == "POST": form = LicenseForm(request.POST, hide_old_license_versions=True) if form.is_valid(): - request.session[f"{session_key_prefix}-describe_license"] = form.cleaned_data["license"] + request.session[f"{session_key_prefix}-describe_license"] = form.cleaned_data["license"].id return HttpResponseRedirect(reverse("accounts-describe-pack") + f"?session={session_key_prefix}") else: form = LicenseForm(hide_old_license_versions=True) @@ -985,9 +983,9 @@ def describe_pack(request): data = form.cleaned_data if data["new_pack"]: pack, created = Pack.objects.get_or_create(user=request.user, name=data["new_pack"]) - request.session[f"{session_key_prefix}-describe_pack"] = pack + request.session[f"{session_key_prefix}-describe_pack"] = pack.id elif data["pack"]: - request.session[f"{session_key_prefix}-describe_pack"] = data["pack"] + request.session[f"{session_key_prefix}-describe_pack"] = data["pack"].id else: request.session[f"{session_key_prefix}-describe_pack"] = False return HttpResponseRedirect(reverse("accounts-describe-sounds") + f"?session={session_key_prefix}") diff --git a/sounds/views.py b/sounds/views.py index e251cb492..7e1477892 100644 --- a/sounds/views.py +++ b/sounds/views.py @@ -52,6 +52,7 @@ from sounds.models import ( DeletedSound, Download, + License, Pack, PackDownload, PackDownloadSound, @@ -420,9 +421,7 @@ def sound_edit(request, username, sound_id): session_key_prefix = request.GET.get( "session", str(uuid.uuid4())[0:8] ) # Get existing session key if we are already in an edit session or create a new one - request.session[f"{session_key_prefix}-edit_sounds"] = [ - sound - ] # Add the list of sounds to edit in the session object + request.session[f"{session_key_prefix}-edit_sounds"] = [sound.id] request.session[f"{session_key_prefix}-len_original_edit_sounds"] = 1 return edit_and_describe_sounds_helper(request, session_key_prefix=session_key_prefix) @@ -595,10 +594,17 @@ def update_edited_sound(sound, data): for pack_to_process in packs_to_process: pack_to_process.process() - files = request.session.get( - f"{session_key_prefix}-describe_sounds", None - ) # List of File objects of sounds to describe - sounds = request.session.get(f"{session_key_prefix}-edit_sounds", None) # List of Sound objects to edit + files = request.session.get(f"{session_key_prefix}-describe_sounds", None) + sound_ids = request.session.get(f"{session_key_prefix}-edit_sounds", None) + # Back-compat shim for sessions written before the switch to JSON-safe values + # (pickled File / Sound instances). Remove once SESSION_SERIALIZER is flipped to + # JSONSerializer in the follow-up PR. + if files and not isinstance(files[0], dict): + files = [{"name": f.name, "full_path": f.full_path} for f in files] + if sound_ids and not isinstance(sound_ids[0], int): + sound_ids = [s.id for s in sound_ids] + # Preserve today's `is None` vs `== []` distinction: empty list ≠ missing key. + sounds = list(Sound.objects.ordered_ids(sound_ids)) if sound_ids is not None else None if (describing and files is None) or (not describing and sounds is None): # Expecting either a list of sounds or audio files to describe, got none. Redirect to main manage sounds page. return HttpResponseRedirect(reverse("accounts-manage-sounds", args=["published"])) @@ -617,17 +623,20 @@ def update_edited_sound(sound, data): (len_original_describe_edit_sounds - len(all_remaining_sounds_to_edit_or_describe)) / forms_per_round + 1 ) files_data_for_players = [] # Used when describing sounds (not when editing) to be able to show sound players - preselected_license = request.session.get( - f"{session_key_prefix}-describe_license", False - ) # Pre-selected from the license selection page when describing multiple sounds - preselected_pack = request.session.get( - f"{session_key_prefix}-describe_pack", False - ) # Pre-selected from the pack selection page when describing multiple sounds + preselected_license_id = request.session.get(f"{session_key_prefix}-describe_license", False) + preselected_pack_id = request.session.get(f"{session_key_prefix}-describe_pack", False) + # Back-compat shim (bool is a subclass of int, so the int check covers False/True too). + if preselected_license_id and not isinstance(preselected_license_id, int): + preselected_license_id = preselected_license_id.id + if preselected_pack_id and not isinstance(preselected_pack_id, int): + preselected_pack_id = preselected_pack_id.id + preselected_license = License.objects.filter(id=preselected_license_id).first() if preselected_license_id else False + preselected_pack = Pack.objects.filter(id=preselected_pack_id).first() if preselected_pack_id else False for count, element in enumerate(sounds_to_edit_or_describe): prefix = str(count) if describing: - audio_file_path = element.full_path + audio_file_path = element["full_path"] duration = get_duration_from_processing_before_describe_files(audio_file_path) if duration > 0.0: processing_before_describe_base_url = get_processing_before_describe_sound_base_url(audio_file_path) @@ -650,7 +659,7 @@ def update_edited_sound(sound, data): form = SoundEditAndDescribeForm( request.POST, prefix=prefix, - file_full_path=element.full_path if describing else None, + file_full_path=element["full_path"] if describing else None, explicit_disable=element.is_explicit if not describing else False, hide_old_license_versions="3.0" not in element.license.deed_url if not describing else True, user_packs=Pack.objects.filter(user=request.user if describing else element.user).exclude( @@ -679,7 +688,7 @@ def update_edited_sound(sound, data): form.cleaned_data["sources"] ) # Add sources ids to list so sources sound selector can be initialized if describing: - form.audio_filename = element.name + form.audio_filename = element["name"] else: form.sound_id = element.id else: @@ -699,7 +708,7 @@ def update_edited_sound(sound, data): ) else: sound_sources_ids = [] - initial = dict(name=os.path.splitext(element.name)[0]) + initial = dict(name=os.path.splitext(element["name"])[0]) if preselected_license: initial["license"] = preselected_license if preselected_pack: @@ -715,7 +724,7 @@ def update_edited_sound(sound, data): ) form.sound_sources_ids = sound_sources_ids if describing: - form.audio_filename = element.name + form.audio_filename = element["name"] else: form.sound_id = element.id forms.append(form) @@ -757,8 +766,13 @@ def update_edited_sound(sound, data): else: return HttpResponseRedirect(reverse("accounts-describe-sounds") + f"?session={session_key_prefix}") else: - # Remove sounds successfully described from session data - request.session[f"{session_key_prefix}-edit_sounds"] = sounds[forms_per_round:] + # Remove sounds successfully edited from session data. + # CRITICAL: derive remaining ids from the *refetched* sounds list, not from + # the raw sound_ids. Sound.objects.ordered_ids drops deleted ids, so + # len(sounds) ≤ len(sound_ids); slicing sound_ids[forms_per_round:] would + # re-present sounds already processed in this round whenever any earlier id + # had been deleted — silent duplicate edits. + request.session[f"{session_key_prefix}-edit_sounds"] = [s.id for s in sounds[forms_per_round:]] # If user was only editing one sound and has finished, clear session and redirect to the sound page if len(forms) == 1 and len_original_describe_edit_sounds == 1: