Fix artifact-prefixed uploads with empty media root

CHANGES/8041.bugfix

@@ -0,0 +1 @@
+Fixed uploads of files whose names start with `artifact` when the media root is empty.

pulpcore/app/models/fields.py

@@ -44,15 +44,17 @@ def pre_save(self, model_instance, add):
 
         There are two ways to get artifact into Pulp: sync and upload.
 
-        The upload case
-         - file is not stored yet, aka file._committed = False
-         - nothing to do here in addition to Django pre_save actions
+        The upload case:
+         - assigning a File object sets _committed=False, meaning this FieldFile has not saved it
+         - the File object may still refer to a file already in artifact storage, so check any
+           absolute source path before saving
+         - Django pre_save writes the File object to upload_to without extra handling here
 
         The sync case:
-         - file is already stored in a temporary location, aka file._committed = True
-         - it needs to be moved into Pulp artifact storage if it's not there
-         - TemporaryDownloadedFile takes care of correctly set storage path
-         - only then Django pre_save actions should be performed
+         - assigning a path sets _committed=True, even when the file is in a temporary location
+         - a file already at upload_to stays in place; other files in artifact storage are rejected
+         - TemporaryDownloadedFile wraps an outside source so storage can move or copy it
+         - when needed, mark the file uncommitted so Django pre_save stores it at upload_to
 
         Args:
             model_instance (`class::pulpcore.plugin.Artifact`): The instance this field belongs to.
@@ -64,13 +66,30 @@ def pre_save(self, model_instance, add):
 
         """
         file = model_instance.file
+        # A path assignment creates a committed FieldFile whose name is the source path.
+        # A File assignment is uncommitted, so inspect the wrapped File's source name.
+        source_path = file.name if file._committed else file.file.name
+        if not file._committed and (not source_path or not os.path.isabs(source_path)):
+            # A relative File name does not identify a location in artifact storage.
+            return super().pre_save(model_instance, add)
+
         artifact_storage_path = self.upload_to(model_instance, "")
 
-        already_in_place = file.name in [
-            artifact_storage_path,
-            os.path.join(settings.MEDIA_ROOT, artifact_storage_path),
-        ]
-        is_in_artifact_storage = file.name.startswith(os.path.join(settings.MEDIA_ROOT, "artifact"))
+        stored_file_path = None
+        stored_artifact_dir = None
+        try:
+            stored_file_path = file.storage.path(artifact_storage_path)
+            stored_artifact_dir = file.storage.path("artifact")
+        except NotImplementedError:
+            # Object storage backends do not have absolute filesystem paths.
+            pass
+
+        already_in_place = source_path in (artifact_storage_path, stored_file_path)
+        # Match the artifact directory, not other names beginning with "artifact".
+        artifact_storage_prefixes = [os.path.join("artifact", "")]
+        if stored_artifact_dir is not None:
+            artifact_storage_prefixes.append(os.path.join(stored_artifact_dir, ""))
+        is_in_artifact_storage = source_path.startswith(tuple(artifact_storage_prefixes))
 
         if not already_in_place and is_in_artifact_storage:
             raise ValueError(
@@ -80,10 +99,13 @@ def pre_save(self, model_instance, add):
                     "prior to Artifact creation."
                 )
             )
-        move = file._committed and file.name != artifact_storage_path
-        if move:
+        if not file._committed:
+            return super().pre_save(model_instance, add)
+
+        if file.name != artifact_storage_path:
             if not already_in_place:
                 file._file = TemporaryDownloadedFile(open(file.name, "rb"))
+            # Let Django save the source and record its storage-relative name.
             file._committed = False
 
         return super().pre_save(model_instance, add)

pulpcore/tests/functional/api/test_crd_artifacts.py

@@ -55,6 +55,17 @@ def test_upload_valid_attrs(pulpcore_bindings, pulpcore_random_file, monitor_tas
             _do_upload_valid_attrs(pulpcore_bindings, pulpcore_random_file["name"], data)
 
 
+@pytest.mark.parallel
+def test_upload_artifact_prefixed_filename(pulpcore_bindings, tmp_path):
+    file = tmp_path / f"artifact-{uuid.uuid4()}.txt"
+    contents = os.urandom(1024)
+    file.write_bytes(contents)
+
+    artifact = pulpcore_bindings.ArtifactsApi.create(str(file))
+
+    assert artifact.sha256 == hashlib.sha256(contents).hexdigest()
+
+
 def test_upload_empty_file(pulpcore_bindings, tmp_path, monitor_task):
     """Upload an empty file.
 

pulpcore/tests/unit/models/test_fields.py

@@ -0,0 +1,47 @@
+import hashlib
+
+import pytest
+from django.core.files import File
+from django.core.files.uploadedfile import SimpleUploadedFile
+
+from pulpcore.app.models import Artifact, Domain
+from pulpcore.app.models.storage import FileSystem
+
+
+@pytest.fixture
+def artifact_storage_root(monkeypatch, tmp_path):
+    storage_root = tmp_path / "media"
+    storage = FileSystem(location=str(storage_root))
+    monkeypatch.setattr(Domain, "get_storage", lambda self: storage)
+    return storage_root
+
+
+@pytest.mark.django_db
+def test_artifact_upload_with_empty_media_root(settings, artifact_storage_root):
+    settings.MEDIA_ROOT = ""
+
+    contents = b"artifact upload"
+    artifact = Artifact.objects.create(
+        file=SimpleUploadedFile("artifact-upload.txt", contents),
+        size=len(contents),
+        **{digest: hashlib.new(digest, contents).hexdigest() for digest in Artifact.DIGEST_FIELDS},
+    )
+
+    artifact.refresh_from_db()
+    assert artifact.file.read() == contents
+
+
+@pytest.mark.django_db
+def test_artifact_file_object_from_artifact_storage_is_rejected(artifact_storage_root):
+    contents = b"artifact already in storage"
+    source_path = artifact_storage_root / "artifact" / "other"
+    source_path.parent.mkdir(parents=True)
+    source_path.write_bytes(contents)
+    digests = {
+        digest: hashlib.new(digest, contents).hexdigest() for digest in Artifact.DIGEST_FIELDS
+    }
+
+    with source_path.open("rb") as source:
+        artifact = Artifact(file=File(source), size=len(contents), **digests)
+        with pytest.raises(ValueError, match="already present in Artifact storage"):
+            artifact.save()