umbragit/UmbraTrans
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Restore the guarantee migration 0005 had to drop

Browse Source 
`projects` moved into a per-tenant schema and `jobs` stayed in `public`. A foreign key
cannot cross that boundary without postgres_fdw, so 0005 dropped
`jobs_project_id_fkey`, `artifacts_project_id_fkey` and a composite `user_roles` key. The
migration recorded the consequence honestly - "the application still refuses it; the
server no longer does" - but the application did not.

So after the split nothing stopped a `public.jobs` row naming a project in a different
tenant, which is precisely the leak the schema split exists to prevent. `submit_job` now
verifies it.

Two things that were wrong in the first attempt, both caught by running the tests:

- The lookup was unqualified, relying on `search_path`. `ComputeRepository._tx` does not
  set it - only the account repository does - so the check ran against `public` and could
  never see any project, refusing every project-scoped job. The schema is now derived from
  the organization id and named explicitly: the check is self-contained, and does not
  depend on ambient state deciding whether a security check runs at all.
- It filtered on `projects.organization_id`. That column no longer exists - the schema the
  row sits in *is* the tenant, which is the whole point of the split. There is no
  predicate to write.

`test_a_nonexistent_project_is_refused` asserts that a caller cannot distinguish "no such
project" from "another tenant's project". From inside a tenant schema the two are the same
observation, and they should be.

Confirmed the tests detect the gap: with the check removed, three of five fail, including
`test_the_refused_job_is_not_created` - which exists because a refusal that still writes a
row is not a refusal.

547 passed, 29 skipped. Deployment untouched: 30 tables, 0 tenant schemas.
This commit is contained in:
umbra audio
2026-10-03 14:16:35 -07:00
parent 24d7ea6a41
commit 715b7bc69f
2 changed files with 215 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

44
umbratrans_web/compute.py
View File

@@ -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,)

171
umbratrans_web/tests/test_job_project_tenancy.py Normal file
View File

@@ -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(),
)