From f4969d5c1fc0707720372f354b04d6518eafa88f Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Sat, 31 Oct 2026 09:34:05 +0100 Subject: [PATCH] Refuse a stale permission save, confirm or report it, show View to read-only holders, keep an absolute return_to beside a locale --- CHANGELOG.md | 25 +++++ docs/reading-cost.md | 17 +++ e2e-tests/full-flow.spec.ts | 11 ++ examples/plugins/admin/admin-grants.test.ts | 37 ++++++- examples/plugins/admin/admin-grants.ts | 37 ++++++- examples/plugins/admin/admin-groups.ts | 52 +++++---- examples/plugins/admin/admin-users.test.ts | 8 ++ examples/plugins/admin/admin-users.ts | 38 ++++--- examples/plugins/admin/i18n/en-US.ts | 3 + examples/plugins/admin/i18n/sv-SE.ts | 3 + .../views/partials/permission-picker.ejs | 9 +- src/auth/routes.ts | 3 +- src/http/app.test.ts | 102 ++++++++++++++++-- src/i18n/locales/en-US.ts | 1 + src/i18n/locales/sv-SE.ts | 1 + src/ui/icons.ts | 1 + views/partials/icons.ejs | 1 + 17 files changed, 303 insertions(+), 46 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8dcc752..e22368c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,31 @@ at boot. Entries start at 0.3.0. ## Unreleased +### The admin example refuses a permission save made from an out-of-date page + +Two administrators editing one user's or group's permissions no longer overwrite each other. The +form carries a token for the grants it showed, and a save made after they changed is refused with +"These permissions changed while you had the page open", re-rendering the current state. A POST with +no token is refused the same way, so a script posting to `/admin/users/:id/permissions` or +`/admin/groups/:name/permissions` must first read the token from the page's `baseline` field. + +### The admin example confirms a permission save and reports a partial one + +A save now lands on the page with "Permissions saved." If the permission service fails midway, the +page says the save stopped part-way and shows what is stored, instead of the generic error page; the +changes applied before the failure still sign the user's live sessions out. + +### A read-only holder sees View, not Edit, on the users list + +A holder of `users:read` without `users:write` gets a "View" row action with an eye icon. The new +`common.view` catalog key is in `en-US` and `sv-SE`; a mounted core catalog must add it. + +### A language chosen on a sign-in link no longer drops an absolute return target + +`/login?return_to=&locale=sv-SE` passed the visitor to the dashboard after sign-in and +dropped the OAuth2 login challenge in `return_to`. The target now reaches Kratos, and the chosen +language is lost across that round-trip instead. + ### Every page carries a smaller icon sprite `i-chart`, `i-copy` and `i-download` are gone from the inlined icon sprite, so a plugin that diff --git a/docs/reading-cost.md b/docs/reading-cost.md index 18c02a0..821295e 100644 --- a/docs/reading-cost.md +++ b/docs/reading-cost.md @@ -122,3 +122,20 @@ Read most (opens and searches): - `public/css/styles.css` 1 - `renovate.json` 1 - `src/e2e-console-guard.test.ts` 1 + +## Reading cost, chunk 2f36d29bd746, finished 2026-10-31T08:19:09Z + +85029 new input tokens before the first commit. + +Read most (opens and searches): + +- `e2e-tests/full-flow.spec.ts` 4 +- `README.md` 3 +- `src/auth/routes.test.ts` 3 +- `todo.md` 3 +- `src/auth/kratos-public.ts` 2 +- `src/http/app.test.ts` 2 +- `ci.sh` 1 +- `compose.yml` 1 +- `e2e-tests/auth-refresh.spec.ts` 1 +- `src/auth/flow-view.ts` 1 diff --git a/e2e-tests/full-flow.spec.ts b/e2e-tests/full-flow.spec.ts index 8f90403..7e6996d 100644 --- a/e2e-tests/full-flow.spec.ts +++ b/e2e-tests/full-flow.spec.ts @@ -150,6 +150,8 @@ test.describe.serial("authenticated admin journey", () => { // Permissions are declared in plugin code, so the group's detail page offers them as a fixed // checkbox list rather than a create form — there is no Permissions screen to visit. await page.goto(`/admin/groups/${group}`); + const staleTab = await watchedPage(page.context()); // opened now, so it holds this page's grants + await staleTab.goto(`/admin/groups/${group}`); const scheduling = page.locator('input[name="permission"][value="scheduling:read"]'); await expect(scheduling).toHaveCount(1); // declared by the reference plugin, so it's on offer await expect(scheduling).not.toBeChecked(); @@ -157,7 +159,16 @@ test.describe.serial("authenticated admin journey", () => { await page.locator('form:has(input[name="permission"]) button[type="submit"]').click(); await expect(page).toHaveURL(new RegExp(`/admin/groups/${group}`)); + await expect(page.getByText("Permissions saved.")).toBeVisible(); await expect(page.locator('input[name="permission"][value="scheduling:read"]')).toBeChecked(); + + // The other tab no longer matches Keto, so its save is refused and shows the grant made above. + await staleTab.locator('input[name="permission"][value="scheduling:write"]').check(); + await staleTab.locator('form:has(input[name="permission"]) button[type="submit"]').click(); + await expect(staleTab.getByText("changed while you had the page open")).toBeVisible(); + await expect(staleTab.locator('input[name="permission"][value="scheduling:read"]')).toBeChecked(); + await expect(staleTab.locator('input[name="permission"][value="scheduling:write"]')).not.toBeChecked(); + await staleTab.close(); }); test("OAuth2 clients CRUD: register a client (writes go to Hydra), see the one-time secret once, then delete it via the confirm step", async () => { diff --git a/examples/plugins/admin/admin-grants.test.ts b/examples/plugins/admin/admin-grants.test.ts index ce01cdc..32899a1 100644 --- a/examples/plugins/admin/admin-grants.test.ts +++ b/examples/plugins/admin/admin-grants.test.ts @@ -3,7 +3,8 @@ import assert from "node:assert/strict"; import { test } from "node:test"; import type { PermissionDecl } from "@plainpages/plugin-api"; -import { buildPermissionPicker, grantDiff, grantTuple, groupSubject, userSubject } from "./admin-grants.ts"; +import type { KetoClient } from "@plainpages/plugin-api"; +import { applyGrants, baselineToken, buildPermissionPicker, grantDiff, grantTuple, groupSubject, userSubject } from "./admin-grants.ts"; const declared: PermissionDecl[] = [ { description: "View users", name: "users:read" }, @@ -81,3 +82,37 @@ test("buildPermissionPicker says so when no plugin declares a permission, rather assert.deepEqual(picker.choices, []); 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]+$/); + assert.equal(baselineToken(declared, ["users:write", "users:read"]), token); + assert.equal(baselineToken(declared, ["users:read", "users:write", "legacy:thing"]), token); // undeclared: never shown, never saved + assert.notEqual(baselineToken(declared, ["users:read"]), token); + assert.notEqual(baselineToken(declared, []), token); + assert.notEqual(baselineToken(declared, []), ""); // an empty set is a state too, not an absent token + assert.equal(buildPermissionPicker({ action: "/x", declared, direct: ["users:read", "users:write"] }).baseline, token); +}); + +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 => ({ + check: async () => false, + deleteTuple: async (tp) => { if (tp.object === failOn) throw new Error("boom"); log.push(`delete ${tp.object}`); }, + expand: async () => ({ type: "leaf" }), + listRelations: async () => ({ nextPageToken: null, tuples: [] }), + writeTuple: async (tp) => { if (tp.object === failOn) throw new Error("boom"); log.push(`write ${tp.object}`); }, + }); + const diff = { grant: ["a:read", "b:read"], revoke: ["c:read"] }; + + const ok = await applyGrants(keto(), userSubject("u1"), diff); + assert.deepEqual(ok, { applied: diff }); + assert.deepEqual(log, ["write a:read", "write b:read", "delete c:read"]); + + log.length = 0; + const failed = await applyGrants(keto("b:read"), userSubject("u1"), diff); + assert.deepEqual(failed.applied, { grant: ["a:read"], revoke: [] }); + assert.ok(failed.error instanceof Error); + assert.deepEqual(log, ["write a:read"]); // nothing after the failure was attempted +}); diff --git a/examples/plugins/admin/admin-grants.ts b/examples/plugins/admin/admin-grants.ts index ab7219e..232decc 100644 --- a/examples/plugins/admin/admin-grants.ts +++ b/examples/plugins/admin/admin-grants.ts @@ -6,11 +6,14 @@ // what the installed plugins declare in code. Nothing here invents a name, which is why the old // Permissions screen is gone: a grant is a property of a user or a group, edited where they are. +import { createHash } from "node:crypto"; import type { KetoClient, PermissionDecl, RelationTuple, SubjectSet, Translate } from "@plainpages/plugin-api"; const PERMISSION_NS = "Permission"; const GRANTED = "granted"; export const PERMISSIONS_FIELD = "permission"; // the checkbox name the two forms post +export const BASELINE_FIELD = "baseline"; // the hidden field carrying baselineToken +export const SAVED_NOTICE = "permissions-saved"; // the `info-msg` a successful save redirects with export type GrantSubject = { subject_id: string } | { subject_set: SubjectSet }; @@ -53,6 +56,7 @@ export interface PermissionChoice { export interface PermissionPicker { action: string; + baseline: string; // baselineToken of the direct grants this form was rendered from choices: PermissionChoice[]; empty: string | undefined; // set when no plugin declares a permission — the picker has nothing to offer error?: string; // a rejected save (e.g. the self-revoke guard), rendered above the list @@ -60,6 +64,7 @@ export interface PermissionPicker { hint: string; inheritedNote: string | undefined; // set when at least one choice is group-held, to explain the disabled row legend: string; + notice?: string; // a confirmation, rendered above the list // Set for a group: its members hold these transitively, so a change reaches them at their next // re-mint rather than at once. The user picker revokes live tokens, so it says nothing. pending: string | undefined; @@ -90,6 +95,7 @@ export function buildPermissionPicker(opts: { })); return { action: opts.action, + baseline: baselineToken(opts.declared, opts.direct), choices, empty: opts.declared.length === 0 ? t("admin.grants.none") : undefined, field: PERMISSIONS_FIELD, @@ -107,7 +113,7 @@ export function buildPermissionPicker(opts: { // What a submitted set changes. Pure so the diff is testable without Keto: only declared names are // considered, so a crafted POST cannot grant something no plugin gates on, and a held-but-undeclared // name (left over from an uninstalled plugin) is never silently revoked by an unrelated save. -export function grantDiff(declared: readonly PermissionDecl[], held: string[], wanted: string[]): { grant: string[]; revoke: string[] } { +export function grantDiff(declared: readonly PermissionDecl[], held: string[], wanted: string[]): GrantChange { const offered = new Set(declared.map((d) => d.name)); const heldSet = new Set(held); const wantedSet = new Set(wanted.filter((name) => offered.has(name))); @@ -117,7 +123,30 @@ export function grantDiff(declared: readonly PermissionDecl[], held: string[], w }; } -export async function applyGrants(keto: KetoClient, subject: GrantSubject, diff: { grant: string[]; revoke: string[] }): Promise { - for (const name of diff.grant) await keto.writeTuple(grantTuple(name, subject)); - for (const name of diff.revoke) await keto.deleteTuple(grantTuple(name, subject)); +// 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. +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); + +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. +export async function applyGrants(keto: KetoClient, subject: GrantSubject, diff: GrantChange): Promise<{ applied: GrantChange; error?: unknown }> { + const applied: GrantChange = { grant: [], revoke: [] }; + try { + for (const name of diff.grant) { await keto.writeTuple(grantTuple(name, subject)); applied.grant.push(name); } + for (const name of diff.revoke) { await keto.deleteTuple(grantTuple(name, subject)); applied.revoke.push(name); } + } catch (error) { + return { applied, error }; + } + return { applied }; } diff --git a/examples/plugins/admin/admin-groups.ts b/examples/plugins/admin/admin-groups.ts index 67d89f0..230e8b3 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 "@plainpages/plugin-api"; -import { applyGrants, buildPermissionPicker, effectivePermissions, grantDiff, grantTuple, groupSubject, heldPermissions, type PermissionPicker, PERMISSIONS_FIELD } from "./admin-grants.ts"; +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 type { FieldConfig } from "./admin-users.ts"; @@ -347,36 +347,50 @@ export const groupsCreate = withGroups(async (deps) => { // GET /admin/groups/new — the create form. export const groupsNewForm = withGroups((deps) => groupFormResult(deps, {}), "write"); -// GET /admin/groups/:name — the detail + membership page. -export const groupsDetail = withGroupName(async ({ ctx, keto, kratosAdmin }, name) => { +// The detail + membership page. `notice` and `error` ride the permission picker, which is where a save reports. +async function groupDetailPage({ ctx, keto, kratosAdmin }: GroupsDeps, name: string, say: { error?: string; notice?: string } = {}): Promise { const { emailById, options } = await memberCandidates(keto, kratosAdmin); const members = (await pagedTuples(keto, { namespace: GROUP_NS, object: name, relation: MEMBERS })).map((t) => memberView(t, emailById)); const subject = groupSubject(name); const [direct, effective] = await Promise.all([heldPermissions(keto, subject), effectivePermissions(keto, subject, ctx.declaredPermissions)]); - const permissions = buildPermissionPicker({ - action: `${detailHref(name)}/permissions`, - declared: ctx.declaredPermissions, - direct, - effective, // a group nested in another group inherits its permissions too - readOnly: !can(ctx, permissionName("groups", "write")), - t: ctx.t, - transitive: true, // members inherit, so a change here lands at their next re-mint, not at once - }); + const permissions: PermissionPicker = { + ...buildPermissionPicker({ + action: `${detailHref(name)}/permissions`, + declared: ctx.declaredPermissions, + direct, + effective, // a group nested in another group inherits its permissions too + readOnly: !can(ctx, permissionName("groups", "write")), + t: ctx.t, + transitive: true, // members inherit, so a change here lands at their next re-mint, not at once + }), + ...(say.error ? { error: say.error } : {}), + ...(say.notice ? { notice: say.notice } : {}), + }; 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. +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") } : {})); // 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 — // the documented instant-revoke tradeoff for anything held through a group. -export const groupsPermissions = withGroupName(async ({ ctx, keto, user }, name) => { +export const groupsPermissions = withGroupName(async (deps, name) => { + const { ctx, keto, user } = deps; const form = (await guardedForm(ctx))!; const subject = groupSubject(name); - const diff = grantDiff(ctx.declaredPermissions, await heldPermissions(keto, subject), form.getAll(PERMISSIONS_FIELD)); - await applyGrants(keto, subject, diff); - if (diff.grant.length > 0 || diff.revoke.length > 0) { - ctx.log.info("admin: group permissions changed", { actor: user.id, granted: diff.grant.join(","), group: name, revoked: diff.revoke.join(",") }); + 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))); + 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(",") }); } - return { redirect: detailHref(name) }; + 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 { redirect: `${detailHref(name)}?info-msg=${SAVED_NOTICE}` }; }); // POST /admin/groups/:name/members — add a member (skip an invalid member or a self-nest). diff --git a/examples/plugins/admin/admin-users.test.ts b/examples/plugins/admin/admin-users.test.ts index bafce03..cdfdd63 100644 --- a/examples/plugins/admin/admin-users.test.ts +++ b/examples/plugins/admin/admin-users.test.ts @@ -60,6 +60,14 @@ test("buildUsersListModel filters by search + status, sorts, and paginates", () // Edit link points at the per-identity route. assert.match(all.table.rows[0]!.actions![0]!.href!, new RegExp(`/admin/users/${people[0]!.id}$`)); + assert.deepEqual([all.table.rows[0]!.actions![0]!.icon, all.table.rows[0]!.actions![0]!.label], ["i-edit", "Edit"]); +}); + +// The detail page of a `users:read` holder is read-only, so the row action must not promise an edit. +test("buildUsersListModel: a read-only holder's row action says View, with the same target", () => { + const people: Identity[] = [{ id: "u1", traits: { email: "a@x.io" } }]; + const action = buildUsersListModel({ canWrite: false, identities: people, url: "http://x/admin/users" }).table.rows[0]!.actions![0]!; + assert.deepEqual([action.icon, action.label, action.href], ["i-eye", "View", "/admin/users/u1"]); }); test("buildUserFormModel: create mode has an editable email + password, no edit actions", () => { diff --git a/examples/plugins/admin/admin-users.ts b/examples/plugins/admin/admin-users.ts index 00e3d73..e60c2d6 100644 --- a/examples/plugins/admin/admin-users.ts +++ b/examples/plugins/admin/admin-users.ts @@ -4,7 +4,7 @@ // 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, type PermissionPicker, PERMISSIONS_FIELD, userSubject } from "./admin-grants.ts"; +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"; const SCHEMA_ID = "default"; // matches kratos.yml identity.default_schema_id @@ -138,12 +138,12 @@ export function buildUsersListModel(opts: { canWrite: opts.canWrite !== false, filterBar: listFilterBar(state, all.length, t), pagination: listPagination(state, page, t), - table: listTable(rows, state, sort, t), + table: listTable(rows, state, sort, t, opts.canWrite !== false), title: t("admin.users.title"), }; } -function listTable(rows: UserView[], state: ListState, sort: { dir: "asc" | "desc"; field: string } | null, t: Translate) { +function listTable(rows: UserView[], state: ListState, sort: { dir: "asc" | "desc"; field: string } | null, t: Translate, canWrite: boolean) { return { actions: true, caption: t("admin.users.title"), @@ -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: "i-edit", label: t("common.edit") }], + actions: [{ href: `${ADMIN_USERS_BASE}/${encodeURIComponent(u.id)}`, icon: canWrite ? "i-edit" : "i-eye", label: t(canWrite ? "common.edit" : "common.view") }], cells: [ { user: { initials: u.initials, name: u.name } }, u.email, @@ -329,7 +329,9 @@ 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) => { - const permissions = await userPermissionPicker(deps, 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); return formResult(deps.ctx, { canWrite: canWriteUsers(deps.ctx), identity, ...(permissions ? { permissions } : {}) }); }); @@ -338,7 +340,7 @@ const canWriteUsers = (ctx: RequestContext): boolean => can(ctx, permissionName( // The checkbox list of declared permissions: ticked where this user holds one, and disabled where // the grant comes from a group (real, but removed on that group). Undefined when Keto isn't wired — // the rest of the edit page still works. -async function userPermissionPicker(deps: UsersDeps, id: string, error?: string): Promise { +async function userPermissionPicker(deps: UsersDeps, id: string, error?: string, notice?: string): Promise { if (!deps.keto) return undefined; const subject = userSubject(id); const [direct, effective] = await Promise.all([ @@ -355,6 +357,7 @@ async function userPermissionPicker(deps: UsersDeps, id: string, error?: string) t: deps.ctx.t, }), ...(error ? { error } : {}), + ...(notice ? { notice } : {}), }; } @@ -366,21 +369,30 @@ export const usersPermissions = withTarget(async (deps, identity, id) => { const form = (await guardedForm(ctx))!; if (!keto) return unavailable(ctx, ctx.t("admin.capability.keto")); const subject = userSubject(id); - const diff = grantDiff(ctx.declaredPermissions, await heldPermissions(keto, subject), form.getAll(PERMISSIONS_FIELD)); + const held = await heldPermissions(keto, subject); + const refuse = async (error: string, status: number): Promise => { + 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)); // 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. if (id === user.id && diff.revoke.length > 0) { ctx.log.warn("admin: refused a self-revoke of permissions", { actor: user.id, refused: diff.revoke.join(",") }); - const permissions = await userPermissionPicker(deps, id, ctx.t("admin.grants.selfRevoke")); - return { ...formResult(ctx, { canWrite: canWriteUsers(ctx), identity, ...(permissions ? { permissions } : {}) }), status: 400 }; + return refuse(ctx.t("admin.grants.selfRevoke"), 400); } - await applyGrants(keto, subject, diff); - if (diff.grant.length > 0 || diff.revoke.length > 0) { + const { applied, error } = await applyGrants(keto, subject, diff); + if (applied.grant.length > 0 || applied.revoke.length > 0) { revoke?.(id); - ctx.log.info("admin: user permissions changed", { actor: user.id, granted: diff.grant.join(","), revoked: diff.revoke.join(","), target: id }); + ctx.log.info("admin: user permissions changed", { actor: user.id, granted: applied.grant.join(","), revoked: applied.revoke.join(","), target: id }); } - return { redirect: `${ADMIN_USERS_BASE}/${encodeURIComponent(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 { redirect: `${ADMIN_USERS_BASE}/${encodeURIComponent(id)}?info-msg=${SAVED_NOTICE}` }; }); // POST /admin/users/:id — save edits; a Kratos 4xx re-renders the form (400). diff --git a/examples/plugins/admin/i18n/en-US.ts b/examples/plugins/admin/i18n/en-US.ts index c885ce6..52473cb 100644 --- a/examples/plugins/admin/i18n/en-US.ts +++ b/examples/plugins/admin/i18n/en-US.ts @@ -53,9 +53,12 @@ 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.pending": "Members get this at their next sign-in (up to 10 minutes).", "admin.grants.save": "Save permissions", + "admin.grants.saved": "Permissions saved.", "admin.grants.selfRevoke": "You can't revoke your own permissions — ask another administrator, so you can't lock yourself out.", + "admin.grants.stale": "These permissions changed while you had the page open, so nothing was saved. The list now shows the current state. Review it and save again.", "admin.groups.actions": "Group actions", "admin.groups.addMember": "Add a member", diff --git a/examples/plugins/admin/i18n/sv-SE.ts b/examples/plugins/admin/i18n/sv-SE.ts index 03b2348..a170713 100644 --- a/examples/plugins/admin/i18n/sv-SE.ts +++ b/examples/plugins/admin/i18n/sv-SE.ts @@ -53,9 +53,12 @@ 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.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.", "admin.grants.selfRevoke": "Du kan inte återkalla dina egna behörigheter — be en annan administratör, så att du inte låser ute dig själv.", + "admin.grants.stale": "Behörigheterna ändrades medan du hade sidan öppen, så ingenting sparades. Listan visar nu aktuellt läge. Granska den och spara igen.", "admin.groups.actions": "Gruppåtgärder", "admin.groups.addMember": "Lägg till en medlem", diff --git a/examples/plugins/admin/views/partials/permission-picker.ejs b/examples/plugins/admin/views/partials/permission-picker.ejs index 12796b8..9414aaf 100644 --- a/examples/plugins/admin/views/partials/permission-picker.ejs +++ b/examples/plugins/admin/views/partials/permission-picker.ejs @@ -7,13 +7,19 @@ 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, pending, readOnly, submit }). + 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 }). %>

<%= permissions.legend %>

<% if (permissions.error) { -%> <%- include("partials/alert", { text: permissions.error, tone: "neg" }) %> <% } -%> +<% if (permissions.notice) { -%> +<%- include("partials/alert", { text: permissions.notice, tone: "pos" }) %> +<% } -%> <% if (permissions.empty) { -%>

<%= permissions.empty %>

<% } else { -%> @@ -28,6 +34,7 @@ <% } else { -%>
+
<%= permissions.legend %> <% permissions.choices.forEach((c) => { -%> diff --git a/src/auth/routes.ts b/src/auth/routes.ts index 3354b81..ef764f4 100644 --- a/src/auth/routes.ts +++ b/src/auth/routes.ts @@ -46,7 +46,8 @@ function flowReturnTo(ctx: RequestContext, secureCookies: boolean): string | und const raw = ctx.url.searchParams.get("return_to"); const local = localPath(raw); const chosen = chosenLocale(ctx); - if (!local && !chosen) return raw ?? undefined; + if (raw && !local) return raw; // beside a chosen language too: the target wins, the language is lost + if (!local && !chosen) return undefined; const complete = new URL(`${selfOrigin(ctx, secureCookies)}/auth/complete`); if (local) complete.searchParams.set("return_to", ctx.localeHref(local)); if (chosen) complete.searchParams.set("locale", chosen); diff --git a/src/http/app.test.ts b/src/http/app.test.ts index a3cbb59..13e5af8 100644 --- a/src/http/app.test.ts +++ b/src/http/app.test.ts @@ -855,6 +855,11 @@ test("login return_to: a chosen language alone still routes through /auth/comple assert.match(lastReturnTo ?? "", /^http:\/\/[^/]+\/auth\/complete\?locale=sv-SE$/); await fetch(url + "/login?locale=sv-SE&return_to=" + encodeURIComponent("/admin/users"), { redirect: "manual" }); assert.match(lastReturnTo ?? "", /^http:\/\/[^/]+\/auth\/complete\?return_to=%2Fadmin%2Fusers%3Flocale%3Dsv-SE&locale=sv-SE$/); + + // An absolute target still reaches Kratos beside a chosen language; the language is lost instead. + const abs = "http://localhost/oauth2/login?login_challenge=abc"; + await fetch(url + "/login?locale=sv-SE&return_to=" + encodeURIComponent(abs), { redirect: "manual" }); + assert.equal(lastReturnTo, abs); }); // "Ory down ⇒ no logins" is documented; the auth path should say so honestly (503), not the @@ -953,6 +958,8 @@ async function adminHarness(t: TestContext, opts: AppOptions = {}) { // What the plugin itself declares — the harness holds every screen's read and write, so a screen // test exercises the screen rather than the gate. assertAdminGate covers the refusals. const ADMIN_ALL = (adminManifest.permissions ?? []).map((p) => p.name); +// The permission form's concurrency token, read off the page the way a browser would post it back. +const baselineOf = (html: string): string => /name="baseline" value="([0-9a-f]+)"/.exec(html)?.[1] ?? ""; // Every admin route is gated: anonymous → /login, a signed-in non-admin → 403. async function assertAdminGate(url: string, get: (path: string, permissions?: string[]) => Promise, path: string) { const anon = await fetch(url + path, { redirect: "manual" }); @@ -1313,7 +1320,8 @@ test("admin Groups screen: gate, list, create, detail/membership, delete (CSRF-g 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`); + const permissionsBaseline = baselineOf(await (await get("/admin/groups/eng")).text()); + await post("/admin/groups/eng/permissions", `_csrf=${token}&baseline=${permissionsBaseline}&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. @@ -1349,9 +1357,11 @@ test("admin permission grants: the picker offers the declared catalog, and a sav } assert.match(edit, /value="users:read"[^>]*checked/); // held → ticked assert.doesNotMatch(edit, /value="groups:write"[^>]*checked/); // not held → unticked + assert.doesNotMatch(edit, /Permissions saved/); // no confirmation before a save // Save a new set: users:write is added, users:read is dropped — the POST is the whole truth. - const saved = await post(`/admin/users/${ada}/permissions`, `_csrf=${token}&permission=users%3Awrite&permission=groups%3Aread`); + const baseline = baselineOf(edit); + const saved = await post(`/admin/users/${ada}/permissions`, `_csrf=${token}&baseline=${baseline}&permission=users%3Awrite&permission=groups%3Aread`); assert.equal(saved.status, 303); assert.deepEqual( tuples.filter((tp) => tp.subject_id === `user:${ada}`).map((tp) => tp.object).sort(), @@ -1359,16 +1369,93 @@ test("admin permission grants: the picker offers the declared catalog, and a sav ); assert.equal(denylist.isRevoked(ada, 0), true); // a change to your own grants revokes live tokens + // The redirect lands on a page that confirms the save, and shows the new state. + const landing = saved.headers.get("location") ?? ""; + assert.match(landing, new RegExp(`^/admin/users/${ada}\\?info-msg=permissions-saved$`)); + const confirmed = await (await get(landing)).text(); + assert.match(confirmed, /Permissions saved/); + assert.match(confirmed, /value="users:write"[^>]*checked/); + // A crafted POST can't grant something no plugin declares. - await post(`/admin/users/${ada}/permissions`, `_csrf=${token}&permission=users%3Awrite&permission=superuser%3Aall`); + await post(`/admin/users/${ada}/permissions`, `_csrf=${token}&baseline=${baselineOf(confirmed)}&permission=users%3Awrite&permission=superuser%3Aall`); assert.ok(!tuples.some((tp) => tp.object === "superuser:all")); // The same picker on a group writes the group's subject_set, which Keto resolves transitively. tuples.push({ namespace: "Group", object: "eng", relation: "members", subject_id: `user:${ada}` }); - await post("/admin/groups/eng/permissions", `_csrf=${token}&permission=groups%3Aread`); + const groupBaseline = baselineOf(await (await get("/admin/groups/eng")).text()); + const groupSaved = await post("/admin/groups/eng/permissions", `_csrf=${token}&baseline=${groupBaseline}&permission=groups%3Aread`); + assert.equal(groupSaved.headers.get("location"), "/admin/groups/eng?info-msg=permissions-saved"); assert.ok(tuples.some((tp) => tp.namespace === "Permission" && tp.object === "groups:read" && tp.subject_set?.object === "eng")); }); +// Two admins open the same page; the second save would silently undo the first. The form carries a +// token for the grants it was rendered from, and a save made from any other state is refused. +test("admin permission grants: a save from a stale page is refused, and the page re-renders the current state", async (t) => { + const ada = randomUUID(); + const identities: Identity[] = [{ id: ada, traits: { email: "ada@example.com" } }]; + const tuples: RelationTuple[] = [{ namespace: "Permission", object: "users:read", relation: "granted", subject_id: `user:${ada}` }]; + const keto = fakeKeto(tuples); + const kratosAdmin = stubAdmin({ getIdentity: async (id) => identities.find((i) => i.id === id) ?? null, listIdentities: async () => ({ identities, nextPageToken: null }) }); + const { get, post, token } = await adminHarness(t, { keto, kratosAdmin }); + const stale = baselineOf(await (await get(`/admin/users/${ada}`)).text()); + + // Another admin grants users:write while this page is open. + tuples.push({ namespace: "Permission", object: "users:write", relation: "granted", subject_id: `user:${ada}` }); + const before = JSON.stringify(tuples); + const refused = await post(`/admin/users/${ada}/permissions`, `_csrf=${token}&baseline=${stale}&permission=users%3Aread&permission=groups%3Aread`); + assert.equal(refused.status, 409); + const page = await refused.text(); + assert.match(page, /changed while you had the page open/); + assert.equal(JSON.stringify(tuples), before, "nothing was applied"); + assert.match(page, /value="users:write"[^>]*checked/); // the re-render shows the other admin's grant + assert.notEqual(baselineOf(page), stale); + + // Saving from the re-rendered page goes through. + const retried = await post(`/admin/users/${ada}/permissions`, `_csrf=${token}&baseline=${baselineOf(page)}&permission=users%3Aread&permission=users%3Awrite&permission=groups%3Aread`); + assert.equal(retried.status, 303); + assert.ok(tuples.some((tp) => tp.object === "groups:read" && tp.subject_id === `user:${ada}`)); + + // A POST with no token at all (a crafted request, or a form rendered before this check existed) is stale too. + assert.equal((await post(`/admin/users/${ada}/permissions`, `_csrf=${token}&permission=users%3Aread`)).status, 409); + + // Groups are checked the same way. + tuples.push({ namespace: "Group", object: "eng", relation: "members", subject_id: `user:${ada}` }); + const groupStale = baselineOf(await (await get("/admin/groups/eng")).text()); + tuples.push({ namespace: "Permission", object: "groups:read", relation: "granted", subject_set: { namespace: "Group", object: "eng", relation: "members" } }); + const groupRefused = await post("/admin/groups/eng/permissions", `_csrf=${token}&baseline=${groupStale}&permission=users%3Aread`); + assert.equal(groupRefused.status, 409); + assert.match(await groupRefused.text(), /changed while you had the page open/); + assert.ok(tuples.some((tp) => tp.object === "groups:read" && tp.subject_set?.object === "eng"), "the group keeps its grant"); +}); + +// applyGrants is not a transaction: a Keto error midway leaves some changes applied. The save says +// so, instead of the generic error page, and still revokes the tokens of what did change. +test("admin permission grants: a Keto failure midway reports a partial save and keeps what was applied", async (t) => { + const ada = randomUUID(); + const identities: Identity[] = [{ id: ada, traits: { email: "ada@example.com" } }]; + const tuples: RelationTuple[] = []; + let writes = 0; + const keto = fakeKeto(tuples, { + writeTuple: async (tp) => { + if (++writes === 2) throw new Error("keto unreachable"); + tuples.push(tp); + }, + }); + const kratosAdmin = stubAdmin({ getIdentity: async (id) => identities.find((i) => i.id === id) ?? null, listIdentities: async () => ({ identities, nextPageToken: null }) }); + const denylist = createDenylist(); + const { get, post, token } = await adminHarness(t, { denylist, keto, kratosAdmin }); + const baseline = baselineOf(await (await get(`/admin/users/${ada}`)).text()); + + const res = await post(`/admin/users/${ada}/permissions`, `_csrf=${token}&baseline=${baseline}&permission=groups%3Aread&permission=users%3Aread&permission=users%3Awrite`); + assert.equal(res.status, 502); + const page = await res.text(); + assert.match(page, /stopped part-way/); + assert.match(page, /value="groups:read"[^>]*checked/); // the first write landed, and the page shows what is stored + assert.doesNotMatch(page, /value="users:write"[^>]*checked/); + assert.deepEqual(tuples.map((tp) => tp.object), ["groups:read"]); + assert.equal(denylist.isRevoked(ada, 0), true); // the half that was applied still revokes live tokens +}); + // Revoking your own grants can remove the last users:write on the deployment, and the instant-revoke // hook lands it on the next request — recovery would be a curl against Keto. Guarded like // self-deactivate and self-delete are. (`admin1` is the harness's own sub.) @@ -1377,15 +1464,16 @@ test("admin permission grants: you can't revoke your own permissions, but you ca const tuples: RelationTuple[] = [{ namespace: "Permission", object: "users:write", relation: "granted", subject_id: "user:admin1" }]; const keto = fakeKeto(tuples); const kratosAdmin = stubAdmin({ getIdentity: async (id) => identities.find((i) => i.id === id) ?? null, listIdentities: async () => ({ identities, nextPageToken: null }) }); - const { post, token } = await adminHarness(t, { keto, kratosAdmin }); + const { get, post, token } = await adminHarness(t, { keto, kratosAdmin }); + const baseline = baselineOf(await (await get("/admin/users/admin1")).text()); - const refused = await post("/admin/users/admin1/permissions", `_csrf=${token}`); // every box cleared + const refused = await post("/admin/users/admin1/permissions", `_csrf=${token}&baseline=${baseline}`); // every box cleared assert.equal(refused.status, 400); assert.match(await refused.text(), /lock yourself out/); assert.ok(tuples.some((tp) => tp.object === "users:write" && tp.subject_id === "user:admin1"), "nothing was revoked"); // Granting yourself more is not a lockout, so it goes through. - const granted = await post("/admin/users/admin1/permissions", `_csrf=${token}&permission=users%3Awrite&permission=groups%3Aread`); + const granted = await post("/admin/users/admin1/permissions", `_csrf=${token}&baseline=${baseline}&permission=users%3Awrite&permission=groups%3Aread`); assert.equal(granted.status, 303); assert.ok(tuples.some((tp) => tp.object === "groups:read" && tp.subject_id === "user:admin1")); }); diff --git a/src/i18n/locales/en-US.ts b/src/i18n/locales/en-US.ts index 471eb78..37a85d7 100644 --- a/src/i18n/locales/en-US.ts +++ b/src/i18n/locales/en-US.ts @@ -46,6 +46,7 @@ const messages = { "common.edit": "Edit", "common.new": "New", "common.remove": "Remove", + "common.view": "View", "consent.allow": "Allow", "consent.deny": "Deny", diff --git a/src/i18n/locales/sv-SE.ts b/src/i18n/locales/sv-SE.ts index 85487eb..59a701d 100644 --- a/src/i18n/locales/sv-SE.ts +++ b/src/i18n/locales/sv-SE.ts @@ -37,6 +37,7 @@ const messages: CoreMessages = { "common.edit": "Redigera", "common.new": "Ny", "common.remove": "Ta bort", + "common.view": "Visa", "consent.allow": "Tillåt", "consent.deny": "Neka", diff --git a/src/ui/icons.ts b/src/ui/icons.ts index 8853ebf..58ff47c 100644 --- a/src/ui/icons.ts +++ b/src/ui/icons.ts @@ -13,6 +13,7 @@ export const ICON_NAMES: Record = { "i-chev": "chevron-right", "i-cols": "columns-3", "i-edit": "pencil", + "i-eye": "eye", "i-globe": "globe", "i-grid": "layout-grid", "i-kebab": "ellipsis-vertical", diff --git a/views/partials/icons.ejs b/views/partials/icons.ejs index f53a6c2..330bcbc 100644 --- a/views/partials/icons.ejs +++ b/views/partials/icons.ejs @@ -9,6 +9,7 @@ +