fix(fields): FLS-filter the lookup dropdown's and record picker's drawn columns; gate PeoplePicker's $expand (#10373)

.changeset/10223-lookup-candidates-expand.md

@@ -22,9 +22,11 @@ Field-level security gates that list the way it gates the other
 `buildExpandFields` call sites: once the permission policy has loaded, a
 reference column the user may not read on the referenced object is left out of
 `$expand`; before the policy loads, nothing is filtered. `@object-ui/fields`
-now depends on `@object-ui/permissions` for that check. A column left out of
-`$expand`, like any column from a backend that ignores the parameter, still
-arrives as a bare id and is resolved one by one as before.
+now depends on `@object-ui/permissions` for that check. A reference column
+from a backend that ignores the parameter still arrives as a bare id and is
+resolved one by one as before. A column the policy denies is not drawn at all,
+and a field it denies is not shown in an option's label or in the picker's
+title column (objectui#10373).
 
 `buildExpandFields` covers `user` columns as well as `lookup`,
 `master_detail` and `tree`. A previewed `user` column therefore now shows the

.changeset/10373-fields-display-fls.md

@@ -0,0 +1,48 @@
+---
+'@object-ui/fields': patch
+---
+
+fix(fields): the lookup dropdown and record picker stop showing fields field-level security denies, and PeoplePicker's `$expand` is gated
+
+A lookup field's dropdown and its browse-all picker (`RecordPickerDialog`)
+filtered their `$expand` through field-level security, but not what they
+draw. Once the permission policy had loaded, a column the user may not read
+on the referenced object was still previewed under each dropdown candidate,
+and still headed and rendered in every picker row. A denied relation column,
+left out of `$expand`, arrived as a bare id, and the lookup cell renderer then
+fetched each related record on its own. A denied display field still labelled
+every option, and a `titleFormat` template still printed every field it
+named.
+
+Both now treat those fields the way `RelatedList` treats its columns: once the
+policy has loaded, a column the user may not read on the referenced object is
+not previewed in the dropdown, and is neither headed nor rendered in the
+picker. The picker's display column is gated like any other column. Its id
+column never is, and when the policy leaves no other column to draw, the
+picker draws the id column so that every row can still be told apart and
+chosen. A `renderGrid` slot on the picker receives the same columns. Before
+the policy loads, nothing is filtered, and the columns are re-derived when it
+arrives.
+
+An option's label is built from the row with the denied fields removed,
+the same row ObjectStack's `FieldMasker` already serves, so on that backend
+labels do not change. A denied display field therefore falls through to the
+next source of a label, ending at the record id, and a `titleFormat` template,
+in the dropdown's labels and in the picker's display column, leaves a denied
+field's slot empty. The label is a display value only: the committed value is
+unchanged, the records the picker hands to `onSelectRecords` are unchanged,
+and the option the dropdown hands to `onSelectRecord` still carries the served
+row's other fields beside its label.
+
+`PeoplePicker`, which a lookup opens when its field sets `picker: 'search'`,
+derives its `$expand` from dotted `subtitle` paths such as
+`primary_business_unit_id.name` when no `expand` is passed, and that list had
+no permission check. Once the policy has loaded, a relation the user may not
+read on the object the picker queries is now left out of `$expand`, whether
+the list was derived from the subtitle paths or passed as `expand`. A subtitle
+segment that goes through a relation left out shows nothing, as it does when a
+backend ignores `$expand`.
+
+This is defence in depth: ObjectStack's `FieldMasker` already removes the
+fields a user may not read from the rows it returns. The change matters for a
+backend that does not.

.changeset/lookup-dropdown-cell-renderer-5492.md

@@ -33,9 +33,12 @@ at the time neither surface's request carried populate: the picker resolved a
 foreign-key id to a name client-side, in the lookup cell renderer, and the
 dropdown inherited exactly that. A later change (objectui#10223) has both
 requests ask for `$expand` on the reference columns they display, minus any
-the loaded permission policy denies; a value that still arrives as a bare id
-is resolved by that same cell renderer. An unresolved reference therefore renders
-what the picker renders for it, and keeps its column: a slot is dropped only
-when the record holds no value for the field, decided on the raw value and
-never on what the renderer makes of it, so an unresolved id can never degrade
-into a silently empty column.
+the loaded permission policy denies, and another (objectui#10373) stops both
+surfaces from drawing a column that policy denies or showing a field it denies
+in a record's title. A readable value that still arrives as a bare id is
+resolved by that same cell renderer. An unresolved reference therefore renders
+what the picker renders for it, and keeps its column: among the columns the
+policy lets the user read, a slot is dropped only when the record holds no
+value for the field, decided on the raw value and never on what the renderer
+makes of it, so an unresolved id can never degrade into a silently empty
+column.

packages/fields/src/widgets/LookupField.displayFls-10373.test.tsx

@@ -0,0 +1,285 @@
+/**
+ * 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.
+ */
+
+/**
+ * The lookup dropdown previews only the columns the user may read —
+ * objectui#10373.
+ *
+ * Under the renderer-side FLS rulings (objectui#7215 / objectui#7230, applied
+ * by the objectui#7429 sweep) field-level security gates the OUTPUT.
+ * `RelatedList` gates both its `$expand` and the columns it draws
+ * (`keepReadableColumns`); the lookup dropdown gated only its `$expand`
+ * (objectui#10223), so a column the policy denies was still previewed under
+ * every candidate, and a denied RELATION column, left out of `$expand`, arrived
+ * as a bare key that the lookup cell renderer then resolved with a read of its
+ * own.
+ *
+ * What is pinned, against the real `PermissionProvider` (not a stub):
+ *
+ *  - a preview column the loaded policy denies is not rendered, and a denied
+ *    relation column is neither rendered nor resolved row by row;
+ *  - the readable preview columns still render (the pin reads a FILTER, not a
+ *    preview that vanished);
+ *  - with no policy loaded (no provider: `isLoaded` is false) nothing is
+ *    filtered;
+ *  - the option label is a display value, built from the row with the denied
+ *    fields removed: on a backend that does NOT strip, a denied display field
+ *    yields exactly the label a stripping backend (ObjectStack's
+ *    `FieldMasker`) yields, no option is dropped, and choosing one still
+ *    commits its id; the record `onSelectRecord` receives keeps its shape;
+ *  - a `titleFormat` naming a denied field does not render it in the label.
+ */
+
+import * as React from 'react';
+import { describe, it, expect, vi, afterEach } from 'vitest';
+import { render, screen, cleanup, waitFor, act, fireEvent } from '@testing-library/react';
+import '@testing-library/jest-dom';
+import { SchemaRendererContext } from '@object-ui/react';
+import { PermissionProvider } from '@object-ui/permissions';
+import { LookupField } from './LookupField';
+// Registers the cell-renderer bridge the preview renders through.
+import '../index';
+
+const CANDIDATES = 5;
+
+const ACCOUNT_FIELDS: Record<string, any> = {
+  name: { type: 'text', label: 'Name' },
+  code: { type: 'text', label: 'Code' },
+  secret: { type: 'text', label: 'Secret' },
+  region: { type: 'lookup', label: 'Region', reference_to: 'region' },
+};
+
+interface BackendOptions {
+  /**
+   * Fields the backend removes from every row it serves, as ObjectStack's
+   * `FieldMasker` does for the fields a policy denies. Default: none — the
+   * non-stripping backend this gate defends against.
+   */
+  strip?: string[];
+  titleFormat?: string;
+}
+
+/** A backend that honours `$expand`, and strips only the fields it is told to. */
+function makeBackend(prefix: string, { strip = [], titleFormat }: BackendOptions = {}) {
+  const regions: Record<string, { id: string; name: string }> = {};
+  const rows: Record<string, any>[] = [];
+  for (let i = 0; i < CANDIDATES; i++) {
+    const regionId = `${prefix}_region_${i}`;
+    regions[regionId] = { id: regionId, name: `Region ${i}` };
+    rows.push({ id: `${prefix}_acct_${i}`, name: `Account ${i}`, code: `C-${i}`, secret: `S-${i}`, region: regionId });
+  }
+  const find = vi.fn(async (objectName: string, params?: Record<string, any>) => {
+    if (objectName !== 'account') return { data: [], total: 0 };
+    const expand: string[] = Array.isArray(params?.$expand) ? params!.$expand : [];
+    const data = rows.map((r) => {
+      const row = { ...r };
+      if (expand.includes('region')) row.region = regions[r.region];
+      for (const f of strip) delete row[f];
+      return row;
+    });
+    return { data, total: rows.length };
+  });
+  const findOne = vi.fn(async (objectName: string, id: string) =>
+    objectName === 'region' ? regions[id] ?? null : null,
+  );
+  const getObjectSchema = vi.fn(async (objectName: string) => {
+    if (objectName === 'account') {
+      return {
+        name: 'account',
+        fields: ACCOUNT_FIELDS,
+        highlightFields: ['code', 'secret', 'region'],
+        ...(titleFormat ? { titleFormat } : {}),
+      };
+    }
+    if (objectName === 'region') {
+      return { name: 'region', fields: { name: { type: 'text', label: 'Name' } } };
+    }
+    return undefined;
+  });
+  return { find, findOne, getObjectSchema } as any;
+}
+
+type Backend = ReturnType<typeof makeBackend>;
+
+/** Reads that are not the candidate query: per-row resolution of `region`. */
+function perRowReads(ds: Backend): number {
+  const finds = ds.find.mock.calls.filter(([objectName]: [string]) => objectName !== 'account').length;
+  return finds + ds.findOne.mock.calls.length;
+}
+
+type Wrap = (node: React.ReactElement) => React.ReactElement;
+const bare: Wrap = (node) => node;
+
+/** The real role-based provider, denying the named `account` fields to `viewer`. */
+function denying(...fields: string[]): Wrap {
+  return (node) => (
+    <PermissionProvider
+      roles={[]}
+      userRoles={['viewer']}
+      permissions={[
+        {
+          object: 'account',
+          roles: {
+            viewer: { actions: ['read'], fieldPermissions: fields.map((field) => ({ field, read: false })) },
+          },
+        },
+      ]}
+    >
+      {node}
+    </PermissionProvider>
+  );
+}
+
+async function settle(): Promise<void> {
+  for (let i = 0; i < 6; i++) {
+    await act(async () => {
+      await new Promise((r) => setTimeout(r, 0));
+    });
+  }
+}
+
+async function openDropdown(
+  ds: Backend,
+  wrap: Wrap,
+  onChange: (v: unknown) => void = () => {},
+  extra: Record<string, unknown> = {},
+): Promise<void> {
+  render(
+    wrap(
+      <SchemaRendererContext.Provider value={{ dataSource: ds } as any}>
+        <LookupField
+          value={undefined}
+          onChange={onChange}
+          dataSource={ds}
+          field={{ reference_to: 'account' } as never}
+          {...(extra as object)}
+        />
+      </SchemaRendererContext.Provider>,
+    ),
+  );
+  await waitFor(() => expect(ds.getObjectSchema).toHaveBeenCalledWith('account'));
+  await settle();
+  await act(async () => {
+    fireEvent.click(screen.getByTestId('lookup-trigger'));
+  });
+  await waitFor(() => expect(screen.getAllByRole('option')).toHaveLength(CANDIDATES));
+  await settle();
+}
+
+/** Each listed option's label, as its row shows it. */
+function optionLabels(): string[] {
+  return screen.getAllByRole('option').map((el) => el.querySelector('span.block')?.textContent ?? '');
+}
+
+function previews(field: string): string[] {
+  return Array.from(document.querySelectorAll(`[data-lookup-preview="${field}"]`)).map(
+    (el) => el.textContent ?? '',
+  );
+}
+
+afterEach(() => {
+  cleanup();
+  try {
+    localStorage.clear();
+  } catch {
+    /* no storage in this environment */
+  }
+});
+
+describe('LookupField — the dropdown previews only readable columns (objectui#10373)', () => {
+  it('a preview column the loaded policy denies is not rendered; the readable one still is', async () => {
+    const ds = makeBackend('deny');
+    await openDropdown(ds, denying('secret'));
+
+    expect(previews('secret')).toEqual([]);
+    expect(previews('code')).toHaveLength(CANDIDATES);
+    expect(previews('code')[0]).toBe('C-0');
+    // The readable relation is still previewed, expanded, with no per-row read.
+    expect(previews('region')[0]).toBe('Region 0');
+  });
+
+  it('a denied relation column is neither rendered nor resolved row by row', async () => {
+    const ds = makeBackend('rel');
+    await openDropdown(ds, denying('region'));
+
+    const candidateQuery = ds.find.mock.calls.find(([o]: [string]) => o === 'account')![1];
+    expect(candidateQuery.$expand).toBeUndefined();
+    expect(previews('region')).toEqual([]);
+    expect(perRowReads(ds)).toBe(0);
+    expect(previews('secret')).toHaveLength(CANDIDATES);
+  });
+
+  it('control: with no policy loaded (no provider) nothing is filtered', async () => {
+    const ds = makeBackend('none');
+    await openDropdown(ds, bare);
+
+    expect(previews('secret')).toHaveLength(CANDIDATES);
+    expect(previews('code')).toHaveLength(CANDIDATES);
+    expect(previews('region')[0]).toBe('Region 0');
+  });
+
+  it('a denied display field: the label a stripping backend yields, no option dropped, the id committed', async () => {
+    // What a stripping backend renders under the same policy — the reference.
+    await openDropdown(makeBackend('title', { strip: ['name'] }), denying('name'));
+    const stripped = optionLabels();
+    cleanup();
+
+    const ds = makeBackend('title');
+    const onChange = vi.fn();
+    await openDropdown(ds, denying('name'), onChange);
+
+    const labels = optionLabels();
+    expect(labels).toHaveLength(CANDIDATES);
+    expect(labels).toEqual(stripped);
+    expect(labels.join(' ')).not.toContain('Account');
+    await act(async () => {
+      fireEvent.click(screen.getAllByRole('option')[2]);
+    });
+    expect(onChange).toHaveBeenCalledWith('title_acct_2');
+  });
+
+  it('control: with no policy loaded, the display field labels the option', async () => {
+    await openDropdown(makeBackend('titlenone'), bare);
+    expect(optionLabels()[0]).toBe('Account 0');
+  });
+
+  it('a denied display field changes the label only: the record `onSelectRecord` receives keeps its shape', async () => {
+    const pick = async (wrap: Wrap): Promise<Record<string, unknown>> => {
+      const onSelectRecord = vi.fn();
+      await openDropdown(makeBackend('payload'), wrap, () => {}, { onSelectRecord });
+      await act(async () => {
+        fireEvent.click(screen.getAllByRole('option')[1]);
+      });
+      expect(onSelectRecord).toHaveBeenCalledTimes(1);
+      const payload = onSelectRecord.mock.calls[0][0] as Record<string, unknown>;
+      cleanup();
+      // The pick is remembered as "recently used"; the second pick must see
+      // the same list the first one did.
+      localStorage.clear();
+      return payload;
+    };
+    const open = await pick(bare);
+    const gated = await pick(denying('name'));
+
+    expect(gated.value).toBe('payload_acct_1');
+    expect(Object.keys(gated).sort()).toEqual(Object.keys(open).sort());
+    expect({ ...gated, label: undefined }).toEqual({ ...open, label: undefined });
+    expect(gated.label).not.toBe(open.label);
+  });
+
+  it('a `titleFormat` naming a denied field does not render it in the label', async () => {
+    await openDropdown(makeBackend('tf', { titleFormat: '{name} - {secret}' }), denying('secret'));
+    expect(optionLabels()[0]).toBe('Account 0');
+    expect(optionLabels().join(' ')).not.toContain('S-');
+  });
+
+  it('control: with no policy loaded, the same `titleFormat` renders every field it names', async () => {
+    await openDropdown(makeBackend('tfnone', { titleFormat: '{name} - {secret}' }), bare);
+    expect(optionLabels()[0]).toBe('Account 0 - S-0');
+  });
+});