From 6cd39a549783dcd8e5d0f9ddb128c398f7dc02e2 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 19 Oct 2026 22:28:18 +0000 Subject: [PATCH] fix(app-shell): ask the RLS rule about every clause that parses, not only an unlowerable one (objectui#13082) Studio's RLS editor asked `@objectstack/lint`'s `validateRlsPredicateEnforceability` only for a clause `isSupportedRlsExpression` refuses. A lowerable predicate the rule refuses on what it reads, such as `current_user.nope == owner_id` (`rls-predicate-unknown-user-variable`), linted clean and autosaved. `rlsPredicateVerdict` now asks the rule for every USING / CHECK clause that parses, on the same one-clause stack, and carries each finding's severity and message as written. The verdicts that read the clause alone reach the editor; the ones that read the object's field map, its field types or the host engine stay at publish, for the reasons the docblock gives. Claude-Session: https://claude.ai/code/session_014MXhk1yjWh37d634hLy3p7 Co-authored-by: Claude --- .changeset/13082-rls-reference-passes.md | 9 ++ packages/app-shell/README.md | 12 +- .../metadata-admin/CelPredicateField.tsx | 8 +- .../PermissionAdvancedFacets.cel.test.tsx | 30 ++++ ...Authoring.rlsReferencePasses-13082.test.ts | 151 ++++++++++++++++++ .../src/views/metadata-admin/celAuthoring.ts | 107 ++++++++----- 6 files changed, 269 insertions(+), 48 deletions(-) create mode 100644 .changeset/13082-rls-reference-passes.md create mode 100644 packages/app-shell/src/views/metadata-admin/celAuthoring.rlsReferencePasses-13082.test.ts diff --git a/.changeset/13082-rls-reference-passes.md b/.changeset/13082-rls-reference-passes.md new file mode 100644 index 0000000000..abe909c090 --- /dev/null +++ b/.changeset/13082-rls-reference-passes.md @@ -0,0 +1,9 @@ +--- +'@object-ui/app-shell': patch +--- + +Studio's RLS policy editor now asks `@objectstack/lint`'s RLS rule about every `USING` or `CHECK` clause that parses, not only about one the runtime cannot lower (objectui#13082). A predicate that lowers but reads something the rule refuses, such as `current_user.nope == owner_id`, was silent before: the draft autosaved, and publish then refused it as `rls-predicate-unknown-user-variable`. It is now the rule's error, in the rule's words, and it holds Save and autosave. + +The same holds for the rule's other verdicts that read the clause alone: a `current_user` value of the wrong type for its position (`owner_id == current_user.org_user_ids`) and a refused `null` comparand (`status in ['a', null]`), both `rls-predicate-unenforceable`. A valid predicate such as `owner_id == current_user.id` stays clean. + +The verdicts that read more than the clause stay at publish: an unknown field (the editor's field list is read from the published object and misses fields in a pending object draft, so a near miss stays a non-blocking "did you mean" warning), a comparison between fields of incompatible types (the editor holds no field types), and the server engine's own filter check. diff --git a/packages/app-shell/README.md b/packages/app-shell/README.md index 2054221ff7..b9349ffbd6 100644 --- a/packages/app-shell/README.md +++ b/packages/app-shell/README.md @@ -722,10 +722,14 @@ server** instead of maintaining a second grammar: - **Inline lint** (`CelPredicateField`) — `validateExpression` flags parse faults inline (blocking Save) and unknown-field near-misses as non-blocking - "did-you-mean" warnings; a `USING` or `CHECK` predicate the runtime cannot - lower (`isSupportedRlsExpression`) is an error that blocks Save, carrying - `@objectstack/lint`'s `rls-predicate-*` message as written — the verdict - publish and `os build` give. + "did-you-mean" warnings; a `USING` or `CHECK` predicate `@objectstack/lint`'s + `rls-predicate-*` rule refuses is an error that blocks Save, carrying the + rule's message as written — the verdict publish and `os build` give. The + editor asks the rule about the clause alone: a predicate the runtime cannot + lower, and, for one that lowers, a `current_user` member nothing resolves, a + `current_user` value of the wrong type for its position or a refused `null` + comparand. The verdicts that read the object's field map or field types, or + the server's engine, stay at publish. - **Field autocomplete** — `introspectScope` supplies the target object's fields plus scope vars (`current_user`, `record`, …) and stdlib functions as you type, so an identifier that would silently never match is caught early. diff --git a/packages/app-shell/src/views/metadata-admin/CelPredicateField.tsx b/packages/app-shell/src/views/metadata-admin/CelPredicateField.tsx index ce39140161..a3963e09ad 100644 --- a/packages/app-shell/src/views/metadata-admin/CelPredicateField.tsx +++ b/packages/app-shell/src/views/metadata-admin/CelPredicateField.tsx @@ -16,9 +16,9 @@ * author-time safeties, all backed by the framework's canonical CEL engine * (see {@link file://./celAuthoring.ts}): * - * - inline LINT — parse faults and, on an RLS clause, a predicate the - * runtime cannot lower (both blocking), and typo / wrong-layer advisories, - * surfaced under the field as you type (debounced); + * - inline LINT — parse faults and, on an RLS clause, a predicate + * `@objectstack/lint`'s RLS rule refuses (both blocking), and typo / + * wrong-layer advisories, surfaced under the field as you type (debounced); * - as-you-type AUTOCOMPLETE — the target object's fields, the scope roots * (`current_user`, `record`, …) and the stdlib functions, so a mistyped * identifier is caught before it silently never matches; @@ -82,7 +82,7 @@ export interface CelPredicateFieldProps { objectName?: string; /** Known field names of {@link objectName}. */ fieldNames?: string[]; - /** Which RLS clause this is (turns on the blocking lowerability verdict). */ + /** Which RLS clause this is (turns on the blocking verdicts of `@objectstack/lint`'s RLS rule). */ clause?: 'using' | 'check'; /** * Evaluation scope of the authoring site (objectui#1582). `'flattened'` diff --git a/packages/app-shell/src/views/metadata-admin/PermissionAdvancedFacets.cel.test.tsx b/packages/app-shell/src/views/metadata-admin/PermissionAdvancedFacets.cel.test.tsx index 3ea37330ac..93499f3306 100644 --- a/packages/app-shell/src/views/metadata-admin/PermissionAdvancedFacets.cel.test.tsx +++ b/packages/app-shell/src/views/metadata-admin/PermissionAdvancedFacets.cel.test.tsx @@ -69,6 +69,36 @@ describe('PermissionAdvancedFacets · CEL authoring safety (objectui#2413)', () expect(onCelErrorsChange).toHaveBeenLastCalledWith(0); }); + // objectui#13082 — a clause that lowers but reads something the rule refuses + // holds Save the same way: the card's predicate in USING, in CHECK and in + // both, and its control. + const ownerPolicy = (using: string, check: string) => ({ + draft: { rowLevelSecurity: [{ name: 'p1', object: 'account', operation: 'all', using, check, enabled: true }] }, + loadObjectFields: async () => ['organization_id', 'owner_id'], + }); + for (const [label, using, check, expected] of [ + ['an unknown `current_user` variable in USING', 'current_user.nope == owner_id', '', 1], + ['an unknown `current_user` variable in CHECK', 'owner_id == current_user.id', 'current_user.nope == owner_id', 1], + ['an unknown `current_user` variable in both clauses', 'current_user.nope == owner_id', 'current_user.nope == owner_id', 2], + ] as const) { + it(`reports ${label} up as blocking (objectui#13082)`, async () => { + const onCelErrorsChange = vi.fn(); + const user = userEvent.setup(); + renderFacets({ ...ownerPolicy(using, check), onCelErrorsChange }); + await user.click(screen.getByText('perm.rls.title')); + await waitFor(() => expect(onCelErrorsChange).toHaveBeenLastCalledWith(expected), { timeout: 3000 }); + }); + } + + it('CONTROL — `owner_id == current_user.id` on both clauses leaves Save open (objectui#13082)', async () => { + const onCelErrorsChange = vi.fn(); + const user = userEvent.setup(); + renderFacets({ ...ownerPolicy('owner_id == current_user.id', 'owner_id == current_user.id'), onCelErrorsChange }); + await user.click(screen.getByText('perm.rls.title')); + await waitFor(() => expect(screen.getAllByText('perm.cel.valid')).toHaveLength(2), { timeout: 3000 }); + expect(onCelErrorsChange).toHaveBeenLastCalledWith(0); + }); + it('opens the test-run dialog for a policy', async () => { const user = userEvent.setup(); renderFacets(); diff --git a/packages/app-shell/src/views/metadata-admin/celAuthoring.rlsReferencePasses-13082.test.ts b/packages/app-shell/src/views/metadata-admin/celAuthoring.rlsReferencePasses-13082.test.ts new file mode 100644 index 0000000000..68cfdfefd4 --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/celAuthoring.rlsReferencePasses-13082.test.ts @@ -0,0 +1,151 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * objectui#13082 — the RLS editor asks `@objectstack/lint`'s + * `validateRlsPredicateEnforceability` for EVERY clause that parses, not only + * for one the runtime cannot lower, so a lowerable predicate the publish door + * refuses on what it POINTS AT is refused here too: at the rule's severity, in + * the rule's words. + * + * Every expected finding below is read from the published rule at call time + * (a one-clause stack, the same input the editor builds); no message is written + * in this file. Which passes the editor carries and which stay at the publish + * door, and why, is `rlsPredicateVerdict`'s docblock in `celAuthoring.ts`. + */ + +import { describe, it, expect } from 'vitest'; +import { lintCelPredicate } from './celAuthoring'; +import { isSupportedRlsExpression } from '@objectstack/formula'; +import { + validateRlsPredicateEnforceability, + RLS_PREDICATE_UNENFORCEABLE, + RLS_PREDICATE_UNKNOWN_FIELD, + RLS_PREDICATE_UNKNOWN_USER_VARIABLE, +} from '@objectstack/lint'; + +const HINT = { objectName: 'account', fields: ['organization_id', 'owner_id', 'status', 'amount'] }; +const CLAUSES = ['using', 'check'] as const; +type Clause = (typeof CLAUSES)[number]; + +/** The rule's verdict on the editor's input: one clause, no object. */ +const ruleOn = (clause: Clause, source: string) => + validateRlsPredicateEnforceability({ permissions: [{ rowLevelSecurity: [{ [clause]: source }] }] }); + +describe('celAuthoring · the RLS rule judges a lowerable clause too (objectui#13082)', () => { + // The card's predicate: it lowers, so the shape pass has nothing to say, + // and the reference pass refuses it. + for (const clause of CLAUSES) { + it(`rates ${clause.toUpperCase()} \`current_user.nope == owner_id\` the rule's unknown-user-variable error`, async () => { + const source = 'current_user.nope == owner_id'; + expect(isSupportedRlsExpression(source)).toBe(true); + const findings = ruleOn(clause, source); + expect(findings.map((f) => [f.rule, f.severity])).toEqual([[RLS_PREDICATE_UNKNOWN_USER_VARIABLE, 'error']]); + expect(await lintCelPredicate(source, { ...HINT, clause })).toEqual([ + { severity: 'error', message: findings[0].message }, + ]); + }); + + it(`CONTROL — ${clause.toUpperCase()} \`owner_id == current_user.id\` stays clean`, async () => { + expect(ruleOn(clause, 'owner_id == current_user.id')).toEqual([]); + expect(await lintCelPredicate('owner_id == current_user.id', { ...HINT, clause })).toEqual([]); + }); + } + + // The rule's other verdicts on a lowerable clause that read the clause and + // nothing else: a `current_user` value of the wrong type for its position, + // and a comparand the shared filter faces refuse. + for (const [label, source] of [ + ['a membership set where one value belongs', 'owner_id == current_user.org_user_ids'], + ['one value where a set belongs', 'owner_id in current_user.id'], + ['the whole caller context where a set belongs', 'status in current_user'], + ['a null list member', 'status in [null]'], + ] as const) { + for (const clause of CLAUSES) { + it(`rates ${clause.toUpperCase()} with ${label} the rule's unenforceable error`, async () => { + expect(isSupportedRlsExpression(source)).toBe(true); + const findings = ruleOn(clause, source); + expect(findings.map((f) => [f.rule, f.severity])).toEqual([[RLS_PREDICATE_UNENFORCEABLE, 'error']]); + expect(await lintCelPredicate(source, { ...HINT, clause })).toEqual([ + { severity: 'error', message: findings[0].message }, + ]); + }); + } + } + + // The same faces refuse a `null` beside a typed member, or a `null` + // ordering bound; the engine's type check refuses those first, as a + // blocking parse fault, and the rule is asked only once the predicate parses. + for (const source of ["status in ['a', null]", 'amount > null']) { + it(`\`${source}\` is already the engine's blocking error, and only that one`, async () => { + expect(isSupportedRlsExpression(source)).toBe(true); + expect(ruleOn('using', source).map((f) => f.rule)).toEqual([RLS_PREDICATE_UNENFORCEABLE]); + const issues = await lintCelPredicate(source, { ...HINT, clause: 'using' }); + const errors = issues.filter((i) => i.severity === 'error'); + expect(errors).toHaveLength(1); + expect(errors[0].message).not.toBe(ruleOn('using', source)[0].message); + }); + } + + it("keeps the rule's silence on an app-staged membership key (`owner_id in current_user.team_ids`)", async () => { + for (const clause of CLAUSES) { + expect(ruleOn(clause, 'owner_id in current_user.team_ids')).toEqual([]); + expect(await lintCelPredicate('owner_id in current_user.team_ids', { ...HINT, clause })).toEqual([]); + } + }); + + it('leaves the shape verdict of #13061 as it was: a clause that cannot lower carries the shape error alone', async () => { + const source = 'upper(status) == "OPEN"'; + expect(isSupportedRlsExpression(source)).toBe(false); + for (const clause of CLAUSES) { + const findings = ruleOn(clause, source); + expect(findings).toHaveLength(1); + expect(await lintCelPredicate(source, { ...HINT, clause })).toEqual([ + { severity: 'error', message: findings[0].message }, + ]); + } + }); +}); + +describe('celAuthoring · the passes that stay at the publish door give no editor error (objectui#13082)', () => { + /** The rule's verdict with the object declared, as the publish door's universe holds it. */ + const ruleWithObject = (source: string, fields: Record) => + validateRlsPredicateEnforceability({ + objects: [{ name: 'account', fields }], + permissions: [{ rowLevelSecurity: [{ object: 'account', operation: 'all', using: source }] }], + }); + + // The editor's field list is a name list read from the PUBLISHED object; a + // field the author added in a pending object draft is not on it, and the + // package publish folds that draft in. An unknown field therefore gets at + // most the engine's non-blocking near-miss advisory here, never the rule's + // error. + it('a field missing from the editor list holds no Save, though the rule refuses it against that list', async () => { + const source = "region == 'EU'"; + const listed = Object.fromEntries(HINT.fields.map((name) => [name, {}])); + expect(ruleWithObject(source, listed).map((f) => f.rule)).toEqual([RLS_PREDICATE_UNKNOWN_FIELD]); + expect(ruleWithObject(source, { ...listed, region: { type: 'text' } })).toEqual([]); + const issues = await lintCelPredicate(source, { ...HINT, clause: 'using' }); + expect(issues.filter((i) => i.severity === 'error')).toEqual([]); + }); + + it('absent field metadata gives no error', async () => { + for (const fields of [undefined, []]) { + for (const clause of CLAUSES) { + const issues = await lintCelPredicate('nope_field == current_user.id', { objectName: 'account', fields, clause }); + expect(issues.filter((i) => i.severity === 'error')).toEqual([]); + } + } + }); + + // Field TYPES decide these two arms; the editor holds names only. + for (const [label, source, fields] of [ + ['a text field compared with a number field', 'status != amount', { status: { type: 'text' }, amount: { type: 'number' } }], + ['a field compared with a list-holding field', 'status != tags', { status: { type: 'text' }, tags: { type: 'json' } }], + ] as const) { + it(`${label}: refused by the rule over declared types, no error in the editor`, async () => { + expect(ruleWithObject(source, fields).map((f) => f.rule)).toEqual([RLS_PREDICATE_UNENFORCEABLE]); + const issues = await lintCelPredicate(source, { objectName: 'account', fields: Object.keys(fields), clause: 'using' }); + expect(issues.filter((i) => i.severity === 'error')).toEqual([]); + }); + } +}); diff --git a/packages/app-shell/src/views/metadata-admin/celAuthoring.ts b/packages/app-shell/src/views/metadata-admin/celAuthoring.ts index 1c4e132a78..f9df94a897 100644 --- a/packages/app-shell/src/views/metadata-admin/celAuthoring.ts +++ b/packages/app-shell/src/views/metadata-admin/celAuthoring.ts @@ -44,9 +44,9 @@ export interface CelSchemaHint { fields?: string[]; /** * Which RLS clause is being edited: `USING` (read filter) or `CHECK` (write - * filter). Naming one turns on the lowerability verdict — a clause the - * runtime cannot lower to a row filter is an `error` (see - * {@link lintCelPredicate}). Omitted on every non-RLS surface. + * filter). Naming one turns on `@objectstack/lint`'s RLS rule — a clause it + * refuses, lowerable or not, is an `error` (see {@link lintCelPredicate}). + * Omitted on every non-RLS surface. */ clause?: 'using' | 'check'; /** @@ -108,8 +108,8 @@ export type CelLintMessageKey = 'engine.celLint.notTheRow'; /** * A single lint finding surfaced inline under the editor. `error` blocks save - * (a parse fault, or an RLS clause the runtime would drop); `warning` is - * advisory (a typo, a wrong-layer root). + * (a parse fault, or an RLS clause `@objectstack/lint`'s rule refuses); + * `warning` is advisory (a typo, a wrong-layer root). * * Every error, and every advisory the engine or `@objectstack/lint` words, * carries that producer's `message` as written. An advisory this module words @@ -192,8 +192,6 @@ interface FormulaModule { input: unknown, schema?: unknown, ) => 'number' | 'text' | 'boolean' | 'date' | 'unknown'; - /** The RLS shape gate: `false` = the runtime cannot lower this clause and drops the policy. */ - isSupportedRlsExpression?: (expression: string) => boolean; ExpressionEngine?: { evaluate: ( expr: { dialect: string; source: string }, @@ -227,7 +225,7 @@ function loadFormula(): Promise { return cached; } -/* ── Lazy RLS lowerability verdict (objectui#13061) ──────────────────── */ +/* ── Lazy RLS rule verdict (objectui#13061, objectui#13082) ──────────── */ /** One finding of `@objectstack/lint`'s published RLS rule — the fields read here. */ interface RlsPredicateFinding { @@ -260,33 +258,61 @@ function loadRlsRule(): Promise { } /** - * The server's verdict on an RLS clause the runtime cannot lower, in the - * server's words (objectui#13061). - * - * The verdict is the runtime's. The RLS compiler drops a clause it cannot lower - * to a row filter, and the policy then grants (or admits) nothing — it fails - * CLOSED. `isSupportedRlsExpression` (`@objectstack/formula`) is the decision - * the compiler makes, and the one `@objectstack/lint`'s rule calls at severity - * `error`; asking the same function here means Studio and the server cannot - * disagree about which clauses are dropped. - * - * The words are the lint's. ⛔ No sentence for this verdict is written here: - * the finding is the one the published rule returns for a one-clause stack - * holding this source, with its severity and message as written. For a clause - * the gate refuses, the rule reports through its shape pass alone. - * - * Only the gate's question — will this clause lower at all. The rule's other - * passes judge what a lowerable clause POINTS AT and need the object's - * metadata; they stay the publish door's. An engine without the gate, or a - * lint without the rule, gives no verdict here, the same degradation as every + * `@objectstack/lint`'s verdicts on one RLS clause, in the lint's words + * (objectui#13061, objectui#13082). + * + * The verdicts are the publish door's. `validateRlsPredicateEnforceability` is + * the `rls-predicate-*` rule `os build` and the runtime publish gate run, so + * asking it here means Studio and the server cannot disagree about a clause. + * The runtime drops a policy the rule refuses, and the policy then grants (or + * admits) nothing: it fails CLOSED. + * + * The words are the lint's. ⛔ No sentence for any of these verdicts is written + * here, and none is synthesized: each finding is one the published rule returns + * for a one-clause stack holding this source, with its severity and message as + * written. What the rule does not say, the editor does not say. + * + * ## What the one-clause stack lets the rule judge + * + * Every clause that parses is asked, lowerable or not (objectui#13082; until + * then only a clause `isSupportedRlsExpression` refuses was). A clause the + * engine already refuses as a parse fault is not asked, so one fault earns one + * blocking finding (a `null` beside a typed list member is one). The stack names + * no object, so the rule answers exactly what the clause alone decides — the + * answer the publish door gives for the same clause. For a clause the runtime + * cannot lower that is the shape pass. For one that lowers it is each + * reference verdict that reads the clause and nothing else: a `current_user` + * member nothing pre-resolves, a `current_user` value of the wrong type for + * its position, a comparand the shared filter faces refuse. Which verdicts + * those are is the rule's, not a list kept here: + * `celAuthoring.rlsReferencePasses-13082.test.ts` pins each one against the + * rule. + * + * ## What stays at the publish door, and why + * + * The rule's other reference passes read metadata the editor does not hold. + * The one-clause stack carries no object, so they are silent here by the + * rule's own construction (an object the stack does not declare is one it + * cannot judge), and absent metadata is never an editor verdict: + * + * - an unknown field reads the object's field map as the door's universe + * holds it: the live objects plus the drafts a package publish carries with + * the write. The editor holds a list of field NAMES read from the published + * object (the host's `loadObjectFields`), so a field the author added in a + * pending object draft is missing from it, and the rule would refuse a + * predicate the package publish accepts. Here an unknown field gets at most + * the engine's non-blocking "did you mean" warning, for a near miss + * ({@link lintCelPredicate}); + * - a field compared with a field that holds a list or an object, or with a + * field of another comparison class, reads each field's declared TYPE; the + * editor holds names only; + * - the engine's admission of the read scope reads the host engine's + * `judgeFilter`, which only the server holds. + * + * A lint without the rule gives no verdict, the same degradation as every * other entry point of this module. */ -async function rlsLowerabilityVerdict( - mod: FormulaModule, - source: string, - clause: 'using' | 'check', -): Promise { - if (!mod.isSupportedRlsExpression || mod.isSupportedRlsExpression(source)) return []; +async function rlsPredicateVerdict(source: string, clause: 'using' | 'check'): Promise { const rule = await loadRlsRule(); if (!rule) return []; const issues: CelLintIssue[] = []; @@ -573,9 +599,9 @@ async function rowCanonAdvisory(source: string, slot: string | undefined): Promi /** * Validate one CEL source. Returns the findings to surface inline: parse * faults as `error` (block save), unknown-field near-misses as `warning` - * (advisory). With {@link CelSchemaHint.clause} set, an RLS clause the runtime - * cannot lower is an `error` too — the server's verdict in the server's words - * (see `rlsLowerabilityVerdict`). + * (advisory). With {@link CelSchemaHint.clause} set, `@objectstack/lint`'s RLS + * rule judges the clause too, and each finding keeps the rule's severity and + * words — the publish door's verdict (see `rlsPredicateVerdict`). * * RLS predicates reference fields BARE (`organization_id == current_user.…`), * like a flattened flow condition — so a bare identifier is legal and an @@ -615,14 +641,15 @@ export async function lintCelPredicate( for (const w of res?.warnings ?? []) { if (w?.message) issues.push({ severity: 'warning', message: w.message }); } - // Lowerability (objectui#13061) — see `rlsLowerabilityVerdict`. Both RLS - // clauses, only once the predicate parses (a parse fault already surfaced - // above), at the server's severity and in its words. + // The RLS rule's verdicts (objectui#13061, objectui#13082) — see + // `rlsPredicateVerdict`. Both RLS clauses, lowerable or not, only once the + // predicate parses (a parse fault already surfaced above), at the rule's + // severity and in its words. // (`some`, not `every`: an inferred predicate on `every` would narrow // `issues` to warnings and refuse the push below.) if (!issues.some((i) => i.severity === 'error') && (hint.clause === 'using' || hint.clause === 'check')) { try { - issues.push(...(await rlsLowerabilityVerdict(mod, source, hint.clause))); + issues.push(...(await rlsPredicateVerdict(source, hint.clause))); } catch { /* author-time assistance — never let it break the lint */ }