diff --git a/umbratrans_web/postgres_repository.py b/umbratrans_web/postgres_repository.py index 6a95a20..4ebcb3b 100644 --- a/umbratrans_web/postgres_repository.py +++ b/umbratrans_web/postgres_repository.py @@ -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"]), diff --git a/umbratrans_web/repository.py b/umbratrans_web/repository.py index 7bccc9a..f806117 100644 --- a/umbratrans_web/repository.py +++ b/umbratrans_web/repository.py @@ -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.") diff --git a/umbratrans_web/service.py b/umbratrans_web/service.py index 69eb270..26434de 100644 --- a/umbratrans_web/service.py +++ b/umbratrans_web/service.py @@ -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",