diff --git a/core/switch_core/db/models.py b/core/switch_core/db/models.py index 7124c0b0a..f61bfb12f 100644 --- a/core/switch_core/db/models.py +++ b/core/switch_core/db/models.py @@ -1,4 +1,5 @@ import uuid +from datetime import datetime from sqlalchemy import ( DDL, @@ -259,6 +260,76 @@ class ApiKey(TenantScoped, Base): ) +# ── Invitations ──────────────────────────────────────────────────────────────── + + +class Invitation(TenantScoped, Base): + """A credential that grants membership in a tenant (CHOO-2722). + + `email` is null for a shareable link and set for an invitation addressed + to one person; nothing at this layer refuses acceptance by a different + address, which is a decision for whatever accepts the invitation, not for + the row that describes it. + + `token_hash` is unique across the whole deployment rather than per tenant, + the same reasoning as `api_keys.key_hash`: accepting an invitation is + exactly the credential-resolution shape that runs before any tenant is + bound, so the hash has to be resolvable on its own. `tenant_of_invitation` + (`db/tenant_lookup.py`) is the lookup that does it. The hash, never the + token: nothing in this schema, this store, or anything built on either + holds the plaintext once `InvitationStore.create` has returned it. + + `role` is a checked string rather than an enum type, matching + `tenant_members.role` — the role an acceptance would grant, not one held + by anything yet. + + `uses_remaining` and `expires_at` bound how long and how many times the + token works; `revoked_at` is a third, independent way to stop it early. + None of the three are optional here — a table that could not expire or be + revoked would not be a credential, and the design this implements is + explicit that expiry and revocation are not optional. + + The floor under `uses_remaining` is a constraint rather than a convention + because the thing it guards against is a lost race, not a typo: two + concurrent acceptances of a single-use invitation can both read `1` and + both write `0`, and read-committed will let both commit. `consume` + (`db/stores/invitation_store.py`) is the decrement that cannot lose that + race; the constraint is what makes any other decrement fail loudly instead + of over-granting membership. + """ + + __tablename__ = "invitations" + __table_args__ = ( + CheckConstraint( + "role IN ('owner', 'admin', 'member')", name="ck_invitations_role" + ), + CheckConstraint( + "uses_remaining >= 0", name="ck_invitations_uses_remaining_not_negative" + ), + ) + + id: Mapped[str] = mapped_column(Text, primary_key=True, default=_uuid) + role: Mapped[str] = mapped_column(Text, nullable=False) + email: Mapped[str | None] = mapped_column(Text, nullable=True) + # Global on purpose, like api_keys.key_hash: accepting an invitation + # resolves the hash before a tenant is known, so it cannot be scoped by + # one. + token_hash: Mapped[str] = mapped_column(Text, unique=True, nullable=False) + expires_at: Mapped[datetime] = mapped_column( + DateTime(timezone=True), nullable=False + ) + uses_remaining: Mapped[int] = mapped_column(Integer, nullable=False) + revoked_at: Mapped[datetime | None] = mapped_column( + DateTime(timezone=True), nullable=True + ) + created_by: Mapped[str] = mapped_column( + Text, ForeignKey("users.id"), nullable=False + ) + created_at: Mapped[datetime] = mapped_column( + DateTime(timezone=True), server_default=func.now(), nullable=False + ) + + # ── Clients ──────────────────────────────────────────────────────────────────── diff --git a/core/switch_core/db/stores/__init__.py b/core/switch_core/db/stores/__init__.py index c9243625c..dbee2199f 100644 --- a/core/switch_core/db/stores/__init__.py +++ b/core/switch_core/db/stores/__init__.py @@ -5,6 +5,7 @@ from switch_core.db.stores.collaboration_bridge_store import CollaborationBridgeStore from switch_core.db.stores.document_store import DocumentStore from switch_core.db.stores.external_user_store import ExternalUserStore +from switch_core.db.stores.invitation_store import InvitationStore from switch_core.db.stores.message_store import MessageStore from switch_core.db.stores.reference_store import ReferenceStore from switch_core.db.stores.reference_type_store import ReferenceTypeStore @@ -25,6 +26,7 @@ "CollaborationBridgeStore", "DocumentStore", "ExternalUserStore", + "InvitationStore", "MessageStore", "ReferenceStore", "ReferenceTypeStore", diff --git a/core/switch_core/db/stores/invitation_store.py b/core/switch_core/db/stores/invitation_store.py new file mode 100644 index 000000000..0e6b73d1e --- /dev/null +++ b/core/switch_core/db/stores/invitation_store.py @@ -0,0 +1,166 @@ +from __future__ import annotations + +import hashlib +import secrets +from datetime import UTC, datetime + +from sqlalchemy import ColumnElement, and_, func, select, update +from sqlalchemy.ext.asyncio import AsyncSession + +from switch_core.db.models import Invitation + +_TOKEN_BYTES = 32 + + +class InvitationNotUsableError(Exception): + """An invitation was asked to grant membership and could not. + + Raised rather than returned as `None`, because the three ways to get here + — revoked, expired, spent — are all states a caller has to answer for + rather than states it can usefully retry or ignore. Also raised when the + invitation does not exist at all, or belongs to a tenant this session is + not bound to: the row-level-security policy makes those indistinguishable + from the outside, which is the point of it. + """ + + +def _usable() -> ColumnElement[bool]: + """The three independent gates on an invitation, as one predicate. + + Written once and shared by everything that asks the question, because + three gates hand-rolled per call site is how the second call site ends up + checking two. Postgres evaluates it — expiry against the database's clock, + not the caller's — so `consume` can decide and decrement in one statement. + """ + return and_( + Invitation.revoked_at.is_(None), + Invitation.expires_at > func.now(), + Invitation.uses_remaining > 0, + ) + + +def generate_invitation_token() -> tuple[str, str]: + """A fresh invitation token and the hash that is all Switch ever stores. + + The plaintext lives only in the tuple this returns. Nothing keeps a + second copy of it: `Invitation` has no column for it, so there is nothing + on the row — and therefore nothing a query, a log line, or a repr of that + row could leak — beyond the hash. A caller that loses the plaintext has + to revoke and mint again, the same as a lost API key. + """ + token = secrets.token_urlsafe(_TOKEN_BYTES) + return token, hashlib.sha256(token.encode()).hexdigest() + + +class InvitationStore: + async def create( + self, + session: AsyncSession, + *, + role: str, + email: str | None, + expires_at: datetime, + uses_remaining: int, + created_by: str, + ) -> tuple[Invitation, str]: + """Mint an invitation, returning the row and its plaintext token. + + The token is generated here, inside the one call that can hand it + back — never before, never separately, and never assigned to the row + that persists. A caller that needs to show the token to whoever it + invited must do so from this call's return value; there is no later + read that will give it back. + """ + token, token_hash = generate_invitation_token() + invitation = Invitation( + role=role, + email=email, + token_hash=token_hash, + expires_at=expires_at, + uses_remaining=uses_remaining, + created_by=created_by, + ) + session.add(invitation) + await session.flush() + return invitation, token + + async def get_by_token_hash( + self, session: AsyncSession, token_hash: str + ) -> Invitation | None: + """The invitation named by a token's hash. + + Called on a session already bound to the invitation's tenant — + resolved first through `db/tenant_lookup.py`'s `tenant_of_invitation`, + the same two-step shape a bearer token's own resolution takes. Row- + level security refuses this read on a session with nothing bound. + """ + result = await session.execute( + select(Invitation).where(Invitation.token_hash == token_hash) + ) + return result.scalar_one_or_none() + + async def get_valid_by_token_hash( + self, session: AsyncSession, token_hash: str + ) -> Invitation | None: + """The invitation named by a token's hash, if it is still usable. + + What `get_by_token_hash` finds says nothing about whether the token + still works: a revoked, expired or spent invitation is an ordinary row + and reads back like any other. This is the read for anyone about to + act on one — showing the invitee what they were invited to, say — + and it answers with the same predicate `consume` enforces. + + A `None` here is not permission to skip `consume`'s own check. The row + can be spent between the two by whoever else holds the same link; + `consume` is where that race is settled. + """ + result = await session.execute( + select(Invitation).where(Invitation.token_hash == token_hash, _usable()) + ) + return result.scalar_one_or_none() + + async def consume(self, session: AsyncSession, invitation_id: str) -> Invitation: + """Spend one use of an invitation, or refuse. + + One conditional `UPDATE`: the gates are in the `WHERE`, so the row is + checked and decremented in the same statement and Postgres arbitrates + between concurrent acceptances. Read-then-write cannot do that — two + acceptances of a single-use invitation both read `1`, both write `0`, + both commit, and one invite grants two memberships. + + No row updated means no usable invitation was there to update, which + is what `InvitationNotUsableError` reports. + """ + result = await session.execute( + update(Invitation) + .where(Invitation.id == invitation_id, _usable()) + .values(uses_remaining=Invitation.uses_remaining - 1) + .returning(Invitation) + .execution_options(populate_existing=True) + ) + invitation = result.scalar_one_or_none() + if invitation is None: + raise InvitationNotUsableError( + f"Invitation {invitation_id} is revoked, expired, spent, or not " + "visible to this session" + ) + return invitation + + async def list_for_tenant(self, session: AsyncSession) -> list[Invitation]: + """Every invitation of the session's bound tenant. + + No `WHERE tenant_id = …` of its own: the policy is what narrows this, + the same as every other listing in this package. + """ + result = await session.execute( + select(Invitation).order_by(Invitation.created_at) + ) + return list(result.scalars().all()) + + async def revoke(self, session: AsyncSession, invitation_id: str) -> Invitation: + invitation = await session.get(Invitation, invitation_id) + if invitation is None: + raise ValueError(f"Invitation not found: {invitation_id}") + invitation.revoked_at = datetime.now(UTC) + await session.flush() + return invitation diff --git a/core/switch_core/db/tenant_lookup.py b/core/switch_core/db/tenant_lookup.py index 0fdc56b88..dcb3fbcfe 100644 --- a/core/switch_core/db/tenant_lookup.py +++ b/core/switch_core/db/tenant_lookup.py @@ -1,4 +1,4 @@ -"""The whole exemption from row-level security, written out as seven functions. +"""The whole exemption from row-level security, written out as eight functions. Row-level security is enforced by `require_tenant_id()` (`db/rls_ddl.py`), which raises when no tenant is bound. That is the property everything else @@ -32,7 +32,8 @@ - **They are a closed list.** `TENANT_LOOKUPS` below is the list; a test compares it against the functions actually installed, against the migration's frozen copy, and against what each one answers when called as - the restricted role. Adding an eighth is an edit a reviewer sees. + the restricted role. Adding one is an edit a reviewer sees, and is meant to + be argued with rather than waved through. **What the exemption gives, stated exactly.** Every lookup returns `setof text` — tenant ids, never a row of a scoped table. That much is the part @@ -63,8 +64,8 @@ So the property this design actually holds is: **the exemption discloses the shape of the deployment — which tenants exist, and which tenant a given user, -credential, room, bridge or connector belongs to — and no row of any -tenant-scoped table.** It is a boundary on data, not on metadata. Narrowing +credential, room, bridge, connector or invitation belongs to — and no row of +any tenant-scoped table.** It is a boundary on data, not on metadata. Narrowing the second is a question about who may hold the runtime role's credentials at all, since everything above is reachable by anyone who has them. @@ -85,7 +86,7 @@ identifier the caller already has, and everything after it is scoped. **A lookup whose caller already knows the answer does not belong here.** There -was an eighth, `tenant_of_client`, and it was the one called most: every +was one more, `tenant_of_client`, and it was the one called most: every client's transport asked it, once per transport, and so did every agent client's `start`. Both were built from a `clients` row that names the tenant in a column, so the question was asked of the database with the answer already @@ -95,6 +96,18 @@ function nobody needs is still a function every role could call, so the shorter list is the whole point of noticing. +**`tenant_of_invitation` is the eighth, and it is the same shape as +`tenant_of_api_key`.** Accepting an invitation is credential resolution: the +request carries a token and nothing else, no tenant is bound yet, and the +table it would have to read (`invitations`) is tenant-scoped like everything +else, so the policy refuses exactly the read that has to happen first. Resolve +the tenant here, bind it, then read the invitation itself — its role, its +email, whether it is spent, expired or revoked — through the ordinary scoped +store, whose `get_valid_by_token_hash` and `consume` are where those three +gates are actually enforced. Nothing about *that* row crosses the exemption; +only the tenant id does, which is the property every lookup in this module +rests on. + Why not the obvious alternatives is argued in `docs/old/multi-tenancy-phase1-db.md`, "The bootstrap problem"; the short version is that returning rows instead of tenant ids would put a second copy @@ -257,6 +270,18 @@ def signature(self) -> str: query="SELECT tenant_id FROM server_connectors WHERE id = p_connector_id", purpose="Same as the bridge, for a server-side connector.", ), + TenantLookup( + name="tenant_of_invitation", + argument="token_hash", + query="SELECT tenant_id FROM invitations WHERE token_hash = p_token_hash", + purpose=( + "Which tenant an invitation belongs to, resolved from the hash of " + "its token before any tenant is bound — the same credential-" + "resolution shape as a bearer token, and necessary for the same " + "reason: accepting an invitation is exactly the read the policy " + "refuses to a session with nothing bound yet." + ), + ), ) TENANT_LOOKUPS_BY_NAME: dict[str, TenantLookup] = { @@ -312,9 +337,9 @@ def attach_tenant_lookups(metadata: MetaData) -> None: # ── Calling them ────────────────────────────────────────────────────────────── # One `text()` per lookup, written out rather than assembled from `lookup.name` -# at call time: the seven names are fixed and known here, so there is nothing +# at call time: the eight names are fixed and known here, so there is nothing # for a call site to build. The assertion below is what keeps this dict from -# quietly falling behind `TENANT_LOOKUPS` — an eighth lookup with no entry +# quietly falling behind `TENANT_LOOKUPS` — a ninth lookup with no entry # here fails at import, not with a `KeyError` on whatever request reaches it # first. _LOOKUP_STATEMENTS: dict[str, TextClause] = { @@ -337,6 +362,9 @@ def attach_tenant_lookups(metadata: MetaData) -> None: "tenant_of_server_connector": text( "SELECT tenant_id FROM tenant_of_server_connector(:argument) AS tenant_id" ), + "tenant_of_invitation": text( + "SELECT tenant_id FROM tenant_of_invitation(:argument) AS tenant_id" + ), } assert _LOOKUP_STATEMENTS.keys() == TENANT_LOOKUPS_BY_NAME.keys(), ( @@ -425,3 +453,10 @@ async def tenant_of_server_connector( ) -> str | None: lookup = TENANT_LOOKUPS_BY_NAME["tenant_of_server_connector"] return _at_most_one(lookup, await _call(session_factory, lookup, connector_id)) + + +async def tenant_of_invitation( + session_factory: async_sessionmaker[AsyncSession], token_hash: str +) -> str | None: + lookup = TENANT_LOOKUPS_BY_NAME["tenant_of_invitation"] + return _at_most_one(lookup, await _call(session_factory, lookup, token_hash)) diff --git a/core/switch_core/migrations/versions/5daaea6b674d_invitations.py b/core/switch_core/migrations/versions/5daaea6b674d_invitations.py new file mode 100644 index 000000000..a44e9c4ce --- /dev/null +++ b/core/switch_core/migrations/versions/5daaea6b674d_invitations.py @@ -0,0 +1,120 @@ +"""invitations, and the lookup that resolves one to a tenant + +A credential that grants membership in a tenant (CHOO-2722, +`docs/old/multi-tenancy-phase2-tenants.md` §5). It is tenant-scoped like +everything else, so it gets the same `tenant_isolation` policy as the other +tables — written out below rather than inherited, because `265ed188ad6f` +installed the policies as they stood then and a table added afterwards has to +bring its own. + +`token_hash` is unique across the deployment rather than per tenant, for the +same reason `api_keys.key_hash` is: accepting an invitation resolves the hash +before a tenant is known, so a per-tenant index could not answer the question +being asked. Only the hash is ever stored — the token itself never reaches +this table, or any other. + +`uses_remaining >= 0` is a constraint and not a convention because the failure +it prevents is a lost race rather than a typo: two concurrent acceptances of a +single-use invitation can both read `1` and both write `0` under read- +committed, and both commit. The store's `consume` does the decrement as one +conditional `UPDATE` so that cannot happen; this constraint is what makes any +future decrement that forgets fail loudly rather than grant membership twice. + +`tenant_of_invitation(token_hash)` is the lookup that resolves it, and it is +an addition to the closed set of `SECURITY DEFINER` functions `9c41a7b0e5d8` +installed as the whole exemption from row-level security. That set is +deliberately short and every entry in it is callable by anyone holding the +runtime role's credentials, so adding one is meant to be argued with. The +argument for this one: accepting an invitation is exactly the credential- +resolution shape the rest of the module is built on — a token and nothing +else, no tenant bound yet — and without it there is no way to bind a tenant +before reading the row the token names. + +The DDL below is a verbatim copy of `switch_core/db/rls_ddl.py` and +`switch_core/db/tenant_lookup.py` as they stood when this migration was +written, copied rather than imported for the same reason every other revision +in this chain copies rather than imports: a migration is a record of a change +that already happened, and importing the live module would let a later edit +silently change what this one means. +`tests/switch_core/db/test_frozen_ddl_matches_create_all.py` is what keeps the +copy from drifting. + +Revision ID: 5daaea6b674d +Revises: 8ef6d4038ecc +Create Date: 2026-09-11 00:00:00.000000 + +""" + +from collections.abc import Sequence + +import sqlalchemy as sa +from alembic import op + +revision: str = "5daaea6b674d" +down_revision: str | None = "8ef6d4038ecc" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + +TABLE = "invitations" +POLICY_NAME = "tenant_isolation" + +ENABLE_RLS = f'ALTER TABLE "{TABLE}" ENABLE ROW LEVEL SECURITY' + +CREATE_POLICY = f"""CREATE POLICY {POLICY_NAME} ON "{TABLE}" + FOR ALL + USING ("tenant_id" = (SELECT require_tenant_id())) + WITH CHECK ("tenant_id" = (SELECT require_tenant_id()))""" + +DROP_POLICY = f'DROP POLICY IF EXISTS {POLICY_NAME} ON "{TABLE}"' + +SECURE_SEARCH_PATH = "pg_catalog, public, pg_temp" + +CREATE_TENANT_OF_INVITATION = f"""CREATE OR REPLACE FUNCTION tenant_of_invitation(p_token_hash text) + RETURNS SETOF text + LANGUAGE sql STABLE SECURITY DEFINER + SET search_path = {SECURE_SEARCH_PATH} +AS $$SELECT tenant_id FROM invitations WHERE token_hash = p_token_hash$$""" + +DROP_TENANT_OF_INVITATION = "DROP FUNCTION IF EXISTS tenant_of_invitation(text)" + + +def upgrade() -> None: + op.create_table( + TABLE, + sa.Column("id", sa.Text(), nullable=False), + sa.Column("tenant_id", sa.Text(), nullable=False), + sa.Column("role", sa.Text(), nullable=False), + sa.Column("email", sa.Text(), nullable=True), + sa.Column("token_hash", sa.Text(), nullable=False), + sa.Column("expires_at", sa.DateTime(timezone=True), nullable=False), + sa.Column("uses_remaining", sa.Integer(), nullable=False), + sa.Column("revoked_at", sa.DateTime(timezone=True), nullable=True), + sa.Column("created_by", sa.Text(), nullable=False), + sa.Column( + "created_at", + sa.DateTime(timezone=True), + server_default=sa.text("now()"), + nullable=False, + ), + sa.CheckConstraint( + "role IN ('owner', 'admin', 'member')", name="ck_invitations_role" + ), + sa.CheckConstraint( + "uses_remaining >= 0", name="ck_invitations_uses_remaining_not_negative" + ), + sa.PrimaryKeyConstraint("id"), + sa.ForeignKeyConstraint( + ["tenant_id"], ["tenants.id"], name="fk_invitations_tenant" + ), + sa.ForeignKeyConstraint(["created_by"], ["users.id"]), + sa.UniqueConstraint("token_hash"), + ) + op.execute(ENABLE_RLS) + op.execute(CREATE_POLICY) + op.execute(CREATE_TENANT_OF_INVITATION) + + +def downgrade() -> None: + op.execute(DROP_TENANT_OF_INVITATION) + op.execute(DROP_POLICY) + op.drop_table(TABLE) diff --git a/core/tests/switch_core/db/stores/test_invitation_store.py b/core/tests/switch_core/db/stores/test_invitation_store.py new file mode 100644 index 000000000..2514be439 --- /dev/null +++ b/core/tests/switch_core/db/stores/test_invitation_store.py @@ -0,0 +1,485 @@ +"""InvitationStore, and the row-level-security isolation it depends on. + +The store tests run against `session_factory`, the ambient-tenant fixture +every other store test in this package uses. The isolation tests run against +`rls_harness`, the same harness `test_row_level_security.py` uses to connect +as a role the policies actually apply to — the point being made here is +exactly the one that module states: an invitation belonging to tenant A must +be invisible to a session bound to tenant B, through the ordinary store layer, +with no tenant filter of the store's own doing the work. +""" + +from __future__ import annotations + +import asyncio +import hashlib +import uuid +from datetime import UTC, datetime, timedelta + +import pytest +from sqlalchemy import text +from sqlalchemy.exc import DBAPIError, IntegrityError +from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker + +from switch_core.db.models import Invitation, Tenant, User +from switch_core.db.session_scope import tenant_session +from switch_core.db.stores.invitation_store import ( + InvitationNotUsableError, + InvitationStore, + generate_invitation_token, +) +from tests.conftest import RLSHarness + +_STORE = InvitationStore() + + +async def _make_user(session: AsyncSession, name: str) -> User: + user = User(name=name, email=f"{name}-{uuid.uuid4().hex[:8]}@test", role="user") + session.add(user) + await session.flush() + return user + + +def _expires_soon() -> datetime: + return datetime.now(UTC) + timedelta(days=7) + + +async def _make_invitation( + session_factory: async_sessionmaker[AsyncSession], + *, + uses_remaining: int = 1, + expires_at: datetime | None = None, + revoked: bool = False, +) -> tuple[str, str]: + """An invitation in the state the test needs, as `(id, token_hash)`.""" + async with session_factory() as session: + owner = await _make_user(session, "alice") + invitation, token = await _STORE.create( + session, + role="member", + email=None, + expires_at=expires_at if expires_at is not None else _expires_soon(), + uses_remaining=uses_remaining, + created_by=owner.id, + ) + if revoked: + invitation.revoked_at = datetime.now(UTC) + await session.commit() + return invitation.id, hashlib.sha256(token.encode()).hexdigest() + + +# The three independent ways an invitation stops working. Shared by the two +# classes below so that a fourth gate cannot be added to one and forgotten in +# the other. +_UNUSABLE: list[dict[str, object]] = [ + {"revoked": True}, + {"expires_at": datetime.now(UTC) - timedelta(seconds=1)}, + {"uses_remaining": 0}, +] +_UNUSABLE_IDS = ["revoked", "expired", "spent"] + + +async def _consume_racing( + session_factory: async_sessionmaker[AsyncSession], + invitation_id: str, + barrier: asyncio.Barrier, +) -> bool: + """Consume in a session of its own, timed to collide with the other one. + + The `SELECT 1` before the barrier is what makes the collision real. A + session that has not spoken to the database yet has no connection: the + second acceptance would spend the race connecting and authenticating, land + after the first has already committed, and read the state it was supposed + to read *concurrently with*. That test passes against a double-spending + implementation, which is the opposite of the point. + """ + async with session_factory() as session: + await session.execute(text("SELECT 1")) + await barrier.wait() + try: + await _STORE.consume(session, invitation_id) + except InvitationNotUsableError: + return False + await session.commit() + return True + + +class TestTokenGeneration: + def test_the_token_and_its_hash_are_not_the_same_string(self) -> None: + token, token_hash = generate_invitation_token() + assert token != token_hash + assert hashlib.sha256(token.encode()).hexdigest() == token_hash + + def test_two_calls_never_collide(self) -> None: + first_token, first_hash = generate_invitation_token() + second_token, second_hash = generate_invitation_token() + assert first_token != second_token + assert first_hash != second_hash + + +class TestInvitationStoreRoundTrip: + async def test_create_returns_the_token_exactly_once( + self, session_factory: async_sessionmaker[AsyncSession] + ) -> None: + """The row this call persists carries no plaintext at all — only what + `create` hands back in the same call ever exists in the clear.""" + async with session_factory() as session: + owner = await _make_user(session, "alice") + invitation, token = await _STORE.create( + session, + role="member", + email=None, + expires_at=_expires_soon(), + uses_remaining=1, + created_by=owner.id, + ) + await session.commit() + + assert token + assert invitation.token_hash == hashlib.sha256(token.encode()).hexdigest() + assert not hasattr(invitation, "token") + + async def test_created_invitation_is_found_by_the_hash_of_its_token( + self, session_factory: async_sessionmaker[AsyncSession] + ) -> None: + async with session_factory() as session: + owner = await _make_user(session, "alice") + invitation, token = await _STORE.create( + session, + role="admin", + email="invitee@example.test", + expires_at=_expires_soon(), + uses_remaining=1, + created_by=owner.id, + ) + await session.commit() + + token_hash = hashlib.sha256(token.encode()).hexdigest() + async with session_factory() as session: + found = await _STORE.get_by_token_hash(session, token_hash) + assert found is not None + assert found.id == invitation.id + assert found.role == "admin" + assert found.email == "invitee@example.test" + + async def test_an_unknown_token_hash_resolves_to_nothing( + self, session_factory: async_sessionmaker[AsyncSession] + ) -> None: + async with session_factory() as session: + assert await _STORE.get_by_token_hash(session, "no-such-hash") is None + + async def test_listing_reports_every_invitation_of_the_tenant( + self, session_factory: async_sessionmaker[AsyncSession] + ) -> None: + async with session_factory() as session: + owner = await _make_user(session, "alice") + await _STORE.create( + session, + role="member", + email=None, + expires_at=_expires_soon(), + uses_remaining=1, + created_by=owner.id, + ) + await _STORE.create( + session, + role="admin", + email="second@example.test", + expires_at=_expires_soon(), + uses_remaining=5, + created_by=owner.id, + ) + listed = await _STORE.list_for_tenant(session) + assert {i.role for i in listed} == {"member", "admin"} + + async def test_revoke_stamps_revoked_at( + self, session_factory: async_sessionmaker[AsyncSession] + ) -> None: + async with session_factory() as session: + owner = await _make_user(session, "alice") + invitation, _token = await _STORE.create( + session, + role="member", + email=None, + expires_at=_expires_soon(), + uses_remaining=1, + created_by=owner.id, + ) + await session.commit() + assert invitation.revoked_at is None + + revoked = await _STORE.revoke(session, invitation.id) + assert revoked.revoked_at is not None + + async def test_revoking_a_missing_invitation_raises( + self, session_factory: async_sessionmaker[AsyncSession] + ) -> None: + async with session_factory() as session: + with pytest.raises(ValueError, match="Invitation not found"): + await _STORE.revoke(session, "nope") + + +class TestAskingWhetherAnInvitationIsUsable: + """`get_by_token_hash` finds the row; it says nothing about the token. + + Revoked, expired and spent are three independent gates, and the reason + they are expressed once in the store rather than at each call site is that + a caller checking two of the three looks exactly like a caller checking + all three until the day it doesn't. + """ + + @pytest.mark.parametrize("kwargs", _UNUSABLE, ids=_UNUSABLE_IDS) + async def test_an_unusable_invitation_is_found_but_not_valid( + self, + session_factory: async_sessionmaker[AsyncSession], + kwargs: dict[str, object], + ) -> None: + _id, token_hash = await _make_invitation(session_factory, **kwargs) # type: ignore[arg-type] + async with session_factory() as session: + assert await _STORE.get_by_token_hash(session, token_hash) is not None, ( + "an unusable invitation is still an ordinary row" + ) + assert await _STORE.get_valid_by_token_hash(session, token_hash) is None, ( + "an unusable invitation must not read back as usable" + ) + + async def test_a_usable_invitation_reads_back_from_both( + self, session_factory: async_sessionmaker[AsyncSession] + ) -> None: + invitation_id, token_hash = await _make_invitation(session_factory) + async with session_factory() as session: + valid = await _STORE.get_valid_by_token_hash(session, token_hash) + assert valid is not None + assert valid.id == invitation_id + + +class TestConsume: + async def test_consuming_spends_one_use( + self, session_factory: async_sessionmaker[AsyncSession] + ) -> None: + invitation_id, _hash = await _make_invitation(session_factory, uses_remaining=3) + async with session_factory() as session: + consumed = await _STORE.consume(session, invitation_id) + assert consumed.uses_remaining == 2 + await session.commit() + + @pytest.mark.parametrize("kwargs", _UNUSABLE, ids=_UNUSABLE_IDS) + async def test_an_unusable_invitation_refuses_to_be_consumed( + self, + session_factory: async_sessionmaker[AsyncSession], + kwargs: dict[str, object], + ) -> None: + """The gates are in the `UPDATE`'s `WHERE`, so each of the three is + enforced by the statement that would have granted membership rather + than by whatever the caller remembered to check first.""" + invitation_id, _hash = await _make_invitation(session_factory, **kwargs) # type: ignore[arg-type] + async with session_factory() as session: + with pytest.raises(InvitationNotUsableError): + await _STORE.consume(session, invitation_id) + + async def test_consuming_an_invitation_that_does_not_exist_refuses( + self, session_factory: async_sessionmaker[AsyncSession] + ) -> None: + async with session_factory() as session: + with pytest.raises(InvitationNotUsableError): + await _STORE.consume(session, "no-such-invitation") + + async def test_two_concurrent_acceptances_of_one_use_grant_exactly_one( + self, session_factory: async_sessionmaker[AsyncSession] + ) -> None: + """The race this whole shape exists for. + + Read the row, check `uses_remaining > 0`, decrement in Python, flush: + both transactions read `1`, both write `0`, read-committed lets both + commit, and one single-use invitation grants two memberships. One + conditional `UPDATE` cannot do that — the second blocks on the row + lock, re-evaluates its `WHERE` against the committed version, matches + nothing, and refuses. + """ + invitation_id, _hash = await _make_invitation(session_factory, uses_remaining=1) + + barrier = asyncio.Barrier(2) + outcomes = await asyncio.gather( + _consume_racing(session_factory, invitation_id, barrier), + _consume_racing(session_factory, invitation_id, barrier), + ) + + assert sorted(outcomes) == [False, True], ( + f"exactly one acceptance should have won, got {outcomes}" + ) + async with session_factory() as session: + invitation = await session.get(Invitation, invitation_id) + assert invitation is not None + assert invitation.uses_remaining == 0 + + async def test_the_database_refuses_a_negative_remaining_count( + self, session_factory: async_sessionmaker[AsyncSession] + ) -> None: + """`>= 0` and not `> 0`: a spent invitation is a `0`, and it has to + stay representable. What the constraint rules out is the state below + that, which no correct decrement produces and any careless one + would.""" + async with session_factory() as session: + owner = await _make_user(session, "alice") + with pytest.raises(IntegrityError, match="ck_invitations_uses_remaining"): + await _STORE.create( + session, + role="member", + email=None, + expires_at=_expires_soon(), + uses_remaining=-1, + created_by=owner.id, + ) + + +class TestInvitationIsolation: + """The property the design doc calls out by name: an invitation belonging + to tenant A must be invisible to a session bound to tenant B.""" + + async def test_tenant_b_cannot_read_tenant_as_invitation_by_token_hash( + self, rls_harness: RLSHarness + ) -> None: + tenant_a = f"tenant-{uuid.uuid4().hex[:8]}" + tenant_b = f"tenant-{uuid.uuid4().hex[:8]}" + async with rls_harness.owner() as session: + session.add_all( + [ + Tenant(id=tenant_a, slug=tenant_a, name=tenant_a), + Tenant(id=tenant_b, slug=tenant_b, name=tenant_b), + ] + ) + await session.commit() + + async with tenant_session(rls_harness.restricted, tenant_a) as session: + owner = await _make_user(session, "alice") + _invitation, token = await _STORE.create( + session, + role="member", + email=None, + expires_at=_expires_soon(), + uses_remaining=1, + created_by=owner.id, + ) + await session.commit() + + token_hash = hashlib.sha256(token.encode()).hexdigest() + async with tenant_session(rls_harness.restricted, tenant_b) as session: + found = await _STORE.get_by_token_hash(session, token_hash) + + assert found is None, ( + "tenant B's session read tenant A's invitation through " + "InvitationStore.get_by_token_hash — the policy's USING clause " + "should have hidden it" + ) + + async def test_tenant_bs_listing_never_includes_tenant_as_invitations( + self, rls_harness: RLSHarness + ) -> None: + tenant_a = f"tenant-{uuid.uuid4().hex[:8]}" + tenant_b = f"tenant-{uuid.uuid4().hex[:8]}" + async with rls_harness.owner() as session: + session.add_all( + [ + Tenant(id=tenant_a, slug=tenant_a, name=tenant_a), + Tenant(id=tenant_b, slug=tenant_b, name=tenant_b), + ] + ) + await session.commit() + + async with tenant_session(rls_harness.restricted, tenant_a) as session: + owner_a = await _make_user(session, "alice") + await _STORE.create( + session, + role="member", + email=None, + expires_at=_expires_soon(), + uses_remaining=1, + created_by=owner_a.id, + ) + await session.commit() + + async with tenant_session(rls_harness.restricted, tenant_b) as session: + owner_b = await _make_user(session, "bob") + await _STORE.create( + session, + role="admin", + email=None, + expires_at=_expires_soon(), + uses_remaining=1, + created_by=owner_b.id, + ) + await session.commit() + + async with tenant_session(rls_harness.restricted, tenant_b) as session: + listed = await _STORE.list_for_tenant(session) + + assert [i.tenant_id for i in listed] == [tenant_b], ( + "list_for_tenant issues no tenant filter of its own; tenant A's " + "invitation reaching this listing means the policy let it through" + ) + + async def test_tenant_b_cannot_consume_tenant_as_invitation( + self, rls_harness: RLSHarness + ) -> None: + """`consume` is an `UPDATE`, and the policy's `USING` clause applies to + the rows it may reach the same way it applies to a `SELECT`. Knowing + the id is not knowing the tenant.""" + tenant_a = f"tenant-{uuid.uuid4().hex[:8]}" + tenant_b = f"tenant-{uuid.uuid4().hex[:8]}" + async with rls_harness.owner() as session: + session.add_all( + [ + Tenant(id=tenant_a, slug=tenant_a, name=tenant_a), + Tenant(id=tenant_b, slug=tenant_b, name=tenant_b), + ] + ) + await session.commit() + + async with tenant_session(rls_harness.restricted, tenant_a) as session: + owner = await _make_user(session, "alice") + invitation, _token = await _STORE.create( + session, + role="member", + email=None, + expires_at=_expires_soon(), + uses_remaining=1, + created_by=owner.id, + ) + invitation_id = invitation.id + await session.commit() + + async with tenant_session(rls_harness.restricted, tenant_b) as session: + with pytest.raises(InvitationNotUsableError): + await _STORE.consume(session, invitation_id) + + async with tenant_session(rls_harness.restricted, tenant_a) as session: + still_there = await session.get(Invitation, invitation_id) + assert still_there is not None + assert still_there.uses_remaining == 1 + + async def test_a_session_with_no_tenant_bound_cannot_read_invitations( + self, rls_harness: RLSHarness + ) -> None: + """Populated on purpose: Postgres does not evaluate a policy for a + scan that yields no rows, so this would pass vacuously against an + empty table even with no policy installed at all.""" + tenant_a = f"tenant-{uuid.uuid4().hex[:8]}" + async with rls_harness.owner() as session: + session.add(Tenant(id=tenant_a, slug=tenant_a, name=tenant_a)) + await session.commit() + + async with tenant_session(rls_harness.restricted, tenant_a) as session: + owner = await _make_user(session, "alice") + await _STORE.create( + session, + role="member", + email=None, + expires_at=_expires_soon(), + uses_remaining=1, + created_by=owner.id, + ) + await session.commit() + + async with rls_harness.restricted() as session: + with pytest.raises(DBAPIError, match="app.tenant_id is not set"): + await _STORE.list_for_tenant(session) diff --git a/core/tests/switch_core/db/test_tenant_lookup.py b/core/tests/switch_core/db/test_tenant_lookup.py index 5debb4bd3..70cb344bc 100644 --- a/core/tests/switch_core/db/test_tenant_lookup.py +++ b/core/tests/switch_core/db/test_tenant_lookup.py @@ -35,6 +35,7 @@ from __future__ import annotations import uuid +from datetime import UTC, datetime, timedelta from pathlib import Path from types import ModuleType @@ -50,6 +51,7 @@ ApiKey, Client, CollaborationBridge, + Invitation, Room, ServerConnector, Tenant, @@ -59,12 +61,14 @@ from switch_core.db.tenant_lookup import ( SECURE_SEARCH_PATH, TENANT_LOOKUPS, + TENANT_LOOKUPS_BY_NAME, TenantLookupError, all_tenant_ids, create_lookup_ddl, tenant_of_agent_oauth_client, tenant_of_api_key, tenant_of_collaboration_bridge, + tenant_of_invitation, tenant_of_room, tenant_of_server_connector, tenants_of_user, @@ -85,6 +89,15 @@ # from passing as 'we removed it from the database'. _DROPPED_SINCE = {"tenant_of_client": "b1d7c4f0a92e"} +# The same bookkeeping in the other direction: a lookup the live module names +# that `9c41a7b0e5d8` never created, and the revision that did create it. The +# frozen-copy comparison below has to know about both to stay exact — without +# this entry the only way to keep it green would be to loosen it to a subset +# check, and a subset check passes for a lookup that exists in the module and +# in no migration at all, which is a deployment whose invitation acceptance +# cannot resolve a tenant with the whole suite green. +_ADDED_SINCE = {"tenant_of_invitation": "5daaea6b674d"} + def _revision_module(revision: str) -> ModuleType: """One revision module, loaded through Alembic. @@ -118,6 +131,8 @@ def __init__(self) -> None: self.oauth_client_a: str = "" self.user_a: str = "" self.user_in_both: str = "" + self.invitation_token_hash_a: str = "" + self.invitation_token_hash_b: str = "" async def _two_populated_tenants(harness: RLSHarness) -> _Fixture: @@ -201,6 +216,16 @@ async def _two_populated_tenants(harness: RLSHarness) -> _Fixture: connection_config={}, ) session.add_all([bridge, connector]) + invitation = Invitation( + tenant_id=tenant_id, + role="member", + email=None, + token_hash=f"invitation-hash-{tag}-{suffix}", + expires_at=datetime.now(UTC) + timedelta(days=1), + uses_remaining=1, + created_by=user_both.id, + ) + session.add(invitation) session.add( Agent( tenant_id=tenant_id, @@ -224,6 +249,9 @@ async def _two_populated_tenants(harness: RLSHarness) -> _Fixture: fixture.room_a = room.id fixture.bridge_a = bridge.id fixture.connector_a = connector.id + fixture.invitation_token_hash_a = invitation.token_hash + else: + fixture.invitation_token_hash_b = invitation.token_hash await session.commit() return fixture @@ -398,6 +426,33 @@ async def test_a_room_bridge_and_connector_resolve_to_their_tenant( == fixture.tenant_a ) + async def test_an_invitation_token_hash_resolves_to_its_tenant( + self, rls_harness: RLSHarness + ) -> None: + fixture = await _two_populated_tenants(rls_harness) + assert ( + await tenant_of_invitation( + rls_harness.restricted, fixture.invitation_token_hash_a + ) + == fixture.tenant_a + ) + assert ( + await tenant_of_invitation( + rls_harness.restricted, fixture.invitation_token_hash_b + ) + == fixture.tenant_b + ) + + async def test_an_unknown_invitation_token_resolves_to_nothing( + self, rls_harness: RLSHarness + ) -> None: + """An invitation link nobody minted is a 404, not a 500 — same shape + as an unrecognised bearer token.""" + await _two_populated_tenants(rls_harness) + assert ( + await tenant_of_invitation(rls_harness.restricted, "no-such-hash") is None + ) + async def test_an_ambiguous_answer_is_refused_rather_than_picked( self, rls_harness: RLSHarness ) -> None: @@ -564,15 +619,44 @@ def test_the_frozen_list_matches_the_live_one(self) -> None: lookup.query, ) for lookup in TENANT_LOOKUPS + if lookup.name not in _ADDED_SINCE } assert frozen == live, ( "the frozen LOOKUPS in migration 9c41a7b0e5d8 no longer match " "db/tenant_lookup.py. A deployment built by Alembic would get the " "migration's functions and every test above would still pass " "against create_all's. If the divergence is deliberate, express " - "it as a new migration rather than by editing this one — and, if " - "the new migration drops a lookup, name it in _DROPPED_SINCE " - "above so this comparison stays exact rather than being loosened." + "it as a new migration rather than by editing this one — and name " + "the lookup in _DROPPED_SINCE or _ADDED_SINCE above, whichever " + "the new migration does, so this comparison stays exact rather " + "than being loosened." + ) + + def test_the_added_lookup_is_installed_by_a_revision_and_not_only_here( + self, + ) -> None: + """The mirror of the dropped-lookup test, and the more dangerous half. + + A lookup added to `db/tenant_lookup.py` is built by `create_all`, so + every test in this file exercises it and passes. A deployment's schema + is built by Alembic, which knows nothing about it: the function is + absent, and the first call — an invitation acceptance trying to + resolve a tenant — fails at runtime in production and nowhere else. + + Comparing the rendered statement rather than the pieces, for the same + reason the test below does: a revision that created the function + `SECURITY INVOKER`, or without the `search_path`, would install + something that cannot read across tenants at all. + """ + revision = _ADDED_SINCE["tenant_of_invitation"] + lookup = TENANT_LOOKUPS_BY_NAME["tenant_of_invitation"] + module = _revision_module(revision) + assert module.CREATE_TENANT_OF_INVITATION == create_lookup_ddl(lookup), ( + f"revision {revision} would install tenant_of_invitation with " + "different DDL from the one db/tenant_lookup.py builds." + ) + assert module.DROP_TENANT_OF_INVITATION == ( + f"DROP FUNCTION IF EXISTS {lookup.signature}" ) def test_the_dropped_lookup_is_dropped_by_a_revision_and_not_only_here( @@ -627,6 +711,8 @@ def test_the_statement_it_would_run_is_the_statement_the_module_builds( """ module = _migration_module() for lookup in TENANT_LOOKUPS: + if lookup.name in _ADDED_SINCE: + continue parameters = "" if lookup.parameter is None else f"{lookup.parameter} text" assert module.create_lookup_ddl( lookup.name, parameters, lookup.query