From d8927ba1a3c3a2f92650b18199dfd7a5a5313262 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 19 Aug 2026 17:48:29 +0000 Subject: [PATCH] fix(console): suppress the Approvals Inbox record link when the viewer cannot read the target MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Approver routing goes by position; record visibility is a separate gate, and nothing reconciles the two. An approver routed a request about a record they cannot read was still offered the row's record chip, which landed on the record page's "Record not found — the record you are looking for does not exist or may have been deleted". The row (and the drawer's record title, which offers the same link to the same record for the same viewer) now render the title as plain text when a readability probe says the target is unreadable. The title itself still shows: it comes from the request's payload snapshot, which is what the approver decides from. The probe is one batched `id in (…)` list read per distinct object, projected to the id column, and each target is probed at most once per mount — measured at one call for a page of rows sharing an object, independent of row count. It fails open: unknown, unanswered or failed leaves the link exactly as it is today. ⛔ The server's access semantics are untouched and unreported: it answers 404 rather than 403 deliberately, so as not to confirm a record's existence to a principal not permitted to see it. Nothing here says why a target is unreadable, no copy is added, and no test asserts anything about 404-vs-403. Part of #5211 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RV6yuVCxymHYE16PL9vQkE --- .changeset/lucky-buttons-clap.md | 12 + .../ApprovalsInboxPage.recordLink.test.tsx | 255 ++++++++++++++++++ .../src/pages/system/ApprovalsInboxPage.tsx | 46 +++- .../pages/system/recordReadability.test.ts | 153 +++++++++++ .../src/pages/system/recordReadability.ts | 249 +++++++++++++++++ 5 files changed, 714 insertions(+), 1 deletion(-) create mode 100644 .changeset/lucky-buttons-clap.md create mode 100644 apps/console/src/pages/system/ApprovalsInboxPage.recordLink.test.tsx create mode 100644 apps/console/src/pages/system/recordReadability.test.ts create mode 100644 apps/console/src/pages/system/recordReadability.ts diff --git a/.changeset/lucky-buttons-clap.md b/.changeset/lucky-buttons-clap.md new file mode 100644 index 000000000..fb7e6239f --- /dev/null +++ b/.changeset/lucky-buttons-clap.md @@ -0,0 +1,12 @@ +--- +'@object-ui/console': patch +--- + +Approvals Inbox: stop offering a record link that dead-ends for the viewer it is +offered to. Approver routing goes by position while record visibility is a +separate gate, so an approver can be routed a request about a record they cannot +read — the row's record chip then landed on the record page's "Record not found". +The row (and the drawer's record title) now suppress the link for exactly those +targets, decided by one batched readability read per distinct object. Nothing +else changes: the title still shows, the approval decision path is untouched, and +the server's access semantics are neither read nor reported on. diff --git a/apps/console/src/pages/system/ApprovalsInboxPage.recordLink.test.tsx b/apps/console/src/pages/system/ApprovalsInboxPage.recordLink.test.tsx new file mode 100644 index 000000000..46912873a --- /dev/null +++ b/apps/console/src/pages/system/ApprovalsInboxPage.recordLink.test.tsx @@ -0,0 +1,255 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Approvals Inbox — the record link is offered only to a viewer who can open it + * (objectui#5211). + * + * ## The defect + * + * Approver routing goes by position; record visibility is a separate gate, and + * nothing reconciles the two. An approver routed a request about a record they + * cannot read was still offered the row's record chip, which landed on the + * record page's "Record not found — … does not exist or may have been deleted". + * Per the maintainer's ruling (2026-08-19) the link is suppressed for exactly + * those rows. + * + * ## What each case is measuring + * + * - the unreadable row renders **no link** (and still renders the title, which + * comes from the request's payload snapshot); + * - the readable row is untouched — link, and the same href as before; + * - the whole page costs **one** readability read for two rows of one object — + * the batching the ruling made the deciding criterion; + * - the **decision path is untouched**: one case pins inline approve WITHOUT + * involving the probe (so it holds on the pre-change tree too — that is what + * makes it a pin), and one shows that a row whose link was suppressed is still + * decidable, which is the thing that already worked and must keep working. + * + * ⛔ Nothing here asserts anything about the server answering `404` rather than + * `403` for the by-id read. That is the server's deliberate choice and is out of + * scope in both directions; the probe issues a list read, never a by-id one. + * + * No build artifact sits between the edit and this test: the root Vitest config + * aliases every `@object-ui` specifier at that package's `src` directory, and + * the page and probe under test are this app's own source. (Writing that path + * with a glob segment would close this comment — the sequence is a block-comment + * terminator, and the parse error it produces points at a line far below.) + */ + +import '@testing-library/jest-dom/vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render, screen, cleanup, fireEvent, waitFor, within } from '@testing-library/react'; +import { MemoryRouter, Routes, Route } from 'react-router-dom'; + +const APP = 'com.objectstack.account'; + +const { adapterFind, approvalsApiStub, rows, ADAPTER, AUTH, I18N } = vi.hoisted(() => { + const rows = [ + { + id: 'req_readable', + process_name: 'invoice_approval', + process_label: 'Invoice Approval', + object_name: 'showcase_invoice', + object_label: 'Invoice', + record_id: 'inv_readable', + record_title: 'Readable Invoice', + status: 'pending', + pending_approvers: ['u_1'], + submitter_id: 'u_2', + submitter_name: 'Sam Submitter', + submitted_at: '2026-08-19T00:00:00.000Z', + }, + { + id: 'req_hidden', + process_name: 'invoice_approval', + process_label: 'Invoice Approval', + object_name: 'showcase_invoice', + object_label: 'Invoice', + record_id: 'inv_hidden', + record_title: 'Hidden Invoice', + status: 'pending', + pending_approvers: ['u_1'], + submitter_id: 'u_2', + submitter_name: 'Sam Submitter', + submitted_at: '2026-08-19T00:00:00.000Z', + }, + ]; + + /** + * The server's own behaviour, reduced: a record the principal's grant filters + * out never appears in the row set, so an `id in (…)` read simply comes back + * without it. `inv_hidden` is that record. + */ + const adapterFind = vi.fn(async (_object: string, params?: Record) => { + const ids = (params?.$filter as { id?: { $in?: string[] } } | undefined)?.id?.$in ?? []; + return { data: ids.filter((id) => id !== 'inv_hidden').map((id) => ({ id })) }; + }); + + const approvalsApiStub = { + listRequests: vi.fn(async () => ({ data: rows })), + getRequest: vi.fn(async (id: string) => ({ data: rows.find((r) => r.id === id) })), + listActions: vi.fn(async () => ({ data: [] })), + approve: vi.fn(async () => ({ data: rows[0], finalized: true })), + reject: vi.fn(async () => ({ data: rows[0], finalized: true })), + }; + + // STABLE singletons. A mocked hook that returns a fresh object per render + // gives every render a new `user` / `adapter` identity, which re-runs the + // page's load effect (and the probe's) forever — the page never leaves its + // loading skeleton and every query below times out on an empty table. + const ADAPTER = { find: adapterFind }; + const AUTH = { user: { id: 'u_1', email: 'approver@example.com' } }; + const I18N = { + t: (key: string, options?: Record) => String(options?.defaultValue ?? key), + language: 'en', + }; + + return { adapterFind, approvalsApiStub, rows, ADAPTER, AUTH, I18N }; +}); + +vi.mock('@object-ui/i18n', () => ({ useObjectTranslation: () => I18N })); + +vi.mock('@object-ui/auth', () => { + const authFetch = vi.fn(async () => new Response('{}', { status: 200 })); + return { + useAuth: () => AUTH, + createAuthenticatedFetch: () => authFetch, + TokenStorage: { get: () => null }, + }; +}); + +vi.mock('@object-ui/app-shell', () => ({ + useAdapter: () => ADAPTER, + DeclaredActionsBar: () => null, + isViaOverrideRow: () => false, +})); + +// `buildApproverIdentities` stays REAL — the "can this approver act" pin is only +// worth anything if the identity resolution under it is the shipped one. +vi.mock('../../services/approvalsApi', async (importOriginal) => ({ + ...(await importOriginal>()), + approvalsApi: approvalsApiStub, +})); + +// Imported after the mocks so the page picks them up. +import { ApprovalsInboxPage } from './ApprovalsInboxPage'; + +function renderInbox() { + return render( + + + } /> + + , + ); +} + +/** The desktop table row holding `title` (the mobile card renders no links). */ +function rowFor(title: string): HTMLElement { + const found = screen.getAllByRole('row').find((r) => within(r).queryByText(title)); + if (!found) throw new Error(`no row for ${title}`); + return found; +} + +/** + * Approve the row holding `title`, through the row button and its confirmation. + * + * Query and click in ONE synchronous step, with `fireEvent`: the page declares + * `RecordCell` / `InlineActions` INSIDE its component body, so every re-render + * is a fresh component type and React replaces those DOM nodes. A multi-tick + * `userEvent` interaction lets a re-render land between its pointerdown and its + * click, and the rest of the sequence goes to a detached node — the click then + * silently does nothing and the confirmation never opens. + */ +async function approveFromRow(title: string): Promise { + fireEvent.click(within(rowFor(title)).getByRole('button', { name: 'Approve' })); + const dialog = await screen.findByRole('alertdialog'); + fireEvent.click(within(dialog).getByRole('button', { name: 'Approve' })); +} + +beforeEach(() => { + adapterFind.mockClear(); + for (const fn of Object.values(approvalsApiStub)) fn.mockClear(); +}); +afterEach(cleanup); + +describe('Approvals Inbox — record link readability (objectui#5211)', () => { + it('suppresses the record link on a row whose target this viewer cannot read', async () => { + renderInbox(); + + // The readable row is untouched: still a link, still the same href. + const link = await screen.findByRole('link', { name: /Readable Invoice/ }); + expect(link).toHaveAttribute('href', `/apps/${APP}/showcase_invoice/record/inv_readable`); + + // The unreadable one loses the link once the probe has answered… + await waitFor(() => + expect(screen.queryByRole('link', { name: /Hidden Invoice/ })).not.toBeInTheDocument(), + ); + // …and only the link. The title still shows — it comes from the request's + // payload snapshot, which is what the approver decides from. + expect(screen.getAllByText('Hidden Invoice').length).toBeGreaterThan(0); + expect(within(rowFor('Hidden Invoice')).queryByRole('link')).not.toBeInTheDocument(); + }); + + it('costs ONE readability read for a page of rows sharing an object', async () => { + renderInbox(); + await screen.findByRole('link', { name: /Readable Invoice/ }); + await waitFor(() => expect(adapterFind).toHaveBeenCalled()); + + // Two rows, one object => one batched list read. Not one per row. + expect(adapterFind).toHaveBeenCalledTimes(1); + expect(adapterFind).toHaveBeenCalledWith('showcase_invoice', { + $filter: { id: { $in: ['inv_readable', 'inv_hidden'] } }, + $select: ['id'], + $top: 2, + }); + }); + + /** + * A PIN, deliberately independent of the readability probe: it must pass on + * the tree BEFORE this change as well as after, which is what makes it + * evidence that the decision path was left alone rather than evidence that + * the change works. (It is stated separately from the case below because a + * pin coupled to the new behaviour proves nothing about the old.) + */ + it('pins the inline decision path — approve still fires for the row it is clicked on', async () => { + renderInbox(); + // Rows are up (desktop table + mobile card each render one per request). + await screen.findAllByRole('button', { name: 'Approve' }); + + await approveFromRow('Readable Invoice'); + + await waitFor(() => + expect(approvalsApiStub.approve).toHaveBeenCalledWith('req_readable', { actor_id: 'u_1' }), + ); + }); + + it('still lets the approver decide a row whose link was suppressed', async () => { + renderInbox(); + // Wait for the rows FIRST. Waiting only for the link's absence passes + // vacuously while the page is still a loading skeleton — measured: on the + // pre-change tree this case went green for that reason alone. + await screen.findByRole('link', { name: /Readable Invoice/ }); + await waitFor(() => + expect(screen.queryByRole('link', { name: /Hidden Invoice/ })).not.toBeInTheDocument(), + ); + + await approveFromRow('Hidden Invoice'); + + await waitFor(() => + expect(approvalsApiStub.approve).toHaveBeenCalledWith('req_hidden', { actor_id: 'u_1' }), + ); + }); + + it('keeps every link when the probe cannot answer (fail open)', async () => { + adapterFind.mockImplementationOnce(async () => { throw new Error('NETWORK'); }); + renderInbox(); + + await screen.findByRole('link', { name: /Readable Invoice/ }); + await waitFor(() => expect(adapterFind).toHaveBeenCalled()); + // Unknown is not "unreadable": a failed probe must never withhold a link + // from someone who could have used it. + expect(await screen.findByRole('link', { name: /Hidden Invoice/ })).toBeInTheDocument(); + expect(rows).toHaveLength(2); + }); +}); diff --git a/apps/console/src/pages/system/ApprovalsInboxPage.tsx b/apps/console/src/pages/system/ApprovalsInboxPage.tsx index 129206205..41016f24f 100644 --- a/apps/console/src/pages/system/ApprovalsInboxPage.tsx +++ b/apps/console/src/pages/system/ApprovalsInboxPage.tsx @@ -103,6 +103,7 @@ import { type ApprovalActionRow, type ApprovalActionAttachment, } from '../../services/approvalsApi'; +import { useRecordReadability } from './recordReadability'; type TabKey = 'pending' | 'submitted' | 'all'; @@ -478,6 +479,25 @@ export function ApprovalsInboxPage() { const [selected, setSelected] = useState(null); const [actions, setActions] = useState([]); const [drawerLoading, setDrawerLoading] = useState(false); + + /** + * objectui#5211 — an approver can be routed a request about a record they + * cannot read (approver routing goes by position; record visibility is a + * separate gate). The link below then dead-ends on the record page's + * "may have been deleted", so it is suppressed for exactly those rows. + * + * One batched list read per distinct object covers the whole page — see + * `recordReadability.ts` for the cost model, the fail-open rule, and why + * nothing here says anything about WHY a target is unreadable. + * + * `selected` joins the loaded rows because the drawer can be deep-linked + * (`?request=`) to a request that is not in the current row set. + */ + const readabilityTargets = useMemo( + () => (selected ? [...rows, selected] : rows), + [rows, selected], + ); + const readability = useRecordReadability(readabilityTargets); // Approve/reject/reassign/send-back/… are server-declared actions rendered by // DeclaredActionsBar (objectui#2697 + framework#3300); their param dialog // collects the comment and — since the shared upload-widget renderer (#2700/ @@ -1080,17 +1100,25 @@ export function ApprovalsInboxPage() { // Surface the decision-relevant amount inline so a reviewer can triage the // queue without opening each request (#2762 P1-3). const amount = decisionAmountEntry(r); + // objectui#5211: no link into a record this viewer cannot open. The title + // still shows — it comes from the request's own payload snapshot, which the + // approver was already given — it just stops being an anchor. + const title = r.record_title || formatIdentity(r.record_id); return (
+ {readability.isUnreadable(r) ? ( +
{title}
+ ) : ( e.stopPropagation()} className="inline-flex items-center gap-1 text-sm hover:underline truncate max-w-full" title={r.record_id} > - {r.record_title || formatIdentity(r.record_id)} + {title} + )}
{objectDisplay(r)} {amount && ( @@ -1680,6 +1708,15 @@ export function ApprovalsInboxPage() {
+ {/* Same suppression as the row (objectui#5211) — the drawer + offers the same link to the same record for the same + viewer, so fixing only the row would move the dead end + one click deeper instead of removing it. */} + {readability.isUnreadable(selected) ? ( +
+ {selected.record_title || formatIdentity(selected.record_id)} +
+ ) : ( {selected.record_title || formatIdentity(selected.record_id)} + )}
{objectDisplay(selected)}
@@ -2064,6 +2102,12 @@ export function ApprovalsInboxPage() { {tr('returnedHint', 'An approver sent this back to you. The record is unlocked — fix the data, then resubmit to start a new approval round.')}
+ {/* NOT readability-suppressed (objectui#5211), deliberately: + this branch renders only for the SUBMITTER of a returned + request, whose whole job here is to edit that record — + a different persona from the approver the suppression is + for, and one who demonstrably reached the record to + submit it. */}