From 5f7796c445733c2e7789739c850eef6ce4c7ac16 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 24 Aug 2026 13:54:49 +0000 Subject: [PATCH] fix(components): bind current_user on form section/field visibleWhen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Form section and field `visibleWhen` predicates now evaluate against the host shell's predicate scope, the same bag per-option `visibleWhen` and the page/app-nav node gate already receive. All five `resolveFieldRuleState` / `evalFieldPredicate` call sites for AUTHORED predicates in the form renderer passed `undefined` for `scope`, so a form-field gate naming `current_user` named an unbound root — and the visibility fallback is fail-open, so the gate showed the field to everyone instead of hiding it. `usePredicateScope()` moves above `readonlyFieldNames`: that memo factory runs synchronously during render, so the hook's historical position ~75 lines below put it in the temporal dead zone at that call site. The hook is still called unconditionally, once per render. The synthesised legacy `condition: { field, equals }` predicate keeps `undefined` — it can only ever name `record.`. Adds a parity pin running one authored predicate text through all five binding surfaces in three modes: denied (hides), allowed (still renders), faulted (still fails open). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CSoz9uGhaaSgiq3hshtN7L --- .../form-field-predicate-current-user-6010.md | 43 +++ .../predicate-scope-parity-6010.test.tsx | 293 ++++++++++++++++++ .../components/src/renderers/form/form.tsx | 53 +++- 3 files changed, 377 insertions(+), 12 deletions(-) create mode 100644 .changeset/form-field-predicate-current-user-6010.md create mode 100644 packages/components/src/renderers/form/__tests__/predicate-scope-parity-6010.test.tsx diff --git a/.changeset/form-field-predicate-current-user-6010.md b/.changeset/form-field-predicate-current-user-6010.md new file mode 100644 index 0000000000..6946763e4b --- /dev/null +++ b/.changeset/form-field-predicate-current-user-6010.md @@ -0,0 +1,43 @@ +--- +'@object-ui/components': minor +--- + +**Behaviour change.** Form section and field `visibleWhen` predicates that silently failed +OPEN now actually evaluate — and a rule that resolves FALSE now hides the field +(objectui#6010). + +`current_user` was bound on two of the three `visibleWhen` surfaces and not the third. A +page component or app/nav gate got it (`ExpressionProvider` → `SchemaRenderer`), and a +per-option gate got it (`resolveCascadingOptions(…, predicateScope)`), but every +`resolveFieldRuleState` call in the form renderer passed `undefined` for the scope +argument, so a form SECTION or FIELD predicate saw `record` and `previous` and nothing +else. `'sales_manager' in current_user.positions` therefore named an **unbound root** +there, and the visibility fallback is fail-open — so the gate did not hide the field from +the people it named, it **showed the field to everyone**, with no signal beyond one +deduped `console.warn`. + +**What changes for you, in the direction that matters:** if you authored a form-field or +form-section `visibleWhen` naming `current_user`, saw the field render, and concluded the +rule was permissive — it was not permissive, it was broken, and it is now enforced. That +same field will now **hide** for every user the predicate resolves FALSE for. Audit any +`visibleWhen` on a form field or section that references `current_user` / `user` / +`ctx.user` / `os.user` before upgrading; a predicate that was quietly inert becomes live. + +Two things deliberately do **not** change: + +- **A genuinely unbound root still fails open.** A predicate the engine cannot evaluate at + all still logs one warning and leaves the element visible. Only *evaluated-and-false* + hides. `visibleWhen` remains presentation, not access control — use field-level security + or RLS to stop someone reading something. +- **The deprecated `visibleOn` alias** on a form field now binds the same scope, because + ADR-0089 D2 folds it into `visibleWhen` at parse; binding one scope for the canonical + spelling and another for its alias would have reproduced the same defect one spelling + over. The synthesised legacy `condition: { field, equals }` predicate is unaffected — it + is generated from a structured object and can only ever name `record.`. + +This restores the contract both ADRs already declared: ADR-0068 D1 — *"a predicate authored +against any one form evaluates identically"* — and ADR-0089 D1 — *"runtime record surfaces +bind `record` + `current_user`"*. All five surfaces are now pinned against one authored +predicate text in +`packages/components/src/renderers/form/__tests__/predicate-scope-parity-6010.test.tsx`, +so the next divergence is loud instead of silent. diff --git a/packages/components/src/renderers/form/__tests__/predicate-scope-parity-6010.test.tsx b/packages/components/src/renderers/form/__tests__/predicate-scope-parity-6010.test.tsx new file mode 100644 index 0000000000..4358e72cb6 --- /dev/null +++ b/packages/components/src/renderers/form/__tests__/predicate-scope-parity-6010.test.tsx @@ -0,0 +1,293 @@ +/** + * 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#6010 — ONE authored `visibleWhen` text, every binding surface, one + * verdict. + * + * The three surfaces ADR-0068 D1 names are page/app-nav, per-option, and form + * section/field; the table below runs five rows, because "form section/field" + * is two declarations sharing one call site and because the deprecated + * `visibleOn` alias is the same key by ADR-0089 D2 and must not become a sixth + * spelling with a scope of its own. + * + * ADR-0068 D1 states the contract this file pins: *"a predicate authored + * against any one form evaluates identically"* — `current_user` (plus the + * `user` / `ctx.user` / `os.user` aliases) is the canonical identity root on + * every runtime record surface, and ADR-0089 D1 repeats it from the naming + * side (*"runtime record surfaces bind `record` + `current_user`"*). + * + * It was true on two surfaces and false on the third. `ExpressionProvider` + * builds the scope; `SchemaRenderer` consumed it for a page/app-nav node, and + * `form.tsx` forwarded it to `resolveCascadingOptions` for per-option + * `visibleWhen` — but every `resolveFieldRuleState` call passed `undefined`, + * so a form SECTION or FIELD predicate saw `record` and `previous` and nothing + * else. `'sales_manager' in current_user.positions` therefore named an UNBOUND + * root there, and `resolveFieldRuleState` passes `true` as the visibility + * fallback — so the gate did not hide the field from the people it named, it + * showed the field to EVERYONE. Fail-open in the dangerous direction, silent + * except for one deduped console line. + * + * ## Why a parity file and not three per-surface assertions + * + * The defect exists precisely because two of three surfaces got the scope and + * one did not. No pin that looks at a single surface can express "they agree"; + * only a table that runs the SAME predicate text through all of them can. That + * is the deliverable here — the argument change in `form.tsx` is not. + * + * ## The three cases, and why the second one is the important one + * + * Each surface is asked the same three questions: + * + * 1. **DENIED** — the predicate evaluates and resolves FALSE ⇒ hidden. + * 2. **ALLOWED** — the same predicate text, a user it admits ⇒ still SHOWN. + * This is the counter-probe, and it is the half that matters: "the + * predicate is now evaluated" is otherwise satisfiable by hiding + * everything, which is a worse bug than fail-open and is completely + * invisible to case 1 on its own. + * 3. **FAULTED** — a predicate naming a genuinely unbound root ⇒ still + * SHOWN. Fail-open is EXISTING behaviour that #6010 does not change, and + * pinning it is what keeps case 1 meaningful: without it, "hidden" and + * "the root is not bound" are the same observation. With it, the file + * distinguishes *evaluated-and-false* from *faulted*. + * + * ## Reverse verification (direction predicted BEFORE running) + * + * Revert the `scope` argument on ONE of the three `resolveFieldRuleState` call + * sites and the DENIED row for that surface — and only that surface — goes + * red, in the SHOWN direction (the element starts rendering again), because + * the fallback is fail-open. The ALLOWED and FAULTED rows stay green + * everywhere, on every surface, which is exactly why case 1 alone would not + * have caught the regression's shape. + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, screen, cleanup, fireEvent, waitFor } from '@testing-library/react'; +import { ComponentRegistry } from '@object-ui/core'; +import { SchemaRenderer, PredicateScopeProvider } from '@object-ui/react'; +// Module-scope import (not `beforeAll`) — objectui#3010/#3021. +import '../../../renderers'; + +/** + * The canonical wire shape. `@objectstack/spec` normalizes an authored + * `visibleWhen` into a `{ dialect: 'cel' }` envelope at parse (ADR-0089 D2), so + * this is what every surface actually receives in production. + * + * ⚠️ It is also the only shape in which this comparison is about the SCOPE. + * Measured while writing this file: a BARE STRING carrying the CEL membership + * operator is not portable across the surfaces, and for a reason that has + * nothing to do with `current_user`. The form renderer treats a bare string as + * CEL (`fieldRules.ts`'s `toExpression` defaults `dialect` to `'cel'`), while + * `SchemaRenderer` routes it to the LEGACY expression engine, which has no `in` + * operator and rejects it — *"Unexpected token \"i\" at position 16"* — then + * fails open. Pinning the bare string here would therefore have compared two + * dialects and read the difference as a scope difference. Dual-dialect routing + * is existing, documented behaviour on the node gate; it is not #6010 and is + * deliberately not touched. + */ +const cel = (source: string) => ({ dialect: 'cel', source }); + +/** THE authored predicate. One text, asked of every surface below. */ +const GATE = cel("'sales_manager' in current_user.positions"); + +/** + * A predicate whose root NOTHING binds — not the form, not the host scope. + * Deliberately not a typo of `current_user`: the point is a root that is + * genuinely absent on every surface, so "faulted" is unambiguous. + */ +const UNBOUND_ROOT = cel("'sales_manager' in no_such_root.positions"); + +/** + * The scope `packages/app-shell/src/providers/ExpressionProvider.tsx:59,70` + * really mounts, transcribed rather than imported: `@object-ui/app-shell` + * depends on `@object-ui/components`, so importing it from here would invert + * the dependency. The aliases are not decoration — `SchemaRenderer` re-derives + * `current_user` from `scope.user`, so a scope carrying only `current_user` + * would gate correctly on the two form surfaces and NOT on the page one, which + * is the very asymmetry this file exists to refuse. + */ +function hostScope(positions: string[]) { + const user = { id: 'u1', name: 'Kim', positions }; + return { current_user: user, user, ctx: { user }, os: { user }, app: {}, data: {}, features: {} }; +} + +const DENIED = hostScope(['sales']); +const ALLOWED = hostScope(['sales_manager']); + +// --- surface 1: page component / app-nav node ------------------------------ + +const PROBE_NAME = 'probe-6010'; +const PROBE_TYPE = 'element:probe-6010'; +ComponentRegistry.register( + PROBE_NAME, + (() =>
) as never, + { namespace: 'element', skipFallback: true } as never, +); + +function pageSurface(predicate: unknown, scope: Record): boolean { + render( + + + , + ); + return screen.queryByTestId('probe-6010') !== null; +} + +// --- surfaces 2-4: the form renderer --------------------------------------- + +function renderForm(schema: Record, scope: Record) { + const Form = ComponentRegistry.get('form')!; + return render( + +
+ , + ); +} + +/** + * Per-option `visibleWhen`, read through the form's OWN cascade clear (#2284 / + * #4247): an option the resolved set no longer offers has its stored value + * dropped before submit. Asserting the payload rather than opening the picker + * keeps this observable without driving a Radix listbox through synthetic DOM + * events, and it reads the same `resolveCascadingOptions(…, predicateScope)` + * call the render path uses. + */ +async function optionSurface(predicate: unknown, scope: Record): Promise { + const onSubmit = vi.fn(); + renderForm( + { + showSubmit: true, + submitLabel: 'Save', + fields: [ + { + name: 'tier', + label: 'Tier', + type: 'select', + options: [ + { label: 'Standard', value: 'std' }, + { label: 'Manager only', value: 'mgr', visibleWhen: predicate }, + ], + }, + ], + defaultValues: { tier: 'mgr' }, + onSubmit, + }, + scope, + ); + fireEvent.click(screen.getByRole('button', { name: /save/i })); + await waitFor(() => expect(onSubmit).toHaveBeenCalledTimes(1)); + return (onSubmit.mock.calls[0][0] as Record).tier === 'mgr'; +} + +/** Form FIELD `visibleWhen` — the `renderFormField` call site. */ +function fieldSurface(predicate: unknown, scope: Record): boolean { + renderForm( + { + fields: [ + { name: 'title', label: 'Title', type: 'input' }, + { name: 'salary', label: 'Salary', type: 'input', visibleWhen: predicate }, + ], + }, + scope, + ); + // Paired control: the un-gated sibling proves the form rendered at all, so a + // missing `salary` is a verdict and not an inability. + expect(screen.getByLabelText(/title/i)).toBeInTheDocument(); + return screen.queryByLabelText(/salary/i) !== null; +} + +/** Form SECTION `visibleWhen` — the same call site, `type: 'section-divider'`. */ +function sectionSurface(predicate: unknown, scope: Record): boolean { + renderForm( + { + fields: [ + { name: 'title', label: 'Title', type: 'input' }, + { name: 'pay', label: 'Compensation', type: 'section-divider', visibleWhen: predicate }, + ], + }, + scope, + ); + expect(screen.getByLabelText(/title/i)).toBeInTheDocument(); + return screen.queryByText('Compensation') !== null; +} + +/** + * The deprecated `visibleOn` spelling of the very same key. ADR-0089 D2 folds + * it into `visibleWhen` at parse, so a renderer that binds one scope for the + * canonical spelling and another for the alias reaches two verdicts for one + * authored rule — the #6010 defect at a smaller scale. + */ +function fieldVisibleOnSurface(predicate: unknown, scope: Record): boolean { + renderForm( + { + fields: [ + { name: 'title', label: 'Title', type: 'input' }, + { name: 'salary', label: 'Salary', type: 'input', visibleOn: predicate }, + ], + }, + scope, + ); + expect(screen.getByLabelText(/title/i)).toBeInTheDocument(); + return screen.queryByLabelText(/salary/i) !== null; +} + +type Surface = { + label: string; + show: (predicate: unknown, scope: Record) => boolean | Promise; +}; + +const SURFACES: Surface[] = [ + { label: 'page component / app-nav node `visibleWhen`', show: pageSurface }, + { label: 'per-option `visibleWhen` (select options)', show: optionSurface }, + { label: 'form SECTION `visibleWhen`', show: sectionSurface }, + { label: 'form FIELD `visibleWhen`', show: fieldSurface }, + { label: 'form FIELD `visibleOn` (ADR-0089 D2 alias)', show: fieldVisibleOnSurface }, +]; + +afterEach(() => { + cleanup(); + vi.restoreAllMocks(); +}); + +describe('#6010 — `current_user` binds identically on every `visibleWhen` surface', () => { + describe('DENIED — the predicate evaluates and resolves FALSE, so the element hides', () => { + for (const surface of SURFACES) { + it(surface.label, async () => { + expect(await surface.show(GATE, DENIED)).toBe(false); + }); + } + }); + + describe('ALLOWED — the SAME predicate text, a user it admits, still renders', () => { + // The counter-probe. Without it, "hides for a denied user" is satisfied by + // a renderer that hides unconditionally. + for (const surface of SURFACES) { + it(surface.label, async () => { + expect(await surface.show(GATE, ALLOWED)).toBe(true); + }); + } + }); + + describe('FAULTED — a genuinely unbound root still fails OPEN (unchanged by #6010)', () => { + // Existing behaviour, pinned so the DENIED rows above mean "evaluated and + // false" rather than "could not be evaluated". Asserted against the DENIED + // user on purpose: the only difference from the DENIED block is the ROOT + // the predicate names, so a fail-CLOSED regression cannot hide behind the + // membership. + for (const surface of SURFACES) { + it(surface.label, async () => { + // Fail-open is loud (#5149 / #5454 leg 3) — the warning is another + // surface's contract, and silencing it here keeps this file about the + // verdict. + vi.spyOn(console, 'warn').mockImplementation(() => {}); + vi.spyOn(console, 'error').mockImplementation(() => {}); + expect(await surface.show(UNBOUND_ROOT, DENIED)).toBe(true); + }); + } + }); +}); diff --git a/packages/components/src/renderers/form/form.tsx b/packages/components/src/renderers/form/form.tsx index 4af8a56137..244dccfeca 100644 --- a/packages/components/src/renderers/form/form.tsx +++ b/packages/components/src/renderers/form/form.tsx @@ -1153,6 +1153,21 @@ ComponentRegistry.register('form', // cannot each answer the mode differently. const isCreateForm = previousRecord === undefined; + // Global predicate scope (from the host shell's ExpressionProvider) — carries + // `current_user` (plus the ADR-0068 D1 `user` / `ctx.user` / `os.user` aliases, + // `app`, `data`, `features`) so a `visibleWhen` can gate on role/context in + // addition to sibling field values. Empty object when no provider is mounted. + // + // ⛔ Declared HERE, above `readonlyFieldNames`, and not at its historical spot + // ~75 lines down (#6010). It used to sit below, which is exactly why the + // field-rule call sites could not have it: `readonlyFieldNames`' `useMemo` + // factory runs SYNCHRONOUSLY during this render, so a `predicateScope` + // declared after it is in the temporal dead zone at that point and reading it + // there is a `ReferenceError` on the first render, not a missing binding. The + // hook is still called unconditionally and exactly once per render — only its + // position among this component's hooks moved, which React does not constrain. + const predicateScope = usePredicateScope(); + const ruleRecord = React.useMemo(() => { // Seed every declared field to `null` so a predicate referencing a field // that's absent / not-yet-registered evaluates against a present-null @@ -1211,7 +1226,7 @@ ComponentRegistry.register('form', serverOwnedValue: isServerOwnedValue(f, isCreateForm), }, previousRecord, - undefined, + predicateScope, // Same locator as the render path (#5149). A failure here reports the // field, not a bare rule kind; `warnPredicateFailure` dedupes by // predicate source, so re-evaluating a rule the renderer already @@ -1221,7 +1236,7 @@ ComponentRegistry.register('form', if (st.readonly) locked.add(name); } return locked; - }, [fields, ruleRecord, previousRecord, isCreateForm]); + }, [fields, ruleRecord, previousRecord, isCreateForm, predicateScope]); // When a field's CEL rule relaxes — it becomes hidden (visibleWhen FALSE) or // no longer required (requiredWhen FALSE) — clear any stale validation error @@ -1247,14 +1262,14 @@ ComponentRegistry.register('form', serverOwnedValue: isServerOwnedValue(f, isCreateForm), }, previousRecord, - undefined, + predicateScope, `field '${name}'`, ); // View-level FormField.visibleOn hides the field the same way a // field-level visibleWhen does (#2212) — fold it into the verdict. const viewVisible = (f as any).visibleOn == null || - evalFieldPredicate((f as any).visibleOn, ruleRecord, true, previousRecord, undefined, { + evalFieldPredicate((f as any).visibleOn, ruleRecord, true, previousRecord, predicateScope, { context: `visibleOn of field '${name}'`, }); // A hidden field shows no errors at all; an un-required field clears @@ -1262,19 +1277,19 @@ ComponentRegistry.register('form', const errType = (errs[name] as { type?: string } | undefined)?.type; if (!st.visible || !viewVisible || (!st.required && errType === 'required')) form.clearErrors(name); } + // `predicateScope` joins `ruleRecord` here for the same reason it is passed + // above (#6010): a scope change — the host swapping organisations, so + // `current_user.positions` changes — can flip a `visibleWhen` to FALSE just + // as a keystroke can, and a stale required-error on the field it just hid + // must clear on that transition too. // eslint-disable-next-line react-hooks/exhaustive-deps - }, [ruleRecord]); + }, [ruleRecord, predicateScope]); // Read DataSource from SchemaRendererContext and propagate it to field // widgets as a prop so they can dynamically load related records. const schemaCtx = React.useContext(SchemaRendererContext); const contextDataSource = schemaCtx?.dataSource ?? null; - // Global predicate scope (from the host shell's ExpressionProvider) — carries - // `current_user` etc. so per-option `visibleWhen` can gate on role/context in - // addition to sibling field values. Empty object when no provider is mounted. - const predicateScope = usePredicateScope(); - // Field name → label, for the "select the parent first" gate hint (#2284). const fieldLabelByName = React.useMemo(() => { const m: Record = {}; @@ -1927,6 +1942,13 @@ ComponentRegistry.register('form', // live record (issue #1584), so it agrees with `visibleWhen` and // the server. Fail-open (a broken predicate shows the field), // matching the CEL rules below. + // + // Deliberately still `undefined` for `scope` while the two CEL faces below + // now receive `predicateScope` (#6010): this predicate is not authored, it + // is SYNTHESISED here from a structured `{ field, equals/notEquals/in }` + // object, so its text can only ever name `record.`. There is no + // authoring path by which it could reference `current_user`, so binding a + // scope it cannot read would buy nothing and widen the surface. const legacyConditionCel = legacyConditionToCel(condition); if ( legacyConditionCel && @@ -1958,7 +1980,14 @@ ComponentRegistry.register('form', serverOwnedValue: isServerOwnedValue(field, isCreateForm), }, previousRecord, - undefined, + // The host shell's predicate scope — `current_user` and friends (#6010). + // The SAME bag `resolveCascadingOptions` below already receives, so one + // authored predicate text means one thing on every `visibleWhen` surface + // (ADR-0068 D1 / ADR-0089 D1: runtime record surfaces bind `record` + + // `current_user`). Before this it was `undefined`, and a form-field gate + // naming `current_user` named an UNBOUND root — which fails OPEN below, + // i.e. showed the field to everyone. + predicateScope, `field '${name}'`, ); if (!ruleState.visible) return null; @@ -1971,7 +2000,7 @@ ComponentRegistry.register('form', // #5149 (#2212). if ( visibleOn != null && - !evalFieldPredicate(visibleOn, ruleRecord, true, previousRecord, undefined, { + !evalFieldPredicate(visibleOn, ruleRecord, true, previousRecord, predicateScope, { context: `visibleOn of field '${name}'`, }) ) {