From f4969d5c1fc0707720372f354b04d6518eafa88f Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Sat, 31 Oct 2026 09:34:05 +0100 Subject: [PATCH 1/8] Refuse a stale permission save, confirm or report it, show View to read-only holders, keep an absolute return_to beside a locale --- CHANGELOG.md | 25 +++++ docs/reading-cost.md | 17 +++ e2e-tests/full-flow.spec.ts | 11 ++ examples/plugins/admin/admin-grants.test.ts | 37 ++++++- examples/plugins/admin/admin-grants.ts | 37 ++++++- examples/plugins/admin/admin-groups.ts | 52 +++++---- examples/plugins/admin/admin-users.test.ts | 8 ++ examples/plugins/admin/admin-users.ts | 38 ++++--- examples/plugins/admin/i18n/en-US.ts | 3 + examples/plugins/admin/i18n/sv-SE.ts | 3 + .../views/partials/permission-picker.ejs | 9 +- src/auth/routes.ts | 3 +- src/http/app.test.ts | 102 ++++++++++++++++-- src/i18n/locales/en-US.ts | 1 + src/i18n/locales/sv-SE.ts | 1 + src/ui/icons.ts | 1 + views/partials/icons.ejs | 1 + 17 files changed, 303 insertions(+), 46 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8dcc752..e22368c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,31 @@ at boot. Entries start at 0.3.0. ## Unreleased +### The admin example refuses a permission save made from an out-of-date page + +Two administrators editing one user's or group's permissions no longer overwrite each other. The +form carries a token for the grants it showed, and a save made after they changed is refused with +"These permissions changed while you had the page open", re-rendering the current state. A POST with +no token is refused the same way, so a script posting to `/admin/users/:id/permissions` or +`/admin/groups/:name/permissions` must first read the token from the page's `baseline` field. + +### The admin example confirms a permission save and reports a partial one + +A save now lands on the page with "Permissions saved." If the permission service fails midway, the +page says the save stopped part-way and shows what is stored, instead of the generic error page; the +changes applied before the failure still sign the user's live sessions out. + +### A read-only holder sees View, not Edit, on the users list + +A holder of `users:read` without `users:write` gets a "View" row action with an eye icon. The new +`common.view` catalog key is in `en-US` and `sv-SE`; a mounted core catalog must add it. + +### A language chosen on a sign-in link no longer drops an absolute return target + +`/login?return_to=&locale=sv-SE` passed the visitor to the dashboard after sign-in and +dropped the OAuth2 login challenge in `return_to`. The target now reaches Kratos, and the chosen +language is lost across that round-trip instead. + ### Every page carries a smaller icon sprite `i-chart`, `i-copy` and `i-download` are gone from the inlined icon sprite, so a plugin that diff --git a/docs/reading-cost.md b/docs/reading-cost.md index 18c02a0..821295e 100644 --- a/docs/reading-cost.md +++ b/docs/reading-cost.md @@ -122,3 +122,20 @@ Read most (opens and searches): - `public/css/styles.css` 1 - `renovate.json` 1 - `src/e2e-console-guard.test.ts` 1 + +## Reading cost, chunk 2f36d29bd746, finished 2026-10-31T08:19:09Z + +85029 new input tokens before the first commit. + +Read most (opens and searches): + +- `e2e-tests/full-flow.spec.ts` 4 +- `README.md` 3 +- `src/auth/routes.test.ts` 3 +- `todo.md` 3 +- `src/auth/kratos-public.ts` 2 +- `src/http/app.test.ts` 2 +- `ci.sh` 1 +- `compose.yml` 1 +- `e2e-tests/auth-refresh.spec.ts` 1 +- `src/auth/flow-view.ts` 1 diff --git a/e2e-tests/full-flow.spec.ts b/e2e-tests/full-flow.spec.ts index 8f90403..7e6996d 100644 --- a/e2e-tests/full-flow.spec.ts +++ b/e2e-tests/full-flow.spec.ts @@ -150,6 +150,8 @@ test.describe.serial("authenticated admin journey", () => { // Permissions are declared in plugin code, so the group's detail page offers them as a fixed // checkbox list rather than a create form — there is no Permissions screen to visit. await page.goto(`/admin/groups/${group}`); + const staleTab = await watchedPage(page.context()); // opened now, so it holds this page's grants + await staleTab.goto(`/admin/groups/${group}`); const scheduling = page.locator('input[name="permission"][value="scheduling:read"]'); await expect(scheduling).toHaveCount(1); // declared by the reference plugin, so it's on offer await expect(scheduling).not.toBeChecked(); @@ -157,7 +159,16 @@ test.describe.serial("authenticated admin journey", () => { await page.locator('form:has(input[name="permission"]) button[type="submit"]').click(); await expect(page).toHaveURL(new RegExp(`/admin/groups/${group}`)); + await expect(page.getByText("Permissions saved.")).toBeVisible(); await expect(page.locator('input[name="permission"][value="scheduling:read"]')).toBeChecked(); + + // The other tab no longer matches Keto, so its save is refused and shows the grant made above. + await staleTab.locator('input[name="permission"][value="scheduling:write"]').check(); + await staleTab.locator('form:has(input[name="permission"]) button[type="submit"]').click(); + await expect(staleTab.getByText("changed while you had the page open")).toBeVisible(); + await expect(staleTab.locator('input[name="permission"][value="scheduling:read"]')).toBeChecked(); + await expect(staleTab.locator('input[name="permission"][value="scheduling:write"]')).not.toBeChecked(); + await staleTab.close(); }); test("OAuth2 clients CRUD: register a client (writes go to Hydra), see the one-time secret once, then delete it via the confirm step", async () => { diff --git a/examples/plugins/admin/admin-grants.test.ts b/examples/plugins/admin/admin-grants.test.ts index ce01cdc..32899a1 100644 --- a/examples/plugins/admin/admin-grants.test.ts +++ b/examples/plugins/admin/admin-grants.test.ts @@ -3,7 +3,8 @@ import assert from "node:assert/strict"; import { test } from "node:test"; import type { PermissionDecl } from "@plainpages/plugin-api"; -import { buildPermissionPicker, grantDiff, grantTuple, groupSubject, userSubject } from "./admin-grants.ts"; +import type { KetoClient } from "@plainpages/plugin-api"; +import { applyGrants, baselineToken, buildPermissionPicker, grantDiff, grantTuple, groupSubject, userSubject } from "./admin-grants.ts"; const declared: PermissionDecl[] = [ { description: "View users", name: "users:read" }, @@ -81,3 +82,37 @@ test("buildPermissionPicker says so when no plugin declares a permission, rather assert.deepEqual(picker.choices, []); assert.ok(picker.empty); }); + +// The concurrency token of the permission form: equal exactly when the direct grants it offered are. +test("baselineToken depends on the offered grants held, not on their order or on undeclared ones", () => { + const token = baselineToken(declared, ["users:read", "users:write"]); + assert.match(token, /^[0-9a-f]+$/); + assert.equal(baselineToken(declared, ["users:write", "users:read"]), token); + assert.equal(baselineToken(declared, ["users:read", "users:write", "legacy:thing"]), token); // undeclared: never shown, never saved + assert.notEqual(baselineToken(declared, ["users:read"]), token); + assert.notEqual(baselineToken(declared, []), token); + assert.notEqual(baselineToken(declared, []), ""); // an empty set is a state too, not an absent token + assert.equal(buildPermissionPicker({ action: "/x", declared, direct: ["users:read", "users:write"] }).baseline, token); +}); + +test("applyGrants applies writes then deletes, and on a Keto failure stops and reports what was applied", async () => { + const log: string[] = []; + const keto = (failOn?: string): KetoClient => ({ + check: async () => false, + deleteTuple: async (tp) => { if (tp.object === failOn) throw new Error("boom"); log.push(`delete ${tp.object}`); }, + expand: async () => ({ type: "leaf" }), + listRelations: async () => ({ nextPageToken: null, tuples: [] }), + writeTuple: async (tp) => { if (tp.object === failOn) throw new Error("boom"); log.push(`write ${tp.object}`); }, + }); + const diff = { grant: ["a:read", "b:read"], revoke: ["c:read"] }; + + const ok = await applyGrants(keto(), userSubject("u1"), diff); + assert.deepEqual(ok, { applied: diff }); + assert.deepEqual(log, ["write a:read", "write b:read", "delete c:read"]); + + log.length = 0; + const failed = await applyGrants(keto("b:read"), userSubject("u1"), diff); + assert.deepEqual(failed.applied, { grant: ["a:read"], revoke: [] }); + assert.ok(failed.error instanceof Error); + assert.deepEqual(log, ["write a:read"]); // nothing after the failure was attempted +}); diff --git a/examples/plugins/admin/admin-grants.ts b/examples/plugins/admin/admin-grants.ts index ab7219e..232decc 100644 --- a/examples/plugins/admin/admin-grants.ts +++ b/examples/plugins/admin/admin-grants.ts @@ -6,11 +6,14 @@ // what the installed plugins declare in code. Nothing here invents a name, which is why the old // Permissions screen is gone: a grant is a property of a user or a group, edited where they are. +import { createHash } from "node:crypto"; import type { KetoClient, PermissionDecl, RelationTuple, SubjectSet, Translate } from "@plainpages/plugin-api"; const PERMISSION_NS = "Permission"; const GRANTED = "granted"; export const PERMISSIONS_FIELD = "permission"; // the checkbox name the two forms post +export const BASELINE_FIELD = "baseline"; // the hidden field carrying baselineToken +export const SAVED_NOTICE = "permissions-saved"; // the `info-msg` a successful save redirects with export type GrantSubject = { subject_id: string } | { subject_set: SubjectSet }; @@ -53,6 +56,7 @@ export interface PermissionChoice { export interface PermissionPicker { action: string; + baseline: string; // baselineToken of the direct grants this form was rendered from choices: PermissionChoice[]; empty: string | undefined; // set when no plugin declares a permission — the picker has nothing to offer error?: string; // a rejected save (e.g. the self-revoke guard), rendered above the list @@ -60,6 +64,7 @@ export interface PermissionPicker { hint: string; inheritedNote: string | undefined; // set when at least one choice is group-held, to explain the disabled row legend: string; + notice?: string; // a confirmation, rendered above the list // 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; @@ -90,6 +95,7 @@ export function buildPermissionPicker(opts: { })); return { action: opts.action, + baseline: baselineToken(opts.declared, opts.direct), choices, empty: opts.declared.length === 0 ? t("admin.grants.none") : undefined, field: PERMISSIONS_FIELD, @@ -107,7 +113,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: readonly PermissionDecl[], held: string[], wanted: string[]): { grant: string[]; revoke: string[] } { +export function grantDiff(declared: readonly PermissionDecl[], held: string[], wanted: string[]): GrantChange { const offered = new Set(declared.map((d) => d.name)); const heldSet = new Set(held); const wantedSet = new Set(wanted.filter((name) => offered.has(name))); @@ -117,7 +123,30 @@ export function grantDiff(declared: readonly PermissionDecl[], held: string[], w }; } -export async function applyGrants(keto: KetoClient, subject: GrantSubject, diff: { grant: string[]; revoke: string[] }): Promise { - for (const name of diff.grant) await keto.writeTuple(grantTuple(name, subject)); - for (const name of diff.revoke) await keto.deleteTuple(grantTuple(name, subject)); +// Proves which direct grants a form was rendered from, so a save made after someone else changed +// them is refused rather than silently undoing that change. Covers only the declared names the form +// offered, like grantDiff, and is a digest so an empty set is still a token and a missing field is not. +export function baselineToken(declared: readonly PermissionDecl[], held: string[]): string { + const offered = new Set(declared.map((d) => d.name)); + return createHash("sha256").update(held.filter((name) => offered.has(name)).sort().join("\n")).digest("hex").slice(0, 16); +} + +// True when the submitted token is missing or no longer matches Keto. +export const isStale = (form: { get(name: string): string | null }, declared: readonly PermissionDecl[], held: string[]): boolean => + form.get(BASELINE_FIELD) !== baselineToken(declared, held); + +export interface GrantChange { grant: string[]; revoke: string[] } + +// Keto has no transaction, so the writes and deletes run one by one and stop at the first failure. +// What was applied by then is returned beside the error, so the caller can still revoke live tokens +// and tell the admin the save is only partly done. +export async function applyGrants(keto: KetoClient, subject: GrantSubject, diff: GrantChange): Promise<{ applied: GrantChange; error?: unknown }> { + const applied: GrantChange = { grant: [], revoke: [] }; + try { + for (const name of diff.grant) { await keto.writeTuple(grantTuple(name, subject)); applied.grant.push(name); } + for (const name of diff.revoke) { await keto.deleteTuple(grantTuple(name, subject)); applied.revoke.push(name); } + } catch (error) { + return { applied, error }; + } + return { applied }; } diff --git a/examples/plugins/admin/admin-groups.ts b/examples/plugins/admin/admin-groups.ts index 67d89f0..230e8b3 100644 --- a/examples/plugins/admin/admin-groups.ts +++ b/examples/plugins/admin/admin-groups.ts @@ -7,7 +7,7 @@ // each returning a RouteResult. 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 "@plainpages/plugin-api"; -import { applyGrants, buildPermissionPicker, effectivePermissions, grantDiff, grantTuple, groupSubject, heldPermissions, type PermissionPicker, PERMISSIONS_FIELD } from "./admin-grants.ts"; +import { applyGrants, buildPermissionPicker, effectivePermissions, grantDiff, grantTuple, groupSubject, heldPermissions, isStale, type PermissionPicker, PERMISSIONS_FIELD, SAVED_NOTICE } from "./admin-grants.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"; @@ -347,36 +347,50 @@ export const groupsCreate = withGroups(async (deps) => { // GET /admin/groups/new — the create form. 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) => { +// The detail + membership page. `notice` and `error` ride the permission picker, which is where a save reports. +async function groupDetailPage({ ctx, keto, kratosAdmin }: GroupsDeps, name: string, say: { error?: string; notice?: string } = {}): Promise { const { emailById, options } = await memberCandidates(keto, kratosAdmin); const members = (await pagedTuples(keto, { namespace: GROUP_NS, object: name, relation: MEMBERS })).map((t) => memberView(t, emailById)); const subject = groupSubject(name); const [direct, effective] = await Promise.all([heldPermissions(keto, subject), effectivePermissions(keto, subject, ctx.declaredPermissions)]); - const permissions = buildPermissionPicker({ - action: `${detailHref(name)}/permissions`, - declared: ctx.declaredPermissions, - direct, - 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 - }); + const permissions: PermissionPicker = { + ...buildPermissionPicker({ + action: `${detailHref(name)}/permissions`, + declared: ctx.declaredPermissions, + direct, + 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 + }), + ...(say.error ? { error: say.error } : {}), + ...(say.notice ? { notice: say.notice } : {}), + }; 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" }; -}); +} + +// GET /admin/groups/:name — the detail + membership page; `info-msg` is read as on the user page. +export const groupsDetail = withGroupName((deps, name) => + groupDetailPage(deps, name, deps.ctx.url.searchParams.get("info-msg") === SAVED_NOTICE ? { notice: deps.ctx.t("admin.grants.saved") } : {})); // POST /admin/groups/:name/permissions — the submitted checkboxes are the desired set. Members hold // a group's permissions transitively, so the change reaches them at their next login or re-mint — // the documented instant-revoke tradeoff for anything held through a group. -export const groupsPermissions = withGroupName(async ({ ctx, keto, user }, name) => { +export const groupsPermissions = withGroupName(async (deps, name) => { + const { ctx, keto, user } = deps; const form = (await guardedForm(ctx))!; const subject = groupSubject(name); - const diff = grantDiff(ctx.declaredPermissions, await heldPermissions(keto, subject), form.getAll(PERMISSIONS_FIELD)); - await applyGrants(keto, subject, diff); - if (diff.grant.length > 0 || diff.revoke.length > 0) { - ctx.log.info("admin: group permissions changed", { actor: user.id, granted: diff.grant.join(","), group: name, revoked: diff.revoke.join(",") }); + const held = await heldPermissions(keto, subject); + if (isStale(form, ctx.declaredPermissions, held)) return { ...(await groupDetailPage(deps, name, { error: ctx.t("admin.grants.stale") })), status: 409 }; + const { applied, error } = await applyGrants(keto, subject, grantDiff(ctx.declaredPermissions, held, form.getAll(PERMISSIONS_FIELD))); + if (applied.grant.length > 0 || applied.revoke.length > 0) { + ctx.log.info("admin: group permissions changed", { actor: user.id, granted: applied.grant.join(","), group: name, revoked: applied.revoke.join(",") }); } - return { redirect: detailHref(name) }; + if (error) { + ctx.log.error("admin: group permissions only partly saved", { actor: user.id, error: String(error), group: name }); + return { ...(await groupDetailPage(deps, name, { error: ctx.t("admin.grants.partial") })), status: 502 }; + } + return { redirect: `${detailHref(name)}?info-msg=${SAVED_NOTICE}` }; }); // POST /admin/groups/:name/members — add a member (skip an invalid member or a self-nest). diff --git a/examples/plugins/admin/admin-users.test.ts b/examples/plugins/admin/admin-users.test.ts index bafce03..cdfdd63 100644 --- a/examples/plugins/admin/admin-users.test.ts +++ b/examples/plugins/admin/admin-users.test.ts @@ -60,6 +60,14 @@ test("buildUsersListModel filters by search + status, sorts, and paginates", () // Edit link points at the per-identity route. assert.match(all.table.rows[0]!.actions![0]!.href!, new RegExp(`/admin/users/${people[0]!.id}$`)); + assert.deepEqual([all.table.rows[0]!.actions![0]!.icon, all.table.rows[0]!.actions![0]!.label], ["i-edit", "Edit"]); +}); + +// The detail page of a `users:read` holder is read-only, so the row action must not promise an edit. +test("buildUsersListModel: a read-only holder's row action says View, with the same target", () => { + const people: Identity[] = [{ id: "u1", traits: { email: "a@x.io" } }]; + const action = buildUsersListModel({ canWrite: false, identities: people, url: "http://x/admin/users" }).table.rows[0]!.actions![0]!; + assert.deepEqual([action.icon, action.label, action.href], ["i-eye", "View", "/admin/users/u1"]); }); test("buildUserFormModel: create mode has an editable email + password, no edit actions", () => { diff --git a/examples/plugins/admin/admin-users.ts b/examples/plugins/admin/admin-users.ts index 00e3d73..e60c2d6 100644 --- a/examples/plugins/admin/admin-users.ts +++ b/examples/plugins/admin/admin-users.ts @@ -4,7 +4,7 @@ // a shared `withUser` gate. import { can, type Identity, type KetoClient, type KratosAdmin, KratosError, paginate, parseListQuery, type RecoveryCode, type RequestContext, type RouteHandler, type RouteResult, type Translate, type User } from "@plainpages/plugin-api"; -import { applyGrants, buildPermissionPicker, effectivePermissions, grantDiff, heldPermissions, type PermissionPicker, PERMISSIONS_FIELD, userSubject } from "./admin-grants.ts"; +import { applyGrants, buildPermissionPicker, effectivePermissions, grantDiff, heldPermissions, isStale, type PermissionPicker, PERMISSIONS_FIELD, SAVED_NOTICE, userSubject } from "./admin-grants.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 @@ -138,12 +138,12 @@ export function buildUsersListModel(opts: { canWrite: opts.canWrite !== false, filterBar: listFilterBar(state, all.length, t), pagination: listPagination(state, page, t), - table: listTable(rows, state, sort, t), + table: listTable(rows, state, sort, t, opts.canWrite !== false), title: t("admin.users.title"), }; } -function listTable(rows: UserView[], state: ListState, sort: { dir: "asc" | "desc"; field: string } | null, t: Translate) { +function listTable(rows: UserView[], state: ListState, sort: { dir: "asc" | "desc"; field: string } | null, t: Translate, canWrite: boolean) { return { actions: true, caption: t("admin.users.title"), @@ -153,7 +153,7 @@ function listTable(rows: UserView[], state: ListState, sort: { dir: "asc" | "des return { href: listHref(state, { page: 1, sort: next }), label: t(c.label), sort: dir, sortable: true }; }), rows: rows.map((u) => ({ - actions: [{ href: `${ADMIN_USERS_BASE}/${encodeURIComponent(u.id)}`, icon: "i-edit", label: t("common.edit") }], + actions: [{ href: `${ADMIN_USERS_BASE}/${encodeURIComponent(u.id)}`, icon: canWrite ? "i-edit" : "i-eye", label: t(canWrite ? "common.edit" : "common.view") }], cells: [ { user: { initials: u.initials, name: u.name } }, u.email, @@ -329,7 +329,9 @@ export const usersNewForm = withUser(({ ctx }) => Promise.resolve(formResult(ctx // GET /admin/users/:id — the edit form, prefilled. export const usersEditForm = withTarget(async (deps, identity, id) => { - const permissions = await userPermissionPicker(deps, id); + // `info-msg` is honored only as the exact value a save redirects with, so a crafted link can't put words on the page. + const saved = deps.ctx.url.searchParams.get("info-msg") === SAVED_NOTICE; + const permissions = await userPermissionPicker(deps, id, undefined, saved ? deps.ctx.t("admin.grants.saved") : undefined); return formResult(deps.ctx, { canWrite: canWriteUsers(deps.ctx), identity, ...(permissions ? { permissions } : {}) }); }); @@ -338,7 +340,7 @@ const canWriteUsers = (ctx: RequestContext): boolean => can(ctx, permissionName( // The checkbox list of declared permissions: ticked where this user holds one, and disabled where // the grant comes from a group (real, but removed on that group). Undefined when Keto isn't wired — // the rest of the edit page still works. -async function userPermissionPicker(deps: UsersDeps, id: string, error?: string): Promise { +async function userPermissionPicker(deps: UsersDeps, id: string, error?: string, notice?: string): Promise { if (!deps.keto) return undefined; const subject = userSubject(id); const [direct, effective] = await Promise.all([ @@ -355,6 +357,7 @@ async function userPermissionPicker(deps: UsersDeps, id: string, error?: string) t: deps.ctx.t, }), ...(error ? { error } : {}), + ...(notice ? { notice } : {}), }; } @@ -366,21 +369,30 @@ export const usersPermissions = withTarget(async (deps, identity, id) => { const form = (await guardedForm(ctx))!; if (!keto) return unavailable(ctx, ctx.t("admin.capability.keto")); const subject = userSubject(id); - const diff = grantDiff(ctx.declaredPermissions, await heldPermissions(keto, subject), form.getAll(PERMISSIONS_FIELD)); + const held = await heldPermissions(keto, subject); + const refuse = async (error: string, status: number): Promise => { + const permissions = await userPermissionPicker(deps, id, error); + return { ...formResult(ctx, { canWrite: canWriteUsers(ctx), identity, ...(permissions ? { permissions } : {}) }), status }; + }; + if (isStale(form, ctx.declaredPermissions, held)) return refuse(ctx.t("admin.grants.stale"), 409); + const diff = grantDiff(ctx.declaredPermissions, held, form.getAll(PERMISSIONS_FIELD)); // Self-lockout guard, matching the self-deactivate/self-delete ones: revoking your own grants can // remove the last `users:write` on the deployment, and the instant-revoke hook lands it on the very // next request — leaving a `curl` against Keto as the only way back in. 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 }; + return refuse(ctx.t("admin.grants.selfRevoke"), 400); } - await applyGrants(keto, subject, diff); - if (diff.grant.length > 0 || diff.revoke.length > 0) { + const { applied, error } = await applyGrants(keto, subject, diff); + if (applied.grant.length > 0 || applied.revoke.length > 0) { revoke?.(id); - ctx.log.info("admin: user permissions changed", { actor: user.id, granted: diff.grant.join(","), revoked: diff.revoke.join(","), target: id }); + ctx.log.info("admin: user permissions changed", { actor: user.id, granted: applied.grant.join(","), revoked: applied.revoke.join(","), target: id }); } - return { redirect: `${ADMIN_USERS_BASE}/${encodeURIComponent(id)}` }; + if (error) { + ctx.log.error("admin: user permissions only partly saved", { actor: user.id, error: String(error), target: id }); + return refuse(ctx.t("admin.grants.partial"), 502); + } + return { redirect: `${ADMIN_USERS_BASE}/${encodeURIComponent(id)}?info-msg=${SAVED_NOTICE}` }; }); // POST /admin/users/:id — save edits; a Kratos 4xx re-renders the form (400). diff --git a/examples/plugins/admin/i18n/en-US.ts b/examples/plugins/admin/i18n/en-US.ts index c885ce6..52473cb 100644 --- a/examples/plugins/admin/i18n/en-US.ts +++ b/examples/plugins/admin/i18n/en-US.ts @@ -53,9 +53,12 @@ const messages = { "admin.grants.inherited": "Greyed-out permissions come from a group. Change them on that group.", "admin.grants.legend": "Permissions", "admin.grants.none": "No installed plugin declares a permission, so there is nothing to grant.", + "admin.grants.partial": "Saving stopped part-way because the permission service failed, so only some changes were applied. The list shows what is stored now. Save again to finish.", "admin.grants.pending": "Members get this at their next sign-in (up to 10 minutes).", "admin.grants.save": "Save permissions", + "admin.grants.saved": "Permissions saved.", "admin.grants.selfRevoke": "You can't revoke your own permissions — ask another administrator, so you can't lock yourself out.", + "admin.grants.stale": "These permissions changed while you had the page open, so nothing was saved. The list now shows the current state. Review it and save again.", "admin.groups.actions": "Group actions", "admin.groups.addMember": "Add a member", diff --git a/examples/plugins/admin/i18n/sv-SE.ts b/examples/plugins/admin/i18n/sv-SE.ts index 03b2348..a170713 100644 --- a/examples/plugins/admin/i18n/sv-SE.ts +++ b/examples/plugins/admin/i18n/sv-SE.ts @@ -53,9 +53,12 @@ const messages: AdminMessages = { "admin.grants.inherited": "Gråmarkerade behörigheter kommer från en grupp. Ändra dem på gruppen.", "admin.grants.legend": "Behörigheter", "admin.grants.none": "Ingen installerad plugin deklarerar någon behörighet, så det finns inget att tilldela.", + "admin.grants.partial": "Sparandet avbröts halvvägs eftersom behörighetstjänsten fallerade, så bara en del av ändringarna genomfördes. Listan visar vad som är sparat nu. Spara igen för att slutföra.", "admin.grants.pending": "Medlemmar får detta vid nästa inloggning (upp till 10 minuter).", "admin.grants.save": "Spara behörigheter", + "admin.grants.saved": "Behörigheterna är sparade.", "admin.grants.selfRevoke": "Du kan inte återkalla dina egna behörigheter — be en annan administratör, så att du inte låser ute dig själv.", + "admin.grants.stale": "Behörigheterna ändrades medan du hade sidan öppen, så ingenting sparades. Listan visar nu aktuellt läge. Granska den och spara igen.", "admin.groups.actions": "Gruppåtgärder", "admin.groups.addMember": "Lägg till en medlem", diff --git a/examples/plugins/admin/views/partials/permission-picker.ejs b/examples/plugins/admin/views/partials/permission-picker.ejs index 12796b8..9414aaf 100644 --- a/examples/plugins/admin/views/partials/permission-picker.ejs +++ b/examples/plugins/admin/views/partials/permission-picker.ejs @@ -7,13 +7,19 @@ there) and every row when `readOnly` (the viewer holds :read but not :write). Neither can be diffed into an accidental revoke, because grantDiff compares against the direct grants only. - Locals: csrfToken, permissions ({ action, choices, empty, error, field, hint, inheritedNote, legend, pending, readOnly, submit }). + The hidden `baseline` is a token for the direct grants this page showed: a save made after they changed + is refused (see baselineToken in admin-grants.ts). + + Locals: csrfToken, permissions ({ action, baseline, choices, empty, error, field, hint, inheritedNote, legend, notice, pending, readOnly, submit }). %>

<%= permissions.legend %>

<% if (permissions.error) { -%> <%- include("partials/alert", { text: permissions.error, tone: "neg" }) %> <% } -%> +<% if (permissions.notice) { -%> +<%- include("partials/alert", { text: permissions.notice, tone: "pos" }) %> +<% } -%> <% if (permissions.empty) { -%>

<%= permissions.empty %>

<% } else { -%> @@ -28,6 +34,7 @@ <% } else { -%>
+
<%= permissions.legend %> <% permissions.choices.forEach((c) => { -%> diff --git a/src/auth/routes.ts b/src/auth/routes.ts index 3354b81..ef764f4 100644 --- a/src/auth/routes.ts +++ b/src/auth/routes.ts @@ -46,7 +46,8 @@ function flowReturnTo(ctx: RequestContext, secureCookies: boolean): string | und const raw = ctx.url.searchParams.get("return_to"); const local = localPath(raw); const chosen = chosenLocale(ctx); - if (!local && !chosen) return raw ?? undefined; + if (raw && !local) return raw; // beside a chosen language too: the target wins, the language is lost + if (!local && !chosen) return undefined; const complete = new URL(`${selfOrigin(ctx, secureCookies)}/auth/complete`); if (local) complete.searchParams.set("return_to", ctx.localeHref(local)); if (chosen) complete.searchParams.set("locale", chosen); diff --git a/src/http/app.test.ts b/src/http/app.test.ts index a3cbb59..13e5af8 100644 --- a/src/http/app.test.ts +++ b/src/http/app.test.ts @@ -855,6 +855,11 @@ test("login return_to: a chosen language alone still routes through /auth/comple assert.match(lastReturnTo ?? "", /^http:\/\/[^/]+\/auth\/complete\?locale=sv-SE$/); await fetch(url + "/login?locale=sv-SE&return_to=" + encodeURIComponent("/admin/users"), { redirect: "manual" }); assert.match(lastReturnTo ?? "", /^http:\/\/[^/]+\/auth\/complete\?return_to=%2Fadmin%2Fusers%3Flocale%3Dsv-SE&locale=sv-SE$/); + + // An absolute target still reaches Kratos beside a chosen language; the language is lost instead. + const abs = "http://localhost/oauth2/login?login_challenge=abc"; + await fetch(url + "/login?locale=sv-SE&return_to=" + encodeURIComponent(abs), { redirect: "manual" }); + assert.equal(lastReturnTo, abs); }); // "Ory down ⇒ no logins" is documented; the auth path should say so honestly (503), not the @@ -953,6 +958,8 @@ async function adminHarness(t: TestContext, opts: AppOptions = {}) { // What the plugin itself declares — the harness holds every screen's read and write, so a screen // test exercises the screen rather than the gate. assertAdminGate covers the refusals. const ADMIN_ALL = (adminManifest.permissions ?? []).map((p) => p.name); +// The permission form's concurrency token, read off the page the way a browser would post it back. +const baselineOf = (html: string): string => /name="baseline" value="([0-9a-f]+)"/.exec(html)?.[1] ?? ""; // Every admin route is gated: anonymous → /login, a signed-in non-admin → 403. async function assertAdminGate(url: string, get: (path: string, permissions?: string[]) => Promise, path: string) { const anon = await fetch(url + path, { redirect: "manual" }); @@ -1313,7 +1320,8 @@ test("admin Groups screen: gate, list, create, detail/membership, delete (CSRF-g assert.ok(!tuples.some((tp) => tp.object === "eng" && tp.subject_id === `user:${grace}`)); // Give it a permission first, so the delete below has an orphan to avoid leaving behind. - await post("/admin/groups/eng/permissions", `_csrf=${token}&permission=users%3Aread`); + const permissionsBaseline = baselineOf(await (await get("/admin/groups/eng")).text()); + await post("/admin/groups/eng/permissions", `_csrf=${token}&baseline=${permissionsBaseline}&permission=users%3Aread`); assert.ok(tuples.some((tp) => tp.namespace === "Permission" && tp.object === "users:read" && tp.subject_set?.object === "eng")); // Delete the group: a confirm step (GET) then the POST removes every member tuple, back to the list. @@ -1349,9 +1357,11 @@ test("admin permission grants: the picker offers the declared catalog, and a sav } assert.match(edit, /value="users:read"[^>]*checked/); // held → ticked assert.doesNotMatch(edit, /value="groups:write"[^>]*checked/); // not held → unticked + assert.doesNotMatch(edit, /Permissions saved/); // no confirmation before a save // Save a new set: users:write is added, users:read is dropped — the POST is the whole truth. - const saved = await post(`/admin/users/${ada}/permissions`, `_csrf=${token}&permission=users%3Awrite&permission=groups%3Aread`); + const baseline = baselineOf(edit); + const saved = await post(`/admin/users/${ada}/permissions`, `_csrf=${token}&baseline=${baseline}&permission=users%3Awrite&permission=groups%3Aread`); assert.equal(saved.status, 303); assert.deepEqual( tuples.filter((tp) => tp.subject_id === `user:${ada}`).map((tp) => tp.object).sort(), @@ -1359,16 +1369,93 @@ test("admin permission grants: the picker offers the declared catalog, and a sav ); assert.equal(denylist.isRevoked(ada, 0), true); // a change to your own grants revokes live tokens + // The redirect lands on a page that confirms the save, and shows the new state. + const landing = saved.headers.get("location") ?? ""; + assert.match(landing, new RegExp(`^/admin/users/${ada}\\?info-msg=permissions-saved$`)); + const confirmed = await (await get(landing)).text(); + assert.match(confirmed, /Permissions saved/); + assert.match(confirmed, /value="users:write"[^>]*checked/); + // A crafted POST can't grant something no plugin declares. - await post(`/admin/users/${ada}/permissions`, `_csrf=${token}&permission=users%3Awrite&permission=superuser%3Aall`); + await post(`/admin/users/${ada}/permissions`, `_csrf=${token}&baseline=${baselineOf(confirmed)}&permission=users%3Awrite&permission=superuser%3Aall`); assert.ok(!tuples.some((tp) => tp.object === "superuser:all")); // The same picker on a group writes the group's subject_set, which Keto resolves transitively. tuples.push({ namespace: "Group", object: "eng", relation: "members", subject_id: `user:${ada}` }); - await post("/admin/groups/eng/permissions", `_csrf=${token}&permission=groups%3Aread`); + const groupBaseline = baselineOf(await (await get("/admin/groups/eng")).text()); + const groupSaved = await post("/admin/groups/eng/permissions", `_csrf=${token}&baseline=${groupBaseline}&permission=groups%3Aread`); + assert.equal(groupSaved.headers.get("location"), "/admin/groups/eng?info-msg=permissions-saved"); assert.ok(tuples.some((tp) => tp.namespace === "Permission" && tp.object === "groups:read" && tp.subject_set?.object === "eng")); }); +// Two admins open the same page; the second save would silently undo the first. The form carries a +// token for the grants it was rendered from, and a save made from any other state is refused. +test("admin permission grants: a save from a stale page is refused, and the page re-renders the current state", async (t) => { + const ada = randomUUID(); + const identities: Identity[] = [{ id: ada, traits: { email: "ada@example.com" } }]; + const tuples: RelationTuple[] = [{ namespace: "Permission", object: "users:read", relation: "granted", subject_id: `user:${ada}` }]; + const keto = fakeKeto(tuples); + const kratosAdmin = stubAdmin({ getIdentity: async (id) => identities.find((i) => i.id === id) ?? null, listIdentities: async () => ({ identities, nextPageToken: null }) }); + const { get, post, token } = await adminHarness(t, { keto, kratosAdmin }); + const stale = baselineOf(await (await get(`/admin/users/${ada}`)).text()); + + // Another admin grants users:write while this page is open. + tuples.push({ namespace: "Permission", object: "users:write", relation: "granted", subject_id: `user:${ada}` }); + const before = JSON.stringify(tuples); + const refused = await post(`/admin/users/${ada}/permissions`, `_csrf=${token}&baseline=${stale}&permission=users%3Aread&permission=groups%3Aread`); + assert.equal(refused.status, 409); + const page = await refused.text(); + assert.match(page, /changed while you had the page open/); + assert.equal(JSON.stringify(tuples), before, "nothing was applied"); + assert.match(page, /value="users:write"[^>]*checked/); // the re-render shows the other admin's grant + assert.notEqual(baselineOf(page), stale); + + // Saving from the re-rendered page goes through. + const retried = await post(`/admin/users/${ada}/permissions`, `_csrf=${token}&baseline=${baselineOf(page)}&permission=users%3Aread&permission=users%3Awrite&permission=groups%3Aread`); + assert.equal(retried.status, 303); + assert.ok(tuples.some((tp) => tp.object === "groups:read" && tp.subject_id === `user:${ada}`)); + + // A POST with no token at all (a crafted request, or a form rendered before this check existed) is stale too. + assert.equal((await post(`/admin/users/${ada}/permissions`, `_csrf=${token}&permission=users%3Aread`)).status, 409); + + // Groups are checked the same way. + tuples.push({ namespace: "Group", object: "eng", relation: "members", subject_id: `user:${ada}` }); + const groupStale = baselineOf(await (await get("/admin/groups/eng")).text()); + tuples.push({ namespace: "Permission", object: "groups:read", relation: "granted", subject_set: { namespace: "Group", object: "eng", relation: "members" } }); + const groupRefused = await post("/admin/groups/eng/permissions", `_csrf=${token}&baseline=${groupStale}&permission=users%3Aread`); + assert.equal(groupRefused.status, 409); + assert.match(await groupRefused.text(), /changed while you had the page open/); + assert.ok(tuples.some((tp) => tp.object === "groups:read" && tp.subject_set?.object === "eng"), "the group keeps its grant"); +}); + +// applyGrants is not a transaction: a Keto error midway leaves some changes applied. The save says +// so, instead of the generic error page, and still revokes the tokens of what did change. +test("admin permission grants: a Keto failure midway reports a partial save and keeps what was applied", async (t) => { + const ada = randomUUID(); + const identities: Identity[] = [{ id: ada, traits: { email: "ada@example.com" } }]; + const tuples: RelationTuple[] = []; + let writes = 0; + const keto = fakeKeto(tuples, { + writeTuple: async (tp) => { + if (++writes === 2) throw new Error("keto unreachable"); + tuples.push(tp); + }, + }); + const kratosAdmin = stubAdmin({ getIdentity: async (id) => identities.find((i) => i.id === id) ?? null, listIdentities: async () => ({ identities, nextPageToken: null }) }); + const denylist = createDenylist(); + const { get, post, token } = await adminHarness(t, { denylist, keto, kratosAdmin }); + const baseline = baselineOf(await (await get(`/admin/users/${ada}`)).text()); + + const res = await post(`/admin/users/${ada}/permissions`, `_csrf=${token}&baseline=${baseline}&permission=groups%3Aread&permission=users%3Aread&permission=users%3Awrite`); + assert.equal(res.status, 502); + const page = await res.text(); + assert.match(page, /stopped part-way/); + assert.match(page, /value="groups:read"[^>]*checked/); // the first write landed, and the page shows what is stored + assert.doesNotMatch(page, /value="users:write"[^>]*checked/); + assert.deepEqual(tuples.map((tp) => tp.object), ["groups:read"]); + assert.equal(denylist.isRevoked(ada, 0), true); // the half that was applied still revokes live tokens +}); + // Revoking your own grants can remove the last users:write on the deployment, and the instant-revoke // hook lands it on the next request — recovery would be a curl against Keto. Guarded like // self-deactivate and self-delete are. (`admin1` is the harness's own sub.) @@ -1377,15 +1464,16 @@ test("admin permission grants: you can't revoke your own permissions, but you ca const tuples: RelationTuple[] = [{ namespace: "Permission", object: "users:write", relation: "granted", subject_id: "user:admin1" }]; const keto = fakeKeto(tuples); const kratosAdmin = stubAdmin({ getIdentity: async (id) => identities.find((i) => i.id === id) ?? null, listIdentities: async () => ({ identities, nextPageToken: null }) }); - const { post, token } = await adminHarness(t, { keto, kratosAdmin }); + const { get, post, token } = await adminHarness(t, { keto, kratosAdmin }); + const baseline = baselineOf(await (await get("/admin/users/admin1")).text()); - const refused = await post("/admin/users/admin1/permissions", `_csrf=${token}`); // every box cleared + const refused = await post("/admin/users/admin1/permissions", `_csrf=${token}&baseline=${baseline}`); // every box cleared assert.equal(refused.status, 400); assert.match(await refused.text(), /lock yourself out/); assert.ok(tuples.some((tp) => tp.object === "users:write" && tp.subject_id === "user:admin1"), "nothing was revoked"); // Granting yourself more is not a lockout, so it goes through. - const granted = await post("/admin/users/admin1/permissions", `_csrf=${token}&permission=users%3Awrite&permission=groups%3Aread`); + const granted = await post("/admin/users/admin1/permissions", `_csrf=${token}&baseline=${baseline}&permission=users%3Awrite&permission=groups%3Aread`); assert.equal(granted.status, 303); assert.ok(tuples.some((tp) => tp.object === "groups:read" && tp.subject_id === "user:admin1")); }); diff --git a/src/i18n/locales/en-US.ts b/src/i18n/locales/en-US.ts index 471eb78..37a85d7 100644 --- a/src/i18n/locales/en-US.ts +++ b/src/i18n/locales/en-US.ts @@ -46,6 +46,7 @@ const messages = { "common.edit": "Edit", "common.new": "New", "common.remove": "Remove", + "common.view": "View", "consent.allow": "Allow", "consent.deny": "Deny", diff --git a/src/i18n/locales/sv-SE.ts b/src/i18n/locales/sv-SE.ts index 85487eb..59a701d 100644 --- a/src/i18n/locales/sv-SE.ts +++ b/src/i18n/locales/sv-SE.ts @@ -37,6 +37,7 @@ const messages: CoreMessages = { "common.edit": "Redigera", "common.new": "Ny", "common.remove": "Ta bort", + "common.view": "Visa", "consent.allow": "Tillåt", "consent.deny": "Neka", diff --git a/src/ui/icons.ts b/src/ui/icons.ts index 8853ebf..58ff47c 100644 --- a/src/ui/icons.ts +++ b/src/ui/icons.ts @@ -13,6 +13,7 @@ export const ICON_NAMES: Record = { "i-chev": "chevron-right", "i-cols": "columns-3", "i-edit": "pencil", + "i-eye": "eye", "i-globe": "globe", "i-grid": "layout-grid", "i-kebab": "ellipsis-vertical", diff --git a/views/partials/icons.ejs b/views/partials/icons.ejs index f53a6c2..330bcbc 100644 --- a/views/partials/icons.ejs +++ b/views/partials/icons.ejs @@ -9,6 +9,7 @@ + -- 2.52.0 From 346d232cf7730d32501614c28604fca1a42b60d8 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Sat, 31 Oct 2026 09:35:22 +0100 Subject: [PATCH 2/8] =?UTF-8?q?Answer=20security=20round=201:=200=20?= =?UTF-8?q?=F0=9F=94=B4,=200=20=F0=9F=9F=A1=20(0=20misread);=20no=20findin?= =?UTF-8?q?gs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit -- 2.52.0 From 85bfadd040e703431c95a0faabf0a1b4f69ff910 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Sat, 31 Oct 2026 09:46:11 +0100 Subject: [PATCH 3/8] =?UTF-8?q?Answer=20stability=20round=201:=201=20?= =?UTF-8?q?=F0=9F=94=B4,=204=20=F0=9F=9F=A1=20(0=20misread);=20allow=20the?= =?UTF-8?q?=20stale-save=20409=20in=20the=20e2e=20console=20guard,=20repor?= =?UTF-8?q?t=20a=20partial=20save=20when=20Keto=20cannot=20be=20read=20aga?= =?UTF-8?q?in,=20add=20the=20missing=20group,=20no-op=20and=20plain-login?= =?UTF-8?q?=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- CHANGELOG.md | 40 +++++----- e2e-tests/full-flow.spec.ts | 10 ++- examples/plugins/admin/admin-grants.test.ts | 15 +++- examples/plugins/admin/admin-grants.ts | 23 +++--- examples/plugins/admin/admin-groups.ts | 15 ++-- examples/plugins/admin/admin-shared.ts | 10 +++ examples/plugins/admin/admin-users.ts | 14 ++-- examples/plugins/admin/i18n/en-US.ts | 3 +- examples/plugins/admin/i18n/sv-SE.ts | 3 +- .../views/partials/permission-picker.ejs | 6 +- src/http/app.test.ts | 78 ++++++++++++++++++- src/i18n/locales/en-US.ts | 1 - src/i18n/locales/sv-SE.ts | 1 - 13 files changed, 158 insertions(+), 61 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e22368c..d22b83a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,31 +6,33 @@ at boot. Entries start at 0.3.0. ## Unreleased -### The admin example refuses a permission save made from an out-of-date page - -Two administrators editing one user's or group's permissions no longer overwrite each other. The -form carries a token for the grants it showed, and a save made after they changed is refused with -"These permissions changed while you had the page open", re-rendering the current state. A POST with -no token is refused the same way, so a script posting to `/admin/users/:id/permissions` or -`/admin/groups/:name/permissions` must first read the token from the page's `baseline` field. - -### The admin example confirms a permission save and reports a partial one - -A save now lands on the page with "Permissions saved." If the permission service fails midway, the -page says the save stopped part-way and shows what is stored, instead of the generic error page; the -changes applied before the failure still sign the user's live sessions out. - -### A read-only holder sees View, not Edit, on the users list - -A holder of `users:read` without `users:write` gets a "View" row action with an eye icon. The new -`common.view` catalog key is in `en-US` and `sv-SE`; a mounted core catalog must add it. - ### A language chosen on a sign-in link no longer drops an absolute return target `/login?return_to=&locale=sv-SE` passed the visitor to the dashboard after sign-in and dropped the OAuth2 login challenge in `return_to`. The target now reaches Kratos, and the chosen language is lost across that round-trip instead. +### The admin example refuses a permission save made from an out-of-date page + +Two administrators editing one user's or group's permissions no longer overwrite each other. The +form carries a token for the grants it showed. A save that would change something, made after those +grants changed, is refused with "These permissions changed while you had the page open", and the page +shows the current grants. A POST with no token is refused the same way, so a script posting to +`/admin/users/:id/permissions` or `/admin/groups/:name/permissions` must first read the token from the +page's `baseline` field. + +### The admin example confirms a permission save and reports a partial one + +A save now lands on the page with "Permissions saved." If the permission service fails midway, the +page says the save stopped part-way instead of showing the generic error page. On a user, the +changes that landed still reach the user's live sessions at once when the revocation denylist is on. + +### A read-only holder sees View, not Edit, on the admin example's users list + +A holder of `users:read` without `users:write` gets a "View" row action with an eye icon. The admin +example's catalogs gain `admin.grants.partial`, `admin.grants.saved`, `admin.grants.stale` and +`admin.users.view`, so a translated catalog mounted under `locales/plugins/admin/` must add them. + ### Every page carries a smaller icon sprite `i-chart`, `i-copy` and `i-download` are gone from the inlined icon sprite, so a plugin that diff --git a/e2e-tests/full-flow.spec.ts b/e2e-tests/full-flow.spec.ts index 7e6996d..0f822f7 100644 --- a/e2e-tests/full-flow.spec.ts +++ b/e2e-tests/full-flow.spec.ts @@ -1,5 +1,5 @@ import type { Browser, Page } from "@playwright/test"; -import { expect, test, watchedPage } from "./console-guard.ts"; +import { allowConsole, expect, test, watchedPage } from "./console-guard.ts"; import { randomUUID } from "node:crypto"; // Full browser E2E: the real Playwright UI against the live stack via the same-origin @@ -136,7 +136,7 @@ test.describe.serial("authenticated admin journey", () => { await expect(page.locator("tr", { hasText: email })).toHaveCount(0); }); - test("groups CRUD: create a group (writes go to Keto), see it listed, then grant it a permission", async () => { + test("groups CRUD: create a group (writes go to Keto), see it listed, then grant it a permission", async ({ browser }) => { // A Keto set exists only while it has ≥1 member, so create needs a first member (the form // enforces it); pick the first option (a user) from the required picker. const group = `e2e-grp-${suffix}`; @@ -150,7 +150,9 @@ test.describe.serial("authenticated admin journey", () => { // Permissions are declared in plugin code, so the group's detail page offers them as a fixed // checkbox list rather than a create form — there is no Permissions screen to visit. await page.goto(`/admin/groups/${group}`); - const staleTab = await watchedPage(page.context()); // opened now, so it holds this page's grants + allowConsole(/status of 409 .*\/admin\/groups\/[^/]+\/permissions/); // the stale save below, refused on purpose + // A second browser signed in as the same admin, opened before the save below, so its form carries the old grants. + const staleTab = await watchedPage(await browser.newContext({ storageState: await page.context().storageState() })); await staleTab.goto(`/admin/groups/${group}`); const scheduling = page.locator('input[name="permission"][value="scheduling:read"]'); await expect(scheduling).toHaveCount(1); // declared by the reference plugin, so it's on offer @@ -168,7 +170,7 @@ test.describe.serial("authenticated admin journey", () => { await expect(staleTab.getByText("changed while you had the page open")).toBeVisible(); await expect(staleTab.locator('input[name="permission"][value="scheduling:read"]')).toBeChecked(); await expect(staleTab.locator('input[name="permission"][value="scheduling:write"]')).not.toBeChecked(); - await staleTab.close(); + await staleTab.context().close(); }); test("OAuth2 clients CRUD: register a client (writes go to Hydra), see the one-time secret once, then delete it via the confirm step", async () => { diff --git a/examples/plugins/admin/admin-grants.test.ts b/examples/plugins/admin/admin-grants.test.ts index 32899a1..27d9500 100644 --- a/examples/plugins/admin/admin-grants.test.ts +++ b/examples/plugins/admin/admin-grants.test.ts @@ -4,7 +4,7 @@ import assert from "node:assert/strict"; import { test } from "node:test"; import type { PermissionDecl } from "@plainpages/plugin-api"; import type { KetoClient } from "@plainpages/plugin-api"; -import { applyGrants, baselineToken, buildPermissionPicker, grantDiff, grantTuple, groupSubject, userSubject } from "./admin-grants.ts"; +import { applyGrants, baselineToken, buildPermissionPicker, grantDiff, grantTuple, groupSubject, isStale, userSubject } from "./admin-grants.ts"; const declared: PermissionDecl[] = [ { description: "View users", name: "users:read" }, @@ -83,7 +83,6 @@ test("buildPermissionPicker says so when no plugin declares a permission, rather assert.ok(picker.empty); }); -// The concurrency token of the permission form: equal exactly when the direct grants it offered are. test("baselineToken depends on the offered grants held, not on their order or on undeclared ones", () => { const token = baselineToken(declared, ["users:read", "users:write"]); assert.match(token, /^[0-9a-f]+$/); @@ -95,6 +94,18 @@ test("baselineToken depends on the offered grants held, not on their order or on assert.equal(buildPermissionPicker({ action: "/x", declared, direct: ["users:read", "users:write"] }).baseline, token); }); +test("isStale refuses a missing or old token, but only for a save that would change something", () => { + const held = ["users:read"]; + const change = grantDiff(declared, held, ["users:read", "users:write"]); + const nothing = grantDiff(declared, held, ["users:read"]); + const current = new URLSearchParams({ baseline: baselineToken(declared, held) }); + const old = new URLSearchParams({ baseline: baselineToken(declared, []) }); + assert.equal(isStale(current, declared, held, change), false); + assert.equal(isStale(old, declared, held, change), true); + assert.equal(isStale(new URLSearchParams(), declared, held, change), true); // no token at all + assert.equal(isStale(old, declared, held, nothing), false); // a repeated save has nothing to refuse +}); + test("applyGrants applies writes then deletes, and on a Keto failure stops and reports what was applied", async () => { const log: string[] = []; const keto = (failOn?: string): KetoClient => ({ diff --git a/examples/plugins/admin/admin-grants.ts b/examples/plugins/admin/admin-grants.ts index 232decc..a6b7513 100644 --- a/examples/plugins/admin/admin-grants.ts +++ b/examples/plugins/admin/admin-grants.ts @@ -15,6 +15,9 @@ export const PERMISSIONS_FIELD = "permission"; // the checkbox name the two form export const BASELINE_FIELD = "baseline"; // the hidden field carrying baselineToken export const SAVED_NOTICE = "permissions-saved"; // the `info-msg` a successful save redirects with +// Only that exact value counts, so a crafted link can't put words on the page. +export const isSavedNotice = (url: URL): boolean => url.searchParams.get("info-msg") === SAVED_NOTICE; + export type GrantSubject = { subject_id: string } | { subject_set: SubjectSet }; export const userSubject = (id: string): GrantSubject => ({ subject_id: `user:${id}` }); @@ -123,23 +126,25 @@ export function grantDiff(declared: readonly PermissionDecl[], held: string[], w }; } -// Proves which direct grants a form was rendered from, so a save made after someone else changed -// them is refused rather than silently undoing that change. Covers only the declared names the form -// offered, like grantDiff, and is a digest so an empty set is still a token and a missing field is not. +// A token for the direct grants a form was rendered from. Like grantDiff it covers only the declared +// names the form offered. It is a digest so an empty set is still a token; it is a guard against a +// stale page, not a secret, since anyone who can read the page can compute it. export function baselineToken(declared: readonly PermissionDecl[], held: string[]): string { const offered = new Set(declared.map((d) => d.name)); return createHash("sha256").update(held.filter((name) => offered.has(name)).sort().join("\n")).digest("hex").slice(0, 16); } -// True when the submitted token is missing or no longer matches Keto. -export const isStale = (form: { get(name: string): string | null }, declared: readonly PermissionDecl[], held: string[]): boolean => - form.get(BASELINE_FIELD) !== baselineToken(declared, held); +// True when the form's token is missing or no longer matches Keto *and* the save would change +// something. A save that changes nothing, such as a double-clicked Save whose first POST landed, is +// harmless, so it is not refused. +export const isStale = (form: { get(name: string): string | null }, declared: readonly PermissionDecl[], held: string[], diff: GrantChange): boolean => + (diff.grant.length > 0 || diff.revoke.length > 0) && form.get(BASELINE_FIELD) !== baselineToken(declared, held); export interface GrantChange { grant: string[]; revoke: string[] } -// Keto has no transaction, so the writes and deletes run one by one and stop at the first failure. -// What was applied by then is returned beside the error, so the caller can still revoke live tokens -// and tell the admin the save is only partly done. +// KetoClient writes one tuple per call, so the writes and deletes run one by one and stop at the first +// failure. What was applied by then is returned beside the error, so the caller can still revoke live +// tokens and tell the admin the save is only partly done. export async function applyGrants(keto: KetoClient, subject: GrantSubject, diff: GrantChange): Promise<{ applied: GrantChange; error?: unknown }> { const applied: GrantChange = { grant: [], revoke: [] }; try { diff --git a/examples/plugins/admin/admin-groups.ts b/examples/plugins/admin/admin-groups.ts index 230e8b3..81f1a3a 100644 --- a/examples/plugins/admin/admin-groups.ts +++ b/examples/plugins/admin/admin-groups.ts @@ -7,8 +7,8 @@ // each returning a RouteResult. 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 "@plainpages/plugin-api"; -import { applyGrants, buildPermissionPicker, effectivePermissions, grantDiff, grantTuple, groupSubject, heldPermissions, isStale, type PermissionPicker, PERMISSIONS_FIELD, SAVED_NOTICE } from "./admin-grants.ts"; -import { ADMIN_EN, type AdminAction, ADMIN_GROUPS_BASE, buildConfirmModel, guardedForm, notFound, permissionName, requirePermission, unavailable } from "./admin-shared.ts"; +import { applyGrants, buildPermissionPicker, effectivePermissions, grantDiff, grantTuple, groupSubject, heldPermissions, isSavedNotice, isStale, type PermissionPicker, PERMISSIONS_FIELD, SAVED_NOTICE } from "./admin-grants.ts"; +import { ADMIN_EN, type AdminAction, ADMIN_GROUPS_BASE, buildConfirmModel, guardedForm, notFound, partialSave, permissionName, requirePermission, unavailable } from "./admin-shared.ts"; import type { FieldConfig } from "./admin-users.ts"; const GROUP_NS = "Group"; @@ -369,9 +369,9 @@ async function groupDetailPage({ ctx, keto, kratosAdmin }: GroupsDeps, name: str 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" }; } -// GET /admin/groups/:name — the detail + membership page; `info-msg` is read as on the user page. +// GET /admin/groups/:name — the detail + membership page. export const groupsDetail = withGroupName((deps, name) => - groupDetailPage(deps, name, deps.ctx.url.searchParams.get("info-msg") === SAVED_NOTICE ? { notice: deps.ctx.t("admin.grants.saved") } : {})); + groupDetailPage(deps, name, isSavedNotice(deps.ctx.url) ? { notice: deps.ctx.t("admin.grants.saved") } : {})); // POST /admin/groups/:name/permissions — the submitted checkboxes are the desired set. Members hold // a group's permissions transitively, so the change reaches them at their next login or re-mint — @@ -381,14 +381,15 @@ export const groupsPermissions = withGroupName(async (deps, name) => { const form = (await guardedForm(ctx))!; const subject = groupSubject(name); const held = await heldPermissions(keto, subject); - if (isStale(form, ctx.declaredPermissions, held)) return { ...(await groupDetailPage(deps, name, { error: ctx.t("admin.grants.stale") })), status: 409 }; - const { applied, error } = await applyGrants(keto, subject, grantDiff(ctx.declaredPermissions, held, form.getAll(PERMISSIONS_FIELD))); + const diff = grantDiff(ctx.declaredPermissions, held, form.getAll(PERMISSIONS_FIELD)); + if (isStale(form, ctx.declaredPermissions, held, diff)) return { ...(await groupDetailPage(deps, name, { error: ctx.t("admin.grants.stale") })), status: 409 }; + const { applied, error } = await applyGrants(keto, subject, diff); if (applied.grant.length > 0 || applied.revoke.length > 0) { ctx.log.info("admin: group permissions changed", { actor: user.id, granted: applied.grant.join(","), group: name, revoked: applied.revoke.join(",") }); } if (error) { ctx.log.error("admin: group permissions only partly saved", { actor: user.id, error: String(error), group: name }); - return { ...(await groupDetailPage(deps, name, { error: ctx.t("admin.grants.partial") })), status: 502 }; + return partialSave(ctx, async () => ({ ...(await groupDetailPage(deps, name, { error: ctx.t("admin.grants.partial") })), status: 502 })); } return { redirect: `${detailHref(name)}?info-msg=${SAVED_NOTICE}` }; }); diff --git a/examples/plugins/admin/admin-shared.ts b/examples/plugins/admin/admin-shared.ts index 8e8b424..7aaaa74 100644 --- a/examples/plugins/admin/admin-shared.ts +++ b/examples/plugins/admin/admin-shared.ts @@ -83,6 +83,16 @@ export function unavailable(ctx: RequestContext, what: string): RouteResult { return { data: { chrome: ctx.chrome, message: ctx.t("admin.unavailable.message", { what }), title: ctx.t("admin.unavailable.title") }, status: 503, view: "notice" }; } +// The page a partial permission save reports on reads Keto again. If that read fails too, the report +// stands alone on the notice page, so the admin still hears that part of the save landed. +export async function partialSave(ctx: RequestContext, render: () => Promise): Promise { + try { + return await render(); + } catch { + return { data: { chrome: ctx.chrome, message: ctx.t("admin.grants.partial"), title: ctx.t("admin.unavailable.title") }, status: 502, view: "notice" }; + } +} + // Model for the shared destructive-confirm page (views/confirm.ejs). The view reads the shell fields // (brand/csrf/theme/user/nav) from ctx.chrome; this carries only the page body + title/breadcrumbs. export function buildConfirmModel(opts: { diff --git a/examples/plugins/admin/admin-users.ts b/examples/plugins/admin/admin-users.ts index e60c2d6..1ba1778 100644 --- a/examples/plugins/admin/admin-users.ts +++ b/examples/plugins/admin/admin-users.ts @@ -4,8 +4,8 @@ // a shared `withUser` gate. import { can, type Identity, type KetoClient, type KratosAdmin, KratosError, paginate, parseListQuery, type RecoveryCode, type RequestContext, type RouteHandler, type RouteResult, type Translate, type User } from "@plainpages/plugin-api"; -import { applyGrants, buildPermissionPicker, effectivePermissions, grantDiff, heldPermissions, isStale, type PermissionPicker, PERMISSIONS_FIELD, SAVED_NOTICE, userSubject } from "./admin-grants.ts"; -import { ADMIN_EN, type AdminAction, ADMIN_USERS_BASE, buildConfirmModel, guardedForm, notFound, permissionName, requirePermission, unavailable } from "./admin-shared.ts"; +import { applyGrants, buildPermissionPicker, effectivePermissions, grantDiff, heldPermissions, isSavedNotice, isStale, type PermissionPicker, PERMISSIONS_FIELD, SAVED_NOTICE, userSubject } from "./admin-grants.ts"; +import { ADMIN_EN, type AdminAction, ADMIN_USERS_BASE, buildConfirmModel, guardedForm, notFound, partialSave, permissionName, requirePermission, unavailable } from "./admin-shared.ts"; const SCHEMA_ID = "default"; // matches kratos.yml identity.default_schema_id const DEFAULT_PAGE_SIZE = 25; @@ -153,7 +153,7 @@ function listTable(rows: UserView[], state: ListState, sort: { dir: "asc" | "des return { href: listHref(state, { page: 1, sort: next }), label: t(c.label), sort: dir, sortable: true }; }), rows: rows.map((u) => ({ - actions: [{ href: `${ADMIN_USERS_BASE}/${encodeURIComponent(u.id)}`, icon: canWrite ? "i-edit" : "i-eye", label: t(canWrite ? "common.edit" : "common.view") }], + actions: [{ href: `${ADMIN_USERS_BASE}/${encodeURIComponent(u.id)}`, icon: canWrite ? "i-edit" : "i-eye", label: t(canWrite ? "common.edit" : "admin.users.view") }], cells: [ { user: { initials: u.initials, name: u.name } }, u.email, @@ -329,9 +329,7 @@ export const usersNewForm = withUser(({ ctx }) => Promise.resolve(formResult(ctx // GET /admin/users/:id — the edit form, prefilled. export const usersEditForm = withTarget(async (deps, identity, id) => { - // `info-msg` is honored only as the exact value a save redirects with, so a crafted link can't put words on the page. - const saved = deps.ctx.url.searchParams.get("info-msg") === SAVED_NOTICE; - const permissions = await userPermissionPicker(deps, id, undefined, saved ? deps.ctx.t("admin.grants.saved") : undefined); + const permissions = await userPermissionPicker(deps, id, undefined, isSavedNotice(deps.ctx.url) ? deps.ctx.t("admin.grants.saved") : undefined); return formResult(deps.ctx, { canWrite: canWriteUsers(deps.ctx), identity, ...(permissions ? { permissions } : {}) }); }); @@ -374,8 +372,8 @@ export const usersPermissions = withTarget(async (deps, identity, id) => { const permissions = await userPermissionPicker(deps, id, error); return { ...formResult(ctx, { canWrite: canWriteUsers(ctx), identity, ...(permissions ? { permissions } : {}) }), status }; }; - if (isStale(form, ctx.declaredPermissions, held)) return refuse(ctx.t("admin.grants.stale"), 409); const diff = grantDiff(ctx.declaredPermissions, held, form.getAll(PERMISSIONS_FIELD)); + if (isStale(form, ctx.declaredPermissions, held, diff)) return refuse(ctx.t("admin.grants.stale"), 409); // Self-lockout guard, matching the self-deactivate/self-delete ones: revoking your own grants can // remove the last `users:write` on the deployment, and the instant-revoke hook lands it on the very // next request — leaving a `curl` against Keto as the only way back in. @@ -390,7 +388,7 @@ export const usersPermissions = withTarget(async (deps, identity, id) => { } if (error) { ctx.log.error("admin: user permissions only partly saved", { actor: user.id, error: String(error), target: id }); - return refuse(ctx.t("admin.grants.partial"), 502); + return partialSave(ctx, () => refuse(ctx.t("admin.grants.partial"), 502)); } return { redirect: `${ADMIN_USERS_BASE}/${encodeURIComponent(id)}?info-msg=${SAVED_NOTICE}` }; }); diff --git a/examples/plugins/admin/i18n/en-US.ts b/examples/plugins/admin/i18n/en-US.ts index 52473cb..5c3d1ed 100644 --- a/examples/plugins/admin/i18n/en-US.ts +++ b/examples/plugins/admin/i18n/en-US.ts @@ -53,7 +53,7 @@ const messages = { "admin.grants.inherited": "Greyed-out permissions come from a group. Change them on that group.", "admin.grants.legend": "Permissions", "admin.grants.none": "No installed plugin declares a permission, so there is nothing to grant.", - "admin.grants.partial": "Saving stopped part-way because the permission service failed, so only some changes were applied. The list shows what is stored now. Save again to finish.", + "admin.grants.partial": "Saving stopped part-way because the permission service failed, so only some changes were applied. Check the permissions and make any remaining changes again.", "admin.grants.pending": "Members get this at their next sign-in (up to 10 minutes).", "admin.grants.save": "Save permissions", "admin.grants.saved": "Permissions saved.", @@ -152,6 +152,7 @@ const messages = { "admin.users.status.inactive": "Inactive", "admin.users.status.label": "Status", "admin.users.title": "Users", + "admin.users.view": "View", }; export type AdminMessages = typeof messages; diff --git a/examples/plugins/admin/i18n/sv-SE.ts b/examples/plugins/admin/i18n/sv-SE.ts index a170713..78bdd80 100644 --- a/examples/plugins/admin/i18n/sv-SE.ts +++ b/examples/plugins/admin/i18n/sv-SE.ts @@ -53,7 +53,7 @@ const messages: AdminMessages = { "admin.grants.inherited": "Gråmarkerade behörigheter kommer från en grupp. Ändra dem på gruppen.", "admin.grants.legend": "Behörigheter", "admin.grants.none": "Ingen installerad plugin deklarerar någon behörighet, så det finns inget att tilldela.", - "admin.grants.partial": "Sparandet avbröts halvvägs eftersom behörighetstjänsten fallerade, så bara en del av ändringarna genomfördes. Listan visar vad som är sparat nu. Spara igen för att slutföra.", + "admin.grants.partial": "Sparandet avbröts halvvägs eftersom behörighetstjänsten gav fel, så bara en del av ändringarna genomfördes. Kontrollera behörigheterna och gör de återstående ändringarna igen.", "admin.grants.pending": "Medlemmar får detta vid nästa inloggning (upp till 10 minuter).", "admin.grants.save": "Spara behörigheter", "admin.grants.saved": "Behörigheterna är sparade.", @@ -152,6 +152,7 @@ const messages: AdminMessages = { "admin.users.status.inactive": "Inaktiv", "admin.users.status.label": "Status", "admin.users.title": "Användare", + "admin.users.view": "Visa", }; export default messages; diff --git a/examples/plugins/admin/views/partials/permission-picker.ejs b/examples/plugins/admin/views/partials/permission-picker.ejs index 9414aaf..8cb25df 100644 --- a/examples/plugins/admin/views/partials/permission-picker.ejs +++ b/examples/plugins/admin/views/partials/permission-picker.ejs @@ -7,9 +7,6 @@ there) and every row when `readOnly` (the viewer holds :read but not :write). Neither can be diffed into an accidental revoke, because grantDiff compares against the direct grants only. - The hidden `baseline` is a token for the direct grants this page showed: a save made after they changed - is refused (see baselineToken in admin-grants.ts). - Locals: csrfToken, permissions ({ action, baseline, choices, empty, error, field, hint, inheritedNote, legend, notice, pending, readOnly, submit }). %>
@@ -32,7 +29,8 @@ <% }) -%>
<% } else { -%> - + <%# The fragment brings the card, and the notice in it, into view on a long page. %> +
diff --git a/src/http/app.test.ts b/src/http/app.test.ts index 13e5af8..72990c2 100644 --- a/src/http/app.test.ts +++ b/src/http/app.test.ts @@ -851,6 +851,9 @@ test("login return_to: a chosen language alone still routes through /auth/comple t.after(() => app.close()); const url = `http://localhost:${(app.address() as AddressInfo).port}`; + lastReturnTo = "unset"; + await fetch(url + "/login", { redirect: "manual" }); + assert.equal(lastReturnTo, undefined, "no target and no language: Kratos keeps its default"); await fetch(url + "/login?locale=sv-SE", { redirect: "manual" }); assert.match(lastReturnTo ?? "", /^http:\/\/[^/]+\/auth\/complete\?locale=sv-SE$/); await fetch(url + "/login?locale=sv-SE&return_to=" + encodeURIComponent("/admin/users"), { redirect: "manual" }); @@ -1388,8 +1391,7 @@ test("admin permission grants: the picker offers the declared catalog, and a sav assert.ok(tuples.some((tp) => tp.namespace === "Permission" && tp.object === "groups:read" && tp.subject_set?.object === "eng")); }); -// Two admins open the same page; the second save would silently undo the first. The form carries a -// token for the grants it was rendered from, and a save made from any other state is refused. +// Two admins open the same page; the second save would silently undo the first. test("admin permission grants: a save from a stale page is refused, and the page re-renders the current state", async (t) => { const ada = randomUUID(); const identities: Identity[] = [{ id: ada, traits: { email: "ada@example.com" } }]; @@ -1428,8 +1430,7 @@ test("admin permission grants: a save from a stale page is refused, and the page assert.ok(tuples.some((tp) => tp.object === "groups:read" && tp.subject_set?.object === "eng"), "the group keeps its grant"); }); -// applyGrants is not a transaction: a Keto error midway leaves some changes applied. The save says -// so, instead of the generic error page, and still revokes the tokens of what did change. +// A Keto error midway leaves some changes applied; the save says so and still revokes the user's live tokens. test("admin permission grants: a Keto failure midway reports a partial save and keeps what was applied", async (t) => { const ada = randomUUID(); const identities: Identity[] = [{ id: ada, traits: { email: "ada@example.com" } }]; @@ -1456,6 +1457,75 @@ test("admin permission grants: a Keto failure midway reports a partial save and assert.equal(denylist.isRevoked(ada, 0), true); // the half that was applied still revokes live tokens }); +// A double-clicked Save posts twice from one page; the second post finds the first one's result. +// A save that changes nothing has nothing to refuse and nothing to revoke. +test("admin permission grants: a save that changes nothing is not refused for a stale page and leaves live tokens alone", async (t) => { + const ada = randomUUID(); + const identities: Identity[] = [{ id: ada, traits: { email: "ada@example.com" } }]; + const tuples: RelationTuple[] = [{ namespace: "Permission", object: "users:read", relation: "granted", subject_id: `user:${ada}` }]; + const kratosAdmin = stubAdmin({ getIdentity: async (id) => identities.find((i) => i.id === id) ?? null, listIdentities: async () => ({ identities, nextPageToken: null }) }); + const denylist = createDenylist(); + const { get, post, token } = await adminHarness(t, { denylist, keto: fakeKeto(tuples), kratosAdmin }); + const stale = baselineOf(await (await get(`/admin/users/${ada}`)).text()); + tuples.push({ namespace: "Permission", object: "users:write", relation: "granted", subject_id: `user:${ada}` }); + const before = JSON.stringify(tuples); + + const repeated = await post(`/admin/users/${ada}/permissions`, `_csrf=${token}&baseline=${stale}&permission=users%3Aread&permission=users%3Awrite`); + assert.equal(repeated.status, 303); + assert.equal(JSON.stringify(tuples), before); + assert.equal(denylist.isRevoked(ada, 0), false); +}); + +// Keto answered the writes but not the re-read: the report must still reach the admin. +test("admin permission grants: a partial save is still reported when Keto cannot be read again", async (t) => { + const ada = randomUUID(); + const identities: Identity[] = [{ id: ada, traits: { email: "ada@example.com" } }]; + const tuples: RelationTuple[] = []; + let writes = 0; + let down = false; + let failureTakesKetoDown = true; + const keto = fakeKeto(tuples, { + listRelations: async (q = {}) => { + if (down) throw new Error("keto unreachable"); + return { nextPageToken: null, tuples: tuples.filter((tp) => matchesTuple(tp, q)) }; + }, + writeTuple: async (tp) => { + if (++writes === 2) { down = failureTakesKetoDown; throw new Error("keto unreachable"); } + tuples.push(tp); + }, + }); + const kratosAdmin = stubAdmin({ getIdentity: async (id) => identities.find((i) => i.id === id) ?? null, listIdentities: async () => ({ identities, nextPageToken: null }) }); + const { get, post, token } = await adminHarness(t, { keto, kratosAdmin }); + const baseline = baselineOf(await (await get(`/admin/users/${ada}`)).text()); + + const res = await post(`/admin/users/${ada}/permissions`, `_csrf=${token}&baseline=${baseline}&permission=groups%3Aread&permission=users%3Aread`); + assert.equal(res.status, 502); + assert.match(await res.text(), /stopped part-way/); + assert.deepEqual(tuples.map((tp) => tp.object), ["groups:read"]); + + // The same on a group, whose page needs Keto for its members as well. + down = false; + writes = 0; + tuples.length = 0; + tuples.push({ namespace: "Group", object: "eng", relation: "members", subject_id: `user:${ada}` }); + const groupBaseline = baselineOf(await (await get("/admin/groups/eng")).text()); + const groupRes = await post("/admin/groups/eng/permissions", `_csrf=${token}&baseline=${groupBaseline}&permission=groups%3Aread&permission=users%3Aread`); + assert.equal(groupRes.status, 502); + assert.match(await groupRes.text(), /stopped part-way/); + assert.ok(tuples.some((tp) => tp.object === "groups:read" && tp.subject_set?.object === "eng"), "the first write landed"); + + // With Keto still answering, the group page itself reports it and shows what is stored. + failureTakesKetoDown = false; + down = false; + writes = 0; + tuples.length = 1; + const again = await post("/admin/groups/eng/permissions", `_csrf=${token}&baseline=${baselineOf(await (await get("/admin/groups/eng")).text())}&permission=groups%3Aread&permission=users%3Aread`); + assert.equal(again.status, 502); + const page = await again.text(); + assert.match(page, /stopped part-way/); + assert.match(page, /value="groups:read"[^>]*checked/); +}); + // Revoking your own grants can remove the last users:write on the deployment, and the instant-revoke // hook lands it on the next request — recovery would be a curl against Keto. Guarded like // self-deactivate and self-delete are. (`admin1` is the harness's own sub.) diff --git a/src/i18n/locales/en-US.ts b/src/i18n/locales/en-US.ts index 37a85d7..471eb78 100644 --- a/src/i18n/locales/en-US.ts +++ b/src/i18n/locales/en-US.ts @@ -46,7 +46,6 @@ const messages = { "common.edit": "Edit", "common.new": "New", "common.remove": "Remove", - "common.view": "View", "consent.allow": "Allow", "consent.deny": "Deny", diff --git a/src/i18n/locales/sv-SE.ts b/src/i18n/locales/sv-SE.ts index 59a701d..85487eb 100644 --- a/src/i18n/locales/sv-SE.ts +++ b/src/i18n/locales/sv-SE.ts @@ -37,7 +37,6 @@ const messages: CoreMessages = { "common.edit": "Redigera", "common.new": "Ny", "common.remove": "Ta bort", - "common.view": "Visa", "consent.allow": "Tillåt", "consent.deny": "Neka", -- 2.52.0 From 21877aeaddf62becbb7b66bec88f8354c6ab92f3 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Sat, 31 Oct 2026 09:46:17 +0100 Subject: [PATCH 4/8] =?UTF-8?q?Answer=20product-owner=20round=201:=201=20?= =?UTF-8?q?=F0=9F=94=B4,=203=20=F0=9F=9F=A1=20(0=20misread);=20View=20move?= =?UTF-8?q?s=20to=20the=20admin=20catalog,=20a=20repeated=20save=20is=20no?= =?UTF-8?q?=20longer=20refused,=20the=20partial-save=20copy=20no=20longer?= =?UTF-8?q?=20promises=20a=20retry,=20the=20notice's=20card=20is=20the=20f?= =?UTF-8?q?orm's=20fragment,=20the=20CHANGELOG=20names=20the=20new=20admin?= =?UTF-8?q?=20keys?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit -- 2.52.0 From 3c0878ed1aa4b6f17086ff66bf69fdeee84af086 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Sat, 31 Oct 2026 09:46:17 +0100 Subject: [PATCH 5/8] =?UTF-8?q?Answer=20prose=20round=201:=200=20?= =?UTF-8?q?=F0=9F=94=B4,=204=20=F0=9F=9F=A1=20(0=20misread);=20correct=20t?= =?UTF-8?q?he=20CHANGELOG=20session=20claim=20and=20order,=20rewrite=20the?= =?UTF-8?q?=20partial=20copy=20and=20the=20baseline=20comments,=20one=20he?= =?UTF-8?q?lper=20for=20info-msg?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit -- 2.52.0 From b863ea26d99104f7593b60096f3e377684ee6769 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Sat, 31 Oct 2026 09:48:49 +0100 Subject: [PATCH 6/8] Remove the shipped items 4, 7, 22 and 39 from the plan, and file the Edit user heading and the one-call Keto save --- todo.md | 51 ++++++++++++++++++--------------------------------- 1 file changed, 18 insertions(+), 33 deletions(-) diff --git a/todo.md b/todo.md index 726b815..6a47507 100644 --- a/todo.md +++ b/todo.md @@ -6,18 +6,15 @@ `Bar = 1` -`Next ID = 40` +`Next ID = 42` ## Items | ID | Release | Blocked by | Exempt | Item | L | E | I | R | S | Moves | Score | |---|---|---|---|---|---|---|---|---|---|---|---| -| 4 | 0.4.21 | | | **Refuse a permission save whose baseline no longer matches Keto, so concurrent edits are not silently lost.** | 2 | 2 | 2 | 4 | 4 | 2 by 4 | 2.8 | -| 7 | 0.4.21 | | | **Confirm a successful permission save, and report a partial failure instead of hiding it.** | 2 | 2 | 2 | 4 | 4 | 2 by 4 | 2.8 | -| 39 | 0.4.21 | | | **Pass an absolute `return_to` on to Kratos even when the URL also carries a chosen `locale`.** | 1 | 2 | 2 | 3 | 2 | 2 by 2 | 2.8 | -| 22 | 0.4.21 | | | **Offer a `users:read` holder a View row action on the users list, not Edit.** | 2 | 2 | 1 | 3 | 3 | 2 by 2 | 2.1 | | 34 | 0.4.21 | | | **Serve every stylesheet and script under a content-versioned URL with a year-long immutable `Cache-Control`, so a page view downloads none of them again.** | 4 | 2 | 3 | 7 | 6 | 8 by 6, 2 by 2 | 1.8 | | 3 | 0.4.21 | | | **Guard the group paths against self-lockout, or accept the gap explicitly.** | 3 | 2 | 2 | 5 | 6 | 2 by 6 | 1.4 | +| 40 | 0.4.21 | | | **Show a `users:read` holder "View user" as the user page's heading and breadcrumb, not "Edit user".** | 1 | 2 | 1 | 2 | 2 | 2 by 1 | 1.4 | | 1 | 0.4.21 | | owner | **Publish every release to Docker Hub, and give the Hub page its short description.** | 3 | 3 | 2 | 6 | 5 | 1 by 3 | 1.3 | | 32 | 0.4.21 | | | **Fail the gate on a stylesheet feature the browser floor lacks, unless it is guarded.** | 4 | 2 | 2 | 6 | 7 | code health by 2 | 1.1 | | 13 | 0.4.21 | | | **Accept a verification code pasted with surrounding whitespace.** | 4 | 2 | 3 | 7 | 7 | 2 by 8 | 1.0 | @@ -28,38 +25,11 @@ | 28 | 0.5.0 | | | **Let a plugin add entries to the profile menu through the plugin API.** | 2 | 2 | 2 | 4 | 4 | 7 by 4, 1 by 2 | 4.2 | | 2 | 0.5.0 | | | **Rename the admin example plugin to a name that says what it administers.** | 3 | 1 | 2 | 4 | 6 | 9 by 3, code health by 2 | 2.6 | | 5 | 0.5.0 | | | **List grants that no installed plugin declares, with a remove action.** | 2 | 2 | 2 | 4 | 4 | 2 by 3 | 2.1 | +| 41 | 0.5.0 | | | **Apply a permission save as one Keto call, so it cannot stop part-way.** | 2 | 2 | 2 | 4 | 3 | 2 by 2 | 2.1 | | 14 | 0.5.0 | | | **Drop a double-clicked duplicate submit, with no client-side JavaScript.** | 4 | 2 | 3 | 7 | 7 | 2 by 7, 1 by 4 | 1.4 | ## Details -### 4. Refuse a permission save whose baseline no longer matches Keto, so concurrent edits are not silently lost. - -Sketch: post the rendered set as a hidden baseline; when it no longer matches Keto, re-render with -"this changed while you had the page open" rather than applying. - -### 7. Confirm a successful permission save, and report a partial failure instead of hiding it. - -`applyGrants` loops writes then deletes with no transaction, so a Keto error midway leaves a -half-applied set behind the generic error page. The `alert alert-pos` pattern of the recovery-code -banner is available. - -### 39. Pass an absolute `return_to` on to Kratos even when the URL also carries a chosen `locale`. - -`flowReturnTo` (`src/auth/routes.ts`) returns the absolute target as-is only when no locale is chosen. -With `/login?return_to=&locale=sv-SE` and no `?flow=`, it builds -`/auth/complete?locale=sv-SE` instead and drops the absolute target, so after sign-in the -visitor lands on the dashboard and the OAuth2 login challenge never continues. Normal paths don't hit -it, because Hydra's redirect carries no `locale` and the language picker acts on a page that already -holds a `?flow=`; a hand-built URL does. Fix by keeping the absolute target when `localPath` rejects -it, losing the language across that round-trip instead, and add the case to the `login return_to` -test in `src/http/app.test.ts`. - -### 22. Offer a `users:read` holder a View row action on the users list, not Edit. - -The pencil opens the read-only detail page, so it is harmless, but the label contradicts what the -reader can do. Needs `canWrite` threaded into `listTable`, a `common.view` core catalog key and an -`i-eye` icon. - ### 34. Serve every stylesheet and script under a content-versioned URL with a year-long immutable `Cache-Control`, so a page view downloads none of them again. `serveStatic` (`src/http/static.ts`) sends only `content-length` and `content-type`, so every page @@ -97,6 +67,13 @@ on a group you belong to, leaving that group, or deleting it can still strip you warning. AGENTS.md records this as a known gap. The robust fix is a "last effective holder" check, which needs a reverse Keto query. +### 40. Show a `users:read` holder "View user" as the user page's heading and breadcrumb, not "Edit user". + +The users list now says View for a read-only holder, but the page it opens still titles itself +`admin.users.edit` ("Edit user") with an "Edit" breadcrumb. `buildUserFormModel` in +`examples/plugins/admin/admin-users.ts` already receives `canWrite`; the two admin catalog keys it +needs are new. + ### 1. Publish every release to Docker Hub, and give the Hub page its short description. Hub's newest tag is 0.4.0, while git has v0.4.1 onwards, and the overview still says 0.4.0. No @@ -162,6 +139,14 @@ E2E stacks and README move with it. `grantDiff` never revokes an undeclared name, and nothing shows one either, so it can't be audited or cleaned, and reinstalling its plugin silently reactivates access nobody remembers granting. +### 41. Apply a permission save as one Keto call, so it cannot stop part-way. + +`applyGrants` writes and deletes one tuple per request and reports a partial save when one fails. +**Check first:** Keto documents `PATCH /admin/relation-tuples`, which should apply a list of inserts +and deletes in one transaction; confirm that against the Keto version `ory/` pins. If so, add the +call to `KetoClient`, which is contract surface (`@plainpages/plugin-api`) and so needs 0.5.0, and the +partial-failure path in the admin example can go. + ### 14. Drop a double-clicked duplicate submit, with no client-side JavaScript. A second identical POST is expected from a non-technical user, and today creates two -- 2.52.0 From 48c89baf8efbc791807ac2df751719f3983e0bff Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Sat, 31 Oct 2026 09:48:53 +0100 Subject: [PATCH 7/8] =?UTF-8?q?Answer=20stability=20round=202:=200=20?= =?UTF-8?q?=F0=9F=94=B4,=200=20=F0=9F=9F=A1=20(0=20misread);=20no=20findin?= =?UTF-8?q?gs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit -- 2.52.0 From 910ac18798a6d67f5a1bca1525551e51992eee43 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Sat, 31 Oct 2026 09:48:53 +0100 Subject: [PATCH 8/8] =?UTF-8?q?Answer=20product-owner=20round=202:=200=20?= =?UTF-8?q?=F0=9F=94=B4,=200=20=F0=9F=9F=A1=20(0=20misread);=20one=20?= =?UTF-8?q?=F0=9F=9F=A2=20answered=20by=20filing=20the=20Edit=20user=20hea?= =?UTF-8?q?ding=20as=20item=2040?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit -- 2.52.0