fix(app-shell): package-door permission save carries every editor-authorable facet (#4302)

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

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<string, unknown>;
+  /** A pending package draft, if any (ADR-0086 P2 D6). */
+  draft: Record<string, unknown> | null;
+  packageObjects: Array<{ name: string; label?: string }>;
+  objectFields: Record<string, { fields?: unknown } | undefined>;
+  saved: Array<Record<string, unknown>>;
+  savedOpts: Array<Record<string, unknown> | 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<string, unknown>,
+      opts?: Record<string, unknown>,
+    ) => {
+      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(
+    <MemoryRouter>
+      <PermissionMatrixEditPage type="permission" name="sales_perms" packageId={packageId} />
+    </MemoryRouter>,
+  );
+}
+
+/** 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<string, unknown>), 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,
+    });
+  });
+});