feat: add oauth2 scope columns and single-use delete queries - #28007
Conversation
Migration 000567 adds a nullable `scope text` to oauth2_provider_app_codes and oauth2_provider_app_tokens so the scope negotiated at /oauth2/authorize can travel from a code to the token it is exchanged for. No backfill, and every insert writes NULL for now, which reads as unrestricted access, so behavior is unchanged. DeleteOAuth2ProviderAppCodeByIDReturningID and DeleteAPIKeyByIDReturningID return sql.ErrNoRows when the row is already gone, letting the grant paths enforce single use without a read-then-write race. The existing blind deletes and their call sites are unchanged. Refs PLAT-478
|
/coder-agents-review |
|
Chat: Review posted | View chat Review history
deep-review v0.9.0 | Round 1 | Last posted: Round 1, 15 findings (6 P2, 3 P3, 1 P4, 2 Nit, 3 Note), COMMENT. Review Finding inventoryFinding inventory, PR #28007Findings
Contested and acknowledgedNone yet. Round logRound 1Netero first pass: 1 Note (merged into CRF-1). Panel of 19 (17 trigger-matched + wildcards Meruem, Zoro). 6 P2, 3 P3, 1 P4, 2 Nit, 3 Note posted; 2 dropped. Convergence: single-use contract untested (11 reviewers), fail-open NULL encoding (6), scope not carried forward (3). Structural alternative preserved: RETURNING * + fetchAndQuery (Robin, Zoro). Contradiction flagged: Gon (header bloat) vs Leorio (header praise). Severity tiebreakers applied upward on CRF-1 (P2 over eight P3s) and CRF-4 (Razor P2 over Knov/Chopper P3; downgrade case was probability-based, consequence identical to CRF-3). Reviewed against 87fdd2b..7efa327. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
BobbyHo
changed the title
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is disciplined groundwork: additive nullable columns with metadata-only ALTERs, an exact-inverse down migration, NULL semantics recorded via COMMENT ON COLUMN so they survive into dump.sql and models.go, new queries added alongside the blind :exec deletes instead of repurposing them (all 8 DeleteAPIKeyByID call sites untouched), and follow-ups tracked in named Linear issues. The panel verified the central claims rather than trusting the description: Hisoka reproduced the concurrent DELETE ... RETURNING race against live Postgres (one winner, loser blocks on the row lock and gets zero rows at READ COMMITTED), Komugi traced the dbauthz Get-then-delete and confirmed the DELETE remains the sole arbiter so the TOCTOU is benign, and Razor/Kite confirmed the ON DELETE CASCADE from tokens to api_keys makes the key the correct single-use arbiter for refresh. Mafu-san audited every factual claim in the PR description against the tree and found it survives auditing. As Leorio put it about the query docs: "YES. This is how you document a query."
Findings: 6 P2, 3 P3, 1 P4, 2 Nit, 3 Note. No P0/P1, so this is a COMMENT review. The P2s cluster around three decisions that are cheapest to make now, while the columns have zero writers: (1) the single-use contract both new queries exist for is pinned by no test, and the api_keys variant's SQL never executes anywhere (CRF-1); (2) NULL-means-unrestricted makes the zero value the most privileged state, a fail-open encoding six reviewers flagged independently (CRF-2); (3) the two token-insert sites hardcode NULL instead of carrying the scope forward, contradicting the narrowing invariant this same PR writes into the schema, and the fix is behavior-neutral today (CRF-3, CRF-4). One structural alternative worth weighing before merge: switching the queries to RETURNING * lets both hand-written dbauthz wrappers collapse into the existing fetchAndQuery generic and hands phase-2 callers the deleted row instead of an ID they already had (CRF-9).
One genuine panel disagreement, flagged rather than resolved: Gon audited all 8 new comments and found four restate the NULL-semantics fact that lives at the COMMENT ON COLUMN definition (CRF-7, CRF-8), while Leorio praised the same writing as the standard the codebase should follow. Both agree on one thing: the present-tense enforcement claims describe code that does not exist yet (CRF-6). Whether the duplicates are deleted or reworded is the author's call; leaving them verbatim is the one option both reviewers reject.
Process notes: the failing "Pixel / Review" CI check appears to be an external app check rather than a build or test failure (no Pixel workflow exists in .github/workflows); worth a human confirming it is not actionable. CRF-14 sits outside this diff but directly under phase 2's feet: the expandRBACScope doc comment promises a ScopeAll fallback the code does not have.
coderd/database/modelmethods.go:288
Note [CRF-14] Adjacent to this PR's domain: the comment on expandRBACScope promises it "defaults to rbac.ScopeAll for backward compatibility" when the list is empty, but the code returns xerrors.New("no scopes provided"). (Mafuuu)
Outside this diff, but it is the enforcement engine the PR description says only needs real data fed into it, and its documented fallback contradicts its behavior. Whoever wires phase 2 will read that comment.
🤖
🤖 This review was automatically generated with Coder Agents.
Both scope columns were nullable with NULL meaning "unrestricted", which
made the most privileged state the one a forgotten field produces:
sql.NullString{} is NULL is full access, and exhaustruct is satisfied by
exactly that literal. An audit of either table could not separate a
deliberate legacy grant from a mint path that dropped the scope.
Backfill both columns to coder:all, which records what existing rows
already have in fact since apikey.Generate defaults minted OAuth2 keys to
that scope, then apply NOT NULL and CHECK (scope <> ''). NOT NULL alone
would not be enough: sqlc maps text NOT NULL to a Go string whose zero
value inserts cleanly, so the fail-closed property needs both clauses. No
DEFAULT survives, or an INSERT omitting the column would silently receive
an unrestricted grant. Matches the encoding api_keys.scopes and
workspace_agents.api_key_scope already use, and follows migration 000389's
backfill-then-constrain shape.
The two grant paths now carry the parent's scope forward
(Scope: dbCode.Scope, Scope: dbToken.Scope) instead of hardcoding an empty
value, which is RFC 6749 section 6's default and removes the phase-ordering
hazard where a scoped token could refresh into an unrestricted one.
ProcessAuthorize writes the sentinel, since persisting a requested scope
before validation exists would store unvalidated client input.
Refs PLAT-478
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…etes Both single-use deletes returned a bare id, which forced a hand-written dbauthz wrapper each. Returning the whole row lets them collapse into the existing fetchAndQuery generic, since that helper unifies its fetch and query on one rbac.Objecter and a bare id satisfies no such interface. Each 10-line wrapper becomes a single call, and a caller now reads the deleted row's state, including a code's negotiated scope, from the same atomic delete rather than trusting an earlier unauthorized read. Renamed to ...ByIDReturningRow, since ...ReturningID no longer describes them. Add TestSingleUseDeleteByIDReturningRow, which pins the contract both queries exist for: the first delete returns the row, a second returns sql.ErrNoRows. Neither query previously executed against a real database on its already-gone path, so converting one back to :exec or adding a soft delete would have broken single use with CI still green. The concurrent exactly-one-winner half is deliberately not covered here; it exercises Postgres row-lock semantics rather than this code. Rename migration 000567 to oauth2_scope_columns. It adds columns and constraints; enforcement lands in a later phase, and migration names freeze at merge. Refs PLAT-478 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…000569 origin/main merged 000567_chat_file_purge_indexes and 000568_service_account_notifications after this branch's point. CI validates the PR merge, where two files numbered 000567 coexisted and the migrate iofs driver panicked with "duplicate migration file", taking down gen, lint, sqlc-vet and every test-go-pg job. Git reports the merge as MERGEABLE because the two are different filenames; the collision is on the version number, which git cannot see. Renumbered with ./coderd/database/migrations/fix_migration_numbers.sh. Refs PLAT-478 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two test sites built InsertOAuth2ProviderAppCodeParams and
InsertOAuth2ProviderAppTokenParams without Scope, so after the columns became
NOT NULL with CHECK (scope <> '') they inserted an empty string and tripped the
constraint. Broke TestOAuth2ProviderTokenExchange/ExpiredCode and every
TestOAuth2ProviderTokenRefresh subtest on the Linux postgres jobs.
exhaustruct is disabled for _test.go (.golangci.yaml:222), so nothing forces
the field in tests and the constraint is the only backstop. Audited every
remaining InsertOAuth2ProviderApp{Code,Token}Params literal in the tree; these
two were the only omissions, and no raw SQL inserts bypass sqlc.
Refs PLAT-478
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
OAuth2ScopeUnrestricted was an alias for ApiKeyScopeCoderAll, so the unrestricted grant had two spellings while every other call site (coderd/apikey.go, coderd/apikey/apikey.go, coderd/users.go) names ApiKeyScopeCoderAll directly. Use that name at the oauth2 code and token sites too, with an explicit string conversion marking where the api_key_scope enum crosses into the text columns. The alias carried no enforcement. The property that a grant's authority is always stated, and that a caller omitting the column fails rather than receiving full access, comes from NOT NULL plus CHECK (scope <> '') in migration 000569 and is unaffected.
…hub.com/coder/coder into coder-oauth2-scope-enforcement-plat-470
DeleteAPIKeyByIDReturningRow and DeleteOAuth2ProviderAppCodeByIDReturningRow had no production caller, here or on the Phase 2 branch. Both exist for the redemption path that makes the code delete the single-use arbiter and reads the negotiated scope off the returned row, but that call-site swap is in neither phase, so the queries and the test pinning their contract were dead weight across five generated files plus two dbauthz authorization decisions no caller could exercise. The plain :exec deletes they were added alongside are untouched and remain what authorizationCodeGrant and the revoke paths call. PLAT-480 covers reintroducing the query and its contract test in the PR that switches authorizationCodeGrant over to it. Refs PLAT-478
…forcement-plat-470 # Conflicts: # coderd/database/querier_test.go
2 participants
OAuth2 tokens issued by Coder ignore scope entirely. The authorize endpoint parses the
scopeparameter and then discards it, and both grant paths mint API keys with full API access regardless of what the client requested or what the app's allowlist permits. There is also nowhere to put a negotiated scope: nothing carries one from the authorize step to the token it produces.Schema and query groundwork for that pipeline. No behavior change on its own.
000569adds ascopecolumn tooauth2_provider_app_codesandoauth2_provider_app_tokens, so a negotiated scope can travel from a code to the token it is exchanged for, and from a token to its refreshed successor.coder:all, then both columns become NOT NULL with a non-empty CHECK. Every OAuth2 key is unrestricted in fact today, so the backfill only writes that down, and a caller that omits the column now fails instead of silently issuing full access.DeleteOAuth2ProviderAppCodeByIDReturningRowandDeleteAPIKeyByIDReturningRow, which returnsql.ErrNoRowswhen the row is already gone. Postgres serializes concurrent deletes on the row lock, so exactly one caller gets a row back, which is what will let the grant paths enforce single use of a code or refresh token without a read-then-write race.coder:alluntil a later phase negotiates a real value.Phase 1 of PLAT-470, tracked as PLAT-478. Scope validation at authorize, applying the negotiated scope in the code grant, and refresh narrowing follow as separate PRs.
Verified locally:
make genandmake lintclean, the migrations suite passes both up and down, and dbauthz'sTestMethodTestSuitepasses.End-to-end scope enforcement flow (green marks what this PR touches)
flowchart TD subgraph authorize["/oauth2/authorize"] AZ1["ShowAuthorizePage (GET)<br/>renders consent page"] AZ2["ProcessAuthorize (POST)<br/>scope parsed, then discarded"] Q1["InsertOAuth2ProviderAppCode<br/>gains a Scope param"] AZ1 --> AZ2 --> Q1 end Q1 --> CODES[("oauth2_provider_app_codes<br/>new column: scope text NOT NULL")] subgraph codegrant["POST /oauth2/token, grant_type=authorization_code"] G1["authorizationCodeGrant"] Q2["GetOAuth2ProviderAppCodeByPrefix<br/>now returns Scope"] Q4["DeleteOAuth2ProviderAppCodeByIDReturningRow<br/>added, no caller yet"] G2["apikey.Generate + UserRBACSubject<br/>hardcoded to full access"] G1 --> Q2 --> G2 G1 -.-> Q4 end CODES --> G1 G2 --> Q3 Q3["InsertOAuth2ProviderAppToken<br/>gains a Scope param"] Q3 --> TOKENS[("oauth2_provider_app_tokens<br/>new column: scope text NOT NULL")] subgraph refresh["POST /oauth2/token, grant_type=refresh_token"] G3["refreshTokenGrant"] Q5["GetOAuth2ProviderAppTokenByPrefix<br/>now returns Scope"] Q6["DeleteAPIKeyByIDReturningRow<br/>added, no caller yet"] G3 --> Q5 G3 -.-> Q6 end TOKENS --> G3 Q5 --> Q3 subgraph enforce["Every authenticated API request"] E1["httpmw ExtractAPIKey"] --> E2["APIKey.ScopeSet()"] --> E3["UserRBACSubject"] --> E4["dbauthz authorize"] end TOKENS --> E1 classDef changed fill:#c8e6c9,stroke:#2e7d32,stroke-width:2px,color:#1b3c1e classDef dormant fill:#c8e6c9,stroke:#2e7d32,stroke-width:2px,stroke-dasharray:5 3,color:#1b3c1e class Q1,Q2,Q3,Q5,CODES,TOKENS changed class Q4,Q6 dormantSolid green is added or changed here. Dashed green exists but has no caller yet. Everything else is unchanged, including the enforcement engine at the bottom, which already reads a key's scopes correctly and only needs real data fed into it.
Suggested reading order
Most of the diff is generated.
dump.sql,models.go,querier.go,queries.sql.go,check_constraint.go, and the dbmock and dbmetrics packages all come frommake gen.migrations/000569_oauth2_scope_columns.{up,down}.sql: additive column, backfill, NOT NULL, CHECK, and aCOMMENT ON COLUMNon each.queries/oauth2.sqlandqueries/apikeys.sql:scopeadded to both insert column lists, plus the two new returning-row deletes alongside the untouched originals. TheGet...ByPrefixselects needed no edit, since they areSELECT *.dbauthz/dbauthz.go: hand-written wrappers for the two new queries, each fetching by ID, authorizing delete against the fetched object, then delegating. The genericdeleteQhelper does not fit, since it requires the delete to return onlyerror.oauth2provider/authorize.goandoauth2provider/tokens.go: the only production changes, all behavior-neutral.dbgen/dbgen.goanddbauthz/dbauthz_test.go: seed threading, plus a case per new query.MethodTestSuitefails with "Method never called" for anything untested.Neither type needs to become auditable, which
make lintconfirms by not erroring onenterprise/audit/table.go.