Enforce the permission-name rule at discovery, for every plugin

This commit is contained in:
2026-08-05 13:00:23 +02:00
parent 90cbc47607
commit fb4382be9d
14 changed files with 153 additions and 54 deletions
@@ -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 <resource>:<action> 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.
+8 -9
View File
@@ -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
// <resource>:<action> 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 }); }
+17 -8
View File
@@ -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 <resource>:<action> — 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 <resource>:<action>, 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 ----
+15 -6
View File
@@ -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";
// `<resource>:<action>` (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";
// `<resource>:<action>` (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;
}
+2 -1
View File
@@ -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 <resource>:<action>, 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",
+7 -7
View File
@@ -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
// `<resource>:read` and a POST on `<resource>: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 `<resource>:read` and a POST on `<resource>: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");