Skip to content

Commit afd7e27

Browse files
committed
fix(db): give the SSO index migration the concurrent-build convention it skipped
packages/db/scripts/migrate.ts documents the required shape for CONCURRENTLY statements, and the previous migration follows it. 0284 did not, and the omission is silently destructive. migrate.ts sets a session lock_timeout of 5s, which survives the embedded COMMIT. CREATE INDEX CONCURRENTLY waits on every concurrent write transaction in the database — not only ones touching this table — so on a busy database the build is cancelled with 55P03 and leaves an INVALID index. The retry then replays the file, IF NOT EXISTS skips the invalid index, and DROP INDEX removes the only working index on provider_id. The migration journals as applied and exits 0 with provider_id unindexed and uniqueness unenforced, reopening the cross-tenant provider resolution this migration exists to close. Adds SET lock_timeout = 0 around the concurrent statements, a pre-drop of the target index name so a replay rebuilds rather than skips, and restores the 5s timeout afterwards. Verified by stranding an INVALID index and replaying: the end state is a valid unique index with uniqueness enforced. Also corrects the sso() comment that claimed domainVerified confines linking to matching email domains. link-account.mjs blocks on `!isTrustedProvider && !userInfo.emailVerified`, so an IdP asserting email_verified links regardless of domain — the flag narrows nothing on its own.
1 parent 33e2cee commit afd7e27

4 files changed

Lines changed: 38 additions & 52 deletions

File tree

apps/sim/ee/sso/components/sso-settings.tsx

Lines changed: 8 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -48,11 +48,7 @@ interface SSOProvider {
4848
providerType: 'oidc' | 'saml'
4949
}
5050

51-
/**
52-
* Claim/attribute names each protocol uses out of the box. Kept as the fallback
53-
* rather than seeded into form state so switching protocol needs no reset logic
54-
* and the inputs can show them as placeholders.
55-
*/
51+
/** Claim names each protocol uses out of the box; shown as input placeholders. */
5652
const OIDC_DEFAULT_MAPPING = { id: 'sub', email: 'email', name: 'name', image: 'picture' } as const
5753
const SAML_DEFAULT_MAPPING = {
5854
id: 'http://schemas.xmlsoap.org/ws/2005/05/identity/claims/nameidentifier',
@@ -257,30 +253,6 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) {
257253
setShowAdvanced(false)
258254
}
259255

260-
const isFormValid = () => {
261-
const requiredFields = ['providerId', 'issuerUrl', 'domain']
262-
const hasRequiredFields = requiredFields.every((field) => {
263-
const value = formData[field as keyof typeof formData]
264-
return typeof value === 'string' && value.trim() !== ''
265-
})
266-
267-
const providerType = formData.providerType || 'oidc'
268-
269-
if (providerType === 'oidc') {
270-
return (
271-
hasRequiredFields &&
272-
formData.clientId.trim() !== '' &&
273-
formData.clientSecret.trim() !== '' &&
274-
formData.scopes.trim() !== ''
275-
)
276-
}
277-
if (providerType === 'saml') {
278-
return hasRequiredFields && formData.entryPoint.trim() !== '' && formData.cert.trim() !== ''
279-
}
280-
281-
return false
282-
}
283-
284256
const handleSubmit = async (e?: React.FormEvent) => {
285257
e?.preventDefault()
286258

@@ -359,9 +331,7 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) {
359331

360332
const handleInputChange = (field: keyof typeof formData, value: string | boolean) => {
361333
const next = { ...formData, [field]: value }
362-
// Claim names are protocol-specific — OIDC's `email` means nothing to a SAML
363-
// IdP — so carrying an override across a protocol switch would save a mapping
364-
// that cannot resolve. Clear them and fall back to the new protocol's defaults.
334+
// Claim names are protocol-specific, so an override must not survive a switch.
365335
if (field === 'providerType') {
366336
next.mapId = ''
367337
next.mapEmail = ''
@@ -499,8 +469,6 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) {
499469
</p>
500470
</SettingRow>
501471

502-
{/* Admins land here after saving, so this view has to carry the same
503-
two values an IdP needs as the form does. */}
504472
{existingProvider.providerType === 'saml' && (
505473
<SettingRow label='SP Entity ID'>
506474
<ChipCopyInput value={getBaseUrl()} copyLabel='Copy entity ID' />
@@ -554,7 +522,10 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) {
554522
...saveDiscardActions({
555523
dirty: hasChanges,
556524
saving: configureSSOMutation.isPending,
557-
saveDisabled: hasAnyErrors(errors) || !isFormValid(),
525+
// Deliberately not disabled on validation errors: showErrors is only
526+
// set by handleSubmit, so a disabled Save left the admin with a greyed
527+
// out button and no message. Clicking now reveals what is wrong.
528+
saveDisabled: false,
558529
saveLabel: isEditing ? 'Update' : 'Save',
559530
savingLabel: isEditing ? 'Updating...' : 'Saving...',
560531
onSave: () => void handleSubmit(),
@@ -908,10 +879,7 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) {
908879
onChange={(value: string) =>
909880
handleInputChange('identifierFormat', value)
910881
}
911-
options={SAML_NAMEID_FORMATS.map((a) => ({
912-
label: a.label,
913-
value: a.value,
914-
}))}
882+
options={[...SAML_NAMEID_FORMATS]}
915883
placeholder='Provider default'
916884
/>
917885
</SettingRow>
@@ -942,10 +910,7 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) {
942910
</p>
943911
</SettingRow>
944912

945-
{/* SAML IdP admins are typically handed vendor metadata; Sim does not
946-
publish a metadata document, so surface the two values that document
947-
would carry. Sim's SP entity ID is its base URL — the same value the
948-
register route embeds in the generated SP metadata. */}
913+
{/* Sim publishes no SP metadata document; these are the values it would carry. */}
949914
{isSaml && (
950915
<SettingRow label='SP Entity ID'>
951916
<ChipCopyInput value={getBaseUrl()} copyLabel='Copy entity ID' />
@@ -956,10 +921,6 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) {
956921
</SettingRow>
957922
)}
958923

959-
{/* Identity providers vary in which claim carries each value — Entra,
960-
for instance, can send the address as `upn` rather than `email`.
961-
Leaving a field blank uses the protocol default shown as its
962-
placeholder, so the common case needs no input at all. */}
963924
<div className='flex flex-col gap-2'>
964925
<Button
965926
type='button'

apps/sim/ee/sso/components/verified-domains-section.tsx

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,14 @@ export function VerifiedDomainsSection({ organizationId }: VerifiedDomainsSectio
142142
<ChipInput
143143
value={newDomain}
144144
onChange={(event) => setNewDomain(event.target.value)}
145+
onKeyDown={(event) => {
146+
// This section renders inside the SSO provider <form>, so a bare
147+
// Enter would submit that form instead of adding the domain.
148+
if (event.key === 'Enter') {
149+
event.preventDefault()
150+
void handleAdd()
151+
}
152+
}}
145153
placeholder='acme.com'
146154
className='min-w-0 flex-1'
147155
/>

apps/sim/lib/auth/auth.ts

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1113,10 +1113,14 @@ export const auth = betterAuth({
11131113
*
11141114
* Sim does not use Better Auth's own DNS challenge endpoints: ownership is
11151115
* proven by Sim's `sso_domain` flow before a provider can be registered,
1116-
* and the register route mirrors that decision onto this flag. Linking is
1117-
* still constrained to emails whose domain matches the provider's domain
1118-
* (`validateEmailDomain`), so this grants no trust beyond the verified
1119-
* domain itself.
1116+
* and the register route mirrors that decision onto this flag.
1117+
*
1118+
* This path is constrained to emails whose domain matches the provider's
1119+
* (`validateEmailDomain`). It is NOT the only path: `link-account.mjs`
1120+
* blocks on `!isTrustedProvider && !userInfo.emailVerified`, so an IdP
1121+
* that asserts `email_verified` links regardless of domain — see the
1122+
* note on `trustEmailVerified` above. This flag narrows nothing on its
1123+
* own; it exists so linking survives IdPs that omit the claim.
11201124
*/
11211125
domainVerification: { enabled: true },
11221126
organizationProvisioning: {

packages/db/migrations/0284_sso_provider_domain_verified.sql

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,20 @@ ALTER TABLE "sso_provider" ADD COLUMN IF NOT EXISTS "domain_verified" boolean DE
3636

3737
COMMIT;--> statement-breakpoint
3838

39+
-- `lock_timeout = 0` for the concurrent builds, per the convention in
40+
-- packages/db/scripts/migrate.ts. CREATE INDEX CONCURRENTLY waits on every
41+
-- concurrent write transaction in the database, not just ones touching this
42+
-- table, so the session's 5s DDL timeout would cancel the build (55P03) and
43+
-- strand an INVALID index that the IF NOT EXISTS below would then skip forever.
44+
SET lock_timeout = 0;--> statement-breakpoint
45+
46+
-- Clear any INVALID index left by a previously cancelled build, so a replay
47+
-- rebuilds it instead of skipping it.
48+
DROP INDEX CONCURRENTLY IF EXISTS "sso_provider_provider_id_unique";--> statement-breakpoint
49+
3950
-- Build the unique index before dropping the old plain one, so provider_id is never unindexed.
4051
CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS "sso_provider_provider_id_unique" ON "sso_provider" USING btree ("provider_id");--> statement-breakpoint
4152

42-
DROP INDEX CONCURRENTLY IF EXISTS "sso_provider_provider_id_idx";
53+
DROP INDEX CONCURRENTLY IF EXISTS "sso_provider_provider_id_idx";--> statement-breakpoint
54+
55+
SET lock_timeout = '5s';

0 commit comments

Comments
 (0)