Skip to content

Commit 670b288

Browse files
waleedlatif1claude
andcommitted
refactor(scim): one removal invariant, honest provenance, finished primitive extraction, tests
Findings from an independent three-way audit (architecture, correctness, tests) after the review rounds: - Removing a member is one transaction that also revokes sessions and personal keys and retires the directory row into its tombstone, so a member can never be gone while the directory says otherwise. The four defensive re-checks the review rounds had added are gone; deprovision is one path and the settings UI removal gains the same revocation - Every satisfied mapping is recorded; access the person already held is recorded as `adopted` and left alone on withdrawal unless the directory is the source of truth. Reconcile is idempotent again - Permission-group conflict rules live once, in the shared membership primitive; the settings routes and the workspace-member route use the shared primitives; direct workspace grants honor managed-membership lock - Lock order: the permission-group leaf lock precedes every write to its row; deadlocks map to 503 like lock timeouts; credential issue serializes on the organization lock; reconcile batches shrink to 25 - PATCH keeps unmodelled attributes under `extra` as create and replace do, so Entra's default mappings no longer fail whole updates - Unknown group members are dropped with a warning; Entra's legacy User schema marker is tolerated; the directory re-asserts an account address that drifted; relink lifts a lingering suspension; a reconcile pass advances the watermark only when it completes and re-reads settings per batch; instance-organization deployments refuse a connection for any other organization; provisioning cleans up through account deletion - Base URL, credential predicate, request-log type, audit recording, and seat policy each live in one place; `ssoProviderId` and the unused credential scope parameter are removed - Tests for the reconciler, user updates, deprovision, the route builder, entitlement, auto-map, and the shared conflict rules; docs corrected Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DGXcwsHzYGp6pUDWRtJKpz
1 parent b2aea87 commit 670b288

63 files changed

Lines changed: 1982 additions & 935 deletions

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: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ It pairs with [SSO](/platform/enterprise/sso). SSO proves who someone is when th
2424
| Updates their name or email | Updates the Sim account, and ends their sessions if the address changed |
2525
| Deactivates them | Blocks sign-in and stops their personal API keys. Everything they own, and every grant they hold, is left untouched; shared workspace keys keep working |
2626
| Reactivates them | Restores access exactly as it was |
27-
| Removes them from the app | Removes their organization membership and reassigns what they owned |
27+
| Removes them from the app | Removes their organization membership, ends their sessions, deletes their personal API keys, and reassigns what they owned |
2828
| Adds them to a group | Grants whatever that group maps to |
2929

3030
Deactivation is reversible and never destructive. Someone on leave keeps their workflows, their credentials, and their workspace history; they simply cannot sign in.
@@ -131,7 +131,7 @@ A group can carry several mappings. When two groups grant the same workspace at
131131

132132
Sim records every grant it makes on your behalf. When someone leaves a group, only what the directory granted is taken back — access a workspace administrator granted by hand stays.
133133

134-
The one exception is **managed membership locking**, which is on by default. With it on, the directory is the source of truth: Sim refuses invitations, role changes, and manual grants for provisioned members, because the next sync would revert them anyway. Turn it off if you want to layer manual access on top of directory access.
134+
The one exception is **managed membership locking**, which is on by default. With it on, the directory is the source of truth: Sim refuses invitations, workspace grants, and role changes for provisioned members, because the next sync would revert them anyway. Removals stay possible so an administrator can always act in an emergency. Turn it off if you want to layer manual access on top of directory access.
135135

136136
## Provisioning and SSO together
137137

@@ -143,7 +143,7 @@ If you want the directory to be the only way in, enable **Disable just-in-time p
143143

144144
**Settings → SSO → Directory provisioning → Activity** lists recent requests with their status and, for a failure, what was wrong. Providers report a failed cycle without saying what they sent, so this is usually the fastest way to see the cause.
145145

146-
Sim also re-applies every group mapping on a schedule, so drift cannot persist. You can run it on demand with **Reconcile now**.
146+
Sim also re-applies every group mapping once an hour, so drift cannot persist. You can run it on demand with **Reconcile now**, which is also how a change to the connection settings reaches members before the next sync.
147147

148148
## Reference
149149

apps/sim/app/api/cron/scim-reconcile/route.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { createLogger } from '@sim/logger'
22
import { toError } from '@sim/utils/errors'
33
import { type NextRequest, NextResponse } from 'next/server'
44
import { verifyCronAuth } from '@/lib/auth/internal'
5-
import { isBillingEnabled, isScimEnabled } from '@/lib/core/config/env-flags'
5+
import { isScimEnabled } from '@/lib/core/config/env-flags'
66
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
77
import { runScimReconcileSweep } from '@/lib/scim/reconcile/job'
88

@@ -16,7 +16,7 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
1616
const authError = verifyCronAuth(request, 'SCIM reconciliation')
1717
if (authError) return authError
1818

19-
if (!isBillingEnabled && !isScimEnabled) {
19+
if (!isScimEnabled) {
2020
return NextResponse.json({ success: true, connections: 0, skipped: 'disabled' })
2121
}
2222

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,15 +10,17 @@ import { bulkAddPermissionGroupMembersContract } from '@/lib/api/contracts/permi
1010
import { getValidationErrorMessage, parseRequest } from '@/lib/api/server'
1111
import { getSession } from '@/lib/auth'
1212
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
13+
import {
14+
findScopeConflicts,
15+
type ScopeConflict,
16+
} from '@/lib/permission-groups/application/group-membership'
1317
import { PERMISSION_GROUP_MEMBER_CONSTRAINTS } from '@/lib/permission-groups/constraints'
1418
import { acquirePermissionGroupOrgLock } from '@/lib/permission-groups/locks'
1519
import {
1620
authorizeOrgAccessControl,
17-
findScopeConflicts,
1821
formatScopeConflictError,
1922
getGroupWorkspaces,
2023
loadGroupInOrganization,
21-
type ScopeConflict,
2224
} from '@/app/api/organizations/[id]/permission-groups/utils'
2325

2426
const logger = createLogger('OrganizationPermissionGroupBulkMembers')

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

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,19 +10,21 @@ import { addPermissionGroupMemberContract } from '@/lib/api/contracts/permission
1010
import { getValidationErrorMessage, parseRequest } from '@/lib/api/server'
1111
import { getSession } from '@/lib/auth'
1212
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
13+
import {
14+
type AllMembersConflict,
15+
findAllMembersWorkspaceConflict,
16+
findScopeConflicts,
17+
type ScopeConflict,
18+
} from '@/lib/permission-groups/application/group-membership'
1319
import { PERMISSION_GROUP_MEMBER_CONSTRAINTS } from '@/lib/permission-groups/constraints'
1420
import { acquirePermissionGroupOrgLock } from '@/lib/permission-groups/locks'
1521
import { isOrganizationMember } from '@/lib/workspaces/permissions/utils'
1622
import {
17-
type AllMembersConflict,
1823
authorizeOrgAccessControl,
19-
findAllMembersWorkspaceConflict,
20-
findScopeConflicts,
2124
formatAllMembersConflictError,
2225
formatScopeConflictError,
2326
getGroupWorkspaces,
2427
loadGroupInOrganization,
25-
type ScopeConflict,
2628
} from '@/app/api/organizations/[id]/permission-groups/utils'
2729

2830
const logger = createLogger('OrganizationPermissionGroupMembers')

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

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,23 +10,25 @@ import { updatePermissionGroupContract } from '@/lib/api/contracts/permission-gr
1010
import { getValidationErrorMessage, parseRequest } from '@/lib/api/server'
1111
import { getSession } from '@/lib/auth'
1212
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
13+
import {
14+
type AllMembersConflict,
15+
findAllMembersWorkspaceConflict,
16+
findScopeConflicts,
17+
type ScopeConflict,
18+
} from '@/lib/permission-groups/application/group-membership'
1319
import { PERMISSION_GROUP_CONSTRAINTS } from '@/lib/permission-groups/constraints'
1420
import {
1521
type PermissionGroupConfig,
1622
parsePermissionGroupConfig,
1723
} from '@/lib/permission-groups/fields'
1824
import { acquirePermissionGroupOrgLock } from '@/lib/permission-groups/locks'
1925
import {
20-
type AllMembersConflict,
2126
authorizeOrgAccessControl,
22-
findAllMembersWorkspaceConflict,
23-
findScopeConflicts,
2427
findWorkspacesNotInOrganization,
2528
formatAllMembersConflictError,
2629
formatScopeConflictError,
2730
getGroupWorkspaces,
2831
loadGroupInOrganization,
29-
type ScopeConflict,
3032
} from '@/app/api/organizations/[id]/permission-groups/utils'
3133

3234
const logger = createLogger('OrganizationPermissionGroup')

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,10 @@ import { createPermissionGroupContract } from '@/lib/api/contracts/permission-gr
1515
import { getValidationErrorMessage, parseRequest } from '@/lib/api/server'
1616
import { getSession } from '@/lib/auth'
1717
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
18+
import {
19+
type AllMembersConflict,
20+
findAllMembersWorkspaceConflict,
21+
} from '@/lib/permission-groups/application/group-membership'
1822
import { PERMISSION_GROUP_CONSTRAINTS } from '@/lib/permission-groups/constraints'
1923
import {
2024
DEFAULT_PERMISSION_GROUP_CONFIG,
@@ -23,9 +27,7 @@ import {
2327
} from '@/lib/permission-groups/fields'
2428
import { acquirePermissionGroupOrgLock } from '@/lib/permission-groups/locks'
2529
import {
26-
type AllMembersConflict,
2730
authorizeOrgAccessControl,
28-
findAllMembersWorkspaceConflict,
2931
findWorkspacesNotInOrganization,
3032
formatAllMembersConflictError,
3133
getWorkspacesForGroups,
Lines changed: 2 additions & 115 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,7 @@
11
/**
22
* @vitest-environment node
33
*/
4-
import { permissionGroup, permissionGroupMember } from '@sim/db/schema'
5-
import { queueTableRows, resetDbChainMock } from '@sim/testing'
4+
import { resetDbChainMock } from '@sim/testing'
65
import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'
76

87
const { mockIsOrganizationAdminOrOwner, mockIsOrganizationOnEnterprisePlan } = vi.hoisted(() => ({
@@ -18,11 +17,7 @@ vi.mock('@/lib/workspaces/permissions/utils', () => ({
1817
isOrganizationAdminOrOwner: mockIsOrganizationAdminOrOwner,
1918
}))
2019

21-
import {
22-
authorizeOrgAccessControl,
23-
findAllMembersWorkspaceConflict,
24-
findScopeConflicts,
25-
} from '@/app/api/organizations/[id]/permission-groups/utils'
20+
import { authorizeOrgAccessControl } from '@/app/api/organizations/[id]/permission-groups/utils'
2621

2722
afterAll(resetDbChainMock)
2823

@@ -66,111 +61,3 @@ describe('authorizeOrgAccessControl', () => {
6661
expect(response).toBeNull()
6762
})
6863
})
69-
70-
describe('findScopeConflicts', () => {
71-
beforeEach(() => {
72-
vi.clearAllMocks()
73-
resetDbChainMock()
74-
})
75-
76-
const baseParams = {
77-
organizationId: 'org-1',
78-
excludeGroupId: 'group-1',
79-
workspaceIds: ['ws-1'],
80-
candidateUserIds: ['user-1'],
81-
}
82-
83-
const conflictRow = (userId: string, otherGroupName = 'Marketing') => ({
84-
userId,
85-
userName: 'User One',
86-
userEmail: `${userId}@example.com`,
87-
otherGroupId: 'group-2',
88-
otherGroupName,
89-
})
90-
91-
it('returns no conflicts when there are no candidate users', async () => {
92-
queueTableRows(permissionGroupMember, [conflictRow('user-1')])
93-
94-
const conflicts = await findScopeConflicts({ ...baseParams, candidateUserIds: [] })
95-
96-
expect(conflicts).toEqual([])
97-
})
98-
99-
it('returns no conflicts when there are no target workspaces', async () => {
100-
queueTableRows(permissionGroupMember, [conflictRow('user-1')])
101-
102-
const conflicts = await findScopeConflicts({ ...baseParams, workspaceIds: [] })
103-
104-
expect(conflicts).toEqual([])
105-
})
106-
107-
it('flags a candidate already in another group that shares a workspace', async () => {
108-
queueTableRows(permissionGroupMember, [conflictRow('user-1')])
109-
110-
const conflicts = await findScopeConflicts(baseParams)
111-
112-
expect(conflicts.map((c) => c.userId)).toEqual(['user-1'])
113-
expect(conflicts[0].conflictingGroupName).toBe('Marketing')
114-
})
115-
116-
it('returns at most one conflict per user', async () => {
117-
queueTableRows(permissionGroupMember, [
118-
conflictRow('user-1', 'Marketing'),
119-
conflictRow('user-1', 'Sales'),
120-
])
121-
122-
const conflicts = await findScopeConflicts(baseParams)
123-
124-
expect(conflicts).toHaveLength(1)
125-
expect(conflicts[0].conflictingGroupName).toBe('Marketing')
126-
})
127-
128-
it('returns no conflicts when the query finds no overlapping memberships', async () => {
129-
const conflicts = await findScopeConflicts(baseParams)
130-
131-
expect(conflicts).toEqual([])
132-
})
133-
})
134-
135-
describe('findAllMembersWorkspaceConflict', () => {
136-
beforeEach(() => {
137-
vi.clearAllMocks()
138-
resetDbChainMock()
139-
})
140-
141-
const baseParams = {
142-
organizationId: 'org-1',
143-
excludeGroupId: 'group-1',
144-
workspaceIds: ['ws-1', 'ws-2'],
145-
}
146-
147-
it('returns null when there are no target workspaces', async () => {
148-
queueTableRows(permissionGroup, [
149-
{ conflictingGroupId: 'group-2', conflictingGroupName: 'Marketing', workspaceName: 'Acme' },
150-
])
151-
152-
const conflict = await findAllMembersWorkspaceConflict({ ...baseParams, workspaceIds: [] })
153-
154-
expect(conflict).toBeNull()
155-
})
156-
157-
it('returns the conflicting all-members group sharing a workspace', async () => {
158-
queueTableRows(permissionGroup, [
159-
{ conflictingGroupId: 'group-2', conflictingGroupName: 'Marketing', workspaceName: 'Acme' },
160-
])
161-
162-
const conflict = await findAllMembersWorkspaceConflict(baseParams)
163-
164-
expect(conflict).toEqual({
165-
conflictingGroupId: 'group-2',
166-
conflictingGroupName: 'Marketing',
167-
workspaceName: 'Acme',
168-
})
169-
})
170-
171-
it('returns null when no other all-members group targets the workspaces', async () => {
172-
const conflict = await findAllMembersWorkspaceConflict(baseParams)
173-
174-
expect(conflict).toBeNull()
175-
})
176-
})

0 commit comments

Comments
 (0)