From 0e6b5b43067ba8ec9f0e187dfd9de2cecd53fcdf Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Sat, 1 Aug 2026 18:32:10 -0700 Subject: [PATCH 1/2] improvement(permissions): confine workspace role changes to existing members --- .../organizations/[id]/roster/route.test.ts | 4 + .../api/organizations/[id]/roster/route.ts | 34 +- .../[id]/members/[memberId]/route.ts | 13 +- .../v1/admin/workspaces/[id]/members/route.ts | 59 +- .../workspaces/[id]/permissions/route.test.ts | 620 +++++++++++++++++ .../api/workspaces/[id]/permissions/route.ts | 630 ++++++++++++++---- .../organization-member-lists.tsx | 15 +- .../components/teammates/teammates.tsx | 6 +- apps/sim/components/permissions/role-lock.tsx | 12 +- apps/sim/hooks/queries/invitations.ts | 21 + apps/sim/lib/api/contracts/organization.ts | 7 + apps/sim/lib/api/contracts/workspaces.ts | 54 +- apps/sim/lib/db/transaction.ts | 56 ++ .../lib/workspaces/permissions/utils.test.ts | 1 + apps/sim/lib/workspaces/permissions/utils.ts | 7 + 15 files changed, 1353 insertions(+), 186 deletions(-) create mode 100644 apps/sim/app/api/workspaces/[id]/permissions/route.test.ts create mode 100644 apps/sim/lib/db/transaction.ts diff --git a/apps/sim/app/api/organizations/[id]/roster/route.test.ts b/apps/sim/app/api/organizations/[id]/roster/route.test.ts index ac698fe3606..eabd3cf5350 100644 --- a/apps/sim/app/api/organizations/[id]/roster/route.test.ts +++ b/apps/sim/app/api/organizations/[id]/roster/route.test.ts @@ -174,6 +174,8 @@ describe('GET /api/organizations/[id]/roster', () => { workspaceId: 'workspace-1', workspaceName: 'Workspace One', permission: 'admin', + roleSource: 'org-admin', + isBilledAccount: false, }, ], }), @@ -193,6 +195,8 @@ describe('GET /api/organizations/[id]/roster', () => { workspaceId: 'workspace-1', workspaceName: 'Workspace One', permission: 'read', + roleSource: 'explicit', + isBilledAccount: false, }, ], }), diff --git a/apps/sim/app/api/organizations/[id]/roster/route.ts b/apps/sim/app/api/organizations/[id]/roster/route.ts index f27d03b591a..17bdf76153e 100644 --- a/apps/sim/app/api/organizations/[id]/roster/route.ts +++ b/apps/sim/app/api/organizations/[id]/roster/route.ts @@ -89,12 +89,17 @@ export const GET = withRouteHandler( await expireStalePendingInvitationsForOrganization(organizationId) const orgWorkspaces = await db - .select({ id: workspace.id, name: workspace.name }) + .select({ + id: workspace.id, + name: workspace.name, + ownerId: workspace.ownerId, + billedAccountUserId: workspace.billedAccountUserId, + }) .from(workspace) .where(and(eq(workspace.organizationId, organizationId), isNull(workspace.archivedAt))) const orgWorkspaceIds = orgWorkspaces.map((ws) => ws.id) - const workspaceNameById = new Map(orgWorkspaces.map((ws) => [ws.id, ws.name])) + const workspaceById = new Map(orgWorkspaces.map((ws) => [ws.id, ws])) const memberUserIds = memberRows.map((row) => row.userId) const memberPermissions = @@ -117,11 +122,14 @@ export const GET = withRouteHandler( const permissionsByUser = new Map() for (const row of memberPermissions) { + const ws = workspaceById.get(row.workspaceId) const list = permissionsByUser.get(row.userId) ?? [] list.push({ workspaceId: row.workspaceId, - workspaceName: workspaceNameById.get(row.workspaceId) ?? 'Workspace', + workspaceName: ws?.name ?? 'Workspace', permission: row.permission, + roleSource: ws?.ownerId === row.userId ? 'owner' : 'explicit', + isBilledAccount: ws?.billedAccountUserId === row.userId, }) permissionsByUser.set(row.userId, list) } @@ -135,6 +143,14 @@ export const GET = withRouteHandler( workspaceId: ws.id, workspaceName: ws.name, permission: 'admin' as const, + /** + * Owner wins over the derived organization grant, matching + * `getUsersWithPermissions` — otherwise the same person reads as + * `owner` in the teammates list and `org-admin` here. + */ + roleSource: + ws.ownerId === rosterMember.userId ? ('owner' as const) : ('org-admin' as const), + isBilledAccount: ws.billedAccountUserId === rosterMember.userId, })) : (permissionsByUser.get(rosterMember.userId) ?? []), } @@ -183,10 +199,13 @@ export const GET = withRouteHandler( for (const row of externalPermissionRows) { const existing = externalMembersByUser.get(row.userId) + const externalWorkspace = workspaceById.get(row.workspaceId) const workspaceAccess: RosterWorkspaceAccess = { workspaceId: row.workspaceId, - workspaceName: workspaceNameById.get(row.workspaceId) ?? 'Workspace', + workspaceName: externalWorkspace?.name ?? 'Workspace', permission: row.permission, + roleSource: externalWorkspace?.ownerId === row.userId ? 'owner' : 'explicit', + isBilledAccount: externalWorkspace?.billedAccountUserId === row.userId, } if (existing) { @@ -247,8 +266,11 @@ export const GET = withRouteHandler( const list = grantsByInvitation.get(row.invitationId) ?? [] list.push({ workspaceId: row.workspaceId, - workspaceName: workspaceNameById.get(row.workspaceId) ?? 'Workspace', + workspaceName: workspaceById.get(row.workspaceId)?.name ?? 'Workspace', permission: row.permission, + /** A pending invitee holds no row yet, so nothing is inherited. */ + roleSource: 'explicit', + isBilledAccount: false, }) grantsByInvitation.set(row.invitationId, list) } @@ -269,7 +291,7 @@ export const GET = withRouteHandler( const data = { members: rosterMembers, pendingInvitations, - workspaces: orgWorkspaces, + workspaces: orgWorkspaces.map((ws) => ({ id: ws.id, name: ws.name })), } satisfies OrganizationRoster return NextResponse.json({ success: true, diff --git a/apps/sim/app/api/v1/admin/workspaces/[id]/members/[memberId]/route.ts b/apps/sim/app/api/v1/admin/workspaces/[id]/members/[memberId]/route.ts index bcc8df9dcfb..be48377a5ca 100644 --- a/apps/sim/app/api/v1/admin/workspaces/[id]/members/[memberId]/route.ts +++ b/apps/sim/app/api/v1/admin/workspaces/[id]/members/[memberId]/route.ts @@ -44,6 +44,7 @@ import { import { withAdminAuthParams } from '@/app/api/v1/admin/middleware' import { badRequestResponse, + conflictResponse, internalErrorResponse, notFoundResponse, singleResponse, @@ -170,10 +171,20 @@ export const PATCH = withRouteHandler( const now = new Date() - await db + /** + * Conditional on the row read above still existing: a concurrent removal + * between that read and this write would otherwise match nothing and be + * reported to the caller as a successful update. + */ + const updated = await db .update(permissions) .set({ permissionType: permissionLevel, updatedAt: now }) .where(eq(permissions.id, memberId)) + .returning({ id: permissions.id }) + + if (updated.length === 0) { + return conflictResponse('Workspace member changed during the update. Retry.') + } const [userData] = await db .select({ name: user.name, email: user.email, image: user.image }) diff --git a/apps/sim/app/api/v1/admin/workspaces/[id]/members/route.ts b/apps/sim/app/api/v1/admin/workspaces/[id]/members/route.ts index 85523cd6c11..c113359e5b3 100644 --- a/apps/sim/app/api/v1/admin/workspaces/[id]/members/route.ts +++ b/apps/sim/app/api/v1/admin/workspaces/[id]/members/route.ts @@ -9,6 +9,12 @@ * * Response: AdminListResponse * + * `createdAt` is the member's join time. It previously moved on every role + * change, because the in-app role-change endpoint replaced the permission row + * rather than amending it; that endpoint now updates in place, so only + * `updatedAt` tracks role changes. Consumers that diffed `createdAt` to detect + * recently-changed members must read `updatedAt` instead. + * * POST /api/v1/admin/workspaces/[id]/members * * Add a user to a workspace with a specific permission level. @@ -55,6 +61,7 @@ import { import { withAdminAuthParams } from '@/app/api/v1/admin/middleware' import { badRequestResponse, + conflictResponse, internalErrorResponse, listResponse, notFoundResponse, @@ -191,10 +198,20 @@ export const POST = withRouteHandler( if (existingPermission) { if (existingPermission.permissionType !== permissionLevel) { const now = new Date() - await db + /** + * Conditional on the row read above still existing: a concurrent + * removal between that read and this write would otherwise match + * nothing and be reported to the caller as a successful update. + */ + const updated = await db .update(permissions) .set({ permissionType: permissionLevel, updatedAt: now }) .where(eq(permissions.id, existingPermission.id)) + .returning({ id: permissions.id }) + + if (updated.length === 0) { + return conflictResponse('Workspace member changed during the update. Retry.') + } logger.info(`Admin API: Updated user ${userId} permissions in workspace ${workspaceId}`, { previousPermissions: existingPermission.permissionType, @@ -247,15 +264,31 @@ export const POST = withRouteHandler( const now = new Date() const permissionId = generateId() - await db.insert(permissions).values({ - id: permissionId, - userId, - entityType: 'workspace', - entityId: workspaceId, - permissionType: permissionLevel, - createdAt: now, - updatedAt: now, - }) + /** + * The existence read above is unlocked, so two concurrent adds for the + * same user both reach here. Conflicting on the uniqueness constraint + * settles it as the requested role instead of failing the loser with a + * 500 for a request that did what it asked. + */ + const [written] = await db + .insert(permissions) + .values({ + id: permissionId, + userId, + entityType: 'workspace', + entityId: workspaceId, + permissionType: permissionLevel, + createdAt: now, + updatedAt: now, + }) + .onConflictDoUpdate({ + target: [permissions.userId, permissions.entityType, permissions.entityId], + set: { permissionType: permissionLevel, updatedAt: now }, + }) + .returning({ id: permissions.id, createdAt: permissions.createdAt }) + + /** A returned id we did not mint means the conflict branch ran. */ + const wasCreated = written?.id === permissionId logger.info(`Admin API: Added user ${userId} to workspace ${workspaceId}`, { permissions: permissionLevel, @@ -288,16 +321,16 @@ export const POST = withRouteHandler( } return singleResponse({ - id: permissionId, + id: written?.id ?? permissionId, workspaceId, userId, permissions: permissionLevel, - createdAt: now.toISOString(), + createdAt: (written?.createdAt ?? now).toISOString(), updatedAt: now.toISOString(), userName: userData.name, userEmail: userData.email, userImage: userData.image, - action: 'created' as const, + action: wasCreated ? ('created' as const) : ('updated' as const), }) } catch (error) { logger.error('Admin API: Failed to add workspace member', { error, workspaceId }) diff --git a/apps/sim/app/api/workspaces/[id]/permissions/route.test.ts b/apps/sim/app/api/workspaces/[id]/permissions/route.test.ts new file mode 100644 index 00000000000..c4ad6368788 --- /dev/null +++ b/apps/sim/app/api/workspaces/[id]/permissions/route.test.ts @@ -0,0 +1,620 @@ +/** + * @vitest-environment node + */ +import { + auditMock, + auditMockFns, + authMockFns, + createMockRequest, + dbChainMockFns, + hasMockCondition, + permissionsMock, + permissionsMockFns, + queueTableRows, + resetDbChainMock, + schemaMock, +} from '@sim/testing' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { mockSyncWorkspaceEnvCredentials, mockGetEffectiveWorkspacePermission } = vi.hoisted(() => ({ + mockSyncWorkspaceEnvCredentials: vi.fn(), + mockGetEffectiveWorkspacePermission: vi.fn(), +})) + +vi.mock('@sim/audit', () => auditMock) + +vi.mock('@/lib/posthog/server', () => ({ + captureServerEvent: vi.fn(), +})) + +vi.mock('@/lib/credentials/environment', () => ({ + syncWorkspaceEnvCredentials: mockSyncWorkspaceEnvCredentials, +})) + +vi.mock('@/lib/workspaces/permissions/utils', () => ({ + ...permissionsMock, + getWorkspacePermissionsForViewer: vi.fn(), + getEffectiveWorkspacePermission: mockGetEffectiveWorkspacePermission, +})) + +import { PATCH } from '@/app/api/workspaces/[id]/permissions/route' + +const mockGetSession = authMockFns.mockGetSession + +const WORKSPACE_ID = 'workspace-1' +const ADMIN_ID = 'user-admin' +const MEMBER_ID = 'user-member' +const OUTSIDER_ID = 'user-outsider' +/** Distinct from the session user so the billing guard is provable on its own. */ +const BILLED_ID = 'user-billed' +/** Never a target by default, so the owner guard stays inert unless a test wants it. */ +const OWNER_ID = 'user-owner' +const ORG_ID = 'org-1' + +const routeContext = { params: Promise.resolve({ id: WORKSPACE_ID }) } + +const permissionRow = (userId: string, permissionType: 'admin' | 'write' | 'read') => ({ + userId, + permissionType, + email: `${userId}@example.com`, +}) + +/** + * Queues the reads a personal-workspace PATCH performs, in order: the workspace + * row, then the permission rows twice — the unlocked pre-flight read that builds + * the audit/membership snapshot, and the `FOR UPDATE` read inside the + * transaction. The workspace-environment read is left unqueued so it resolves + * empty and the credential sync is skipped. + */ +function queuePersonalWorkspace( + existing: ReturnType[], + billedAccountUserId: string = ADMIN_ID, + locked: ReturnType[] = existing +) { + const workspaceRow = { ownerId: OWNER_ID, billedAccountUserId, organizationId: null } + queueTableRows(schemaMock.workspace, [workspaceRow]) + /** The in-transaction re-read of the same row, taken `FOR UPDATE`. */ + permissionsMockFns.mockGetWorkspaceWithOwner.mockResolvedValue({ + id: WORKSPACE_ID, + ...workspaceRow, + }) + queueTableRows(schemaMock.permissions, existing) + queueTableRows(schemaMock.permissions, locked) +} + +/** + * The organization branch runs an extra `member` read before the permissions + * reads, so its result must be queued in that slot. + */ +function queueOrgWorkspace( + orgAdminTargets: { userId: string }[], + existing: ReturnType[], + /** The in-transaction `FOR UPDATE` re-read, which carries roles to filter. */ + lockedMembers: { userId: string; role: string }[] = [] +) { + const workspaceRow = { + ownerId: OWNER_ID, + billedAccountUserId: BILLED_ID, + organizationId: ORG_ID, + } + queueTableRows(schemaMock.workspace, [workspaceRow]) + permissionsMockFns.mockGetWorkspaceWithOwner.mockResolvedValue({ + id: WORKSPACE_ID, + ...workspaceRow, + }) + queueTableRows(schemaMock.member, orgAdminTargets) + queueTableRows(schemaMock.member, lockedMembers) + queueTableRows(schemaMock.permissions, existing) + queueTableRows(schemaMock.permissions, existing) +} + +describe('workspace permissions route', () => { + beforeEach(() => { + vi.clearAllMocks() + resetDbChainMock() + + mockGetSession.mockResolvedValue({ user: { id: ADMIN_ID, name: 'Admin', email: 'a@b.co' } }) + permissionsMockFns.mockHasWorkspaceAdminAccess.mockResolvedValue(true) + permissionsMockFns.mockGetUsersWithPermissions.mockResolvedValue([]) + mockSyncWorkspaceEnvCredentials.mockResolvedValue(undefined) + mockGetEffectiveWorkspacePermission.mockResolvedValue('admin') + }) + + describe('PATCH', () => { + it('updates permissions for an existing member', async () => { + queuePersonalWorkspace([permissionRow(ADMIN_ID, 'admin'), permissionRow(MEMBER_ID, 'read')]) + dbChainMockFns.returning.mockResolvedValue([{ id: 'perm-1' }]) + + const response = await PATCH( + createMockRequest('PATCH', { updates: [{ userId: MEMBER_ID, permissions: 'write' }] }), + routeContext + ) + + expect(response.status).toBe(200) + expect(dbChainMockFns.update).toHaveBeenCalledWith(schemaMock.permissions) + }) + + /** + * The row-queue mock returns whatever was queued regardless of predicate, so + * a WHERE clause is only testable by inspecting the condition tree. Every + * statement that selects permission rows by user — the pre-flight read, the + * `FOR UPDATE` read, and the write — must be confined to this workspace; + * losing `entityId` on the write would rewrite the target's role in EVERY + * workspace they belong to, and losing it on a read would decide membership + * from someone else's workspace. + */ + it('confines every by-user permissions statement to this workspace', async () => { + queuePersonalWorkspace([permissionRow(ADMIN_ID, 'admin'), permissionRow(MEMBER_ID, 'read')]) + + await PATCH( + createMockRequest('PATCH', { updates: [{ userId: MEMBER_ID, permissions: 'write' }] }), + routeContext + ) + + const eqOn = (condition: unknown, column: unknown, value: unknown) => + hasMockCondition( + condition, + (node) => node.type === 'eq' && node.left === column && node.right === value + ) + + const byUserWheres = dbChainMockFns.where.mock.calls + .map(([condition]) => condition) + .filter((condition) => + hasMockCondition( + condition, + (node) => node.type === 'inArray' && node.column === schemaMock.permissions.userId + ) + ) + + expect(byUserWheres.length).toBeGreaterThan(0) + for (const condition of byUserWheres) { + expect(eqOn(condition, schemaMock.permissions.entityId, WORKSPACE_ID)).toBe(true) + expect(eqOn(condition, schemaMock.permissions.entityType, 'workspace')).toBe(true) + } + }) + + /** + * Rows are locked in userId order so two concurrent batches over the same + * members cannot acquire them in opposite orders and deadlock. The caller is + * included: two admins editing each other would otherwise take + * self-then-target in opposing orders. + */ + it('locks the caller and every target in one ordered statement', async () => { + queuePersonalWorkspace([permissionRow(ADMIN_ID, 'admin'), permissionRow(MEMBER_ID, 'read')]) + + await PATCH( + createMockRequest('PATCH', { updates: [{ userId: MEMBER_ID, permissions: 'write' }] }), + routeContext + ) + + expect(dbChainMockFns.for).toHaveBeenCalledWith('update') + expect(dbChainMockFns.orderBy).toHaveBeenCalledWith(schemaMock.permissions.userId) + + const lockWhere = dbChainMockFns.where.mock.calls + .map(([condition]) => condition) + .find((condition) => + hasMockCondition( + condition, + (node) => + node.type === 'inArray' && + node.column === schemaMock.permissions.userId && + Array.isArray(node.values) && + node.values.includes(ADMIN_ID) && + node.values.includes(MEMBER_ID) + ) + ) + + expect(lockWhere).toBeDefined() + }) + + /** + * The row is updated in place rather than deleted and re-inserted, so + * `permissions.createdAt` — surfaced to the UI as the member's joined date — + * survives a role change. A `createdAt` or `id` in the SET payload would + * mean the old delete+insert shape came back. + */ + it('preserves the joined date by updating the row in place', async () => { + queuePersonalWorkspace([permissionRow(ADMIN_ID, 'admin'), permissionRow(MEMBER_ID, 'read')]) + dbChainMockFns.returning.mockResolvedValue([{ id: 'perm-1' }]) + + await PATCH( + createMockRequest('PATCH', { updates: [{ userId: MEMBER_ID, permissions: 'write' }] }), + routeContext + ) + + expect(dbChainMockFns.insert).not.toHaveBeenCalled() + expect(dbChainMockFns.delete).not.toHaveBeenCalled() + expect(dbChainMockFns.set).toHaveBeenCalledWith({ + permissionType: 'write', + updatedAt: expect.any(Date), + }) + }) + + /** + * The member passes the unlocked pre-flight read and is gone by the time the + * `FOR UPDATE` read runs — the exact interleaving a concurrent removal + * produces. + */ + it('returns 409 when the member is removed concurrently', async () => { + queuePersonalWorkspace( + [permissionRow(ADMIN_ID, 'admin'), permissionRow(MEMBER_ID, 'read')], + ADMIN_ID, + [permissionRow(ADMIN_ID, 'admin')] + ) + + const response = await PATCH( + createMockRequest('PATCH', { updates: [{ userId: MEMBER_ID, permissions: 'write' }] }), + routeContext + ) + + expect(response.status).toBe(409) + /** Routed through `withRouteHandler`, so the body carries a correlation id. */ + await expect(response.json()).resolves.toMatchObject({ + error: "This member's access just changed. Refresh and try again.", + requestId: expect.any(String), + }) + /** Detected before any write, so the whole batch rolls back. */ + expect(dbChainMockFns.set).not.toHaveBeenCalled() + expect(dbChainMockFns.insert).not.toHaveBeenCalled() + expect(permissionsMockFns.mockGetUsersWithPermissions).not.toHaveBeenCalled() + expect(auditMockFns.mockRecordAudit).not.toHaveBeenCalled() + }) + + /** + * `ownerId` and `billedAccountUserId` are read unlocked, and a workspace + * admin can move both from other endpoints — removing the owner transfers + * ownership, and the billed account is directly settable. Here the target + * becomes the owner after the pre-flight read, so only the locked + * re-evaluation can catch it. + */ + it('aborts when the target becomes the owner mid-request', async () => { + queuePersonalWorkspace([permissionRow(ADMIN_ID, 'admin'), permissionRow(MEMBER_ID, 'admin')]) + permissionsMockFns.mockGetWorkspaceWithOwner.mockResolvedValue({ + id: WORKSPACE_ID, + ownerId: MEMBER_ID, + billedAccountUserId: ADMIN_ID, + organizationId: null, + }) + + const response = await PATCH( + createMockRequest('PATCH', { updates: [{ userId: MEMBER_ID, permissions: 'read' }] }), + routeContext + ) + + expect(response.status).toBe(409) + await expect(response.json()).resolves.toMatchObject({ + error: 'This workspace just changed. Refresh and try again.', + }) + expect(dbChainMockFns.set).not.toHaveBeenCalled() + }) + + /** + * The target is a plain member on the unlocked pre-flight read and an + * organization admin by the time the `FOR UPDATE` read runs, so only the + * locked re-evaluation can refuse it. Without the target's `member` row in + * the lock set this commits, leaving an explicit `read` row underneath an + * inherited admin — which is what they silently drop to on leaving the org. + */ + it('aborts when the target becomes an organization admin mid-request', async () => { + queueOrgWorkspace( + [], + [permissionRow(MEMBER_ID, 'read')], + [{ userId: MEMBER_ID, role: 'admin' }] + ) + + const response = await PATCH( + createMockRequest('PATCH', { updates: [{ userId: MEMBER_ID, permissions: 'write' }] }), + routeContext + ) + + expect(response.status).toBe(409) + await expect(response.json()).resolves.toMatchObject({ + error: 'This workspace just changed. Refresh and try again.', + }) + expect(dbChainMockFns.set).not.toHaveBeenCalled() + }) + + it('aborts when the workspace leaves its organization mid-request', async () => { + queuePersonalWorkspace([permissionRow(ADMIN_ID, 'admin'), permissionRow(MEMBER_ID, 'read')]) + permissionsMockFns.mockGetWorkspaceWithOwner.mockResolvedValue({ + id: WORKSPACE_ID, + ownerId: OWNER_ID, + billedAccountUserId: ADMIN_ID, + organizationId: ORG_ID, + }) + + const response = await PATCH( + createMockRequest('PATCH', { updates: [{ userId: MEMBER_ID, permissions: 'write' }] }), + routeContext + ) + + expect(response.status).toBe(409) + expect(dbChainMockFns.set).not.toHaveBeenCalled() + }) + + /** + * Rewriting a row to the role it already holds would bump `updatedAt` and + * emit a "from write to write" audit entry — noise in the trail, and a false + * positive for anything watching `updatedAt` for real changes. + */ + it('skips a member already at the requested role', async () => { + queuePersonalWorkspace([permissionRow(ADMIN_ID, 'admin'), permissionRow(MEMBER_ID, 'write')]) + + const response = await PATCH( + createMockRequest('PATCH', { updates: [{ userId: MEMBER_ID, permissions: 'write' }] }), + routeContext + ) + + expect(response.status).toBe(200) + expect(dbChainMockFns.set).not.toHaveBeenCalled() + expect(auditMockFns.mockRecordAudit).not.toHaveBeenCalled() + }) + + it('rejects a userId that is not already a workspace member', async () => { + queuePersonalWorkspace([permissionRow(ADMIN_ID, 'admin')]) + + const response = await PATCH( + createMockRequest('PATCH', { updates: [{ userId: OUTSIDER_ID, permissions: 'read' }] }), + routeContext + ) + + expect(response.status).toBe(400) + await expect(response.json()).resolves.toEqual({ + error: 'Only existing workspace members can have their permissions updated', + }) + expect(dbChainMockFns.update).not.toHaveBeenCalled() + expect(dbChainMockFns.insert).not.toHaveBeenCalled() + }) + + it('rejects the whole batch when any target is not already a member', async () => { + queuePersonalWorkspace([permissionRow(ADMIN_ID, 'admin'), permissionRow(MEMBER_ID, 'read')]) + + const response = await PATCH( + createMockRequest('PATCH', { + updates: [ + { userId: MEMBER_ID, permissions: 'write' }, + { userId: OUTSIDER_ID, permissions: 'read' }, + ], + }), + routeContext + ) + + expect(response.status).toBe(400) + expect(dbChainMockFns.update).not.toHaveBeenCalled() + expect(dbChainMockFns.insert).not.toHaveBeenCalled() + }) + + it('requires workspace admin access before reading the body', async () => { + permissionsMockFns.mockHasWorkspaceAdminAccess.mockResolvedValue(false) + + const response = await PATCH( + createMockRequest('PATCH', { updates: [{ userId: OUTSIDER_ID, permissions: 'read' }] }), + routeContext + ) + + expect(response.status).toBe(403) + expect(dbChainMockFns.update).not.toHaveBeenCalled() + }) + + it('rejects an empty updates array', async () => { + const response = await PATCH(createMockRequest('PATCH', { updates: [] }), routeContext) + + expect(response.status).toBe(400) + expect(dbChainMockFns.update).not.toHaveBeenCalled() + }) + + /** + * A repeated userId used to slip past the self-demotion guard: it inspected + * the first matching entry while the write loop applied every entry in order, + * so a trailing `read` landed after a leading `admin` had satisfied the check. + */ + it('rejects a batch that names the same user twice', async () => { + queuePersonalWorkspace([permissionRow(ADMIN_ID, 'admin'), permissionRow(MEMBER_ID, 'read')]) + + const response = await PATCH( + createMockRequest('PATCH', { + updates: [ + { userId: MEMBER_ID, permissions: 'admin' }, + { userId: MEMBER_ID, permissions: 'read' }, + ], + }), + routeContext + ) + + expect(response.status).toBe(400) + expect(dbChainMockFns.update).not.toHaveBeenCalled() + }) + + /** + * The exploit the dedup rule exists to stop: a leading `admin` entry satisfies + * the self-demotion guard while a trailing `read` entry is what actually lands. + */ + it('rejects a duplicate-entry attempt to self-demote', async () => { + queuePersonalWorkspace([permissionRow(ADMIN_ID, 'admin')]) + + const response = await PATCH( + createMockRequest('PATCH', { + updates: [ + { userId: ADMIN_ID, permissions: 'admin' }, + { userId: ADMIN_ID, permissions: 'read' }, + ], + }), + routeContext + ) + + expect(response.status).toBe(400) + expect(dbChainMockFns.update).not.toHaveBeenCalled() + }) + + it('refuses to strip the acting admin of their own admin', async () => { + queuePersonalWorkspace([permissionRow(ADMIN_ID, 'admin')]) + + const response = await PATCH( + createMockRequest('PATCH', { updates: [{ userId: ADMIN_ID, permissions: 'read' }] }), + routeContext + ) + + expect(response.status).toBe(400) + await expect(response.json()).resolves.toEqual({ + error: 'Cannot remove your own admin permissions', + }) + expect(dbChainMockFns.update).not.toHaveBeenCalled() + }) + + /** + * The owner holds an ordinary explicit admin row, so nothing but this guard + * distinguishes them — an invited admin could otherwise demote the owner out + * of their own workspace with no path back. + */ + it('refuses to demote the workspace owner', async () => { + queueTableRows(schemaMock.workspace, [ + { ownerId: MEMBER_ID, billedAccountUserId: BILLED_ID, organizationId: null }, + ]) + queueTableRows(schemaMock.permissions, [permissionRow(MEMBER_ID, 'admin')]) + + const response = await PATCH( + createMockRequest('PATCH', { updates: [{ userId: MEMBER_ID, permissions: 'read' }] }), + routeContext + ) + + expect(response.status).toBe(400) + await expect(response.json()).resolves.toEqual({ + error: 'The workspace owner must retain admin permissions', + }) + expect(dbChainMockFns.update).not.toHaveBeenCalled() + }) + + it('aborts when the caller loses admin mid-request', async () => { + queuePersonalWorkspace([permissionRow(ADMIN_ID, 'admin'), permissionRow(MEMBER_ID, 'read')]) + mockGetEffectiveWorkspacePermission.mockResolvedValue('write') + + const response = await PATCH( + createMockRequest('PATCH', { updates: [{ userId: MEMBER_ID, permissions: 'admin' }] }), + routeContext + ) + + expect(response.status).toBe(409) + await expect(response.json()).resolves.toMatchObject({ + error: 'Your workspace permissions changed. Refresh and try again.', + }) + expect(dbChainMockFns.set).not.toHaveBeenCalled() + }) + + it('refuses to demote the workspace billing account', async () => { + queuePersonalWorkspace([permissionRow(BILLED_ID, 'admin')], BILLED_ID) + + const response = await PATCH( + createMockRequest('PATCH', { updates: [{ userId: BILLED_ID, permissions: 'read' }] }), + routeContext + ) + + expect(response.status).toBe(400) + await expect(response.json()).resolves.toEqual({ + error: 'Workspace billing account must retain admin permissions', + }) + expect(dbChainMockFns.update).not.toHaveBeenCalled() + }) + + /** + * An organization admin holds no permission row — their workspace admin is + * derived — but the members list still shows them. Membership must therefore + * mean "visible in the members list", or the caller is told someone they are + * looking at is not a member instead of why the role is fixed. + */ + it('explains the lock for an organization admin who holds no permission row', async () => { + queueOrgWorkspace([{ userId: MEMBER_ID }], [permissionRow(ADMIN_ID, 'admin')]) + + const response = await PATCH( + createMockRequest('PATCH', { updates: [{ userId: MEMBER_ID, permissions: 'read' }] }), + routeContext + ) + + expect(response.status).toBe(400) + await expect(response.json()).resolves.toEqual({ + error: 'Organization admins are workspace admins and their role cannot be changed', + }) + }) + + it('refuses to change the role of an organization admin', async () => { + queueOrgWorkspace([{ userId: MEMBER_ID }], [permissionRow(MEMBER_ID, 'admin')]) + + const response = await PATCH( + createMockRequest('PATCH', { updates: [{ userId: MEMBER_ID, permissions: 'read' }] }), + routeContext + ) + + expect(response.status).toBe(400) + await expect(response.json()).resolves.toEqual({ + error: 'Organization admins are workspace admins and their role cannot be changed', + }) + expect(dbChainMockFns.update).not.toHaveBeenCalled() + }) + + it('returns 404 when the workspace does not exist', async () => { + const response = await PATCH( + createMockRequest('PATCH', { updates: [{ userId: MEMBER_ID, permissions: 'read' }] }), + routeContext + ) + + expect(response.status).toBe(404) + expect(dbChainMockFns.update).not.toHaveBeenCalled() + }) + + /** + * Targets sharing a role share a statement, so a batch costs one write per + * distinct role rather than one per member — the transaction holds its pooled + * connection for a bounded number of round trips regardless of batch size. + */ + it('writes one statement per distinct role, not per member', async () => { + const members = ['user-a', 'user-b', 'user-c', 'user-d'] + queuePersonalWorkspace([ + permissionRow(ADMIN_ID, 'admin'), + ...members.map((userId) => permissionRow(userId, 'read')), + ]) + + const response = await PATCH( + createMockRequest('PATCH', { + updates: members.map((userId) => ({ userId, permissions: 'write' })), + }), + routeContext + ) + + expect(response.status).toBe(200) + expect(dbChainMockFns.set).toHaveBeenCalledTimes(1) + expect(dbChainMockFns.set).toHaveBeenCalledWith({ + permissionType: 'write', + updatedAt: expect.any(Date), + }) + }) + + it('applies every member of a multi-item batch', async () => { + queuePersonalWorkspace([ + permissionRow(ADMIN_ID, 'admin'), + permissionRow(MEMBER_ID, 'read'), + permissionRow(OUTSIDER_ID, 'read'), + ]) + dbChainMockFns.returning.mockResolvedValue([{ id: 'perm-1' }]) + + const response = await PATCH( + createMockRequest('PATCH', { + updates: [ + { userId: OUTSIDER_ID, permissions: 'admin' }, + { userId: MEMBER_ID, permissions: 'write' }, + ], + }), + routeContext + ) + + expect(response.status).toBe(200) + expect(dbChainMockFns.set).toHaveBeenCalledTimes(2) + expect(dbChainMockFns.set).toHaveBeenCalledWith({ + permissionType: 'admin', + updatedAt: expect.any(Date), + }) + expect(dbChainMockFns.set).toHaveBeenCalledWith({ + permissionType: 'write', + updatedAt: expect.any(Date), + }) + }) + }) +}) diff --git a/apps/sim/app/api/workspaces/[id]/permissions/route.ts b/apps/sim/app/api/workspaces/[id]/permissions/route.ts index 3086192a6eb..288b262ff37 100644 --- a/apps/sim/app/api/workspaces/[id]/permissions/route.ts +++ b/apps/sim/app/api/workspaces/[id]/permissions/route.ts @@ -2,24 +2,139 @@ import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit' import { db } from '@sim/db' import { member, permissions, user, workspace, workspaceEnvironment } from '@sim/db/schema' import { createLogger } from '@sim/logger' -import { ORG_ADMIN_ROLES } from '@sim/platform-authz/workspace' -import { generateId } from '@sim/utils/id' -import { and, eq, inArray } from 'drizzle-orm' +import { isOrgAdminRole, ORG_ADMIN_ROLES } from '@sim/platform-authz/workspace' +import { and, eq, inArray, sql } from 'drizzle-orm' import { type NextRequest, NextResponse } from 'next/server' -import { updateWorkspacePermissionsContract } from '@/lib/api/contracts/workspaces' -import { parseRequest } from '@/lib/api/server' +import { + updateWorkspacePermissionsContract, + type WorkspacePermission, +} from '@/lib/api/contracts/workspaces' +import { getValidationErrorMessage, parseRequest } from '@/lib/api/server' import { getSession } from '@/lib/auth' +import { HttpError } from '@/lib/core/utils/http-error' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { syncWorkspaceEnvCredentials } from '@/lib/credentials/environment' +import { withTransactionRetry } from '@/lib/db/transaction' +import type { DbOrTx } from '@/lib/db/types' import { captureServerEvent } from '@/lib/posthog/server' import { - getUsersWithPermissions, + getEffectiveWorkspacePermission, getWorkspacePermissionsForViewer, + getWorkspaceWithOwner, hasWorkspaceAdminAccess, } from '@/lib/workspaces/permissions/utils' const logger = createLogger('WorkspacesPermissionsAPI') +/** + * A target's permission row no longer matched at write time — it was removed, + * or replaced, by a concurrent writer. Thrown inside the transaction so the + * whole batch rolls back instead of resurrecting a revoked collaborator or + * writing a role decided against stale standing. + * + * `message` reaches the client verbatim via `withRouteHandler`, so it is worded + * for the person who clicked. + */ +class StaleWorkspaceMembershipError extends HttpError { + readonly statusCode = 409 + readonly userId: string + + constructor(userId: string) { + super("This member's access just changed. Refresh and try again.") + this.name = 'StaleWorkspaceMembershipError' + this.userId = userId + } +} + +/** + * The caller's own workspace admin standing was revoked while the request was in + * flight. Authority is checked before the body is parsed, several reads ahead of + * the write, so it is re-read under lock inside the transaction — otherwise a + * just-demoted admin's in-flight batch still commits. + */ +class WorkspaceAdminRevokedError extends HttpError { + readonly statusCode = 409 + + constructor() { + super('Your workspace permissions changed. Refresh and try again.') + this.name = 'WorkspaceAdminRevokedError' + } +} + +/** + * The workspace moved under the request — ownership transferred, the billed + * account changed, or it joined/left an organization — so a guard that passed + * on the pre-flight read no longer holds against the locked row. + */ +class WorkspaceContextChangedError extends HttpError { + readonly statusCode = 409 + + constructor() { + super('This workspace just changed. Refresh and try again.') + this.name = 'WorkspaceContextChangedError' + } +} + +/** + * Bounds the wait on the row locks below so a stuck holder fails fast + * (SQLSTATE 55P03) instead of parking a pooled connection indefinitely. + */ +const PERMISSIONS_LOCK_TIMEOUT_MS = 5000 + +/** Organization owners/admins among `userIds`, empty for a personal workspace. */ +async function loadOrgAdminTargets( + executor: DbOrTx, + organizationId: string | null, + userIds: string[] +): Promise> { + if (!organizationId) return new Set() + const rows = await executor + .select({ userId: member.userId }) + .from(member) + .where( + and( + eq(member.organizationId, organizationId), + inArray(member.userId, userIds), + inArray(member.role, [...ORG_ADMIN_ROLES]) + ) + ) + return new Set(rows.map((row) => row.userId)) +} + +/** + * Roles that are inherited rather than granted, and so cannot be edited here. + * + * Every input is workspace state that another request can change underneath + * this one, so this runs twice: once on the pre-flight read to answer with the + * specific reason, and once inside the transaction against the locked row. One + * function so the two evaluations cannot drift. + */ +function findInheritedRoleViolation( + updates: readonly { userId: string; permissions: WorkspacePermission }[], + state: { + ownerId: string + billedAccountUserId: string | null + orgAdminUserIds: ReadonlySet + } +): string | null { + if (updates.some((update) => state.orgAdminUserIds.has(update.userId))) { + return 'Organization admins are workspace admins and their role cannot be changed' + } + if (updates.some((update) => update.userId === state.ownerId && update.permissions !== 'admin')) { + return 'The workspace owner must retain admin permissions' + } + const billedAccountUserId = state.billedAccountUserId + if ( + billedAccountUserId && + updates.some( + (update) => update.userId === billedAccountUserId && update.permissions !== 'admin' + ) + ) { + return 'Workspace billing account must retain admin permissions' + } + return null +} + /** * GET /api/workspaces/[id]/permissions * @@ -31,25 +146,20 @@ const logger = createLogger('WorkspacesPermissionsAPI') */ export const GET = withRouteHandler( async (request: NextRequest, { params }: { params: Promise<{ id: string }> }) => { - try { - const { id: workspaceId } = await params - const session = await getSession() - - if (!session?.user?.id) { - return NextResponse.json({ error: 'Authentication required' }, { status: 401 }) - } + const { id: workspaceId } = await params + const session = await getSession() - const result = await getWorkspacePermissionsForViewer(workspaceId, session.user.id) + if (!session?.user?.id) { + return NextResponse.json({ error: 'Authentication required' }, { status: 401 }) + } - if (!result) { - return NextResponse.json({ error: 'Workspace not found or access denied' }, { status: 404 }) - } + const result = await getWorkspacePermissionsForViewer(workspaceId, session.user.id) - return NextResponse.json(result) - } catch (error) { - logger.error('Error fetching workspace permissions:', error) - return NextResponse.json({ error: 'Failed to fetch workspace permissions' }, { status: 500 }) + if (!result) { + return NextResponse.json({ error: 'Workspace not found or access denied' }, { status: 404 }) } + + return NextResponse.json(result) } ) @@ -59,128 +169,383 @@ export const GET = withRouteHandler( * Updates permissions for existing workspace members. * Only admin users can update permissions. * + * Every target must already hold a workspace permission row — this endpoint + * cannot introduce a member. Adding one goes through the invitation flow, which + * enforces the plan, seat, and consent gates. + * + * Each change is an in-place UPDATE of the existing row, so the row's + * `createdAt` (surfaced as the member's joined date) survives a role change, and + * a target removed concurrently matches no row and yields a 409 rather than + * being re-created. + * + * Roles that are inherited rather than granted cannot be edited here, matching + * the lock the members list shows: the workspace owner, organization + * owners/admins, the billing account, and the caller's own admin. The caller's + * authority is re-read under lock inside the transaction, so a batch from an + * admin who was demoted mid-request does not commit. + * * @param workspaceId - The workspace ID from the URL parameters - * @param updates - Array of permission updates for users + * @param updates - Array of permission updates for existing members * @returns Success message or error */ export const PATCH = withRouteHandler( async (request: NextRequest, context: { params: Promise<{ id: string }> }) => { - try { - const { id: workspaceId } = await context.params - const session = await getSession() + const { id: workspaceId } = await context.params + const session = await getSession() - if (!session?.user?.id) { - return NextResponse.json({ error: 'Authentication required' }, { status: 401 }) - } + if (!session?.user?.id) { + return NextResponse.json({ error: 'Authentication required' }, { status: 401 }) + } - const hasAdminAccess = await hasWorkspaceAdminAccess(session.user.id, workspaceId) + const hasAdminAccess = await hasWorkspaceAdminAccess(session.user.id, workspaceId) - if (!hasAdminAccess) { - return NextResponse.json( - { error: 'Admin access required to update permissions' }, - { status: 403 } + if (!hasAdminAccess) { + return NextResponse.json( + { error: 'Admin access required to update permissions' }, + { status: 403 } + ) + } + + /** + * The default validation response reports a generic "Validation error" and + * puts the authored message in `details`, which the client never reads — so + * the duplicate-userId, batch-size, and id-length rules would all surface as + * the same unhelpful string. + */ + const parsed = await parseRequest(updateWorkspacePermissionsContract, request, context, { + validationErrorResponse: (error) => + NextResponse.json({ error: getValidationErrorMessage(error) }, { status: 400 }), + }) + if (!parsed.success) return parsed.response + const body = parsed.data.body + + const workspaceRow = await db + .select({ + ownerId: workspace.ownerId, + billedAccountUserId: workspace.billedAccountUserId, + organizationId: workspace.organizationId, + }) + .from(workspace) + .where(eq(workspace.id, workspaceId)) + .limit(1) + + if (!workspaceRow.length) { + return NextResponse.json({ error: 'Workspace not found' }, { status: 404 }) + } + + const ownerId = workspaceRow[0].ownerId + const billedAccountUserId = workspaceRow[0].billedAccountUserId + const organizationId = workspaceRow[0].organizationId + + const targetUserIds = body.updates.map((update) => update.userId) + + /** + * Current standing and display name for the targets. Scoped to the targets: + * nothing downstream reads a non-target row, and an unscoped read + * materializes every member of the workspace on a single-role change. + */ + const existingPerms = await db + .select({ + userId: permissions.userId, + email: user.email, + }) + .from(permissions) + .innerJoin(user, eq(permissions.userId, user.id)) + .where( + and( + eq(permissions.entityType, 'workspace'), + eq(permissions.entityId, workspaceId), + inArray(permissions.userId, targetUserIds) ) - } + ) - const parsed = await parseRequest(updateWorkspacePermissionsContract, request, context) - if (!parsed.success) return parsed.response - const body = parsed.data.body + const emailByUserId = new Map(existingPerms.map((row) => [row.userId, row.email])) - const workspaceRow = await db - .select({ - billedAccountUserId: workspace.billedAccountUserId, - organizationId: workspace.organizationId, - }) - .from(workspace) - .where(eq(workspace.id, workspaceId)) - .limit(1) + const orgAdminUserIds = await loadOrgAdminTargets(db, organizationId, targetUserIds) - if (!workspaceRow.length) { - return NextResponse.json({ error: 'Workspace not found' }, { status: 404 }) - } + /** + * Membership is checked before anything else about the target, so the + * inherited-role answers below only ever describe someone the caller can + * already see in the members list. Checking them first made this an oracle: + * it would tell any caller whether an arbitrary userId was the workspace's + * billing account. + * + * "Member" therefore means what the members list means — an explicit row OR + * a derived organization admin. Organization admins hold no permission row, + * so testing rows alone would answer "not a member" about someone the caller + * is looking at, instead of explaining why their role is fixed. + * + * This endpoint only *changes* standing, never grants it. Adding a + * collaborator belongs to the invitation flow, which owns the gates this + * route cannot apply — the external-collaborator paid-plan requirement, + * seat provisioning, and the invitee's acceptance. + */ + const nonMemberUserIds = targetUserIds.filter( + (userId) => !emailByUserId.has(userId) && !orgAdminUserIds.has(userId) + ) + if (nonMemberUserIds.length > 0) { + logger.warn('Rejected permission update for non-members', { + workspaceId, + nonMemberCount: nonMemberUserIds.length, + }) + return NextResponse.json( + { error: 'Only existing workspace members can have their permissions updated' }, + { status: 400 } + ) + } + + if ( + body.updates.some( + (update) => update.userId === session.user.id && update.permissions !== 'admin' + ) + ) { + return NextResponse.json( + { error: 'Cannot remove your own admin permissions' }, + { status: 400 } + ) + } - const billedAccountUserId = workspaceRow[0].billedAccountUserId - const organizationId = workspaceRow[0].organizationId + const preflightViolation = findInheritedRoleViolation(body.updates, { + ownerId, + billedAccountUserId, + orgAdminUserIds, + }) + if (preflightViolation) { + return NextResponse.json({ error: preflightViolation }, { status: 400 }) + } + /** + * Retried on a deadlock: the ordered locks below close this route against + * itself, but member removal takes the billed account before the departing + * user, so a cross-path cycle remains. The victim of one is asked by the + * database to try again, and answering it with a 500 would surface as + * "Internal server error" for a click that would have worked. + */ + const { previousRoles, changedUserIds } = await withTransactionRetry(async (tx) => { + await tx.execute( + sql`select set_config('lock_timeout', ${`${PERMISSIONS_LOCK_TIMEOUT_MS}ms`}, true)` + ) + + /** + * An inherited org-admin grant lives in `member`, not `permissions`, so it + * is fenced separately — and first, so every invocation of this route takes + * the tables in the same order. Same order as + * `validateLockedWorkspaceInvitationContext`: member, workspace, permissions. + */ + const lockUserIds = [...new Set([session.user.id, ...targetUserIds])] + + /** + * The caller's inherited authority and the targets' inherited-admin standing + * both live here, so both are locked — in one ordered statement, so `member` + * has a total acquisition order for the same reason `permissions` does + * below. Locking only the caller left a target's promotion to organization + * admin able to land between the guard and the write. + */ + let orgAdminUserIds: ReadonlySet = new Set() if (organizationId) { - const targetUserIds = body.updates.map((update) => update.userId) - const orgAdminTargets = await db - .select({ userId: member.userId }) + const lockedMembers = await tx + .select({ userId: member.userId, role: member.role }) .from(member) .where( - and( - eq(member.organizationId, organizationId), - inArray(member.userId, targetUserIds), - inArray(member.role, [...ORG_ADMIN_ROLES]) - ) - ) - if (orgAdminTargets.length > 0) { - return NextResponse.json( - { error: 'Organization admins are workspace admins and their role cannot be changed' }, - { status: 400 } + and(eq(member.organizationId, organizationId), inArray(member.userId, lockUserIds)) ) - } + .orderBy(member.userId) + .for('update') + orgAdminUserIds = new Set( + lockedMembers.filter((row) => isOrgAdminRole(row.role)).map((row) => row.userId) + ) } - const selfUpdate = body.updates.find((update) => update.userId === session.user.id) - if (selfUpdate && selfUpdate.permissions !== 'admin') { - return NextResponse.json( - { error: 'Cannot remove your own admin permissions' }, - { status: 400 } - ) + /** + * `ownerId`, `billedAccountUserId`, and `organizationId` were read + * unlocked, and a workspace admin can move all three from other endpoints + * — ownership transfers when the owner is removed, and the billed account + * is directly settable. Re-reading them under lock is what stops a batch + * vetted against the old row from demoting whoever those columns point at + * now, which would strand a workspace owner on `read` with no way back. + */ + const lockedWorkspace = await getWorkspaceWithOwner(workspaceId, { + executor: tx, + forUpdate: true, + }) + if (!lockedWorkspace || lockedWorkspace.organizationId !== organizationId) { + throw new WorkspaceContextChangedError() } - if ( - billedAccountUserId && - body.updates.some( - (update) => update.userId === billedAccountUserId && update.permissions !== 'admin' - ) - ) { - return NextResponse.json( - { error: 'Workspace billing account must retain admin permissions' }, - { status: 400 } + const lockedViolation = findInheritedRoleViolation(body.updates, { + ownerId: lockedWorkspace.ownerId, + billedAccountUserId: lockedWorkspace.billedAccountUserId, + orgAdminUserIds, + }) + if (lockedViolation) { + logger.warn('Permission update raced a workspace change', { + workspaceId, + reason: lockedViolation, + }) + throw new WorkspaceContextChangedError() + } + + /** + * One ordered lock over the caller's row and every target's, which is what + * makes the batch deadlock-free against another invocation of this route: + * `ORDER BY` sits below `FOR UPDATE`, so Postgres acquires the row locks in + * userId order no matter what order the request listed them in. Including + * the caller matters — two admins editing each other would otherwise take + * self-then-target in opposite orders and deadlock. + * + * It does not order against other writers of these rows (member removal + * takes the billed account before the departing user), so it closes this + * route against itself rather than closing the table globally. + */ + const lockedRows = await tx + .select({ userId: permissions.userId, permissionType: permissions.permissionType }) + .from(permissions) + .where( + and( + eq(permissions.entityType, 'workspace'), + eq(permissions.entityId, workspaceId), + inArray(permissions.userId, lockUserIds) + ) ) + .orderBy(permissions.userId) + .for('update') + + const lockedByUserId = new Map(lockedRows.map((row) => [row.userId, row.permissionType])) + + /** + * Re-establish the caller's authority now that it cannot change again. + * `hasWorkspaceAdminAccess` ran before the body was parsed and several + * reads ago, so without this a batch from an admin demoted mid-request + * still commits. + */ + const callerPermission = await getEffectiveWorkspacePermission( + session.user.id, + { id: workspaceId, organizationId }, + tx + ) + if (callerPermission !== 'admin') { + logger.warn('Permission update raced revocation of the caller', { + workspaceId, + actorId: session.user.id, + }) + throw new WorkspaceAdminRevokedError() } - // Capture existing permissions and user info for audit metadata - const existingPerms = await db - .select({ - userId: permissions.userId, - permissionType: permissions.permissionType, - email: user.email, + /** + * The membership check above ran unlocked; this is the authoritative one. + * A target missing here was removed in between, so the batch rolls back + * rather than reviving it. + */ + const removedUserId = targetUserIds.find((userId) => !lockedByUserId.has(userId)) + if (removedUserId !== undefined) { + logger.warn('Permission update raced a concurrent membership change', { + workspaceId, + userId: removedUserId, }) - .from(permissions) - .innerJoin(user, eq(permissions.userId, user.id)) - .where(and(eq(permissions.entityType, 'workspace'), eq(permissions.entityId, workspaceId))) + throw new StaleWorkspaceMembershipError(removedUserId) + } - const permLookup = new Map( - existingPerms.map((p) => [p.userId, { permission: p.permissionType, email: p.email }]) + /** + * Entries that ask for the role the member already holds are dropped + * rather than rewritten. Writing them would bump `updatedAt` and emit a + * `MEMBER_ROLE_CHANGED` audit entry reading "from admin to admin" — noise + * in the trail, and a false positive for anything watching `updatedAt` to + * detect real changes. + * + * Targets sharing a role then share a statement, so the batch costs one + * write per distinct role — at most three — instead of one per member. + * Every row is already locked, so the order the groups run in has no + * bearing on lock acquisition. Relies on the contract rejecting a repeated + * `userId`: without that a user could land in two groups and their final + * role would depend on which group ran last. + */ + const changedUpdates = body.updates.filter( + (update) => lockedByUserId.get(update.userId) !== update.permissions ) + const userIdsByPermission = new Map() + for (const update of changedUpdates) { + const group = userIdsByPermission.get(update.permissions) + if (group) group.push(update.userId) + else userIdsByPermission.set(update.permissions, [update.userId]) + } - await db.transaction(async (tx) => { - for (const update of body.updates) { - await tx - .delete(permissions) - .where( - and( - eq(permissions.userId, update.userId), - eq(permissions.entityType, 'workspace'), - eq(permissions.entityId, workspaceId) - ) + /** + * One timestamp for the batch: it commits atomically, so the rows should + * not disagree about when they changed. + */ + const updatedAt = new Date() + for (const [permission, userIds] of userIdsByPermission) { + await tx + .update(permissions) + .set({ permissionType: permission, updatedAt }) + .where( + and( + eq(permissions.entityType, 'workspace'), + eq(permissions.entityId, workspaceId), + inArray(permissions.userId, userIds) ) + ) + } + + return { + previousRoles: lockedByUserId, + changedUserIds: new Set(changedUpdates.map((update) => update.userId)), + } + }) + + /** + * The change is durable from here on, so it is recorded before anything + * that can still throw. Ordering this after the reads below meant a + * transient failure in them produced a committed but entirely unaudited + * role change. + */ + for (const update of body.updates) { + if (!changedUserIds.has(update.userId)) continue + + captureServerEvent( + session.user.id, + 'workspace_member_role_changed', + { workspace_id: workspaceId, new_role: update.permissions }, + { groups: { workspace: workspaceId } } + ) + + /** + * `previousRole` comes from the locked read, not the unlocked one above: + * the pre-flight snapshot can be overtaken by another admin's change, and + * recording a transition that never happened corrupts the trail. + */ + const targetEmail = emailByUserId.get(update.userId) + const previousRole = previousRoles.get(update.userId) ?? null - await tx.insert(permissions).values({ - id: generateId(), - userId: update.userId, - entityType: 'workspace' as const, - entityId: workspaceId, - permissionType: update.permissions, - createdAt: new Date(), - updatedAt: new Date(), - }) - } + recordAudit({ + workspaceId, + actorId: session.user.id, + action: AuditAction.MEMBER_ROLE_CHANGED, + resourceType: AuditResourceType.WORKSPACE, + resourceId: workspaceId, + resourceName: targetEmail ?? update.userId, + actorName: session.user.name ?? undefined, + actorEmail: session.user.email ?? undefined, + description: `Changed permissions for ${targetEmail ?? update.userId} from ${previousRole ?? 'unknown'} to ${update.permissions}`, + metadata: { + targetUserId: update.userId, + targetEmail: targetEmail ?? undefined, + previousRole, + newRole: update.permissions, + }, + request, }) + } + /** + * Credential membership follows workspace access, but it cannot join the + * transaction above and the role change is already committed. A failure + * anywhere past the commit is therefore a reconciliation task, not a reason + * to answer 500 — that would misreport the committed change and invite a + * retry that changes nothing. The read that feeds the sync is inside the + * guard for the same reason. + */ + try { const [wsEnvRow] = await db .select({ variables: workspaceEnvironment.variables }) .from(workspaceEnvironment) @@ -194,45 +559,14 @@ export const PATCH = withRouteHandler( actingUserId: session.user.id, }) } - - const updatedUsers = await getUsersWithPermissions(workspaceId) - - for (const update of body.updates) { - captureServerEvent( - session.user.id, - 'workspace_member_role_changed', - { workspace_id: workspaceId, new_role: update.permissions }, - { groups: { workspace: workspaceId } } - ) - - recordAudit({ - workspaceId, - actorId: session.user.id, - action: AuditAction.MEMBER_ROLE_CHANGED, - resourceType: AuditResourceType.WORKSPACE, - resourceId: workspaceId, - resourceName: permLookup.get(update.userId)?.email ?? update.userId, - actorName: session.user.name ?? undefined, - actorEmail: session.user.email ?? undefined, - description: `Changed permissions for ${permLookup.get(update.userId)?.email ?? update.userId} from ${permLookup.get(update.userId)?.permission ?? 'none'} to ${update.permissions}`, - metadata: { - targetUserId: update.userId, - targetEmail: permLookup.get(update.userId)?.email ?? undefined, - previousRole: permLookup.get(update.userId)?.permission ?? null, - newRole: update.permissions, - }, - request, - }) - } - - return NextResponse.json({ - message: 'Permissions updated successfully', - users: updatedUsers, - total: updatedUsers.length, - }) } catch (error) { - logger.error('Error updating workspace permissions:', error) - return NextResponse.json({ error: 'Failed to update workspace permissions' }, { status: 500 }) + logger.error('Workspace env credential membership needs reconciliation', { + workspaceId, + targetUserIds, + error, + }) } + + return NextResponse.json({ message: 'Permissions updated successfully' }) } ) diff --git a/apps/sim/app/workspace/[workspaceId]/settings/components/team-management/components/organization-member-lists/organization-member-lists.tsx b/apps/sim/app/workspace/[workspaceId]/settings/components/team-management/components/organization-member-lists/organization-member-lists.tsx index 5a435d644f9..aae0637e267 100644 --- a/apps/sim/app/workspace/[workspaceId]/settings/components/team-management/components/organization-member-lists/organization-member-lists.tsx +++ b/apps/sim/app/workspace/[workspaceId]/settings/components/team-management/components/organization-member-lists/organization-member-lists.tsx @@ -290,13 +290,20 @@ export function OrganizationMemberLists({ workspaceId: string, access: RosterWorkspaceAccess ) => { - const rowUserIsOrgAdmin = isOrgAdminRole(member.role) const isSelf = member.userId === currentUserId const wouldDemoteSelf = isSelf && access.permission === 'admin' + /** + * Every reason here has a matching server guard, so a locked control is one + * the route would have refused. Derived from the roster payload rather than + * from the org role alone, which missed the workspace owner and the billing + * account. + */ + const lockReason = workspaceRoleLockReason(access.roleSource, { + isBilledAccount: access.isBilledAccount, + }) const disabled = - !canManage || rowUserIsOrgAdmin || wouldDemoteSelf || updatePermissions.isPending - const lockReason = rowUserIsOrgAdmin ? workspaceRoleLockReason('org-admin') : null - const canRemoveFromWorkspace = canManage && !rowUserIsOrgAdmin && !isSelf + !canManage || lockReason !== null || wouldDemoteSelf || updatePermissions.isPending + const canRemoveFromWorkspace = canManage && !isOrgAdminRole(member.role) && !isSelf return ( ({ @@ -216,7 +218,9 @@ export function Teammates() { roleControl={(() => { const lockReason = teammate.isPending ? null - : workspaceRoleLockReason(teammate.roleSource) + : workspaceRoleLockReason(teammate.roleSource, { + isBilledAccount: teammate.isBilledAccount, + }) return ( { + /** + * `requestJson` validates the body against the contract before it fetches, + * so a contract failure arrives as a raw `ZodError` whose `message` is the + * serialized issue array. Read the issue instead, which is where the + * authored message lives on both that path and the server's `details`. + */ + const issue = extractValidationIssues(error)[0]?.message + toast.error("Couldn't update role", { + description: issue ?? getErrorMessage(error, 'Please try again in a moment.'), + }) + }, onSettled: (_data, _error, variables) => { queryClient.invalidateQueries({ queryKey: workspaceKeys.permissions(variables.workspaceId), diff --git a/apps/sim/lib/api/contracts/organization.ts b/apps/sim/lib/api/contracts/organization.ts index 5bdd6476dfc..bbd1fcf69ce 100644 --- a/apps/sim/lib/api/contracts/organization.ts +++ b/apps/sim/lib/api/contracts/organization.ts @@ -259,6 +259,13 @@ export const rosterWorkspaceAccessSchema = z.object({ workspaceId: z.string(), workspaceName: z.string(), permission: workspacePermissionSchema, + /** + * Why this role is fixed, when it is. Carried so the roster can disable the + * controls the workspace-permissions route refuses, the way the teammates list + * already does — without them it offers an edit that can only fail. + */ + roleSource: z.enum(['owner', 'explicit', 'org-admin']), + isBilledAccount: z.boolean(), }) export const rosterMemberSchema = z.object({ diff --git a/apps/sim/lib/api/contracts/workspaces.ts b/apps/sim/lib/api/contracts/workspaces.ts index 9cc7e41ab11..b08921be22c 100644 --- a/apps/sim/lib/api/contracts/workspaces.ts +++ b/apps/sim/lib/api/contracts/workspaces.ts @@ -1,5 +1,5 @@ import { z } from 'zod' -import { nonEmptyIdSchema } from '@/lib/api/contracts/primitives' +import { nonEmptyIdSchema, requiredFieldSchema } from '@/lib/api/contracts/primitives' import { type ContractJsonResponse, defineRouteContract } from '@/lib/api/contracts/types' export const workspaceScopeSchema = z.enum(['active', 'archived', 'all']) @@ -94,6 +94,7 @@ export const workspaceUserSchema = z.object({ isExternal: z.boolean(), joinedAt: z.string(), roleSource: z.enum(['owner', 'explicit', 'org-admin']), + isBilledAccount: z.boolean(), }) export type WorkspaceUser = z.output @@ -114,13 +115,42 @@ export const workspacePermissionsResponseSchema = z.object({ export type WorkspacePermissions = z.output +/** + * Role changes for users who are **already** workspace members. The route + * rejects any `userId` without an existing workspace permission row — adding a + * collaborator goes through the invitation flow, which owns the plan, seat, and + * consent gates this endpoint has no way to apply. + */ export const updateWorkspacePermissionsBodySchema = z.object({ - updates: z.array( - z.object({ - userId: z.string(), - permissions: workspacePermissionSchema, - }) - ), + updates: z + .array( + z.object({ + userId: requiredFieldSchema('User ID is required').max(128, 'User ID is too long'), + permissions: workspacePermissionSchema, + }) + ) + .min(1, 'updates must contain at least one permission change') + .max(100, 'Cannot update more than 100 permissions at once') + /** + * One entry per user. Repeating a userId made the batch self-contradictory: + * the route's guards inspect the first matching entry while the write loop + * applied every entry in order, so a second entry could carry a role the + * guards had already vetted the first one against. + */ + .superRefine((updates, ctx) => { + const seen = new Set() + for (const [index, update] of updates.entries()) { + if (seen.has(update.userId)) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: [index, 'userId'], + message: 'Each user may appear only once in updates', + }) + return + } + seen.add(update.userId) + } + }), }) export const workspaceMemberSchema = z.object({ @@ -317,11 +347,15 @@ export const updateWorkspacePermissionsContract = defineRouteContract({ path: '/api/workspaces/[id]/permissions', params: workspaceParamsSchema, body: updateWorkspacePermissionsBodySchema, + /** + * Acknowledgement only. The roster this used to echo was discarded by every + * caller — the members list is owned by the GET above and refetched on + * settle — so building it cost three queries per role change and made a + * post-commit read failure able to report an applied change as a 500. + */ response: { mode: 'json', - schema: workspacePermissionsResponseSchema.extend({ - message: z.string(), - }), + schema: z.object({ message: z.string() }), }, }) diff --git a/apps/sim/lib/db/transaction.ts b/apps/sim/lib/db/transaction.ts new file mode 100644 index 00000000000..91339811d19 --- /dev/null +++ b/apps/sim/lib/db/transaction.ts @@ -0,0 +1,56 @@ +import { db } from '@sim/db' +import { createLogger } from '@sim/logger' +import { getPostgresErrorCode } from '@sim/utils/errors' +import { sleep } from '@sim/utils/helpers' +import { backoffWithJitter } from '@sim/utils/retry' +import type { DbOrTx } from '@/lib/db/types' + +const logger = createLogger('DbTransaction') + +/** + * `serialization_failure` and `deadlock_detected`. Postgres raises both only + * after aborting and fully rolling back the transaction, which is what makes a + * fresh attempt safe rather than a duplicate. + */ +const RETRYABLE_TRANSACTION_CODES = new Set(['40001', '40P01']) + +const DEFAULT_ATTEMPTS = 3 + +export function isRetryableTransactionError(error: unknown): boolean { + const code = getPostgresErrorCode(error) + return code !== undefined && RETRYABLE_TRANSACTION_CODES.has(code) +} + +/** + * Runs `fn` in a transaction, retrying when Postgres aborts it as a deadlock + * victim or on a serialization failure. + * + * Both are the database asking the loser to try again, and neither leaves + * partial work behind — so without a retry they surface as a generic 500 for a + * condition that would have succeeded on a second attempt. Any other error, + * including the typed domain errors a caller throws to force a rollback, + * propagates on the first occurrence. + * + * `fn` must be free of side effects outside the transaction: it can run more + * than once, and only the committing attempt is durable. + */ +export async function withTransactionRetry( + fn: (tx: DbOrTx) => Promise, + options: { attempts?: number; label?: string } = {} +): Promise { + const attempts = options.attempts ?? DEFAULT_ATTEMPTS + + for (let attempt = 1; ; attempt += 1) { + try { + return await db.transaction(fn) + } catch (error) { + if (attempt >= attempts || !isRetryableTransactionError(error)) throw error + logger.warn('Retrying transaction aborted by the database', { + label: options.label, + attempt, + code: getPostgresErrorCode(error), + }) + await sleep(backoffWithJitter(attempt, null, { baseMs: 25, maxMs: 200 })) + } + } +} diff --git a/apps/sim/lib/workspaces/permissions/utils.test.ts b/apps/sim/lib/workspaces/permissions/utils.test.ts index 5d1a0379c58..64d1aa2011d 100644 --- a/apps/sim/lib/workspaces/permissions/utils.test.ts +++ b/apps/sim/lib/workspaces/permissions/utils.test.ts @@ -196,6 +196,7 @@ describe('Permission Utils', () => { isExternal: false, joinedAt: '2026-04-22T00:00:00.000Z', roleSource: 'explicit', + isBilledAccount: false, }, ]) }) diff --git a/apps/sim/lib/workspaces/permissions/utils.ts b/apps/sim/lib/workspaces/permissions/utils.ts index 8832e56641e..f890badd998 100644 --- a/apps/sim/lib/workspaces/permissions/utils.ts +++ b/apps/sim/lib/workspaces/permissions/utils.ts @@ -281,6 +281,11 @@ export interface WorkspaceMemberWithRole { * derived and cannot be changed through the member UI. */ roleSource: MemberRoleSource + /** + * The account the workspace bills to. Its role is pinned to `admin` by the + * workspace-permissions route, so the member UI must not offer to change it. + */ + isBilledAccount: boolean } export async function getUsersWithPermissions( @@ -318,6 +323,7 @@ export async function getUsersWithPermissions( isExternal: !isOwner && row.userOrganizationId !== ws.organizationId, joinedAt: row.joinedAt.toISOString(), roleSource: isOwner ? 'owner' : 'explicit', + isBilledAccount: row.userId === ws.billedAccountUserId, }) } @@ -358,6 +364,7 @@ export async function getUsersWithPermissions( isExternal: false, joinedAt: row.joinedAt.toISOString(), roleSource: isOwner ? 'owner' : 'org-admin', + isBilledAccount: row.userId === ws.billedAccountUserId, }) } } From 21820e02acb5032ee124516e5c9c2f7eb45b6a31 Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Sat, 1 Aug 2026 18:48:20 -0700 Subject: [PATCH 2/2] improvement(permissions): retry lock timeouts and audit admin upserts truthfully MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bounding the row-lock wait with `lock_timeout` made a competing writer raise 55P03, which the retry set excluded — so the bounded wait failed the request where the unbounded one would have waited. 55P03 is now retried like the other contention aborts, the timeout is shortened since each attempt releases its pooled connection, and contention that outlives every attempt answers with a busy conflict instead of a generic server error. The admin member upsert recorded MEMBER_ADDED even when the conflict branch amended an existing membership, putting a join that never happened in the workspace audit trail. It now records a role change, matching the action the response already reported. Adds the missing test coverage for withTransactionRetry. Co-Authored-By: Claude --- .../v1/admin/workspaces/[id]/members/route.ts | 21 ++++-- .../workspaces/[id]/permissions/route.test.ts | 23 ++++++ .../api/workspaces/[id]/permissions/route.ts | 46 ++++++++++-- apps/sim/lib/db/transaction.test.ts | 71 +++++++++++++++++++ apps/sim/lib/db/transaction.ts | 29 +++++--- 5 files changed, 171 insertions(+), 19 deletions(-) create mode 100644 apps/sim/lib/db/transaction.test.ts diff --git a/apps/sim/app/api/v1/admin/workspaces/[id]/members/route.ts b/apps/sim/app/api/v1/admin/workspaces/[id]/members/route.ts index c113359e5b3..1b3b429a9d3 100644 --- a/apps/sim/app/api/v1/admin/workspaces/[id]/members/route.ts +++ b/apps/sim/app/api/v1/admin/workspaces/[id]/members/route.ts @@ -290,18 +290,27 @@ export const POST = withRouteHandler( /** A returned id we did not mint means the conflict branch ran. */ const wasCreated = written?.id === permissionId - logger.info(`Admin API: Added user ${userId} to workspace ${workspaceId}`, { - permissions: permissionLevel, - permissionId, - }) + logger.info( + wasCreated + ? `Admin API: Added user ${userId} to workspace ${workspaceId}` + : `Admin API: Updated user ${userId} permissions in workspace ${workspaceId}`, + { permissions: permissionLevel, permissionId } + ) + /** + * The conflict branch amended a membership that already existed, so it is + * a role change rather than an addition — recording it as `MEMBER_ADDED` + * would put a join that never happened in the workspace's audit trail. + */ recordAudit({ workspaceId, actorId: 'admin-api', - action: AuditAction.MEMBER_ADDED, + action: wasCreated ? AuditAction.MEMBER_ADDED : AuditAction.MEMBER_ROLE_CHANGED, resourceType: AuditResourceType.WORKSPACE, resourceId: workspaceId, - description: `Admin API added member to workspace with ${permissionLevel} permissions`, + description: wasCreated + ? `Admin API added member to workspace with ${permissionLevel} permissions` + : `Admin API changed workspace member permissions to ${permissionLevel}`, metadata: { targetUserId: userId, permissions: permissionLevel }, request, }) diff --git a/apps/sim/app/api/workspaces/[id]/permissions/route.test.ts b/apps/sim/app/api/workspaces/[id]/permissions/route.test.ts index c4ad6368788..b163b2bb428 100644 --- a/apps/sim/app/api/workspaces/[id]/permissions/route.test.ts +++ b/apps/sim/app/api/workspaces/[id]/permissions/route.test.ts @@ -314,6 +314,29 @@ describe('workspace permissions route', () => { expect(dbChainMockFns.set).not.toHaveBeenCalled() }) + /** + * The `lock_timeout` this route sets makes Postgres abort the transaction + * under contention. Retries cover the transient case; when they run out the + * caller must still get something actionable, not the driver error rendered + * as "Internal server error". + */ + it('answers contention that outlives the retries with a busy conflict', async () => { + queuePersonalWorkspace([permissionRow(ADMIN_ID, 'admin'), permissionRow(MEMBER_ID, 'read')]) + dbChainMockFns.transaction.mockRejectedValue( + Object.assign(new Error('canceling statement due to lock timeout'), { code: '55P03' }) + ) + + const response = await PATCH( + createMockRequest('PATCH', { updates: [{ userId: MEMBER_ID, permissions: 'write' }] }), + routeContext + ) + + expect(response.status).toBe(409) + await expect(response.json()).resolves.toMatchObject({ + error: 'This workspace is busy right now. Try again in a moment.', + }) + }) + it('aborts when the workspace leaves its organization mid-request', async () => { queuePersonalWorkspace([permissionRow(ADMIN_ID, 'admin'), permissionRow(MEMBER_ID, 'read')]) permissionsMockFns.mockGetWorkspaceWithOwner.mockResolvedValue({ diff --git a/apps/sim/app/api/workspaces/[id]/permissions/route.ts b/apps/sim/app/api/workspaces/[id]/permissions/route.ts index 288b262ff37..4d5809de87d 100644 --- a/apps/sim/app/api/workspaces/[id]/permissions/route.ts +++ b/apps/sim/app/api/workspaces/[id]/permissions/route.ts @@ -3,6 +3,7 @@ import { db } from '@sim/db' import { member, permissions, user, workspace, workspaceEnvironment } from '@sim/db/schema' import { createLogger } from '@sim/logger' import { isOrgAdminRole, ORG_ADMIN_ROLES } from '@sim/platform-authz/workspace' +import { getPostgresErrorCode } from '@sim/utils/errors' import { and, eq, inArray, sql } from 'drizzle-orm' import { type NextRequest, NextResponse } from 'next/server' import { @@ -14,7 +15,7 @@ import { getSession } from '@/lib/auth' import { HttpError } from '@/lib/core/utils/http-error' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { syncWorkspaceEnvCredentials } from '@/lib/credentials/environment' -import { withTransactionRetry } from '@/lib/db/transaction' +import { isRetryableTransactionError, withTransactionRetry } from '@/lib/db/transaction' import type { DbOrTx } from '@/lib/db/types' import { captureServerEvent } from '@/lib/posthog/server' import { @@ -75,11 +76,31 @@ class WorkspaceContextChangedError extends HttpError { } } +/** + * Another writer held the rows for longer than every attempt allowed. Distinct + * from the conflicts above: nothing about the request is wrong and the state is + * unchanged, so the answer is to retry rather than to reload — and it must not + * reach the client as a generic server error, which is what bounding the lock + * wait would otherwise have produced. + */ +class WorkspaceBusyError extends HttpError { + readonly statusCode = 409 + + constructor() { + super('This workspace is busy right now. Try again in a moment.') + this.name = 'WorkspaceBusyError' + } +} + /** * Bounds the wait on the row locks below so a stuck holder fails fast * (SQLSTATE 55P03) instead of parking a pooled connection indefinitely. + * + * Kept short because `withTransactionRetry` retries that timeout: the connection + * is released between attempts, so three bounded waits contend better than one + * long one and never pin a pool slot for more than this. */ -const PERMISSIONS_LOCK_TIMEOUT_MS = 5000 +const PERMISSIONS_LOCK_TIMEOUT_MS = 3000 /** Organization owners/admins among `userIds`, empty for a personal workspace. */ async function loadOrgAdminTargets( @@ -315,10 +336,11 @@ export const PATCH = withRouteHandler( } /** - * Retried on a deadlock: the ordered locks below close this route against + * Retried on contention: the ordered locks below close this route against * itself, but member removal takes the billed account before the departing - * user, so a cross-path cycle remains. The victim of one is asked by the - * database to try again, and answering it with a 500 would surface as + * user, so a cross-path cycle remains — and the `lock_timeout` above turns a + * slow competing writer into an abort of its own. Both are the database + * asking for a retry, and answering either with a 500 would surface as * "Internal server error" for a click that would have worked. */ const { previousRoles, changedUserIds } = await withTransactionRetry(async (tx) => { @@ -491,6 +513,20 @@ export const PATCH = withRouteHandler( previousRoles: lockedByUserId, changedUserIds: new Set(changedUpdates.map((update) => update.userId)), } + }).catch((error) => { + /** + * Contention that outlived every attempt. Answering with the driver error + * would render as "Internal server error" for a request that is simply + * queued behind another writer. + */ + if (isRetryableTransactionError(error)) { + logger.warn('Permission update exhausted retries under contention', { + workspaceId, + code: getPostgresErrorCode(error), + }) + throw new WorkspaceBusyError() + } + throw error }) /** diff --git a/apps/sim/lib/db/transaction.test.ts b/apps/sim/lib/db/transaction.test.ts new file mode 100644 index 00000000000..783dab66e64 --- /dev/null +++ b/apps/sim/lib/db/transaction.test.ts @@ -0,0 +1,71 @@ +/** + * @vitest-environment node + */ +import { dbChainMockFns, resetDbChainMock } from '@sim/testing' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { isRetryableTransactionError, withTransactionRetry } from '@/lib/db/transaction' + +/** Shaped like the `postgres` driver's error, which carries `code` on the error. */ +function pgError(code: string): Error { + return Object.assign(new Error(`pg error ${code}`), { code }) +} + +describe('withTransactionRetry', () => { + beforeEach(() => { + vi.clearAllMocks() + resetDbChainMock() + }) + + describe('isRetryableTransactionError', () => { + /** + * `55P03` is what a caller's own `lock_timeout` raises. Excluding it would + * make bounding the lock wait worse than not bounding it. + */ + it.each(['40001', '40P01', '55P03'])('treats %s as retryable', (code) => { + expect(isRetryableTransactionError(pgError(code))).toBe(true) + }) + + it.each(['23505', '23503', '42P01'])('treats %s as terminal', (code) => { + expect(isRetryableTransactionError(pgError(code))).toBe(false) + }) + + it('treats a non-postgres error as terminal', () => { + expect(isRetryableTransactionError(new Error('boom'))).toBe(false) + }) + }) + + it('returns the callback result without retrying when it succeeds', async () => { + const fn = vi.fn().mockResolvedValue('ok') + + await expect(withTransactionRetry(fn)).resolves.toBe('ok') + expect(dbChainMockFns.transaction).toHaveBeenCalledTimes(1) + }) + + it.each(['40001', '40P01', '55P03'])( + 'retries a %s abort and returns the later result', + async (code) => { + const fn = vi.fn().mockRejectedValueOnce(pgError(code)).mockResolvedValue('recovered') + + await expect(withTransactionRetry(fn)).resolves.toBe('recovered') + expect(fn).toHaveBeenCalledTimes(2) + } + ) + + it('gives up after the attempt cap and rethrows the driver error', async () => { + const fn = vi.fn().mockRejectedValue(pgError('40P01')) + + await expect(withTransactionRetry(fn, { attempts: 3 })).rejects.toMatchObject({ code: '40P01' }) + expect(fn).toHaveBeenCalledTimes(3) + }) + + /** + * The typed domain errors callers throw to force a rollback must not be + * replayed — a second attempt would reach the same decision. + */ + it('does not retry an error the database did not raise', async () => { + const fn = vi.fn().mockRejectedValue(new Error('membership changed')) + + await expect(withTransactionRetry(fn)).rejects.toThrow('membership changed') + expect(fn).toHaveBeenCalledTimes(1) + }) +}) diff --git a/apps/sim/lib/db/transaction.ts b/apps/sim/lib/db/transaction.ts index 91339811d19..9e4e2e7f960 100644 --- a/apps/sim/lib/db/transaction.ts +++ b/apps/sim/lib/db/transaction.ts @@ -8,11 +8,16 @@ import type { DbOrTx } from '@/lib/db/types' const logger = createLogger('DbTransaction') /** - * `serialization_failure` and `deadlock_detected`. Postgres raises both only - * after aborting and fully rolling back the transaction, which is what makes a - * fresh attempt safe rather than a duplicate. + * `serialization_failure`, `deadlock_detected`, and `lock_not_available`. + * Postgres raises all three only after aborting the transaction, so the work is + * fully rolled back and a fresh attempt is a retry rather than a duplicate. + * + * `55P03` is what a caller's own `lock_timeout` produces: it means a competing + * transaction still held the row, which is the case most likely to succeed on a + * second attempt. Excluding it would make bounding the wait strictly worse than + * not bounding it — the request would fail instead of waiting. */ -const RETRYABLE_TRANSACTION_CODES = new Set(['40001', '40P01']) +const RETRYABLE_TRANSACTION_CODES = new Set(['40001', '40P01', '55P03']) const DEFAULT_ATTEMPTS = 3 @@ -22,15 +27,23 @@ export function isRetryableTransactionError(error: unknown): boolean { } /** - * Runs `fn` in a transaction, retrying when Postgres aborts it as a deadlock - * victim or on a serialization failure. + * Runs `fn` in a transaction, retrying when Postgres aborts it for contention — + * a deadlock, a serialization failure, or a lock timeout. * - * Both are the database asking the loser to try again, and neither leaves - * partial work behind — so without a retry they surface as a generic 500 for a + * All three are the database asking the loser to try again, and none leaves + * partial work behind, so without a retry they surface as a generic 500 for a * condition that would have succeeded on a second attempt. Any other error, * including the typed domain errors a caller throws to force a rollback, * propagates on the first occurrence. * + * Each attempt is its own transaction, so the pooled connection is released + * between them — pairing a short `lock_timeout` with a retry holds a connection + * for far less time than a single unbounded wait would. + * + * When the attempts are exhausted the original Postgres error propagates; + * callers that answer HTTP should map it with {@link isRetryableTransactionError} + * rather than let it become a generic 500. + * * `fn` must be free of side effects outside the transaction: it can run more * than once, and only the committing attempt is durable. */