diff --git a/.changeset/permission-facets-rls-priority-retired-7130.md b/.changeset/permission-facets-rls-priority-retired-7130.md new file mode 100644 index 0000000000..5cc76da7f7 --- /dev/null +++ b/.changeset/permission-facets-rls-priority-retired-7130.md @@ -0,0 +1,39 @@ +--- +"@object-ui/app-shell": patch +--- + +The Studio RLS editor no longer authors the retired `rowLevelSecurity[].priority` key (objectstack#7130) + +`rowLevelSecurity[].priority` was removed in `@objectstack/spec` 17.0.0 +(objectstack#3896) and left as a `retiredKey` tombstone in +`packages/spec/src/security/rls.zod.ts` — an authored value is REJECTED at parse +time with the upgrade prescription, not ignored. It promised "conflict +resolution" that cannot exist: applicable policies OR-combine (most permissive +wins), so there is no conflict to order. + +`PermissionAdvancedFacets` — the structured RLS editor on the Studio permission +matrix — was still typing the key and seeding `priority: 0` on every policy its +"Add policy" button created. Its docblock described the shapes as mirroring the +framework spec, but that mirror was sampled before the removal. Nothing on the +save path removed the key: `doSave` sends the draft verbatim, and at package +scope `mergePermissionSlice` copies `rowLevelSecurity` from the freshly-read +base while taking only `objects`/`fields` from the edit — so at environment +scope (the only scope where these facets are persisted) the seeded key went +straight into the saved permission set. A user who added an RLS policy through +the editor therefore wrote a permission set the parser refuses. + +Three changes, all editor-side: + +- the local `RlsPolicy` shape drops `priority`; +- the Add-policy seed drops `priority: 0` — it now authors exactly + `{name,object,operation,using,enabled}`; +- policies are stripped of the retired key as they are read out of the draft, so + a permission set already carrying `priority` (written by this editor before + this fix) comes out clean the moment any RLS edit re-emits the list. This is + editor hygiene, not a data migration: a set nobody opens is untouched, and the + strip is keyed to the named tombstone rather than being a blanket unknown-key + purge, so every live key the editor does not itself render survives a + round-trip. + +The docblock stops claiming the shapes are "sampled from live data" — that +sampling is exactly how a removed key stayed in the editor for ten days. diff --git a/packages/app-shell/src/views/metadata-admin/PermissionAdvancedFacets.retiredKeys.test.tsx b/packages/app-shell/src/views/metadata-admin/PermissionAdvancedFacets.retiredKeys.test.tsx new file mode 100644 index 0000000000..14e1c619cd --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/PermissionAdvancedFacets.retiredKeys.test.tsx @@ -0,0 +1,129 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Pins that the RLS facet never authors `rowLevelSecurity[].priority` + * (objectstack#7130). + * + * The key is a `retiredKey` tombstone in `@objectstack/spec` 17.0.0 + * (`packages/spec/src/security/rls.zod.ts` — removed by objectstack#3896): + * an authored value is REJECTED at parse time with the upgrade prescription, + * not ignored. This editor used to seed `priority: 0` on every policy its Add + * button created, so every permission set saved after touching the structured + * RLS editor carried a parse-rejected key. + * + * Two directions are pinned, because the seed is only half of it: + * - the Add-policy seed's key set, so the key cannot quietly return; + * - an edit-and-save round-trip of an ALREADY-poisoned stored policy, which + * every write path spreads (`{ ...pol, name }`) and would otherwise carry + * the key back out verbatim. + */ + +import * as React from 'react'; +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { render, screen, cleanup } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { PermissionAdvancedFacets } from './PermissionAdvancedFacets'; + +afterEach(cleanup); + +const t = (k: string) => k; + +type Draft = Record; + +/** + * Render the facets with a `setDraft` spy, and expose the drafts the component + * asked for — the updater is applied to the draft it was rendered with, which + * is what the host's whole-record Save would then persist. + */ +function renderFacets(draft: Draft) { + const drafts: Draft[] = []; + const setDraft = vi.fn((updater: (prev: Draft) => Draft) => { + drafts.push(updater(draft)); + }); + const props = { + draft, + setDraft, + writable: true, + allSetNames: [] as string[], + t, + }; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + render(); + const policiesAfter = () => { + const last = drafts[drafts.length - 1]; + return (last?.rowLevelSecurity ?? []) as Array>; + }; + return { drafts, policiesAfter }; +} + +/** The RLS facet is collapsed by default — expand it to mount the editors. */ +async function openRls(user: ReturnType) { + await user.click(screen.getByText('perm.rls.title')); +} + +describe('PermissionAdvancedFacets · RLS retired-key hygiene (objectstack#7130)', () => { + it('the Add-policy seed authors exactly the live spec keys — no `priority`', async () => { + const user = userEvent.setup(); + const { policiesAfter } = renderFacets({ rowLevelSecurity: [] }); + await openRls(user); + await user.click(screen.getByRole('button', { name: /perm\.rls\.add/ })); + + const policies = policiesAfter(); + expect(policies).toHaveLength(1); + // Key SET, not just the absence of `priority`: a pin on the whole seed is + // what stops a retired key drifting back in beside a live one. + expect(Object.keys(policies[0]).sort()).toEqual( + ['enabled', 'name', 'object', 'operation', 'using'].sort(), + ); + expect('priority' in policies[0]).toBe(false); + }); + + it('an edit-and-save round-trip of a stored policy carrying `priority` comes out clean', async () => { + const user = userEvent.setup(); + const { policiesAfter } = renderFacets({ + // What this editor itself wrote before objectstack#7130. + rowLevelSecurity: [ + { + name: 'p1', + object: 'account', + operation: 'all', + using: 'owner_id == current_user.id', + check: 'owner_id == current_user.id', + enabled: true, + priority: 0, + }, + ], + }); + await openRls(user); + // Any edit re-emits the whole policy list; the name field is the cheapest. + await user.type(screen.getByPlaceholderText('perm.rls.name'), 'x'); + + const policies = policiesAfter(); + expect(policies).toHaveLength(1); + expect('priority' in policies[0]).toBe(false); + // Falsification: the strip is keyed to the tombstone, not a blanket + // unknown-key purge — every live key the editor does not render survives. + expect(policies[0].check).toBe('owner_id == current_user.id'); + expect(policies[0].using).toBe('owner_id == current_user.id'); + expect(policies[0].enabled).toBe(true); + expect(policies[0].name).toBe('p1x'); + }); + + it('a policy the editor never touches is still emitted without `priority` once any policy is edited', async () => { + const user = userEvent.setup(); + const { policiesAfter } = renderFacets({ + rowLevelSecurity: [ + { name: 'p1', object: 'account', operation: 'all', using: 'true', enabled: true, priority: 0 }, + { name: 'p2', object: 'contact', operation: 'select', using: 'true', enabled: true, priority: 5 }, + ], + }); + await openRls(user); + // Adding a policy re-emits the existing ones alongside the new seed. + await user.click(screen.getByRole('button', { name: /perm\.rls\.add/ })); + + const policies = policiesAfter(); + expect(policies).toHaveLength(3); + expect(policies.some((p) => 'priority' in p)).toBe(false); + expect(policies.map((p) => p.name)).toEqual(['p1', 'p2', '']); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/PermissionAdvancedFacets.tsx b/packages/app-shell/src/views/metadata-admin/PermissionAdvancedFacets.tsx index 5d14af0179..a3ee1defee 100644 --- a/packages/app-shell/src/views/metadata-admin/PermissionAdvancedFacets.tsx +++ b/packages/app-shell/src/views/metadata-admin/PermissionAdvancedFacets.tsx @@ -33,10 +33,12 @@ import type { CelLintIssue } from './celAuthoring'; * in Setup (PermissionFacetLink, P1). Each editor reads/writes the draft's * parsed camelCase field (`rowLevelSecurity` / `tabPermissions` / `adminScope`) * — tolerating a JSON string on load so legacy rows survive — and is persisted - * by the editor's existing whole-record Save. Shapes mirror the framework spec - * (sampled from live data): RLS policies `{name,object,operation,using,check, - * enabled,priority}`; admin scope `{businessUnit,includeSubtree,manage*, - * authorEnvironmentSets,assignablePermissionSets[]}`. + * by the editor's existing whole-record Save. The shapes below are the subset + * of the framework spec this editor authors, checked against the spec schemas + * rather than sampled from live data (objectstack#7130): RLS policies + * `{name,object,operation,using,check,enabled}`; admin scope + * `{businessUnit,includeSubtree,manage*,authorEnvironmentSets, + * assignablePermissionSets[]}`. */ interface RlsPolicy { @@ -46,7 +48,35 @@ interface RlsPolicy { using?: string; check?: string; enabled?: boolean; - priority?: number; +} + +/** + * Keys this editor must never author onto an RLS policy, because the framework + * spec REJECTS them at parse time. + * + * `rowLevelSecurity[].priority` was removed in `@objectstack/spec` 17.0.0 + * (objectstack#3896) and left as a `retiredKey` tombstone + * (`packages/spec/src/security/rls.zod.ts`): an authored value is refused with + * the upgrade prescription rather than ignored. Applicable policies OR-combine + * (most permissive wins), so the "conflict resolution" it promised cannot + * exist and there is nothing to preserve. + * + * Until objectstack#7130 this editor seeded `priority: 0` on every policy its + * Add button created, so drafts it wrote can still carry the key. `policies` + * below is the single value every write path spreads from, so stripping on + * load — not a data migration — is what makes an edit-and-save round-trip of + * such a policy come out parseable. + */ +const RETIRED_RLS_KEYS = ['priority'] as const; + +/** Drop {@link RETIRED_RLS_KEYS} from a policy read out of the draft. */ +function stripRetiredRlsKeys(policy: RlsPolicy): RlsPolicy { + if (!policy || typeof policy !== 'object') return policy; + const present = RETIRED_RLS_KEYS.filter((k) => k in policy); + if (present.length === 0) return policy; + const next: Record = { ...policy }; + for (const k of present) delete next[k]; + return next as RlsPolicy; } interface AdminScope { @@ -158,7 +188,10 @@ export function PermissionAdvancedFacets({ onCelErrorsChange, t, }: PermissionAdvancedFacetsProps) { - const policies = React.useMemo(() => asArray(draft.rowLevelSecurity), [draft.rowLevelSecurity]); + const policies = React.useMemo( + () => asArray(draft.rowLevelSecurity).map(stripRetiredRlsKeys), + [draft.rowLevelSecurity], + ); const scope = React.useMemo(() => asObject(draft.adminScope), [draft.adminScope]); const tabs = React.useMemo(() => asObject(draft.tabPermissions), [draft.tabPermissions]); @@ -365,7 +398,7 @@ export function PermissionAdvancedFacets({ onClick={() => setPolicies([ ...policies, - { name: '', object: '*', operation: 'all', using: '', enabled: true, priority: 0 }, + { name: '', object: '*', operation: 'all', using: '', enabled: true }, ]) } >