diff --git a/umbratrans_web/compute.py b/umbratrans_web/compute.py index ca2b683..2cb9a8b 100644 --- a/umbratrans_web/compute.py +++ b/umbratrans_web/compute.py @@ -33,9 +33,11 @@ from typing import Any, Mapping, Sequence from uuid import UUID, uuid4 import psycopg +from psycopg import sql from .errors import ConflictError, InputError, NotFoundError, RepositoryError from .security import PasswordService, generate_token +from .tenancy import tenant_schema_name LOGGER = logging.getLogger(__name__) @@ -1184,6 +1186,46 @@ class ComputeRepository: # -- the queue --------------------------------------------------------- + def _require_project_in_tenant( + self, connection, organization_id: UUID, project_id: UUID + ) -> None: + """Refuse a job naming a project this organization does not have. + + `jobs` and `projects` now live in different schemas, so `jobs.project_id` cannot + have a foreign key - a cross-schema reference needs `postgres_fdw`. That FK was + the only thing stopping a job from naming a project belonging to a different + tenant, and migration 0005 dropped it. The check that was free in the database + has to be made here, in the layer that knows the tenant. + + A real gap, recorded rather than papered over: the same applies to + `artifacts.project_id`, whose FK also went. + + There is no `organization_id` predicate because after migration 0005 `projects` + has no such column: the schema it sits in *is* the tenant. So this resolves in + the caller's schema, and another tenant's project is not filtered out - it is + simply not there. That is the property the split was chosen for. + """ + + # Qualified explicitly rather than relying on `search_path`. The account + # repository sets it per transaction; the compute repository does not, and + # borrowing that behaviour here would mean one more place where ambient state + # decides whether a security check runs at all. Deriving the schema from the + # organization id makes the check self-contained: it reads the caller's tenant or + # it reads nothing. + schema = tenant_schema_name(organization_id) + found = connection.execute( + sql.SQL("SELECT id FROM {}.{} WHERE id = {}").format( + sql.Identifier(schema), sql.Identifier("projects"), sql.Placeholder() + ), + (project_id,), + ).fetchone() + if found is None: + raise NotFoundError( + "project_not_in_organization", + f"Project {project_id} does not exist in this organization.", + ) + + def submit_job( self, organization_id: UUID, @@ -1207,6 +1249,8 @@ class ComputeRepository: requirement = requires or ModelRequirement() with self._connection() as connection, self._tx(connection): + if project_id is not None: + self._require_project_in_tenant(connection, organization_id, project_id) if idempotency_key: existing = connection.execute( "SELECT * FROM public.jobs WHERE idempotency_key = %s", (idempotency_key,) diff --git a/umbratrans_web/tests/test_job_project_tenancy.py b/umbratrans_web/tests/test_job_project_tenancy.py new file mode 100644 index 0000000..f006ecb --- /dev/null +++ b/umbratrans_web/tests/test_job_project_tenancy.py @@ -0,0 +1,171 @@ +"""A job may only name a project its own organization has. + +Migration 0005 moved `projects` into a per-tenant schema and left `jobs` in `public`. That +split forced a choice about `jobs.project_id`: a foreign key cannot cross a schema +boundary without `postgres_fdw`, so `artifacts_project_id_fkey` and +`jobs_project_id_fkey` were **dropped**. + +That FK was the only thing preventing a job from naming a project belonging to a +different tenant. Dropping it opened a cross-tenant hole, so the guarantee it provided is +now asserted in the layer that knows the tenant: `submit_job` looks the project up +unqualified, which resolves inside the caller's schema, and refuses the job if it is not +there. + +These tests assert the refusal rather than the presence of a query, because the failure +mode this file guards against is a check that reads as passing because its mechanism did +nothing - and "the lookup is in the code" is exactly such a check. + +Requires a disposable database. +""" + +from __future__ import annotations + +import os +import uuid + +import pytest + +from umbratrans_web.compute import ModelRequirement +from umbratrans_web.errors import NotFoundError + +_SCRATCH = os.environ.get("UMBRATRANS_TEST_DSN", "").strip() +_DISPOSABLE = os.environ.get("UMBRATRANS_TEST_DATABASE_IS_DISPOSABLE") == "1" + +pytestmark = pytest.mark.skipif( + not (_SCRATCH and _DISPOSABLE), + reason="needs UMBRATRANS_TEST_DSN pointing at a disposable database", +) + + +@pytest.fixture +def two_tenants(): + """Two organizations, each with a project, and a repository. + + Rows are random and left behind. A deleting teardown would have to cross the tenant + schema boundary the same way the foreign keys did, which is the thing this migration + removed. + """ + + from umbratrans_web.compute import ComputeRepository + from umbratrans_web.tenancy import tenant_search_path + + repository = ComputeRepository(_SCRATCH, min_size=1, max_size=6, passwords=None) + first, second = uuid.uuid4(), uuid.uuid4() + projects: dict[uuid.UUID, uuid.UUID] = {} + with repository._connection() as connection, repository._tx(connection): + for organization in (first, second): + connection.execute( + "INSERT INTO public.organizations (id, name, slug) VALUES (%s, %s, %s)", + (organization, f"Project ref {organization.hex[:8]}", + f"projref-{organization.hex[:8]}"), + ) + project = uuid.uuid4() + projects[organization] = project + # The tenant schema has to be on the search path before `projects` can be + # named unqualified - which is exactly the arrangement the application runs + # under, and the reason this insert is unqualified rather than + # `tenant_x.projects`. + connection.execute( + "SELECT set_config('search_path', %s, true)", + (tenant_search_path(organization),), + ) + # No organization_id: migration 0005 removed the column, because the schema + # the row sits in already names the tenant. Supplying it here would fail, and + # correctly so - it would be asserting a column the design deliberately does + # not have. + connection.execute( + "INSERT INTO projects (id, name, slug) VALUES (%s, %s, %s)", + (project, f"Book {project.hex[:8]}", f"book-{project.hex[:8]}"), + ) + try: + yield repository, first, second, projects + finally: + repository.close() + + +def test_a_job_may_name_its_own_organizations_project(two_tenants) -> None: + """The ordinary case must not be broken by the check.""" + + repository, first, _second, projects = two_tenants + + job = repository.submit_job( + first, + kind="speak", + requested_by="test", + requires=ModelRequirement(), + project_id=projects[first], + ) + + assert job.project_id == projects[first] + + +def test_a_job_may_not_name_another_organizations_project(two_tenants) -> None: + """The hole migration 0005 opened, and the reason this check exists.""" + + repository, first, second, projects = two_tenants + + with pytest.raises(NotFoundError): + repository.submit_job( + first, + kind="speak", + requested_by="test", + requires=ModelRequirement(), + project_id=projects[second], + ) + + +def test_the_refused_job_is_not_created(two_tenants) -> None: + """A refusal that still leaves a row is not a refusal. + + The check runs inside the same transaction as the insert, so the exception has to + roll the whole thing back. Asserting only that it raised would pass if the job were + written and then the error raised afterwards. + """ + + repository, first, second, projects = two_tenants + + with pytest.raises(NotFoundError): + repository.submit_job( + first, + kind="speak", + requested_by="test", + requires=ModelRequirement(), + project_id=projects[second], + ) + + with repository._connection() as connection: + count = connection.execute( + "SELECT count(*) FROM public.jobs WHERE project_id = %s", (projects[second],) + ).fetchone()["count"] + assert count == 0, "the refused job was written anyway" + + +def test_a_job_with_no_project_is_unaffected(two_tenants) -> None: + """Most work is not project-scoped, and the check must not require one.""" + + repository, first, _second, _projects = two_tenants + + job = repository.submit_job(first, kind="speak", requested_by="test", + requires=ModelRequirement()) + + assert job.project_id is None + + +def test_a_nonexistent_project_is_refused(two_tenants) -> None: + """Same answer as a project in another tenant, and for the same reason. + + A caller cannot tell the two apart, which is correct: from inside a tenant schema + the other tenant's project is not visible, and an id that is not there and an id + that is elsewhere look identical. + """ + + repository, first, _second, _projects = two_tenants + + with pytest.raises(NotFoundError): + repository.submit_job( + first, + kind="speak", + requested_by="test", + requires=ModelRequirement(), + project_id=uuid.uuid4(), + ) \ No newline at end of file