From 930d9fc489ee403b52d7ccf17d2d96f86a572ff4 Mon Sep 17 00:00:00 2001 From: Thomas Camlong Date: Sun, 16 Aug 2026 21:25:14 +0200 Subject: [PATCH] fix: address CodeRabbit review findings - Apply app search permission filter after limit (access filtering before truncation) - Allow board-modify-all users to search apps (spotlight regression) - Skip integration permission queries for anonymous sessions - Add deterministic sort tie-breakers (kind, name, id) - Extract shared integration access relations helper - Replace double type assertion with typed lookup in savePermissions - Add regression tests: board-modify-all app search, integration-create bundling, global full-access branch, matrix round-trip expansion --- packages/api/src/router/app.ts | 13 ++++- packages/api/src/router/group.ts | 12 ++--- packages/api/src/router/home.ts | 7 +-- .../router/integration/integration-router.ts | 47 ++++++++++--------- packages/api/src/router/test/app.spec.ts | 19 +++++++- packages/api/src/router/test/group.spec.ts | 29 ++++++++++++ .../integration/integration-router.spec.ts | 30 ++++++++++++ .../definitions/src/test/permissions.spec.ts | 5 ++ 8 files changed, 124 insertions(+), 38 deletions(-) diff --git a/packages/api/src/router/app.ts b/packages/api/src/router/app.ts index d0e4b4b18..c3605bb78 100644 --- a/packages/api/src/router/app.ts +++ b/packages/api/src/router/app.ts @@ -69,8 +69,7 @@ export const appRouter = createTRPCRouter({ orderBy: asc(apps.name), }); }), - search: permissionRequiredProcedure - .requiresPermission("app-modify-all") + search: protectedProcedure .input( z.object({ query: z.string(), @@ -91,6 +90,16 @@ export const appRouter = createTRPCRouter({ }, }) .query(({ ctx, input }) => { + if ( + !ctx.session.user.permissions.includes("app-modify-all") && + !ctx.session.user.permissions.includes("board-modify-all") + ) { + throw new TRPCError({ + code: "FORBIDDEN", + message: "Permission denied", + }); + } + return ctx.db.query.apps.findMany({ where: like(apps.name, `%${input.query}%`), orderBy: asc(apps.name), diff --git a/packages/api/src/router/group.ts b/packages/api/src/router/group.ts index 5396af94d..41d2b2c91 100644 --- a/packages/api/src/router/group.ts +++ b/packages/api/src/router/group.ts @@ -26,7 +26,7 @@ import { createTRPCRouter, onboardingProcedure, permissionRequiredProcedure, pro const createPermissionParents = { "app-create": "app-modify-all", "integration-create": "integration-full-all", - "board-create": "board-modify-all", + "board-create": "board-full-all", "search-engine-create": "search-engine-modify-all", } satisfies Partial>; @@ -312,10 +312,8 @@ export const groupRouter = createTRPCRouter({ const permissions = [ ...new Set( - input.permissions.flatMap((permission) => { - const parent = createPermissionParents[permission as keyof typeof createPermissionParents] as - | GroupPermissionKey - | undefined; + input.permissions.flatMap((permission): GroupPermissionKey[] => { + const parent = permission in createPermissionParents ? createPermissionParents[permission] : null; return parent ? [permission, parent] : [permission]; }), ), @@ -324,9 +322,7 @@ export const groupRouter = createTRPCRouter({ await handleTransactionsAsync(ctx.db, { async handleAsync(db, schema) { await db.transaction(async (transaction) => { - await transaction - .delete(schema.groupPermissions) - .where(eq(schema.groupPermissions.groupId, input.groupId)); + await transaction.delete(schema.groupPermissions).where(eq(schema.groupPermissions.groupId, input.groupId)); if (permissions.length === 0) { return; } diff --git a/packages/api/src/router/home.ts b/packages/api/src/router/home.ts index c7ed9c138..b30dab329 100644 --- a/packages/api/src/router/home.ts +++ b/packages/api/src/router/home.ts @@ -124,9 +124,10 @@ export const homeRouter = createTRPCRouter({ } const hasGlobalFullIntegrationAccess = ctx.session?.user.permissions.includes("integration-full-all") ?? false; - const fullAccessIntegrationIds = hasGlobalFullIntegrationAccess - ? [] - : await getFullAccessIntegrationIdsAsync(ctx.db, ctx.session?.user.id ?? ""); + const fullAccessIntegrationIds = + hasGlobalFullIntegrationAccess || !ctx.session?.user + ? [] + : await getFullAccessIntegrationIdsAsync(ctx.db, ctx.session.user.id); if (hasGlobalFullIntegrationAccess || fullAccessIntegrationIds.length > 0) { statistics.push({ titleKey: "integration", diff --git a/packages/api/src/router/integration/integration-router.ts b/packages/api/src/router/integration/integration-router.ts index d1235f9c8..a3a20f6e9 100644 --- a/packages/api/src/router/integration/integration-router.ts +++ b/packages/api/src/router/integration/integration-router.ts @@ -119,21 +119,18 @@ export const integrationRouter = createTRPCRouter({ const integrationsFromDb = await ctx.db.query.integrations.findMany({ where: like(integrations.name, `%${input.query}%`), orderBy: asc(integrations.name), - with: { - userPermissions: { - where: eq(integrationUserPermissions.userId, ctx.session.user.id), - }, - groupPermissions: { - where: inArray( - integrationGroupPermissions.groupId, - groupsOfCurrentUser.map((group) => group.groupId), - ), - }, - }, + with: getIntegrationAccessRelationsAsync( + ctx.session.user.id, + groupsOfCurrentUser.map((group) => group.groupId), + ), }); return integrationsFromDb .filter((integration) => hasIntegrationQueryAccess(ctx.session, integration)) + .toSorted( + (integrationA, integrationB) => + integrationA.name.localeCompare(integrationB.name) || integrationA.id.localeCompare(integrationB.id), + ) .slice(0, input.limit) .map( ({ userPermissions: _userPermissions, groupPermissions: _groupPermissions, ...integration }) => integration, @@ -719,6 +716,15 @@ const hasIntegrationQueryAccess = ( }, ) => hasGlobalIntegrationAccess(session) || constructIntegrationPermissions(integration, session).hasUseAccess; +const getIntegrationAccessRelationsAsync = (userId: string, groupIds: string[]) => ({ + userPermissions: { + where: eq(integrationUserPermissions.userId, userId), + }, + groupPermissions: { + where: inArray(integrationGroupPermissions.groupId, groupIds), + }, +}); + const getAccessibleIntegrationsAsync = async (ctx: { db: Database; session: Session }, where?: SQL) => { const groupsOfCurrentUser = await ctx.db.query.groupMembers.findMany({ where: eq(groupMembers.userId, ctx.session.user.id), @@ -726,17 +732,10 @@ const getAccessibleIntegrationsAsync = async (ctx: { db: Database; session: Sess const integrationsFromDb = await ctx.db.query.integrations.findMany({ where, - with: { - userPermissions: { - where: eq(integrationUserPermissions.userId, ctx.session.user.id), - }, - groupPermissions: { - where: inArray( - integrationGroupPermissions.groupId, - groupsOfCurrentUser.map((group) => group.groupId), - ), - }, - }, + with: getIntegrationAccessRelationsAsync( + ctx.session.user.id, + groupsOfCurrentUser.map((group) => group.groupId), + ), }); return integrationsFromDb @@ -754,7 +753,9 @@ const getAccessibleIntegrationsAsync = async (ctx: { db: Database; session: Sess }) .toSorted( (integrationA, integrationB) => - integrationKinds.indexOf(integrationA.kind) - integrationKinds.indexOf(integrationB.kind), + integrationKinds.indexOf(integrationA.kind) - integrationKinds.indexOf(integrationB.kind) || + integrationA.name.localeCompare(integrationB.name) || + integrationA.id.localeCompare(integrationB.id), ); }; diff --git a/packages/api/src/router/test/app.spec.ts b/packages/api/src/router/test/app.spec.ts index 6706ca6a4..b2368ef6d 100644 --- a/packages/api/src/router/test/app.spec.ts +++ b/packages/api/src/router/test/app.spec.ts @@ -127,8 +127,8 @@ describe("getPaginated should return apps to authenticated users", () => { }); }); -describe("search should return apps to authenticated users", () => { - test("should throw FORBIDDEN without app-modify-all permission", async () => { +describe("search should require app-modify-all or board-modify-all permission", () => { + test("should throw FORBIDDEN without app-modify-all or board-modify-all permission", async () => { const caller = appRouter.createCaller({ db: createDb(), deviceType: undefined, @@ -137,6 +137,21 @@ describe("search should return apps to authenticated users", () => { await expect(caller.search({ query: "Homarr" })).rejects.toThrow("Permission denied"); }); + + test("should return apps with board-modify-all permission", async () => { + const db = createDb(); + const caller = appRouter.createCaller({ + db, + deviceType: undefined, + session: createDefaultSession(["board-modify-all"]), + }); + await db.insert(apps).values({ id: "1", name: "Homarr", iconUrl: "https://homarr.dev/icon.svg" }); + + const result = await caller.search({ query: "Homarr" }); + + expect(result).toHaveLength(1); + expect(result[0]!.id).toBe("1"); + }); }); describe("byId should return an app by id", () => { diff --git a/packages/api/src/router/test/group.spec.ts b/packages/api/src/router/test/group.spec.ts index 8af77faa9..961b3b8c5 100644 --- a/packages/api/src/router/test/group.spec.ts +++ b/packages/api/src/router/test/group.spec.ts @@ -475,6 +475,35 @@ describe("savePermissions should save permissions for group", () => { expect(permissions.map(({ permission }) => permission).toSorted()).toEqual(["app-create", "app-modify-all"]); }); + test("with an integration create permission it should persist its parent permission", async () => { + // Arrange + const db = createDb(); + const caller = groupRouter.createCaller({ db, deviceType: undefined, session: adminSession }); + + const groupId = createId(); + await db.insert(groups).values({ + id: groupId, + name: "Group", + position: 1, + }); + + // Act + await caller.savePermissions({ + groupId, + permissions: ["integration-create"], + }); + + // Assert + const permissions = await db.query.groupPermissions.findMany({ + where: eq(groupPermissions.groupId, groupId), + }); + + expect(permissions.map(({ permission }) => permission).toSorted()).toEqual([ + "integration-create", + "integration-full-all", + ]); + }); + test("with a create permission and its parent it should persist exactly two rows", async () => { // Arrange const db = createDb(); diff --git a/packages/api/src/router/test/integration/integration-router.spec.ts b/packages/api/src/router/test/integration/integration-router.spec.ts index fde85f951..a14aa3780 100644 --- a/packages/api/src/router/test/integration/integration-router.spec.ts +++ b/packages/api/src/router/test/integration/integration-router.spec.ts @@ -59,6 +59,36 @@ describe("all should return all integrations", () => { expect(result.every(({ permissions }) => permissions.hasFullAccess && permissions.hasUseAccess)).toBe(true); }); + test("with integration full access and no per-integration rows should return all integrations with full access", async () => { + const db = createDb(); + const caller = integrationRouter.createCaller({ + db, + deviceType: undefined, + session: defaultSessionWithPermissions(["integration-full-all"]), + }); + + await db.insert(integrations).values([ + { + id: "1", + name: "Home assistant", + kind: "homeAssistant", + url: "http://homeassist.local", + }, + { + id: "2", + name: "Home plex server", + kind: "plex", + url: "http://plex.local", + }, + ]); + + const result = await caller.all(); + expect(result).toHaveLength(2); + for (const integration of result) { + expect(integration.permissions.hasFullAccess).toBe(true); + } + }); + test("with integration use access should return all integrations", async () => { const db = createDb(); const caller = integrationRouter.createCaller({ diff --git a/packages/definitions/src/test/permissions.spec.ts b/packages/definitions/src/test/permissions.spec.ts index e74a938f0..5c8da676a 100644 --- a/packages/definitions/src/test/permissions.spec.ts +++ b/packages/definitions/src/test/permissions.spec.ts @@ -116,4 +116,9 @@ describe("matrix helpers should round-trip stably", () => { expect(roundTripped).toEqual(expect.arrayContaining(permissions)); expect(roundTripped).toHaveLength(permissions.length); }); + + test("expect a single higher-level key to expand to its full set on round-trip", () => { + const roundTripped = matrixStateToPermissions(permissionsToMatrixState(["board-full-all"])); + expect(roundTripped).toEqual(["board-view-all", "board-modify-all", "board-full-all"]); + }); });