Skip to content

Commit b2aea87

Browse files
waleedlatif1claude
andcommitted
fix(scim): mapping provenance column, drop the unused SSO binding, and close the third review round
- scim_group_mapping.source ('manual' | 'automatic') carries provenance so a deleted author cannot turn a manual mapping into an automatic one; the unreleased 0323 migration is regenerated from the schema - scim_connection.sso_provider_id removed: nothing read it - Entitlement mirrors SSO access: flag is the outer gate, self-hosted is entitled by the flag, hosted requires the enterprise plan; protected and discovery routes answer 404 when the flag is off - Group PATCH: replace without a value is refused - Deprovision re-checks membership under the lock before account-wide effects; relinking an active identity lifts a lingering suspension; instance-organization deployments refuse a connection for any other org - Lowering a workspace grant that is no longer at the recorded level re-establishes the desired level instead of recording it as applied - Connection audit names the actual transition; update responses render inside the write transaction - Reconcile every hour with settings re-read per batch - UI: error states for groups and activity; default group excluded from the mapping picker Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
1 parent d6b077b commit b2aea87

21 files changed

Lines changed: 165 additions & 124 deletions

File tree

apps/sim/app/api/organizations/[id]/scim/route.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,6 @@ export const PUT = defineInternalJsonRoute({
3939
organizationId: params.id,
4040
...(body.status !== undefined ? { status: body.status } : {}),
4141
...(body.settings !== undefined ? { settings: body.settings } : {}),
42-
...(body.ssoProviderId !== undefined ? { ssoProviderId: body.ssoProviderId } : {}),
4342
}),
4443
useCase: configureScimConnection,
4544
present: ({ connection }) => ({ connection }),

apps/sim/ee/scim/components/scim-section.tsx

Lines changed: 41 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -205,8 +205,17 @@ interface GroupMappingsProps {
205205
}
206206

207207
function GroupMappings({ organizationId }: GroupMappingsProps) {
208-
const { data: groups, isLoading } = useScimGroupMappings(organizationId)
209-
const { data: permissionGroups = [] } = usePermissionGroups(organizationId)
208+
const {
209+
data: groups,
210+
isLoading,
211+
isError,
212+
error,
213+
isFetching,
214+
refetch,
215+
} = useScimGroupMappings(organizationId)
216+
const { data: allPermissionGroups = [] } = usePermissionGroups(organizationId)
217+
/** The default group governs by having no members, so it cannot be a membership target. */
218+
const permissionGroups = allPermissionGroups.filter((group) => !group.isDefault)
210219
const { data: workspaces = [] } = useOrganizationWorkspaces(organizationId)
211220
const deleteMapping = useDeleteScimGroupMapping()
212221

@@ -227,6 +236,17 @@ function GroupMappings({ organizationId }: GroupMappingsProps) {
227236
if (isLoading) {
228237
return <SettingsEmptyState variant='inline'>Loading groups...</SettingsEmptyState>
229238
}
239+
if (isError) {
240+
return (
241+
<SettingsQueryErrorState
242+
error={error}
243+
fallback='Failed to load directory groups'
244+
isRetrying={isFetching}
245+
onRetry={() => void refetch()}
246+
variant='inline'
247+
/>
248+
)
249+
}
230250
if (!groups || groups.length === 0) {
231251
return (
232252
<SettingsEmptyState variant='inline'>
@@ -278,11 +298,29 @@ interface ActivityListProps {
278298
}
279299

280300
function ActivityList({ organizationId }: ActivityListProps) {
281-
const { data: entries, isLoading } = useScimActivity(organizationId)
301+
const {
302+
data: entries,
303+
isLoading,
304+
isError,
305+
error,
306+
isFetching,
307+
refetch,
308+
} = useScimActivity(organizationId)
282309

283310
if (isLoading) {
284311
return <SettingsEmptyState variant='inline'>Loading activity...</SettingsEmptyState>
285312
}
313+
if (isError) {
314+
return (
315+
<SettingsQueryErrorState
316+
error={error}
317+
fallback='Failed to load directory activity'
318+
isRetrying={isFetching}
319+
onRetry={() => void refetch()}
320+
variant='inline'
321+
/>
322+
)
323+
}
286324
if (!entries || entries.length === 0) {
287325
return <SettingsEmptyState variant='inline'>No requests yet.</SettingsEmptyState>
288326
}

apps/sim/lib/api/contracts/organization-scim.ts

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,6 @@ const scimConnectionSchema = z.object({
3939
status: z.enum(['active', 'disabled']),
4040
baseUrl: z.string(),
4141
settings: scimConnectionSettingsSchema,
42-
ssoProviderId: z.string().nullable(),
4342
lastRequestAt: z.string().nullable(),
4443
reconciledAt: z.string().nullable(),
4544
createdAt: z.string(),
@@ -65,7 +64,6 @@ export const configureScimConnectionContract = defineRouteContract({
6564
body: z.object({
6665
status: z.enum(['active', 'disabled']).optional(),
6766
settings: scimConnectionSettingsSchema.optional(),
68-
ssoProviderId: z.string().min(1).max(128).nullable().optional(),
6967
}),
7068
response: { mode: 'json', schema: z.object({ connection: scimConnectionSchema }) },
7169
})

apps/sim/lib/api/server/routes/scim-route.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,8 @@ export function createScimRouteBuilder(dependencies: ScimRouteDependencies) {
167167
if (request.method !== options.contract.method) {
168168
throw new ScimError(405, undefined, `${request.method} is not supported here`)
169169
}
170+
/** A deployment without the feature exposes no provisioning surface at all. */
171+
if (!isScimEnabled) throw new ScimError(404, undefined, 'Not found')
170172
assertAcceptableMediaType(request)
171173

172174
/**

apps/sim/lib/scim/application/admin/connection-view.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,6 @@ export async function loadConnectionView(
114114
status: row.status === 'disabled' ? 'disabled' : 'active',
115115
baseUrl: scimBaseUrl(),
116116
settings: row.settings,
117-
ssoProviderId: row.ssoProviderId,
118117
lastRequestAt: row.lastRequestAt?.toISOString() ?? null,
119118
reconciledAt: row.reconciledAt?.toISOString() ?? null,
120119
createdAt: row.createdAt.toISOString(),

apps/sim/lib/scim/application/admin/connection.ts

Lines changed: 10 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,8 @@
11
import { AuditAction, AuditResourceType } from '@sim/audit'
22
import { db } from '@sim/db'
3-
import {
4-
type ScimConnectionSettings,
5-
scimConnection,
6-
scimRequestLog,
7-
ssoProvider,
8-
} from '@sim/db/schema'
3+
import { type ScimConnectionSettings, scimConnection, scimRequestLog } from '@sim/db/schema'
94
import { generateId } from '@sim/utils/id'
10-
import { and, desc, eq } from 'drizzle-orm'
5+
import { desc, eq } from 'drizzle-orm'
116
import type { ScimConnectionSettingsInput } from '@/lib/api/contracts/organization-scim'
127
import { acquireOrganizationMutationLock } from '@/lib/billing/organizations/membership'
138
import { OrchestrationError } from '@/lib/core/orchestration/types'
@@ -40,31 +35,11 @@ export interface ConfigureScimConnectionInput {
4035
organizationId: string
4136
status?: 'active' | 'disabled'
4237
settings?: ScimConnectionSettingsInput
43-
ssoProviderId?: string | null
4438
}
4539

4640
export const configureScimConnection = defineAuthorizedScimAdminUseCase({
4741
operation: scimAdminOperations.configure,
4842
async execute({ input, context }: ScimAdminUseCaseArgs<ConfigureScimConnectionInput>) {
49-
if (input.ssoProviderId) {
50-
const [provider] = await db
51-
.select({ id: ssoProvider.id })
52-
.from(ssoProvider)
53-
.where(
54-
and(
55-
eq(ssoProvider.id, input.ssoProviderId),
56-
eq(ssoProvider.organizationId, context.organizationId)
57-
)
58-
)
59-
.limit(1)
60-
if (!provider) {
61-
throw new OrchestrationError(
62-
'not_found',
63-
'That SSO provider does not belong to this organization'
64-
)
65-
}
66-
}
67-
6843
/**
6944
* A default grant hands every provisioned member a workspace, so the
7045
* workspace must be this organization's — the same check a group mapping
@@ -80,7 +55,7 @@ export const configureScimConnection = defineAuthorizedScimAdminUseCase({
8055
* carrying a stale copy of the earlier one's field — and two first-time
8156
* enables, where there is no row yet to lock, cannot both insert.
8257
*/
83-
const { created, status } = await db.transaction(async (tx) => {
58+
const { created, previousStatus, status } = await db.transaction(async (tx) => {
8459
await acquireOrganizationMutationLock(tx, context.organizationId)
8560
const [existing] = await tx
8661
.select({
@@ -111,36 +86,35 @@ export const configureScimConnection = defineAuthorizedScimAdminUseCase({
11186
.set({
11287
status: nextStatus,
11388
settings: nextSettings,
114-
...(input.ssoProviderId !== undefined ? { ssoProviderId: input.ssoProviderId } : {}),
11589
updatedAt: new Date(),
11690
})
11791
.where(eq(scimConnection.id, existing.id))
11892
} else {
11993
await tx.insert(scimConnection).values({
12094
id: generateId(),
12195
organizationId: context.organizationId,
122-
ssoProviderId: input.ssoProviderId ?? null,
12396
status: nextStatus,
12497
settings: nextSettings,
12598
createdBy: context.actorUserId,
12699
})
127100
}
128-
return { created: !existing, status: nextStatus }
101+
return { created: !existing, previousStatus: existing?.status ?? null, status: nextStatus }
129102
})
130103

131104
const view = await loadConnectionView(context.organizationId)
132105
if (!view || view.status !== status) {
133106
throw new OrchestrationError('internal', 'The connection could not be read back')
134107
}
135-
return { connection: view, created }
108+
return { connection: view, created, previousStatus }
136109
},
110+
/** The action names the transition: enabling (first time or again), disabling, or editing in place. */
137111
projectAudit: ({ result }) => ({
138112
action:
139-
result.connection.status === 'active'
140-
? result.created
113+
result.connection.status !== result.previousStatus
114+
? result.connection.status === 'active'
141115
? AuditAction.SCIM_CONNECTION_ENABLED
142-
: AuditAction.SCIM_CONNECTION_SETTINGS_UPDATED
143-
: AuditAction.SCIM_CONNECTION_DISABLED,
116+
: AuditAction.SCIM_CONNECTION_DISABLED
117+
: AuditAction.SCIM_CONNECTION_SETTINGS_UPDATED,
144118
resourceType: AuditResourceType.SCIM_CONNECTION,
145119
resourceId: result.connection.id,
146120
metadata: { status: result.connection.status },

apps/sim/lib/scim/application/admin/mappings.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -217,6 +217,7 @@ export const upsertScimGroupMapping = defineAuthorizedScimAdminUseCase({
217217
workspaceId: input.targetKind === 'workspace' ? input.workspaceId : null,
218218
permissionType: input.targetKind === 'workspace' ? input.permissionType : null,
219219
role: input.targetKind === 'org_role' ? input.role : null,
220+
source: 'manual',
220221
createdBy: context.actorUserId,
221222
}
222223

apps/sim/lib/scim/application/users/deprovision-user.ts

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,19 @@ export const deprovisionScimUser = defineAuthorizedScimUseCase({
103103
* A stale row for someone who already left — and may since have joined
104104
* another organization — must not sign them out or revoke their keys there.
105105
*/
106-
if (membership) {
106+
const [rejoined] = membership
107+
? await tx
108+
.select({ id: member.id })
109+
.from(member)
110+
.where(
111+
and(
112+
eq(member.organizationId, context.organizationId),
113+
eq(member.userId, current.userId)
114+
)
115+
)
116+
.limit(1)
117+
: []
118+
if (membership && !rejoined) {
107119
await revokeUserSessionsTx(tx, {
108120
userId: current.userId,
109121
organizationId: context.organizationId,

apps/sim/lib/scim/application/users/provision-user.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,14 @@ import { reconcileOrganizationSeats } from '@/lib/billing/organizations/seats'
1515
import { isTeam } from '@/lib/billing/plan-helpers'
1616
import { ENTITLED_SUBSCRIPTION_STATUSES } from '@/lib/billing/subscriptions/utils'
1717
import type { DbOrTx } from '@/lib/db/types'
18+
import {
19+
getInstanceOrganizationId,
20+
isInstanceOrganizationMode,
21+
} from '@/lib/organizations/instance-org'
1822
import {
1923
invalidateAfterSessionRevocation,
2024
suspendMemberTx,
25+
unsuspendMemberTx,
2126
} from '@/lib/organizations/members/lifecycle'
2227
import { captureServerEvent } from '@/lib/posthog/server'
2328
import {
@@ -127,6 +132,22 @@ export const provisionScimUser = defineAuthorizedScimUseCase({
127132
* hang off them. Reimplementing that with a direct insert would skip every
128133
* one.
129134
*/
135+
/**
136+
* In instance-organization mode every account is placed in the instance
137+
* organization at creation, and an account belongs to one organization. A
138+
* connection for any other organization could never admit anyone.
139+
*/
140+
if (isInstanceOrganizationMode()) {
141+
const instanceOrganizationId = await getInstanceOrganizationId()
142+
if (instanceOrganizationId && instanceOrganizationId !== context.organizationId) {
143+
throw new ScimError(
144+
409,
145+
undefined,
146+
'This deployment places every account in its instance organization, so directory provisioning is available only for that organization.'
147+
)
148+
}
149+
}
150+
130151
const resolution = await resolveProvisionedIdentity(db, {
131152
connectionId: context.connection.id,
132153
organizationId: context.organizationId,
@@ -209,6 +230,8 @@ export const provisionScimUser = defineAuthorizedScimUseCase({
209230
*/
210231
if (resolution.action === 'link') {
211232
await syncAccountIdentityTx(tx, { userId, email, name: attributes.name.formatted })
233+
/** A relinked account may still carry the suspension a lost deprovisioning left behind. */
234+
if (attributes.active) await unsuspendMemberTx(tx, { userId, source: 'scim' })
212235
}
213236

214237
const inserted = await insertScimUser(tx, {

apps/sim/lib/scim/application/users/update-user.ts

Lines changed: 14 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -164,14 +164,16 @@ async function loadUserForUpdate(
164164
return current
165165
}
166166

167+
/** Rendered inside the write transaction, so a concurrent delete cannot make a committed update unreadable. */
167168
async function renderUpdated(
169+
tx: DbOrTx,
168170
connectionId: string,
169171
scimUserId: string,
170172
baseUrl: string
171173
): Promise<ReturnType<typeof toUserResource>> {
172-
const record = await findScimUserById(db, connectionId, scimUserId)
174+
const record = await findScimUserById(tx, connectionId, scimUserId)
173175
if (!record) throw new ScimError(500, undefined, 'The updated user could not be read back')
174-
const groups = (await loadGroupsForScimUsers(db, [record.id])).get(record.id) ?? []
176+
const groups = (await loadGroupsForScimUsers(tx, [record.id])).get(record.id) ?? []
175177
return toUserResource(toUserResourceRow(record, groups), baseUrl)
176178
}
177179

@@ -225,7 +227,7 @@ export const replaceScimUser = defineAuthorizedScimUseCase({
225227
input,
226228
context,
227229
}: ScimUseCaseArgs<ReplaceScimUserInput>): Promise<UpdateScimUserResult> {
228-
const { scimUserId, userId, outcome } = await db.transaction(async (tx) => {
230+
return db.transaction(async (tx) => {
229231
const current = await loadUserForUpdate(tx, context, input.scimUserId)
230232

231233
/**
@@ -245,21 +247,16 @@ export const replaceScimUser = defineAuthorizedScimUseCase({
245247
* changes nothing must not write, audit, or re-project, or a 2,000-user
246248
* organization produces 2,000 spurious audit rows per sync.
247249
*/
248-
if (userAttributesEqual(current.attributes, next)) {
249-
return { scimUserId: current.id, userId: current.userId, outcome: null }
250-
}
250+
const outcome = userAttributesEqual(current.attributes, next)
251+
? null
252+
: await applyUserUpdate(tx, context, current, next)
251253
return {
252254
scimUserId: current.id,
253255
userId: current.userId,
254-
outcome: await applyUserUpdate(tx, context, current, next),
256+
outcome,
257+
resource: await renderUpdated(tx, context.connection.id, current.id, context.baseUrl),
255258
}
256259
})
257-
return {
258-
scimUserId,
259-
userId,
260-
outcome,
261-
resource: await renderUpdated(context.connection.id, scimUserId, context.baseUrl),
262-
}
263260
},
264261
projectAudit: ({ result }) => auditEntries(result),
265262
afterSuccess: async ({ result, context }) =>
@@ -277,7 +274,7 @@ export const patchScimUser = defineAuthorizedScimUseCase({
277274
input,
278275
context,
279276
}: ScimUseCaseArgs<PatchScimUserInput>): Promise<UpdateScimUserResult> {
280-
const { scimUserId, userId, outcome } = await db.transaction(async (tx) => {
277+
return db.transaction(async (tx) => {
281278
const current = await loadUserForUpdate(tx, context, input.scimUserId)
282279

283280
const { next, changed } = applyUserPatch(current.attributes, input.operations)
@@ -289,19 +286,14 @@ export const patchScimUser = defineAuthorizedScimUseCase({
289286
* projection pass, and a `lastModified` bump for a request that meant
290287
* nothing.
291288
*/
292-
if (!changed) return { scimUserId: current.id, userId: current.userId, outcome: null }
289+
const outcome = changed ? await applyUserUpdate(tx, context, current, next) : null
293290
return {
294291
scimUserId: current.id,
295292
userId: current.userId,
296-
outcome: await applyUserUpdate(tx, context, current, next),
293+
outcome,
294+
resource: await renderUpdated(tx, context.connection.id, current.id, context.baseUrl),
297295
}
298296
})
299-
return {
300-
scimUserId,
301-
userId,
302-
outcome,
303-
resource: await renderUpdated(context.connection.id, scimUserId, context.baseUrl),
304-
}
305297
},
306298
projectAudit: ({ result }) => auditEntries(result),
307299
afterSuccess: async ({ result, context }) =>

0 commit comments

Comments
 (0)