From 3e8d63500c9b09cedb31248b60a5730a226db422 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 26 Aug 2026 09:11:20 +0000 Subject: [PATCH] fix(app-shell): publish the shared `current_user` normaliser from `RecordFormPage` (#6515) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `RecordFormPage` built its own predicate identity — `{ name, email, role, positions }` — instead of calling `buildExpressionUser`. Against the normaliser that shape is missing `id` and `isPlatformAdmin`, both named by real gates (`ctx.user.isPlatformAdmin == true` on `sys_environment`'s "Change Plan (admin)"; `record.id == ctx.user.id` throughout `sys_user`). An absent key is not `false`: the predicate FAULTS, and a faulting visibility predicate fails OPEN, so the gate silently did not bite. The signed-out branch diverged on its own account too — it carried no `isPlatformAdmin` key at all. The normaliser moves from `console/AppContent.tsx` to `providers/expressionUser.ts`, a leaf module beside the `ExpressionProvider` it feeds. That move is what makes the fix available at all: this view is `lazy()`-loaded BY `AppContent`, so importing the normaliser from its old home would put a static edge from the split chunk back into the module it was split out of — the edge `check-eager-closure-budget` weighs. `console/AppContent.js` and the package entry both re-export the name, so nothing published moved. Fail-open on a genuine evaluation error is deliberately unchanged (objectui#6443 / #6487 / #6445); what changed is that these predicates no longer fault. Adds the shape pin objectui#6110's contract implied but never got: a render-level assertion that a mount site publishes exactly `buildExpressionUser(session)` under all four identity spellings, plus a source ratchet that refuses a NEW mount site which derives the descriptor by hand. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011SfZeFWrhGLHmfq61xbz4q --- ...515-record-form-current-user-normaliser.md | 28 ++ packages/app-shell/src/console/AppContent.tsx | 56 +--- packages/app-shell/src/index.ts | 9 +- .../expressionUser.mountParity.test.tsx | 298 ++++++++++++++++++ .../expressionUser.mountSites.ratchet.test.ts | 233 ++++++++++++++ .../app-shell/src/providers/expressionUser.ts | 81 +++++ .../app-shell/src/views/RecordFormPage.tsx | 33 +- 7 files changed, 681 insertions(+), 57 deletions(-) create mode 100644 .changeset/6515-record-form-current-user-normaliser.md create mode 100644 packages/app-shell/src/providers/expressionUser.mountParity.test.tsx create mode 100644 packages/app-shell/src/providers/expressionUser.mountSites.ratchet.test.ts create mode 100644 packages/app-shell/src/providers/expressionUser.ts diff --git a/.changeset/6515-record-form-current-user-normaliser.md b/.changeset/6515-record-form-current-user-normaliser.md new file mode 100644 index 0000000000..759d69feeb --- /dev/null +++ b/.changeset/6515-record-form-current-user-normaliser.md @@ -0,0 +1,28 @@ +--- +'@object-ui/app-shell': patch +--- + +Field- and action-visibility gates on the full-screen record form page now see the same +`current_user` every other console surface sees (objectui#6515). `RecordFormPage` built its +own descriptor — `{ name, email, role, positions }` — instead of calling the shared +`buildExpressionUser` normaliser, so `id` and `isPlatformAdmin` were simply absent from the +predicate scope that page publishes. + +An absent key is not `false`. A predicate naming one of them FAULTS, and a faulting +visibility predicate fails OPEN, so the gate silently did not bite: a field gated on +`ctx.user.isPlatformAdmin == true` (the shape `sys_environment`'s "Change Plan (admin)" +action uses) rendered for every user on this page, and an id comparison against +`ctx.user.id` (the shape `sys_user`'s own gates use throughout `platform-objects`) did the +same. Nothing on screen distinguished that from a gate that had said yes. The signed-out +branch diverged on its own account too — it carried no `isPlatformAdmin` key at all, where +`buildExpressionUser(null)` carries `false`. + +Fail-open on a genuine evaluation error is deliberately unchanged (objectui#6443 / #6487 / +#6445); what changed is that these predicates no longer fault in the first place. + +The normaliser moved from `console/AppContent.tsx` to `providers/expressionUser.ts`, beside +the `ExpressionProvider` it feeds. That move is what made the fix available: `RecordFormPage` +is `lazy()`-loaded BY `AppContent`, so importing the normaliser from its old home would have +put a static edge from the split chunk back into the module it was split out of. Both +`console/AppContent.js` and the package entry re-export the name, so `buildExpressionUser` +is published exactly as before. diff --git a/packages/app-shell/src/console/AppContent.tsx b/packages/app-shell/src/console/AppContent.tsx index b3f7e36608..0963374bd8 100644 --- a/packages/app-shell/src/console/AppContent.tsx +++ b/packages/app-shell/src/console/AppContent.tsx @@ -27,6 +27,7 @@ import { createExpressionEvaluator, evaluateVisibility, } from '../providers/ExpressionProvider.js'; +import { buildExpressionUser } from '../providers/expressionUser.js'; import { useTrackRouteAsRecent } from '../hooks/useTrackRouteAsRecent.js'; import { resolveRecordFormTarget, resolveFormViewLayout, resolveNavigateCreateUrl, resolveNavigateEditUrl, resolvePostCreateTarget } from '../utils/recordFormNavigation.js'; import { deriveRecordSurface, deriveRecordFlowSurface } from '@object-ui/plugin-view'; @@ -119,50 +120,21 @@ function DraftReviewNavigator({ appName }: { appName: string | undefined }) { * The predicate-evaluation identity `ExpressionProvider` binds as * `current_user` / `ctx.user` / `os.user`. * - * Extracted from `AppContent`'s body in objectui#5424 so the SHAPE it - * advertises is assertable on its own — the defect it carried was a key that - * was always `undefined`, which no render-level assertion can see. + * MOVED to `../providers/expressionUser.js` in objectui#6515 and re-exported + * here so the published name is unchanged. It moved because `AppContent` + * `lazy()`-loads `views/RecordFormPage.tsx`, which mounts an + * `ExpressionProvider` of its own: importing the normaliser from THIS module + * would put a static edge from that split chunk back into the module it was + * split out of — the edge `scripts/check-eager-closure-budget.mjs` weighs — so + * the view hand-rolled a narrower descriptor instead, and the `id` / + * `isPlatformAdmin` it dropped made every predicate naming them FAULT and fail + * OPEN. The new home is a leaf module beside the provider it feeds, which every + * mount site can import without dragging a chunk along. * - * `roles` is deliberately ABSENT, not merely empty. It used to be forwarded as - * `roles: (user as any).roles`, and the protocol-17 session face emits no - * `roles` key at all (framework ADR-0090 D3 renamed it to `positions` with no - * deprecation window — measured in objectui#5389), so the key reached every CEL - * predicate as `undefined`: an author writing `'manager' in current_user.roles` - * got a shape that answered, wrongly, rather than one that was plainly not - * there. `positions` below is the published spelling and carries the same - * names. Not paired as a fallback — that is what ADR-0090 D3 forbids - * (`packages/auth/src/types.ts`). - * - * The signed-out branch never had `roles` either, so removing it also makes the - * two branches agree on one shape. + * This re-export is the back-compat half of that move; the doc comment that + * explains the SHAPE lives with the function. */ -export function buildExpressionUser(user: unknown): Record { - const u = user as - | { id?: string; name?: string; email?: string; role?: string; [key: string]: unknown } - | null - | undefined; - if (!u) { - return { name: 'Anonymous', email: '', role: 'guest', isPlatformAdmin: false, positions: [] }; - } - return { - id: u.id, - name: u.name, - email: u.email, - role: u.role ?? 'user', - // Surface the platform-admin flag so action `visible` CEL predicates - // gated on `ctx.user.isPlatformAdmin == true` (e.g. sys_environment - // "Change Plan (admin)") evaluate correctly. Previously only - // name/email/role were forwarded → isPlatformAdmin-gated actions were - // hidden even for platform admins. - isPlatformAdmin: u.isPlatformAdmin ?? false, - // Positions are what the SERVER binds as `current_user` for per-option - // `visibleWhen` authorization gating (ADR-0058; framework EvalUser — - // objectui#2284). Forwarding them lets a position-gated option - // (`'admin' in current_user.positions`) hide client-side too, instead - // of failing open as visible and only being rejected on submit. - positions: u.positions ?? [], - }; -} +export { buildExpressionUser }; export function AppContent({ extraRoutes, extraRoutesNoApp }: AppContentProps = {}) { const [connectionState, setConnectionState] = useState('disconnected'); diff --git a/packages/app-shell/src/index.ts b/packages/app-shell/src/index.ts index 6742326c8c..1e8c4fc603 100644 --- a/packages/app-shell/src/index.ts +++ b/packages/app-shell/src/index.ts @@ -20,8 +20,15 @@ export { ExpressionProvider, useExpressionContext, evaluateVisibility } from './ * `'x' in current_user.positions` an unbound-key fault, which fails OPEN, so a * second mount site re-deriving this by hand would reintroduce exactly the * asymmetry #6010's parity pin exists to refuse. + * + * objectui#6515 — that is exactly what `RecordFormPage` did, because the + * normaliser used to live in `console/AppContent.js` and the view is + * `lazy()`-loaded BY that module. The specifier below moved to the leaf module + * beside the provider it feeds; the NAME published from this entry did not. + * `console/AppContent.js` re-exports it too, for importers already reaching it + * there. */ -export { buildExpressionUser } from './console/AppContent.js'; +export { buildExpressionUser } from './providers/expressionUser.js'; // Hooks export { useObjectActions } from './hooks/useObjectActions.js'; diff --git a/packages/app-shell/src/providers/expressionUser.mountParity.test.tsx b/packages/app-shell/src/providers/expressionUser.mountParity.test.tsx new file mode 100644 index 0000000000..e48372dff0 --- /dev/null +++ b/packages/app-shell/src/providers/expressionUser.mountParity.test.tsx @@ -0,0 +1,298 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#6515 — the SHAPE pin objectui#6110's contract implies but never got. + * + * ## What went wrong, and why nothing caught it + * + * objectui#6110 exported `buildExpressionUser` so that "every console surface + * that mounts the provider publishes the SAME `current_user` shape", and said + * in as many words that a second mount site re-deriving the shape by hand + * "would reintroduce exactly the asymmetry #6010's parity pin exists to + * refuse". `RecordFormPage` was such a site and did exactly that: + * + * { name, email, role, positions } // the hand-rolled descriptor + * + * — no `id`, no `isPlatformAdmin`. Both are named by real gates + * (`ctx.user.isPlatformAdmin == true` on `sys_environment`'s "Change Plan + * (admin)"; `record.id == ctx.user.id` throughout `sys_user`). An ABSENT key is + * not `false`: the predicate FAULTS, and `evaluateVisibility` fails OPEN on a + * fault, so an admin-only field rendered for every user with nothing on screen + * to say the gate had not bitten. objectui#6493 fixed which ROOTS were bound on + * this page; it did not touch the SHAPE of the object bound under them. + * + * Every pin that existed asserted the normaliser's own output + * (`AppContent.expressionUserShape.test.ts`). None asserted what a MOUNT SITE + * publishes, so a site that never called the normaliser was invisible to all of + * them. + * + * ## What this file asserts, and why "shape" and not "was it called" + * + * The probe sits INSIDE the page's own `ExpressionProvider` and reads the + * predicate scope that provider actually publishes — the same + * `PredicateScopeContext` `useCondition` / `useExpression` read at every + * consumer. The assertion is `toStrictEqual(buildExpressionUser(session))` over + * all four published spellings of the identity. That is a SHAPE comparison + * against the normaliser's live output, so it fails for a site that hand-rolls + * a descriptor whatever route it took to build it — including one that calls + * the normaliser and then spreads extra keys over it, which a spy on the + * function could not see. `toStrictEqual` (not `toEqual`) so a key written as + * explicit `undefined` fails too: that is precisely the class of defect + * objectui#5424 measured on this same object. + * + * `mountSites.ratchet.test.ts` is the other half — this file proves the shape + * at the site it can render, that one refuses a NEW site that stops calling the + * normaliser at all. Neither is sufficient alone. + * + * ## Reverse verification (direction predicted BEFORE running) + * + * Restore the hand-rolled descriptor in `RecordFormPage`: + * - both parity cases go RED on the missing `id` / `isPlatformAdmin`; + * - `hides an isPlatformAdmin-gated field from a non-admin` goes RED as + * `toContain` — the fault fails OPEN, so the field is PRESENT; + * - the same for the signed-IN `ctx.user.id` case, and for the signed-out + * `isPlatformAdmin` case; + * - `RECORDS: a ctx.user.id gate STILL fails open for a signed-out visitor` + * stays GREEN both ways — it measures the normaliser's own anonymous + * branch, which this card does not touch; + * - `grants ... to a platform admin` stays GREEN — fail-open and a correct + * `true` are indistinguishable at the call site, which is the whole reason + * the excluded-user cases carry the pin. + */ + +import '@testing-library/jest-dom/vitest'; +import * as React from 'react'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render, cleanup, waitFor } from '@testing-library/react'; +import { MemoryRouter, Routes, Route } from 'react-router-dom'; +import { I18nProvider } from '@object-ui/i18n'; +// Not on the `@object-ui/react` barrel. The vitest alias maps that barrel to +// `packages/react/src`, so this deep path is the SAME module instance the +// `ExpressionProvider` under test publishes through (the idiom +// `anonSeedScope-5746.enumeration.test.tsx` uses for the same reason). +import { usePredicateScope } from '../../../react/src/hooks/useExpression'; +import { buildExpressionUser } from './expressionUser'; +import { RecordFormPage } from '../views/RecordFormPage'; + +const h = React.createElement; + +const { publishedScopes, formSchemas, getAuthConfig, authState } = vi.hoisted(() => ({ + publishedScopes: [] as Record[], + formSchemas: [] as any[], + getAuthConfig: vi.fn(async () => ({ features: {} as Record })), + authState: { + user: null as Record | null, + activeOrganization: null as { name: string } | null, + }, +})); + +vi.mock('sonner', () => ({ + toast: Object.assign(vi.fn(), { + success: vi.fn(), error: vi.fn(), info: vi.fn(), + warning: vi.fn(), loading: vi.fn(), dismiss: vi.fn(), + }), +})); + +vi.mock('@object-ui/auth', () => ({ + useAuth: () => ({ + get user() { return authState.user; }, + getAuthConfig, + get activeOrganization() { return authState.activeOrganization; }, + }), +})); + +/** + * The probe. It stands in for `ObjectForm`, which is the page's only child + * inside its `ExpressionProvider`, so what it reads is what a real descendant + * of that provider reads — no interception, no spy on the provider. + */ +vi.mock('@object-ui/plugin-form', () => ({ + ObjectForm: ({ schema }: any) => { + formSchemas.push(schema); + publishedScopes.push(usePredicateScope()); + return h('div', { 'data-testid': 'object-form' }); + }, +})); + +const metadataState = { objects: [] as any[], loading: false }; +vi.mock('../providers/MetadataProvider', () => ({ useMetadata: () => metadataState })); +vi.mock('../providers/AdapterProvider', () => ({ useAdapter: () => null })); + +/** The served shape: `ExpressionInputSchema` normalises authored strings into this. */ +const cel = (source: string) => ({ dialect: 'cel', source }); + +const CONTACTS = { + name: 'contacts', + label: 'Contacts', + fields: { + name: { type: 'text' }, + // The `sys_environment` "Change Plan (admin)" gate, verbatim in shape. + plan: { type: 'text', visible: cel('ctx.user.isPlatformAdmin == true') }, + // The same gate under the canonical spelling. + plan_canonical: { type: 'text', visible: cel('current_user.isPlatformAdmin == true') }, + // `sys_user`'s own gates compare a record id against `ctx.user.id`; this is + // the reachable half of that shape on a page with no `record` root. + self_note: { type: 'text', visible: cel("ctx.user.id == 'u_admin'") }, + }, +}; + +/** A measured protocol-17 session: a plain user, no platform-admin grant. */ +const CLERK = { + id: 'u_clerk', name: 'Bo', email: 'bo@example.com', role: 'user', + positions: ['sales_clerk'], +}; + +/** The same face for a permission-set-derived platform admin. */ +const ADMIN = { + id: 'u_admin', name: 'Ada', email: 'ada@example.com', role: 'user', + positions: ['user', 'platform_admin'], isPlatformAdmin: true, +}; + +function renderPage() { + return render( + h(I18nProvider, { + config: { defaultLanguage: 'en', detectBrowserLanguage: false }, + children: h( + MemoryRouter, + { initialEntries: ['/apps/crm/contacts/new'] }, + h(Routes, null, h(Route, { + path: '/apps/:appName/:objectName/new', + element: h(RecordFormPage, { mode: 'create' }), + })), + ), + }), + ); +} + +/** + * The LAST published scope / field list. `features` is fetched, so the first + * render is always the pre-fetch one; reading anything but the last would + * assert against a bag that is not the one the page settles on. + */ +const lastScope = (): Record => publishedScopes[publishedScopes.length - 1]; +const lastFields = (): string[] => formSchemas[formSchemas.length - 1].fields; + +beforeEach(() => { + metadataState.objects = [CONTACTS]; + metadataState.loading = false; + authState.user = CLERK; + authState.activeOrganization = null; + getAuthConfig.mockResolvedValue({ features: {} }); + publishedScopes.length = 0; + formSchemas.length = 0; +}); +afterEach(cleanup); + +describe('objectui#6515 — the record form page publishes the normaliser’s `current_user`, not a hand-rolled one', () => { + it('publishes exactly `buildExpressionUser(session)` under all four identity spellings', async () => { + authState.user = ADMIN; + renderPage(); + await waitFor(() => expect(publishedScopes.length).toBeGreaterThan(0)); + + const expected = buildExpressionUser(ADMIN); + const scope = lastScope(); + + // The pin. Before the fix this read + // `{ name, email, role, positions }` — two keys short. + expect(scope.current_user).toStrictEqual(expected); + // ADR-0068 D1 binds one object under four names; asserting all four is what + // stops a site publishing a normalised `current_user` and a private object + // under an alias. + expect(scope.user).toStrictEqual(expected); + expect(scope.ctx.user).toStrictEqual(expected); + expect(scope.os.user).toStrictEqual(expected); + }); + + it('publishes exactly `buildExpressionUser(null)` on the signed-out branch', async () => { + // The signed-out branch diverged on its own: it carried no `isPlatformAdmin` + // key at all, so `ctx.user.isPlatformAdmin == true` faulted for a signed-out + // visitor rather than answering FALSE. + authState.user = null; + renderPage(); + await waitFor(() => expect(publishedScopes.length).toBeGreaterThan(0)); + + const expected = buildExpressionUser(null); + expect(expected.isPlatformAdmin).toBe(false); + expect(lastScope().current_user).toStrictEqual(expected); + expect(lastScope().ctx.user).toStrictEqual(expected); + }); + + it('hides an `isPlatformAdmin`-gated field from a non-admin', async () => { + authState.user = CLERK; + renderPage(); + await waitFor(() => expect(formSchemas.length).toBeGreaterThan(0)); + + // Ungated fields are unaffected — the filter still lets everything else by. + expect(lastFields()).toContain('name'); + // Both were `toContain` before the fix: the key was absent, the predicate + // faulted, and `evaluateVisibility` fails OPEN — so the admin-only field + // was on screen for a sales clerk. + expect(lastFields()).not.toContain('plan'); + expect(lastFields()).not.toContain('plan_canonical'); + }); + + it('hides a `ctx.user.id`-gated field from the user it excludes', async () => { + authState.user = CLERK; + renderPage(); + await waitFor(() => expect(formSchemas.length).toBeGreaterThan(0)); + + // `id` was the other dropped key. Same fault, same fail-open. + expect(lastFields()).not.toContain('self_note'); + }); + + it('hides the `isPlatformAdmin` gate from a signed-out visitor', async () => { + authState.user = null; + renderPage(); + await waitFor(() => expect(formSchemas.length).toBeGreaterThan(0)); + + expect(lastFields()).toContain('name'); + // Was `toContain` before the fix: the hand-rolled anonymous descriptor + // carried no `isPlatformAdmin` key at all, so the gate faulted and failed + // open for a visitor with no session whatsoever. + expect(lastFields()).not.toContain('plan'); + expect(lastFields()).not.toContain('plan_canonical'); + }); + + it('RECORDS: a `ctx.user.id` gate STILL fails open for a signed-out visitor', async () => { + // Measurement, not endorsement — and NOT something this card changes. + // + // `buildExpressionUser(null)` returns `{ name, email, role, + // isPlatformAdmin, positions }`. There is no `id` key on that branch, so + // `ctx.user.id == '…'` faults for a signed-out visitor and fails OPEN, at + // EVERY mount site — `AppContent` and `InternalFormRoute` included, both of + // which have always called the normaliser. That asymmetry is in the + // normaliser's own anonymous branch, not in this page, and closing it means + // changing the shape `AppContent.expressionUserShape.test.ts` pins, which + // is a decision of its own (`id: null`? `id: undefined`? — objectui#5424 + // rejected present-and-undefined as the shape that teaches the wrong + // thing). Filed separately rather than widened into this PR. + // + // Pinned so the follow-up has a red test to turn green, and so this stays + // a measured fact rather than an assumption. The signed-IN case above is + // the one this card fixes: there `id` is present and the gate bites. + authState.user = null; + renderPage(); + await waitFor(() => expect(formSchemas.length).toBeGreaterThan(0)); + + expect('id' in buildExpressionUser(null)).toBe(false); + expect(lastFields()).toContain('self_note'); + }); + + it('grants the `isPlatformAdmin` gate to a platform admin', async () => { + // Control. GREEN before and after — a fault and a true both render the + // field, which is exactly why it cannot stand in for the cases above. + authState.user = ADMIN; + renderPage(); + await waitFor(() => expect(formSchemas.length).toBeGreaterThan(0)); + + expect(lastFields()).toContain('plan'); + expect(lastFields()).toContain('plan_canonical'); + expect(lastFields()).toContain('self_note'); + }); +}); diff --git a/packages/app-shell/src/providers/expressionUser.mountSites.ratchet.test.ts b/packages/app-shell/src/providers/expressionUser.mountSites.ratchet.test.ts new file mode 100644 index 0000000000..41d8e17484 --- /dev/null +++ b/packages/app-shell/src/providers/expressionUser.mountSites.ratchet.test.ts @@ -0,0 +1,233 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#6515 ratchet — no mount site derives `current_user` by hand. + * + * ## Why a ratchet and not just the render test + * + * `expressionUser.mountParity.test.tsx` asserts the SHAPE a mount site + * publishes, which is the assertion that matters — but it can only assert it + * for the sites it renders. This card exists because a site that NEVER called + * the normaliser was invisible to every pin in the repo: objectui#6110 exported + * `buildExpressionUser` and wrote down that a second mount site re-deriving the + * shape by hand "would reintroduce exactly the asymmetry #6010's parity pin + * exists to refuse", and then `RecordFormPage` did precisely that, undetected, + * across objectui#5424, #6010 and #6493. + * + * So this half enumerates the mount sites from SOURCE and refuses one that + * builds its own descriptor. A third site added tomorrow is caught here even + * though no render test knows it exists. + * + * ## What counts as a mount site + * + * The two ways this tier binds an identity into a predicate scope: + * - `` — the declarative one; + * - `createExpressionEvaluator({ user: … })` — the imperative one, used where + * a surface builds its field list ABOVE the provider it mounts and so + * cannot read the scope back through the hook. + * + * `providers/ExpressionProvider.tsx` is exempt: it is the SEAM, not a mount + * site. It forwards the `user` prop its caller handed it, and the callers are + * what this file checks. + * + * ## If this fails + * + * Do not add an accept pattern. Call `buildExpressionUser(user)` — the single + * normaliser, importable from `providers/expressionUser.js` (a leaf module, so + * a `lazy()`-loaded view can import it without a static edge back into + * `AppContent`, which is what made the original site hand-roll it). + */ + +import { describe, it, expect } from 'vitest'; +import { readdirSync, readFileSync, statSync } from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +import { buildExpressionUser } from './expressionUser'; +// The back-compat re-export. objectui#6515 moved the function out of this +// module; `toBe` (identity, not shape) is what proves the name still resolves +// to the SAME function rather than to a second copy that could drift. +import { buildExpressionUser as viaAppContent } from '../console/AppContent'; + +const here = path.dirname(fileURLToPath(import.meta.url)); +// packages/app-shell/src/providers -> repo root +const repoRoot = path.resolve(here, '../../../..'); + +/** Roots that hold this tier's expression mount sites. */ +const SCAN_ROOTS = ['packages/app-shell/src', 'apps/console/src']; + +/** The seam itself — it forwards its caller's prop; its callers are the sites. */ +const EXEMPT = new Set([ + path.join('packages', 'app-shell', 'src', 'providers', 'ExpressionProvider.tsx'), +]); + +function collectSourceFiles(): string[] { + const out: string[] = []; + const walk = (dir: string) => { + let entries; + try { + entries = readdirSync(dir, { withFileTypes: true }); + } catch { + return; + } + for (const entry of entries) { + const name = entry.name; + if (name === 'node_modules' || name === 'dist' || name.startsWith('.wt-') || name === '__tests__') { + continue; + } + const full = path.join(dir, name); + if (entry.isDirectory()) { + walk(full); + continue; + } + if (!/\.tsx?$/.test(name)) continue; + // Production sources only. Tests legitimately mount the provider with a + // literal fixture user — that is the point of a fixture. + if (/\.(test|spec|stories)\.tsx?$/.test(name)) continue; + if (statSync(full).isFile()) out.push(full); + } + }; + for (const root of SCAN_ROOTS) walk(path.join(repoRoot, root)); + return out; +} + +/** + * Blank out comment LINES before matching, so a doc-comment EXAMPLE + * (`ExpressionProvider.tsx`'s own `@example` block mounts the provider with a + * `currentUser` that does not exist) is not read as a mount site. + * + * Line-based on purpose. The obvious `/\*[\s\S]*?\*\//` strip is WRONG on real + * sources and was measured wrong on `AppContent.tsx` here: a `/*` that occurs + * inside a string or a `//` comment pairs with the next REAL `*\/`, and the + * non-greedy span silently swallows the code between them — it ate the very + * `createExpressionEvaluator` call this file exists to find, leaving a scan + * that reported no violations because it had deleted the site. Dropping whole + * comment lines cannot span code. + */ +function stripComments(src: string): string { + return src + .split('\n') + .map((line) => (/^\s*(?:\*|\/\/|\/\*)/.test(line) ? '' : line)) + .join('\n'); +} + +/** Balanced-brace read starting AT the opening brace. */ +function readBalanced(src: string, open: number): string { + let depth = 0; + for (let i = open; i < src.length; i++) { + if (src[i] === '{') depth++; + else if (src[i] === '}') { + depth--; + if (depth === 0) return src.slice(open + 1, i); + } + } + return src.slice(open + 1); +} + +interface Site { + file: string; + kind: 'ExpressionProvider' | 'createExpressionEvaluator'; + expression: string; +} + +function collectSites(file: string, src: string): Site[] { + const sites: Site[] = []; + + // Declarative: `` + const jsx = /', m.index); + const tag = src.slice(m.index, close === -1 ? src.length : close); + const at = tag.indexOf('user={'); + if (at === -1) continue; + sites.push({ + file, + kind: 'ExpressionProvider', + expression: readBalanced(tag, at + 'user='.length).trim(), + }); + } + + // Imperative: `createExpressionEvaluator({ ... user: EXPR ... })` + const call = /createExpressionEvaluator\s*\(\s*\{/g; + while ((m = call.exec(src))) { + const body = readBalanced(src, src.indexOf('{', m.index)); + const user = /(?:^|[,{\n])\s*user\s*:\s*([^\n]+?)\s*,?\s*$/m.exec(body); + if (!user) continue; + sites.push({ file, kind: 'createExpressionEvaluator', expression: user[1].trim() }); + } + + return sites; +} + +/** + * Does this `user` expression trace to the normaliser? + * + * Either it calls it outright, or it is a bare identifier whose declaration in + * the same file calls it. Anything else — an object literal, a conditional, a + * spread over the normaliser's output — is a hand-rolled descriptor. + */ +function tracesToNormaliser(expression: string, src: string): boolean { + if (/\bbuildExpressionUser\s*\(/.test(expression)) return true; + const ident = /^[A-Za-z_$][\w$]*$/.exec(expression); + if (!ident) return false; + const decl = new RegExp( + `\\b(?:const|let|var)\\s+${ident[0]}\\b[^;]*?\\bbuildExpressionUser\\s*\\(`, + 's', + ); + return decl.test(src); +} + +describe('objectui#6515 — every expression mount site binds the shared `current_user` normaliser', () => { + const files = collectSourceFiles(); + + it('finds the mount sites it claims to govern', () => { + // A scan that silently matches nothing is the failure mode this guards + // against in itself — it would pass forever. The three production sites + // today are `AppContent` (both kinds), `RecordFormPage` (both kinds) and + // the console's `InternalFormRoute`. + const sites = files + .filter((f) => !EXEMPT.has(path.relative(repoRoot, f))) + .flatMap((f) => collectSites(f, stripComments(readFileSync(f, 'utf8')))); + expect(sites.length).toBeGreaterThanOrEqual(5); + expect(new Set(sites.map((s) => path.basename(s.file)))).toEqual( + new Set(['AppContent.tsx', 'RecordFormPage.tsx', 'InternalFormRoute.tsx']), + ); + }); + + it('refuses a mount site that derives the descriptor by hand', () => { + const offenders: string[] = []; + for (const file of files) { + const rel = path.relative(repoRoot, file); + if (EXEMPT.has(rel)) continue; + const src = stripComments(readFileSync(file, 'utf8')); + for (const site of collectSites(file, src)) { + if (!tracesToNormaliser(site.expression, src)) { + offenders.push(`${rel} [${site.kind}] user={${site.expression}}`); + } + } + } + expect(offenders).toEqual([]); + }); +}); + +describe('objectui#6515 — the published name did not move', () => { + it('still resolves through `console/AppContent`, to the SAME function', () => { + // A source `export` keyword is not the test; identity is. A second copy + // would satisfy every `export` grep and still drift. + expect(viaAppContent).toBe(buildExpressionUser); + }); + + it('is still re-exported from the package entry', () => { + // The entry's BUILT `.d.ts` is checked in the PR body against a real + // rebuild; this is the cheap regression pin beside it. + const entry = readFileSync(path.join(repoRoot, 'packages/app-shell/src/index.ts'), 'utf8'); + expect(entry).toMatch(/export\s*\{\s*buildExpressionUser\s*\}\s*from/); + }); +}); diff --git a/packages/app-shell/src/providers/expressionUser.ts b/packages/app-shell/src/providers/expressionUser.ts new file mode 100644 index 0000000000..805b2084b7 --- /dev/null +++ b/packages/app-shell/src/providers/expressionUser.ts @@ -0,0 +1,81 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * The predicate-evaluation identity `ExpressionProvider` binds as + * `current_user` / `ctx.user` / `os.user`. + * + * Extracted from `AppContent`'s body in objectui#5424 so the SHAPE it + * advertises is assertable on its own — the defect it carried was a key that + * was always `undefined`, which no render-level assertion can see. + * + * ## Why it lives HERE and not in `AppContent` (objectui#6515) + * + * It sat in `console/AppContent.tsx`, and `AppContent` `lazy()`-loads the views + * that also mount an `ExpressionProvider` — `views/RecordFormPage.tsx` is one. + * A view importing the normaliser from its old home would create a STATIC edge + * from the lazily-split chunk back into the console module it was split out of, + * which is precisely what `scripts/check-eager-closure-budget.mjs` weighs. So + * the site that most needed the shared normaliser was the one site that could + * not afford to import it, and it hand-rolled the descriptor instead — missing + * `id` and `isPlatformAdmin`, the two roots real predicates name + * (`ctx.user.isPlatformAdmin == true` gates `sys_environment`'s "Change Plan + * (admin)" action; `record.id == ctx.user.id` is the shape `sys_user`'s own + * gates use). An absent key is not `false`: it makes the predicate FAULT, and a + * faulting visibility predicate fails OPEN (`evaluateVisibility`), so the gate + * silently did not bite. + * + * This module is a LEAF — it imports nothing — so every mount site can reach it + * without dragging a chunk along, and it sits beside the `ExpressionProvider` + * it feeds rather than beside one of its callers. `AppContent` and the package + * index both re-export the name, so nothing published moved. + * + * ## Shape notes + * + * `roles` is deliberately ABSENT, not merely empty. It used to be forwarded as + * `roles: (user as any).roles`, and the protocol-17 session face emits no + * `roles` key at all (framework ADR-0090 D3 renamed it to `positions` with no + * deprecation window — measured in objectui#5389), so the key reached every CEL + * predicate as `undefined`: an author writing `'manager' in current_user.roles` + * got a shape that answered, wrongly, rather than one that was plainly not + * there. `positions` below is the published spelling and carries the same + * names. Not paired as a fallback — that is what ADR-0090 D3 forbids + * (`packages/auth/src/types.ts`). + * + * The signed-out branch never had `roles` either, so removing it also makes the + * two branches agree on one shape. Both branches carry `isPlatformAdmin` and + * `positions` for the same reason: a predicate naming either must evaluate to + * FALSE, not fault. + */ +export function buildExpressionUser(user: unknown): Record { + const u = user as + | { id?: string; name?: string; email?: string; role?: string; [key: string]: unknown } + | null + | undefined; + if (!u) { + return { name: 'Anonymous', email: '', role: 'guest', isPlatformAdmin: false, positions: [] }; + } + return { + id: u.id, + name: u.name, + email: u.email, + role: u.role ?? 'user', + // Surface the platform-admin flag so action `visible` CEL predicates + // gated on `ctx.user.isPlatformAdmin == true` (e.g. sys_environment + // "Change Plan (admin)") evaluate correctly. Previously only + // name/email/role were forwarded → isPlatformAdmin-gated actions were + // hidden even for platform admins. + isPlatformAdmin: u.isPlatformAdmin ?? false, + // Positions are what the SERVER binds as `current_user` for per-option + // `visibleWhen` authorization gating (ADR-0058; framework EvalUser — + // objectui#2284). Forwarding them lets a position-gated option + // (`'admin' in current_user.positions`) hide client-side too, instead + // of failing open as visible and only being rejected on submit. + positions: u.positions ?? [], + }; +} diff --git a/packages/app-shell/src/views/RecordFormPage.tsx b/packages/app-shell/src/views/RecordFormPage.tsx index 144412ad87..4a474d372b 100644 --- a/packages/app-shell/src/views/RecordFormPage.tsx +++ b/packages/app-shell/src/views/RecordFormPage.tsx @@ -42,6 +42,7 @@ import { createExpressionEvaluator, evaluateVisibility, } from '../providers/ExpressionProvider.js'; +import { buildExpressionUser } from '../providers/expressionUser.js'; import { SkeletonDetail } from '../skeletons/index.js'; import { ManagedByBadge } from '../components/ManagedByBadge.js'; import { useAuth } from '@object-ui/auth'; @@ -166,20 +167,24 @@ export function RecordFormPage({ mode }: RecordFormPageProps) { // expression consumers). Memoised on the underlying user identity so a // re-render that doesn't change the user does not invalidate downstream // memoisations. - const expressionUser = useMemo( - () => - user - ? { - name: user.name, - email: user.email, - role: user.role ?? 'user', - // Server-parity actor shape for per-option `visibleWhen` gating - // (ADR-0058): the rule-validator binds `current_user.positions`. - positions: (user as any).positions ?? [], - } - : { name: 'Anonymous', email: '', role: 'guest', positions: [] }, - [user], - ); + // + // objectui#6515 — THE normaliser, not a second derivation of it. This was + // hand-rolled as `{ name, email, role, positions }`, which dropped `id` and + // `isPlatformAdmin` from every predicate evaluated on this page. Those two + // are named by real gates (`ctx.user.isPlatformAdmin == true` on + // `sys_environment`'s "Change Plan (admin)"; `record.id == ctx.user.id` + // throughout `sys_user`), and an ABSENT key is not `false` — it makes the + // predicate FAULT, and `evaluateVisibility` fails OPEN on a fault, so an + // admin-only field or action rendered for everyone with nothing on screen to + // say so. The signed-out branch diverged too: it carried no `isPlatformAdmin` + // at all, while `buildExpressionUser(null)` carries `false`. + // + // It is imported from `providers/`, NOT from `console/AppContent.js` where it + // used to live: this view is `lazy()`-loaded BY `AppContent`, so that import + // would be a static edge from the split chunk back into the module it was + // split out of — what `check-eager-closure-budget` weighs. The leaf module + // beside the provider is what makes the shared normaliser reachable here. + const expressionUser = useMemo(() => buildExpressionUser(user), [user]); // Evaluator for the field-visibility expressions below, over the SAME bag // the `ExpressionProvider` at the bottom of this file publishes to the form's