From 366e148ccff7947948123f47ca8317c76b7ff5c1 Mon Sep 17 00:00:00 2001
From: ash <ah37@sanger.ac.uk>
Date: Wed, 31 Jan 2024 09:45:57 +0000
Subject: [PATCH] Disallow troublesome characters in environment names (#40)

* Mark failed environments as failed
---
 softpack_core/artifacts.py            |  4 +++
 softpack_core/schemas/environment.py  | 22 ++++++++++----
 tests/integration/test_environment.py | 41 +++++++++++++++++++++++++--
 3 files changed, 59 insertions(+), 8 deletions(-)

diff --git a/softpack_core/artifacts.py b/softpack_core/artifacts.py
index 3cf4426..cfd9029 100644
--- a/softpack_core/artifacts.py
+++ b/softpack_core/artifacts.py
@@ -53,6 +53,7 @@ class State(Enum):
 
     ready = 'ready'
     queued = 'queued'
+    failed = 'failed'
 
 
 class Artifacts:
@@ -60,6 +61,7 @@ class Artifacts:
 
     environments_root = "environments"
     environments_file = "softpack.yml"
+    builder_out = "builder.out"
     module_file = "module"
     readme_file = "README.md"
     built_by_softpack_file = ".built_by_softpack"
@@ -131,6 +133,8 @@ def spec(self) -> Box:
 
             if Artifacts.module_file in self.obj:
                 info["state"] = State.ready
+            elif Artifacts.builder_out in self.obj:
+                info["state"] = State.failed
             else:
                 info["state"] = State.queued
 
diff --git a/softpack_core/schemas/environment.py b/softpack_core/schemas/environment.py
index d7f76b7..9bf55bc 100644
--- a/softpack_core/schemas/environment.py
+++ b/softpack_core/schemas/environment.py
@@ -5,6 +5,7 @@
 """
 
 import io
+import re
 from dataclasses import dataclass
 from pathlib import Path
 from traceback import format_exception_only
@@ -146,15 +147,24 @@ class EnvironmentInput:
     description: str
     packages: list[PackageInput]
 
-    def validate(cls) -> Union[None, InvalidInputError]:
-        """Validate that all values have been supplied.
+    def validate(self) -> Union[None, InvalidInputError]:
+        """Validate all values.
+
+        Checks all values have been supplied.
+        Checks that name consists only of alphanumerics, dash, and underscore.
 
         Returns:
             None if good, or InvalidInputError if not all values supplied.
         """
-        if any(len(value) == 0 for value in vars(cls).values()):
+        if any(len(value) == 0 for value in vars(self).values()):
             return InvalidInputError(message="all fields must be filled in")
 
+        if not re.fullmatch(r"^[a-zA-Z0-9_-]+$", self.name):
+            return InvalidInputError(
+                message="name must only contain alphanumerics, "
+                "dash, and underscore"
+            )
+
         return None
 
     @classmethod
@@ -239,9 +249,9 @@ def create(cls, env: EnvironmentInput) -> CreateResponse:  # type: ignore
         Returns:
             A message confirming the success or failure of the operation.
         """
-        result = env.validate()
-        if result is not None:
-            return result
+        input_err = env.validate()
+        if input_err is not None:
+            return input_err
 
         name = env.name
         version = 1
diff --git a/tests/integration/test_environment.py b/tests/integration/test_environment.py
index 15a0f23..78b1f57 100644
--- a/tests/integration/test_environment.py
+++ b/tests/integration/test_environment.py
@@ -93,12 +93,34 @@ def test_create(httpx_post, testable_env_input: EnvironmentInput) -> None:
     )
     assert file_in_remote(path)
 
-    orig_name = testable_env_input.name
+
+def test_create_name_empty_disallowed(httpx_post, testable_env_input):
     testable_env_input.name = ""
     result = Environment.create(testable_env_input)
     assert isinstance(result, InvalidInputError)
 
-    testable_env_input.name = orig_name
+
+def test_create_name_spaces_disallowed(httpx_post, testable_env_input):
+    testable_env_input.name = "names cannot have spaces"
+    result = Environment.create(testable_env_input)
+    assert isinstance(result, InvalidInputError)
+
+
+def test_create_name_slashes_disallowed(httpx_post, testable_env_input):
+    testable_env_input.name = "names/cannot/have/slashes"
+    result = Environment.create(testable_env_input)
+    assert isinstance(result, InvalidInputError)
+
+
+def test_create_name_dashes_and_number_first_allowed(
+    httpx_post, testable_env_input
+):
+    testable_env_input.name = "7-zip_piz-7"
+    result = Environment.create(testable_env_input)
+    assert isinstance(result, CreateEnvironmentSuccess)
+
+
+def test_create_path_invalid_disallowed(httpx_post, testable_env_input):
     testable_env_input.path = "invalid/path"
     result = Environment.create(testable_env_input)
     assert isinstance(result, InvalidInputError)
@@ -225,6 +247,21 @@ async def test_states(httpx_post, testable_env_input):
     assert env.type == Artifacts.built_by_softpack
     assert env.state == State.queued
 
+    upload = UploadFile(
+        filename=Artifacts.builder_out, file=io.BytesIO(b"some output")
+    )
+
+    result = await Environment.write_artifact(
+        file=upload,
+        folder_path=f"{testable_env_input.path}/{testable_env_input.name}-1",
+        file_name=upload.filename,
+    )
+    assert isinstance(result, WriteArtifactSuccess)
+
+    env = get_env_from_iter(testable_env_input.name + "-1")
+    assert env is not None
+    assert env.state == State.failed
+
     upload = UploadFile(
         filename=Artifacts.module_file, file=io.BytesIO(b"#%Module")
     )