Generic OIDC client#3
Merged
Merged
Conversation
The OAuth/OIDC *client* side — ManyRows signing users in via someone else's IdP — as a one-to-many per-app config (an app can wire several at once), distinct from the bespoke Google/Apple/Microsoft/GitHub singletons and from the oidc_* *provider* tables (00002, ManyRows AS IdP). Two modes baked into the schema: 'oidc' (discover endpoints from an issuer URL, verify a signed id_token) and 'oauth2' (explicit endpoints, identity from userinfo). A per-mode CHECK enforces the right endpoints per row. Client secret stored AAD-encrypted like the bespoke providers. Repo CRUD + a test that doubles as migration validation (CHECK constraints, NULL/COALESCE round-trip, claim-field defaults).
ResolveOAuthSignInIdentity used one `source` value as both the coarse
user.source AND the user_identities.provider link key. That breaks for
generic external IdPs: user_identities is unique on (user_id, provider)
and (pool, provider, subject), so every generic provider sharing
provider="oidc" would (a) let a user link only ONE external IdP and
(b) collide on `sub` across issuers (a sub is unique only per-issuer).
Split them: new optional ProviderKey threads through to the identity
match/write as "idp:<slug>" (per-IdP, mode-neutral, can't collide with
the bespoke colon-free keys), while user.source stays the coarse
UserSourceExternalIDP ("external"). ProviderKey defaults to
string(source) when empty, so the bespoke four are byte-for-byte
unchanged — they don't set it.
New test proves two IdPs sharing a sub link as two distinct identities
on one pool user. Full api suite green.
Low-level client for signing users in via an external IdP, both modes:
oidc — discover endpoints from the issuer's well-known doc, verify a
signed id_token against the provider JWKS (RSA + EC keys; RS/
PS/ES * methods), checking iss / aud==client_id / exp / nonce.
oauth2 — explicit endpoints, identity read from the userinfo endpoint
(no id_token). Generalizes the GitHub posture.
PKCE (S256) and nonce on the authorize leg; configurable claim/field
mapping (sub/email/email_verified/name) with OIDC-standard defaults;
discovery + JWKS caches keyed per-provider with a 1h TTL. Takes
primitives (no core import) so it stays a composable building block.
Tested against an httptest mock IdP: happy path, nonce mismatch, wrong
audience, tampered signature, expired token, userinfo email-fallback,
OAuth2 userinfo with a non-standard subject field, and PKCE+nonce on
the authorize URL.
One route pair — /auth/idp/{providerSlug}/{authorize,callback} — serves
every configured provider, resolving the config by slug. Mirrors the
bespoke Tier-1 providers: signed single-use state (keyed by the
"idp:<slug>" provider key), opener-origin allowlist, popup postMessage
wrapper, and the shared completeTier1OAuthLogin (so cookie + bearer
delivery, 2FA, audit, and the new per-IdP identity linking all come for
free).
PKCE (S256) + OIDC nonce are derived deterministically from
HMAC(serverKey, state) rather than persisted: the state row already
enforces single-use + expiry, and deriving from the secret key means the
public state value can't reveal the verifier. Zero new per-flow storage,
no change to the shared oauth_states infra.
Also extracts the opener-origin allow check into a shared
isOpenerOriginAllowed helper (bespoke Tier-1 handler refactored onto it;
full api suite green) and adds the AuthMethodExternalIDP audit method.
Derivation unit-tested (determinism, S256, RFC-7636 length, key+state
dependence). The full browser-flow integration test pairs with the
admin endpoint (it provisions the encrypted client secret).
The generic callback checked only that an email was present, not that the IdP marked it verified — unlike every bespoke provider (Google rejects unverified, GitHub filters to verified, Microsoft uses xms_edov) and contrary to completeTier1OAuthLogin's documented contract. Without the check, an external IdP that lets a user assert an arbitrary unverified email could hijack an existing account: the email-fallback branch in ResolveOAuthSignInIdentity matches the victim by email and links the attacker's external identity to them, then issues a session. Reject with error.emailNotVerified (403) when email_verified isn't true, mirroring the bespoke Google path. Providers that don't emit the claim are rejected until a per-IdP opt-out exists (tracked).
The provider key was "idp:<slug>", but identities are matched POOL-wide
(FindUserByIdentity keys on user_pool_id) while slugs are unique only
per-app. Two apps sharing a pool could both use slug "okta" for
DIFFERENT issuers, and since a `sub` is unique only per-issuer, that
could match two distinct people onto one account — the exact hazard the
provider-key decoupling was meant to remove, left half-solved. Key by
the config's UUID instead (pool-global, stable for the provider's life).
This surfaced two schema constraints sized for the bespoke names that
reject "idp:<uuid>" (40 chars), fixed in migration 00006:
- user_identities.provider varchar(20) -> varchar(64)
- oauth_states.provider CHECK allowlist -> also accept 'idp:%'
(this one would have failed SignOAuthState at runtime — the
authorize handler was broken for generic providers; no test
exercised it until now).
New state sign/verify round-trip test covers the oauth_states path;
the per-provider isolation test now uses UUID keys. Full suite green.
Reject cleartext URLs so tokens, client secrets, and authorization codes can't ride http, and a downgraded endpoint can't be fetched server-side (SSRF/MITM). Enforced on: the configured OIDC issuer (before discovery), every endpoint the discovery doc returns (downgrade defense against a malicious issuer), and the explicit OAuth2-mode endpoints. http is allowed only to loopback hosts so local dev and tests work.
REST sub-resource on an app (mirrors the webhooks shape):
GET/POST /external-idps
POST /external-idps/validate-discovery
PUT/DELETE /external-idps/{idpId}
Create encrypts the client secret AAD-bound to the row id (generated
before encryption). Update merges the secret: an empty clientSecret
keeps the stored ciphertext (UpdateExternalIDP is a full overwrite, so
the audit-flagged wipe is avoided), a non-empty one rotates it — the UI
never round-trips the secret. Responses expose hasClientSecret, never
the secret itself. validate-discovery powers the admin "fetch discovery"
button, surfacing a bad issuer as a 400 before the provider is saved.
Up-front validation (slug format, mode, required fields per mode, https
via the now-exported oidc.RequireSecureURL) yields clean 400s; duplicate
slug -> 409.
Tests: CRUD lifecycle, the secret-merge invariant (asserted on the raw
DB ciphertext), all validation rejections, and discovery against a mock.
Drives the real WorkspaceExternalIDPAuthorize + Callback handlers through a chi router (workspace + app context) against an httptest mock OpenID provider (discovery + JWKS + token). Closes the integration gap the audit flagged — this exercises the path that the oauth_states CHECK silently broke. Asserts the happy path (authorize URL targets the IdP with PKCE+nonce; callback exchanges, verifies the id_token, mints a session, links the identity under idp:<config-uuid> with the right subject) and the verified-email gate (an unverified id_token email is refused and no account is created). Full api + oidc suites green.
claimString only accepted a string, so a provider returning a numeric
user id — common in OAuth2 userinfo (GitHub-style {"id": 12345}) and
the occasional non-spec numeric `sub` — extracted as "" and the sign-in
failed with "no usable identity" / "missing subject". Coerce json.Number
and float64 to a string. FetchUserinfo now decodes with UseNumber so a
large id keeps full precision (json.Number) instead of a lossy float64.
Test: OAuth2 userinfo with a numeric id resolves the subject.
Two robustness fixes for the external IdP fetches: - Cap every decoded response (discovery, JWKS, token, userinfo) at 1 MiB via io.LimitReader. These bodies are a few KB in practice; the cap stops a malformed or misconfigured endpoint from OOMing the auth server with an unbounded body. - Add 60s leeway to id_token validation so a freshly minted token from an IdP whose clock runs slightly ahead isn't spuriously rejected at exp/nbf.
…t-out Two remaining backend items: - Disconnect (#44): the self-serve and admin "delete connected identity" endpoints allowlisted only the four bespoke providers, so a generic idp:<uuid> identity couldn't be unlinked. Extend both to accept the idp: namespace via core.IsExternalIDPProviderKey. - Verified-email opt-out (#46): migration 00007 adds trust_unverified_email (default false = secure; a bool whose zero value is the safe default, so omitting it can't weaken the check). The callback gate now reads `!TrustUnverifiedEmail && !EmailVerified`, letting an admin trust an IdP that verifies emails but omits the email_verified claim. Plumbed through the model, repo, and admin DTO. E2E gains a third case proving a trust_unverified_email=true provider accepts an unverified email while the default provider still rejects it.
HandleGetAppForAppKit now includes externalIdps: [{slug, displayName,
buttonIcon}] for the app's enabled providers, so AppKit can render a
sign-in button per provider. Public-safe by construction — the DTO has
no client id/secret/endpoint fields — and best-effort (a lookup failure
logs and omits them rather than breaking the whole AppKit boot).
E2E gains a sub-test asserting the config lists enabled providers, hides
disabled ones, and leaks no sensitive fields.
Generalize the OAuth popup helper (runPopupOAuth) so a flow can supply an explicit authorize path + callback messageType instead of deriving them from one of the four bespoke providers — the bespoke call sites are unchanged (the new fields default to the provider-derived values). The generic path opens /auth/idp/<slug>/authorize and listens for the external-idp-oauth-callback postMessage. Auth renders one outlined button per app.externalIdps entry (label = displayName, icon = buttonIcon with a key fallback; Icon returns null for unknown names so an arbitrary icon can't break the button), wired to the same popup → token-pair → onTokenPair path the bespoke providers use. The OAuth section now also shows when only external IdPs are configured. tsc + bundle build clean.
A new tab in AppAuthMethods renders the generic external-IdP providers as a CRUD list (display name, slug, mode, enabled chip; edit/delete) with an "Add provider" dialog form covering: OIDC vs OAuth2 mode (conditional issuer-URL vs explicit endpoints), a "Fetch discovery" validate button, client id/secret (blank-keeps-existing on edit), scopes, an advanced accordion for claim mapping + button icon, and the trust_unverified_email switch with a security warning. Wired to the admin REST endpoints (GET/POST/PUT/DELETE /external-idps + validate-discovery). tsc + build clean.
Audit pass 4 (frontend + #44/#46 deltas). The disconnect endpoint is
/a/me/identities/{provider} and an external-IdP key is "idp:<uuid>" — a
colon. A new routing test pins down that chi does NOT percent-decode
path params: the raw colon routes correctly, but %3A does not round-trip.
So the client must send the key verbatim; URL-encoding it would silently
break the lookup (DeleteUserIdentity would search "idp%3A<uuid>"). The
current Profile.tsx already sends it raw — added a comment so nobody
"helpfully" wraps it in encodeURIComponent later, plus a regression guard
in the test.
Also: the connected-accounts row showed the raw "idp:<uuid>" key as the
label; fall back to "Single sign-on" for external IdPs.
The server translation table (T) returned the raw key for the new error.externalIdp* codes, so the admin UI surfaced "error.externalIdp\ InvalidSlug" etc. verbatim (WriteError sends body.message = T(code), which extractApiError prefers). Added English messages for all nine codes so every client shows human text. The admin validation test now asserts the 400 body carries a translated message (message != code), guarding against an untranslated code regressing. Minor UI polish: clear the "Fetch discovery" confirmation when the issuer URL changes so a stale success banner can't linger.
… tests Audit pass 5 — closing the gaps I'd flagged as unaudited: - azp check (OIDC §3.1.3.7): a multi-audience id_token must carry azp == our client. WithAudience only checks our client is *among* the audiences; without azp, a token minted for a different client that co-lists ours would pass. Single-aud tokens are unaffected. Tested both ways against the mock IdP. - Provider-deletion behavior pinned: user_identities has no FK to external_idps, so deleting a provider leaves the identity links orphaned (inert — the key is the provider's UUID, never re-queried) rather than cascade-deleting user data. New test asserts the link survives and the user is untouched. - Concurrency: a new test hammers Authenticate from 24 goroutines against one issuer to stress the shared discovery + JWKS caches; passes under -race (whole new package + external-IdP api tests also -race clean). Verified-and-clean (no change needed): the janitor already sweeps oauth_states by expiry, so the generic flow's idp: state rows are cleaned up; cross-provider JWKS/key confusion is prevented by per-config iss+aud+jwks scoping.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.