From 6ecb7c70b07da8967620881b86dbd96bd70a91c9 Mon Sep 17 00:00:00 2001 From: yannickmonney Date: Wed, 24 Jun 2026 14:38:02 +0200 Subject: [PATCH 01/10] fix(platform): scope SCIM provisioning to the token's tenant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit POST /scim/v2/Users resolved an existing user globally by email and reused that account — grafting a cross-tenant membership onto, and renaming, a user owned by another org. provisionUser now classifies ownership: a user owned only by another org is rejected as a 409 uniqueness conflict (coded ConvexError mapped in the SCIM HTTP layer), and the global user row is renamed only when this org already owns the membership. Also fixes a Group PATCH that combined an `add` with a value-less `remove` silently dropping the adds (the clear-all now composes with adds/removes), and converts the SCIM auth-gate throws to coded ConvexError so production no longer redacts them to a generic "Server Error". Refs #2036, #2085, #2049 --- services/platform/convex/scim/data.ts | 14 ++++ services/platform/convex/scim/http_actions.ts | 15 ++++ .../convex/scim/internal_mutations.ts | 73 ++++++++++++++++++- services/platform/convex/scim/mutations.ts | 14 +++- .../convex/scim/provisioning_logic.test.ts | 59 ++++++++++++++- services/platform/convex/scim/queries.ts | 16 +++- 6 files changed, 180 insertions(+), 11 deletions(-) diff --git a/services/platform/convex/scim/data.ts b/services/platform/convex/scim/data.ts index 955e2e092..7774dfb65 100644 --- a/services/platform/convex/scim/data.ts +++ b/services/platform/convex/scim/data.ts @@ -116,6 +116,20 @@ export async function listOrgMembers( ]); } +/** + * All `member` rows for a user across every org. The SCIM create path uses this + * to decide whether a user matched globally by email is owned by another tenant + * before reusing it (#2036). + */ +export async function listUserMemberships( + ctx: ScimReadCtx, + userId: string, +): Promise { + return gatherAll(ctx, 'member', [ + { field: 'userId', value: userId, operator: 'eq' }, + ]); +} + /** Map of `userId → user` for an org, built from two paginated scans (no N+1). */ export async function buildOrgUserMap( ctx: ScimReadCtx, diff --git a/services/platform/convex/scim/http_actions.ts b/services/platform/convex/scim/http_actions.ts index 3e6b6b4cf..9af8ce4f7 100644 --- a/services/platform/convex/scim/http_actions.ts +++ b/services/platform/convex/scim/http_actions.ts @@ -9,6 +9,9 @@ * `userName eq` / `displayName eq` / `members[value eq "…"]` are not supported. */ +import { ConvexError } from 'convex/values'; + +import { isRecord } from '../../lib/utils/type-utils'; import { internal } from '../_generated/api'; import { type ActionCtx, httpAction } from '../_generated/server'; import type { PlatformRole } from '../enterprise_sso/types'; @@ -116,6 +119,18 @@ function withScimAuth( req, ); } catch (error) { + // A coded ConvexError from the provisioning layer maps to its SCIM + // status — a cross-tenant create collision is a 409, not a 500 (#2036). + if (error instanceof ConvexError && isRecord(error.data)) { + const data = error.data; + if (data.code === 'scim_user_conflict') { + const detail = + typeof data.message === 'string' + ? data.message + : 'User already exists'; + return scimError(409, detail, 'uniqueness'); + } + } console.error('[scim] handler error', error); return scimError(500, 'Internal server error'); } diff --git a/services/platform/convex/scim/internal_mutations.ts b/services/platform/convex/scim/internal_mutations.ts index 2fc6fd698..22950d254 100644 --- a/services/platform/convex/scim/internal_mutations.ts +++ b/services/platform/convex/scim/internal_mutations.ts @@ -1,4 +1,4 @@ -import { v } from 'convex/values'; +import { ConvexError, v } from 'convex/values'; import { getString, isRecord } from '../../lib/utils/type-utils'; import { components } from '../_generated/api'; @@ -10,11 +10,13 @@ import { upsertMemberMirror, upsertTeamMemberMirror, } from '../members/mirror_sync'; +import type { BetterAuthMember } from '../members/types'; import { findMember, findTeamById, findUserByEmail, listTeamMembers, + listUserMemberships, } from './data'; import { deleteLink, getLink, upsertLink } from './links'; import type { ScimGroupRecord, ScimUserRecord } from './types'; @@ -160,6 +162,38 @@ export function planActivation( return { role: current, restoreRole: current }; } +/** + * Classify how a SCIM create may touch a user already matched globally by + * email, given that user's full membership set and the token's org. A SCIM + * token must never graft a membership onto, or rename, an account another + * tenant owns — `owned-elsewhere` is rejected by the create path (#2036). + */ +export function classifyUserOwnership( + memberships: readonly { organizationId: string }[], + organizationId: string, +): 'owned-here' | 'unowned' | 'owned-elsewhere' { + if (memberships.some((m) => m.organizationId === organizationId)) { + return 'owned-here'; + } + return memberships.length > 0 ? 'owned-elsewhere' : 'unowned'; +} + +/** + * Compose a SCIM Group membership PATCH into the final desired user-id set: a + * clear-all / replace base, then adds, then removes. Keeps an `add` paired with + * a value-less `remove members` from being silently dropped (#2085[13]). + */ +export function composeDesiredMembers( + replaceMembers: readonly string[], + addMembers: readonly string[], + removeMembers: readonly string[], +): string[] { + const desired = new Set(replaceMembers); + for (const id of addMembers) desired.add(id); + for (const id of removeMembers) desired.delete(id); + return [...desired]; +} + /** * Create-or-upsert a user + org membership from a SCIM User resource. * Idempotent on `(org, email)` — used by both POST (create) and PUT (replace). @@ -179,9 +213,29 @@ export const provisionUser = internalMutation({ const existingUser = await findUserByEmail(ctx, args.email); let userId: string; + let memberHere: BetterAuthMember | undefined; if (existingUser) { + // A SCIM token is scoped to its own tenant. If the email already maps to + // a global user owned by ANOTHER org, refuse to reuse it — never graft a + // membership onto, or rename, an account this org does not own (#2036). + // The `scim_user_conflict` code is mapped to a 409 by the HTTP layer. + const memberships = await listUserMemberships(ctx, existingUser._id); + if ( + classifyUserOwnership(memberships, args.organizationId) === + 'owned-elsewhere' + ) { + throw new ConvexError({ + code: 'scim_user_conflict', + message: `User ${args.email} belongs to another organization`, + }); + } + memberHere = memberships.find( + (m) => m.organizationId === args.organizationId, + ); userId = existingUser._id; - if (args.name && existingUser.name !== args.name) { + // Only rename when this org already owns the membership — a SCIM token + // must not rewrite the global user row of an account it does not own. + if (memberHere && args.name && existingUser.name !== args.name) { await ctx.runMutation(components.betterAuth.adapter.updateMany, { input: { model: 'user', @@ -211,7 +265,7 @@ export const provisionUser = internalMutation({ } const link = await getLink(ctx, args.organizationId, userId); - const member = await findMember(ctx, args.organizationId, userId); + const member = memberHere; const plan = planActivation( args.active, member?.role, @@ -532,7 +586,18 @@ export const patchGroup = internalMutation({ } if (args.replaceMembers !== undefined) { - await setTeamMembers(ctx, args.teamId, args.replaceMembers); + // A clear-all / replace sets the base set; adds and removes in the SAME + // PATCH compose on top so an `add` paired with a value-less `remove + // members` isn't silently dropped (#2085[13]). + await setTeamMembers( + ctx, + args.teamId, + composeDesiredMembers( + args.replaceMembers, + args.addMembers, + args.removeMembers, + ), + ); } else { const current = await listTeamMembers(ctx, args.teamId); const currentIds = new Set(current.map((m) => m.userId)); diff --git a/services/platform/convex/scim/mutations.ts b/services/platform/convex/scim/mutations.ts index 7a1bfa271..e3a234751 100644 --- a/services/platform/convex/scim/mutations.ts +++ b/services/platform/convex/scim/mutations.ts @@ -1,4 +1,4 @@ -import { v } from 'convex/values'; +import { ConvexError, v } from 'convex/values'; // Raw `mutation` (not the RLS wrapper) + explicit admin checks — same pattern // as `members/mutations.ts` and the enterprise-SSO config mutations. @@ -24,13 +24,21 @@ async function requireAdmin( organizationId: string, ): Promise { const authUser = await getAuthUserIdentity(ctx); - if (!authUser) throw new Error('Unauthenticated'); + if (!authUser) { + throw new ConvexError({ + code: 'unauthenticated', + message: 'Unauthenticated', + }); + } const role = await getCallerRole(ctx, { organizationId, userId: authUser.userId, }); if (!isAdmin(role)) { - throw new Error('Only admins can manage SCIM provisioning'); + throw new ConvexError({ + code: 'forbidden', + message: 'Only admins can manage SCIM provisioning', + }); } return { userId: authUser.userId, email: authUser.email, role: role ?? '' }; } diff --git a/services/platform/convex/scim/provisioning_logic.test.ts b/services/platform/convex/scim/provisioning_logic.test.ts index 986527efd..48593a6a2 100644 --- a/services/platform/convex/scim/provisioning_logic.test.ts +++ b/services/platform/convex/scim/provisioning_logic.test.ts @@ -1,6 +1,10 @@ import { describe, expect, it } from 'vitest'; -import { planActivation } from './internal_mutations'; +import { + classifyUserOwnership, + composeDesiredMembers, + planActivation, +} from './internal_mutations'; /** * Pure tests for the activation/deactivation policy that backs SCIM @@ -48,3 +52,56 @@ describe('planActivation', () => { ); }); }); + +/** + * The org-ownership gate that stops a SCIM token in one tenant from grafting + * onto / renaming a user account owned by another tenant (#2036). + */ +describe('classifyUserOwnership', () => { + it('reuses a user already a member of the token org', () => { + expect(classifyUserOwnership([{ organizationId: 'orgA' }], 'orgA')).toBe( + 'owned-here', + ); + }); + + it('rejects a user owned only by another org (cross-tenant)', () => { + expect(classifyUserOwnership([{ organizationId: 'orgB' }], 'orgA')).toBe( + 'owned-elsewhere', + ); + }); + + it('treats a membership-less global user as unowned (attachable)', () => { + expect(classifyUserOwnership([], 'orgA')).toBe('unowned'); + }); + + it('prefers ownership-here when the user is in both orgs', () => { + expect( + classifyUserOwnership( + [{ organizationId: 'orgB' }, { organizationId: 'orgA' }], + 'orgA', + ), + ).toBe('owned-here'); + }); +}); + +/** + * SCIM Group PATCH membership composition — a clear-all/replace must still apply + * the adds in the same PATCH (#2085[13]). + */ +describe('composeDesiredMembers', () => { + it('keeps adds when combined with a value-less remove (replace=[])', () => { + expect(composeDesiredMembers([], ['a', 'b'], [])).toEqual(['a', 'b']); + }); + + it('composes a replace base with adds and removes', () => { + expect(composeDesiredMembers(['a', 'b'], ['c'], ['a'])).toEqual(['b', 'c']); + }); + + it('clears all members for a bare value-less remove', () => { + expect(composeDesiredMembers([], [], [])).toEqual([]); + }); + + it('dedupes an id present in both replace and add', () => { + expect(composeDesiredMembers(['a'], ['a', 'b'], [])).toEqual(['a', 'b']); + }); +}); diff --git a/services/platform/convex/scim/queries.ts b/services/platform/convex/scim/queries.ts index 7eb47da51..3773d62c6 100644 --- a/services/platform/convex/scim/queries.ts +++ b/services/platform/convex/scim/queries.ts @@ -1,4 +1,4 @@ -import { v } from 'convex/values'; +import { ConvexError, v } from 'convex/values'; import { SSO_CONFIG_DOMAIN, @@ -42,12 +42,22 @@ export const get = query({ returns: scimConfigViewValidator, handler: async (ctx, args) => { const authUser = await getAuthUserIdentity(ctx); - if (!authUser) throw new Error('Unauthenticated'); + if (!authUser) { + throw new ConvexError({ + code: 'unauthenticated', + message: 'Unauthenticated', + }); + } const role = await getCallerRole(ctx, { organizationId: args.organizationId, userId: authUser.userId, }); - if (!role) throw new Error('Not a member of this organization'); + if (!role) { + throw new ConvexError({ + code: 'forbidden', + message: 'Not a member of this organization', + }); + } const baseUrl = scimBaseUrl(); const row = await ctx.db From 4d33c34141a3a70c3b63bc9dc528cf11a74836a4 Mon Sep 17 00:00:00 2001 From: yannickmonney Date: Wed, 24 Jun 2026 14:38:04 +0200 Subject: [PATCH 02/10] fix(platform): route SSO sign-in by email and validate redirects OIDC/OAuth2 sign-in ignored the user's email and always used the first enabled connection, so on a multi-org deployment a user landed at the wrong org's IdP. The login page now resolves the org from the typed email via /api/sso/discover and threads the resolved organizationId through /authorize, the signed state and the callback (and ?org= for SAML) so the code is exchanged against the same connection. With no email it falls back to the single-connection flow. The trusted-headers authenticate endpoint reflected ?redirect= into navigation with no origin validation (open redirect). A shared sanitizeInternalRedirect util restricts it to a same-origin path, applied in the handler and defensively on the login-page forward. SSO config auth gates now throw coded ConvexError. Refs #2082, #2037, #2049 --- services/platform/app/routes/_auth/log-in.tsx | 62 +++++++++++++++++-- .../convex/enterprise_sso/config/actions.ts | 14 ++++- .../convex/enterprise_sso/config/queries.ts | 16 ++++- .../enterprise_sso/login/authorize_handler.ts | 6 +- .../enterprise_sso/login/callback_handler.ts | 19 +++++- .../authenticate_handler.ts | 9 ++- .../lib/shared/utils/safe-redirect.test.ts | 38 ++++++++++++ .../lib/shared/utils/safe-redirect.ts | 38 ++++++++++++ 8 files changed, 185 insertions(+), 17 deletions(-) create mode 100644 services/platform/lib/shared/utils/safe-redirect.test.ts create mode 100644 services/platform/lib/shared/utils/safe-redirect.ts diff --git a/services/platform/app/routes/_auth/log-in.tsx b/services/platform/app/routes/_auth/log-in.tsx index 0ce395d2e..4a6c1807f 100644 --- a/services/platform/app/routes/_auth/log-in.tsx +++ b/services/platform/app/routes/_auth/log-in.tsx @@ -26,7 +26,9 @@ import { toast } from '@/app/hooks/use-toast'; import { authClient } from '@/lib/auth-client'; import { getEnv } from '@/lib/env'; import { useT } from '@/lib/i18n/client'; +import { sanitizeInternalRedirect } from '@/lib/shared/utils/safe-redirect'; import { seo } from '@/lib/utils/seo'; +import { getString, isRecord } from '@/lib/utils/type-utils'; const searchSchema = z.object({ redirectTo: z.string().optional(), @@ -78,7 +80,12 @@ export function LogInPage() { const redirectToTrustedHeadersAuth = useCallback(() => { const siteUrl = getEnv('SITE_URL'); const basePath = getEnv('BASE_PATH'); - const target = redirectTo || `${basePath}/dashboard`; + // Forward only a validated same-origin path — defence in depth against the + // open redirect the authenticate endpoint also guards (#2037). + const target = sanitizeInternalRedirect( + redirectTo, + `${basePath}/dashboard`, + ); window.location.href = `${siteUrl}${basePath}/api/trusted-headers/authenticate?redirect=${encodeURIComponent(target)}`; }, [redirectTo]); useEffect(() => { @@ -255,12 +262,57 @@ export function LogInPage() { } }; - const handleSsoLogin = useCallback(() => { + const handleSsoLogin = useCallback(async () => { const siteUrl = getEnv('SITE_URL'); const basePath = getEnv('BASE_PATH'); - const callbackUri = `${siteUrl}${basePath}/http_api/api/sso/callback`; - window.location.href = `${siteUrl}${basePath}/http_api/api/sso/authorize?redirect_uri=${encodeURIComponent(callbackUri)}`; - }, []); + const base = `${siteUrl}${basePath}/http_api/api/sso`; + + // Route by the typed email's domain so a deployment with more than one SSO + // connection reaches the IdP of the matching org, instead of always the + // first enabled connection (#2082). With no email (the common single-org + // case) or an inconclusive lookup, fall back to the global connection. + const email = form.getValues('email').trim(); + let organizationId: string | undefined; + let protocol: string = ssoConfig?.providerType === 'saml' ? 'saml' : 'oidc'; + if (email) { + try { + const res = await fetch(`${base}/discover`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ email }), + }); + if (res.ok) { + const data: unknown = await res.json(); + if (isRecord(data) && data.ssoEnabled === true) { + const orgId = getString(data, 'organizationId'); + const proto = getString(data, 'protocol'); + if (orgId) { + organizationId = orgId; + if (proto) protocol = proto; + } + } + } + } catch (error) { + console.warn('[sso] discovery failed; using default connection', error); + } + } + + // SAML uses SP-initiated redirect (AuthnRequest); OIDC/OAuth2 use the + // authorization-code flow via /authorize. + if (protocol === 'saml') { + const samlUrl = new URL(`${base}/saml/login`); + if (organizationId) samlUrl.searchParams.set('org', organizationId); + window.location.href = samlUrl.toString(); + return; + } + const authorizeUrl = new URL(`${base}/authorize`); + authorizeUrl.searchParams.set('redirect_uri', `${base}/callback`); + if (email) authorizeUrl.searchParams.set('email', email); + if (organizationId) { + authorizeUrl.searchParams.set('organizationId', organizationId); + } + window.location.href = authorizeUrl.toString(); + }, [ssoConfig?.providerType, form]); // Passkey / WebAuthn sign-in (#1508). Drives the browser's get-credential // ceremony; on success the session is live, so refresh the cache and route diff --git a/services/platform/convex/enterprise_sso/config/actions.ts b/services/platform/convex/enterprise_sso/config/actions.ts index e5a835fbb..64244b86e 100644 --- a/services/platform/convex/enterprise_sso/config/actions.ts +++ b/services/platform/convex/enterprise_sso/config/actions.ts @@ -1,4 +1,4 @@ -import { v } from 'convex/values'; +import { ConvexError, v } from 'convex/values'; import { internal } from '../../_generated/api'; import { action, type ActionCtx } from '../../_generated/server'; @@ -32,13 +32,21 @@ async function requireAdmin( internal.enterprise_sso.config.internal_queries.getAuthUser, {}, ); - if (!authUser) throw new Error('Unauthenticated'); + if (!authUser) { + throw new ConvexError({ + code: 'unauthenticated', + message: 'Unauthenticated', + }); + } const role = await ctx.runQuery( internal.enterprise_sso.config.internal_queries.getCallerRole, { organizationId, userId: authUser._id }, ); if (!isAdmin(role)) { - throw new Error('Only admins can configure SSO'); + throw new ConvexError({ + code: 'forbidden', + message: 'Only admins can configure SSO', + }); } return { userId: authUser._id, email: authUser.email, role: role ?? 'admin' }; } diff --git a/services/platform/convex/enterprise_sso/config/queries.ts b/services/platform/convex/enterprise_sso/config/queries.ts index 0294e5d6b..b0b2c2409 100644 --- a/services/platform/convex/enterprise_sso/config/queries.ts +++ b/services/platform/convex/enterprise_sso/config/queries.ts @@ -1,4 +1,4 @@ -import { v } from 'convex/values'; +import { ConvexError, v } from 'convex/values'; import { SSO_CONFIG_DOMAIN, @@ -101,12 +101,22 @@ export const get = query({ // to the shared `SsoConnectionView` the UI consumes. handler: async (ctx, args): Promise => { const authUser = await getAuthUserIdentity(ctx); - if (!authUser) throw new Error('Unauthenticated'); + if (!authUser) { + throw new ConvexError({ + code: 'unauthenticated', + message: 'Unauthenticated', + }); + } const role = await getCallerRole(ctx, { organizationId: args.organizationId, userId: authUser.userId, }); - if (!role) throw new Error('Not a member of this organization'); + if (!role) { + throw new ConvexError({ + code: 'forbidden', + message: 'Not a member of this organization', + }); + } const base = publicBase(); const scimBaseUrl = base ? `${base}/scim/v2` : null; diff --git a/services/platform/convex/enterprise_sso/login/authorize_handler.ts b/services/platform/convex/enterprise_sso/login/authorize_handler.ts index 9072f5336..251b894b0 100644 --- a/services/platform/convex/enterprise_sso/login/authorize_handler.ts +++ b/services/platform/convex/enterprise_sso/login/authorize_handler.ts @@ -34,6 +34,7 @@ export async function ssoAuthorizeHandler( try { const url = new URL(req.url); const email = url.searchParams.get('email'); + const organizationId = url.searchParams.get('organizationId') || undefined; const promptParam = url.searchParams.get('prompt'); const seamlessParam = url.searchParams.get('seamless'); const claimsParam = url.searchParams.get('claims'); @@ -50,7 +51,7 @@ export async function ssoAuthorizeHandler( const config = await ctx.runQuery( internal.enterprise_sso.internal_queries.resolveSignInConfig, - {}, + { organizationId }, ); if (!config) { return new Response('No SSO configuration found', { status: 404 }); @@ -89,6 +90,9 @@ export async function ssoAuthorizeHandler( redirectUri, timestamp: Date.now(), seamless: prompt === 'none', + // Bind the resolved org to the state so the callback exchanges the code + // against the SAME connection — not whichever is first enabled (#2082). + organizationId: config.organizationId, ...(encryptedPkceVerifier ? { pkce: encryptedPkceVerifier } : {}), }); const base64Payload = btoa(statePayload) diff --git a/services/platform/convex/enterprise_sso/login/callback_handler.ts b/services/platform/convex/enterprise_sso/login/callback_handler.ts index d5d1c9c9a..428834d81 100644 --- a/services/platform/convex/enterprise_sso/login/callback_handler.ts +++ b/services/platform/convex/enterprise_sso/login/callback_handler.ts @@ -84,7 +84,12 @@ export async function ssoCallbackHandler( const authorizeUrl = buildAuthorizeRedirectUrl( url.origin, stateData.redirectUri, - { prompt: 'login' }, + { + prompt: 'login', + ...(stateData.organizationId + ? { organizationId: stateData.organizationId } + : {}), + }, ); return new Response(null, { status: 302, @@ -100,6 +105,9 @@ export async function ssoCallbackHandler( const claimsChallenge = extractClaimsChallenge(errorDescription); const params: Record = { prompt: 'login' }; + if (stateData.organizationId) { + params['organizationId'] = stateData.organizationId; + } if (claimsChallenge) params['claims'] = claimsChallenge; const authorizeUrl = buildAuthorizeRedirectUrl( url.origin, @@ -151,7 +159,12 @@ export async function ssoCallbackHandler( return redirectWithError(url.origin, 'Invalid state signature'); } - let state: { redirectUri: string; timestamp: number; pkce?: string }; + let state: { + redirectUri: string; + timestamp: number; + pkce?: string; + organizationId?: string; + }; try { const base64 = verifiedPayload.replace(/-/g, '+').replace(/_/g, '/'); const padded = base64 + '='.repeat((4 - (base64.length % 4)) % 4); @@ -168,7 +181,7 @@ export async function ssoCallbackHandler( const config = await ctx.runQuery( internal.enterprise_sso.internal_queries.resolveSignInConfig, - {}, + { organizationId: state.organizationId }, ); if (!config) { return redirectWithError(frontendOrigin, 'SSO configuration not found'); diff --git a/services/platform/convex/trusted_headers_auth/authenticate_handler.ts b/services/platform/convex/trusted_headers_auth/authenticate_handler.ts index 24afc6003..88ba33f57 100644 --- a/services/platform/convex/trusted_headers_auth/authenticate_handler.ts +++ b/services/platform/convex/trusted_headers_auth/authenticate_handler.ts @@ -14,6 +14,7 @@ import { makeFunctionReference } from 'convex/server'; +import { sanitizeInternalRedirect } from '../../lib/shared/utils/safe-redirect'; import type { ActionCtx } from '../_generated/server'; import { createAuth } from '../auth'; import { signCookieValue } from '../enterprise_sso/sign_cookie_value'; @@ -67,8 +68,12 @@ export async function trustedHeadersAuthenticateHandler( const url = new URL(req.url); const frontendOrigin = url.origin; const basePath = process.env.BASE_PATH || ''; - const redirectTo = - url.searchParams.get('redirect') || `${basePath}/dashboard`; + // Validate the redirect target to a same-origin path — a user-supplied + // absolute or protocol-relative URL here is an open redirect (#2037). + const redirectTo = sanitizeInternalRedirect( + url.searchParams.get('redirect'), + `${basePath}/dashboard`, + ); // Guard: trusted headers must be enabled if (process.env.TRUSTED_HEADERS_ENABLED !== 'true') { diff --git a/services/platform/lib/shared/utils/safe-redirect.test.ts b/services/platform/lib/shared/utils/safe-redirect.test.ts new file mode 100644 index 000000000..c53817ac5 --- /dev/null +++ b/services/platform/lib/shared/utils/safe-redirect.test.ts @@ -0,0 +1,38 @@ +import { describe, expect, it } from 'vitest'; + +import { isSafeInternalPath, sanitizeInternalRedirect } from './safe-redirect'; + +describe('isSafeInternalPath', () => { + it('accepts root-relative paths', () => { + expect(isSafeInternalPath('/')).toBe(true); + expect(isSafeInternalPath('/dashboard')).toBe(true); + expect(isSafeInternalPath('/sign-in')).toBe(true); + expect(isSafeInternalPath('/dashboard/abc?tab=1#section')).toBe(true); + }); + + it('rejects absolute and protocol-relative URLs', () => { + expect(isSafeInternalPath('https://evil.com')).toBe(false); + expect(isSafeInternalPath('//evil.com')).toBe(false); + expect(isSafeInternalPath('/\\evil.com')).toBe(false); + expect(isSafeInternalPath('http:/evil.com')).toBe(false); + }); + + it('rejects values that are not root-relative paths', () => { + expect(isSafeInternalPath('')).toBe(false); + expect(isSafeInternalPath('dashboard')).toBe(false); + expect(isSafeInternalPath(' /dashboard')).toBe(false); + }); +}); + +describe('sanitizeInternalRedirect', () => { + it('returns a safe path unchanged', () => { + expect(sanitizeInternalRedirect('/dashboard', '/home')).toBe('/dashboard'); + }); + + it('falls back for unsafe or missing values', () => { + expect(sanitizeInternalRedirect('https://evil.com', '/home')).toBe('/home'); + expect(sanitizeInternalRedirect('//evil.com', '/home')).toBe('/home'); + expect(sanitizeInternalRedirect(null, '/home')).toBe('/home'); + expect(sanitizeInternalRedirect(undefined, '/home')).toBe('/home'); + }); +}); diff --git a/services/platform/lib/shared/utils/safe-redirect.ts b/services/platform/lib/shared/utils/safe-redirect.ts new file mode 100644 index 000000000..3fa79c1cc --- /dev/null +++ b/services/platform/lib/shared/utils/safe-redirect.ts @@ -0,0 +1,38 @@ +/** + * Redirect-target hardening shared by the auth flows. A redirect parameter a + * user can influence must resolve to a path on our own origin — never an + * attacker-controlled absolute or protocol-relative URL (open redirect / + * phishing). See issue #2037. + */ + +const RESOLVE_BASE = 'https://internal.invalid'; + +/** + * True when `value` is a safe root-relative path on our own origin: a single + * leading slash, no scheme, and no protocol-relative (`//`) or backslash (`/\`) + * host smuggling. Query string and fragment are allowed. + */ +export function isSafeInternalPath(value: string): boolean { + if (!value.startsWith('/')) return false; + // `//evil.com` and `/\evil.com` are read as a host by browsers (the latter + // because URL parsing folds `\` to `/` for http(s) schemes). Reject both. + if (value.length > 1 && (value[1] === '/' || value[1] === '\\')) return false; + // Defence in depth: resolved against a fixed origin, a safe path must not + // escape it. + try { + return new URL(value, RESOLVE_BASE).origin === RESOLVE_BASE; + } catch { + return false; + } +} + +/** + * Return `raw` when it is a safe root-relative path, otherwise `fallback`. + * `fallback` is trusted (caller-constructed) and returned as-is. + */ +export function sanitizeInternalRedirect( + raw: string | null | undefined, + fallback: string, +): string { + return typeof raw === 'string' && isSafeInternalPath(raw) ? raw : fallback; +} From c1ac55609320880eb24168d9549e291dbc6afdae Mon Sep 17 00:00:00 2001 From: yannickmonney Date: Wed, 24 Jun 2026 14:38:06 +0200 Subject: [PATCH 03/10] fix(platform): polish enterprise SSO + 2FA settings UX and a11y Switching a configured connection's protocol to OIDC with a blank client secret passed client validation, then the backend threw an untranslated raw Error. The secret requirement is now gated on whether an OIDC secret is actually stored (so a SAML->OIDC switch requires one), and the backend raises a coded, localized ConvexError instead. The SSO settings form's Select/Switch controls no longer mount uncontrolled (their values default), removing the controlled/uncontrolled console warnings. The standing 2FA grace / low-backup-code banners use a polite role="status" instead of assertive role="alert", and the password-expiry gate matches the /forced-change-password/$id route by inclusion so it stops re-navigating to it. Refs #2057, #2095, #2085 --- .../components/two-factor-grace-banner.tsx | 2 +- .../two-factor-low-backup-codes-banner.tsx | 2 +- .../auth/hooks/use-password-expiry-gate.ts | 5 ++- .../components/enterprise-sso-form.test.tsx | 44 +++++++++++++++++++ .../components/enterprise-sso-form.tsx | 38 +++++++++------- .../enterprise_sso/config/file_actions.ts | 9 +++- services/platform/messages/de.json | 3 +- services/platform/messages/en.json | 3 +- services/platform/messages/fr.json | 3 +- 9 files changed, 85 insertions(+), 24 deletions(-) diff --git a/services/platform/app/features/auth/components/two-factor-grace-banner.tsx b/services/platform/app/features/auth/components/two-factor-grace-banner.tsx index e9475cb2e..5f11c2627 100644 --- a/services/platform/app/features/auth/components/two-factor-grace-banner.tsx +++ b/services/platform/app/features/auth/components/two-factor-grace-banner.tsx @@ -40,7 +40,7 @@ export function TwoFactorGraceBanner({ return (
{ if (!data || !data.expired) return; - if (location.pathname.endsWith(FORCED_CHANGE_PATH)) return; + // The route is `/forced-change-password/$id`, so the pathname ends with the + // id, never the literal segment — match by inclusion so the gate actually + // short-circuits on that page instead of re-navigating to it (#2085[06]). + if (location.pathname.includes(`/${FORCED_CHANGE_PATH}`)) return; const id = (params as { id?: string }).id ?? organizationId; void navigate({ to: '/forced-change-password/$id', diff --git a/services/platform/app/features/settings/enterprise-sso/components/enterprise-sso-form.test.tsx b/services/platform/app/features/settings/enterprise-sso/components/enterprise-sso-form.test.tsx index 4c23c43e5..1d9e7f65b 100644 --- a/services/platform/app/features/settings/enterprise-sso/components/enterprise-sso-form.test.tsx +++ b/services/platform/app/features/settings/enterprise-sso/components/enterprise-sso-form.test.tsx @@ -333,4 +333,48 @@ describe('EnterpriseSsoForm validation + save', () => { const errors = await screen.findAllByText(/this field is required/i); expect(errors.length).toBeGreaterThan(0); }); + + it('requires a client secret when switching a SAML connection to OIDC (#2057)', async () => { + upsertOidcMock.mockClear(); + const { user } = renderForm(samlConfig); + + // Switch the protocol SAML → Microsoft Entra ID. + await user.click(screen.getByRole('combobox', { name: /protocol/i })); + await user.click( + await screen.findByRole('option', { name: /microsoft entra id/i }), + ); + + // A SAML-only connection has no stored OIDC secret to reuse, so a blank + // secret must keep Save blocked even though issuer + client id are filled. + await user.type( + screen.getByLabelText(/issuer url/i), + 'https://login.example.com', + ); + await user.type(screen.getByLabelText(/^client id$/i), 'client-123'); + + const saveButton = await screen.findByRole('button', { name: /^save$/i }); + await waitFor(() => expect(saveButton).toBeDisabled()); + expect(upsertOidcMock).not.toHaveBeenCalled(); + }); + + it('mounts controls defined so no uncontrolled→controlled warning fires as config loads (#2095)', () => { + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + const { rerender } = render( + + + , + ); + rerender( + + + , + ); + const warned = errorSpy.mock.calls.some((call) => + /uncontrolled to controlled|controlled to uncontrolled|changing an uncontrolled|changing a controlled/i.test( + String(call[0]), + ), + ); + errorSpy.mockRestore(); + expect(warned).toBe(false); + }); }); diff --git a/services/platform/app/features/settings/enterprise-sso/components/enterprise-sso-form.tsx b/services/platform/app/features/settings/enterprise-sso/components/enterprise-sso-form.tsx index 67155cbc0..a96ebe710 100644 --- a/services/platform/app/features/settings/enterprise-sso/components/enterprise-sso-form.tsx +++ b/services/platform/app/features/settings/enterprise-sso/components/enterprise-sso-form.tsx @@ -32,6 +32,7 @@ import type { PlatformRole, SsoConnectionView, } from '@/lib/shared/schemas/enterprise_sso'; +import { convexErrorCode, convexErrorMessage } from '@/lib/utils/convex-error'; import { narrowStringUnion } from '@/lib/utils/type-utils'; import { @@ -171,12 +172,13 @@ export function EnterpriseSsoForm({ organizationId, config }: Props) { const canEdit = !cannotManage; // ------------------------------------------------------------------------- - // Validation schema (memoized on `t` and `config?.configured`). The protocol - // Select drives which fields are required; `clientSecret` is required only - // for a brand-new connection (an existing connection keeps its stored secret - // when the field is left blank). + // Validation schema (memoized on `t` and whether an OIDC secret is stored). + // The protocol Select drives which fields are required; `clientSecret` is + // required whenever no OIDC client secret is already stored — i.e. a new + // connection OR a switch to OIDC from a SAML-only connection. An existing + // OIDC connection keeps its stored secret when the field is left blank. // ------------------------------------------------------------------------- - const isConfigured = !!config?.configured; + const hasStoredOidcSecret = !!config?.oidc; const schema = useMemo(() => { const requiredMsg = t('integrations.enterpriseSso.validation.required'); const urlMsg = t('integrations.enterpriseSso.validation.url'); @@ -232,9 +234,12 @@ export function EnterpriseSsoForm({ organizationId, config }: Props) { if (!data.issuer.trim()) req('issuer'); else if (!looksLikeUrl(data.issuer.trim())) url('issuer'); if (!data.clientId.trim()) req('clientId'); - // Secret is required only when configuring a NEW connection. On an - // existing connection a blank secret means "keep the stored one". - if (!isConfigured && !data.clientSecret.trim()) req('clientSecret'); + // Required unless an OIDC secret is already stored. A blank secret on + // an existing OIDC connection means "keep the stored one"; switching + // to OIDC from a SAML-only connection has no stored secret to reuse. + if (!hasStoredOidcSecret && !data.clientSecret.trim()) { + req('clientSecret'); + } if (data.protocol === 'oauth2') { for (const field of [ @@ -255,7 +260,7 @@ export function EnterpriseSsoForm({ organizationId, config }: Props) { if (!data.idpCertificate.trim()) req('idpCertificate'); } }); - }, [t, isConfigured]); + }, [t, hasStoredOidcSecret]); // ------------------------------------------------------------------------- // Seed the form once the stored connection loads. `data` is `undefined` @@ -391,11 +396,12 @@ export function EnterpriseSsoForm({ organizationId, config }: Props) { variant: 'success', }); } catch (error) { + const fallback = t('integrations.enterpriseSso.saveFailed'); toast({ title: - error instanceof Error - ? error.message - : t('integrations.enterpriseSso.saveFailed'), + convexErrorCode(error) === 'sso_client_secret_required' + ? t('integrations.enterpriseSso.validation.clientSecretRequired') + : convexErrorMessage(error, fallback), variant: 'destructive', }); throw error; @@ -559,7 +565,7 @@ export function EnterpriseSsoForm({ organizationId, config }: Props) { id="sso-protocol" label={t('integrations.enterpriseSso.protocolLabel')} description={t('integrations.enterpriseSso.protocolHelp')} - value={field.value} + value={field.value ?? 'entra-id'} onValueChange={(value) => { const next = narrowStringUnion( value, @@ -749,7 +755,7 @@ export function EnterpriseSsoForm({ organizationId, config }: Props) { { errorSpy.mockRestore(); expect(warned).toBe(false); }); + + it('adds a role-mapping rule and saves it (#2085[12])', async () => { + upsertOidcMock.mockClear(); + revealClientIdMock.mockResolvedValueOnce('client-xyz'); + const { user } = renderForm(connectedOidc); + + // Wait for the stored client id to be revealed so the OIDC form is valid. + await waitFor(() => + expect(screen.getByLabelText(/^client id$/i)).toHaveValue('client-xyz'), + ); + + // The editor is visible (the connection auto-provisions roles). Add a rule + // mapping the IdP group "Engineering" to the default member role. + await user.click(screen.getByRole('button', { name: /add rule/i })); + await user.type(screen.getByLabelText(/matches value/i), 'Engineering'); + + const saveButton = await screen.findByRole('button', { name: /^save$/i }); + await waitFor(() => expect(saveButton).toBeEnabled()); + await user.click(saveButton); + + await waitFor(() => expect(upsertOidcMock).toHaveBeenCalledTimes(1)); + expect(upsertOidcMock.mock.calls[0][0].roleMappingRules).toEqual([ + { source: 'group', pattern: 'Engineering', targetRole: 'member' }, + ]); + }); }); diff --git a/services/platform/app/features/settings/enterprise-sso/components/enterprise-sso-form.tsx b/services/platform/app/features/settings/enterprise-sso/components/enterprise-sso-form.tsx index a96ebe710..d5a36b40e 100644 --- a/services/platform/app/features/settings/enterprise-sso/components/enterprise-sso-form.tsx +++ b/services/platform/app/features/settings/enterprise-sso/components/enterprise-sso-form.tsx @@ -2,7 +2,7 @@ import { Badge } from '@tale/ui/badge'; import { Button } from '@tale/ui/button'; -import { HStack, Stack } from '@tale/ui/layout'; +import { HStack, Row, Stack } from '@tale/ui/layout'; import { StatusIndicator } from '@tale/ui/status-indicator'; import { Text } from '@tale/ui/text'; import { Check, Copy, Loader2 } from 'lucide-react'; @@ -14,7 +14,12 @@ import { useRef, useState, } from 'react'; -import { Controller } from 'react-hook-form'; +import { + Controller, + type Control, + useFieldArray, + useWatch, +} from 'react-hook-form'; import { z } from 'zod'; import { @@ -30,6 +35,7 @@ import { useToast } from '@/app/hooks/use-toast'; import { useT } from '@/lib/i18n/client'; import type { PlatformRole, + RoleMappingRule, SsoConnectionView, } from '@/lib/shared/schemas/enterprise_sso'; import { convexErrorCode, convexErrorMessage } from '@/lib/utils/convex-error'; @@ -111,10 +117,26 @@ interface SsoFormData { // Provisioning defaultRole: PlatformRole; autoRole: boolean; + roleMappingRules: RoleMappingRule[]; autoTeam: boolean; excludeGroups: string; } +const ROLE_RULE_SOURCES = [ + 'group', + 'appRole', + 'jobTitle', + 'claim', +] as const satisfies readonly RoleMappingRule['source'][]; +type RoleRuleSource = (typeof ROLE_RULE_SOURCES)[number]; + +const ROLE_RULE_TARGETS = [ + 'admin', + 'developer', + 'editor', + 'member', +] as const satisfies readonly PlatformRole[]; + const isOidcProtocol = (p: UiProtocol): p is Exclude => p !== 'saml'; @@ -208,6 +230,20 @@ export function EnterpriseSsoForm({ organizationId, config }: Props) { 'disabled', ]), autoRole: z.boolean(), + roleMappingRules: z.array( + z.object({ + source: z.enum(ROLE_RULE_SOURCES), + pattern: z.string(), + targetRole: z.enum([ + 'admin', + 'developer', + 'editor', + 'member', + 'disabled', + ]), + claim: z.string().optional(), + }), + ), autoTeam: z.boolean(), excludeGroups: z.string(), }) @@ -325,6 +361,7 @@ export function EnterpriseSsoForm({ organizationId, config }: Props) { idpCertificate, defaultRole: config.provisioning.defaultRole, autoRole: config.provisioning.autoProvisionRole, + roleMappingRules: config.provisioning.roleMappingRules, autoTeam: config.provisioning.autoProvisionTeam, excludeGroups: config.provisioning.excludeGroups.join(', '), }; @@ -335,7 +372,9 @@ export function EnterpriseSsoForm({ organizationId, config }: Props) { const provisioning = { autoProvisionRole: values.autoRole, defaultRole: values.defaultRole, - roleMappingRules: config?.provisioning.roleMappingRules ?? [], + roleMappingRules: values.roleMappingRules.filter((r) => + r.pattern.trim(), + ), autoProvisionTeam: values.autoTeam, excludeGroups: values.excludeGroups .split(',') @@ -425,6 +464,7 @@ export function EnterpriseSsoForm({ organizationId, config }: Props) { const protocol = watch('protocol') ?? 'entra-id'; const isOidcLike = isOidcProtocol(protocol); + const autoRole = watch('autoRole') ?? false; // ------------------------------------------------------------------------- // Prefill clientId once for an existing OIDC connection (the read view no @@ -780,6 +820,7 @@ export function EnterpriseSsoForm({ organizationId, config }: Props) { /> )} /> + {autoRole && } ); } + +/** + * Editor for the IdP → platform role-mapping rules (drives the "auto-assign + * roles from the IdP" toggle, which is otherwise inert without rules). The + * first matching rule wins; a user who matches none gets the default role. + */ +function RoleMappingRulesEditor({ + control, +}: { + control: Control; +}) { + const { t } = useT('settings'); + const { fields, append, remove } = useFieldArray({ + control, + name: 'roleMappingRules', + }); + const sourceOptions = ROLE_RULE_SOURCES.map((s) => ({ + value: s, + label: t(`integrations.enterpriseSso.roleMapping.source.${s}`), + })); + const roleOptions = ROLE_RULE_TARGETS.map((r) => ({ + value: r, + label: t(`integrations.enterpriseSso.role.${r}`), + })); + + return ( + + + {t('integrations.enterpriseSso.roleMapping.help')} + + {fields.length === 0 ? ( + + {t('integrations.enterpriseSso.roleMapping.empty')} + + ) : ( + + {fields.map((field, index) => ( + remove(index)} + /> + ))} + + )} + + + + + ); +} + +function RoleMappingRuleRow({ + control, + index, + sourceOptions, + roleOptions, + onRemove, +}: { + control: Control; + index: number; + sourceOptions: { value: string; label: string }[]; + roleOptions: { value: string; label: string }[]; + onRemove: () => void; +}) { + const { t } = useT('settings'); + const source = useWatch({ + control, + name: `roleMappingRules.${index}.source`, + }); + + return ( + + + ( + + )} + /> + ( + + )} + /> + )} + + ); +} diff --git a/services/platform/app/routes/2fa-enroll.tsx b/services/platform/app/routes/2fa-enroll.tsx index 7e491a90c..8441211d4 100644 --- a/services/platform/app/routes/2fa-enroll.tsx +++ b/services/platform/app/routes/2fa-enroll.tsx @@ -20,14 +20,16 @@ import { useSearch, } from '@tanstack/react-router'; import { QRCodeSVG } from 'qrcode.react'; -import { useState } from 'react'; +import { useEffect, useState } from 'react'; import { z } from 'zod'; import { Input } from '@/app/components/ui/forms/input'; import { LogoLink } from '@/app/components/ui/logo/logo-link'; import { PasskeyRegisterDialog } from '@/app/features/settings/account/components/passkey-register-dialog'; +import { useConvexQuery } from '@/app/hooks/use-convex-query'; import { useReactQueryClient } from '@/app/hooks/use-react-query-client'; import { toast } from '@/app/hooks/use-toast'; +import { api } from '@/convex/_generated/api'; import { authClient } from '@/lib/auth-client'; import { useT } from '@/lib/i18n/client'; import { extractSecret, normalizeOtpauthURI } from '@/lib/utils/totp'; @@ -83,6 +85,21 @@ function TwoFactorEnrollPage() { const [submitting, setSubmitting] = useState(false); const [passkeyDialogOpen, setPasskeyDialogOpen] = useState(false); + // Parity with the forced-change-password wall: don't trap a user who has no + // reason to be on the enforced enrollment wall — already enrolled, or the org + // doesn't enforce 2FA (#2085[04]). The session stays active either way. + const { data: status } = useConvexQuery( + api.two_factor.queries.getStatus, + {}, + { requireAuth: false }, + ); + useEffect(() => { + if (!status || !status.authenticated) return; + if (status.twoFactorEnabled || !status.enforced) { + void navigate({ to: redirectTo || '/dashboard', replace: true }); + } + }, [status, navigate, redirectTo]); + async function beginEnrollment(e: React.FormEvent) { e.preventDefault(); if (submitting || !password) return; diff --git a/services/platform/messages/de.json b/services/platform/messages/de.json index 397f2d22d..75bd981a4 100644 --- a/services/platform/messages/de.json +++ b/services/platform/messages/de.json @@ -5756,6 +5756,23 @@ "url": "Gib eine gültige URL ein (inklusive https://).", "clientSecretRequired": "Ein Client-Secret ist erforderlich." }, + "roleMapping": { + "help": "Ordne Gruppen, App-Rollen oder Jobtitel aus dem Identity-Provider einer Plattformrolle zu. Wer auf keine Regel passt, erhält die Standardrolle.", + "empty": "Noch keine Regeln — alle erhalten die Standardrolle.", + "addRule": "Regel hinzufügen", + "removeRule": "Entfernen", + "sourceLabel": "Quelle", + "source": { + "group": "IdP-Gruppe", + "appRole": "App-Rolle", + "jobTitle": "Jobtitel", + "claim": "Claim" + }, + "patternLabel": "Passender Wert", + "targetRoleLabel": "Weist Rolle zu", + "claimLabel": "Claim-Pfad", + "claimHelp": "Punktpfad in die Token-Claims, z. B. realm_access.roles." + }, "guide": { "title": "Einrichtungsleitfaden", "redirectLabel": "Weiterleitungs-/Callback-URL", @@ -6917,6 +6934,7 @@ "disabled": "Zwei-Faktor-Authentifizierung deaktiviert", "passwordPromptDescription": "Bestätige dein Passwort, um die Einrichtung der Zwei-Faktor-Authentifizierung zu starten.", "disablePromptDescription": "Bestätige dein Passwort, um die Zwei-Faktor-Authentifizierung auszuschalten.", + "disableEnforcedWarning": "Deine Organisation verlangt Zwei-Faktor-Authentifizierung. Wenn du sie deaktivierst, musst du sie bei der nächsten Anmeldung erneut einrichten.", "regeneratePromptDescription": "Bestätige dein Passwort. Beim Neuerzeugen werden alle bisherigen Backup-Codes ungültig." }, "passkeys": { diff --git a/services/platform/messages/en.json b/services/platform/messages/en.json index 7d3ba31c2..ca0066163 100644 --- a/services/platform/messages/en.json +++ b/services/platform/messages/en.json @@ -6018,6 +6018,23 @@ "url": "Enter a valid URL (including https://).", "clientSecretRequired": "A client secret is required." }, + "roleMapping": { + "help": "Map identity-provider groups, app roles, or job titles to a platform role. Users who match no rule get the default role.", + "empty": "No rules yet — every user gets the default role.", + "addRule": "Add rule", + "removeRule": "Remove", + "sourceLabel": "Source", + "source": { + "group": "IdP group", + "appRole": "App role", + "jobTitle": "Job title", + "claim": "Claim" + }, + "patternLabel": "Matches value", + "targetRoleLabel": "Assigns role", + "claimLabel": "Claim path", + "claimHelp": "Dot-path into the token claims, e.g. realm_access.roles." + }, "guide": { "title": "Setup guide", "redirectLabel": "Redirect / callback URL", @@ -7179,6 +7196,7 @@ "disabled": "Two-factor authentication disabled", "passwordPromptDescription": "Confirm your password to start setting up two-factor authentication.", "disablePromptDescription": "Confirm your password to turn off two-factor authentication.", + "disableEnforcedWarning": "Your organization requires two-factor authentication. If you disable it, you'll have to set it up again the next time you sign in.", "regeneratePromptDescription": "Confirm your password. Generating new codes invalidates all previous backup codes." }, "passkeys": { diff --git a/services/platform/messages/fr.json b/services/platform/messages/fr.json index 3d3086c05..90088648e 100644 --- a/services/platform/messages/fr.json +++ b/services/platform/messages/fr.json @@ -5757,6 +5757,23 @@ "url": "Saisissez une URL valide (avec https://).", "clientSecretRequired": "Un secret client est requis." }, + "roleMapping": { + "help": "Associez les groupes, rôles d'application ou intitulés de poste du fournisseur d'identité à un rôle de la plateforme. Les utilisateurs sans correspondance reçoivent le rôle par défaut.", + "empty": "Aucune règle pour l'instant — tout le monde reçoit le rôle par défaut.", + "addRule": "Ajouter une règle", + "removeRule": "Supprimer", + "sourceLabel": "Source", + "source": { + "group": "Groupe IdP", + "appRole": "Rôle d'application", + "jobTitle": "Intitulé de poste", + "claim": "Claim" + }, + "patternLabel": "Valeur correspondante", + "targetRoleLabel": "Attribue le rôle", + "claimLabel": "Chemin du claim", + "claimHelp": "Chemin pointé dans les claims du jeton, p. ex. realm_access.roles." + }, "guide": { "title": "Guide de configuration", "redirectLabel": "URL de redirection / de rappel", @@ -6918,6 +6935,7 @@ "disabled": "Authentification à double facteur désactivée", "passwordPromptDescription": "Confirme ton mot de passe pour commencer la configuration de l'authentification à double facteur.", "disablePromptDescription": "Confirme ton mot de passe pour désactiver l'authentification à double facteur.", + "disableEnforcedWarning": "Votre organisation exige l'authentification à double facteur. Si vous la désactivez, vous devrez la reconfigurer à votre prochaine connexion.", "regeneratePromptDescription": "Confirme ton mot de passe. Générer de nouveaux codes invalide tous les codes de secours précédents." }, "passkeys": { From a5ac8abe0c088da2c83923fe421a0576c851792d Mon Sep 17 00:00:00 2001 From: yannickmonney Date: Wed, 24 Jun 2026 16:00:48 +0200 Subject: [PATCH 06/10] fix(platform): scope the 2fa-enroll guard to already-enrolled users The #2085[04] guard navigated away whenever 2FA was not enforced, which bounced users off /2fa-enroll even when they came to enroll voluntarily, and could interrupt an in-progress enrollment (verifyTotp flips twoFactorEnabled before the backup codes are shown). Restrict the auto-exit to an already-enrolled user still on the initial password step, so the standalone enrollment route stays usable. Caught by auth-account.spec. Refs #2085 --- services/platform/app/routes/2fa-enroll.tsx | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/services/platform/app/routes/2fa-enroll.tsx b/services/platform/app/routes/2fa-enroll.tsx index 8441211d4..deed835c0 100644 --- a/services/platform/app/routes/2fa-enroll.tsx +++ b/services/platform/app/routes/2fa-enroll.tsx @@ -85,9 +85,12 @@ function TwoFactorEnrollPage() { const [submitting, setSubmitting] = useState(false); const [passkeyDialogOpen, setPasskeyDialogOpen] = useState(false); - // Parity with the forced-change-password wall: don't trap a user who has no - // reason to be on the enforced enrollment wall — already enrolled, or the org - // doesn't enforce 2FA (#2085[04]). The session stays active either way. + // Don't trap a user who is ALREADY enrolled on the enrollment wall — bounce + // them out (parity with the forced-change-password guard). Gate on the + // initial 'password' step so an in-progress enrollment is never interrupted + // (verifyTotp flips twoFactorEnabled before the backup codes are shown), and + // so /2fa-enroll stays usable for voluntary enrollment by users 2FA isn't + // enforced for (#2085[04]). const { data: status } = useConvexQuery( api.two_factor.queries.getStatus, {}, @@ -95,10 +98,10 @@ function TwoFactorEnrollPage() { ); useEffect(() => { if (!status || !status.authenticated) return; - if (status.twoFactorEnabled || !status.enforced) { + if (status.twoFactorEnabled && step.kind === 'password') { void navigate({ to: redirectTo || '/dashboard', replace: true }); } - }, [status, navigate, redirectTo]); + }, [status, step.kind, navigate, redirectTo]); async function beginEnrollment(e: React.FormEvent) { e.preventDefault(); From 90f45208e42efb9b418e6066068a3ebed3c4dadf Mon Sep 17 00:00:00 2001 From: yannickmonney Date: Wed, 24 Jun 2026 18:38:36 +0200 Subject: [PATCH 07/10] fix(platform): request Microsoft Graph User.Read for Entra SSO userinfo The Entra ID adapter's getUserInfo() reads the signed-in user from Microsoft Graph /me, which requires the User.Read delegated permission. When an org's configured scopes omitted it (e.g. only GroupMember.Read.All), the access token came back without Graph user access and the userinfo call 403'd, breaking the whole OIDC/Entra sign-in after a successful token exchange. buildAuthorizeUrl now always ensures https://graph.microsoft.com/User.Read is requested, and the settings form's default Entra scope set lists it explicitly. Verified live against a real Entra tenant over a tunnel: sign-in now completes through /me (200) to a provisioned session. --- .../enterprise-sso/components/enterprise-sso-form.tsx | 2 +- .../platform/convex/enterprise_sso/entra_id/adapter.ts | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/services/platform/app/features/settings/enterprise-sso/components/enterprise-sso-form.tsx b/services/platform/app/features/settings/enterprise-sso/components/enterprise-sso-form.tsx index d5a36b40e..ab96c5e3e 100644 --- a/services/platform/app/features/settings/enterprise-sso/components/enterprise-sso-form.tsx +++ b/services/platform/app/features/settings/enterprise-sso/components/enterprise-sso-form.tsx @@ -89,7 +89,7 @@ interface Props { const DEFAULT_SCOPES: Record = { 'entra-id': - 'openid email profile offline_access https://graph.microsoft.com/GroupMember.Read.All', + 'openid email profile offline_access https://graph.microsoft.com/User.Read https://graph.microsoft.com/GroupMember.Read.All', 'generic-oidc': 'openid email profile', oauth2: 'email profile', saml: '', diff --git a/services/platform/convex/enterprise_sso/entra_id/adapter.ts b/services/platform/convex/enterprise_sso/entra_id/adapter.ts index 402e29347..5dc158c34 100644 --- a/services/platform/convex/enterprise_sso/entra_id/adapter.ts +++ b/services/platform/convex/enterprise_sso/entra_id/adapter.ts @@ -39,6 +39,13 @@ function buildAuthorizeUrl( ); const scopes = [...config.scopes]; + // getUserInfo always reads the signed-in user from Microsoft Graph `/me`, + // which needs the User.Read delegated permission. Ensure it is requested even + // when the configured scopes omit it (a scope set that lists only + // GroupMember.Read.All would otherwise 403 the userinfo call). + if (!scopes.some((s) => /(^|\/)user\.read$/i.test(s))) { + scopes.push('https://graph.microsoft.com/User.Read'); + } if (params.additionalScopes) { for (const scope of params.additionalScopes) { if (!scopes.includes(scope)) { From ec57986eab4f95ba77a34653bc2ed1e2ed9f8090 Mon Sep 17 00:00:00 2001 From: yannickmonney Date: Wed, 24 Jun 2026 18:39:13 +0200 Subject: [PATCH 08/10] fix(platform): de-provision the user on SCIM DELETE (RFC 7644), protect owner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A SCIM HTTP DELETE was mapped to a soft-deactivate (PATCH active:false): it returned 204 but the user stayed retrievable via GET (200) and kept appearing in GET /Users. RFC 7644 §3.6 requires a deleted resource to no longer be returned, and it left User DELETE inconsistent with Group DELETE (which truly removes). Add deprovisionUser: it removes the org membership, its mirror, and the provisioning link so a subsequent GET/PATCH returns 404, symmetric with deleteGroup. The global Better Auth user row is preserved (the person may belong to other orgs; a SCIM token never mutates an account it doesn't own — #2036). The IdP's usual de-provisioning signal, PATCH active:false, still soft- deactivates (membership kept, role restorable) — that path is unchanged. The sole owner is never removed (that would orphan the org): DELETE on the owner returns 403 (scimType "mutability"). New pure classifyDeprovision() covers the 404 / owner-protected / remove decision; verified live against a real Entra tenant (DELETE 204 then GET 404; owner DELETE 403). --- services/platform/convex/scim/http_actions.ts | 22 +++--- .../convex/scim/internal_mutations.ts | 77 ++++++++++++++++++- .../convex/scim/provisioning_logic.test.ts | 31 +++++++- 3 files changed, 117 insertions(+), 13 deletions(-) diff --git a/services/platform/convex/scim/http_actions.ts b/services/platform/convex/scim/http_actions.ts index 9af8ce4f7..aa1778276 100644 --- a/services/platform/convex/scim/http_actions.ts +++ b/services/platform/convex/scim/http_actions.ts @@ -290,17 +290,21 @@ async function patchUserResource( } async function deleteUser(rc: ScimRc, userId: string): Promise { - const existing = await rc.ctx.runQuery( - internal.scim.internal_queries.getUserRecord, + // A SCIM DELETE removes the resource: the user is de-provisioned from this + // org so a later GET returns 404 (RFC 7644 §3.6). A plain disable is the + // separate `PATCH active:false` path (soft-deactivate, restorable). + const result = await rc.ctx.runMutation( + internal.scim.internal_mutations.deprovisionUser, { organizationId: rc.organizationId, userId }, ); - if (!existing) return scimError(404, `User ${userId} not found`); - await rc.ctx.runMutation(internal.scim.internal_mutations.patchUser, { - organizationId: rc.organizationId, - userId, - defaultRole: rc.defaultRole, - active: false, - }); + if (result === 'not-found') return scimError(404, `User ${userId} not found`); + if (result === 'owner-protected') { + return scimError( + 403, + 'Cannot de-provision the organization owner', + 'mutability', + ); + } return scimNoContent(); } diff --git a/services/platform/convex/scim/internal_mutations.ts b/services/platform/convex/scim/internal_mutations.ts index 22950d254..8436fb8b8 100644 --- a/services/platform/convex/scim/internal_mutations.ts +++ b/services/platform/convex/scim/internal_mutations.ts @@ -6,6 +6,7 @@ import { internalMutation, type MutationCtx } from '../_generated/server'; import * as AuditLogHelpers from '../audit_logs/helpers'; import { platformRoleValidator } from '../enterprise_sso/validators'; import { + deleteMemberMirrorByMemberId, deleteTeamMemberMirrorByTeamMemberId, upsertMemberMirror, upsertTeamMemberMirror, @@ -15,6 +16,7 @@ import { findMember, findTeamById, findUserByEmail, + findUserById, listTeamMembers, listUserMemberships, } from './data'; @@ -178,6 +180,19 @@ export function classifyUserOwnership( return memberships.length > 0 ? 'owned-elsewhere' : 'unowned'; } +/** + * Decide how an HTTP DELETE resolves for a SCIM User, from the caller's + * membership in the token's org: a missing membership is a 404; the org owner + * is protected (removing it would orphan the org); anything else is removed. + */ +export function classifyDeprovision( + member: { role?: string } | undefined, +): 'not-found' | 'owner-protected' | 'deprovision' { + if (!member) return 'not-found'; + if ((member.role ?? '').toLowerCase() === 'owner') return 'owner-protected'; + return 'deprovision'; +} + /** * Compose a SCIM Group membership PATCH into the final desired user-id set: a * clear-all / replace base, then adds, then removes. Keeps an `add` paired with @@ -335,9 +350,11 @@ export const provisionUser = internalMutation({ }); /** - * Apply a SCIM User PATCH (active toggle + optional name/email). Also serves - * DELETE (soft-deactivate) by passing `active: false`. Returns null if the - * user is not a member of the org. + * Apply a SCIM User PATCH (active toggle + optional name/email). A SCIM + * `active: false` — the IdP's primary de-provisioning signal — soft-deactivates: + * the membership is KEPT with role `disabled` so a later `active: true` restores + * the prior role. (A hard `DELETE` is the separate `deprovisionUser` path.) + * Returns null if the user is not a member of the org. */ export const patchUser = internalMutation({ args: { @@ -427,6 +444,60 @@ export const patchUser = internalMutation({ }, }); +/** + * Hard de-provision a SCIM User (HTTP DELETE): drop the org membership, its + * mirror, and the provisioning link, so the resource is gone from this tenant's + * SCIM view — a subsequent GET/PATCH returns 404, per RFC 7644 §3.6 ("the + * resource ... MUST NOT be returned"). This is the symmetric counterpart to + * `deleteGroup`. The global Better Auth `user` row is intentionally preserved + * (the person may belong to other orgs, and Tale never lets a SCIM token mutate + * an account it doesn't own — #2036). + * + * This is DISTINCT from a SCIM `active: false`, which `patchUser` + * soft-deactivates (membership kept, restorable) — the IdP's usual + * de-provisioning signal. The sole owner is never removed: deleting it would + * orphan the org, so that case is reported as `owner-protected` (HTTP 403). + */ +export const deprovisionUser = internalMutation({ + args: { organizationId: v.string(), userId: v.string() }, + returns: v.union( + v.literal('deprovisioned'), + v.literal('not-found'), + v.literal('owner-protected'), + ), + handler: async (ctx, args) => { + const member = await findMember(ctx, args.organizationId, args.userId); + const verdict = classifyDeprovision(member); + if (verdict === 'not-found' || verdict === 'owner-protected') + return verdict; + // `verdict === 'deprovision'` implies a member exists; re-assert it so the + // type narrows (classifyDeprovision already guaranteed it at runtime). + if (!member) return 'not-found'; + + const user = await findUserById(ctx, args.userId); + + await ctx.runMutation(components.betterAuth.adapter.deleteOne, { + input: { + model: 'member', + where: [{ field: '_id', value: member._id, operator: 'eq' }], + }, + }); + await deleteMemberMirrorByMemberId(ctx, member._id); + await deleteLink(ctx, args.organizationId, args.userId); + + await logScim( + ctx, + args.organizationId, + 'scim_deprovision_user', + 'member', + args.userId, + user?.email ?? args.userId, + { previous: { role: member.role } }, + ); + return 'deprovisioned'; + }, +}); + // --------------------------------------------------------------------------- // Groups // --------------------------------------------------------------------------- diff --git a/services/platform/convex/scim/provisioning_logic.test.ts b/services/platform/convex/scim/provisioning_logic.test.ts index 48593a6a2..d42be62c1 100644 --- a/services/platform/convex/scim/provisioning_logic.test.ts +++ b/services/platform/convex/scim/provisioning_logic.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from 'vitest'; import { + classifyDeprovision, classifyUserOwnership, composeDesiredMembers, planActivation, @@ -8,7 +9,7 @@ import { /** * Pure tests for the activation/deactivation policy that backs SCIM - * `active:false`, DELETE (soft), and reactivation. No backend required. + * `active:false` (soft-deactivate) and reactivation. No backend required. */ describe('planActivation', () => { it('creates a new active member at the default role', () => { @@ -84,6 +85,34 @@ describe('classifyUserOwnership', () => { }); }); +/** + * HTTP DELETE de-provisions a SCIM User (RFC 7644 §3.6 — the resource must no + * longer be returned), distinct from the soft `active:false` disable. The org + * owner is protected so a SCIM token can never orphan an org. + */ +describe('classifyDeprovision', () => { + it('404s when the user is not a member of the org', () => { + expect(classifyDeprovision(undefined)).toBe('not-found'); + }); + + it('removes a regular member', () => { + expect(classifyDeprovision({ role: 'member' })).toBe('deprovision'); + }); + + it('removes an admin member', () => { + expect(classifyDeprovision({ role: 'admin' })).toBe('deprovision'); + }); + + it('protects the org owner from SCIM removal (case-insensitive)', () => { + expect(classifyDeprovision({ role: 'owner' })).toBe('owner-protected'); + expect(classifyDeprovision({ role: 'OWNER' })).toBe('owner-protected'); + }); + + it('treats a missing role as a removable member', () => { + expect(classifyDeprovision({})).toBe('deprovision'); + }); +}); + /** * SCIM Group PATCH membership composition — a clear-all/replace must still apply * the adds in the same PATCH (#2085[13]). From 822a11c9debae070d06abe36ceb049fbfa330924 Mon Sep 17 00:00:00 2001 From: yannickmonney Date: Wed, 24 Jun 2026 18:50:37 +0200 Subject: [PATCH 09/10] style(workflow): reformat desk-process.json for oxfmt 0.44.0 Pure whitespace reformat (a short operations array collapsed to one line) so the repo-wide oxfmt --check passes under the CI toolchain version. No workflow semantics or version field changed. Surfaced by merging main, where oxfmt 0.44.0 flags the file the committed formatting predates. --- .../apps/issue-desk/workflows/issue-desk/desk-process.json | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/builtin-configs/apps/issue-desk/workflows/issue-desk/desk-process.json b/builtin-configs/apps/issue-desk/workflows/issue-desk/desk-process.json index 7137aa6c8..4324ffb97 100644 --- a/builtin-configs/apps/issue-desk/workflows/issue-desk/desk-process.json +++ b/builtin-configs/apps/issue-desk/workflows/issue-desk/desk-process.json @@ -21,10 +21,7 @@ "integrations": [ { "name": "github", - "operations": [ - "get_issue", - "create_pull_request" - ] + "operations": ["get_issue", "create_pull_request"] } ] }, From 210e699584319d7c06aa53a026d5bdc47b0eeb40 Mon Sep 17 00:00:00 2001 From: yannickmonney Date: Wed, 24 Jun 2026 19:26:19 +0200 Subject: [PATCH 10/10] fix(platform): re-sync the IdP-mapped role to existing SSO members each login MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "Auto-assign roles from the IdP" only set a member's role at first provision: findOrCreateSsoUser computed the mapped role but, for an existing membership, discarded it and returned. So an IdP promotion/demotion (e.g. adding a user to an admin group) never propagated — the role stuck at whatever the user's first login produced, a correctness and security gap (stale elevated roles). handle_sso_login now passes syncRole = autoProvisionRole; when set, findOrCreateSsoUser re-applies the mapped role to an existing membership. The org owner is never touched (would orphan the org) and no-op writes are skipped (new pure shouldSyncMemberRole, unit-tested). Found live against a real Entra tenant: a user in an "All Company" group with an "All Company -> admin" rule stayed `member` across logins; with the fix the re-login promotes them to `admin`. Group->team sync verified in the same run (teams created from the user's Entra groups). --- .../find_or_create_sso_user.test.ts | 31 ++++++++++ .../enterprise_sso/find_or_create_sso_user.ts | 57 ++++++++++++++++++- .../convex/enterprise_sso/handle_sso_login.ts | 3 + .../enterprise_sso/internal_mutations.ts | 3 + 4 files changed, 93 insertions(+), 1 deletion(-) create mode 100644 services/platform/convex/enterprise_sso/find_or_create_sso_user.test.ts diff --git a/services/platform/convex/enterprise_sso/find_or_create_sso_user.test.ts b/services/platform/convex/enterprise_sso/find_or_create_sso_user.test.ts new file mode 100644 index 000000000..e8b76ff22 --- /dev/null +++ b/services/platform/convex/enterprise_sso/find_or_create_sso_user.test.ts @@ -0,0 +1,31 @@ +import { describe, expect, it } from 'vitest'; + +import { shouldSyncMemberRole } from './find_or_create_sso_user'; + +/** + * "Auto-assign roles from the IdP" must keep an EXISTING member's role in sync + * on every login (a promotion/demotion in the IdP should propagate), not just at + * first provision — but it must never demote the org owner. + */ +describe('shouldSyncMemberRole', () => { + it('promotes an existing member when the mapped role differs', () => { + expect(shouldSyncMemberRole(true, 'member', 'admin')).toBe(true); + }); + + it('demotes an existing member when the mapped role drops', () => { + expect(shouldSyncMemberRole(true, 'admin', 'member')).toBe(true); + }); + + it('is a no-op when the role is unchanged', () => { + expect(shouldSyncMemberRole(true, 'admin', 'admin')).toBe(false); + }); + + it('never touches the owner (would orphan the org)', () => { + expect(shouldSyncMemberRole(true, 'owner', 'admin')).toBe(false); + }); + + it('does nothing when auto-assign is off', () => { + expect(shouldSyncMemberRole(false, 'member', 'admin')).toBe(false); + expect(shouldSyncMemberRole(undefined, 'member', 'admin')).toBe(false); + }); +}); diff --git a/services/platform/convex/enterprise_sso/find_or_create_sso_user.ts b/services/platform/convex/enterprise_sso/find_or_create_sso_user.ts index 42bb5b265..a414cf5ff 100644 --- a/services/platform/convex/enterprise_sso/find_or_create_sso_user.ts +++ b/services/platform/convex/enterprise_sso/find_or_create_sso_user.ts @@ -21,8 +21,33 @@ type FindOrCreateSsoUserArgs = { accessTokenExpiresAt?: number; organizationId: string; role: PlatformRole; + /** + * Re-apply `role` to an existing membership on every login (set when the org + * enables "auto-assign roles from the IdP"), so an IdP promotion/demotion + * propagates instead of sticking at the role from the user's first login. + */ + syncRole?: boolean; }; +function num(value: unknown): number | undefined { + return typeof value === 'number' ? value : undefined; +} + +/** + * Whether to overwrite an existing membership's role with the IdP-mapped role on + * login. Only when "auto-assign roles from the IdP" is on (`syncRole`), never + * for an `owner` (that would orphan the org), and not for a no-op. + */ +export function shouldSyncMemberRole( + syncRole: boolean | undefined, + currentRole: string | undefined, + newRole: string, +): boolean { + return ( + Boolean(syncRole) && currentRole !== 'owner' && currentRole !== newRole + ); +} + type FindOrCreateSsoUserResult = { userId: string | null; isNewUser: boolean; @@ -118,7 +143,10 @@ export async function findOrCreateSsoUser( }, ); - const existingMembership = membershipRes?.page?.[0]; + const existingMembershipRaw = membershipRes?.page?.[0]; + const existingMembership = isRecord(existingMembershipRaw) + ? existingMembershipRaw + : undefined; if (!existingMembership) { const memberCreatedAt = Date.now(); @@ -146,6 +174,33 @@ export async function findOrCreateSsoUser( createdAt: memberCreatedAt, }); } + } else if (args.syncRole) { + // "Auto-assign roles from the IdP" is authoritative: re-apply the mapped + // role to the existing membership so an IdP promotion/demotion takes + // effect on the next login. Never touch an `owner` (that would orphan the + // org), and skip a no-op write. + const memberId = extractMemberId(existingMembership); + const currentRole = getString(existingMembership, 'role'); + if ( + memberId && + shouldSyncMemberRole(args.syncRole, currentRole, args.role) + ) { + await ctx.runMutation(components.betterAuth.adapter.updateMany, { + input: { + model: 'member' as const, + where: [{ field: '_id', value: memberId, operator: 'eq' }], + update: { role: args.role }, + }, + paginationOpts: { cursor: null, numItems: 1 }, + }); + await upsertMemberMirror(ctx, { + memberId, + userId: existingUserId, + organizationId: args.organizationId, + role: args.role, + createdAt: num(existingMembership.createdAt) ?? Date.now(), + }); + } } return { userId: existingUserId, isNewUser: false }; diff --git a/services/platform/convex/enterprise_sso/handle_sso_login.ts b/services/platform/convex/enterprise_sso/handle_sso_login.ts index f6e290570..e042f8f01 100644 --- a/services/platform/convex/enterprise_sso/handle_sso_login.ts +++ b/services/platform/convex/enterprise_sso/handle_sso_login.ts @@ -79,6 +79,9 @@ export async function handleSsoLogin( accessTokenExpiresAt: args.accessTokenExpiresAt, organizationId: args.organizationId, role, + // IdP is authoritative for roles when auto-assign is on — keep an + // existing member's role in sync on every login, not just at creation. + syncRole: config.autoProvisionRole, }, ); diff --git a/services/platform/convex/enterprise_sso/internal_mutations.ts b/services/platform/convex/enterprise_sso/internal_mutations.ts index 2777820a1..a85752229 100644 --- a/services/platform/convex/enterprise_sso/internal_mutations.ts +++ b/services/platform/convex/enterprise_sso/internal_mutations.ts @@ -22,6 +22,9 @@ export const findOrCreateSsoUser = internalMutation({ accessTokenExpiresAt: v.optional(v.number()), organizationId: v.string(), role: platformRoleValidator, + // When true (auto-assign roles from the IdP is on), re-apply the mapped role + // to an EXISTING membership on every login so IdP role changes propagate. + syncRole: v.optional(v.boolean()), }, returns: v.object({ userId: v.union(v.string(), v.null()),