diff --git a/.changeset/12706-setup-route-access-verdict.md b/.changeset/12706-setup-route-access-verdict.md new file mode 100644 index 0000000000..15879ae617 --- /dev/null +++ b/.changeset/12706-setup-route-access-verdict.md @@ -0,0 +1,9 @@ +--- +'@object-ui/app-shell': patch +--- + +A caller the Setup app refuses gets the access-denied screen on Setup's item pages instead of those pages under another app's chrome (objectui#12706). + +A member without `setup.access` who opened `/apps/setup` landed on the default app's landing (the CRM dashboard, say), and `/apps/setup/metadata/...` drew the permission-set list and the permission-set and position editors under that app's chrome. The server refused every read that mattered, so nothing leaked, but the screen was wrong. Setup's root and the metadata-admin routes (with the legacy `component/metadata/...` spellings that forward onto them) now wait on the access verdict for the app the URL names, the same by-name probe an ordinary app's routes already wait on, and draw "You don't have access to this app" when the server refuses, with nothing of the page mounted. An admitted caller sees no change, and every verdict other than a refusal renders the page as before. Setup's `system/*` pages and the record pages under the Setup segment are not gated: a member reaches the approvals inbox there, and the records it links to. + +The permission-set editor's Assigned Users section no longer draws a refused holders read as "0" and "No users assigned yet": it says the refusal in the server's words, and the empty copy follows only a read that succeeded. A refused Remove is said the same way instead of being swallowed. The position editor's holders section says a refused or failed holders read the same way, with no count, instead of drawing the refusal above "Nobody holds this position yet." diff --git a/packages/app-shell/README.md b/packages/app-shell/README.md index 99073c7086..622029153b 100644 --- a/packages/app-shell/README.md +++ b/packages/app-shell/README.md @@ -574,7 +574,20 @@ family for them: it uses the same metadata-admin list and editors at read-only on both assignment objects (objectstack's own pin for that is the test `organization_admin is read-only on global RBAC tables to prevent privilege escalation`), so that administrator sees the holders with no Add and - no Remove. + no Remove. When the server refuses the holders read itself, each section + says the refusal in the server's words and draws neither its count nor its + empty copy: the empty copy follows only a read that succeeded and came back + empty (objectui#12706). +- **Who gets these pages at all.** A caller the Setup app refuses (no + `setup.access`; the server leaves Setup out of that caller's app list) gets + the console's "You don't have access to this app" screen on these pages and + on Setup's root, the screen an ordinary app's routes already draw. The + metadata-admin routes and Setup's root wait on the access verdict for the app + the URL names (the by-name probe) before anything of the page mounts; every + verdict other than a refusal renders the page as before (objectui#12706). + Setup's `system/*` pages and the record pages under the Setup segment do not + wait on it: a member reaches the approvals inbox there, from the server's + `/system/approvals` links, and the records the inbox opens. The list has an active switch and an active/inactive filter, through `catalog-activation.ts`. Both show the activation ledger's state diff --git a/packages/app-shell/src/console/AppContent.tsx b/packages/app-shell/src/console/AppContent.tsx index a280da7698..2d9eec3ad3 100644 --- a/packages/app-shell/src/console/AppContent.tsx +++ b/packages/app-shell/src/console/AppContent.tsx @@ -289,6 +289,31 @@ export function AppContent({ extraRoutes, extraRoutesNoApp }: AppContentProps = // excluded so they keep their default-app fallback. const requestedAppMissing = !!appName && !matchedApp && !isSpecialRoute; + // objectui#12706 — the pseudo-routes above skip `requestedAppMissing`, so + // until this the access verdict was never asked for them: a session the Setup + // app refuses (no `setup.access`; the server leaves Setup out of its list) + // opened `/apps/setup` and got the DEFAULT app's landing under that app's + // chrome, and `/apps/setup/metadata/...` drew Setup's item pages (the + // permission-set and position editors) the same way. + // + // Two of them now wait on the verdict for the app the URL names (the same + // probe as `requestedAppMissing` below) and draw the access-denied screen on + // `denied`: the metadata-admin routes (`metadata/...`, with the legacy + // spellings that redirect onto them) and Setup's root, the app's own landing. + // Every other verdict keeps the pseudo-route rendering above, so an app that + // does not exist, a server that does not answer and an adapter without the + // probe change nothing. + // + // ⛔ Not the rest of the Setup subtree. `/system/*` links forward onto + // `/apps/setup/system/*` (`SystemRedirect`, `ConsoleShell.tsx`), the server's + // approval links (`/system/approvals?request=`) among them, and that inbox + // opens records at `/apps/setup/OBJECT/record/ID` (`ApprovalsInboxPage`'s + // `recordHref`). A member reaches both under the Setup segment, so whether + // they wait on Setup's verdict is a product decision, left open on + // objectui#12706. + const isSetupRootRoute = location.pathname === '/apps/setup' || location.pathname === '/apps/setup/'; + const appSurfaceAwaitsVerdict = !!appName && !matchedApp && (isMetadataRoute || isSetupRootRoute); + // Post-publish readiness: when a requested app isn't in the loaded metadata, // re-check ONCE (the registry can lag a beat behind a publish) before // concluding it's absent — so a just-built app resolves on its own instead @@ -377,8 +402,13 @@ export function AppContent({ extraRoutes, extraRoutesNoApp }: AppContentProps = // costing one skipped refresh rather than a wrong screen, which is why it was // filed separately; objectui#4522 keyed it the same way.) const [accessProbe, setAccessProbe] = useState<{ app: string; verdict: AppAccessVerdict } | null>(null); + // A missing app is asked once its re-check has settled; an app surface under + // a pseudo-route (objectui#12706, above) once the list has loaded, since a + // list still loading has matched nothing yet. + const verdictWanted = + (requestedAppMissing && missingRecheck === 'done') || (appSurfaceAwaitsVerdict && !metadataLoading); useEffect(() => { - if (!requestedAppMissing || previewDrafts || missingRecheck !== 'done' || !appName) { + if (!verdictWanted || previewDrafts || !appName) { // Cleared on the way out too, so the Retry button below (which returns // `missingRecheck` to 'idle') re-asks instead of replaying an answer. setAccessProbe(p => (p === null ? p : null)); @@ -403,7 +433,7 @@ export function AppContent({ extraRoutes, extraRoutesNoApp }: AppContentProps = // (which now means nothing was asked at all) — objectui#9262. .catch(() => { if (!cancelled) setAccessProbe({ app: probed, verdict: 'unreachable' }); }); return () => { cancelled = true; }; - }, [requestedAppMissing, previewDrafts, missingRecheck, appName, accessProbe, dataSource]); + }, [verdictWanted, previewDrafts, appName, accessProbe, dataSource]); // Never the previous app's answer: while a probe for a newly-requested app is // in flight, this is null and the branch below waits rather than rendering the // one it already has. @@ -806,52 +836,64 @@ export function AppContent({ extraRoutes, extraRoutesNoApp }: AppContentProps = // reachable with no active app: a fresh env where AI just drafted everything // has ZERO published apps, and that is the surface you review & publish from.) + // objectui#4252 — the app EXISTS and this session may not open it. Say that, + // and say it as the permanent decision it is: the old copy named a transient + // deploy state, which sent a downstream acceptance round chasing a platform + // defect for two test batches over a missing permission-set binding. + // + // No Retry here — retrying a permission decision cannot change it, and a + // button that promises otherwise is the same misdirection one layer down. + // The way back is HOME instead, because this screen (like every no-app + // surface in this file) returns ABOVE the single `ConsoleLayout` mount and + // so carries no header, no navigation and no workspace switcher — the + // objectui#4473 strand, which a dead end here would recreate. Which home is + // the DECLARED one (objectui#7373): a control-plane customer bounced to the + // environment launcher lands among cards that act on an environment their + // deployment does not have. Router-relative for the same reason as the + // #4473 fix: ``/`navigate` resolve through the host's `basename`, + // and both the declared landing and the launcher fallback are part of the + // outer skeleton every host mounting this component provides (see this + // file's header). + // + // One screen for both branches that draw a `denied` verdict below. + const renderAppAccessDenied = () => ( +
+ + + {t('empty.appAccessDenied', { defaultValue: "You don't have access to this app" })} + + + {t('empty.appAccessDeniedDescription', { + defaultValue: + 'This app exists, but your account is not authorized to open it. Ask an administrator to grant you access.', + })} + +
+ +
+ +
+ ); + + // objectui#12706 — an app surface under a pseudo-route, for an app the list + // does not carry (`appSurfaceAwaitsVerdict`, computed near the top). Nothing + // of it renders before the verdict, so a refused caller never mounts the + // page; `denied` is the screen above, and every other verdict falls through + // to the pseudo-route rendering below, unchanged. + if (appSurfaceAwaitsVerdict && !previewDrafts) { + if (accessVerdict === null) return ; + if (accessVerdict === 'denied') return renderAppAccessDenied(); + } + // A normal app was requested but isn't in the loaded metadata. Re-check once // for the post-publish lag, then say it's not available — NEVER render a // different app, and don't show the misleading "no apps configured" screen // below (there ARE apps). requestedAppMissing already excludes pseudo-routes. if (requestedAppMissing) { if (missingRecheck !== 'done' || accessVerdict === null) return ; - // objectui#4252 — the app EXISTS and this session may not open it. Say that, - // and say it as the permanent decision it is: the old copy named a transient - // deploy state, which sent a downstream acceptance round chasing a platform - // defect for two test batches over a missing permission-set binding. - // - // No Retry here — retrying a permission decision cannot change it, and a - // button that promises otherwise is the same misdirection one layer down. - // The way back is HOME instead, because this screen (like every no-app - // surface in this file) returns ABOVE the single `ConsoleLayout` mount and - // so carries no header, no navigation and no workspace switcher — the - // objectui#4473 strand, which a dead end here would recreate. Which home is - // the DECLARED one (objectui#7373): a control-plane customer bounced to the - // environment launcher lands among cards that act on an environment their - // deployment does not have. Router-relative for the same reason as the - // #4473 fix: ``/`navigate` resolve through the host's `basename`, - // and both the declared landing and the launcher fallback are part of the - // outer skeleton every host mounting this component provides (see this - // file's header). - if (accessVerdict === 'denied') { - return ( -
- - - {t('empty.appAccessDenied', { defaultValue: "You don't have access to this app" })} - - - {t('empty.appAccessDeniedDescription', { - defaultValue: - 'This app exists, but your account is not authorized to open it. Ask an administrator to grant you access.', - })} - -
- -
- -
- ); - } + if (accessVerdict === 'denied') return renderAppAccessDenied(); // objectui#9262 — the remaining verdicts, each saying only what the probe // obtained. The screen this replaced asserted ONE transient deploy state // ("it may still be publishing") over every one of them; that sentence is diff --git a/packages/app-shell/src/console/__tests__/AppContent.setupItemRoutesVerdict-12706.test.tsx b/packages/app-shell/src/console/__tests__/AppContent.setupItemRoutesVerdict-12706.test.tsx new file mode 100644 index 0000000000..5c7e95efb5 --- /dev/null +++ b/packages/app-shell/src/console/__tests__/AppContent.setupItemRoutesVerdict-12706.test.tsx @@ -0,0 +1,346 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * A caller the Setup app refuses gets the app's access verdict on Setup's item + * pages, not the pages under another app's chrome (objectui#12706). + * + * ## The measured defect + * + * A plain member (no `setup.access`) opened `/apps/setup` and landed on the CRM + * app's dashboard, under the CRM app's chrome; `/apps/setup/metadata/permission` + * drew the permission-set list, and `/apps/setup/metadata/permission/NAME` and + * `/apps/setup/metadata/position/NAME` drew the editors. The server refused + * every read that matters (403), so it was a wrong screen, not an access hole. + * + * The cause: `AppContent` asked the app's access verdict only on its + * `requestedAppMissing` branch, and that branch excludes every pseudo-route + * (`isSpecialRoute`: create-app, a `system` or `metadata` segment, and the + * whole `/apps/setup` subtree). For an ordinary app every nested route already + * waits on the verdict; for Setup, no route did. A pseudo-route falls back to + * the default launcher app, so `/apps/setup` ran the CRM app's landing. + * + * ## What is pinned + * + * The metadata-admin routes and Setup's root wait on the verdict for the app + * the URL names, and draw `app-access-denied` on `denied` with nothing of the + * page mounted. Every other verdict, and an admitted caller, render as before. + * The approvals inbox and the record pages it links to stay reachable: a member + * approver reaches them under the Setup segment (the server's notification + * links are `/system/approvals?request=ID`, which forward onto + * `/apps/setup/system/approvals`), so they are outside this gate. + * + * ## Why the route is stubbed at the TRANSPORT + * + * As in `AppContent.deniedVsUnpublished.test.tsx`: a real `ObjectStackAdapter` + * over a stubbed `fetch`, so the verdict comes from the real probe reading the + * real envelope, not from a fixture of its own conclusion. + * + * NOTE ON SCOPE: this file measures WHICH SURFACE renders. `ConsoleLayout` and + * the lazy pages are stubbed; each stub counts its mounts, so "nothing of the + * page mounted" is a count, not the absence of a test id at one instant. + */ + +import '@testing-library/jest-dom/vitest'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import React from 'react'; +import { render, screen } from '@testing-library/react'; +import { MemoryRouter, Routes, Route, useLocation, useParams } from 'react-router-dom'; +import { ObjectStackAdapter } from '@object-ui/data-objectstack'; + +// --------------------------------------------------------------------------- +// Mocks — everything that takes part in the DECISION stays real. +// --------------------------------------------------------------------------- + +/** How many times each stubbed page mounted, by test id. */ +const mounts: Record = {}; +const counted = (testId: string, text?: string) => { + mounts[testId] = (mounts[testId] ?? 0) + 1; + return
{text}
; +}; + +// AGENTS.md §测试纪律 — the lazy page modules are stubbed so no unbounded +// `import()` races a bounded `findBy` window. +vi.mock('../../views/metadata-admin', () => ({ + MetadataDirectoryPage: () => counted('metadata-directory-page'), + MetadataResourceListPage: () => { + const { type } = useParams<{ type?: string }>(); + return counted('metadata-resource-list-page', type); + }, + MetadataResourceEditPage: () => { + const { type, name } = useParams<{ type?: string; name?: string }>(); + return counted('metadata-resource-edit-page', `${type}/${name}`); + }, + MetadataResourceHistoryPage: () => counted('metadata-resource-history-page'), + MetadataDiagnosticsPage: () => counted('metadata-diagnostics-page'), +})); +vi.mock('../../views/RecordDetailView', () => ({ + RecordDetailView: () => { + const { objectName, recordId } = useParams<{ objectName?: string; recordId?: string }>(); + return counted('record-detail-view', `${objectName}/${recordId}`); + }, +})); +vi.mock('../../views/ObjectView', () => ({ + ObjectView: () => { + const { objectName } = useParams<{ objectName?: string }>(); + return counted('object-view', objectName); + }, +})); +vi.mock('@object-ui/plugin-designer', async (importOriginal) => ({ + ...(await importOriginal()), + CreateAppPage: () =>
, + EditAppPage: () =>
, + DashboardDesignPage: () =>
, +})); + +vi.mock('../../layout/ConsoleLayout', () => ({ + ConsoleLayout: ({ activeAppName, children }: { activeAppName?: string; children?: React.ReactNode }) => ( +
+
chrome
+ {children} +
+ ), +})); +vi.mock('../../chrome/CommandPalette', () => ({ CommandPalette: () => null })); +vi.mock('../../chrome/KeyboardShortcutsDialog', () => ({ KeyboardShortcutsDialog: () => null })); +vi.mock('../../chrome/OnboardingWalkthrough', () => ({ OnboardingWalkthrough: () => null })); + +vi.mock('@object-ui/i18n', async (importOriginal) => ({ + ...(await importOriginal>()), + useObjectTranslation: () => ({ + t: (key: string, options?: Record) => String(options?.defaultValue ?? key), + }), + useObjectLabel: () => ({ + objectLabel: ({ label }: { label?: string }) => label, + objectPluralLabel: ({ label }: { label?: string }) => label, + appLabel: ({ name, label }: { name: string; label?: string }) => label ?? name, + }), +})); + +vi.mock('@object-ui/auth', async (importOriginal) => ({ + ...(await importOriginal>()), + useAuth: () => ({ + user: { id: 'u_member', name: 'Member', email: 'member@example.com', role: 'member' }, + getAuthConfig: async () => ({ features: {} }), + activeOrganization: { id: 'org_1', name: 'Org' }, + }), + // The least-privileged value, so nothing here is an admin-only result. + useWorkspaceAdminStatus: () => ({ isAdmin: false, isResolved: true }), +})); + +const actionRunnerStub = { registerHandler: vi.fn(), getContext: () => ({}) }; +vi.mock('@object-ui/react', async (importOriginal) => ({ + ...(await importOriginal>()), + useActionRunner: () => ({ execute: vi.fn(), runner: actionRunnerStub }), + useGlobalUndo: () => {}, + useMutationInvalidationBridge: () => {}, +})); + +// --------------------------------------------------------------------------- +// The transport. One stubbed `fetch` behind a real adapter + real client. +// --------------------------------------------------------------------------- + +const json = (status: number, body: unknown) => + new Response(JSON.stringify(body), { status, headers: { 'Content-Type': 'application/json' } }); + +/** `GET /api/v1/meta/app/:name` for an app that exists and refuses this session. */ +const deniedBody = (app: string) => ({ + success: false, + error: { code: 'PERMISSION_DENIED', message: `You do not have permission to open the '${app}' app.` }, +}); +/** …and for a name with nothing behind it. */ +const ABSENT_BODY = { + error: { code: 'RESOURCE_NOT_FOUND', message: 'Metadata item not found or access denied.' }, +}; + +/** URLs the by-name meta route was asked for, in order. */ +let metaItemRequests: string[] = []; +/** How the by-name route answers, per app name; unlisted names are absent. */ +let byName: Record Response> = {}; + +function makeAdapter() { + const fetchImpl = vi.fn(async (input: RequestInfo | URL) => { + const url = String(input); + if (url.includes('/api/v1/discovery')) { + return json(200, { success: true, data: { version: 'v0', routes: {}, capabilities: {} } }); + } + const item = /\/api\/v1\/meta\/app\/([^/?]+)/.exec(url); + if (item) { + metaItemRequests.push(url); + const answer = byName[decodeURIComponent(item[1])]; + return answer ? answer() : json(404, ABSENT_BODY); + } + // Nothing else is part of this question; a loud 500 keeps an unexpected + // call from passing as a silent success. + return json(500, { error: { code: 'UNEXPECTED_REQUEST', message: url } }); + }); + return new ObjectStackAdapter({ baseUrl: 'http://test.local', fetch: fetchImpl }); +} + +let adapter: unknown = null; +vi.mock('../../providers/AdapterProvider', async (importOriginal) => ({ + ...(await importOriginal>()), + useAdapter: () => adapter, +})); + +/** + * The list as the SERVER hands it to this session: an app the session may not + * open is simply ABSENT (`filterAppForUser`). The member's one app is CRM, the + * launcher's default, which a pseudo-route falls back to. + */ +const CRM_APP = { + name: 'crm', + label: 'CRM', + isDefault: true, + navigation: [{ id: 'nav_leads', type: 'object', objectName: 'crm_lead', label: 'Leads' }], +}; +const SETUP_APP = { name: 'setup', label: 'Setup', navigation: [] }; +let metadataApps: unknown[] = []; +vi.mock('../../providers/MetadataProvider', async (importOriginal) => ({ + ...(await importOriginal>()), + useMetadata: () => ({ + apps: metadataApps, + objects: [], + loading: false, + ensureType: undefined, + error: null, + refresh: vi.fn(async () => {}), + }), +})); + +import { AppContent } from '../AppContent'; + +function LocationProbe() { + const location = useLocation(); + return
{location.pathname}
; +} + +/** The host's `system/approvals` route, as `apps/console`'s `systemRoutes` mounts it. */ +function ApprovalsInboxStub() { + return counted('approvals-inbox-page'); +} +const hostSystemRoutes = } />; + +function renderConsoleAt(initialUrl: string) { + return render( + + + + } + /> + home
} /> + + , + ); +} + +const pathname = () => screen.getByTestId('pathname').textContent; + +/** Every page stub this file counts; a refused caller mounts none of them. */ +const PAGE_STUBS = [ + 'metadata-directory-page', + 'metadata-resource-list-page', + 'metadata-resource-edit-page', + 'object-view', + 'record-detail-view', +]; +const pagesMounted = () => PAGE_STUBS.filter((id) => (mounts[id] ?? 0) > 0); + +describe('AppContent — Setup\'s item routes wait on the app\'s access verdict (objectui#12706)', () => { + beforeEach(() => { + vi.clearAllMocks(); + for (const k of Object.keys(mounts)) delete mounts[k]; + metadataApps = [CRM_APP]; + metaItemRequests = []; + byName = { setup: () => json(403, deniedBody('setup')) }; + adapter = makeAdapter(); + }); + + it.each([ + ['the permission-set editor', '/apps/setup/metadata/permission/crm_sales_user'], + ['the position editor', '/apps/setup/metadata/position/sales_rep'], + ['a metadata list', '/apps/setup/metadata/permission'], + ['the metadata directory', '/apps/setup/metadata'], + ])('a refused caller gets app-access-denied on %s, with nothing of the page mounted', async (_label, url) => { + renderConsoleAt(`${url}?scope=environment`); + + expect(await screen.findByTestId('app-access-denied')).toBeInTheDocument(); + expect(screen.queryByTestId('console-layout')).not.toBeInTheDocument(); + expect(pagesMounted()).toEqual([]); + // The verdict asked is Setup's, the app the URL names, never the fallback's. + expect(metaItemRequests).toHaveLength(1); + expect(metaItemRequests[0]).toContain('/api/v1/meta/app/setup'); + // The URL is left as the caller typed it. + expect(pathname()).toBe(url); + }); + + it('a refused caller opening Setup\'s root gets app-access-denied, not the default app\'s landing', async () => { + // Before this card the same URL ran the CRM app's landing and moved the URL + // onto it (`/apps/setup/crm_lead`), under the CRM app's chrome. + renderConsoleAt('/apps/setup'); + + expect(await screen.findByTestId('app-access-denied')).toBeInTheDocument(); + expect(screen.queryByTestId('console-layout')).not.toBeInTheDocument(); + expect(pagesMounted()).toEqual([]); + expect(pathname()).toBe('/apps/setup'); + // The screen owns its way back (it renders above `ConsoleLayout`). + expect(screen.getByTestId('app-access-denied-home')).toBeInTheDocument(); + }); + + it('a legacy metadata spelling under the refused app gets the same verdict', async () => { + renderConsoleAt('/apps/setup/component/metadata/resource/crm_sales_user?type=permission'); + + expect(await screen.findByTestId('app-access-denied')).toBeInTheDocument(); + expect(pagesMounted()).toEqual([]); + }); + + it('the verdict is the app\'s own, not a Setup rule: another refused app\'s metadata route is refused too', async () => { + byName.finance = () => json(403, deniedBody('finance')); + + renderConsoleAt('/apps/finance/metadata/object'); + + expect(await screen.findByTestId('app-access-denied')).toBeInTheDocument(); + expect(pagesMounted()).toEqual([]); + expect(metaItemRequests[0]).toContain('/api/v1/meta/app/finance'); + }); + + it('CONTROL — an admitted caller gets the page, and nothing is probed', async () => { + metadataApps = [CRM_APP, SETUP_APP]; + + renderConsoleAt('/apps/setup/metadata/permission/crm_sales_user?scope=environment'); + + expect(await screen.findByTestId('metadata-resource-edit-page')).toHaveTextContent('permission/crm_sales_user'); + expect(screen.getByTestId('console-layout')).toHaveAttribute('data-active-app', 'setup'); + expect(screen.queryByTestId('app-access-denied')).not.toBeInTheDocument(); + expect(metaItemRequests).toEqual([]); + }); + + it('CONTROL — a server that has no such app changes nothing: the page renders as before', async () => { + // A 404 is not a refusal; the pseudo-route keeps its default-app fallback. + byName = {}; + + renderConsoleAt('/apps/setup/metadata/permission'); + + expect(await screen.findByTestId('metadata-resource-list-page')).toHaveTextContent('permission'); + expect(screen.getByTestId('console-layout')).toHaveAttribute('data-active-app', 'crm'); + expect(screen.queryByTestId('app-access-denied')).not.toBeInTheDocument(); + }); + + it('CONTROL — the approvals inbox stays reachable under the Setup segment for a refused caller', async () => { + renderConsoleAt('/apps/setup/system/approvals'); + + expect(await screen.findByTestId('approvals-inbox-page')).toBeInTheDocument(); + expect(screen.queryByTestId('app-access-denied')).not.toBeInTheDocument(); + expect(metaItemRequests).toEqual([]); + }); + + it('CONTROL — the record page the inbox links to stays reachable for a refused caller', async () => { + // `ApprovalsInboxPage` opens a request's record at `/apps/setup/OBJECT/record/ID`. + renderConsoleAt('/apps/setup/crm_opportunity/record/opp_000001'); + + expect(await screen.findByTestId('record-detail-view')).toHaveTextContent('crm_opportunity/opp_000001'); + expect(screen.queryByTestId('app-access-denied')).not.toBeInTheDocument(); + expect(metaItemRequests).toEqual([]); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/AssignedUsersSection.readRefused-12706.test.tsx b/packages/app-shell/src/views/metadata-admin/AssignedUsersSection.readRefused-12706.test.tsx new file mode 100644 index 0000000000..de1d410849 --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/AssignedUsersSection.readRefused-12706.test.tsx @@ -0,0 +1,123 @@ +/** + * ObjectUI + * Copyright (c) 2026 ObjectStack. Licensed under the MIT license. + * + * A refused holders read is not an empty list (objectui#12706). + * + * Measured on a real server: a member opened a permission set's page, the + * section's own read `GET /api/v1/data/sys_user_permission_set` answered 403, + * and the section drew "0" and "No users assigned yet" over a set that has a + * holder. `PositionHoldersSection` beside it said the refusal. This file holds + * that the section says the refusal in the server's words, draws neither the + * count nor the empty copy while it does, and draws the empty copy only after + * a read that succeeded and came back empty. A refused Remove is said too: it + * used to be swallowed with the row left in place and nothing said. + * + * The adapter is a real `ObjectStackAdapter` over a stubbed `fetch`, so the + * refusal reaches the section through the real client's error stamping; the + * 403 body below is the one the server sent in that measurement, verbatim. + */ + +import * as React from 'react'; +import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest'; +import { render, screen, waitFor, cleanup, fireEvent } from '@testing-library/react'; +import { AdapterCtx } from '@object-ui/react'; +import { ObjectStackAdapter } from '@object-ui/data-objectstack'; +import { AssignedUsersSection } from './AssignedUsersSection'; + +vi.mock('@object-ui/fields', async (importOriginal) => ({ + ...(await importOriginal()), + RecordPickerDialog: () => null, +})); + +const json = (status: number, body: unknown) => + new Response(JSON.stringify(body), { status, headers: { 'Content-Type': 'application/json' } }); + +/** The server's sentence for a refused read or write, as it sent it. */ +const REFUSAL = 'You do not have permission to perform this action. Contact your administrator if you need access.'; +const refusedBody = (object: string) => ({ error: REFUSAL, code: 'PERMISSION_DENIED', object }); + +const EMPTY_COPY = /No users assigned yet/i; + +/** How the data route answers, per `METHOD object`; unlisted reads answer no rows. */ +let answers: Record Response> = {}; + +function makeAdapter() { + const fetchImpl = vi.fn(async (input: RequestInfo | URL, init?: RequestInit) => { + const url = String(input); + const method = (init?.method ?? 'GET').toUpperCase(); + if (url.includes('/api/v1/discovery')) { + return json(200, { success: true, data: { version: 'v0', routes: {}, capabilities: {} } }); + } + const data = /\/api\/v1\/data\/([^/?]+)/.exec(url); + if (data) { + const answer = answers[`${method} ${data[1]}`]; + if (answer) return answer(); + if (method === 'GET') return json(200, { object: data[1], records: [], total: 0 }); + } + return json(500, { error: { code: 'UNEXPECTED_REQUEST', message: `${method} ${url}` } }); + }); + return new ObjectStackAdapter({ baseUrl: 'http://test.local', fetch: fetchImpl }); +} + +function renderSection() { + render( + + + , + ); +} + +/** The section's header line: its title, then the count when one is drawn. */ +const headerText = () => screen.getByText('Assigned Users').parentElement?.textContent ?? ''; + +beforeEach(() => { + answers = {}; +}); +afterEach(() => { + cleanup(); +}); + +describe('AssignedUsersSection — a refused read is said, not drawn as empty (objectui#12706)', () => { + it('a 403 holders read shows the refusal, and neither "No users assigned yet" nor a count', async () => { + answers['GET sys_user_permission_set'] = () => json(403, refusedBody('sys_user_permission_set')); + + renderSection(); + + const said = await screen.findByTestId('assigned-users-read-failed'); + expect(said).toHaveAttribute('role', 'alert'); + expect(said.textContent).toBe(REFUSAL); + expect(screen.queryByText(EMPTY_COPY)).toBeNull(); + expect(headerText()).toBe('Assigned Users'); + }); + + it('CONTROL — a 200 empty read shows the empty copy and the count, and no refusal', async () => { + renderSection(); + + await waitFor(() => expect(screen.getByText(EMPTY_COPY)).toBeTruthy()); + expect(screen.queryByTestId('assigned-users-read-failed')).toBeNull(); + expect(screen.queryByRole('alert')).toBeNull(); + expect(headerText()).toBe('Assigned Users0'); + }); + + it('a refused Remove says so in the server\'s words, and the row stays', async () => { + answers['GET sys_user_permission_set'] = () => + json(200, { + object: 'sys_user_permission_set', + records: [{ id: 'grant_1', permission_set: 'crm_sales_user', user_id: 'u_direct' }], + total: 1, + }); + answers['GET sys_user'] = () => + json(200, { object: 'sys_user', records: [{ id: 'u_direct', name: 'Direct Dana', email: 'dana@example.com' }], total: 1 }); + answers['DELETE sys_user_permission_set'] = () => json(403, refusedBody('sys_user_permission_set')); + + renderSection(); + await waitFor(() => expect(screen.getByText('Direct Dana')).toBeTruthy()); + + fireEvent.click(screen.getByLabelText('Remove')); + + const alert = await screen.findByRole('alert'); + expect(alert.textContent).toBe(REFUSAL); + expect(screen.getByText('Direct Dana')).toBeTruthy(); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/AssignedUsersSection.tsx b/packages/app-shell/src/views/metadata-admin/AssignedUsersSection.tsx index 91688f271f..5aee2d90a3 100644 --- a/packages/app-shell/src/views/metadata-admin/AssignedUsersSection.tsx +++ b/packages/app-shell/src/views/metadata-admin/AssignedUsersSection.tsx @@ -55,7 +55,7 @@ import { Button } from '@object-ui/components'; import { resolveAffordance, type SchemaLike } from '@object-ui/core'; import { RecordPickerDialog } from '@object-ui/fields'; import { usePermissions } from '@object-ui/permissions'; -import { useAdapter, useMetadata } from '@object-ui/react'; +import { extractWriteErrorMessage, useAdapter, useMetadata } from '@object-ui/react'; import { Plus, X, Users, Loader2, AlertCircle } from 'lucide-react'; import { useMetadataLocale } from './i18n.js'; @@ -102,6 +102,8 @@ function useCopy() { seatFull: (n: number) => 'AI 席位已用完(' + n + '/' + n + ')。请先移除一个用户,或在许可证中提升席位上限,再分配新用户。', addFailed: '分配失败,请重试。', + removeFailed: '移除失败,请重试。', + loadFailed: '无法读取已分配用户。', countOf: (n: number) => n + ' 人', direct: '直授', viaPosition: (p: string) => '经岗位 ' + p, @@ -120,6 +122,8 @@ function useCopy() { seatFull: (n: number) => 'All ' + n + ' AI seat(s) are in use. Remove a user or raise the license cap before assigning another.', addFailed: 'Failed to assign. Please try again.', + removeFailed: 'Failed to remove. Please try again.', + loadFailed: 'Could not read the users assigned to this set.', countOf: (n: number) => String(n), direct: 'direct', viaPosition: (p: string) => 'via position ' + p, @@ -151,6 +155,15 @@ function useCopy() { const asArray = (res: any): any[] => Array.isArray(res) ? res : res?.data ?? []; +/** + * The server's own sentence out of a refused call, or `''` when it gave none. + * Read through `@object-ui/react`'s shared reader, the one place the adapters' + * error shapes are told apart (AGENTS #1), rather than a chain of guessed keys + * here: the ObjectStack client puts the sentence on `message` and the parsed + * body on `details`, and sets neither `body` nor `error` on the error. + */ +const refusalSentence = (err: unknown): string => extractWriteErrorMessage(err) ?? ''; + const personLabel = (u: any): string => u?.full_name || u?.name || u?.display_name || u?.email || String(u?.id ?? ''); @@ -186,6 +199,13 @@ export function AssignedUsersSection({ permissionSetName }: AssignedUsersSection const [pickerOpen, setPickerOpen] = React.useState(false); const [busy, setBusy] = React.useState(false); const [error, setError] = React.useState(null); + /** + * The holders read failed (objectui#12706). A refused read is not an empty + * list: while this is set, the section says the read failed, in the server's + * words as `PositionHoldersSection` does, and draws neither a count nor the + * empty copy. `said` is the server's sentence, `''` when it gave none. + */ + const [readFailure, setReadFailure] = React.useState<{ said: string } | null>(null); /** * May the caller add or remove a holder? Each is a write of a @@ -278,9 +298,11 @@ export function AssignedUsersSection({ permissionSetName }: AssignedUsersSection }), ); setEveryoneBound(boundEveryone); - } catch { + setReadFailure(null); + } catch (err) { setRows([]); setEveryoneBound(false); + setReadFailure({ said: refusalSentence(err) }); } finally { setLoading(false); } @@ -331,11 +353,13 @@ export function AssignedUsersSection({ permissionSetName }: AssignedUsersSection try { await adapter.delete(GRANT_OBJECT, grantId); await load(); - } catch { - /* keep the row; a failed delete is non-destructive */ + } catch (err) { + // The row stays, and the refusal is said in the server's words, as a + // failed Add is (objectui#12706). + setError(refusalSentence(err) || c.removeFailed); } }, - [adapter, load], + [adapter, load, c], ); return ( @@ -344,7 +368,7 @@ export function AssignedUsersSection({ permissionSetName }: AssignedUsersSection
{c.title} - {!loading && ( + {!loading && !readFailure && ( {c.countOf(rows.length)} )}
@@ -375,6 +399,17 @@ export function AssignedUsersSection({ permissionSetName }: AssignedUsersSection
)} + {!loading && readFailure && ( +
+ + {readFailure.said || c.loadFailed} +
+ )} + {everyoneBound && (
@@ -387,7 +422,7 @@ export function AssignedUsersSection({ permissionSetName }: AssignedUsersSection {c.loading}
- ) : rows.length === 0 ? ( + ) : readFailure ? null : rows.length === 0 ? ( !everyoneBound && (
{/* The click-Add sentence names a button only `canAdd` renders. */} diff --git a/packages/app-shell/src/views/metadata-admin/PositionHoldersSection.readRefused-12706.test.tsx b/packages/app-shell/src/views/metadata-admin/PositionHoldersSection.readRefused-12706.test.tsx new file mode 100644 index 0000000000..505e0efa2c --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/PositionHoldersSection.readRefused-12706.test.tsx @@ -0,0 +1,103 @@ +/** + * ObjectUI + * Copyright (c) 2026 ObjectStack. Licensed under the MIT license. + * + * A refused holders read is not an empty list (objectui#12706). + * + * Measured on a real server by the card's step 4: the position editor's own + * read `GET /api/v1/data/sys_user_position` answered 403, and the section drew + * the refusal as an alert ABOVE "Nobody holds this position yet." — the read + * failure set the rows to none, and the empty branch drew whatever the error + * held. This file holds that the section says the refusal in the server's + * words, draws neither the count nor the empty copy while it does, and draws + * the empty copy only after a read that succeeded and came back empty. It is + * the same refusal state `AssignedUsersSection` holds beside it. + * + * The adapter is a real `ObjectStackAdapter` over a stubbed `fetch`, so the + * refusal reaches the section through the real client's error stamping; the + * 403 body below is the one the server sent in that measurement, verbatim. + */ + +import * as React from 'react'; +import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest'; +import { render, screen, waitFor, cleanup } from '@testing-library/react'; +import { AdapterCtx } from '@object-ui/react'; +import { ObjectStackAdapter } from '@object-ui/data-objectstack'; +import { PositionHoldersSection } from './PositionHoldersSection'; + +vi.mock('@object-ui/fields', async (importOriginal) => ({ + ...(await importOriginal()), + RecordPickerDialog: () => null, +})); + +const json = (status: number, body: unknown) => + new Response(JSON.stringify(body), { status, headers: { 'Content-Type': 'application/json' } }); + +/** The server's sentence for a refused read, as it sent it. */ +const REFUSAL = 'You do not have permission to perform this action. Contact your administrator if you need access.'; +const refusedBody = (object: string) => ({ error: REFUSAL, code: 'PERMISSION_DENIED', object }); + +const EMPTY_COPY = /Nobody holds this position yet/i; + +/** How the data route answers, per `METHOD object`; unlisted reads answer no rows. */ +let answers: Record Response> = {}; + +function makeAdapter() { + const fetchImpl = vi.fn(async (input: RequestInfo | URL, init?: RequestInit) => { + const url = String(input); + const method = (init?.method ?? 'GET').toUpperCase(); + if (url.includes('/api/v1/discovery')) { + return json(200, { success: true, data: { version: 'v0', routes: {}, capabilities: {} } }); + } + const data = /\/api\/v1\/data\/([^/?]+)/.exec(url); + if (data) { + const answer = answers[`${method} ${data[1]}`]; + if (answer) return answer(); + if (method === 'GET') return json(200, { object: data[1], records: [], total: 0 }); + } + return json(500, { error: { code: 'UNEXPECTED_REQUEST', message: `${method} ${url}` } }); + }); + return new ObjectStackAdapter({ baseUrl: 'http://test.local', fetch: fetchImpl }); +} + +function renderSection() { + render( + + + , + ); +} + +/** The section's header line: its title, then the count when one is drawn. */ +const headerText = () => screen.getByText('Position holders').parentElement?.textContent ?? ''; + +beforeEach(() => { + answers = {}; +}); +afterEach(() => { + cleanup(); +}); + +describe('PositionHoldersSection — a refused read is said, not drawn as empty (objectui#12706)', () => { + it('a 403 holders read shows the refusal, and neither "Nobody holds this position yet." nor a count', async () => { + answers['GET sys_user_position'] = () => json(403, refusedBody('sys_user_position')); + + renderSection(); + + const said = await screen.findByTestId('position-holders-read-failed'); + expect(said).toHaveAttribute('role', 'alert'); + expect(said.textContent).toBe(REFUSAL); + expect(screen.queryByText(EMPTY_COPY)).toBeNull(); + expect(screen.getAllByRole('alert')).toHaveLength(1); + expect(headerText()).toBe('Position holders'); + }); + + it('CONTROL — a 200 empty read shows the empty copy and the count, and no refusal', async () => { + renderSection(); + + await waitFor(() => expect(screen.getByText(EMPTY_COPY)).toBeTruthy()); + expect(screen.queryByTestId('position-holders-read-failed')).toBeNull(); + expect(screen.queryByRole('alert')).toBeNull(); + expect(headerText()).toBe('Position holders0'); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/PositionHoldersSection.tsx b/packages/app-shell/src/views/metadata-admin/PositionHoldersSection.tsx index a0b47e4bf1..f7be960ec2 100644 --- a/packages/app-shell/src/views/metadata-admin/PositionHoldersSection.tsx +++ b/packages/app-shell/src/views/metadata-admin/PositionHoldersSection.tsx @@ -35,7 +35,7 @@ import { Button } from '@object-ui/components'; import { resolveAffordance, type SchemaLike } from '@object-ui/core'; import { RecordPickerDialog } from '@object-ui/fields'; import { usePermissions } from '@object-ui/permissions'; -import { useAdapter, useMetadata } from '@object-ui/react'; +import { extractWriteErrorMessage, useAdapter, useMetadata } from '@object-ui/react'; import { AlertCircle, Loader2, Plus, UserCheck, X } from 'lucide-react'; import { useMetadataLocale } from './i18n.js'; @@ -62,11 +62,14 @@ const asArray = (res: any): any[] => (Array.isArray(res) ? res : res?.data ?? [] const personLabel = (u: any): string => u?.full_name || u?.name || u?.display_name || u?.email || String(u?.id ?? ''); -/** The server's own sentence out of a refused write, else the fallback. */ -function refusalText(err: any, fallback: string): string { - const raw = String(err?.body?.error ?? err?.error ?? err?.message ?? '').trim(); - return raw || fallback; -} +/** + * The server's own sentence out of a refused call, or `''` when it gave none. + * Read through `@object-ui/react`'s shared reader, the one place the adapters' + * error shapes are told apart (AGENTS #1), as `AssignedUsersSection` reads it: + * the ObjectStack client puts the sentence on `message` and the parsed body on + * `details`, and sets neither `body` nor `error` on the error. + */ +const refusalSentence = (err: unknown): string => extractWriteErrorMessage(err) ?? ''; function useCopy() { const zh = useMetadataLocale() === 'zh-CN'; @@ -107,6 +110,14 @@ export function PositionHoldersSection({ name }: PositionHoldersSectionProps): R const [pickerOpen, setPickerOpen] = React.useState(false); const [busy, setBusy] = React.useState(false); const [error, setError] = React.useState(null); + /** + * The holders read failed (objectui#12706). A refused read is not an empty + * list: while this is set, the section says the read failed, in the server's + * words, and draws neither a count nor "Nobody holds this position yet." — + * the empty copy follows only a read that succeeded. `said` is the server's + * sentence, `''` when it gave none. + */ + const [readFailure, setReadFailure] = React.useState<{ said: string } | null>(null); /** * May the caller add or remove a holder? Each is a write of a @@ -165,13 +176,14 @@ export function PositionHoldersSection({ name }: PositionHoldersSectionProps): R }; }), ); - } catch (err: any) { + setReadFailure(null); + } catch (err) { setRows([]); - setError(refusalText(err, c.loadFailed)); + setReadFailure({ said: refusalSentence(err) }); } finally { setLoading(false); } - }, [adapter, name, c]); + }, [adapter, name]); React.useEffect(() => { void load(); @@ -191,8 +203,8 @@ export function PositionHoldersSection({ name }: PositionHoldersSectionProps): R await adapter.create(USER_POSITION_OBJECT, { user_id: uid, [USER_POSITION_NAME_FIELD]: name }); } await load(); - } catch (err: any) { - setError(refusalText(err, c.addFailed)); + } catch (err) { + setError(refusalSentence(err) || c.addFailed); } finally { setBusy(false); setPickerOpen(false); @@ -207,8 +219,8 @@ export function PositionHoldersSection({ name }: PositionHoldersSectionProps): R try { await adapter.delete(USER_POSITION_OBJECT, assignmentId); await load(); - } catch (err: any) { - setError(refusalText(err, c.removeFailed)); + } catch (err) { + setError(refusalSentence(err) || c.removeFailed); } }, [adapter, load, c], @@ -220,7 +232,9 @@ export function PositionHoldersSection({ name }: PositionHoldersSectionProps): R
{c.title} - {!loading && {rows.length}} + {!loading && !readFailure && ( + {rows.length} + )}
{canAdd && (
+ ) : readFailure ? ( +
+ + {readFailure.said || c.loadFailed} +
) : rows.length === 0 ? (
{c.empty}
) : (