diff --git a/.changeset/12700-nav-can-permissions.md b/.changeset/12700-nav-can-permissions.md new file mode 100644 index 0000000000..28c1e1f862 --- /dev/null +++ b/.changeset/12700-nav-can-permissions.md @@ -0,0 +1,10 @@ +--- +'@object-ui/app-shell': minor +'@object-ui/react': minor +--- + +A navigation entry whose `visible` predicate cannot be evaluated is no longer drawn for everyone (objectui#12700). + +The shell's navigation surfaces (the sidebar, the app root's landing, the activity roster read from app navigation, the `nav:menu` block and the command palette's navigation list) answer a FAULTING `visible` predicate by withholding the entry, where they used to draw it. The case that made this matter is a `current_user.can("OBJECT", "read")` gate asked where no `/auth/me/permissions` payload is bound: the expression engine refuses it, and the shell used to read that refusal as "shown", so permission-gated navigation hid nothing there. Where the payload is bound, the gate answers from it as before: a member without the grant is not shown the entry and one with it is. The fault is still reported once per predicate, now with a consequence line saying the entry was withheld. A blank gate is still "no gate" and is drawn. This is UI gating, not a security boundary: the server enforces what a member may read. + +`@object-ui/react`: `PredicateGateKind` gains a `'withholding'` member, the consequence line for a visibility gate whose fault is answered by withholding the entry. `@object-ui/app-shell`'s published `evaluateVisibility` keeps its fail-open answer; the object-field gate uses it unchanged. diff --git a/packages/app-shell/src/chrome/CommandPalette.tsx b/packages/app-shell/src/chrome/CommandPalette.tsx index 0564316405..42e1f46080 100644 --- a/packages/app-shell/src/chrome/CommandPalette.tsx +++ b/packages/app-shell/src/chrome/CommandPalette.tsx @@ -43,7 +43,7 @@ import { import { useRecordSearch } from '@object-ui/react'; import { usePermissions } from '@object-ui/permissions'; import { useTheme } from './ThemeProvider.js'; -import { useExpressionContext, evaluateVisibility } from '../providers/ExpressionProvider.js'; +import { useExpressionContext, evaluateNavigationVisibility } from '../providers/ExpressionProvider.js'; import { useObjectTranslation } from '@object-ui/i18n'; import { resolveKeyedI18nLabel, getRecordDisplayName, appRouteSegment } from '../utils/index.js'; import { getIcon } from '../utils/getIcon.js'; @@ -142,7 +142,7 @@ export function CommandPalette(props: CommandPaletteProps) { // Extract navigation items from active app, filtering by visibility expressions const navItems = flattenNavigation(activeApp?.navigation || []).filter( - (item) => evaluateVisibility(item.visible ?? item.visibleOn, evaluator) + (item) => evaluateNavigationVisibility(item.visible ?? item.visibleOn, evaluator) ); // The entries the palette shows for the query: a navigation entry or app by diff --git a/packages/app-shell/src/hooks/useNavMemberGates.ts b/packages/app-shell/src/hooks/useNavMemberGates.ts index 4e6a3eebbe..4cd5c44388 100644 --- a/packages/app-shell/src/hooks/useNavMemberGates.ts +++ b/packages/app-shell/src/hooks/useNavMemberGates.ts @@ -41,6 +41,12 @@ * an unchecked member. With no permission provider mounted, `can` answers * `true`, exactly as it does for the sidebar. * + * The `visible` evaluation is the exception to that "unknown reads as yes", and + * deliberately (objectui#12700): it goes through `evaluateNavigationVisibility`, + * so a predicate the evaluator cannot answer — `current_user.can(...)` where no + * permissions payload is bound, as on a frame with no permission provider — + * withholds its entry and reports once, instead of drawing it for everyone. + * * Internal module: not re-exported by the package entry. */ @@ -49,7 +55,7 @@ import { hasVisibleNavigationItems, type NavigationVisibilityOptions } from '@ob import type { NavigationItem } from '@object-ui/types'; import { usePermissions } from '@object-ui/permissions'; import { useMetadata } from '../providers/MetadataProvider.js'; -import { useExpressionContext, evaluateVisibility } from '../providers/ExpressionProvider.js'; +import { useExpressionContext, evaluateNavigationVisibility } from '../providers/ExpressionProvider.js'; import { withoutUnreadableObjectEntries, type ObjectReadCheck } from '../utils/navObjectReadGate.js'; /** The member's gates, as the sidebar applies them. */ @@ -71,7 +77,7 @@ export interface NavMemberGates { export function useNavMemberGates(): NavMemberGates { const { evaluator } = useExpressionContext(); const evalVis = useCallback( - (expr: string | boolean | undefined) => evaluateVisibility(expr, evaluator), + (expr: string | boolean | undefined) => evaluateNavigationVisibility(expr, evaluator), [evaluator], ); diff --git a/packages/app-shell/src/layout/__tests__/ConsoleLayout.appBrandingLogo-10827.test.tsx b/packages/app-shell/src/layout/__tests__/ConsoleLayout.appBrandingLogo-10827.test.tsx index b09882f3ae..20c1bfaf03 100644 --- a/packages/app-shell/src/layout/__tests__/ConsoleLayout.appBrandingLogo-10827.test.tsx +++ b/packages/app-shell/src/layout/__tests__/ConsoleLayout.appBrandingLogo-10827.test.tsx @@ -70,6 +70,7 @@ vi.mock('../../providers/MetadataProvider', () => ({ vi.mock('../../providers/ExpressionProvider', () => ({ useExpressionContext: () => ({ evaluator: null }), evaluateVisibility: (expr: unknown) => expr !== false && expr !== 'false', + evaluateNavigationVisibility: (expr: unknown) => expr !== false && expr !== 'false', })); // Lazy lucide DynamicIcon would suspend mid-test; a null icon is enough here. diff --git a/packages/app-shell/src/layout/__tests__/LocalizedSidebarTrigger.appBar-12429.test.tsx b/packages/app-shell/src/layout/__tests__/LocalizedSidebarTrigger.appBar-12429.test.tsx index 88ead7b909..f8fc6dcdaf 100644 --- a/packages/app-shell/src/layout/__tests__/LocalizedSidebarTrigger.appBar-12429.test.tsx +++ b/packages/app-shell/src/layout/__tests__/LocalizedSidebarTrigger.appBar-12429.test.tsx @@ -63,6 +63,7 @@ vi.mock('../../providers/MetadataProvider', () => ({ vi.mock('../../providers/ExpressionProvider', () => ({ useExpressionContext: () => ({ evaluator: null }), evaluateVisibility: (expr: unknown) => expr !== false && expr !== 'false', + evaluateNavigationVisibility: (expr: unknown) => expr !== false && expr !== 'false', })); vi.mock('../../utils', () => ({ diff --git a/packages/app-shell/src/layout/__tests__/UnifiedSidebar.derivedAreaVisibility.test.tsx b/packages/app-shell/src/layout/__tests__/UnifiedSidebar.derivedAreaVisibility.test.tsx index ba3e83ac89..36ec09d5ae 100644 --- a/packages/app-shell/src/layout/__tests__/UnifiedSidebar.derivedAreaVisibility.test.tsx +++ b/packages/app-shell/src/layout/__tests__/UnifiedSidebar.derivedAreaVisibility.test.tsx @@ -72,6 +72,7 @@ vi.mock('../../providers/ExpressionProvider', () => ({ // Mirrors the real evaluateVisibility's literal handling — enough for // fixtures gating with `visible: false`. evaluateVisibility: (expr: unknown) => expr !== false && expr !== 'false', + evaluateNavigationVisibility: (expr: unknown) => expr !== false && expr !== 'false', })); vi.mock('../../utils', () => ({ diff --git a/packages/app-shell/src/layout/__tests__/UnifiedSidebar.docAudience-10188.test.tsx b/packages/app-shell/src/layout/__tests__/UnifiedSidebar.docAudience-10188.test.tsx index 1411f55e09..fb6ac4f307 100644 --- a/packages/app-shell/src/layout/__tests__/UnifiedSidebar.docAudience-10188.test.tsx +++ b/packages/app-shell/src/layout/__tests__/UnifiedSidebar.docAudience-10188.test.tsx @@ -54,6 +54,7 @@ vi.mock('@object-ui/permissions', async (importOriginal) => ({ vi.mock('../../providers/ExpressionProvider', () => ({ useExpressionContext: () => ({ evaluator: null }), evaluateVisibility: (expr: unknown) => expr !== false && expr !== 'false', + evaluateNavigationVisibility: (expr: unknown) => expr !== false && expr !== 'false', })); vi.mock('../../utils', () => ({ diff --git a/packages/app-shell/src/layout/__tests__/UnifiedSidebar.favoritesMenu-12427.test.tsx b/packages/app-shell/src/layout/__tests__/UnifiedSidebar.favoritesMenu-12427.test.tsx index e50af47464..9fff57f629 100644 --- a/packages/app-shell/src/layout/__tests__/UnifiedSidebar.favoritesMenu-12427.test.tsx +++ b/packages/app-shell/src/layout/__tests__/UnifiedSidebar.favoritesMenu-12427.test.tsx @@ -66,6 +66,7 @@ vi.mock('../../providers/MetadataProvider', () => ({ vi.mock('../../providers/ExpressionProvider', () => ({ useExpressionContext: () => ({ evaluator: null }), evaluateVisibility: (expr: unknown) => expr !== false && expr !== 'false', + evaluateNavigationVisibility: (expr: unknown) => expr !== false && expr !== 'false', })); vi.mock('../../utils', () => ({ diff --git a/packages/app-shell/src/layout/__tests__/UnifiedSidebar.mobileAppHeader-12201.test.tsx b/packages/app-shell/src/layout/__tests__/UnifiedSidebar.mobileAppHeader-12201.test.tsx index 8b2843e00d..7a34bc3b3d 100644 --- a/packages/app-shell/src/layout/__tests__/UnifiedSidebar.mobileAppHeader-12201.test.tsx +++ b/packages/app-shell/src/layout/__tests__/UnifiedSidebar.mobileAppHeader-12201.test.tsx @@ -61,6 +61,7 @@ vi.mock('../../providers/MetadataProvider', () => ({ vi.mock('../../providers/ExpressionProvider', () => ({ useExpressionContext: () => ({ evaluator: null }), evaluateVisibility: (expr: unknown) => expr !== false && expr !== 'false', + evaluateNavigationVisibility: (expr: unknown) => expr !== false && expr !== 'false', })); vi.mock('../../utils', () => ({ diff --git a/packages/app-shell/src/layout/__tests__/UnifiedSidebar.navCanPermissions-12700.test.tsx b/packages/app-shell/src/layout/__tests__/UnifiedSidebar.navCanPermissions-12700.test.tsx new file mode 100644 index 0000000000..3b10432230 --- /dev/null +++ b/packages/app-shell/src/layout/__tests__/UnifiedSidebar.navCanPermissions-12700.test.tsx @@ -0,0 +1,217 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * UnifiedSidebar — a navigation entry gated on `current_user.can(object, verb)` + * follows the member's `/auth/me/permissions` answer, and is WITHHELD wherever + * that answer is not bound (objectui#12700). + * + * HotCRM declares `visible: "current_user.can(\"crm_opportunity\", \"read\")"` + * on a navigation entry. The fixture gates a `dashboard` entry, NOT an `object` + * entry on `crm_opportunity`: an object entry is already pruned by the read + * gate (objectui#12109), which would hide it whatever the predicate said and + * leave this file measuring the wrong gate. + * + * Everything on the path is real — `MePermissionsProvider` (fed through its + * `fetcher`, i.e. only the network is mocked), `ExpressionProvider` binding the + * payload for `can`, `MetadataProvider`, `@object-ui/layout`'s item guard — + * except the chrome hooks the sibling harness + * `UnifiedSidebar.objectReadGate-12109.test.tsx` also stubs (labels, recents, + * pins, context selectors), none of which decides visibility. + */ + +import '@testing-library/jest-dom/vitest'; +import { describe, it, expect, vi, beforeEach, afterEach, type MockInstance } from 'vitest'; +import React from 'react'; +import { render, screen, waitFor, act } from '@testing-library/react'; +import { MemoryRouter } from 'react-router-dom'; +import type { NavigationItem } from '@object-ui/types'; +import { MePermissionsProvider, type MePermissionsResponse } from '@object-ui/permissions'; +import { __resetVisibilityPredicateWarnings } from '@object-ui/react'; + +vi.mock('@object-ui/i18n', async (importOriginal) => ({ + ...(await importOriginal>()), + useObjectTranslation: () => ({ + t: (key: string, options?: Record) => String(options?.defaultValue ?? key), + language: 'en', + }), + useObjectLabel: () => ({ + objectLabel: ({ label }: { label?: string }) => label, + viewLabel: (_o: string, _v: string, fallback?: string) => fallback, + dashboardLabel: ({ label }: { label?: string }) => label, + appLabel: ({ label }: { label?: string }) => label, + }), +})); + +vi.mock('@object-ui/auth', async (importOriginal) => ({ + ...(await importOriginal>()), + useAuth: () => ({ user: null, activeOrganization: null }), + useWorkspaceAdminStatus: () => ({ isAdmin: false, isResolved: true }), +})); + +vi.mock('../../utils', () => ({ + resolveKeyedI18nLabel: (label: unknown) => (typeof label === 'string' ? label : ''), + matchAppBySegment: (apps: Array<{ name?: string }>, segment?: string) => + apps.find((a) => a?.name === segment), + appRouteSegment: (app: { name?: string }) => app?.name, +})); + +vi.mock('../../utils/getIcon', () => ({ getIcon: () => () => null })); +vi.mock('../../hooks/useRecentItems', () => ({ useRecentItems: () => ({ recentItems: [] }) })); +vi.mock('../../hooks/useFavorites', () => ({ + useFavorites: () => ({ favorites: [], removeFavorite: vi.fn() }), +})); +vi.mock('../../hooks/useNavPins', () => ({ + useNavPins: () => ({ togglePin: vi.fn(), applyPins: (items: unknown) => items }), +})); +vi.mock('../../hooks/useNavActionDispatch', () => ({ + useNavActionDispatch: () => vi.fn(), +})); +vi.mock('../../context/NavigationContext', () => ({ + useNavigationContext: () => ({ context: 'app', currentAppName: 'crm' }), +})); +vi.mock('../ContextSelectors', () => ({ + useAppContextSelectors: () => ({ contextValues: {}, element: null }), + contextSelectorQueryKey: (id: string) => (id === 'active_package' ? 'package' : id), + STUDIO_PACKAGE_SELECTOR_ID: 'active_package', +})); +vi.mock('../LocalizedSidebarTrigger', () => ({ + LocalizedSidebarTrigger: () => null, +})); + +import { SidebarProvider } from '@object-ui/components'; +import { MetadataProvider } from '../../providers/MetadataProvider'; +import { ExpressionProvider } from '../../providers/ExpressionProvider'; +import { UnifiedSidebar } from '../UnifiedSidebar'; + +/** The served shape: the spec normalizes an authored `visible` string into a CEL envelope. */ +const CAN_READ_OPPORTUNITY = { dialect: 'cel', source: 'current_user.can("crm_opportunity", "read")' }; + +const NAVIGATION: NavigationItem[] = [ + { id: 'nav_accounts', type: 'object', objectName: 'crm_account', label: 'Accounts' }, + { id: 'nav_pipeline', type: 'dashboard', dashboardName: 'pipeline', label: 'Pipeline', visible: CAN_READ_OPPORTUNITY as never }, + { id: 'nav_help', type: 'url', url: '/help', label: 'Help' }, +]; + +const CRM_APP = { name: 'crm', label: 'CRM', active: true, navigation: NAVIGATION }; + +function answerFor(opportunityRead: boolean): MePermissionsResponse { + return { + authenticated: true, + userId: 'u_agent', + tenantId: null, + roles: [], + permissionSets: ['service_agent'], + systemPermissions: [], + objects: { + crm_account: { allowRead: true }, + crm_opportunity: { allowRead: opportunityRead }, + }, + fields: {}, + } as MePermissionsResponse; +} + +function adapterFor(app: Record) { + return { + clearCache: vi.fn(), + getClient: () => ({ + meta: { + getItems: (type: string) => Promise.resolve({ type, items: type === 'app' ? [app] : [] }), + getItem: () => Promise.resolve({ item: null }), + }, + }), + } as unknown as Parameters[0]['adapter']; +} + +/** The console's in-app mount: the sidebar under `ExpressionProvider`. */ +function shell() { + return ( + + + + + + + + + + ); +} + +function respond(answer: MePermissionsResponse): Response { + return { ok: true, status: 200, json: async () => answer } as unknown as Response; +} + +function renderWithPermissions(fetcher: (...args: unknown[]) => Promise) { + return render( + } + > + {shell()} + , + ); +} + +const link = (name: string) => screen.queryByRole('link', { name }); +const warnings = (spy: { mock: { calls: unknown[][] } }) => + spy.mock.calls.map((c) => c.map(String).join(' ')).join('\n'); + +let warn: MockInstance; +beforeEach(() => { + localStorage.clear(); + sessionStorage.clear(); + __resetVisibilityPredicateWarnings(); + warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); +}); +afterEach(() => { + warn.mockRestore(); +}); + +describe('UnifiedSidebar — a `current_user.can()` navigation gate follows `/auth/me/permissions` (objectui#12700)', () => { + it('(a) a member without read on the object is not shown the gated entry, and nothing is reported', async () => { + renderWithPermissions(async () => respond(answerFor(false))); + await waitFor(() => expect(link('Accounts')).toBeInTheDocument()); + expect(link('Pipeline')).not.toBeInTheDocument(); + // A denial is a verdict, not a fault: the refusal line is not printed. + expect(warnings(warn)).not.toMatch(/app-shell:visible/); + }); + + it('(b) a member with read is shown it', async () => { + renderWithPermissions(async () => respond(answerFor(true))); + await waitFor(() => expect(link('Pipeline')).toHaveAttribute('href', '/apps/crm/dashboard/pipeline')); + expect(warnings(warn)).not.toMatch(/app-shell:visible/); + }); + + it('(c) while the answer is pending the entry is absent; it appears once the answer grants read', async () => { + let answer!: (r: Response) => void; + const pending = new Promise((resolve) => { answer = resolve; }); + const fetcher = vi.fn(() => pending); + renderWithPermissions(fetcher); + // The console's posture: the provider withholds the whole shell, not one entry. + expect(screen.getByTestId('permissions-loading')).toBeInTheDocument(); + expect(link('Pipeline')).not.toBeInTheDocument(); + await act(async () => { answer(respond(answerFor(true))); }); + await waitFor(() => expect(link('Pipeline')).toBeInTheDocument()); + expect(fetcher).toHaveBeenCalledTimes(1); + }); + + it('(d) CONTROL: ungated entries are drawn for the member the gate hides from', async () => { + renderWithPermissions(async () => respond(answerFor(false))); + await waitFor(() => expect(link('Accounts')).toBeInTheDocument()); + expect(link('Help')).toBeInTheDocument(); + }); + + it('with no permissions payload bound the gated entry is WITHHELD, the rest drawn, and the refusal reported once', async () => { + // A frame with no permission provider (the Home and Studio frames). + render(shell()); + await waitFor(() => expect(link('Accounts')).toBeInTheDocument()); + expect(link('Help')).toBeInTheDocument(); + expect(link('Pipeline')).not.toBeInTheDocument(); + const lines = warn.mock.calls.map((c) => String(c[0])).filter((l) => l.includes('app-shell:visible')); + expect(lines).toHaveLength(1); + expect(lines[0]).toMatch(/carries no permission data/); + expect(lines[0]).toMatch(/WITHHELD/); + expect(lines[0]).not.toMatch(/did NOT bite/); + }); +}); diff --git a/packages/app-shell/src/layout/__tests__/UnifiedSidebar.navLabelInheritsTarget-9868.test.tsx b/packages/app-shell/src/layout/__tests__/UnifiedSidebar.navLabelInheritsTarget-9868.test.tsx index bc7e87ecea..2376283280 100644 --- a/packages/app-shell/src/layout/__tests__/UnifiedSidebar.navLabelInheritsTarget-9868.test.tsx +++ b/packages/app-shell/src/layout/__tests__/UnifiedSidebar.navLabelInheritsTarget-9868.test.tsx @@ -66,6 +66,7 @@ vi.mock('../../providers/MetadataProvider', () => ({ vi.mock('../../providers/ExpressionProvider', () => ({ useExpressionContext: () => ({ evaluator: null }), evaluateVisibility: (expr: unknown) => expr !== false && expr !== 'false', + evaluateNavigationVisibility: (expr: unknown) => expr !== false && expr !== 'false', })); vi.mock('../../utils', () => ({ diff --git a/packages/app-shell/src/layout/__tests__/UnifiedSidebar.objectReadGate-12109.test.tsx b/packages/app-shell/src/layout/__tests__/UnifiedSidebar.objectReadGate-12109.test.tsx index 4de6e83de0..dc951c47be 100644 --- a/packages/app-shell/src/layout/__tests__/UnifiedSidebar.objectReadGate-12109.test.tsx +++ b/packages/app-shell/src/layout/__tests__/UnifiedSidebar.objectReadGate-12109.test.tsx @@ -50,6 +50,7 @@ vi.mock('@object-ui/auth', async (importOriginal) => ({ vi.mock('../../providers/ExpressionProvider', () => ({ useExpressionContext: () => ({ evaluator: null }), evaluateVisibility: (expr: unknown) => expr !== false && expr !== 'false', + evaluateNavigationVisibility: (expr: unknown) => expr !== false && expr !== 'false', })); vi.mock('../../utils', () => ({ diff --git a/packages/app-shell/src/layout/__tests__/UnifiedSidebar.pinnedOrder-12059.test.tsx b/packages/app-shell/src/layout/__tests__/UnifiedSidebar.pinnedOrder-12059.test.tsx index 41d00b771d..6736bc3202 100644 --- a/packages/app-shell/src/layout/__tests__/UnifiedSidebar.pinnedOrder-12059.test.tsx +++ b/packages/app-shell/src/layout/__tests__/UnifiedSidebar.pinnedOrder-12059.test.tsx @@ -70,6 +70,7 @@ vi.mock('../../providers/MetadataProvider', () => ({ vi.mock('../../providers/ExpressionProvider', () => ({ useExpressionContext: () => ({ evaluator: null }), evaluateVisibility: (expr: unknown) => expr !== false && expr !== 'false', + evaluateNavigationVisibility: (expr: unknown) => expr !== false && expr !== 'false', })); vi.mock('../../utils', () => ({ diff --git a/packages/app-shell/src/layout/__tests__/UnifiedSidebar.recentPackage-11863.test.tsx b/packages/app-shell/src/layout/__tests__/UnifiedSidebar.recentPackage-11863.test.tsx index ea65b25fa0..062535a515 100644 --- a/packages/app-shell/src/layout/__tests__/UnifiedSidebar.recentPackage-11863.test.tsx +++ b/packages/app-shell/src/layout/__tests__/UnifiedSidebar.recentPackage-11863.test.tsx @@ -61,6 +61,7 @@ vi.mock('../../providers/MetadataProvider', () => ({ vi.mock('../../providers/ExpressionProvider', () => ({ useExpressionContext: () => ({ evaluator: null }), evaluateVisibility: (expr: unknown) => expr !== false && expr !== 'false', + evaluateNavigationVisibility: (expr: unknown) => expr !== false && expr !== 'false', })); vi.mock('../../utils', () => ({ diff --git a/packages/app-shell/src/layout/__tests__/UnifiedSidebar.recentRecordsAndViews-12604.test.tsx b/packages/app-shell/src/layout/__tests__/UnifiedSidebar.recentRecordsAndViews-12604.test.tsx index 6026dc1afa..48320380a6 100644 --- a/packages/app-shell/src/layout/__tests__/UnifiedSidebar.recentRecordsAndViews-12604.test.tsx +++ b/packages/app-shell/src/layout/__tests__/UnifiedSidebar.recentRecordsAndViews-12604.test.tsx @@ -58,6 +58,7 @@ vi.mock('../../providers/MetadataProvider', () => ({ vi.mock('../../providers/ExpressionProvider', () => ({ useExpressionContext: () => ({ evaluator: null }), evaluateVisibility: (expr: unknown) => expr !== false && expr !== 'false', + evaluateNavigationVisibility: (expr: unknown) => expr !== false && expr !== 'false', })); vi.mock('../../utils', () => ({ diff --git a/packages/app-shell/src/layout/__tests__/UnifiedSidebar.studioNoHome-12113.test.tsx b/packages/app-shell/src/layout/__tests__/UnifiedSidebar.studioNoHome-12113.test.tsx index 07b031379c..05f717c7ab 100644 --- a/packages/app-shell/src/layout/__tests__/UnifiedSidebar.studioNoHome-12113.test.tsx +++ b/packages/app-shell/src/layout/__tests__/UnifiedSidebar.studioNoHome-12113.test.tsx @@ -88,6 +88,7 @@ vi.mock('../../providers/MetadataProvider', () => ({ vi.mock('../../providers/ExpressionProvider', () => ({ useExpressionContext: () => ({ evaluator: null }), evaluateVisibility: (expr: unknown) => expr !== false && expr !== 'false', + evaluateNavigationVisibility: (expr: unknown) => expr !== false && expr !== 'false', })); vi.mock('../../utils', () => ({ diff --git a/packages/app-shell/src/layout/__tests__/systemNavDatasourcesHop.test.tsx b/packages/app-shell/src/layout/__tests__/systemNavDatasourcesHop.test.tsx index 802798d0f2..8e72f145d4 100644 --- a/packages/app-shell/src/layout/__tests__/systemNavDatasourcesHop.test.tsx +++ b/packages/app-shell/src/layout/__tests__/systemNavDatasourcesHop.test.tsx @@ -97,6 +97,7 @@ vi.mock('../../providers/MetadataProvider', () => ({ vi.mock('../../providers/ExpressionProvider', () => ({ useExpressionContext: () => ({ evaluator: null }), evaluateVisibility: (expr: unknown) => expr !== false && expr !== 'false', + evaluateNavigationVisibility: (expr: unknown) => expr !== false && expr !== 'false', })); vi.mock('../../utils', () => ({ diff --git a/packages/app-shell/src/layout/__tests__/systemNavObjectsHop.test.tsx b/packages/app-shell/src/layout/__tests__/systemNavObjectsHop.test.tsx index 5e0b2bf2e6..3e8e44fb41 100644 --- a/packages/app-shell/src/layout/__tests__/systemNavObjectsHop.test.tsx +++ b/packages/app-shell/src/layout/__tests__/systemNavObjectsHop.test.tsx @@ -113,6 +113,7 @@ vi.mock('../../providers/MetadataProvider', () => ({ vi.mock('../../providers/ExpressionProvider', () => ({ useExpressionContext: () => ({ evaluator: null }), evaluateVisibility: (expr: unknown) => expr !== false && expr !== 'false', + evaluateNavigationVisibility: (expr: unknown) => expr !== false && expr !== 'false', })); vi.mock('../../utils', () => ({ diff --git a/packages/app-shell/src/layout/__tests__/systemNavSettingsTarget.test.tsx b/packages/app-shell/src/layout/__tests__/systemNavSettingsTarget.test.tsx index cade7cdb4f..51f7667e08 100644 --- a/packages/app-shell/src/layout/__tests__/systemNavSettingsTarget.test.tsx +++ b/packages/app-shell/src/layout/__tests__/systemNavSettingsTarget.test.tsx @@ -132,6 +132,7 @@ vi.mock('../../providers/MetadataProvider', () => ({ vi.mock('../../providers/ExpressionProvider', () => ({ useExpressionContext: () => ({ evaluator: null }), evaluateVisibility: (expr: unknown) => expr !== false && expr !== 'false', + evaluateNavigationVisibility: (expr: unknown) => expr !== false && expr !== 'false', })); vi.mock('../../utils', () => ({ diff --git a/packages/app-shell/src/providers/ExpressionProvider.tsx b/packages/app-shell/src/providers/ExpressionProvider.tsx index ff3deb40ae..0bbc473b4e 100644 --- a/packages/app-shell/src/providers/ExpressionProvider.tsx +++ b/packages/app-shell/src/providers/ExpressionProvider.tsx @@ -396,6 +396,41 @@ export function useExpressionContext(): ExpressionContextValue { */ const APP_SHELL_VISIBLE_SURFACE = 'app-shell:visible'; +/** + * Report one faulting app-shell `visible` predicate, through the shared + * reporter, under this surface's constant `type`. + */ +function reportAppShellVisibleFault( + // The key the AUTHOR wrote, which is what the printed line tells them to + // grep for. Nav and area items carry `visible`, the default; an object + // field's predicate is `visibleWhen` (objectui#6514), and naming it + // `visible` there would send an author to a key `FieldSchema` REFUSES. + authoredKey: string, + expression: unknown, + reason: unknown, + // What the surface's fault policy DID with the entry: `'visibility'` where a + // fault is answered fail-open, `'withholding'` where it is answered + // fail-closed (the navigation gate, objectui#12700). + gate: 'visibility' | 'withholding', +): void { + reportUnresolvableVisibilityPredicate( + APP_SHELL_VISIBLE_SURFACE, + undefined, + authoredKey, + expression, + reason, + // objectui#6487. Until this argument existed the line closed with the + // NODE tier's advice, telling an author whose nav predicate faulted to + // check `record` and `page.` — two roots the bag built in + // `ExpressionProvider` above does not contain at all — while the identity + // aliases and `features` that it DOES contain went unnamed. (`app` was in + // that list until objectui#8155 removed the root; the advice no longer + // names it either.) + 'app-shell', + gate, + ); +} + /** * Evaluate a visibility expression. * Supports: @@ -415,7 +450,10 @@ const APP_SHELL_VISIBLE_SURFACE = 'app-shell:visible'; * the app author had no working way to hide a menu item by role. * * Returns true if the item should be visible (fail-open on evaluation errors, - * matching the evaluator's own default). + * matching the evaluator's own default). A NAVIGATION entry is not gated here: + * it goes through {@link evaluateNavigationVisibility}, which withholds on a + * fault (objectui#12700). This fail-open answer is the object-field gate's + * (`isObjectFieldVisible`) and the published export's. */ export function evaluateVisibility( expression: string | boolean | { dialect?: string; source?: string } | undefined, @@ -436,32 +474,13 @@ export function evaluateVisibility( // reaches that swallowed fault, and it costs nothing: no `throwOnError` // second evaluation, same single engine call as before. // - // FAIL-OPEN IS UNCHANGED, deliberately. A predicate that cannot be evaluated - // still returns `true`, so the nav item, area or field still renders for - // everyone — including the role the predicate was written to exclude. That is - // the shipped permission-boundary semantics; flipping it to fail-closed is a - // behaviour change, not a diagnostic, and it is not this card's to make. This - // card makes the silence stop, nothing else. + // FAIL-OPEN here, deliberately: a predicate that cannot be evaluated still + // returns `true`. This function is the object-field gate's and the published + // export's; the NAVIGATION gate moved to `evaluateNavigationVisibility`, + // which withholds on a fault (objectui#12700, ruled). objectui#6443 made the + // fault loud and left the verdict alone. const report = (reason: unknown): void => - reportUnresolvableVisibilityPredicate( - APP_SHELL_VISIBLE_SURFACE, - undefined, - // The key the AUTHOR wrote, which is what the printed line tells them to - // grep for. Nav and area items carry `visible`, the default; an object - // field's predicate is `visibleWhen` (objectui#6514), and naming it - // `visible` there would send an author to a key `FieldSchema` REFUSES. - authoredKey, - expression, - reason, - // objectui#6487. Until this argument existed the line closed with the - // NODE tier's advice, telling an author whose nav predicate faulted to - // check `record` and `page.` — two roots the bag built in - // `ExpressionProvider` above does not contain at all — while the identity - // aliases and `features` that it DOES contain went unnamed. (`app` was in - // that list until objectui#8155 removed the root; the advice no longer - // names it either.) - 'app-shell', - ); + reportAppShellVisibleFault(authoredKey, expression, reason, 'visibility'); try { return evaluator.evaluateCondition(expression, { onFault: report }); @@ -474,6 +493,71 @@ export function evaluateVisibility( } } +/** + * Should this NAVIGATION entry be drawn? — the gate every navigation surface of + * the shell asks of an entry's (or an area's) authored `visible`: the sidebar, + * the app root's landing and the activity roster (all through + * `useNavMemberGates`), the `nav:menu` block and the command palette's + * navigation list (objectui#12700). + * + * The verdict is {@link evaluateVisibility}'s on every predicate that + * evaluates. It differs on a FAULT only: a predicate this evaluator cannot + * answer WITHHOLDS the entry instead of drawing it for everyone. The case that + * made this a ruling is `current_user.can(object, verb)` asked where no + * permissions payload is bound — outside `MePermissionsProvider`, or before it + * holds an answer: the engine refuses (`useExpressionPermissions` binds + * nothing rather than `{}`), and the fail-open default turned that refusal into + * "shown", so permission-gated navigation hid nothing. A navigation gate is + * hidden until known. The refusal stays loud: the fault is reported once per + * predicate, with the engine's reason and the `'withholding'` consequence line. + * + * Only a FAULT withholds. A blank gate is "no gate" in every mode of the + * evaluator (ADR-0137 D4) and is still drawn: when a first, fail-soft + * evaluation reports a fault, the predicate is asked once more on the + * evaluator's own fail-closed seam (`throwOnError`), and only a throw there + * withholds. A predicate that evaluates never pays that second call. + * + * ⛔ This is UI gating, not a security boundary: the server enforces what a + * member may read whatever the shell draws. + * + * Internal to the package: not re-exported by the package entry. + */ +export function evaluateNavigationVisibility( + expression: string | boolean | { dialect?: string; source?: string } | undefined, + evaluator: ExpressionEvaluator, +): boolean { + if (expression === undefined || expression === null) return true; + if (expression === true || expression === 'true') return true; + if (expression === false || expression === 'false') return false; + + let reason: unknown; + let faulted = false; + try { + const verdict = evaluator.evaluateCondition(expression, { + onFault: (r) => { + faulted = true; + reason = r; + }, + }); + if (!faulted) return verdict; + } catch (err) { + // The evaluator itself threw: no answer at all, so the entry is withheld. + reportAppShellVisibleFault('visible', expression, err, 'withholding'); + return false; + } + + try { + const verdict = evaluator.evaluateCondition(expression, { throwOnError: true }); + // Not a fault on the fail-closed seam: the blank gate, drawn as before and + // reported with the fail-open consequence it actually had. + reportAppShellVisibleFault('visible', expression, reason, 'visibility'); + return verdict; + } catch { + reportAppShellVisibleFault('visible', expression, reason, 'withholding'); + return false; + } +} + /** * The two field-visibility keys `@objectstack/spec`'s `FieldSchema` DECLARES. * diff --git a/packages/app-shell/src/providers/__tests__/navigationVisibility-12700.test.ts b/packages/app-shell/src/providers/__tests__/navigationVisibility-12700.test.ts new file mode 100644 index 0000000000..7724ce635b --- /dev/null +++ b/packages/app-shell/src/providers/__tests__/navigationVisibility-12700.test.ts @@ -0,0 +1,86 @@ +/** + * 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#12700 — the navigation gate withholds an entry whose `visible` + * predicate cannot be evaluated; the object-field gate keeps its fail-open + * answer for the very same fault. + * + * The evaluators are built through the shell's own builder, so the bag is the + * one `ExpressionProvider` publishes, with and without a permissions payload. + */ + +import { describe, it, expect, vi, beforeEach, afterEach, type MockInstance } from 'vitest'; +import { toEvalPermissions } from '@objectstack/formula'; +import { __resetVisibilityPredicateWarnings } from '@object-ui/react'; +import { + createExpressionEvaluator, + evaluateNavigationVisibility, + evaluateVisibility, + isObjectFieldVisible, +} from '../ExpressionProvider'; + +const CAN_READ = { dialect: 'cel', source: "current_user.can('crm_opportunity', 'read')" }; +const USER = { id: 'u1', name: 'Ada' }; + +const withPermissions = (allowRead: boolean) => + createExpressionEvaluator({ + user: USER, + permissions: toEvalPermissions({ crm_opportunity: { allowRead } }), + }); +const unbound = () => createExpressionEvaluator({ user: USER }); + +let warn: MockInstance; +beforeEach(() => { + __resetVisibilityPredicateWarnings(); + warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); +}); +afterEach(() => warn.mockRestore()); + +const appShellLines = () => + warn.mock.calls.map((c) => String(c[0])).filter((l) => l.includes('app-shell:visible')); + +describe('evaluateNavigationVisibility (objectui#12700)', () => { + it('answers from the bound payload: granted shown, denied hidden, silently', () => { + expect(evaluateNavigationVisibility(CAN_READ, withPermissions(true))).toBe(true); + expect(evaluateNavigationVisibility(CAN_READ, withPermissions(false))).toBe(false); + expect(appShellLines()).toHaveLength(0); + }); + + it('withholds the entry when no payload is bound, and reports once per predicate with the withholding consequence', () => { + const evaluator = unbound(); + expect(evaluateNavigationVisibility(CAN_READ, evaluator)).toBe(false); + expect(evaluateNavigationVisibility(CAN_READ, evaluator)).toBe(false); + const lines = appShellLines(); + expect(lines).toHaveLength(1); + expect(lines[0]).toMatch(/carries no permission data/); + expect(lines[0]).toMatch(/WITHHELD/); + }); + + it('withholds on a fault of any other kind too (an unknown root)', () => { + expect(evaluateNavigationVisibility({ dialect: 'cel', source: 'nosuchroot.flag == true' }, unbound())).toBe(false); + expect(appShellLines()).toHaveLength(1); + }); + + it('CONTROL: literals and an absent gate keep their answers', () => { + expect(evaluateNavigationVisibility(undefined, unbound())).toBe(true); + expect(evaluateNavigationVisibility(true, unbound())).toBe(true); + expect(evaluateNavigationVisibility(false, unbound())).toBe(false); + expect(evaluateNavigationVisibility({ dialect: 'cel', source: "current_user.id == 'u1'" }, unbound())).toBe(true); + expect(appShellLines()).toHaveLength(0); + }); + + it('CONTROL: a blank gate is "no gate" and is still drawn', () => { + expect(evaluateNavigationVisibility({ dialect: 'cel', source: ' ' }, unbound())).toBe(true); + }); + + it('CONTROL: the object-field gate and the published `evaluateVisibility` stay fail-open on the same fault', () => { + expect(evaluateVisibility(CAN_READ, unbound())).toBe(true); + expect(isObjectFieldVisible({ visibleWhen: CAN_READ }, unbound())).toBe(true); + }); +}); diff --git a/packages/app-shell/src/views/nav-menu-renderer.tsx b/packages/app-shell/src/views/nav-menu-renderer.tsx index 4e58470c5c..97670dab47 100644 --- a/packages/app-shell/src/views/nav-menu-renderer.tsx +++ b/packages/app-shell/src/views/nav-menu-renderer.tsx @@ -131,7 +131,7 @@ import { type NavTemplateContext, } from '@object-ui/layout'; import type { NavigationItem } from '@object-ui/types'; -import { useExpressionContext, evaluateVisibility } from '../providers/ExpressionProvider.js'; +import { useExpressionContext, evaluateNavigationVisibility } from '../providers/ExpressionProvider.js'; import { useNavActionDispatch } from '../hooks/useNavActionDispatch.js'; import { useNavTargetLabel } from '../hooks/useNavTargetLabel.js'; import { useNavDocTargetCheck } from '../hooks/useNavDocTargetCheck.js'; @@ -177,7 +177,7 @@ export const NavMenuRenderer: React.FC = ({ const { evaluator } = useExpressionContext(); const evalVis = useCallback( - (expr: string | boolean | undefined) => evaluateVisibility(expr, evaluator), + (expr: string | boolean | undefined) => evaluateNavigationVisibility(expr, evaluator), [evaluator], ); diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index 8d33a1b5c7..8adb0a50d5 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -67,11 +67,13 @@ export type { PredicateScopeTier } from './utils/visibilityDiagnostic.js'; // a type for the same reason as the tier above: the reporter is public, so a // caller has to be able to spell its arguments. // -// ⚠️ THREE members since objectui#6503, not two. `'concealment'` was added for +// ⚠️ More than two members: read the union for the set. `'concealment'` +// (objectui#6503) was added for // the `hidden` / `hiddenOn` legs, whose verdict `SchemaRenderer` does NOT // negate — the same fail-soft `true` that shows a node on the four negated legs // REMOVES it there, so "the gate did NOT bite" was the one sentence that could -// not be true for them. Widening a union re-exported from this entry is a +// not be true for them; `'withholding'` (objectui#12700) for the app-shell's +// fail-closed navigation gate. Widening a union re-exported from this entry is a // type-level change for any consumer that switches EXHAUSTIVELY over it or // keys a `Record` by it; no runtime signature moved, and every value that was // accepted before still is. diff --git a/packages/react/src/utils/visibilityDiagnostic.ts b/packages/react/src/utils/visibilityDiagnostic.ts index 0267e759af..e26f50e063 100644 --- a/packages/react/src/utils/visibilityDiagnostic.ts +++ b/packages/react/src/utils/visibilityDiagnostic.ts @@ -224,10 +224,19 @@ const SCOPE_TIER_ADVICE: Record = { * package entry, and the chain stops there (no sibling package re-exports it, * and no file outside `packages/react` names it). A consumer switching * EXHAUSTIVELY over the union, or holding a `Record`, - * gains a third case to answer. That is a type-level change only: no runtime + * gains a case to answer. That is a type-level change only: no runtime * signature moved, and every value that was accepted before still is. + * + * `'withholding'` - the app-shell's navigation `visible` gate (objectui#12700). + * A visibility predicate on a navigation entry, but one whose FAULT is answered + * fail-CLOSED: the shell withholds an entry whose gate it cannot evaluate (most + * often a `current_user.can(...)` asked before the permissions payload is + * there) instead of drawing it for everyone. So the default BITES, and the + * sentence `'visibility'` prints - "reads on screen exactly like one that said + * yes" - would be the opposite of what the member sees. Same opening line as + * `'visibility'`, for the same reason `'concealment'` keeps it. */ -export type PredicateGateKind = 'visibility' | 'enablement' | 'concealment'; +export type PredicateGateKind = 'visibility' | 'enablement' | 'concealment' | 'withholding'; /** * The two parts of the message that vary with the gate: the opening line (what @@ -267,6 +276,15 @@ const GATE_KIND_COPY: Record