Skip to content

Commit 7b31f55

Browse files
PlaneInABottleGitHub Copilot
authored and
test
committed
fix: harden workflow API auth follow-ups
Co-authored-by: GitHub Copilot <github-copilot[bot]@users.noreply.github.com>
1 parent 0616aa6 commit 7b31f55

File tree

7 files changed

+71
-131
lines changed

7 files changed

+71
-131
lines changed

‎apps/sim/app/api/workflows/[id]/deploy/route.test.ts‎

Lines changed: 36 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'
77

88
const {
99
mockCleanupWebhooksForWorkflow,
10+
mockRecordAudit,
1011
mockDbLimit,
1112
mockDbOrderBy,
1213
mockDbFrom,
@@ -23,8 +24,10 @@ const {
2324
mockUndeployWorkflow,
2425
mockValidatePublicApiAllowed,
2526
mockValidateWorkflowAccess,
27+
mockValidateWorkflowPermissions,
2628
} = vi.hoisted(() => ({
2729
mockCleanupWebhooksForWorkflow: vi.fn(),
30+
mockRecordAudit: vi.fn(),
2831
mockDbLimit: vi.fn(),
2932
mockDbOrderBy: vi.fn(),
3033
mockDbFrom: vi.fn(),
@@ -41,14 +44,15 @@ const {
4144
mockUndeployWorkflow: vi.fn(),
4245
mockValidatePublicApiAllowed: vi.fn(),
4346
mockValidateWorkflowAccess: vi.fn(),
47+
mockValidateWorkflowPermissions: vi.fn(),
4448
}))
4549

4650
vi.mock('@sim/logger', () => ({
4751
createLogger: () => ({ info: vi.fn(), warn: vi.fn(), error: vi.fn() }),
4852
}))
4953

5054
vi.mock('@/lib/workflows/utils', () => ({
51-
validateWorkflowPermissions: vi.fn(),
55+
validateWorkflowPermissions: (...args: unknown[]) => mockValidateWorkflowPermissions(...args),
5256
}))
5357

5458
vi.mock('@/app/api/workflows/middleware', () => ({
@@ -105,7 +109,7 @@ vi.mock('@/lib/mcp/workflow-mcp-sync', () => ({
105109
vi.mock('@/lib/audit/log', () => ({
106110
AuditAction: {},
107111
AuditResourceType: {},
108-
recordAudit: vi.fn(),
112+
recordAudit: (...args: unknown[]) => mockRecordAudit(...args),
109113
}))
110114

111115
vi.mock('@/ee/access-control/utils/permission-check', () => ({
@@ -142,7 +146,13 @@ describe('Workflow deploy route', () => {
142146
it('allows API-key auth for deploy using hybrid auth userId', async () => {
143147
mockValidateWorkflowAccess.mockResolvedValue({
144148
workflow: { id: 'wf-1', name: 'Test Workflow', workspaceId: 'ws-1' },
145-
auth: { success: true, userId: 'api-user', authType: 'api_key' },
149+
auth: {
150+
success: true,
151+
userId: 'api-user',
152+
userName: 'API Key User',
153+
userEmail: 'api@example.com',
154+
authType: 'api_key',
155+
},
146156
})
147157
mockDeployWorkflow.mockResolvedValue({
148158
success: true,
@@ -164,12 +174,26 @@ describe('Workflow deploy route', () => {
164174
deployedBy: 'api-user',
165175
workflowName: 'Test Workflow',
166176
})
177+
expect(mockValidateWorkflowPermissions).not.toHaveBeenCalled()
178+
expect(mockRecordAudit).toHaveBeenCalledWith(
179+
expect.objectContaining({
180+
actorId: 'api-user',
181+
actorName: 'API Key User',
182+
actorEmail: 'api@example.com',
183+
})
184+
)
167185
})
168186

169187
it('allows API-key auth for undeploy using hybrid auth userId', async () => {
170188
mockValidateWorkflowAccess.mockResolvedValue({
171189
workflow: { id: 'wf-1', name: 'Test Workflow', workspaceId: 'ws-1' },
172-
auth: { success: true, userId: 'api-user', authType: 'api_key' },
190+
auth: {
191+
success: true,
192+
userId: 'api-user',
193+
userName: 'API Key User',
194+
userEmail: 'api@example.com',
195+
authType: 'api_key',
196+
},
173197
})
174198
mockUndeployWorkflow.mockResolvedValue({ success: true })
175199

@@ -183,6 +207,14 @@ describe('Workflow deploy route', () => {
183207
const data = await response.json()
184208
expect(data.isDeployed).toBe(false)
185209
expect(mockUndeployWorkflow).toHaveBeenCalledWith({ workflowId: 'wf-1' })
210+
expect(mockValidateWorkflowPermissions).not.toHaveBeenCalled()
211+
expect(mockRecordAudit).toHaveBeenCalledWith(
212+
expect.objectContaining({
213+
actorId: 'api-user',
214+
actorName: 'API Key User',
215+
actorEmail: 'api@example.com',
216+
})
217+
)
186218
})
187219

188220
it('checks public API restrictions against hybrid auth userId', async () => {

‎apps/sim/app/api/workflows/[id]/deploy/route.ts‎

Lines changed: 12 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ import {
2222
createSchedulesForDeploy,
2323
validateWorkflowSchedules,
2424
} from '@/lib/workflows/schedules'
25-
import { validateWorkflowPermissions } from '@/lib/workflows/utils'
2625
import { validateWorkflowAccess } from '@/app/api/workflows/middleware'
2726
import { createErrorResponse, createSuccessResponse } from '@/app/api/workflows/utils'
2827
import type { WorkflowState } from '@/stores/workflows/workflow/types'
@@ -35,18 +34,12 @@ export const runtime = 'nodejs'
3534
type LifecycleAdminAccessResult = {
3635
error: { message: string; status: number } | null | undefined
3736
auth: AuthResult | null | undefined
38-
session: Awaited<ReturnType<typeof validateWorkflowPermissions>>['session'] | null | undefined
39-
workflow:
40-
| Awaited<ReturnType<typeof validateWorkflowPermissions>>['workflow']
41-
| Awaited<ReturnType<typeof validateWorkflowAccess>>['workflow']
42-
| null
43-
| undefined
37+
workflow: Awaited<ReturnType<typeof validateWorkflowAccess>>['workflow'] | null | undefined
4438
}
4539

4640
async function validateLifecycleAdminAccess(
4741
request: NextRequest,
48-
workflowId: string,
49-
requestId: string
42+
workflowId: string
5043
): Promise<LifecycleAdminAccessResult> {
5144
const hybridAccess = await validateWorkflowAccess(request, workflowId, {
5245
requireDeployment: false,
@@ -57,35 +50,13 @@ async function validateLifecycleAdminAccess(
5750
return {
5851
error: hybridAccess.error,
5952
auth: hybridAccess.auth,
60-
session: null,
6153
workflow: hybridAccess.workflow,
6254
}
6355
}
6456

65-
if (hybridAccess.auth?.authType === 'session') {
66-
const sessionAccess = await validateWorkflowPermissions(workflowId, requestId, 'admin')
67-
const auth: AuthResult | null = sessionAccess.session?.user?.id
68-
? {
69-
success: true,
70-
userId: sessionAccess.session.user.id,
71-
userName: sessionAccess.session.user.name,
72-
userEmail: sessionAccess.session.user.email,
73-
authType: 'session',
74-
}
75-
: null
76-
77-
return {
78-
error: sessionAccess.error,
79-
auth,
80-
session: sessionAccess.session,
81-
workflow: sessionAccess.workflow,
82-
}
83-
}
84-
8557
return {
8658
error: null,
8759
auth: hybridAccess.auth,
88-
session: null,
8960
workflow: hybridAccess.workflow,
9061
}
9162
}
@@ -176,17 +147,12 @@ export async function POST(request: NextRequest, { params }: { params: Promise<{
176147
const { id } = await params
177148

178149
try {
179-
const {
180-
auth,
181-
error,
182-
session,
183-
workflow: workflowData,
184-
} = await validateLifecycleAdminAccess(request, id, requestId)
150+
const { auth, error, workflow: workflowData } = await validateLifecycleAdminAccess(request, id)
185151
if (error) {
186152
return createErrorResponse(error.message, error.status)
187153
}
188154

189-
const actorUserId: string | null = session?.user?.id ?? auth?.userId ?? null
155+
const actorUserId: string | null = auth?.userId ?? null
190156
if (!actorUserId) {
191157
logger.warn(`[${requestId}] Unable to resolve actor user for workflow deployment: ${id}`)
192158
return createErrorResponse('Unable to determine deploying user', 400)
@@ -339,8 +305,8 @@ export async function POST(request: NextRequest, { params }: { params: Promise<{
339305
recordAudit({
340306
workspaceId: workflowData?.workspaceId || null,
341307
actorId: actorUserId,
342-
actorName: session?.user?.name,
343-
actorEmail: session?.user?.email,
308+
actorName: auth?.userName,
309+
actorEmail: auth?.userEmail,
344310
action: AuditAction.WORKFLOW_DEPLOYED,
345311
resourceType: AuditResourceType.WORKFLOW,
346312
resourceId: id,
@@ -384,7 +350,7 @@ export async function PATCH(request: NextRequest, { params }: { params: Promise<
384350
const { id } = await params
385351

386352
try {
387-
const { auth, error, session } = await validateLifecycleAdminAccess(request, id, requestId)
353+
const { auth, error } = await validateLifecycleAdminAccess(request, id)
388354
if (error) {
389355
return createErrorResponse(error.message, error.status)
390356
}
@@ -400,7 +366,7 @@ export async function PATCH(request: NextRequest, { params }: { params: Promise<
400366
const { validatePublicApiAllowed, PublicApiNotAllowedError } = await import(
401367
'@/ee/access-control/utils/permission-check'
402368
)
403-
const actorUserId = session?.user?.id ?? auth?.userId
369+
const actorUserId = auth?.userId
404370
try {
405371
await validatePublicApiAllowed(actorUserId)
406372
} catch (err) {
@@ -431,17 +397,12 @@ export async function DELETE(
431397
const { id } = await params
432398

433399
try {
434-
const {
435-
auth,
436-
error,
437-
session,
438-
workflow: workflowData,
439-
} = await validateLifecycleAdminAccess(request, id, requestId)
400+
const { auth, error, workflow: workflowData } = await validateLifecycleAdminAccess(request, id)
440401
if (error) {
441402
return createErrorResponse(error.message, error.status)
442403
}
443404

444-
const actorUserId = session?.user?.id ?? auth?.userId ?? null
405+
const actorUserId = auth?.userId ?? null
445406
if (!actorUserId) {
446407
return createErrorResponse('Unable to determine undeploying user', 400)
447408
}
@@ -467,8 +428,8 @@ export async function DELETE(
467428
recordAudit({
468429
workspaceId: workflowData?.workspaceId || null,
469430
actorId: actorUserId,
470-
actorName: session?.user?.name,
471-
actorEmail: session?.user?.email,
431+
actorName: auth?.userName,
432+
actorEmail: auth?.userEmail,
472433
action: AuditAction.WORKFLOW_UNDEPLOYED,
473434
resourceType: AuditResourceType.WORKFLOW,
474435
resourceId: id,

‎apps/sim/app/api/workflows/[id]/deployed/route.test.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,6 @@ describe('Workflow deployed-state route', () => {
5656
expect(mockValidateWorkflowAccess).toHaveBeenCalledWith(req, 'wf-1', {
5757
requireDeployment: false,
5858
action: 'read',
59-
allowInternalSecret: false,
6059
})
6160
})
6261
})

‎apps/sim/app/api/workflows/[id]/deployed/route.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,6 @@ export async function GET(request: NextRequest, { params }: { params: Promise<{
3434
const validation = await validateWorkflowAccess(request, id, {
3535
requireDeployment: false,
3636
action: 'read',
37-
allowInternalSecret: false,
3837
})
3938
if (validation.error) {
4039
const response = createErrorResponse(validation.error.message, validation.error.status)

‎apps/sim/app/api/workflows/[id]/route.test.ts‎

Lines changed: 20 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -413,6 +413,7 @@ describe('Workflow By ID API Route', () => {
413413
const response = await DELETE(req, { params })
414414

415415
expect(response.status).toBe(200)
416+
expect(mockAuthorizeWorkflowByWorkspacePermission).not.toHaveBeenCalled()
416417
})
417418

418419
it('should prevent deletion of the last workflow in workspace', async () => {
@@ -453,22 +454,11 @@ describe('Workflow By ID API Route', () => {
453454
})
454455

455456
it.concurrent('should deny deletion for non-admin users', async () => {
456-
const mockWorkflow = {
457-
id: 'workflow-123',
458-
userId: 'other-user',
459-
name: 'Test Workflow',
460-
workspaceId: 'workspace-456',
461-
}
462-
463-
mockGetSession({ user: { id: 'user-123' } })
464-
465-
mockGetWorkflowById.mockResolvedValue(mockWorkflow)
466-
mockAuthorizeWorkflowByWorkspacePermission.mockResolvedValue({
467-
allowed: false,
468-
status: 403,
469-
message: 'Unauthorized: Access denied to admin this workflow',
470-
workflow: mockWorkflow,
471-
workspacePermission: null,
457+
mockValidateWorkflowAccess.mockResolvedValue({
458+
error: {
459+
message: 'Unauthorized: Access denied to admin this workflow',
460+
status: 403,
461+
},
472462
})
473463

474464
const req = new NextRequest('http://localhost:3000/api/workflows/workflow-123', {
@@ -537,6 +527,7 @@ describe('Workflow By ID API Route', () => {
537527
expect(response.status).toBe(200)
538528
const data = await response.json()
539529
expect(data.workflow.name).toBe('Updated Workflow')
530+
expect(mockAuthorizeWorkflowByWorkspacePermission).not.toHaveBeenCalled()
540531
})
541532

542533
it('should allow users with write permission to update workflow', async () => {
@@ -584,24 +575,13 @@ describe('Workflow By ID API Route', () => {
584575
})
585576

586577
it('should deny update for users with only read permission', async () => {
587-
const mockWorkflow = {
588-
id: 'workflow-123',
589-
userId: 'other-user',
590-
name: 'Test Workflow',
591-
workspaceId: 'workspace-456',
592-
}
593-
594578
const updateData = { name: 'Updated Workflow' }
595579

596-
mockGetSession({ user: { id: 'user-123' } })
597-
598-
mockGetWorkflowById.mockResolvedValue(mockWorkflow)
599-
mockAuthorizeWorkflowByWorkspacePermission.mockResolvedValue({
600-
allowed: false,
601-
status: 403,
602-
message: 'Unauthorized: Access denied to write this workflow',
603-
workflow: mockWorkflow,
604-
workspacePermission: 'read',
580+
mockValidateWorkflowAccess.mockResolvedValue({
581+
error: {
582+
message: 'Unauthorized: Access denied to write this workflow',
583+
status: 403,
584+
},
605585
})
606586

607587
const req = new NextRequest('http://localhost:3000/api/workflows/workflow-123', {
@@ -770,7 +750,10 @@ describe('Workflow By ID API Route', () => {
770750

771751
const updatedWorkflow = { ...mockWorkflow, folderId: 'folder-2', updatedAt: new Date() }
772752

773-
mockGetSession({ user: { id: 'user-123' } })
753+
mockValidateWorkflowAccess.mockResolvedValue({
754+
workflow: mockWorkflow,
755+
auth: { success: true, userId: 'user-123', authType: 'session' },
756+
})
774757
mockGetWorkflowById.mockResolvedValue(mockWorkflow)
775758
mockAuthorizeWorkflowByWorkspacePermission.mockResolvedValue({
776759
allowed: true,
@@ -812,7 +795,10 @@ describe('Workflow By ID API Route', () => {
812795
workspaceId: 'workspace-456',
813796
}
814797

815-
mockGetSession({ user: { id: 'user-123' } })
798+
mockValidateWorkflowAccess.mockResolvedValue({
799+
workflow: mockWorkflow,
800+
auth: { success: true, userId: 'user-123', authType: 'session' },
801+
})
816802
mockGetWorkflowById.mockResolvedValue(mockWorkflow)
817803
mockAuthorizeWorkflowByWorkspacePermission.mockResolvedValue({
818804
allowed: true,

0 commit comments

Comments
 (0)