From 7e933801402dcb98a266e8a29e8fd9633d722929 Mon Sep 17 00:00:00 2001 From: Thomas Camlong Date: Tue, 18 Aug 2026 15:47:26 +0200 Subject: [PATCH] refactor(permissions): move the management access rules into the shared definitions home.getStats re-implemented the same rule the pages and navigation use, which is exactly the drift that let a stat card link to a page that returned not-found. getAppManagementAccess and getIntegrationManagementAccess now hold the rule, and the nextjs helper is only a session/lookup adapter around them. --- .../nextjs/src/app/[locale]/manage/_access.ts | 69 ++++--------------- .../src/app/[locale]/manage/apps/page.tsx | 3 +- packages/api/src/router/home.ts | 20 +++--- packages/definitions/src/permissions.ts | 40 +++++++++++ .../definitions/src/test/permissions.spec.ts | 49 +++++++++++++ 5 files changed, 115 insertions(+), 66 deletions(-) diff --git a/apps/nextjs/src/app/[locale]/manage/_access.ts b/apps/nextjs/src/app/[locale]/manage/_access.ts index 89d621e6e..90ffcc52b 100644 --- a/apps/nextjs/src/app/[locale]/manage/_access.ts +++ b/apps/nextjs/src/app/[locale]/manage/_access.ts @@ -1,73 +1,30 @@ import type { Session } from "@homarr/auth"; import { api } from "@homarr/api/server"; +import { getAppManagementAccess, getIntegrationManagementAccess } from "@homarr/definitions"; /** - * Access rules for the management sections, shared by the navigation and the pages themselves so - * the two can never drift apart. The rule is always the same: a section is reachable when the user - * can do at least one thing in it, and the section only ever lists what the user is allowed to see. + * Session-aware wrappers around the shared management access rules in `@homarr/definitions`, so the + * navigation and the pages resolve access exactly the same way the API does. */ -export interface AppsSectionAccess { - /** May see and edit every app */ - canManageAll: boolean; - /** May add new apps */ - canCreate: boolean; - /** May delete apps */ - canDelete: boolean; - /** May open /manage/apps at all */ - canAccess: boolean; -} - -export const getAppsSectionAccess = (session: Session | null): AppsSectionAccess => { - const permissions = session?.user.permissions ?? []; - const canManageAll = permissions.includes("app-modify-all"); - const canCreate = permissions.includes("app-create"); - - return { - canManageAll, - canCreate, - canDelete: permissions.includes("app-full-all"), - canAccess: canManageAll || canCreate, - }; -}; - -export interface IntegrationsSectionAccess { - /** May manage every integration */ - canManageAll: boolean; - /** May add new integrations */ - canCreate: boolean; - /** May manage at least one specific integration through a delegated grant */ - hasDelegatedAccess: boolean; - /** May open /manage/integrations at all */ - canAccess: boolean; -} +export const getAppsSectionAccess = (session: Session | null) => + getAppManagementAccess(session?.user.permissions ?? []); /** - * Pure variant for callers that already fetched the integrations the user can see, so the page and - * the navigation never disagree and the list is only queried once per request. + * Pure variant for callers that already fetched the integrations the user can see, so the list is + * only queried once per request. */ export const getIntegrationsSectionAccess = ( session: Session | null, visibleIntegrations: { permissions: { hasFullAccess: boolean } }[], -): IntegrationsSectionAccess => { - const permissions = session?.user.permissions ?? []; - const canManageAll = permissions.includes("integration-full-all"); - const canCreate = permissions.includes("integration-create"); - const hasDelegatedAccess = - !canManageAll && visibleIntegrations.some((integration) => integration.permissions.hasFullAccess); +) => + getIntegrationManagementAccess( + session?.user.permissions ?? [], + visibleIntegrations.some((integration) => integration.permissions.hasFullAccess), + ); - return { - canManageAll, - canCreate, - hasDelegatedAccess, - canAccess: canManageAll || canCreate || hasDelegatedAccess, - }; -}; - -export const getIntegrationsSectionAccessAsync = async ( - session: Session | null, -): Promise => { +export const getIntegrationsSectionAccessAsync = async (session: Session | null) => { const permissions = session?.user.permissions ?? []; // Only pay for the lookup when the answer is not already known from the global permissions. diff --git a/apps/nextjs/src/app/[locale]/manage/apps/page.tsx b/apps/nextjs/src/app/[locale]/manage/apps/page.tsx index 7f6211b00..e1ec727a1 100644 --- a/apps/nextjs/src/app/[locale]/manage/apps/page.tsx +++ b/apps/nextjs/src/app/[locale]/manage/apps/page.tsx @@ -42,8 +42,9 @@ interface AppsPageProps { export default async function AppsPage(props: AppsPageProps) { const session = await auth(); if (!session) redirect("/auth/login"); - const { canManageAll, canCreate, canDelete, canAccess } = getAppsSectionAccess(session); + const { canManageAll, canCreate, canAccess } = getAppsSectionAccess(session); if (!canAccess) notFound(); + const canDelete = session.user.permissions.includes("app-full-all"); const searchParams = searchParamsSchema.parse(await props.searchParams); // Without app-modify-all the user may add apps but not browse the ones they cannot manage, diff --git a/packages/api/src/router/home.ts b/packages/api/src/router/home.ts index c9f29de41..eecd6c116 100644 --- a/packages/api/src/router/home.ts +++ b/packages/api/src/router/home.ts @@ -16,6 +16,7 @@ import { searchEngines, users, } from "@homarr/db/schema"; +import { getAppManagementAccess, getIntegrationManagementAccess } from "@homarr/definitions"; import type { TranslationObject } from "@homarr/translation"; import { createTRPCRouter, publicProcedure } from "../trpc"; @@ -123,29 +124,30 @@ export const homeRouter = createTRPCRouter({ }); } - // These mirror the access rules of /manage/integrations and /manage/apps exactly: a card is only - // shown when the page behind it is reachable, and it counts only what the user is allowed to see. - const hasGlobalFullIntegrationAccess = ctx.session?.user.permissions.includes("integration-full-all") ?? false; - const canCreateIntegrations = ctx.session?.user.permissions.includes("integration-create") ?? false; + // A card is only shown when the page behind it is reachable, and it counts only what the user is + // allowed to see. The rules come from @homarr/definitions so this cannot drift from the pages. + const permissions = ctx.session?.user.permissions ?? []; + const hasGlobalFullIntegrationAccess = permissions.includes("integration-full-all"); const fullAccessIntegrationIds = hasGlobalFullIntegrationAccess || !ctx.session?.user ? [] : await getFullAccessIntegrationIdsAsync(ctx.db, ctx.session.user.id); - if (hasGlobalFullIntegrationAccess || canCreateIntegrations || fullAccessIntegrationIds.length > 0) { + const integrationAccess = getIntegrationManagementAccess(permissions, fullAccessIntegrationIds.length > 0); + if (integrationAccess.canAccess) { statistics.push({ titleKey: "integration", subtitleKey: "resources", - count: hasGlobalFullIntegrationAccess ? await db.$count(integrations) : fullAccessIntegrationIds.length, + count: integrationAccess.canManageAll ? await db.$count(integrations) : fullAccessIntegrationIds.length, path: "/manage/integrations", }); } - const canManageAllApps = ctx.session?.user.permissions.includes("app-modify-all") ?? false; - if (canManageAllApps || (ctx.session?.user.permissions.includes("app-create") ?? false)) { + const appAccess = getAppManagementAccess(permissions); + if (appAccess.canAccess) { statistics.push({ titleKey: "app", subtitleKey: "resources", - count: canManageAllApps ? await db.$count(apps) : 0, + count: appAccess.canManageAll ? await db.$count(apps) : 0, path: "/manage/apps", }); } diff --git a/packages/definitions/src/permissions.ts b/packages/definitions/src/permissions.ts index c61963954..d3da72f2a 100644 --- a/packages/definitions/src/permissions.ts +++ b/packages/definitions/src/permissions.ts @@ -198,3 +198,43 @@ export const matrixStateToPermissions = (state: PermissionMatrixState): GroupPer return acc; }, [] as GroupPermissionKey[]); }; + +export interface ManagementSectionAccess { + /** May see and manage every resource of the section */ + canManageAll: boolean; + /** May add new resources to the section */ + canCreate: boolean; + /** May open the section at all */ + canAccess: boolean; +} + +/** + * A management section is reachable when the user can do at least one thing in it. Both rules live + * here so the navigation, the pages themselves and the management home statistics cannot drift + * apart - each of them consumes the same function. + */ +export const getAppManagementAccess = (permissions: readonly GroupPermissionKey[]): ManagementSectionAccess => { + const canManageAll = permissions.includes("app-modify-all"); + const canCreate = permissions.includes("app-create"); + + return { canManageAll, canCreate, canAccess: canManageAll || canCreate }; +}; + +/** + * `hasDelegatedFullAccess` is whether the user was granted full access to at least one specific + * integration; it has to be looked up per request, so the caller resolves it. + */ +export const getIntegrationManagementAccess = ( + permissions: readonly GroupPermissionKey[], + hasDelegatedFullAccess: boolean, +): ManagementSectionAccess & { hasDelegatedFullAccess: boolean } => { + const canManageAll = permissions.includes("integration-full-all"); + const canCreate = permissions.includes("integration-create"); + + return { + canManageAll, + canCreate, + hasDelegatedFullAccess, + canAccess: canManageAll || canCreate || hasDelegatedFullAccess, + }; +}; diff --git a/packages/definitions/src/test/permissions.spec.ts b/packages/definitions/src/test/permissions.spec.ts index d64093feb..419f5fd0a 100644 --- a/packages/definitions/src/test/permissions.spec.ts +++ b/packages/definitions/src/test/permissions.spec.ts @@ -2,6 +2,8 @@ import { describe, expect, test } from "vitest"; import type { GroupPermissionKey, PermissionMatrixState } from "../permissions"; import { + getAppManagementAccess, + getIntegrationManagementAccess, getPermissionsWithChildren, getPermissionsWithParents, groupPermissionKeys, @@ -187,6 +189,14 @@ describe("matrix helpers should round-trip stably", () => { expect(roundTripped).toHaveLength(permissions.length); }); + test("expect a redundant create key to be normalised away by the level that implies it", () => { + // app-modify-all already grants app-create, so the matrix stores the minimal set. The permission + // is not lost: getPermissionsWithChildren re-derives it whenever permissions are read. + const roundTripped = matrixStateToPermissions(permissionsToMatrixState(["app-create", "app-modify-all"])); + expect(roundTripped).toEqual(["app-use-all", "app-modify-all"]); + expect(getPermissionsWithChildren(roundTripped)).toContain("app-create"); + }); + test("expect a single higher-level key to expand to its full set on round-trip", () => { expect(matrixStateToPermissions(permissionsToMatrixState(["board-full-all"]))).toEqual([ "board-view-all", @@ -195,3 +205,42 @@ describe("matrix helpers should round-trip stably", () => { ]); }); }); + +describe("management section access", () => { + test.each([ + [[], false, false], + [["app-create"], true, false], + [["app-modify-all"], true, true], + [["app-full-all", "app-modify-all"], true, true], + [["app-use-all"], false, false], + [["board-modify-all"], false, false], + ] satisfies [GroupPermissionKey[], boolean, boolean][])( + "expect apps with %s to be reachable: %s, manage-all: %s", + (permissions, canAccess, canManageAll) => { + const access = getAppManagementAccess(permissions); + expect(access.canAccess).toBe(canAccess); + expect(access.canManageAll).toBe(canManageAll); + }, + ); + + test.each([ + [[], false, false, false], + [[], true, true, false], + [["integration-create"], false, true, false], + [["integration-full-all"], false, true, true], + [["integration-use-all"], false, false, false], + [["integration-interact-all"], false, false, false], + [["integration-interact-all"], true, true, false], + ] satisfies [GroupPermissionKey[], boolean, boolean, boolean][])( + "expect integrations with %s and delegated access %s to be reachable: %s, manage-all: %s", + (permissions, hasDelegatedFullAccess, canAccess, canManageAll) => { + const access = getIntegrationManagementAccess(permissions, hasDelegatedFullAccess); + expect(access.canAccess).toBe(canAccess); + expect(access.canManageAll).toBe(canManageAll); + }, + ); + + test("a user who can only use integrations cannot reach the management section", () => { + expect(getIntegrationManagementAccess(["integration-use-all"], false).canAccess).toBe(false); + }); +});