Answer stability round 1: 0 🔴, 6 🟡 (0 misread); pin which tuples a group delete keeps, test the self-refusal reasons and the failed save, one refuseForm comment

This commit is contained in:
2026-11-08 23:11:15 +01:00
parent 5088352fcf
commit a0ad0ddf2d
5 changed files with 58 additions and 51 deletions
+7 -7
View File
@@ -12,6 +12,12 @@ The host's contract version is now 0.5.0, so discovery refuses a plugin whose `a
`0.4.x`. Change the literal in each plugin's manifest when you upgrade, and add `assetHref` to any `0.4.x`. Change the literal in each plugin's manifest when you upgrade, and add `assetHref` to any
`RequestContext` your tests build by hand: `assetHref: (href) => href`. `RequestContext` your tests build by hand: `assetHref: (href) => href`.
### The core dashboard view is `dashboard`, not `index`
`views/index.ejs` is now `views/dashboard.ejs`. The `apiVersion` check can't see this: a plugin view
that still does `include("index")` fails when it renders, with the 500 page. Change it to
`include("dashboard")`.
### `KetoClient.transactTuples` applies inserts and deletes as one transaction ### `KetoClient.transactTuples` applies inserts and deletes as one transaction
`transactTuples({ delete, insert })` sends both lists to Keto in one `PATCH /admin/relation-tuples`, `transactTuples({ delete, insert })` sends both lists to Keto in one `PATCH /admin/relation-tuples`,
@@ -26,13 +32,7 @@ and answers with the error page. It used to leave some grants gone and the membe
### The admin example keeps a user's permissions section when it refuses a self-deactivate or self-delete ### The admin example keeps a user's permissions section when it refuses a self-deactivate or self-delete
The refusal page used to drop the permissions section of the user form. It now shows the section, as The refusal page used to drop the permissions section of the user form.
every other re-render of the form does.
### The core dashboard view is `dashboard`, not `index`
`views/index.ejs` is now `views/dashboard.ejs`, named for the `/dashboard` page it renders. A plugin
view that does `include("index")` must include `dashboard` instead.
### The admin example lists a user's or group's grants that no installed plugin declares, with a Remove ### The admin example lists a user's or group's grants that no installed plugin declares, with a Remove
+6 -6
View File
@@ -431,20 +431,20 @@ export const groupsDeleteConfirm = withGroupName(async (deps, name) => {
}) }, view: "confirm" }; }) }, view: "confirm" };
}, "write"); }, "write");
// POST /auth-admin/groups/:name/delete — remove every member tuple and held grant in one Keto // POST /auth-admin/groups/:name/delete — one Keto transaction removes the group's member tuples
// transaction (the group ceases to exist, and a failure leaves it whole). // and the grants it holds, so a Keto failure leaves the group as it was.
export const groupsDelete = withGroupName(async (deps, name) => { export const groupsDelete = withGroupName(async (deps, name) => {
const { ctx, keto, user } = deps; const { ctx, keto, user } = deps;
await guardedForm(ctx); // CSRF-verify the POST await guardedForm(ctx); // CSRF-verify the POST
const lost = await lostOnDelete(deps, name); const lost = await lostOnDelete(deps, name);
if (lost.length > 0) return refuseSelfLockout(deps, name, "delete", lost); if (lost.length > 0) return refuseSelfLockout(deps, name, "delete", lost);
// Drop what the group *holds* along with what it *contains*: a Keto set exists only through its // A Keto set exists only through its tuples, so leaving the grants behind would resurrect every
// tuples, so leaving the grants behind would resurrect every permission the moment someone // permission the moment someone re-created a group with the same name.
// re-created a group with the same name.
const subject = groupSubject(name); const subject = groupSubject(name);
const held = await heldPermissions(keto, subject); const held = await heldPermissions(keto, subject);
const grants = held.map((permission) => grantTuple(permission, subject));
const members = await pagedTuples(keto, { namespace: GROUP_NS, object: name, relation: MEMBERS }); const members = await pagedTuples(keto, { namespace: GROUP_NS, object: name, relation: MEMBERS });
await keto.transactTuples({ delete: [...held.map((permission) => grantTuple(permission, subject)), ...members], insert: [] }); await keto.transactTuples({ delete: [...grants, ...members], insert: [] });
ctx.log.info("admin: group deleted", { actor: user.id, group: name, revoked: held.join(",") }); ctx.log.info("admin: group deleted", { actor: user.id, group: name, revoked: held.join(",") });
return { redirect: ADMIN_GROUPS_BASE }; return { redirect: ADMIN_GROUPS_BASE };
}); });
+2 -2
View File
@@ -391,7 +391,6 @@ export const usersUpdate = withTarget(async (deps, identity, id) => {
try { try {
await kratosAdmin.updateIdentity(id, updateIdentityPayload(identity, input)); await kratosAdmin.updateIdentity(id, updateIdentityPayload(identity, input));
} catch (err) { } catch (err) {
// Re-render with the picker, or the permissions section vanishes off the page on a failed save.
if (err instanceof KratosError) return refuseForm(deps, identity, id, ctx.t("admin.users.error.save")); if (err instanceof KratosError) return refuseForm(deps, identity, id, ctx.t("admin.users.error.save"));
throw err; throw err;
} }
@@ -443,7 +442,8 @@ export const usersRecovery = withTarget(async (deps, identity, id) => {
return formResult(ctx, { canWrite: canWriteUsers(ctx), identity, recovery, ...(await pickerOrNothing(deps, id)) }); return formResult(ctx, { canWrite: canWriteUsers(ctx), identity, recovery, ...(await pickerOrNothing(deps, id)) });
}); });
// The 400 re-render of the user's form with `error`: the form every refusal of a user action shows. // The 400 re-render of the user's form with `error` above it. `usersPermissions` refuses through its
// own `refuse`, which puts the error in the permissions section instead.
async function refuseForm(deps: UsersDeps, identity: Identity, id: string, error: string): Promise<RouteResult> { async function refuseForm(deps: UsersDeps, identity: Identity, id: string, error: string): Promise<RouteResult> {
return { ...formResult(deps.ctx, { canWrite: canWriteUsers(deps.ctx), error, identity, ...(await pickerOrNothing(deps, id)) }), status: 400 }; return { ...formResult(deps.ctx, { canWrite: canWriteUsers(deps.ctx), error, identity, ...(await pickerOrNothing(deps, id)) }), status: 400 };
} }
+32 -6
View File
@@ -11,6 +11,7 @@ import { staticJwks } from "../auth/jwks.ts";
import type { OAuth2Client } from "../auth/hydra-admin.ts"; import type { OAuth2Client } from "../auth/hydra-admin.ts";
import type { RelationTuple } from "../auth/keto-client.ts"; import type { RelationTuple } from "../auth/keto-client.ts";
import type { Identity } from "../auth/kratos-admin.ts"; import type { Identity } from "../auth/kratos-admin.ts";
import { KratosError } from "../auth/kratos-public.ts";
import { SESSION_COOKIE } from "../auth/login.ts"; import { SESSION_COOKIE } from "../auth/login.ts";
import type { Plugin } from "../plugin-host/plugin.ts"; import type { Plugin } from "../plugin-host/plugin.ts";
import adminManifest from "../../examples/plugins/auth-admin/plugin.ts"; import adminManifest from "../../examples/plugins/auth-admin/plugin.ts";
@@ -213,6 +214,11 @@ test("admin Groups screen: gate, list, create, detail/membership, delete (CSRF-g
// Delete the group: a confirm step (GET) then the POST removes every member tuple, back to the list. // Delete the group: a confirm step (GET) then the POST removes every member tuple, back to the list.
assert.match(await (await get("/auth-admin/groups/eng/delete")).text(), /Cancel/); assert.match(await (await get("/auth-admin/groups/eng/delete")).text(), /Cancel/);
const bystanders: RelationTuple[] = [
{ namespace: "Group", object: "ops", relation: "members", subject_id: `user:${ada}` },
{ namespace: "Permission", object: "users:read", relation: "granted", subject_id: `user:${ada}` },
];
tuples.push(...bystanders);
const del = await post("/auth-admin/groups/eng/delete", `_csrf=${token}`); const del = await post("/auth-admin/groups/eng/delete", `_csrf=${token}`);
assert.equal(del.status, 303); assert.equal(del.status, 303);
assert.equal(del.headers.get("location"), "/auth-admin/groups"); assert.equal(del.headers.get("location"), "/auth-admin/groups");
@@ -220,6 +226,7 @@ test("admin Groups screen: gate, list, create, detail/membership, delete (CSRF-g
// …and the permissions it held go with it. A Keto set exists only through its tuples, so an // …and the permissions it held go with it. A Keto set exists only through its tuples, so an
// orphaned grant would resurrect the moment someone re-created a group with the same name. // orphaned grant would resurrect the moment someone re-created a group with the same name.
assert.ok(!tuples.some((tp) => tp.namespace === "Permission" && tp.subject_set?.object === "eng")); assert.ok(!tuples.some((tp) => tp.namespace === "Permission" && tp.subject_set?.object === "eng"));
assert.ok(bystanders.every((tp) => tuples.includes(tp)), "only the deleted group's own tuples go");
// An invalid group name in the path → 404; malformed %-encoding doesn't 500. // An invalid group name in the path → 404; malformed %-encoding doesn't 500.
assert.equal((await get("/auth-admin/groups/Bad%20Name")).status, 404); assert.equal((await get("/auth-admin/groups/Bad%20Name")).status, 404);
@@ -374,17 +381,36 @@ test("admin users: a refused self-deactivate or self-delete keeps the permission
const { get, post, token } = await adminHarness(t, { keto: fakeKeto(), kratosAdmin }); const { get, post, token } = await adminHarness(t, { keto: fakeKeto(), kratosAdmin });
assert.notEqual(baselineOf(await (await get("/auth-admin/users/admin1")).text()), "", "the plain form carries the section"); assert.notEqual(baselineOf(await (await get("/auth-admin/users/admin1")).text()), "", "the plain form carries the section");
const refusals = [ const refusals: [Response, RegExp][] = [
await post("/auth-admin/users/admin1/state", `_csrf=${token}`), [await post("/auth-admin/users/admin1/state", `_csrf=${token}`), /deactivate your own account/],
await get("/auth-admin/users/admin1/delete"), [await get("/auth-admin/users/admin1/delete"), /delete your own account/],
await post("/auth-admin/users/admin1/delete", `_csrf=${token}`), [await post("/auth-admin/users/admin1/delete", `_csrf=${token}`), /delete your own account/],
]; ];
for (const res of refusals) { for (const [res, reason] of refusals) {
assert.equal(res.status, 400); assert.equal(res.status, 400);
assert.notEqual(baselineOf(await res.text()), ""); const html = await res.text();
assert.match(html, reason);
assert.notEqual(baselineOf(html), "");
} }
}); });
test("admin users: a Kratos rejection of a save re-renders the form with its permissions, any other failure is a 500", async (t) => {
const identities: Identity[] = [{ id: "ada", traits: { email: "ada@example.com" } }];
let failure: Error = new KratosError("rejected", 400, "");
const kratosAdmin = stubAdmin({
getIdentity: async (id) => identities.find((i) => i.id === id) ?? null,
listIdentities: async () => ({ identities, nextPageToken: null }),
updateIdentity: async () => { throw failure; },
});
const { post, token } = await adminHarness(t, { keto: fakeKeto(), kratosAdmin });
const rejected = await post("/auth-admin/users/ada", `_csrf=${token}&first=Ada`);
assert.equal(rejected.status, 400);
assert.notEqual(baselineOf(await rejected.text()), "");
failure = new Error("kratos unreachable");
assert.equal((await post("/auth-admin/users/ada", `_csrf=${token}&first=Ada`)).status, 500);
});
test("admin group delete: a Keto failure leaves the group's grants and members in place", async (t) => { test("admin group delete: a Keto failure leaves the group's grants and members in place", async (t) => {
const ada = randomUUID(); const ada = randomUUID();
const identities: Identity[] = [{ id: ada, traits: { email: "ada@example.com" } }]; const identities: Identity[] = [{ id: ada, traits: { email: "ada@example.com" } }];
+11 -30
View File
@@ -6,19 +6,17 @@
`Bar = 1` `Bar = 1`
`Next ID = 71` `Next ID = 72`
## Items ## Items
| ID | Release | Blocked by | Exempt | Item | L | E | I | R | S | Moves | Score | | ID | Release | Blocked by | Exempt | Item | L | E | I | R | S | Moves | Score |
|---|---|---|---|---|---|---|---|---|---|---|---| |---|---|---|---|---|---|---|---|---|---|---|---|
| 62 | 0.5.0 | | | **Delete a group's grants and its membership set in one Keto transaction, so a failure cannot leave it half-deleted.** | 1 | 2 | 2 | 3 | 2 | code health by 2, 2 by 3 | 16.2 |
| 63 | 0.5.0 | | | **Correct the stale and false comments and docs the comprehension seats flagged, so each states what the code does today.** | 2 | 2 | 1 | 3 | 3 | code health by 3, 9 by 2 | 15.4 | | 63 | 0.5.0 | | | **Correct the stale and false comments and docs the comprehension seats flagged, so each states what the code does today.** | 2 | 2 | 1 | 3 | 3 | code health by 3, 9 by 2 | 15.4 |
| 68 | 0.5.0 | | | **Make `serveHome` and `serveDashboard` return their result to the request pipeline, as every other stage does, instead of sending it themselves.** | 2 | 1 | 3 | 4 | 3 | code health by 3 | 13.5 | | 68 | 0.5.0 | | | **Make `serveHome` and `serveDashboard` return their result to the request pipeline, as every other stage does, instead of sending it themselves.** | 2 | 1 | 3 | 4 | 3 | code health by 3 | 13.5 |
| 69 | 0.5.0 | | | **Rename `views/index.ejs` to `views/dashboard.ejs`, so the core view's file names the page it renders.** | 1 | 2 | 2 | 3 | 2 | code health by 2 | 12.0 |
| 64 | 0.5.0 | | | **Keep the permissions section on the user form when a self-deactivate or self-delete is refused.** | 1 | 2 | 1 | 2 | 2 | 2 by 3, code health by 1 | 10.2 |
| 66 | 0.5.0 | | | **Make `src/auth/bootstrap.ts` read its settings through `src/config.ts` and call Kratos and Keto through their client modules.** | 2 | 1 | 3 | 4 | 4 | code health by 3 | 9.0 | | 66 | 0.5.0 | | | **Make `src/auth/bootstrap.ts` read its settings through `src/config.ts` and call Kratos and Keto through their client modules.** | 2 | 1 | 3 | 4 | 4 | code health by 3 | 9.0 |
| 70 | 0.5.0 | | | **Give the duplicated paging loop, Link-header parser and `FieldConfig` type one home each, and rename `buildPluginChrome` to `buildPageChrome`.** | 2 | 1 | 1 | 2 | 3 | code health by 2 | 9.0 | | 70 | 0.5.0 | | | **Give the duplicated paging loop, Link-header parser and `FieldConfig` type one home each, and rename `buildPluginChrome` to `buildPageChrome`.** | 2 | 1 | 1 | 2 | 3 | code health by 2 | 9.0 |
| 71 | 0.5.0 | | | **Delete, with a group, every membership tuple in another group that names it, so re-creating the name restores no nested membership.** | 1 | 2 | 2 | 3 | 2 | 2 by 3 | 4.2 |
| 19 | 0.6.0 | | question | **Run the documented Docker commands on macOS and fix whatever misbehaves.** | 2 | 2 | 1 | 3 | 3 | 1 by 3 | 3.3 | | 19 | 0.6.0 | | question | **Run the documented Docker commands on macOS and fix whatever misbehaves.** | 2 | 2 | 1 | 3 | 3 | 1 by 3 | 3.3 |
| 16 | 0.6.0 | | question | **Decide whether the single Keto `Resource` namespace splits into per-domain namespaces.** | 3 | 2 | 3 | 6 | 6 | 7 by 2 | 0.5 | | 16 | 0.6.0 | | question | **Decide whether the single Keto `Resource` namespace splits into per-domain namespaces.** | 3 | 2 | 3 | 6 | 6 | 7 by 2 | 0.5 |
| 65 | 0.6.0 | | | **Export `KetoError` from `@plainpages/plugin-api`, so a plugin can tell a Keto failure apart with `instanceof`.** | 1 | 2 | 1 | 2 | 1 | 7 by 3, 1 by 1 | 6.7 | | 65 | 0.6.0 | | | **Export `KetoError` from `@plainpages/plugin-api`, so a plugin can tell a Keto failure apart with `instanceof`.** | 1 | 2 | 1 | 2 | 1 | 7 by 3, 1 by 1 | 6.7 |
@@ -32,14 +30,6 @@
## Details ## Details
### 62. Delete a group's grants and its membership set in one Keto transaction, so a failure cannot leave it half-deleted.
`groupsDelete` in `examples/plugins/auth-admin/admin-groups.ts` drops each held grant with its own
`deleteTuple`, then the membership tuple, so a Keto failure part-way leaves a group with some grants
gone and its members still listed. Send all the deletes through `KetoClient.transactTuples`, as
`applyGrants` in `admin-grants.ts` does. The comment on delete order stays true inside one
transaction.
### 63. Correct the stale and false comments and docs the comprehension seats flagged, so each states what the code does today. ### 63. Correct the stale and false comments and docs the comprehension seats flagged, so each states what the code does today.
Prose only; no behaviour changes. Make each listed place easier to read, since the seats named Prose only; no behaviour changes. Make each listed place easier to read, since the seats named
@@ -80,24 +70,6 @@ result with whatever names its renderer and let the one send stage render it. Ma
`prepareRequest` and `handleRequest` easier to read, places the seats found hard. The 19-field `prepareRequest` and `handleRequest` easier to read, places the seats found hard. The 19-field
`Host` bag is left alone: no checkable target for it came out of the scoring run. `Host` bag is left alone: no checkable target for it came out of the scoring run.
### 69. Rename `views/index.ejs` to `views/dashboard.ejs`, so the core view's file names the page it renders.
`index.ejs` renders `/dashboard` (`src/http/app.ts` ~L168); `home.ejs` renders `/` and matches the
manifest's `home` slot, so it keeps its name. Core `views/` is an `include()` root for plugin views
(`src/plugin-host/view-resolver.ts`), so a plugin may include `index` today: the rename changes the
contract surface (AGENTS.md → Rules) and ships in a minor. Update `docs/reading-cost.md` and the
README file map. Make `views/index.ejs` easier to find, a place the seats found hard.
### 64. Keep the permissions section on the user form when a self-deactivate or self-delete is refused.
In `examples/plugins/auth-admin/admin-users.ts`, the self-refusals in `usersState` (~L405),
`usersDeleteConfirm` (~L415) and `usersDelete` (~L429) call `formResult` without
`...(await pickerOrNothing(deps, id))` or `canWrite`, so the re-rendered form loses its permissions
section; `usersUpdate` and `usersRecovery` keep it, and `pickerOrNothing`'s comment promises it.
The groups side re-renders through `groupDetailPage`, which keeps it. Make `usersPermissions` and
the `pickerOrNothing` contract easier to read, a place the seats found hard. Not exempt: a
comprehension finding, not a 🔴, and no maintain goal is broken.
### 66. Make `src/auth/bootstrap.ts` read its settings through `src/config.ts` and call Kratos and Keto through their client modules. ### 66. Make `src/auth/bootstrap.ts` read its settings through `src/config.ts` and call Kratos and Keto through their client modules.
`seedAdminAndPermissions` (~L221-233) reads `ADMIN_EMAIL`, `ADMIN_PASSWORD`, `KETO_WRITE_URL`, `seedAdminAndPermissions` (~L221-233) reads `ADMIN_EMAIL`, `ADMIN_PASSWORD`, `KETO_WRITE_URL`,
@@ -121,6 +93,15 @@ easier to read, a place the seats found hard.
included; rename it, its test and the README ~L1027 mention. Make `buildPluginChrome` easier to included; rename it, its test and the README ~L1027 mention. Make `buildPluginChrome` easier to
read, a place the seats found hard. read, a place the seats found hard.
### 71. Delete, with a group, every membership tuple in another group that names it, so re-creating the name restores no nested membership.
Groups nest: a parent holds `Group:<parent>#members` with `subject_set` `Group:<child>#members`.
`groupsDelete` in `examples/plugins/auth-admin/admin-groups.ts` drops the child's own member tuples
and held grants, but not those parent tuples, so a group re-created with the deleted name is back
in every parent, the orphan class its comment already guards against for grants. List them with
`pagedTuples` filtered on `subject_set` (`namespace: GROUP_NS`, `object: name`, `relation: MEMBERS`)
and add them to the same `transactTuples` delete; extend the comment to say so.
### 19. Run the documented Docker commands on macOS and fix whatever misbehaves. ### 19. Run the documented Docker commands on macOS and fix whatever misbehaves.
Needs a Mac. Two suspects, both from `--user "$(id -u):$(id -g)"`: a macOS `id -g` is `20`, which is Needs a Mac. Two suspects, both from `--user "$(id -u):$(id -g)"`: a macOS `id -g` is `20`, which is