Handle existing Boefje name

boefjes/boefjes/dependencies/plugins.py

@@ -16,8 +16,8 @@
 from boefjes.sql.plugin_storage import create_plugin_storage
 from boefjes.storage.interfaces import (
     ConfigStorage,
-    ExistingPluginId,
-    ExistingPluginName,
+    DuplicatePlugin,
+    IntegrityError,
     NotFound,
     PluginNotFound,
     PluginStorage,
@@ -107,28 +107,40 @@ def upsert_settings(self, settings: dict, organisation_id: str, plugin_id: str):
     def create_boefje(self, boefje: Boefje) -> None:
         try:
             self.local_repo.by_id(boefje.id)
-            raise ExistingPluginId(boefje.id)
+            raise DuplicatePlugin("id")
         except KeyError:
             try:
                 plugin = self.local_repo.by_name(boefje.name)
 
                 if plugin.type == "boefje":
-                    raise ExistingPluginName(boefje.name)
+                    raise DuplicatePlugin("name")
                 else:
-                    self.plugin_storage.create_boefje(boefje)
+                    try:
+                        with self.plugin_storage as storage:
+                            storage.create_boefje(boefje)
+                    except IntegrityError as error:
+                        raise DuplicatePlugin(self._translate_duplicate_plugin(error.message))
             except KeyError:
-                self.plugin_storage.create_boefje(boefje)
+                try:
+                    with self.plugin_storage as storage:
+                        storage.create_boefje(boefje)
+                except IntegrityError as error:
+                    raise DuplicatePlugin(self._translate_duplicate_plugin(error.message))
+
+    def _translate_duplicate_plugin(self, error_message):
+        translations = {"boefje_plugin_id": "id", "boefje_name": "name"}
+        return next((value for key, value in translations.items() if key in error_message), None)
 
     def create_normalizer(self, normalizer: Normalizer) -> None:
         try:
             self.local_repo.by_id(normalizer.id)
-            raise ExistingPluginId(normalizer.id)
+            raise DuplicatePlugin("id")
         except KeyError:
             try:
                 plugin = self.local_repo.by_name(normalizer.name)
 
                 if plugin.types == "normalizer":
-                    raise ExistingPluginName(normalizer.name)
+                    raise DuplicatePlugin("name")
                 else:
                     self.plugin_storage.create_normalizer(normalizer)
             except KeyError:

boefjes/boefjes/katalogus/plugins.py

@@ -1,6 +1,7 @@
 import datetime
 from functools import partial
 
+import structlog
 from croniter import croniter
 from fastapi import APIRouter, Body, Depends, HTTPException, status
 from fastapi.responses import FileResponse, JSONResponse, Response
@@ -17,14 +18,16 @@
 from boefjes.katalogus.organisations import check_organisation_exists
 from boefjes.models import FilterParameters, PaginationParameters, PluginType
 from boefjes.sql.plugin_storage import get_plugin_storage
-from boefjes.storage.interfaces import PluginStorage
+from boefjes.storage.interfaces import DuplicatePlugin, IntegrityError, NotAllowed, PluginStorage
 
 router = APIRouter(
     prefix="/organisations/{organisation_id}",
     tags=["plugins"],
     dependencies=[Depends(check_organisation_exists)],
 )
 
+logger = structlog.get_logger(__name__)
+
 
 # check if query matches plugin id, name or description
 def _plugin_matches_query(plugin: PluginType, query: str) -> bool:
@@ -96,14 +99,17 @@ def get_plugin(
 
 @router.post("/plugins", status_code=status.HTTP_201_CREATED)
 def add_plugin(plugin: PluginType, plugin_service: PluginService = Depends(get_plugin_service)):
-    with plugin_service as service:
-        plugin.static = False  # Creation through the API implies that these cannot be static
+    try:
+        with plugin_service as service:
+            plugin.static = False  # Creation through the API implies that these cannot be static
 
-        if plugin.type == "boefje":
-            return service.create_boefje(plugin)
+            if plugin.type == "boefje":
+                return service.create_boefje(plugin)
 
-        if plugin.type == "normalizer":
-            return service.create_normalizer(plugin)
+            if plugin.type == "normalizer":
+                return service.create_normalizer(plugin)
+    except DuplicatePlugin as error:
+        raise HTTPException(status.HTTP_400_BAD_REQUEST, error.message)
 
     raise HTTPException(status.HTTP_400_BAD_REQUEST, "Creation of Bits is not supported")
 
@@ -164,8 +170,15 @@ def update_boefje(
     boefje: BoefjeIn,
     storage: PluginStorage = Depends(get_plugin_storage),
 ):
-    with storage as p:
-        p.update_boefje(boefje_id, boefje.model_dump(exclude_unset=True))
+    # todo: update boefje should be done in the plugin service
+    try:
+        with storage as p:
+            try:
+                p.update_boefje(boefje_id, boefje.model_dump(exclude_unset=True))
+            except NotAllowed:
+                raise HTTPException(status.HTTP_403_FORBIDDEN, "Updating a static plugin is not allowed")
+    except IntegrityError as error:
+        raise HTTPException(status.HTTP_400_BAD_REQUEST, error.message)
 
 
 @router.delete("/boefjes/{boefje_id}", status_code=status.HTTP_204_NO_CONTENT)

boefjes/boefjes/sql/db.py

@@ -43,7 +43,7 @@ def session_managed_iterator(service_factory: Callable[[Session], Any]) -> Itera
     try:
         yield service
     except Exception as error:
-        logger.exception("An error occurred: %s. Rolling back session", error)
+        logger.error("An error occurred: %s. Rolling back session", error)
         session.rollback()
         raise error
     finally:

boefjes/boefjes/sql/session.py

@@ -30,7 +30,7 @@ def __enter__(self) -> Self:
 
     def __exit__(self, exc_type: type[Exception], exc_value: str, exc_traceback: str) -> None:  # noqa: F841
         if exc_type is not None:
-            logger.error("An error occurred: %s. Rolling back session", exc_value, exc_info=True)
+            logger.error("An error occurred: %s. Rolling back session", exc_value)
             self.session.rollback()
 
             return
@@ -45,5 +45,6 @@ def __exit__(self, exc_type: type[Exception], exc_value: str, exc_traceback: str
         except exc.DatabaseError as e:
             raise StorageError("A storage error occurred") from e
         finally:
-            logger.exception("Committing failed, rolling back")
-            self.session.rollback()
+            if exc_type is not None or self.session.is_active:
+                logger.debug("Committing failed, rolling back")
+                self.session.rollback()

boefjes/boefjes/storage/interfaces.py

@@ -55,14 +55,9 @@ def __init__(self, plugin_id: str):
         super().__init__(f"Plugin with id '{plugin_id}' is static, so updating it is not allowed")
 
 
-class ExistingPluginId(NotAllowed):
-    def __init__(self, plugin_id: str):
-        super().__init__(f"Plugin id '{plugin_id}' is already used")
-
-
-class ExistingPluginName(NotAllowed):
-    def __init__(self, plugin_name: str):
-        super().__init__(f"Plugin name '{plugin_name}' is already used")
+class DuplicatePlugin(NotAllowed):
+    def __init__(self, key: str):
+        super().__init__(f"Duplicate plugin {key}")
 
 
 class OrganisationStorage(ABC):

boefjes/tests/conftest.py

@@ -121,7 +121,6 @@ def handle(self, item: BoefjeMeta | NormalizerMeta):
         time.sleep(self.sleep_time)
 
         if str(item.id) == "9071c9fd-2b9f-440f-a524-ef1ca4824fd4":
-            time.sleep(0.1)
             raise self.exception()
 
         self.queue.put(item)

boefjes/tests/integration/test_api.py

@@ -45,12 +45,12 @@ def test_cannot_add_plugin_reserved_id(test_client, organisation):
     boefje = Boefje(id="dns-records", name="My test boefje", static=False)
     response = test_client.post(f"/v1/organisations/{organisation.id}/plugins", content=boefje.model_dump_json())
     assert response.status_code == 400
-    assert response.json() == {"message": "Plugin id 'dns-records' is already used"}
+    assert response.json() == {"detail": "Duplicate plugin id"}
 
     normalizer = Normalizer(id="kat_nmap_normalize", name="My test normalizer")
     response = test_client.post(f"/v1/organisations/{organisation.id}/plugins", content=normalizer.model_dump_json())
     assert response.status_code == 400
-    assert response.json() == {"message": "Plugin id 'kat_nmap_normalize' is already used"}
+    assert response.json() == {"detail": "Duplicate plugin id"}
 
 
 def test_add_boefje(test_client, organisation):
@@ -80,7 +80,7 @@ def test_cannot_add_static_plugin_with_duplicate_name(test_client, organisation)
     boefje = Boefje(id="test_plugin", name="DNS records", static=False)
     response = test_client.post(f"/v1/organisations/{organisation.id}/plugins", content=boefje.model_dump_json())
     assert response.status_code == 400
-    assert response.json() == {"message": "Plugin name 'DNS records' is already used"}
+    assert response.json() == {"detail": "Duplicate plugin name"}
 
 
 def test_cannot_add_plugin_with_duplicate_name(test_client, organisation):
@@ -91,10 +91,7 @@ def test_cannot_add_plugin_with_duplicate_name(test_client, organisation):
     boefje = Boefje(id="test_plugin_2", name="My test boefje", static=False)
     response = test_client.post(f"/v1/organisations/{organisation.id}/plugins", content=boefje.model_dump_json())
     assert response.status_code == 400
-    assert response.json() == {
-        "message": 'duplicate key value violates unique constraint "unique_boefje_name"\n'
-        "DETAIL:  Key (name)=(My test boefje) already exists.\n"
-    }
+    assert response.json() == {"detail": "Duplicate plugin name"}
 
     normalizer = Normalizer(id="test_normalizer", name="My test normalizer", static=False)
     response = test_client.post(f"/v1/organisations/{organisation.id}/plugins", content=normalizer.model_dump_json())
@@ -238,7 +235,7 @@ def test_cannot_update_static_plugins(test_client, organisation):
     assert response.json()["enabled"] is True
 
     response = test_client.patch(f"/v1/organisations/{organisation.id}/boefjes/dns-records", json={"version": "v1.2"})
-    assert response.status_code == 400
+    assert response.status_code == 403
 
     response = test_client.get(f"/v1/organisations/{organisation.id}/plugins/dns-records")
     assert response.json()["version"] != "v1.2"

boefjes/tests/test_app.py

@@ -95,6 +95,7 @@ def test_two_processes_handler_exception(manager: SchedulerWorkerManager, item_h
 
     # We expect the first two patches to set the task status to running of both task and then process 1 to finish, as
     # the exception has been set up with a small delay.
+    assert len(patched_tasks) == 6
     assert sorted(patched_tasks[:3]) == sorted(
         [
             ("70da7d4f-f41f-4940-901b-d98a92e9014b", "running"),  # Process 1

rocky/katalogus/client.py

@@ -1,3 +1,4 @@
+import json
 from io import BytesIO
 
 import httpx
@@ -68,19 +69,47 @@ def serialize_produces(self, produces: set[type[OOI]]):
 
 
 class KATalogusError(Exception):
-    message: str = _("The KATalogus has an unexpected error. Check the logs for further details.")
+    @property
+    def message(self):
+        return self._message
+
+    def __init__(self, message: str | None = None):
+        if message is None:
+            message = _("The KATalogus has an unexpected error. Check the logs for further details.")
+
+        self._message = message
+
+        super().__init__(message)
 
     def __str__(self):
-        return str(self.message)
+        return self._message
+
+
+class DuplicatePluginError(KATalogusError):
+    def __init__(self, error_message: str):
+        super().__init__(error_message)
+
+
+class DuplicateNameError(KATalogusError):
+    def __init__(self):
+        super().__init__(_("Boefje with this name already exists."))
+
+
+class DuplicateIdError(KATalogusError):
+    def __init__(self):
+        super().__init__(_("Boefje with this ID already exists."))
+
+
+class KATalogusNotAllowedError(KATalogusError):
+    def __init__(self):
+        super().__init__(_("Editing this boefje is not allowed because it is static."))
 
 
 class KATalogusHTTPStatusError(KATalogusError):
-    def __init__(self, *args: object, status_code: str | None = None) -> None:
-        super().__init__(*args)
-        status_message = ""
-        if status_code is not None:
-            status_message = f"{status_code}: "
-        self.message = status_message + _("A HTTP error occurred. Check logs for more info.")
+    def __init__(self, error: httpx.HTTPStatusError):
+        self.error = error
+
+        super().__init__(_("An HTTP %d error occurred. Check logs for more info.").format(error.response.status_code))
 
 
 class KATalogusClientV1:
@@ -111,7 +140,7 @@ def get_plugins(self, **params) -> list[Plugin]:
             response = self.session.get(f"{self.organization_uri}/plugins", params=params)
             response.raise_for_status()
         except httpx.HTTPStatusError as error:
-            raise KATalogusHTTPStatusError(status_code=str(error.response.status_code))
+            raise KATalogusHTTPStatusError(error)
         return [parse_plugin(plugin) for plugin in response.json()]
 
     def get_plugin(self, plugin_id: str) -> Plugin:
@@ -210,29 +239,42 @@ def get_cover(self, boefje_id: str) -> BytesIO:
         return BytesIO(response.content)
 
     def create_plugin(self, plugin: Plugin) -> None:
-        response = self.session.post(
-            f"{self.organization_uri}/plugins",
-            headers={"Content-Type": "application/json"},
-            content=plugin.model_dump_json(exclude_none=True),
-        )
-        response.raise_for_status()
-
-        if response.status_code == codes.CREATED:
-            logger.info("Plugin %s", plugin.name)
-        else:
-            logger.info("Plugin %s could not be created", plugin.name)
+        try:
+            response = self.session.post(
+                f"{self.organization_uri}/plugins",
+                headers={"Content-Type": "application/json"},
+                content=plugin.model_dump_json(exclude_none=True),
+            )
+            response.raise_for_status()
+        except httpx.HTTPStatusError as error:
+            if error.response.status_code == codes.CREATED:
+                logger.info("Plugin %s created", plugin.name)
+            else:
+                logger.info("Plugin %s could not be created", plugin.name)
+            error_message = json.loads(error.response.text).get("detail")
+            if error.response.status_code == codes.BAD_REQUEST and "Duplicate plugin" in error_message:
+                raise DuplicatePluginError(error_message)
+            else:
+                raise error
 
     def edit_plugin(self, plugin: Plugin) -> None:
-        response = self.session.patch(
-            f"{self.organization_uri}/boefjes/{plugin.id}",
-            content=plugin.model_dump_json(exclude_none=True),
-        )
-        response.raise_for_status()
-
-        if response.status_code == codes.CREATED:
-            logger.info("Plugin %s", plugin.name)
-        else:
-            logger.info("Plugin %s could not be created", plugin.name)
+        try:
+            response = self.session.patch(
+                f"{self.organization_uri}/boefjes/{plugin.id}",
+                content=plugin.model_dump_json(exclude_none=True),
+            )
+            response.raise_for_status()
+        except httpx.HTTPStatusError as error:
+            if error.response.status_code == codes.CREATED:
+                logger.info("Plugin %s updated", plugin.name)
+            else:
+                logger.info("Plugin %s could not be updated", plugin.name)
+            if error.response.status_code == codes.BAD_REQUEST and "duplicate key" in error.response.text:
+                raise DuplicatePluginError("Duplicate plugin name")
+            if error.response.status_code in [codes.FORBIDDEN, codes.NOT_FOUND]:
+                raise KATalogusNotAllowedError
+            else:
+                raise error
 
 
 def parse_boefje(boefje: dict) -> Boefje: