mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-10 23:15:40 +00:00
xarmian
2a00775481
* feat(oauth): schema + storage layer for OAuth 2.1 server (TASK-1023, sub-PR A of TASK-951)
First of 5 sub-PRs landing the OAuth 2.1 authorization server in
PLAN-943. This one is foundation only — no HTTP exposure, no fosite
import, no public surface change.
Schema (5 tables, parallel SQLite + Postgres migrations):
- oauth_clients — RFC 7591 Dynamic Client Registration; public clients only for v1
- oauth_authorization_codes — short-lived codes for the auth-code grant
- oauth_access_tokens — opaque HMAC; subject denormalized for fast user-bound queries
- oauth_refresh_tokens — same shape; access_token_signature link + request_id chain
- oauth_pkce_requests — PKCE session keyed by auth-code signature
Storage layer (internal/store/oauth.go):
- 12 public methods covering fosite's ClientManager + CoreStorage +
PKCERequestStorage + TokenRevocationStorage interface shapes,
using pad-internal types so the package stays fosite-free.
- Three sentinel errors (ErrOAuthNotFound, ErrOAuthInvalidatedCode,
ErrOAuthInactiveToken) that sub-PR B's adapter maps to the
matching fosite errors.
- request_id IS the chain identifier (fosite preserves it across
rotations — handler/oauth2/flow_refresh.go:86), so family
revocation is a single indexed UPDATE rather than a separate
chain_id column.
14 tests covering: client CRUD + idempotent delete + empty-slice
normalization, auth-code create/get/invalidate (including the
"return payload alongside ErrInvalidatedCode" contract fosite
relies on for family revocation), access-token CRUD + delete,
refresh CRUD + RotateRefreshToken (single-row flip), refresh-token
family revocation (entire chain via request_id, leaves other
chains untouched), access-token family revocation, PKCE CRUD, and
required-field validation.
Both backends share test bodies via testStore(t); set
PAD_TEST_POSTGRES_URL=... to run the same suite against Postgres.
Out of scope for this PR (subsequent sub-PRs):
- fosite import + adapter (sub-PR B / TASK-1024)
- DCR + authorize + token endpoints (sub-PR C / TASK-1025)
- revoke + introspect endpoints (sub-PR D / TASK-1026)
- MCPBearerAuth OAuth integration (sub-PR E / TASK-1027)
* fix(oauth): always insert active=true; drop broken zero-value Active override per Codex review (round 1)
Codex round 1 caught a P1 in insertOAuthRequestRow:
active := defaultActive
if req.Active != defaultActive {
active = req.Active // <- zero-value collides
}
When defaultActive=true and req.Active=false (the zero value), this
branch fires and the row is stored with active=FALSE — silently
producing immediately-revoked tokens. Any sub-PR B adapter that
built an OAuthRequest without explicitly setting Active=true would
ship broken.
Fix: hardcode active=TRUE on insert. Drop the defaultActive
parameter (it's always true for the three flagged tables; PKCE
has no active column). Pre-seeding inactive isn't a supported flow
— fosite never does it, and tests that need a revoked row do
Create + Invalidate / Rotate / RevokeFamily as a two-step.
Regression test TestOAuth_Insert_AlwaysActive constructs an
OAuthRequest with zero-value Active and asserts the row is
readable as active for all three table types (codes, access,
refresh). Without the fix the test fails on the first GetAccessToken
call with ErrOAuthInactiveToken.
* fix(oauth): RotateRefreshToken revokes both refresh + access families per Codex review (round 2)
Codex round 2 caught: my RotateRefreshToken only marked the named
refresh row inactive, but fosite's reference MemoryStore.RotateRefreshToken
(storage/memory.go:497-504) revokes BOTH the refresh family AND the
access family for the grant's request_id. Without this, every access
token issued before a refresh remained active until TTL — defeating
the rotation's invalidation contract.
Fix: RotateRefreshToken now delegates to RevokeRefreshTokenFamily +
RevokeAccessTokenFamily (both already existed). The signatureToRotate
parameter becomes vestigial — fosite passes it but the family revoke
catches every chain member regardless of which row triggered the
rotation. The new pair fosite immediately issues via
CreateAccessTokenSession + CreateRefreshTokenSession inherits the
same request_id (flow_refresh.go:86) and lands active=TRUE per the
round-1 hardcode, so the net post-rotation state is "all old rows
in this grant inactive, the new pair active."
Test rewrite: TestOAuth_RotateRefreshToken_FlipsActiveOnSingleRow
asserted the OPPOSITE behavior (only one row touched) — that was
the original bug. Replaced with TestOAuth_RotateRefreshToken_RevokesEntireGrant
which seeds a refresh + access pair in the same chain, plus a
distinct unrelated grant, then asserts after rotation:
- old refresh + old access both inactive
- unrelated grant untouched (request_id-scoped)
* fix(oauth): DeleteOAuthClient cascades dependent rows in a tx per Codex review (round 3)
Round 3 finding: DeleteOAuthClient errored with FK constraint
violation for any client that had ever issued a grant. The
migrations declare client_id FKs without ON DELETE CASCADE — by
design, so a stray DELETE FROM oauth_clients elsewhere fails
loudly rather than silently nuking grants — but that meant the
"officially supported" delete path was unusable.
Fix: DeleteOAuthClient now runs five sequential DELETEs inside a
single transaction:
1. oauth_pkce_requests
2. oauth_refresh_tokens
3. oauth_access_tokens
4. oauth_authorization_codes
5. oauth_clients
Order matters (children before parent) because the FKs aren't
cascading. The tx makes it atomic — if any step fails, nothing's
deleted, so we never leave a half-deleted client. Idempotent
because every WHERE matches nothing on a non-existent client.
Test TestOAuth_DeleteOAuthClient_CascadesDependentRows seeds a row
in each of the four dependent tables, deletes the client, and
asserts ErrOAuthNotFound on every dependent row + the client itself.
Without the fix this fails on the first DELETE FROM oauth_clients
with an FK constraint violation.
* fix(oauth): SELECT FOR UPDATE row lock in DeleteOAuthClient on Postgres per Codex review (round 4)
Codex round 4 caught a Postgres race in DeleteOAuthClient: the
five-DELETE cascade is atomic, but between the child-row deletes
and the parent delete, a concurrent fosite handler can insert a
fresh grant/token referencing the same client_id. The parent
DELETE then fails with an FK violation and the whole tx rolls
back — the cascade is correct, but unreliable under concurrent
OAuth issuance.
Fix: take SELECT id FROM oauth_clients WHERE id = ? FOR UPDATE
as the very first statement in the tx (Postgres only). The
exclusive row-level lock blocks any concurrent statement that
tries to read the client row — which fosite does on FK resolution
during grant/token inserts — until our tx commits.
Skipped on SQLite because:
(a) BEGIN IMMEDIATE serializes the entire write workload globally
(DSN configures _txlock=immediate per store.go), so the race
doesn't exist.
(b) FOR UPDATE syntax isn't reliably accepted across SQLite
drivers.
ErrNoRows on the lock query is treated as "client doesn't exist
yet" — the subsequent DELETEs match nothing and the call remains
idempotent. Tests still pass on the SQLite path; the Postgres
path's race fix will be exercised by CI's PAD_TEST_POSTGRES_URL
runs and any future concurrency test we add.
92 lines
4.0 KiB
Go
92 lines
4.0 KiB
Go
package models
|
|
|
|
import "time"
|
|
|
|
// Models for the OAuth 2.1 authorization server (PLAN-943 TASK-951).
|
|
// These are pad-internal types used by internal/store/oauth.go to
|
|
// persist OAuth state. Sub-PR B (TASK-1024) introduces fosite, and
|
|
// sub-PR C wires fosite.Requester ⇄ OAuthRequest adapters in the
|
|
// HTTP handlers. The store layer never imports fosite so the schema
|
|
// boundary stays clean.
|
|
|
|
// OAuthClient is a registered OAuth client (RFC 7591 Dynamic Client
|
|
// Registration). Only public clients (no secret) are supported in
|
|
// v1 — see migrations/048_oauth.sql for why. The Public flag is a
|
|
// forward-compat toggle; future confidential-client support adds a
|
|
// secret column without changing this struct's shape.
|
|
//
|
|
// Field names use the RFC 7591 vocabulary so the JSON form can be
|
|
// served back to clients verbatim from the registration endpoint
|
|
// (sub-PR C).
|
|
type OAuthClient struct {
|
|
ID string `json:"client_id"`
|
|
Name string `json:"client_name"`
|
|
RedirectURIs []string `json:"redirect_uris"`
|
|
GrantTypes []string `json:"grant_types"`
|
|
ResponseTypes []string `json:"response_types"`
|
|
TokenEndpointAuthMethod string `json:"token_endpoint_auth_method"`
|
|
Scopes []string `json:"scope"`
|
|
Public bool `json:"-"` // internal — never serialized
|
|
LogoURL string `json:"logo_uri,omitempty"`
|
|
CreatedAt time.Time `json:"client_id_issued_at"`
|
|
}
|
|
|
|
// OAuthRequest is the persisted form of a fosite.Requester (without
|
|
// the fosite import). Carries everything the auth-code, access-token,
|
|
// refresh-token, and PKCE storage rows need:
|
|
//
|
|
// - Signature: the HMAC-derived lookup key (the row's primary key).
|
|
// - RequestID: fosite's stable Requester.GetID() — preserved across
|
|
// refresh-token rotations, so it doubles as the chain identifier
|
|
// for theft-detection family revocation.
|
|
// - RequestedAt: original grant timestamp; used by the cleaner.
|
|
// - ClientID: FK to oauth_clients.
|
|
// - Scopes / GrantedScopes: space-separated.
|
|
// - RequestForm: URL-encoded form data from the original request
|
|
// (PKCE handler reads code_challenge from here).
|
|
// - SessionData: JSON-encoded session struct (subject + custom
|
|
// claims). Sub-PR B defines pad's session type; the store layer
|
|
// just round-trips it as bytes.
|
|
// - Audience / GrantedAudience: space-separated RFC 8707 resource
|
|
// indicators.
|
|
// - Active: token-revocation toggle. RevokeRefreshToken /
|
|
// RevokeAccessToken set this false; GetXxxSession returns
|
|
// ErrInactiveToken (a sentinel defined in oauth.go) when active=false.
|
|
// - Subject: denormalized from SessionData for fast subject-bound
|
|
// queries (admin "list active tokens for user X" surfaces). Empty
|
|
// for auth-code and PKCE rows where there is no subject yet.
|
|
// - AccessTokenSignature: refresh-only; links the refresh row to
|
|
// the access row issued in the same grant (or rotation step).
|
|
// Empty for non-refresh rows.
|
|
type OAuthRequest struct {
|
|
Signature string
|
|
RequestID string
|
|
RequestedAt time.Time
|
|
ClientID string
|
|
Scopes string
|
|
GrantedScopes string
|
|
RequestForm string
|
|
SessionData string
|
|
Audience string
|
|
GrantedAudience string
|
|
Active bool
|
|
Subject string
|
|
AccessTokenSignature string
|
|
}
|
|
|
|
// OAuthClientCreate is the input shape for the storage-level
|
|
// CreateClient method. The Postgres / SQLite migration already
|
|
// constrains required fields; this type lets callers set only the
|
|
// fields they have without populating the timestamp (the store sets
|
|
// CreatedAt from now()).
|
|
type OAuthClientCreate struct {
|
|
Name string
|
|
RedirectURIs []string
|
|
GrantTypes []string
|
|
ResponseTypes []string
|
|
TokenEndpointAuthMethod string
|
|
Scopes []string
|
|
Public bool
|
|
LogoURL string
|
|
}
|