Skip to content

Commit f492eef

Browse files
committed
fix(scim): close provisioning races and refresh policy state
1 parent 705a0d1 commit f492eef

9 files changed

Lines changed: 283 additions & 40 deletions

File tree

‎apps/sim/ee/access-control/components/group-detail.tsx‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1588,7 +1588,9 @@ export function GroupDetail({
15881588
<div className='flex items-center justify-between gap-3'>
15891589
<span className='text-[var(--text-muted)] text-small'>
15901590
{members.length === 0
1591-
? 'Applies to all members of its workspaces. Add members to restrict it to specific people.'
1591+
? group.membershipMode === 'inherit'
1592+
? 'Applies to all members of its workspaces. Add members to restrict it to specific people.'
1593+
: 'No members assigned.'
15921594
: `Restricted to ${members.length} member${members.length === 1 ? '' : 's'}`}
15931595
</span>
15941596
<Chip

‎apps/sim/ee/scim/TESTING.md‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,8 @@ timeout.
7474
by conflict recovery and reconciliation.
7575
- A concurrent membership-lock change while a group removal waits on the
7676
organization lock; withdrawal uses the committed setting.
77+
- Concurrent name-matching changes while POST, PUT, and PATCH wait on the
78+
organization lock; mapping uses the committed setting in both toggle directions.
7779
- Managed-membership enforcement through the actual batch invitation and
7880
workspace permission APIs, including successful edits to unmanaged members.
7981
- Session revocation on deactivation and permanent OAuth grant revocation on
@@ -125,9 +127,10 @@ the admission operation and Enterprise entitlement reads through PostgreSQL.
125127

126128
The real PostgreSQL/Redis suite
127129
`lib/mothership/request/application/removal-stops-runs.integration.ts` verifies
128-
that deactivation writes durable Stops for admitted Chat runs, reactivation cannot
129-
revive those runs, an existing account first provisioned inactive is stopped,
130-
and a rolled-back suspension emits no durable or delivered Stop.
130+
that deactivation writes durable Stops for admitted Chat runs, including concurrent
131+
interactive and headless admission. Reactivation cannot revive those runs or a
132+
headless run admitted during suspension. An existing account first provisioned
133+
inactive is stopped, and a rolled-back suspension emits no durable or delivered Stop.
131134

132135
## Verification dates and limits
133136

‎apps/sim/ee/scim/hooks/scim.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,7 @@ export function useUpsertScimGroupMapping() {
140140
requestJson(upsertScimGroupMappingContract, { params: { id: organizationId }, body }),
141141
onSettled: (_data, _error, { organizationId }) => {
142142
queryClient.invalidateQueries({ queryKey: scimKeys.mappings(organizationId) })
143-
queryClient.invalidateQueries({ queryKey: permissionGroupKeys.list(organizationId) })
143+
queryClient.invalidateQueries({ queryKey: permissionGroupKeys.all })
144144
},
145145
})
146146
}
@@ -157,7 +157,7 @@ export function useDeleteScimGroupMapping() {
157157
requestJson(deleteScimGroupMappingContract, { params: { id: organizationId, mappingId } }),
158158
onSettled: (_data, _error, { organizationId }) => {
159159
queryClient.invalidateQueries({ queryKey: scimKeys.mappings(organizationId) })
160-
queryClient.invalidateQueries({ queryKey: permissionGroupKeys.list(organizationId) })
160+
queryClient.invalidateQueries({ queryKey: permissionGroupKeys.all })
161161
},
162162
})
163163
}
@@ -170,7 +170,7 @@ export function useReconcileScimConnection() {
170170
onSettled: (_data, _error, organizationId) => {
171171
queryClient.invalidateQueries({ queryKey: scimKeys.connection(organizationId) })
172172
queryClient.invalidateQueries({ queryKey: scimKeys.mappings(organizationId) })
173-
queryClient.invalidateQueries({ queryKey: permissionGroupKeys.list(organizationId) })
173+
queryClient.invalidateQueries({ queryKey: permissionGroupKeys.all })
174174
},
175175
})
176176
}

‎apps/sim/ee/scim/lib/application/groups/manage-groups.test.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,7 @@ describe('replaceScimGroup', () => {
7777
queueTableRows(scimConnection, [
7878
{ id: 'conn-1', organizationId: 'org-1', status: 'active', settings: {} },
7979
])
80+
queueTableRows(scimConnection, [{ status: 'active', settings: {} }])
8081
let group = { ...initialGroup }
8182
const memberIds = new Set(['user-1'])
8283
mocks.findGroup.mockImplementation(async () => group)

‎apps/sim/ee/scim/lib/application/groups/manage-groups.ts‎

Lines changed: 24 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { AuditAction, AuditResourceType } from '@sim/audit'
22
import { db } from '@sim/db'
3-
import { scimGroup } from '@sim/db/schema'
3+
import { type ScimConnectionSettings, scimConnection, scimGroup } from '@sim/db/schema'
44
import { and, eq, ne } from 'drizzle-orm'
55
import type { ScimPatchOperation } from '@/lib/api/contracts/scim'
66
import { acquireOrganizationMutationLock } from '@/lib/billing/organizations/membership'
@@ -57,11 +57,26 @@ import {
5757
*/
5858
async function withGroupWrite<T extends { stoppedRuns: StoppedRun[] }>(
5959
context: ScimUseCaseContext,
60-
work: (tx: DbTransaction) => Promise<T>
60+
work: (tx: DbTransaction, settings: ScimConnectionSettings) => Promise<T>
6161
): Promise<Omit<T, 'stoppedRuns'>> {
6262
const { stoppedRuns, ...result } = await db.transaction(async (tx) => {
6363
await acquireOrganizationMutationLock(tx, context.organizationId)
64-
return work(tx)
64+
const [connection] = await tx
65+
.select({ status: scimConnection.status, settings: scimConnection.settings })
66+
.from(scimConnection)
67+
.where(
68+
and(
69+
eq(scimConnection.id, context.connection.id),
70+
eq(scimConnection.organizationId, context.organizationId)
71+
)
72+
)
73+
.limit(1)
74+
if (!connection || connection.status !== 'active') {
75+
throw new ScimError(401, undefined, 'Invalid SCIM token', {
76+
'WWW-Authenticate': 'Bearer realm="SCIM"',
77+
})
78+
}
79+
return work(tx, connection.settings)
6580
})
6681
deliverRunStops(stoppedRuns)
6782
return result
@@ -185,7 +200,7 @@ export const createScimGroup = defineAuthorizedScimUseCase({
185200
context,
186201
}: ScimUseCaseArgs<CreateScimGroupInput>): Promise<ScimGroupWriteResult> {
187202
const { group } = input
188-
return withGroupWrite(context, async (tx) => {
203+
return withGroupWrite(context, async (tx, settings) => {
189204
await assertDisplayNameAvailable(tx, {
190205
connectionId: context.connection.id,
191206
displayName: group.displayName,
@@ -202,7 +217,7 @@ export const createScimGroup = defineAuthorizedScimUseCase({
202217
}
203218
await assertMemberCount(tx, created.id)
204219

205-
const mapped = context.connection.settings.autoMapPermissionGroupsByName
220+
const mapped = settings.autoMapPermissionGroupsByName
206221
? await autoMapPermissionGroupByName(tx, {
207222
organizationId: context.organizationId,
208223
scimGroupId: created.id,
@@ -253,7 +268,7 @@ export const replaceScimGroup = defineAuthorizedScimUseCase({
253268
input,
254269
context,
255270
}: ScimUseCaseArgs<ReplaceScimGroupInput>): Promise<ScimGroupWriteResult> {
256-
return withGroupWrite(context, async (tx) => {
271+
return withGroupWrite(context, async (tx, settings) => {
257272
const current = await findScimGroupById(tx, context.connection.id, input.groupId)
258273
if (!current) throw notFound('SCIM Group not found')
259274

@@ -277,7 +292,7 @@ export const replaceScimGroup = defineAuthorizedScimUseCase({
277292
})
278293
}
279294
let adopted = false
280-
if (renamed && context.connection.settings.autoMapPermissionGroupsByName) {
295+
if (renamed && settings.autoMapPermissionGroupsByName) {
281296
const mapped = await autoMapPermissionGroupByName(tx, {
282297
organizationId: context.organizationId,
283298
scimGroupId: current.id,
@@ -358,7 +373,7 @@ export const patchScimGroup = defineAuthorizedScimUseCase({
358373
}: ScimUseCaseArgs<PatchScimGroupInput>): Promise<PatchScimGroupResult> {
359374
const patch = parseGroupPatch(input.operations)
360375

361-
return withGroupWrite(context, async (tx) => {
376+
return withGroupWrite(context, async (tx, settings) => {
362377
const current = await findScimGroupById(tx, context.connection.id, input.groupId)
363378
if (!current) throw notFound('SCIM Group not found')
364379

@@ -402,11 +417,7 @@ export const patchScimGroup = defineAuthorizedScimUseCase({
402417
...(externalIdChanged ? { externalId: patch.externalId } : {}),
403418
})
404419
}
405-
if (
406-
renamed &&
407-
patch.displayName &&
408-
context.connection.settings.autoMapPermissionGroupsByName
409-
) {
420+
if (renamed && patch.displayName && settings.autoMapPermissionGroupsByName) {
410421
const mapped = await autoMapPermissionGroupByName(tx, {
411422
organizationId: context.organizationId,
412423
scimGroupId: current.id,

‎apps/sim/lib/mothership/async-runs/repository.test.ts‎

Lines changed: 22 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -42,14 +42,15 @@ describe('run admission and early Stop', () => {
4242
expect(await requestRunStop(scope)).toBeNull()
4343
expect(dbChainMockFns.values).toHaveBeenCalledWith(scope)
4444
expect(dbChainMockFns.onConflictDoNothing).toHaveBeenCalled()
45-
expect(dbChainMockFns.execute).toHaveBeenCalledTimes(2)
4645
expect(
47-
hasMockCondition(
48-
dbChainMockFns.where.mock.calls[0]?.[0],
49-
(condition) =>
50-
condition.type === 'eq' &&
51-
condition.left === copilotRuns.userId &&
52-
condition.right === scope.userId
46+
dbChainMockFns.where.mock.calls.some(([predicate]) =>
47+
hasMockCondition(
48+
predicate,
49+
(condition) =>
50+
condition.type === 'eq' &&
51+
condition.left === copilotRuns.userId &&
52+
condition.right === scope.userId
53+
)
5354
)
5455
).toBe(true)
5556
})
@@ -148,12 +149,13 @@ describe('run admission and early Stop', () => {
148149
[copilotRequestStops.workspaceId, scope.workspaceId],
149150
[copilotRequestStops.streamId, scope.streamId],
150151
]) {
151-
/** The chat row is read first; the Stop lookup follows it. */
152152
expect(
153-
hasMockCondition(
154-
dbChainMockFns.where.mock.calls[1]?.[0],
155-
(condition) =>
156-
condition.type === 'eq' && condition.left === column && condition.right === value
153+
dbChainMockFns.where.mock.calls.some(([predicate]) =>
154+
hasMockCondition(
155+
predicate,
156+
(condition) =>
157+
condition.type === 'eq' && condition.left === column && condition.right === value
158+
)
157159
)
158160
).toBe(true)
159161
}
@@ -167,12 +169,14 @@ describe('run admission and early Stop', () => {
167169
expect.objectContaining({ workspaceId: scope.workspaceId, status: 'active' })
168170
)
169171
expect(
170-
hasMockCondition(
171-
dbChainMockFns.where.mock.calls[0]?.[0],
172-
(condition) =>
173-
condition.type === 'eq' &&
174-
condition.left === copilotChats.userId &&
175-
condition.right === scope.userId
172+
dbChainMockFns.where.mock.calls.some(([predicate]) =>
173+
hasMockCondition(
174+
predicate,
175+
(condition) =>
176+
condition.type === 'eq' &&
177+
condition.left === copilotChats.userId &&
178+
condition.right === scope.userId
179+
)
176180
)
177181
).toBe(true)
178182
})

‎apps/sim/lib/mothership/async-runs/repository.ts‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
copilotOrganizationRequestStops,
1010
copilotRequestStops,
1111
copilotRuns,
12+
user,
1213
} from '@sim/db/schema'
1314
import { createLogger } from '@sim/logger'
1415
import { filterUndefined, toRecordOrNull } from '@sim/utils/object'
@@ -154,14 +155,16 @@ export interface CreateRunSegmentInput {
154155

155156
type RunAdmissionTransaction = Parameters<Parameters<typeof db.transaction>[0]>[0]
156157

157-
/** Serializes Stop and admission, including when no run row exists to lock yet. */
158+
/** Serializes account suspension, Stop and admission, even before a run row exists. */
158159
export async function withRunAdmissionLock<T>(
159160
userId: string,
160161
streamId: string,
161162
action: (tx: RunAdmissionTransaction) => Promise<T>
162163
): Promise<T> {
163164
return db.transaction(async (tx) => {
164165
await tx.execute(sql`SET LOCAL statement_timeout = '10s'`)
166+
/** Lock the account before any chat/run writes; suspension updates it before scanning runs. */
167+
await tx.select({ id: user.id }).from(user).where(eq(user.id, userId)).for('share')
165168
const key = JSON.stringify(['copilot-run-admission', userId, streamId])
166169
await acquireAdvisoryXactLock(tx, 'copilot_run_admission', key)
167170
return action(tx)
@@ -462,8 +465,10 @@ export async function insertRunSegment(tx: RunAdmissionTransaction, input: Creat
462465
workspaceId: copilotChats.workspaceId,
463466
organizationId: copilotChats.organizationId,
464467
deletedAt: copilotChats.deletedAt,
468+
suspendedAt: user.suspendedAt,
465469
})
466470
.from(copilotChats)
471+
.innerJoin(user, eq(user.id, copilotChats.userId))
467472
.where(and(eq(copilotChats.id, input.chatId), eq(copilotChats.userId, input.userId)))
468473
.for('share')
469474
const inheritsOwner = !input.workspaceId && !input.organizationId
@@ -475,7 +480,10 @@ export async function insertRunSegment(tx: RunAdmissionTransaction, input: Creat
475480
.from(table)
476481
.where(predicate)
477482
.limit(1)
478-
const closedAt = stop?.stoppedAt ?? chat?.deletedAt
483+
const closedAt = stop?.stoppedAt ?? chat?.deletedAt ?? chat?.suspendedAt
484+
if (chat?.suspendedAt && !stop) {
485+
await recordRunStopTx(tx, { ...input, workspaceId, organizationId }, false)
486+
}
479487
const [run] = await tx
480488
.insert(copilotRuns)
481489
.values({

0 commit comments

Comments
 (0)