Skip to content

Commit 8c1efe3

Browse files
committed
fix(scim): judge directory stops against the pass's final access
- A reconciliation pass that demotes an admin or withdraws a grant while granting the same workspace back no longer stops the run there: access is checked once after every withdrawal and grant - Fix the readonly skipped result that failed type-check - Cover a committed batch delivering when a later batch rolls back, and observe rollback deliveries deterministically
1 parent cca2156 commit 8c1efe3

5 files changed

Lines changed: 191 additions & 84 deletions

File tree

‎apps/sim/ee/scim/lib/projection/reconcile-user.test.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ const hoistedMocks = vi.hoisted(() => ({
2323
lowerWorkspace: vi.fn(),
2424
readPermission: vi.fn(),
2525
revokeWorkspace: vi.fn(),
26+
stopRuns: vi.fn(),
2627
}))
2728

2829
vi.mock('@/lib/billing/organizations/membership', () => organizationMembershipMock)
@@ -42,6 +43,7 @@ vi.mock('@/lib/workspaces/access/workspace-access', () => ({
4243
lowerWorkspaceAccessTx: hoistedMocks.lowerWorkspace,
4344
readWorkspacePermission: hoistedMocks.readPermission,
4445
revokeWorkspaceAccessTx: hoistedMocks.revokeWorkspace,
46+
stopRunsWithoutWorkspaceAccessTx: hoistedMocks.stopRuns,
4547
permissionRank: (permission: string) => ({ read: 1, write: 2, admin: 3 })[permission] ?? 0,
4648
}))
4749

@@ -122,6 +124,7 @@ describe('reconcileUserProjection', () => {
122124
ownershipTransferred: false,
123125
stoppedRuns: [],
124126
})
127+
mocks.stopRuns.mockResolvedValue([])
125128
mocks.addMember.mockResolvedValue('added')
126129
mocks.removeMember.mockResolvedValue('removed')
127130
mocks.changeMemberRole.mockResolvedValue({
@@ -207,6 +210,7 @@ describe('reconcileUserProjection', () => {
207210
organizationId: 'org-1',
208211
userId: 'u-1',
209212
role: 'admin',
213+
deferRunStops: true,
210214
})
211215
})
212216

‎apps/sim/ee/scim/lib/projection/reconcile-user.ts‎

Lines changed: 37 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ import {
3333
permissionRank,
3434
readWorkspacePermission,
3535
revokeWorkspaceAccessTx,
36+
stopRunsWithoutWorkspaceAccessTx,
3637
} from '@/lib/workspaces/access/workspace-access'
3738
import {
3839
type MappingRow,
@@ -254,9 +255,7 @@ async function applyGrant(
254255
}
255256
case 'org_role': {
256257
if (grant.targetId !== 'admin') return 'skipped'
257-
/** A promotion ends no access, so it stops no runs. */
258-
const change = await setOrganizationRole(tx, params.organizationId, params.userId, 'admin')
259-
return change.outcome
258+
return setOrganizationRole(tx, params.organizationId, params.userId, 'admin')
260259
}
261260
case 'permission_group': {
262261
const outcome = await addPermissionGroupMemberTx(tx, {
@@ -282,33 +281,36 @@ async function setOrganizationRole(
282281
organizationId: string,
283282
userId: string,
284283
role: 'admin' | 'member'
285-
): Promise<{ outcome: GrantOutcome; stoppedRuns: StoppedRun[] }> {
286-
const skipped = { outcome: 'skipped', stoppedRuns: [] } as const
284+
): Promise<GrantOutcome> {
287285
try {
288-
const change = await changeMemberRoleTx(tx, { organizationId, userId, role })
289-
return change.changed
290-
? { outcome: 'applied', stoppedRuns: change.stoppedRuns }
291-
: { outcome: 'unchanged', stoppedRuns: [] }
286+
const change = await changeMemberRoleTx(tx, {
287+
organizationId,
288+
userId,
289+
role,
290+
deferRunStops: true,
291+
})
292+
return change.changed ? 'applied' : 'unchanged'
292293
} catch (error) {
293294
if (error instanceof OrchestrationError && error.code === 'conflict') {
294295
logger.warn('Skipped an organization role mapping for the owner', { organizationId, userId })
295-
return skipped
296+
return 'skipped'
296297
}
297298
if (error instanceof OrchestrationError && error.code === 'not_found') {
298299
logger.warn('Skipped an organization role mapping for a user who is no longer a member', {
299300
organizationId,
300301
userId,
301302
})
302-
return skipped
303+
return 'skipped'
303304
}
304305
throw error
305306
}
306307
}
307308

308309
/**
309-
* Withdraws one grant, returning the runs it stopped. Returns null when the
310-
* grant must stay in place — the access could not be handed on — so the
311-
* provenance row survives and the next pass retries instead of forgetting.
310+
* Withdraws one grant, returning the workspaces where it may have ended the
311+
* user's access. Returns null when the grant must stay in place — the access
312+
* could not be handed on — so the provenance row survives and the next pass
313+
* retries instead of forgetting.
312314
*
313315
* Adopted access — held by hand before any mapping covered it — is the person's
314316
* own, so the directory only forgets its record of it, unless the organization
@@ -323,7 +325,7 @@ async function withdrawGrant(
323325
lockManualMembership: boolean
324326
foreignWorkspaceIds: Set<string>
325327
}
326-
): Promise<StoppedRun[] | null> {
328+
): Promise<string[] | null> {
327329
const { grant } = params
328330
if (grant.origin === 'adopted' && !params.lockManualMembership) return []
329331
switch (grant.targetKind) {
@@ -362,6 +364,7 @@ async function withdrawGrant(
362364
const outcome = await revokeWorkspaceAccessTx(tx, {
363365
workspaceId: grant.targetId,
364366
userId: params.userId,
367+
deferRunStops: true,
365368
})
366369
if (!outcome.revoked) {
367370
logger.warn('Left workspace access in place: ownership could not be handed on', {
@@ -371,12 +374,18 @@ async function withdrawGrant(
371374
})
372375
return null
373376
}
374-
return outcome.stoppedRuns
377+
return [grant.targetId]
375378
}
376379
case 'org_role': {
377380
if (grant.targetId !== 'admin') return []
378-
const change = await setOrganizationRole(tx, params.organizationId, params.userId, 'member')
379-
return change.stoppedRuns
381+
const demoted = await setOrganizationRole(tx, params.organizationId, params.userId, 'member')
382+
if (demoted !== 'applied') return []
383+
/** An organization admin reaches every organization workspace; the demotion may end any of them. */
384+
const workspaces = await tx
385+
.select({ id: workspace.id })
386+
.from(workspace)
387+
.where(eq(workspace.organizationId, params.organizationId))
388+
return workspaces.map(({ id }) => id)
380389
}
381390
case 'permission_group':
382391
try {
@@ -474,18 +483,19 @@ export async function reconcileUserProjection(
474483
const plan = planGrantChanges(desired, current, actual)
475484

476485
const delta = emptyDelta()
486+
const accessEndedIn = new Set<string>()
477487
const lockManualMembership = params.settings.lockManualMembership === true
478488

479489
/** Withdrawals first, so a move between groups frees its workspace slot. */
480490
for (const grant of plan.withdraw) {
481-
const stoppedRuns = await withdrawGrant(tx, {
491+
const accessEnded = await withdrawGrant(tx, {
482492
organizationId: params.organizationId,
483493
userId: record.userId,
484494
grant,
485495
lockManualMembership,
486496
foreignWorkspaceIds,
487497
})
488-
if (!stoppedRuns) continue
498+
if (!accessEnded) continue
489499
await tx
490500
.delete(scimProjectionGrant)
491501
.where(
@@ -496,7 +506,7 @@ export async function reconcileUserProjection(
496506
)
497507
)
498508
delta.removed.push(grant)
499-
delta.stoppedRuns.push(...stoppedRuns)
509+
for (const workspaceId of accessEnded) accessEndedIn.add(workspaceId)
500510
}
501511

502512
for (const { grant, previousGrant } of plan.apply) {
@@ -595,6 +605,12 @@ export async function reconcileUserProjection(
595605
else delta.added.push(grant)
596606
}
597607

608+
/**
609+
* Runs are judged once every withdrawal and grant above is in place: a run in
610+
* a workspace this pass takes away and gives back, by grant or by role, keeps
611+
* going.
612+
*/
613+
delta.stoppedRuns = await stopRunsWithoutWorkspaceAccessTx(tx, record.userId, [...accessEndedIn])
598614
return delta
599615
}
600616

0 commit comments

Comments
 (0)