diff --git a/.changeset/permission-backfill-row-state-columns.md b/.changeset/permission-backfill-row-state-columns.md new file mode 100644 index 0000000000..8c7a5e6753 --- /dev/null +++ b/.changeset/permission-backfill-row-state-columns.md @@ -0,0 +1,31 @@ +--- +"@objectstack/plugin-security": patch +--- + +fix(security): 让 permission-set 投影只写 spec 认的键，并把静默失败的 backfill 变响亮 (#4669) + +ADR-0094 D4 的 permission-set backfill 在 #4001 之后 **100% 失败**：`sys_permission_set` +每一行都有 `active` 存储列，`permissionSetBodyFromRow()` 把整行转成 metadata body 时把它 +一起带上，而 #4001 已经把 `PermissionSetSchema` 封成 `.strict()` —— 于是每一次 +`saveMetaItem` 都抛 `[invalid_metadata] … Unrecognized key(s) on this permission set: +'active'`。失败被 `catch` 成一条 `warn`、计数器不加一，所以测试全绿、没有任何自动信号： +一个整条停摆的投影路径就这样过了一个发布周期。 + +**归属判定：`active` 是行状态，不是声明。** 它的全部消费面 —— 表列、`highlightFields`、 +Setup 列表视图的过滤器、两个启停动作的 `bodyExtra: { active: … }` —— 都是记录的运行时开关， +不是作者声明的能力边界。所以修法是在**投影侧挑键**，而不是把状态提升进 spec +（`packages/spec/**` 零改动）。 + +- `permissionSetBodyFromRow()` / `mergeRowPatchIntoBody()` 现在都经过一个**从 + `PermissionSetSchema.shape` 派生**的键白名单（不是手抄的字符串数组 —— 手抄的话 spec 加键 + 时这里又会静默漏，正是本 bug 的翻版）。存储列（`active`、时间戳、`managed_by` / + `package_id` / `customized`）一律不进 metadata body；`#4001` 之前**已经落库**、body 里 + 仍带着 `active` 的历史 overlay 行，也在同一个闸口被滤掉，因此它们的数据门编辑不再报 422。 +- 两个启停动作行为不变：只含行状态的 PATCH 不再被改写成 metadata 写入，而是原样交给驱动 + 执行列写入（保留 history / `updated_at` / FLS 等正常语义），并且不会再给一个包自带的 + permission set 平白造出一条“customization” overlay。投影通道则不再从 body 读 `active` —— + 一次投影不会再用陈旧 body 把管理员刚停用的 set 重新打开。 +- backfill 真失败时按 AGENTS.md「Degradation log levels」(#4632) 变响亮：`error` 级、 + 文案写明后果（记录照常列出、看起来一切正常，但定义不在 metadata 里，重新 provision 不会 + 重建它）与修复动作，并新增 `ProjectionReconcileOutcome.backfillFailed` 计数，让降级出现在 + 结果里而不只在日志里。 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 d8592cf6bd..59566c2ae0 100644 --- a/packages/plugins/plugin-security/src/permission-set-projection.test.ts +++ b/packages/plugins/plugin-security/src/permission-set-projection.test.ts @@ -9,9 +9,12 @@ */ import { describe, it, expect } from 'vitest'; +import { PermissionSetSchema } from '@objectstack/spec/security'; import { permissionSetRowFields, permissionSetBodyFromRow, + permissionSpecBodyKeys, + pickRowStateColumns, mergeRowPatchIntoBody, recordDiffersFromBody, upsertEnvPermissionSet, @@ -81,6 +84,21 @@ function makeProtocol(ql: any, declared: Record = {}) { projector = fn; }, async saveMetaItem(req: { type: string; name: string; item: any; actor?: string }) { + // [#4669] The REAL `PermissionSetSchema`, exactly as `saveMetaItem` runs + // it (metadata-protocol/src/protocol.ts → `resolveOverlaySchema`), same + // `[invalid_metadata]` 422 envelope. Without this the mock accepts any + // object and the suite stays green while every real backfill fails — + // which is how a 100%-failing projection shipped. + const parsed = PermissionSetSchema.safeParse(req.item); + if (!parsed.success) { + const summary = parsed.error.issues + .map((i: any) => `${i.path.join('.') || ''}: ${i.message}`) + .join('; '); + const err: any = new Error(`[invalid_metadata] permission/${req.name} failed spec validation: ${summary}`); + err.code = 'INVALID_METADATA'; + err.status = 422; + throw err; + } const existing = overlayFor(req.name); if (existing) existing.metadata = JSON.stringify(req.item); else { @@ -157,12 +175,83 @@ describe('permissionSetBodyFromRow / permissionSetRowFields (round-trip)', () => expect(body.rowLevelSecurity[0].using).toBe('org == current_user.org'); expect(body.tabPermissions).toEqual({ crm_leads: 'visible' }); expect(body.adminScope.businessUnit).toBe('Sales'); - expect(body.active).toBe(true); // and projecting the rebuilt body changes nothing expect(recordDiffersFromBody(row, body)).toBe(false); }); }); +// ── The definition ⊆ spec contract (#4669) ───────────────────────────────── +// +// `sys_permission_set` carries columns the DEFINITION does not (`active`, the +// timestamps, the provenance trio). Feeding them to `saveMetaItem` is what +// #4001's `.strict()` schema rejects, and what took the ADR-0094 D4 backfill +// to a 100% failure rate. + +describe('row→body projection keeps ONLY spec-declared keys (#4669)', () => { + const legacyRow = () => ({ + id: 'ps_1', + name: 'organization_admin', + ...permissionSetRowFields(envBody()), + // every storage column a real row carries… + active: true, + managed_by: 'admin', + package_id: null, + customized: false, + created_at: '2026-01-01T00:00:00Z', + updated_at: '2026-02-02T00:00:00Z', + // …plus a column this code has never heard of + some_future_column: 'whatever', + }); + + it('the whitelist is DERIVED from the spec schema, not transcribed', () => { + const keys = permissionSpecBodyKeys(); + // identical to PermissionSetSchema's own shape — the single source + expect([...keys].sort()).toEqual(Object.keys((PermissionSetSchema as any).shape).sort()); + expect(keys.has('objects')).toBe(true); + expect(keys.has('systemPermissions')).toBe(true); + expect(keys.has('adminScope')).toBe(true); + // `active` is a TABLE column, never a spec key — that is the whole bug + expect(keys.has('active')).toBe(false); + }); + + it('drops `active` and every other storage column from the projected body', () => { + const body = permissionSetBodyFromRow(legacyRow()); + for (const col of ['active', 'managed_by', 'package_id', 'customized', 'created_at', 'updated_at', 'id', 'some_future_column']) { + expect(body, `storage column '${col}' must not enter the metadata body`).not.toHaveProperty(col); + } + // the definition itself survives intact + expect(body.objects).toEqual(envBody().objects); + expect(body.systemPermissions).toEqual(envBody().systemPermissions); + }); + + it('every key the projection emits is one the spec ACCEPTS (parsed by the real schema)', () => { + const parsed = PermissionSetSchema.safeParse(permissionSetBodyFromRow(legacyRow())); + expect(parsed.success, parsed.success ? '' : JSON.stringify(parsed.error.issues)).toBe(true); + // …and the reverse guard: a spec RENAME must fail here rather than silently + // dropping the value at runtime. + const keys = permissionSpecBodyKeys(); + for (const key of Object.keys(permissionSetBodyFromRow(legacyRow()))) { + expect(keys.has(key), `body key '${key}' is not declared by PermissionSetSchema`).toBe(true); + } + }); + + it('filters a body STORED before #4001 (data at rest can still carry `active`)', () => { + // a legacy sys_metadata overlay written while the schema still stripped it + const legacyStored = { ...envBody(), active: false, _packageId: 'com.x' }; + const merged = mergeRowPatchIntoBody(legacyStored, { label: 'Renamed' }); + expect(merged).not.toHaveProperty('active'); + expect(merged).not.toHaveProperty('_packageId'); + expect(PermissionSetSchema.safeParse(merged).success).toBe(true); + }); + + it('pickRowStateColumns isolates the record-state columns (normalized)', () => { + expect(pickRowStateColumns({ active: 'false', label: 'x' })).toEqual({ active: false }); + expect(pickRowStateColumns({ active: true })).toEqual({ active: true }); + expect(pickRowStateColumns({ label: 'x' })).toBeNull(); + expect(pickRowStateColumns(null)).toBeNull(); + }); +}); + describe('upsertEnvPermissionSet (ADR-0094 — record is a pure projection)', () => { it('CREATES a missing record (managed_by admin) — Studio-authored sets appear in Setup', async () => { const ql = makeQl(); @@ -176,16 +265,28 @@ describe('upsertEnvPermissionSet (ADR-0094 — record is a pure projection)', () expect(JSON.parse(row.object_permissions)).toEqual(envBody().objects); }); - it('projects all facets (and active) onto an existing env-authored row', async () => { + it('projects all facets onto an existing env-authored row', async () => { const ql = makeQl(); ql.permRows.push({ id: 'ps_env', name: 'organization_admin', managed_by: 'user', system_permissions: '[]', active: true }); - const r = await upsertEnvPermissionSet(ql, envBody({ active: false })); + const r = await upsertEnvPermissionSet(ql, envBody()); expect(r.updated).toBe(1); const row = ql.permRows[0]; expect(row.id).toBe('ps_env'); // id stable — junction FKs stay valid expect(JSON.parse(row.system_permissions)).toEqual(['setup.access', 'manage_org_users']); expect(JSON.parse(row.admin_scope).businessUnit).toBe('Sales'); - expect(row.active).toBe(false); + }); + + it('[#4669] NEVER re-flips `active` from a body — it is row state, not definition', async () => { + // A body carrying `active` can only be legacy data at rest (pre-#4001) or a + // caller mistake. Projecting it would silently undo an admin's + // deactivate — the record's switch is the record's own. + const ql = makeQl(); + ql.permRows.push({ id: 'ps_env', name: 'organization_admin', managed_by: 'user', system_permissions: '[]', active: false }); + await upsertEnvPermissionSet(ql, { ...envBody(), active: true } as any); + expect(ql.permRows[0].active, 'a stale body must not re-activate a deactivated set').toBe(false); + // …and a record the projector CREATES starts active (column default). + await upsertEnvPermissionSet(ql, envBody({ name: 'fresh_set' })); + expect(ql.permRows.find((r: any) => r.name === 'fresh_set')?.active).toBe(true); }); it('projects onto a legacy row with ABSENT provenance (platform default)', async () => { @@ -426,8 +527,10 @@ describe('createPermissionSetWriteThrough (data door → metadata store)', () => // metadata is the store that changed… const overlay = JSON.parse(ql.metaRows[0].metadata); expect(overlay.systemPermissions).toEqual(['setup.access']); - expect(overlay.active).toBe(false); expect(overlay.objects).toEqual(envBody().objects); // unmentioned facets preserved + // [#4669] …but `active` rode along as a COLUMN, never as a body key: the + // definition stays spec-clean while the record's switch still flips. + expect(overlay).not.toHaveProperty('active'); // …and the record followed via projection expect(JSON.parse(ql.permRows[0].system_permissions)).toEqual(['setup.access']); expect(ql.permRows[0].active).toBe(false); @@ -435,6 +538,65 @@ describe('createPermissionSetWriteThrough (data door → metadata store)', () => expect(opCtx.result?.id).toBe(rowId); }); + it('[#4669] the activate/deactivate ACTIONS write the column and nothing else', async () => { + // `sys-permission-set.object.ts` ships two `type:'api'` actions that PATCH + // /data/sys_permission_set/{id} with `bodyExtra: { active: true|false }`. + // A row-state-only patch is not a definition write: it passes through to + // the driver, mints no overlay, and touches the metadata store not at all. + const ql = makeQl(); + const protocol = makeProtocol(ql); + registerPermissionSetProjection(protocol, { ql }); + await protocol.saveMetaItem({ type: 'permission', name: 'organization_admin', item: envBody() }); + const rowId = ql.permRows[0].id; + const savesBefore = protocol.saves.length; + const metaRowsBefore = JSON.stringify(ql.metaRows); + const mw = makeMiddleware(ql, protocol); + + for (const active of [false, true]) { + const nextCalled = await run(mw, { + object: 'sys_permission_set', operation: 'update', context: userCtx, + data: { id: rowId, active }, + }); + expect(nextCalled, 'the driver performs the column write, with its ordinary semantics').toBe(true); + } + expect(protocol.saves.length, 'no metadata write for a pure row-state patch').toBe(savesBefore); + expect(JSON.stringify(ql.metaRows)).toBe(metaRowsBefore); + }); + + it('[#4669] deactivating a PACKAGE-owned set mints no customization overlay', async () => { + const ql = makeQl(); + const declaredBody = envBody({ name: 'crm_rep', systemPermissions: ['pkg.baseline'] }); + (ql as any).registry = { listItems: (t: string) => (t === 'permission' ? [declaredBody] : []) }; + const protocol = makeProtocol(ql, { crm_rep: declaredBody }); + registerPermissionSetProjection(protocol, { ql }); + ql.permRows.push({ id: 'ps_pkg', name: 'crm_rep', managed_by: 'package', package_id: 'com.example.crm', system_permissions: '["pkg.baseline"]', active: true }); + const mw = makeMiddleware(ql, protocol); + const nextCalled = await run(mw, { + object: 'sys_permission_set', operation: 'update', context: userCtx, data: { id: 'ps_pkg', active: false }, + }); + expect(nextCalled).toBe(true); + expect(ql.metaRows.length, 'switching a packaged set off is not a customization of it').toBe(0); + expect(ql.permRows[0].customized).toBeUndefined(); + }); + + it('[#4669] INSERT honours an explicit `active` on the record (Clone action sends one)', async () => { + const ql = makeQl(); + const protocol = makeProtocol(ql); + registerPermissionSetProjection(protocol, { ql }); + const mw = makeMiddleware(ql, protocol); + const opCtx: any = { + object: 'sys_permission_set', operation: 'insert', context: userCtx, + data: { + name: 'support_agent', label: 'Support Agent', active: false, + object_permissions: JSON.stringify({ ticket: { allowRead: true } }), + }, + }; + await run(mw, opCtx); + expect(protocol.saves[0].item, 'the definition never carries row state').not.toHaveProperty('active'); + expect(ql.permRows[0].active).toBe(false); + expect(opCtx.result?.active).toBe(false); + }); + it('UPDATE that renames is rejected (the name is the metadata identity)', async () => { const ql = makeQl(); const protocol = makeProtocol(ql); @@ -570,6 +732,7 @@ describe('reconcilePermissionSetProjection', () => { }); const out = await reconcilePermissionSetProjection(protocol, { ql }); expect(out.backfilledIntoMetadata).toBe(1); + expect(out.backfillFailed).toBe(0); expect(ql.metaRows.length).toBe(1); const body = JSON.parse(ql.metaRows[0].metadata); expect(body.objects).toEqual({ ticket: { allowRead: true } }); @@ -578,6 +741,83 @@ describe('reconcilePermissionSetProjection', () => { expect(out2.backfilledIntoMetadata).toBe(0); }); + it('[#4669] a row carrying the `active` STORAGE COLUMN backfills instead of failing spec validation', async () => { + // The reported symptom: every `sys_permission_set` row has an `active` + // column, `permissionSetBodyFromRow` handed it to `saveMetaItem`, and + // #4001's `.strict()` schema rejected all of them — a 100%-failing + // backfill behind one `warn`, with `backfilledIntoMetadata` stuck at 0. + const ql = makeQl(); + const protocol = makeProtocol(ql); // validates with the real PermissionSetSchema + ql.permRows.push({ + id: 'ps_d8', name: 'd8_qc_user', managed_by: 'admin', + active: true, customized: false, package_id: null, + created_at: '2026-01-01T00:00:00Z', updated_at: '2026-01-02T00:00:00Z', + label: 'D8 QC User', ...permissionSetRowFields(envBody({ name: 'd8_qc_user' })), + }); + const logs: Array<{ level: string; msg: string }> = []; + const logger = { + info: (m: string) => logs.push({ level: 'info', msg: m }), + warn: (m: string) => logs.push({ level: 'warn', msg: m }), + error: (m: string) => logs.push({ level: 'error', msg: m }), + }; + const out = await reconcilePermissionSetProjection(protocol, { ql, logger }); + expect(out.backfilledIntoMetadata).toBe(1); + expect(out.backfillFailed).toBe(0); + expect(logs.some((l) => /backfill into metadata failed|FAILED/i.test(l.msg))).toBe(false); + const stored = JSON.parse(ql.metaRows[0].metadata); + expect(stored).not.toHaveProperty('active'); + expect(stored.name).toBe('d8_qc_user'); + }); + + it('[#4669/#4632] a REAL backfill failure is loud: error level, counted, consequence + fix', async () => { + const ql = makeQl(); + const protocol = makeProtocol(ql); + // Not a key problem — the stored facet JSON itself is off-contract, so no + // amount of key-filtering saves it. This is the case that MUST shout. + ql.permRows.push({ + id: 'ps_bad', name: 'broken_set', managed_by: 'admin', active: true, + label: 'Broken Set', object_permissions: JSON.stringify({ ticket: { allowRead: 'yes-please' } }), + }); + ql.permRows.push({ + id: 'ps_bad2', name: 'broken_set_2', managed_by: 'admin', active: true, + label: 'Broken Set 2', object_permissions: JSON.stringify({ ticket: { nonsense: true } }), + }); + // `error` follows the platform Logger contract: (message, error?, meta?). + const logs: Array<{ level: string; msg: string; meta?: any; cause?: Error }> = []; + const logger = { + info: (m: string, meta?: any) => logs.push({ level: 'info', msg: m, meta }), + warn: (m: string, meta?: any) => logs.push({ level: 'warn', msg: m, meta }), + error: (m: string, cause?: Error, meta?: any) => logs.push({ level: 'error', msg: m, cause, meta }), + }; + const out = await reconcilePermissionSetProjection(protocol, { ql, logger }); + + // counted in the RESULT — not only in a log line nobody reads + expect(out.backfillFailed).toBe(2); + expect(out.backfilledIntoMetadata).toBe(0); + expect(ql.metaRows.length).toBe(0); + + // level: error, never warn/info for a durability degradation + const errors = logs.filter((l) => l.level === 'error'); + expect(errors.length).toBeGreaterThan(0); + expect(logs.some((l) => l.level === 'warn' && /backfill/i.test(l.msg))).toBe(false); + // said ONCE, at the first failure — not once per failed write + const firstFailure = errors[0]!; + expect(errors.filter((l) => /backfill into metadata FAILED/.test(l.msg)).length).toBe(1); + // the consequence… + expect(firstFailure.msg).toMatch(/Nothing will look broken/); + expect(firstFailure.msg).toMatch(/re-provision/); + // …and the fix + expect(firstFailure.msg).toMatch(/Fix:/); + expect(firstFailure.meta?.name).toBe('broken_set'); + + // the summary carries the failure too — an `info` "reconciled" line over a + // failed backfill is the reassuring half-truth the rule exists to remove + const summary = errors.at(-1)!; + expect(summary.msg).toMatch(/2 FAILED backfill/); + expect(summary.meta?.failedNames).toEqual(['broken_set', 'broken_set_2']); + expect(logs.some((l) => l.level === 'info' && /reconciled/.test(l.msg))).toBe(false); + }); + it('heals a record that drifted from an EXISTING metadata definition (metadata wins)', async () => { const ql = makeQl(); const declared = { member_default: envBody({ name: 'member_default', systemPermissions: ['declared.baseline'] }) }; diff --git a/packages/plugins/plugin-security/src/permission-set-projection.ts b/packages/plugins/plugin-security/src/permission-set-projection.ts index 3ebd629632..b715fce3e2 100644 --- a/packages/plugins/plugin-security/src/permission-set-projection.ts +++ b/packages/plugins/plugin-security/src/permission-set-projection.ts @@ -37,6 +37,8 @@ * runtime-created sets a home package. */ +import { PermissionSetSchema } from '@objectstack/spec/security'; + export const SYSTEM_CTX = { isSystem: true }; export function genId(prefix: string): string { @@ -61,6 +63,17 @@ export async function tryUpdate(ql: any, object: string, data: any): Promise) => void; warn?: (m: string, meta?: Record) => void; + /** + * Durability-degradation channel (AGENTS.md "Degradation log levels", #4632): + * a metadata write that was supposed to land and did not is an `error`, not a + * `warn` — nothing looks broken afterwards, which is exactly why the failure + * has to be loud. + * + * Signature matches `Logger.error` in `@objectstack/spec/contracts` + * (`message, error?, meta?` — the cause is its own argument), so the kernel + * logger satisfies this interface as-is. + */ + error?: (m: string, error?: Error, meta?: Record) => void; } /** Aggregated outcome of a projection pass (shared with the boot seeders). */ @@ -104,16 +117,82 @@ const parseMaybeJson = (v: any, fallback: any): any => { const asBool = (v: any): boolean => !(v === false || v === 0 || v === '0' || v === 'false'); +/** + * `sys_permission_set` columns that are ROW STATE, not part of the metadata + * DEFINITION — the spec declares no such key, so they must never travel into a + * metadata body (#4669). Each entry maps a column to the normalizer the record + * expects, so the data door can write it straight onto the record. + * + * `active` is the on/off switch the Setup list views filter on and the two + * lifecycle actions toggle (`bodyExtra: { active: true|false }` in + * `objects/sys-permission-set.object.ts`). It is runtime state OF THE RECORD, + * never a capability boundary its author declared — which is precisely why it + * is not, and should not be, a key on `PermissionSetSchema`. + */ +const ROW_STATE_COLUMNS: Readonly any>> = { + active: asBool, +}; + +/** + * The body keys the permission SPEC declares, read from the Zod schema's own + * shape — derived, never transcribed (#4669). + * + * `sys_permission_set` is a projection of a metadata definition, but the TABLE + * carries columns the definition does not: {@link ROW_STATE_COLUMNS}, the + * timestamps, and the `managed_by` / `package_id` / `customized` provenance. + * Until #4001 a row-derived body that dragged those along still parsed — + * `PermissionSetSchema` stripped the extras silently — so handing a whole row + * to `saveMetaItem` appeared to work. #4001 sealed the schema `.strict()`, and + * every such body began failing validation with `[invalid_metadata] … + * Unrecognized key(s) on this permission set: 'active'`, which took the + * ADR-0094 D4 boot backfill to a 100% failure rate. + * + * Derived from `.shape` rather than hand-listed because a literal list here + * would go stale the moment the spec grows a key — silently dropping it from + * every projected body, i.e. reproducing the very defect this fixes one layer + * over. `permission-set-projection.test.ts` additionally pins that every key + * the row→body seams can emit is one the spec declares, so a spec RENAME fails + * a test instead of quietly losing the value at runtime. + * + * Resolved on first use, not at module load: `PermissionSetSchema` is a + * {@link lazySchema} proxy and touching `.shape` at import time would + * materialize it for every process that merely loads this module. + */ +let cachedSpecBodyKeys: ReadonlySet | null = null; +export function permissionSpecBodyKeys(): ReadonlySet { + return (cachedSpecBodyKeys ??= new Set( + Object.keys((PermissionSetSchema as unknown as { shape?: Record }).shape ?? {}), + )); +} + +/** + * Keep only what the permission spec declares. THE choke point every row→body + * seam passes through, so no storage column can reach `saveMetaItem` — neither + * from a live row nor from a body STORED before #4001 (data at rest written + * while the schema still stripped the extras can still carry `active`). + */ +function pickSpecDeclaredKeys(candidate: Record): Record { + const keys = permissionSpecBodyKeys(); + const body: Record = {}; + for (const [key, value] of Object.entries(candidate)) { + if (keys.has(key)) body[key] = value; + } + return body; +} + /** * Inverse of {@link permissionSetRowFields}: rebuild a PermissionSet body from * a `sys_permission_set` row (snake_case JSON-string columns → camelCase * body). Used by the one-time boot backfill (a legacy data-door-created * record becomes a metadata item) and by the data-door update merge when a * name has no metadata presence yet. + * + * The result is filtered through {@link pickSpecDeclaredKeys}: a DEFINITION is + * what the spec declares, and nothing else off the row goes with it (#4669). */ export function permissionSetBodyFromRow(row: any): any { const adminScope = row?.admin_scope ? parseMaybeJson(row.admin_scope, undefined) : undefined; - return { + return pickSpecDeclaredKeys({ name: row?.name, label: row?.label ?? row?.name, ...(row?.description != null ? { description: row.description } : {}), @@ -123,8 +202,34 @@ export function permissionSetBodyFromRow(row: any): any { rowLevelSecurity: parseMaybeJson(row?.row_level_security, []), tabPermissions: parseMaybeJson(row?.tab_permissions, {}), ...(adminScope ? { adminScope } : {}), - ...(row?.active != null ? { active: asBool(row.active) } : {}), - }; + }); +} + +/** + * The row-state columns a data-door payload carries, normalized for the + * record — `null` when it carries none. These bypass the metadata store + * entirely: they are the record's own state (#4669). + */ +export function pickRowStateColumns(payload: any): Record | null { + if (!payload || typeof payload !== 'object') return null; + const out: Record = {}; + for (const [col, normalize] of Object.entries(ROW_STATE_COLUMNS)) { + if (col in payload) out[col] = normalize(payload[col]); + } + return Object.keys(out).length > 0 ? out : null; +} + +/** + * Does this data-door payload touch the DEFINITION at all? Identity (`id` / + * `name`) and {@link ROW_STATE_COLUMNS} do not; anything else is treated as a + * definition edit and routes through the metadata store (an unrecognized + * column therefore keeps the pre-#4669 behavior rather than silently skipping + * the write-through). + */ +function touchesDefinition(payload: Record): boolean { + return Object.keys(payload).some( + (k) => k !== 'id' && k !== 'name' && !(k in ROW_STATE_COLUMNS), + ); } /** @@ -222,7 +327,13 @@ export async function upsertEnvPermissionSet( id: genId('ps'), name: ps.name, ...permissionSetRowFields(ps), - active: ps.active != null ? asBool(ps.active) : true, + // [#4669] `active` is ROW STATE, never read from the definition body: a + // new record starts active (same as the package seeder and the field's + // own `defaultValue`), and the data door writes the column directly + // afterwards. Taking it from the body would also let a body STORED + // before #4001 — which may still carry a stale `active` — silently + // re-flip a record an admin had just deactivated. + active: true, // [A4 #2920] Unified provenance vocab: an env/Studio-authored set is // ADMIN-owned (formerly stamped 'user'). No runtime path branches on the // value except the 'package' guard, so this is a pure vocab rename. @@ -236,8 +347,9 @@ export async function upsertEnvPermissionSet( // Facets follow the effective body; provenance columns are never touched // here — a package-owned row keeps its owner while carrying the overlay's // customization, and an env row keeps its user/platform/legacy provenance. + // [#4669] Facets only — `active` is row state and is NOT projected from the + // body (a projection pass must never re-flip a record's on/off switch). const patch: Record = { id: existing.id, ...permissionSetRowFields(ps) }; - if (ps.active != null) patch.active = asBool(ps.active); // Only stamp `customized` on package-owned rows (an overlay of a packaged // set). For env rows the concept doesn't apply — clear any stale flag. if (customized !== undefined) { @@ -468,7 +580,17 @@ async function resolveTargetRows(ql: any, opCtx: any): Promise { return []; } -/** Column-patch → body-key merge for the data-door update redirect. */ +/** + * Column-patch → body-key merge for the data-door update redirect. + * + * Row-state columns ({@link ROW_STATE_COLUMNS}) are deliberately NOT merged — + * they are the record's state, applied to the record itself by the + * write-through (#4669). The merged result is filtered through + * {@link pickSpecDeclaredKeys} because `base` may be a body STORED before + * #4001 sealed the schema and can still carry a stripped-at-the-time `active`; + * without the filter that legacy row would make every subsequent data-door + * edit of the set fail spec validation. + */ export function mergeRowPatchIntoBody(base: any, patch: Record): any { const body: any = { ...stripDecorations(base) }; if ('label' in patch) body.label = patch.label; @@ -476,7 +598,6 @@ export function mergeRowPatchIntoBody(base: any, patch: Record): an if (patch.description == null) delete body.description; else body.description = patch.description; } - if ('active' in patch) body.active = asBool(patch.active); if ('object_permissions' in patch) body.objects = parseMaybeJson(patch.object_permissions, {}); if ('field_permissions' in patch) body.fields = parseMaybeJson(patch.field_permissions, {}); if ('system_permissions' in patch) body.systemPermissions = parseMaybeJson(patch.system_permissions, []); @@ -488,7 +609,7 @@ export function mergeRowPatchIntoBody(base: any, patch: Record): an else body.adminScope = scope; } if (!body.objects || typeof body.objects !== 'object') body.objects = {}; - return body; + return pickSpecDeclaredKeys(body); } /** Effective (layered, overlay-wins) body for a record's name, else the row itself. */ @@ -591,9 +712,19 @@ export function createPermissionSetWriteThrough( try { await protocol.saveMetaItem({ type: 'permission', name: row.name, item: permissionSetBodyFromRow(row), ...actorArg }); } catch (e) { - logger?.warn?.('[security] failed to re-author restored permission set into metadata', { - name: row.name, error: (e as Error)?.message, - }); + // [#4632 — AGENTS.md "Degradation log levels"] Durability, not + // functionality: the record is back and lists normally, but its + // definition never returned to the metadata store — the stores + // disagree silently until someone notices the set behaves like a + // legacy data-door row. + logger?.error?.( + '[security] restored permission set was NOT re-authored into metadata (ADR-0094 D3) — the record is ' + + 'back and looks healthy, but the metadata store has no definition for it, so a metadata-driven ' + + 're-provision will not recreate it. Fix: make the record body spec-valid (the error names the ' + + 'offending key) and re-save the set through Setup, or re-run boot reconciliation.', + e as Error, + { name: row.name }, + ); } } return; @@ -619,7 +750,15 @@ export function createPermissionSetWriteThrough( // (PermissionSetSchema) runs inside saveMetaItem and rejects an // off-contract body with a structured 422. await protocol.saveMetaItem({ type: 'permission', name, item: permissionSetBodyFromRow(row), ...actorArg }); - results.push((await projectAndFetch(protocol, name)) ?? { name }); + const record: any = (await projectAndFetch(protocol, name)) ?? { name }; + // [#4669] Row state does not round-trip through metadata — the + // projector created the record with the column default, so an explicit + // `active` in the payload (the Clone action sends one) is applied here. + const rowState = pickRowStateColumns(row); + if (rowState && record.id && await tryUpdate(ql, 'sys_permission_set', { id: record.id, ...rowState })) { + Object.assign(record, rowState); + } + results.push(record); } opCtx.result = Array.isArray(opCtx.data) ? results : results[0]; return; // driver write intentionally skipped — the record is projector-owned @@ -639,12 +778,23 @@ export function createPermissionSetWriteThrough( err.status = 400; throw err; } + // [#4669] A patch that touches ONLY row state (`active` — what the + // activate/deactivate actions send as `bodyExtra`) is not a definition + // write at all: it goes to the driver untouched, so the column write + // keeps its ordinary engine semantics (history, updated_at, FLS) and no + // spurious "customization" overlay is minted on a packaged set. Routing + // it through the metadata store is what #4001's strict schema rejects. + if (!touchesDefinition(patch)) return next(); + const rowState = pickRowStateColumns(patch); const results: any[] = []; for (const row of targets) { const base = await effectiveBodyForRow(protocol, ql, row); const body = mergeRowPatchIntoBody(base, patch); body.name = row.name; await protocol.saveMetaItem({ type: 'permission', name: row.name, item: body, ...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 }); results.push((await projectAndFetch(protocol, row.name)) ?? { id: row.id, name: row.name }); } opCtx.result = results.length === 1 ? results[0] : results; @@ -684,6 +834,14 @@ export interface ProjectionReconcileOutcome { backfilledIntoMetadata: number; /** Records re-projected because they drifted from the effective body. */ driftHealed: number; + /** + * Records whose backfill FAILED — each one is a definition the metadata + * store does not have and now never got. Counted (not just logged) so a + * caller/test can see the degradation without grepping a log: #4669 stayed + * invisible for a whole release precisely because the only signal was a + * `warn` and the counters stayed at zero. + */ + backfillFailed: number; } /** Compare a record's projected columns against a body — true when they differ. */ @@ -695,7 +853,8 @@ export function recordDiffersFromBody(row: any, body: any): boolean { } if ((row?.label ?? null) !== (want.label ?? null)) return true; if ((row?.description ?? null) !== (want.description ?? null)) return true; - if (body?.active != null && asBool(row?.active) !== asBool(body.active)) return true; + // [#4669] `active` is row state, not a spec key — a definition body cannot + // declare it, so a record is never "drifted" on account of it. return false; } @@ -718,7 +877,10 @@ export async function reconcilePermissionSetProjection( protocol: any, deps: ProjectionDeps, ): Promise { - const out: ProjectionReconcileOutcome = { projectedFromMetadata: 0, backfilledIntoMetadata: 0, driftHealed: 0 }; + const out: ProjectionReconcileOutcome = { + projectedFromMetadata: 0, backfilledIntoMetadata: 0, driftHealed: 0, backfillFailed: 0, + }; + const failedNames: string[] = []; const { ql, logger } = deps; if (!ql || typeof ql.find !== 'function' || !protocol || typeof protocol.getMetaItemLayered !== 'function') { return out; @@ -768,9 +930,30 @@ export async function reconcilePermissionSetProjection( }); out.backfilledIntoMetadata += 1; } catch (e) { - logger?.warn?.('[security] permission-set backfill into metadata failed (ADR-0094 D4)', { - name: row.name, error: (e as Error)?.message, - }); + out.backfillFailed += 1; + failedNames.push(String(row.name)); + // [#4632 — AGENTS.md "Degradation log levels"] DURABILITY degradation: + // the record keeps listing and keeps resolving, so nothing looks + // broken — while the definition it is supposed to project stays absent + // from the only authoritative store. Said ONCE, at the first failure, + // with the consequence and the fix; the rest are counted and named in + // the summary line below. #4669: this was a `warn` with no counter, + // which is why a 100%-failing backfill sat green for a release. + if (out.backfillFailed === 1) { + logger?.error?.( + '[security] permission-set backfill into metadata FAILED (ADR-0094 D4) — this environment has ' + + '`sys_permission_set` records with NO metadata definition backing them, and the one-time backfill ' + + 'did not write one. Nothing will look broken: the records still list in Setup and the evaluator ' + + 'still resolves them from the table — but the definitions are absent from the metadata store, so a ' + + 'metadata-driven re-provision (fresh environment, package reinstall, `meta resync`) recreates none ' + + 'of them, and every boot retries and fails identically. Fix: make the record body spec-valid — the ' + + 'error below names the offending key; `permissionSetBodyFromRow()` already drops storage columns ' + + '(`active`, timestamps, provenance), so a rejection here means the stored facet JSON itself is ' + + 'off-contract — then reboot to re-run reconciliation, or delete the orphan record.', + e as Error, + { name: row.name }, + ); + } } } else if (recordDiffersFromBody(row, effective)) { // These names have NO env overlay (skipped above), so the effective @@ -788,6 +971,19 @@ export async function reconcilePermissionSetProjection( } } - logger?.info?.('[security] sys_permission_set projection reconciled (ADR-0094 D4)', { ...out }); + if (out.backfillFailed > 0) { + // The summary carries the same level as the degradation it summarizes — + // an `info` "reconciled" line over a failed backfill is the reassuring + // half-truth this rule exists to remove. + logger?.error?.( + `[security] sys_permission_set projection reconciled with ${out.backfillFailed} FAILED backfill(s) ` + + '(ADR-0094 D4) — those records have no metadata definition and will not survive a re-provision. ' + + 'See the first-failure error above for the offending key and the fix.', + undefined, + { ...out, failedNames: failedNames.slice(0, 10) }, + ); + } else { + logger?.info?.('[security] sys_permission_set projection reconciled (ADR-0094 D4)', { ...out }); + } return out; }