diff --git a/.changeset/21861-write-through-update-keeps-package.md b/.changeset/21861-write-through-update-keeps-package.md new file mode 100644 index 00000000000..5dc331c303e --- /dev/null +++ b/.changeset/21861-write-through-update-keeps-package.md @@ -0,0 +1,11 @@ +--- +'@objectstack/plugin-security': patch +--- + +A data-door edit of a permission set saved into a writable runtime package updates that set's stored definition instead of forking it + +Clause-②: no + +Setup saves a permission set through the data door (`PATCH /api/v1/data/sys_permission_set/:id`), which redirects the edit into the metadata store. A stored definition row is keyed by its package as well as its name, and the redirected save named no package. For a set saved into a writable runtime package (`PUT /api/v1/meta/permission/:name?package=`), whose only stored row is bound to that package, the save therefore created a second, package-less row carrying the edit and left the package's row untouched: two active definitions for one name, with the package's copy no longer receiving the organization's edits. The save now goes into the package the edited row is bound to, read from that row through the metadata door's own item read, so the edit lands on the set's one row and the row stays in its package. + +Unchanged: a set with no package binding still saves with none, and a set a code package ships is still refused with `403 NOT_OVERRIDABLE` before anything is read or written. The edit answers `200` as before; no error code, route or field moves. diff --git a/packages/plugins/plugin-security/src/permission-set-projection.test.ts b/packages/plugins/plugin-security/src/permission-set-projection.test.ts index 944ef6ee9c2..5391b93df3b 100644 --- a/packages/plugins/plugin-security/src/permission-set-projection.test.ts +++ b/packages/plugins/plugin-security/src/permission-set-projection.test.ts @@ -110,21 +110,30 @@ function makeQl() { * Mock metadata protocol over the ql's sys_metadata table: env-scope active * overlays, layered read (overlay-wins over `declared`), and the ADR-0094 * awaited mutation-projector seam. + * + * [#21861] The package dimension of a row is modelled, because the defect it + * hid lives there. A save targets the ONE row keyed by its `packageId` (absent + * = the package-less row), as `SysMetadataRepository.put` does, so a save that + * names no package beside a package-bound row mints a second row instead of + * quietly overwriting the only one. The reads serve the first env-wide row of + * the name in any package (`findServedOverlayRow` with no package), and the + * single-item read states that row's `package_id` on the item as `_packageId`, + * as `getMetaItem` does. */ function makeProtocol(ql: any, declared: Record = {}) { let projector: ((evt: any) => Promise) | null = null; - const overlayFor = (name: string) => - ql.metaRows.find( - (r: any) => - r.type === 'permission' && r.name === name && r.state === 'active' && (r.organization_id ?? null) === null, - ); + const isActiveEnvRow = (r: any, name: string) => + r.type === 'permission' && r.name === name && r.state === 'active' && (r.organization_id ?? null) === null; + const overlayFor = (name: string) => ql.metaRows.find((r: any) => isActiveEnvRow(r, name)); + const rowAt = (name: string, packageId: string | null) => + ql.metaRows.find((r: any) => isActiveEnvRow(r, name) && (r.package_id ?? null) === packageId); const protocol = { saves: [] as any[], deletes: [] as any[], registerMutationProjector(_type: string, fn: (evt: any) => Promise) { projector = fn; }, - async saveMetaItem(req: { type: string; name: string; item: any; actor?: string }) { + async saveMetaItem(req: { type: string; name: string; item: any; actor?: string; packageId?: string | null }) { // [#6858 / ADR-0094 D5-R] ADR-0005's tier gate, first — ahead of schema // validation, exactly as `protocol.ts` orders it. `declared` IS this // stub's artifact registry, so `declared[name] !== undefined` is the @@ -165,12 +174,15 @@ function makeProtocol(ql: any, declared: Record = {}) { err.status = 422; throw err; } - const existing = overlayFor(req.name); + const packageId = req.packageId ?? null; + const existing = rowAt(req.name, packageId); if (existing) existing.metadata = JSON.stringify(req.item); else { ql.metaRows.push({ - id: `meta_${req.name}`, type: 'permission', name: req.name, state: 'active', - organization_id: null, metadata: JSON.stringify(req.item), + id: packageId === null ? `meta_${req.name}` : `meta_${req.name}@${packageId}`, + type: 'permission', name: req.name, state: 'active', + organization_id: null, ...(packageId === null ? {} : { package_id: packageId }), + metadata: JSON.stringify(req.item), }); } protocol.saves.push({ ...req }); @@ -195,6 +207,20 @@ function makeProtocol(ql: any, declared: Record = {}) { overlayScope: overlay ? 'env' : null, effective: overlay ?? code, }; }, + getMetaItemCalls: 0, + async getMetaItem(req: { type: string; name: string }) { + protocol.getMetaItemCalls += 1; + const o = overlayFor(req.name); + const item = o + ? { ...JSON.parse(o.metadata), ...(o.package_id ? { _packageId: o.package_id } : {}) } + : declared[req.name]; + if (item === undefined) { + const err: any = new Error(`permission/${req.name} not found`); + err.status = 404; + throw err; + } + return { type: 'permission', name: req.name, item }; + }, }; return protocol; } @@ -1071,6 +1097,111 @@ describe('createPermissionSetWriteThrough (data door → metadata store)', () => }); }); +// ── [#21861] The update leg writes back into the row it edits ────────────── + +describe('[#21861] a data-door UPDATE writes back into the stored row it edits — no package-less fork', () => { + const userCtx = { userId: 'usr_admin' }; + const PKG = 'com.acme.runtime'; + + /** Every active stored row of `name`, as its scope, its binding and the description its body carries. */ + const activeRows = (ql: any, name: string) => + ql.metaRows + .filter((r: any) => r.name === name && r.state === 'active') + .map((r: any) => ({ + organization_id: r.organization_id ?? null, + package_id: r.package_id ?? null, + description: JSON.parse(r.metadata).description, + })); + + /** A stored set (one row, optionally package-bound) with its projected record. */ + async function storedSet(packageId?: string) { + const ql = makeQl(); + const protocol = makeProtocol(ql); + registerPermissionSetProjection(protocol, { ql }); + await protocol.saveMetaItem({ + type: 'permission', name: 'support_agent', item: envBody({ name: 'support_agent' }), + ...(packageId ? { packageId } : {}), + }); + protocol.saves.length = 0; + return { ql, protocol, rowId: ql.permRows[0].id as string }; + } + + it('a set stored in a writable runtime package: the edit lands on that row, which stays bound', async () => { + const { ql, protocol, rowId } = await storedSet(PKG); + expect(activeRows(ql, 'support_agent'), 'before').toEqual([{ organization_id: null, package_id: PKG, description: undefined }]); + + const opCtx: any = { + object: 'sys_permission_set', operation: 'update', context: userCtx, + data: { id: rowId, description: 'edited at the data door' }, + }; + expect(await run(makeMiddleware(ql, protocol), opCtx)).toBe(false); + + expect(activeRows(ql, 'support_agent'), 'after: still one row, same binding, carrying the edit') + .toEqual([{ organization_id: null, package_id: PKG, description: 'edited at the data door' }]); + expect(protocol.saves.map((s: any) => s.packageId), 'the save names the row\'s own package').toEqual([PKG]); + expect(opCtx.result?.description, 'the projected record follows').toBe('edited at the data door'); + }); + + it('a package-less set: the save still names no package, and its one row carries the edit', async () => { + const { ql, protocol, rowId } = await storedSet(); + expect(activeRows(ql, 'support_agent'), 'before').toEqual([{ organization_id: null, package_id: null, description: undefined }]); + + await run(makeMiddleware(ql, protocol), { + object: 'sys_permission_set', operation: 'update', context: userCtx, + data: { id: rowId, description: 'edited' }, + }); + + expect(protocol.saves).toHaveLength(1); + expect(protocol.saves[0], 'no binding is invented for a set that has none').not.toHaveProperty('packageId'); + expect(activeRows(ql, 'support_agent'), 'after') + .toEqual([{ organization_id: null, package_id: null, description: 'edited' }]); + }); + + it('a filtered edit spanning a package-bound and a package-less set writes each back into its own row', async () => { + const ql = makeQl(); + const protocol = makeProtocol(ql); + registerPermissionSetProjection(protocol, { ql }); + await protocol.saveMetaItem({ type: 'permission', name: 'pkg_set', item: envBody({ name: 'pkg_set' }), packageId: PKG }); + await protocol.saveMetaItem({ type: 'permission', name: 'org_set', item: envBody({ name: 'org_set' }) }); + protocol.saves.length = 0; + const before = { pkg: activeRows(ql, 'pkg_set'), org: activeRows(ql, 'org_set') }; + expect(before).toEqual({ + pkg: [{ organization_id: null, package_id: PKG, description: undefined }], + org: [{ organization_id: null, package_id: null, description: undefined }], + }); + + await run(makeMiddleware(ql, protocol), { + object: 'sys_permission_set', operation: 'update', context: userCtx, + data: { description: 'bulk' }, options: { where: { name: { $in: ['pkg_set', 'org_set'] } } }, + }); + + expect({ pkg: activeRows(ql, 'pkg_set'), org: activeRows(ql, 'org_set') }, 'after').toEqual({ + pkg: [{ organization_id: null, package_id: PKG, description: 'bulk' }], + org: [{ organization_id: null, package_id: null, description: 'bulk' }], + }); + expect(protocol.saves.map((s: any) => [s.name, s.packageId ?? null])) + .toEqual([['pkg_set', PKG], ['org_set', null]]); + }); + + it('a set a code package ships is still refused by the lock (403 NOT_OVERRIDABLE) before any binding read or save', async () => { + const ql = makeQl(); + const declaredBody = { ...envBody({ name: 'crm_rep' }), _packageId: 'com.example.crm' }; + (ql as any).registry = { listItems: (t: string) => (t === 'permission' ? [declaredBody] : []) }; + const protocol = makeProtocol(ql, { crm_rep: declaredBody }); + ql.permRows.push({ id: 'ps_pkg', name: 'crm_rep', managed_by: 'package', package_id: 'com.example.crm' }); + const before = activeRows(ql, 'crm_rep'); + + await expect(run(makeMiddleware(ql, protocol), { + object: 'sys_permission_set', operation: 'update', context: userCtx, + data: { id: 'ps_pkg', description: 'customized' }, + })).rejects.toMatchObject({ code: 'NOT_OVERRIDABLE', status: 403 }); + + expect(activeRows(ql, 'crm_rep'), 'the refused edit stored nothing').toEqual(before); + expect(protocol.saves).toHaveLength(0); + expect(protocol.getMetaItemCalls, 'the binding is never read for a refused edit').toBe(0); + }); +}); + // ── Boot reconciliation + one-time backfill (ADR-0094 D4) ─────────────────── describe('reconcilePermissionSetProjection', () => { diff --git a/packages/plugins/plugin-security/src/permission-set-projection.ts b/packages/plugins/plugin-security/src/permission-set-projection.ts index 9310106e6d4..e77bd8715a7 100644 --- a/packages/plugins/plugin-security/src/permission-set-projection.ts +++ b/packages/plugins/plugin-security/src/permission-set-projection.ts @@ -1091,6 +1091,47 @@ export function createPermissionSetWriteThrough( } }; + /** + * [#21861] The `saveMetaItem` argument that writes an update back into the + * stored row it edits: `{ packageId }` when that row is bound to a package, + * nothing when it is not. + * + * A `sys_metadata` row is keyed `(org, type, name, package_id)`, and a save + * that names no package targets the package-less row. So an update leg that + * named none, for a set whose only row is bound to a writable runtime + * package (`PUT /meta/permission/:name?package=`), minted a second, + * package-less row carrying the edit beside the untouched package-bound one: + * two active rows for one name. + * + * The row is the one the update merges its patch into — the `overlay` layer + * of `envelope`, which {@link effectiveBodyForRow} takes as the base — and its + * binding is read from the metadata door's own single-item read, which serves + * that row by the same served-row resolution the layered read uses and states + * the row's `package_id` on it as `_packageId`. ⛔ Never from the patch, the + * projected record (the projector writes no binding onto an admin row), or a + * registry item: those are copies, and the row is the fact. + * + * No `overlay` layer means no stored row, so there is nothing to fork and the + * save stays package-less, exactly as before. Every target reaching this read + * has already passed the lock (verdict `org`), so a code-shipped set never + * gets here. A read that fails is not caught: guessing the binding would + * choose which row the save lands in. + */ + const storedRowPackageArg = async ( + protocol: any, + name: string, + envelope: unknown, + ): Promise<{ packageId?: string }> => { + const overlay = (envelope as { overlay?: unknown } | null | undefined)?.overlay; + if (overlay === null || overlay === undefined) return {}; + // A protocol with no single-item read (minimal embeddings, unit-test stubs) + // keys no row by package either. + if (typeof protocol.getMetaItem !== 'function') return {}; + const served = await protocol.getMetaItem({ type: 'permission', name }); + const packageId = served?.item?._packageId; + return typeof packageId === 'string' && packageId !== '' ? { packageId } : {}; + }; + const projectAndFetch = async (protocol: any, name: string): Promise => { // The awaited projector inside saveMetaItem/deleteMetaItem normally did // this already — re-running is an idempotent upsert, and covers the @@ -1368,10 +1409,13 @@ export function createPermissionSetWriteThrough( const rowState = pickRowStateColumns(patch); const results: any[] = []; for (const row of targets) { - const base = await effectiveBodyForRow(protocol, ql, row, layeredByName.get(String(row.name))); + const envelope = layeredByName.get(String(row.name)); + const base = await effectiveBodyForRow(protocol, ql, row, envelope); const body = mergeRowPatchIntoBody(base, patch); body.name = row.name; - await protocol.saveMetaItem({ type: 'permission', name: row.name, item: body, ...actorArg }); + // [#21861] Into the row the base came from — see `storedRowPackageArg`. + const packageArg = await storedRowPackageArg(protocol, String(row.name), envelope); + await protocol.saveMetaItem({ type: 'permission', name: row.name, item: body, ...packageArg, ...actorArg }); // Row state rides along on the same patch but lands on the record, not // in the definition (the projector above never touches these columns). if (rowState) await tryUpdate(ql, 'sys_permission_set', { id: row.id, ...rowState }); diff --git a/packages/qa/dogfood/test/permission-set-write-through-package-binding.dogfood.test.ts b/packages/qa/dogfood/test/permission-set-write-through-package-binding.dogfood.test.ts new file mode 100644 index 00000000000..9697e9514ee --- /dev/null +++ b/packages/qa/dogfood/test/permission-set-write-through-package-binding.dogfood.test.ts @@ -0,0 +1,174 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#21861] A data-door edit of a permission set updates the set's OWN +// `sys_metadata` row. For a set saved into a writable runtime package, that row +// is bound to the package, and the edit must land on it rather than beside it. +// Over the real showcase composition. +// +// ## What was broken +// +// The data door (`PATCH /api/v1/data/sys_permission_set/:id`) redirects a +// definition edit into the metadata store (`createPermissionSetWriteThrough`, +// plugin-security). Its update leg merged the patch into the stored body and +// saved it with no package. A `sys_metadata` row is keyed +// `(org, type, name, package_id)`, and a save that names no package targets the +// package-less row, so for a set whose only row was bound to a runtime package +// the save minted a SECOND, package-less active row carrying the edit, and left +// the package-bound row as it was. Two active rows for one name; the package's +// copy of the set silently stopped receiving the org's edits. +// +// Measured on `origin/main` (88a39c09) through this file's own steps before the +// fix: the runtime-package set's first data-door edit answered `200`, and the +// active rows for its name went from one, bound to the package, to two: the +// untouched package-bound row and a package-less row carrying the edit. +// +// ## The pins hold all three populations +// +// - a set saved into a writable runtime package: `200`, exactly one active row +// before and after, still bound to the package and carrying the edit. Edited +// twice, before and after the list read every Studio page load issues, +// because that read is what made the edit reachable from Setup at all (it +// used to trip the packaged-set lock until the lock read the row's +// provenance); +// - a package-less set: `200`, one package-less row before and after, carrying +// the edit — the fix must not start binding sets that have no package; +// - a set a code package ships: still `403 NOT_OVERRIDABLE`, and the edit +// mints no row — the lock is untouched. +// +// Rows are counted under both type spellings, across every scope, so a fork in +// any of them is visible. Each case counts before and after its edit. + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import showcaseStack from '@objectstack/example-showcase'; +import { bootStack, type VerifyStack } from '@objectstack/verify'; +import { fileURLToPath } from 'node:url'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +/** Package-relative refs resolve against the cwd — see the sibling cold-boot files. */ +const SHOWCASE_DIR = fileURLToPath(new URL('../../../../examples/app-showcase/', import.meta.url)); +const SYS = { context: { isSystem: true } } as const; + +/** A writable runtime package, created through `POST /packages`. */ +const PKG = 'com.dogfood.bind21861'; +/** A set saved into that package through the metadata door. */ +const PKG_SET = 'bind21861_pkgset'; +/** A set the org created through the data door: no package. */ +const ORG_SET = 'bind21861_orgset'; +/** The control: a set the showcase package (`com.example.showcase`) ships. */ +const SHIPPED = 'showcase_contributor'; + +/** Both doors answer the lock's refusal as `{ error: , code }`: the code is top-level. */ +const refusalCode = (json: any): unknown => json?.code; + +interface StoredRow { organization_id: string | null; package_id: string | null; description: unknown } + +describe('[#21861] a data-door edit of a permission set updates its own sys_metadata row (showcase)', () => { + let prevCwd: string; + let dir: string; + let stack: VerifyStack | undefined; + let token: string; + let ql: any; + + /** + * Every ACTIVE stored row for `name`, under either type spelling and in any + * scope, as its scope, its package binding and the description its body + * carries. + */ + const activeRows = async (name: string): Promise => { + const rows: any[] = []; + for (const type of ['permission', 'permissions']) { + rows.push(...await ql.find('sys_metadata', { where: { type, name, state: 'active' }, limit: 10 }, SYS)); + } + return rows.map((r) => { + const body = typeof r.metadata === 'string' ? JSON.parse(r.metadata) : r.metadata; + return { + organization_id: r.organization_id ?? null, + package_id: r.package_id ?? null, + description: body?.description, + }; + }); + }; + + /** A data-door edit of the record, the way Setup saves it. */ + const patchRecord = async (name: string, description: string) => { + const [row] = await ql.find('sys_permission_set', { where: { name }, limit: 1 }, SYS); + expect(row?.id, `the ${name} record exists`).toBeTruthy(); + const res = await stack!.apiAs(token, 'PATCH', `/data/sys_permission_set/${row.id}`, { description }); + return { status: res.status, json: await res.json().catch(() => ({})), id: row.id as string }; + }; + + /** Edit `name` through the data door and assert it landed on its one row, bound to `packageId`. */ + const editLandsOnItsOwnRow = async (name: string, packageId: string | null, description: string) => { + const before = await activeRows(name); + expect(before.map(({ organization_id, package_id }) => ({ organization_id, package_id })), 'before the edit') + .toEqual([{ organization_id: null, package_id: packageId }]); + + const patch = await patchRecord(name, description); + expect(patch.status, JSON.stringify(patch.json)).toBe(200); + + const after = await activeRows(name); + expect(after, 'after the edit: still one active row, same binding, carrying the edit') + .toEqual([{ organization_id: null, package_id: packageId, description }]); + const [record] = await ql.find('sys_permission_set', { where: { id: patch.id }, limit: 1 }, SYS); + expect(record?.description, 'the projected record follows the row').toBe(description); + }; + + beforeAll(async () => { + prevCwd = process.cwd(); + process.chdir(SHOWCASE_DIR); + dir = mkdtempSync(join(tmpdir(), 'dogfood-21861-')); + stack = await bootStack(showcaseStack, { databaseFile: join(dir, 'showcase.db') }); + token = await stack.signIn(); + ql = await stack.kernel.getServiceAsync('objectql'); + + // A writable runtime package, and a set saved into it at the metadata door. + const pkg = await stack.apiAs(token, 'POST', '/packages', { + manifest: { id: PKG, name: 'Write-through binding probe', version: '0.1.0', type: 'app' }, + }); + expect(pkg.status, JSON.stringify(await pkg.clone().json().catch(() => ({})))).toBe(201); + const saved = await stack.apiAs(token, 'PUT', `/meta/permission/${PKG_SET}?package=${PKG}`, { + name: PKG_SET, + label: 'Runtime package set', + objects: { showcase_task: { allowRead: true } }, + }); + expect(saved.status, JSON.stringify(await saved.clone().json().catch(() => ({})))).toBe(200); + + // The org's own set, created through the data door. + const created = await stack.apiAs(token, 'POST', '/data/sys_permission_set', { + name: ORG_SET, + label: 'Org-owned set', + object_permissions: JSON.stringify({ showcase_task: { allowRead: true } }), + }); + expect(created.status, JSON.stringify(await created.clone().json().catch(() => ({})))).toBe(201); + }, 300_000); + + afterAll(async () => { + await stack?.stop(); + if (prevCwd) process.chdir(prevCwd); + if (dir) rmSync(dir, { recursive: true, force: true }); + }); + + it('runtime-package set: the data-door edit answers 200 and lands on its own package-bound row', async () => { + await editLandsOnItsOwnRow(PKG_SET, PKG, 'Edited at the data door'); + }); + + it('runtime-package set: the same holds after the list read every Studio page load issues', async () => { + const list = await stack!.apiAs(token, 'GET', '/meta/permission'); + expect(list.status).toBe(200); + await editLandsOnItsOwnRow(PKG_SET, PKG, 'Edited again, after the list read'); + }); + + it('package-less set: the data-door edit still saves package-less, on its one row', async () => { + await editLandsOnItsOwnRow(ORG_SET, null, 'Org set, edited at the data door'); + }); + + it('control: a set a code package ships is still refused with 403 NOT_OVERRIDABLE, and no row is minted', async () => { + const before = await activeRows(SHIPPED); + const patch = await patchRecord(SHIPPED, 'customized'); + expect({ status: patch.status, code: refusalCode(patch.json) }, JSON.stringify(patch.json)) + .toEqual({ status: 403, code: 'NOT_OVERRIDABLE' }); + expect(await activeRows(SHIPPED), 'the refused edit stored nothing').toEqual(before); + }); +});