Skip to content

Commit 41ab8b1

Browse files
waleedlatif1claude
andcommitted
fix(scim): second audit round — bugs, abstractions, tests
Findings from a full code review of the SCIM surface and a regression review of the shared primitives it extracted: - attributes/excludedAttributes projection no longer fails response validation (Entra excludes groups on every page); projected attributes are optional in the response contract - defaultWorkspaceGrants are checked against the organization, closing a cross-tenant workspace grant - User PUT/PATCH serialize on the organization/user advisory locks instead of a scim_user row lock that inverted lock order against projection - PATCH add emails keeps the existing primary; canonical nested name and enterprise objects are accepted in a path-less replace - A group mapped to the org admin role skips the owner instead of failing the sync; a withdrawal that cannot hand ownership on is retried next pass - Relinking a recreated identity syncs the account's email and name - disableJit is enforced at SSO admission rather than flipped once on the provider; reconcile refuses a disabled connection clearly - Conflicts that are not duplicates no longer carry scimType uniqueness - Workspace revocation hands the workspace to its billed account first, matching the members route - Batch invitations return 403 for a managed member; role change maps a lock timeout to 409; managed-membership locking is inert with the flag off - Settings routes' all-members conflict rule ignores explicit-mode groups - Admin wrapper projects audit like the directory wrapper; admin use cases split into connection, credentials, mappings - Pure grant resolution/diff extracted; group list N+1 removed; no-op group writes skipped; roster shows directory deactivation - Tests: grants, authentication, identity resolution, lifecycle primitives, projection-vs-contract, email add, canonical nesting (139 SCIM tests) - Dead code removed; unrelated sandbox bundle churn reverted Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
1 parent 1592ef1 commit 41ab8b1

56 files changed

Lines changed: 2106 additions & 1073 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

apps/docs/content/docs/platform/enterprise/scim.mdx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -149,13 +149,13 @@ Sim also re-applies every group mapping on a schedule, so drift cannot persist.
149149
- Base URL: `https://<your-sim-domain>/api/scim/v2`
150150
- Authentication: `Authorization: Bearer <credential>`
151151
- Resources: `/Users`, `/Groups`, plus `/ServiceProviderConfig`, `/ResourceTypes`, and `/Schemas`
152-
- Filters: `eq`, joined with `and`, on `userName`, `externalId`, `emails.value`, and `displayName`
152+
- Filters: `eq`, joined with `and`, on `id`, `userName`, `externalId`, `emails.value`, `active`, and `displayName`
153153
- Page size: up to 100 per request
154154

155155
<FAQ items={[
156156
{
157157
question: "What happens to someone's workflows when they are deactivated?",
158-
answer: "Nothing. A deactivation blocks sign-in and stops their API keys, and leaves every workspace, workflow, and credential they own exactly as it was. Reactivating them restores access. Only a removal — which your provider sends explicitly — reassigns what they owned."
158+
answer: "Nothing. A deactivation blocks sign-in and stops their API keys, and leaves every workspace, workflow, and credential they own exactly as it was. Reactivating them restores access. Only a removal — which your provider sends explicitly — reassigns what they owned. A deactivated member also keeps their seat until they are removed."
159159
},
160160
{
161161
question: "Can the directory provision someone outside our verified domains?",

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import { reconcileOrganizationSeats } from '@/lib/billing/organizations/seats'
1919
import { ForbiddenOperationError } from '@/lib/core/application'
2020
import { OrchestrationError, statusForOrchestrationError } from '@/lib/core/orchestration/types'
2121
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
22+
import { isRetryableTransactionError } from '@/lib/db/transaction'
2223
import { changeMemberRoleTx } from '@/lib/organizations/members/lifecycle'
2324
import { captureServerEvent } from '@/lib/posthog/server'
2425
import { assertMembershipNotScimManaged } from '@/lib/scim/managed-membership'
@@ -220,7 +221,6 @@ export const PUT = withRouteHandler(
220221
await assertMembershipNotScimManaged({
221222
organizationId,
222223
userId: memberId,
223-
action: 'change-role',
224224
})
225225

226226
/**
@@ -292,6 +292,13 @@ export const PUT = withRouteHandler(
292292
{ status: statusForOrchestrationError(error.code) }
293293
)
294294
}
295+
/** The role change now serializes on the organization lock; a timeout is "retry", not a fault. */
296+
if (isRetryableTransactionError(error)) {
297+
return NextResponse.json(
298+
{ error: 'The organization is busy; retry in a moment' },
299+
{ status: 409 }
300+
)
301+
}
295302

296303
logger.error('Failed to update organization member role', {
297304
organizationId: (await context.params).id,

apps/sim/app/api/organizations/[id]/permission-groups/[groupId]/members/route.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -281,7 +281,7 @@ export const DELETE = withRouteHandler(
281281
throw new Error('MEMBER_NOT_FOUND')
282282
}
283283

284-
if (!lockedGroup.isDefault) {
284+
if (!lockedGroup.isDefault && lockedGroup.membershipMode === 'inherit') {
285285
const [memberCountRow] = await tx
286286
.select({ value: count() })
287287
.from(permissionGroupMember)

apps/sim/app/api/organizations/[id]/permission-groups/utils.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ export async function loadGroupInOrganization(
5858
createdAt: permissionGroup.createdAt,
5959
updatedAt: permissionGroup.updatedAt,
6060
isDefault: permissionGroup.isDefault,
61+
membershipMode: permissionGroup.membershipMode,
6162
})
6263
.from(permissionGroup)
6364
.where(and(eq(permissionGroup.id, groupId), eq(permissionGroup.organizationId, organizationId)))
@@ -209,7 +210,8 @@ export interface AllMembersConflict {
209210
* all-members group already targeting one of those workspaces, or `null`. Two
210211
* all-members groups on one workspace would both claim everyone there, so this
211212
* is rejected at assignment time. The candidate group (`excludeGroupId`) is
212-
* ignored.
213+
* ignored, and so is any group in `explicit` membership mode: empty, it governs
214+
* nobody rather than everyone, so it cannot collide.
213215
*/
214216
export async function findAllMembersWorkspaceConflict(
215217
params: { organizationId: string; excludeGroupId: string; workspaceIds: string[] },
@@ -234,6 +236,7 @@ export async function findAllMembersWorkspaceConflict(
234236
and(
235237
eq(permissionGroup.organizationId, organizationId),
236238
eq(permissionGroup.isDefault, false),
239+
eq(permissionGroup.membershipMode, 'inherit'),
237240
ne(permissionGroup.id, excludeGroupId),
238241
inArray(permissionGroupWorkspace.workspaceId, workspaceIds),
239242
sql`not exists (

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ const MEMBER_ROWS = [
5757
userName: 'Admin User',
5858
userEmail: 'admin@example.com',
5959
userImage: null,
60+
userSuspendedAt: null,
6061
},
6162
{
6263
memberId: 'member-reader',
@@ -66,6 +67,7 @@ const MEMBER_ROWS = [
6667
userName: 'Reader User',
6768
userEmail: 'reader@example.com',
6869
userImage: 'https://example.com/reader.png',
70+
userSuspendedAt: null,
6971
},
7072
]
7173

@@ -118,6 +120,7 @@ describe('GET /api/organizations/[id]/roster', () => {
118120
name: 'Admin User',
119121
email: 'admin@example.com',
120122
image: null,
123+
suspendedAt: null,
121124
workspaces: [],
122125
},
123126
{
@@ -128,6 +131,7 @@ describe('GET /api/organizations/[id]/roster', () => {
128131
name: 'Reader User',
129132
email: 'reader@example.com',
130133
image: 'https://example.com/reader.png',
134+
suspendedAt: null,
131135
workspaces: [],
132136
},
133137
],
@@ -167,6 +171,7 @@ describe('GET /api/organizations/[id]/roster', () => {
167171
userName: 'External User',
168172
userEmail: 'external@example.com',
169173
userImage: null,
174+
userSuspendedAt: null,
170175
workspaceId: 'workspace-1',
171176
permission: 'read',
172177
createdAt: new Date('2026-03-01T00:00:00.000Z'),

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@ export const GET = withRouteHandler(
8383
userName: user.name,
8484
userEmail: user.email,
8585
userImage: user.image,
86+
userSuspendedAt: user.suspendedAt,
8687
})
8788
.from(member)
8889
.innerJoin(user, eq(member.userId, user.id))
@@ -96,6 +97,7 @@ export const GET = withRouteHandler(
9697
name: row.userName,
9798
email: row.userEmail,
9899
image: row.userImage,
100+
suspendedAt: row.userSuspendedAt?.toISOString() ?? null,
99101
workspaces: [] as RosterWorkspaceAccess[],
100102
}))
101103

@@ -189,6 +191,7 @@ export const GET = withRouteHandler(
189191
userName: user.name,
190192
userEmail: user.email,
191193
userImage: user.image,
194+
userSuspendedAt: user.suspendedAt,
192195
workspaceId: permissions.entityId,
193196
permission: permissions.permissionType,
194197
createdAt: permissions.createdAt,
@@ -218,6 +221,7 @@ export const GET = withRouteHandler(
218221
name: string
219222
email: string
220223
image: string | null
224+
suspendedAt: string | null
221225
workspaces: RosterWorkspaceAccess[]
222226
}
223227
>()
@@ -247,6 +251,7 @@ export const GET = withRouteHandler(
247251
name: row.userName,
248252
email: row.userEmail,
249253
image: row.userImage,
254+
suspendedAt: row.userSuspendedAt?.toISOString() ?? null,
250255
workspaces: [workspaceAccess],
251256
})
252257
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import {
55
internalRateLimits,
66
internalSessionAuth,
77
} from '@/lib/api/server/routes'
8-
import { listScimActivity } from '@/lib/scim/application/admin/manage-connection'
8+
import { listScimActivity } from '@/lib/scim/application/admin/connection'
99

1010
/** Recent provisioning requests, so a failing sync can be diagnosed from Sim. */
1111
export const GET = defineInternalJsonRoute({

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import {
55
internalRateLimits,
66
internalSessionAuth,
77
} from '@/lib/api/server/routes'
8-
import { revokeScimCredential } from '@/lib/scim/application/admin/manage-connection'
8+
import { revokeScimCredential } from '@/lib/scim/application/admin/credentials'
99

1010
export const DELETE = defineInternalJsonRoute({
1111
contract: revokeScimCredentialContract,
@@ -18,4 +18,5 @@ export const DELETE = defineInternalJsonRoute({
1818
credentialId: params.credentialId,
1919
}),
2020
useCase: revokeScimCredential,
21+
present: ({ success }) => ({ success }),
2122
})

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import {
55
internalRateLimits,
66
internalSessionAuth,
77
} from '@/lib/api/server/routes'
8-
import { issueScimCredential } from '@/lib/scim/application/admin/manage-connection'
8+
import { issueScimCredential } from '@/lib/scim/application/admin/credentials'
99

1010
/** Issues a bearer credential. The secret is returned once and never stored. */
1111
export const POST = defineInternalJsonRoute({
@@ -23,4 +23,5 @@ export const POST = defineInternalJsonRoute({
2323
...(body.expiresInDays !== undefined ? { expiresInDays: body.expiresInDays } : {}),
2424
}),
2525
useCase: issueScimCredential,
26+
present: ({ secret, credential }) => ({ secret, credential }),
2627
})

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import {
55
internalRateLimits,
66
internalSessionAuth,
77
} from '@/lib/api/server/routes'
8-
import { deleteScimGroupMapping } from '@/lib/scim/application/admin/manage-connection'
8+
import { deleteScimGroupMapping } from '@/lib/scim/application/admin/mappings'
99

1010
export const DELETE = defineInternalJsonRoute({
1111
contract: deleteScimGroupMappingContract,
@@ -15,4 +15,5 @@ export const DELETE = defineInternalJsonRoute({
1515
errorPolicy: internalOrchestrationErrorPolicy,
1616
mapInput: ({ params }) => ({ organizationId: params.id, mappingId: params.mappingId }),
1717
useCase: deleteScimGroupMapping,
18+
present: ({ success, reconciledUsers }) => ({ success, reconciledUsers }),
1819
})

0 commit comments

Comments
 (0)