From 67c716ce304ca6d491af6bfa7d123793676f0b28 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Jul 2026 12:52:58 +0000 Subject: [PATCH] =?UTF-8?q?feat(analytics,spec):=20executeAggregate=20brid?= =?UTF-8?q?ge=20carries=20ExecutionContext=20=E2=80=94=20ADR-0021=20D-C=20?= =?UTF-8?q?second=20belt=20(#3602)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The analytics→engine bridge now forwards the request's ExecutionContext to `engine.aggregate`, so the engine's own middleware chain scopes analytics reads independently of the analytics layer's `getReadScope`. `BaseEngineOptions.context` has always been `.optional()`, so nothing forced the bridge to pass it — and it did not. An authenticated aggregate reached the engine with no principal, plugin-security's principal-less fall-open skipped its RLS injection, and the only thing left scoping the query was the strategy remembering to call `getReadScope`. #3597 was a strategy that did not, and both belts were off at once. `getReadScope` stays: the two resolve scope through different paths (engine middleware vs `security.getReadFilter`), and a deployment without plugin-security has only the analytics layer. Depth, not a replacement. - `StrategyContext` gains `context?: ExecutionContext`, bound per call by `AnalyticsService.callCtx` — unconditionally, including when no read-scope provider is configured, since that deployment needs the engine belt most. - `StrategyContext.executeAggregate` and the plugin/service `executeAggregate` config options gain `context?: ExecutionContext`. Additive: a custom bridge that ignores it behaves exactly as before. - `fetchRecordLabels` — the dimension display-label lookup — is row-granular (one row per record, real display names) and ran with neither read scope nor context. Its ids come from a scoped aggregate today, so it leaked nothing, but that was the caller's invariant, not the bridge's. Now ANDs the target object's read scope in (`$and`, never a key merge) and forwards the context. - `ObjectQLStrategy.generateSql` emitted no WHERE at all, so `/analytics/sql` read as an unscoped table scan while the real aggregate was scoped. Now renders the caller's filters and the read scope, and runs the same joined-scope guard `execute()` does. Never executed, so this was misleading output rather than a leak. - `BootOptions.analytics` lets a gate boot with the analytics belt off; the new dogfood case asserts the engine belt alone still scopes a member to their own rows. Verified to bite: reverting the context forwarding makes it count 5 instead of 2. Deliberately not fixed here: `ObjectQLStrategy.execute()` ignores `timeDimensions[].dateRange` entirely, so `generateSql` does not render a BETWEEN either. Filed as #3650. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013st2KArjmLQSuVzpDS91jj --- ...ics-execute-aggregate-execution-context.md | 47 +++ packages/qa/dogfood/package.json | 1 + .../test/analytics-rls.dogfood.test.ts | 72 ++++ .../execution-context-bridge.test.ts | 337 ++++++++++++++++++ .../src/analytics-service.ts | 24 +- .../service-analytics/src/dimension-labels.ts | 20 +- .../services/service-analytics/src/plugin.ts | 44 ++- .../src/strategies/objectql-strategy.ts | 139 +++++++- .../spec/src/contracts/analytics-service.ts | 43 +++ packages/verify/src/harness.ts | 15 +- pnpm-lock.yaml | 3 + 11 files changed, 725 insertions(+), 20 deletions(-) create mode 100644 .changeset/analytics-execute-aggregate-execution-context.md create mode 100644 packages/services/service-analytics/src/__tests__/execution-context-bridge.test.ts diff --git a/.changeset/analytics-execute-aggregate-execution-context.md b/.changeset/analytics-execute-aggregate-execution-context.md new file mode 100644 index 0000000000..1f391cb6eb --- /dev/null +++ b/.changeset/analytics-execute-aggregate-execution-context.md @@ -0,0 +1,47 @@ +--- +"@objectstack/spec": minor +"@objectstack/service-analytics": minor +"@objectstack/verify": minor +--- + +feat(analytics): the executeAggregate bridge carries ExecutionContext — ADR-0021 D-C second belt + +The analytics→engine bridge now forwards the request's `ExecutionContext` to +`engine.aggregate`, so the engine's own middleware chain scopes analytics reads +independently of the analytics layer's `getReadScope`. + +**Why.** `BaseEngineOptions.context` has always been `.optional()`, so nothing +forced the bridge to pass it — and it did not. An authenticated aggregate +reached the engine with no principal, plugin-security's principal-less fall-open +skipped its RLS injection, and the only thing left scoping the query was the +strategy remembering to call `getReadScope`. #3597 was a strategy that did not, +and both belts were off at once. + +`getReadScope` stays: the two resolve scope through different paths (engine +middleware vs `security.getReadFilter`), and a deployment without +plugin-security has only the analytics layer. This is depth, not a replacement. + +- `StrategyContext` gains `context?: ExecutionContext`, bound per call by + `AnalyticsService` from `query()` / `generateSql()` / `queryDataset()`. +- `StrategyContext.executeAggregate` and the `AnalyticsServicePlugin` / + `AnalyticsService` `executeAggregate` config options gain `context?: + ExecutionContext`. **Custom bridges should forward it** to their engine; the + built-in auto-bridge does. Purely additive — an existing bridge that ignores + it keeps working exactly as before. +- `DimensionLabelDeps.fetchRecordLabels` gains an optional trailing `context`, + and `resolveDimensionLabels` an optional trailing `context`. +- `BootOptions.analytics` (`@objectstack/verify`) overrides the + AnalyticsServicePlugin instance, so a gate can boot with the analytics belt + off and assert the engine-side belt alone still scopes. + +**Also fixed on the same seam:** + +- `fetchRecordLabels` — the dimension display-label lookup — is row-granular + (one row per record, real display names) and ran with neither read scope nor + context. Its `ids` are confined to a scoped aggregate's output today, so it + leaked nothing, but that was the caller's invariant, not the bridge's. It now + ANDs the target object's read scope in and forwards the context. +- `ObjectQLStrategy.generateSql` emitted no `WHERE` at all, so the + `/analytics/sql` preview read as an unscoped table scan while the real + aggregate was scoped. It now renders the caller's filters and the read scope. + The preview never executed, so this was misleading output rather than a leak. diff --git a/packages/qa/dogfood/package.json b/packages/qa/dogfood/package.json index 213b23c9ad..782bb72ddd 100644 --- a/packages/qa/dogfood/package.json +++ b/packages/qa/dogfood/package.json @@ -20,6 +20,7 @@ "@objectstack/plugin-auth": "workspace:*", "@objectstack/plugin-security": "workspace:*", "@objectstack/plugin-webhooks": "workspace:*", + "@objectstack/service-analytics": "workspace:*", "@objectstack/service-messaging": "workspace:*", "@objectstack/service-storage": "workspace:*", "@objectstack/spec": "workspace:*", diff --git a/packages/qa/dogfood/test/analytics-rls.dogfood.test.ts b/packages/qa/dogfood/test/analytics-rls.dogfood.test.ts index 2236be69f6..f67412dec8 100644 --- a/packages/qa/dogfood/test/analytics-rls.dogfood.test.ts +++ b/packages/qa/dogfood/test/analytics-rls.dogfood.test.ts @@ -24,6 +24,7 @@ import { describe, it, expect, beforeAll, afterAll } from 'vitest'; import { bootStack, type VerifyStack } from '@objectstack/verify'; +import { AnalyticsServicePlugin } from '@objectstack/service-analytics'; import { rlsFixtureStack, ownerScopedMemberSet, @@ -144,3 +145,74 @@ describe('dogfood: analytics aggregates are RLS-scoped to the caller (#3597)', ( expect(memberSees).toBeLessThan(all.length); }); }); + +/** + * The SECOND belt, alone (#3602). + * + * Everything above runs the fully-wired stack, where the analytics layer scopes + * the query itself (#3601). That proves the system is safe today; it cannot + * prove the belt BENEATH it works, because the first belt masks it. #3597 was + * exactly a first-belt miss — a strategy that never called `getReadScope` — and + * a third strategy will eventually repeat it. + * + * So boot with the analytics belt explicitly OFF (`getReadScope: () => undefined`) + * and assert the member still counts only their own rows. The only thing left + * scoping the read is the ExecutionContext the `executeAggregate` bridge now + * hands `engine.aggregate`, which the security middleware turns into an RLS + * predicate on `opCtx.ast.where`. Before #3602 the bridge passed no context, the + * middleware's principal-less fall-open skipped its own injection, and this case + * would count every note in the table. + */ +describe('dogfood: engine-side RLS scopes analytics even with the analytics belt off (#3602)', () => { + let stack: VerifyStack; + let memberToken: string; + + beforeAll(async () => { + stack = await bootStack(rlsFixtureStack as never, { + security: rlsFixtureSecurity(ownerScopedMemberSet), + analytics: new AnalyticsServicePlugin({ getReadScope: () => undefined }), + }); + + const adminToken = await stack.signIn(); + memberToken = await stack.signUp('analytics-depth@verify.test'); + + for (let i = 0; i < ADMIN_NOTES; i++) { + const r = await stack.apiAs(adminToken, 'POST', '/data/rls_note', { + name: `admin-note-${i}`, + body: 'owned by admin', + }); + expect(r.status).toBeLessThan(300); + } + for (let i = 0; i < MEMBER_NOTES; i++) { + const r = await stack.apiAs(memberToken, 'POST', '/data/rls_note', { + name: `member-note-${i}`, + body: 'owned by member', + }); + expect(r.status).toBeLessThan(300); + } + }, 90_000); + + afterAll(async () => { + await stack?.stop(); + }); + + it('the member counts ONLY their own rows on the ObjectQL (date-bucketed) path', async () => { + const res = await stack.apiAs(memberToken, 'POST', '/analytics/dataset/query', { + dataset: notesByDay, + selection: { dimensions: ['created'], measures: ['cnt'] }, + }); + expect(res.status).toBe(200); + const body = (await res.json()) as { rows?: Array> }; + const seen = (body.rows ?? []).reduce((sum, row) => sum + Number(row.cnt ?? 0), 0); + + expect(seen).toBe(MEMBER_NOTES); + }); + + it('the rows it cannot see DO exist — the belt is scoping, not an empty table', async () => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const ql = await stack.kernel.getServiceAsync('objectql'); + const all = (await ql.find('rls_note', { context: { isSystem: true } })) as unknown[]; + + expect(all).toHaveLength(ADMIN_NOTES + MEMBER_NOTES); + }); +}); diff --git a/packages/services/service-analytics/src/__tests__/execution-context-bridge.test.ts b/packages/services/service-analytics/src/__tests__/execution-context-bridge.test.ts new file mode 100644 index 0000000000..888958dcbb --- /dev/null +++ b/packages/services/service-analytics/src/__tests__/execution-context-bridge.test.ts @@ -0,0 +1,337 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * ADR-0021 D-C — SECOND belt (#3602): the analytics→engine bridge carries the + * request's `ExecutionContext`. + * + * `objectql-read-scope.test.ts` covers the FIRST belt (#3601): the strategy ANDs + * `getReadScope`'s predicate into the filter it hands the engine. That belt only + * works if every strategy remembers to call it — #3597 is what happens when one + * does not, and a third strategy will eventually repeat it. + * + * The second belt is independent: hand the engine the caller's context and the + * engine's own middleware chain (`mergeReadContext` → RLS injection) scopes the + * read whether or not the analytics layer did. `BaseEngineOptions.context` was + * always `.optional()`, so nothing ever forced the bridge to pass it — and it + * did not, which is precisely why plugin-security's principal-less fall-open + * left BOTH belts off at once in #3597. + * + * These cases assert on what reaches the bridge, because that is the seam the + * engine's RLS hangs off of. + */ + +import { describe, it, expect } from 'vitest'; +import { DatasetSchema } from '@objectstack/spec/ui'; +import type { ExecutionContext } from '@objectstack/spec/kernel'; +import type { FilterCondition } from '@objectstack/spec/data'; +import { AnalyticsService } from '../analytics-service.js'; +import { AnalyticsServicePlugin } from '../plugin.js'; +import { compileDataset } from '../dataset-compiler.js'; + +const dataset = DatasetSchema.parse({ + name: 'sales', + label: 'Sales', + object: 'opportunity', + dimensions: [{ name: 'region', field: 'region', type: 'string' }], + measures: [{ name: 'revenue', aggregate: 'sum', field: 'amount' }], +}); + +type AggOpts = { + groupBy?: string[]; + aggregations?: Array<{ field: string; method: string; alias: string }>; + filter?: Record; + timezone?: string; + context?: ExecutionContext; +}; + +const ctxA = { tenantId: 'org_A', userId: 'u_a' } as ExecutionContext; +const ctxB = { tenantId: 'org_B', userId: 'u_b' } as ExecutionContext; + +const objectqlOnly = () => ({ nativeSql: false, objectqlAggregate: true, inMemory: false }); + +const readScope = (_o: string, context?: ExecutionContext): FilterCondition | undefined => + context?.tenantId ? { organization_id: context.tenantId } : undefined; + +function makeService(seen: AggOpts[], overrides: Record = {}) { + const compiled = compileDataset(dataset); + return new AnalyticsService({ + cubes: [compiled.cube], + queryCapabilities: objectqlOnly, + executeAggregate: async (_object: string, opts: AggOpts) => { seen.push(opts); return []; }, + getReadScope: readScope, + ...overrides, + }); +} + +const baseQuery = { cube: 'sales', dimensions: ['region'], measures: ['revenue'] }; + +describe('executeAggregate bridge carries the ExecutionContext (#3602)', () => { + it('forwards the caller context to the aggregate bridge', async () => { + const seen: AggOpts[] = []; + await makeService(seen).query(baseQuery, ctxA); + + expect(seen[0].context).toBe(ctxA); + }); + + it('forwards the context even when NO read-scope provider is configured', async () => { + // The depth case. With the analytics-layer belt absent, the engine's own + // RLS is the ONLY thing standing between this query and every tenant's + // rows — so the context must reach it. Gating context on `getReadScope` + // being wired would starve exactly the deployment that needs it most. + const seen: AggOpts[] = []; + await makeService(seen, { getReadScope: undefined }).query(baseQuery, ctxA); + + expect(seen[0].context).toBe(ctxA); + expect(seen[0].filter).toBeUndefined(); + }); + + it('gives each caller its own context on a shared service instance', async () => { + const seen: AggOpts[] = []; + const service = makeService(seen); + + await service.query(baseQuery, ctxA); + await service.query(baseQuery, ctxB); + + expect(seen.map((s) => s.context)).toEqual([ctxA, ctxB]); + }); + + it('leaves context undefined when the caller supplied none', async () => { + // In-memory / dev / system-internal use. Unchanged behaviour: absent is not + // "unrestricted" — the engine applies its own policy to a context-less op. + const seen: AggOpts[] = []; + await makeService(seen).query(baseQuery); + + expect(seen[0].context).toBeUndefined(); + }); + + it('carries context on the date-bucketed path (where NativeSQL declines)', async () => { + const compiled = compileDataset( + DatasetSchema.parse({ + name: 'sales_t', + label: 'Sales', + object: 'opportunity', + dimensions: [{ name: 'created', field: 'created_at', type: 'date' }], + measures: [{ name: 'revenue', aggregate: 'sum', field: 'amount' }], + }), + ); + const seen: AggOpts[] = []; + const service = new AnalyticsService({ + cubes: [compiled.cube], + queryCapabilities: () => ({ nativeSql: true, objectqlAggregate: true, inMemory: false }), + executeRawSql: async () => { throw new Error('NativeSQL must decline on granularity'); }, + executeAggregate: async (_o: string, opts: AggOpts) => { seen.push(opts); return []; }, + getReadScope: readScope, + }); + + await service.query( + { cube: 'sales_t', measures: ['revenue'], timeDimensions: [{ dimension: 'created', granularity: 'month' }] }, + ctxA, + ); + + expect(seen[0].context).toBe(ctxA); + }); +}); + +// ── The auto-bridge, which is what production actually runs ────────────────── + +/** Minimal PluginContext: the four members `AnalyticsServicePlugin.init` uses. */ +function fakePluginContext(services: Record) { + const registered: Record = {}; + return { + ctx: { + getService: (name: string) => services[name] ?? registered[name], + registerService: (name: string, svc: unknown) => { registered[name] = svc; }, + replaceService: (name: string, svc: unknown) => { registered[name] = svc; }, + logger: { info() {}, warn() {}, error() {}, debug() {} }, + }, + registered, + }; +} + +describe('plugin auto-bridge → engine.aggregate (#3602)', () => { + it('threads the context into the engine call', async () => { + const calls: Array> = []; + const engine = { + aggregate: async (_object: string, options: Record) => { + calls.push(options); + return []; + }, + getObject: () => undefined, + }; + const { ctx, registered } = fakePluginContext({ data: engine }); + const compiled = compileDataset(dataset); + + await new AnalyticsServicePlugin({ + cubes: [compiled.cube], + queryCapabilities: objectqlOnly, + getReadScope: readScope, + }).init(ctx as never); + + await (registered.analytics as AnalyticsService).query(baseQuery, ctxA); + + // `engine.aggregate` merges this into the operation context, which is what + // lets the middleware chain inject RLS into `opCtx.ast.where`. + expect(calls[0].context).toBe(ctxA); + // First belt still intact — the two are additive, not either/or. + expect(calls[0].where).toEqual({ organization_id: 'org_A' }); + }); +}); + +// ── Item 2: the record-label lookup rides the same bridge ──────────────────── + +const labelDataset = DatasetSchema.parse({ + name: 'tasks_by_account', + label: 'Tasks by account', + object: 'task', + dimensions: [{ name: 'account', field: 'account', type: 'string' }], + measures: [{ name: 'cnt', aggregate: 'count' }], +}); + +/** What the ENGINE sees — the bridge translates `filter` → `where`. */ +type EngineCall = { object: string; where?: Record; context?: ExecutionContext }; + +/** + * Drive the label path through the real plugin wiring and return every call the + * engine saw. The dimension-label pass runs on `queryDataset`, after the + * aggregate returns rows carrying FK ids. + */ +async function runLabelLookup(opts: { + scope: (o: string, c?: ExecutionContext) => FilterCondition | undefined; + context?: ExecutionContext; +}) { + const seen: EngineCall[] = []; + const engine = { + aggregate: async (object: string, options: Record) => { + seen.push({ object, ...options } as EngineCall); + // The grouped aggregate returns an FK id; the label pass then resolves it. + return object === 'task' ? [{ account: 'acc1', cnt: 1 }] : [{ id: 'acc1', name: 'Acme Corp', _c: 1 }]; + }, + getObject: (name: string) => + name === 'task' + ? { fields: { account: { type: 'lookup', reference: 'crm_account' } } } + : name === 'crm_account' + ? { fields: { name: { type: 'text' } } } + : undefined, + }; + const { ctx, registered } = fakePluginContext({ data: engine }); + + await new AnalyticsServicePlugin({ + queryCapabilities: objectqlOnly, + getReadScope: opts.scope, + }).init(ctx as never); + + const result = await (registered.analytics as AnalyticsService).queryDataset( + labelDataset as never, + { dimensions: ['account'], measures: ['cnt'] } as never, + opts.context, + ); + return { seen, result }; +} + +describe('fetchRecordLabels is scoped like any other read (#3602 item 2)', () => { + it('ANDs the target object read scope into the id lookup', async () => { + const { seen } = await runLabelLookup({ scope: readScope, context: ctxA }); + + // [0] is the dataset aggregate; [1] is the id→label lookup. That second + // call returns ONE ROW PER RECORD with real display names — row-granular, + // so it needs the same scoping as any other read. + expect(seen[1].object).toBe('crm_account'); + expect(seen[1].where).toEqual({ + $and: [{ id: { $in: ['acc1'] } }, { organization_id: 'org_A' }], + }); + }); + + it('forwards the context so the engine scopes it too', async () => { + const { seen } = await runLabelLookup({ scope: readScope, context: ctxA }); + + expect(seen[1].context).toBe(ctxA); + }); + + it('resolves the label when the record is in scope', async () => { + const { result } = await runLabelLookup({ scope: readScope, context: ctxA }); + + expect(result.rows[0].account).toBe('Acme Corp'); + }); + + it('leaves the raw id when the scope provider fails — never an unscoped read', async () => { + // Fail-closed: the throw propagates out of `fetchRecordLabels`, the caller + // catches it and rows keep their ids. Labels are lost; rows are not exposed. + // Only the LOOKUP target fails here — the dataset's own query must still + // run, or this would just be re-testing `resolveReadScopes` (#3601). + const { seen, result } = await runLabelLookup({ + scope: (o) => { + if (o === 'crm_account') throw new Error('security service unavailable'); + return { organization_id: 'org_A' }; + }, + context: ctxA, + }); + + expect(seen).toHaveLength(1); // the lookup never reached the engine + expect(result.rows[0].account).toBe('acc1'); + }); +}); + +// ── Item 3: the /analytics/sql preview must match what executes ────────────── + +describe('ObjectQLStrategy.generateSql reflects the executed query (#3602 item 3)', () => { + it('renders the read scope in the WHERE clause', async () => { + const { sql, params } = await makeService([]).generateSql(baseQuery, ctxA); + + expect(sql).toContain('WHERE ("opportunity"."organization_id" = $1)'); + expect(params).toEqual(['org_A']); + }); + + it('renders the caller filters alongside the scope', async () => { + const { sql, params } = await makeService([]).generateSql( + { ...baseQuery, where: { region: 'West' } }, + ctxA, + ); + + expect(sql).toContain('WHERE region = $1 AND ("opportunity"."organization_id" = $2)'); + expect(params).toEqual(['West', 'org_A']); + }); + + it('emits no WHERE when there is neither a filter nor a scope', async () => { + const { sql, params } = await makeService([], { getReadScope: undefined }).generateSql(baseQuery, ctxA); + + expect(sql).not.toContain('WHERE'); + expect(params).toEqual([]); + }); + + it('renders the operators the analytics layer emits', async () => { + const { sql, params } = await makeService([], { getReadScope: undefined }).generateSql( + { ...baseQuery, where: { region: { $in: ['West', 'East'] }, amount: { $gte: 10 } } }, + ctxA, + ); + + expect(sql).toContain('region IN ($1, $2)'); + expect(sql).toContain('amount >= $3'); + expect(params).toEqual(['West', 'East', 10]); + }); + + it('denies the preview when a joined object carries an unenforceable scope', async () => { + // The preview must not render SQL for a query `execute()` would reject — + // that mismatch is the whole defect this item fixes. + const compiled = compileDataset( + DatasetSchema.parse({ + name: 'sales_by_account', + label: 'Sales by account', + object: 'opportunity', + include: ['account'], + dimensions: [{ name: 'region', field: 'account.region', type: 'string' }], + measures: [{ name: 'revenue', aggregate: 'sum', field: 'amount' }], + }), + ); + const service = new AnalyticsService({ + cubes: [compiled.cube], + queryCapabilities: objectqlOnly, + executeAggregate: async () => [], + getReadScope: () => ({ organization_id: 'org_A' }), + getAllowedRelationships: () => compiled.allowedRelationships, + }); + + await expect( + service.generateSql({ cube: 'sales_by_account', dimensions: ['region'], measures: ['revenue'] }, ctxA), + ).rejects.toThrow(/cannot enforce the read scope of joined object\(s\) "account"/); + }); +}); diff --git a/packages/services/service-analytics/src/analytics-service.ts b/packages/services/service-analytics/src/analytics-service.ts index 9a01611aa9..56f54c3fa2 100644 --- a/packages/services/service-analytics/src/analytics-service.ts +++ b/packages/services/service-analytics/src/analytics-service.ts @@ -113,6 +113,15 @@ export interface AnalyticsServiceConfig { groupBy?: string[]; aggregations?: Array<{ field: string; method: string; alias: string }>; filter?: Record; + /** Reference timezone (IANA) for date bucketing — ADR-0053 Phase 2. */ + timezone?: string; + /** + * ADR-0021 D-C (#3602) — the request's ExecutionContext. Bridges MUST + * forward it to `engine.aggregate` so engine-side RLS applies; see + * `StrategyContext.executeAggregate` for why this is a second belt rather + * than a replacement for `getReadScope`. + */ + context?: ExecutionContext; }) => Promise[]>; /** * Fallback IAnalyticsService (e.g. MemoryAnalyticsService). @@ -329,7 +338,12 @@ export class AnalyticsService implements IAnalyticsService { query: AnalyticsQuery, context?: ExecutionContext, ): Promise { - if (!this.readScopeProvider) return this.baseCtx; + // #3602 — `context` rides along unconditionally. It is the ENGINE-side belt + // (forwarded to `engine.aggregate`, where the middleware chain applies its + // own RLS), so it must not be gated on the analytics-side belt being wired: + // a deployment with no `getReadScope` provider is exactly the one that most + // needs the engine to scope for it. + if (!this.readScopeProvider) return { ...this.baseCtx, context }; // Pre-resolve the read scope for every object the strategy will scan (base // + all declared joins) BEFORE the synchronous SQL builder runs, since the // provider may be async (the production `security.getReadFilter` bridge). @@ -337,6 +351,7 @@ export class AnalyticsService implements IAnalyticsService { const scopes = await this.resolveReadScopes(query, context); return { ...this.baseCtx, + context, getReadScope: (objectName: string) => scopes.get(objectName) ?? null, }; } @@ -605,13 +620,16 @@ export class AnalyticsService implements IAnalyticsService { .map((d) => ({ name: d.name, field: d.field, type: d.type, dateGranularity: d.dateGranularity })); if (dims.length) { try { - await resolveDimensionLabels(dataset.object, dims, result.rows, this.labelResolver); + // #3602 — hand the resolver the request's context: its lookup label + // pass reads ONE ROW PER RECORD from the related object, so it must be + // scoped to the caller like any other read. + await resolveDimensionLabels(dataset.object, dims, result.rows, this.labelResolver, context); // Totals rows (#1753) carry dimension values too (a row subtotal is // keyed by its row bucket) — resolve each grouping's own subset. for (const total of result.totals ?? []) { const subset = dims.filter((d) => total.dimensions.includes(d.name)); if (subset.length) { - await resolveDimensionLabels(dataset.object, subset, total.rows, this.labelResolver); + await resolveDimensionLabels(dataset.object, subset, total.rows, this.labelResolver, context); } } } catch (e) { diff --git a/packages/services/service-analytics/src/dimension-labels.ts b/packages/services/service-analytics/src/dimension-labels.ts index c26840c251..db68a8ec42 100644 --- a/packages/services/service-analytics/src/dimension-labels.ts +++ b/packages/services/service-analytics/src/dimension-labels.ts @@ -22,6 +22,8 @@ * {@link DimensionLabelDeps} so this module stays free of any engine dependency. */ +import type { ExecutionContext } from '@objectstack/spec/kernel'; + /** The minimal field shape this resolver needs. */ export interface FieldMetaLite { type?: string; @@ -39,8 +41,18 @@ export interface DimensionLabelDeps { * Fetch a map of `id → display label` for the given ids of a target object. * The implementation chooses the target's display field. Returning an empty * map (e.g. no display field, no data access) leaves the ids unresolved. + * + * `context` is the request's ExecutionContext (#3602). This lookup returns + * one row PER RECORD — real display names, at row granularity, not aggregate + * granularity — so the implementation MUST scope it to what the caller may + * see. Passing it is not optional in production wiring; the parameter is + * optional only so unit fakes can ignore it. */ - fetchRecordLabels(targetObject: string, ids: unknown[]): Promise>; + fetchRecordLabels( + targetObject: string, + ids: unknown[], + context?: ExecutionContext, + ): Promise>; } const LOOKUP_TYPES = new Set(['lookup', 'master_detail']); @@ -100,12 +112,16 @@ export function formatDateBucket(value: unknown, granularity?: DateGranularity | * (row key = `name`) * @param rows - result rows, mutated in place * @param deps - injected runtime capabilities + * @param context - the request's ExecutionContext, forwarded to + * {@link DimensionLabelDeps.fetchRecordLabels} so the per-record label lookup + * is scoped to the caller's visibility (#3602) */ export async function resolveDimensionLabels( baseObject: string, dims: Array<{ name: string; field: string; type?: string; dateGranularity?: DateGranularity | string }>, rows: Record[], deps: DimensionLabelDeps, + context?: ExecutionContext, ): Promise { if (!rows.length || !dims.length) return; const fields = deps.getObjectFields(baseObject); @@ -148,7 +164,7 @@ export async function resolveDimensionLabels( new Set(rows.map((r) => r[dim.name]).filter((v) => v != null)), ); if (ids.length === 0) continue; - const labelById = await deps.fetchRecordLabels(meta.reference, ids); + const labelById = await deps.fetchRecordLabels(meta.reference, ids, context); if (!labelById || labelById.size === 0) continue; for (const row of rows) { const label = labelById.get(row[dim.name]); diff --git a/packages/services/service-analytics/src/plugin.ts b/packages/services/service-analytics/src/plugin.ts index 6c97fb026f..8bade759c3 100644 --- a/packages/services/service-analytics/src/plugin.ts +++ b/packages/services/service-analytics/src/plugin.ts @@ -23,6 +23,12 @@ interface DataEngineLike { aggregations?: Array<{ function: string; field: string; alias: string }>; /** Reference timezone (IANA) for date bucketing — ADR-0053 Phase 2. */ timezone?: string; + /** + * `BaseEngineOptions.context` — identity/tenant of the request. The engine + * merges it into the operation context (`mergeReadContext`), which is what + * lets its middleware chain inject RLS into `opCtx.ast.where` (#3602). + */ + context?: ExecutionContext; }): Promise; execute?(command: unknown, options?: Record): Promise; /** Return the registered object schema (relationship → target + display-label resolution). */ @@ -81,6 +87,12 @@ export interface AnalyticsServicePluginOptions { filter?: Record; /** Reference timezone (IANA) for date bucketing — ADR-0053 Phase 2. */ timezone?: string; + /** + * ADR-0021 D-C (#3602) — the request's ExecutionContext. A custom bridge + * MUST forward it to its engine so engine-side RLS applies; dropping it is + * what made the built-in bridge fall open in #3597. + */ + context?: ExecutionContext; }) => Promise[]>; /** * ADR-0021 D-C — context-aware per-object read scope (tenant + RLS). The @@ -186,7 +198,7 @@ export class AnalyticsServicePlugin implements Plugin { 'will retry per-query. Register ObjectQLPlugin or pass executeAggregate.', ); } - executeAggregate = async (objectName, { groupBy, aggregations, filter, timezone }) => { + executeAggregate = async (objectName, { groupBy, aggregations, filter, timezone, context }) => { const engine = tryGetDataEngine(); if (!engine) { throw new Error( @@ -205,6 +217,12 @@ export class AnalyticsServicePlugin implements Plugin { // ADR-0053 Phase 2: thread the reference tz so date buckets resolve on // that zone's calendar days (engine buckets in-memory when non-UTC). timezone, + // ADR-0021 D-C (#3602): thread the caller's identity so the engine's + // middleware chain scopes the read itself. `BaseEngineOptions.context` + // is `.optional()`, so nothing ever forced this bridge to pass it — + // and it did not, which is how an authenticated aggregate reached the + // engine with no principal and plugin-security fell open (#3597). + context, }); return rows as Record[]; }; @@ -353,17 +371,37 @@ export class AnalyticsServicePlugin implements Plugin { }; const labelResolver: DimensionLabelDeps = { getObjectFields: (objectName) => dataEngine()?.getObject?.(objectName)?.fields, - fetchRecordLabels: async (targetObject, ids) => { + fetchRecordLabels: async (targetObject, ids, context) => { const map = new Map(); const displayField = pickDisplayField(dataEngine()?.getObject?.(targetObject)?.fields); if (!displayField || !executeAggregate || ids.length === 0) return map; + // #3602 — this call is row-granular, not aggregate-granular: grouping by + // (id, displayField) returns the REAL display name of every matched + // record. Today the `ids` are already confined to what the caller can + // see (they come out of a scoped aggregate, #3601), so it leaks nothing + // — but that is an invariant of the current caller, not of this bridge. + // Widen the id source (a new strategy, a new caller) and it becomes a + // row-level read with nothing between it and the table. So scope it + // here, on both belts, the same way the aggregate path is scoped. + // + // Fail-closed: a throwing provider propagates out of `fetchRecordLabels` + // rather than degrading to an unscoped read. `resolveDimensionLabels`' + // caller catches it and leaves the raw ids in place — labels are lost, + // rows are not exposed. + const scope = await getReadScope?.(targetObject, context); + // `$and`, never a key merge: the scope may name `id` too, and a spread + // would let the id list overwrite the security predicate. + const filter: Record = scope + ? { $and: [{ id: { $in: ids } }, scope as Record] } + : { id: { $in: ids } }; // Group by (id, displayField) — one row per record — reusing the aggregate // bridge rather than adding a record-fetch capability. A count keeps engines // that require ≥1 aggregation happy; the count itself is unused. const rows = await executeAggregate(targetObject, { groupBy: ['id', displayField], aggregations: [{ field: 'id', method: 'count', alias: '_c' }], - filter: { id: { $in: ids } }, + filter, + context, }); for (const r of rows) { if (r.id != null && r[displayField] != null) map.set(r.id, String(r[displayField])); diff --git a/packages/services/service-analytics/src/strategies/objectql-strategy.ts b/packages/services/service-analytics/src/strategies/objectql-strategy.ts index 8436e53d84..ef1a989c04 100644 --- a/packages/services/service-analytics/src/strategies/objectql-strategy.ts +++ b/packages/services/service-analytics/src/strategies/objectql-strategy.ts @@ -4,6 +4,12 @@ import type { AnalyticsQuery, AnalyticsResult } from '@objectstack/spec/contract import type { Cube } from '@objectstack/spec/data'; import type { AnalyticsStrategy, StrategyContext } from './types.js'; import { normalizeAnalyticsFilters, coerceFilterValueForObjectQL } from './filter-normalizer.js'; +import { compileScopedFilterToSql } from '../read-scope-sql.js'; + +/** Scalar analytics operators → their SQL spelling (display SQL only). */ +const SCALAR_SQL_OPS: Record = { + equals: '=', notEquals: '!=', gt: '>', gte: '>=', lt: '<', lte: '<=', +}; /** * ObjectQLStrategy — Priority 2 @@ -82,7 +88,7 @@ export class ObjectQLStrategy implements AnalyticsStrategy { // ADR-0021 D-C — the read scope (tenant + RLS) MUST be ANDed in before the // query leaves the strategy. Rejects the query outright when a joined object // carries a scope this path cannot express (#3597). - this.assertJoinedScopesEnforceable(cube, query, groupBy, filter, ctx); + this.assertJoinedScopesEnforceable(cube, query, ctx); const rows = await ctx.executeAggregate!(objectName, { // Structured groupBy items ({field, dateGranularity}) pass through the @@ -95,6 +101,12 @@ export class ObjectQLStrategy implements AnalyticsStrategy { // on that zone's calendar days. A non-UTC zone makes the engine bucket // in-memory (uniform across drivers); UTC/unset keeps the DB fast path. timezone: query.timezone, + // ADR-0021 D-C (#3602): the second belt. `withReadScope` above is this + // layer's own scoping; handing the engine the context makes ITS middleware + // inject RLS too, so a future strategy that forgets `withReadScope` still + // cannot read across tenants. Without it the operation reaches the engine + // principal-less and plugin-security falls open — the #3597 shape. + context: ctx.context, }); // Remap short field names back to cube-qualified names @@ -145,12 +157,60 @@ export class ObjectQLStrategy implements AnalyticsStrategy { } const tableName = this.extractObjectName(cube); + + // ADR-0021 D-C (#3602) — the preview carries the SAME predicate the query + // actually runs with. Emitting only SELECT/GROUP BY made `/analytics/sql` + // read as an unscoped full-table scan while the real aggregate was scoped + // (#3601), so anyone debugging a "why is this row missing" was handed SQL + // that could not reproduce the result. Nothing leaked — this string is + // never executed — but a preview that contradicts execution is worse than + // no preview. + // + // Faithfulness cuts both ways: `execute()` does NOT apply + // `timeDimensions[].dateRange` on this path (only `where` reaches the + // engine), so neither does this. Rendering a BETWEEN here would invent a + // predicate the ObjectQL path never applies. That gap is real but separate + // — filed as #3650, not papered over here. + this.assertJoinedScopesEnforceable(cube, query, ctx); + + const params: unknown[] = []; + const whereParts: string[] = []; + for (const f of normalizeAnalyticsFilters(query)) { + const clause = this.buildFilterClauseSql( + this.resolveFieldName(cube, f.member, 'any'), + f.operator, + f.values, + params, + ); + if (clause) whereParts.push(clause); + } + // Read scope last, so it reads as the outermost constraint. Compiled by the + // same fail-closed compiler `NativeSQLStrategy` uses — it throws rather than + // drop a predicate, which is the correct posture even for a display string: + // silently omitting the scope is exactly the misleading output being fixed. + const scope = ctx.getReadScope?.(tableName); + if (scope != null) { + const { sql: scopeSql, params: scopeParams } = compileScopedFilterToSql(scope, tableName); + if (scopeSql) { + let i = 0; + // `compileScopedFilterToSql` emits `?`; renumber into this builder's $N. + const rendered = scopeSql.replace(/\?/g, () => { + params.push(scopeParams[i++]); + return `$${params.length}`; + }); + whereParts.push(`(${rendered})`); + } + } + let sql = `SELECT ${selectParts.join(', ')} FROM "${tableName}"`; + if (whereParts.length > 0) { + sql += ` WHERE ${whereParts.join(' AND ')}`; + } if (groupByParts.length > 0) { sql += ` GROUP BY ${groupByParts.join(', ')}`; } - return { sql, params: [] }; + return { sql, params }; } // ── Helpers ────────────────────────────────────────────────────── @@ -204,19 +264,11 @@ export class ObjectQLStrategy implements AnalyticsStrategy { private assertJoinedScopesEnforceable( cube: Cube, query: AnalyticsQuery, - groupBy: Array, - filter: Record, ctx: StrategyContext, ): void { if (typeof ctx.getReadScope !== 'function') return; - // Field names as they will reach the engine. A dotted name is a relationship - // traversal; its first segment is the join alias. - const referenced = [ - ...groupBy.map((g) => (typeof g === 'string' ? g : g.field)), - ...Object.keys(filter), - ...(query.measures ?? []).map((m) => this.resolveMeasureAggregation(cube, m).field), - ]; + const referenced = this.referencedFieldNames(cube, query); const offending = new Set(); for (const fieldName of referenced) { @@ -240,6 +292,71 @@ export class ObjectQLStrategy implements AnalyticsStrategy { ); } + /** + * Every field name the query puts in front of the engine — group-bys, filter + * members and measure sources — resolved exactly as the corresponding clause + * resolves it in `execute()`. A dotted name is a relationship traversal whose + * first segment is the join alias, which is what the scope guard keys off. + * + * Derived from `(cube, query)` alone so `execute()` and `generateSql()` run + * the guard over the identical field set; duplicates are fine, the caller + * dedupes by offending object. + */ + private referencedFieldNames(cube: Cube, query: AnalyticsQuery): string[] { + const names: string[] = []; + for (const dim of query.dimensions ?? []) { + names.push(this.resolveFieldName(cube, dim, 'dimension')); + } + for (const td of query.timeDimensions ?? []) { + names.push(this.resolveFieldName(cube, td.dimension, 'dimension')); + } + for (const f of normalizeAnalyticsFilters(query)) { + names.push(this.resolveFieldName(cube, f.member, 'any')); + } + for (const m of query.measures ?? []) { + names.push(this.resolveMeasureAggregation(cube, m).field); + } + return names; + } + + /** + * Render one normalized filter as a display SQL predicate for `generateSql`. + * + * Mirrors `NativeSQLStrategy.buildFilterClause`'s operator vocabulary so the + * two previews read alike, but binds through `coerceFilterValueForObjectQL`: + * the comparand shown is the one THIS path actually hands the engine (a real + * boolean, not SQL's 1/0). Returns null for an operator/value combination + * that carries no predicate, matching `execute()`, which drops it too. + */ + private buildFilterClauseSql( + col: string, + operator: string, + values: string[] | undefined, + params: unknown[], + ): string | null { + if (operator === 'set') return `${col} IS NOT NULL`; + if (operator === 'notSet') return `${col} IS NULL`; + + if (!values || values.length === 0) return null; + + if (operator === 'in' || operator === 'notIn') { + const placeholders = values + .map((v) => { params.push(coerceFilterValueForObjectQL(v)); return `$${params.length}`; }) + .join(', '); + return `${col} ${operator === 'in' ? 'IN' : 'NOT IN'} (${placeholders})`; + } + + if (operator === 'contains' || operator === 'notContains') { + params.push(`%${values[0]}%`); + return `${col} ${operator === 'contains' ? 'LIKE' : 'NOT LIKE'} $${params.length}`; + } + + const op = SCALAR_SQL_OPS[operator]; + if (!op) return null; + params.push(coerceFilterValueForObjectQL(values[0])); + return `${col} ${op} $${params.length}`; + } + /** * Resolve a member ref to a `{ sql, type? }` definition. * diff --git a/packages/spec/src/contracts/analytics-service.ts b/packages/spec/src/contracts/analytics-service.ts index 4a92559e1a..92ffb1c083 100644 --- a/packages/spec/src/contracts/analytics-service.ts +++ b/packages/spec/src/contracts/analytics-service.ts @@ -249,6 +249,31 @@ export interface StrategyContext { * fast path. */ timezone?: string; + /** + * ADR-0021 D-C (#3602) — the request's ExecutionContext, forwarded to + * `engine.aggregate` (`BaseEngineOptions.context`) so the ENGINE's own + * middleware chain scopes the read. + * + * This is the second belt, independent of {@link StrategyContext.getReadScope}. + * The two resolve scope through different paths — this one through the + * engine's middleware (`mergeReadContext` → RLS/sharing injection into + * `opCtx.ast.where`), `getReadScope` through `security.getReadFilter` at + * the analytics layer — and both are kept on purpose: + * + * - Without this, a strategy that forgets to call `getReadScope` runs + * completely unscoped. That is exactly how #3597 happened, and the + * context-less bridge is what let it through: with no principal on the + * operation, the security middleware's fall-open skipped its own RLS + * injection, so BOTH belts were off at once. + * - Without `getReadScope`, deployments that do not install + * plugin-security have no engine-side RLS at all — the analytics layer + * is their only belt. + * + * Optional because a bridge to a non-ObjectQL backend may have nowhere + * to put it; the `@objectstack/service-analytics` auto-bridge always + * forwards it. + */ + context?: ExecutionContext; }): Promise[]>; /** * Fallback in-memory analytics service (e.g. MemoryAnalyticsService from driver-memory). @@ -259,6 +284,24 @@ export interface StrategyContext { generateSql?(query: AnalyticsQuery): Promise<{ sql: string; params: unknown[] }>; }; + /** + * ADR-0021 D-C (#3602) — the ExecutionContext of the request being served + * (tenant, user, roles, transaction). + * + * Bound per call by the `IAnalyticsService` implementation from the + * `context` argument of `query()` / `generateSql()` / `queryDataset()`. + * Strategies forward it to `executeAggregate` so the engine's middleware + * chain can apply its own RLS — the depth-in-defense layer beneath + * {@link StrategyContext.getReadScope}, which only works if every strategy + * remembers to call it (#3597 is what happens when one does not). + * + * `undefined` means the caller supplied no context (in-memory/dev use, or a + * system-internal query). It does NOT mean "unrestricted": the analytics + * layer's own scoping still applies, and the engine treats a context-less + * operation per its own policy. + */ + context?: ExecutionContext; + /** * ADR-0021 D-C — per-object read scope (RLS + tenant isolation). * diff --git a/packages/verify/src/harness.ts b/packages/verify/src/harness.ts index 738b0ae73f..f3d04cc153 100644 --- a/packages/verify/src/harness.ts +++ b/packages/verify/src/harness.ts @@ -73,6 +73,19 @@ export interface BootOptions { * `member_default`. Defaults to a vanilla `new SecurityPlugin()`. */ security?: SecurityPlugin; + /** + * Override the AnalyticsServicePlugin instance. Defaults to a vanilla + * `new AnalyticsServicePlugin()`, which auto-bridges `getReadScope` to the + * `security` service. + * + * The reason to override is to prove ADR-0021 D-C's SECOND belt in isolation + * (#3602): pass `new AnalyticsServicePlugin({ getReadScope: () => undefined })` + * to switch the analytics-layer scoping OFF, leaving only the ExecutionContext + * the bridge hands `engine.aggregate` — i.e. the engine's own RLS middleware. + * A gate booted that way fails if the second belt ever stops carrying its own + * weight, which no assertion against the fully-wired stack can detect. + */ + analytics?: AnalyticsServicePlugin; /** * Boot multi-tenant: register enterprise `@objectstack/organizations` plugin BEFORE the * SecurityPlugin so the wildcard `organization_id` RLS policies that ship in @@ -156,7 +169,7 @@ export async function bootStack( // Service plugins `objectstack dev` auto-loads for an app of this shape. await kernel.use(new SettingsServicePlugin()); - await kernel.use(new AnalyticsServicePlugin()); + await kernel.use(opts.analytics ?? new AnalyticsServicePlugin()); // `autoDefaultOrganization: false` (ADR-0081 D1): the harness proves the two // ENDS of the isolation spectrum — pure single-tenant (no org, no scoping) // and, via `opts.multiTenant`, full multi-org (the enterprise plugin owns diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 7b501e83c3..13d1025287 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1604,6 +1604,9 @@ importers: '@objectstack/plugin-webhooks': specifier: workspace:* version: link:../../plugins/plugin-webhooks + '@objectstack/service-analytics': + specifier: workspace:* + version: link:../../services/service-analytics '@objectstack/service-messaging': specifier: workspace:* version: link:../../services/service-messaging