diff --git a/packages/sso/src/oidc.test.ts b/packages/sso/src/oidc.test.ts index 446a2f1a8f..0e4c1ba9d4 100644 --- a/packages/sso/src/oidc.test.ts +++ b/packages/sso/src/oidc.test.ts @@ -2279,7 +2279,7 @@ describe("SSO OIDC IDP-initiated bounce", async () => { expect(res.status).toBe(302); const location = res.headers.get("location") || ""; expect(location).not.toContain("http://localhost:8080/authorize"); - expect(location).toContain("please_restart_the_process"); + expect(location).toContain("state_not_found"); }); it("should carry ssoProviderId in the bounced state when options.redirectURI is configured", async () => { @@ -2329,6 +2329,6 @@ describe("SSO OIDC IDP-initiated bounce", async () => { ); expect(res.status).toBe(302); - expect(res.headers.get("location")).toContain("please_restart_the_process"); + expect(res.headers.get("location")).toContain("state_not_found"); }); }); diff --git a/packages/sso/src/providers.test.ts b/packages/sso/src/providers.test.ts index b43c309c51..51e12393ca 100644 --- a/packages/sso/src/providers.test.ts +++ b/packages/sso/src/providers.test.ts @@ -1467,7 +1467,6 @@ kBGIJYs= samlConfig: { entryPoint: "https://idp.example.com/sso", cert: TEST_CERT, - callbackUrl: "http://localhost:3000/api/sso/callback", audience: "my-audience", wantAssertionsSigned: true, spMetadata: {}, @@ -1516,7 +1515,6 @@ kBGIJYs= samlConfig: { entryPoint: "https://idp.example.com/sso", cert: TEST_CERT, - callbackUrl: "http://localhost:3000/api/sso/callback", audience: "my-audience", wantAssertionsSigned: true, spMetadata: {}, @@ -1558,7 +1556,6 @@ kBGIJYs= samlConfig: { entryPoint: "https://idp.example.com/sso", cert: TEST_CERT, - callbackUrl: "http://localhost:3000/api/sso/callback", audience: "my-audience", wantAssertionsSigned: true, spMetadata: {}, diff --git a/packages/sso/src/routes/providers.ts b/packages/sso/src/routes/providers.ts index feb9931115..15b515ddc6 100644 --- a/packages/sso/src/routes/providers.ts +++ b/packages/sso/src/routes/providers.ts @@ -6,6 +6,11 @@ import { } from "better-auth/api"; import * as z from "zod"; import { DEFAULT_MAX_SAML_METADATA_SIZE } from "../constants"; +import { + DiscoveryError, + mapDiscoveryErrorToAPIError, + validateSkipDiscoveryEndpoints, +} from "../oidc"; import { resolveSigningCerts, validateCertSources, @@ -29,6 +34,10 @@ interface SSOProviderRecord { const ADMIN_ROLES = ["owner", "admin"]; +export function hasOrgAdminRole(member: Pick): boolean { + return member.role.split(",").some((r) => ADMIN_ROLES.includes(r.trim())); +} + type ParsedCert = ReturnType; type SanitizedCert = ParsedCert | { error: string }; @@ -67,9 +76,7 @@ async function isOrgAdmin( { field: "organizationId", value: organizationId }, ], }); - if (!member) return false; - const roles = member.role.split(","); - return roles.some((r) => ADMIN_ROLES.includes(r.trim())); + return member ? hasOrgAdminRole(member) : false; } async function batchCheckOrgAdmin( @@ -93,8 +100,7 @@ async function batchCheckOrgAdmin( const adminOrgIds = new Set(); for (const member of members) { - const roles = member.role.split(","); - if (roles.some((r: string) => ADMIN_ROLES.includes(r.trim()))) { + if (hasOrgAdminRole(member)) { adminOrgIds.add(member.organizationId); } } @@ -504,6 +510,17 @@ export const updateSSOProvider = (options: SSOOptions) => { } if (body.oidcConfig) { + try { + validateSkipDiscoveryEndpoints(body.oidcConfig, (url) => + ctx.context.isTrustedOrigin(url), + ); + } catch (error) { + if (error instanceof DiscoveryError) { + throw mapDiscoveryErrorToAPIError(error); + } + throw error; + } + const currentOidcConfig = parseAndValidateConfig( existingProvider.oidcConfig, "OIDC", diff --git a/packages/sso/src/routes/saml-pipeline.ts b/packages/sso/src/routes/saml-pipeline.ts index 62afac8320..1c9d6d83c0 100644 --- a/packages/sso/src/routes/saml-pipeline.ts +++ b/packages/sso/src/routes/saml-pipeline.ts @@ -1,3 +1,4 @@ +import { isAPIError } from "@better-auth/core/utils/is-api-error"; import type { User } from "better-auth"; import { APIError } from "better-auth/api"; import { setSessionCookie } from "better-auth/cookies"; @@ -380,24 +381,36 @@ export async function processSAMLResponse( validateEmailDomain(userInfo.email as string, provider.domain)); const postAuthRedirect = relayState?.callbackURL || ctx.context.baseURL; + const errorUrl = relayState?.errorURL || samlRedirectUrl; - const result = await handleOAuthUserInfo(ctx, { - userInfo: { - email: userInfo.email as string, - name: (userInfo.name || userInfo.email) as string, - id: userInfo.id as string, - emailVerified: Boolean(userInfo.emailVerified), - }, - account: { - providerId, - accountId: userInfo.id as string, - accessToken: "", - refreshToken: "", - }, - callbackURL: postAuthRedirect, - disableSignUp: options?.disableImplicitSignUp, - isTrustedProvider, - }); + let result: Awaited>; + try { + result = await handleOAuthUserInfo(ctx, { + userInfo: { + email: userInfo.email as string, + name: (userInfo.name || userInfo.email) as string, + id: userInfo.id as string, + emailVerified: Boolean(userInfo.emailVerified), + }, + account: { + providerId, + accountId: userInfo.id as string, + accessToken: "", + refreshToken: "", + }, + callbackURL: postAuthRedirect, + disableSignUp: options?.disableImplicitSignUp, + isTrustedProvider, + }); + } catch (e) { + if (isAPIError(e) && e.body?.code) { + const params = new URLSearchParams({ error: e.body.code }); + if (e.body.message) params.set("error_description", e.body.message); + const sep = errorUrl.includes("?") ? "&" : "?"; + throw ctx.redirect(`${errorUrl}${sep}${params.toString()}`); + } + throw e; + } if (result.error) { throw ctx.redirect( @@ -442,6 +455,7 @@ export async function processSAMLResponse( const samlSessionKey = `${constants.SAML_SESSION_KEY_PREFIX}${providerId}:${extract.nameID}`; const samlSessionData: SAMLSessionRecord = { sessionId: session.id, + sessionToken: session.token, providerId, nameID: extract.nameID, sessionIndex: (extract as SAMLAssertionExtract).sessionIndex diff --git a/packages/sso/src/routes/sso.ts b/packages/sso/src/routes/sso.ts index f221830fe3..a062cb35be 100644 --- a/packages/sso/src/routes/sso.ts +++ b/packages/sso/src/routes/sso.ts @@ -1,3 +1,4 @@ +import { isAPIError } from "@better-auth/core/utils/is-api-error"; import { BetterFetchError, betterFetch } from "@better-fetch/fetch"; import type { PrivateKeyJwtSigningAlgorithm, @@ -36,12 +37,14 @@ import { discoverOIDCConfig, ensureRuntimeDiscovery, mapDiscoveryErrorToAPIError, + validateSkipDiscoveryEndpoints, } from "../oidc"; import { validateCertSources, validateConfigAlgorithms } from "../saml"; import { SAML_ERROR_CODES } from "../saml/error-codes"; import { generateRelayState } from "../saml-state"; import type { AuthnRequestRecord, + Member, OIDCConfig, SAMLConfig, SAMLSessionRecord, @@ -56,6 +59,7 @@ import { createSP, findSAMLProvider, } from "./helpers"; +import { hasOrgAdminRole } from "./providers"; import { getSafeRedirectUrl, processSAMLResponse } from "./saml-pipeline"; import { registerSSOProviderBodySchema } from "./schemas"; @@ -386,7 +390,7 @@ export const registerSSOProvider = (options: O) => { } if (ctx.body.organizationId) { - const organization = await ctx.context.adapter.findOne({ + const member = await ctx.context.adapter.findOne({ model: "member", where: [ { @@ -399,11 +403,17 @@ export const registerSSOProvider = (options: O) => { }, ], }); - if (!organization) { + if (!member) { throw new APIError("BAD_REQUEST", { message: "You are not a member of the organization", }); } + if (ctx.context.hasPlugin("organization") && !hasOrgAdminRole(member)) { + throw new APIError("FORBIDDEN", { + message: + "You must be an organization owner or admin to register SSO providers", + }); + } } const existingProvider = await ctx.context.adapter.findOne({ @@ -425,6 +435,19 @@ export const registerSSOProvider = (options: O) => { }); } + if (body.oidcConfig) { + try { + validateSkipDiscoveryEndpoints(body.oidcConfig, (url) => + ctx.context.isTrustedOrigin(url), + ); + } catch (error) { + if (error instanceof DiscoveryError) { + throw mapDiscoveryErrorToAPIError(error); + } + throw error; + } + } + let hydratedOIDCConfig: HydratedOIDCConfig | null = null; if (body.oidcConfig && !body.oidcConfig.skipDiscovery) { try { @@ -1442,33 +1465,48 @@ async function handleOIDCCallback( (provider as { domainVerified?: boolean }).domainVerified === true && validateEmailDomain(userInfo.email, provider.domain); - const linked = await handleOAuthUserInfo(ctx, { - userInfo: { - email: userInfo.email, - name: userInfo.name || "", - id: userInfo.id, - image: userInfo.image, - emailVerified: options?.trustEmailVerified - ? userInfo.emailVerified || false - : false, - }, - account: { - idToken: tokenResponse.idToken, - accessToken: tokenResponse.accessToken, - refreshToken: tokenResponse.refreshToken, - accountId: userInfo.id, - providerId: provider.providerId, - accessTokenExpiresAt: tokenResponse.accessTokenExpiresAt, - refreshTokenExpiresAt: tokenResponse.refreshTokenExpiresAt, - scope: tokenResponse.scopes?.join(","), - }, - callbackURL, - disableSignUp: options?.disableImplicitSignUp && !requestSignUp, - overrideUserInfo: config.overrideUserInfo, - isTrustedProvider, - }); + let linked: Awaited>; + try { + linked = await handleOAuthUserInfo(ctx, { + userInfo: { + email: userInfo.email, + name: userInfo.name || "", + id: userInfo.id, + image: userInfo.image, + emailVerified: options?.trustEmailVerified + ? userInfo.emailVerified || false + : false, + }, + account: { + idToken: tokenResponse.idToken, + accessToken: tokenResponse.accessToken, + refreshToken: tokenResponse.refreshToken, + accountId: userInfo.id, + providerId: provider.providerId, + accessTokenExpiresAt: tokenResponse.accessTokenExpiresAt, + refreshTokenExpiresAt: tokenResponse.refreshTokenExpiresAt, + scope: tokenResponse.scopes?.join(","), + }, + callbackURL, + disableSignUp: options?.disableImplicitSignUp && !requestSignUp, + overrideUserInfo: config.overrideUserInfo, + isTrustedProvider, + }); + } catch (e) { + if (isAPIError(e) && e.body?.code) { + const baseURL = errorURL || callbackURL; + const params = new URLSearchParams({ error: e.body.code }); + if (e.body.message) params.set("error_description", e.body.message); + const sep = baseURL.includes("?") ? "&" : "?"; + throw ctx.redirect(`${baseURL}${sep}${params.toString()}`); + } + throw e; + } if (linked.error) { - throw ctx.redirect(`${errorURL || callbackURL}?error=${linked.error}`); + const baseURL = errorURL || callbackURL; + const params = new URLSearchParams({ error: linked.error }); + const sep = baseURL.includes("?") ? "&" : "?"; + throw ctx.redirect(`${baseURL}${sep}${params.toString()}`); } const { session, user } = linked.data!; @@ -1997,7 +2035,7 @@ async function handleLogoutRequest( sessionIndex === data.sessionIndex ) { await ctx.context.internalAdapter - .deleteSession(data.sessionId) + .deleteSession(data.sessionToken) .catch((e: unknown) => ctx.context.logger.warn("Failed to delete session during SLO", { error: e, @@ -2036,7 +2074,9 @@ async function handleLogoutRequest( const currentSession = await getSessionFromCtx(ctx); if (currentSession?.session) { - await ctx.context.internalAdapter.deleteSession(currentSession.session.id); + await ctx.context.internalAdapter.deleteSession( + currentSession.session.token, + ); } deleteSessionCookie(ctx); @@ -2179,7 +2219,7 @@ export const initiateSLO = (options?: SSOOptions) => { ), ); - await ctx.context.internalAdapter.deleteSession(session.session.id); + await ctx.context.internalAdapter.deleteSession(session.session.token); deleteSessionCookie(ctx);