Skip to content

Commit 8e1c930

Browse files
os-zhuangclaude
andcommitted
fix(analytics): scope dimension-label lookup to the referenced object's RLS (#3602)
A dataset that groups by a lookup/master_detail dimension resolves the grouped FK ids to the related record's display name via a per-record read (`group by id`) dressed as an aggregate. That read carried no read scope, so it revealed related-record display names whenever the referenced object's RLS is stricter than the base object whose rows carry the id — a user could read a name the referenced object's own RLS would hide. (Same-object and looser-referenced cases were already safe: the ids come from the post-#3597 scoped aggregate. This closes the stricter-referenced case, the one residual flagged in #3602.) The label lookup now applies the REFERENCED object's own read scope, bound to the request via the same getReadScope provider the aggregate path uses, composed with `$and` (never key-merge) so the id predicate can't displace it. Fail-closed: if that object's scope can't be resolved, the dimension's labels are skipped (raw id renders) instead of fetched unscoped. No change when no read-scope provider is configured. `DimensionLabelDeps.fetchRecordLabels` gains an optional `scope` arg and `resolveDimensionLabels` an optional `resolveScope` resolver — both service-analytics-internal, no spec/contract change. Tests: dimension-labels unit cases prove the referenced-object scope is threaded and that resolution failure fails closed; a query-dataset integration case proves AnalyticsService.queryDataset binds the referenced object's scope (not the base object's) — reverting the wiring turns it red. Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 6ba3788 commit 8e1c930

6 files changed

Lines changed: 203 additions & 6 deletions

File tree

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
---
2+
"@objectstack/service-analytics": patch
3+
---
4+
5+
fix(analytics): scope the dimension-label lookup to the referenced object's RLS (#3602)
6+
7+
When a dataset groups by a `lookup`/`master_detail` dimension, analytics resolves
8+
the grouped FK ids to the related record's display name via a per-record read
9+
(`group by id`) dressed as an aggregate. That read carried **no read scope**, so
10+
it revealed related-record display names whenever the referenced object's RLS is
11+
stricter than the base object whose rows carry the id — a user could see a name
12+
the referenced object's own RLS would hide. (Same-object and looser-referenced
13+
cases were already safe because the ids come from the post-#3597 scoped
14+
aggregate; this closes the stricter-referenced case.)
15+
16+
The label lookup now applies the **referenced object's own** read scope — bound
17+
to the request via the same `getReadScope` provider the aggregate path uses,
18+
composed with `$and` (never key-merge) so it can't be displaced by the id
19+
predicate. Fail-closed: if that object's scope can't be resolved, the dimension's
20+
labels are skipped (the raw id renders) rather than fetched unscoped. No behaviour
21+
change when no read-scope provider is configured.
22+
23+
Internal `DimensionLabelDeps.fetchRecordLabels` gains an optional `scope` argument
24+
and `resolveDimensionLabels` an optional `resolveScope` resolver; both are
25+
service-analytics-internal (no spec/contract change).

packages/services/service-analytics/src/__tests__/dimension-labels.test.ts

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,71 @@ describe('resolveDimensionLabels', () => {
117117
expect(calls).toBe(1);
118118
expect(rows.map((r) => r.account)).toEqual(['Acme Corp', 'Acme Corp', 'Globex']);
119119
});
120+
121+
// ── #3602 — the label lookup must carry the REFERENCED object's read scope ──
122+
describe('lookup label read scope (#3602)', () => {
123+
it('passes the referenced object scope through to fetchRecordLabels', async () => {
124+
let seenScope: unknown = 'UNSET';
125+
let seenTarget: string | undefined;
126+
const d = deps({
127+
fetchRecordLabels: async (target, ids, scope) => {
128+
seenTarget = target;
129+
seenScope = scope;
130+
return new Map<unknown, string>(ids.map((id) => [id, `name-${String(id)}`]));
131+
},
132+
});
133+
const rows = [{ account: 'acc1', n: 1 }];
134+
// resolveScope returns the referenced object's own RLS predicate.
135+
await resolveDimensionLabels('task', [{ name: 'account', field: 'account' }], rows, d, (target) => {
136+
expect(target).toBe('crm_account'); // the REFERENCED object, not the base
137+
return { organization_id: 'org_A' };
138+
});
139+
expect(seenTarget).toBe('crm_account');
140+
expect(seenScope).toEqual({ organization_id: 'org_A' });
141+
});
142+
143+
it('fails CLOSED: when the scope cannot be resolved, the id is left raw (no unscoped fetch)', async () => {
144+
let fetched = false;
145+
const d = deps({
146+
fetchRecordLabels: async (_t, ids) => {
147+
fetched = true;
148+
return new Map<unknown, string>(ids.map((id) => [id, 'LEAKED NAME']));
149+
},
150+
});
151+
const rows = [{ account: 'acc1', n: 1 }];
152+
await resolveDimensionLabels('task', [{ name: 'account', field: 'account' }], rows, d, () => {
153+
throw new Error('security service unavailable');
154+
});
155+
// The label fetch must NOT have run, and the raw id must survive.
156+
expect(fetched).toBe(false);
157+
expect(rows).toEqual([{ account: 'acc1', n: 1 }]);
158+
});
159+
160+
it('a select dimension is unaffected by scope resolution (no referenced object)', async () => {
161+
let scopeCalls = 0;
162+
const rows = [{ status: 'backlog', n: 1 }];
163+
await resolveDimensionLabels('task', [{ name: 'status', field: 'status' }], rows, deps(), () => {
164+
scopeCalls++;
165+
return undefined;
166+
});
167+
expect(scopeCalls).toBe(0); // scope is only resolved for lookup/master_detail dims
168+
expect(rows).toEqual([{ status: 'Backlog', n: 1 }]);
169+
});
170+
171+
it('no resolver (no security configured) → unscoped fetch, unchanged behaviour', async () => {
172+
let seenScope: unknown = 'UNSET';
173+
const d = deps({
174+
fetchRecordLabels: async (_t, ids, scope) => {
175+
seenScope = scope;
176+
return new Map<unknown, string>(ids.map((id) => [id, 'Acme Corp']));
177+
},
178+
});
179+
const rows = [{ account: 'acc1', n: 1 }];
180+
await resolveDimensionLabels('task', [{ name: 'account', field: 'account' }], rows, d /* no resolveScope */);
181+
expect(seenScope).toBeUndefined();
182+
expect(rows).toEqual([{ account: 'Acme Corp', n: 1 }]);
183+
});
184+
});
120185
});
121186

122187
describe('formatDateBucket', () => {

packages/services/service-analytics/src/__tests__/query-dataset.test.ts

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -393,4 +393,52 @@ describe('AnalyticsService.queryDataset', () => {
393393
expect(result.totals[1].dimensions).toEqual([]);
394394
expect(result.drillRawTotals[1]).toEqual([{}]);
395395
});
396+
397+
// ── #3602 — the lookup label read is scoped to the REFERENCED object's RLS ──
398+
it('threads the referenced object read scope (not the base object) into the label fetch', async () => {
399+
const byAccount = DatasetSchema.parse({
400+
name: 'sales_acct_scoped', label: 'Sales', object: 'opportunity', include: [],
401+
dimensions: [{ name: 'account', field: 'account', type: 'lookup', label: 'Account' }],
402+
measures: [{ name: 'revenue', aggregate: 'sum', field: 'amount' }],
403+
});
404+
const labelScopes: Array<{ target: string; scope: unknown }> = [];
405+
const svc = new AnalyticsService({
406+
queryCapabilities: () => ({ nativeSql: true, objectqlAggregate: false, inMemory: false }),
407+
executeRawSql: async () => [{ account: 'acc1', revenue: 1000 }],
408+
// Per-object scope: opportunity and crm_account get DIFFERENT predicates, so
409+
// the assertion proves the label fetch used the referenced object's scope,
410+
// not the base object's.
411+
getReadScope: (object, ctx?: ExecutionContext) => {
412+
if (!ctx?.tenantId) return undefined;
413+
return object === 'crm_account'
414+
? { organization_id: ctx.tenantId, is_public: true }
415+
: { organization_id: ctx.tenantId };
416+
},
417+
labelResolver: {
418+
getObjectFields: (obj) => ({
419+
opportunity: { account: { type: 'lookup', reference: 'crm_account' } },
420+
crm_account: { name: { type: 'text' } },
421+
} as Record<string, Record<string, { type?: string; reference?: string }>>)[obj],
422+
fetchRecordLabels: async (target, ids, scope) => {
423+
labelScopes.push({ target, scope });
424+
const m = new Map<unknown, string>();
425+
for (const id of ids) m.set(id, `name-${String(id)}`);
426+
return m;
427+
},
428+
},
429+
});
430+
431+
const result = await svc.queryDataset(
432+
byAccount,
433+
{ dimensions: ['account'], measures: ['revenue'] },
434+
{ tenantId: 'org_A' } as ExecutionContext,
435+
) as any;
436+
437+
// The label fetch ran against the referenced object, carrying ITS scope.
438+
expect(labelScopes).toEqual([
439+
{ target: 'crm_account', scope: { organization_id: 'org_A', is_public: true } },
440+
]);
441+
// And the label actually resolved (end-to-end sanity).
442+
expect(result.rows).toEqual([{ account: 'name-acc1', revenue: 1000 }]);
443+
});
396444
});

packages/services/service-analytics/src/analytics-service.ts

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -604,14 +604,23 @@ export class AnalyticsService implements IAnalyticsService {
604604
.filter((d) => !!d.field)
605605
.map((d) => ({ name: d.name, field: d.field, type: d.type, dateGranularity: d.dateGranularity }));
606606
if (dims.length) {
607+
// #3602 — bind the referenced object's read scope to THIS request so the
608+
// label lookup (a per-record read of the related object) cannot surface a
609+
// record the referenced object's RLS would hide. Same provider the
610+
// aggregate path uses; `undefined` when no provider is configured, in
611+
// which case labels fetch unscoped exactly as before.
612+
const provider = this.readScopeProvider;
613+
const resolveScope = provider
614+
? (targetObject: string) => provider(targetObject, context)
615+
: undefined;
607616
try {
608-
await resolveDimensionLabels(dataset.object, dims, result.rows, this.labelResolver);
617+
await resolveDimensionLabels(dataset.object, dims, result.rows, this.labelResolver, resolveScope);
609618
// Totals rows (#1753) carry dimension values too (a row subtotal is
610619
// keyed by its row bucket) — resolve each grouping's own subset.
611620
for (const total of result.totals ?? []) {
612621
const subset = dims.filter((d) => total.dimensions.includes(d.name));
613622
if (subset.length) {
614-
await resolveDimensionLabels(dataset.object, subset, total.rows, this.labelResolver);
623+
await resolveDimensionLabels(dataset.object, subset, total.rows, this.labelResolver, resolveScope);
615624
}
616625
}
617626
} catch (e) {

packages/services/service-analytics/src/dimension-labels.ts

Lines changed: 45 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,10 +39,34 @@ export interface DimensionLabelDeps {
3939
* Fetch a map of `id → display label` for the given ids of a target object.
4040
* The implementation chooses the target's display field. Returning an empty
4141
* map (e.g. no display field, no data access) leaves the ids unresolved.
42+
*
43+
* `scope` (ADR-0021 D-C, #3602) is the TARGET object's own read scope — the
44+
* RLS/tenant `FilterCondition` the implementation must AND into the label
45+
* lookup so this never reveals a related record the target object's RLS would
46+
* hide. The label lookup is a per-record read (`group by id`) dressed as an
47+
* aggregate; without the scope it leaks display names whenever the referenced
48+
* object is more restricted than the base object whose rows carry the id.
49+
* `undefined` means "no scope for this object" (global table / unrestricted
50+
* caller) — the same contract as the read-scope provider.
4251
*/
43-
fetchRecordLabels(targetObject: string, ids: unknown[]): Promise<Map<unknown, string>>;
52+
fetchRecordLabels(
53+
targetObject: string,
54+
ids: unknown[],
55+
scope?: Record<string, unknown>,
56+
): Promise<Map<unknown, string>>;
4457
}
4558

59+
/**
60+
* Resolve the TARGET object's read scope for a label lookup (#3602). Returns the
61+
* object's RLS/tenant `FilterCondition`, `null`/`undefined` when the object is
62+
* unscoped, or a rejected promise when the scope cannot be resolved — in which
63+
* case the resolver fails CLOSED (skips that dimension's labels) rather than
64+
* fetching unscoped names.
65+
*/
66+
export type LabelScopeResolver = (
67+
targetObject: string,
68+
) => Promise<Record<string, unknown> | null | undefined> | Record<string, unknown> | null | undefined;
69+
4670
const LOOKUP_TYPES = new Set(['lookup', 'master_detail']);
4771

4872
/** Date-dimension granularity (mirrors the dataset `dateGranularity` enum). */
@@ -100,12 +124,18 @@ export function formatDateBucket(value: unknown, granularity?: DateGranularity |
100124
* (row key = `name`)
101125
* @param rows - result rows, mutated in place
102126
* @param deps - injected runtime capabilities
127+
* @param resolveScope - (ADR-0021 D-C, #3602) resolves the referenced object's
128+
* own read scope for a lookup/master_detail dimension's label fetch. When it
129+
* throws, that dimension's labels are SKIPPED (fail-closed — the raw id renders
130+
* instead) rather than fetched unscoped. Omit when no read-scope provider is
131+
* configured (labels then fetch unscoped, as before — no security in play).
103132
*/
104133
export async function resolveDimensionLabels(
105134
baseObject: string,
106135
dims: Array<{ name: string; field: string; type?: string; dateGranularity?: DateGranularity | string }>,
107136
rows: Record<string, unknown>[],
108137
deps: DimensionLabelDeps,
138+
resolveScope?: LabelScopeResolver,
109139
): Promise<void> {
110140
if (!rows.length || !dims.length) return;
111141
const fields = deps.getObjectFields(baseObject);
@@ -148,7 +178,20 @@ export async function resolveDimensionLabels(
148178
new Set(rows.map((r) => r[dim.name]).filter((v) => v != null)),
149179
);
150180
if (ids.length === 0) continue;
151-
const labelById = await deps.fetchRecordLabels(meta.reference, ids);
181+
// #3602 — the label lookup reads the REFERENCED object by id. Scope it to
182+
// that object's own RLS so it never surfaces a related record the target's
183+
// RLS would hide (leak fires when the referenced object is stricter than
184+
// the base). Fail closed: if the scope can't be resolved, skip this
185+
// dimension's labels (raw id renders) rather than fetch unscoped.
186+
let scope: Record<string, unknown> | null | undefined;
187+
if (resolveScope) {
188+
try {
189+
scope = await resolveScope(meta.reference);
190+
} catch {
191+
continue;
192+
}
193+
}
194+
const labelById = await deps.fetchRecordLabels(meta.reference, ids, scope ?? undefined);
152195
if (!labelById || labelById.size === 0) continue;
153196
for (const row of rows) {
154197
const label = labelById.get(row[dim.name]);

packages/services/service-analytics/src/plugin.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -353,17 +353,24 @@ export class AnalyticsServicePlugin implements Plugin {
353353
};
354354
const labelResolver: DimensionLabelDeps = {
355355
getObjectFields: (objectName) => dataEngine()?.getObject?.(objectName)?.fields,
356-
fetchRecordLabels: async (targetObject, ids) => {
356+
fetchRecordLabels: async (targetObject, ids, scope) => {
357357
const map = new Map<unknown, string>();
358358
const displayField = pickDisplayField(dataEngine()?.getObject?.(targetObject)?.fields);
359359
if (!displayField || !executeAggregate || ids.length === 0) return map;
360+
// #3602 — AND the referenced object's own read scope into the id filter,
361+
// with `$and` (never key-merge) so it cannot be displaced by the id
362+
// predicate — the same composition the strategy uses for the aggregate.
363+
// Without it this per-record read leaks display names the target's RLS
364+
// would hide (fires when the referenced object is stricter than the base).
365+
const idFilter: Record<string, unknown> = { id: { $in: ids } };
366+
const filter = scope ? { $and: [idFilter, scope] } : idFilter;
360367
// Group by (id, displayField) — one row per record — reusing the aggregate
361368
// bridge rather than adding a record-fetch capability. A count keeps engines
362369
// that require ≥1 aggregation happy; the count itself is unused.
363370
const rows = await executeAggregate(targetObject, {
364371
groupBy: ['id', displayField],
365372
aggregations: [{ field: 'id', method: 'count', alias: '_c' }],
366-
filter: { id: { $in: ids } },
373+
filter,
367374
});
368375
for (const r of rows) {
369376
if (r.id != null && r[displayField] != null) map.set(r.id, String(r[displayField]));

0 commit comments

Comments
 (0)