From 88f97742e3da35ef1438416f68b0859fd06fca36 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 8 Aug 2026 14:46:24 -0700 Subject: [PATCH] fix(knowledge): stop listing workspace knowledge bases on stale creator identity GET /api/knowledge without a workspaceId ORed on knowledge_base.user_id with no permission check, so a user removed from a workspace kept seeing metadata for every KB they created there. Scope the creator fallback to legacy KBs with no workspaceId, matching the workspace-filtered branch and the detail path. --- apps/sim/lib/knowledge/service.test.ts | 70 ++++++++++++++++++++- apps/sim/lib/knowledge/service.ts | 41 ++++++------ packages/testing/src/mocks/database.mock.ts | 2 + 3 files changed, 91 insertions(+), 22 deletions(-) diff --git a/apps/sim/lib/knowledge/service.test.ts b/apps/sim/lib/knowledge/service.test.ts index 8b9ca673f2d..7d2511fc40b 100644 --- a/apps/sim/lib/knowledge/service.test.ts +++ b/apps/sim/lib/knowledge/service.test.ts @@ -1,7 +1,15 @@ /** * @vitest-environment node */ -import { dbChainMockFns, permissionsMock, permissionsMockFns, resetDbChainMock } from '@sim/testing' +import { + dbChainMockFns, + flattenMockConditions, + hasMockCondition, + permissionsMock, + permissionsMockFns, + resetDbChainMock, + schemaMock, +} from '@sim/testing' import { beforeEach, describe, expect, it, vi } from 'vitest' const { @@ -31,7 +39,63 @@ vi.mock('@/lib/billing/core/usage', () => ({ ensureUserStatsExists: mockEnsureUserStatsExists, })) -import { KnowledgeBasePermissionError, updateKnowledgeBase } from '@/lib/knowledge/service' +import { + getKnowledgeBases, + KnowledgeBasePermissionError, + updateKnowledgeBase, +} from '@/lib/knowledge/service' + +/** + * The listing query authorizes on current workspace membership, never on stale creator + * identity: a user removed from a workspace must stop seeing knowledge bases they created + * there. The creator fallback exists only for legacy knowledge bases with no `workspaceId`. + */ +describe('getKnowledgeBases — creator fallback is scoped to legacy non-workspace KBs', () => { + beforeEach(() => { + vi.clearAllMocks() + resetDbChainMock() + }) + + /** Every disjunct that grants on `knowledgeBase.userId`, from the last select chain's WHERE. */ + const capturedCreatorBranches = (): unknown[] => { + const [condition] = dbChainMockFns.where.mock.calls.at(-1) ?? [] + const orNode = flattenMockConditions(condition).find((node) => node.type === 'or') + expect(orNode, 'WHERE clause has no or(...) branch').toBeDefined() + return (orNode?.conditions as unknown[]).filter((disjunct) => + hasMockCondition( + disjunct, + (node) => + node.type === 'eq' && + node.left === schemaMock.knowledgeBase.userId && + node.right === 'user-a' + ) + ) + } + + /** The creator fallback must be the sole grant for legacy KBs and never reach workspace KBs. */ + const expectCreatorBranchIsLegacyOnly = () => { + const branches = capturedCreatorBranches() + expect(branches).toHaveLength(1) + expect( + hasMockCondition( + branches[0], + (node) => node.type === 'isNull' && node.column === schemaMock.knowledgeBase.workspaceId + ) + ).toBe(true) + } + + it('requires workspaceId IS NULL on the creator branch when no workspace filter is given', async () => { + await getKnowledgeBases('user-a', undefined, 'all') + + expectCreatorBranchIsLegacyOnly() + }) + + it('keeps the same guard on the workspace-filtered branch', async () => { + await getKnowledgeBases('user-a', 'ws-1', 'active') + + expectCreatorBranchIsLegacyOnly() + }) +}) /** * These tests guard the workspace mass-assignment fix: @@ -82,7 +146,7 @@ describe('updateKnowledgeBase — workspace transfer authorization', () => { await expect( updateKnowledgeBase('kb-1', { workspaceId: null }, 'req-1', { actorUserId: 'owner' }) - ).rejects.not.toBeInstanceOf(KnowledgeBasePermissionError) + ).resolves.toBeDefined() expect(permissionsMockFns.mockGetUserEntityPermissions).not.toHaveBeenCalled() }) diff --git a/apps/sim/lib/knowledge/service.ts b/apps/sim/lib/knowledge/service.ts index cbac1283b63..2a8445b7339 100644 --- a/apps/sim/lib/knowledge/service.ts +++ b/apps/sim/lib/knowledge/service.ts @@ -104,6 +104,21 @@ export async function getKnowledgeBases( ? sql`${knowledgeBase.deletedAt} IS NOT NULL` : isNull(knowledgeBase.deletedAt) + /** + * Legacy knowledge bases predate workspaces and have no `workspaceId`, so the creator is + * their only possible authority. Anything with a `workspaceId` must clear + * `currentWorkspaceMembership` instead — creator identity goes stale the moment a member + * is removed from the workspace. + */ + const legacyOwnedKnowledgeBase = and( + eq(knowledgeBase.userId, userId), + isNull(knowledgeBase.workspaceId) + ) + const currentWorkspaceMembership = and( + isNotNull(permissions.userId), + isNull(workspace.archivedAt) + ) + const knowledgeBasesWithCounts = await db .select({ id: knowledgeBase.id, @@ -143,25 +158,13 @@ export async function getKnowledgeBases( .where( and( scopeCondition, - workspaceId - ? // When filtering by workspace - or( - // Knowledge bases belonging to the specified workspace (user must have workspace permissions) - and( - eq(knowledgeBase.workspaceId, workspaceId), - isNotNull(permissions.userId), - isNull(workspace.archivedAt) - ), - // Fallback: User-owned knowledge bases without workspace (legacy) - and(eq(knowledgeBase.userId, userId), isNull(knowledgeBase.workspaceId)) - ) - : // When not filtering by workspace, use original logic - or( - // User owns the knowledge base directly - eq(knowledgeBase.userId, userId), - // User has permissions on the knowledge base's workspace - and(isNotNull(permissions.userId), isNull(workspace.archivedAt)) - ) + or( + and( + workspaceId ? eq(knowledgeBase.workspaceId, workspaceId) : undefined, + currentWorkspaceMembership + ), + legacyOwnedKnowledgeBase + ) ) ) .groupBy(knowledgeBase.id) diff --git a/packages/testing/src/mocks/database.mock.ts b/packages/testing/src/mocks/database.mock.ts index 61d152233b7..e6da8a5b51e 100644 --- a/packages/testing/src/mocks/database.mock.ts +++ b/packages/testing/src/mocks/database.mock.ts @@ -29,6 +29,8 @@ export function createMockSql() { toSQL: () => ({ sql: strings.join('?'), params: values }), /** Mirrors drizzle's `sql``…`.as(alias)` for aliased select expressions. */ as: (alias: string) => ({ ...fragment, alias }), + /** Mirrors drizzle's `sql``…`.mapWith(decoder)` for typed select expressions. */ + mapWith: (decoder: unknown) => ({ ...fragment, decoder }), } return fragment }