Skip to content

Commit 718c4e7

Browse files
committed
fix(auth): align SSO admission locks and reject stale provider links
1 parent 5efdea5 commit 718c4e7

11 files changed

Lines changed: 230 additions & 20 deletions

‎apps/docs/openapi-v2-resources.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7954,7 +7954,7 @@
79547954
"get": {
79557955
"operationId": "listCredentialMembers",
79567956
"summary": "List Credential Members",
7957-
"description": "List explicit credential grants, including revoked grants, and inherited workspace administrator access. Requires workspace read access. Credentials must be OAuth or service-account connections. Workspace API keys return `403`; use a personal API key or scoped OAuth token.\n\nOAuth scope: `api:read`.",
7957+
"description": "List explicit credential grants, including revoked grants, and inherited workspace administrator access. Requires workspace read access. Personal API keys and OAuth tokens can access OAuth or service-account credentials; sessions can also access workspace environment credentials. Workspace API keys return `403`; use a personal API key or scoped OAuth token.\n\nOAuth scope: `api:read`.",
79587958
"x-sim-operation": "credentials.members.list",
79597959
"x-oauth-scope": "api:read",
79607960
"tags": ["Credentials"],

‎apps/sim/lib/api/contracts/v2/openapi/credential-members.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ export const credentialMemberOpenApiRoutes = [
1818
applicationOperation: credentialOperations.listMembers,
1919
operationId: 'listCredentialMembers',
2020
summary: 'List Credential Members',
21-
description: `List explicit credential grants, including revoked grants, and inherited workspace administrator access. Requires workspace read access. Credentials must be OAuth or service-account connections. ${WORKSPACE_API_KEY_DENIED}`,
21+
description: `List explicit credential grants, including revoked grants, and inherited workspace administrator access. Requires workspace read access. Personal API keys and OAuth tokens can access OAuth or service-account credentials; sessions can also access workspace environment credentials. ${WORKSPACE_API_KEY_DENIED}`,
2222
tags: ['Credentials'],
2323
errors: RESOURCE_ERRORS,
2424
success: { description: 'List Credential Members result.', headers: RATE_LIMIT_HEADERS },

‎apps/sim/lib/api/mcp/generated/v2-operations.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1726,7 +1726,7 @@ export const V2_MCP_OPERATIONS = {
17261726
contract: v2ListCredentialMembersContract,
17271727
summary: 'List Credential Members',
17281728
description:
1729-
'List explicit credential grants, including revoked grants, and inherited workspace administrator access. Requires workspace read access. Credentials must be OAuth or service-account connections. Workspace API keys return `403`; use a personal API key or scoped OAuth token.\n\nOAuth scope: `api:read`.',
1729+
'List explicit credential grants, including revoked grants, and inherited workspace administrator access. Requires workspace read access. Personal API keys and OAuth tokens can access OAuth or service-account credentials; sessions can also access workspace environment credentials. Workspace API keys return `403`; use a personal API key or scoped OAuth token.\n\nOAuth scope: `api:read`.',
17301730
workspaceKeyUnsupported: true,
17311731
handler: () =>
17321732
import('@/app/api/v2/credentials/[credentialId]/members/route').then((route) => route.GET),

‎apps/sim/lib/auth/sim-auth-adapter.ts‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,8 @@ import { db } from '@sim/db'
22
import * as schema from '@sim/db/schema'
33
import type { BetterAuthOptions } from 'better-auth'
44
import { drizzleAdapter } from 'better-auth/adapters/drizzle'
5-
import { eq } from 'drizzle-orm'
5+
import { APIError } from 'better-auth/api'
6+
import { and, eq } from 'drizzle-orm'
67
import { runWithAuthDatabase } from '@/lib/auth/database-context'
78
import {
89
type AuthDatabase,
@@ -39,7 +40,20 @@ function createTransactionAdapter(
3940
.from(schema.ssoProvider)
4041
.where(eq(schema.ssoProvider.providerId, input.data.providerId))
4142
.limit(1)
42-
if (provider) await lockSsoProvider(tx, input.data.providerId)
43+
if (provider) {
44+
await lockSsoProvider(tx, input.data.providerId)
45+
const [current] = await tx
46+
.select({ id: schema.ssoProvider.id })
47+
.from(schema.ssoProvider)
48+
.where(
49+
and(
50+
eq(schema.ssoProvider.id, provider.id),
51+
eq(schema.ssoProvider.providerId, input.data.providerId)
52+
)
53+
)
54+
.limit(1)
55+
if (!current) throw new APIError('NOT_FOUND', { message: 'SSO provider not found' })
56+
}
4357
}
4458
return guarded.create(input)
4559
},

‎apps/sim/lib/auth/sso/application/admit-sso-user.integration.ts‎

Lines changed: 62 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { createDeferred } from '@sim/testing/helpers/deferred'
12
import { auditMock } from '@sim/testing/mocks/audit.mock'
23
import { envFlagsMock } from '@sim/testing/mocks/env-flags.mock'
34
import { generateId } from '@sim/utils/id'
@@ -18,13 +19,20 @@ vi.mock('@/lib/billing/organizations/seats', () => ({ reconcileOrganizationSeats
1819
vi.mock('@/lib/posthog/server', () => ({ captureServerEvent: vi.fn() }))
1920

2021
async function loadRuntime() {
21-
const [{ db }, schema, { eq, inArray }, { admitSsoUser }] = await Promise.all([
22+
const [
23+
{ db },
24+
schema,
25+
{ eq, inArray, sql },
26+
{ admitSsoUser },
27+
{ acquireOrganizationMutationLock },
28+
] = await Promise.all([
2229
import('@sim/db'),
2330
import('@sim/db/schema'),
2431
import('drizzle-orm'),
2532
import('@/lib/auth/sso/application/admit-sso-user'),
33+
import('@/lib/billing/organizations/membership'),
2634
])
27-
return { db, schema, eq, inArray, admitSsoUser }
35+
return { db, schema, eq, inArray, sql, admitSsoUser, acquireOrganizationMutationLock }
2836
}
2937

3038
describe('SSO admission with a hosted SCIM directory in PostgreSQL', () => {
@@ -133,6 +141,58 @@ describe('SSO admission with a hosted SCIM directory in PostgreSQL', () => {
133141
.where(eq(schema.member.userId, userId))
134142
}
135143

144+
it('allows a provider edit to commit while sign-in admission waits for organization mutation', async () => {
145+
const { db, schema, eq, sql } = runtime
146+
await db
147+
.update(schema.scimConnection)
148+
.set({ settings: { disableJit: false } })
149+
.where(eq(schema.scimConnection.id, connectionId))
150+
const held = createDeferred<number>()
151+
const release = createDeferred<void>()
152+
const pending: Promise<unknown>[] = []
153+
try {
154+
const mutation = db.transaction(async (tx) => {
155+
await runtime.acquireOrganizationMutationLock(tx, organizationId)
156+
const [connection] = await tx.execute<{ pid: number }>(sql`SELECT pg_backend_pid() AS pid`)
157+
held.resolve(connection.pid)
158+
await release.promise
159+
await tx
160+
.select({ id: schema.ssoProvider.id })
161+
.from(schema.ssoProvider)
162+
.where(eq(schema.ssoProvider.providerId, providerId))
163+
.for('update', { noWait: true })
164+
await tx
165+
.update(schema.ssoProvider)
166+
.set({ jitProvisioningEnabled: false })
167+
.where(eq(schema.ssoProvider.providerId, providerId))
168+
})
169+
pending.push(mutation)
170+
void mutation.catch((error: unknown) => held.reject(error))
171+
const blockerPid = await held.promise
172+
const admission = admit()
173+
pending.push(admission)
174+
void admission.catch(() => undefined)
175+
await vi.waitFor(
176+
async () => {
177+
const [waiting] = await db.execute<{ pid: number }>(sql`
178+
SELECT pid FROM pg_stat_activity
179+
WHERE datname = current_database() AND wait_event = 'advisory'
180+
AND ${blockerPid}::int = ANY(pg_blocking_pids(pid))
181+
`)
182+
expect(waiting).toBeDefined()
183+
},
184+
{ timeout: 5_000, interval: 25 }
185+
)
186+
release.resolve()
187+
await mutation
188+
await expect(admission).resolves.toEqual({ kind: 'provisioning-disabled', organizationId })
189+
expect(await membership()).toEqual([])
190+
} finally {
191+
release.resolve()
192+
await Promise.allSettled(pending)
193+
}
194+
})
195+
136196
it('honors disableJit without a global billing read creating fresh membership', async () => {
137197
await expect(admit()).resolves.toEqual({ kind: 'provisioning-disabled', organizationId })
138198
expect(await membership()).toEqual([])

‎apps/sim/lib/auth/sso/application/admit-sso-user.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@ function queueIdentity({
7171
accountLinked?: boolean
7272
email?: string
7373
} = {}) {
74+
queueTableRows(schemaMock.ssoProvider, [{ id: 'sso-1', organizationId }])
7475
queueTableRows(schemaMock.ssoProvider, [
7576
{
7677
id: 'sso-1',
@@ -152,7 +153,6 @@ describe('SSO JIT admission', () => {
152153
kind: 'denied',
153154
reason: 'provider-not-trusted',
154155
})
155-
expect(mockAcquireOrganizationUserMutationLocks).not.toHaveBeenCalled()
156156
expect(mockEnsureUserInOrganizationTx).not.toHaveBeenCalled()
157157
})
158158

‎apps/sim/lib/auth/sso/application/admit-sso-user.ts‎

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,20 @@ async function runAdmissionTransaction(
7474
providerId: string
7575
): Promise<SuccessfulAdmission> {
7676
return db.transaction(async (tx) => {
77+
const [scope] = await tx
78+
.select({ id: ssoProvider.id, organizationId: ssoProvider.organizationId })
79+
.from(ssoProvider)
80+
.where(eq(ssoProvider.providerId, providerId))
81+
.limit(1)
82+
if (!scope) {
83+
return { providerId, result: { kind: 'denied', reason: 'provider-not-found' } }
84+
}
85+
if (scope.organizationId) {
86+
await acquireOrganizationUserMutationLocks(tx, {
87+
userId,
88+
organizationIds: [scope.organizationId],
89+
})
90+
}
7791
const [provider] = await tx
7892
.select({
7993
id: ssoProvider.id,
@@ -83,11 +97,11 @@ async function runAdmissionTransaction(
8397
organizationId: ssoProvider.organizationId,
8498
})
8599
.from(ssoProvider)
86-
.where(eq(ssoProvider.providerId, providerId))
100+
.where(and(eq(ssoProvider.id, scope.id), eq(ssoProvider.providerId, providerId)))
87101
.limit(1)
88102
.for('share')
89103

90-
if (!provider) {
104+
if (!provider || provider.organizationId !== scope.organizationId) {
91105
return { providerId, result: { kind: 'denied', reason: 'provider-not-found' } }
92106
}
93107
if (!provider.domainVerified) {
@@ -127,11 +141,6 @@ async function runAdmissionTransaction(
127141
}
128142
}
129143

130-
await acquireOrganizationUserMutationLocks(tx, {
131-
userId,
132-
organizationIds: [provider.organizationId],
133-
})
134-
135144
/**
136145
* An organization whose directory is the only way in has said so on its
137146
* SCIM connection; a first sign-in must not create a membership the directory

‎apps/sim/lib/auth/sso/application/provider-settings.integration.ts‎

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { execFile } from 'node:child_process'
2+
import { Resolver } from 'node:dns/promises'
23
import { mkdtemp, rm, writeFile } from 'node:fs/promises'
34
import { createServer } from 'node:http'
45
import { tmpdir } from 'node:os'
@@ -748,6 +749,16 @@ describe('Organization SSO administration through API credentials', () => {
748749
.select()
749750
.from(schema.ssoDomain)
750751
.where(eq(schema.ssoDomain.organizationId, organizationId))
752+
const [pendingClaim] = await db
753+
.insert(schema.ssoDomain)
754+
.values({
755+
id: generateId(),
756+
organizationId,
757+
domain: `aaa-pending-${suffix}.test`,
758+
status: 'pending',
759+
verificationToken: generateId(),
760+
})
761+
.returning()
751762
const directory = await mkdtemp(resolve(tmpdir(), 'sim-sso-cli-'))
752763
const cliPath = resolve(process.cwd(), '../../packages/sim-cli/src/index.ts')
753764
const secret = 'fixture-secret+with-newline\n'
@@ -790,7 +801,7 @@ describe('Organization SSO administration through API credentials', () => {
790801
params: Promise.resolve({ organizationId, providerId }),
791802
})
792803
: await apiDomainVerification.POST(request, {
793-
params: Promise.resolve({ organizationId, domainId: claim.id }),
804+
params: Promise.resolve({ organizationId, domainId: pendingClaim.id }),
794805
})
795806
outgoing.statusCode = response.status
796807
response.headers.forEach((value, name) => outgoing.setHeader(name, value))
@@ -800,14 +811,22 @@ describe('Organization SSO administration through API credentials', () => {
800811
outgoing.end(JSON.stringify({ fixtureError: getErrorMessage(error) }))
801812
}
802813
})
814+
const dns = vi
815+
.spyOn(Resolver.prototype, 'resolveTxt')
816+
.mockImplementation(async (host) =>
817+
host === `_sim-challenge.${pendingClaim.domain}`
818+
? [[`sim-domain-verification=${pendingClaim.verificationToken}`]]
819+
: []
820+
)
803821
try {
822+
setEnvFlags({ isHosted: true })
804823
await new Promise<void>((ready) => server.listen(0, '127.0.0.1', ready))
805824
const address = server.address()
806825
if (!address || typeof address === 'string') throw new Error('Fixture did not bind loopback')
807826
endpoint = `http://127.0.0.1:${address.port}`
808827
const commands = [
809828
['organizations', 'sso', 'providers', 'primary', providerId],
810-
['organizations', 'domains', 'verify', claim.id],
829+
['organizations', 'domains', 'verify', pendingClaim.id],
811830
[
812831
'organizations',
813832
'sso',
@@ -874,10 +893,18 @@ describe('Organization SSO administration through API credentials', () => {
874893
expect(attempts[2].stdout).not.toContain(secret)
875894

876895
expect(JSON.parse(attempts[1].stdout)).toMatchObject({
877-
id: claim.id,
896+
id: pendingClaim.id,
878897
status: 'verified',
879898
})
899+
const [verified] = await db
900+
.select()
901+
.from(schema.ssoDomain)
902+
.where(eq(schema.ssoDomain.id, pendingClaim.id))
903+
expect(verified.status).toBe('verified')
904+
expect(verified.verifiedAt).not.toBeNull()
880905
} finally {
906+
dns.mockRestore()
907+
setEnvFlags({ isHosted: false })
881908
await new Promise<void>((complete, reject) => {
882909
server.close((error) => (error ? reject(error) : complete()))
883910
server.closeAllConnections()

‎apps/sim/lib/auth/sso/provider-concurrency.integration.ts‎

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,105 @@ describe('SSO account link concurrency', () => {
4242
runtime = await loadRuntime()
4343
}, 60_000)
4444

45+
it.each([
46+
[false, false],
47+
[true, false],
48+
[false, true],
49+
[true, true],
50+
])(
51+
'refuses an account link when provider deletion wins with transactional adapter=%s and replacement=%s',
52+
async (transactional, replacement) => {
53+
const { db, schema, eq, sql } = runtime
54+
const userId = generateId()
55+
const providerId = `deleted-${generateId()}`
56+
const held = createDeferred<number>()
57+
const release = createDeferred<void>()
58+
const pending: Promise<unknown>[] = []
59+
try {
60+
const now = new Date()
61+
await db.insert(schema.user).values({
62+
id: userId,
63+
name: 'SSO deletion concurrency',
64+
email: `${userId}@example.com`,
65+
emailVerified: true,
66+
createdAt: now,
67+
updatedAt: now,
68+
})
69+
await db.insert(schema.ssoProvider).values({
70+
id: generateId(),
71+
userId,
72+
providerId,
73+
issuer: 'https://original.example.com',
74+
domain: 'example.com',
75+
})
76+
const deletion = db.transaction(async (tx) => {
77+
await runtime.lockSsoProvider(tx, providerId)
78+
const [connection] = await tx.execute<{ pid: number }>(
79+
sql`SELECT pg_backend_pid() AS pid`
80+
)
81+
held.resolve(connection.pid)
82+
await release.promise
83+
await tx.delete(schema.ssoProvider).where(eq(schema.ssoProvider.providerId, providerId))
84+
if (replacement)
85+
await tx.insert(schema.ssoProvider).values({
86+
id: generateId(),
87+
userId,
88+
providerId,
89+
issuer: 'https://replacement.example.com',
90+
domain: 'example.com',
91+
})
92+
})
93+
pending.push(deletion)
94+
void deletion.catch((error: unknown) => held.reject(error))
95+
const blockerPid = await held.promise
96+
const adapter = runtime.createSimAuthAdapter({})
97+
const input = {
98+
model: 'account',
99+
forceAllowId: true,
100+
data: {
101+
id: generateId(),
102+
accountId: generateId(),
103+
userId,
104+
providerId,
105+
createdAt: now,
106+
updatedAt: now,
107+
},
108+
}
109+
const insertion = (
110+
transactional ? adapter.transaction((tx) => tx.create(input)) : adapter.create(input)
111+
).then(
112+
(value) => ({ status: 'fulfilled' as const, value }),
113+
(reason: unknown) => ({ status: 'rejected' as const, reason })
114+
)
115+
pending.push(insertion)
116+
await vi.waitFor(
117+
async () => {
118+
const [waiting] = await db.execute<{ pid: number }>(sql`
119+
SELECT pid FROM pg_stat_activity
120+
WHERE datname = current_database() AND wait_event = 'advisory'
121+
AND ${blockerPid}::int = ANY(pg_blocking_pids(pid))
122+
`)
123+
expect(waiting).toBeDefined()
124+
},
125+
{ timeout: 5_000, interval: 25 }
126+
)
127+
release.resolve()
128+
await deletion
129+
expect(await insertion).toMatchObject({ status: 'rejected', reason: { statusCode: 404 } })
130+
expect(
131+
await db.select().from(schema.account).where(eq(schema.account.providerId, providerId))
132+
).toEqual([])
133+
} finally {
134+
release.resolve()
135+
await Promise.allSettled(pending)
136+
await db.delete(schema.account).where(eq(schema.account.providerId, providerId))
137+
await db.delete(schema.ssoProvider).where(eq(schema.ssoProvider.providerId, providerId))
138+
await db.delete(schema.user).where(eq(schema.user.id, userId))
139+
}
140+
},
141+
30_000
142+
)
143+
45144
it.each([false, true])(
46145
'allows a non-SSO account link while an SSO mutation lock is held with transactional adapter=%s',
47146
async (transactional) => {

0 commit comments

Comments
 (0)