Deleting a group revokes the permissions it granted

2026-08-05 15:26:25 +02:00
parent 1787754781
commit edcd9fefc8
11 changed files with 48 additions and 17 deletions
@@ -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
@@ -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
@@ -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", () => {
@@ -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,
@@ -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 };
 });

@@ -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",

@@ -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",

@@ -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);
@@ -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 }).
 %>
  <section class="form-card" aria-labelledby="permissions-h">
    <h2 class="card-title" id="permissions-h"><%= permissions.legend %></h2>
@@ -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.
@@ -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.