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 b30c262..babb24a 100644 --- a/todo.md +++ b/todo.md @@ -28,8 +28,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.