fix(permissions): model app use as a capability, not a level below modify

Codex review: app-modify-all implies only app-create - app-use-all is implied by
app-full-all alone. Putting use beneath modify in the ordered ladder therefore
fabricated a grant: a legacy group holding just app-modify-all displayed as
Modify, and touching the row or applying a preset silently granted use of every
app.

Ladders may now only contain keys that genuinely imply each other. Everything
else is a capability with its own checkbox, so the app row is
No access | Modify | Full plus "Use all apps" and "Create apps". A test asserts
every ladder is a real chain, so this class of mismatch fails CI rather than
shipping.

Also documents in the API docs why app list endpoints can now return FORBIDDEN
and why integration lists may return fewer records, as AGENTS.md requires.
This commit is contained in:
Thomas Camlong committed 2026-08-19 09:42:04 +02:00
1 parent 25bd88f30a
commit e49e53a32f
6 files changed
+243 -163

No files matched your search

+46 -31
View File
@@ -121,50 +121,65 @@ export const groupPermissionKeys = objectKeys(groupPermissions).reduce((acc, key
/**
* Describes how each permission category is presented in the group permission matrix UI.
*
* `levels` is an ordered escalation of access: picking level N grants every key up to N.
* `create` is orthogonal to `levels` - it is the "may add new resources" capability and can be
* granted on its own (e.g. "members may create their own boards but not see anyone else's").
* Some levels already imply the create key through {@link groupPermissionParents}; use
* {@link isCreateImpliedByLevel} to detect that instead of hard-coding it.
* `levels` is an ordered escalation of access: every level implies the one below it through
* {@link groupPermissionParents}, so picking level N grants everything up to N.
*
* Together, `levels` and `create` cover every key in {@link groupPermissionKeys}, so the matrix
* is a lossless representation of a group's permissions.
* `capabilities` are the keys that are NOT part of that ladder - "may add new resources", "may
* upload", "may put any app on a board". They can be granted on their own (e.g. "members may create
* their own boards but not see anyone else's") and some levels already imply them; use
* {@link isCapabilityImpliedByLevel} to detect that instead of hard-coding it.
*
* Only keys that genuinely imply each other may share a ladder. `app-modify-all` implies
* `app-create` but NOT `app-use-all` (only `app-full-all` does), so `app-use-all` is a capability -
* putting it in the ladder would make picking "Modify" silently grant use of every app.
*
* Together, `levels` and `capabilities` cover every key in {@link groupPermissionKeys}, so the
* matrix is a lossless representation of a group's permissions.
*/
export const permissionMatrix = {
board: { levels: ["board-view-all", "board-modify-all", "board-full-all"], create: "board-create" },
app: { levels: ["app-use-all", "app-modify-all", "app-full-all"], create: "app-create" },
board: { levels: ["board-view-all", "board-modify-all", "board-full-all"], capabilities: ["board-create"] },
app: { levels: ["app-modify-all", "app-full-all"], capabilities: ["app-use-all", "app-create"] },
integration: {
levels: ["integration-use-all", "integration-interact-all", "integration-full-all"],
create: "integration-create",
capabilities: ["integration-create"],
},
"search-engine": { levels: ["search-engine-modify-all", "search-engine-full-all"], create: "search-engine-create" },
media: { levels: ["media-view-all", "media-full-all"], create: "media-upload" },
other: { levels: ["other-view-logs"], create: null },
admin: { levels: ["admin"], create: null },
} as const satisfies Record<string, { levels: readonly GroupPermissionKey[]; create: GroupPermissionKey | null }>;
"search-engine": {
levels: ["search-engine-modify-all", "search-engine-full-all"],
capabilities: ["search-engine-create"],
},
media: { levels: ["media-view-all", "media-full-all"], capabilities: ["media-upload"] },
other: { levels: ["other-view-logs"], capabilities: [] },
admin: { levels: ["admin"], capabilities: [] },
} as const satisfies Record<
string,
{ levels: readonly GroupPermissionKey[]; capabilities: readonly GroupPermissionKey[] }
>;
export type PermissionMatrixCategory = keyof typeof permissionMatrix;
export interface PermissionMatrixCategoryState {
/** 0 = no access, N = every key up to `permissionMatrix[category].levels[N - 1]` */
level: number;
/** Whether the category's create/upload key is granted */
create: boolean;
/** The category's granted capability keys, including the ones the level already implies */
capabilities: readonly GroupPermissionKey[];
}
export type PermissionMatrixState = Record<PermissionMatrixCategory, PermissionMatrixCategoryState>;
/**
* Whether the given level already grants the category's create key through permission inheritance.
* Whether the given level already grants a capability through permission inheritance.
* For example board level 3 (`board-full-all`) implies `board-create`.
*/
export const isCreateImpliedByLevel = (category: PermissionMatrixCategory, level: number) => {
const createKey = permissionMatrix[category].create;
if (createKey === null || level <= 0) {
export const isCapabilityImpliedByLevel = (
category: PermissionMatrixCategory,
capability: GroupPermissionKey,
level: number,
) => {
if (level <= 0) {
return false;
}
return getPermissionsWithChildren([...permissionMatrix[category].levels.slice(0, level)]).includes(createKey);
return getPermissionsWithChildren([...permissionMatrix[category].levels.slice(0, level)]).includes(capability);
};
export const permissionsToMatrixState = (permissions: GroupPermissionKey[]): PermissionMatrixState => {
@@ -177,10 +192,12 @@ export const permissionsToMatrixState = (permissions: GroupPermissionKey[]): Per
level = index + 1;
}
});
const createKey = permissionMatrix[category].create;
acc[category] = {
level,
create: createKey !== null && (permissionSet.has(createKey) || isCreateImpliedByLevel(category, level)),
capabilities: permissionMatrix[category].capabilities.filter(
(capability) => permissionSet.has(capability) || isCapabilityImpliedByLevel(category, capability, level),
),
};
return acc;
}, {} as PermissionMatrixState);
@@ -188,13 +205,11 @@ export const permissionsToMatrixState = (permissions: GroupPermissionKey[]): Per
export const matrixStateToPermissions = (state: PermissionMatrixState): GroupPermissionKey[] => {
return objectKeys(permissionMatrix).reduce((acc, category) => {
acc.push(...permissionMatrix[category].levels.slice(0, state[category].level));
// Skip the create key when the chosen level already implies it, so the stored permissions
// stay minimal and a state -> permissions -> state round-trip is exact.
const createKey = permissionMatrix[category].create;
if (createKey !== null && state[category].create && !isCreateImpliedByLevel(category, state[category].level)) {
acc.push(createKey);
}
const { level, capabilities } = state[category];
acc.push(...permissionMatrix[category].levels.slice(0, level));
// Skip capabilities the chosen level already implies, so the stored permissions stay minimal
// and a state -> permissions -> state round-trip is exact.
acc.push(...capabilities.filter((capability) => !isCapabilityImpliedByLevel(category, capability, level)));
return acc;
}, [] as GroupPermissionKey[]);
};
@@ -7,20 +7,20 @@ import {
getPermissionsWithChildren,
getPermissionsWithParents,
groupPermissionKeys,
isCreateImpliedByLevel,
isCapabilityImpliedByLevel,
matrixStateToPermissions,
permissionMatrix,
permissionsToMatrixState,
} from "../permissions";
const emptyState = (overrides: Partial<PermissionMatrixState> = {}): PermissionMatrixState => ({
board: { level: 0, create: false },
app: { level: 0, create: false },
integration: { level: 0, create: false },
"search-engine": { level: 0, create: false },
media: { level: 0, create: false },
other: { level: 0, create: false },
admin: { level: 0, create: false },
board: { level: 0, capabilities: [] },
app: { level: 0, capabilities: [] },
integration: { level: 0, capabilities: [] },
"search-engine": { level: 0, capabilities: [] },
media: { level: 0, capabilities: [] },
other: { level: 0, capabilities: [] },
admin: { level: 0, capabilities: [] },
...overrides,
});
@@ -69,16 +69,23 @@ describe("getPermissionsWithChildren should return the correct permissions", ()
describe("the matrix must be able to represent every permission", () => {
test("every group permission key is covered by exactly one category", () => {
const covered = Object.values(permissionMatrix).flatMap(({ levels, create }) => [
...levels,
...(create === null ? [] : [create]),
]);
const covered = Object.values(permissionMatrix).flatMap(({ levels, capabilities }) => [...levels, ...capabilities]);
expect(covered.toSorted()).toEqual(groupPermissionKeys.toSorted());
});
test.each(groupPermissionKeys.map((key) => [key]))("expect %s to survive a round-trip on its own", (key) => {
expect(matrixStateToPermissions(permissionsToMatrixState([key]))).toContain(key);
});
test("every ladder is a real chain, so no level can fabricate a sibling permission", () => {
Object.values(permissionMatrix).forEach(({ levels }) => {
levels.forEach((key, index) => {
// Selecting a level must never grant more than that level's key already implies.
const implied = getPermissionsWithChildren([key]);
expect(levels.slice(0, index + 1).every((lower) => implied.includes(lower))).toBe(true);
});
});
});
});
describe("permissionsToMatrixState should resolve the highest present level per category", () => {
@@ -92,70 +99,96 @@ describe("permissionsToMatrixState should resolve the highest present level per
test("resolves an empty selection to all zero", () => {
const state = permissionsToMatrixState([]);
expect(Object.values(state).every(({ level, create }) => level === 0 && !create)).toBe(true);
expect(Object.values(state).every(({ level, capabilities }) => level === 0 && capabilities.length === 0)).toBe(
true,
);
});
test("resolves a standalone create key without granting any level", () => {
expect(permissionsToMatrixState(["board-create"]).board).toEqual({ level: 0, create: true });
expect(permissionsToMatrixState(["board-create"]).board).toEqual({ level: 0, capabilities: ["board-create"] });
});
test("marks create as granted when the level implies it", () => {
expect(permissionsToMatrixState(["board-full-all"]).board.create).toBe(true);
expect(permissionsToMatrixState(["app-modify-all"]).app.create).toBe(true);
expect(permissionsToMatrixState(["media-full-all"]).media.create).toBe(true);
test("marks a capability as granted when the level implies it", () => {
expect(permissionsToMatrixState(["board-full-all"]).board.capabilities).toEqual(["board-create"]);
expect(permissionsToMatrixState(["app-modify-all"]).app.capabilities).toEqual(["app-create"]);
expect(permissionsToMatrixState(["media-full-all"]).media.capabilities).toEqual(["media-upload"]);
});
test("does not mark create as granted for a level that does not imply it", () => {
expect(permissionsToMatrixState(["board-modify-all"]).board.create).toBe(false);
expect(permissionsToMatrixState(["integration-interact-all"]).integration.create).toBe(false);
test("does not mark a capability as granted for a level that does not imply it", () => {
expect(permissionsToMatrixState(["board-modify-all"]).board.capabilities).toEqual([]);
expect(permissionsToMatrixState(["integration-interact-all"]).integration.capabilities).toEqual([]);
});
test("app-use-all is a capability, not a level, because app-modify-all does not imply it", () => {
// Regression: modelling use as the level below modify made picking Modify silently grant the
// use of every app, which the permission graph does not do.
const state = permissionsToMatrixState(["app-modify-all"]);
expect(state.app).toEqual({ level: 1, capabilities: ["app-create"] });
expect(matrixStateToPermissions(state)).toEqual(["app-modify-all"]);
});
test("app-full-all implies both app capabilities", () => {
expect(permissionsToMatrixState(["app-full-all"]).app).toEqual({
level: 2,
capabilities: ["app-use-all", "app-create"],
});
});
});
describe("isCreateImpliedByLevel", () => {
describe("isCapabilityImpliedByLevel", () => {
test.each([
["board", 0, false],
["board", 1, false],
["board", 2, false],
["board", 3, true],
["app", 1, false],
["app", 2, true],
["integration", 2, false],
["integration", 3, true],
["search-engine", 1, true],
["media", 1, false],
["media", 2, true],
["other", 1, false],
["admin", 1, false],
] satisfies [keyof typeof permissionMatrix, number, boolean][])(
"expect %s at level %i to imply create: %s",
(category, level, expected) => {
expect(isCreateImpliedByLevel(category, level)).toBe(expected);
["board", "board-create", 0, false],
["board", "board-create", 1, false],
["board", "board-create", 2, false],
["board", "board-create", 3, true],
["app", "app-create", 1, true],
["app", "app-use-all", 1, false],
["app", "app-use-all", 2, true],
["integration", "integration-create", 2, false],
["integration", "integration-create", 3, true],
["search-engine", "search-engine-create", 1, true],
["media", "media-upload", 1, false],
["media", "media-upload", 2, true],
] satisfies [keyof typeof permissionMatrix, GroupPermissionKey, number, boolean][])(
"expect %s at level %i to imply %s: %s",
(category, capability, level, expected) => {
expect(isCapabilityImpliedByLevel(category, capability, level)).toBe(expected);
},
);
});
describe("matrixStateToPermissions should expand levels into permission keys", () => {
test("expands a level into the ordered level keys", () => {
expect(matrixStateToPermissions(emptyState({ board: { level: 2, create: false } }))).toEqual([
expect(matrixStateToPermissions(emptyState({ board: { level: 2, capabilities: [] } }))).toEqual([
"board-view-all",
"board-modify-all",
]);
});
test("expands the admin category into the admin key", () => {
expect(matrixStateToPermissions(emptyState({ admin: { level: 1, create: false } }))).toEqual(["admin"]);
expect(matrixStateToPermissions(emptyState({ admin: { level: 1, capabilities: [] } }))).toEqual(["admin"]);
});
test("emits a standalone create key without any level", () => {
expect(matrixStateToPermissions(emptyState({ board: { level: 0, create: true } }))).toEqual(["board-create"]);
test("emits a standalone capability without any level", () => {
expect(matrixStateToPermissions(emptyState({ board: { level: 0, capabilities: ["board-create"] } }))).toEqual([
"board-create",
]);
});
test("emits media-upload as the create key of the media category", () => {
expect(matrixStateToPermissions(emptyState({ media: { level: 0, create: true } }))).toEqual(["media-upload"]);
test("emits media-upload as the capability of the media category", () => {
expect(matrixStateToPermissions(emptyState({ media: { level: 0, capabilities: ["media-upload"] } }))).toEqual([
"media-upload",
]);
});
test("omits a create key that the chosen level already implies", () => {
expect(matrixStateToPermissions(emptyState({ board: { level: 3, create: true } }))).toEqual([
test("emits app-use-all without granting any app level", () => {
expect(matrixStateToPermissions(emptyState({ app: { level: 0, capabilities: ["app-use-all"] } }))).toEqual([
"app-use-all",
]);
});
test("omits a capability that the chosen level already implies", () => {
expect(matrixStateToPermissions(emptyState({ board: { level: 3, capabilities: ["board-create"] } }))).toEqual([
"board-view-all",
"board-modify-all",
"board-full-all",
@@ -169,31 +202,25 @@ describe("matrix helpers should round-trip stably", () => {
[["integration-use-all", "integration-interact-all", "integration-full-all"]],
[["admin"]],
[[]],
[
[
"app-use-all",
"app-modify-all",
"search-engine-modify-all",
"search-engine-full-all",
"media-view-all",
"other-view-logs",
],
],
[["search-engine-modify-all", "search-engine-full-all", "media-view-all", "other-view-logs"]],
[["board-create"]],
[["media-upload"]],
[["board-create", "app-create", "integration-create", "search-engine-create", "media-upload"]],
[["board-create", "board-view-all"]],
[["app-modify-all"]],
[["app-use-all"]],
[["app-use-all", "app-modify-all"]],
] satisfies [GroupPermissionKey[]][])("expect %s to round-trip", (permissions) => {
const roundTripped = matrixStateToPermissions(permissionsToMatrixState(permissions));
expect(roundTripped).toEqual(expect.arrayContaining(permissions));
expect(roundTripped).toHaveLength(permissions.length);
});
test("expect a redundant create key to be normalised away by the level that implies it", () => {
test("expect a redundant capability 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(roundTripped).toEqual(["app-modify-all"]);
expect(getPermissionsWithChildren(roundTripped)).toContain("app-create");
});
@@ -239,8 +266,4 @@ describe("management section access", () => {
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);
});
});
+2 -2
View File
@@ -674,14 +674,14 @@
"empty": "No permissions selected"
},
"noneDescription": "Members get no access to this area.",
"createIncluded": "Already included in “{level}”",
"level": {
"view": "View",
"use": "Use",
"interact": "Interact",
"modify": "Modify",
"full": "Full"
}
},
"capabilityIncluded": "Already included in “{level}”"
}
},
"memberNotice": {