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

Reach the tenant tables by name, not by a filter somebody remembers

Browse Source 
Every project statement now says `projects` unqualified and takes its
organization through search_path. A shared table filtered by
`organization_id = %s` is a filter somebody has to remember, and there is always
a write path that would not have it; an unqualified name resolves into the
caller's own schema or it does not resolve at all. The strongest possible
mistake - forgetting to scope a query - now raises instead of returning a wider
result set.

Project.organization_id survives in the model and is filled in from the schema
the answer came from. Callers still see which tenant a row belongs to; it is
derived from where the row is rather than from what the row says about itself.

Two consequences that are worth more than the mechanical change:

  - assign_user_to_project now takes the organization as an argument. It used to
    read it off the project row and copy it into user_roles, and there is no
    column to read now. user_roles is platform-level, so it cannot be written to
    by looking inside a tenant schema - so the caller, which has already resolved
    the project, has to say it. The protocol declares it, and the in-memory store
    accepts and ignores it rather than being the one implementation with a
    different signature.

  - A platform administrator listing projects without naming an organization now
    gets the answer assembled across tenant schemas, because there is no shared
    table to ask. That is one round trip per organization and it is paid only by
    that one caller; every per-tenant path, which is what an ordinary request
    does, is still a single statement. The merge reproduces the ordering and the
    total a shared table would have given, so paging across the boundary is the
    same as paging inside one.
This commit is contained in:
umbra audio
2026-10-03 13:32:46 -07:00
parent 2264a6cc42
commit 79b3484083
3 changed files with 223 additions and 72 deletions
Download Patch File Download Diff File Expand all files Collapse all files

267
umbratrans_web/postgres_repository.py
View File

@@ -510,20 +510,38 @@ class PostgresAccountRepository(AccountRepository):
)
# -- projects ------------------------------------------------------------
#
# Every statement below names `projects` **unqualified**, and the organization
# reaches the right one through `search_path` rather than through a predicate.
# That is deliberate and it is the whole point: a shared table filtered by
# `organization_id = %s` is a filter somebody has to remember, and there is
# always a write path that would not have it. An unqualified name resolves into
# the caller's own schema or it does not resolve at all.
#
# `Project.organization_id` therefore comes from the schema the row was found
# in rather than from a column. It is still filled in - the model and the API
# both carry it, and a caller needs to be able to see which tenant a row
# belongs to - but it is derived from where the row is rather than from what
# the row says about itself.
def create_project(self, project: Project) -> Project:
with self._connection() as connection:
"""Insert into the organization's own schema.
The organization comes from the row: a project cannot be created without
one, so there is nothing to ask for separately.
"""
with self._connection(organization_id=project.organization_id) as connection:
try:
connection.execute(
"""
INSERT INTO public.projects (
id, organization_id, name, slug, status, created_by, created_at, updated_at
INSERT INTO projects (
id, name, slug, status, created_by, created_at, updated_at
)
VALUES (%s, %s, %s, %s, %s, %s, %s, %s)
VALUES (%s, %s, %s, %s, %s, %s, %s)
""",
(
project.id,
project.organization_id,
project.name,
project.slug,
str(project.status),
@@ -537,24 +555,38 @@ class PostgresAccountRepository(AccountRepository):
return project
def get_project(self, project_id: UUID, *, organization_id: UUID | None = None) -> Project | None:
"""Fetch a project, matching the organization in the query.
"""Fetch one project.
The organization is part of the predicate rather than a check afterwards.
A project identifier is unique on its own, so filtering afterwards would
fetch a row and then decide it was not the caller's - which is a row read
that should not have happened at all.
With an organization, the lookup runs in that organization's schema, so a
project belonging to another tenant is not merely filtered out of a result
set: there is no row to read. A project identifier is unique on its own, so
an unscoped lookup against a shared table would fetch another tenant's row
and then decide it was not the caller's - a read that should never have
happened, and one that a later change to the check could turn into a leak.
Without one - a platform administrator who has not named an organization -
there is no single schema to look in, so the tenant schemas are searched in
turn. That is a platform operation by definition: the caller is asking for
"any project's", and there is no statement that can answer it without
naming every organization.
"""
conditions, params = ["p.id = %s"], [project_id]
if organization_id is not None:
conditions.append("p.organization_id = %s")
params.append(organization_id)
with self._connection() as connection:
row = connection.execute(
f"SELECT {_PROJECT_COLUMNS} FROM public.projects p WHERE {' AND '.join(conditions)}",
params,
).fetchone()
return _project_from_row(row) if row else None
with self._connection(organization_id=organization_id) as connection:
row = connection.execute(
f"SELECT {_PROJECT_COLUMNS} FROM projects p WHERE p.id = %s", (project_id,)
).fetchone()
if row is not None:
return _project_from_row(row, organization_id=organization_id)
return None
for candidate in self._organizations_with_tenant_schema():
with self._connection(organization_id=candidate) as connection:
row = connection.execute(
f"SELECT {_PROJECT_COLUMNS} FROM projects p WHERE p.id = %s", (project_id,)
).fetchone()
if row is not None:
return _project_from_row(row, organization_id=candidate)
return None
def get_project_by_slug(self, slug: str, *, organization_id: UUID) -> Project | None:
"""Fetch by handle, case-insensitively.
@@ -565,23 +597,25 @@ class PostgresAccountRepository(AccountRepository):
appearing to work for every other store in the system. The cast looks like
a type accommodation for the parameter and is in fact a silent change of
behaviour.
The organization is no longer part of the predicate, because it is part of
the schema: the same slug in another tenant is a different table.
"""
with self._connection() as connection:
with self._connection(organization_id=organization_id) as connection:
row = connection.execute(
f"SELECT {_PROJECT_COLUMNS} FROM public.projects p "
"WHERE p.organization_id = %s AND p.slug = %s",
(organization_id, str(slug)),
f"SELECT {_PROJECT_COLUMNS} FROM projects p WHERE p.slug = %s",
(str(slug),),
).fetchone()
return _project_from_row(row) if row else None
return _project_from_row(row, organization_id=organization_id) if row else None
def save_project(self, project: Project) -> Project:
with self._connection() as connection:
with self._connection(organization_id=project.organization_id) as connection:
row = connection.execute(
"""
UPDATE public.projects
UPDATE projects
SET name = %s, slug = %s, status = %s, updated_at = %s
WHERE id = %s AND organization_id = %s
WHERE id = %s
RETURNING id
""",
(
@@ -590,7 +624,6 @@ class PostgresAccountRepository(AccountRepository):
str(project.status),
project.updated_at or utc_now(),
project.id,
project.organization_id,
),
).fetchone()
if row is None:
@@ -605,45 +638,112 @@ class PostgresAccountRepository(AccountRepository):
limit: int = 50,
offset: int = 0,
) -> tuple[list[Project], int]:
"""List projects, from one tenant's schema or from all of them.
The scoped form is one statement in one schema. The unscoped form - a
platform administrator who has not named an organization - is assembled
across tenant schemas, because there is no shared table to ask.
"""
if organization_id is None:
return self._find_projects_across_tenants(statuses=statuses, limit=limit, offset=offset)
conditions: list[str] = []
params: list[Any] = []
if organization_id is not None:
conditions.append("p.organization_id = %s")
params.append(organization_id)
if statuses:
# Every status the model knows is rendered, so a value outside the
# database's own check constraint is refused by the store rather than
# silently matching nothing.
wanted = [str(status) for status in ProjectStatus]
for status in statuses:
if str(status) not in wanted:
raise RepositoryError("unmodelled_project_status", f"Unrecognised project status {status!r}.")
conditions.append("p.status = ANY(%s)")
params.append([str(status) for status in statuses])
conditions.extend(_status_conditions(statuses))
params.extend(_status_params(statuses))
where = ("WHERE " + " AND ".join(conditions)) if conditions else ""
with self._connection() as connection:
total_row = connection.execute(
f"SELECT count(*) AS total FROM public.projects p {where}", params
).fetchone()
window = max(offset, 0) + max(limit, 0)
with self._connection(organization_id=organization_id) as connection:
total_row = connection.execute(f"SELECT count(*) AS total FROM projects p {where}", params).fetchone()
total = int(total_row["total"]) if total_row else 0
if total == 0:
return [], 0
rows = connection.execute(
f"SELECT {_PROJECT_COLUMNS} FROM public.projects p {where} "
f"SELECT {_PROJECT_COLUMNS} FROM projects p {where} "
"ORDER BY p.created_at DESC, p.id LIMIT %s OFFSET %s",
[*params, max(limit, 0), max(offset, 0)],
[*params, window, max(offset, 0)],
).fetchall()
return [_project_from_row(row) for row in rows], total
return [_project_from_row(row, organization_id=organization_id) for row in rows], total
def _find_projects_across_tenants(
self,
*,
statuses: Sequence[ProjectStatus] | None = None,
limit: int = 50,
offset: int = 0,
) -> tuple[list[Project], int]:
"""The platform-wide listing, assembled one tenant schema at a time.
Each tenant contributes its own total and its own first ``offset + limit``
rows; merging those and slicing gives the same answer a single shared table
would have. Asking each tenant for more than that is unnecessary, because a
row outside its tenant's first window cannot be inside the global one.
That is one round trip per organization, which at a hundred organizations
is a hundred round trips. It is the price of not sharing the table, and it
is paid only by a platform administrator listing across every tenant - the
per-tenant path above, which is everything an ordinary request does, is one
statement.
"""
window = max(offset, 0) + max(limit, 0)
collected: list[tuple[datetime, str, Project]] = []
total = 0
for candidate in self._organizations_with_tenant_schema():
conditions: list[str] = []
params: list[Any] = []
if statuses:
conditions.extend(_status_conditions(statuses))
params.extend(_status_params(statuses))
where = ("WHERE " + " AND ".join(conditions)) if conditions else ""
with self._connection(organization_id=candidate) as connection:
total_row = connection.execute(f"SELECT count(*) AS total FROM projects p {where}", params).fetchone()
total += int(total_row["total"]) if total_row else 0
rows = connection.execute(
f"SELECT {_PROJECT_COLUMNS} FROM projects p {where} ORDER BY p.created_at DESC, p.id LIMIT %s",
[*params, window],
).fetchall()
collected.extend(
(row["created_at"], str(row["id"]), _project_from_row(row, organization_id=candidate))
for row in rows
)
# The ordering a shared table would have given: newest first, identifier
# breaking ties, so a page boundary does not depend on which tenant
# answered first.
collected.sort(key=lambda item: (item[0], item[1]), reverse=True)
page = [project for _, _, project in collected][max(offset, 0) : max(offset, 0) + max(limit, 0)]
return page, total
def count_projects(self, *, organization_id: UUID | None = None) -> int:
if organization_id is None:
sql, params = "SELECT count(*) AS total FROM public.projects", []
else:
sql, params = "SELECT count(*) AS total FROM public.projects WHERE organization_id = %s", [organization_id]
with self._connection() as connection:
row = connection.execute(sql, params).fetchone()
return sum(
self.count_projects(organization_id=candidate)
for candidate in self._organizations_with_tenant_schema()
)
with self._connection(organization_id=organization_id) as connection:
row = connection.execute("SELECT count(*) AS total FROM projects").fetchone()
return int(row["total"]) if row else 0
def _organizations_with_tenant_schema(self) -> list[UUID]:
"""Every organization that has a tenant schema, for the cross-tenant paths.
The join against ``pg_namespace`` is not decoration. An organization can
exist without a schema if it was inserted by something that bypassed the
trigger, and a cross-tenant listing that pointed at a missing schema would
fail on that one organization and return nothing for any of them - a single
half-provisioned tenant taking the whole platform view down with it.
"""
with self._connection() as connection:
rows = connection.execute(
"SELECT o.id FROM public.organizations o "
"JOIN pg_namespace n ON n.nspname = app.tenant_schema_name(o.id) "
"ORDER BY o.id"
).fetchall()
return [row["id"] for row in rows]
def list_project_member_ids(self, project_id: UUID) -> list[UUID]:
"""Project-scoped assignments, from the same table the roles live in.
@@ -677,26 +777,34 @@ class PostgresAccountRepository(AccountRepository):
#: buried in a query.
PROJECT_ASSIGNMENT_ROLE = "viewer"
def assign_user_to_project(self, project_id: UUID, user_id: UUID) -> bool:
def assign_user_to_project(self, project_id: UUID, user_id: UUID, *, organization_id: UUID) -> bool:
"""Add a project-scoped assignment, or report that there already is one.
Idempotent rather than erroring, because assigning the same account twice
is a no-op an administrator can reasonably make. The insert is
conditional so two concurrent requests cannot both add one.
is a no-op an administrator can reasonably make. The insert is conditional
so two concurrent requests cannot both add one.
``organization_id`` is required here rather than derived, and that is a
consequence of the isolation model rather than a preference. This statement
used to read ``p.organization_id`` off the project row and copy it into
``user_roles``. There is no column to read now - the schema the project was
found in *is* the organization - and ``user_roles`` is a platform table, so
it cannot be written to by looking inside a tenant schema. The caller has
already resolved the project and therefore already knows.
"""
with self._connection() as connection:
with self._connection(organization_id=organization_id) as connection:
row = connection.execute(
"""
INSERT INTO public.user_roles (user_id, role_id, scope, organization_id, project_id, status)
SELECT %s, r.id, 'project', p.organization_id, p.id, 'active'
FROM public.projects p
SELECT %s, r.id, 'project', %s, p.id, 'active'
FROM projects p
JOIN public.roles r ON r.name = %s AND r.scope = 'project'
WHERE p.id = %s
ON CONFLICT DO NOTHING
RETURNING id
""",
(user_id, self.PROJECT_ASSIGNMENT_ROLE, project_id),
(user_id, organization_id, self.PROJECT_ASSIGNMENT_ROLE, project_id),
).fetchone()
if row is not None:
return True
@@ -709,9 +817,7 @@ class PostgresAccountRepository(AccountRepository):
).fetchone()
if existing is not None:
return False
project_row = connection.execute(
"SELECT 1 AS present FROM public.projects WHERE id = %s", (project_id,)
).fetchone()
project_row = connection.execute("SELECT 1 AS present FROM projects WHERE id = %s", (project_id,)).fetchone()
if project_row is None:
raise NotFoundError("project_not_found")
raise RepositoryError(
@@ -1478,13 +1584,38 @@ def _user_from_row(
#: Written for a table aliased ``ur``.
TENANT_ROW_PREDICATE = "ur.scope <> 'project'"
_PROJECT_COLUMNS = (
"p.id, p.organization_id, p.name, p.slug, p.status, p.created_by, p.created_at, p.updated_at"
)
def _status_conditions(statuses: Sequence[ProjectStatus]) -> list[str]:
"""The predicate for a status filter, refusing a value the model does not have.
Every status the model knows is rendered, so a value outside the database's own
check constraint is refused by the store rather than silently matching nothing.
"No projects have status 'paused'" and "that status does not exist" are
different answers and only one of them is true.
"""
wanted = [str(status) for status in ProjectStatus]
for status in statuses:
if str(status) not in wanted:
raise RepositoryError("unmodelled_project_status", f"Unrecognised project status {status!r}.")
return ["p.status = ANY(%s)"]
def _project_from_row(row: Mapping[str, Any]) -> Project:
"""Map a ``projects`` row.
def _status_params(statuses: Sequence[ProjectStatus]) -> list[Any]:
return [[str(status) for status in statuses]]
#: The columns a project is read with.
#:
#: ``organization_id`` is **not** among them, and that is the migration talking:
#: the column is gone from the table, because the schema the row was found in
#: already says which organization owns it. The model still carries an organization
#: and callers still see one, so :func:`_project_from_row` fills it in from the
#: schema the answer came from.
_PROJECT_COLUMNS = "p.id, p.name, p.slug, p.status, p.created_by, p.created_at, p.updated_at"
def _project_from_row(row: Mapping[str, Any], *, organization_id: UUID) -> Project:
"""Map a ``projects`` row, taking the organization from the schema it is in.
The status is mapped through the same table of modelled values the accounts
use, and an unrecognised value is refused rather than coerced. The database's
@@ -1495,7 +1626,7 @@ def _project_from_row(row: Mapping[str, Any]) -> Project:
return Project(
id=row["id"],
organization_id=row["organization_id"],
organization_id=organization_id,
name=row["name"],
slug=str(row["slug"]),
status=_project_status(row["status"]),

26
umbratrans_web/repository.py
View File

@@ -135,8 +135,18 @@ class AccountRepository(Protocol):
raise NotImplementedError(f"{type(self).__name__} does not implement list_project_member_ids().")
def assign_user_to_project(self, project_id: UUID, user_id: UUID) -> bool:
"""Assign an account to a project. Returns False if already assigned."""
def assign_user_to_project(self, project_id: UUID, user_id: UUID, *, organization_id: UUID) -> bool:
"""Assign an account to a project. Returns False if already assigned.
``organization_id`` is required, and it is required because the assignment
table is platform-level while the project table is not. Before the
schema-per-tenant migration this statement read the organization off the
project row; there is no such column now, and the tenant schema the project
was found in is where the organization came from. Every implementation has
to accept it, including the ones that keep the organization on the row and
have no use for it, because a protocol parameter is what makes the call
sites agree.
"""
raise NotImplementedError(f"{type(self).__name__} does not implement assign_user_to_project().")
@@ -431,7 +441,12 @@ class InMemoryAccountRepository:
with self._lock:
return sorted(self._project_members.get(project_id, set()))
def assign_user_to_project(self, project_id: UUID, user_id: UUID) -> bool:
def assign_user_to_project(self, project_id: UUID, user_id: UUID, *, organization_id: UUID) -> bool:
# `organization_id` is accepted and not used: this store keeps the
# organization on the project row itself, so it has no schema to resolve
# and nothing to look up. It is here because the protocol declares it, and
# because an argument one implementation needs and another ignores is
# exactly the signature drift the protocol check exists to catch.
with self._lock:
members = self._project_members.setdefault(project_id, set())
if user_id in members:
@@ -719,6 +734,11 @@ def build_repository(settings: object, *, repository: AccountRepository | None =
# rows. The failure is silent by nature: policies evaluate false, the
# database looks empty, and nothing raises.
repository.verify_rls_contract()
# The same argument for the tenant schema name, which exists in two
# languages and has the same silent-and-total failure if they disagree:
# every tenant query resolves in a schema that does not exist, and the
# error names the statement rather than the cause.
repository.verify_tenant_schema_contract()
return repository
raise RepositoryError("unknown_repository", "Unknown repository kind.")

2
umbratrans_web/service.py
View File

@@ -2093,7 +2093,7 @@ class AccountService:
if account.organization_id != project.organization_id:
raise ForbiddenError("cross_organization_forbidden")
self._repository.assign_user_to_project(project.id, account.id)
self._repository.assign_user_to_project(project.id, account.id, organization_id=project.organization_id)
self._audit(
action=ACTION_PROJECT_ASSIGNMENT_ADDED,
outcome="success",