Skip to content

Commit ea6c276

Browse files
committed
fix(audit-log): address PR review — nullable workspaceId, enum usage, remove redundant queries
- Make audit_log.workspace_id nullable with ON DELETE SET NULL (logs survive workspace/user deletion) - Make audit_log.actor_id nullable with ON DELETE SET NULL - Replace all 53 routes' string literal action/resourceType with AuditAction.X and AuditResourceType.X enums - Fix empty workspaceId ('') → null for OAuth, form, and org routes to avoid FK violations - Remove redundant DB queries in chat manage route (use checkChatAccess return data) - Fix organization routes to pass workspaceId: null instead of organizationId
1 parent 0a2d89c commit ea6c276

File tree

65 files changed

+293
-301
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

65 files changed

+293
-301
lines changed

apps/sim/app/api/auth/oauth/disconnect/route.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import { createLogger } from '@sim/logger'
44
import { and, eq, like, or } from 'drizzle-orm'
55
import { type NextRequest, NextResponse } from 'next/server'
66
import { z } from 'zod'
7-
import { recordAudit } from '@/lib/audit/log'
7+
import { AuditAction, AuditResourceType, recordAudit } from '@/lib/audit/log'
88
import { getSession } from '@/lib/auth'
99
import { generateRequestId } from '@/lib/core/utils/request'
1010
import { syncAllWebhooksForCredentialSet } from '@/lib/webhooks/utils.server'
@@ -120,10 +120,10 @@ export async function POST(request: NextRequest) {
120120
}
121121

122122
recordAudit({
123-
workspaceId: '',
123+
workspaceId: null,
124124
actorId: session.user.id,
125-
action: 'oauth.disconnected',
126-
resourceType: 'oauth',
125+
action: AuditAction.OAUTH_DISCONNECTED,
126+
resourceType: AuditResourceType.OAUTH,
127127
resourceId: providerId ?? provider,
128128
actorName: session.user.name ?? undefined,
129129
actorEmail: session.user.email ?? undefined,

apps/sim/app/api/chat/manage/[id]/route.test.ts

Lines changed: 21 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -50,14 +50,7 @@ describe('Chat Edit API Route', () => {
5050
}))
5151

5252
vi.doMock('@sim/db/schema', () => ({
53-
chat: {
54-
id: 'id',
55-
identifier: 'identifier',
56-
userId: 'userId',
57-
workflowId: 'workflowId',
58-
title: 'title',
59-
},
60-
workflow: { id: 'id', workspaceId: 'workspaceId' },
53+
chat: { id: 'id', identifier: 'identifier', userId: 'userId' },
6154
}))
6255

6356
// Mock logger - use loggerMock from @sim/testing
@@ -225,8 +218,11 @@ describe('Chat Edit API Route', () => {
225218
workflowId: 'workflow-123',
226219
}
227220

228-
mockCheckChatAccess.mockResolvedValue({ hasAccess: true, chat: mockChat })
229-
mockLimit.mockResolvedValueOnce([{ workspaceId: 'workspace-123' }])
221+
mockCheckChatAccess.mockResolvedValue({
222+
hasAccess: true,
223+
chat: mockChat,
224+
workspaceId: 'workspace-123',
225+
})
230226

231227
const req = new NextRequest('http://localhost:3000/api/chat/manage/chat-123', {
232228
method: 'PATCH',
@@ -320,8 +316,11 @@ describe('Chat Edit API Route', () => {
320316
workflowId: 'workflow-123',
321317
}
322318

323-
mockCheckChatAccess.mockResolvedValue({ hasAccess: true, chat: mockChat })
324-
mockLimit.mockResolvedValueOnce([{ workspaceId: 'workspace-123' }])
319+
mockCheckChatAccess.mockResolvedValue({
320+
hasAccess: true,
321+
chat: mockChat,
322+
workspaceId: 'workspace-123',
323+
})
325324

326325
const req = new NextRequest('http://localhost:3000/api/chat/manage/chat-123', {
327326
method: 'PATCH',
@@ -380,9 +379,11 @@ describe('Chat Edit API Route', () => {
380379
}),
381380
}))
382381

383-
mockCheckChatAccess.mockResolvedValue({ hasAccess: true })
384-
mockLimit.mockResolvedValueOnce([{ workflowId: 'workflow-123', title: 'Test Chat' }])
385-
mockLimit.mockResolvedValueOnce([{ workspaceId: 'workspace-123' }])
382+
mockCheckChatAccess.mockResolvedValue({
383+
hasAccess: true,
384+
chat: { title: 'Test Chat', workflowId: 'workflow-123' },
385+
workspaceId: 'workspace-123',
386+
})
386387

387388
const req = new NextRequest('http://localhost:3000/api/chat/manage/chat-123', {
388389
method: 'DELETE',
@@ -403,9 +404,11 @@ describe('Chat Edit API Route', () => {
403404
}),
404405
}))
405406

406-
mockCheckChatAccess.mockResolvedValue({ hasAccess: true })
407-
mockLimit.mockResolvedValueOnce([{ workflowId: 'workflow-123', title: 'Test Chat' }])
408-
mockLimit.mockResolvedValueOnce([{ workspaceId: 'workspace-123' }])
407+
mockCheckChatAccess.mockResolvedValue({
408+
hasAccess: true,
409+
chat: { title: 'Test Chat', workflowId: 'workflow-123' },
410+
workspaceId: 'workspace-123',
411+
})
409412

410413
const req = new NextRequest('http://localhost:3000/api/chat/manage/chat-123', {
411414
method: 'DELETE',

apps/sim/app/api/chat/manage/[id]/route.ts

Lines changed: 18 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
import { db } from '@sim/db'
2-
import { chat, workflow } from '@sim/db/schema'
2+
import { chat } from '@sim/db/schema'
33
import { createLogger } from '@sim/logger'
44
import { eq } from 'drizzle-orm'
55
import type { NextRequest } from 'next/server'
66
import { z } from 'zod'
7-
import { recordAudit } from '@/lib/audit/log'
7+
import { AuditAction, AuditResourceType, recordAudit } from '@/lib/audit/log'
88
import { getSession } from '@/lib/auth'
99
import { isDev } from '@/lib/core/config/feature-flags'
1010
import { encryptSecret } from '@/lib/core/security/encryption'
@@ -104,7 +104,11 @@ export async function PATCH(request: NextRequest, { params }: { params: Promise<
104104
try {
105105
const validatedData = chatUpdateSchema.parse(body)
106106

107-
const { hasAccess, chat: existingChatRecord } = await checkChatAccess(chatId, session.user.id)
107+
const {
108+
hasAccess,
109+
chat: existingChatRecord,
110+
workspaceId: chatWorkspaceId,
111+
} = await checkChatAccess(chatId, session.user.id)
108112

109113
if (!hasAccess || !existingChatRecord) {
110114
return createErrorResponse('Chat not found or access denied', 404)
@@ -218,19 +222,13 @@ export async function PATCH(request: NextRequest, { params }: { params: Promise<
218222

219223
logger.info(`Chat "${chatId}" updated successfully`)
220224

221-
const [workflowRecord] = await db
222-
.select({ workspaceId: workflow.workspaceId })
223-
.from(workflow)
224-
.where(eq(workflow.id, existingChat[0].workflowId))
225-
.limit(1)
226-
227225
recordAudit({
228-
workspaceId: workflowRecord?.workspaceId || '',
226+
workspaceId: chatWorkspaceId || null,
229227
actorId: session.user.id,
230228
actorName: session.user.name,
231229
actorEmail: session.user.email,
232-
action: 'chat.updated',
233-
resourceType: 'chat',
230+
action: AuditAction.CHAT_UPDATED,
231+
resourceType: AuditResourceType.CHAT,
234232
resourceId: chatId,
235233
resourceName: title || existingChat[0].title,
236234
description: `Updated chat deployment "${title || existingChat[0].title}"`,
@@ -272,37 +270,27 @@ export async function DELETE(
272270
return createErrorResponse('Unauthorized', 401)
273271
}
274272

275-
const { hasAccess } = await checkChatAccess(chatId, session.user.id)
273+
const {
274+
hasAccess,
275+
chat: chatRecord,
276+
workspaceId: chatWorkspaceId,
277+
} = await checkChatAccess(chatId, session.user.id)
276278

277279
if (!hasAccess) {
278280
return createErrorResponse('Chat not found or access denied', 404)
279281
}
280282

281-
const [chatRecord] = await db
282-
.select({ workflowId: chat.workflowId, title: chat.title })
283-
.from(chat)
284-
.where(eq(chat.id, chatId))
285-
.limit(1)
286-
287-
const [workflowRecord] = chatRecord
288-
? await db
289-
.select({ workspaceId: workflow.workspaceId })
290-
.from(workflow)
291-
.where(eq(workflow.id, chatRecord.workflowId))
292-
.limit(1)
293-
: [undefined]
294-
295283
await db.delete(chat).where(eq(chat.id, chatId))
296284

297285
logger.info(`Chat "${chatId}" deleted successfully`)
298286

299287
recordAudit({
300-
workspaceId: workflowRecord?.workspaceId || '',
288+
workspaceId: chatWorkspaceId || null,
301289
actorId: session.user.id,
302290
actorName: session.user.name,
303291
actorEmail: session.user.email,
304-
action: 'chat.deleted',
305-
resourceType: 'chat',
292+
action: AuditAction.CHAT_DELETED,
293+
resourceType: AuditResourceType.CHAT,
306294
resourceId: chatId,
307295
resourceName: chatRecord?.title || chatId,
308296
description: `Deleted chat deployment "${chatRecord?.title || chatId}"`,

apps/sim/app/api/chat/route.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import { eq } from 'drizzle-orm'
55
import type { NextRequest } from 'next/server'
66
import { v4 as uuidv4 } from 'uuid'
77
import { z } from 'zod'
8-
import { recordAudit } from '@/lib/audit/log'
8+
import { AuditAction, AuditResourceType, recordAudit } from '@/lib/audit/log'
99
import { getSession } from '@/lib/auth'
1010
import { isDev } from '@/lib/core/config/feature-flags'
1111
import { encryptSecret } from '@/lib/core/security/encryption'
@@ -175,7 +175,7 @@ export async function POST(request: NextRequest) {
175175
userId: session.user.id,
176176
identifier,
177177
title,
178-
description: description || '',
178+
description: description || null,
179179
customizations: mergedCustomizations,
180180
isActive: true,
181181
authType,
@@ -226,12 +226,12 @@ export async function POST(request: NextRequest) {
226226
}
227227

228228
recordAudit({
229-
workspaceId: workflowRecord.workspaceId || '',
229+
workspaceId: workflowRecord.workspaceId || null,
230230
actorId: session.user.id,
231231
actorName: session.user.name,
232232
actorEmail: session.user.email,
233-
action: 'chat.deployed',
234-
resourceType: 'chat',
233+
action: AuditAction.CHAT_DEPLOYED,
234+
resourceType: AuditResourceType.CHAT,
235235
resourceId: id,
236236
resourceName: title,
237237
description: `Deployed chat "${title}"`,

apps/sim/app/api/chat/utils.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ export async function checkWorkflowAccessForChatCreation(
5252
export async function checkChatAccess(
5353
chatId: string,
5454
userId: string
55-
): Promise<{ hasAccess: boolean; chat?: any }> {
55+
): Promise<{ hasAccess: boolean; chat?: any; workspaceId?: string }> {
5656
const chatData = await db
5757
.select({
5858
chat: chat,
@@ -78,7 +78,9 @@ export async function checkChatAccess(
7878
action: 'admin',
7979
})
8080

81-
return authorization.allowed ? { hasAccess: true, chat: chatRecord } : { hasAccess: false }
81+
return authorization.allowed
82+
? { hasAccess: true, chat: chatRecord, workspaceId: workflowWorkspaceId }
83+
: { hasAccess: false }
8284
}
8385

8486
export async function validateChatAuth(

apps/sim/app/api/credential-sets/[id]/invite/route.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import { and, eq } from 'drizzle-orm'
55
import { type NextRequest, NextResponse } from 'next/server'
66
import { z } from 'zod'
77
import { getEmailSubject, renderPollingGroupInvitationEmail } from '@/components/emails'
8-
import { recordAudit } from '@/lib/audit/log'
8+
import { AuditAction, AuditResourceType, recordAudit } from '@/lib/audit/log'
99
import { getSession } from '@/lib/auth'
1010
import { hasCredentialSetsAccess } from '@/lib/billing'
1111
import { getBaseUrl } from '@/lib/core/utils/urls'
@@ -179,8 +179,8 @@ export async function POST(req: NextRequest, { params }: { params: Promise<{ id:
179179
recordAudit({
180180
workspaceId: result.set.organizationId,
181181
actorId: session.user.id,
182-
action: 'credential_set_invitation.created',
183-
resourceType: 'credential_set',
182+
action: AuditAction.CREDENTIAL_SET_INVITATION_CREATED,
183+
resourceType: AuditResourceType.CREDENTIAL_SET,
184184
resourceId: id,
185185
actorName: session.user.name ?? undefined,
186186
actorEmail: session.user.email ?? undefined,
@@ -252,8 +252,8 @@ export async function DELETE(req: NextRequest, { params }: { params: Promise<{ i
252252
recordAudit({
253253
workspaceId: result.set.organizationId,
254254
actorId: session.user.id,
255-
action: 'credential_set_invitation.revoked',
256-
resourceType: 'credential_set',
255+
action: AuditAction.CREDENTIAL_SET_INVITATION_REVOKED,
256+
resourceType: AuditResourceType.CREDENTIAL_SET,
257257
resourceId: id,
258258
actorName: session.user.name ?? undefined,
259259
actorEmail: session.user.email ?? undefined,

apps/sim/app/api/credential-sets/[id]/members/route.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { account, credentialSet, credentialSetMember, member, user } from '@sim/
33
import { createLogger } from '@sim/logger'
44
import { and, eq, inArray } from 'drizzle-orm'
55
import { type NextRequest, NextResponse } from 'next/server'
6-
import { recordAudit } from '@/lib/audit/log'
6+
import { AuditAction, AuditResourceType, recordAudit } from '@/lib/audit/log'
77
import { getSession } from '@/lib/auth'
88
import { hasCredentialSetsAccess } from '@/lib/billing'
99
import { syncAllWebhooksForCredentialSet } from '@/lib/webhooks/utils.server'
@@ -181,8 +181,8 @@ export async function DELETE(req: NextRequest, { params }: { params: Promise<{ i
181181
recordAudit({
182182
workspaceId: result.set.organizationId,
183183
actorId: session.user.id,
184-
action: 'credential_set_member.removed',
185-
resourceType: 'credential_set',
184+
action: AuditAction.CREDENTIAL_SET_MEMBER_REMOVED,
185+
resourceType: AuditResourceType.CREDENTIAL_SET,
186186
resourceId: id,
187187
actorName: session.user.name ?? undefined,
188188
actorEmail: session.user.email ?? undefined,

apps/sim/app/api/credential-sets/[id]/route.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import { createLogger } from '@sim/logger'
44
import { and, eq } from 'drizzle-orm'
55
import { type NextRequest, NextResponse } from 'next/server'
66
import { z } from 'zod'
7-
import { recordAudit } from '@/lib/audit/log'
7+
import { AuditAction, AuditResourceType, recordAudit } from '@/lib/audit/log'
88
import { getSession } from '@/lib/auth'
99
import { hasCredentialSetsAccess } from '@/lib/billing'
1010

@@ -135,8 +135,8 @@ export async function PUT(req: NextRequest, { params }: { params: Promise<{ id:
135135
recordAudit({
136136
workspaceId: result.set.organizationId,
137137
actorId: session.user.id,
138-
action: 'credential_set.updated',
139-
resourceType: 'credential_set',
138+
action: AuditAction.CREDENTIAL_SET_UPDATED,
139+
resourceType: AuditResourceType.CREDENTIAL_SET,
140140
resourceId: id,
141141
actorName: session.user.name ?? undefined,
142142
actorEmail: session.user.email ?? undefined,
@@ -192,8 +192,8 @@ export async function DELETE(req: NextRequest, { params }: { params: Promise<{ i
192192
recordAudit({
193193
workspaceId: result.set.organizationId,
194194
actorId: session.user.id,
195-
action: 'credential_set.deleted',
196-
resourceType: 'credential_set',
195+
action: AuditAction.CREDENTIAL_SET_DELETED,
196+
resourceType: AuditResourceType.CREDENTIAL_SET,
197197
resourceId: id,
198198
actorName: session.user.name ?? undefined,
199199
actorEmail: session.user.email ?? undefined,

apps/sim/app/api/credential-sets/route.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import { createLogger } from '@sim/logger'
44
import { and, count, desc, eq } from 'drizzle-orm'
55
import { NextResponse } from 'next/server'
66
import { z } from 'zod'
7-
import { recordAudit } from '@/lib/audit/log'
7+
import { AuditAction, AuditResourceType, recordAudit } from '@/lib/audit/log'
88
import { getSession } from '@/lib/auth'
99
import { hasCredentialSetsAccess } from '@/lib/billing'
1010

@@ -169,8 +169,8 @@ export async function POST(req: Request) {
169169
recordAudit({
170170
workspaceId: organizationId,
171171
actorId: session.user.id,
172-
action: 'credential_set.created',
173-
resourceType: 'credential_set',
172+
action: AuditAction.CREDENTIAL_SET_CREATED,
173+
resourceType: AuditResourceType.CREDENTIAL_SET,
174174
resourceId: newCredentialSet.id,
175175
actorName: session.user.name ?? undefined,
176176
actorEmail: session.user.email ?? undefined,

apps/sim/app/api/folders/[id]/duplicate/route.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import { createLogger } from '@sim/logger'
44
import { and, eq } from 'drizzle-orm'
55
import { type NextRequest, NextResponse } from 'next/server'
66
import { z } from 'zod'
7-
import { recordAudit } from '@/lib/audit/log'
7+
import { AuditAction, AuditResourceType, recordAudit } from '@/lib/audit/log'
88
import { getSession } from '@/lib/auth'
99
import { generateRequestId } from '@/lib/core/utils/request'
1010
import { duplicateWorkflow } from '@/lib/workflows/persistence/duplicate'
@@ -119,8 +119,8 @@ export async function POST(req: NextRequest, { params }: { params: Promise<{ id:
119119
recordAudit({
120120
workspaceId: targetWorkspaceId,
121121
actorId: session.user.id,
122-
action: 'folder.duplicated',
123-
resourceType: 'folder',
122+
action: AuditAction.FOLDER_DUPLICATED,
123+
resourceType: AuditResourceType.FOLDER,
124124
resourceId: newFolderId,
125125
actorName: session.user.name ?? undefined,
126126
actorEmail: session.user.email ?? undefined,

0 commit comments

Comments
 (0)