fix(app-shell): stop authoring the retired rowLevelSecurity[].priority key in the Studio RLS editor (objectstack#7130)

.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.

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<string, unknown>;
+
+/**
+ * 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(<PermissionAdvancedFacets {...(props as any)} />);
+  const policiesAfter = () => {
+    const last = drafts[drafts.length - 1];
+    return (last?.rowLevelSecurity ?? []) as Array<Record<string, unknown>>;
+  };
+  return { drafts, policiesAfter };
+}
+
+/** The RLS facet is collapsed by default — expand it to mount the editors. */
+async function openRls(user: ReturnType<typeof userEvent.setup>) {
+  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', '']);
+  });
+});

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<string, unknown> = { ...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<RlsPolicy[]>(() => asArray(draft.rowLevelSecurity), [draft.rowLevelSecurity]);
+  const policies = React.useMemo<RlsPolicy[]>(
+    () => asArray<RlsPolicy>(draft.rowLevelSecurity).map(stripRetiredRlsKeys),
+    [draft.rowLevelSecurity],
+  );
   const scope = React.useMemo<AdminScope>(() => asObject(draft.adminScope), [draft.adminScope]);
   const tabs = React.useMemo<TabPerms>(() => 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 },
                 ])
               }
             >