Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ export const POST = withRouteHandler(
memberId: selfMember.id,
spareSessionToken: session.session.token,
})
deliverRunStops(removeResult.stoppedRuns ?? [])
deliverRunStops(removeResult.stoppedRuns)

if (!removeResult.success) {
logger.error('Transfer succeeded but self-removal failed', {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ export const DELETE = withRouteHandler(
memberId,
skipBillingLogic,
})
deliverRunStops(result.stoppedRuns ?? [])
deliverRunStops(result.stoppedRuns)

if (!result.success) {
if (result.error === 'Cannot remove organization owner') {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ describe('replaceScimGroup', () => {
memberIds.delete(scimUserId)
)
mocks.countMembers.mockImplementation(async () => memberIds.size)
mocks.reconcile.mockResolvedValue({ added: [], removed: [], raised: [], stoppedRuns: [] })
mocks.touch.mockImplementation(async () => {
group = { ...group, updatedAt: new Date('2026-01-02T00:00:00.000Z') }
})
Expand Down
25 changes: 17 additions & 8 deletions apps/sim/ee/scim/lib/application/groups/manage-groups.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import { and, eq, ne } from 'drizzle-orm'
import type { ScimPatchOperation } from '@/lib/api/contracts/scim'
import { acquireOrganizationMutationLock } from '@/lib/billing/organizations/membership'
import type { DbOrTx, DbTransaction } from '@/lib/db/types'
import type { StoppedRun } from '@/lib/mothership/async-runs/repository'
import { deliverRunStops } from '@/lib/mothership/request/application/stop-delivery'
import {
defineAuthorizedScimUseCase,
type ScimUseCaseArgs,
Expand Down Expand Up @@ -50,16 +52,19 @@ import {
/**
* Every group write runs in one transaction under the organization lock, which
* heads the documented lock order, so two full-membership PATCHes cannot both
* compute from the same stale membership and keep members from both.
* compute from the same stale membership and keep members from both. The runs
* its projection stopped are delivered once that transaction commits.
*/
function withGroupWrite<T>(
async function withGroupWrite<T extends { stoppedRuns: StoppedRun[] }>(
context: ScimUseCaseContext,
work: (tx: DbTransaction) => Promise<T>
): Promise<T> {
return db.transaction(async (tx) => {
): Promise<Omit<T, 'stoppedRuns'>> {
const { stoppedRuns, ...result } = await db.transaction(async (tx) => {
await acquireOrganizationMutationLock(tx, context.organizationId)
return work(tx)
})
deliverRunStops(stoppedRuns)
return result
}

async function assertDisplayNameAvailable(
Expand Down Expand Up @@ -205,7 +210,7 @@ export const createScimGroup = defineAuthorizedScimUseCase({
})
: 'no-match'

await reconcileUsersProjection(tx, {
const projection = await reconcileUsersProjection(tx, {
connectionId: context.connection.id,
organizationId: context.organizationId,
scimUserIds: memberIds,
Expand All @@ -225,6 +230,7 @@ export const createScimGroup = defineAuthorizedScimUseCase({
resource: toGroupResource({ ...created, members }, context.baseUrl),
touchedUserIds: memberIds,
renamed: true,
stoppedRuns: projection.stoppedRuns,
}
})
},
Expand Down Expand Up @@ -295,7 +301,7 @@ export const replaceScimGroup = defineAuthorizedScimUseCase({
await assertMemberCount(tx, current.id)
if (touched.size > 0) await touchScimGroup(tx, current.id)

await reconcileUsersProjection(tx, {
const projection = await reconcileUsersProjection(tx, {
connectionId: context.connection.id,
organizationId: context.organizationId,
scimUserIds: [...touched],
Expand All @@ -317,6 +323,7 @@ export const replaceScimGroup = defineAuthorizedScimUseCase({
resource: toGroupResource({ ...refreshed, members }, context.baseUrl),
touchedUserIds: [...touched],
renamed: renamed || (current.externalId ?? null) !== (refreshed.externalId ?? null),
stoppedRuns: projection.stoppedRuns,
}
})
},
Expand Down Expand Up @@ -428,7 +435,7 @@ export const patchScimGroup = defineAuthorizedScimUseCase({
await assertMemberCount(tx, current.id)
if (touched.size > 0) await touchScimGroup(tx, current.id)

await reconcileUsersProjection(tx, {
const projection = await reconcileUsersProjection(tx, {
connectionId: context.connection.id,
organizationId: context.organizationId,
scimUserIds: [...touched],
Expand All @@ -447,6 +454,7 @@ export const patchScimGroup = defineAuthorizedScimUseCase({
patch.kind === 'full' ? (patch.displayName ?? current.displayName) : current.displayName,
touchedUserIds: [...touched],
renamed,
stoppedRuns: projection.stoppedRuns,
}
})
},
Expand Down Expand Up @@ -481,7 +489,7 @@ export const deleteScimGroup = defineAuthorizedScimUseCase({
*/
const memberIds = await loadGroupMemberIds(tx, current.id)
await deleteScimGroupRow(tx, current.id)
await reconcileUsersProjection(tx, {
const projection = await reconcileUsersProjection(tx, {
connectionId: context.connection.id,
organizationId: context.organizationId,
scimUserIds: memberIds,
Expand All @@ -491,6 +499,7 @@ export const deleteScimGroup = defineAuthorizedScimUseCase({
groupId: current.id,
displayName: current.displayName,
memberCount: memberIds.length,
stoppedRuns: projection.stoppedRuns,
}
})
},
Expand Down
2 changes: 1 addition & 1 deletion apps/sim/ee/scim/lib/application/users/deprovision-user.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ export const deprovisionScimUser = defineAuthorizedScimUseCase({
memberId: membership.id,
revokePersonalApiKeys: true,
})
deliverRunStops(removal.stoppedRuns ?? [])
deliverRunStops(removal.stoppedRuns)
if (!removal.success) {
throw new ScimError(409, undefined, removal.error ?? 'The member could not be removed')
}
Expand Down
1 change: 1 addition & 0 deletions apps/sim/ee/scim/lib/application/users/provision-user.ts
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,7 @@ export const provisionScimUser = defineAuthorizedScimUseCase({
connectionId: context.connection.id,
externalId: attributes.externalId,
})
/** The new directory record has no provenance to withdraw, so this stops no runs. */
await reconcileUserProjection(tx, {
connectionId: context.connection.id,
organizationId: context.organizationId,
Expand Down
2 changes: 1 addition & 1 deletion apps/sim/ee/scim/lib/application/users/update-user.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ describe('user updates', () => {
beforeEach(() => {
resetDbChainMock()
mocks.loadGroups.mockResolvedValue(new Map())
mocks.reconcile.mockResolvedValue({ added: [], removed: [], raised: [] })
mocks.reconcile.mockResolvedValue({ added: [], removed: [], raised: [], stoppedRuns: [] })
})

it('serializes on the organization and user locks before computing the update', async () => {
Expand Down
75 changes: 45 additions & 30 deletions apps/sim/ee/scim/lib/application/users/update-user.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import { normalizeEmail } from '@sim/utils/string'
import type { ScimPatchOperation } from '@/lib/api/contracts/scim'
import { acquireOrganizationUserMutationLocks } from '@/lib/billing/organizations/membership'
import type { DbOrTx, DbTransaction } from '@/lib/db/types'
import type { StoppedRun } from '@/lib/mothership/async-runs/repository'
import { deliverRunStops } from '@/lib/mothership/request/application/stop-delivery'
import { suspendMemberTx, unsuspendMemberTx } from '@/lib/organizations/members/lifecycle'
import {
invalidateAfterSessionRevocation,
Expand Down Expand Up @@ -42,18 +44,24 @@ import {
* deactivation actually does.
*/

export interface UpdateOutcome {
interface UpdateOutcome {
emailChanged: boolean
deactivated: boolean
reactivated: boolean
}

/** An applied update and the runs its projection stopped, delivered once the write commits. */
interface AppliedUserUpdate {
outcome: UpdateOutcome
stoppedRuns: StoppedRun[]
}

async function applyUserUpdate(
tx: DbTransaction,
context: ScimUseCaseContext,
current: ScimUserRecord,
next: ScimUserAttributes
): Promise<UpdateOutcome> {
): Promise<AppliedUserUpdate> {
const nextEmail = primaryEmail(next)
const nextName = accountName(next)
const emailChanged = accountEmailDiverged(current, next)
Expand Down Expand Up @@ -108,14 +116,17 @@ async function applyUserUpdate(
active: next.active,
})

await reconcileUserProjection(tx, {
const projection = await reconcileUserProjection(tx, {
connectionId: context.connection.id,
organizationId: context.organizationId,
scimUserId: current.id,
settings: context.connection.settings,
})

return { emailChanged, deactivated, reactivated }
return {
outcome: { emailChanged, deactivated, reactivated },
stoppedRuns: projection.stoppedRuns,
}
}

export interface UpdateScimUserResult {
Expand All @@ -125,6 +136,27 @@ export interface UpdateScimUserResult {
resource: ReturnType<typeof toUserResource>
}

/** Runs a user write in one transaction and delivers the runs it stopped once that commits. */
async function writeUser(
context: ScimUseCaseContext,
scimUserId: string,
update: (tx: DbTransaction, current: ScimUserRecord) => Promise<AppliedUserUpdate | null>
): Promise<UpdateScimUserResult> {
const { stoppedRuns, ...result } = await db.transaction(async (tx) => {
const current = await loadUserForUpdate(tx, context, scimUserId)
const applied = await update(tx, current)
return {
scimUserId: current.id,
userId: current.userId,
outcome: applied?.outcome ?? null,
resource: await renderUpdated(tx, context.connection.id, current.id, context.baseUrl),
stoppedRuns: applied?.stoppedRuns ?? [],
}
})
deliverRunStops(stoppedRuns)
return result
}

/**
* Loads the stored resource under the organization and user advisory locks.
*
Expand Down Expand Up @@ -229,9 +261,7 @@ export const replaceScimUser = defineAuthorizedScimUseCase({
input,
context,
}: ScimUseCaseArgs<ReplaceScimUserInput>): Promise<UpdateScimUserResult> {
return db.transaction(async (tx) => {
const current = await loadUserForUpdate(tx, context, input.scimUserId)

return writeUser(context, input.scimUserId, async (tx, current) => {
/**
* A replace keeps attributes Sim does not model that the directory sent on
* a previous write but omitted now, so a partial mapping does not erase
Expand All @@ -249,16 +279,10 @@ export const replaceScimUser = defineAuthorizedScimUseCase({
* changes nothing must not write, audit, or re-project, or a 2,000-user
* organization produces 2,000 spurious audit rows per sync.
*/
const outcome =
userAttributesEqual(current.attributes, next) && !accountIdentityDiverged(current, next)
? null
: await applyUserUpdate(tx, context, current, next)
return {
scimUserId: current.id,
userId: current.userId,
outcome,
resource: await renderUpdated(tx, context.connection.id, current.id, context.baseUrl),
}
return userAttributesEqual(current.attributes, next) &&
!accountIdentityDiverged(current, next)
? null
: applyUserUpdate(tx, context, current, next)
})
},
projectAudit: ({ result }) => auditEntries(result),
Expand All @@ -277,9 +301,7 @@ export const patchScimUser = defineAuthorizedScimUseCase({
input,
context,
}: ScimUseCaseArgs<PatchScimUserInput>): Promise<UpdateScimUserResult> {
return db.transaction(async (tx) => {
const current = await loadUserForUpdate(tx, context, input.scimUserId)

return writeUser(context, input.scimUserId, async (tx, current) => {
const { next, changed } = applyUserPatch(current.attributes, input.operations)

/**
Expand All @@ -289,16 +311,9 @@ export const patchScimUser = defineAuthorizedScimUseCase({
* projection pass, and a `lastModified` bump for a request that meant
* nothing.
*/
const outcome =
changed || accountIdentityDiverged(current, next)
? await applyUserUpdate(tx, context, current, next)
: null
return {
scimUserId: current.id,
userId: current.userId,
outcome,
resource: await renderUpdated(tx, context.connection.id, current.id, context.baseUrl),
}
return changed || accountIdentityDiverged(current, next)
? applyUserUpdate(tx, context, current, next)
: null
})
},
projectAudit: ({ result }) => auditEntries(result),
Expand Down
4 changes: 4 additions & 0 deletions apps/sim/ee/scim/lib/projection/reconcile-user.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ const hoistedMocks = vi.hoisted(() => ({
lowerWorkspace: vi.fn(),
readPermission: vi.fn(),
revokeWorkspace: vi.fn(),
stopRuns: vi.fn(),
}))

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

Expand Down Expand Up @@ -122,6 +124,7 @@ describe('reconcileUserProjection', () => {
ownershipTransferred: false,
stoppedRuns: [],
})
mocks.stopRuns.mockResolvedValue([])
mocks.addMember.mockResolvedValue('added')
mocks.removeMember.mockResolvedValue('removed')
mocks.changeMemberRole.mockResolvedValue({
Expand Down Expand Up @@ -207,6 +210,7 @@ describe('reconcileUserProjection', () => {
organizationId: 'org-1',
userId: 'u-1',
role: 'admin',
deferRunStops: true,
})
})

Expand Down
Loading
Loading