diff --git a/.changeset/11212-permission-gates-fail-closed.md b/.changeset/11212-permission-gates-fail-closed.md new file mode 100644 index 0000000000..663aac61a9 --- /dev/null +++ b/.changeset/11212-permission-gates-fail-closed.md @@ -0,0 +1,32 @@ +--- +'@object-ui/components': minor +'@object-ui/plugin-detail': minor +'@object-ui/react': minor +--- + +fix: an action gated on `current_user.can(object, verb)` stays hidden until the permissions payload has loaded on every action `visible` surface, a `page:header` action gated through `disabled` stays disabled, and `record:quick_actions` can answer `current_user` at all + +An action whose `visible` asks `current_user.can(...)` has no answer until the signed-in +user's permissions have loaded. On `action:group` (both display modes, and the group's own +`visible`), `action:icon` and a related list's toolbar, that unanswerable gate used to +SHOW the action, so a button meant for permission holders flashed up for everyone while +the page loaded, and stayed up where the permissions never load (the `/forms/:name` route, +a standalone embed). These renderers now treat a `visible` predicate that cannot be +evaluated the way `action:button` and `action:menu` already did: the action is hidden and +the console names it once. This is the rule for the key, not a special case for `can`: a +`visible` that faults for any other reason (a misspelled root such as `nope.x == 1`) is +hidden too, where it used to be shown. + +On `page:header`, a `disabled` predicate that cannot be evaluated now renders the button +DISABLED instead of enabled, so `disabled: "!current_user.can('account', 'delete')"` +shows a greyed-out button until the answer arrives. This also applies to a `disabled` +predicate that faults for another reason. A header action's `visible` and `hidden` +predicates keep their fail directions. + +`record:quick_actions` filters its actions against the action runner's context, which +held the host's `user` but never `current_user`, so `current_user.can(...)` and every +other `current_user.*` gate there hid the action for everyone, the users who hold the +permission included. `useActionEngine` (`@object-ui/react`) now binds the signed-in user +from the surrounding `ExpressionProvider` as `current_user`, so a quick action is shown +to users who hold the permission and hidden from those who don't, and while the +permissions are loading. diff --git a/content/docs/layout/page-header.mdx b/content/docs/layout/page-header.mdx index 6fd2d3f45a..aa67d7763e 100644 --- a/content/docs/layout/page-header.mdx +++ b/content/docs/layout/page-header.mdx @@ -157,9 +157,13 @@ declaration gates both places. not a `false`. - Call it on the signed-in user. `user.can(…)`, `ctx.user.can(…)` and `os.user.can(…)` are the same call; a bare `can(…)`, or `record.can(…)`, is a fault. -- Until the permissions payload has loaded there is no answer: the call faults, and a - header action whose `visible` faults is not rendered (see above). Gate on `visible`, - not `disabled` — a `disabled` predicate that faults leaves the action enabled. +- Until the permissions payload has loaded there is no answer: the call faults, and the + gate stays closed. A header action whose `visible` faults is not rendered (see above), + and one whose `disabled` faults is rendered disabled, so + `disabled: "!current_user.can('account', 'delete')"` shows a greyed-out button until the + answer arrives. The action renderers that draw actions elsewhere (`action:button`, + `action:group`, `action:icon`, a related list's toolbar, `record:quick_actions`) hide a + faulting `visible` the same way. - `requiredPermissions` is a different gate: it names system capabilities, which the platform action route enforces as well as the UI. `current_user.can` asks about object permissions and does nothing on the server. diff --git a/packages/app-shell/src/providers/__tests__/currentUserCan-failClosed-11212.render.test.tsx b/packages/app-shell/src/providers/__tests__/currentUserCan-failClosed-11212.render.test.tsx new file mode 100644 index 0000000000..ebcbf17ef2 --- /dev/null +++ b/packages/app-shell/src/providers/__tests__/currentUserCan-failClosed-11212.render.test.tsx @@ -0,0 +1,364 @@ +/** + * 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#11212 — Rider 1 of objectui#4421 on the legs that used to fail SOFT. + * + * The ruling, verbatim: "permission-shaped bindings are **fail-closed while the + * permissions payload has not loaded** — the opposite of the predicate default + * — or the first-paint leak reappears. Pin this." + * + * `currentUserCan-4421.render.test.tsx` pins the legs that already evaluated + * `visible` fail-closed (the row menu, `page:header`'s `visible`, + * `action:button`). This file pins the rest, through the same real providers: + * `MePermissionsProvider` holding a `/auth/me/permissions` payload, + * `ExpressionProvider` publishing the predicate scope, and an `ActionProvider` + * under it — the console's shape. Each leg is driven through the three states + * on ONE verb (`delete`): + * + * 1. NOT LOADED — no permission provider answers (a mount outside + * `MePermissionsProvider`: the console's `/forms/:name` route, a + * standalone embed). The engine refuses `can()`, the leg's fault policy + * applies, and the console says so. + * 2. LOADED, GRANTED. + * 3. LOADED, DENIED — a verdict, not a fault: nothing is reported. + * + * | leg | not loaded | granted | denied | + * |:----------------------------------------------------|:-----------|:--------|:---------| + * | `action:group` inline member `visible` | hidden | shown | hidden | + * | `action:group` dropdown member `visible` | hidden | shown | hidden | + * | `action:group` host `visible` | hidden | shown | hidden | + * | `action:icon` `visible` | hidden | shown | hidden | + * | related-list toolbar (`RelatedToolbarButton`) | hidden | shown | hidden | + * | `record:quick_actions` `visible` (ActionRunner bag) | hidden | shown | hidden | + * | `page:header` `disabled: !current_user.can(…)` | DISABLED | enabled | disabled | + * + * Before objectui#11212 the first five rows answered SHOWN while not loaded + * (their `visible` leg failed soft to `true`), `record:quick_actions` answered + * hidden in EVERY state (the `ActionRunner` bag bound `user` / `ctx.user` but + * never `current_user`, so the call faulted even for a grant holder), and the + * header's `disabled` leg answered ENABLED while not loaded (a faulting + * `disabled` fell back to "not disabled"). + * + * Every arm carries an ungated companion, so "hidden" can never mean "nothing + * rendered", and one action NAME per arm: every fault report on these legs is + * warn-once per (locator, predicate) for the life of the module, and the + * locator carries the name, so a shared name would let the NOT-LOADED report + * silence the DENIED arm's "nothing reported" for the wrong reason. + * + * The last block pins that the policy is per KEY, not a `can()` special case: + * a predicate that faults for an unrelated reason (an unbound root) gets the + * same answer on the same legs. + */ + +import '@testing-library/jest-dom/vitest'; +import * as React from 'react'; +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 { ActionProvider, RecordContextProvider } from '@object-ui/react'; +import { MePermissionsProvider, type MePermissionsResponse } from '@object-ui/permissions'; +import { ExpressionProvider } from '../ExpressionProvider'; +// Module-scope side-effect imports: the registry renderers must be registered +// before the first render (AGENTS.md §测试纪律 — import phase, not a hook). +// Deep paths into `src`, the same module instances the aliases resolve — the +// convention `currentUserCan-4421.render.test.tsx` set. +import '../../../../components/src/renderers/action/action-group'; +import '../../../../components/src/renderers/action/action-icon'; +import '../../../../components/src/renderers/layout/containers'; +import { RelatedToolbarButton } from '../../../../plugin-detail/src/RelatedList'; +import { RecordQuickActionsRenderer } from '../../../../plugin-detail/src/renderers/record-quick-actions'; + +const h = React.createElement; + +type State = 'not-loaded' | 'granted' | 'denied'; + +/** The logical delete an app ships in place of the built-in Delete. */ +const CAN_DELETE = { dialect: 'cel', source: "current_user.can('account', 'delete')" }; +/** The same verdict as a `disabled` gate: greyed out unless the caller may delete. */ +const CANNOT_DELETE = { dialect: 'cel', source: "!current_user.can('account', 'delete')" }; +/** A predicate that faults for a reason that has nothing to do with permissions. */ +const UNBOUND = { dialect: 'cel', source: 'nope.deep == 1' }; + +const LABEL = 'Void account'; +const COMPANION_LABEL = 'Print'; + +/** One action name per arm — see the file header. */ +const nameOf = (leg: string, state: State | 'unbound') => `void_${leg}_${state.replace('-', '_')}`; + +const USER = { id: 'u1', name: 'Ada', email: 'ada@example.com', role: 'user', positions: ['everyone'] }; +const RECORD = { id: 'acc_1', name: 'Northwind', status: 'open' }; + +function payload(allowDelete: boolean): MePermissionsResponse { + return { + authenticated: true, + userId: 'u1', + tenantId: null, + roles: [], + permissionSets: ['account_clerk'], + // `allowEdit` is granted in BOTH loaded states, so a verb mix-up (reading + // edit for delete) would show the action in the denied state. + objects: { account: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete } }, + fields: {}, + }; +} + +/** The console's shape: permissions above the predicate scope, a runner under it. */ +function withState(state: State, surface: React.ReactNode) { + const tree = ( + + {surface} + + ); + if (state === 'not-loaded') return tree; + return {tree}; +} + +function registered(type: string): React.ComponentType { + const R = ComponentRegistry.get(type); + if (!R) throw new Error(`${type} is not registered`); + return R as React.ComponentType; +} + +/** Radix opens a menu on `pointerdown`; the portal mounts on the next tick. */ +async function openMenu(name: RegExp) { + fireEvent.pointerDown(screen.getByRole('button', { name }), { button: 0, ctrlKey: false, pointerType: 'mouse' }); + await waitFor(() => expect(screen.getByText(COMPANION_LABEL)).toBeInTheDocument()); +} + +function warnings(spy: { mock: { calls: unknown[][] } }): string { + return spy.mock.calls.map((c) => c.map(String).join(' ')).join('\n'); +} + +/** The `useCondition` `throwOnError` leg's report (it names the predicate, not the engine reason). */ +const threw = (source: string) => + new RegExp(`was hidden/disabled: its predicate threw — CEL predicate failed to evaluate: ${escape(source)}`); +/** `ActionEngine.getActionsForLocation`'s report, for the `ActionRunner` bag. */ +const engineThrew = (source: string) => + new RegExp(`hidden: its \`visible\` predicate threw — CEL predicate failed to evaluate: ${escape(source)}`); +/** `evalRowPredicate`'s labelled report, which forwards the engine's own reason. */ +const ENGINE_REASON = /carries no permission data/; +function escape(s: string) { + return s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} + +interface VisibleLeg { + /** Mount the gated action (named per arm) and its ungated companion. */ + mount: (name: string, gate: unknown) => React.ReactNode; + /** Reveal what the leg draws when it is behind a menu trigger. */ + open?: () => Promise; + present: () => boolean; + companion: () => boolean; + fault: (source: string) => RegExp; +} + +const byText = (text: string) => () => screen.queryByText(text) !== null; + +const VISIBLE_LEGS: Record = { + 'action:group inline member': { + mount: (name, gate) => + h(registered('action:group'), { + schema: { + type: 'action:group', + display: 'inline', + actions: [ + { name, label: LABEL, type: 'script', visible: gate }, + { name: 'print', label: COMPANION_LABEL, type: 'script' }, + ], + }, + data: RECORD, + }), + present: byText(LABEL), + companion: byText(COMPANION_LABEL), + fault: threw, + }, + 'action:group dropdown member': { + mount: (name, gate) => + h(registered('action:group'), { + schema: { + type: 'action:group', + display: 'dropdown', + label: 'More', + actions: [ + { name, label: LABEL, type: 'script', visible: gate }, + { name: 'print', label: COMPANION_LABEL, type: 'script' }, + ], + }, + data: RECORD, + }), + open: () => openMenu(/More/), + present: byText(LABEL), + companion: byText(COMPANION_LABEL), + fault: threw, + }, + 'action:group host': { + // The group's OWN gate hides the whole group, so the companion is a + // second, ungated group beside it. The host label is per arm for the same + // warn-once reason as the member names. + mount: (name, gate) => + h( + React.Fragment, + null, + h(registered('action:group'), { + schema: { + type: 'action:group', + display: 'inline', + label: name, + visible: gate, + actions: [{ name, label: LABEL, type: 'script' }], + }, + data: RECORD, + }), + h(registered('action:group'), { + schema: { type: 'action:group', display: 'inline', actions: [{ name: 'print', label: COMPANION_LABEL, type: 'script' }] }, + data: RECORD, + }), + ), + present: byText(LABEL), + companion: byText(COMPANION_LABEL), + fault: threw, + }, + 'action:icon': { + mount: (name, gate) => + h( + React.Fragment, + null, + h(registered('action:icon'), { schema: { type: 'action:icon', actionType: 'script', name, label: LABEL, visible: gate }, data: RECORD }), + h(registered('action:icon'), { schema: { type: 'action:icon', actionType: 'script', name: 'print', label: COMPANION_LABEL }, data: RECORD }), + ), + present: () => screen.queryByLabelText(LABEL) !== null, + companion: () => screen.queryByLabelText(COMPANION_LABEL) !== null, + fault: threw, + }, + 'related-list toolbar (RelatedToolbarButton)': { + mount: (name, gate) => + h( + React.Fragment, + null, + h(RelatedToolbarButton, { action: { name, label: LABEL, visible: gate } as never, onToolbarAction: () => {} }), + h(RelatedToolbarButton, { action: { name: 'print', label: COMPANION_LABEL } as never, onToolbarAction: () => {} }), + ), + present: byText(LABEL), + companion: byText(COMPANION_LABEL), + fault: threw, + }, + 'record:quick_actions (the ActionRunner bag, via useActionEngine)': { + mount: (name, gate) => + h( + RecordContextProvider, + { objectName: 'account', recordId: RECORD.id, data: RECORD } as never, + h(RecordQuickActionsRenderer, { + schema: { + actions: [ + { name, label: LABEL, type: 'script', locations: ['record_header'], visible: gate }, + { name: 'print', label: COMPANION_LABEL, type: 'script', locations: ['record_header'] }, + ], + } as never, + }), + ), + present: byText(LABEL), + companion: byText(COMPANION_LABEL), + fault: engineThrew, + }, +}; + +afterEach(() => { + cleanup(); + vi.restoreAllMocks(); +}); + +describe.each(Object.entries(VISIBLE_LEGS))( + 'current_user.can on the %s `visible` leg — fail-closed while not loaded (objectui#11212)', + (legName, leg) => { + const leg$ = legName.replace(/[^a-z]+/gi, '_'); + + it('NOT LOADED: hidden, and reported', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + render(withState('not-loaded', leg.mount(nameOf(leg$, 'not-loaded'), CAN_DELETE))); + if (leg.open) await leg.open(); + expect(leg.companion()).toBe(true); + expect(leg.present()).toBe(false); + expect(warnings(warn)).toMatch(leg.fault(CAN_DELETE.source)); + }); + + it('LOADED, GRANTED: shown', async () => { + render(withState('granted', leg.mount(nameOf(leg$, 'granted'), CAN_DELETE))); + if (leg.open) await leg.open(); + expect(leg.companion()).toBe(true); + expect(leg.present()).toBe(true); + }); + + it('LOADED, DENIED: hidden, with nothing reported — a verdict, not a fault', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + render(withState('denied', leg.mount(nameOf(leg$, 'denied'), CAN_DELETE))); + if (leg.open) await leg.open(); + expect(leg.companion()).toBe(true); + expect(leg.present()).toBe(false); + expect(warnings(warn)).not.toMatch(/permission data|can\(|predicate threw/); + }); + }, +); + +/** `page:header` with the gated action and an ungated companion, both inline. */ +function header(name: string, gate: unknown) { + return h( + RecordContextProvider, + { objectName: 'account', recordId: RECORD.id, data: RECORD, objectSchema: { name: 'account', fields: {} } } as never, + h(registered('page:header'), { + schema: { + type: 'page:header', + title: 'Northwind', + maxVisible: 10, + actions: [ + { name, label: LABEL, type: 'api', locations: ['record_header'], disabled: gate }, + { name: 'print', label: COMPANION_LABEL, type: 'api', locations: ['record_header'] }, + ], + }, + }), + ); +} +const headerButton = () => screen.getByText(LABEL).closest('button'); + +describe('current_user.can on the record header `disabled` leg — DISABLED while not loaded (objectui#11212)', () => { + it('NOT LOADED: rendered DISABLED, and reported', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + render(withState('not-loaded', header(nameOf('header', 'not-loaded'), CANNOT_DELETE))); + expect(screen.getByText(COMPANION_LABEL)).toBeInTheDocument(); + expect(headerButton()).toBeDisabled(); + expect(warnings(warn)).toMatch(ENGINE_REASON); + }); + + it('LOADED, GRANTED: enabled', () => { + render(withState('granted', header(nameOf('header', 'granted'), CANNOT_DELETE))); + expect(headerButton()).not.toBeDisabled(); + }); + + it('LOADED, DENIED: disabled, with nothing reported', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + render(withState('denied', header(nameOf('header', 'denied'), CANNOT_DELETE))); + expect(headerButton()).toBeDisabled(); + expect(warnings(warn)).not.toMatch(/permission data|can\(|failed to evaluate/); + }); +}); + +describe('the policy is per KEY, not a `can()` special case: an unrelated fault gets the same answer (objectui#11212)', () => { + it.each(Object.entries(VISIBLE_LEGS))('%s — a `visible` that faults on an unbound root is hidden', async (legName, leg) => { + vi.spyOn(console, 'warn').mockImplementation(() => {}); + const leg$ = legName.replace(/[^a-z]+/gi, '_'); + render(withState('granted', leg.mount(nameOf(leg$, 'unbound'), UNBOUND))); + if (leg.open) await leg.open(); + expect(leg.companion()).toBe(true); + expect(leg.present()).toBe(false); + }); + + it('record header — a `disabled` that faults on an unbound root renders DISABLED', () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}); + render(withState('granted', header(nameOf('header', 'unbound'), UNBOUND))); + expect(headerButton()).toBeDisabled(); + }); +}); diff --git a/packages/components/src/__tests__/page-header-predicate-dialect.test.tsx b/packages/components/src/__tests__/page-header-predicate-dialect.test.tsx index c739cd282a..692dec3ee3 100644 --- a/packages/components/src/__tests__/page-header-predicate-dialect.test.tsx +++ b/packages/components/src/__tests__/page-header-predicate-dialect.test.tsx @@ -317,12 +317,19 @@ describe('page:header — fail-closed stays fail-closed, and says so once (#3521 } }); - it('leaves a faulting `disabled` predicate enabled — the historical direction', () => { + it('renders a faulting `disabled` predicate DISABLED — the closed direction on this key (objectui#11212)', () => { + // It was left ENABLED until objectui#11212, sharing `visible`'s `false` + // fallback — which on this key is the OPEN answer. Rider 1 of + // objectui#4421 (a permission-shaped gate is closed while the permissions + // payload has not loaded) decided the `disabled` leg's own direction. const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); try { renderHeader({ name: 'zoo_broken_disabled_3521', disabled: 'no_such_var_disabled_3521 == 1' }); expect(button()).toBeTruthy(); - expect(button()).not.toBeDisabled(); + expect(button()).toBeDisabled(); + expect( + warn.mock.calls.filter(c => String(c[0]).includes('zoo_broken_disabled_3521') && String(c[0]).includes('disabled')), + ).toHaveLength(1); } finally { warn.mockRestore(); } diff --git a/packages/components/src/renderers/action/__tests__/action-record-predicate-root.test.tsx b/packages/components/src/renderers/action/__tests__/action-record-predicate-root.test.tsx index 9eeafee397..ba261879ff 100644 --- a/packages/components/src/renderers/action/__tests__/action-record-predicate-root.test.tsx +++ b/packages/components/src/renderers/action/__tests__/action-record-predicate-root.test.tsx @@ -35,16 +35,17 @@ * `visible`; it shows / disables / enables on a fail-SOFT leg). * • bare `status` and `data.*`, each on the HOLDING row and the FAILING row — * the same verdict on both. A retired spelling is an unknown variable, so - * it takes the site's EXISTING fault policy (#3871's table, in - * `action-template-predicate-gate.test.tsx`): hidden on the fail-closed - * `visible` legs (`action:button`, `action:menu` item, and therefore the - * `action:bar` overflow, which IS an `action:menu`); shown / greyed / - * enabled on the fail-soft legs. "The same verdict on both rows" is what - * "no longer bound" looks like from outside, and the ruling's cost - * statement is exactly that pair. - * • a genuinely faulting predicate (`nope.deep == 1`, an unbound root) keeps - * each site's EXISTING error policy — unchanged by either card, and pinned - * so neither can be read as having quietly converted a fail-soft leg. + * it takes the site's fault policy: hidden on every `visible` leg + * (`action:button`, `action:menu` item — and therefore the `action:bar` + * overflow, which IS an `action:menu` — and, since objectui#11212, + * `action:icon` and both `action:group` leaves too); greyed / enabled on + * the fail-soft `disabled` / `enabled` legs. "The same verdict on both + * rows" is what "no longer bound" looks like from outside, and the + * ruling's cost statement is exactly that pair. + * • a genuinely faulting predicate (`nope.deep == 1`, an unbound root) takes + * each site's error policy — unchanged by the binding cards (#4075 / + * #5741); objectui#11212 made every `visible` leg here fail CLOSED, which + * is why the `action:icon` / `action:group` cases below read hidden. * * Every "not rendered" assertion carries an ungated companion, so a green can * never mean "the host itself vanished". @@ -233,20 +234,19 @@ describe('action:icon — the row binds as `record.*` (objectui#4075 / #5741)', iconHidden(); }); - it.each(RETIRED)('a `visible` written as %s no longer discriminates — shown on BOTH rows (fail-soft leg)', (_root, holding, failing) => { + it.each(RETIRED)('a `visible` written as %s no longer discriminates — hidden on BOTH rows (fail-closed leg)', (_root, holding, failing) => { mountIcon({ name: 'act', label: LABEL, visible: holding }); - iconShown(); + iconHidden(); cleanup(); mountIcon({ name: 'act', label: LABEL, visible: failing }); - iconShown(); + iconHidden(); }); - it('`visible` keeps its EXISTING fail-soft policy on a faulting predicate', () => { - // Not what this PR decides: `action:icon` has never passed `throwOnError` - // on `visible` (#3871's table). Pinned so the binding fix cannot be read as - // having quietly changed the error policy too. + it('`visible` fails CLOSED on a faulting predicate (objectui#11212)', () => { + // `action:icon` passes `throwOnError` on `visible` since objectui#11212, as + // `action:button` does; it failed soft (shown) before. mountIcon({ name: 'act', label: LABEL, visible: FAULT }); - iconShown(); + iconHidden(); }); it('a holding `disabled` written as record.* greys the icon', () => { @@ -378,19 +378,19 @@ describe.each([ hidden(); }); - it.each(RETIRED)('a `visible` written as %s no longer discriminates — shown on BOTH rows (fail-soft leg)', async (_root, holding, failing) => { + it.each(RETIRED)('a `visible` written as %s no longer discriminates — hidden on BOTH rows (fail-closed leg)', async (_root, holding, failing) => { await mount({ name: 'act', label: LABEL, type: 'script', visible: holding }); - shown(); + hidden(); cleanup(); await mount({ name: 'act', label: LABEL, type: 'script', visible: failing }); - shown(); + hidden(); }); - it('`visible` keeps its EXISTING fail-soft policy on a faulting predicate', async () => { - // As with `action:icon`: `action:group`'s leaves have never passed - // `throwOnError` (#3871's table). The binding fix does not change it. + it('`visible` fails CLOSED on a faulting predicate (objectui#11212)', async () => { + // As with `action:icon`: both `action:group` leaves pass `throwOnError` on + // `visible` since objectui#11212; they failed soft (shown) before. await mount({ name: 'act', label: LABEL, type: 'script', visible: FAULT }); - shown(); + hidden(); }); }); diff --git a/packages/components/src/renderers/action/__tests__/action-template-predicate-gate.test.tsx b/packages/components/src/renderers/action/__tests__/action-template-predicate-gate.test.tsx index fd66df8276..d1f4ba85ea 100644 --- a/packages/components/src/renderers/action/__tests__/action-template-predicate-gate.test.tsx +++ b/packages/components/src/renderers/action/__tests__/action-template-predicate-gate.test.tsx @@ -151,21 +151,27 @@ const SITES: Site[] = [ }, { id: 'action:icon', - failClosed: false, + // Fail-closed since objectui#11212 (it was fail-soft when #3871 landed — + // the "before" table above). + failClosed: true, mount: (a, s) => mountLeaf('action:icon', a, s), present: iconPresent, disabled: iconDisabled, }, { id: 'action:group inline member', - failClosed: false, + // Fail-closed since objectui#11212 (it was fail-soft when #3871 landed — + // the "before" table above). + failClosed: true, mount: mountInlineGroup, present: buttonPresent, disabled: buttonDisabled, }, { id: 'action:group dropdown member', - failClosed: false, + // Fail-closed since objectui#11212 (it was fail-soft when #3871 landed — + // the "before" table above). + failClosed: true, mount: (a, s) => mountInMenu( {}} />, s), present: menuItemPresent, disabled: menuItemDisabled, @@ -232,9 +238,9 @@ describe.each(SITES)('$id — `${…}` template `disabled` / `enabled` (objectui /** * The two HOSTS whose own `visible` is a separate call site from their members' - * (`action-bar.tsx:117` fail-closed, `action-group.tsx:206` fail-soft). A host - * gated away takes its whole toolbar with it, so these are pinned on the - * container rather than on a leaf. + * (`action:bar`'s and `action:group`'s own `useCondition`, both fail-closed — + * `action:group`'s since objectui#11212). A host gated away takes its whole + * toolbar with it, so these are pinned on the container rather than on a leaf. */ describe('action:bar host `visible` — `${…}` template (objectui#3871, fail-closed)', () => { const barSchema = (visible: unknown) => ({ @@ -264,7 +270,7 @@ describe('action:bar host `visible` — `${…}` template (objectui#3871, fail-c }); }); -describe('action:group host `visible` — `${…}` template (objectui#3871, fail-soft)', () => { +describe('action:group host `visible` — `${…}` template (objectui#3871, fail-closed since objectui#11212)', () => { function renderGroup(visible: unknown, scope: Record) { const Group = getRenderer('action:group'); return render( diff --git a/packages/components/src/renderers/action/action-group.tsx b/packages/components/src/renderers/action/action-group.tsx index c1cccdd8a5..69dce023cf 100644 --- a/packages/components/src/renderers/action/action-group.tsx +++ b/packages/components/src/renderers/action/action-group.tsx @@ -63,6 +63,28 @@ export interface ActionGroupSchema { [key: string]: any; } +/** + * One member's `visible` verdict — shared by both display modes' leaves, so the + * same member cannot be hidden in one mode and shown in the other. + * + * It fails CLOSED on a predicate that FAULTS (`throwOnError`), the policy + * `action:button`, `action:menu` and `action:bar` already apply to `visible` + * (objectui#11212, Rider 1 of objectui#4421): a precondition that cannot be + * evaluated hides the action rather than showing one whose guard is broken, and + * the fault is reported once, naming the action. It is the KEY's policy, not a + * special case for any one call: `current_user.can(…)` while the permissions + * payload has not loaded and an unbound root (`nope.x == 1`) both hide. These + * leaves used to fail SOFT to `true`, SHOWING an action whose gate could not be + * answered. Pinned three-state in `app-shell`'s + * `currentUserCan-failClosed-11212.render.test.tsx`. + */ +function useMemberVisible(action: UIActionSchema, recordData: Record): boolean { + return useCondition(toPredicateInput(action.visible), recordData, { + throwOnError: true, + label: `action "${action.name ?? action.label ?? 'action:group member'}" (visible)`, + }); +} + /** * Inline action button within a group. */ @@ -86,7 +108,10 @@ const InlineActionButton: React.FC<{ // `data.status`. This leaf used to evaluate against nothing at all, so a // row-scoped predicate faulted on its root (objectui#4075). const recordData = usePredicateRecordContext(record); - const isVisible = useCondition(toPredicateInput(action.visible), recordData); + // `visible` fails CLOSED on a predicate that faults, as on `action:button` and + // `action:menu` (objectui#11212): one fault policy per key on every action + // `visible` leg. See `useMemberVisible`. + const isVisible = useMemberVisible(action, recordData); // Spec field is `disabled` (boolean | CEL — disabled when TRUE). objectstack-ai/objectstack#1885 wired // it in action-button only; this leaf kept reading the legacy non-spec // `enabled`, so a spec-authored `disabled` guard did nothing here. `disabled` @@ -175,7 +200,9 @@ export const DropdownActionItem: React.FC<{ // its predicate in one display mode and fault in the other (objectui#4075, // the binding half of the objectui#3812 / #3842 "one leaf, one answer" rule). const recordData = usePredicateRecordContext(record); - const isVisible = useCondition(toPredicateInput(action.visible), recordData); + // Same fail-closed `visible` as `InlineActionButton` — one member, one answer + // in both display modes (objectui#11212). + const isVisible = useMemberVisible(action, recordData); // Spec `disabled` primary, legacy non-spec `enabled` fallback (see // InlineActionButton above — objectstack-ai/objectstack#1885 follow-through). const isDisabledPred = useCondition(toPredicateInput((action as any).disabled), recordData); @@ -257,7 +284,13 @@ const ActionGroupRenderer = forwardRef = ({ schema, className, ...props }) => { * Evaluate one header-action predicate. `label` is the locator carried into * the fault warning, so a hidden button names itself in the console. * - * `fallback: false` is what preserves both historical fail directions: a - * faulting `visible`/`disabled` hides/enables nothing new (fail-closed), and - * a faulting `hidden` leaves the action rendered exactly as the old - * `catch → undefined` did. + * `fallback` is the verdict a predicate that FAULTS resolves to, and it is + * the key's own fail direction, so the caller names it: + * + * - `visible` → `false`: a faulting gate hides the action (fail-closed); + * - `hidden` → `false`: a faulting `hidden` leaves the action rendered, + * exactly as the old `catch → undefined` did; + * - `disabled` → `true`: a faulting gate DISABLES the action (fail-closed, + * objectui#11212 — Rider 1 of objectui#4421: a permission-shaped gate is + * closed while the permissions payload has not loaded). It used to + * share `visible`'s `false`, which on this key means ENABLED — so + * `disabled: !current_user.can(…)` left the action pressable until the + * payload arrived, the one leg of this surface that failed open. */ const evalHeaderPredicate = React.useCallback( - (pred: unknown, label: string): boolean => + (pred: unknown, label: string, fallback: boolean): boolean => evalRowPredicate(pred as never, ctx?.data ?? {}, { - fallback: false, + fallback, scope: headerPredicateScope, fields: headerPredicateFields, warnOnError: true, @@ -1754,7 +1762,7 @@ const PageHeaderRenderer: React.FC = ({ schema, className, ...props }) => { // applies by passing `def.visible` through untouched. // On a fault (`fallback: false`) the action hides rather than risk // surfacing a destructive button in the wrong state. - if (!evalHeaderPredicate(v, `page:header action "${String(a?.name)}" visible`)) { + if (!evalHeaderPredicate(v, `page:header action "${String(a?.name)}" visible`, false)) { return false; } } @@ -1777,7 +1785,7 @@ const PageHeaderRenderer: React.FC = ({ schema, className, ...props }) => { // `fallback: false` keeps `hidden`'s historical fail direction: a // predicate that cannot be evaluated does NOT hide the action (the // old `catch → undefined` read as "not hidden"). - if (evalHeaderPredicate(h, `page:header action "${String(a?.name)}" hidden`)) { + if (evalHeaderPredicate(h, `page:header action "${String(a?.name)}" hidden`, false)) { return false; } } @@ -1984,11 +1992,14 @@ const PageHeaderRenderer: React.FC = ({ schema, className, ...props }) => { // envelope). Without this a CEL `disabled` silently did nothing (only // boolean was honoured). // - // Same entry, same bindings and same fail direction as `visible` above: - // ONE evaluator for this surface, so a `disabled` predicate cannot speak a - // different dialect from the `visible` predicate sitting next to it in the - // same action (objectui#3521). A faulting predicate still leaves the button - // enabled (`fallback: false`), it just says so once now. + // Same entry and same bindings as `visible` above: ONE evaluator for this + // surface, so a `disabled` predicate cannot speak a different dialect from + // the `visible` predicate sitting next to it in the same action + // (objectui#3521). Its fail direction is its OWN (objectui#11212): a + // predicate that faults DISABLES the button (`fallback: true`) and says so + // once — the closed answer on this key, as `visible`'s `false` is on that + // one. An absent or empty gate is still "not disabled"; only a DECLARED + // predicate that cannot be evaluated takes the fallback. const resolveDisabled = (d: any, actionName: unknown): boolean => { if (d === undefined || d === null) return false; if (typeof d === 'boolean') return d; @@ -1996,7 +2007,7 @@ const PageHeaderRenderer: React.FC = ({ schema, className, ...props }) => { ? d : (d && typeof d === 'object' && typeof (d as any).source === 'string' ? (d as any).source : undefined); if (!src) return false; - return evalHeaderPredicate(d, `page:header action "${String(actionName)}" disabled`); + return evalHeaderPredicate(d, `page:header action "${String(actionName)}" disabled`, true); }; // A live inline-edit session disables actions the host flagged with // `disableDuringInlineEdit` (objectui#2572 item 4) — see `inlineEditing` diff --git a/packages/plugin-detail/src/RelatedList.tsx b/packages/plugin-detail/src/RelatedList.tsx index 5e9c46fc55..f830068f17 100644 --- a/packages/plugin-detail/src/RelatedList.tsx +++ b/packages/plugin-detail/src/RelatedList.tsx @@ -428,13 +428,25 @@ function dropRedactedColumns(cols: T[], redacted: ReadonlySet): T[] { * `invite_user` (`visible: "features.organization != false"`) hides when its * predicate is false. `features`/`user` resolve from the ambient * ExpressionProvider scope. + * + * The verdict fails CLOSED on a predicate that FAULTS (`throwOnError`), as + * every action `visible` leg does (objectui#11212, Rider 1 of objectui#4421): + * a precondition that cannot be evaluated — `current_user.can(…)` before the + * permissions payload has loaded, an unbound root — hides the button and is + * reported once, instead of showing an action whose guard is broken. This leg + * used to fail SOFT to `true`. */ export const RelatedToolbarButton: React.FC<{ action: RelatedRowActionDef; onToolbarAction: (action: RelatedRowActionDef) => void | Promise; }> = ({ action, onToolbarAction }) => { const visiblePred = action.visible; - const isVisible = useCondition(toPredicateInput(visiblePred)); + // A toolbar action is list-level: there is no row to bind, so the context is + // the ambient predicate scope alone. + const isVisible = useCondition(toPredicateInput(visiblePred), undefined, { + throwOnError: true, + label: `related-list toolbar action "${action.name}" (visible)`, + }); if (visiblePred && !isVisible) return null; const ActionIcon = action.icon ? resolveIconComponent(action.icon) : null; return ( diff --git a/packages/plugin-detail/src/__tests__/related-toolbar-visible.test.tsx b/packages/plugin-detail/src/__tests__/related-toolbar-visible.test.tsx index 5473f04c75..9bec3f5c77 100644 --- a/packages/plugin-detail/src/__tests__/related-toolbar-visible.test.tsx +++ b/packages/plugin-detail/src/__tests__/related-toolbar-visible.test.tsx @@ -15,7 +15,7 @@ * `invite_user` (`visible: "features.organization != false"`) showed even when * the org feature was disabled. */ -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, vi } from 'vitest'; import { render, screen } from '@testing-library/react'; import '@testing-library/jest-dom'; import React from 'react'; @@ -57,15 +57,18 @@ describe('RelatedList list_toolbar button — visible CEL', () => { /** * objectui#3871 — the same gate, with the predicate written in the documented - * `${…}` template spelling. This leg does NOT opt into `throwOnError`, so the - * double wrap came back from the evaluator as the unparsed string and - * `Boolean(…)` read it as a constant `true`: the toolbar action was shown - * whatever its predicate said. `invite_user` is the live example in the prose - * above — spelled as a template it would have appeared with the org feature off. + * `${…}` template spelling. When #3871 landed this leg did NOT opt into + * `throwOnError`, so the double wrap came back from the evaluator as the + * unparsed string and `Boolean(…)` read it as a constant `true`: the toolbar + * action was shown whatever its predicate said. `invite_user` is the live + * example in the prose above — spelled as a template it would have appeared + * with the org feature off. * - * Reverse verification: restore the unconditional wrap and the "false hides" - * case goes red; the "true shows" case was green already (shown either way), so - * both are here and only one is the detector. + * Reverse verification, as of #3871: restore the unconditional wrap and the + * "false hides" case went red; the "true shows" case was green already (shown + * either way). This leg fails CLOSED since objectui#11212, so today the same + * restore would turn the OTHER case red (a throw hides): both are here, so + * either policy has a detector. */ describe('RelatedList list_toolbar button — `${…}` template `visible` (objectui#3871)', () => { const TEMPLATE = '${features.organization !== false}'; @@ -86,3 +89,29 @@ describe('RelatedList list_toolbar button — `${…}` template `visible` (objec expect(screen.getByTestId('related-toolbar-action-invite_user')).toBeInTheDocument(); }); }); + +/** + * objectui#11212 — the toolbar `visible` fails CLOSED on a predicate that + * FAULTS, as every action `visible` leg does (Rider 1 of objectui#4421). It + * used to fail soft to `true`, showing an action whose gate could not be + * answered. The policy is the key's, so an unrelated fault — an unbound root + * here — gets the same answer as `current_user.can(…)` before the permissions + * payload loads (pinned three-state in `app-shell`'s + * `currentUserCan-failClosed-11212.render.test.tsx`). + */ +describe('RelatedList list_toolbar button — a faulting `visible` hides (objectui#11212)', () => { + it('hides a toolbar action whose predicate faults, and reports it once, naming the action', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + try { + renderButton({ name: 'invite_user_11212', label: 'Invite User', visible: 'nope.deep == 1' }); + renderButton({ name: 'export', label: 'Export' }); + expect(screen.getByTestId('related-toolbar-action-export')).toBeInTheDocument(); + expect(screen.queryByTestId('related-toolbar-action-invite_user_11212')).toBeNull(); + const reports = warn.mock.calls.filter((c) => String(c[0]).includes('"invite_user_11212" (visible)')); + expect(reports).toHaveLength(1); + expect(String(reports[0][0])).toContain('its predicate threw'); + } finally { + warn.mockRestore(); + } + }); +}); diff --git a/packages/react/src/hooks/useActionEngine.ts b/packages/react/src/hooks/useActionEngine.ts index 49521f7abc..a891aef95d 100644 --- a/packages/react/src/hooks/useActionEngine.ts +++ b/packages/react/src/hooks/useActionEngine.ts @@ -26,12 +26,14 @@ import { useCallback, useContext, useMemo } from 'react'; import { ActionEngine, + subjectPermissionsOf, type ActionLocation, type ActionDef, type ActionContext, type ActionResult, } from '@object-ui/core'; import { ActionCtxReact } from '../context/ActionContext.js'; +import { usePredicateScope } from './useExpression.js'; export interface UseActionEngineOptions { /** Action definitions to register */ @@ -69,7 +71,34 @@ export function useActionEngine(options: UseActionEngineOptions = {}): UseAction const providerCtx = useContext(ActionCtxReact); const sharedRunner = providerCtx?.runner ?? null; + // The acting subject the host's predicate scope binds as `current_user` + // (objectui#11212): the ONE object `ExpressionProvider` publishes under + // `current_user` / `user` / `ctx.user` / `os.user`, carrying the caller's + // effective object permissions once they have loaded. The engine filters + // `visible` against the RUNNER's bag, which a host seeds with its own `user` + // and never with `current_user` — so `current_user.can(…)`, and any other + // `current_user.*` predicate, faulted on this path in every state and the + // action was hidden even for a grant holder, while the same predicate + // answered on every predicate-scope surface. Binding the scope's subject + // itself — the same object, not a copy, because the engine answers `can` + // only for a receiver IDENTICAL to the bound `current_user` — is what makes + // the two paths one bag (objectui#6493). With no host scope (`{}`) nothing + // is bound and the path behaves as it always did. + // + // Only `current_user` is added. The runner's `user` / `ctx.user` / `os.user` + // stay the host's own object: it carries what the runner itself reads (the + // `systemPermissions` capability set), which the scope's subject does not. + const subject = usePredicateScope().current_user as Record | undefined; + // Keyed on what the binding READS, never on the subject's identity (a + // memoised value upstream — AGENTS.md #10): its serialisable fields, and the + // permissions map it carries, which the upstream adapter caches per payload + // outside React. A discarded-and-recomputed subject with the same content + // keeps the engine; the permissions arriving (or leaving) rebuilds it. + const subjectKey = subject === undefined ? '' : JSON.stringify(subject); + const subjectPermissions = subjectPermissionsOf(subject); + const engine = useMemo(() => { + const bound: Partial = subject !== undefined ? { current_user: subject } : {}; // When standalone (no surrounding ``), normalize the // context so predicates can use both `record`/`user` and `ctx.*`. const normalizedStandalone = (context && Object.keys(context).length > 0) @@ -78,8 +107,9 @@ export function useActionEngine(options: UseActionEngineOptions = {}): UseAction ctx: ((context as any).ctx && typeof (context as any).ctx === 'object') ? { ...context, ...(context as any).ctx } : { ...context }, + ...bound, } - : context; + : { ...context, ...bound }; const e = sharedRunner ? new ActionEngine(sharedRunner) : new ActionEngine(normalizedStandalone as any); // When sharing a provider runner, MERGE per-render flat keys into the // existing `ctx` instead of overwriting it. The provider seeds @@ -97,13 +127,18 @@ export function useActionEngine(options: UseActionEngineOptions = {}): UseAction const merged = { ...context, ctx: { ...existingCtx, ...callerCtx }, + ...bound, }; runner.updateContext(merged as any); + } else if (sharedRunner && subject !== undefined) { + // A caller that passes no per-render keys: the subject is still bound, + // onto the same shared runner the location filter reads. + e.getRunner().updateContext(bound); } e.registerActions(actions); return e; // eslint-disable-next-line react-hooks/exhaustive-deps - }, [sharedRunner, JSON.stringify(actions), JSON.stringify(context)]); + }, [sharedRunner, JSON.stringify(actions), JSON.stringify(context), subjectKey, subjectPermissions]); const getActionsForLocation = useCallback( (location: ActionLocation) => engine.getActionsForLocation(location), diff --git a/packages/react/src/hooks/useExpression.ts b/packages/react/src/hooks/useExpression.ts index c93fffd8d7..a29ff79535 100644 --- a/packages/react/src/hooks/useExpression.ts +++ b/packages/react/src/hooks/useExpression.ts @@ -105,10 +105,11 @@ export { toPredicateInput } from '@object-ui/core'; * faults exactly as it always did on the server (`buildScope({ record })` * mounts exactly `['record']`: `Unknown variable: status` / `Unknown variable: * data`), and each `useCondition` leg applies its EXISTING fault policy — the - * throwing legs (`throwOnError`: `action:button` / `action:menu` `visible`, - * `DeclaredActionsBar`'s `visible`) hide and report `was hidden/disabled: its - * predicate threw`, naming the variable; the non-throwing legs fail soft to - * `true`. The same holds for a legacy `${data.x}` / `${x}` string: one bag + * throwing legs (`throwOnError`: every action `visible` — `action:button`, + * `action:menu`, `action:bar`, `DeclaredActionsBar`, and since objectui#11212 + * `action:group`, `action:icon` and the related-list toolbar) hide and report + * `was hidden/disabled: its predicate threw`, naming the variable; the + * non-throwing legs fail soft to `true`. The same holds for a legacy `${data.x}` / `${x}` string: one bag * shape, both dialects. Nothing detects a retired spelling here; it is simply * unbound. `@object-ui/core`'s `evaluator/rowPredicateCanon.ts` carries the * canon statement, the server measurement, the layer scoping (`data` stays