diff --git a/AGENTS.md b/AGENTS.md index 733f206..3dbb0c2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -93,16 +93,39 @@ them. Revisit only if the stated reason stops holding. gates on one operation, so it gates on a permission, and a bundle is just a group with several grants (groups nest). Ory's own "permission" (the `Resource` `permits`: view/edit/delete) is the separate per-row tier. -- **A permission name is always `:`** — `scheduling:read`, `users:write`. Enforced - where names are minted (the admin plugin's create form, `isValidPermissionName`) rather than only - documented, so the convention survives an operator adding one by hand. A bare word names *who - someone is* — a role — and roles are groups here; the old catch-all `admin` permission was exactly - that mistake and was split into `users:`/`groups:`/`permissions:`/`oauth2-clients:` × `read`/`write` - 2026-08-05. Two consequences worth keeping straight: `` is global, not plugin-scoped - (hence `oauth2-clients`, not `clients`), and *addressing* a permission stays looser than *creating* - one (`isPermissionPathSegment`) so a name written before the rule can still be opened and deleted - instead of stranding in Keto. `ADMIN_PERMISSIONS` therefore defaults to empty: every permission is - owned by the plugin that gates on it, and a host-invented default would gate nothing. +- **A permission name is always `:`** — `scheduling:read`, `users:write`. A bare + word names *who someone is* — a role — and roles are groups here; the old catch-all `admin` + permission was exactly that mistake, split into `users:`/`groups:`/`permissions:`/`oauth2-clients:` + × `read`/`write` 2026-08-05. **Enforced at discovery** (`isValidPermissionName` in + `plugin-host/plugin.ts`, checked by `shapeError` over every route/nav `permission` and every + declared name), fail-loud like every other manifest rule — not only in the admin GUI, which an + operator removes by not copying the example in. The admin plugin's create form calls the same host + function; there is one regex. Decisions around it: + - `` is **global, not plugin-scoped** (hence `oauth2-clients`, not `clients`). Deliberate + cross-plugin sharing is a goal, so the pre-2026-08-05 `:` guidance was wrong: users + are the *host's*, not the admin plugin's. Cost: collision-freedom became a convention rather than + structural. Accepted — the alternative penalizes the sharing case. + - **Declaring a permission stays optional.** Requiring every gated route to declare its permission + would make `findConflicts` see all overlaps, but would then warn on exactly the legitimate + sharing case above. Shape is enforced; declaration is not. + - ***Addressing* a permission is looser than *minting* one** (`isPermissionPathSegment`) so a name + written before the rule can still be opened and deleted instead of stranding in Keto. Both mint + points are guarded — the create form, and `rolesAddMember`, whose write would otherwise create a + permission under a hand-typed name. + - `ADMIN_PERMISSIONS` **defaults to empty**: every permission is owned by the plugin that gates on + it, and a host-invented default would gate nothing. This makes the seed a function of what + `bootstrap` discovers, so `bootstrap` bind-mounts `./plugins` like `web` does, and a plugin + dropped in after first boot needs `docker compose up -d` (which re-runs the one-shot), not + `restart web`. Valid while bootstrap is the only writer of grants. + - **`actionForMethod` is plugin-local and must not migrate into `#plugin-api`.** Inside the admin + example it buys one thing: the route table and the in-handler guard derive from one function, so + 29 routes × 2 gate sites cannot drift. As a general mechanism it would make authorization a + function of the transport verb, and a route table must answer "what does this need?" on its own. +- **`users:write` and `groups:write` are equivalent to full administrative access**, and the split + does not change that: `groups:write` adds you to any group, including one holding every permission; + `users:write` mints a recovery code for any account. The containment the split buys is real on the + **read** half only (`users:read` is a safe helpdesk grant). Don't let the per-resource naming imply + otherwise in docs. Raised by the architecture review 2026-08-05. - **Plainpages says "user" everywhere; Ory's word for it is "identity".** Kratos calls the record an identity, but Ory's own docs state it uses that term *interchangeably* with "users" and "accounts" — so this is house style, not a renamed concept, and "user" is the word readers diff --git a/README.md b/README.md index d833f75..73c7900 100644 --- a/README.md +++ b/README.md @@ -285,6 +285,16 @@ bundle it back up with a group if you want one grant to hand out several: Group:it-support ──> Permission:users:read, Permission:users:write, Permission:groups:read, … ``` +The host checks the shape at **discovery**: a plugin gating on — or declaring — a name that isn't +`:` stops the boot, like any other bad manifest. Declaring is still optional, so +two plugins may deliberately share a name. + +> **A `:write` is not a small grant.** Splitting by resource contains the **read** half — `users:read` +> alone is a safe helpdesk grant. It contains the write half much less than the naming suggests: +> `groups:write` lets someone add themselves to a group that holds every permission, and `users:write` +> lets them mint a recovery code for any account and sign in as it. Treat `users:write` and +> `groups:write` as full administrative access. + ### A worked example Alice works support and leads scheduling; Bob works support; Carol administers the system. @@ -716,6 +726,12 @@ arbitrary depth, counts, and icons; see `composeNav` for the node shape. A node' **Lucide icon**, referenced by its sprite id (e.g. `i-cal` → lucide `calendar`); the available ids are `ICON_NAMES` in `src/ui/icons.ts`, and adding one means registering its lucide name there. +**Gating a section header.** Putting the `permission` on the header is the simple form — the whole +subtree disappears with it. When the children need *different* permissions, leave the header ungated +and gate each child: `composeNav` drops a header whose children all filtered out. That second form +only works while the header carries **no `href`** — give it one and it survives the filter as an +ungated leaf, visible to everyone. The admin example uses it (four screens, four permissions). + #### Public pages & menu items A route or nav node may be marked **`public: true`** — reachable by **anyone, signed in or not**, diff --git a/examples/plugins/admin/admin-permissions.test.ts b/examples/plugins/admin/admin-permissions.test.ts index fee4c66..b3fa16d 100644 --- a/examples/plugins/admin/admin-permissions.test.ts +++ b/examples/plugins/admin/admin-permissions.test.ts @@ -12,7 +12,6 @@ import { buildPermissionsListModel, expandToEffectiveUsers, isPermissionPathSegment, - isValidPermissionName, permissionGrantTuple, } from "./admin-permissions.ts"; import type { ExpandTree, RelationTuple } from "#plugin-api"; @@ -23,14 +22,6 @@ const userTuple = (permission: string, n: number): RelationTuple => const groupTuple = (permission: string, group: string): RelationTuple => ({ namespace: "Permission", object: permission, relation: "granted", subject_set: { namespace: "Group", object: group, relation: "members" } }); -test("isValidPermissionName requires : so the convention holds for anything created here", () => { - for (const ok of ["users:read", "scheduling:write", "oauth2-clients:read", "team-a:a1_b9"]) assert.equal(isValidPermissionName(ok), true, ok); - // A bare word is what this rule exists to stop — "admin" says who you are, not what you may do. - for (const bad of ["admin", "", "Users:read", "users:", ":read", "users:read:extra", "a b:read", "-bad:read", `${"a".repeat(60)}:read`]) { - assert.equal(isValidPermissionName(bad), false, bad); - } -}); - test("isPermissionPathSegment stays loose enough to address a permission that predates the rule", () => { // Addressing is not creating: an "admin" tuple left in Keto must still open and delete, or it is // stranded. It only has to be a safe URL/Keto object name. diff --git a/examples/plugins/admin/admin-permissions.ts b/examples/plugins/admin/admin-permissions.ts index 3864084..ea1abcb 100644 --- a/examples/plugins/admin/admin-permissions.ts +++ b/examples/plugins/admin/admin-permissions.ts @@ -8,8 +8,8 @@ // Kratos is read only to label members. Below the builders are thin per-route handlers (keyed on // ctx.params) over a shared `withRoles` gate — admin-only, CSRF-guarded. -import { type ExpandTree, type KetoClient, type KratosAdmin, paginate, parseListQuery, type RelationTuple, type RequestContext, type RouteHandler, type RouteResult, type Translate, type User } from "#plugin-api"; -import { ADMIN_EN, ADMIN_PERMISSIONS_BASE, adminPermission, buildConfirmModel, guardedForm, notFound, requirePermission, unavailable } from "./admin-shared.ts"; +import { type ExpandTree, isValidPermissionName, type KetoClient, type KratosAdmin, paginate, parseListQuery, type RelationTuple, type RequestContext, type RouteHandler, type RouteResult, type Translate, type User } from "#plugin-api"; +import { ADMIN_EN, ADMIN_PERMISSIONS_BASE, buildConfirmModel, guardedForm, notFound, permissionName, requirePermission, unavailable } from "./admin-shared.ts"; import { type GroupView, groupsFromTuples, @@ -26,7 +26,7 @@ const PERMISSION_NS = "Permission"; const GRANTED = "granted"; // The one irreversible move on this screen: delete this permission, or revoke your own grant of it, // and nobody can grant anything ever again. Guarded like the `admin` permission it replaces. -const LOCKOUT_PERMISSION = adminPermission("permissions", "POST"); +const LOCKOUT_PERMISSION = permissionName("permissions", "write"); const DEFAULT_PAGE_SIZE = 25; const PAGE_SIZES = [25, 50, 100]; // Expand far past any sane group-nesting depth so the effective-access view never silently @@ -38,16 +38,11 @@ const EXPAND_MAX_DEPTH = 50; export type PermissionView = GroupView; export const permissionsFromTuples = groupsFromTuples; -const PERMISSION_NAME = /^[a-z0-9][a-z0-9_-]*:[a-z0-9][a-z0-9_-]*$/; const PERMISSION_SEGMENT = /^[a-z0-9][a-z0-9_:-]*$/; -// Creating one enforces the convention, so it holds going forward. -export function isValidPermissionName(name: string): boolean { - return name.length <= 64 && PERMISSION_NAME.test(name); -} - // Addressing one only has to recognise a name Keto can already hold: a permission written before // this rule — or by another tool — stays viewable and deletable instead of 404ing out of reach. +// Minting one goes through the host's `isValidPermissionName`, the same rule discovery enforces. export function isPermissionPathSegment(name: string): boolean { return name.length <= 64 && PERMISSION_SEGMENT.test(name); } @@ -347,6 +342,10 @@ export const rolesDetail = withRoleName((deps, name) => permissionDetailResult(d export const rolesAddMember = withRoleName(async (deps, name) => { const { ctx, keto, revoke, user } = deps; const form = (await guardedForm(ctx))!; + // A permission exists only while a tuple carries it, so this write would *create* one under a + // hand-typed name — the second mint point, and the one that would slip past the create form's + // : rule. Assigning to something that doesn't exist is a 404, not a create. + if (!(await roleExists(keto, name))) return notFound(ctx); const member = (form.get("member") ?? "").trim(); const tuple = permissionGrantTuple(name, member); // the picker only offers real users/groups if (tuple) { await keto.writeTuple(tuple); revokeUserMember(revoke, member); ctx.log.info("admin: permission assigned", { actor: user.id, member, permission: name }); } diff --git a/examples/plugins/admin/admin-shared.test.ts b/examples/plugins/admin/admin-shared.test.ts index 042b84c..d91f922 100644 --- a/examples/plugins/admin/admin-shared.test.ts +++ b/examples/plugins/admin/admin-shared.test.ts @@ -6,8 +6,8 @@ import assert from "node:assert/strict"; import type { IncomingMessage, ServerResponse } from "node:http"; import { Readable } from "node:stream"; import { test } from "node:test"; -import { GuardError, type Log, type PageChrome, type RequestContext, type User } from "#plugin-api"; -import { ADMIN_EN, ADMIN_NAV, ADMIN_USERS_BASE, adminPermission, buildConfirmModel, guardedForm, requirePermission } from "./admin-shared.ts"; +import { GuardError, isValidPermissionName, type Log, type PageChrome, type RequestContext, type User } from "#plugin-api"; +import { ADMIN_EN, ADMIN_NAV, ADMIN_USERS_BASE, actionForMethod, buildConfirmModel, guardedForm, permissionName, requirePermission } from "./admin-shared.ts"; const reader: User = { email: "ada@x.io", id: "u1", permissions: ["users:read"] }; const writer: User = { email: "cy@x.io", id: "u3", permissions: ["users:read", "users:write"] }; @@ -31,7 +31,10 @@ test("ADMIN_NAV: an ungated Admin header whose four screens each gate on their o assert.equal(ADMIN_NAV.id, "admin"); // No gate on the header: a user may hold one screen's permission and not another's. composeNav // drops a header left with no visible children, so holding none of the four hides the section. + // Both halves matter — give the header an `href` and it survives the filter as a visible leaf, + // ungated, for anonymous visitors included. assert.equal(ADMIN_NAV.permission, undefined); + assert.equal(ADMIN_NAV.href, undefined); assert.equal(ADMIN_NAV.open, undefined); // the host current-marks + opens; the fragment stays static assert.deepEqual(ADMIN_NAV.children?.map((c) => c.href), ["/admin/users", "/admin/groups", "/admin/permissions", "/admin/clients"]); assert.deepEqual(ADMIN_NAV.children?.map((c) => c.permission), ["users:read", "groups:read", "permissions:read", "oauth2-clients:read"]); @@ -44,12 +47,18 @@ test("ADMIN_NAV: an ungated Admin header whose four screens each gate on their o // ---- permission naming ---- -test("adminPermission builds : — read for GET/HEAD, write for every mutation", () => { - assert.equal(adminPermission("users", "GET"), "users:read"); - assert.equal(adminPermission("users", "HEAD"), "users:read"); // a GET route also answers HEAD - assert.equal(adminPermission("users", "POST"), "users:write"); - assert.equal(adminPermission("groups", "DELETE"), "groups:write"); // anything that isn't a read is a write - assert.equal(adminPermission("oauth2-clients", "get"), "oauth2-clients:read"); // method case is the caller's +test("permissionName builds :, and the host agrees the result is well-formed", () => { + assert.equal(permissionName("users", "read"), "users:read"); + assert.equal(permissionName("oauth2-clients", "write"), "oauth2-clients:write"); + assert.ok(isValidPermissionName(permissionName("oauth2-clients", "write"))); // the rule discovery enforces +}); + +test("actionForMethod: read for GET/HEAD, write for every mutation", () => { + assert.equal(actionForMethod("GET"), "read"); + assert.equal(actionForMethod("HEAD"), "read"); // a GET route also answers HEAD + assert.equal(actionForMethod("POST"), "write"); + assert.equal(actionForMethod("DELETE"), "write"); // anything that isn't a read is a write + assert.equal(actionForMethod("get"), "read"); // method case is the caller's }); // ---- auth gates ---- diff --git a/examples/plugins/admin/admin-shared.ts b/examples/plugins/admin/admin-shared.ts index ab9a5bf..018da7f 100644 --- a/examples/plugins/admin/admin-shared.ts +++ b/examples/plugins/admin/admin-shared.ts @@ -20,12 +20,21 @@ export const ADMIN_CLIENTS_BASE = "/admin/clients"; // `oauth2-clients` rather than `clients` because permission names are one global namespace. export type AdminResource = "groups" | "oauth2-clients" | "permissions" | "users"; -// `:` (README → Users, groups & permissions). Every screen reads on GET/HEAD and -// mutates on POST, so the manifest's route table and the in-handler guard both derive the name here -// rather than each spelling it out — they cannot drift into gating on different permissions. -export function adminPermission(resource: AdminResource, method: string): string { +export type AdminAction = "read" | "write"; + +// `:` (README → Naming a permission). +export function permissionName(resource: AdminResource, action: AdminAction): string { + return `${resource}:${action}`; +} + +// This plugin's mapping from method to action: every screen reads on GET/HEAD and mutates on POST. +// The manifest's route table and the in-handler guard both go through it rather than each spelling +// the permission out, so they cannot drift into gating on different names. Deliberately local — as +// a general mechanism it would make authorization a function of the transport verb, and a route +// table should answer "what does this need?" on its own (AGENTS.md). +export function actionForMethod(method: string): AdminAction { const verb = method.toUpperCase(); - return `${resource}:${verb === "GET" || verb === "HEAD" ? "read" : "write"}`; + return verb === "GET" || verb === "HEAD" ? "read" : "write"; } // The plugin's nav fragment: an ungated "Admin" header + its four screens, each gated on its own @@ -50,7 +59,7 @@ export const ADMIN_NAV: NavNode = { // handler to thread on. GuardError → /login or 403. export function requirePermission(ctx: RequestContext, resource: AdminResource): User { const user = requireSession(ctx); // anonymous → GuardError → /login (return_to kept) - const permission = adminPermission(resource, ctx.req.method ?? "GET"); + const permission = permissionName(resource, actionForMethod(ctx.req.method ?? "GET")); if (!can(ctx, permission)) throw new GuardError(403, `${permission} required`); return user; } diff --git a/examples/plugins/admin/plugin.test.ts b/examples/plugins/admin/plugin.test.ts index 7ebfd2d..276e177 100644 --- a/examples/plugins/admin/plugin.test.ts +++ b/examples/plugins/admin/plugin.test.ts @@ -3,6 +3,7 @@ // with nothing in the logs to explain it. Pin the two halves against each other here. import assert from "node:assert/strict"; import { test } from "node:test"; +import { isValidPermissionName } from "#plugin-api"; import manifest from "./plugin.ts"; const routes = manifest.routes ?? []; @@ -23,7 +24,7 @@ test("the manifest declares no permission it never gates on", () => { }); test("every declared permission is :, and reads and writes are split per resource", () => { - for (const name of declared) assert.match(name, /^[a-z0-9][a-z0-9_-]*:(read|write)$/, name); + for (const name of declared) assert.ok(isValidPermissionName(name), name); // the host's rule, not a copy of it assert.deepEqual([...declared].sort(), [ "groups:read", "groups:write", "oauth2-clients:read", "oauth2-clients:write", diff --git a/examples/plugins/admin/plugin.ts b/examples/plugins/admin/plugin.ts index 40863bf..6cdc204 100644 --- a/examples/plugins/admin/plugin.ts +++ b/examples/plugins/admin/plugin.ts @@ -11,15 +11,15 @@ import { clientsCreate, clientsDeleteConfirm, clientsDelete, clientsDetail, clie import { groupsAddMember, groupsCreate, groupsDelete, groupsDeleteConfirm, groupsDetail, groupsList, groupsNewForm, groupsRemoveMember } from "./admin-groups.ts"; import { rolesAddMember, rolesCreate, rolesDelete, rolesDeleteConfirm, rolesDetail, rolesList, rolesNewForm, rolesRemoveMember } from "./admin-permissions.ts"; import { usersCreate, usersDeleteConfirm, usersDelete, usersEditForm, usersList, usersNewForm, usersRecovery, usersState, usersUpdate } from "./admin-users.ts"; -import { ADMIN_NAV, adminPermission, type AdminResource } from "./admin-shared.ts"; +import { ADMIN_NAV, actionForMethod, type AdminResource, permissionName } from "./admin-shared.ts"; -// One route factory per screen: `permission` is derived by `adminPermission`, so a GET gates on -// `:read` and a POST on `:write` and the table below cannot drift from the guard -// each handler runs. The host redirects an anonymous visitor to /login, gives a signed-in user -// missing the permission the 403 page, and filters the nav the same way. Handlers are thin and keyed -// on ctx.params (the host extracts :id / :name), the idiomatic per-route style. +// One route factory per screen: a GET gates on `:read` and a POST on `:write`, +// derived through the same two helpers the in-handler guard uses, so the table below cannot drift +// from it. The host redirects an anonymous visitor to /login, gives a signed-in user missing the +// permission the 403 page, and filters the nav the same way. Handlers are thin and keyed on +// ctx.params (the host extracts :id / :name), the idiomatic per-route style. const on = (resource: AdminResource) => (method: HttpMethod, path: string, handler: RouteHandler): Route => - ({ handler, method, path, permission: adminPermission(resource, method) }); + ({ handler, method, path, permission: permissionName(resource, actionForMethod(method)) }); const users = on("users"); const groups = on("groups"); diff --git a/src/plugin-host/discovery.test.ts b/src/plugin-host/discovery.test.ts index 7c94431..7283721 100644 --- a/src/plugin-host/discovery.test.ts +++ b/src/plugin-host/discovery.test.ts @@ -50,8 +50,13 @@ const badCases: Array<{ name: string; files: Record; match: RegE { name: "non-function dashboard", files: { "weirddash/plugin.ts": `export default { apiVersion: "1.0.0", dashboard: "nope" };` }, match: /weirddash.*dashboard.*function/s }, { name: "reserved dashboard id shadows the gated dashboard", files: { "dashboard/plugin.ts": full("dashboard") }, match: /dashboard.*reserved/s }, { name: "duplicate nav id across plugins", files: { "a/plugin.ts": full("a").replace("a:root", "dup"), "b/plugin.ts": full("b").replace("b:root", "dup") }, match: /nav id "dup"/ }, - { name: "a route marked public AND permission is contradictory", files: { "contra/plugin.ts": `export default { apiVersion: "1.0.0", routes: [{ method: "GET", path: "/", public: true, permission: "x", handler: () => ({ html: "x" }) }] };` }, match: /contra.*public.*permission/s }, - { name: "a nav node marked public AND permission is contradictory", files: { "contranav/plugin.ts": `export default { apiVersion: "1.0.0", nav: [{ id: "n", label: "N", public: true, permission: "x" }] };` }, match: /contranav.*public.*permission/s }, + { name: "a route marked public AND permission is contradictory", files: { "contra/plugin.ts": `export default { apiVersion: "1.0.0", routes: [{ method: "GET", path: "/", public: true, permission: "x:read", handler: () => ({ html: "x" }) }] };` }, match: /contra.*public.*permission/s }, + { name: "a nav node marked public AND permission is contradictory", files: { "contranav/plugin.ts": `export default { apiVersion: "1.0.0", nav: [{ id: "n", label: "N", public: true, permission: "x:read" }] };` }, match: /contranav.*public.*permission/s }, + // A permission name is : wherever the manifest mentions one. Enforced here, not + // only in the admin GUI, so it holds for a plugin installed without that GUI. + { name: "a route gating on a bare word", files: { "bare/plugin.ts": `export default { apiVersion: "1.0.0", routes: [{ method: "GET", path: "/", permission: "admin", handler: () => ({ html: "x" }) }] };` }, match: /bare.*admin.*:/s }, + { name: "a nav node gating on a bare word", files: { "barenav/plugin.ts": `export default { apiVersion: "1.0.0", nav: [{ id: "n", label: "N", permission: "admin" }] };` }, match: /barenav.*admin.*:/s }, + { name: "a declared permission that is a bare word", files: { "baredecl/plugin.ts": `export default { apiVersion: "1.0.0", permissions: [{ name: "admin" }] };` }, match: /baredecl.*admin.*:/s }, { name: "two plugins claim the public home", files: { "a/plugin.ts": `export default { apiVersion: "1.0.0", home: () => ({ html: "a" }) };`, "b/plugin.ts": `export default { apiVersion: "1.0.0", home: () => ({ html: "b" }) };` }, match: /home/ }, { name: "two plugins claim the gated dashboard", files: { "a/plugin.ts": `export default { apiVersion: "1.0.0", dashboard: () => ({ html: "a" }) };`, "b/plugin.ts": `export default { apiVersion: "1.0.0", dashboard: () => ({ html: "b" }) };` }, match: /dashboard/ }, ]; diff --git a/src/plugin-host/discovery.ts b/src/plugin-host/discovery.ts index 88e9af2..ececc56 100644 --- a/src/plugin-host/discovery.ts +++ b/src/plugin-host/discovery.ts @@ -7,7 +7,7 @@ import { existsSync, readdirSync } from "node:fs"; import { dirname, join } from "node:path"; import { fileURLToPath, pathToFileURL } from "node:url"; -import { checkApiVersion, findConflicts, isValidPluginId, RESERVED_PLUGIN_IDS, type Plugin, type PluginManifest } from "./plugin.ts"; +import { checkApiVersion, findConflicts, isValidPermissionName, isValidPluginId, RESERVED_PLUGIN_IDS, type Plugin, type PluginManifest } from "./plugin.ts"; const rootDir = join(dirname(fileURLToPath(import.meta.url)), "..", ".."); @@ -100,6 +100,20 @@ function shapeError(manifest: PluginManifest): string | null { } const navContradiction = findPublicNavContradiction(manifest.nav); if (navContradiction) return navContradiction; + // Every permission name the manifest mentions — gated on or declared — must be `:`. + // A bare word names a role, and roles are groups here (README → Naming a permission). + for (const route of Array.isArray(manifest.routes) ? manifest.routes : []) { + if (route?.permission != null && !isValidPermissionName(route.permission)) { + return `route "${route.method} ${route.path}" gates on "${route.permission}"; a permission name is :, e.g. "things:read"`; + } + } + for (const decl of Array.isArray(manifest.permissions) ? manifest.permissions : []) { + if (decl?.name == null || !isValidPermissionName(decl.name)) { + return `declared permission "${decl?.name}" is not :, e.g. "things:read"`; + } + } + const navPermission = findInvalidNavPermission(manifest.nav); + if (navPermission) return navPermission; return null; } @@ -113,6 +127,17 @@ function findPublicNavContradiction(nodes: PluginManifest["nav"]): string | null return null; } +function findInvalidNavPermission(nodes: PluginManifest["nav"]): string | null { + for (const node of Array.isArray(nodes) ? nodes : []) { + if (node?.permission != null && !isValidPermissionName(node.permission)) { + return `nav node "${node.label ?? node.id ?? "?"}" gates on "${node.permission}"; a permission name is :, e.g. "things:read"`; + } + const inChild = findInvalidNavPermission(node?.children); + if (inChild) return inChild; + } + return null; +} + function messageOf(err: unknown): string { return err instanceof Error ? err.message : String(err); } diff --git a/src/plugin-host/plugin-api.ts b/src/plugin-host/plugin-api.ts index e8024ca..b2a7bd7 100644 --- a/src/plugin-host/plugin-api.ts +++ b/src/plugin-host/plugin-api.ts @@ -4,7 +4,7 @@ // contract boundary in code — the host may refactor any other src/* freely as long as it holds, so // a plugin should import from here, never reach into deeper modules. See README.md → Building plugins. -export { definePlugin } from "./plugin.ts"; +export { definePlugin, isValidPermissionName } from "./plugin.ts"; export type { HttpMethod, Plugin, PluginHooks, PluginManifest, PermissionDecl, Route, RouteHandler, RouteResult } from "./plugin.ts"; export type { RequestContext, User } from "../http/context.ts"; export type { PageChrome } from "../ui/chrome.ts"; diff --git a/src/plugin-host/plugin.test.ts b/src/plugin-host/plugin.test.ts index 5f9041b..0a060c1 100644 --- a/src/plugin-host/plugin.test.ts +++ b/src/plugin-host/plugin.test.ts @@ -5,6 +5,7 @@ import { definePlugin, findConflicts, HOST_API_VERSION, + isValidPermissionName, isValidPluginId, parseSemver, RESERVED_PLUGIN_IDS, @@ -47,6 +48,16 @@ test("isValidPluginId accepts lowercase/digits/dashes anywhere and rejects every } }); +test("isValidPermissionName requires : — a bare word names a role, and roles are groups", () => { + for (const ok of ["users:read", "scheduling:write", "oauth2-clients:read", "team-a:a1_b9", "invoices:approve"]) { + assert.ok(isValidPermissionName(ok), ok); + } + // "admin" is the shape this rule exists to stop: it says who someone is, not what they may do. + for (const bad of ["admin", "", "Users:read", "users:", ":read", "users:read:extra", "a b:read", "-bad:read", "a/b:read", `${"a".repeat(60)}:read`]) { + assert.ok(!isValidPermissionName(bad), bad); + } +}); + test("parseSemver follows the semver core, rejecting ranges, prefixes, leading zeros and missing parts", () => { assert.deepEqual(parseSemver("1.2.3"), { major: 1, minor: 2, patch: 3 }); assert.deepEqual(parseSemver("1.2.3-rc.1+build.5"), { major: 1, minor: 2, patch: 3 }); // prerelease/build tolerated, ignored diff --git a/src/plugin-host/plugin.ts b/src/plugin-host/plugin.ts index 76df224..a719755 100644 --- a/src/plugin-host/plugin.ts +++ b/src/plugin-host/plugin.ts @@ -45,6 +45,15 @@ export interface PermissionDecl { name: string; } +// `:`, each half lowercase alphanumeric with dashes/underscores inside. The 64-char +// cap keeps a name usable as a Keto object and a URL path segment. Enforced at discovery like every +// other manifest rule, so the convention holds for plugins the admin GUI never touches. +const PERMISSION_NAME = /^[a-z0-9][a-z0-9_-]*:[a-z0-9][a-z0-9_-]*$/; + +export function isValidPermissionName(name: string): boolean { + return name.length <= 64 && PERMISSION_NAME.test(name); +} + // Optional hooks on system actions. Crash-isolation is a non-goal — a throwing hook fails loud. export interface PluginHooks { onBoot?: () => Promise | void; // after discovery, before the server listens diff --git a/todo.md b/todo.md index 84b4375..c784daa 100644 --- a/todo.md +++ b/todo.md @@ -2,6 +2,7 @@ ## Unfinnished work +- [ ] (Three decisions inherited from the 2026-08-05 architecture review, to settle inside the next item rather than re-derive: (1) `permissions:write` should mean "may grant a permission to anyone", not "may use the Permissions screen" — so the lockout guard relocates to Users/Groups instead of dying with the screen. (2) The `:` rule is now enforced at discovery, so deleting the screen no longer removes its enforcement — but the *create form* goes, so minting moves to whatever the new picker writes. (3) Orphans get worse, not better: `isPermissionPathSegment` exists so a name predating the rule stays deletable; once permissions are a fixed list from code, any Keto name not in that list becomes invisible — either give the Users/Groups screens an "unknown permissions held" affordance or decide explicitly to leave them to `curl`.) - [ ] Permissions should be a list in code. Since no permissions exists in the database out of the box, but there are a fixed number of permissions in the plugins that the end consumer and user of plain pages can use, these permissions must surface to the UI somehow. The effects is that the permissions page should be deleted completely, and the users and groups pages should gain the functionality to add permissions to their things instead, provided the user have the right permissiosn to do so, of course. Run the product reviewer agent on this todo also. - [ ] The seeded admin@plainpages.local are assigned twice to the permission "admin", should only be one, right? (the "admin" permission name can be switched after previous todos have been done) - [ ] In Playwright tests, try different resolutions and sizes, from BIG desktop down to tiny phone.