From bf3da2f92449012c01ee941438f865af61ffc46d Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 05:23:29 +0000 Subject: [PATCH] fix(app-shell): package-door permission save carries every editor-authorable facet (#4302) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An RLS policy authored in Studio's package door was silently discarded: Save returned 200 with a success toast and the PUT body carried no `rowLevelSecurity` key at all, so no row filter was ever persisted while the surface still showed the permission set as configured. Tab visibility and the delegated-admin scope were reverted the same way. `mergePermissionSlice` returned `{...base, name, label, isDefault, objects, fields}` — five keys from the edited draft and every other facet from a fresh server read. That whitelist had drifted behind the editor. It is now inverted: a facet the editor can author comes from `edited`, and `base` supplies only what the editor cannot author. The package scoping ADR-0086 P0 actually needs is `objects` / `fields`, which still go through the same row-level merge, so other packages' contributed rows are preserved byte-for-byte. A structural guard scans the editor sources for the keys their `setDraft(...)` updaters write and fails when the slice does not carry one, so the next facet added to the editor cannot silently drop on this door. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3 --- .../package-door-permission-facets-4302.md | 13 + ...ionMatrixEditor.packageDoorFacets.test.tsx | 248 +++++++++++++++++ .../permission-slice.authoredKeys.test.ts | 260 ++++++++++++++++++ .../permission-slice.facetDrift.test.ts | 137 +++++++++ .../metadata-admin/permission-slice.test.ts | 5 +- .../views/metadata-admin/permission-slice.ts | 76 ++++- 6 files changed, 725 insertions(+), 14 deletions(-) create mode 100644 .changeset/package-door-permission-facets-4302.md create mode 100644 packages/app-shell/src/views/metadata-admin/PermissionMatrixEditor.packageDoorFacets.test.tsx create mode 100644 packages/app-shell/src/views/metadata-admin/permission-slice.authoredKeys.test.ts create mode 100644 packages/app-shell/src/views/metadata-admin/permission-slice.facetDrift.test.ts diff --git a/.changeset/package-door-permission-facets-4302.md b/.changeset/package-door-permission-facets-4302.md new file mode 100644 index 0000000000..9f560a5cb1 --- /dev/null +++ b/.changeset/package-door-permission-facets-4302.md @@ -0,0 +1,13 @@ +--- +'@object-ui/app-shell': patch +--- + +A row-level-security policy authored under a package is now saved — Studio's package door carries every facet the permission editor can author + +Opening the Access pillar inside a writable package, adding an RLS policy (name, object, USING, CHECK) and clicking a fully-enabled Save produced a 200, a success toast, and no row filter anywhere. The PUT body was `{"name":…,"label":…,"objects":{},"fields":{}}` — the `rowLevelSecurity` key was not on the wire at all, so the record read back after publish carried no policy while the surface still showed one. Tab visibility and the delegated-admin scope were reverted the same way. A silent revert is worse than a refusal here: the admin is left with positive, false evidence that a control is in place. + +`mergePermissionSlice` — the package door's save path (ADR-0086 P0) — rebuilt the record from a freshly-read base and took only `name`, `label`, `isDefault`, `objects` and `fields` from the editor. Every other facet came from the server's stored copy, whatever the author had just typed. The whitelist was correct when it was written and then drifted: `rowLevelSecurity`, `tabPermissions` and `adminScope` became authorable afterwards and nothing said so. + +The rule is now inverted rather than extended by four more names: a facet the editor can author comes from the edited draft, and the freshly-read base supplies only what the editor cannot author. The package scoping that P0 actually needs is `objects` / `fields` and nothing else — the load path narrows exactly those two maps and hands every other facet to the editors unscoped — so other packages' contributed rows are still preserved byte-for-byte by the same row-level merge as before. A facet the edited body does not carry at all still comes from the base: absence means "this caller does not model the facet", never "the author cleared it", and clearing still persists as clearing because the facet editors always write a value (an emptied policy list is `[]`, not a missing key). + +Drift cannot recur silently. A structural guard scans the editor sources for the keys their `setDraft(...)` updaters write into the draft and fails when one of them is not carried by the slice, so the next facet added to the editor reds in CI instead of being discarded on Save. The environment door, which was never affected, is pinned unchanged as a guard, and the pins assert the saved body rather than the toast — a 200 is exactly what this defect already produced. diff --git a/packages/app-shell/src/views/metadata-admin/PermissionMatrixEditor.packageDoorFacets.test.tsx b/packages/app-shell/src/views/metadata-admin/PermissionMatrixEditor.packageDoorFacets.test.tsx new file mode 100644 index 0000000000..38dd28a1ae --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/PermissionMatrixEditor.packageDoorFacets.test.tsx @@ -0,0 +1,248 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * objectui#4302 — what the package door actually PUTs. + * + * The card's evidence was the wire, not a toast: an author filled in a + * row-level-security policy under a writable package, Save reported success, + * and the body was + * + * `{"name":"qa_st2_rls","label":"QA ST2 RLS","objects":{},"fields":{}}` + * + * — no `rowLevelSecurity` key at all, so the row filter was never persisted + * while the surface claimed a control was in place. Tab visibility and the + * delegated-admin scope were reverted the same way. + * + * These pins therefore assert the SAVED BODY, driving the real + * `PermissionMatrixEditPage` (load effect, facet editors, `doSave`) against a + * fake client that behaves like the server. The environment door — the one that + * already worked — is pinned unchanged as a guard, so the fix cannot be a + * regression traded for a fix. + */ + +import '@testing-library/jest-dom/vitest'; +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { cleanup, fireEvent, render, screen, waitFor, within } from '@testing-library/react'; +import { MemoryRouter } from 'react-router-dom'; + +const USING = 'owner_id == current_user.id'; + +/** + * The USING editor is selected by its placeholder, not by its label: the RLS + * facet renders `CelPredicateField` without the optional `id`, so its + * `Label htmlFor` associates with no control and `getByLabelText` cannot find + * it. Reported as an out-of-scope accessibility finding rather than fixed here. + */ +const USING_PLACEHOLDER = 'organization_id == current_user.organization_id'; + +interface Server { + /** The published record — what `layered()` answers, i.e. the merge `base`. */ + set: Record; + /** A pending package draft, if any (ADR-0086 P2 D6). */ + draft: Record | null; + packageObjects: Array<{ name: string; label?: string }>; + objectFields: Record; + saved: Array>; + savedOpts: Array | undefined>; +} + +function makeClient(server: Server) { + return { + layered: async () => ({ effective: server.set, code: null, overlay: null, overlayScope: null }), + getDraft: async () => (server.draft ? { item: server.draft } : null), + list: async (type: string) => { + if (type === 'object') return server.packageObjects.map((o) => ({ item: o })); + return []; + }, + get: async (type: string, name: string) => (type === 'object' ? server.objectFields[name] ?? null : null), + save: async ( + _type: string, + _name: string, + payload: Record, + opts?: Record, + ) => { + server.saved.push(payload); + server.savedOpts.push(opts); + server.set = payload; + return payload; + }, + } as any; +} + +let clientImpl: any; + +vi.mock('./useMetadata', () => ({ + useMetadataClient: () => clientImpl, + useMetadataTypes: () => ({ + loading: false, + error: null, + entries: [{ type: 'permission', label: 'Permission', allowOrgOverride: true }], + }), +})); + +vi.mock('./AssignedUsersSection', () => ({ AssignedUsersSection: () => null })); + +import { PermissionMatrixEditPage } from './PermissionMatrixEditor'; + +afterEach(cleanup); + +function freshServer(): Server { + return { + draft: null, + packageObjects: [{ name: 'a_account' }, { name: 'a_contact' }], + objectFields: { + a_account: { fields: [{ name: 'organization_id' }, { name: 'name' }] }, + a_contact: { fields: [{ name: 'email' }] }, + }, + saved: [], + savedOpts: [], + set: { + name: 'sales_perms', + label: 'Sales', + systemPermissions: ['api_enabled'], + // Tab rows are NOT package-scoped by the panel — the editor shows the + // whole map and edits it in place, which is why holding it back on save + // discarded the author's edit rather than protecting anyone's rows. + tabPermissions: { a_account: 'visible', b_order: 'default_on' }, + objects: { + a_account: { allowRead: true, allowCreate: true }, + a_contact: { allowRead: true }, + // Package B's contribution — must survive every save byte-for-byte. + b_order: { allowRead: true, allowEdit: true, viewAllRecords: true }, + }, + fields: { 'b_order.total': { readable: true, editable: true } }, + }, + }; +} + +function renderDoor(packageId?: string) { + return render( + + + , + ); +} + +/** Expand a collapsed facet section by its title. */ +function openFacet(title: string) { + fireEvent.click(screen.getByRole('button', { name: new RegExp(title) })); +} + +/** + * Author one policy, one tab-visibility change and a delegated-admin scope + * through the real facet editors — the card's reproduction, minus the browser. + */ +async function authorEveryFacet() { + openFacet('Row-Level Security'); + fireEvent.click(await screen.findByRole('button', { name: /Add policy/ })); + fireEvent.change(await screen.findByPlaceholderText('Policy name'), { + target: { value: 'qa_st2_rls' }, + }); + fireEvent.change(screen.getByPlaceholderText('Object (* = all)'), { + target: { value: 'a_account' }, + }); + fireEvent.change(screen.getByPlaceholderText(USING_PLACEHOLDER), { target: { value: USING } }); + + openFacet('Tab Visibility'); + fireEvent.change(await screen.findByDisplayValue('Visible'), { target: { value: 'hidden' } }); + + openFacet('Delegated Admin Scope'); + const businessUnit = (await screen.findByText('Business unit')).parentElement!.querySelector( + 'input', + )!; + fireEvent.change(businessUnit, { target: { value: 'emea' } }); +} + +const AUTHORED_POLICY = { + name: 'qa_st2_rls', + object: 'a_account', + operation: 'all', + using: USING, + enabled: true, +}; + +describe('PermissionMatrixEditPage — the PACKAGE door persists every authored facet (objectui#4302)', () => { + it('PUTs the authored RLS policy, tab visibility and admin scope', async () => { + const server = freshServer(); + clientImpl = makeClient(server); + renderDoor('app.a'); + await screen.findByText('a_account'); + + await authorEveryFacet(); + + fireEvent.click(screen.getByRole('button', { name: /^Save$/ })); + await waitFor(() => expect(server.saved).toHaveLength(1)); + const body = server.saved[0] as any; + + // The row filter reaches the wire — the key whose absence was the defect. + expect(body.rowLevelSecurity).toEqual([AUTHORED_POLICY]); + // …and so do the other two facets the whitelist reverted. + expect(body.tabPermissions).toEqual({ a_account: 'hidden', b_order: 'default_on' }); + expect(body.adminScope).toEqual({ businessUnit: 'emea' }); + + // ADR-0086 P0 — package B's rows still survive byte-for-byte. + expect(body.objects.b_order).toEqual({ + allowRead: true, + allowEdit: true, + viewAllRecords: true, + }); + expect(body.fields['b_order.total']).toEqual({ readable: true, editable: true }); + // ADR-0086 P2 (D6/D7) — still a package-stamped DRAFT write, not a live one. + expect(server.savedOpts[0]).toMatchObject({ mode: 'draft', packageId: 'app.a' }); + }); + + it('keeps a policy carried by the pending draft when the published base has none', async () => { + // The D6 reopen path: a previous package save staged the policy in the + // draft; the published record has not moved. The merge base is the + // PUBLISHED read, so a whitelist that takes facets from `base` erases the + // staged policy on the next save — silently, again. + const server = freshServer(); + server.draft = { ...(server.set as Record), rowLevelSecurity: [AUTHORED_POLICY] }; + clientImpl = makeClient(server); + renderDoor('app.a'); + await screen.findByText('a_account'); + + // An edit elsewhere on the panel — the author is not touching RLS at all. + const row = screen.getByText('a_account').closest('tr')!; + fireEvent.click(within(row).getByRole('button', { name: 'None' })); + + fireEvent.click(screen.getByRole('button', { name: /^Save$/ })); + await waitFor(() => expect(server.saved).toHaveLength(1)); + const body = server.saved[0] as any; + + expect(body.rowLevelSecurity).toEqual([AUTHORED_POLICY]); + expect(body.objects.a_account).toEqual({}); + expect(body.objects.b_order).toEqual({ + allowRead: true, + allowEdit: true, + viewAllRecords: true, + }); + }); +}); + +describe('PermissionMatrixEditPage — the ENVIRONMENT door is unchanged (guard)', () => { + it('still PUTs the whole record, with no draft/package options', async () => { + const server = freshServer(); + clientImpl = makeClient(server); + renderDoor(undefined); + await screen.findByText('a_account'); + + await authorEveryFacet(); + + fireEvent.click(screen.getByRole('button', { name: /^Save$/ })); + await waitFor(() => expect(server.saved).toHaveLength(1)); + const body = server.saved[0] as any; + + expect(body.rowLevelSecurity).toEqual([AUTHORED_POLICY]); + expect(body.tabPermissions).toEqual({ a_account: 'hidden', b_order: 'default_on' }); + expect(body.adminScope).toEqual({ businessUnit: 'emea' }); + // The environment door writes the record live — no `mode: 'draft'`, no + // `packageId`, and no slice merge in front of it. + expect(server.savedOpts[0]).toEqual({ force: false }); + expect(body.objects.b_order).toEqual({ + allowRead: true, + allowEdit: true, + viewAllRecords: true, + }); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/permission-slice.authoredKeys.test.ts b/packages/app-shell/src/views/metadata-admin/permission-slice.authoredKeys.test.ts new file mode 100644 index 0000000000..7ee182aa4f --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/permission-slice.authoredKeys.test.ts @@ -0,0 +1,260 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Structural drift guard for the package-door slice (objectui#4302). + * + * `mergePermissionSlice` takes the facets it carries from a NAMED list + * ({@link EDITOR_AUTHORED_KEYS}); everything else comes from the freshly-read + * `base`. That list is only correct while it keeps up with what the Access + * matrix editor can actually author — and it did not: `rowLevelSecurity`, + * `tabPermissions` and `adminScope` were authorable for three releases while the + * slice still carried five keys, so a row-level-security policy authored under a + * package was reverted on Save with no error anywhere. + * + * A silent drop is what made that expensive, so the guard is mechanical: scan + * the editor sources for the keys their `setDraft(...)` updaters write into the + * draft, and fail if any of them is not carried by the slice. The next facet + * added to the editor cannot silently drop on this door — it reds here first. + * + * Scope of the scan (and its limits, stated rather than implied): it reads the + * object literals inside `setDraft(...)` calls in the two files that own the + * draft. A facet written through a helper defined elsewhere would not be seen — + * which is why the floor assertion below pins the keys the scan MUST find, so a + * scanner that goes blind reds instead of passing on an empty set. + */ + +import { readFileSync } from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { describe, expect, it } from 'vitest'; +import { EDITOR_AUTHORED_KEYS } from './permission-slice'; + +const here = path.dirname(fileURLToPath(import.meta.url)); + +/** The two sources that own the permission-set draft the package door saves. */ +const DRAFT_OWNERS = ['PermissionMatrixEditor.tsx', 'PermissionAdvancedFacets.tsx']; + +/** + * Keys the scan must find, so an empty or broken scan cannot read as coverage. + * Removing a facet from the editor reds this deliberately — update it with the + * removal, never to make a red go away. + */ +const SCAN_FLOOR = [ + 'name', + 'label', + 'systemPermissions', + 'objects', + 'fields', + 'rowLevelSecurity', + 'tabPermissions', + 'adminScope', +]; + +/* ── A very small source scanner ─────────────────────────────────────────── */ + +/** Advance past a string/template literal that starts at `i`. */ +function skipQuoted(src: string, i: number): number { + const quote = src[i]; + i++; + while (i < src.length) { + if (src[i] === '\\') i += 2; + else if (src[i] === quote) return i + 1; + else i++; + } + return i; +} + +/** Advance past a `//` or block comment that starts at `i` (or return `i`). */ +function skipComment(src: string, i: number): number { + if (src[i] === '/' && src[i + 1] === '/') { + const nl = src.indexOf('\n', i); + return nl === -1 ? src.length : nl + 1; + } + if (src[i] === '/' && src[i + 1] === '*') { + const end = src.indexOf('*/', i + 2); + return end === -1 ? src.length : end + 2; + } + return i; +} + +/** + * Text between the bracket at `open` and its match, skipping strings and + * comments. `open` must index a `(`, `[` or `{`. + */ +function balanced(src: string, open: number): { inner: string; end: number } { + const pairs: Record = { '(': ')', '[': ']', '{': '}' }; + const close = pairs[src[open]]; + let depth = 0; + let i = open; + while (i < src.length) { + const c = src[i]; + const afterComment = skipComment(src, i); + if (afterComment !== i) { + i = afterComment; + continue; + } + if (c === '"' || c === "'" || c === '`') { + i = skipQuoted(src, i); + continue; + } + if (c === src[open]) depth++; + else if (c === close) { + depth--; + if (depth === 0) return { inner: src.slice(open + 1, i), end: i + 1 }; + } + i++; + } + return { inner: src.slice(open + 1), end: src.length }; +} + +/** Split `inner` on its TOP-LEVEL commas (nesting, strings and comments aware). */ +function topLevelSegments(inner: string): string[] { + const out: string[] = []; + let depth = 0; + let start = 0; + let i = 0; + while (i < inner.length) { + const c = inner[i]; + const afterComment = skipComment(inner, i); + if (afterComment !== i) { + i = afterComment; + continue; + } + if (c === '"' || c === "'" || c === '`') { + i = skipQuoted(inner, i); + continue; + } + if (c === '(' || c === '[' || c === '{') depth++; + else if (c === ')' || c === ']' || c === '}') depth--; + else if (c === ',' && depth === 0) { + out.push(inner.slice(start, i)); + start = i + 1; + } + i++; + } + out.push(inner.slice(start)); + return out; +} + +/** Strip comments so a `//`-commented key can never be read as authored. */ +function stripComments(text: string): string { + let out = ''; + let i = 0; + while (i < text.length) { + const afterComment = skipComment(text, i); + if (afterComment !== i) { + out += ' '; + i = afterComment; + continue; + } + if (text[i] === '"' || text[i] === "'" || text[i] === '`') { + const end = skipQuoted(text, i); + out += text.slice(i, end); + i = end; + continue; + } + out += text[i]; + i++; + } + return out; +} + +/** The statically-known keys an object literal's body declares at its top level. */ +function literalKeys(inner: string): string[] { + const keys: string[] = []; + for (const rawSegment of topLevelSegments(inner)) { + const segment = stripComments(rawSegment).trim(); + if (!segment) continue; + if (segment.startsWith('...')) continue; // spread — carries no named key + if (segment.startsWith('[')) continue; // computed key — not statically known + const m = /^(?:'([^']*)'|"([^"]*)"|([A-Za-z_$][\w$]*))\s*(:|$)/.exec(segment); + if (m) keys.push(m[1] ?? m[2] ?? m[3]); + } + return keys; +} + +/** + * Every key written into the draft by a `setDraft(...)` updater in `src`. + * Only object literals whose first token is a spread count — those are the + * `{ ...prev, : value }` shape a draft update takes; a literal that does + * not spread the previous draft is a fresh value, not a draft write. + */ +export function scanDraftAuthoredKeys(src: string): Set { + const keys = new Set(); + const CALL = /\bsetDraft\s*\(/g; + let call: RegExpExecArray | null; + while ((call = CALL.exec(src))) { + const { inner: callArgs } = balanced(src, call.index + call[0].length - 1); + let i = 0; + while (i < callArgs.length) { + const c = callArgs[i]; + const afterComment = skipComment(callArgs, i); + if (afterComment !== i) { + i = afterComment; + continue; + } + if (c === '"' || c === "'" || c === '`') { + i = skipQuoted(callArgs, i); + continue; + } + if (c === '{') { + const { inner, end } = balanced(callArgs, i); + if (/^\s*\.\.\./.test(inner)) { + // An object literal that spreads the previous draft: its top-level + // keys are the facets this update writes. Skip past it so a NESTED + // literal's keys are never read as top-level facets. + for (const k of literalKeys(inner)) keys.add(k); + i = end; + continue; + } + } + i++; + } + } + return keys; +} + +/* ── The guard ───────────────────────────────────────────────────────────── */ + +describe('permission slice — carried keys keep up with the editor (objectui#4302)', () => { + const scanned = new Set(); + for (const file of DRAFT_OWNERS) { + for (const k of scanDraftAuthoredKeys(readFileSync(path.join(here, file), 'utf8'))) { + scanned.add(k); + } + } + + it('the scanner reads real draft updates (not an empty set)', () => { + // Anti-empty-green: a scanner that silently matched nothing would make + // every assertion below pass while guarding nothing at all. + const fixture = ` + setDraft((p) => ({ ...p, alpha: 1, beta })); + setDraft((prev) => { + const objects = { ...(prev.objects ?? {}) }; + return { ...prev, objects }; + }); + setDraft((p) => ({ ...p, nested: { ...asObject(p.nested), inner: 2 } })); + setOther((prev) => ({ ...prev, notADraftKey: 3 })); + setDraft((p) => ({ ...p, computed: v, [dynamic]: v })); + `; + expect([...scanDraftAuthoredKeys(fixture)].sort()).toEqual([ + 'alpha', + 'beta', + 'computed', + 'nested', + 'objects', + ]); + }); + + it('finds every facet the editor is known to author', () => { + expect([...scanned].sort()).toEqual(expect.arrayContaining([...SCAN_FLOOR].sort())); + }); + + it('carries every facet the editor can author — no silent drop on the package door', () => { + const carried = new Set(EDITOR_AUTHORED_KEYS); + const dropped = [...scanned].filter((k) => !carried.has(k)).sort(); + // A key here is authored in the editor and thrown away by the package + // door's slice merge: Save succeeds and the author's edit never persists. + expect(dropped).toEqual([]); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/permission-slice.facetDrift.test.ts b/packages/app-shell/src/views/metadata-admin/permission-slice.facetDrift.test.ts new file mode 100644 index 0000000000..1c533d81d3 --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/permission-slice.facetDrift.test.ts @@ -0,0 +1,137 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * objectui#4302 — the package door must persist EVERY facet the editor can + * author, not just the five keys the slice used to name. + * + * `mergePermissionSlice` rebuilds the record from a freshly-read `base` so other + * packages' object/field rows survive byte-for-byte (ADR-0086 P0). It used to + * return `{...base, name, label, isDefault, objects, fields}` — a whitelist that + * had drifted behind the editor: `rowLevelSecurity`, `tabPermissions`, + * `adminScope` and `systemPermissions` were taken from `base` even when the + * author had just edited them, so a row-level-security policy authored in Studio + * was silently reverted while Save reported success. + * + * The direction that matters for these pins is the one the defect needs: + * `edited` and `base` must DISAGREE. A fixture where both carry the same facet + * value cannot see the bug at all — that is exactly why it survived the original + * P0 suite, whose `edited` fixtures carry no advanced facets. + */ + +import { describe, expect, it } from 'vitest'; +import { mergePermissionSlice, type PermissionSetDraft } from './permission-slice'; + +const SCOPE_A = ['a_account', 'a_contact']; + +/** The record as the server holds it at Save time (the fresh `layered` read). */ +function baseRecord(): PermissionSetDraft { + return { + name: 'sales_perms', + label: 'Sales permissions', + systemPermissions: ['api_enabled'], + tabPermissions: { a_account: 'visible', b_order: 'default_on' }, + rowLevelSecurity: [ + { name: 'stored_policy', object: 'a_account', operation: 'all', using: 'true', enabled: true }, + ], + adminScope: { businessUnit: 'stored_bu', includeSubtree: false }, + objects: { + a_account: { allowRead: true }, + b_order: { allowRead: true, allowEdit: true }, + }, + fields: { 'b_order.total': { readable: true, editable: true } }, + }; +} + +/** + * What the editor holds after the author edited the panel: the in-scope slice + * plus EVERY advanced facet, because the load path narrows `objects`/`fields` + * only (`resetDraftBaseline({...full, objects: sliced, fields: sliced})`) and + * hands the rest of the record to the facet editors unscoped. + */ +function editedDraft(): PermissionSetDraft { + return { + name: 'sales_perms', + label: 'Sales permissions', + systemPermissions: ['api_enabled', 'bulk_api'], + tabPermissions: { a_account: 'hidden', b_order: 'default_on' }, + rowLevelSecurity: [ + { name: 'stored_policy', object: 'a_account', operation: 'all', using: 'true', enabled: true }, + { + name: 'qa_st2_rls', + object: 'a_account', + operation: 'all', + using: 'organization_id == current_user.organization_id', + enabled: true, + }, + ], + adminScope: { businessUnit: 'authored_bu', includeSubtree: true }, + objects: { a_account: { allowRead: true, allowEdit: true } }, + fields: {}, + }; +} + +describe('mergePermissionSlice — the package door carries every authored facet (objectui#4302)', () => { + it('persists an RLS policy authored in the editor instead of reverting to the stored list', () => { + const merged = mergePermissionSlice(baseRecord(), editedDraft(), SCOPE_A); + + const policies = merged.rowLevelSecurity as Array>; + expect(policies).toHaveLength(2); + expect(policies.map((p) => p.name)).toEqual(['stored_policy', 'qa_st2_rls']); + expect(policies[1]).toEqual({ + name: 'qa_st2_rls', + object: 'a_account', + operation: 'all', + using: 'organization_id == current_user.organization_id', + enabled: true, + }); + }); + + it('persists authored tab visibility', () => { + const merged = mergePermissionSlice(baseRecord(), editedDraft(), SCOPE_A); + expect(merged.tabPermissions).toEqual({ a_account: 'hidden', b_order: 'default_on' }); + }); + + it('persists an authored delegated-admin scope', () => { + const merged = mergePermissionSlice(baseRecord(), editedDraft(), SCOPE_A); + expect(merged.adminScope).toEqual({ businessUnit: 'authored_bu', includeSubtree: true }); + }); + + it('persists authored system permissions (capability picker)', () => { + const merged = mergePermissionSlice(baseRecord(), editedDraft(), SCOPE_A); + expect(merged.systemPermissions).toEqual(['api_enabled', 'bulk_api']); + }); + + it('persists the REMOVAL of a facet — an emptied policy list does not resurrect from base', () => { + // Deleting the last policy leaves `[]` in the draft (the facet editor always + // writes a value), so an empty array is an authored value, not an absence. + const edited = { ...editedDraft(), rowLevelSecurity: [], tabPermissions: {} }; + const merged = mergePermissionSlice(baseRecord(), edited, SCOPE_A); + expect(merged.rowLevelSecurity).toEqual([]); + expect(merged.tabPermissions).toEqual({}); + }); + + it('leaves a facet the edited body does not carry at all to `base`', () => { + // A caller that never modelled the facet (a partial `edited`) must not be + // able to erase it — absence is "not authored", not "cleared". This is the + // contract the original ADR-0086 P0 suite pins, kept unchanged. + const edited: PermissionSetDraft = { + name: 'sales_perms', + objects: { a_account: { allowRead: true } }, + fields: {}, + }; + const merged = mergePermissionSlice(baseRecord(), edited, SCOPE_A); + expect(merged.rowLevelSecurity).toEqual(baseRecord().rowLevelSecurity); + expect(merged.tabPermissions).toEqual(baseRecord().tabPermissions); + expect(merged.adminScope).toEqual(baseRecord().adminScope); + expect(merged.systemPermissions).toEqual(['api_enabled']); + expect(merged.label).toBe('Sales permissions'); + }); + + it('still preserves another package\'s rows byte-for-byte (ADR-0086 P0)', () => { + const base = baseRecord(); + const merged = mergePermissionSlice(base, editedDraft(), SCOPE_A); + expect(merged.objects.b_order).toBe(base.objects.b_order); + expect(merged.fields!['b_order.total']).toBe(base.fields!['b_order.total']); + expect(merged.objects.a_account).toEqual({ allowRead: true, allowEdit: true }); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/permission-slice.test.ts b/packages/app-shell/src/views/metadata-admin/permission-slice.test.ts index 7ce8a62af8..dda3509f10 100644 --- a/packages/app-shell/src/views/metadata-admin/permission-slice.test.ts +++ b/packages/app-shell/src/views/metadata-admin/permission-slice.test.ts @@ -89,7 +89,10 @@ describe('mergePermissionSlice — Save preserves other packages byte-for-byte', expect(merged.objects.a_contact).toBeUndefined(); // removed grant gone expect(merged.fields!['a_contact.email']).toBeUndefined(); - // Set-level extras survive from base. + // Set-level extras this `edited` does not carry AT ALL survive from base. + // (objectui#4302: a facet the edited draft DOES carry now comes from the + // edited draft — pinned in permission-slice.facetDrift.test.ts. This case + // pins the other half: absence means "not modelled", never "cleared".) expect(merged.systemPermissions).toEqual(['api_enabled']); expect(merged.tabPermissions).toEqual(full.tabPermissions); }); diff --git a/packages/app-shell/src/views/metadata-admin/permission-slice.ts b/packages/app-shell/src/views/metadata-admin/permission-slice.ts index 3ccacc1134..23d2173eb4 100644 --- a/packages/app-shell/src/views/metadata-admin/permission-slice.ts +++ b/packages/app-shell/src/views/metadata-admin/permission-slice.ts @@ -13,8 +13,9 @@ * * Overwriting the whole record (the pre-P0 behavior) silently drops the rows * other packages contributed. {@link mergePermissionSlice} rebuilds the record - * from a freshly-read base, keeping the set-level identity and every - * out-of-scope row, and overlaying only the in-scope rows the user edited. + * from a freshly-read base, keeping every out-of-scope row, and overlaying the + * in-scope rows plus every facet the editor can author + * ({@link EDITOR_AUTHORED_KEYS}). */ export interface ObjectPerm { @@ -43,10 +44,41 @@ export interface PermissionSetDraft { fields?: Record; systemPermissions?: string[]; tabPermissions?: Record; + rowLevelSecurity?: unknown[]; + adminScope?: Record; // Any extra keys are carried through untouched on save. [extra: string]: unknown; } +/** + * The facets the Access matrix editor can AUTHOR — the keys {@link + * mergePermissionSlice} must take from the edited draft rather than from the + * freshly-read base (objectui#4302). + * + * This list is the whole contract of the package door's save: a facet the + * editor can author and this list does not name is silently reverted on Save, + * with a 200 and no error anywhere — which is how a row-level-security policy + * authored in Studio was discarded while the surface showed it as configured. + * `permission-slice.authoredKeys.test.ts` scans the editor sources for the keys + * their `setDraft(...)` updaters write and fails if any is missing here, so the + * next facet added to the editor cannot repeat that silently. + * + * `objects` / `fields` are authored too, but only within the package's scope — + * {@link mergePermissionSlice} carries them through its row-level merge instead + * of wholesale, so other packages' contributions survive (ADR-0086 P0). + */ +export const EDITOR_AUTHORED_KEYS = [ + 'name', + 'label', + 'isDefault', + 'objects', + 'fields', + 'systemPermissions', + 'rowLevelSecurity', + 'tabPermissions', + 'adminScope', +] as const; + /** * Object name embedded in a `${object}.${field}` field-permission key. Object * and field names are field-name-safe (snake_case, no dots), so the object is @@ -87,9 +119,23 @@ export function scopePermissionSet( * * Out-of-scope rows (other packages' contributions) are copied verbatim from * `base`; in-scope rows are taken entirely from `edited` (so removing a grant - * in the package panel deletes only that package's row). Set-level identity and - * any extra keys (systemPermissions, tabPermissions, …) come from `base`, with - * name / label taking the user's edits. + * in the package panel deletes only that package's row). + * + * Everything else follows one rule (objectui#4302): a facet the editor can + * author ({@link EDITOR_AUTHORED_KEYS}) comes from `edited`; `base` supplies + * only what the editor cannot author. It used to be the other way round — five + * named keys from `edited` and every remaining facet from `base` — which meant + * `rowLevelSecurity`, `tabPermissions` and `adminScope` were reverted to the + * stored values the moment they were edited under a package, with a 200 and no + * error. The scoping the package door needs is `objects` / `fields` and nothing + * else: the load path narrows exactly those two maps and hands every other + * facet to the editors unscoped, so a facet held back here protected no one and + * only discarded the author's work. + * + * A key ABSENT from `edited` still comes from `base`. Absence means "this + * caller does not model the facet", not "the author cleared it" — the facet + * editors always write a value (an emptied policy list is `[]`, present), so + * clearing still persists as clearing. */ export function mergePermissionSlice( base: PermissionSetDraft, @@ -114,12 +160,16 @@ export function mergePermissionSlice( if (scopeSet.has(fieldKeyObject(k))) fields[k] = v; } - return { - ...base, - name: edited.name, - label: edited.label, - isDefault: edited.isDefault, - objects, - fields, - }; + const merged: PermissionSetDraft = { ...base }; + for (const key of EDITOR_AUTHORED_KEYS) { + if (key in edited) { + (merged as Record)[key] = (edited as Record)[key]; + } + } + // …except `objects` / `fields`, which are authored per-row: the maps built + // above already carry the in-scope rows from `edited` and every out-of-scope + // row from `base`, so they override the wholesale copies the loop made. + merged.objects = objects; + merged.fields = fields; + return merged; }