feat(db): invitations table and the lookup that resolves one to a tenant (CHOO-2722)

[wojtyniakAQ]

Rebased onto main

Was stacked on #444; that and #445 have both merged, so this is now a single commit on top of main plus a review-fix commit. Still schema only — no HTTP routes; those are #447.

Summary

• An invitations table. Tenant-scoped, so it inherits the policy by default rather than by argument. It stores a hash of the token and never the token — the plaintext is returned once, by the create call, and exists nowhere else.
• tenant_of_invitation(token_hash) as an eighth SECURITY DEFINER lookup. Resolving an invitation before any tenant is bound is the same shape as resolving a bearer token or an OIDC client id: it answers which tenant, never what row, which is the property the module argues makes its exemption defensible. The list is closed and pinned by tests against the installed functions and the migration's frozen copy, so adding to it is an edit a reviewer sees — as intended.
• Store methods: create, read by token hash, read valid by token hash, list a tenant's invitations, consume, revoke.

Review fixes (second commit)

• uses_remaining has a floor, and spending a use is one statement. ck_invitations_uses_remaining_not_negative on the table, and consume as a single conditional UPDATE … WHERE uses_remaining > 0 AND revoked_at IS NULL AND expires_at > now() … RETURNING. Read-then-write double-spends — two acceptances of a single-use invitation both read 1, both write 0, read-committed commits both, and one invite grants two memberships. The constraint is >= 0 and not > 0 because a spent invitation has to stay representable.
• The three gates are expressed once. Revoked, expired and spent were checkable only by hand, at each call site, which is how the second call site ends up checking two of three. One shared predicate now backs both get_valid_by_token_hash and consume. Refusal raises rather than returning None.
• The datetime columns are annotated as datetimes. expires_at, revoked_at and created_at were Mapped[str] on DateTime(timezone=True), which is what forced the # type: ignore on revoke's assignment — the suppression was silencing a correct complaint. The wider Mapped[str]-on-datetime pattern elsewhere in models.py is left alone.

Test plan

• just check, just typecheck — pass. Full suite: 3157 passed, 6 skipped, 11 deselected, 1 xfailed.
• Isolation proved to bite. Unscoping the table — adding it to the global list, i.e. the mistake this codebase's catalogue test exists to catch — fails four tests: a session bound to one workspace reading another's invitation by token, the same via the listing, an unbound session reading any invitation at all, and the catalogue guard itself. Restored, all pass.
• The concurrency test was worthless until it was fixed. Written the obvious way it passed against a deliberately double-spending consume: the second acceptance spent the race opening its connection and landed after the first had committed. Both sessions now issue a statement and meet at a barrier before racing, so they collide with connections already in hand — and the naive implementation then fails it on three consecutive runs.
• The other two new assertions bite. Dropping revoked_at IS NULL from the shared predicate fails both parametrised revoked cases; removing the check constraint fails the floor test.

⚠️ Still collides with Phase 4 (#435)

• Both add an eighth lookup. work/multi-tenancy-phase4 adds one for its messaging installs. Two branches claiming the same slot in a deliberately closed, test-pinned list is a semantic conflict, and each also freezes a copy of the schema in its own migration — a textual merge will produce something that passes neither test.
• Phase 4's migration is chained off b1d7c4f0a92e, which is several revisions behind main's head. It will fork the migration chain the moment it merges, independently of this branch.

Whoever merges second has to reconcile the lookup list and the frozen DDL by hand, not by rebase.

🤖 Generated with Claude Code

[petr-sandbox]

1. uses_remaining has no CHECK, and this is the PR where that gets cheap.

Nothing stops create(uses_remaining=0) or a negative, and nothing at the database level will stop a decrement going below zero later. That matters because the consume path is the next PR's, and the obvious implementation of it — read the row, check uses_remaining > 0, invitation.uses_remaining -= 1, flush — double-spends under concurrency. Two redemptions of a single-use invitation both read 1 and both issue SET uses_remaining = 0; read-committed does not stop that, both commit, and two people get membership from one invite.

Two lines here close it for good:

• CheckConstraint("uses_remaining >= 0", name="ck_invitations_uses_remaining"), so even a naive later implementation fails loudly rather than over-granting.
• A consume method on this store doing the decrement as a single conditional UPDATE … SET uses_remaining = uses_remaining - 1 WHERE id = :id AND uses_remaining > 0 RETURNING …, so the database arbitrates and the caller cannot get it wrong.

This is a schema PR, and this is a schema-shaped fix. Putting it in the routes PR means reviewing it in a diff that is mostly HTTP.

2. The store offers no way to ask whether an invitation is usable, and three separate gates to forget.

get_by_token_hash returns revoked, expired and spent invitations alike, with no signal. tenant_of_invitation's docstring says the scoped read is where "its role, its email, whether it is spent or revoked" gets checked — but there is no method that checks any of it, so every caller will hand-roll revoked_at is None and expires_at > now and uses_remaining > 0. Three independent gates, hand-rolled per call site, is how one of them gets left out of the second call site.

Either a get_valid_by_token_hash that expresses the predicate once, or an is_valid on the model. Cheap now, and it keeps the invariant in the one module that owns it.

3. The datetime annotations are wrong, and the diff contains its own proof.

expires_at: Mapped[str], revoked_at: Mapped[str | None] and created_at: Mapped[str] all sit on DateTime(timezone=True) columns, while InvitationStore.create correctly takes expires_at: datetime. The tell is in the same diff:

invitation.revoked_at = datetime.now(UTC)  # type: ignore[assignment]

That type: ignore is not working around a SQLAlchemy limitation — it is silencing a correct complaint about a wrong annotation. I changed the three to Mapped[datetime] / Mapped[datetime | None], added the import, and removed the ignore: mypy passes clean. Nothing else needed changing.

It is a pre-existing house pattern (TenantMember.created_at does the same), so I'm not asking for a sweep. But this is the first place it forces a suppression in new code, and expires_at is a value callers will compare and do arithmetic on, so it's the worst one to leave as str.

[petr-sandbox]

Just some notes.

[petr-sandbox]

Rebased onto main (#444 and #445 both merged; base retargeted) and pushed the three fixes as a second commit.

1 — uses_remaining has no CHECK, and the consume path double-spends. Both closed here. ck_invitations_uses_remaining_not_negative on the table, and a consume that does the decrement as one conditional UPDATE … WHERE uses_remaining > 0 AND revoked_at IS NULL AND expires_at > now() … RETURNING, so Postgres arbitrates and the caller cannot get it wrong. >= 0 rather than > 0 because a spent invitation is a 0 and has to stay representable.

2 — no way to ask whether an invitation is usable. One shared predicate now backs get_valid_by_token_hash and consume, so the three gates are written once in the module that owns the invariant. Refusal from consume raises InvitationNotUsableError rather than returning None — revoked, expired and spent are states the caller answers for, not states it retries. tenant_of_invitation's docstring now names the two methods that actually enforce what it promised.

3 — the datetime annotations. Fixed exactly as described; the type: ignore is gone and mypy is clean. The wider Mapped[str]-on-datetime pattern elsewhere is left alone.

One thing worth flagging, because it nearly got past me.

The concurrency test was worthless as first written. I ran it against a deliberately naive consume — read the row, check, subtract in Python, flush — and it passed. Two asyncio.gather'd acceptances do not actually collide when one of them has to open a connection first: the second spends the race on TCP and auth, lands after the first has committed, reads 0, and correctly refuses. Exactly the observation the test was supposed to make impossible, arrived at for the wrong reason.

Both sessions now issue a statement before meeting at an asyncio.Barrier, so they race with connections already in hand. The naive implementation then fails on three consecutive runs, and the real one passes. Worth remembering for any future race test in this suite — a pass is not evidence until you have watched it fail.

Full suite: 3157 passed, 6 skipped, 11 deselected, 1 xfailed. just check and just typecheck clean.

🤖 Generated with Claude Code