From 151117680a9583e4ddaf6281669b50169b870557 Mon Sep 17 00:00:00 2001 From: lilleman Date: Wed, 5 Aug 2026 15:08:31 +0200 Subject: [PATCH] Extend the read-only treatment to OAuth2 clients and write-intent GETs --- AGENTS.md | 18 ++++++++--- examples/plugins/admin/admin-clients.ts | 26 +++++++++------ examples/plugins/admin/admin-grants.ts | 11 +++++-- examples/plugins/admin/admin-groups.ts | 15 +++++---- examples/plugins/admin/admin-shared.ts | 8 +++-- examples/plugins/admin/admin-users.ts | 32 ++++++++++++------- examples/plugins/admin/plugin.test.ts | 6 +++- examples/plugins/admin/plugin.ts | 21 ++++++------ .../plugins/admin/views/client-detail.ejs | 2 +- examples/plugins/admin/views/clients.ejs | 3 +- .../views/partials/client-detail-body.ejs | 2 ++ .../views/partials/permission-picker.ejs | 3 ++ src/http/app.test.ts | 21 ++++++++++-- src/http/context.ts | 4 +-- todo.md | 2 ++ 15 files changed, 120 insertions(+), 54 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index e89a152..0dbb090 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -138,12 +138,20 @@ them. Revisit only if the stated reason stops holding. operator (a helpdesk account with `users:read`), and the host's 403 is the backstop, not the UX: the list/detail models carry `canWrite` and the views drop create/save/delete/add/remove, while the permission picker still renders — disabled — because *seeing* who holds what is the point of `:read`. + A *write-intent GET* — a create form or a delete-confirm page — is the exception to + `actionForMethod`: it gates on `:write` (declared in the route table and passed to the handler's + guard, so the two still agree), because a page whose only purpose is to start a write should refuse + a reader rather than render a form whose submit 403s. Two grant-specific guards go with it, both restoring behaviour the deleted Permissions screen had: - you cannot revoke your own grants (self-lockout would need a `curl` against Keto to undo, which the - operator persona can't do — same shape as the self-deactivate/self-delete guards), and a permission - held *through a group* renders ticked-but-disabled rather than unticked, because showing it unticked - stated the opposite of the truth and unticking it wrote nothing while looking like a successful - revoke. Raised by the architecture + product reviews 2026-08-05. + you cannot revoke your own **direct** grants on the Users screen (self-lockout would need a `curl` + against Keto to undo, which the operator persona can't do — same shape as the self-deactivate/ + self-delete guards), and a permission held *through a group* renders ticked-but-disabled rather than + unticked, because showing it unticked stated the opposite of the truth and unticking it wrote + nothing while looking like a successful revoke. **Known gap, same scope the deleted screen had:** + the group paths are unguarded — unticking a permission on a group you belong to, removing yourself + from it, or deleting it can all still strip your own effective access. The robust "last effective + holder" check needs a reverse Keto query and is deferred. Raised by the architecture + product + + stability reviews 2026-08-05. - **`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 diff --git a/examples/plugins/admin/admin-clients.ts b/examples/plugins/admin/admin-clients.ts index 4eacf32..0ba324d 100644 --- a/examples/plugins/admin/admin-clients.ts +++ b/examples/plugins/admin/admin-clients.ts @@ -5,8 +5,8 @@ // PRG redirect (mirrors the Users "trigger recovery" one-time code). Below the builders are thin // per-route handlers (keyed on ctx.params) over a shared `withClients` gate — admin-only, CSRF-guarded. -import { type HydraAdmin, HydraError, type OAuth2Client, paginate, parseListQuery, type RequestContext, type RouteHandler, type RouteResult, type Translate, type User } from "#plugin-api"; -import { ADMIN_CLIENTS_BASE, ADMIN_EN, buildConfirmModel, guardedForm, notFound, requirePermission, unavailable } from "./admin-shared.ts"; +import { can, type HydraAdmin, HydraError, type OAuth2Client, paginate, parseListQuery, type RequestContext, type RouteHandler, type RouteResult, type Translate, type User } from "#plugin-api"; +import { ADMIN_CLIENTS_BASE, ADMIN_EN, type AdminAction, buildConfirmModel, guardedForm, notFound, permissionName, requirePermission, unavailable } from "./admin-shared.ts"; import type { FieldConfig } from "./admin-users.ts"; const DEFAULT_PAGE_SIZE = 25; @@ -100,6 +100,7 @@ function listHref(state: ListState, overrides: Partial = {}): string } export function buildClientsListModel(opts: { + canWrite?: boolean; clients: OAuth2Client[]; csrfToken?: string; t?: Translate; @@ -119,6 +120,7 @@ export function buildClientsListModel(opts: { return { breadcrumbs: [{ href: ADMIN_CLIENTS_BASE, label: t("admin.nav.section") }, { label: t("admin.clients.title") }], + canWrite: opts.canWrite !== false, filterBar: listFilterBar(state, t), pagination: listPagination(state, page, t), table: listTable(rows, t), @@ -208,6 +210,7 @@ export function buildClientFormModel(opts: { } export function buildClientDetailModel(opts: { + canWrite?: boolean; client: ClientView; created?: boolean; // just registered → success banner + the one-time secret (if any) csrfToken?: string; @@ -218,6 +221,7 @@ export function buildClientDetailModel(opts: { const base = detailHref(opts.client.id); return { breadcrumbs: [{ href: ADMIN_CLIENTS_BASE, label: t("admin.clients.title") }, { label: opts.client.name }], + canWrite: opts.canWrite !== false, client: opts.client, created: opts.created ?? false, csrfToken: opts.csrfToken ?? "", @@ -243,9 +247,9 @@ function readClientInput(form: URLSearchParams): ClientInput { // Hydra capability (else a themed 503). Each route below is a thin handler over these. interface ClientsDeps { ctx: RequestContext; hydra: HydraAdmin; user: User; } -function withClients(inner: (deps: ClientsDeps) => Promise): RouteHandler { +function withClients(inner: (deps: ClientsDeps) => Promise, action?: AdminAction): RouteHandler { return async (ctx) => { - const user = requirePermission(ctx, "oauth2-clients"); + const user = requirePermission(ctx, "oauth2-clients", action); const hydra = ctx.system?.hydra; if (!hydra) return unavailable(ctx, ctx.t("admin.capability.hydra")); return inner({ ctx, hydra, user }); @@ -253,24 +257,26 @@ function withClients(inner: (deps: ClientsDeps) => Promise): RouteH } // Same, plus the target client from ctx.params.id (unknown → themed 404). -function withClient(inner: (deps: ClientsDeps, client: OAuth2Client, id: string) => Promise): RouteHandler { +function withClient(inner: (deps: ClientsDeps, client: OAuth2Client, id: string) => Promise, action?: AdminAction): RouteHandler { return withClients(async (deps) => { const id = deps.ctx.params["id"] ?? ""; const client = await deps.hydra.getClient(id); if (!client) return notFound(deps.ctx); return inner(deps, client, id); - }); + }, action); } const clientFormResult = (ctx: RequestContext, extra: { error?: string; values?: Partial }): RouteResult => ({ data: { chrome: ctx.chrome, model: buildClientFormModel({ csrfToken: ctx.chrome.csrfToken, t: ctx.t, ...extra }) }, view: "client-form" }); +const canWriteClients = (ctx: RequestContext): boolean => can(ctx, permissionName("oauth2-clients", "write")); + const clientDetailResult = (ctx: RequestContext, client: OAuth2Client, extra: { created?: boolean; secret?: string } = {}): RouteResult => - ({ data: { chrome: ctx.chrome, model: buildClientDetailModel({ client: toClientView(client), csrfToken: ctx.chrome.csrfToken, t: ctx.t, ...extra }) }, view: "client-detail" }); + ({ data: { chrome: ctx.chrome, model: buildClientDetailModel({ canWrite: canWriteClients(ctx), client: toClientView(client), csrfToken: ctx.chrome.csrfToken, t: ctx.t, ...extra }) }, view: "client-detail" }); // GET /admin/clients — the list. export const clientsList = withClients(async ({ ctx, hydra }) => { const { clients } = await hydra.listClients({ pageSize: LIST_FETCH_SIZE }); - return { data: { chrome: ctx.chrome, model: buildClientsListModel({ clients, csrfToken: ctx.chrome.csrfToken, t: ctx.t, url: ctx.url }) }, view: "clients" }; + return { data: { chrome: ctx.chrome, model: buildClientsListModel({ canWrite: canWriteClients(ctx), clients, csrfToken: ctx.chrome.csrfToken, t: ctx.t, url: ctx.url }) }, view: "clients" }; }); // POST /admin/clients — register; on success show the one-time secret directly (no PRG, Hydra never @@ -291,7 +297,7 @@ export const clientsCreate = withClients(async ({ ctx, hydra, user }) => { }); // GET /admin/clients/new — the register form. -export const clientsNewForm = withClients(({ ctx }) => Promise.resolve(clientFormResult(ctx, {}))); +export const clientsNewForm = withClients(({ ctx }) => Promise.resolve(clientFormResult(ctx, {})), "write"); // GET /admin/clients/:id — the detail (read-only; the secret is shown only once, at creation). export const clientsDetail = withClient((deps, client) => Promise.resolve(clientDetailResult(deps.ctx, client))); @@ -306,7 +312,7 @@ export const clientsDeleteConfirm = withClient((deps, client, id) => { cancelHref: base, confirmAction: `${base}/delete`, confirmLabel: tt("admin.clients.delete"), message: tt("admin.clients.deleteMessage", { name }), title: tt("admin.clients.delete"), }) }, view: "confirm" }); -}); +}, "write"); // POST /admin/clients/:id/delete — perform it. export const clientsDelete = withClient(async ({ ctx, hydra, user }, _client, id) => { diff --git a/examples/plugins/admin/admin-grants.ts b/examples/plugins/admin/admin-grants.ts index a3d04ff..aab8863 100644 --- a/examples/plugins/admin/admin-grants.ts +++ b/examples/plugins/admin/admin-grants.ts @@ -37,7 +37,7 @@ export async function heldPermissions(keto: KetoClient, subject: GrantSubject): // Every declared permission the subject effectively holds — direct grants *plus* anything reached // through a group, which is what actually lands in their JWT. One Keto check per declared name; // the catalog is small and this is an admin screen (login does the same walk). -export async function effectivePermissions(keto: KetoClient, subject: GrantSubject, declared: PermissionDecl[]): Promise { +export async function effectivePermissions(keto: KetoClient, subject: GrantSubject, declared: readonly PermissionDecl[]): Promise { const held = await Promise.all(declared.map((decl) => keto.check({ namespace: PERMISSION_NS, object: decl.name, relation: GRANTED, ...subject }))); return declared.filter((_, i) => held[i]).map((decl) => decl.name); } @@ -60,6 +60,9 @@ export interface PermissionPicker { hint: string; inheritedNote: string | undefined; // set when at least one choice is group-held, to explain the disabled row legend: string; + // Set for a group: its members hold these transitively, so a change reaches them at their next + // re-mint rather than at once. The user picker revokes live tokens, so it says nothing. + pending: string | undefined; readOnly: boolean; // the viewer holds :read but not :write — show the state, offer no save submit: string; } @@ -69,11 +72,12 @@ export interface PermissionPicker { // (grantDiff). An inherited row is disabled, so it never posts and can never be diffed into a revoke. export function buildPermissionPicker(opts: { action: string; - declared: PermissionDecl[]; + declared: readonly PermissionDecl[]; direct: string[]; effective?: string[]; // omit when the caller can't resolve group-held grants; then only direct shows readOnly?: boolean; t?: Translate; + transitive?: boolean; // a group: its members inherit, so the change lands at their next re-mint }): PermissionPicker { const t = opts.t ?? ((k: string) => k); const directSet = new Set(opts.direct); @@ -92,6 +96,7 @@ export function buildPermissionPicker(opts: { hint: t("admin.grants.hint"), inheritedNote: choices.some((c) => c.inherited) ? t("admin.grants.inherited") : undefined, legend: t("admin.grants.legend"), + pending: opts.transitive === true ? t("admin.grants.pending") : undefined, readOnly: opts.readOnly === true, submit: t("admin.grants.save"), }; @@ -100,7 +105,7 @@ export function buildPermissionPicker(opts: { // What a submitted set changes. Pure so the diff is testable without Keto: only declared names are // considered, so a crafted POST cannot grant something no plugin gates on, and a held-but-undeclared // name (left over from an uninstalled plugin) is never silently revoked by an unrelated save. -export function grantDiff(declared: PermissionDecl[], held: string[], wanted: string[]): { grant: string[]; revoke: string[] } { +export function grantDiff(declared: readonly PermissionDecl[], held: string[], wanted: string[]): { grant: string[]; revoke: string[] } { const offered = new Set(declared.map((d) => d.name)); const heldSet = new Set(held); const wantedSet = new Set(wanted.filter((name) => offered.has(name))); diff --git a/examples/plugins/admin/admin-groups.ts b/examples/plugins/admin/admin-groups.ts index 7fcf7ca..5047b12 100644 --- a/examples/plugins/admin/admin-groups.ts +++ b/examples/plugins/admin/admin-groups.ts @@ -8,7 +8,7 @@ import { can, type KetoClient, type KratosAdmin, paginate, parseListQuery, type RelationQuery, type RelationTuple, type RequestContext, type RouteHandler, type RouteResult, type SubjectSet, type Translate, type User } from "#plugin-api"; import { applyGrants, buildPermissionPicker, effectivePermissions, grantDiff, groupSubject, heldPermissions, type PermissionPicker, PERMISSIONS_FIELD } from "./admin-grants.ts"; -import { ADMIN_EN, ADMIN_GROUPS_BASE, buildConfirmModel, guardedForm, notFound, permissionName, requirePermission, unavailable } from "./admin-shared.ts"; +import { ADMIN_EN, type AdminAction, ADMIN_GROUPS_BASE, buildConfirmModel, guardedForm, notFound, permissionName, requirePermission, unavailable } from "./admin-shared.ts"; import type { FieldConfig } from "./admin-users.ts"; const GROUP_NS = "Group"; @@ -298,9 +298,9 @@ async function groupExists(keto: KetoClient, name: string): Promise { // below is a thin handler over these. interface GroupsDeps { ctx: RequestContext; keto: KetoClient; kratosAdmin: KratosAdmin; user: User; } -function withGroups(inner: (deps: GroupsDeps) => Promise): RouteHandler { +function withGroups(inner: (deps: GroupsDeps) => Promise, action?: AdminAction): RouteHandler { return async (ctx) => { - const user = requirePermission(ctx, "groups"); + const user = requirePermission(ctx, "groups", action); const keto = ctx.system?.keto; const kratosAdmin = ctx.system?.kratosAdmin; if (!keto || !kratosAdmin) return unavailable(ctx, ctx.t("admin.capability.keto")); @@ -309,12 +309,12 @@ function withGroups(inner: (deps: GroupsDeps) => Promise): RouteHan } // Same, plus the validated :name from ctx.params (an invalid group name → themed 404). -function withGroupName(inner: (deps: GroupsDeps, name: string) => Promise): RouteHandler { +function withGroupName(inner: (deps: GroupsDeps, name: string) => Promise, action?: AdminAction): RouteHandler { return withGroups((deps) => { const name = deps.ctx.params["name"] ?? ""; if (!isValidGroupName(name)) return Promise.resolve(notFound(deps.ctx)); return inner(deps, name); - }); + }, action); } const groupFormResult = async (deps: GroupsDeps, extra: { error?: string; values?: { member?: string; name?: string } }): Promise => { @@ -345,7 +345,7 @@ export const groupsCreate = withGroups(async (deps) => { }); // GET /admin/groups/new — the create form. -export const groupsNewForm = withGroups((deps) => groupFormResult(deps, {})); +export const groupsNewForm = withGroups((deps) => groupFormResult(deps, {}), "write"); // GET /admin/groups/:name — the detail + membership page. export const groupsDetail = withGroupName(async ({ ctx, keto, kratosAdmin }, name) => { @@ -360,6 +360,7 @@ export const groupsDetail = withGroupName(async ({ ctx, keto, kratosAdmin }, nam effective, // a group nested in another group inherits its permissions too readOnly: !can(ctx, permissionName("groups", "write")), t: ctx.t, + transitive: true, // members inherit, so a change here lands at their next re-mint, not at once }); return { data: { chrome: ctx.chrome, model: buildGroupDetailModel({ canWrite: !permissions.readOnly, candidates: options, csrfToken: ctx.chrome.csrfToken, group: { name }, members, permissions, t: ctx.t }) }, view: "group-detail" }; }); @@ -395,7 +396,7 @@ export const groupsDeleteConfirm = withGroupName((deps, name) => { cancelHref: base, confirmAction: `${base}/delete`, confirmLabel: tt("admin.groups.delete"), message: tt("admin.groups.deleteMessage", { name }), title: tt("admin.groups.delete"), }) }, view: "confirm" }); -}); +}, "write"); // POST /admin/groups/:name/delete — remove every member tuple (the group ceases to exist). export const groupsDelete = withGroupName(async ({ ctx, keto, user }, name) => { diff --git a/examples/plugins/admin/admin-shared.ts b/examples/plugins/admin/admin-shared.ts index 22d5929..aeed4e1 100644 --- a/examples/plugins/admin/admin-shared.ts +++ b/examples/plugins/admin/admin-shared.ts @@ -57,9 +57,13 @@ export const ADMIN_NAV: NavNode = { // declares the same permission, so the host enforces it before the handler runs; this is // defence-in-depth and what a direct unit test relies on. Returns the (non-null) user for the // handler to thread on. GuardError → /login or 403. -export function requirePermission(ctx: RequestContext, resource: AdminResource): User { +// `action` defaults to the method's, and is passed explicitly by a *write-intent GET* — a create form +// or a delete-confirm page, whose only purpose is to start a write. Those refuse a reader honestly +// instead of rendering a form whose submit would 403; the route table declares the same override, so +// the two still cannot disagree. +export function requirePermission(ctx: RequestContext, resource: AdminResource, action?: AdminAction): User { const user = requireSession(ctx); // anonymous → GuardError → /login (return_to kept) - const permission = permissionName(resource, actionForMethod(ctx.req.method ?? "GET")); + const permission = permissionName(resource, action ?? actionForMethod(ctx.req.method ?? "GET")); if (!can(ctx, permission)) throw new GuardError(403, `${permission} required`); return user; } diff --git a/examples/plugins/admin/admin-users.ts b/examples/plugins/admin/admin-users.ts index 4237c6d..14982b5 100644 --- a/examples/plugins/admin/admin-users.ts +++ b/examples/plugins/admin/admin-users.ts @@ -6,7 +6,7 @@ import { can, type Identity, type KetoClient, type KratosAdmin, KratosError, paginate, parseListQuery, type RecoveryCode, type RequestContext, type RouteHandler, type RouteResult, type Translate, type User } from "#plugin-api"; import { applyGrants, buildPermissionPicker, effectivePermissions, grantDiff, heldPermissions, type PermissionPicker, PERMISSIONS_FIELD, userSubject } from "./admin-grants.ts"; -import { ADMIN_EN, ADMIN_USERS_BASE, buildConfirmModel, guardedForm, notFound, permissionName, requirePermission, unavailable } from "./admin-shared.ts"; +import { ADMIN_EN, type AdminAction, ADMIN_USERS_BASE, buildConfirmModel, guardedForm, notFound, permissionName, requirePermission, unavailable } from "./admin-shared.ts"; const SCHEMA_ID = "default"; // matches kratos.yml identity.default_schema_id const DEFAULT_PAGE_SIZE = 25; @@ -283,9 +283,9 @@ interface UsersDeps { ctx: RequestContext; keto: KetoClient | undefined; kratosA // Resolve the shared deps, then run `inner`. The route's own `permission` already gated at the host; // `requirePermission` is defence-in-depth and yields the user. GuardError (auth/CSRF) → host maps it. -function withUser(inner: (deps: UsersDeps) => Promise): RouteHandler { +function withUser(inner: (deps: UsersDeps) => Promise, action?: AdminAction): RouteHandler { return async (ctx) => { - const user = requirePermission(ctx, "users"); + const user = requirePermission(ctx, "users", action); const kratosAdmin = ctx.system?.kratosAdmin; if (!kratosAdmin) return unavailable(ctx, ctx.t("admin.capability.kratos")); return inner({ ctx, keto: ctx.system?.keto, kratosAdmin, revoke: ctx.system?.revoke, user }); @@ -294,13 +294,13 @@ function withUser(inner: (deps: UsersDeps) => Promise): RouteHandle // Same, plus the target identity from ctx.params.id (unknown id → themed 404). The router already // decoded the id and 404s malformed %-encoding, so no manual decode is needed here. -function withTarget(inner: (deps: UsersDeps, identity: Identity, id: string) => Promise): RouteHandler { +function withTarget(inner: (deps: UsersDeps, identity: Identity, id: string) => Promise, action?: AdminAction): RouteHandler { return withUser(async (deps) => { const id = deps.ctx.params["id"] ?? ""; const identity = await deps.kratosAdmin.getIdentity(id); if (!identity) return notFound(deps.ctx); return inner(deps, identity, id); - }); + }, action); } const formResult = (ctx: RequestContext, extra: Parameters[0]): RouteResult => @@ -326,7 +326,7 @@ export const usersCreate = withUser(async ({ ctx, kratosAdmin, user }) => { }); // GET /admin/users/new — the empty create form. -export const usersNewForm = withUser(({ ctx }) => Promise.resolve(formResult(ctx, {}))); +export const usersNewForm = withUser(({ ctx }) => Promise.resolve(formResult(ctx, {})), "write"); // GET /admin/users/:id — the edit form, prefilled. export const usersEditForm = withTarget(async (deps, identity, id) => { @@ -372,6 +372,7 @@ export const usersPermissions = withTarget(async (deps, identity, id) => { // remove the last `users:write` on the deployment, and the instant-revoke hook lands it on the very // next request. Recovery would be a curl against Keto — not something the operator persona can do. if (id === user.id && diff.revoke.length > 0) { + ctx.log.warn("admin: refused a self-revoke of permissions", { actor: user.id, refused: diff.revoke.join(",") }); const permissions = await userPermissionPicker(deps, id, ctx.t("admin.grants.selfRevoke")); return { ...formResult(ctx, { canWrite: canWriteUsers(ctx), identity, ...(permissions ? { permissions } : {}) }), status: 400 }; } @@ -384,12 +385,14 @@ export const usersPermissions = withTarget(async (deps, identity, id) => { }); // POST /admin/users/:id — save edits; a Kratos 4xx re-renders the form (400). -export const usersUpdate = withTarget(async ({ ctx, kratosAdmin }, identity, id) => { +export const usersUpdate = withTarget(async (deps, identity, id) => { + const { ctx, kratosAdmin } = deps; const input = readUserInput((await guardedForm(ctx))!); try { await kratosAdmin.updateIdentity(id, updateIdentityPayload(identity, input)); } catch (err) { - if (err instanceof KratosError) return { ...formResult(ctx, { error: ctx.t("admin.users.error.save"), identity }), status: 400 }; + // Re-render with the picker, or the permissions section vanishes off the page on a failed save. + if (err instanceof KratosError) return { ...formResult(ctx, { canWrite: canWriteUsers(ctx), error: ctx.t("admin.users.error.save"), identity, ...(await pickerOrNothing(deps, id)) }), status: 400 }; throw err; } return { redirect: `${ADMIN_USERS_BASE}/${encodeURIComponent(id)}` }; @@ -418,7 +421,7 @@ export const usersDeleteConfirm = withTarget((deps, identity, id) => { cancelHref: back, confirmAction: `${back}/delete`, confirmLabel: tt("admin.users.delete"), message: tt("admin.users.deleteMessage", { email: view.email }), title: tt("admin.users.delete"), }) }, view: "confirm" }); -}); +}, "write"); // POST /admin/users/:id/delete — perform it; revoke the gone account's live tokens. Refuses self-delete. export const usersDelete = withTarget(async ({ ctx, kratosAdmin, revoke, user }, identity, id) => { @@ -431,12 +434,19 @@ export const usersDelete = withTarget(async ({ ctx, kratosAdmin, revoke, user }, }); // POST /admin/users/:id/recovery — mint a one-time recovery code, shown on the edit page. -export const usersRecovery = withTarget(async ({ ctx, kratosAdmin }, identity, id) => { +export const usersRecovery = withTarget(async (deps, identity, id) => { + const { ctx, kratosAdmin } = deps; await guardedForm(ctx); // CSRF-verify the POST const recovery = await kratosAdmin.createRecoveryCode(id); - return formResult(ctx, { identity, recovery }); + return formResult(ctx, { canWrite: canWriteUsers(ctx), identity, recovery, ...(await pickerOrNothing(deps, id)) }); }); +// The picker as a spreadable fragment, so a re-render never silently drops the section. +async function pickerOrNothing(deps: UsersDeps, id: string): Promise<{ permissions?: PermissionPicker }> { + const permissions = await userPermissionPicker(deps, id); + return permissions ? { permissions } : {}; +} + function createError(err: KratosError, t: Translate): string { return err.status === 409 ? t("admin.users.error.duplicate") diff --git a/examples/plugins/admin/plugin.test.ts b/examples/plugins/admin/plugin.test.ts index 44b77c1..9c24248 100644 --- a/examples/plugins/admin/plugin.test.ts +++ b/examples/plugins/admin/plugin.test.ts @@ -51,8 +51,12 @@ test("every declared permission is :, and reads and writes are }); test("GET routes gate on read and mutations on write, so a reader can open a screen but not change it", () => { + // …except a write-intent GET — a create form or a delete-confirm page, which exists only to start a + // write. Those gate on `:write` so a reader is refused there rather than at the submit. + const writeIntent = (path: string): boolean => path.endsWith("/new") || path.endsWith("/delete"); for (const route of routes) { - const action = route.method === "GET" ? "read" : "write"; + const action = route.method === "GET" && !writeIntent(route.path) ? "read" : "write"; assert.ok(route.permission?.endsWith(`:${action}`), `${route.method} ${route.path} → ${route.permission}`); } + assert.equal(routes.filter((r) => r.method === "GET" && writeIntent(r.path)).length, 6); // 2 per screen }); diff --git a/examples/plugins/admin/plugin.ts b/examples/plugins/admin/plugin.ts index 7523f19..c50120b 100644 --- a/examples/plugins/admin/plugin.ts +++ b/examples/plugins/admin/plugin.ts @@ -10,15 +10,18 @@ import { definePlugin, type HttpMethod, type Route, type RouteHandler } from "#p import { clientsCreate, clientsDeleteConfirm, clientsDelete, clientsDetail, clientsList, clientsNewForm } from "./admin-clients.ts"; import { groupsAddMember, groupsCreate, groupsDelete, groupsDeleteConfirm, groupsDetail, groupsList, groupsNewForm, groupsPermissions, groupsRemoveMember } from "./admin-groups.ts"; import { usersCreate, usersDeleteConfirm, usersDelete, usersEditForm, usersList, usersNewForm, usersPermissions, usersRecovery, usersState, usersUpdate } from "./admin-users.ts"; -import { ADMIN_NAV, actionForMethod, type AdminResource, permissionName } from "./admin-shared.ts"; +import { ADMIN_NAV, actionForMethod, type AdminAction, type AdminResource, permissionName } from "./admin-shared.ts"; // 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: permissionName(resource, actionForMethod(method)) }); +// `action` overrides the method's default for a *write-intent GET* — a create form or a +// delete-confirm page, which exists only to start a write and so refuses a reader rather than +// rendering a form whose submit would 403. The handler's own guard takes the same override. +const on = (resource: AdminResource) => (method: HttpMethod, path: string, handler: RouteHandler, action?: AdminAction): Route => + ({ handler, method, path, permission: permissionName(resource, action ?? actionForMethod(method)) }); const users = on("users"); const groups = on("groups"); @@ -42,30 +45,30 @@ export default definePlugin({ // Users users("GET", "/users", usersList), users("POST", "/users", usersCreate), - users("GET", "/users/new", usersNewForm), + users("GET", "/users/new", usersNewForm, "write"), users("GET", "/users/:id", usersEditForm), users("POST", "/users/:id", usersUpdate), users("POST", "/users/:id/state", usersState), - users("GET", "/users/:id/delete", usersDeleteConfirm), + users("GET", "/users/:id/delete", usersDeleteConfirm, "write"), users("POST", "/users/:id/delete", usersDelete), users("POST", "/users/:id/recovery", usersRecovery), users("POST", "/users/:id/permissions", usersPermissions), // Groups groups("GET", "/groups", groupsList), groups("POST", "/groups", groupsCreate), - groups("GET", "/groups/new", groupsNewForm), + groups("GET", "/groups/new", groupsNewForm, "write"), groups("GET", "/groups/:name", groupsDetail), groups("POST", "/groups/:name/members", groupsAddMember), - groups("GET", "/groups/:name/delete", groupsDeleteConfirm), + groups("GET", "/groups/:name/delete", groupsDeleteConfirm, "write"), groups("POST", "/groups/:name/delete", groupsDelete), groups("POST", "/groups/:name/members/delete", groupsRemoveMember), groups("POST", "/groups/:name/permissions", groupsPermissions), // OAuth2 clients clients("GET", "/clients", clientsList), clients("POST", "/clients", clientsCreate), - clients("GET", "/clients/new", clientsNewForm), + clients("GET", "/clients/new", clientsNewForm, "write"), clients("GET", "/clients/:id", clientsDetail), - clients("GET", "/clients/:id/delete", clientsDeleteConfirm), + clients("GET", "/clients/:id/delete", clientsDeleteConfirm, "write"), clients("POST", "/clients/:id/delete", clientsDelete), ], }); diff --git a/examples/plugins/admin/views/client-detail.ejs b/examples/plugins/admin/views/client-detail.ejs index 54a34f0..17f4897 100644 --- a/examples/plugins/admin/views/client-detail.ejs +++ b/examples/plugins/admin/views/client-detail.ejs @@ -3,7 +3,7 @@ shell. Doubles as the post-register page when `created`/`secret` are set. %><% const nav = include("partials/nav-tree", { nodes: chrome.nav }); - const body = include("partials/client-detail-body", { client: model.client, created: model.created, csrfToken: chrome.csrfToken, del: model.delete, secret: model.secret }); + const body = include("partials/client-detail-body", { canWrite: model.canWrite, client: model.client, created: model.created, csrfToken: chrome.csrfToken, del: model.delete, secret: model.secret }); -%> <%- include("partials/shell", { body, diff --git a/examples/plugins/admin/views/clients.ejs b/examples/plugins/admin/views/clients.ejs index f0dca77..cc3916e 100644 --- a/examples/plugins/admin/views/clients.ejs +++ b/examples/plugins/admin/views/clients.ejs @@ -6,7 +6,8 @@ const filters = include("partials/filter-bar", model.filterBar); const table = include("partials/data-table", model.table); const pager = include("partials/pagination", model.pagination); - const actions = '' + t("admin.clients.registerClient") + ''; + // Only offer "Register client" to an oauth2-clients:write holder — a :read one would get the 403 page. + const actions = model.canWrite === false ? "" : '' + t("admin.clients.registerClient") + ''; -%> <%- include("partials/shell", { actions, diff --git a/examples/plugins/admin/views/partials/client-detail-body.ejs b/examples/plugins/admin/views/partials/client-detail-body.ejs index 1fee872..6eeec30 100644 --- a/examples/plugins/admin/views/partials/client-detail-body.ejs +++ b/examples/plugins/admin/views/partials/client-detail-body.ejs @@ -31,8 +31,10 @@
<%= t("admin.clients.field.redirectUris") %>
<% if (c.redirectUris.length) { %>
    <% c.redirectUris.forEach((u) => { %>
  • <%= u %>
    • <% }) %>
<% } else { %>—<% } %>
+<% if (locals.canWrite !== false) { -%>
">

<%= t("admin.clients.rereg") %>

<%= t("admin.clients.delete") %>
+<% } -%> diff --git a/examples/plugins/admin/views/partials/permission-picker.ejs b/examples/plugins/admin/views/partials/permission-picker.ejs index bd09a2c..7d955a9 100644 --- a/examples/plugins/admin/views/partials/permission-picker.ejs +++ b/examples/plugins/admin/views/partials/permission-picker.ejs @@ -42,5 +42,8 @@ <% if (permissions.inheritedNote) { -%>

<%= permissions.inheritedNote %>

<% } -%> +<% if (permissions.pending) { -%> +

<%= permissions.pending %>

+<% } -%> <% } -%> diff --git a/src/http/app.test.ts b/src/http/app.test.ts index 3af29b9..92cc98a 100644 --- a/src/http/app.test.ts +++ b/src/http/app.test.ts @@ -1110,7 +1110,7 @@ test("admin Users screen: gate, list/filter, create, edit, deactivate, delete, r // Nav: the admin plugin's section composes into the one global menu, and each screen is filtered // by its own read permission — proving the drop-in nav fragment. A user holding only users:read - // sees Users and nothing else; holding none of the four, composeNav drops the emptied header. + // sees Users and nothing else; holding none of the three, composeNav drops the emptied header. assert.match(await (await get("/dashboard")).text(), /href="\/admin\/users"/); const usersOnlyNav = await (await get("/dashboard", ["users:read"])).text(); assert.match(usersOnlyNav, /href="\/admin\/users"/); @@ -1317,7 +1317,10 @@ test("admin screens render no write affordance for a read-only holder", async (t const identities: Identity[] = [{ id: ada, traits: { email: "ada@example.com" } }]; const keto = fakeKeto([{ namespace: "Group", object: "eng", relation: "members", subject_id: `user:${ada}` }]); const kratosAdmin = stubAdmin({ getIdentity: async (id) => identities.find((i) => i.id === id) ?? null, listIdentities: async () => ({ identities, nextPageToken: null }) }); - const { get } = await adminHarness(t, { keto, kratosAdmin }); + // Hydra is wired so the clients screen renders for real — without it the page is a 503 and the + // "no Register button" assertion below would pass without proving anything. + const hydra = stubHydra({ listClients: async () => ({ clients: [{ client_id: "existing", client_name: "Reporting" }], nextPageToken: null }) }); + const { get } = await adminHarness(t, { hydra, keto, kratosAdmin }); const readOnly = ["users:read", "groups:read"]; const list = await (await get("/admin/users", readOnly)).text(); @@ -1337,6 +1340,20 @@ test("admin screens render no write affordance for a read-only holder", async (t assert.doesNotMatch(group, /Delete group/); assert.doesNotMatch(group, /Save permissions/); + // The OAuth2-clients screen is held to the same rule (it was the one this test was written to catch). + const clientsRes = await get("/admin/clients", ["oauth2-clients:read"]); + assert.equal(clientsRes.status, 200); // a real render, not the capability-missing 503 + const clients = await clientsRes.text(); + assert.match(clients, /Reporting/); // the list is there — that's what :read buys + assert.doesNotMatch(clients, /href="\/admin\/clients\/new"/); + + // A write-intent GET — a create form or a delete-confirm — refuses a reader outright rather than + // rendering a form whose submit would 403. + for (const path of ["/admin/users/new", "/admin/groups/new", `/admin/users/${ada}/delete`, "/admin/groups/eng/delete"]) { + assert.equal((await get(path, readOnly)).status, 403, path); + } + assert.equal((await get("/admin/clients/new", ["oauth2-clients:read"])).status, 403); + // A writer sees the affordances the reader didn't. const writable = await (await get(`/admin/users/${ada}`, ["users:read", "users:write"])).text(); assert.match(writable, /Save changes/); diff --git a/src/http/context.ts b/src/http/context.ts index 9b50235..653040b 100644 --- a/src/http/context.ts +++ b/src/http/context.ts @@ -41,7 +41,7 @@ export interface RequestContext { // Every permission the installed plugins declare, deduped and sorted — the fixed list an admin // screen offers when granting one. Pairs with `permissions` below: this is what *exists*, that is // what *this user holds*. Empty when no installed plugin declares any. - declaredPermissions: PermissionDecl[]; + declaredPermissions: readonly PermissionDecl[]; params: Record; // path params from the route match, e.g. /users/:id → { id } permissions: string[]; // user?.permissions ?? [] — coarse gate without a null-check query: URLSearchParams; // alias of url.searchParams, for ctx.query.get("q") @@ -66,7 +66,7 @@ export interface BuildContextOptions { // ctx.chrome (a json/redirect handler, or the public "/" with a standalone home, pays nothing). // The host's factory is memoised, so the menu composes at most once per request across contexts. chrome?: () => PageChrome; - declaredPermissions?: PermissionDecl[]; + declaredPermissions?: readonly PermissionDecl[]; user?: User | null; locale?: string; localeHref?: (href: string) => string; diff --git a/todo.md b/todo.md index 6a7af23..445020f 100644 --- a/todo.md +++ b/todo.md @@ -2,6 +2,8 @@ ## Unfinnished work +- [ ] Deleting a group orphans its permission grants. `groupsDelete` removes every `Group:#members` tuple, but nothing removes `Permission:X#granted@Group:#members` — so re-creating a group with the same name silently restores every permission it used to hold. Pre-existing (the same orphan was reachable via the old Permissions screen), but granting to a group is now a one-click flow on the group's own page, which raises the odds a lot. Raised by the stability review 2026-08-05. +- [ ] Guard the group paths to self-lockout, or accept them explicitly. The self-revoke guard covers only your own *direct* grants on the Users screen; unticking a permission on a group you belong to, removing yourself from that group, or deleting it can all still strip your own effective access with no warning. Same scope the deleted Permissions screen had, and recorded in AGENTS.md as a known gap — the robust fix is a "last effective holder" check, which needs a reverse Keto query. Raised by the stability review 2026-08-05. - [ ] The permission picker has no concurrency baseline, so two operators editing the same user/group silently discard each other's change (standard lost-update on a set-based form — and the natural "two of us are onboarding the new hire" workflow produces exactly it). Sketch: post the rendered set as a hidden baseline; if it no longer matches Keto, re-render with "this changed while you had the page open" rather than applying. Fits the existing "the form is the whole truth" model instead of fighting it. Raised by the product review 2026-08-05. - [ ] A grant whose plugin was uninstalled is invisible and unremovable in the GUI. `grantDiff` deliberately never revokes an undeclared name (so an unrelated save can't drop it), but nothing *shows* it either — so it can't be audited or cleaned, and reinstalling that plugin silently reactivates access nobody remembers granting. Sketch: a read-only "held, but no installed plugin offers this" list with a remove action. Raised by the product review 2026-08-05. - [ ] A plugin may gate a route on a permission it never declares — declaring stays optional on purpose (mandatory declaration would warn on the legitimate cross-plugin sharing case). The cost is a dead end: the picker is built from declarations only, so that route is ungrantable from the GUI with no boot error, no warning, and a permanent 403 as the operator's only clue. Sketch: a discovery *warning* (not an error) naming the gated-but-undeclared permission. Raised by the product review 2026-08-05.