From aee6fa97dd702947953631f6725767e9b81eef88 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 20:21:16 +0000 Subject: [PATCH] fix(plugin-auth): stop emitting the retired oidcConfig.mapping.id on OIDC SSO registration (#8193) @better-auth/sso declares oidcConfig.mapping as a strict object. `id` was a real member in 1.6.20 and was honoured at login; the pinned 1.7.0-rc.2 retires it and reads the federated subject from the OIDC `sub` claim directly, so the key the bridge always sent made every OIDC registration answer 400. Emit { email, name } (the strict schema's required members), refuse a non-`sub` user-ID claim loudly rather than discarding it silently, and pin the path with tests that drive the real /sso/register endpoint of a real better-auth instance. Also corrects the stale "verified against 1.6.20" attestation in auth-manager.ts to what was actually re-measured against 1.7.0-rc.2. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PEVB6w7D7uCszR9Mw1BL73 --- ...oidc-sso-register-mapping-strict-object.md | 46 ++++++++ .../plugins/plugin-auth/src/auth-manager.ts | 20 +++- .../src/register-sso-provider.test.ts | 107 +++++++++++++++++- .../plugin-auth/src/register-sso-provider.ts | 36 +++++- 4 files changed, 202 insertions(+), 7 deletions(-) create mode 100644 .changeset/oidc-sso-register-mapping-strict-object.md diff --git a/.changeset/oidc-sso-register-mapping-strict-object.md b/.changeset/oidc-sso-register-mapping-strict-object.md new file mode 100644 index 0000000000..fd6672ba35 --- /dev/null +++ b/.changeset/oidc-sso-register-mapping-strict-object.md @@ -0,0 +1,46 @@ +--- +"@objectstack/plugin-auth": patch +--- + +fix(plugin-auth): OIDC SSO provider registration works again — stop emitting the retired `oidcConfig.mapping.id` key (#8193) + +Registering an external OIDC identity provider through the `sys_sso_provider` +`register_sso_provider` action failed **every time**, with HTTP 400: + +``` +[body.oidcConfig.mapping] Unrecognized key: "id" +``` + +Not intermittent and not configuration-dependent — the OIDC half of the +registration bridge was unusable for every deployment, and nothing was +persisted. SAML registration was unaffected. + +The bridge unconditionally emitted a claim mapping of +`{ id, email, name }`. `@better-auth/sso` declares `oidcConfig.mapping` as a +**strict** object, so a member it does not declare is rejected outright rather +than ignored. + +**`id` was not a key that moved — it was retired upstream.** In 1.6.20 the +mapping was a plain (non-strict) object that did carry `id`, and the plugin +honoured it when resolving the federated user. The pinned 1.7.0-rc.2 removes the +member and reads the federated subject from the OIDC `sub` claim directly, then +cross-checks it against the ID token. There is consequently no new home for the +key: `extraFields` is the one open member of the strict object, but a value +placed at `extraFields.id` is overwritten by `sub` before it is ever used, so +re-homing the key there would have looked configured while doing nothing. + +The emitted mapping is now `{ email, name }` — the two members the strict schema +requires — and the email/name claim mappings collected by the form continue to +work exactly as before. + +**The user-ID claim mapping is now refused instead of ignored.** Because the +subject claim is no longer configurable at all, a registration that asks for a +non-`sub` user-ID claim is answered with a clear `INVALID_REQUEST` explaining +that the subject is always read from `sub`, rather than being accepted and +silently discarded. Leaving the field empty — or setting it to `sub`, the value +the form suggests — registers as normal. + +Pinned by a regression test that drives the real `/sso/register` endpoint of a +real better-auth instance, so the emitted body is judged by the installed +package's own schema and the next dependency bump that moves this surface fails +loudly instead of shipping. diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index bfa7ee9049..25f1412aa0 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -2512,11 +2512,21 @@ export class AuthManager { if (enabled.sso) { await this.addOptionalPlugin(plugins, 'sso', async () => { const { sso } = await import('@better-auth/sso'); - // NOTE: unlike `oauthProvider`, @better-auth/sso hardcodes its `ssoProvider` - // model and accepts NO `schema` option (verified against 1.6.20 — no - // mergeSchema, runtime never reads options.schema). Its table mapping to - // `sys_sso_provider` must therefore be resolved by the better-auth adapter - // / a global model map, not per-plugin here (see AUTH_SSO_PROVIDER_SCHEMA). + // NOTE: the `ssoProvider` model is bridged to `sys_sso_provider` by the + // better-auth adapter / a global model map, not per-plugin here (see + // AUTH_SSO_PROVIDER_SCHEMA). + // + // That bridge dates from 1.6.20, where @better-auth/sso hardcoded the + // model and read no `schema` option. Re-checked against the pinned + // 1.7.0-rc.2 (`node_modules/@better-auth/sso/dist`) on 2026-08-12: that is + // no longer true — `SSOOptions.schema.ssoProvider` now exists + // (index-D1yk91me.d.mts) and the runtime honours `modelName` plus a + // per-field `fieldName` map (index.mjs, the plugin's `schema:` block). The + // adapter-level bridge is kept as-is here because it is what the rest of + // the auth stack is wired to; whether to move it onto the plugin option is + // a separate change, not a silent one. Only the mapping surface below was + // re-verified in depth — see register-sso-provider.ts for the + // `oidcConfig.mapping` strict-object findings. // // `organizationProvisioning.defaultRole` (ADR-0024 V1): a first-time // federated login is JIT-provisioned into the user's domain-matched org diff --git a/packages/plugins/plugin-auth/src/register-sso-provider.test.ts b/packages/plugins/plugin-auth/src/register-sso-provider.test.ts index 488d735f3e..939526d685 100644 --- a/packages/plugins/plugin-auth/src/register-sso-provider.test.ts +++ b/packages/plugins/plugin-auth/src/register-sso-provider.test.ts @@ -1,6 +1,9 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { describe, it, expect, vi } from 'vitest'; -import { runRegisterSamlProviderFromForm } from './register-sso-provider'; +import { betterAuth } from 'better-auth'; +import { memoryAdapter } from 'better-auth/adapters/memory'; +import { sso } from '@better-auth/sso'; +import { runRegisterSamlProviderFromForm, runRegisterSsoProviderFromForm } from './register-sso-provider'; const makeReq = (body: any) => new Request('http://localhost:3000/api/v1/auth/admin/sso/register-saml', { @@ -9,6 +12,108 @@ const makeReq = (body: any) => body: JSON.stringify(body), }); +const makeOidcReq = (body: any) => + new Request('http://localhost:3000/api/v1/auth/admin/sso/register', { + method: 'POST', + headers: { 'content-type': 'application/json', origin: 'http://localhost:3000' }, + body: JSON.stringify(body), + }); + +const OIDC_FORM = { + providerId: 'acme', + issuer: 'https://idp.acme.com', + domain: 'acme.com', + clientId: 'cid', + clientSecret: 'csecret', +}; + +/** + * A REAL `betterAuth()` instance carrying the REAL `@better-auth/sso` plugin, so + * the body this bridge emits is judged by the installed package's own Zod + * schema — not by a hand-copied restatement of it that would drift silently on + * the next dependency bump. + */ +const makeRealAuthHandler = () => { + const auth = betterAuth({ + baseURL: 'http://localhost:3000', + basePath: '/api/v1/auth', + secret: 'register-sso-provider-test-secret-0123456789', + database: memoryAdapter({}), + plugins: [sso()], + }); + return (request: Request) => auth.handler(request); +}; + +describe('runRegisterSsoProviderFromForm (OIDC) — the emitted body must satisfy the INSTALLED @better-auth/sso schema', () => { + // Regression pin for the end-to-end break where the bridge always emitted + // `oidcConfig.mapping.id`, which `oidcMappingSchema` (a `z.strictObject` with + // no `id` member since 1.7.0-rc.2) rejects outright — every OIDC registration + // answered `400 [body.oidcConfig.mapping] Unrecognized key: "id"`. + // + // These cases drive the REAL `/sso/register` endpoint. `@better-auth/sso` + // validates the request body BEFORE the endpoint's session gate, so an + // unauthenticated call separates the two failure modes cleanly: + // • body rejected by the schema → 400 VALIDATION_ERROR (the bug) + // • body accepted, stopped by the gate → 401 Unauthorized (the fix) + // Reaching 401 is therefore positive evidence that the emitted body parsed. + it('clears the real body schema and reaches the endpoint session gate', async () => { + const res = await runRegisterSsoProviderFromForm(makeRealAuthHandler(), makeOidcReq(OIDC_FORM)); + + expect(res.body.error?.message ?? '').not.toMatch(/Unrecognized key/); + expect(res.status).toBe(401); + expect(res.body.error?.code).toBe('SSO_REGISTER_FAILED'); + expect(res.body.error?.message).toBe('Unauthorized'); + }); + + it('clears the real body schema with operator-supplied claim mappings too', async () => { + const res = await runRegisterSsoProviderFromForm( + makeRealAuthHandler(), + makeOidcReq({ ...OIDC_FORM, mapEmail: 'upn', mapName: 'display_name', scopes: 'openid email' }), + ); + + expect(res.body.error?.message ?? '').not.toMatch(/Unrecognized key/); + expect(res.status).toBe(401); + }); + + it('emits exactly the strict schema’s required members — never the retired `id`', async () => { + let dispatched: any = null; + const handle = vi.fn(async (req: Request) => { + if (req.url.endsWith('/get-session')) return new Response('null', { status: 200 }); + dispatched = await req.clone().json(); + return new Response(JSON.stringify({ providerId: 'acme' }), { status: 200 }); + }); + + const res = await runRegisterSsoProviderFromForm(handle, makeOidcReq(OIDC_FORM)); + + expect(res.status).toBe(200); + expect(dispatched.oidcConfig.mapping).toEqual({ email: 'email', name: 'name' }); + expect(Object.keys(dispatched.oidcConfig.mapping)).not.toContain('id'); + }); + + // The subject claim is no longer configurable anywhere in the plugin: rc.2 + // reads it from `sub` and overwrites any `extraFields.id` with it. Telling the + // caller beats accepting a value we would silently discard. + it('rejects a non-`sub` user-ID claim loudly instead of silently discarding it', async () => { + const handle = vi.fn(); + const res = await runRegisterSsoProviderFromForm(handle, makeOidcReq({ ...OIDC_FORM, mapId: 'employee_id' })); + + expect(res.status).toBe(400); + expect(res.body.error?.code).toBe('INVALID_REQUEST'); + expect(res.body.error?.message).toMatch(/not configurable/); + expect(handle).not.toHaveBeenCalled(); + }); + + it('accepts an explicit `sub` (the value the form field suggests) as the no-op it is', async () => { + const res = await runRegisterSsoProviderFromForm( + makeRealAuthHandler(), + makeOidcReq({ ...OIDC_FORM, mapId: 'sub' }), + ); + + expect(res.status).toBe(401); + expect(res.body.error?.message).toBe('Unauthorized'); + }); +}); + describe('runRegisterSamlProviderFromForm (ADR-0069 P3)', () => { it('reshapes flat fields into nested samlConfig + derives the ACS URL, re-dispatching to /sso/register', async () => { let dispatched: { url: string; body: any } | null = null; diff --git a/packages/plugins/plugin-auth/src/register-sso-provider.ts b/packages/plugins/plugin-auth/src/register-sso-provider.ts index 791f6b81c9..bfd00d17ed 100644 --- a/packages/plugins/plugin-auth/src/register-sso-provider.ts +++ b/packages/plugins/plugin-auth/src/register-sso-provider.ts @@ -78,6 +78,8 @@ async function resolveActiveOrganizationId( * cookie / bearer + Origin; its body carries the flat form * fields ({ providerId, issuer, domain, clientId, clientSecret, * discoveryEndpoint?, scopes?, mapId?, mapEmail?, mapName? }). + * `mapId` is accepted only as the (empty or `sub`) no-op it now + * is — see the mapping block below. */ export async function runRegisterSsoProviderFromForm( handle: AuthRequestHandler, @@ -119,8 +121,40 @@ export async function runRegisterSsoProviderFromForm( const oidcConfig: Record = { clientId, clientSecret }; if (discoveryEndpoint) oidcConfig.discoveryEndpoint = discoveryEndpoint; oidcConfig.scopes = scopesRaw ? scopesRaw.split(/[\s,]+/).filter(Boolean) : ['openid', 'email', 'profile']; + + // `oidcConfig.mapping` is a `z.strictObject` in `@better-auth/sso@1.7.0-rc.2` + // (dist/index.mjs, `oidcMappingSchema`): members { email, emailVerified?, + // name, image?, extraFields? }, with `email` and `name` REQUIRED and NO `id` + // member. Emitting `id` is therefore a hard 400 on EVERY registration: + // [body.oidcConfig.mapping] Unrecognized key: "id" + // + // `id` is not a key that moved — it was RETIRED upstream, so there is nowhere + // to re-home it. 1.6.20 declared the mapping as a plain (non-strict) + // `z.object` that DID carry `id`, and honoured it at login + // (`id: rawUserInfo[mapping.id || "sub"]`). 1.7.0-rc.2 deletes the member and + // hard-wires the federated subject to the OIDC `sub` claim + // (`id: readStringClaim(rawUserInfo, "sub")` / `id: idToken.sub`), then + // cross-checks it (`id_token_subject_missing`, + // `id_token_userinfo_subject_mismatch`). `extraFields` is NOT a substitute: + // it parses, but it is spread BEFORE `id` in the profile literal, so an + // `extraFields.id` is silently overwritten by `sub` — a no-op that reads as + // configured. The subject claim is simply not configurable any more, so a + // caller that asks for a different one is told so instead of being ignored. + const mapId = str(body?.mapId); + if (mapId && mapId !== 'sub') { + return { + status: 400, + body: { + success: false, + error: { + code: 'INVALID_REQUEST', + message: + 'The user ID claim is not configurable: the federated subject is always read from the OIDC "sub" claim. Leave the user-ID claim mapping empty (or set it to "sub").', + }, + }, + }; + } oidcConfig.mapping = { - id: str(body?.mapId) || 'sub', email: str(body?.mapEmail) || 'email', name: str(body?.mapName) || 'name', };