Answer stability round 1: 1 🔴, 4 🟡 (0 misread); allow the stale-save 409 in the e2e console guard, report a partial save when Keto cannot be read again, add the missing group, no-op and plain-login tests

2026-10-31 09:46:11 +01:00
parent 346d232cf7
commit 85bfadd040
13 changed files with 158 additions and 61 deletions
@@ -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 => ({
@@ -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 {
@@ -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}` };
 });
@@ -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<RouteResult>): Promise<RouteResult> {
+  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: {
@@ -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}` };
 });
@@ -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;
@@ -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;
@@ -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 }).
 %>
  <section class="form-card" aria-labelledby="permissions-h">
@@ -32,7 +29,8 @@
 <% }) -%>
    </fieldset>
 <% } else { -%>
-    <form method="post" action="<%= localeHref(permissions.action) %>">
+    <%# The fragment brings the card, and the notice in it, into view on a long page. %>
+    <form method="post" action="<%= localeHref(permissions.action + '#permissions-h') %>">
      <input type="hidden" name="_csrf" value="<%= csrfToken %>">
      <input type="hidden" name="baseline" value="<%= permissions.baseline %>">
      <fieldset class="check-group">