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..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 @@ -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,28 +264,53 @@ 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, - }) - - logger.info(`Admin API: Added user ${userId} to workspace ${workspaceId}`, { - permissions: permissionLevel, - permissionId, - }) - + /** + * 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( + 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, }) @@ -288,16 +330,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..b163b2bb428 --- /dev/null +++ b/apps/sim/app/api/workspaces/[id]/permissions/route.test.ts @@ -0,0 +1,643 @@ +/** + * @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() + }) + + /** + * 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({ + 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..4d5809de87d 100644 --- a/apps/sim/app/api/workspaces/[id]/permissions/route.ts +++ b/apps/sim/app/api/workspaces/[id]/permissions/route.ts @@ -2,24 +2,160 @@ 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 { getPostgresErrorCode } from '@sim/utils/errors' +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 { isRetryableTransactionError, 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' + } +} + +/** + * 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 = 3000 + +/** 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 +167,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 +190,398 @@ 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) + + 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) - const hasAdminAccess = await hasWorkspaceAdminAccess(session.user.id, workspaceId) + 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 - if (!hasAdminAccess) { - return NextResponse.json( - { error: 'Admin access required to update permissions' }, - { status: 403 } + 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 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 — 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) => { + 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]) - ) + and(eq(member.organizationId, organizationId), inArray(member.userId, lockUserIds)) ) - if (orgAdminTargets.length > 0) { - return NextResponse.json( - { error: 'Organization admins are workspace admins and their role cannot be changed' }, - { status: 400 } - ) - } + .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)), + } + }).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 + }) + + /** + * 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 } } + ) - 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(), - }) - } + /** + * `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 + + 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 +595,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.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 new file mode 100644 index 00000000000..9e4e2e7f960 --- /dev/null +++ b/apps/sim/lib/db/transaction.ts @@ -0,0 +1,69 @@ +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`, `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', '55P03']) + +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 for contention — + * a deadlock, a serialization failure, or a lock timeout. + * + * 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. + */ +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, }) } }