Skip to content

Commit 2ca3f9e

Browse files
waleedlatif1waleedlatif1
committed
fix(sso): require a session to list SSO providers
The unauthenticated branch of GET /api/auth/sso/providers returned the domain of every configured provider. Sign-in resolves one address at a time through /api/auth/sso/resolve, so the list now answers 401 without a session. Resolve drops the unused providerType field.
1 parent c448b87 commit 2ca3f9e

6 files changed

Lines changed: 167 additions & 120 deletions

File tree

‎apps/sim/app/api/auth/sso/providers/route.test.ts‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import {
5+
createMockRequest,
6+
dbChainMock,
7+
dbChainMockFns,
8+
queueTableRows,
9+
resetDbChainMock,
10+
schemaMock,
11+
} from '@sim/testing'
12+
import { beforeEach, describe, expect, it, vi } from 'vitest'
13+
14+
const { mockGetSession } = vi.hoisted(() => ({ mockGetSession: vi.fn() }))
15+
16+
vi.mock('@sim/db', () => ({ ...dbChainMock, ...schemaMock }))
17+
vi.mock('@/lib/auth', () => ({ getSession: mockGetSession }))
18+
19+
import { GET } from '@/app/api/auth/sso/providers/route'
20+
21+
const providerRow = {
22+
id: 'row-1',
23+
providerId: 'acme-okta',
24+
domain: 'acme.com',
25+
issuer: 'https://acme.okta.test',
26+
oidcConfig: JSON.stringify({ clientId: 'client', clientSecret: 'a-long-client-secret-wxyz' }),
27+
samlConfig: null,
28+
userId: 'user-1',
29+
organizationId: 'org-1',
30+
jitProvisioningEnabled: true,
31+
domainVerified: true,
32+
domainKey: 'acme.com',
33+
isNamedPrimary: false,
34+
}
35+
36+
describe('GET /api/auth/sso/providers', () => {
37+
beforeEach(() => {
38+
vi.clearAllMocks()
39+
resetDbChainMock()
40+
mockGetSession.mockResolvedValue({ user: { id: 'user-1' } })
41+
})
42+
43+
it('refuses a caller without a session before reading any provider', async () => {
44+
mockGetSession.mockResolvedValue(null)
45+
const res = await GET(createMockRequest('GET'))
46+
expect(res.status).toBe(401)
47+
expect(dbChainMockFns.select).not.toHaveBeenCalled()
48+
})
49+
50+
it('lists only the providers the caller registered when no organization is named', async () => {
51+
queueTableRows(schemaMock.ssoProvider, [providerRow])
52+
const res = await GET(createMockRequest('GET'))
53+
expect(res.status).toBe(200)
54+
const { providers } = await res.json()
55+
expect(providers).toHaveLength(1)
56+
expect(providers[0]).toMatchObject({ providerId: 'acme-okta', providerType: 'oidc' })
57+
expect(JSON.parse(providers[0].oidcConfig)).toMatchObject({ clientSecretHint: 'wxyz' })
58+
expect(providers[0].oidcConfig).not.toContain('a-long-client-secret')
59+
const condition = JSON.stringify(dbChainMockFns.where.mock.calls[0][0])
60+
expect(condition).toContain('user-1')
61+
})
62+
63+
it('refuses an organization the caller does not administer', async () => {
64+
queueTableRows(schemaMock.member, [{ organizationId: 'org-1', role: 'member' }])
65+
const res = await GET(
66+
createMockRequest(
67+
'GET',
68+
undefined,
69+
{},
70+
'http://localhost/api/auth/sso/providers?organizationId=org-1'
71+
)
72+
)
73+
expect(res.status).toBe(403)
74+
})
75+
})

‎apps/sim/app/api/auth/sso/providers/route.ts‎

Lines changed: 65 additions & 81 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import { listSsoProvidersContract } from '@/lib/api/contracts/auth'
1111
import { parseRequest } from '@/lib/api/server'
1212
import { getSession } from '@/lib/auth'
1313
import { markSignInProviders } from '@/lib/auth/sso/primary-provider'
14-
import { enforceIpRateLimit } from '@/lib/core/rate-limiter'
1514
import { REDACTED_MARKER } from '@/lib/core/security/redaction'
1615
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
1716

@@ -32,102 +31,87 @@ function buildClientSecretHint(clientSecret: unknown): string | null {
3231
return clientSecret.slice(-4)
3332
}
3433

34+
/**
35+
* Lists the identity providers the caller administers: an organization's when an
36+
* owner or admin names it, otherwise the ones the caller registered.
37+
*
38+
* Signed-in only. Sign-in resolves one address at a time through
39+
* `/api/auth/sso/resolve`; nothing needs every configured domain, and listing
40+
* them would publish which organizations use SSO.
41+
*/
3542
export const GET = withRouteHandler(async (request: NextRequest) => {
3643
try {
3744
const session = await getSession()
3845
if (!session?.user?.id) {
39-
const rateLimited = await enforceIpRateLimit('sso-providers', request, {
40-
maxTokens: 20,
41-
refillRate: 20,
42-
refillIntervalMs: 60_000,
43-
})
44-
if (rateLimited) return rateLimited
46+
return NextResponse.json({ error: 'Unauthorized' }, { status: 401 })
4547
}
4648
const parsed = await parseRequest(listSsoProvidersContract, request, {})
4749
if (!parsed.success) return parsed.response
4850
const { organizationId } = parsed.data.query
51+
const userId = session.user.id
4952

50-
let providers
51-
if (session?.user?.id) {
52-
const userId = session.user.id
53-
54-
let verifiedOrganizationId: string | null = null
55-
if (organizationId) {
56-
const [membership] = await db
57-
.select({ organizationId: member.organizationId, role: member.role })
58-
.from(member)
59-
.where(and(eq(member.userId, userId), eq(member.organizationId, organizationId)))
60-
.limit(1)
61-
if (!membership) {
62-
return NextResponse.json({ error: 'Forbidden' }, { status: 403 })
63-
}
64-
if (membership.role !== 'owner' && membership.role !== 'admin') {
65-
return NextResponse.json({ error: 'Forbidden' }, { status: 403 })
66-
}
67-
verifiedOrganizationId = membership.organizationId
53+
let verifiedOrganizationId: string | null = null
54+
if (organizationId) {
55+
const [membership] = await db
56+
.select({ organizationId: member.organizationId, role: member.role })
57+
.from(member)
58+
.where(and(eq(member.userId, userId), eq(member.organizationId, organizationId)))
59+
.limit(1)
60+
if (!membership) {
61+
return NextResponse.json({ error: 'Forbidden' }, { status: 403 })
6862
}
63+
if (membership.role !== 'owner' && membership.role !== 'admin') {
64+
return NextResponse.json({ error: 'Forbidden' }, { status: 403 })
65+
}
66+
verifiedOrganizationId = membership.organizationId
67+
}
6968

70-
const whereClause = verifiedOrganizationId
71-
? eq(ssoProvider.organizationId, verifiedOrganizationId)
72-
: eq(ssoProvider.userId, userId)
73-
74-
const results = await db
75-
.select({
76-
id: ssoProvider.id,
77-
providerId: ssoProvider.providerId,
78-
domain: ssoProvider.domain,
79-
issuer: ssoProvider.issuer,
80-
oidcConfig: ssoProvider.oidcConfig,
81-
samlConfig: ssoProvider.samlConfig,
82-
userId: ssoProvider.userId,
83-
organizationId: ssoProvider.organizationId,
84-
jitProvisioningEnabled: ssoProvider.jitProvisioningEnabled,
85-
domainVerified: ssoProvider.domainVerified,
86-
domainKey: ssoProviderDomainKey,
87-
isNamedPrimary,
88-
})
89-
.from(ssoProvider)
90-
.leftJoin(ssoDomain, verifiedDomainOfProvider)
91-
.where(whereClause)
92-
.orderBy(asc(ssoProvider.providerId))
69+
const whereClause = verifiedOrganizationId
70+
? eq(ssoProvider.organizationId, verifiedOrganizationId)
71+
: eq(ssoProvider.userId, userId)
9372

94-
providers = markSignInProviders(results).map((provider) => {
95-
let oidcConfig = provider.oidcConfig
96-
if (oidcConfig) {
97-
try {
98-
const parsed = JSON.parse(oidcConfig)
99-
const hint = buildClientSecretHint(parsed.clientSecret)
100-
parsed.clientSecret = REDACTED_MARKER
101-
if (hint) parsed.clientSecretHint = hint
102-
oidcConfig = JSON.stringify(parsed)
103-
} catch {
104-
oidcConfig = null
105-
}
106-
}
107-
return {
108-
...provider,
109-
oidcConfig,
110-
providerType: (provider.samlConfig ? 'saml' : 'oidc') as 'oidc' | 'saml',
111-
}
73+
const results = await db
74+
.select({
75+
id: ssoProvider.id,
76+
providerId: ssoProvider.providerId,
77+
domain: ssoProvider.domain,
78+
issuer: ssoProvider.issuer,
79+
oidcConfig: ssoProvider.oidcConfig,
80+
samlConfig: ssoProvider.samlConfig,
81+
userId: ssoProvider.userId,
82+
organizationId: ssoProvider.organizationId,
83+
jitProvisioningEnabled: ssoProvider.jitProvisioningEnabled,
84+
domainVerified: ssoProvider.domainVerified,
85+
domainKey: ssoProviderDomainKey,
86+
isNamedPrimary,
11287
})
113-
} else {
114-
const results = await db
115-
.select({
116-
domain: ssoProvider.domain,
117-
})
118-
.from(ssoProvider)
119-
120-
providers = results.map((provider) => ({
121-
domain: provider.domain,
122-
}))
123-
}
88+
.from(ssoProvider)
89+
.leftJoin(ssoDomain, verifiedDomainOfProvider)
90+
.where(whereClause)
91+
.orderBy(asc(ssoProvider.providerId))
12492

125-
logger.info('Fetched SSO providers', {
126-
userId: session?.user?.id,
127-
authenticated: !!session?.user?.id,
128-
providerCount: providers.length,
93+
const providers = markSignInProviders(results).map((provider) => {
94+
let oidcConfig = provider.oidcConfig
95+
if (oidcConfig) {
96+
try {
97+
const parsed = JSON.parse(oidcConfig)
98+
const hint = buildClientSecretHint(parsed.clientSecret)
99+
parsed.clientSecret = REDACTED_MARKER
100+
if (hint) parsed.clientSecretHint = hint
101+
oidcConfig = JSON.stringify(parsed)
102+
} catch {
103+
oidcConfig = null
104+
}
105+
}
106+
return {
107+
...provider,
108+
oidcConfig,
109+
providerType: (provider.samlConfig ? 'saml' : 'oidc') as 'oidc' | 'saml',
110+
}
129111
})
130112

113+
logger.info('Fetched SSO providers', { userId, providerCount: providers.length })
114+
131115
return NextResponse.json({ providers })
132116
} catch (error) {
133117
logger.error('Failed to fetch SSO providers', { error })

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

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -26,17 +26,17 @@ describe('POST /api/auth/sso/resolve', () => {
2626
})
2727

2828
it('names the provider that serves the address domain', async () => {
29-
queueTableRows(schemaMock.ssoProvider, [{ providerId: 'acme-okta', samlConfig: null }])
29+
queueTableRows(schemaMock.ssoProvider, [{ providerId: 'acme-okta' }])
3030
const res = await POST(createMockRequest('POST', { email: 'Ada@Acme.com' }))
3131
expect(res.status).toBe(200)
32-
await expect(res.json()).resolves.toEqual({ providerId: 'acme-okta', providerType: 'oidc' })
32+
await expect(res.json()).resolves.toEqual({ providerId: 'acme-okta' })
3333
const [condition] = dbChainMockFns.where.mock.calls[0]
3434
expect(JSON.stringify(condition)).toContain('acme.com')
3535
expect(JSON.stringify(condition)).toContain('domainVerified')
3636
})
3737

3838
it('prefers the provider the verified domain names, then provider id', async () => {
39-
queueTableRows(schemaMock.ssoProvider, [{ providerId: 'acme-okta', samlConfig: null }])
39+
queueTableRows(schemaMock.ssoProvider, [{ providerId: 'acme-okta' }])
4040
await POST(createMockRequest('POST', { email: 'ada@acme.com' }))
4141
expect(dbChainMockFns.leftJoin).toHaveBeenCalledWith(schemaMock.ssoDomain, expect.anything())
4242
const [named, byId] = dbChainMockFns.orderBy.mock.calls[0]
@@ -46,7 +46,7 @@ describe('POST /api/auth/sso/resolve', () => {
4646
})
4747

4848
it('honors a test link only for a provider that serves the address domain', async () => {
49-
queueTableRows(schemaMock.ssoProvider, [{ providerId: 'acme-entra', samlConfig: null }])
49+
queueTableRows(schemaMock.ssoProvider, [{ providerId: 'acme-entra' }])
5050
const res = await POST(
5151
createMockRequest('POST', { email: 'ada@acme.com', providerId: 'acme-entra' })
5252
)
@@ -65,12 +65,6 @@ describe('POST /api/auth/sso/resolve', () => {
6565
expect(res.status).toBe(404)
6666
})
6767

68-
it('reports SAML providers as such', async () => {
69-
queueTableRows(schemaMock.ssoProvider, [{ providerId: 'acme-adfs', samlConfig: '{}' }])
70-
const res = await POST(createMockRequest('POST', { email: 'ada@acme.com' }))
71-
await expect(res.json()).resolves.toMatchObject({ providerType: 'saml' })
72-
})
73-
7468
it('answers 404 when no provider serves the domain', async () => {
7569
queueTableRows(schemaMock.ssoProvider, [])
7670
const res = await POST(createMockRequest('POST', { email: 'ada@nowhere.test' }))

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

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,11 @@ import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
1616
* Names the identity provider that signs in an email address.
1717
*
1818
* Unauthenticated by nature, like the sign-in page that calls it, and admitted
19-
* per address. It discloses nothing the public provider list does not: which
20-
* domains have SSO, and the provider id that already appears in the callback
21-
* URL. Only a provider whose domain is verified is named: an unverified claim
22-
* has no authority over the address, and sending someone to its IdP would fail
23-
* at the callback anyway.
19+
* per address. It answers for the one domain asked about, and names only the
20+
* provider id that the sign-in redirect and callback URL expose anyway; there is
21+
* deliberately no way to list every domain with SSO. Only a provider whose domain
22+
* is verified is named: an unverified claim has no authority over the address,
23+
* and sending someone to its IdP would fail at the callback anyway.
2424
*
2525
* The provider the domain names as primary wins, then the first verified by id,
2626
* which is also the only one when a domain has a single provider. A test sign-in
@@ -46,7 +46,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
4646

4747
const requestedProviderId = parsed.data.body.providerId
4848
const [provider] = await db
49-
.select({ providerId: ssoProvider.providerId, samlConfig: ssoProvider.samlConfig })
49+
.select({ providerId: ssoProvider.providerId })
5050
.from(ssoProvider)
5151
.leftJoin(ssoDomain, verifiedDomainOfProvider)
5252
.where(
@@ -68,8 +68,5 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
6868
)
6969
}
7070

71-
return NextResponse.json({
72-
providerId: provider.providerId,
73-
providerType: provider.samlConfig ? 'saml' : 'oidc',
74-
})
71+
return NextResponse.json({ providerId: provider.providerId })
7572
})

‎apps/sim/ee/sso/components/sso-form.test.tsx‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,7 @@ describe('SSOForm sign-in errors', () => {
144144
mockSsoSignIn.mockReset()
145145
mockUseSearchParams.mockReset()
146146
mockRequestJson.mockReset()
147-
mockRequestJson.mockResolvedValue({ providerId: 'example-okta', providerType: 'oidc' })
147+
mockRequestJson.mockResolvedValue({ providerId: 'example-okta' })
148148
;(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true
149149
container = document.createElement('div')
150150
document.body.appendChild(container)
@@ -188,7 +188,7 @@ describe('SSOForm sign-in errors', () => {
188188
})
189189

190190
it('signs in through the provider a test link names, and returns to that link on failure', async () => {
191-
mockRequestJson.mockResolvedValue({ providerId: 'example-okta', providerType: 'oidc' })
191+
mockRequestJson.mockResolvedValue({ providerId: 'example-okta' })
192192
mockSsoSignIn.mockResolvedValue({ data: { url: 'https://idp.example.com' }, error: null })
193193
renderInteractive('email=user%40example.com&provider=example-okta')
194194

‎apps/sim/lib/api/contracts/auth.ts‎

Lines changed: 14 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -95,22 +95,22 @@ export const ssoRegistrationContract = defineRouteContract({
9595
})
9696

9797
const ssoProviderListEntrySchema = z.object({
98-
id: z.string().optional(),
99-
providerId: z.string().optional(),
100-
domain: z.string().nullable(),
101-
issuer: z.string().nullable().optional(),
102-
oidcConfig: z.string().nullable().optional(),
103-
samlConfig: z.string().nullable().optional(),
104-
userId: z.string().nullable().optional(),
105-
organizationId: z.string().nullable().optional(),
106-
jitProvisioningEnabled: z.boolean().optional(),
98+
id: z.string(),
99+
providerId: z.string(),
100+
domain: z.string(),
101+
issuer: z.string(),
102+
oidcConfig: z.string().nullable(),
103+
samlConfig: z.string().nullable(),
104+
userId: z.string(),
105+
organizationId: z.string().nullable(),
106+
jitProvisioningEnabled: z.boolean(),
107107
/** The domain as sign-in compares it: trimmed, lower-cased, a leading `*.` dropped. Providers sharing it share a primary. */
108-
domainKey: z.string().optional(),
108+
domainKey: z.string(),
109109
/** Whether this provider's domain is verified, so it can sign people in. */
110-
domainVerified: z.boolean().optional(),
110+
domainVerified: z.boolean(),
111111
/** Whether sign-in for this provider's domain goes through it. */
112-
isPrimary: z.boolean().optional(),
113-
providerType: z.enum(['oidc', 'saml']).optional(),
112+
isPrimary: z.boolean(),
113+
providerType: z.enum(['oidc', 'saml']),
114114
})
115115

116116
export const listSsoProvidersContract = defineRouteContract({
@@ -167,10 +167,7 @@ export const resolveSsoProviderContract = defineRouteContract({
167167
}),
168168
response: {
169169
mode: 'json',
170-
schema: z.object({
171-
providerId: z.string(),
172-
providerType: z.enum(['oidc', 'saml']),
173-
}),
170+
schema: z.object({ providerId: z.string() }),
174171
},
175172
})
176173

0 commit comments

Comments
 (0)