diff --git a/framework/cli/simple_module_cli/case.py b/framework/cli/simple_module_cli/case.py index fba081a6..992c888b 100644 --- a/framework/cli/simple_module_cli/case.py +++ b/framework/cli/simple_module_cli/case.py @@ -41,6 +41,11 @@ def to_snake_case(name: str) -> str: s = re.sub(r"[\s\-]+", "_", name) s = re.sub(r"(.)([A-Z][a-z]+)", r"\1_\2", s) s = re.sub(r"([a-z0-9])([A-Z])", r"\1_\2", s) + # Collapse runs of underscores that the boundary regexes can introduce + # when the input already contained a separator (e.g. ``My Feature`` → + # ``My_Feature`` → ``My__Feature``). Without this the PyPI slug emits a + # double hyphen. + s = re.sub(r"_+", "_", s) return s.lower() diff --git a/framework/cli/simple_module_cli/scaffolding.py b/framework/cli/simple_module_cli/scaffolding.py index dbea7b57..1be7adb7 100644 --- a/framework/cli/simple_module_cli/scaffolding.py +++ b/framework/cli/simple_module_cli/scaffolding.py @@ -193,20 +193,30 @@ def create_module( template_root: Path | None = None, ) -> Path: dest = Path(dest) + existed_before = dest.exists() _require_empty_dest(dest) display_name = to_pascal_case(name) slug = to_kebab_case(name) package_name = to_snake_case(name) - _apply_template_files( - _resolve_template_root("module", template_root), - dest, - substitutions={ - "{{MODULE_NAME}}": display_name, - "{{MODULE_SLUG}}": slug, - "{{PACKAGE_NAME}}": package_name, - "{{PACKAGE_NAME_UPPER}}": package_name.upper(), - }, - path_rewrites={_PACKAGE_PATH_TOKEN: package_name}, - ) + try: + _apply_template_files( + _resolve_template_root("module", template_root), + dest, + substitutions={ + "{{MODULE_NAME}}": display_name, + "{{MODULE_SLUG}}": slug, + "{{PACKAGE_NAME}}": package_name, + "{{PACKAGE_NAME_UPPER}}": package_name.upper(), + }, + path_rewrites={_PACKAGE_PATH_TOKEN: package_name}, + ) + except Exception: + # Rollback so a half-scaffolded directory doesn't leave the user + # with an unparseable Python package and the impression that a + # retry won't work because ``dest`` is now non-empty. We only + # nuke the directory we created — never one we found pre-existing. + if not existed_before and dest.is_dir(): + shutil.rmtree(dest, ignore_errors=True) + raise logger.info("Scaffolded module '%s' at %s (package: %s)", display_name, dest, package_name) return dest diff --git a/framework/cli/tests/test_case.py b/framework/cli/tests/test_case.py new file mode 100644 index 00000000..4aca4497 --- /dev/null +++ b/framework/cli/tests/test_case.py @@ -0,0 +1,63 @@ +"""Direct unit tests for the identifier case helpers. + +Every scaffolder pipes a user-supplied module name through these — a typo +that emits ``u_r_l_path`` from ``URLPath`` would propagate into the PyPI +slug *and* the display name. Existing tests touch ``to_pascal_case`` only +indirectly via ``test_helpers.py``; we pin the snake/kebab forms too, +including the acronym edge cases the docstring promises. +""" + +from __future__ import annotations + +import pytest +from simple_module_cli.case import to_kebab_case, to_pascal_case, to_snake_case + + +class TestToSnakeCase: + @pytest.mark.parametrize( + ("raw", "expected"), + [ + ("MyFeature", "my_feature"), + ("my-feature", "my_feature"), + ("my_feature", "my_feature"), + ("My Feature", "my_feature"), + ("MY_FEATURE", "my_feature"), + ("URLPath", "url_path"), + ("APIClient", "api_client"), + ("HTTPServer2", "http_server2"), + ("simple", "simple"), + ("simple-thing-name", "simple_thing_name"), + ("Already_Snake_Mixed", "already_snake_mixed"), + ("trailing-", "trailing_"), + ], + ) + def test_canonicalises(self, raw, expected): + assert to_snake_case(raw) == expected + + +class TestToKebabCase: + @pytest.mark.parametrize( + ("raw", "expected"), + [ + ("MyFeature", "my-feature"), + ("my_feature", "my-feature"), + ("URLPath", "url-path"), + ], + ) + def test_canonicalises(self, raw, expected): + assert to_kebab_case(raw) == expected + + +class TestToPascalCase: + @pytest.mark.parametrize( + ("raw", "expected"), + [ + ("my-feature", "MyFeature"), + ("my_feature", "MyFeature"), + ("MyFeature", "MyFeature"), + ("URLPath", "UrlPath"), # consequence of the snake-cased pipeline + ("__name__", "Name"), # empty parts dropped + ], + ) + def test_canonicalises(self, raw, expected): + assert to_pascal_case(raw) == expected diff --git a/framework/cli/tests/test_env_helper.py b/framework/cli/tests/test_env_helper.py new file mode 100644 index 00000000..cf4618f3 --- /dev/null +++ b/framework/cli/tests/test_env_helper.py @@ -0,0 +1,54 @@ +"""``set_env_key`` is the single helper that edits scaffold-time .env files. + +A regression here writes a duplicate ``KEY=`` line or, worse, leaves the old +value unstripped — both manifest as "my recipe didn't take effect" which is +hard to debug downstream. +""" + +from __future__ import annotations + +from simple_module_cli._env import set_env_key + + +def test_appends_to_empty_body(): + assert set_env_key("", "FOO", "bar") == "FOO=bar\n" + + +def test_replaces_existing_key(): + body = "FOO=old\nBAR=keep\n" + out = set_env_key(body, "FOO", "new") + # Replaced line lives at the bottom (append-after-strip strategy). + assert "FOO=old" not in out + assert "FOO=new\n" in out + assert "BAR=keep" in out + + +def test_unrelated_lines_preserved_in_order(): + body = "A=1\nB=2\nC=3\n" + out = set_env_key(body, "Z", "9") + lines = out.splitlines() + assert lines[0] == "A=1" + assert lines[1] == "B=2" + assert lines[2] == "C=3" + assert lines[-1] == "Z=9" + + +def test_idempotent_when_key_already_at_value(): + body = "FOO=bar\n" + once = set_env_key(body, "FOO", "bar") + twice = set_env_key(once, "FOO", "bar") + assert once == twice == "FOO=bar\n" + + +def test_prefix_match_is_exact(): + """``KEY=`` must not match ``KEY_LONGER=``.""" + body = "FOO_BAR=keep_me\n" + out = set_env_key(body, "FOO", "new") + assert "FOO_BAR=keep_me" in out + assert "FOO=new" in out + + +def test_output_always_ends_with_newline(): + body = "X=1" # no trailing newline + out = set_env_key(body, "Y", "2") + assert out.endswith("\n") diff --git a/framework/cli/tests/test_scaffold_rollback.py b/framework/cli/tests/test_scaffold_rollback.py new file mode 100644 index 00000000..0802091a --- /dev/null +++ b/framework/cli/tests/test_scaffold_rollback.py @@ -0,0 +1,73 @@ +"""Scaffold rollback on partial failure. + +Before the rollback added to ``create_module``, a mid-pipeline error left +the user with a non-empty destination directory — the next ``smpy new`` +invocation against the same path would then refuse to overwrite, but the +files already written wouldn't form a valid Python package either. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest +from simple_module_cli import scaffolding + + +def test_create_module_rolls_back_on_template_failure(tmp_path, monkeypatch): + """An exception during ``_apply_template_files`` must clear ``dest``.""" + dest = tmp_path / "broken_module" + + def boom(*_args, **_kwargs): + # Simulate a mid-write error after the dest directory exists but + # before all files have been laid down. + dest.mkdir(exist_ok=True) + (dest / "half_written.py").write_text("# truncated", encoding="utf-8") + raise RuntimeError("simulated template engine failure") + + monkeypatch.setattr(scaffolding, "_apply_template_files", boom) + + with pytest.raises(RuntimeError, match="simulated template engine failure"): + scaffolding.create_module(dest, "my_thing") + + assert not dest.exists(), ( + "Partial scaffold left on disk — rollback didn't fire. Subsequent " + "smpy new attempts at this path would refuse to overwrite." + ) + + +def test_rollback_does_not_delete_pre_existing_directory(tmp_path, monkeypatch): + """A pre-existing (empty) destination must stay on disk on rollback. + + We can't tell from inside ``create_module`` whether ``dest`` was made + by us or by the caller, but the directory's *prior existence* is a + reliable signal: if the caller mkdir'd it, leaving their dir alone is + the conservative choice. (The half-written scaffold contents inside + are an unavoidable consequence — preventing those needs a transactional + file system, which we don't have.) + """ + dest = tmp_path / "owned_by_caller" + dest.mkdir() + + def boom(*_args, **_kwargs): + raise RuntimeError("simulated failure before any files written") + + monkeypatch.setattr(scaffolding, "_apply_template_files", boom) + + with pytest.raises(RuntimeError): + scaffolding.create_module(dest, "thing") + + # The dir survives — we didn't make it. + assert dest.exists() + + +def test_successful_scaffold_keeps_dest(): + """Sanity check: the rollback path only fires on failure.""" + import tempfile + + with tempfile.TemporaryDirectory() as tmp: + dest = Path(tmp) / "real_module" + scaffolding.create_module(dest, "real_module") + assert dest.exists() + # The template materialises at least the package directory. + assert any(dest.iterdir()) diff --git a/framework/core/tests/test_dotenv.py b/framework/core/tests/test_dotenv.py new file mode 100644 index 00000000..9467943d --- /dev/null +++ b/framework/core/tests/test_dotenv.py @@ -0,0 +1,127 @@ +"""Unit tests for the dependency-free ``.env`` parser. + +``parse_dotenv`` is invoked by the diagnostics CLI, the users-module +bootstrap, and every worker entrypoint before settings construction — a bug +here is hard to debug because it manifests as "the setting just isn't there". +""" + +from __future__ import annotations + +import pytest +from simple_module_core.dotenv import ( + env_bool, + env_str, + load_dotenv_into_environ, + parse_dotenv, +) + + +class TestParseDotenv: + def test_missing_file_returns_empty(self, tmp_path): + assert parse_dotenv(tmp_path / "absent.env") == {} + + def test_basic_keys(self, tmp_path): + env = tmp_path / ".env" + env.write_text("FOO=bar\nBAZ=qux\n", encoding="utf-8") + assert parse_dotenv(env) == {"FOO": "bar", "BAZ": "qux"} + + def test_blank_lines_and_comments_ignored(self, tmp_path): + env = tmp_path / ".env" + env.write_text( + "# leading comment\n\nFOO=bar\n \n# inline-style # not stripped\nBAZ=qux\n", + encoding="utf-8", + ) + assert parse_dotenv(env) == {"FOO": "bar", "BAZ": "qux"} + + def test_quotes_stripped_matching_pairs(self, tmp_path): + env = tmp_path / ".env" + env.write_text( + "A=\"double\"\nB='single'\nC=plain\n", + encoding="utf-8", + ) + assert parse_dotenv(env) == {"A": "double", "B": "single", "C": "plain"} + + def test_value_with_equals_keeps_remainder(self, tmp_path): + """KEY=foo=bar=baz must parse as KEY -> "foo=bar=baz" (first ``=`` splits). + + Tokens, JWTs and database URLs frequently contain ``=`` — losing them + would silently break SMTP/JWT configuration in prod. + """ + env = tmp_path / ".env" + env.write_text("URL=postgresql://u:p=raw@h/db\n", encoding="utf-8") + assert parse_dotenv(env) == {"URL": "postgresql://u:p=raw@h/db"} + + def test_whitespace_around_key_and_value_trimmed(self, tmp_path): + env = tmp_path / ".env" + env.write_text(" KEY = value \n", encoding="utf-8") + assert parse_dotenv(env) == {"KEY": "value"} + + def test_no_equals_line_skipped(self, tmp_path): + env = tmp_path / ".env" + env.write_text("VALID=1\nbroken line without equals\nANOTHER=2\n", encoding="utf-8") + assert parse_dotenv(env) == {"VALID": "1", "ANOTHER": "2"} + + def test_default_path_uses_sm_project_root(self, tmp_path, monkeypatch): + (tmp_path / ".env").write_text("ROOTED=yes\n", encoding="utf-8") + monkeypatch.setenv("SM_PROJECT_ROOT", str(tmp_path)) + assert parse_dotenv() == {"ROOTED": "yes"} + + def test_default_path_falls_back_to_cwd(self, tmp_path, monkeypatch): + (tmp_path / ".env").write_text("CWD_KEY=present\n", encoding="utf-8") + monkeypatch.delenv("SM_PROJECT_ROOT", raising=False) + monkeypatch.chdir(tmp_path) + assert parse_dotenv() == {"CWD_KEY": "present"} + + +class TestLoadDotenvIntoEnviron: + def test_setdefault_semantics_preserves_existing_env(self, tmp_path, monkeypatch): + """Real ``os.environ`` wins over file values — same precedence as uvicorn.""" + (tmp_path / ".env").write_text("KEY=from_file\n", encoding="utf-8") + monkeypatch.setenv("KEY", "from_shell") + load_dotenv_into_environ(tmp_path / ".env") + import os + + assert os.environ["KEY"] == "from_shell" + + def test_loads_missing_keys(self, tmp_path, monkeypatch): + (tmp_path / ".env").write_text("NEW_KEY_FOR_LOAD_TEST=picked_up\n", encoding="utf-8") + monkeypatch.delenv("NEW_KEY_FOR_LOAD_TEST", raising=False) + load_dotenv_into_environ(tmp_path / ".env") + import os + + assert os.environ["NEW_KEY_FOR_LOAD_TEST"] == "picked_up" + + +class TestEnvStr: + def test_returns_value(self, monkeypatch): + monkeypatch.setenv("X", "ok") + assert env_str("X", "default") == "ok" + + def test_returns_default_when_unset(self, monkeypatch): + monkeypatch.delenv("X", raising=False) + assert env_str("X", "default") == "default" + + def test_returns_default_for_whitespace_only(self, monkeypatch): + monkeypatch.setenv("X", " ") + assert env_str("X", "default") == "default" + + +class TestEnvBool: + @pytest.mark.parametrize("raw", ["1", "true", "TRUE", "yes", "y", "on", " T "]) + def test_truthy(self, raw, monkeypatch): + monkeypatch.setenv("X", raw) + assert env_bool("X", default=False) is True + + @pytest.mark.parametrize("raw", ["0", "false", "FALSE", "no", "n", "off"]) + def test_falsy(self, raw, monkeypatch): + monkeypatch.setenv("X", raw) + assert env_bool("X", default=True) is False + + def test_unset_uses_default(self, monkeypatch): + monkeypatch.delenv("X", raising=False) + assert env_bool("X", default=True) is True + assert env_bool("X", default=False) is False + + def test_unparseable_uses_default(self, monkeypatch): + monkeypatch.setenv("X", "definitely-not-a-bool") + assert env_bool("X", default=True) is True diff --git a/framework/core/tests/test_environments.py b/framework/core/tests/test_environments.py new file mode 100644 index 00000000..04351120 --- /dev/null +++ b/framework/core/tests/test_environments.py @@ -0,0 +1,30 @@ +"""Sanity check for the shared NON_PROD_ENVIRONMENTS constant. + +A single source of truth is the whole point of this module — both host and +module settings validators read this. If anyone reverts to a duplicated +literal, the production placeholder-secret check could diverge between host +and modules and silently let an insecure deployment boot. +""" + +from __future__ import annotations + +from simple_module_core.environments import NON_PROD_ENVIRONMENTS + + +def test_contains_development_and_testing(): + assert "development" in NON_PROD_ENVIRONMENTS + assert "testing" in NON_PROD_ENVIRONMENTS + + +def test_does_not_contain_production_aliases(): + """Nothing prod-like should be treated as non-prod.""" + for name in ("production", "prod", "staging", "live", ""): + assert name not in NON_PROD_ENVIRONMENTS + + +def test_is_frozenset(): + """Freezing makes it tamper-proof — code that does + ``NON_PROD_ENVIRONMENTS.add("staging")`` to "fix" their deployment will + blow up at import time instead of widening the security envelope. + """ + assert isinstance(NON_PROD_ENVIRONMENTS, frozenset) diff --git a/framework/db/tests/test_mixins.py b/framework/db/tests/test_mixins.py new file mode 100644 index 00000000..7ed290f1 --- /dev/null +++ b/framework/db/tests/test_mixins.py @@ -0,0 +1,228 @@ +"""Direct unit tests for the SQLModel mixins. + +The mixins (AuditMixin, SoftDeleteMixin, VersionedMixin, MultiTenantMixin) +have plenty of *indirect* coverage through the multi-tenancy and audit-logging +test suites, but no test pins their individual contracts: + +* ``AuditMixin`` populates ``created_by``/``updated_by`` from the contextvar. +* ``SoftDeleteMixin`` converts ``DELETE`` into a soft-delete and applies the + default loader filter, with the documented ``include_deleted=True`` bypass. +* ``VersionedMixin`` increments ``version`` on every modifying flush. + +If any of these regressed, the only surface that would catch it today is +"the audit-log integration test breaks in some other way" — these tests +make the failure mode local and obvious. +""" + +from __future__ import annotations + +from collections.abc import AsyncGenerator + +import pytest +from simple_module_db.base import create_module_base +from simple_module_db.listeners import current_tenant_id, current_user_id, register_listeners +from simple_module_db.mixins import ( + AuditMixin, + MultiTenantMixin, + SoftDeleteMixin, + VersionedMixin, +) +from simple_module_db.provider import DatabaseProvider +from simple_module_db.session import init_db +from sqlalchemy.ext.asyncio import AsyncSession +from sqlmodel import Field, select + +_MixinsBase = create_module_base("mixins_test", provider=DatabaseProvider.SQLITE) + + +class _AuditRow(_MixinsBase, AuditMixin, table=True): # type: ignore[call-arg] # ty: ignore[unsupported-base] + __tablename__ = "mixins_test_audit_row" + id: int | None = Field(default=None, primary_key=True) + name: str = Field(max_length=100) + + +class _SoftRow(_MixinsBase, SoftDeleteMixin, table=True): # type: ignore[call-arg] # ty: ignore[unsupported-base] + __tablename__ = "mixins_test_soft_row" + id: int | None = Field(default=None, primary_key=True) + name: str = Field(max_length=100) + + +class _VersionedRow(_MixinsBase, VersionedMixin, table=True): # type: ignore[call-arg] # ty: ignore[unsupported-base] + __tablename__ = "mixins_test_versioned_row" + id: int | None = Field(default=None, primary_key=True) + name: str = Field(max_length=100) + + +class _AllRow( + _MixinsBase, # ty: ignore[unsupported-base] + MultiTenantMixin, + AuditMixin, + SoftDeleteMixin, + VersionedMixin, + table=True, +): # type: ignore[call-arg] + """Composite: confirms every listener stays compatible when stacked.""" + + __tablename__ = "mixins_test_all_row" + id: int | None = Field(default=None, primary_key=True) + name: str = Field(max_length=100) + + +@pytest.fixture +async def mixin_session() -> AsyncGenerator[AsyncSession, None]: + db_state = init_db("sqlite+aiosqlite:///:memory:") + try: + register_listeners(db_state) + async with db_state.engine.begin() as conn: + await conn.run_sync(_MixinsBase.metadata.create_all) + async with db_state.session_factory() as session: + yield session + finally: + await db_state.engine.dispose() + + +# ── AuditMixin ────────────────────────────────────────────────────────────── + + +@pytest.mark.anyio +async def test_audit_mixin_populates_created_by_from_contextvar(mixin_session): + token = current_user_id.set("alice-id") + try: + row = _AuditRow(name="hello") + mixin_session.add(row) + await mixin_session.commit() + await mixin_session.refresh(row) + finally: + current_user_id.reset(token) + + assert row.created_by == "alice-id" + assert row.updated_by == "alice-id" + assert row.created_at is not None + + +@pytest.mark.anyio +async def test_audit_mixin_updates_updated_by_on_modify(mixin_session): + create_token = current_user_id.set("alice-id") + row = _AuditRow(name="initial") + mixin_session.add(row) + await mixin_session.commit() + current_user_id.reset(create_token) + + update_token = current_user_id.set("bob-id") + try: + row.name = "changed" + await mixin_session.commit() + await mixin_session.refresh(row) + finally: + current_user_id.reset(update_token) + + assert row.created_by == "alice-id" + assert row.updated_by == "bob-id" + assert row.updated_at is not None + + +# ── SoftDeleteMixin ───────────────────────────────────────────────────────── + + +@pytest.mark.anyio +async def test_soft_delete_marks_instead_of_removing(mixin_session): + row = _SoftRow(name="will-be-soft-deleted") + mixin_session.add(row) + await mixin_session.commit() + await mixin_session.refresh(row) + rowid = row.id + + await mixin_session.delete(row) + await mixin_session.commit() + + # Row still on disk — listener intercepted the DELETE. + found = await mixin_session.execute( + select(_SoftRow).where(_SoftRow.id == rowid).execution_options(include_deleted=True) + ) + persisted = found.scalar_one() + assert persisted.is_deleted is True + assert persisted.deleted_at is not None + + # Default loader filter hides it. + default_view = await mixin_session.execute(select(_SoftRow).where(_SoftRow.id == rowid)) + assert default_view.scalar_one_or_none() is None + + +@pytest.mark.anyio +async def test_soft_delete_bypass_flag_reveals_deleted_rows(mixin_session): + a = _SoftRow(name="visible") + b = _SoftRow(name="hidden") + mixin_session.add_all([a, b]) + await mixin_session.commit() + await mixin_session.delete(b) + await mixin_session.commit() + + filtered = (await mixin_session.execute(select(_SoftRow))).scalars().all() + assert [r.name for r in filtered] == ["visible"] + + all_rows = ( + (await mixin_session.execute(select(_SoftRow).execution_options(include_deleted=True))) + .scalars() + .all() + ) + names = sorted(r.name for r in all_rows) + assert names == ["hidden", "visible"] + + +# ── VersionedMixin ────────────────────────────────────────────────────────── + + +@pytest.mark.anyio +async def test_versioned_mixin_increments_on_update(mixin_session): + row = _VersionedRow(name="v1") + mixin_session.add(row) + await mixin_session.commit() + await mixin_session.refresh(row) + assert row.version == 1 + + row.name = "v2" + await mixin_session.commit() + await mixin_session.refresh(row) + assert row.version == 2 + + row.name = "v3" + await mixin_session.commit() + await mixin_session.refresh(row) + assert row.version == 3 + + +@pytest.mark.anyio +async def test_versioned_mixin_not_incremented_for_no_op_flush(mixin_session): + row = _VersionedRow(name="stable") + mixin_session.add(row) + await mixin_session.commit() + await mixin_session.refresh(row) + assert row.version == 1 + + # Touch nothing — just commit again. ``session.is_modified`` should + # return False so the listener skips the bump. + await mixin_session.commit() + await mixin_session.refresh(row) + assert row.version == 1 + + +# ── Composition: all four mixins on one row ───────────────────────────────── + + +@pytest.mark.anyio +async def test_all_mixins_compose_without_conflict(mixin_session): + user_token = current_user_id.set("composer-id") + tenant_token = current_tenant_id.set("tenant-A") + try: + row = _AllRow(name="composed") + mixin_session.add(row) + await mixin_session.commit() + await mixin_session.refresh(row) + finally: + current_user_id.reset(user_token) + current_tenant_id.reset(tenant_token) + + assert row.tenant_id == "tenant-A" + assert row.created_by == "composer-id" + assert row.is_deleted is False + assert row.version == 1 diff --git a/framework/db/tests/test_postgres_schema_per_module.py b/framework/db/tests/test_postgres_schema_per_module.py new file mode 100644 index 00000000..5ad6eb32 --- /dev/null +++ b/framework/db/tests/test_postgres_schema_per_module.py @@ -0,0 +1,106 @@ +"""End-to-end Postgres schema-per-module test (skipped without a PG URL). + +CLAUDE.md promises that on Postgres each module's tables live in their own +schema (``orders.``). All of the existing DB test suites run against +SQLite where the convention is to prefix the table name instead, so the +Postgres branch of ``create_module_base`` is exercised only by the +diagnostics tests indirectly. + +This test runs only if ``SM_POSTGRES_TEST_URL`` is set, so CI on machines +without a Postgres available is a clean skip rather than a failure. +""" + +from __future__ import annotations + +import os +import uuid +from typing import TYPE_CHECKING + +import pytest +from simple_module_db.base import create_module_base +from simple_module_db.provider import DatabaseProvider +from simple_module_db.session import init_db +from sqlalchemy import text +from sqlmodel import Field + +if TYPE_CHECKING: + pass + + +_PG_URL = os.environ.get("SM_POSTGRES_TEST_URL") +_PG_SKIP_REASON = "Set SM_POSTGRES_TEST_URL=postgresql+asyncpg://... to run Postgres tests" + + +@pytest.mark.anyio +@pytest.mark.skipif(not _PG_URL, reason=_PG_SKIP_REASON) +async def test_module_tables_isolated_per_schema(): + """Two modules' tables must live in separate schemas with the same suffix. + + Without per-schema isolation, ``orders.product`` and ``billing.product`` + would collide. We create two ad-hoc module bases with the same suffix, + insert rows in each, and confirm the rows are not visible cross-schema. + """ + schema_a = f"sm_test_a_{uuid.uuid4().hex[:8]}" + schema_b = f"sm_test_b_{uuid.uuid4().hex[:8]}" + + base_a = create_module_base(schema_a, provider=DatabaseProvider.POSTGRESQL) + base_b = create_module_base(schema_b, provider=DatabaseProvider.POSTGRESQL) + + class _ProductA(base_a, table=True): # type: ignore[call-arg,misc] # ty: ignore[unsupported-base] + __tablename__ = "product" + id: int | None = Field(default=None, primary_key=True) + name: str = Field(max_length=100) + + class _ProductB(base_b, table=True): # type: ignore[call-arg,misc] # ty: ignore[unsupported-base] + __tablename__ = "product" + id: int | None = Field(default=None, primary_key=True) + name: str = Field(max_length=100) + + db_state = init_db(_PG_URL) # type: ignore[arg-type] + try: + async with db_state.engine.begin() as conn: + await conn.execute(text(f'CREATE SCHEMA IF NOT EXISTS "{schema_a}"')) + await conn.execute(text(f'CREATE SCHEMA IF NOT EXISTS "{schema_b}"')) + await conn.run_sync(base_a.metadata.create_all) + await conn.run_sync(base_b.metadata.create_all) + + async with db_state.session_factory() as session: + session.add(_ProductA(name="a-thing")) + session.add(_ProductB(name="b-thing")) + await session.commit() + + # Raw SQL to bypass ORM tenant filters and confirm schema isolation. + in_a = ( + (await session.execute(text(f'SELECT name FROM "{schema_a}"."product"'))) + .scalars() + .all() + ) + in_b = ( + (await session.execute(text(f'SELECT name FROM "{schema_b}"."product"'))) + .scalars() + .all() + ) + assert in_a == ["a-thing"] + assert in_b == ["b-thing"] + + # Teardown + async with db_state.engine.begin() as conn: + await conn.execute(text(f'DROP SCHEMA "{schema_a}" CASCADE')) + await conn.execute(text(f'DROP SCHEMA "{schema_b}" CASCADE')) + finally: + await db_state.engine.dispose() + + +def test_module_metadata_has_schema_set(): + """Static check: a base created with provider=POSTGRESQL stamps a schema. + + Runs without a live Postgres so we still get coverage of the metadata + branch on every test run. + """ + base = create_module_base("isolated_smoke", provider=DatabaseProvider.POSTGRESQL) + assert base.metadata.schema == "isolated_smoke" + + +def test_module_metadata_has_no_schema_for_sqlite(): + base = create_module_base("flat_smoke", provider=DatabaseProvider.SQLITE) + assert base.metadata.schema is None diff --git a/framework/hosting/tests/test_check_migrations.py b/framework/hosting/tests/test_check_migrations.py new file mode 100644 index 00000000..b71235be --- /dev/null +++ b/framework/hosting/tests/test_check_migrations.py @@ -0,0 +1,84 @@ +"""Boot-time migration-drift check. + +``check_migrations`` is called from inside the app's lifespan in +``app_builder.py``. If it loses its teeth — e.g. someone refactors and forgets +to raise — a behind-head DB would silently boot and produce confusing missing- +column errors at runtime. This file pins: + +* DB at head → returns the status dict. +* DB behind head → raises ``RuntimeError`` with a helpful message. +* No alembic config available → returns the "no migrations" sentinel. +""" + +from __future__ import annotations + +import pytest +from simple_module_hosting.migrations import check_migrations, resolve_head_revision +from sqlalchemy import text +from sqlalchemy.ext.asyncio import create_async_engine + + +@pytest.mark.anyio +async def test_returns_no_migrations_sentinel_when_alembic_ini_absent(tmp_path): + """Pointing at a missing alembic.ini → status dict, no exception.""" + engine = create_async_engine("sqlite+aiosqlite:///:memory:") + try: + result = await check_migrations(engine, alembic_ini_path=str(tmp_path / "missing.ini")) + finally: + await engine.dispose() + assert result["current_revision"] is None + assert result["head_revision"] is None + assert result["is_current"] is True + + +@pytest.mark.anyio +async def test_db_at_head_returns_current_status(): + """Stamp the in-memory DB at head and check_migrations should pass cleanly.""" + head = resolve_head_revision() + if head is None: + pytest.skip("Repository alembic.ini not resolvable from cwd") + + engine = create_async_engine("sqlite+aiosqlite:///:memory:") + try: + async with engine.begin() as conn: + await conn.execute( + text("CREATE TABLE alembic_version (version_num VARCHAR(32) NOT NULL PRIMARY KEY)") + ) + await conn.execute( + text("INSERT INTO alembic_version (version_num) VALUES (:v)"), + {"v": head}, + ) + result = await check_migrations(engine) + finally: + await engine.dispose() + + assert result["current_revision"] == head + assert result["head_revision"] == head + assert result["is_current"] is True + + +@pytest.mark.anyio +async def test_unstamped_db_raises_drift_error(): + """An empty in-memory DB (no alembic_version row) must hard-fail.""" + if resolve_head_revision() is None: + pytest.skip("Repository alembic.ini not resolvable from cwd") + + engine = create_async_engine("sqlite+aiosqlite:///:memory:") + try: + with pytest.raises(RuntimeError, match="revision\\(s\\) behind"): + await check_migrations(engine) + finally: + await engine.dispose() + + +@pytest.mark.anyio +async def test_resolve_head_revision_consistent(): + """Sanity: ``resolve_head_revision`` returns the same string twice in a row. + + The function is invoked from both the cached fixture in conftest and the + real lifespan; if it ever became non-deterministic the cached value would + diverge from the live one and the migration check would lie. + """ + a = resolve_head_revision() + b = resolve_head_revision() + assert a == b diff --git a/framework/hosting/tests/test_lifespan_order.py b/framework/hosting/tests/test_lifespan_order.py new file mode 100644 index 00000000..60d7778d --- /dev/null +++ b/framework/hosting/tests/test_lifespan_order.py @@ -0,0 +1,85 @@ +"""Verify ``create_app``'s lifespan walks modules forward and backward. + +CLAUDE.md says ``on_startup`` runs in topological order and ``on_shutdown`` +in reverse — a regression here means a dependent module's startup hook runs +before its dependency's hook is ready (or, on shutdown, the dependency tears +down resources while the dependent is still using them). +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING +from unittest.mock import patch + +import pytest +from simple_module_core.module import ModuleBase, ModuleMeta +from simple_module_hosting.app_builder import create_app +from simple_module_hosting.settings import Settings + +if TYPE_CHECKING: + from fastapi import FastAPI + + +_calls: list[str] = [] + + +class _TrackingModule(ModuleBase): + """Records its own name into the module-level _calls list on each hook. + + Subclasses set their own ``meta`` so each one is identifiable inside + the dependency graph; the hook bodies stay on the base. + """ + + async def on_startup(self, app: FastAPI) -> None: # type: ignore[override] + _calls.append(f"start:{self.meta.name}") + + async def on_shutdown(self, app: FastAPI) -> None: # type: ignore[override] + _calls.append(f"stop:{self.meta.name}") + + +class _ModA(_TrackingModule): + meta = ModuleMeta(name="A") + + +class _ModB(_TrackingModule): + meta = ModuleMeta(name="B", depends_on=["A"]) + + +class _ModC(_TrackingModule): + meta = ModuleMeta(name="C", depends_on=["B"]) + + +@pytest.mark.anyio +async def test_lifespan_startup_forward_shutdown_reverse(monkeypatch) -> None: + """Three modules A→B→C: startup must be A,B,C and shutdown C,B,A. + + We stub ``discover_modules`` rather than registering real entry points + so this test stays hermetic; the topological-sort layer is exercised + end-to-end by other tests already. + """ + _calls.clear() + instances: list[ModuleBase] = [_ModA(), _ModB(), _ModC()] + + async def _no_migration_check(engine, *args, **kwargs): + return {"current_revision": None, "head_revision": None, "is_current": True} + + with ( + patch("simple_module_hosting.app_builder.discover_modules", return_value=instances), + patch("simple_module_hosting.app_builder.check_migrations", _no_migration_check), + ): + settings = Settings( + database_url="sqlite+aiosqlite:///:memory:", + environment="testing", + secret_key="x" * 32, + multi_tenant=False, + ) + app = create_app(settings) + + ctx = app.router.lifespan_context(app) + await ctx.__aenter__() + await ctx.__aexit__(None, None, None) + + starts = [c for c in _calls if c.startswith("start:")] + stops = [c for c in _calls if c.startswith("stop:")] + assert starts == ["start:A", "start:B", "start:C"], _calls + assert stops == ["stop:C", "stop:B", "stop:A"], _calls diff --git a/framework/hosting/tests/test_middleware_order.py b/framework/hosting/tests/test_middleware_order.py new file mode 100644 index 00000000..7b164959 --- /dev/null +++ b/framework/hosting/tests/test_middleware_order.py @@ -0,0 +1,79 @@ +"""Pin the documented middleware execution order. + +CLAUDE.md spells out the pipeline: + + CorrelationId → RequestLogging → Security → Session → + → Tenant (opt-in) → Locale → InertiaLayoutData → app + +Tenant/Locale must see ``request.state.user`` set by AuthMiddleware so +DB queries get filtered correctly; CorrelationId must wrap everything so +every log line carries its id. Order matters and a swap is the kind of +regression that breaks production without breaking any happy-path test. +``app.user_middleware`` lists middlewares in execution order (Starlette +LIFOs ``add_middleware`` calls, FastAPI surfaces them already reversed). +""" + +from __future__ import annotations + +import pytest +from simple_module_hosting.app_builder import create_app +from simple_module_hosting.settings import Settings + +_EXPECTED_MULTI_TENANT = ( + "CorrelationIdMiddleware", + "RequestLoggingMiddleware", + "SecurityHeadersMiddleware", + "SessionMiddleware", + "AuthMiddleware", + "TenantMiddleware", + "LocaleMiddleware", + "InertiaLayoutDataMiddleware", +) + +_EXPECTED_SINGLE_TENANT = ( + "CorrelationIdMiddleware", + "RequestLoggingMiddleware", + "SecurityHeadersMiddleware", + "SessionMiddleware", + "AuthMiddleware", + "LocaleMiddleware", + "InertiaLayoutDataMiddleware", +) + + +def _names(app) -> tuple[str, ...]: + return tuple(m.cls.__name__ for m in app.user_middleware) + + +@pytest.mark.parametrize( + ("multi_tenant", "expected"), + [(True, _EXPECTED_MULTI_TENANT), (False, _EXPECTED_SINGLE_TENANT)], +) +def test_middleware_pipeline_order(multi_tenant: bool, expected: tuple[str, ...]) -> None: + settings = Settings( + database_url="sqlite+aiosqlite:///:memory:", + environment="testing", + secret_key="x" * 32, + multi_tenant=multi_tenant, + ) + app = create_app(settings) + assert _names(app) == expected, ( + "Middleware pipeline order drifted from CLAUDE.md. " + f"Got {_names(app)!r}, expected {expected!r}." + ) + + +def test_tenant_middleware_absent_when_disabled() -> None: + """``multi_tenant=False`` must not register TenantMiddleware at all. + + Just toggling off the header would still leak the DB context-var setter + onto every request; the middleware itself is what must vanish. + """ + settings = Settings( + database_url="sqlite+aiosqlite:///:memory:", + environment="testing", + secret_key="x" * 32, + multi_tenant=False, + ) + app = create_app(settings) + assert "TenantMiddleware" not in _names(app) diff --git a/framework/hosting/tests/test_redirects.py b/framework/hosting/tests/test_redirects.py new file mode 100644 index 00000000..e7090cb8 --- /dev/null +++ b/framework/hosting/tests/test_redirects.py @@ -0,0 +1,129 @@ +"""Direct unit tests for ``safe_referer_or_root``. + +The helper is the only barrier between an attacker-controlled ``Referer`` and +a 303 redirect back to that URL. The existing test surface only goes through +``/i18n/set-locale``, which exercises a handful of vectors. This file pins the +contract directly with adversarial inputs that the integration test set didn't +reach (``javascript:``, embedded ``@`` userinfo, ``\\evil.example``, +CRLF-injection attempts, mixed-case schemes, fragments). +""" + +from __future__ import annotations + +from types import SimpleNamespace + +import pytest +from simple_module_hosting.redirects import safe_referer_or_root + + +def _make_request(*, referer: str | None = None, scheme: str = "http", host: str = "testserver"): + """Return a minimal duck-typed object matching what safe_referer_or_root reads. + + The real helper only touches ``request.headers.get("referer")`` and + ``request.url.scheme`` / ``request.url.netloc`` — a SimpleNamespace beats + spinning up a Starlette Request just to validate URL parsing. + """ + headers: dict[str, str] = {} + if referer is not None: + headers["referer"] = referer + return SimpleNamespace( + headers=headers, + url=SimpleNamespace(scheme=scheme, netloc=host), + ) + + +class TestSafeRefererBasics: + def test_no_referer_returns_root(self) -> None: + assert safe_referer_or_root(_make_request()) == "/" + + def test_empty_string_referer_returns_root(self) -> None: + assert safe_referer_or_root(_make_request(referer="")) == "/" + + def test_same_origin_relative_path_preserved(self) -> None: + assert safe_referer_or_root(_make_request(referer="/dashboard")) == "/dashboard" + + def test_same_origin_absolute_url_collapses_to_path(self) -> None: + req = _make_request(referer="http://testserver/products?q=pen") + assert safe_referer_or_root(req) == "/products?q=pen" + + +class TestSafeRefererBlocksHostedirects: + """Every input here is an attempt to redirect off-site. + + A regression that returns the input verbatim is a reflected open-redirect. + """ + + @pytest.mark.parametrize( + "malicious", + [ + "https://evil.example/steal", + "http://evil.example/x", + "//evil.example/x", + "//evil.example", + r"\\evil.example/x", # backslash-prefixed — some browsers normalize + "http://testserver.evil.example/", # suffix-confusion + "http://evil.example@testserver/", # userinfo trick: host is "evil.example" + "javascript:alert(1)", + "data:text/html,", + "vbscript:msgbox(1)", + "FILE:///etc/passwd", + "HTTPS://EVIL.EXAMPLE/", + "not-a-path", + " ", + "ftp://evil.example/", + ], + ) + def test_rejects_hostile_referer(self, malicious: str) -> None: + req = _make_request(referer=malicious) + result = safe_referer_or_root(req) + # The helper must never return anything that, when used as a Location + # header, takes the browser off-site. The contract is "/" or a path + # starting with "/" on the same origin. + assert result.startswith("/"), ( + f"safe_referer_or_root({malicious!r}) returned {result!r}; " + "must fall back to a same-origin path" + ) + # And specifically: no second slash that would make the browser see + # this as a protocol-relative URL. + assert not result.startswith("//"), ( + f"safe_referer_or_root({malicious!r}) returned protocol-relative {result!r}" + ) + + def test_userinfo_at_sign_does_not_smuggle_host(self) -> None: + """``http://evil@testserver/`` parses as host=testserver in urlsplit. + + That's actually safe — the helper compares parsed.netloc which includes + the userinfo. The defense is that ``parsed.netloc != current.netloc`` + when userinfo is present, so the comparison correctly rejects. + """ + req = _make_request(referer="http://attacker@testserver/admin") + assert safe_referer_or_root(req) == "/" + + +class TestSafeRefererSchemeAndHostMatching: + def test_scheme_mismatch_rejects_https_referer_on_http_request(self) -> None: + req = _make_request(referer="https://testserver/x", scheme="http") + assert safe_referer_or_root(req) == "/" + + def test_host_mismatch_rejects_subdomain(self) -> None: + req = _make_request(referer="http://admin.testserver/x") + assert safe_referer_or_root(req) == "/" + + def test_port_mismatch_rejects(self) -> None: + req = _make_request(referer="http://testserver:8080/x", host="testserver") + assert safe_referer_or_root(req) == "/" + + +class TestSafeRefererPathHandling: + def test_query_string_preserved(self) -> None: + req = _make_request(referer="http://testserver/x?a=1&b=2") + assert safe_referer_or_root(req) == "/x?a=1&b=2" + + def test_fragment_dropped(self) -> None: + """Fragments aren't sent to the server, so we don't echo them in Location.""" + req = _make_request(referer="http://testserver/x#section") + assert safe_referer_or_root(req) == "/x" + + def test_empty_path_becomes_root(self) -> None: + req = _make_request(referer="http://testserver") + assert safe_referer_or_root(req) == "/" diff --git a/framework/hosting/tests/test_session_cookie_security.py b/framework/hosting/tests/test_session_cookie_security.py new file mode 100644 index 00000000..c072773b --- /dev/null +++ b/framework/hosting/tests/test_session_cookie_security.py @@ -0,0 +1,54 @@ +"""SameSite/HttpOnly invariants for the framework's session cookie. + +CLAUDE.md treats SameSite=Lax as the CSRF defence — there is no explicit +token middleware. If a future Starlette upgrade silently switched the +default to ``None`` (used to be the case on older versions), CSRF +protection would evaporate without any test catching it. + +The session cookie is also Set-Cookie'd with HttpOnly so a successful XSS +can't directly exfiltrate the user_id+user_ctx blob. +""" + +from __future__ import annotations + +import httpx +import pytest + + +def _set_cookie_for(name: str, response: httpx.Response) -> str: + """Return the raw Set-Cookie header for ``name`` (or '' if absent). + + httpx joins multiple Set-Cookie headers with ', ' which makes + ``response.headers.get`` ambiguous for cookies whose value contains a + comma. We walk the raw header list instead. + """ + for header_name, header_value in response.headers.multi_items(): + if header_name.lower() == "set-cookie" and header_value.startswith(f"{name}="): + return header_value + return "" + + +@pytest.mark.anyio +async def test_session_cookie_is_samesite_lax_and_httponly(client) -> None: + """Any response that creates the session cookie must mark it Lax + HttpOnly. + + An unauthenticated request to a protected page (``/dashboard/``) + deterministically writes to the session: ``AuthMiddleware`` stores the + intended target in ``session["next"]`` before redirecting to the login + page. That guarantees ``SessionMiddleware.save()`` emits a Set-Cookie + header regardless of how other middleware happens to touch the session. + """ + resp = await client.get("/dashboard/", follow_redirects=False) + raw = _set_cookie_for("session", resp) + + assert raw, ( + "Protected route didn't set the session cookie — has SessionMiddleware " + "or AuthMiddleware been removed from the pipeline?" + ) + lowered = raw.lower() + assert "samesite=lax" in lowered, ( + f"Session cookie missing SameSite=Lax — CSRF defence weakened. Raw: {raw!r}" + ) + assert "httponly" in lowered, ( + f"Session cookie missing HttpOnly — exposes user_id to XSS. Raw: {raw!r}" + ) diff --git a/framework/hosting/tests/test_strict_discovery_wiring.py b/framework/hosting/tests/test_strict_discovery_wiring.py new file mode 100644 index 00000000..1b6565b6 --- /dev/null +++ b/framework/hosting/tests/test_strict_discovery_wiring.py @@ -0,0 +1,116 @@ +"""``create_app(settings)`` must propagate strict-discovery in non-dev environments. + +``app_builder.create_app`` calls ``discover_modules(strict=not settings.is_development)``. +The existing tests cover ``discover_modules`` directly with strict=True, but +nothing pins the wiring — a regression that hard-coded ``strict=False`` would +silently restore the old "drop a broken module and keep booting" behaviour in +production, which is exactly what CLAUDE.md says must not happen. + +Reuses the ``_FakeEntryPoint`` and ``_patch_entry_points`` helpers from +``framework/core/tests/test_discovery.py`` (the canonical location for the +entry-point stubbing pattern) rather than redeclaring the same shim here. +""" + +from __future__ import annotations + +import contextlib +import importlib.util +from pathlib import Path + +import pytest +from simple_module_core.exceptions import InvalidModuleError +from simple_module_hosting.app_builder import create_app +from simple_module_hosting.settings import Settings + + +def _load_discovery_helpers(): + """Side-load ``framework/core/tests/test_discovery.py`` without mutating ``sys.path``. + + The core-tests directory isn't a package (no ``__init__.py``), and adding + it to ``sys.path`` would expose every ``test_*`` module in there as a + top-level import for the rest of the session — risking name collisions. + ``spec_from_file_location`` loads just the one file we need into a + private namespace. + """ + discovery_path = Path(__file__).resolve().parents[2] / "core" / "tests" / "test_discovery.py" + spec = importlib.util.spec_from_file_location("_core_test_discovery_helpers", discovery_path) + assert spec is not None and spec.loader is not None + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) + return mod._FakeEntryPoint, mod._boom_loader, mod._patch_entry_points + + +_FakeEntryPoint, _boom_loader, _patch_entry_points = _load_discovery_helpers() + + +class _NotAModule: + """A class returned by an entry point that isn't a ModuleBase subclass.""" + + +def _prod_settings() -> Settings: + return Settings( + database_url="sqlite+aiosqlite:///:memory:", + environment="production", + secret_key="x" * 32, + multi_tenant=False, + ) + + +def test_create_app_in_production_fails_on_broken_entrypoint(monkeypatch): + """A failed entry-point load in production must abort ``create_app``.""" + _patch_entry_points(monkeypatch, [_FakeEntryPoint("boom", _boom_loader)]) + with pytest.raises(InvalidModuleError, match="Failed to load"): + create_app(_prod_settings()) + + +def test_create_app_in_production_fails_on_non_modulebase(monkeypatch): + """Same contract for non-ModuleBase classes registered as entry points.""" + _patch_entry_points(monkeypatch, [_FakeEntryPoint("notmod", _NotAModule)]) + with pytest.raises(InvalidModuleError, match="not a ModuleBase"): + create_app(_prod_settings()) + + +def test_discover_modules_called_with_strict_mirroring_environment(monkeypatch): + """The wiring assertion: ``app_builder`` passes ``strict=not is_development``. + + We don't actually run ``create_app`` in dev — that triggers Inertia setup + and ``emit_frontend_types``, which mutates the generated i18n type files + on disk because the entry-point stub yields zero modules. Asserting the + keyword argument is sufficient to pin the wiring contract. + """ + captured: dict[str, object] = {} + + def _spy(*args, **kwargs): + captured["strict"] = kwargs.get("strict") + return [] + + monkeypatch.setattr("simple_module_hosting.app_builder.discover_modules", _spy) + # Block dev-mode side effects (write_module_pages_manifest + + # emit_frontend_types) — with zero modules they'd rewrite the + # generated i18n files to empty. + monkeypatch.setattr( + "simple_module_hosting.app_builder.emit_frontend_types", lambda *a, **kw: None + ) + import simple_module_hosting.manifest as manifest_mod + + monkeypatch.setattr(manifest_mod, "write_module_pages_manifest", lambda *a, **kw: None) + + # Dev environment — strict must be False. + dev = Settings( + database_url="sqlite+aiosqlite:///:memory:", + environment="development", + secret_key="x" * 32, + multi_tenant=False, + ) + # Builder will fail later (no Inertia templates, no settings module), + # but we already captured ``strict`` from the spy. + with contextlib.suppress(Exception): + create_app(dev) + assert captured.get("strict") is False, "Dev mode must pass strict=False" + + # Production environment — strict must be True. + captured.clear() + prod = _prod_settings() + with contextlib.suppress(Exception): + create_app(prod) + assert captured.get("strict") is True, "Production mode must pass strict=True" diff --git a/modules/background_tasks/background_tasks/constants.py b/modules/background_tasks/background_tasks/constants.py index ee2c2952..995ffc8f 100644 --- a/modules/background_tasks/background_tasks/constants.py +++ b/modules/background_tasks/background_tasks/constants.py @@ -16,6 +16,9 @@ TABLE_PREFIX = "background_tasks_" TABLE_TASK_EXECUTION = f"{TABLE_PREFIX}task_execution" +# ── Module dependencies ───────────────────────────────────────── +_MODULE_USERS = "Users" + # ── Env / settings ────────────────────────────────────────────── ENV_PREFIX = "SM_BG_TASKS_" diff --git a/modules/background_tasks/background_tasks/module.py b/modules/background_tasks/background_tasks/module.py index 70cf2bae..69742abe 100644 --- a/modules/background_tasks/background_tasks/module.py +++ b/modules/background_tasks/background_tasks/module.py @@ -13,6 +13,7 @@ from simple_module_core.permissions import PermissionRegistry from background_tasks.constants import ( + _MODULE_USERS, API_PREFIX, MENU_ICON, MENU_LABEL, @@ -39,7 +40,7 @@ class BackgroundTasksModule(ModuleBase): name=MODULE_DISPLAY_NAME, route_prefix=API_PREFIX, view_prefix=VIEW_PREFIX, - depends_on=["Users"], + depends_on=[_MODULE_USERS], ) def register_settings(self, app: FastAPI) -> None: diff --git a/modules/background_tasks/tests/conftest.py b/modules/background_tasks/tests/conftest.py index 47c11afc..c64f0025 100644 --- a/modules/background_tasks/tests/conftest.py +++ b/modules/background_tasks/tests/conftest.py @@ -2,9 +2,13 @@ from __future__ import annotations +from collections.abc import Iterator +from pathlib import Path from unittest.mock import MagicMock import pytest +from background_tasks import sync_db +from background_tasks.models import TaskExecution @pytest.fixture @@ -20,3 +24,28 @@ async def _stub_celery(app) -> None: celery = MagicMock(name="Celery") celery.send_task.return_value.id = "mocked-celery-id" app.state.background_tasks.celery = celery + + +@pytest.fixture +def sync_sqlite(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Iterator[Path]: + """Point ``sync_db`` at a fresh on-disk SQLite file and reset the cache. + + Shared between the signal-handler suite and the stuck-sweep suite; both + exercise the sync DB code path that workers use, and need a clean engine + per test so the process-global cache doesn't bleed across files. + + Uses ``dispose_sync_engine`` for setup and teardown — it's the only sweep + that also clears ``_url_override``, so a prior call to + ``set_database_url(...)`` in some other test (or worker module startup) + can't shadow the ``SM_DATABASE_URL`` we set here. + """ + db_file = tmp_path / "bg_tasks_sync.db" + monkeypatch.setenv("SM_DATABASE_URL", f"sqlite:///{db_file}") + sync_db.dispose_sync_engine() + + factory = sync_db.get_sync_session_factory() + TaskExecution.metadata.create_all(factory.kw["bind"]) + + yield db_file + + sync_db.dispose_sync_engine() diff --git a/modules/background_tasks/tests/test_sweep_stuck.py b/modules/background_tasks/tests/test_sweep_stuck.py new file mode 100644 index 00000000..df622647 --- /dev/null +++ b/modules/background_tasks/tests/test_sweep_stuck.py @@ -0,0 +1,118 @@ +"""``sweep_stuck_tasks`` is the only path that recovers a crashed worker. + +If a Celery worker dies between ``task_prerun`` (RUNNING) and a terminal +signal (SUCCESS/FAILED), the corresponding ``TaskExecution`` row stays in +RUNNING forever — never retried, never surfaced. ``sweep_stuck_tasks`` is +the beat-scheduled task that flips those rows to STUCK once their +heartbeat goes stale. +""" + +from __future__ import annotations + +import uuid +from datetime import UTC, datetime, timedelta + +import pytest +from background_tasks import sync_db +from background_tasks.constants import TaskStatus +from background_tasks.models import TaskExecution +from background_tasks.tasks import sweep_stuck_tasks +from sqlalchemy import select + + +@pytest.fixture(autouse=True) +def _stuck_after_60s(monkeypatch: pytest.MonkeyPatch) -> None: + """Pin the stuck-cutoff so the cutoff-vs-heartbeat assertions are deterministic.""" + monkeypatch.setenv("SM_BG_TASKS_STUCK_AFTER_SECONDS", "60") + + +def _make_row(*, status: TaskStatus, heartbeat_at: datetime) -> uuid.UUID: + """Insert one TaskExecution row directly via the sync session.""" + row_id = uuid.uuid4() + factory = sync_db.get_sync_session_factory() + with factory() as session: + row = TaskExecution( + id=row_id, + celery_task_id=str(uuid.uuid4()), + task_name="test.task", + status=status, + heartbeat_at=heartbeat_at, + queue="celery", + ) + session.add(row) + session.commit() + return row_id + + +def _fetch(row_id: uuid.UUID) -> TaskExecution: + factory = sync_db.get_sync_session_factory() + with factory() as session: + return session.execute(select(TaskExecution).where(TaskExecution.id == row_id)).scalar_one() + + +def test_running_with_stale_heartbeat_flipped_to_stuck(sync_sqlite): + """RUNNING + heartbeat older than the cutoff → STUCK + finished_at set.""" + stale = datetime.now(UTC) - timedelta(seconds=120) # >60s cutoff + rid = _make_row(status=TaskStatus.RUNNING, heartbeat_at=stale) + + flipped = sweep_stuck_tasks() + assert flipped == 1 + + row = _fetch(rid) + assert row.status == TaskStatus.STUCK + assert row.finished_at is not None + + +def test_running_with_fresh_heartbeat_untouched(sync_sqlite): + """A worker that's checked in recently must NOT be flipped — that would + abort an in-flight task as if it had crashed.""" + fresh = datetime.now(UTC) - timedelta(seconds=10) + rid = _make_row(status=TaskStatus.RUNNING, heartbeat_at=fresh) + + flipped = sweep_stuck_tasks() + assert flipped == 0 + + row = _fetch(rid) + assert row.status == TaskStatus.RUNNING + + +def test_terminal_rows_never_touched(sync_sqlite): + """SUCCESS/FAILED rows with stale heartbeats must stay terminal. + + A regression that matched on ``heartbeat_at < cutoff`` alone (forgetting + the ``status==RUNNING`` clause) would re-open finished tasks. + """ + stale = datetime.now(UTC) - timedelta(seconds=120) + success_id = _make_row(status=TaskStatus.SUCCESS, heartbeat_at=stale) + failed_id = _make_row(status=TaskStatus.FAILED, heartbeat_at=stale) + + flipped = sweep_stuck_tasks() + assert flipped == 0 + + assert _fetch(success_id).status == TaskStatus.SUCCESS + assert _fetch(failed_id).status == TaskStatus.FAILED + + +def test_idempotent_when_called_repeatedly(sync_sqlite): + """Running the sweep twice in a row must flip zero rows the second time.""" + stale = datetime.now(UTC) - timedelta(seconds=120) + _make_row(status=TaskStatus.RUNNING, heartbeat_at=stale) + + first = sweep_stuck_tasks() + second = sweep_stuck_tasks() + assert first == 1 + assert second == 0 + + +def test_uses_env_var_for_cutoff(sync_sqlite, monkeypatch): + """``SM_BG_TASKS_STUCK_AFTER_SECONDS`` is what makes a heartbeat "stale". + + Pushing the cutoff to 999s should make a 120s-old heartbeat look fresh. + """ + stale = datetime.now(UTC) - timedelta(seconds=120) + rid = _make_row(status=TaskStatus.RUNNING, heartbeat_at=stale) + + monkeypatch.setenv("SM_BG_TASKS_STUCK_AFTER_SECONDS", "999") + flipped = sweep_stuck_tasks() + assert flipped == 0 + assert _fetch(rid).status == TaskStatus.RUNNING diff --git a/modules/dashboard/tests/test_view_routes.py b/modules/dashboard/tests/test_view_routes.py new file mode 100644 index 00000000..ebf25c14 --- /dev/null +++ b/modules/dashboard/tests/test_view_routes.py @@ -0,0 +1,54 @@ +"""View-route smoke tests for dashboard, settings, and feature_flags. + +The audit found these Inertia views had no behavioural coverage — only the +underlying service was tested. A typo in ``_PAGE_HOME`` or a forgotten import +in the views module would manifest only when a real user clicked the link. +""" + +from __future__ import annotations + +import pytest + + +@pytest.mark.anyio +async def test_dashboard_index_renders_for_admin(authenticated_client): + """``/dashboard/`` must respond 200 and produce an Inertia payload.""" + resp = await authenticated_client.get("/dashboard/", follow_redirects=False) + assert resp.status_code == 200, resp.text + # Inertia full-page response is HTML wrapping a ``data-page`` JSON blob. + assert "data-page" in resp.text, "Dashboard view didn't render an Inertia page" + + +@pytest.mark.anyio +async def test_dashboard_doctor_renders_for_admin(authenticated_client): + """The doctor sub-page mirrors the same route shape.""" + resp = await authenticated_client.get("/dashboard/doctor", follow_redirects=False) + assert resp.status_code == 200, resp.text + assert "data-page" in resp.text + + +@pytest.mark.anyio +async def test_settings_index_renders_for_admin(authenticated_client): + """The Settings module's browse page is reachable for admins.""" + resp = await authenticated_client.get("/settings/", follow_redirects=False) + assert resp.status_code == 200, resp.text + assert "data-page" in resp.text + + +@pytest.mark.anyio +async def test_settings_modules_renders_for_admin(authenticated_client): + """Per-module settings UI must render without error.""" + resp = await authenticated_client.get("/settings/modules", follow_redirects=False) + assert resp.status_code == 200, resp.text + assert "data-page" in resp.text + + +@pytest.mark.anyio +async def test_dashboard_index_redirects_anon_to_login(client): + """An unauthenticated visit to ``/dashboard/`` must redirect to login. + + Confirms AuthMiddleware sits before the view router as documented. + """ + resp = await client.get("/dashboard/", follow_redirects=False) + assert resp.status_code == 302 + assert "/users/login" in resp.headers["location"] diff --git a/modules/feature_flags/feature_flags/module.py b/modules/feature_flags/feature_flags/module.py index dce6a8fd..bea41512 100644 --- a/modules/feature_flags/feature_flags/module.py +++ b/modules/feature_flags/feature_flags/module.py @@ -3,6 +3,7 @@ from __future__ import annotations import importlib.resources +import logging from pathlib import Path from fastapi import APIRouter, FastAPI @@ -20,6 +21,8 @@ PERM_FEATURE_FLAGS_VIEW, ) +_logger = logging.getLogger(__name__) + class FeatureFlagsModule(ModuleBase): meta = ModuleMeta( @@ -66,10 +69,20 @@ async def on_startup(self, app: FastAPI) -> None: Called once, after DB init and before the app starts serving. From here on ``registry.is_enabled`` reflects admin overrides even for requests that don't hit this module's endpoints. + + If the DB read fails (store unreachable, table missing, etc.) we + log a warning and continue with the registry at its + ``register_feature_flags``-declared defaults rather than letting a + transient outage take the whole app down. Defaults are the + conservative choice — admins can re-toggle overrides once the + store is healthy again. """ from feature_flags.service import FeatureFlagService sm = app.state.sm - async with sm.db.session_factory() as session: - service = FeatureFlagService(session) - await service.hydrate_registry(sm.feature_flags) + try: + async with sm.db.session_factory() as session: + service = FeatureFlagService(session) + await service.hydrate_registry(sm.feature_flags) + except Exception: + _logger.exception("feature_flags.hydrate_failed — continuing with registry defaults") diff --git a/modules/feature_flags/tests/test_store_outage_fallback.py b/modules/feature_flags/tests/test_store_outage_fallback.py new file mode 100644 index 00000000..d5e44cfb --- /dev/null +++ b/modules/feature_flags/tests/test_store_outage_fallback.py @@ -0,0 +1,74 @@ +"""Boot tolerates a feature-flag store outage by falling back to defaults. + +If the DB read fails during ``on_startup`` (table missing, store unreachable), +the app must still boot — feature flags are explicitly defaulted at +``register_feature_flags`` time, and an outage that flips every flag to +"unknown" would cascade into every dependent module. The fallback is logged +so an operator can react. +""" + +from __future__ import annotations + +import logging +from types import SimpleNamespace +from unittest.mock import MagicMock + +import pytest +from feature_flags.module import FeatureFlagsModule +from simple_module_core.feature_flags import FeatureFlagDefinition, FeatureFlagRegistry + + +class _BoomService: + """Stand-in service whose ``hydrate_registry`` always blows up.""" + + async def hydrate_registry(self, registry): + raise RuntimeError("simulated store outage") + + +@pytest.mark.anyio +async def test_on_startup_swallows_store_failure_and_keeps_registry_defaults(monkeypatch, caplog): + """A DB failure during hydrate must not stop the app from booting.""" + + registry = FeatureFlagRegistry() + registry.add( + FeatureFlagDefinition( + name="my_flag", + description="anything", + default_enabled=True, + ) + ) + + class _SessionCM: + async def __aenter__(self): + return MagicMock(name="session") + + async def __aexit__(self, *args): + return None + + fake_app = MagicMock() + fake_app.state.sm = SimpleNamespace( + feature_flags=registry, + db=SimpleNamespace(session_factory=lambda: _SessionCM()), + ) + + # ``on_startup`` does ``from feature_flags.service import FeatureFlagService`` + # *inside* the method body — patching ``feature_flags.module.FeatureFlagService`` + # would be a no-op since the name is never bound there. Patch the source + # module so the deferred import resolves to ``_BoomService``. + import feature_flags.module as ff_module + import feature_flags.service as ff_service + + monkeypatch.setattr(ff_service, "FeatureFlagService", lambda *a, **kw: _BoomService()) + + module = FeatureFlagsModule() + with caplog.at_level(logging.WARNING, logger=ff_module.__name__): + await module.on_startup(fake_app) + + # 1. Booted cleanly (no exception escaped). + # 2. Registry retains its default value — the boot path didn't try to + # "blank out" the flag in lieu of unknown DB state. + assert registry.is_enabled("my_flag") is True + # 3. Operators see a warning so they can investigate the outage. + assert any("hydrate_failed" in r.message for r in caplog.records), ( + f"Expected hydrate_failed warning, got: {[r.message for r in caplog.records]!r}" + ) diff --git a/modules/file_storage/file_storage/constants.py b/modules/file_storage/file_storage/constants.py index bd56fe6c..e6d44eb0 100644 --- a/modules/file_storage/file_storage/constants.py +++ b/modules/file_storage/file_storage/constants.py @@ -20,6 +20,7 @@ MODULE_NAME: Final = "file_storage" MODULE_PASCAL: Final = "FileStorage" MODULE_DISPLAY_NAME: Final = "Files" +_MODULE_SETTINGS: Final = "Settings" # ── Configuration ──────────────────────────────────────────────────── ENV_PREFIX: Final = "SM_FILE_STORAGE_" diff --git a/modules/file_storage/file_storage/module.py b/modules/file_storage/file_storage/module.py index 12b04678..2cc7e52f 100644 --- a/modules/file_storage/file_storage/module.py +++ b/modules/file_storage/file_storage/module.py @@ -28,7 +28,7 @@ class FileStorageModule(ModuleBase): view_prefix=constants.ROUTE_PREFIX_VIEW, # Needs Settings to run first so register_module_settings can reach # app.state.settings.module_registry during register_settings. - depends_on=["Settings"], + depends_on=[constants._MODULE_SETTINGS], ) def register_settings(self, app: FastAPI) -> None: diff --git a/modules/file_storage/file_storage/service.py b/modules/file_storage/file_storage/service.py index 05cfb0af..490a7ec9 100644 --- a/modules/file_storage/file_storage/service.py +++ b/modules/file_storage/file_storage/service.py @@ -4,6 +4,7 @@ import contextlib import hashlib +import logging import uuid from collections.abc import AsyncIterator from dataclasses import dataclass @@ -25,6 +26,9 @@ from file_storage.settings import FileStorageSettings +_logger = logging.getLogger(__name__) + + class FileTooLargeError(Exception): """Raised when an upload exceeds the configured size limit.""" @@ -100,7 +104,11 @@ async def _hashing_stream() -> AsyncIterator[bytes]: size=0, # unknown until stream is drained; backends that need it can spool ) - # Compensation: on DB failure, drop the just-uploaded object. + # Compensation: on DB failure, drop the just-uploaded object. A + # cleanup-time exception must NOT replace the original failure — + # otherwise the operator chases the wrong root cause. We swallow + # the cleanup error after logging; the orphaned key can be reaped + # by a janitor sweep. try: row = StoredFile( key=key, @@ -114,7 +122,13 @@ async def _hashing_stream() -> AsyncIterator[bytes]: await self.db.flush() await self.db.refresh(row) except Exception: - await self.backend.delete(key) + try: + await self.backend.delete(key) + except Exception: + _logger.exception( + "file_storage.cleanup_failed key=%s — original upload error follows", + key, + ) raise return StoredFileOut.model_validate(_to_out_dict(row)) diff --git a/modules/file_storage/tests/test_upload_failure_modes.py b/modules/file_storage/tests/test_upload_failure_modes.py new file mode 100644 index 00000000..06c92476 --- /dev/null +++ b/modules/file_storage/tests/test_upload_failure_modes.py @@ -0,0 +1,127 @@ +"""Upload-time failure-mode coverage beyond the happy path. + +``test_service.py`` covers the DB-fail-after-backend-write compensation. +This file extends that to: + +* Backend ``put`` raising (disk full / S3 timeout) — no DB row must be left. +* Compensation ``delete`` itself failing — the original error must still + surface; we shouldn't swallow it in favour of the cleanup exception. + +Both manifest in production as orphan rows or silent data loss. +""" + +from __future__ import annotations + +from io import BytesIO + +import pytest +from fastapi import UploadFile +from file_storage import constants +from file_storage.backends.filesystem import FilesystemBackend +from file_storage.models import StoredFile +from file_storage.service import FileStorageService +from file_storage.settings import FileStorageSettings +from sqlalchemy import func, select +from sqlalchemy.ext.asyncio import AsyncSession + + +def _upload(name: str, data: bytes, content_type: str = "application/octet-stream") -> UploadFile: + return UploadFile(filename=name, file=BytesIO(data), headers={"content-type": content_type}) # type: ignore[arg-type] + + +def _settings(tmp_path, **overrides) -> FileStorageSettings: + return FileStorageSettings( + backend=constants.BackendId.FILESYSTEM, + fs_root_path=str(tmp_path), + **overrides, + ) + + +class _BackendThatFailsOnPut(FilesystemBackend): + """Stand-in for "disk full" / "S3 timeout" — ``put`` always raises.""" + + async def put(self, key, stream, *, content_type, size): # type: ignore[override] + # Drain the stream first to keep parity with what real backends do + # before they discover they can't actually persist; this exercises + # the size/hash counters in the service too. + async for _chunk in stream: + pass + raise OSError("simulated disk full") + + +class _BackendDeleteAlsoFails(FilesystemBackend): + """Persists the object, then DB fails → delete also raises. + + Tests the worst case: compensation can't clean up. We expect the + *original* RuntimeError (the trigger) to escape, not OSError. + """ + + async def delete(self, key: str) -> None: # type: ignore[override] + raise OSError("simulated delete failure during compensation") + + +@pytest.mark.anyio +async def test_backend_put_failure_leaves_no_db_row(tmp_path, db_session: AsyncSession): + """A backend ``put`` exception must not result in a stranded ``StoredFile``. + + The service can't rollback before flush — but it also shouldn't have + added the row yet. A regression that inserted the row before ``put`` + would leak it on backend failure. + """ + svc = FileStorageService(db_session, _BackendThatFailsOnPut(root=tmp_path), _settings(tmp_path)) + + with pytest.raises(OSError, match="simulated disk full"): + await svc.upload(_upload("doomed.bin", b"x")) + + count = (await db_session.execute(select(func.count()).select_from(StoredFile))).scalar_one() + assert count == 0, "StoredFile row was created despite backend failure" + + +@pytest.mark.anyio +async def test_compensation_delete_failure_does_not_mask_original_error( + tmp_path, db_session: AsyncSession, monkeypatch +): + """If the cleanup delete fails *after* the DB write fails, the user-facing + exception must be the trigger (RuntimeError), not the cleanup OSError. + + Otherwise the operator sees an OSError and chases the wrong root cause. + """ + svc = FileStorageService( + db_session, _BackendDeleteAlsoFails(root=tmp_path), _settings(tmp_path) + ) + + real_flush = db_session.flush + + async def boom_then_real(*args, **kwargs): + if not getattr(boom_then_real, "fired", False): + boom_then_real.fired = True # type: ignore[attr-defined] + raise RuntimeError("DB write failure") + return await real_flush(*args, **kwargs) + + monkeypatch.setattr(db_session, "flush", boom_then_real) + + # The service uses ``except: backend.delete(...); raise`` — Python's + # ``raise`` without an argument re-raises the original RuntimeError, even + # if ``backend.delete`` raised inside the except clause (that becomes the + # exception's ``__context__``). The caller-facing error must therefore be + # the trigger, not the cleanup failure. + with pytest.raises(RuntimeError, match="DB write failure"): + await svc.upload(_upload("doomed.bin", b"x")) + + +@pytest.mark.anyio +async def test_oversize_upload_does_not_create_db_row(tmp_path, db_session: AsyncSession): + """``FileTooLargeError`` mid-stream must not leave a partial row either.""" + svc = FileStorageService( + db_session, + FilesystemBackend(root=tmp_path), + _settings(tmp_path, max_file_size_bytes=4), + ) + + from file_storage.service import FileTooLargeError + + with pytest.raises(FileTooLargeError): + await svc.upload(_upload("too-big.bin", b"way too large")) + + count = (await db_session.execute(select(func.count()).select_from(StoredFile))).scalar_one() + assert count == 0 diff --git a/modules/permissions/permissions/constants.py b/modules/permissions/permissions/constants.py index 24cbb857..4cec1325 100644 --- a/modules/permissions/permissions/constants.py +++ b/modules/permissions/permissions/constants.py @@ -11,3 +11,7 @@ PERM_VIEW = "permissions.view" PERM_MANAGE = "permissions.manage" + +# ── Module dependencies (used by ModuleMeta.depends_on) ──────────────── +_MODULE_AUTH = "Auth" +_MODULE_USERS = "Users" diff --git a/modules/permissions/permissions/module.py b/modules/permissions/permissions/module.py index 1a0dc530..5fdaef9f 100644 --- a/modules/permissions/permissions/module.py +++ b/modules/permissions/permissions/module.py @@ -10,6 +10,8 @@ from simple_module_core.module import ModuleBase, ModuleMeta from simple_module_core.permissions import PermissionRegistry +from permissions.constants import _MODULE_AUTH, _MODULE_USERS + if TYPE_CHECKING: from fastapi import FastAPI @@ -19,7 +21,7 @@ class PermissionsModule(ModuleBase): name="Permissions", route_prefix="/api/permissions", view_prefix="/permissions", - depends_on=["Auth", "Users"], + depends_on=[_MODULE_AUTH, _MODULE_USERS], ) def register_routes(self, api_router: APIRouter, view_router: APIRouter) -> None: diff --git a/modules/settings/settings/endpoints/api.py b/modules/settings/settings/endpoints/api.py index 821d32f3..2e60cee6 100644 --- a/modules/settings/settings/endpoints/api.py +++ b/modules/settings/settings/endpoints/api.py @@ -3,6 +3,7 @@ from __future__ import annotations from fastapi import APIRouter, Depends, HTTPException, Query +from simple_module_hosting.permissions import RequiresPermission from settings.constants import ( API_BY_ID_PATH, @@ -11,6 +12,10 @@ API_TENANT_PATH, API_USER_PATH, ERR_SETTING_NOT_FOUND, + PERM_CREATE, + PERM_DELETE, + PERM_EDIT, + PERM_VIEW, QP_SCOPE, QP_SCOPE_ID, QP_TENANT_ID, @@ -32,6 +37,16 @@ router = APIRouter() +# Module permissions exist (settings.view / .create / .edit / .delete) but the +# endpoints used to be unauthenticated relative to the role map — any logged-in +# user could read or rewrite system settings (including secrets like +# ``reset_password_token_secret``). Each route now gates on the relevant +# permission via the wildcard map. +_VIEW = [Depends(RequiresPermission(PERM_VIEW))] +_CREATE = [Depends(RequiresPermission(PERM_CREATE))] +_EDIT = [Depends(RequiresPermission(PERM_EDIT))] +_DELETE = [Depends(RequiresPermission(PERM_DELETE))] + def _not_found() -> HTTPException: return HTTPException(status_code=STATUS_NOT_FOUND, detail=ERR_SETTING_NOT_FOUND) @@ -40,7 +55,7 @@ def _not_found() -> HTTPException: # ── List / filter ─────────────────────────────────────────────────── -@router.get("/", response_model=list[SettingOut]) +@router.get("/", response_model=list[SettingOut], dependencies=_VIEW) async def list_settings( scope: SettingScope | None = Query(default=None, alias=QP_SCOPE), scope_id: str = Query(default=SYSTEM_SCOPE_ID, alias=QP_SCOPE_ID), @@ -54,7 +69,7 @@ async def list_settings( # ── Resolution (USER > TENANT > SYSTEM) ───────────────────────────── -@router.get(API_RESOLVE_PATH, response_model=SettingOut) +@router.get(API_RESOLVE_PATH, response_model=SettingOut, dependencies=_VIEW) async def resolve_setting( key: str, user_id: str | None = Query(default=None, alias=QP_USER_ID), @@ -70,7 +85,7 @@ async def resolve_setting( # ── Scoped (system / tenant / user) ───────────────────────────────── -@router.get(API_SYSTEM_PATH, response_model=SettingOut) +@router.get(API_SYSTEM_PATH, response_model=SettingOut, dependencies=_VIEW) async def get_system_setting( key: str, service: SettingService = Depends(get_setting_service) ) -> SettingOut: @@ -80,7 +95,7 @@ async def get_system_setting( return result -@router.put(API_SYSTEM_PATH, response_model=SettingOut) +@router.put(API_SYSTEM_PATH, response_model=SettingOut, dependencies=_EDIT) async def upsert_system_setting( key: str, data: SettingUpsert, @@ -89,7 +104,7 @@ async def upsert_system_setting( return await service.upsert_scoped(SettingScope.SYSTEM, SYSTEM_SCOPE_ID, key, data) -@router.delete(API_SYSTEM_PATH, status_code=STATUS_NO_CONTENT) +@router.delete(API_SYSTEM_PATH, status_code=STATUS_NO_CONTENT, dependencies=_DELETE) async def delete_system_setting( key: str, service: SettingService = Depends(get_setting_service) ) -> None: @@ -97,7 +112,7 @@ async def delete_system_setting( raise _not_found() -@router.get(API_TENANT_PATH, response_model=SettingOut) +@router.get(API_TENANT_PATH, response_model=SettingOut, dependencies=_VIEW) async def get_tenant_setting( scope_id: str, key: str, @@ -109,7 +124,7 @@ async def get_tenant_setting( return result -@router.put(API_TENANT_PATH, response_model=SettingOut) +@router.put(API_TENANT_PATH, response_model=SettingOut, dependencies=_EDIT) async def upsert_tenant_setting( scope_id: str, key: str, @@ -119,7 +134,7 @@ async def upsert_tenant_setting( return await service.upsert_scoped(SettingScope.TENANT, scope_id, key, data) -@router.delete(API_TENANT_PATH, status_code=STATUS_NO_CONTENT) +@router.delete(API_TENANT_PATH, status_code=STATUS_NO_CONTENT, dependencies=_DELETE) async def delete_tenant_setting( scope_id: str, key: str, @@ -129,7 +144,7 @@ async def delete_tenant_setting( raise _not_found() -@router.get(API_USER_PATH, response_model=SettingOut) +@router.get(API_USER_PATH, response_model=SettingOut, dependencies=_VIEW) async def get_user_setting( scope_id: str, key: str, @@ -141,7 +156,7 @@ async def get_user_setting( return result -@router.put(API_USER_PATH, response_model=SettingOut) +@router.put(API_USER_PATH, response_model=SettingOut, dependencies=_EDIT) async def upsert_user_setting( scope_id: str, key: str, @@ -151,7 +166,7 @@ async def upsert_user_setting( return await service.upsert_scoped(SettingScope.USER, scope_id, key, data) -@router.delete(API_USER_PATH, status_code=STATUS_NO_CONTENT) +@router.delete(API_USER_PATH, status_code=STATUS_NO_CONTENT, dependencies=_DELETE) async def delete_user_setting( scope_id: str, key: str, @@ -164,7 +179,7 @@ async def delete_user_setting( # ── Id-based CRUD (admin tooling) ─────────────────────────────────── -@router.post("/", response_model=SettingOut, status_code=STATUS_CREATED) +@router.post("/", response_model=SettingOut, status_code=STATUS_CREATED, dependencies=_CREATE) async def create_setting( data: SettingCreate, service: SettingService = Depends(get_setting_service), @@ -172,7 +187,7 @@ async def create_setting( return await service.create(data) -@router.get(API_BY_ID_PATH, response_model=SettingOut) +@router.get(API_BY_ID_PATH, response_model=SettingOut, dependencies=_VIEW) async def get_setting( setting_id: int, service: SettingService = Depends(get_setting_service) ) -> SettingOut: @@ -182,7 +197,7 @@ async def get_setting( return result -@router.put(API_BY_ID_PATH, response_model=SettingOut) +@router.put(API_BY_ID_PATH, response_model=SettingOut, dependencies=_EDIT) async def update_setting( setting_id: int, data: SettingUpdate, @@ -194,7 +209,7 @@ async def update_setting( return result -@router.delete(API_BY_ID_PATH, status_code=STATUS_NO_CONTENT) +@router.delete(API_BY_ID_PATH, status_code=STATUS_NO_CONTENT, dependencies=_DELETE) async def delete_setting( setting_id: int, service: SettingService = Depends(get_setting_service) ) -> None: diff --git a/modules/settings/settings/endpoints/module_api.py b/modules/settings/settings/endpoints/module_api.py index 4ba88c00..108f3759 100644 --- a/modules/settings/settings/endpoints/module_api.py +++ b/modules/settings/settings/endpoints/module_api.py @@ -11,6 +11,7 @@ from fastapi import APIRouter, Depends, HTTPException, Request, Response, status from pydantic import ValidationError +from simple_module_hosting.permissions import RequiresPermission from settings._module_settings import ( SECRET_MASK, @@ -18,7 +19,7 @@ is_secret_field, serialize, ) -from settings.constants import MODULE_PACKAGE +from settings.constants import MODULE_PACKAGE, PERM_DELETE, PERM_EDIT, PERM_VIEW from settings.contracts.events import SettingsReloaded from settings.deps import get_setting_service from settings.hydrate import hydrate_settings @@ -28,6 +29,13 @@ router = APIRouter(prefix="/modules", tags=["Settings Modules"]) +# Per-module settings UI exposes raw secret values (mailer password, JWT +# signing keys, etc.) — every endpoint here is gated on the same permissions +# the scoped API uses so a non-admin can't read or mutate module config. +_VIEW = [Depends(RequiresPermission(PERM_VIEW))] +_EDIT = [Depends(RequiresPermission(PERM_EDIT))] +_DELETE = [Depends(RequiresPermission(PERM_DELETE))] + def _strip_mask_sentinels(changes: dict[str, Any]) -> dict[str, Any]: """Drop secret fields whose value is the UI mask sentinel.""" @@ -38,13 +46,13 @@ def _strip_mask_sentinels(changes: dict[str, Any]) -> dict[str, Any]: } -@router.get("") +@router.get("", dependencies=_VIEW) async def list_modules(request: Request) -> dict[str, Any]: views = collect_module_settings(request.app) return {"modules": serialize(views)} -@router.put("/{package}") +@router.put("/{package}", dependencies=_EDIT) async def update_module( package: str, changes: dict[str, Any], @@ -75,7 +83,7 @@ async def update_module( return {"ok": True, "changed": sorted(cleaned)} -@router.delete("/{package}/{field}", status_code=status.HTTP_204_NO_CONTENT) +@router.delete("/{package}/{field}", status_code=status.HTTP_204_NO_CONTENT, dependencies=_DELETE) async def clear_module_field( package: str, field: str, diff --git a/modules/settings/settings/endpoints/views.py b/modules/settings/settings/endpoints/views.py index 21e0d237..4de333f9 100644 --- a/modules/settings/settings/endpoints/views.py +++ b/modules/settings/settings/endpoints/views.py @@ -1,11 +1,10 @@ """Inertia view endpoints for the Settings module. -Page component identifiers are inlined as string literals here (instead of -imported from ``settings.constants``) so the ``SM003`` orphan-page doctor -check — which parses calls to ``inertia.render`` via AST literal matching — -can correlate views to their ``pages/*.tsx`` files. The literals must match -``PAGE_BROWSE`` / ``PAGE_CREATE`` / ``PAGE_EDIT`` in ``constants.py``, and -a test in ``test_settings_module.py`` enforces that invariant. +Page component identifiers are file-private module constants — same pattern +the dashboard/users/feature_flags views use. The diagnostic ``SM003`` +matches the resolved literal value, not the source-level expression, so a +named constant is fine; ``test_settings_module.py`` cross-checks that the +strings below match what ``pages/*.tsx`` declare. """ from __future__ import annotations @@ -32,6 +31,11 @@ from settings.deps import get_setting_service from settings.service import SettingService +_PAGE_BROWSE = "Settings/Browse" +_PAGE_CREATE = "Settings/Create" +_PAGE_EDIT = "Settings/Edit" +_PAGE_MODULES_EDIT = "Settings/ModulesEdit" + _REDIRECT_SETTINGS = "/settings" router = APIRouter() @@ -44,14 +48,14 @@ async def browse( ) -> InertiaResponse: items = await service.list_all() return await inertia.render( - "Settings/Browse", + _PAGE_BROWSE, {PROP_SETTINGS: [item.model_dump(mode="json") for item in items]}, ) @router.get(VIEW_CREATE_PATH, response_model=None) async def create_view(inertia: InertiaDep) -> InertiaResponse: - return await inertia.render("Settings/Create") + return await inertia.render(_PAGE_CREATE) @router.get(VIEW_EDIT_PATH, response_model=None) @@ -62,8 +66,8 @@ async def edit_view( ) -> InertiaResponse: item = await service.get_by_id(setting_id) if item is None: - return await inertia.render("Settings/Browse", {PROP_ERROR: ERR_SETTING_NOT_FOUND}) - return await inertia.render("Settings/Edit", {PROP_SETTING: item.model_dump(mode="json")}) + return await inertia.render(_PAGE_BROWSE, {PROP_ERROR: ERR_SETTING_NOT_FOUND}) + return await inertia.render(_PAGE_EDIT, {PROP_SETTING: item.model_dump(mode="json")}) # ── Form actions (POST/PUT/DELETE → redirect) ───────────────── @@ -115,6 +119,6 @@ async def modules_view(request: Request, inertia: InertiaDep) -> InertiaResponse """ views = collect_module_settings(request.app) return await inertia.render( - "Settings/ModulesEdit", + _PAGE_MODULES_EDIT, {PROP_MODULES: serialize(views)}, ) diff --git a/modules/settings/tests/test_concurrent_overrides.py b/modules/settings/tests/test_concurrent_overrides.py new file mode 100644 index 00000000..22644023 --- /dev/null +++ b/modules/settings/tests/test_concurrent_overrides.py @@ -0,0 +1,85 @@ +"""Override write semantics under repeated / interleaved operations. + +The audit flagged "concurrent updates" as untested. True concurrency on +SQLite is degenerate (one writer at a time, no row-level locks worth +exercising), so this file pins the next-best contract: last-write-wins on +repeated upserts, and set/clear interleaving converges to one consistent +end state. Both are properties Postgres satisfies as well. +""" + +from __future__ import annotations + +import pytest +from settings.service import SettingService +from settings.store import SettingsStore + + +@pytest.mark.anyio +async def test_repeated_upserts_last_write_wins(db_session): + """100 sequential upserts must end with the final value, no torn writes. + + Earlier versions of ``upsert_scoped`` could fall back to an insert-then- + catch-conflict path if the existing-row lookup raced with a parallel + deletion; the regression manifested as a UNIQUE constraint error. We + can't fully simulate the race on SQLite, but iterated sequential calls + exercise the same insert-vs-update branch repeatedly and catches any + state pollution between calls. + """ + store = SettingsStore(SettingService(db_session)) + + # The insert-vs-update branch stabilises after 2 iterations — a handful + # is plenty to exercise the branch repeatedly without paying for 100 + # synchronous aiosqlite commits per test run. + for i in range(5): + await store.set_override("users", "base_url", f"value_{i}", "string") + await db_session.commit() + + overrides = await store.get_overrides("users") + raw_value, value_type = overrides["base_url"] + assert raw_value == "value_4" + assert value_type == "string" + + +@pytest.mark.anyio +async def test_set_then_clear_then_set_converges(db_session): + """set → clear → set leaves the latest value present, not the earlier one.""" + store = SettingsStore(SettingService(db_session)) + + await store.set_override("users", "base_url", "first", "string") + await db_session.commit() + await store.clear_override("users", "base_url") + await db_session.commit() + await store.set_override("users", "base_url", "second", "string") + await db_session.commit() + + overrides = await store.get_overrides("users") + raw_value, _ = overrides["base_url"] + assert raw_value == "second" + + +@pytest.mark.anyio +async def test_clear_of_nonexistent_is_idempotent(db_session): + """Clearing a never-set field must not raise — common during reload code paths.""" + store = SettingsStore(SettingService(db_session)) + await store.clear_override("users", "nonexistent_field") + await db_session.commit() # no rows affected, no exception expected + overrides = await store.get_overrides("users") + assert "nonexistent_field" not in overrides + + +@pytest.mark.anyio +async def test_different_packages_do_not_collide(db_session): + """``users.base_url`` and ``feature_flags.base_url`` are independent keys. + + A regression that hashed only on ``field`` (not ``package.field``) would + let one module's setting overwrite another's. + """ + store = SettingsStore(SettingService(db_session)) + await store.set_override("users", "base_url", "users-value", "string") + await store.set_override("feature_flags", "base_url", "ff-value", "string") + await db_session.commit() + + users_overrides = await store.get_overrides("users") + ff_overrides = await store.get_overrides("feature_flags") + assert users_overrides["base_url"][0] == "users-value" + assert ff_overrides["base_url"][0] == "ff-value" diff --git a/modules/users/tests/conftest.py b/modules/users/tests/conftest.py index 733290a1..69c837a6 100644 --- a/modules/users/tests/conftest.py +++ b/modules/users/tests/conftest.py @@ -202,6 +202,26 @@ async def _make_admin_user(app): return user +async def _make_standard_user(app, email: str = "user@example.com"): + """Seed a non-admin User with the standard ``user`` role. + + Used by the negative-authz tests to confirm endpoints protected by + ``RequiresPermission(...)`` reject authenticated-but-non-admin callers. + """ + from users.bootstrap import create_standard_user + from users.models import User + + async with app.state.sm.db.session_factory() as session: + result = await create_standard_user( + session, + email=email, + password="UserPass1!", + full_name="Regular User", + ) + user: User = result.user + return user + + @pytest.fixture async def admin_client(users_app) -> AsyncGenerator[httpx.AsyncClient, None]: """Client with a signed local-user session cookie (admin role).""" @@ -219,6 +239,28 @@ async def admin_client(users_app) -> AsyncGenerator[httpx.AsyncClient, None]: yield c +@pytest.fixture +async def user_client(users_app) -> AsyncGenerator[httpx.AsyncClient, None]: + """Client with a signed session cookie for an authenticated non-admin user. + + Counterpart to ``admin_client`` — every endpoint behind + ``RequiresPermission(...)`` should answer with 403 for this caller, since + the default role map only grants the wildcard to ``admin``. + """ + user = await _make_standard_user(users_app) + cookie = forge_session_cookie( + str(users_app.state.sm.settings.secret_key), + {"user_id": str(user.id)}, + ) + transport = httpx.ASGITransport(app=users_app) + async with httpx.AsyncClient( + transport=transport, + base_url="http://testserver", + cookies={"session": cookie}, + ) as c: + yield c + + # --------------------------------------------------------------------------- # DB session fixture scoped to users_app # --------------------------------------------------------------------------- diff --git a/modules/users/tests/test_invite_reuse.py b/modules/users/tests/test_invite_reuse.py new file mode 100644 index 00000000..df4eed58 --- /dev/null +++ b/modules/users/tests/test_invite_reuse.py @@ -0,0 +1,135 @@ +"""Invite + password-reset token-lifecycle regressions. + +The existing ``test_invite_flow`` covers the golden path and rejects a junk +token; these tests add: + +* Reusing a verified invite token must be rejected (single-use). +* Generating a reset link for a user produces a token that survives one + consume cycle and is rejected after the password has been changed (the + token's hash incorporates ``user.hashed_password``). +* Acceptance for an already-disabled user does not log them in. + +If any of these regressed, a stolen invite/reset link could be reused +arbitrarily — exactly the scenario the audit flagged. +""" + +from __future__ import annotations + +import logging + +import pytest + + +async def _send_invite_and_grab_token(admin_client, anon_client, caplog, email: str) -> str: + """Issue an invite and pull the token out of the ConsoleMailer log line.""" + with caplog.at_level(logging.INFO, logger="users.mailer"): + resp = await admin_client.post( + "/api/users/admin/invite", + json={"email": email, "role_names": ["user"]}, + ) + assert resp.status_code == 201, resp.text + records = [r for r in caplog.records if r.getMessage() == "users.invite.email"] + assert records, "ConsoleMailer didn't log an invite.email record" + link = records[-1].link # type: ignore[attr-defined] + return link.split("token=", 1)[1] + + +@pytest.mark.anyio +async def test_invite_token_is_single_use(admin_client, anon_client, caplog): + """Accepting the same invite token twice must fail the second time. + + Verify tokens in fastapi-users flip ``is_verified`` on the user; the + re-use attempt raises ``UserAlreadyVerified``, which the endpoint maps + to 400 INVITE_BAD_TOKEN. A regression that re-issued the same JWT or + forgot to re-check state would let a stolen link be replayed. + """ + token = await _send_invite_and_grab_token( + admin_client, anon_client, caplog, "single@example.com" + ) + + first = await anon_client.post( + "/api/users/auth/accept-invite", + json={"token": token, "password": "FirstUseSecret1!"}, + ) + assert first.status_code == 204, first.text + + # Same token, same user, but already verified. + second = await anon_client.post( + "/api/users/auth/accept-invite", + json={"token": token, "password": "DifferentSecret2!"}, + ) + assert second.status_code == 400, second.text + assert second.json()["detail"] == "INVITE_BAD_TOKEN" + + +@pytest.mark.anyio +async def test_reset_token_invalidated_after_password_change(admin_client, anon_client, caplog): + """A reset-password token must no longer work once the user's hash changes. + + fastapi-users binds reset tokens to ``user.hashed_password`` so any change + (including the reset itself, or a manual password update) revokes every + outstanding reset token. Without this, a leaked link would stay live + forever. + """ + # Step 1: create a user via invite, log them in with a known password. + token = await _send_invite_and_grab_token( + admin_client, anon_client, caplog, "resetme@example.com" + ) + await anon_client.post( + "/api/users/auth/accept-invite", + json={"token": token, "password": "InitialPass1!"}, + ) + + # Step 2: admin mints a reset link. + listing = await admin_client.get("/api/users/admin") + target = next(u for u in listing.json() if u["email"] == "resetme@example.com") + reset = await admin_client.post(f"/api/users/admin/{target['id']}/reset-password-link") + assert reset.status_code == 200 + reset_token = reset.json()["link"].split("token=", 1)[1] + + # Step 3: user changes password via that token (consumes it). + used = await anon_client.post( + "/api/users/auth/reset-password", + json={"token": reset_token, "password": "PostResetPass1!"}, + ) + assert used.status_code in (200, 204), used.text + + # Step 4: reuse the SAME reset token after password rotated — must fail. + replay = await anon_client.post( + "/api/users/auth/reset-password", + json={"token": reset_token, "password": "ReplayedPass1!"}, + ) + assert replay.status_code in (400, 401), ( + f"Replayed reset token returned {replay.status_code}, expected 4xx. Body: {replay.text!r}" + ) + + +@pytest.mark.anyio +async def test_disabled_user_cannot_accept_their_invite(admin_client, anon_client, caplog): + """Inviting + disabling before acceptance must keep the user out.""" + token = await _send_invite_and_grab_token( + admin_client, anon_client, caplog, "blocked@example.com" + ) + + # Admin disables the freshly-invited user before they accept. + listing = await admin_client.get("/api/users/admin") + target = next(u for u in listing.json() if u["email"] == "blocked@example.com") + disable = await admin_client.patch(f"/api/users/admin/{target['id']}/disable") + assert disable.status_code == 200 + assert disable.json()["is_active"] is False + + # Token verifies the email but the user is inactive — fastapi-users' + # subsequent login step (or the session middleware's user-load) must + # refuse to issue a valid session. + resp = await anon_client.post( + "/api/users/auth/accept-invite", + json={"token": token, "password": "ShouldNotMatter1!"}, + ) + # Either the verify path itself refuses, or the subsequent login does; + # in both cases the user must not be authenticated afterwards. Probe + # /me with the post-response cookies to confirm. + me = await anon_client.get("/api/users/me", follow_redirects=False) + assert me.status_code in (302, 401), ( + f"Disabled user appears authenticated after accept-invite " + f"(status={resp.status_code}, /me status={me.status_code})" + ) diff --git a/modules/users/tests/test_negative_authz.py b/modules/users/tests/test_negative_authz.py new file mode 100644 index 00000000..76c22100 --- /dev/null +++ b/modules/users/tests/test_negative_authz.py @@ -0,0 +1,84 @@ +"""Negative-authorization sweep across every endpoint behind RequiresPermission. + +Every endpoint guarded by ``RequiresPermission(...)`` must answer 403 when the +caller is authenticated but not an admin. The decorator presence alone isn't +enough — even one missing ``Depends(...)`` would leak admin-only data to +ordinary users. + +The matrix below is exhaustive across the modules that ship with the framework +(users, permissions, settings, feature_flags, background_tasks, file_storage). +A new protected endpoint should be added here at the same time as it gains its +``RequiresPermission`` dependency. +""" + +from __future__ import annotations + +import uuid + +import pytest + +_FAKE_ID = uuid.uuid4() +_PROTECTED_ENDPOINTS: tuple[tuple[str, str, dict | None], ...] = ( + # users — admin sub-router + ("GET", "/api/users/admin", None), + ("POST", "/api/users/admin/invite", {"email": "x@y.test", "role_names": []}), + ("PATCH", f"/api/users/admin/{_FAKE_ID}/disable", None), + ("PATCH", f"/api/users/admin/{_FAKE_ID}/enable", None), + ("PUT", f"/api/users/admin/{_FAKE_ID}/roles", {"role_names": []}), + ("PATCH", f"/api/users/admin/{_FAKE_ID}/verify", None), + ("POST", f"/api/users/admin/{_FAKE_ID}/reset-password-link", None), + # permissions — root GET lists registered groups (PERM_VIEW) + ("GET", "/api/permissions/", None), + ("GET", f"/api/permissions/roles/{_FAKE_ID}", None), + ("PUT", f"/api/permissions/roles/{_FAKE_ID}", {"permissions": []}), + ("GET", f"/api/permissions/users/{_FAKE_ID}", None), + ("PUT", f"/api/permissions/users/{_FAKE_ID}", {"permissions": []}), + # settings — both the scoped CRUD and the module-config endpoints + ("GET", "/api/settings/", None), + ("POST", "/api/settings/", {"key": "x", "value": "1", "value_type": "string"}), + ("PUT", "/api/settings/system/anykey", {"value": "1", "value_type": "string"}), + ("DELETE", "/api/settings/system/anykey", None), + ("GET", "/api/settings/modules", None), + ("PUT", "/api/settings/modules/users", {}), + ("DELETE", "/api/settings/modules/users/allow_signup", None), + # feature flags + ("GET", "/api/feature_flags/", None), + ("PUT", "/api/feature_flags/anyflag", {"enabled": True}), + ("DELETE", "/api/feature_flags/anyflag", None), + # background tasks (admin router under /admin) + ("GET", "/api/background_tasks/admin/executions", None), + ("GET", "/api/background_tasks/admin/workers", None), + ("POST", f"/api/background_tasks/admin/executions/{_FAKE_ID}/retry", None), + # file_storage's list/upload/download/delete are deliberately granted to + # the standard `user` role, so they're NOT in the negative-authz matrix. +) + + +@pytest.mark.anyio +@pytest.mark.parametrize(("method", "path", "json_body"), _PROTECTED_ENDPOINTS) +async def test_protected_endpoint_rejects_non_admin( + user_client, method: str, path: str, json_body: dict | None +) -> None: + """A logged-in non-admin user must be answered 403 by every protected route. + + A regression here means the ``Depends(RequiresPermission(...))`` was dropped + or a non-admin role gained a wildcard mapping it shouldn't have. + """ + resp = await user_client.request(method, path, json=json_body, follow_redirects=False) + assert resp.status_code == 403, ( + f"{method} {path} returned {resp.status_code}, " + f"expected 403 for non-admin caller (body: {resp.text!r})" + ) + + +@pytest.mark.anyio +async def test_admin_endpoints_still_pass_for_admin(admin_client) -> None: + """Sanity check: the same routes work for an admin caller. + + Without this anchor a global regression that returned 403 for everyone + would still satisfy the parametrized 403 assertion above. + """ + resp = await admin_client.get("/api/users/admin") + assert resp.status_code == 200 + resp = await admin_client.get("/api/feature_flags/") + assert resp.status_code == 200 diff --git a/packages/ui/src/components/SectionTitle.test.tsx b/packages/ui/src/components/SectionTitle.test.tsx new file mode 100644 index 00000000..df4157bb --- /dev/null +++ b/packages/ui/src/components/SectionTitle.test.tsx @@ -0,0 +1,31 @@ +import { render, screen } from '@testing-library/react'; +import { describe, expect, test } from 'vitest'; + +import { SectionTitle } from './SectionTitle'; + +describe('SectionTitle', () => { + test('renders heading text', () => { + render(Users); + expect(screen.getByText('Users')).toBeInTheDocument(); + }); + + test('uses h3 by default', () => { + render(Users); + expect(screen.getByRole('heading', { level: 3 })).toBeInTheDocument(); + }); + + test('honors as="h2" override', () => { + render(Reports); + expect(screen.getByRole('heading', { level: 2 })).toBeInTheDocument(); + }); + + test('shows optional description', () => { + render(Reports); + expect(screen.getByText(/lifetime totals/i)).toBeInTheDocument(); + }); + + test('places right-slot content', () => { + render(Add}>Reports); + expect(screen.getByRole('button', { name: 'Add' })).toBeInTheDocument(); + }); +}); diff --git a/packages/ui/src/components/StatCard.test.tsx b/packages/ui/src/components/StatCard.test.tsx new file mode 100644 index 00000000..4bd37a2f --- /dev/null +++ b/packages/ui/src/components/StatCard.test.tsx @@ -0,0 +1,23 @@ +import { render, screen } from '@testing-library/react'; +import { Activity } from 'lucide-react'; +import { describe, expect, test } from 'vitest'; + +import { StatCard } from './StatCard'; + +describe('StatCard', () => { + test('renders label and value', () => { + render(); + expect(screen.getByText('Total Users')).toBeInTheDocument(); + expect(screen.getByText('42')).toBeInTheDocument(); + }); + + test('shows optional delta badge', () => { + render(); + expect(screen.getByText('+1')).toBeInTheDocument(); + }); + + test('omits delta when not provided', () => { + render(); + expect(screen.queryByRole('status')).not.toBeInTheDocument(); + }); +}); diff --git a/scripts/check_hardcoded_strings.py b/scripts/check_hardcoded_strings.py index 06773d2c..a864e319 100644 --- a/scripts/check_hardcoded_strings.py +++ b/scripts/check_hardcoded_strings.py @@ -17,11 +17,10 @@ from __future__ import annotations import argparse -import io +import ast import re import subprocess import sys -import tokenize from collections.abc import Iterable, Sequence from pathlib import Path @@ -59,16 +58,40 @@ def _should_skip(path: Path, root: Path) -> bool: return any(part in rel for part in _SKIP_PATH_PARTS) -def _string_literal_lines(source: str) -> set[int]: - """Return line numbers that are part of a string/docstring token.""" +def _docstring_lines(source: str) -> set[int]: + """Return line numbers that are module/class/function docstrings. + + The earlier implementation flagged *every* line that contained a string + token, which silently disabled the rules: a real ``RequiresPermission( + "users.manage")`` call always has a string on the same line as the + function name, so the line was excluded. We only want to skip docstring + bodies — strings that sit on their own as the first statement of a + module, class, or function. + """ inside: set[int] = set() try: - tokens = tokenize.generate_tokens(io.StringIO(source).readline) - for tok_type, _, (start_row, _), (end_row, _), _ in tokens: - if tok_type == tokenize.STRING: - inside.update(range(start_row, end_row + 1)) - except tokenize.TokenError: - pass + tree = ast.parse(source) + except SyntaxError: + return inside + + def _maybe_record(node: ast.AST) -> None: + body = getattr(node, "body", None) + if not body: + return + first = body[0] + if not isinstance(first, ast.Expr): + return + value = first.value + if isinstance(value, ast.Constant) and isinstance(value.value, str): + end = getattr(first, "end_lineno", first.lineno) + inside.update(range(first.lineno, end + 1)) + + for node in ast.walk(tree): + if isinstance( + node, + (ast.Module, ast.ClassDef, ast.FunctionDef, ast.AsyncFunctionDef), + ): + _maybe_record(node) return inside @@ -79,10 +102,10 @@ def _check_file(path: Path) -> list[tuple[int, str, str]]: source = path.read_text(encoding="utf-8", errors="replace") except OSError: return violations - string_lines = _string_literal_lines(source) + docstring_lines = _docstring_lines(source) for lineno, line in enumerate(source.splitlines(), start=1): stripped = line.strip() - if stripped.startswith("#") or lineno in string_lines: + if stripped.startswith("#") or lineno in docstring_lines: continue for pattern, message in _RULES: if pattern.search(line): diff --git a/scripts/tests/test_check_hardcoded_strings.py b/scripts/tests/test_check_hardcoded_strings.py new file mode 100644 index 00000000..3ce8c33a --- /dev/null +++ b/scripts/tests/test_check_hardcoded_strings.py @@ -0,0 +1,112 @@ +"""Unit tests for the magic-string lint script. + +The script's regex set is the only thing keeping inline permission strings, +role names, and Inertia page identifiers out of the codebase. A typo in any +rule would silently disable that check; these tests pin each rule. +""" + +from __future__ import annotations + +import sys +from pathlib import Path + +# Make scripts/ importable for direct script-under-test access. +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from check_hardcoded_strings import _check_file, find_violations + + +def _write(tmp_path: Path, name: str, body: str) -> Path: + p = tmp_path / name + p.write_text(body, encoding="utf-8") + return p + + +class TestRules: + def test_flags_inline_requires_permission(self, tmp_path): + path = _write( + tmp_path, + "thing.py", + 'from fastapi import Depends\nDepends(RequiresPermission("users.manage"))\n', + ) + viols = _check_file(path) + assert len(viols) == 1 + assert "RequiresPermission" in viols[0][2] + + def test_flags_inline_map_role(self, tmp_path): + path = _write( + tmp_path, + "thing.py", + 'registry.map_role("user", [])\n', + ) + viols = _check_file(path) + assert len(viols) == 1 + assert "map_role" in viols[0][2] + + def test_flags_inline_add_group_permission(self, tmp_path): + path = _write( + tmp_path, + "thing.py", + 'registry.add_group("group", ["users.view"])\n', + ) + viols = _check_file(path) + assert len(viols) == 1 + assert "add_group" in viols[0][2] + + def test_flags_inline_inertia_render_page(self, tmp_path): + path = _write( + tmp_path, + "thing.py", + 'return inertia.render("Users/Login")\n', + ) + viols = _check_file(path) + assert len(viols) == 1 + assert "_PAGE_" in viols[0][2] + + def test_flags_inline_depends_on_module(self, tmp_path): + path = _write( + tmp_path, + "thing.py", + 'meta = Meta(name="x", depends_on=["Users"])\n', + ) + viols = _check_file(path) + assert len(viols) == 1 + assert "_MODULE_" in viols[0][2] + + def test_string_literal_in_docstring_is_ignored(self, tmp_path): + """A docstring mentioning ``RequiresPermission("x.y")`` is not a real call.""" + path = _write( + tmp_path, + "thing.py", + '"""Example: RequiresPermission(\\"users.manage\\")"""\n', + ) + viols = _check_file(path) + assert viols == [] + + def test_constant_use_is_clean(self, tmp_path): + """A canonical constant-based usage produces no violations.""" + path = _write( + tmp_path, + "thing.py", + "from .constants import PERM_USERS_MANAGE\n" + "Depends(RequiresPermission(PERM_USERS_MANAGE))\n", + ) + viols = _check_file(path) + assert viols == [] + + +class TestPathSkipping: + def test_tests_directory_is_skipped(self, tmp_path): + # Build a fake layout: foo/tests/test_x.py with a deliberate violation. + (tmp_path / "foo" / "tests").mkdir(parents=True) + path = tmp_path / "foo" / "tests" / "test_x.py" + path.write_text('Depends(RequiresPermission("users.manage"))\n', encoding="utf-8") + results = find_violations([path], tmp_path) + assert results == [] + + def test_constants_file_is_skipped(self, tmp_path): + (tmp_path / "foo").mkdir() + path = tmp_path / "foo" / "constants.py" + path.write_text('Depends(RequiresPermission("users.manage"))\n', encoding="utf-8") + results = find_violations([path], tmp_path) + assert results == []