From c78e95889cd875ece27b179392f9d307654dd934 Mon Sep 17 00:00:00 2001 From: lilleman Date: Thu, 18 Jun 2026 19:18:50 +0200 Subject: [PATCH] =?UTF-8?q?Address=20whole-project=20architecture=20+=20pr?= =?UTF-8?q?oduct=20reviews=20(todo=20=C2=A75):=20make=20readRoles=20transi?= =?UTF-8?q?tive=20so=20group=E2=86=92role=20grants=20reach=20the=20JWT=20(?= =?UTF-8?q?matches=20the=20Roles=20'Effective=20access'=20view=20+=20OPL?= =?UTF-8?q?=20model;=20per-login=20only),=20per=20the=20user's=20call;=20a?= =?UTF-8?q?dd=20a=20zero-JS=20server-rendered=20confirm=20step=20for=20del?= =?UTF-8?q?ete=20user/group/role=20(views/admin/confirm.ejs=20+=20shared?= =?UTF-8?q?=20buildConfirmModel;=20the=20Delete=20control=20is=20now=20a?= =?UTF-8?q?=20GET=20link,=20the=20delete=20stays=20a=20CSRF-guarded=20POST?= =?UTF-8?q?);=20self-lockout=20guards=20=E2=80=94=20no=20self-delete/deact?= =?UTF-8?q?ivate=20(Users),=20no=20self-revoke=20of=20the=20direct=20admin?= =?UTF-8?q?=20grant=20+=20no=20delete=20of=20the=20admin=20role=20(Roles),?= =?UTF-8?q?=20each=20=E2=86=92=20400=20+=20inline=20error=20(direct-grant?= =?UTF-8?q?=20paths=20incl.=20the=20seeded=20admin;=20group-only-admin=20l?= =?UTF-8?q?ockout=20=3D=20robust=20last-effective-admin=20check=20deferred?= =?UTF-8?q?=20=C2=A79);=20extract=20the=20gate+CSRF=20preamble=20copied=20?= =?UTF-8?q?across=20the=203=20admin=20handlers=20into=20admin-nav.ts=20req?= =?UTF-8?q?uireAdmin/guardedForm;=20shellUser=20keeps=20the=20email=20(nam?= =?UTF-8?q?e=20=3D=20local=20part,=20full=20email=20beneath).=20Reviewers:?= =?UTF-8?q?=20architecture=20no=20Critical/High,=20product=202=20Critical?= =?UTF-8?q?=20+=201=20High=20(all=20fixed).=20Deferred=20(scoped):=20host?= =?UTF-8?q?=20route-table=E2=86=92=C2=A76,=20list/template=20dedup?= =?UTF-8?q?=E2=86=92=C2=A75=20cleanup,=20success-flash/empty-states/dangli?= =?UTF-8?q?ng-refs=E2=86=92=C2=A75=20polish/=C2=A78,=20safeUrl=E2=86=92?= =?UTF-8?q?=C2=A77,=20413/https/=C2=A7N-drift=E2=86=92=C2=A79.=20Tests-fir?= =?UTF-8?q?st=20(extended=20the=203=20admin=20HTTP=20tests=20+=20login/she?= =?UTF-8?q?ll-context=20units);=20typecheck=20+=20244=20units=20+=208=20vi?= =?UTF-8?q?sual=20+=20auth-refresh=20E2E=20green;=20stability-reviewer=20A?= =?UTF-8?q?PPROVE?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- README.md | 8 +++- src/admin-groups.ts | 26 +++++-------- src/admin-nav.ts | 47 +++++++++++++++++++++++ src/admin-roles.ts | 46 +++++++++++----------- src/admin-users.ts | 57 ++++++++++++++-------------- src/app.test.ts | 30 ++++++++++++--- src/login.test.ts | 29 +++++++++----- src/login.ts | 18 +++++---- src/shell-context.test.ts | 4 +- src/shell-context.ts | 5 ++- todo.md | 2 +- views/admin/confirm.ejs | 17 +++++++++ views/partials/confirm-body.ejs | 17 +++++++++ views/partials/group-detail-body.ejs | 2 +- views/partials/role-detail-body.ejs | 2 +- views/partials/user-form-body.ejs | 2 +- 16 files changed, 213 insertions(+), 99 deletions(-) create mode 100644 views/admin/confirm.ejs create mode 100644 views/partials/confirm-body.ejs diff --git a/README.md b/README.md index 19b8c87..c036486 100644 --- a/README.md +++ b/README.md @@ -409,7 +409,7 @@ session cookie. ``` ── AT LOGIN / REFRESH (the only time Ory is on the path) ────────── Kratos verifies credentials - └─► app reads the user's roles from Keto (Keto = source of truth) + └─► app reads the user's roles from Keto (direct + transitive via groups) └─► app writes them as a derived projection on the identity (admin API) └─► whoami(tokenize_as: "plainpages") ─► signed JWT claims: { sub, email, roles:[…from Keto], exp ≈ 10m } @@ -432,7 +432,11 @@ and the user can already read these coarse roles in their own JWT, so nothing is That projection is a per-login cache, authoritative nowhere; nothing edits it by hand, and a stale one self-heals on the next login. -Cost: **one Keto read + one identity refresh per login** — never per request. JWKS +A role can be granted to a user directly or to a **group** the user belongs to; login +resolves both (enumerate the defined roles, ask Keto to resolve each membership), so the +JWT `roles` match what the admin **Effective access** view shows. + +Cost: **a handful of Keto reads + one identity refresh per login** — never per request. JWKS is cached, so even signature verification hits the network only on key rotation. The app stays stateless; "stay signed in" = re-mint the JWT on a short TTL, the one moment authz is recomputed from Keto. diff --git a/src/admin-groups.ts b/src/admin-groups.ts index 8639ad8..53b0700 100644 --- a/src/admin-groups.ts +++ b/src/admin-groups.ts @@ -7,12 +7,9 @@ // building-block view models; `handleAdminGroups` is the imperative shell app.ts dispatches to — it // gates (admin only), CSRF-guards every mutation, and maps each action to a RouteResult. -import { ADMIN_GROUPS_BASE, ADMIN_PERMISSION, adminNav } from "./admin-nav.ts"; +import { ADMIN_GROUPS_BASE, adminNav, buildConfirmModel, guardedForm, requireAdmin } from "./admin-nav.ts"; import type { FieldConfig } from "./admin-users.ts"; -import { readFormBody } from "./body.ts"; import type { RequestContext, User } from "./context.ts"; -import { CSRF_FIELD, verifyCsrfRequest } from "./csrf.ts"; -import { GuardError } from "./guards.ts"; import type { KetoClient, RelationQuery, RelationTuple, SubjectSet } from "./keto-client.ts"; import type { KratosAdmin } from "./kratos-admin.ts"; import { parseListQuery } from "./list-query.ts"; @@ -334,21 +331,11 @@ export async function handleAdminGroups(ctx: RequestContext, csrfToken: string, const path = ctx.url.pathname; if (path !== ADMIN_GROUPS_BASE && !path.startsWith(`${ADMIN_GROUPS_BASE}/`)) return null; - if (!ctx.user) throw new GuardError(401, "authentication required", "/login"); - if (!ctx.roles.includes(ADMIN_PERMISSION)) throw new GuardError(403, "admin role required"); - + const user = requireAdmin(ctx); // signed-in admin only (else GuardError → /login or 403) const { keto, kratosAdmin, menu, render } = deps; - const user = ctx.user; const method = (ctx.req.method ?? "GET").toUpperCase(); const seg = path.slice(ADMIN_GROUPS_BASE.length).split("/").filter(Boolean); - - let form: URLSearchParams | undefined; - if (method === "POST") { - form = await readFormBody(ctx.req); - if (!verifyCsrfRequest({ cookieHeader: ctx.req.headers.cookie, secret: deps.csrfSecret, submitted: form.get(CSRF_FIELD) })) { - throw new GuardError(403, "invalid CSRF token"); - } - } + const form = await guardedForm(ctx, deps.csrfSecret); // parsed + CSRF-verified on POST, else undefined const renderList = async (): Promise => { const groups = groupsFromTuples(await pagedTuples(keto, { namespace: GROUP_NS, relation: MEMBERS })); @@ -397,6 +384,13 @@ export async function handleAdminGroups(ctx: RequestContext, csrfToken: string, if (tuple && tuple.subject_set?.object !== name) await keto.writeTuple(tuple); return { redirect: base }; } + if (seg.length === 2 && seg[1] === "delete" && method === "GET") { + return { html: await render("admin/confirm", { model: buildConfirmModel({ + breadcrumbs: [{ href: ADMIN_GROUPS_BASE, label: "Groups" }, { href: base, label: name }, { label: "Delete" }], + cancelHref: base, confirmAction: `${base}/delete`, confirmLabel: "Delete group", csrfToken, + current: "groups", menu, message: `Delete group ${name}? This removes the group and all its memberships.`, title: "Delete group", user, + }) }) }; + } if (seg.length === 2 && seg[1] === "delete" && method === "POST") { await keto.deleteTuple({ namespace: GROUP_NS, object: name, relation: MEMBERS }); // removes every member tuple return { redirect: ADMIN_GROUPS_BASE }; diff --git a/src/admin-nav.ts b/src/admin-nav.ts index c4b5809..ba9cc6c 100644 --- a/src/admin-nav.ts +++ b/src/admin-nav.ts @@ -4,8 +4,13 @@ // for a non-admin), and `adminNav()` is the in-screen sidebar each admin screen renders (a link home // + the same section, with the active item marked `current`). +import { readFormBody } from "./body.ts"; +import type { RequestContext, User } from "./context.ts"; +import { CSRF_FIELD, verifyCsrfRequest } from "./csrf.ts"; +import { GuardError } from "./guards.ts"; import { type MenuConfig } from "./menu-config.ts"; import { composeNav, type NavNode } from "./nav.ts"; +import { buildShellContext } from "./shell-context.ts"; export const ADMIN_PERMISSION = "admin"; // role token gating the admin section export const ADMIN_USERS_BASE = "/admin/users"; @@ -40,3 +45,45 @@ export function adminNav(roles: string[], menu: MenuConfig, current: AdminScreen adminSection(current), ]], menu.override, roles); } + +// The shared gate for every admin screen: a signed-in admin only. Throws GuardError that app.ts maps +// (anonymous → /login, non-admin → 403). Returns the (non-null) user for the handler to thread on. +export function requireAdmin(ctx: RequestContext): User { + if (!ctx.user) throw new GuardError(401, "authentication required", "/login"); + if (!ctx.roles.includes(ADMIN_PERMISSION)) throw new GuardError(403, "admin role required"); + return ctx.user; +} + +// Read + CSRF-verify a mutation's form body once. Every admin write is a first-party POST form, so a +// POST without a valid double-submit token is refused (GuardError → 403); non-POST ⇒ undefined. +export async function guardedForm(ctx: RequestContext, csrfSecret: string): Promise { + if ((ctx.req.method ?? "GET").toUpperCase() !== "POST") return undefined; + const form = await readFormBody(ctx.req); + if (!verifyCsrfRequest({ cookieHeader: ctx.req.headers.cookie, secret: csrfSecret, submitted: form.get(CSRF_FIELD) })) { + throw new GuardError(403, "invalid CSRF token"); + } + return form; +} + +// Build the model for the shared destructive-action confirm page (views/admin/confirm.ejs): a single +// danger action behind a deliberate second step, plus a cancel link. Reused by all three screens. +export function buildConfirmModel(opts: { + breadcrumbs: { href?: string; label: string }[]; + cancelHref: string; + confirmAction: string; + confirmLabel: string; + csrfToken: string; + current: AdminScreen; + menu: MenuConfig; + message: string; + title: string; + user: User | null; +}) { + return { + cancelHref: opts.cancelHref, + confirm: { action: opts.confirmAction, label: opts.confirmLabel }, + message: opts.message, + nav: adminNav(opts.user?.roles ?? [], opts.menu, opts.current), + shell: buildShellContext({ breadcrumbs: opts.breadcrumbs, csrfToken: opts.csrfToken, menu: opts.menu, title: opts.title, user: opts.user }), + }; +} diff --git a/src/admin-roles.ts b/src/admin-roles.ts index a750faa..437c750 100644 --- a/src/admin-roles.ts +++ b/src/admin-roles.ts @@ -4,12 +4,12 @@ // the user|group membership model of the Groups screen, so the pure helpers (parseSubject, member // pickers, tuple paging) are reused from admin-groups. The one role-specific piece is the **effective // access** view: `keto.expand(Role:#members)` returns the membership tree, which we flatten to -// the distinct set of users who hold the role directly or transitively via a group. (The coarse JWT -// projection reads only direct grants per the README's one-read-per-login design; this view is where -// group→role inheritance is surfaced.) Writes go only to Keto; Kratos is read only to label members. +// the distinct set of users who hold the role directly or transitively via a group. Login resolves +// the same transitive membership into the JWT `roles` (login.ts readRoles), so this view matches what +// a user's token actually grants. Writes go only to Keto; Kratos is read only to label members. // `handleAdminRoles` is the imperative shell app.ts dispatches to — gated admin-only, CSRF-guarded. -import { ADMIN_PERMISSION, ADMIN_ROLES_BASE, adminNav } from "./admin-nav.ts"; +import { ADMIN_PERMISSION, ADMIN_ROLES_BASE, adminNav, buildConfirmModel, guardedForm, requireAdmin } from "./admin-nav.ts"; import { type GroupView, groupsFromTuples, @@ -23,10 +23,7 @@ import { safeDecode, } from "./admin-groups.ts"; import type { FieldConfig } from "./admin-users.ts"; -import { readFormBody } from "./body.ts"; import type { RequestContext, User } from "./context.ts"; -import { CSRF_FIELD, verifyCsrfRequest } from "./csrf.ts"; -import { GuardError } from "./guards.ts"; import type { ExpandTree, KetoClient, RelationTuple } from "./keto-client.ts"; import type { KratosAdmin } from "./kratos-admin.ts"; import { parseListQuery } from "./list-query.ts"; @@ -298,21 +295,11 @@ export async function handleAdminRoles(ctx: RequestContext, csrfToken: string, d const path = ctx.url.pathname; if (path !== ADMIN_ROLES_BASE && !path.startsWith(`${ADMIN_ROLES_BASE}/`)) return null; - if (!ctx.user) throw new GuardError(401, "authentication required", "/login"); - if (!ctx.roles.includes(ADMIN_PERMISSION)) throw new GuardError(403, "admin role required"); - + const user = requireAdmin(ctx); // signed-in admin only (else GuardError → /login or 403) const { keto, kratosAdmin, menu, render } = deps; - const user = ctx.user; const method = (ctx.req.method ?? "GET").toUpperCase(); const seg = path.slice(ADMIN_ROLES_BASE.length).split("/").filter(Boolean); - - let form: URLSearchParams | undefined; - if (method === "POST") { - form = await readFormBody(ctx.req); - if (!verifyCsrfRequest({ cookieHeader: ctx.req.headers.cookie, secret: deps.csrfSecret, submitted: form.get(CSRF_FIELD) })) { - throw new GuardError(403, "invalid CSRF token"); - } - } + const form = await guardedForm(ctx, deps.csrfSecret); // parsed + CSRF-verified on POST, else undefined const renderList = async (): Promise => { const roles = rolesFromTuples(await pagedTuples(keto, { namespace: ROLE_NS, relation: MEMBERS })); @@ -322,12 +309,13 @@ export async function handleAdminRoles(ctx: RequestContext, csrfToken: string, d const { options } = await memberCandidates(keto, kratosAdmin); return { html: await render("admin/role-form", { model: buildRoleFormModel({ csrfToken, memberOptions: options, menu, user, ...extra }) }) }; }; - const renderDetail = async (name: string): Promise => { + const renderDetail = async (name: string, error?: string): Promise => { const { emailById, options } = await memberCandidates(keto, kratosAdmin); const tuples = await pagedTuples(keto, { namespace: ROLE_NS, object: name, relation: MEMBERS }); const members = tuples.map((t) => memberView(t, emailById)); const effective = await effectiveUsers(keto, name, tuples.length > 0, emailById); - return { html: await render("admin/role-detail", { model: buildRoleDetailModel({ candidates: options, csrfToken, effective, members, menu, role: { name }, user }) }) }; + const html = await render("admin/role-detail", { model: buildRoleDetailModel({ candidates: options, csrfToken, effective, members, menu, role: { name }, user, ...(error ? { error } : {}) }) }); + return error ? { html, status: 400 } : { html }; }; // /admin/roles — list (GET) · create (POST) @@ -362,12 +350,26 @@ export async function handleAdminRoles(ctx: RequestContext, csrfToken: string, d if (tuple) await keto.writeTuple(tuple); // the picker only offers real users/groups return { redirect: base }; } + if (seg.length === 2 && seg[1] === "delete" && method === "GET") { + // Self-protection: deleting the admin role removes everyone's admin — refuse it outright. + if (name === ADMIN_PERMISSION) return renderDetail(name, "The admin role can't be deleted — it would remove all admin access."); + return { html: await render("admin/confirm", { model: buildConfirmModel({ + breadcrumbs: [{ href: ADMIN_ROLES_BASE, label: "Roles" }, { href: base, label: name }, { label: "Delete" }], + cancelHref: base, confirmAction: `${base}/delete`, confirmLabel: "Delete role", csrfToken, + current: "roles", menu, message: `Delete role ${name}? This revokes it from everyone it's assigned to.`, title: "Delete role", user, + }) }) }; + } if (seg.length === 2 && seg[1] === "delete" && method === "POST") { + if (name === ADMIN_PERMISSION) return renderDetail(name, "The admin role can't be deleted — it would remove all admin access."); await keto.deleteTuple({ namespace: ROLE_NS, object: name, relation: MEMBERS }); // removes every member tuple return { redirect: ADMIN_ROLES_BASE }; } if (seg.length === 3 && seg[1] === "members" && seg[2] === "delete" && method === "POST") { - const tuple = roleMemberTuple(name, (form!.get("member") ?? "").trim()); + const member = (form!.get("member") ?? "").trim(); + // Self-protection: don't let an admin revoke their own *direct* admin grant (would lock them out). + // Admin held only via a group isn't covered here — the robust "last effective admin" check is §9. + if (name === ADMIN_PERMISSION && member === `user:${user.id}`) return renderDetail(name, "You can't revoke your own admin access."); + const tuple = roleMemberTuple(name, member); if (tuple) await keto.deleteTuple(tuple); return { redirect: base }; } diff --git a/src/admin-users.ts b/src/admin-users.ts index 5be6c76..0edc0ec 100644 --- a/src/admin-users.ts +++ b/src/admin-users.ts @@ -5,11 +5,8 @@ // imperative shell app.ts dispatches to — it gates (admin only), CSRF-guards every mutation, and // maps each action to a RouteResult (render a page, or redirect after a write — PRG). -import { ADMIN_PERMISSION, ADMIN_USERS_BASE, adminNav } from "./admin-nav.ts"; -import { readFormBody } from "./body.ts"; +import { ADMIN_USERS_BASE, adminNav, buildConfirmModel, guardedForm, requireAdmin } from "./admin-nav.ts"; import type { RequestContext, User } from "./context.ts"; -import { CSRF_FIELD, verifyCsrfRequest } from "./csrf.ts"; -import { GuardError } from "./guards.ts"; import type { Identity, KratosAdmin, RecoveryCode } from "./kratos-admin.ts"; import { KratosError } from "./kratos-public.ts"; import { parseListQuery } from "./list-query.ts"; @@ -306,23 +303,11 @@ export async function handleAdminUsers(ctx: RequestContext, csrfToken: string, d const path = ctx.url.pathname; if (path !== ADMIN_USERS_BASE && !path.startsWith(`${ADMIN_USERS_BASE}/`)) return null; - if (!ctx.user) throw new GuardError(401, "authentication required", "/login"); - if (!ctx.roles.includes(ADMIN_PERMISSION)) throw new GuardError(403, "admin role required"); - + const user = requireAdmin(ctx); // signed-in admin only (else GuardError → /login or 403) const { kratosAdmin, menu, render } = deps; - const user = ctx.user; const method = (ctx.req.method ?? "GET").toUpperCase(); const seg = path.slice(ADMIN_USERS_BASE.length).split("/").filter(Boolean); - - // Every mutation is a first-party form → CSRF-guard it (the host doesn't gate plugin routes, - // but it owns these). Reads the body once; the action handlers reuse the parsed form. - let form: URLSearchParams | undefined; - if (method === "POST") { - form = await readFormBody(ctx.req); - if (!verifyCsrfRequest({ cookieHeader: ctx.req.headers.cookie, secret: deps.csrfSecret, submitted: form.get(CSRF_FIELD) })) { - throw new GuardError(403, "invalid CSRF token"); - } - } + const form = await guardedForm(ctx, deps.csrfSecret); // parsed + CSRF-verified on POST, else undefined const renderList = async (): Promise => { const { identities } = await kratosAdmin.listIdentities({ pageSize: LIST_FETCH_SIZE }); @@ -370,18 +355,32 @@ export async function handleAdminUsers(ctx: RequestContext, csrfToken: string, d return null; } - if (seg.length === 2 && method === "POST") { - if (seg[1] === "state") { - await kratosAdmin.updateIdentity(targetId, setStatePayload(identity, identity.state === "inactive" ? "active" : "inactive")); - return { redirect: back }; + if (seg.length === 2) { + const isSelf = targetId === user.id; // self-protection: an admin must not lock themselves out + if (seg[1] === "delete" && method === "GET") { + if (isSelf) return { ...(await renderForm({ error: "You can't delete your own account.", identity })), status: 400 }; + const view = toUserView(identity); + return { html: await render("admin/confirm", { model: buildConfirmModel({ + breadcrumbs: [{ href: ADMIN_USERS_BASE, label: "Users" }, { href: back, label: view.name }, { label: "Delete" }], + cancelHref: back, confirmAction: `${back}/delete`, confirmLabel: "Delete user", csrfToken, + current: "users", menu, message: `Delete ${view.email}? This permanently removes the account and can't be undone.`, title: "Delete user", user, + }) }) }; } - if (seg[1] === "delete") { - await kratosAdmin.deleteIdentity(targetId); - return { redirect: ADMIN_USERS_BASE }; - } - if (seg[1] === "recovery") { - const recovery = await kratosAdmin.createRecoveryCode(targetId); - return renderForm({ identity, recovery }); + if (method === "POST") { + if (seg[1] === "state") { + if (isSelf) return { ...(await renderForm({ error: "You can't deactivate your own account.", identity })), status: 400 }; + await kratosAdmin.updateIdentity(targetId, setStatePayload(identity, identity.state === "inactive" ? "active" : "inactive")); + return { redirect: back }; + } + if (seg[1] === "delete") { + if (isSelf) return { ...(await renderForm({ error: "You can't delete your own account.", identity })), status: 400 }; + await kratosAdmin.deleteIdentity(targetId); + return { redirect: ADMIN_USERS_BASE }; + } + if (seg[1] === "recovery") { + const recovery = await kratosAdmin.createRecoveryCode(targetId); + return renderForm({ identity, recovery }); + } } } return null; diff --git a/src/app.test.ts b/src/app.test.ts index dfb9cdf..2f4560b 100644 --- a/src/app.test.ts +++ b/src/app.test.ts @@ -228,7 +228,7 @@ test("session re-mint: an expired JWT backed by a live Kratos session is silentl const nowSec = Math.floor(Date.now() / 1000); const freshJwt = mintJwt({ email: "a@b.c", exp: nowSec + 600, roles: ["demo:read"], sub: "u1" }); const live = withWhoami(async (o) => (o?.tokenizeAs ? { active: true, identity, tokenized: freshJwt } : { active: true, identity }) as Session); - const keto = stubKeto({ listRelations: async () => ({ nextPageToken: null, tuples: [{ namespace: "Role", object: "demo:read", relation: "members", subject_id: "user:u1" }] }) }); + const keto = stubKeto({ check: async () => true, listRelations: async () => ({ nextPageToken: null, tuples: [{ namespace: "Role", object: "demo:read", relation: "members", subject_id: "user:u1" }] }) }); const expired = `${SESSION_COOKIE}=${mintJwt({ email: "a@b.c", exp: nowSec - 600, roles: ["demo:read"], sub: "u1" })}; plainpages_session=s`; // Live Kratos session: the lapsed token is re-minted — the gated route runs AND a fresh cookie rides the response. @@ -408,7 +408,7 @@ test("login completion (/auth/complete): a live session mints the JWT cookie; no let projected: unknown; const kratos = withWhoami(async (o) => (o?.tokenizeAs ? { active: true, identity, tokenized: "h.p.s" } : { active: true, identity }) as Session); const kratosAdmin = stubAdmin({ updateMetadataPublic: async (_id, meta) => { projected = meta; return identity; } }); - const keto = stubKeto({ listRelations: async () => ({ nextPageToken: null, tuples: [{ namespace: "Role", object: "admin", relation: "members", subject_id: `user:${identity.id}` }] }) }); + const keto = stubKeto({ check: async () => true, listRelations: async () => ({ nextPageToken: null, tuples: [{ namespace: "Role", object: "admin", relation: "members", subject_id: `user:${identity.id}` }] }) }); const complete = async (app: ReturnType, cookie?: string) => { await new Promise((r) => app.listen(0, r)); t.after(() => app.close()); @@ -464,7 +464,7 @@ test("logout (CSRF-guarded POST): valid token revokes the Kratos session + clear test("admin Users screen: gate, list/filter, create, edit, deactivate, delete, recovery (CSRF-guarded)", async (t) => { const mk = (email: string, over: Partial = {}): Identity => ({ id: randomUUID(), schema_id: "default", state: "active", traits: { email, name: { first: "Ada", last: "Lovelace" } }, ...over }); - const store: Identity[] = [mk("ada@example.com"), mk("babbage@example.com", { state: "inactive" })]; + const store: Identity[] = [mk("ada@example.com"), mk("babbage@example.com", { state: "inactive" }), mk("you@example.com", { id: "admin1" })]; let lastCreate: { traits?: unknown } | undefined; const kratosAdmin = stubAdmin({ createIdentity: async (payload) => { lastCreate = payload as { traits?: unknown }; const created = mk("grace@example.com"); store.push(created); return created; }, @@ -528,11 +528,20 @@ test("admin Users screen: gate, list/filter, create, edit, deactivate, delete, r assert.equal(rec.status, 200); assert.match(await rec.text(), /self-service\/recovery\?code=123456/); - // Delete: removes the identity, back to the list. + // Delete needs a deliberate confirm step (zero-JS): GET renders the interstitial, POST performs it. + const confirm = await (await get(`/admin/users/${target.id}/delete`)).text(); + assert.match(confirm, /Cancel/); + assert.match(confirm, new RegExp(`action="/admin/users/${target.id}/delete"`)); const del = await post(`/admin/users/${target.id}/delete`, `_csrf=${token}`); assert.equal(del.status, 303); assert.ok(!store.some((x) => x.id === target.id)); + // Self-protection: an admin can't delete or deactivate their own account (JWT sub = admin1). + assert.equal((await post(`/admin/users/admin1/delete`, `_csrf=${token}`)).status, 400); + assert.ok(store.some((x) => x.id === "admin1")); + assert.equal((await post(`/admin/users/admin1/state`, `_csrf=${token}`)).status, 400); + assert.equal(store.find((x) => x.id === "admin1")!.state, "active"); + // Unknown id → 404. assert.equal((await get(`/admin/users/${randomUUID()}`)).status, 404); }); @@ -610,7 +619,8 @@ test("admin Groups screen: gate, list, create, detail/membership, delete (CSRF-g await post("/admin/groups/eng/members/delete", `_csrf=${token}&member=user:${grace}`); assert.ok(!tuples.some((tp) => tp.object === "eng" && tp.subject_id === `user:${grace}`)); - // Delete the group: removes every member tuple, back to the list. + // Delete the group: a confirm step (GET) then the POST removes every member tuple, back to the list. + assert.match(await (await get("/admin/groups/eng/delete")).text(), /Cancel/); const del = await post("/admin/groups/eng/delete", `_csrf=${token}`); assert.equal(del.status, 303); assert.equal(del.headers.get("location"), "/admin/groups"); @@ -706,12 +716,20 @@ test("admin Roles screen: gate, list, create, assign user/group, effective acces await post("/admin/roles/editor/members/delete", `_csrf=${token}&member=group:eng`); assert.ok(!tuples.some((tp) => tp.namespace === "Role" && tp.object === "editor" && tp.subject_set?.object === "eng")); - // Delete the role: removes every member tuple, back to the list. + // Delete the role: a confirm step (GET) then the POST removes every member tuple, back to the list. + assert.match(await (await get("/admin/roles/editor/delete")).text(), /Cancel/); const del = await post("/admin/roles/editor/delete", `_csrf=${token}`); assert.equal(del.status, 303); assert.equal(del.headers.get("location"), "/admin/roles"); assert.ok(!tuples.some((tp) => tp.namespace === "Role" && tp.object === "editor")); + // Self-protection: the admin role can't be deleted, nor can you revoke your own admin (sub admin1). + tuples.push({ namespace: "Role", object: "admin", relation: "members", subject_id: "user:admin1" }); + assert.equal((await post("/admin/roles/admin/delete", `_csrf=${token}`)).status, 400); + assert.ok(tuples.some((tp) => tp.object === "admin")); + assert.equal((await post("/admin/roles/admin/members/delete", `_csrf=${token}&member=user:admin1`)).status, 400); + assert.ok(tuples.some((tp) => tp.object === "admin" && tp.subject_id === "user:admin1")); + // An invalid role name in the path → 404; malformed %-encoding doesn't 500. assert.equal((await get("/admin/roles/Bad%20Name")).status, 404); assert.equal((await get("/admin/roles/%ZZ")).status, 404); diff --git a/src/login.test.ts b/src/login.test.ts index d4e4ef9..0723c76 100644 --- a/src/login.test.ts +++ b/src/login.test.ts @@ -40,18 +40,29 @@ const publicStub = (over: Partial = {}): KratosPublic => ({ ...over, }); -test("readRoles reads direct Role memberships from Keto — paged, de-duped, sorted", async () => { - const calls: unknown[] = []; +test("readRoles returns roles held directly OR transitively (enumerate defined roles → Keto-check each)", async () => { + const listQ: unknown[] = []; + const checked: string[] = []; + const role = (object: string, subject: Partial): RelationTuple => ({ namespace: "Role", object, relation: "members", ...subject }); const keto = ketoStub({ + // Enumerate every Role tuple (paged, no subject filter) to find the distinct role names — + // subjects vary (a direct user, a group) and a name repeats across pages → de-duped. listRelations: async (q) => { - calls.push(q); - if (!q?.pageToken) return { nextPageToken: "p2", tuples: [roleTuple("editor"), roleTuple("admin")] }; - return { nextPageToken: null, tuples: [roleTuple("admin")] }; // duplicate across pages + listQ.push(q); + if (q?.pageToken === "p2") return { nextPageToken: null, tuples: [role("editor", { subject_id: "user:other" })] }; + return { nextPageToken: "p2", tuples: [ + role("editor", { subject_set: { namespace: "Group", object: "eng", relation: "members" } }), + role("admin", { subject_id: `user:${ID}` }), + role("viewer", { subject_id: "user:stranger" }), + ] }; }, + // Keto resolves transitively: the user holds editor (via a group) + admin (direct), not viewer. + check: async (t) => { checked.push(t.object); return t.object === "admin" || t.object === "editor"; }, }); assert.deepEqual(await readRoles(keto, ID), ["admin", "editor"]); - assert.deepEqual(calls[0], { namespace: "Role", relation: "members", subject_id: `user:${ID}` }); - assert.equal((calls[1] as { pageToken?: string }).pageToken, "p2"); // second page follows the cursor + assert.deepEqual(listQ[0], { namespace: "Role", relation: "members" }); // enumerate, not subject-filtered + assert.equal((listQ[1] as { pageToken?: string }).pageToken, "p2"); // second page follows the cursor + assert.deepEqual(checked.sort(), ["admin", "editor", "viewer"]); // every distinct role checked for the user }); test("completeLogin: read roles → project onto metadata_public → tokenize → JWT (in that order)", async () => { @@ -65,7 +76,7 @@ test("completeLogin: read roles → project onto metadata_public → tokenize }, }); const kratosAdmin = adminStub({ updateMetadataPublic: async (_id, meta) => { events.push("project"); projected = meta; return identity; } }); - const keto = ketoStub({ listRelations: async () => ({ nextPageToken: null, tuples: [roleTuple("admin")] }) }); + const keto = ketoStub({ check: async () => true, listRelations: async () => ({ nextPageToken: null, tuples: [roleTuple("admin")] }) }); const out = await completeLogin({ keto, kratosAdmin, kratosPublic }, "plainpages_session=s"); assert.deepEqual(out, { email: "admin@plainpages.local", identityId: ID, jwt: "h.p.s", roles: ["admin"] }); @@ -90,7 +101,7 @@ test("completeLogin maps a missing email trait to null and throws if the tokeniz test("remintSession: a live Kratos session → fresh cookie + refreshed user; a dead session → a clearing cookie + null", async () => { const identity: Identity = { id: ID, traits: { email: "admin@plainpages.local" } }; const kratosPublic = publicStub({ whoami: async (o) => (o?.tokenizeAs ? { active: true, identity, tokenized: "h.p.s" } : { active: true, identity }) as Session }); - const keto = ketoStub({ listRelations: async () => ({ nextPageToken: null, tuples: [roleTuple("admin")] }) }); + const keto = ketoStub({ check: async () => true, listRelations: async () => ({ nextPageToken: null, tuples: [roleTuple("admin")] }) }); // TTL lapsed but the Kratos session lives → re-read roles from Keto, re-tokenize, fresh cookie. const live = await remintSession({ keto, kratosAdmin: adminStub(), kratosPublic }, "plainpages_session=s"); diff --git a/src/login.ts b/src/login.ts index b219ae7..f4ec36a 100644 --- a/src/login.ts +++ b/src/login.ts @@ -36,19 +36,23 @@ export interface CompletedLogin { roles: string[]; } -// The coarse roles Keto grants a subject directly: `Role:#members@user:`. Returns -// the de-duped, sorted role names (the tuple `object`). One logical read, paged defensively. -// Group→role inheritance lands with the Groups screen (§5); MVP grants are direct. +// The coarse roles a user holds — directly (`Role:#members@user:`) or transitively via a +// group that is a member of the role. Enumerates the defined roles (the distinct objects in the Role +// namespace) and asks Keto to resolve each membership, so a role granted to a group reaches the JWT — +// matching the OPL model and the admin "Effective access" view. At login/refresh only, never per +// request; role count is small, so the per-role checks are cheap and run in parallel. export async function readRoles(keto: KetoClient, identityId: string): Promise { const subject_id = `user:${identityId}`; - const roles = new Set(); + const names = new Set(); let pageToken: string | undefined; do { - const page = await keto.listRelations({ namespace: "Role", relation: "members", subject_id, ...(pageToken ? { pageToken } : {}) }); - for (const t of page.tuples) roles.add(t.object); + const page = await keto.listRelations({ namespace: "Role", relation: "members", ...(pageToken ? { pageToken } : {}) }); + for (const t of page.tuples) names.add(t.object); pageToken = page.nextPageToken ?? undefined; } while (pageToken); - return [...roles].sort(); + const roles = [...names]; + const held = await Promise.all(roles.map((object) => keto.check({ namespace: "Role", object, relation: "members", subject_id }))); + return roles.filter((_, i) => held[i]).sort(); } export async function completeLogin(deps: LoginDeps, cookie: string | undefined): Promise { diff --git a/src/shell-context.test.ts b/src/shell-context.test.ts index 072e872..6d55dc4 100644 --- a/src/shell-context.test.ts +++ b/src/shell-context.test.ts @@ -4,8 +4,8 @@ import { buildShellContext, shellUser } from "./shell-context.ts"; test("shellUser derives the profile from the real user; anonymous → Guest", () => { assert.deepEqual(shellUser(null), { email: "", initials: "G", name: "Guest" }); - // Real user: name = email, initials = first two letters of the local part, upper-cased. - assert.deepEqual(shellUser({ email: "ada@example.com", id: "u1", roles: [] }), { email: "", initials: "AD", name: "ada@example.com" }); + // Real user: name = email local part, email kept, initials = first two letters of the local part. + assert.deepEqual(shellUser({ email: "ada@example.com", id: "u1", roles: [] }), { email: "ada@example.com", initials: "AD", name: "ada" }); }); test("buildShellContext maps branding + breadcrumbs, omitting unset optional fields", () => { diff --git a/src/shell-context.ts b/src/shell-context.ts index dcbbb3b..eef021d 100644 --- a/src/shell-context.ts +++ b/src/shell-context.ts @@ -2,7 +2,8 @@ // (the home dashboard, the built-in admin screens) hands to shell.ejs. Pure. Extracted so the // shell user is the *real* signed-in identity (§4) — no hardcoded demo profile — and branding is // read from one place. The User carries no display name (the JWT holds only id/email/roles), so -// the profile shows the email with initials derived from its local part; anonymous ⇒ "Guest". +// the profile shows the email's local part as the name with the full email beneath, initials from +// the local part; anonymous ⇒ "Guest". import type { User } from "./context.ts"; import { type MenuConfig } from "./menu-config.ts"; @@ -25,7 +26,7 @@ export interface ShellModel { export function shellUser(user: User | null | undefined): ShellUser { if (!user) return { email: "", initials: "G", name: "Guest" }; const local = user.email.split("@")[0] || user.email; - return { email: "", initials: (local.slice(0, 2) || "U").toUpperCase(), name: user.email }; + return { email: user.email, initials: (local.slice(0, 2) || "U").toUpperCase(), name: local }; } export function buildShellContext(opts: { diff --git a/todo.md b/todo.md index ea242a1..196d17c 100644 --- a/todo.md +++ b/todo.md @@ -97,7 +97,7 @@ everything via Docker. - [x] Groups: Keto subject sets — list/create/delete + membership management. → `src/admin-groups.ts`: pure view-model + Keto-tuple builders (`groupsFromTuples`, `parseSubject`/`memberTuple`, `memberView`, `isValidGroupName`, `buildGroups{List,Detail,Form}Model`) + `handleAdminGroups` (the imperative shell app.ts dispatches `/admin/groups*` to). A group is a Keto subject set `Group:#members`; a member is a user (`subject_id=user:`) or a nested group (`subject_set=Group:#members`). Keto has no create-object, so a group exists while it has ≥1 member: **create** writes the first-member tuple (requires a member, rejects a duplicate/invalid name), **delete** removes every member tuple (one delete-by-partial-filter), **add/remove member** write/delete one tuple. Routes: `GET /admin/groups` (list — search/sort/paginate over one Keto namespace scan), `GET|POST /admin/groups/new`+`/` (create), `GET /admin/groups/:name` (membership detail — members by email, add a user/nested group, remove, delete-group), `POST …/members` · `…/members/delete` · `…/delete`. Writes go **only to Keto** (README "stateless"); Kratos is read only to label the member pickers by email. Gated **admin-only** (anon→/login, non-admin→403) and every mutation **CSRF-guarded**, same as Users; reuses the §1 building blocks around the shell. Extracted `src/admin-nav.ts` (shared Dashboard·Users·Groups sidebar nav) so the two screens can't drift; added a generic `rowHeader` `` data-table cell (the group name links to its detail). Tests-first: `admin-groups.test.ts` (builder/validation/subject matrix), `app.test.ts` HTTP integration (gate/list/create/dup-reject/detail/add/remove/delete + CSRF + invalid-name & malformed-`%`→404), `data-table.test.ts` (rowHeader). Stability-reviewer (treated as a local PR): APPROVE; fixed its nits — symmetric subject validation (UUID-check the user id), "already exists" feedback on create, malformed-`%`→404 (`safeDecode`). typecheck + 237 units green. Boot-verified the core Keto interactions live (namespace listing, group-collapse counts, delete-group-by-filter, single-member removal). The full-stack groups-CRUD Playwright E2E is §8's scope (line 123), as with the Users screen. Roles/permissions + global-menu wiring are the next §5 items. - [x] Roles & permissions: Keto relations — assign roles to users/groups; "effective access" view via Keto expand. → `src/admin-roles.ts`: a role is a Keto subject set `Role:#members` (OPL: members are users or groups, resolved transitively — the source of truth the §4 login projects into the JWT). Same shape as the Groups screen, so the pure membership helpers are reused from `admin-groups.ts` (`parseSubject`, `isValidGroupName`, `memberView`, `groupsFromTuples`, and now-exported `pagedTuples`/`memberCandidates`/`safeDecode`). Routes (`handleAdminRoles`, dispatched by app.ts): `GET /admin/roles` (list — search/sort/paginate over one Keto scan), `GET|POST /admin/roles/new`+`/` (create = assign first member; rejects invalid/duplicate name), `GET /admin/roles/:name` (detail), `POST …/members` (assign a user/group) · `…/members/delete` (revoke) · `…/delete` (remove all member tuples). The one role-specific piece is **effective access**: `keto.expand(Role:#members, {maxDepth:50})` → `expandToEffectiveUsers` flattens the tree to the distinct users who hold the role directly *or transitively via a group* (the coarse JWT projection stays direct-only per the README's one-read-per-login design; this view is where group→role inheritance is surfaced). Writes go **only to Keto**; Kratos is read only to label members. Gated admin-only (anon→/login, non-admin→403) + CSRF-guarded, like Users/Groups. Added a "Roles" entry (`i-shield`) to the shared `admin-nav.ts`; new `.plain-list` CSS rule. Tests-first: `admin-roles.test.ts` (builders + expand-flatten matrix) + `app.test.ts` HTTP integration (gate/list/create/dup-reject/assign user&group/effective-access-via-expand/revoke/delete + CSRF + malformed-name→404). Stability-reviewer run as a local PR: APPROVE, no Critical/High; addressed its expand-depth nit (explicit `maxDepth`). 237→243 units + typecheck green. **Live boot-verify caught a real bug the tests missed:** Keto v26.2.0's expand nests the subject under `tuple` (`{type:"leaf",tuple:{subject_id}}`), not at the node top-level as the §4 `ExpandTree` type had guessed — fixed the type + walker + the (wrongly-shaped) fixtures, then re-verified live that a user reachable only through a group surfaces in effective access; torn down. Global-menu wiring is the next §5 item. - [x] Wire into the menu (admin section, permission-gated). → Extracted `adminSection(current?)` in `admin-nav.ts` as the single source of truth for the built-in screens' menu links: a permission-gated (`admin`) "Admin" header whose children are Users/Groups/Roles. Wired into the **global** dashboard menu (`dashboard.ts` appends `adminSection()`) so an admin sees the section on `/`; `composeNav`'s `filterByRoles` drops the whole gated header + subtree for a non-admin/anonymous (cosmetic — the routes themselves stay independently `GuardError(403)`-gated). The in-screen `adminNav()` now reuses the same `adminSection(current)` (Dashboard link + the active-marked section) so the two navs can't drift; narrowed `AdminScreen` to `groups|roles|users` (the home link was never `current`). Reuses existing sprite icons (no icon-guard change). Tests-first: `dashboard.test.ts` (admin→section present with the three hrefs; non-admin→absent) + `app.test.ts` HTTP integration (admin JWT→`/admin/users` link rendered, anonymous→absent). Default anonymous `/` render is byte-equivalent (section filtered out) so the visual E2E is unaffected. README Layout line updated. Stability-reviewer run as a local PR: APPROVE, no Critical/High/Medium. 242→244 units + typecheck green. -- [ ] Run the architecture and the product reviewer agents on the _whole_ project, not just the latest changes, and address their issues. +- [x] Run the architecture and the product reviewer agents on the _whole_ project, not just the latest changes, and address their issues. → Ran both on all of `src/`/`views/`/`config/`/docs (weighted to the §5 admin screens). Architecture: **no Critical/High** (functional-core/imperative-shell genuinely honored, security primitives sound). Product: **2 Critical + 1 High**. **Fixed now (tests-first):** (1) Critical (product) — the Roles "Effective access" view showed group→role membership *transitively* but `login.ts` `readRoles` granted only **direct** memberships into the JWT, so a user holding a role *only via a group* was listed as having it yet gated as if not (two screens contradicting). Per the user's call, made `readRoles` transitive: enumerate the defined roles + Keto-`check` each (resolves group membership), so the JWT now matches the Effective-access view + the OPL model — at login/refresh only, never per request (README login section + `admin-roles.ts` header updated). (2) Critical (product) — no confirmation on destructive actions: added a server-rendered (zero-JS) confirm step (`views/admin/confirm.ejs` + `partials/confirm-body.ejs`, shared `buildConfirmModel`) — `GET /admin/{users,groups,roles}/:id/delete` renders an interstitial (Cancel + the real POST); each detail/edit Delete control is now a link to it. (3) High (product) — self-lockout: an admin can no longer delete or deactivate **their own** account, revoke **their own** (direct) admin grant, or delete the **admin role** outright (each → 400 + inline error). Covers the direct-grant paths (incl. the bootstrap-seeded admin, which holds a direct grant); admin held *only* via a group can still be self-revoked, so the robust "last effective admin won't drop" check is deferred to **§9** (stability-reviewer Medium). (4) MEDIUM (arch M1 pt.1) — extracted the gate+CSRF preamble copied verbatim across the 3 admin handlers into `admin-nav.ts` `requireAdmin`/`guardedForm` (one security-critical copy, can't drift). (5) MEDIUM (arch M4) — `shellUser` no longer blanks the email: name = email local part, full email beneath (matches `toUserView`). Tests-first throughout (extended the 3 admin HTTP tests + login/shell-context units); typecheck + 244 units + 8 visual E2E + the full-stack auth-refresh E2E green (the latter re-verifies live login→transitive `readRoles`→`roles:["admin"]`). **Deferred (reviewer-scoped, not the §5 checkpoint):** the host internal route-table (fold the admin if-ladder + Hydra into `matchRoute`/`isAuthorized`, arch M1 pt.2) → **§6** (the 2nd/3rd Hydra screen is the forcing function); admin list-model/template near-duplication across Users/Groups/Roles (arch M3) → the §5 comment/test-cleanup items below (lines 101–102); success-flash after writes + welcoming empty-list states + warn-on-dangling-group-references + >250-row truncation notice (product Medium) → §5 polish / §8 E2E; `safeUrl()` href helper (arch L1 — the recovery link is server-built, not exploitable today) → **§7** (first untrusted-URL flow); oversized-body→500 should be 413 (arch M2) + prod Ory-URL `https` enforcement (arch L3) + `§N`-in-comments / README Layout drift (arch L4) → **§9** (ops/security). - [ ] Go over all comments in the code and the README and try to make it shorter and more information dense. Remove not strictly needed stuff. - [ ] Go over all tests and combine/unify ones that cover the same stuff or are very related and could be combined in a good way. Remove tests that aren't helping, we only want tests that are actually helpful to us. diff --git a/views/admin/confirm.ejs b/views/admin/confirm.ejs new file mode 100644 index 0000000..bf582be --- /dev/null +++ b/views/admin/confirm.ejs @@ -0,0 +1,17 @@ +<%# + Admin destructive-action confirmation page (todo §5): the confirm body in the app shell. Model + from buildConfirmModel: { message, confirm:{action,label}, cancelHref, nav, shell }. +%><% + const nav = include("partials/nav-tree", { nodes: model.nav }); + const body = include("partials/confirm-body", { cancelHref: model.cancelHref, confirm: model.confirm, csrfToken: model.shell.csrfToken, message: model.message }); +-%> +<%- include("partials/shell", { + body, + brand: model.shell.brand, + breadcrumbs: model.shell.breadcrumbs, + csrfToken: model.shell.csrfToken, + nav, + theme: model.shell.theme, + title: model.shell.title, + user: model.shell.user, +}) %> diff --git a/views/partials/confirm-body.ejs b/views/partials/confirm-body.ejs new file mode 100644 index 0000000..00d2999 --- /dev/null +++ b/views/partials/confirm-body.ejs @@ -0,0 +1,17 @@ +<%# + Destructive-action confirm body (todo §5), captured into the shell content slot. Zero-JS: the + delete is a deliberate second step (a POST form), with a cancel link back. Config: + message string + confirm { action, label } the danger POST endpoint + button label + cancelHref string + csrfToken +%> +
+
+

<%= locals.message %>

+
+ Cancel +
+
+
+
diff --git a/views/partials/group-detail-body.ejs b/views/partials/group-detail-body.ejs index 0196cb9..cca7486 100644 --- a/views/partials/group-detail-body.ejs +++ b/views/partials/group-detail-body.ejs @@ -37,6 +37,6 @@ <% } -%>
-
+ Delete group
diff --git a/views/partials/role-detail-body.ejs b/views/partials/role-detail-body.ejs index 66c6469..5a88e30 100644 --- a/views/partials/role-detail-body.ejs +++ b/views/partials/role-detail-body.ejs @@ -52,6 +52,6 @@ <% } -%>
-
+ Delete role
diff --git a/views/partials/user-form-body.ejs b/views/partials/user-form-body.ejs index 0205428..6706d87 100644 --- a/views/partials/user-form-body.ejs +++ b/views/partials/user-form-body.ejs @@ -30,7 +30,7 @@
-
+ Delete user
<% } -%>