Skip to content

Commit b824a66

Browse files
fix(fields): a lookup whose record read is refused draws the restricted-record placeholder, not a linked raw id (#29708)
<!-- claude-projects-attribution: {"github_login":"zhuangjianguo"} --> _Requested by **Steedos** · [project thread](https://claude.ai/code/project/chan_018ETKaNwMZCNRYD7D8Q9Ubb?thread=cmsg_018ETKaNwMZCNRYD7D8Q9UbbXsaGxtGer5XYc7Y7N6hJir)_ Fixes #29641 Clause-②: no Metadata-protocol: none Claim: 6592434215 ## 维护者速读 - 改了什么:lookup 字段单元格在读取被引用记录时,如果服务端以「无权限」拒绝(403 或 `PERMISSION_DENIED`),现在显示「受限记录」占位:不显示原始 id、不带链接,与「记录不存在」(404)同一外观;拒绝结果会缓存,同一记录只读一次。网络错误、5xx、401 等其他失败保持原来的「未解析」外观(id + 问号)。 - 为什么改:目标对象的读权限拒绝读者时,服务端对记录读取回 403 而不是 404,单元格把它当成读取失败,画出带链接的原始 id,点进去是一个拒绝访问的页面,看起来像坏数据。 - 风险与回滚:只改 `useLookupName` 的 catch 分支,判定复用 `@object-ui/react` 已有的 `classifyLoadError`(`forbidden`),不改数据适配器、不改 `findOne` 的返回值;404 占位、可读记录、在途读取的外观全部不变。回滚即 revert 本 PR。 - 先看:`ui/packages/fields/src/index.tsx` 的 `useLookupName` catch 分支与 `refused` 缓存状态;新测试 `lookupCell.forbiddenRead-29641.test.tsx`。 - 两分钟验证:`pnpm --dir ui exec vitest run packages/fields/src/__tests__/lookupCell.forbiddenRead-29641.test.tsx`,预期 12 个用例全部通过。 - 需要你做的:无,席位落地。 ## What changes A lookup cell names its referenced record by reading it as the viewer (`useLookupName`, `ui/packages/fields/src/index.tsx`). objectui#13335 drew a record the read answered "not found" (`findOne` answers `null`, the adapter's 404) as "Restricted record", with no id and no link. When the target object's read gate refuses the viewer, the server answers the same read with `403 PERMISSION_DENIED`, which the adapter throws, and the hook caught every throw as a failure: the cell drew the raw id beside a question mark, inside a link to a record page that refuses the viewer. | The viewer's read of the referenced record | Before | Now | |---|---|---| | answers `null` (404, hidden by sharing or missing) | Restricted record, no link | unchanged | | throws a refusal: status 403 (`httpStatus`, `status` or `statusCode`) or code `PERMISSION_DENIED` / `FORBIDDEN` | the id with a question mark, linked | Restricted record, no id, no link; cached, read once | | throws anything else (network, 5xx, 401) | the id with a question mark | unchanged | - The refusal is read through `@object-ui/react`'s `classifyLoadError` (`forbidden`), which `@object-ui/fields` already depends on and imports; no new predicate and no message matching. The lookup cache gains a `refused` state, which the hook answers as `absent`, so `LookupCellRenderer` is unchanged apart from a comment. - Measured live before the edit (`pnpm dev:crm`, a member refused `rnv_payroll`): the adapter's `findOne` throws a plain `Error` whose own members are `httpStatus: 403`, `code: 'PERMISSION_DENIED'` and `details` (`error`, `code`, `object`), with no `status` / `statusCode`; `classifyLoadError` answers `forbidden` on it. A missing id under the same object answers the same 403, so the placeholder discloses nothing a not-found would not. - `user` cells: unchanged. `UserCellRenderer` never reads the referenced person (a bare id is `UnresolvedUserReference`), so it has no read answer to classify. - Not changed: `@object-ui/data-objectstack` and what `findOne` returns. ## Tests - `ui/packages/fields/src/__tests__/lookupCell.forbiddenRead-29641.test.tsx` (new, 12): the measured 403 draws the placeholder with no id in text or attributes, no anchor, one `findOne` and no `find`; the four single spellings (`httpStatus`, `status`, `statusCode` 403, code `PERMISSION_DENIED`) each draw it; the refusal is cached (a second cell for the same id draws it with no read) and a 502 on another id afterwards keeps the unresolved face; in a multi-value cell only the first, read entry is restricted. CONTROLS: a `null` answer keeps the placeholder; a 500 with a code, a 503, a network `TypeError` and a 401 each keep the unresolved face with their id. - Unchanged and green: `lookupCell.restrictedRecord-13335.test.tsx`, `lookupCell.referenceNameVisible-29462.test.tsx`, the other lookupCell files and `DetailSection.referenceNameVisible-29462.test.tsx`: 12 files, 97 tests. - Ablations (`scripts/ablation-replace.mjs`, anchor 1 to 0, restored to the HEAD blob, `git diff HEAD` empty), run on the new file plus the #13335 file: A1 the catch records every throw as `err`: 7 failed / 14 passed; A2 the `refused` entry no longer answers `absent`: 7 failed / 14 passed. The 7 are exactly the refusal cases; every control stays green. ## Acceptance notes - Seat additions honoured: the cache pin (refusal cached like a not-found, no cross-key bleed to a later transient failure) is the third test above. - The cell's `useRefObjectSchema` reads the target's object definition (`/meta/object/rnv_payroll`) before and after this change alike; it is not part of the record read and is left as it is. - Local gates on the final head `214f2289` (main merged twice; none of the merged commits touches `ui/packages/fields`): `dispatch-gates --commands` derives 35; all 35 exit 0 (33 in an unlocked loop, `check:type-check-debt` and `pnpm --dir ui exec vitest run scripts/` (170 files) under the verify lock); `--ran`: 35 derived, 35 run, 0 NOT-MEASURED. The same 35 also exit 0 on the earlier head `647dc13a`. `ui/scripts/check-spec-symbol-derivation.mjs` exit 0 (PR #29623 edits the same file's `ReferenceNameOnlyValue`; this diff stays clear of it). `turbo run type-check --filter=@object-ui/fields` exit 0 (48 tasks). `@object-ui/fields` suite: 296 files passed, 1 skipped, 4449 tests. eslint on the two touched ui files: 0 errors. The repo-wide sweep is CI's. ## Verification <details><summary>Live, <code>pnpm dev:crm -- --fresh</code> plus the source Console (Vite), a member refused the payroll object</summary> - Setup is the #29462 recipe: a scratch package `com.example.rnv` with `rnv_payroll` (private, admin-owned) and `rnv_expense` (`public_read`) carrying `Payroll (no key)`, a lookup without `referenceNameVisible`; a member granted read on `rnv_expense` only, impersonated. `GET /api/v1/data/rnv_payroll/ID` as the member: 403 `PERMISSION_DENIED`. - Before (main `fc558923`): list and record page each draw `Payroll (no key)` as the unresolved face with the id, inside a link to `rnv_payroll` (1 anchor). - After (this branch): list and record page each draw "Restricted record" for it, not inside a link, and no anchor to `rnv_payroll` remains. The `referenceNameVisible` fields still draw their served names as text. Screenshots and the per-page readings are in the project folder (`issue-29641/shots/`), not on GitHub. </details> --- _Generated by [Claude Code](https://claude.ai/code/session_01QsR2jRxjiKm4UH4VWn45pF)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 2ea0fc8 commit b824a66

3 files changed

Lines changed: 219 additions & 6 deletions

File tree

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
---
2+
'@object-ui/fields': patch
3+
---
4+
5+
fix(fields): a lookup whose record read is refused draws the restricted-record placeholder, not a linked raw id
6+
7+
Clause-②: no
8+
Metadata-protocol: none
9+
10+
A lookup cell reads the referenced record as the viewer to name it. When the target object's read gate refuses the viewer, the server answers that read with `403 PERMISSION_DENIED`, and the cell used to treat it as a failed read: the raw id beside a question mark, inside a link to a record page that refuses them. A permission refusal (status 403, or the `PERMISSION_DENIED` / `FORBIDDEN` code, as `@object-ui/react`'s `classifyLoadError` reads it) is now drawn like a record that was not found: "Restricted record", with no id and no link, and the refusal is cached so the record is read once. Any other failure (network, 5xx, 401) keeps the unresolved face with its id. `user` cells are unchanged: they never read the referenced person.
Lines changed: 188 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,188 @@
1+
/**
2+
* ObjectUI
3+
* Copyright (c) 2024-present ObjectStack Inc.
4+
*
5+
* This source code is licensed under the MIT license found in the
6+
* LICENSE file in the root directory of this source tree.
7+
*/
8+
9+
/**
10+
* objectstack#29641 — a lookup to a record whose OBJECT the reader may not
11+
* read drew the raw id beside a question mark, inside a link to a record page
12+
* that refuses them, instead of objectui#13335's "Restricted record".
13+
*
14+
* The object read gate answers the record GET with `403 PERMISSION_DENIED`,
15+
* not with the 404 a row hidden by sharing gets, and the ObjectStack adapter's
16+
* `findOne` turns only a 404 into `null`: the 403 is thrown. Measured live on
17+
* `pnpm dev:crm` (a member refused the target object), the thrown value is a
18+
* plain `Error` whose own members are `httpStatus: 403`, `code:
19+
* 'PERMISSION_DENIED'` and `details: { error, code, object }` — the shape
20+
* `MEASURED_REFUSAL` below copies. A missing id under that object answers the
21+
* same 403, so this face discloses nothing a not-found would not.
22+
*
23+
* A refusal of the reader's own read now draws the restricted face, as the
24+
* not-found does. It is read through `@object-ui/react`'s `classifyLoadError`
25+
* (`forbidden`: status 403 on any of the three status members, or the code),
26+
* so each spelling below gets the face. CONTROLS: a `null` answer (the 404)
27+
* is unchanged, and a 5xx, a network failure and a 401 keep the unresolved
28+
* face with their id — a failure that may pass is not a refusal.
29+
*
30+
* Every case uses its own id: the lookup cache is module-level.
31+
*/
32+
33+
import React from 'react';
34+
import { describe, it, expect, afterEach, vi } from 'vitest';
35+
import { render, cleanup, waitFor } from '@testing-library/react';
36+
import '@testing-library/jest-dom';
37+
import { SchemaRendererProvider, RelatedRecordActionsProvider } from '@object-ui/react';
38+
import { getCellRenderer } from '../index';
39+
40+
afterEach(() => cleanup());
41+
42+
const OBJECT = 'hr_payroll_29641';
43+
44+
/** The adapter's thrown value for an object-gate refusal, as measured live. */
45+
function measuredRefusal(): Error {
46+
const message =
47+
'You do not have permission to perform this action. Contact your administrator if you need access.';
48+
return Object.assign(new Error(message), {
49+
code: 'PERMISSION_DENIED',
50+
httpStatus: 403,
51+
details: { error: message, code: 'PERMISSION_DENIED', object: OBJECT },
52+
});
53+
}
54+
55+
/** A recording data source whose `findOne` throws `error` for every id. */
56+
function throwingDataSource(error: () => unknown) {
57+
return {
58+
findOne: vi.fn(async () => {
59+
throw error();
60+
}),
61+
find: vi.fn(async () => ({ data: [] })),
62+
getObjectSchema: vi.fn(async () => ({ name: OBJECT, nameField: 'title', fields: { title: { type: 'text' } } })),
63+
} as any;
64+
}
65+
66+
const hostLinks = {
67+
resolve: () => ({}),
68+
recordHref: (objectName: string, recordId: string | number) => `/o/${objectName}/r/${recordId}`,
69+
openRecord: vi.fn(),
70+
} as any;
71+
72+
function renderLookup(value: unknown, ds: any) {
73+
const Renderer = getCellRenderer('lookup');
74+
return render(
75+
<SchemaRendererProvider dataSource={ds}>
76+
<RelatedRecordActionsProvider value={hostLinks}>
77+
<Renderer value={value as any} field={{ type: 'lookup', name: 'payroll', reference: OBJECT } as any} />
78+
</RelatedRecordActionsProvider>
79+
</SchemaRendererProvider>,
80+
);
81+
}
82+
83+
const restricted = (root: HTMLElement) => root.querySelectorAll('[data-slot="restricted-reference"]');
84+
const unresolved = (root: HTMLElement) => root.querySelectorAll('[data-slot="unresolved-reference"]');
85+
86+
/** Lets the read and the render it causes settle. */
87+
async function settle(ds: any) {
88+
await waitFor(() => expect(ds.findOne).toHaveBeenCalled());
89+
await new Promise((r) => setTimeout(r, 20));
90+
}
91+
92+
describe('objectstack#29641 — the reader\'s read of the referenced record is REFUSED', () => {
93+
it('the measured 403 PERMISSION_DENIED draws "Restricted record", with no id, no link and one read', async () => {
94+
const id = 'PAY-REFUSED-MEASURED';
95+
const ds = throwingDataSource(measuredRefusal);
96+
const { container } = renderLookup(id, ds);
97+
98+
await waitFor(() => expect(restricted(container)).toHaveLength(1));
99+
expect(container.textContent).toBe('Restricted record');
100+
expect(container.innerHTML, 'the id is gone from text and from every attribute').not.toContain(id);
101+
expect(container.querySelector('a'), 'no link to a page that refuses the reader').toBeNull();
102+
expect(unresolved(container), 'not the unresolved face as well').toHaveLength(0);
103+
expect(ds.findOne).toHaveBeenCalledTimes(1);
104+
expect(ds.findOne).toHaveBeenCalledWith(OBJECT, id);
105+
// ⛔ No second record read around the refusal. (The object definition is
106+
// the cell's own `useRefObjectSchema` read, made before and after alike.)
107+
expect(ds.find).not.toHaveBeenCalled();
108+
});
109+
110+
it.each([
111+
['httpStatus 403 alone', () => Object.assign(new Error('Forbidden'), { httpStatus: 403 })],
112+
['status 403 alone', () => Object.assign(new Error('Forbidden'), { status: 403 })],
113+
['statusCode 403 alone', () => Object.assign(new Error('Forbidden'), { statusCode: 403 })],
114+
['code PERMISSION_DENIED alone', () => Object.assign(new Error('denied'), { code: 'PERMISSION_DENIED' })],
115+
])('%s is read as the same refusal', async (label, error) => {
116+
const id = `PAY-REFUSED-${label.replace(/\W+/g, '-')}`;
117+
const { container } = renderLookup(id, throwingDataSource(error));
118+
await waitFor(() => expect(restricted(container)).toHaveLength(1));
119+
expect(container.innerHTML).not.toContain(id);
120+
expect(container.querySelector('a')).toBeNull();
121+
});
122+
123+
it('the refusal is cached like a not-found, and a failure on ANOTHER id stays a failure', async () => {
124+
const refusedId = 'PAY-REFUSED-CACHED';
125+
const failedId = 'PAY-FAILED-AFTER-REFUSAL';
126+
let next: () => unknown = measuredRefusal;
127+
const ds = throwingDataSource(() => next());
128+
129+
const first = renderLookup(refusedId, ds);
130+
await waitFor(() => expect(restricted(first.container)).toHaveLength(1));
131+
first.unmount();
132+
133+
// A second cell for the same id draws restricted at once, without a read.
134+
const again = renderLookup(refusedId, ds);
135+
expect(restricted(again.container)).toHaveLength(1);
136+
expect(again.container.innerHTML).not.toContain(refusedId);
137+
expect(ds.findOne).toHaveBeenCalledTimes(1);
138+
again.unmount();
139+
140+
// A transient failure on another id after it keeps its own, unresolved answer.
141+
next = () => Object.assign(new Error('Bad Gateway'), { httpStatus: 502 });
142+
const other = renderLookup(failedId, ds);
143+
await waitFor(() => expect(ds.findOne).toHaveBeenCalledTimes(2));
144+
await new Promise((r) => setTimeout(r, 20));
145+
expect(restricted(other.container)).toHaveLength(0);
146+
expect(unresolved(other.container)).toHaveLength(1);
147+
expect(other.container.textContent).toBe(failedId);
148+
});
149+
150+
it('in a multi-value cell, only the first entry (the one read) is restricted', async () => {
151+
const first = 'PAY-REFUSED-ARR-1';
152+
const second = 'PAY-NEVER-ASKED-ARR-2';
153+
const { container } = renderLookup([first, second], throwingDataSource(measuredRefusal));
154+
155+
await waitFor(() => expect(restricted(container)).toHaveLength(1));
156+
expect(container.innerHTML).not.toContain(first);
157+
expect(restricted(container)[0]!.closest('a'), 'the restricted chip is not a link').toBeNull();
158+
// Entries after the first are never read, so nothing is known about them.
159+
expect(unresolved(container)).toHaveLength(1);
160+
expect(unresolved(container)[0]!.textContent).toBe(second);
161+
});
162+
});
163+
164+
describe('objectstack#29641 — CONTROLS: every other answer keeps its face', () => {
165+
it('a `null` answer (the adapter\'s 404) is still the restricted face, unchanged', async () => {
166+
const id = 'PAY-NOT-FOUND';
167+
const ds = { findOne: vi.fn(async () => null), find: vi.fn() } as any;
168+
const { container } = renderLookup(id, ds);
169+
await waitFor(() => expect(restricted(container)).toHaveLength(1));
170+
expect(container.innerHTML).not.toContain(id);
171+
expect(container.querySelector('a')).toBeNull();
172+
});
173+
174+
it.each([
175+
['a 500 with a server code', () => Object.assign(new Error('Internal error'), { httpStatus: 500, code: 'INTERNAL_ERROR' })],
176+
['a 503', () => Object.assign(new Error('Service Unavailable'), { status: 503 })],
177+
['a network failure (no HTTP answer)', () => new TypeError('Failed to fetch')],
178+
['a 401 (the session, not the record)', () => Object.assign(new Error('Unauthorized'), { httpStatus: 401, code: 'UNAUTHORIZED' })],
179+
])('%s keeps the unresolved face and its id', async (label, error) => {
180+
const id = `PAY-FAILED-${label.replace(/\W+/g, '-')}`;
181+
const ds = throwingDataSource(error);
182+
const { container } = renderLookup(id, ds);
183+
await settle(ds);
184+
expect(restricted(container)).toHaveLength(0);
185+
expect(unresolved(container)).toHaveLength(1);
186+
expect(container.textContent).toBe(id);
187+
});
188+
});

‎ui/packages/fields/src/index.tsx‎

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ import { valueSchemaFor, resolveFieldScale } from '@objectstack/spec/data';
1818
import { useLocalization, useDisplayLocale, formatDisplayNumber } from '@object-ui/i18n';
1919
import { Badge, Avatar, AvatarImage, AvatarFallback, Button, Checkbox, EmptyValue, cn, setFieldReadFaceResolver } from '@object-ui/components';
2020
import { Check, Copy, Phone as PhoneIcon, MapPin, CircleQuestionMark, Lock, Star } from 'lucide-react';
21-
import { useObjectTranslation } from '@object-ui/react';
21+
import { useObjectTranslation, classifyLoadError } from '@object-ui/react';
2222
import { SchemaRendererContext as _SchemaRendererContext } from '@object-ui/react';
2323
import { useRelatedRecordActions } from '@object-ui/react';
2424
import { usePermissions } from '@object-ui/permissions';
@@ -43,6 +43,8 @@ import { RecordHoverCard, type RecordHoverCardObjectDef } from './RecordHoverCar
4343
type LookupCacheEntry =
4444
| { state: 'pending'; promise: Promise<void> }
4545
| { state: 'ok'; record: Record<string, unknown> | null | undefined; schema: unknown }
46+
// The reader's own read was REFUSED (objectstack#29641): settled, not failed.
47+
| { state: 'refused' }
4648
| { state: 'err' };
4749
const lookupNameCache: Map<string, LookupCacheEntry> = new Map();
4850

@@ -244,8 +246,15 @@ export function isLikelyOpaqueId(v: unknown): boolean {
244246
* answers a record hidden from this reader and a record that does not exist
245247
* identically on purpose (existence non-disclosure, objectui#8631), so this
246248
* state is that fused answer and nothing finer.
247-
* - `undefined` — anything else: not asked, still in flight, the read threw,
248-
* or a record came back that no display field could name.
249+
* It is also the answer to a read the reader is REFUSED (objectstack#29641):
250+
* the target object's read gate answers the record GET with `403
251+
* PERMISSION_DENIED` (for an id that exists and for one that does not), which
252+
* the adapter throws rather than mapping to `null`. Read through the shared
253+
* `classifyLoadError` (`forbidden`), never from the message.
254+
* - `undefined` — anything else: not asked, still in flight, the read failed
255+
* for any other reason (network, 5xx, 401: it may pass on a retry, so it
256+
* says nothing about the record), or a record came back that no display
257+
* field could name.
249258
*/
250259
type LookupNameAnswer = { state: 'named'; name: string } | { state: 'absent' } | undefined;
251260

@@ -324,8 +333,13 @@ function useLookupName(
324333
// heuristic alone can surface an autonumber over the real name.
325334
const schema = await fetchRefObjectSchema(dataSource, referenceTo!);
326335
lookupNameCache.set(cacheKey, { state: 'ok', record, schema });
327-
} catch {
328-
lookupNameCache.set(cacheKey, { state: 'err' });
336+
} catch (err) {
337+
// A refusal is the reader's settled answer, like a not-found; any
338+
// other failure stays a failure (objectstack#29641).
339+
lookupNameCache.set(
340+
cacheKey,
341+
classifyLoadError(err) === 'forbidden' ? { state: 'refused' } : { state: 'err' },
342+
);
329343
}
330344
force((n) => n + 1);
331345
})();
@@ -335,6 +349,7 @@ function useLookupName(
335349

336350
if (!isResolvable) return undefined;
337351
const entry = lookupNameCache.get(cacheKey);
352+
if (entry?.state === 'refused') return { state: 'absent' };
338353
if (entry?.state !== 'ok') return undefined;
339354
if (entry.record == null) return { state: 'absent' };
340355
const name = resolveLookupRecordName(entry.record, entry.schema, displayField, policy, referenceTo);
@@ -2727,7 +2742,7 @@ export function LookupCellRenderer({ value, field }: CellRendererProps): React.R
27272742
const lookupAnswer = useLookupName(referenceTo, primaryPrimitiveId, displayField, perms);
27282743
const resolvedName = lookupAnswer?.state === 'named' ? lookupAnswer.name : undefined;
27292744
// The reader's own read of the first primitive answered "no record"
2730-
// (objectui#13335). Only that id gets the restricted face: in flight, a
2745+
// (objectui#13335) or was refused (objectstack#29641). Only that id gets the restricted face: in flight, a
27312746
// failed read, an unnamed record and the entries never asked keep the
27322747
// unresolved one below.
27332748
const restrictedId = lookupAnswer?.state === 'absent' ? primaryPrimitiveId : undefined;

0 commit comments

Comments
 (0)