diff --git a/README.md b/README.md index dd28507..19a7a8e 100644 --- a/README.md +++ b/README.md @@ -645,7 +645,7 @@ interface RequestContext { req: IncomingMessage; res: ServerResponse; permissions: string[]; // user?.permissions ?? [] — coarse gate without a null-check - declaredPermissions: PermissionDecl[]; // every permission the installed plugins declare, deduped + sorted — what *exists*, vs `permissions` = what this user *holds* + declaredPermissions: readonly PermissionDecl[]; // every permission the installed plugins declare, deduped + sorted — what *exists*, vs `permissions` = what this user *holds* system?: SystemCapabilities; // privileged Ory clients + instant-revoke, for a system plugin (see below); undefined unless the host wired them url: URL; verifyCsrf(submitted): boolean; // gate a form POST against the request's signed CSRF cookie diff --git a/e2e-tests/compose.full.yml b/e2e-tests/compose.full.yml index afb86a6..0c07d59 100644 --- a/e2e-tests/compose.full.yml +++ b/e2e-tests/compose.full.yml @@ -1,5 +1,5 @@ # Full browser E2E — the real Playwright UI flow against the live stack: password + mocked-SSO -# login, menu filtering by permission, users/groups/permissions/OAuth2-clients CRUD, a plugin page, logout. A +# login, menu filtering by permission, users/groups/OAuth2-clients CRUD + permission granting, a plugin page, logout. A # tiny same-origin gateway (proxy, e2e-tests/proxy.ts) fronts web + Kratos on one host so the browser's cookies # round-trip (ory/kratos/e2e-proxy.yml points Kratos at it); a mock OIDC provider backs the SSO test. # docker compose -f compose.yml -f e2e-tests/compose.full.yml run --build --rm e2e diff --git a/examples/plugins/admin/admin-grants.test.ts b/examples/plugins/admin/admin-grants.test.ts index efba871..4965ef9 100644 --- a/examples/plugins/admin/admin-grants.test.ts +++ b/examples/plugins/admin/admin-grants.test.ts @@ -60,9 +60,20 @@ test("buildPermissionPicker distinguishes a direct grant from one inherited thro }); test("buildPermissionPicker in read-only mode still shows the state, and marks itself unwritable", () => { - const picker = buildPermissionPicker({ action: "/x", declared, direct: ["users:read"], readOnly: true }); + const picker = buildPermissionPicker({ action: "/x", declared, direct: ["users:read"], effective: ["users:read", "groups:read"], readOnly: true }); assert.equal(picker.readOnly, true); - assert.deepEqual(picker.choices.map((c) => c.checked), [true, false, false]); // a reader still sees who holds what + assert.deepEqual(picker.choices.map((c) => c.checked), [true, false, true]); // a reader still sees who holds what + // Every row renders disabled for a reader, so the writable copy would be wrong twice over: "tick to + // grant" is false, and "greyed-out means group-held" would misattribute the direct grant. + assert.equal(picker.inheritedNote, undefined); + assert.notEqual(picker.hint, buildPermissionPicker({ action: "/x", declared, direct: [] }).hint); +}); + +test("buildPermissionPicker notes the transitive lag for a group, and stays quiet for a user", () => { + // A group's members inherit, so the change reaches them at their next re-mint; a user's own grant + // change revokes their live tokens, so there is nothing to warn about. + assert.ok(buildPermissionPicker({ action: "/x", declared, direct: [], transitive: true }).pending); + assert.equal(buildPermissionPicker({ action: "/x", declared, direct: [] }).pending, undefined); }); test("buildPermissionPicker says so when no plugin declares a permission, rather than rendering an empty box", () => { diff --git a/examples/plugins/admin/admin-grants.ts b/examples/plugins/admin/admin-grants.ts index aab8863..d7bd671 100644 --- a/examples/plugins/admin/admin-grants.ts +++ b/examples/plugins/admin/admin-grants.ts @@ -93,8 +93,10 @@ export function buildPermissionPicker(opts: { choices, empty: opts.declared.length === 0 ? t("admin.grants.none") : undefined, field: PERMISSIONS_FIELD, - hint: t("admin.grants.hint"), - inheritedNote: choices.some((c) => c.inherited) ? t("admin.grants.inherited") : undefined, + // A reader sees every row disabled, so "tick to grant" is false and "greyed-out means group-held" + // is worse than false — it would misattribute a *direct* grant to a group that doesn't hold it. + hint: t(opts.readOnly === true ? "admin.grants.hintReadOnly" : "admin.grants.hint"), + inheritedNote: opts.readOnly !== true && choices.some((c) => c.inherited) ? t("admin.grants.inherited") : undefined, legend: t("admin.grants.legend"), pending: opts.transitive === true ? t("admin.grants.pending") : undefined, readOnly: opts.readOnly === true, diff --git a/examples/plugins/admin/admin-groups.ts b/examples/plugins/admin/admin-groups.ts index 5047b12..6a40d55 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 "#plugin-api"; -import { applyGrants, buildPermissionPicker, effectivePermissions, grantDiff, groupSubject, heldPermissions, type PermissionPicker, PERMISSIONS_FIELD } from "./admin-grants.ts"; +import { applyGrants, buildPermissionPicker, effectivePermissions, grantDiff, grantTuple, groupSubject, heldPermissions, type PermissionPicker, PERMISSIONS_FIELD } 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"; @@ -401,8 +401,14 @@ export const groupsDeleteConfirm = withGroupName((deps, name) => { // POST /admin/groups/:name/delete — remove every member tuple (the group ceases to exist). export const groupsDelete = withGroupName(async ({ ctx, keto, user }, name) => { await guardedForm(ctx); // CSRF-verify the POST + // Drop what the group *holds* before what it *contains*: a Keto set exists only through its + // tuples, so leaving the grants behind would resurrect every permission the moment someone + // re-created a group with the same name. + const subject = groupSubject(name); + const held = await heldPermissions(keto, subject); + for (const permission of held) await keto.deleteTuple(grantTuple(permission, subject)); await keto.deleteTuple({ namespace: GROUP_NS, object: name, relation: MEMBERS }); - ctx.log.info("admin: group deleted", { actor: user.id, group: name }); + ctx.log.info("admin: group deleted", { actor: user.id, group: name, revoked: held.join(",") }); return { redirect: ADMIN_GROUPS_BASE }; }); diff --git a/examples/plugins/admin/i18n/en-US.ts b/examples/plugins/admin/i18n/en-US.ts index 88ad576..3271b62 100644 --- a/examples/plugins/admin/i18n/en-US.ts +++ b/examples/plugins/admin/i18n/en-US.ts @@ -49,6 +49,7 @@ const messages = { "admin.common.user": "User", "admin.grants.hint": "Which permissions exist is set by the plugins installed on this system. Tick to grant, untick to revoke.", + "admin.grants.hintReadOnly": "Which permissions exist is set by the plugins installed on this system. You can see these, but not change them.", "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.", @@ -63,7 +64,7 @@ const messages = { "admin.groups.column.name": "Group", "admin.groups.create": "Create group", "admin.groups.delete": "Delete group", - "admin.groups.deleteMessage": "Delete group {{name}}? This removes the group and all its memberships.", + "admin.groups.deleteMessage": "Delete group {{name}}? This removes the group, its members, and the permissions it grants them.", "admin.groups.field.name": "Group name", "admin.groups.field.nameHint": "Lowercase letters, digits, dashes and underscores.", "admin.groups.filter": "Filter groups", @@ -88,7 +89,6 @@ const messages = { "admin.notFound.message": "That item doesn't exist.", "admin.notFound.title": "Not found", - "admin.unavailable.message": "{{what}} is not configured on this deployment.", "admin.unavailable.title": "Admin unavailable", diff --git a/examples/plugins/admin/i18n/sv-SE.ts b/examples/plugins/admin/i18n/sv-SE.ts index a5f2696..aa4bd29 100644 --- a/examples/plugins/admin/i18n/sv-SE.ts +++ b/examples/plugins/admin/i18n/sv-SE.ts @@ -49,6 +49,7 @@ const messages: AdminMessages = { "admin.common.user": "Användare", "admin.grants.hint": "Vilka behörigheter som finns bestäms av de plugins som är installerade. Kryssa i för att tilldela, ur för att återkalla.", + "admin.grants.hintReadOnly": "Vilka behörigheter som finns bestäms av de plugins som är installerade. Du kan se dem, men inte ändra dem.", "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.", @@ -63,7 +64,7 @@ const messages: AdminMessages = { "admin.groups.column.name": "Grupp", "admin.groups.create": "Skapa grupp", "admin.groups.delete": "Radera grupp", - "admin.groups.deleteMessage": "Ta bort gruppen {{name}}? Det tar bort gruppen och alla dess medlemskap.", + "admin.groups.deleteMessage": "Ta bort gruppen {{name}}? Det tar bort gruppen, dess medlemmar och de behörigheter den ger dem.", "admin.groups.field.name": "Gruppnamn", "admin.groups.field.nameHint": "Små bokstäver, siffror, bindestreck och understreck.", "admin.groups.filter": "Filtrera grupper", @@ -88,7 +89,6 @@ const messages: AdminMessages = { "admin.notFound.message": "Objektet finns inte.", "admin.notFound.title": "Hittades inte", - "admin.unavailable.message": "{{what}} är inte konfigurerat i den här installationen.", "admin.unavailable.title": "Administrationen är otillgänglig", diff --git a/examples/plugins/admin/views/clients.ejs b/examples/plugins/admin/views/clients.ejs index cc3916e..9749603 100644 --- a/examples/plugins/admin/views/clients.ejs +++ b/examples/plugins/admin/views/clients.ejs @@ -1,6 +1,6 @@ <%# OAuth2 clients admin list: apps that log in *through* us (Hydra). Same building blocks as - the Permissions screen, around the shell, backed by live Hydra OAuth2 clients (admin-clients.ts). + the Groups screen, around the shell, backed by live Hydra OAuth2 clients (admin-clients.ts). %><% const nav = include("partials/nav-tree", { nodes: chrome.nav }); const filters = include("partials/filter-bar", model.filterBar); diff --git a/examples/plugins/admin/views/partials/permission-picker.ejs b/examples/plugins/admin/views/partials/permission-picker.ejs index 7d955a9..12796b8 100644 --- a/examples/plugins/admin/views/partials/permission-picker.ejs +++ b/examples/plugins/admin/views/partials/permission-picker.ejs @@ -7,7 +7,7 @@ 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, readOnly, submit }). + Locals: csrfToken, permissions ({ action, choices, empty, error, field, hint, inheritedNote, legend, pending, readOnly, submit }). %>

<%= permissions.legend %>

diff --git a/src/http/app.test.ts b/src/http/app.test.ts index 92cc98a..dd07608 100644 --- a/src/http/app.test.ts +++ b/src/http/app.test.ts @@ -1238,12 +1238,19 @@ test("admin Groups screen: gate, list, create, detail/membership, delete (CSRF-g await post("/admin/groups/eng/members/delete", `_csrf=${token}&member=user:${grace}`); assert.ok(!tuples.some((tp) => tp.object === "eng" && tp.subject_id === `user:${grace}`)); + // 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`); + 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. assert.match(await (await get("/admin/groups/eng/delete")).text(), /Cancel/); const del = await post("/admin/groups/eng/delete", `_csrf=${token}`); assert.equal(del.status, 303); assert.equal(del.headers.get("location"), "/admin/groups"); assert.ok(!tuples.some((tp) => tp.object === "eng")); + // …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. + assert.ok(!tuples.some((tp) => tp.namespace === "Permission" && tp.subject_set?.object === "eng")); // An invalid group name in the path → 404; malformed %-encoding doesn't 500. assert.equal((await get("/admin/groups/Bad%20Name")).status, 404); @@ -1319,7 +1326,8 @@ test("admin screens render no write affordance for a read-only holder", async (t const kratosAdmin = stubAdmin({ getIdentity: async (id) => identities.find((i) => i.id === id) ?? null, listIdentities: async () => ({ identities, nextPageToken: null }) }); // Hydra is wired so the clients screen renders for real — without it the page is a 503 and the // "no Register button" assertion below would pass without proving anything. - const hydra = stubHydra({ listClients: async () => ({ clients: [{ client_id: "existing", client_name: "Reporting" }], nextPageToken: null }) }); + const reporting = { client_id: "existing", client_name: "Reporting" }; + const hydra = stubHydra({ getClient: async (id) => (id === reporting.client_id ? reporting : null), listClients: async () => ({ clients: [reporting], nextPageToken: null }) }); const { get } = await adminHarness(t, { hydra, keto, kratosAdmin }); const readOnly = ["users:read", "groups:read"]; @@ -1346,6 +1354,10 @@ test("admin screens render no write affordance for a read-only holder", async (t const clients = await clientsRes.text(); assert.match(clients, /Reporting/); // the list is there — that's what :read buys assert.doesNotMatch(clients, /href="\/admin\/clients\/new"/); + // The detail page is where Delete lives, so check it too and not just the list. + const clientDetail = await (await get("/admin/clients/existing", ["oauth2-clients:read"])).text(); + assert.match(clientDetail, /Reporting/); + assert.doesNotMatch(clientDetail, /clients\/existing\/delete/); // A write-intent GET — a create form or a delete-confirm — refuses a reader outright rather than // rendering a form whose submit would 403. diff --git a/todo.md b/todo.md index 445020f..a650da9 100644 --- a/todo.md +++ b/todo.md @@ -2,7 +2,6 @@ ## Unfinnished work -- [ ] Deleting a group orphans its permission grants. `groupsDelete` removes every `Group:#members` tuple, but nothing removes `Permission:X#granted@Group:#members` — so re-creating a group with the same name silently restores every permission it used to hold. Pre-existing (the same orphan was reachable via the old Permissions screen), but granting to a group is now a one-click flow on the group's own page, which raises the odds a lot. Raised by the stability review 2026-08-05. - [ ] Guard the group paths to self-lockout, or accept them explicitly. The self-revoke guard covers only your own *direct* grants on the Users screen; unticking a permission on a group you belong to, removing yourself from that group, or deleting it can all still strip your own effective access with no warning. Same scope the deleted Permissions screen had, and recorded in AGENTS.md as a known gap — the robust fix is a "last effective holder" check, which needs a reverse Keto query. Raised by the stability review 2026-08-05. - [ ] The permission picker has no concurrency baseline, so two operators editing the same user/group silently discard each other's change (standard lost-update on a set-based form — and the natural "two of us are onboarding the new hire" workflow produces exactly it). Sketch: post the rendered set as a hidden baseline; if it no longer matches Keto, re-render with "this changed while you had the page open" rather than applying. Fits the existing "the form is the whole truth" model instead of fighting it. Raised by the product review 2026-08-05. - [ ] A grant whose plugin was uninstalled is invisible and unremovable in the GUI. `grantDiff` deliberately never revokes an undeclared name (so an unrelated save can't drop it), but nothing *shows* it either — so it can't be audited or cleaned, and reinstalling that plugin silently reactivates access nobody remembers granting. Sketch: a read-only "held, but no installed plugin offers this" list with a remove action. Raised by the product review 2026-08-05. @@ -27,8 +26,8 @@ Prioritized. Overall verdict: architecture is sound (contract-first plugin API, functional core/imperative shell, strong test seams); these are refinements. - [ ] **MEDIUM — Add complexity/method-size static analysis to the CI gate.** Only `tsc --strict` today; a size/complexity rule would have caught the `app.ts` growth. Also when wiring CI/CD: keep the merge gate fast (typecheck + units + Ory-free `visual` suite; heavy e2e suites required-but-separate) and make the pipeline the only path to a published image (build once at tag, promote). -- [ ] **MEDIUM — De-duplicate `examples/plugins/admin/admin-groups.ts` and `admin-permissions.ts` (~80% identical).** Same "Keto membership object admin" concept twice; extract a parameterized helper keyed on `{ namespace, base, labels, columns }`, leave permissions' effective-access view as the only delta. Matters extra because this is the reference plugin people copy. -- [ ] **MEDIUM→LOW — Add a list-page view-model helper in `src/ui/`.** Every list screen (users, groups, permissions, shifts) hand-rewrites the same ~40 lines bridging `parseListQuery`/`paginate` to the EJS partials; at minimum a `buildPaginationModel(page, hrefFor)` block. +- [ ] **LOW — The users list offers a pencil "Edit" row action to a `users:read` holder.** The link is harmless (it opens the read-only detail page), but the label contradicts what the reader can do. Needs `canWrite` threaded into `listTable` plus a `common.view` core catalog key and an `i-eye` entry in `ICON_NAMES` — a core registry change for a cosmetic fix, so it was left out of the permission-naming branch. Raised by the stability review 2026-08-05. +- [ ] **MEDIUM→LOW — Add a list-page view-model helper in `src/ui/`.** Every list screen (users, groups, clients, shifts) hand-rewrites the same ~40 lines bridging `parseListQuery`/`paginate` to the EJS partials; at minimum a `buildPaginationModel(page, hrefFor)` block. - [ ] **LOW→MEDIUM — Retire `src/ui/shell-context.ts`.** `ShellModel`/`buildShellContext` has one consumer left (dashboard) and duplicates `PageChrome` on almost every field, incl. identical brand-assembly in `chrome.ts` and `shell-context.ts`. Fold the dashboard onto `ctx.chrome` + title/breadcrumbs; keep `shellUser` as the shared primitive. - [ ] **LOW — Fix stale doc references to removed `docs/plugin-contract.md`** in `views/index.ejs` (user-visible dashboard text; also links /scheduling as if pre-installed) and `examples/plugins/scheduling/views/shifts.ejs`. - [ ] **LOW — Decide (once) on a `ctx.system` facade.** `#plugin-api` exposes raw Ory client shapes, so an Ory client refactor is a major `apiVersion` bump. AGENTS.md accepts this; revisit only if external plugin authors appear. Record the decision.