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
This commit is contained in:
Thomas Camlong committed 2026-08-18 11:05:12 +02:00
1 parent 0839007431
commit 930d9fc489
8 files changed
+124 -38

No files matched your search

+11 -2
View File
@@ -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),
+4 -8
View File
@@ -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<Record<GroupPermissionKey, GroupPermissionKey>>;
@@ -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;
}
+4 -3
View File
@@ -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",
@@ -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),
);
};
+17 -2
View File
@@ -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", () => {
@@ -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();
@@ -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({
@@ -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"]);
});
});