feat(coderd): support public OAuth2 client tokens at the schema layer - #27712
Conversation
74da348 to
eba9acf
Compare
BobbyHo
changed the title
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.9.0 | Round 2 | Last posted: Round 2, 8 findings (1 P1, 3 P2, 2 P3, 2 Note), COMMENT. Review Finding inventoryFinding inventory: PR #27712Findings
Contested and acknowledgedNone. Round logRound 1Netero-only first pass. 1 P2, 1 P3, 1 Note. Panel not yet spawned (Netero gate: P2 present). Round 2 updateChurn guard: PROCEED. CRF-1 and CRF-2 have targeting code changes in 0853bb0 (author claims, unverified by the panel). CRF-3 silent, non-gating (Note requested no change). Round 2 (continued)Netero-only round 2 (Netero gate: P1/P2 present, panel not spawned). This is the second consecutive Netero-only round, so round 3 goes to the panel regardless of Netero's findings. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
First-pass review only. These are mechanical findings from a single first-pass reviewer; the full review panel has not yet looked at this PR and will do so once these are addressed.
The split itself is good work: the migration is genuinely independently mergeable, the backfill uses the same join revoke.go already performs at request time, and the test is not vacuous. It reads 000562_..._up.sql from disk, so it cannot pass without the migration file, and it pins both nullability facts the PR description promises. Regenerating dump.sql from the migration produces no diff, so the checked-in schema matches what the migration actually does.
1 P2, 1 P3, 1 Note.
The P2 is the one that matters: the column is backfilled once and then never written, so the table splits into two populations the moment this ships. The first-pass reviewer put it plainly: "Assume PR2 never lands: the column is permanently half-populated and any future consumer that trusts it, including the promised NOT NULL migration, has to re-backfill and re-reason about it." This PR is reviewed on the assumption that no follow-up arrives, so "PR2 populates it" is not a resolution. The fix stays inside coderd/database.
🤖 This review was automatically generated with Coder Agents.
…ema layer Add app_id to oauth2_provider_app_tokens and make app_secret_id nullable, so a public (secretless, PKCE-only) client's tokens can still be attributed to their owning app for revocation and listing, without joining through a secret that does not exist. This is a single, final migration: app_id is populated at insert time from day one, so there is no window where new rows are written with a NULL app_id, and no second backfill will ever be needed. Fixes a review finding from an earlier version of this branch: the migration previously only added app_id as an unenforced, unpopulated column, deferring app_secret_id's nullability and app_id's write path to a later PR. That left a gap where any token created between this PR merging and the next one landing would carry a NULL app_id permanently, defeating the column's purpose. Doing the full migration now, with the insert path updated in the same change, closes that gap entirely. This requires touching a few call sites outside coderd/database, but only mechanically: revoke.go's ownership checks now compare app_id directly instead of joining through app_secret_id (a genuine simplification, not a shim), and tokens.go's two insert call sites supply the new app_id column and wrap app_secret_id as a NullUUID. No client-type branching exists yet, no public client can be registered, and behavior for today's confidential-only clients is unchanged. That capability (registration skipping secret generation, the token endpoint accepting PKCE alone, discovery advertising "none") lands in a follow-up PR. Layer 1 of a multi-PR split of #27195 (public/secretless PKCE-only OAuth2 clients), broken up for easier review: database schema (this PR), then the oauth2provider handler logic, then API/e2e integration tests. Co-Authored-By: Claude Sonnet 5 <[email protected]>
7e6b855 to
0853bb0
Compare
BobbyHo
changed the title
BobbyHo
changed the title
|
/coder-agents-review |
There was a problem hiding this comment.
Still a first-pass review. These are mechanical findings from a single first-pass reviewer; the full panel has not reviewed this PR yet and will do so next round regardless of what happens here.
Both round 1 findings are fixed, and the fix went further than asked in the right direction. app_id is now written by the insert path, ALTER COLUMN app_id SET NOT NULL turns a missed write into a hard failure instead of a silent NULL, app_secret_id is loosened only after the backfill has run, and the down migration documents that it will refuse to run while public-client rows exist. revoke.go and both list/delete queries were moved off the secret join in the same pass, so no ownership lookup routes through app_secret_id any more.
That expansion is also what this round is about. The PR is no longer a schema-layer change: it now rewrites the ownership check on an unauthenticated endpoint. 1 P1, 2 P2, 1 P3, 1 Note.
The P1 is a consequence of the fix, not of the original code. app_id is now written from app.ID, which extractOAuth2ProviderAppBase resolves from the request's client_id with no secret verification, and the refresh grant requires no client_secret at all. revoke.go then trusts that column as the ownership boundary. The first-pass reviewer traced it: "the token endpoint never binds the presented credential to the authenticated client, and this PR promotes that unbound value to the ownership column." I verified both grants by reading: neither compares dbCode.AppID, dbSecret.AppID, nor dbToken.AppID to app.ID.
One schema-level side effect no reviewer will trace, because there is no code path to follow: oauth2_provider_app_tokens_app_secret_id_fkey is still ON DELETE CASCADE. Deleting or rotating a client secret still deletes the token rows, even though app_id would now identify the owning app without the secret. That is pre-existing behavior and this PR does not change it, but the PR's premise is that app_id is the ownership column and app_secret_id is optional. Worth deciding deliberately whether secret deletion should still end sessions, rather than inheriting it.
Nothing was run against Postgres in either the reviewer's worktree or mine, so no finding this round is backed by execution. All of it is from reading and grep, and the review says so where it matters.
🤖 This review was automatically generated with Coder Agents.
…lient extractOAuth2ProviderAppBase resolves the app purely from the request's client_id (URL param, query, form, or Basic auth username) with no secret verification at that stage. authorizationCodeGrant and refreshTokenGrant both wrote the new app_id column from that unauthenticated value without checking it against the credential actually being redeemed, so a request presenting a valid secret/code/ refresh token for one app, but a different app's client_id, would mint or refresh a token attributed to the wrong app. Since revoke.go now checks app_id directly (rather than joining through app_secret_id, which is null for public clients), a stolen refresh token could be refreshed under an attacker-chosen client_id, re-parenting the token's app_id so the app that actually issued it could no longer revoke it through RFC 7009 or the per-app token list. Add the missing checks: authorizationCodeGrant now rejects when dbSecret.AppID or dbCode.AppID doesn't match the request's client_id, and refreshTokenGrant rejects when dbToken.AppID doesn't match. Each reuses the grant's existing error for that credential (errBadSecret, errBadCode, errBadToken) rather than a distinguishable "wrong app" error, and the inserted app_id is now sourced from the validated credential (dbCode.AppID, dbToken.AppID) instead of the request. Add regression coverage: TestOAuth2ProviderTokenExchange gains a case for a secret belonging to a different app than client_id, a new TestOAuth2ProviderTokenExchangeCodeBelongsToDifferentApp isolates the code-ownership check (it can only be reached when the two apps involved share an identical callback URL, since a request-level redirect_uri check would otherwise mask it behind an unrelated mismatch error), TestOAuth2ProviderTokenRefresh gains a cross-app refresh case, and a new TestOAuth2ProviderRevokeCrossApp covers both the access-token and refresh-token revocation branches revoking under a different app than the one that issued the token. Fixes CRF-4, CRF-5, and CRF-6 from #27712 (review) Co-Authored-By: Claude Sonnet 5 <[email protected]>
becd1b3 to
83d7734
Compare
…et_id
Both `takeFirst` defaults on `OAuth2ProviderAppToken` are unreachable or
broken. `app_secret_id` has an FK to `oauth2_provider_app_secrets(id)`,
so defaulting it to a random UUID was always a constraint violation, and
this branch's new FK on `app_id` does the same to that default. They
survive only because every caller overrides them.
Making `app_secret_id` nullable also made NULL a legal value that
`takeFirst` cannot express, since NULL is its "unset" sentinel: a caller
passing `uuid.NullUUID{}` to seed a public client's secretless token
silently gets a random secret ID instead.
Pass `app_secret_id` through verbatim, and require `app_id` with an
assertion naming the helper to build the parent, matching
`dbgen.GroupMember`. All existing callers already set both fields.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
2 participants
Layer 1 of a multi-PR split of #27195 (public/secretless PKCE-only OAuth2 clients), broken up for easier review: database schema (this PR) → oauth2provider handler logic → API/e2e integration tests.
Goal
Coder's OAuth2 provider only works correctly for confidential clients today. Public clients — native apps that can't safely hold a shared secret, such as the CLI's browser-based login flow, IDE plugins (VS Code, JetBrains), desktop apps, and MCP clients — cannot complete a real OAuth2 flow against Coder, even though OAuth 2.1 §2.1 explicitly defines this client type and RFC 8252 §8.5 requires PKCE alone to be sufficient authentication for it. Every MCP client, CLI login flow, and IDE plugin is a public client by construction, and none of them can complete a secretless flow against Coder today: dynamic registration always classifies a client as confidential regardless of what it asks for, the token endpoint unconditionally requires a
client_secret, and discovery metadata never advertises"none"as a supported auth method.Full write-up: ENG-3029
Overall design (end state across the full PR stack)
[PR2]marks handler-layer changes landing in the next PR in this stack. The green box is what this PR implements.sequenceDiagram autonumber participant C as Public Client (CLI/MCP/IDE plugin) participant S as coderd (chi router) participant H as oauth2provider handlers participant DB as PostgreSQL Note over C,S: Discovery C->>S: GET /.well-known/oauth-authorization-server S->>H: GetAuthorizationServerMetadata() Note over H: [PR2] add "none" to<br/>the returned auth methods list H-->>C: [PR2] 200 { token_endpoint_auth_methods_supported:<br/>[..., "none"] } Note over C,S: Dynamic Client Registration C->>S: POST /oauth2/register<br/>{redirect_uris, token_endpoint_auth_method: "none"} S->>H: CreateDynamicClientRegistration() Note over H: [PR2] client type now reads<br/>the request -> "public" Note over H: [PR2] skip secret generation<br/>for public clients H->>DB: [PR2] INSERT app row<br/>(client_type = 'public') DB-->>H: app row Note over H: [PR2] skip secret insert entirely H-->>C: [PR2] 201 { client_id }<br/>(no client_secret field) Note over C,S: Authorization Code + PKCE flow C->>S: GET /oauth2/authorize?client_id=...&code_challenge=... C->>S: POST /oauth2/tokens (grant_type=authorization_code)<br/>no client_secret S->>H: extractTokenRequest() Note over H: [PR2] client_secret no longer required<br/>for public clients H->>H: authorizationCodeGrant() Note over H: [PR2] skip secret lookup for public clients Note over H: PKCE verification — already mandatory, unchanged rect rgb(198, 239, 206) Note over H,DB: [THIS PR] oauth2_provider_app_tokens.app_id<br/>column added (NOT NULL, populated at insert<br/>time from app.ID) and app_secret_id loosened<br/>to nullable. Revocation now checks app_id<br/>directly. Confidential-client behavior is<br/>unchanged — no public client can be created yet. H->>DB: [PR2] INSERT refresh token row<br/>(no secret reference, for public clients) end DB-->>H: token row H-->>C: 200 { access_token, refresh_token }This PR: database schema
A public client has no
client_secret, so it has nothing to put inoauth2_provider_app_tokens.app_secret_id, which wasNOT NULL. This PR makes that column nullable and instead attributes a token to its owning app through a new, always-populatedapp_idcolumn — so ownership checks (e.g. revocation) work identically for public and confidential clients without joining through a secret that may not exist.app_secret_iduuid NOT NULLapp_iduuid NOT NULL,FOREIGN KEY → oauth2_provider_apps(id) ON DELETE CASCADE, backfilled for every existing row and populated on every new insert from that point onThis is a single, complete migration — not staged across multiple PRs. An earlier version of this branch deferred
app_secret_id's nullability and the insert-time population ofapp_idto a later PR, keeping this PR's diff limited tocoderd/database. Automated review correctly flagged that as unsafe: the migration would backfill existing rows once, but nothing would populateapp_idfor rows written afterward, so the moment this PR merged, new tokens would start accumulating a permanentlyNULLapp_id — and if a release happened to be cut before the follow-up PR landed, that gap could ship to customers and would need a second, later backfill to close. Doing the full migration now avoids that:app_idis correct from the first row written, and the promisedNOT NULLconstraint requires no data repair because it's already enforced.Closing that gap requires a few mechanical, non-branching touches outside
coderd/database:revoke.go's two ownership checks now comparedbToken.AppIDdirectly instead of looking up the app throughapp_secret_id— a genuine simplification (and slightly less code), not a temporary shim.tokens.go's twoInsertOAuth2ProviderAppTokencall sites supply the newapp_idcolumn and wrapapp_secret_idas aNullUUID.oauth2_test.go's one direct-insert test fixture does the same.None of these introduce client-type branching or new capability — every client today is still confidential-only, still always presents a secret, and behavior is unchanged. The full repo builds, vets, and all existing tests pass unmodified in behavior.
Coming next
codersdk'sDetermineClientType()reading the requestedtoken_endpoint_auth_method;registration.goskipping secret generation for public clients (and wrapping the app+secret insert in a single transaction, fixing a pre-existing orphan-row/visibility-race gap);tokens.gomaking the secret check conditional so PKCE alone authenticates a public client;metadata.goadvertising"none"in discovery. No further migration is needed — the schema this PR ships is already final.coderd/oauth2_test.go), the MCP OAuth2 e2e flow (coderd/mcp/mcp_e2e_test.go), and the manual test script (scripts/oauth2/test-mcp-oauth2.sh).Depends on: #27195 (original combined PR, being superseded by this stack)