diff --git a/.changeset/oidc-id-token-userinfo-claims.md b/.changeset/oidc-id-token-userinfo-claims.md new file mode 100644 index 0000000000..01ad3bdb67 --- /dev/null +++ b/.changeset/oidc-id-token-userinfo-claims.md @@ -0,0 +1,6 @@ +--- +"@better-auth/oauth-provider": patch +--- + +Keeps `profile` and `email` scope claims on the OIDC UserInfo response instead of adding them to authorization-code ID tokens by default, advertises `acr_values_supported: ["0"]`, and rejects unsupported +`acr_values` authorization requests instead of silently downgrading them. diff --git a/docs/content/docs/plugins/oauth-provider.mdx b/docs/content/docs/plugins/oauth-provider.mdx index 2b4ea6c3a4..9630e6e18a 100644 --- a/docs/content/docs/plugins/oauth-provider.mdx +++ b/docs/content/docs/plugins/oauth-provider.mdx @@ -1166,8 +1166,8 @@ Scopes allow clients specific access to specific resources. By default, we support the following scopes are supported: * `openid`: Returns the user's ID (`sub` claim). -* `profile`: Returns name, picture, given\_name, family\_name -* `email`: Returns email and email\_verified +* `profile`: Returns name, picture, given\_name, family\_name from UserInfo +* `email`: Returns email and email\_verified from UserInfo * `offline_access`: Returns a refresh token The scopes configuration can contain as many or as few scopes as you wish! Note that `openid` is required to be considered an OIDC server, otherwise this is a standard OAuth 2.1 server. All supported scopes must be in this array. @@ -1182,7 +1182,7 @@ oauthProvider({ Internally supported claims include \["sub", "iss", "aud", "exp", "iat", "sid", "scope", "azp"]. -ID token and user info claims should be namespaced when possible to avoid potential future conflicts. +ID token and UserInfo claims should be namespaced when possible to avoid potential future conflicts. In the authorization code flow, `profile` and `email` request UserInfo claims; they are not added to the ID token unless you add them with `customIdTokenClaims`. `customIdTokenClaims` is additive for protocol-owned ID token claims. Reserved names such as `iss`, `sub`, `aud`, token lifetime claims, `nonce`, `sid`, hash claims, `auth_time`, `acr`, `amr`, and `azp` are stripped at issuance with a warning log. Use namespaced claim names such as `https://example.com/org` for application-specific data. @@ -1494,11 +1494,11 @@ authenticate: async ({ ctx, opts, assertion, expectedAudience }) => { Three claim surfaces resolve contributions in a fixed order. Across all three, third-party extension claims are strictly additive, while the operator's own first-party callbacks may override profile-style identity claims; protocol-owned identity, lifetime, binding, and authentication-context claims are always pinned or reserved by the provider. -| Token | Order (lowest to highest authority) | Pinned or reserved by provider | -| ------------ | ------------------------------------------------------------------------------------------------------------------------------ | ---------------------------------------------------------------------------------------------------------------------------------------------------------- | -| Access token | extension `claims.accessToken` \< per-issuance `accessTokenClaims` \< `customAccessTokenClaims` \< per-resource `customClaims` | reserved RFC 9068 names (`iss`, `sub`, `aud`, `exp`, `iat`, `jti`, `client_id`, `scope`, `auth_time`, `acr`, `amr`), stripped before signing | -| ID token | base identity claims \< `customIdTokenClaims`; extension and per-issuance `idTokenClaims` are reserved-filtered and additive | reserved OIDC/JWT names (`iss`, `sub`, `aud`, `exp`, `nbf`, `iat`, `jti`, `nonce`, `sid`, `at_hash`, `c_hash`, `s_hash`, `auth_time`, `acr`, `amr`, `azp`) | -| UserInfo | base identity claims \< extension `claims.userInfo` (additive only) \< `customUserInfoClaims` | `sub` (re-pinned last) | +| Token | Order (lowest to highest authority) | Pinned or reserved by provider | +| ------------ | ------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Access token | extension `claims.accessToken` \< per-issuance `accessTokenClaims` \< `customAccessTokenClaims` \< per-resource `customClaims` | reserved RFC 9068 names (`iss`, `sub`, `aud`, `exp`, `iat`, `jti`, `client_id`, `scope`, `auth_time`, `acr`, `amr`), stripped before signing | +| ID token | subject/authentication claims \< `customIdTokenClaims`; extension and per-issuance `idTokenClaims` are reserved-filtered and additive | reserved OIDC/JWT names (`iss`, `sub`, `aud`, `exp`, `nbf`, `iat`, `jti`, `nonce`, `sid`, `at_hash`, `c_hash`, `s_hash`, `auth_time`, `acr`, `amr`, `azp`) and scope-derived UserInfo claim names | +| UserInfo | base identity claims \< extension `claims.userInfo` (additive only) \< `customUserInfoClaims` | `sub` (re-pinned last) | Per-issuance `accessTokenClaims` are JWT-only: an opaque access token persists no per-issuance claims, so they do not reappear at introspection. A claim that must be visible at opaque-token introspection belongs in a grant-type-stable `claims.accessToken` contributor, which the introspection path re-derives. @@ -1664,6 +1664,9 @@ The metadata endpoint can be customized so that the publicized scopes and claims All scopes inside the advertisedMetadata section MUST be listed in `scopes` otherwise initialization will fail. +Better Auth advertises `acr_values_supported: ["0"]`, which is the unspecified authentication context. Custom ACR policies are not currently supported; authorization requests for other +`acr_values` are rejected with `invalid_request`. + #### Scopes ```ts title="auth.ts" diff --git a/packages/oauth-provider/src/authorize.test.ts b/packages/oauth-provider/src/authorize.test.ts index 932eff7747..e15f53fed6 100644 --- a/packages/oauth-provider/src/authorize.test.ts +++ b/packages/oauth-provider/src/authorize.test.ts @@ -498,6 +498,102 @@ describe("oauth authorize - max_age (OIDC Core 1.0 §3.1.2.1)", async () => { }); }); +describe("oauth authorize - acr_values (OIDC Core 1.0 §3.1.2.1)", async () => { + const authServerBaseUrl = "http://localhost:3000"; + const rpBaseUrl = "http://localhost:5000"; + const { auth, signInWithTestUser, customFetchImpl } = await getTestInstance({ + baseURL: authServerBaseUrl, + plugins: [ + oauthProvider({ + loginPage: "/login", + consentPage: "/consent", + silenceWarnings: { + oauthAuthServerConfig: true, + openidConfig: true, + }, + }), + jwt(), + ], + }); + const { headers } = await signInWithTestUser(); + const authenticatedClient = createAuthClient({ + plugins: [oauthProviderClient()], + baseURL: authServerBaseUrl, + fetchOptions: { customFetchImpl, headers }, + }); + + let oauthClient: OAuthClient | null; + const redirectUri = `${rpBaseUrl}/api/auth/callback/test`; + beforeAll(async () => { + oauthClient = await auth.api.adminCreateOAuthClient({ + headers, + body: { redirect_uris: [redirectUri], skip_consent: true }, + }); + }); + + function authorizeUrl(acrValues: string) { + if (!oauthClient?.client_id) throw new Error("beforeAll not run properly"); + const url = new URL(`${authServerBaseUrl}/api/auth/oauth2/authorize`); + url.searchParams.set("client_id", oauthClient.client_id); + url.searchParams.set("redirect_uri", redirectUri); + url.searchParams.set("response_type", "code"); + url.searchParams.set("scope", "openid"); + url.searchParams.set("state", "acr-state"); + url.searchParams.set("code_challenge", generateRandomString(43)); + url.searchParams.set("code_challenge_method", "S256"); + url.searchParams.set("acr_values", acrValues); + return url.toString(); + } + + async function redirectFor(acrValues: string) { + let location = ""; + await authenticatedClient.$fetch(authorizeUrl(acrValues), { + onError(context) { + location = context.response.headers.get("Location") || ""; + }, + }); + return location; + } + + /** + * @see https://github.com/better-auth/better-auth/pull/10152 + */ + it("rejects unsupported acr_values instead of silently downgrading", async () => { + const location = await redirectFor("1"); + const errorRedirect = new URL(location); + + expect(errorRedirect.origin + errorRedirect.pathname).toBe(redirectUri); + expect(errorRedirect.searchParams.get("error")).toBe("invalid_request"); + expect(errorRedirect.searchParams.get("error_description")).toBe( + "unsupported acr_values", + ); + expect(errorRedirect.searchParams.get("state")).toBe("acr-state"); + expect(errorRedirect.searchParams.get("code")).toBeNull(); + }); + + it("accepts the advertised unspecified acr value", async () => { + const location = await redirectFor("0"); + const callbackRedirect = new URL(location); + + expect(callbackRedirect.origin + callbackRedirect.pathname).toBe( + redirectUri, + ); + expect(callbackRedirect.searchParams.get("code")).toBeTruthy(); + expect(callbackRedirect.searchParams.get("error")).toBeNull(); + }); + + it("accepts multiple requested acr values when one is supported", async () => { + const location = await redirectFor("1 0"); + const callbackRedirect = new URL(location); + + expect(callbackRedirect.origin + callbackRedirect.pathname).toBe( + redirectUri, + ); + expect(callbackRedirect.searchParams.get("code")).toBeTruthy(); + expect(callbackRedirect.searchParams.get("error")).toBeNull(); + }); +}); + describe("oauth authorize - request_uri resolution", async () => { const authServerBaseUrl = "http://localhost:3000"; const rpBaseUrl = "http://localhost:5000"; diff --git a/packages/oauth-provider/src/authorize.ts b/packages/oauth-provider/src/authorize.ts index 023b9a3aa0..9b2e012623 100644 --- a/packages/oauth-provider/src/authorize.ts +++ b/packages/oauth-provider/src/authorize.ts @@ -5,6 +5,7 @@ import { getSessionFromCtx } from "better-auth/api"; import { generateRandomString, makeSignature } from "better-auth/crypto"; import type { Verification } from "better-auth/db"; import { APIError } from "better-call"; +import { UNSPECIFIED_ACR } from "./authentication-context"; import { oAuthState } from "./oauth"; import type { OAuthErrorCode, OAuthRedirectOnError } from "./oauth-endpoint"; import { mapIssuesToOAuthError } from "./oauth-endpoint"; @@ -134,6 +135,12 @@ function getAuthorizationRequestParameters( return { ...parameters } as unknown as OAuthAuthorizationQuery; } +function isAcrValuesRequestSupported(acrValues: string | undefined): boolean { + if (acrValues === undefined) return true; + const requestedAcrValues = acrValues.split(" ").filter(Boolean); + return requestedAcrValues.includes(UNSPECIFIED_ACR); +} + export const handleRedirect = ( ctx: GenericEndpointContext, uri: string, @@ -434,6 +441,13 @@ export async function authorizeEndpoint( } query = parsedQuery.data as OAuthAuthorizationQuery; ctx.query = query; + if (!isAcrValuesRequestSupported(query.acr_values)) { + return authorizeRedirectOnError(opts)({ + error: "invalid_request", + error_description: "unsupported acr_values", + ctx, + }); + } await oAuthState.set({ query: serializeAuthorizationQuery(query).toString(), }); diff --git a/packages/oauth-provider/src/extensions.test.ts b/packages/oauth-provider/src/extensions.test.ts index 69b0baef47..c096bd5ac6 100644 --- a/packages/oauth-provider/src/extensions.test.ts +++ b/packages/oauth-provider/src/extensions.test.ts @@ -553,10 +553,9 @@ describe("oauth-provider extensions", async () => { expect(idTokenPayload.jti).toBeUndefined(); expect(idTokenPayload.c_hash).toBeUndefined(); expect(idTokenPayload.s_hash).toBeUndefined(); - // Extension id-token claims are additive: they cannot replace the user's - // identity claims or fill reserved OIDC/JWT claim names. - expect(idTokenPayload.email).toBe(user.email); - expect(idTokenPayload.email).not.toBe("evil@malicious.example"); + // Extension id-token claims are additive: they cannot fill standard + // identity claims that scope values only request from UserInfo. + expect(idTokenPayload.email).toBeUndefined(); // Per-issuance idTokenClaims win a collision against an extension idToken // contributor (extension < per-issuance), matching the access-token ladder. expect(idTokenPayload.order_probe).toBe("grant"); diff --git a/packages/oauth-provider/src/metadata.test.ts b/packages/oauth-provider/src/metadata.test.ts index 5580120915..e4f2b55697 100644 --- a/packages/oauth-provider/src/metadata.test.ts +++ b/packages/oauth-provider/src/metadata.test.ts @@ -120,6 +120,7 @@ describe("oauth metadata", async () => { claims_supported: baseClaims, userinfo_endpoint: `${baseURL}/oauth2/userinfo`, subject_types_supported: ["public"], + acr_values_supported: ["0"], id_token_signing_alg_values_supported: ["EdDSA"], end_session_endpoint: `${baseURL}/oauth2/end-session`, prompt_values_supported: [ @@ -357,6 +358,13 @@ describe("oauth metadata", async () => { expect(metadata.request_uri_parameter_supported).toBe(false); }); + it("should advertise the unspecified ACR value by default", async () => { + const { auth } = await createTestInstance(); + const metadata = await auth.api.getOpenIdConfig(); + + expect(metadata.acr_values_supported).toEqual(["0"]); + }); + it("should fail if advertised scope invalid", async () => { const advertisedScopes = ["create:test"]; expect(() => diff --git a/packages/oauth-provider/src/metadata.ts b/packages/oauth-provider/src/metadata.ts index 0874cd5ec5..9db68e0531 100644 --- a/packages/oauth-provider/src/metadata.ts +++ b/packages/oauth-provider/src/metadata.ts @@ -4,6 +4,7 @@ import { PRIVATE_KEY_JWT_SIGNING_ALGORITHMS, } from "@better-auth/core/oauth2"; import type { JWSAlgorithms, JwtOptions } from "better-auth/plugins"; +import { UNSPECIFIED_ACR } from "./authentication-context"; import { validateIssuerUrl } from "./authorize"; import { applyOAuthProviderMetadataExtensions, @@ -176,6 +177,7 @@ export function oidcServerMetadata( subject_types_supported: opts.pairwiseSecret ? ["public", "pairwise"] : ["public"], + acr_values_supported: [UNSPECIFIED_ACR], id_token_signing_alg_values_supported: (() => { if (opts.disableJwtPlugin) return ["HS256" as const]; // Advertise every algorithm the plugin can sign with: the primary diff --git a/packages/oauth-provider/src/oidc-rs256-profile.test.ts b/packages/oauth-provider/src/oidc-rs256-profile.test.ts index e582a4d1c9..eecbfffdee 100644 --- a/packages/oauth-provider/src/oidc-rs256-profile.test.ts +++ b/packages/oauth-provider/src/oidc-rs256-profile.test.ts @@ -84,9 +84,9 @@ describe("OpenID Connect RS256 provider profile", async () => { ], code_challenge_methods_supported: ["S256"], subject_types_supported: ["public"], + acr_values_supported: ["0"], id_token_signing_alg_values_supported: ["RS256"], }); - expect(metadata.acr_values_supported).toBeUndefined(); const jwks = await client.$fetch("/jwks", { method: "GET", @@ -188,11 +188,11 @@ describe("OpenID Connect RS256 provider profile", async () => { sub: expect.any(String), auth_time: expect.any(Number), acr: "0", - name: testUser.name, - email: testUser.email, - email_verified: expect.any(Boolean), }); expect(idToken.payload.at_hash).toEqual(expect.any(String)); + expect(idToken.payload.name).toBeUndefined(); + expect(idToken.payload.email).toBeUndefined(); + expect(idToken.payload.email_verified).toBeUndefined(); const userinfo = await client.$fetch>( "/oauth2/userinfo", diff --git a/packages/oauth-provider/src/token.test.ts b/packages/oauth-provider/src/token.test.ts index 5f71399ce5..036ed4bdeb 100644 --- a/packages/oauth-provider/src/token.test.ts +++ b/packages/oauth-provider/src/token.test.ts @@ -40,27 +40,26 @@ describe("oauth token - authorization_code", async () => { const authServerBaseUrl = "http://localhost:3000"; const rpBaseUrl = "http://localhost:5000"; const validResource = "https://myapi.example.com"; - const { auth, signInWithTestUser, customFetchImpl, testUser } = - await getTestInstance({ - baseURL: authServerBaseUrl, - plugins: [ - jwt({ - jwt: { - issuer: authServerBaseUrl, - }, - }), - oauthProvider({ - loginPage: "/login", - consentPage: "/consent", - resources: [validResource], - enforcePerClientResources: false, - silenceWarnings: { - oauthAuthServerConfig: true, - openidConfig: true, - }, - }), - ], - }); + const { auth, signInWithTestUser, customFetchImpl } = await getTestInstance({ + baseURL: authServerBaseUrl, + plugins: [ + jwt({ + jwt: { + issuer: authServerBaseUrl, + }, + }), + oauthProvider({ + loginPage: "/login", + consentPage: "/consent", + resources: [validResource], + enforcePerClientResources: false, + silenceWarnings: { + oauthAuthServerConfig: true, + openidConfig: true, + }, + }), + ], + }); const { headers } = await signInWithTestUser(); const client = createAuthClient({ @@ -279,7 +278,7 @@ describe("oauth token - authorization_code", async () => { expect(idToken.protectedHeader).toBeDefined(); expect(idToken.payload).toBeDefined(); expect(idToken.payload.sub).toBeDefined(); - expect(idToken.payload.name).toBe(testUser.name); + expect(idToken.payload.name).toBeUndefined(); expect(idToken.payload.email).toBeUndefined(); }); @@ -320,7 +319,7 @@ describe("oauth token - authorization_code", async () => { expect(idToken.payload).toBeDefined(); expect(idToken.payload.sub).toBeDefined(); expect(idToken.payload.name).toBeUndefined(); - expect(idToken.payload.email).toBe(testUser.email); + expect(idToken.payload.email).toBeUndefined(); }); it("scope openid+offline_access should provide opaque access_token, id_token, and refresh_token", async ({ @@ -751,7 +750,7 @@ describe("oauth token - refresh_token", async () => { expect(idToken.protectedHeader).toBeDefined(); expect(idToken.payload).toBeDefined(); expect(idToken.payload.sub).toBeDefined(); - expect(idToken.payload.name).toBeDefined(); + expect(idToken.payload.name).toBeUndefined(); expect(idToken.payload.email).toBeUndefined(); return tokens.data; @@ -1625,30 +1624,29 @@ describe("oauth token - client_credentials", async () => { describe("oauth token - customIdTokenClaims precedence", async () => { const authServerBaseUrl = "http://localhost:3000"; const rpBaseUrl = "http://localhost:5000"; - const { auth, signInWithTestUser, customFetchImpl, testUser } = - await getTestInstance({ - baseURL: authServerBaseUrl, - plugins: [ - jwt({ - jwt: { - issuer: authServerBaseUrl, - }, + const { auth, signInWithTestUser, customFetchImpl } = await getTestInstance({ + baseURL: authServerBaseUrl, + plugins: [ + jwt({ + jwt: { + issuer: authServerBaseUrl, + }, + }), + oauthProvider({ + loginPage: "/login", + consentPage: "/consent", + silenceWarnings: { + oauthAuthServerConfig: true, + openidConfig: true, + }, + customIdTokenClaims: () => ({ + given_name: "CustomFirst", + family_name: "CustomLast", + custom_field: "custom_value", }), - oauthProvider({ - loginPage: "/login", - consentPage: "/consent", - silenceWarnings: { - oauthAuthServerConfig: true, - openidConfig: true, - }, - customIdTokenClaims: () => ({ - given_name: "CustomFirst", - family_name: "CustomLast", - custom_field: "custom_value", - }), - }), - ], - }); + }), + ], + }); const { headers } = await signInWithTestUser(); const client = createAuthClient({ @@ -1693,7 +1691,7 @@ describe("oauth token - customIdTokenClaims precedence", async () => { jwks = createLocalJWKSet(jwksResult.data); }); - it("custom claims should override standard profile claims in id_token", async ({ + it("custom claims can add profile claim names to id_token", async ({ expect, }) => { if (!oauthClient?.client_id || !oauthClient?.client_secret) { @@ -1755,13 +1753,13 @@ describe("oauth token - customIdTokenClaims precedence", async () => { expect(tokens.data?.id_token).toBeDefined(); const idToken = await jwtVerify(tokens.data?.id_token!, jwks); - // Custom claims must override the auto-derived profile claims + // First-party custom claims can deliberately place profile claims in the + // ID token, even though scopes now place them on UserInfo by default. expect(idToken.payload.given_name).toBe("CustomFirst"); expect(idToken.payload.family_name).toBe("CustomLast"); expect(idToken.payload.custom_field).toBe("custom_value"); - // Standard name should still come from the user record (not overridden) - expect(idToken.payload.name).toBe(testUser.name); + expect(idToken.payload.name).toBeUndefined(); expect(idToken.payload.sub).toBeDefined(); }); }); diff --git a/packages/oauth-provider/src/token.ts b/packages/oauth-provider/src/token.ts index 16ce378681..a564d1c270 100644 --- a/packages/oauth-provider/src/token.ts +++ b/packages/oauth-provider/src/token.ts @@ -42,7 +42,7 @@ import type { } from "./types"; import type { GrantType } from "./types/oauth"; import { verificationValueSchema } from "./types/zod"; -import { pickClaims, userNormalClaims } from "./userinfo"; +import { pickClaims } from "./userinfo"; import { clientAllowsGrant, decryptStoredClientSecret, @@ -62,6 +62,15 @@ import { validateClientCredentials, } from "./utils"; +const ID_TOKEN_SCOPE_CLAIM_GUARDS = { + name: undefined, + picture: undefined, + given_name: undefined, + family_name: undefined, + email: undefined, + email_verified: undefined, +} satisfies Record; + /** * Token presentation scheme implied by a confirmation: a DPoP key thumbprint * (`jkt`) yields `"DPoP"`; any other confirmation (including mTLS `x5t#S256`) @@ -320,7 +329,6 @@ async function createIdToken( ) { const iat = Math.floor(Date.now() / 1000); const exp = iat + (opts.idTokenExpiresIn ?? 36000); - const userClaims = userNormalClaims(user, scopes); const resolvedSub = await resolveSubjectIdentifier(user.id, client, opts); const authTimeSec = authTime != null ? Math.floor(authTime.getTime() / 1000) : undefined; @@ -357,7 +365,7 @@ async function createIdToken( client.enableEndSession || client.backchannelLogoutUri, ); const payload: JWTPayload = { - ...userClaims, + ...ID_TOKEN_SCOPE_CLAIM_GUARDS, auth_time: authTimeSec, acr: UNSPECIFIED_ACR, ...customClaims, diff --git a/packages/oauth-provider/src/userinfo.ts b/packages/oauth-provider/src/userinfo.ts index 72459ae30c..1856a04ff7 100644 --- a/packages/oauth-provider/src/userinfo.ts +++ b/packages/oauth-provider/src/userinfo.ts @@ -22,7 +22,7 @@ import { getClient, resolveSubjectIdentifier } from "./utils"; * * @see https://openid.net/specs/openid-connect-core-1_0.html#NormalClaims */ -export function userNormalClaims(user: User, scopes: string[]) { +function userNormalClaims(user: User, scopes: string[]) { const name = user.name.split(" ").filter((v) => v !== ""); const profile = { name: user.name ?? undefined,