Skip to content

Commit b36efc0

Browse files
committed
fix(sso): make one provider per domain hold at the database and resolve only verified providers
- Partial unique index on sso_provider (organization_id, normalized domain), built concurrently after a duplicate pre-check; a lost race maps to the same 409 as the pre-check - Sign-in resolution returns only providers whose domain is verified - Settings controls follow the organization owner/admin model the server enforces, not the creator
1 parent 569c6a6 commit b36efc0

9 files changed

Lines changed: 25965 additions & 17 deletions

File tree

apps/sim/app/api/auth/sso/register/route.test.ts

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -229,6 +229,22 @@ describe('POST /api/auth/sso/register', () => {
229229
expect(mockRegisterSSOProvider).not.toHaveBeenCalled()
230230
})
231231

232+
it('turns a lost race on the domain index into the same 409 as the pre-check', async () => {
233+
queueMembers([{ organizationId: 'org1', role: 'owner' }])
234+
queueProviders([])
235+
mockRegisterSSOProvider.mockRejectedValue(
236+
Object.assign(new Error('duplicate key value violates unique constraint'), {
237+
code: '23505',
238+
constraint_name: 'sso_provider_org_domain_unique',
239+
})
240+
)
241+
const res = await POST(request({ ...OIDC_BODY, orgId: 'org1' }))
242+
const json = await res.json()
243+
expect(res.status).toBe(409)
244+
expect(json.code).toBe('SSO_DOMAIN_ALREADY_ROUTED')
245+
expect(json.error).toContain('acme.com')
246+
})
247+
232248
it('lets the organization add a provider for a different verified domain', async () => {
233249
queueMembers([{ organizationId: 'org1', role: 'owner' }])
234250
queueProviders([])

apps/sim/app/api/auth/sso/register/route.ts

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { db, member, ssoDomain, ssoProvider } from '@sim/db'
22
import { createLogger } from '@sim/logger'
3-
import { getErrorMessage } from '@sim/utils/errors'
3+
import { getErrorMessage, getPostgresConstraintName } from '@sim/utils/errors'
44
import { normalizeSSODomain } from '@sim/utils/sso-domain'
55
import { and, eq, isNull, sql } from 'drizzle-orm'
66
import { type NextRequest, NextResponse } from 'next/server'
@@ -81,6 +81,7 @@ async function fetchOIDCDiscoveryDocument(discoveryUrl: string): Promise<Discove
8181
}
8282

8383
export const POST = withRouteHandler(async (request: NextRequest) => {
84+
let requestedDomain: string | null = null
8485
try {
8586
if (!isSsoEnabled) {
8687
return NextResponse.json({ error: 'SSO is not enabled' }, { status: 400 })
@@ -130,6 +131,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
130131
}
131132

132133
const domain = normalizeSSODomain(body.domain)
134+
requestedDomain = domain
133135
if (!domain) {
134136
return NextResponse.json(
135137
{ error: 'Enter a valid domain, for example acme.com' },
@@ -828,6 +830,20 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
828830
errorDetails: JSON.stringify(error),
829831
})
830832

833+
/**
834+
* The one-provider-per-domain index is the authority when two registrations
835+
* race past the read above; its violation is the same refusal, not a fault.
836+
*/
837+
if (getPostgresConstraintName(error) === 'sso_provider_org_domain_unique') {
838+
return NextResponse.json(
839+
{
840+
error: `${requestedDomain ?? 'This domain'} already signs in through another provider of this organization. Edit that provider, or use a different verified domain.`,
841+
code: 'SSO_DOMAIN_ALREADY_ROUTED',
842+
},
843+
{ status: 409 }
844+
)
845+
}
846+
831847
// Surface Better Auth's own APIError (e.g. a 409 when identity fields change
832848
// while linked accounts exist, or a 404) with its status and message instead
833849
// of a generic 500, so the client shows an actionable error.

apps/sim/app/api/auth/sso/resolve/route.test.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,8 @@ describe('POST /api/auth/sso/resolve', () => {
3131
expect(res.status).toBe(200)
3232
await expect(res.json()).resolves.toEqual({ providerId: 'acme-okta', providerType: 'oidc' })
3333
const [condition] = dbChainMockFns.where.mock.calls[0]
34-
expect(condition?.values).toContain('acme.com')
34+
expect(JSON.stringify(condition)).toContain('acme.com')
35+
expect(JSON.stringify(condition)).toContain('domainVerified')
3536
})
3637

3738
it('reports SAML providers as such', async () => {

apps/sim/app/api/auth/sso/resolve/route.ts

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { db, ssoProvider } from '@sim/db'
22
import { normalizeSSODomain } from '@sim/utils/sso-domain'
3-
import { asc, desc, sql } from 'drizzle-orm'
3+
import { and, asc, eq, sql } from 'drizzle-orm'
44
import { type NextRequest, NextResponse } from 'next/server'
55
import { resolveSsoProviderContract } from '@/lib/api/contracts/auth'
66
import { parseRequest } from '@/lib/api/server'
@@ -13,8 +13,9 @@ import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
1313
* Unauthenticated by nature, like the sign-in page that calls it, and admitted
1414
* per address. It discloses nothing the public provider list does not: which
1515
* domains have SSO, and the provider id that already appears in the callback
16-
* URL. A verified domain wins over a stale unverified claim on the same domain,
17-
* and ties break on provider id so the answer is stable.
16+
* URL. Only a provider whose domain is verified is named: an unverified claim
17+
* has no authority over the address, and sending someone to its IdP would fail
18+
* at the callback anyway. Ties break on provider id so the answer is stable.
1819
*/
1920
export const POST = withRouteHandler(async (request: NextRequest) => {
2021
const rateLimited = await enforceIpRateLimit('sso-resolve', request, {
@@ -35,8 +36,13 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
3536
const [provider] = await db
3637
.select({ providerId: ssoProvider.providerId, samlConfig: ssoProvider.samlConfig })
3738
.from(ssoProvider)
38-
.where(sql`lower(regexp_replace(btrim(${ssoProvider.domain}), '^\\*\\.', '')) = ${domain}`)
39-
.orderBy(desc(ssoProvider.domainVerified), asc(ssoProvider.providerId))
39+
.where(
40+
and(
41+
eq(ssoProvider.domainVerified, true),
42+
sql`lower(regexp_replace(btrim(${ssoProvider.domain}), '^\\*\\.', '')) = ${domain}`
43+
)
44+
)
45+
.orderBy(asc(ssoProvider.providerId))
4046
.limit(1)
4147
if (!provider) {
4248
return NextResponse.json(

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

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import { useState } from 'react'
44
import { ChipConfirmModal, ChipModalTabs, toast } from '@sim/emcn'
55
import { getErrorMessage } from '@sim/utils/errors'
66
import { useQueryStates } from 'nuqs'
7-
import { useSession } from '@/lib/auth/auth-client'
87
import { isEnterprise } from '@/lib/billing/plan-helpers'
98
import { useDeploymentShape } from '@/lib/core/config/deployment-shape'
109
import {
@@ -45,7 +44,6 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) {
4544
ssoSettingsParsers,
4645
ssoSettingsUrlKeys
4746
)
48-
const { data: session } = useSession()
4947
const { billingEnabled, features } = useDeploymentShape()
5048
const billing = useOrganizationBilling(organizationId)
5149
const providers = useSSOProviders({ organizationId })
@@ -79,10 +77,6 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) {
7977
toast.error(getErrorMessage(error, 'Failed to delete identity provider'))
8078
}
8179
}
82-
const canManageProvider =
83-
billingEnabled ||
84-
providerList.length === 0 ||
85-
providerList.some((entry) => entry.userId === session?.user?.id)
8680

8781
if (billingEnabled && billing.isLoading) {
8882
return <SettingsEmptyState variant='inline'>Loading sign-in settings...</SettingsEmptyState>
@@ -129,10 +123,6 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) {
129123
isRetrying={providers.isFetching}
130124
onRetry={() => void providers.refetch()}
131125
/>
132-
) : !canManageProvider ? (
133-
<SettingsEmptyState variant='inline'>
134-
Only the user who configured SSO can manage these settings.
135-
</SettingsEmptyState>
136126
) : signInView === 'list' ? (
137127
<SsoProviderList
138128
providers={providerList}
Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
-- An organization may run several identity providers, but sign-in routes by email domain, so
2+
-- each of its domains must name exactly one provider. The registration route checks this before
3+
-- writing, but that check is a plain read: two concurrent registrations with different provider
4+
-- ids can both see no sibling and both land. This index makes the invariant hold at the database.
5+
--
6+
-- Keyed on the same expression the verify and resolve paths compare domains with, so a legacy
7+
-- leading `*.` or stray case cannot slip a second provider onto a domain already routed.
8+
--
9+
-- Duplicates must be resolved first. Failing here, inside the transaction, avoids letting the
10+
-- CONCURRENT build fail afterwards and strand an INVALID index that IF NOT EXISTS would skip
11+
-- forever. Which row survives is a judgement call, so this reports the ids and stops.
12+
DO $$
13+
DECLARE duplicate_provider_ids text;
14+
BEGIN
15+
SELECT string_agg(provider_id, ', ')
16+
INTO duplicate_provider_ids
17+
FROM "sso_provider"
18+
WHERE "organization_id" IS NOT NULL
19+
AND ("organization_id", lower(regexp_replace(btrim("domain"), '^\*\.', ''))) IN (
20+
SELECT "organization_id", lower(regexp_replace(btrim("domain"), '^\*\.', ''))
21+
FROM "sso_provider"
22+
WHERE "organization_id" IS NOT NULL
23+
GROUP BY 1, 2
24+
HAVING count(*) > 1
25+
);
26+
IF duplicate_provider_ids IS NOT NULL THEN
27+
RAISE EXCEPTION
28+
'sso_provider has several providers on one organization domain: %. Keep one provider per (organization, domain) and re-run.',
29+
duplicate_provider_ids;
30+
END IF;
31+
END $$;--> statement-breakpoint
32+
33+
COMMIT;--> statement-breakpoint
34+
35+
-- `lock_timeout = 0` for the concurrent build, per packages/db/scripts/migrate.ts.
36+
-- CREATE INDEX CONCURRENTLY waits on every concurrent write in the database, not just
37+
-- this table, so the session's 5s DDL timeout would cancel it (55P03) and strand an
38+
-- INVALID index that the IF NOT EXISTS below would skip forever.
39+
SET lock_timeout = 0;--> statement-breakpoint
40+
41+
-- Clear any INVALID index left by a previously cancelled build, so a replay
42+
-- rebuilds it instead of skipping it.
43+
DROP INDEX CONCURRENTLY IF EXISTS "sso_provider_org_domain_unique";--> statement-breakpoint
44+
45+
CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS "sso_provider_org_domain_unique" ON "sso_provider" USING btree ("organization_id", lower(regexp_replace(btrim("domain"), '^\*\.', ''))) WHERE "sso_provider"."organization_id" is not null;--> statement-breakpoint
46+
47+
SET lock_timeout = '5s';

0 commit comments

Comments
 (0)