feat(oauth): self-service tenant OAuth clients - #1489
Conversation
Review of #1489 found four defects and one missing ceiling. An owner could delete an operator-published client. `update_client/3` guarded publication, `delete_client/2` did not, and the row stays owned after publication, so the owner could remove a registration every account signs in through. The `client_id` is random, so nobody could recreate it. `delete_client/2` now refuses, the console hides Delete on a published client the way it already hid Edit, and the operation declares the 422. `get_client_record/2` cast a raw path segment to `:binary_id`, so `/api/oauth/clients/foo` raised a CastError that phoenix_ecto turned into a 400 rather than the 404 the operation documents. It takes the same `valid_uuid?/1` guard `Fountain.Connections` uses. The clients LiveView had no `save` clause for `editing: nil`, so a submit arriving behind the cancel that closed the form killed the view with a CaseClauseError. It is a no-op now. The migration was numbered 20260829010000, before five migrations already merged. Renumbered to sort last, so `--strict-version-order` and `ecto.rollback -n` still walk the repo in authoring order. Registration was unbounded, and every row widens the deployment CORS allowlist. Twenty-five clients per account, an abuse ceiling rather than an allowance, matching how TEAM_CONTACT_CEILING reads. The global-CORS and any-port loopback behaviour the review also raised is ADR 0021's deliberate decision, and stays. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qk6BWNeqca5SAh7nWeJhg8
BinaryBourbon
left a comment
BinaryBourbon
left a comment
There was a problem hiding this comment.
Requesting changes — the branch is too far behind to review, and there's a validation claim in the description I don't want to let pass.
210 commits behind main, conflicting in 10 files, including sdk/contract/contract.json, sdk/contract/omissions.json, sdk/typescript/src/generated/openapi.ts, both SDK version files, docs/api.md and audit_guardrail_test.exs. The last full run here was September 3rd, so every green check is against a main that has since taken the acp runtime stack, the apply-kinds and labels stacks, and two SDK minor releases. Those checks aren't evidence any more.
The contract and generated-openapi conflicts are the ones to be careful with: contract.json and openapi.ts are both generated, so resolving them by hand produces something that looks plausible and is wrong. They need regenerating on the rebased tree, not merging.
~3,074 lines across 41 files. This is the largest open PR we have, and it's past the line we just drew — #1678 was closed and re-cut as nine PRs at less than half this size. The seams here are unusually clean: the client registry + migration, development-vs-published mode, CORS origins derived from redirects (RFC 8252 loopback included), the consent CSP narrowing, the audit events, then console / API / CLI / docs. Any one of those is a reviewable PR. All of them together is not.
Separately, this line needs following up rather than shipping:
full Elixir suite ran 4,226 tests; 9 parallel database checkout timeouts all passed when rerun serially
Nine checkout timeouts is not a rerun-and-move-on result. CLAUDE.md is explicit that a test which fails and then passes with no code change gets investigated and filed, and pool exhaustion in particular is called out — the pool is 20 and the instruction is not to lower it precisely because this is what it looks like when something holds connections too long. Nine at once, in a PR that adds OAuth client registration and LiveView surfaces, is more likely to be this branch holding checkouts than ambient flakiness. Worth finding out which before it becomes someone else's intermittent CI failure.
Recommendation: close and re-cut on current main as a stack. If you keep it open, rebase first and regenerate the contract artifacts, then I'll review it properly.
|
Re-cut as a stack of nine on
Every decision in this PR is carried, including the ones the description does not spell out: the identity check running before the redirect check so a stranger's error page discloses no registration, the Three things changed.
On the nine checkout timeouts in the description. They are worth more than the sentence they got, and they are not this branch. Three local full-suite runs, same machine, same private database: The stack's tip runs the full core suite at 5,007 tests, 0 failures. 🤖 Generated with Claude Code |
`GET/POST /api/oauth/clients` and `GET/PATCH/DELETE /api/oauth/clients/:id`, with the OpenAPI operations, the three schemas and the regenerated wire contract. Full scope, not the sprite scope #1125 first proposed. A registered client is a standing way to obtain a full-scope thirty-day key with one consent, which is exactly the escalation the sprite scope exists to prevent: a sandbox's per-conversation token must not be able to leave one behind. Registering from the console, the CLI or the API with the owner's own key still removes the operator, which was the point. Two details worth naming. Another account's client answers 404 rather than 403, like every other tenant-scoped resource, and so does an id that is not a UUID -- casting a path segment to `:binary_id` raises, and phoenix_ecto turns that into a 400, so the documented 404 would never reach anybody who mistyped an id. `origins` is derived for the response rather than being the stored lookup key, so a caller sees the origins it will actually be calling `/api` from. The routes are recorded in `sdk/contract/omissions.json` rather than claimed by an SDK: registration is a once-per-app setup step a person does in the console or with `fountain oauth-client`, and an SDK caller's work starts after it, with the key the flow mints. #1489 declared them in neither place, which would have failed `scripts/sdk-contract/build.sh --check`. `sdk-no-release`: `src/generated/openapi.ts` is regenerated here with no version bump. The bump is in the last PR of the stack. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S9jevFQT5MkF3rJUieeiHW
`GET/POST /api/oauth/clients` and `GET/PATCH/DELETE /api/oauth/clients/:id`, with the OpenAPI operations, the three schemas and the regenerated wire contract. Full scope, not the sprite scope #1125 first proposed. A registered client is a standing way to obtain a full-scope thirty-day key with one consent, which is exactly the escalation the sprite scope exists to prevent: a sandbox's per-conversation token must not be able to leave one behind. Registering from the console, the CLI or the API with the owner's own key still removes the operator, which was the point. Two details worth naming. Another account's client answers 404 rather than 403, like every other tenant-scoped resource, and so does an id that is not a UUID -- casting a path segment to `:binary_id` raises, and phoenix_ecto turns that into a 400, so the documented 404 would never reach anybody who mistyped an id. `origins` is derived for the response rather than being the stored lookup key, so a caller sees the origins it will actually be calling `/api` from. The routes are recorded in `sdk/contract/omissions.json` rather than claimed by an SDK: registration is a once-per-app setup step a person does in the console or with `fountain oauth-client`, and an SDK caller's work starts after it, with the key the flow mints. #1489 declared them in neither place, which would have failed `scripts/sdk-contract/build.sh --check`. `sdk-no-release`: `src/generated/openapi.ts` is regenerated here with no version bump. The bump is in the last PR of the stack. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S9jevFQT5MkF3rJUieeiHW
2 participants
Summary
This replaces #1265 with an implementation based on current main and the extracted Managoat.OAuth state machine.
Closes #1125.
Verification