feat(oauth): a row a tenant can register an OAuth client into (#1125)

[jhgaylor]

Re-cut of #1489 as a stack of nine on main, under the no-big-PRs rule. The original was 3,074 lines across 41 files and 210 commits behind; this is re-derived against today's tree rather than rebased.

This is the row and nothing else: the oauth_clients table and Fountain.OAuth.Client. ADR 0021 kept the client registry in application config because there were two clients and both were ours; this is the table for everybody else's.

What the schema decides:

• client_id is generated here and never accepted from the caller, because it is the name the whole flow is keyed on.
• published and user_id are never cast from caller attributes.
• A redirect URI must be https unless its host is loopback, must have a host, and must not carry a fragment.
• origin_keys is derived from redirect_uris on every write and GIN-indexed, so a later CORS preflight — which carries no authentication — is one indexed containment rather than a scan over parsed URIs. The key drops the port on loopback, because RFC 8252 §7.3 lets a loopback redirect match on any port and a CORS rule that did not would half-work the moment Vite moved off 5173.

Nothing reads the table yet. Part 1 of 9 for #1125; the context that writes it is #1877.

🤖 Generated with Claude Code

https://claude.ai/code/session_01S9jevFQT5MkF3rJUieeiHW

[BinaryBourbon]

Reviewed as the base of the nine-PR stack:1125 chain; I read all nine in order before writing this. The stack-level read first, then this PR.

The stack

The security argument is the right one and it is argued in the code rather than in the PR body: development mode is the boundary, not the redirect allowlist. authorizable_by?/2 runs before redirect_registered?/2, so a stranger's error page discloses nothing about someone else's registration, and #1878 pins that with a test that asks for a wrong redirect as a stranger and still asserts :development_mode. The "wrong answer, on the record" paragraph in the ADR amendment is worth keeping — wildcarding the sandbox domain is exactly the mistake a later reader would make.

I checked the paths that could route around the owner gate and found none:

• OAuth.authorize/3 re-runs validate_request/2 itself rather than trusting the controller, so the POST /oauth/authorize decision cannot be forged past the GET (tested).
• The device grant takes no client_id (start_device_grant/0), so it is not a second door into the registry.
• Managoat.OAuth.authorize/4 has exactly one call site, inside the overridden authorize/3.
• Codes.exchange/3 binds to the stored client_id and redirect_uri and never consults the registry, so handing it a DB client is safe and the loopback code stays bound to the port that asked for it.

grant_client/2 narrowing the config to [requested redirect_uri] before handing it to the library is the part that will look alarming to a future reader, since it makes the library's own exact-match a tautology. The comment says so, which is the right call, but it does mean redirect_registered?/2 is now the only redirect gate in the system — worth keeping that sentence in the moduledoc if this ever gets refactored.

Findings by PR: #1882 has a red required check and one audit-rule violation (details there). Everything else is nits, listed on the PR they belong to.

This PR

The row is well shaped. client_id generated and never cast, published and user_id never cast, three separate tests for each of those rather than one combined — good. The origin_key / origin_of split (stored lookup key vs. displayable origin) is the sort of distinction that usually gets collapsed six months later; the docstring explains why it must not be.

Two nits, neither blocking:

The URI cap counts before the dedup. validate_redirect_uris/1 binds uris from get_field/2, put_changees Enum.uniq(uris), then runs length(uris) > @max_redirect_uris and the per-URI loop against the pre-uniq list. Eleven copies of one URI is rejected as "at most 10 redirect URIs" though one would be stored, and a duplicated bad URI adds the same error message twice. Validating the uniq'd list reads better and is a one-line move.

uri_error/1 accepts userinfo. https://user:pass@notes.test/cb passes every branch. Harmless here — the redirect only ever reaches its owner — but if published ever becomes self-serve it is the kind of thing that wants to already be refused.

Approving. decisions/0021 gets the amendment in #1885, which is the right place for it.

[jhgaylor]

Review feedback applied; force-pushed (the commit is amended, not appended, so the stack stays one commit per PR).

• The cap now counts the deduplicated list. validate_redirect_uris/1 deduplicates first, so eleven copies of one URI is one URI rather than an "at most 10" rejection, and a repeated bad URI is reported once instead of once per copy. Two tests.
• uri_error/1 rejects userinfo. https://user:pw@notes.test/cb is refused rather than accepted. It only ever reached its owner, so this is hardening ahead of published, not a live hole — and it makes the userinfo comparison in loopback_match?/2 (feat(oauth): a registered client signs in its owner and nobody else (#1125) #1878) mean something on both sides.

Both new tests were checked by reverting the fix: they fail without it.

[BinaryBourbon]

Re-read after the push. Both nits from the first pass are fixed, and fixed in the way that leaves the reason behind.

The cap now counts what gets stored. uris is bound from Enum.uniq(get_field(...)) once and every branch below reads that binding, so the cond and the per-URI reduce see the same list put_change writes. Collapsing the |> then(fn cs -> ... end) wrapper into a plain rebind was the right call too — the wrapper was only there because the old code needed the pre-uniq list alive.

Two tests rather than one is the right split: eleven copies not hitting the cap, and a repeated bad URI producing exactly one error. The second is the one that would have quietly regressed.

userinfo is refused, and it sits in the right place in the cond — after the host check, so https://user:pw@ reports the userinfo rather than something vaguer, and before the loopback/https branch, so http://user:pw@localhost/cb reports the userinfo rather than passing. The uri_error/1 table test picked up the case alongside the others.

One thing worth knowing, not a change: put_change/3 deletes the change when the value equals the data's, which is what makes an identical redirect_uris list a genuine no-op — and that is load-bearing for the no-op update fix in #1877. It works, but the two are now coupled through an Ecto detail neither file names.

Approving.