diff --git a/apps/sim/app/api/table/capability-gate.test.ts b/apps/sim/app/api/table/capability-gate.test.ts index 5dd9e2007c3..6707e0839a4 100644 --- a/apps/sim/app/api/table/capability-gate.test.ts +++ b/apps/sim/app/api/table/capability-gate.test.ts @@ -20,14 +20,26 @@ import { import { NextRequest } from 'next/server' import { beforeEach, describe, expect, it, vi } from 'vitest' -const { mockGetTableById, mockGetUserEntityPermissions, mockAddTableColumn, mockListTableViews } = +const { mockGetTableById, mockCheckWorkspaceAccess, mockAddTableColumn, mockListTableViews } = vi.hoisted(() => ({ mockGetTableById: vi.fn(), - mockGetUserEntityPermissions: vi.fn(), + mockCheckWorkspaceAccess: vi.fn(), mockAddTableColumn: vi.fn(), mockListTableViews: vi.fn(), })) +/** The shape `checkAccess` reads: the viewer's permission plus the workspace it just loaded. */ +function workspaceAccess(permission: string | null, organizationId: string | null = 'org-1') { + return { + exists: true, + hasAccess: permission !== null, + canWrite: permission === 'admin' || permission === 'write', + canAdmin: permission === 'admin', + workspace: { id: 'ws-1', organizationId }, + permission, + } +} + vi.mock('@/lib/permission-groups/config-scope.server', () => permissionGroupScopeMock) vi.mock('@/lib/table', () => ({ @@ -44,7 +56,7 @@ vi.mock('@/lib/table/events', () => ({ signalTableSchemaChanged: vi.fn() })) vi.mock('@/lib/table/orchestration', () => ({ performUpdateTableColumn: vi.fn() })) vi.mock('@/lib/table/wire', () => ({ normalizeColumn: (column: unknown) => column })) vi.mock('@/lib/workspaces/permissions/utils', () => ({ - getUserEntityPermissions: mockGetUserEntityPermissions, + checkWorkspaceAccess: mockCheckWorkspaceAccess, })) vi.mock('@/lib/workspaces/utils', () => ({ getWorkspaceOrganizationId: vi.fn() })) @@ -96,11 +108,42 @@ describe('tables.use gate on the raw /api/table routes', () => { authType: 'session', }) mockGetTableById.mockResolvedValue(TABLE) - mockGetUserEntityPermissions.mockResolvedValue('admin') + mockCheckWorkspaceAccess.mockResolvedValue(workspaceAccess('admin')) mockAddTableColumn.mockResolvedValue({ schema: { columns: [{ name: 'expires_at' }] } }) mockListTableViews.mockResolvedValue([]) }) + /** + * The capability resolver looks the workspace up itself when the organization is omitted, so a + * call site that already access-checked the workspace and drops the id pays a second read of a + * value it is holding — once on every raw table route. Asserted on the resolver rather than on + * a query count because that is where the omission would show. + */ + it('hands the capability resolver the organization it just loaded, not undefined', async () => { + mockCheckWorkspaceAccess.mockResolvedValue(workspaceAccess('admin', 'org-42')) + + await listViews() + + expect(permissionGroupScopeMockFns.mockResolvePermissionGroupConfig).toHaveBeenCalledWith( + expect.any(String), + expect.any(String), + 'org-42' + ) + }) + + /** A personal workspace has no organization; `null` is the answer, and still not a lookup. */ + it('passes null for a workspace that belongs to no organization', async () => { + mockCheckWorkspaceAccess.mockResolvedValue(workspaceAccess('admin', null)) + + await listViews() + + expect(permissionGroupScopeMockFns.mockResolvePermissionGroupConfig).toHaveBeenCalledWith( + expect.any(String), + expect.any(String), + null + ) + }) + describe('when the group withholds Tables', () => { beforeEach(() => { permissionGroupScopeMockFns.mockResolvePermissionGroupConfig.mockResolvedValue({ @@ -131,7 +174,7 @@ describe('tables.use gate on the raw /api/table routes', () => { }) it('still conceals a table the caller cannot reach, rather than naming the capability', async () => { - mockGetUserEntityPermissions.mockResolvedValue(null) + mockCheckWorkspaceAccess.mockResolvedValue(workspaceAccess(null)) const response = await listViews() diff --git a/apps/sim/app/api/table/utils.ts b/apps/sim/app/api/table/utils.ts index daf4ed3d838..02a7243179d 100644 --- a/apps/sim/app/api/table/utils.ts +++ b/apps/sim/app/api/table/utils.ts @@ -23,7 +23,7 @@ import { TableLockedError } from '@/lib/table/mutation-locks' import { isTablePredicate } from '@/lib/table/query-builder/converters' import { validateStoragePredicate } from '@/lib/table/query-builder/validate' import type { TableLockKind } from '@/lib/table/types' -import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils' +import { checkWorkspaceAccess } from '@/lib/workspaces/permissions/utils' import { getWorkspaceOrganizationId } from '@/lib/workspaces/utils' /** @@ -338,12 +338,14 @@ export async function checkAccess( return { ok: false, status: 404 } } - const permission = await getUserEntityPermissions( - roleSubjectUserId(principal), - 'workspace', - table.workspaceId - ) - if (!permissionSatisfies(permission, level)) { + /** + * Resolved through {@link checkWorkspaceAccess} rather than `getUserEntityPermissions`, which + * delegates to it and returns the permission alone. Same single resolution, but it also hands + * back the workspace this check just loaded — and with it the owning organization the + * capability gate below would otherwise look up for itself. + */ + const access = await checkWorkspaceAccess(table.workspaceId, roleSubjectUserId(principal)) + if (!permissionSatisfies(access.permission, level)) { return { ok: false, status: 403 } } @@ -352,7 +354,18 @@ export async function checkAccess( if ( governedUserId && table.workspaceId && - (await isWorkspaceCapabilityWithheld(governedUserId, table.workspaceId, 'tables.use')) + /** + * The organization is passed, not re-derived: omitting it makes the resolver load this very + * workspace a second time (see `getUserPermissionConfig`), which is one extra round trip on + * every raw table route. `access.workspace` is non-null on this line — a missing workspace + * resolves to a null permission, which the gate above already refused. + */ + (await isWorkspaceCapabilityWithheld( + governedUserId, + table.workspaceId, + 'tables.use', + access.workspace?.organizationId ?? null + )) ) { return { ok: false, status: 403, capability: 'tables.use' } } diff --git a/apps/sim/app/api/v1/tables/[tableId]/route.test.ts b/apps/sim/app/api/v1/tables/[tableId]/route.test.ts index d649908cb40..ddd3b5a3e5b 100644 --- a/apps/sim/app/api/v1/tables/[tableId]/route.test.ts +++ b/apps/sim/app/api/v1/tables/[tableId]/route.test.ts @@ -17,18 +17,30 @@ const { mockCheckRateLimit, mockCheckWorkspaceScope, mockGetTableById, - mockGetUserEntityPermissions, + mockCheckWorkspaceAccess, mockPerformDeleteTable, mockResolveWorkspaceRequestActor, } = vi.hoisted(() => ({ mockCheckRateLimit: vi.fn(), mockCheckWorkspaceScope: vi.fn(), mockGetTableById: vi.fn(), - mockGetUserEntityPermissions: vi.fn(), + mockCheckWorkspaceAccess: vi.fn(), mockPerformDeleteTable: vi.fn(), mockResolveWorkspaceRequestActor: vi.fn(), })) +/** The shape `checkAccess` reads: the viewer's permission plus the workspace it just loaded. */ +function workspaceAccess(permission: string | null, organizationId: string | null = 'org-1') { + return { + exists: true, + hasAccess: permission !== null, + canWrite: permission === 'admin' || permission === 'write', + canAdmin: permission === 'admin', + workspace: { id: 'ws-1', organizationId }, + permission, + } +} + vi.mock('@/app/api/v1/middleware', () => ({ checkRateLimit: mockCheckRateLimit, checkWorkspaceScope: mockCheckWorkspaceScope, @@ -69,7 +81,10 @@ vi.mock('@/lib/table', () => ({ })) vi.mock('@/lib/workspaces/permissions/utils', () => ({ - getUserEntityPermissions: mockGetUserEntityPermissions, + checkWorkspaceAccess: mockCheckWorkspaceAccess, + /** The v1 middleware reads the permission alone; `checkAccess` reads the whole access. */ + getUserEntityPermissions: async (...args: unknown[]) => + (await mockCheckWorkspaceAccess(...args)).permission, })) vi.mock('@/lib/workspaces/utils', () => ({ @@ -117,7 +132,7 @@ describe('DELETE /api/v1/tables/[tableId] — orchestration failure projection', name: 'Table', workspaceId: WORKSPACE_ID, }) - mockGetUserEntityPermissions.mockResolvedValue('admin') + mockCheckWorkspaceAccess.mockResolvedValue(workspaceAccess('admin')) }) /** diff --git a/apps/sim/app/api/v1/tables/capability-gate.test.ts b/apps/sim/app/api/v1/tables/capability-gate.test.ts index 1936de7a5c5..cf26117a4bb 100644 --- a/apps/sim/app/api/v1/tables/capability-gate.test.ts +++ b/apps/sim/app/api/v1/tables/capability-gate.test.ts @@ -29,20 +29,35 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' const { mockAuthenticateV1Request, - mockGetUserEntityPermissions, + mockCheckWorkspaceAccess, mockGetWorkspaceBillingSettings, mockGetTableById, } = vi.hoisted(() => ({ mockAuthenticateV1Request: vi.fn(), - mockGetUserEntityPermissions: vi.fn(), + mockCheckWorkspaceAccess: vi.fn(), mockGetWorkspaceBillingSettings: vi.fn(), mockGetTableById: vi.fn(), })) +/** The shape `checkAccess` reads: the viewer's permission plus the workspace it just loaded. */ +function workspaceAccess(permission: string | null, organizationId: string | null = 'org-1') { + return { + exists: true, + hasAccess: permission !== null, + canWrite: permission === 'admin' || permission === 'write', + canAdmin: permission === 'admin', + workspace: { id: 'ws-1', organizationId }, + permission, + } +} + vi.mock('@/lib/permission-groups/config-scope.server', () => permissionGroupScopeMock) vi.mock('@/app/api/v1/auth', () => ({ authenticateV1Request: mockAuthenticateV1Request })) vi.mock('@/lib/workspaces/permissions/utils', () => ({ - getUserEntityPermissions: mockGetUserEntityPermissions, + checkWorkspaceAccess: mockCheckWorkspaceAccess, + /** The v1 middleware reads the permission alone; `checkAccess` reads the whole access. */ + getUserEntityPermissions: async (...args: unknown[]) => + (await mockCheckWorkspaceAccess(...args)).permission, })) vi.mock('@/lib/workspaces/utils', () => ({ getWorkspaceBillingSettings: mockGetWorkspaceBillingSettings, @@ -103,7 +118,7 @@ beforeEach(() => { vi.clearAllMocks() resetPermissionGroupScopeMock() mockAuthenticateV1Request.mockResolvedValue(v1PersonalKeyCredential(MEMBER_ID)) - mockGetUserEntityPermissions.mockResolvedValue('admin') + mockCheckWorkspaceAccess.mockResolvedValue(workspaceAccess('admin')) mockGetWorkspaceBillingSettings.mockResolvedValue({ allowPersonalApiKeys: true }) mockGetTableById.mockResolvedValue(TABLE) }) @@ -147,7 +162,7 @@ describe('tables.use gate on /api/v1/tables/[tableId]', () => { }) it('still refuses either key kind on role, before naming the capability', async () => { - mockGetUserEntityPermissions.mockResolvedValue(null) + mockCheckWorkspaceAccess.mockResolvedValue(workspaceAccess(null)) governedBy({ hideTablesTab: true }) const response = await readTable() diff --git a/apps/sim/lib/table/rows/__tests__/ordering-anchor.test.ts b/apps/sim/lib/table/rows/__tests__/ordering-anchor.test.ts index 0818d09a9d3..483cac22bd4 100644 --- a/apps/sim/lib/table/rows/__tests__/ordering-anchor.test.ts +++ b/apps/sim/lib/table/rows/__tests__/ordering-anchor.test.ts @@ -5,7 +5,7 @@ import { describe, expect, it } from 'vitest' import { OrchestrationError } from '@/lib/core/orchestration/types' import type { DbTransaction } from '@/lib/table/planner' import { TableRowNotFoundError } from '@/lib/table/rows/errors' -import { resolveInsertByNeighbor } from '@/lib/table/rows/ordering' +import { appendAnchors, resolveInsertByNeighbor } from '@/lib/table/rows/ordering' /** * A transaction whose anchor lookup finds nothing — the shape a caller produces @@ -54,3 +54,40 @@ describe('resolveInsertByNeighbor > unknown anchor row', () => { expect((error as OrchestrationError).code).toBe('not_found') }) }) + +/** + * An append needs `max(order_key)` and the next `position`, and asks for both in one statement. + * Two separate reads were two serial round trips inside the row-order advisory lock, which every + * other inserting request on the table is queued behind. + */ +describe('appendAnchors', () => { + function recordingTrx(row: { maxKey: string | null; maxPos: number }) { + const projections: Array> = [] + const chain = { + select: (projection: Record) => { + projections.push(projection) + return chain + }, + from: () => chain, + where: async () => [row], + } + return { trx: chain as unknown as DbTransaction, projections } + } + + it('reads both anchors in a single select', async () => { + const { trx, projections } = recordingTrx({ maxKey: 'a5', maxPos: 7 }) + + const anchors = await appendAnchors(trx, 'table-1') + + expect(projections).toHaveLength(1) + expect(Object.keys(projections[0]).sort()).toEqual(['maxKey', 'maxPos']) + expect(anchors).toEqual({ maxOrderKey: 'a5', nextPosition: 8 }) + }) + + /** An empty table has no key to append after, and its first row takes position 0. */ + it('reports the empty table as no key and position zero', async () => { + const { trx } = recordingTrx({ maxKey: null, maxPos: -1 }) + + expect(await appendAnchors(trx, 'table-1')).toEqual({ maxOrderKey: null, nextPosition: 0 }) + }) +}) diff --git a/apps/sim/lib/table/rows/ordering.ts b/apps/sim/lib/table/rows/ordering.ts index feca6ef446c..c0c1dc08d3f 100644 --- a/apps/sim/lib/table/rows/ordering.ts +++ b/apps/sim/lib/table/rows/ordering.ts @@ -172,6 +172,32 @@ export async function nextRowPosition(trx: DbTransaction, tableId: string): Prom return maxPos + 1 } +/** + * The append anchors — `max(order_key)` and the next free `position` — in ONE round trip. + * + * An append needs both, and asking separately is two serial round trips inside the row-order + * advisory lock, which every other inserting request is waiting on. Postgres plans each `max()` + * as its own InitPlan, so the combined statement still serves each aggregate from its own index + * (`(table_id, order_key, id)` and `(table_id, position)`) with an index-only backward scan — + * exactly the two plans the separate queries produced, in one statement rather than two. + * + * Only for the append case. A positional or neighbor-anchored insert resolves its key by walking + * to a slot, which is a different query that cannot fold in. + */ +export async function appendAnchors( + trx: DbTransaction, + tableId: string +): Promise<{ maxOrderKey: string | null; nextPosition: number }> { + const [row] = await trx + .select({ + maxKey: sql`max(${userTableRows.orderKey})`, + maxPos: sql`coalesce(max(${userTableRows.position}), -1)`.mapWith(Number), + }) + .from(userTableRows) + .where(eq(userTableRows.tableId, tableId)) + return { maxOrderKey: row.maxKey ?? null, nextPosition: row.maxPos + 1 } +} + /** Largest `order_key` for a table, or `null` when empty — the append anchor for new keys. */ export async function maxOrderKey(executor: DbOrTx, tableId: string): Promise { const [{ maxKey }] = await executor @@ -331,15 +357,27 @@ export async function insertOrderedRow(params: { await setTableTxTimeouts(trx) await acquireRowOrderLock(trx, tableId) - // Resolve the authoritative order key from neighbor ids when given, else from - // the requested position. - const orderKey = - afterRowId || beforeRowId - ? await resolveInsertByNeighbor(trx, tableId, afterRowId, beforeRowId) - : await resolveInsertOrderKey(trx, tableId, position) - - // order_key is authoritative — keep a best-effort, no-shift position. - const targetPosition = await nextRowPosition(trx, tableId) + // Resolve the authoritative order key from neighbor ids when given, else from the requested + // position. `order_key` is authoritative — `position` is a best-effort, no-shift companion. + // + // A plain append needs only the two table maxima, so it reads them together + // ({@link appendAnchors}) rather than paying a second round trip under the order lock. The + // anchored and positional forms resolve their key by walking to a slot, so they still ask + // for the next position separately. + const appending = !afterRowId && !beforeRowId && position === undefined + let orderKey: string + let targetPosition: number + if (appending) { + const anchors = await appendAnchors(trx, tableId) + orderKey = keyBetween(anchors.maxOrderKey, null) + targetPosition = anchors.nextPosition + } else { + orderKey = + afterRowId || beforeRowId + ? await resolveInsertByNeighbor(trx, tableId, afterRowId, beforeRowId) + : await resolveInsertOrderKey(trx, tableId, position) + targetPosition = await nextRowPosition(trx, tableId) + } const rows = await mutateTableRowsWithSecretProvenance(trx, { rows: [{ rowId, provenance: secretProvenance }],