Skip to content

Commit f143f51

Browse files
committed
Merge branch 'pgx/h1' into feat/permission-groups-coverage
2 parents 029c4b2 + 92cc06c commit f143f51

32 files changed

Lines changed: 992 additions & 112 deletions
Lines changed: 158 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,158 @@
1+
/**
2+
* @vitest-environment node
3+
*
4+
* The raw `/api/table/**` routes that authenticate with
5+
* `checkSessionOrInternalAuth` accept an internal executor JWT, whose `userId`
6+
* is the subject the executor embedded rather than a person asking for
7+
* anything. Reading it bare applies that person's permission group to a
8+
* delegation the executor exemption deliberately passes ungated — so these pin
9+
* the derivation (`capabilityGovernedAuthUserId`) at each gate, on a group
10+
* whose config would refuse if it were consulted.
11+
*/
12+
import {
13+
hybridAuthMockFns,
14+
permissionGroupScopeMock,
15+
permissionGroupScopeMockFns,
16+
resetPermissionGroupScopeMock,
17+
} from '@sim/testing'
18+
import { NextRequest } from 'next/server'
19+
import { beforeEach, describe, expect, it, vi } from 'vitest'
20+
21+
const mocks = vi.hoisted(() => ({
22+
listWorkspaceExportJobs: vi.fn(),
23+
checkWorkspaceAccess: vi.fn(),
24+
getUserEntityPermissions: vi.fn(),
25+
createTable: vi.fn(),
26+
listTables: vi.fn(),
27+
getWorkspaceTableLimits: vi.fn(),
28+
findActiveFolder: vi.fn(),
29+
getUserSettings: vi.fn(),
30+
runDetached: vi.fn(),
31+
runTableImport: vi.fn(),
32+
}))
33+
34+
vi.mock('@/lib/permission-groups/config-scope.server', () => permissionGroupScopeMock)
35+
vi.mock('@/lib/table/jobs/service', () => ({
36+
listWorkspaceExportJobs: mocks.listWorkspaceExportJobs,
37+
}))
38+
vi.mock('@/lib/workspaces/permissions/utils', () => ({
39+
checkWorkspaceAccess: mocks.checkWorkspaceAccess,
40+
getUserEntityPermissions: mocks.getUserEntityPermissions,
41+
}))
42+
vi.mock('@/lib/table', () => ({
43+
createTable: mocks.createTable,
44+
deleteTable: vi.fn(),
45+
getWorkspaceTableLimits: mocks.getWorkspaceTableLimits,
46+
listTables: mocks.listTables,
47+
releaseJobClaim: vi.fn(),
48+
sanitizeName: (name: string) => name,
49+
TABLE_LIMITS: { MAX_TABLE_NAME_LENGTH: 64 },
50+
}))
51+
vi.mock('@/lib/table/import-runner', () => ({ runTableImport: mocks.runTableImport }))
52+
vi.mock('@/lib/folders/queries', () => ({ findActiveFolder: mocks.findActiveFolder }))
53+
vi.mock('@/lib/users/queries', () => ({ getUserSettings: mocks.getUserSettings }))
54+
vi.mock('@/lib/core/utils/background', () => ({ runDetached: mocks.runDetached }))
55+
vi.mock('@/lib/core/config/env-flags', () => ({ isTriggerDevEnabled: false }))
56+
vi.mock('@/lib/posthog/server', () => ({ captureServerEvent: vi.fn() }))
57+
58+
import { DEFAULT_PERMISSION_GROUP_CONFIG } from '@/lib/permission-groups/fields'
59+
import { POST as importAsync } from '@/app/api/table/import-async/route'
60+
import { GET as listJobs } from '@/app/api/table/jobs/route'
61+
62+
const WORKSPACE_ID = '11111111-1111-4111-8111-111111111111'
63+
const TABLE_ID = '22222222-2222-4222-8222-222222222222'
64+
const ACTOR_ID = 'run-actor'
65+
66+
/** The run's actor, embedded in the executor's internal JWT. */
67+
function authenticateAsExecutor() {
68+
hybridAuthMockFns.mockCheckSessionOrInternalAuth.mockResolvedValue({
69+
success: true,
70+
userId: ACTOR_ID,
71+
authType: 'internal_jwt',
72+
})
73+
}
74+
75+
/** The same person, calling the same route from their own browser session. */
76+
function authenticateAsSession() {
77+
hybridAuthMockFns.mockCheckSessionOrInternalAuth.mockResolvedValue({
78+
success: true,
79+
userId: ACTOR_ID,
80+
authType: 'session',
81+
})
82+
}
83+
84+
function getExportJobs() {
85+
return listJobs(
86+
new NextRequest(`http://localhost/api/table/jobs?workspaceId=${WORKSPACE_ID}&type=export`)
87+
)
88+
}
89+
90+
function startImport() {
91+
return importAsync(
92+
new NextRequest('http://localhost/api/table/import-async', {
93+
method: 'POST',
94+
body: JSON.stringify({
95+
workspaceId: WORKSPACE_ID,
96+
fileKey: `workspace/${WORKSPACE_ID}/upload.csv`,
97+
fileName: 'upload.csv',
98+
}),
99+
headers: { 'content-type': 'application/json' },
100+
})
101+
)
102+
}
103+
104+
describe('the subject the raw table routes gate on', () => {
105+
beforeEach(() => {
106+
vi.clearAllMocks()
107+
resetPermissionGroupScopeMock()
108+
mocks.checkWorkspaceAccess.mockResolvedValue({ hasAccess: true })
109+
mocks.getUserEntityPermissions.mockResolvedValue('admin')
110+
mocks.listWorkspaceExportJobs.mockResolvedValue([{ id: 'job-1' }])
111+
mocks.listTables.mockResolvedValue([])
112+
mocks.getWorkspaceTableLimits.mockResolvedValue({ maxTables: 100 })
113+
mocks.getUserSettings.mockResolvedValue({ timezone: 'UTC' })
114+
mocks.createTable.mockResolvedValue({ id: TABLE_ID })
115+
permissionGroupScopeMockFns.mockResolvePermissionGroupConfig.mockResolvedValue({
116+
...DEFAULT_PERMISSION_GROUP_CONFIG,
117+
hideTablesTab: true,
118+
disableTableExport: true,
119+
})
120+
})
121+
122+
describe('an executor delegation carrying the actor’s id', () => {
123+
beforeEach(authenticateAsExecutor)
124+
125+
it('lists the workspace’s export jobs without consulting the actor’s group', async () => {
126+
const response = await getExportJobs()
127+
128+
expect(await response.json()).toEqual({ success: true, data: { jobs: [{ id: 'job-1' }] } })
129+
expect(permissionGroupScopeMockFns.mockResolvePermissionGroupConfig).not.toHaveBeenCalled()
130+
})
131+
132+
it('starts an import without consulting the actor’s group', async () => {
133+
const response = await startImport()
134+
135+
expect(response.status).toBe(200)
136+
expect(mocks.createTable).toHaveBeenCalled()
137+
expect(permissionGroupScopeMockFns.mockResolvePermissionGroupConfig).not.toHaveBeenCalled()
138+
})
139+
})
140+
141+
describe('the same person on their own session', () => {
142+
beforeEach(authenticateAsSession)
143+
144+
it('is handed an empty export tray', async () => {
145+
const response = await getExportJobs()
146+
147+
expect(await response.json()).toEqual({ success: true, data: { jobs: [] } })
148+
expect(mocks.listWorkspaceExportJobs).not.toHaveBeenCalled()
149+
})
150+
151+
it('is refused the import, and no table is created', async () => {
152+
const response = await startImport()
153+
154+
expect(response.status).toBe(403)
155+
expect(mocks.createTable).not.toHaveBeenCalled()
156+
})
157+
})
158+
})

apps/sim/app/api/table/import-async/route.ts

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { generateId } from '@sim/utils/id'
33
import { type NextRequest, NextResponse } from 'next/server'
44
import { importTableAsyncContract } from '@/lib/api/contracts/tables'
55
import { parseRequest } from '@/lib/api/server'
6-
import { checkSessionOrInternalAuth } from '@/lib/auth/hybrid'
6+
import { capabilityGovernedAuthUserId, checkSessionOrInternalAuth } from '@/lib/auth/hybrid'
77
import { isTriggerDevEnabled } from '@/lib/core/config/env-flags'
88
import { runDetached } from '@/lib/core/utils/background'
99
import { generateRequestId } from '@/lib/core/utils/request'
@@ -54,9 +54,18 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
5454
* and predates the operation boundary. An import always ends in a new table,
5555
* so it is creation, not ordinary use. `tables.create` subsumes `tables.use`:
5656
* its rule is denied by `disableTableCreation` OR `hideTablesTab`, so gating
57-
* on it still refuses a group that hides Tables entirely.
57+
* on it still refuses a group that hides Tables entirely. Keyed to the
58+
* governed subject, which names nobody for an internal-JWT executor call —
59+
* the same rule the synchronous `import-csv` route applies. Not re-read before
60+
* `createTable` below: `resolvePermissionGroupConfig` is memoized per request
61+
* (`withPermissionGroupScope`), so a second call in this handler returns the
62+
* promise this one started and could not observe a revocation.
5863
*/
59-
if (await isWorkspaceCapabilityWithheld(userId, workspaceId, 'tables.create')) {
64+
const governedUserId = capabilityGovernedAuthUserId(authResult)
65+
if (
66+
governedUserId &&
67+
(await isWorkspaceCapabilityWithheld(governedUserId, workspaceId, 'tables.create'))
68+
) {
6069
return capabilityRefusalResponse('tables.create')
6170
}
6271
// The fileKey is client-supplied — ensure it points at this workspace's storage prefix so a

apps/sim/app/api/table/jobs/route.ts

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { createLogger } from '@sim/logger'
22
import { type NextRequest, NextResponse } from 'next/server'
33
import { listTableJobsContract } from '@/lib/api/contracts/tables'
44
import { parseRequest } from '@/lib/api/server'
5-
import { checkSessionOrInternalAuth } from '@/lib/auth/hybrid'
5+
import { capabilityGovernedAuthUserId, checkSessionOrInternalAuth } from '@/lib/auth/hybrid'
66
import { generateRequestId } from '@/lib/core/utils/request'
77
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
88
import { isWorkspaceCapabilityWithheld } from '@/lib/permission-groups/capability-assertions'
@@ -43,8 +43,17 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
4343
* export file. Withheld as an empty list rather than a refusal: the caller has
4444
* no exports they may act on, and erroring the tray would report a failure
4545
* where the honest answer is that there is nothing to show.
46+
*
47+
* Keyed to the governed subject, which names nobody for an internal-JWT
48+
* executor call: `authResult.userId` there is the subject the executor
49+
* embedded, so reading it bare would hand the run's actor's group to a caller
50+
* the executor exemption deliberately passes ungated.
4651
*/
47-
if (await isWorkspaceCapabilityWithheld(authResult.userId, workspaceId, 'tables.export')) {
52+
const governedUserId = capabilityGovernedAuthUserId(authResult)
53+
if (
54+
governedUserId &&
55+
(await isWorkspaceCapabilityWithheld(governedUserId, workspaceId, 'tables.export'))
56+
) {
4857
return NextResponse.json({ success: true, data: { jobs: [] } })
4958
}
5059

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { webhook } from '@sim/db/schema'
5+
import {
6+
auditMock,
7+
createMockRequest,
8+
hybridAuthMockFns,
9+
permissionGroupScopeMock,
10+
permissionGroupScopeMockFns,
11+
posthogServerMock,
12+
queueTableRows,
13+
resetDbChainMock,
14+
telemetryMock,
15+
workflowAuthzMockFns,
16+
} from '@sim/testing'
17+
import { beforeEach, describe, expect, it, vi } from 'vitest'
18+
19+
vi.mock('@sim/audit', () => auditMock)
20+
vi.mock('@/lib/permission-groups/config-scope.server', () => permissionGroupScopeMock)
21+
vi.mock('@/lib/core/telemetry', () => telemetryMock)
22+
vi.mock('@/lib/posthog/server', () => posthogServerMock)
23+
vi.mock('@/lib/webhooks/provider-subscriptions', () => ({ cleanupExternalWebhook: vi.fn() }))
24+
25+
import { DEFAULT_PERMISSION_GROUP_CONFIG } from '@/lib/permission-groups/fields'
26+
import { PATCH } from '@/app/api/webhooks/[id]/route'
27+
28+
const ACTOR_ID = 'actor-1'
29+
30+
function reactivate() {
31+
return PATCH(createMockRequest('PATCH', { isActive: true }), {
32+
params: Promise.resolve({ id: 'webhook-1' }),
33+
})
34+
}
35+
36+
/** The single joined read the PATCH handler issues. */
37+
function queueDormantWebhook(): void {
38+
queueTableRows(webhook, [
39+
{
40+
webhook: { id: 'webhook-1', isActive: false, failedCount: 0 },
41+
workflow: { id: 'workflow-1', userId: ACTOR_ID, workspaceId: 'workspace-1' },
42+
},
43+
])
44+
}
45+
46+
/**
47+
* `triggers.webhook` is withheld from the actor's group. Flipping a dormant
48+
* webhook back on is the act the key names, so a session belonging to that
49+
* person must be refused — and an executor delegation carrying the same id
50+
* must not be, since it holds the actor's role and none of their capabilities.
51+
*/
52+
describe('the subject the webhook reactivation gate is decided about', () => {
53+
beforeEach(() => {
54+
vi.clearAllMocks()
55+
resetDbChainMock()
56+
workflowAuthzMockFns.mockAuthorizeWorkflowByWorkspacePermission.mockResolvedValue({
57+
allowed: true,
58+
status: 200,
59+
workflow: { id: 'workflow-1' },
60+
workspacePermission: 'write',
61+
})
62+
workflowAuthzMockFns.mockAssertWorkflowMutable.mockResolvedValue(undefined)
63+
permissionGroupScopeMockFns.mockResolvePermissionGroupConfig.mockResolvedValue({
64+
...DEFAULT_PERMISSION_GROUP_CONFIG,
65+
disableWebhookTriggers: true,
66+
})
67+
})
68+
69+
it('refuses the actor’s own session', async () => {
70+
hybridAuthMockFns.mockCheckSessionOrInternalAuth.mockResolvedValue({
71+
success: true,
72+
userId: ACTOR_ID,
73+
authType: 'session',
74+
})
75+
queueDormantWebhook()
76+
77+
const response = await reactivate()
78+
79+
expect(response.status).toBe(403)
80+
})
81+
82+
it('lets an internal executor JWT through without consulting that group', async () => {
83+
hybridAuthMockFns.mockCheckSessionOrInternalAuth.mockResolvedValue({
84+
success: true,
85+
userId: ACTOR_ID,
86+
authType: 'internal_jwt',
87+
})
88+
queueDormantWebhook()
89+
90+
const response = await reactivate()
91+
92+
expect(response.status).toBe(200)
93+
expect(permissionGroupScopeMockFns.mockResolvePermissionGroupConfig).not.toHaveBeenCalled()
94+
})
95+
})

apps/sim/app/api/webhooks/[id]/route.ts

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ import {
1515
updateWebhookContract,
1616
} from '@/lib/api/contracts/webhooks'
1717
import { parseRequest } from '@/lib/api/server'
18-
import { checkSessionOrInternalAuth } from '@/lib/auth/hybrid'
18+
import { capabilityGovernedAuthUserId, checkSessionOrInternalAuth } from '@/lib/auth/hybrid'
1919
import { PlatformEvents } from '@/lib/core/telemetry'
2020
import { generateRequestId } from '@/lib/core/utils/request'
2121
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
@@ -150,10 +150,15 @@ export const PATCH = withRouteHandler(
150150
* dormant webhook back on. Only this direction: deactivating must stay
151151
* open, or a policy change would strand a member with a live webhook
152152
* they cannot turn off.
153+
*
154+
* Keyed to the governed subject rather than `auth.userId`: an internal
155+
* executor JWT embeds the run's actor, and gating on it would apply that
156+
* person's capabilities to a delegation that carries only their role.
153157
*/
154-
if (isActive) {
158+
const governedUserId = capabilityGovernedAuthUserId(auth)
159+
if (isActive && governedUserId) {
155160
const withheld = await isWorkspaceCapabilityWithheld(
156-
userId,
161+
governedUserId,
157162
webhooks[0].workflow.workspaceId ?? '',
158163
'triggers.webhook'
159164
)

apps/sim/background/resume-execution.ts

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -401,6 +401,7 @@ async function continueCascadeAfterResume(
401401
const { getTableById } = await import('@/lib/table/service')
402402
const { getRowById } = await import('@/lib/table/rows/service')
403403
const { pickNextEligibleGroupForRow } = await import('@/lib/table/workflow-columns')
404+
const { readStampedCapabilitySubject } = await import('@/lib/table/rows/executions')
404405
const { runRowCascadeLoop } = await import('@/background/workflow-column-execution')
405406

406407
const freshTable = await getTableById(cellContext.tableId)
@@ -409,6 +410,8 @@ async function continueCascadeAfterResume(
409410
if (!freshRow) return
410411
const next = pickNextEligibleGroupForRow(freshTable, freshRow, cellContext.groupId)
411412
if (!next) return
413+
const nextExec = freshRow.executions?.[next.id]
414+
const isQueuedMarker = nextExec?.status === 'pending' && nextExec.executionId == null
412415
await runRowCascadeLoop(
413416
{
414417
tableId: cellContext.tableId,
@@ -424,8 +427,17 @@ async function continueCascadeAfterResume(
424427
* cascades into. Reconstructing this from the resume payload is not
425428
* possible — `payload.userId` is the resumer/attribution, not the gate —
426429
* so it rides the pause snapshot instead.
430+
*
431+
* Unless the next group carries another dispatch's unclaimed pre-stamp:
432+
* that is an explicit request from someone else that this cascade happens
433+
* to be draining, and it runs under the subject persisted with it. The
434+
* same decision both drain points in `workflow-column-execution.ts` make;
435+
* a resume that skipped it would hand a stranger's request the paused
436+
* cell's gate.
427437
*/
428-
capabilityGovernedUserId: cellContext.capabilityGovernedUserId,
438+
capabilityGovernedUserId: isQueuedMarker
439+
? await readStampedCapabilitySubject(cellContext.rowId, next.id)
440+
: cellContext.capabilityGovernedUserId,
429441
},
430442
signal
431443
)

0 commit comments

Comments
 (0)