diff --git a/boefjes/boefjes/dependencies/plugins.py b/boefjes/boefjes/dependencies/plugins.py index c7b6b61ffea..0cfb57d855b 100644 --- a/boefjes/boefjes/dependencies/plugins.py +++ b/boefjes/boefjes/dependencies/plugins.py @@ -17,6 +17,7 @@ from boefjes.storage.interfaces import ( ConfigStorage, ExistingPluginId, + ExistingPluginName, NotFound, PluginNotFound, PluginStorage, @@ -108,14 +109,30 @@ def create_boefje(self, boefje: Boefje) -> None: self.local_repo.by_id(boefje.id) raise ExistingPluginId(boefje.id) except KeyError: - self.plugin_storage.create_boefje(boefje) + try: + plugin = self.local_repo.by_name(boefje.name) + + if plugin.type == "boefje": + raise ExistingPluginName(boefje.name) + else: + self.plugin_storage.create_boefje(boefje) + except KeyError: + self.plugin_storage.create_boefje(boefje) def create_normalizer(self, normalizer: Normalizer) -> None: try: self.local_repo.by_id(normalizer.id) raise ExistingPluginId(normalizer.id) except KeyError: - self.plugin_storage.create_normalizer(normalizer) + try: + plugin = self.local_repo.by_name(normalizer.name) + + if plugin.types == "normalizer": + raise ExistingPluginName(normalizer.name) + else: + self.plugin_storage.create_normalizer(normalizer) + except KeyError: + self.plugin_storage.create_normalizer(normalizer) def _put_boefje(self, boefje_id: str) -> None: """Check existence of a boefje, and insert a database entry if it concerns a local boefje""" diff --git a/boefjes/boefjes/katalogus/root.py b/boefjes/boefjes/katalogus/root.py index 8aa0c1683c5..51859d759e7 100644 --- a/boefjes/boefjes/katalogus/root.py +++ b/boefjes/boefjes/katalogus/root.py @@ -20,7 +20,7 @@ from boefjes.katalogus import organisations, plugins from boefjes.katalogus import settings as settings_router from boefjes.katalogus.version import __version__ -from boefjes.storage.interfaces import NotAllowed, NotFound, StorageError +from boefjes.storage.interfaces import IntegrityError, NotAllowed, NotFound, StorageError with settings.log_cfg.open() as f: logging.config.dictConfig(json.load(f)) @@ -89,6 +89,14 @@ def not_allowed_handler(request: Request, exc: NotAllowed): ) +@app.exception_handler(IntegrityError) +def integrity_error_handler(request: Request, exc: IntegrityError): + return JSONResponse( + status_code=status.HTTP_400_BAD_REQUEST, + content={"message": exc.message}, + ) + + @app.exception_handler(StorageError) def storage_error_handler(request: Request, exc: StorageError): return JSONResponse( diff --git a/boefjes/boefjes/local_repository.py b/boefjes/boefjes/local_repository.py index a29fb92ba35..719b26f06a8 100644 --- a/boefjes/boefjes/local_repository.py +++ b/boefjes/boefjes/local_repository.py @@ -47,6 +47,19 @@ def by_id(self, plugin_id: str) -> PluginType: raise KeyError(f"Can't find plugin {plugin_id}") + def by_name(self, plugin_name: str) -> PluginType: + boefjes = {resource.boefje.name: resource for resource in self.resolve_boefjes().values()} + + if plugin_name in boefjes: + return boefjes[plugin_name].boefje + + normalizers = {resource.normalizer.name: resource for resource in self.resolve_normalizers().values()} + + if plugin_name in normalizers: + return normalizers[plugin_name].normalizer + + raise KeyError(f"Can't find plugin {plugin_name}") + def schema(self, id_: str) -> dict | None: boefjes = self.resolve_boefjes() diff --git a/boefjes/boefjes/migrations/versions/a2c8d54b0124_unique_plugin_names.py b/boefjes/boefjes/migrations/versions/a2c8d54b0124_unique_plugin_names.py new file mode 100644 index 00000000000..84759f49cff --- /dev/null +++ b/boefjes/boefjes/migrations/versions/a2c8d54b0124_unique_plugin_names.py @@ -0,0 +1,29 @@ +"""Unique plugin names + +Revision ID: a2c8d54b0124 +Revises: 870fc302b852 +Create Date: 2024-09-18 14:46:00.881022 + +""" + +from alembic import op + +# revision identifiers, used by Alembic. +revision = "a2c8d54b0124" +down_revision = "870fc302b852" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + # ### commands auto generated by Alembic - please adjust! ### + op.create_unique_constraint("unique_boefje_name", "boefje", ["name"]) + op.create_unique_constraint("unique_normalizer_name", "normalizer", ["name"]) + # ### end Alembic commands ### + + +def downgrade() -> None: + # ### commands auto generated by Alembic - please adjust! ### + op.drop_constraint("unique_normalizer_name", "normalizer", type_="unique") + op.drop_constraint("unique_boefje_name", "boefje", type_="unique") + # ### end Alembic commands ### diff --git a/boefjes/boefjes/sql/db_models.py b/boefjes/boefjes/sql/db_models.py index f9b32a9a9ea..3227822511b 100644 --- a/boefjes/boefjes/sql/db_models.py +++ b/boefjes/boefjes/sql/db_models.py @@ -68,7 +68,7 @@ class BoefjeInDB(SQL_BASE): static = Column(Boolean, nullable=False, server_default="false") # Metadata - name = Column(String(length=64), nullable=False) + name = Column(String(length=64), nullable=False, unique=True) description = Column(types.Text, nullable=True) scan_level = Column(types.Enum(*[str(x.value) for x in ScanLevel], name="scan_level"), nullable=False, default="4") @@ -92,7 +92,7 @@ class NormalizerInDB(SQL_BASE): static = Column(Boolean, nullable=False, server_default="false") # Metadata - name = Column(String(length=64), nullable=False) + name = Column(String(length=64), nullable=False, unique=True) description = Column(types.Text, nullable=True) # Job specifications diff --git a/boefjes/boefjes/sql/session.py b/boefjes/boefjes/sql/session.py index 708b72f4ad8..e1725c8435c 100644 --- a/boefjes/boefjes/sql/session.py +++ b/boefjes/boefjes/sql/session.py @@ -1,9 +1,10 @@ import structlog -from sqlalchemy.exc import DatabaseError +from psycopg2 import errors +from sqlalchemy import exc from sqlalchemy.orm import Session from typing_extensions import Self -from boefjes.storage.interfaces import StorageError +from boefjes.storage.interfaces import IntegrityError, StorageError logger = structlog.get_logger(__name__) @@ -34,15 +35,15 @@ def __exit__(self, exc_type: type[Exception], exc_value: str, exc_traceback: str return - error = None - try: logger.debug("Committing session") self.session.commit() - except DatabaseError as e: - error = e + except exc.IntegrityError as e: + if isinstance(e.orig, errors.UniqueViolation): + raise IntegrityError(str(e.orig)) + raise IntegrityError("An integrity error occurred") from e + except exc.DatabaseError as e: + raise StorageError("A storage error occurred") from e + finally: logger.exception("Committing failed, rolling back") self.session.rollback() - - if error: - raise StorageError("A storage error occurred") from error diff --git a/boefjes/boefjes/storage/interfaces.py b/boefjes/boefjes/storage/interfaces.py index 3f3d58e4a63..08ef95c1392 100644 --- a/boefjes/boefjes/storage/interfaces.py +++ b/boefjes/boefjes/storage/interfaces.py @@ -10,6 +10,13 @@ def __init__(self, message: str): self.message = message +class IntegrityError(StorageError): + """Integrity error during persistence of an entity""" + + def __init__(self, message: str): + self.message = message + + class SettingsNotConformingToSchema(StorageError): def __init__(self, plugin_id: str, validation_error: str): super().__init__(f"Settings for plugin {plugin_id} are not conform the plugin schema: {validation_error}") @@ -53,6 +60,11 @@ 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 OrganisationStorage(ABC): def __enter__(self): return self diff --git a/boefjes/tests/conftest.py b/boefjes/tests/conftest.py index 1c8fde0c10b..7fa5d11b4a4 100644 --- a/boefjes/tests/conftest.py +++ b/boefjes/tests/conftest.py @@ -118,10 +118,12 @@ def __init__(self, exception=Exception): self.exception = exception 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() - time.sleep(self.sleep_time) self.queue.put(item) def get_all(self) -> list[BoefjeMeta | NormalizerMeta]: diff --git a/boefjes/tests/integration/test_api.py b/boefjes/tests/integration/test_api.py index 9cb36dae370..ef2dab4ec98 100644 --- a/boefjes/tests/integration/test_api.py +++ b/boefjes/tests/integration/test_api.py @@ -72,6 +72,39 @@ def test_add_boefje(test_client, organisation): assert response.json() == boefje_dict +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 + + 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"} + + +def test_cannot_add_plugin_with_duplicate_name(test_client, organisation): + boefje = Boefje(id="test_plugin", 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 == 201 + + 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" + } + + 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()) + assert response.status_code == 201 + + normalizer = Normalizer(id="test_normalizer_2", name="My test normalizer", static=False) + response = test_client.post(f"/v1/organisations/{organisation.id}/plugins", content=normalizer.model_dump_json()) + assert response.status_code == 400 + + def test_delete_boefje(test_client, organisation): boefje = Boefje(id="test_plugin", name="My test boefje", static=False) response = test_client.post(f"/v1/organisations/{organisation.id}/plugins", content=boefje.model_dump_json()) diff --git a/boefjes/tests/test_app.py b/boefjes/tests/test_app.py index 70e1523979f..12b8d470619 100644 --- a/boefjes/tests/test_app.py +++ b/boefjes/tests/test_app.py @@ -88,11 +88,29 @@ def test_two_processes_handler_exception(manager: SchedulerWorkerManager, item_h patched_tasks = manager.scheduler_client.get_all_patched_tasks() - assert len(patched_tasks) == 6 - # Handler starts raising an Exception from the second call onward, - # so we have 2 completed tasks and 4 failed tasks. - assert patched_tasks.count(("70da7d4f-f41f-4940-901b-d98a92e9014b", "completed")) == 1 - assert patched_tasks.count(("9071c9fd-2b9f-440f-a524-ef1ca4824fd4", "failed")) == 2 + # Handler starts raising an Exception from the second call onward. So each process picks up a task, of which the one + # with id 9071c9fd-2b9f-440f-a524-ef1ca4824fd4 crashes. Task 70da7d4f-f41f-4940-901b-d98a92e9014b will be picked up + # by the other process in parallel, and completes before the crash of the other task. Since one process completes, + # it pops the same crashing task 9071c9fd-2b9f-440f-a524-ef1ca4824fd4 from the queue to simplify the test. + + # 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 sorted(patched_tasks[:3]) == sorted( + [ + ("70da7d4f-f41f-4940-901b-d98a92e9014b", "running"), # Process 1 + ("70da7d4f-f41f-4940-901b-d98a92e9014b", "completed"), # Process 1 + ("9071c9fd-2b9f-440f-a524-ef1ca4824fd4", "running"), # Process 2 + ] + ) + + # The process completing status then to be set to completed/failed for both tasks. + assert sorted(patched_tasks[3:]) == sorted( + [ + ("9071c9fd-2b9f-440f-a524-ef1ca4824fd4", "running"), # Process 1 + ("9071c9fd-2b9f-440f-a524-ef1ca4824fd4", "failed"), # Process 2 + ("9071c9fd-2b9f-440f-a524-ef1ca4824fd4", "failed"), # Process 1 + ] + ) def test_two_processes_cleanup_unfinished_tasks(