diff --git a/.changeset/analytics-execute-aggregate-execution-context.md b/.changeset/analytics-execute-aggregate-execution-context.md new file mode 100644 index 000000000..23d2ae558 --- /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` and `resolveDimensionLabels` each gain + an optional trailing `context`, beside the `scope` / `resolveScope` that + #3639 added — the same two-belt split as the aggregate path. +- `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). #3639 gave it the analytics-layer + belt (the referenced object's own read scope); it now also carries the + context, so the engine scopes the same read independently. +- `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 213b23c9a..782bb72dd 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 2236be69f..f67412dec 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 000000000..1587a5a36 --- /dev/null +++ b/packages/services/service-analytics/src/__tests__/execution-context-bridge.test.ts @@ -0,0 +1,334 @@ +// 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 }; +} + +/** + * #3639 gave this lookup the analytics-layer belt (the referenced object's own + * read scope) and covers that belt's own behaviour in `dimension-labels.test.ts` + * / `query-dataset.test.ts`. What is asserted here is the SECOND belt on the + * same hook, and — the part neither change covers alone — that the two arrive + * together on one engine call. Two PRs touching one hook is exactly how one of + * them quietly stops being wired. + */ +describe('fetchRecordLabels carries BOTH belts (#3602)', () => { + it('lands the read scope and the context on the same engine call', async () => { + const { seen } = await runLabelLookup({ scope: readScope, context: ctxA }); + + // [0] is the dataset aggregate; [1] is the id→label lookup — one row per + // record, real display names, so it is row-granular and needs both belts. + expect(seen[1].object).toBe('crm_account'); + expect(seen[1].where).toEqual({ + $and: [{ id: { $in: ['acc1'] } }, { organization_id: 'org_A' }], + }); + expect(seen[1].context).toBe(ctxA); + }); + + it('carries the context even when the target object has no read scope', async () => { + // The depth case for this hook: scope resolves to nothing, so the engine's + // own RLS is all that is left. Dropping the context here would make an + // unscoped-at-the-analytics-layer label read also unscoped at the engine. + const { seen } = await runLabelLookup({ scope: () => undefined, context: ctxA }); + + expect(seen[1].where).toEqual({ id: { $in: ['acc1'] } }); + 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'); + }); +}); + +// ── 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('renders nothing for a cross-object query `execute()` would reject', async () => { + // The rendering must not describe a query that cannot run. #3654 made the + // cross-object guard unconditional in `execute()`; running the SAME guard + // here is what keeps the two in step — a rendering that outlives its + // execution path is the mismatch this item exists to remove. + 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 group or aggregate across a relationship .*"account\.region"/); + }); +}); diff --git a/packages/services/service-analytics/src/analytics-service.ts b/packages/services/service-analytics/src/analytics-service.ts index 04064de87..a76cd9fe6 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, }; } @@ -614,13 +629,16 @@ export class AnalyticsService implements IAnalyticsService { ? (targetObject: string) => provider(targetObject, context) : undefined; try { - await resolveDimensionLabels(dataset.object, dims, result.rows, this.labelResolver, resolveScope); + // `context` rides alongside `resolveScope` — the label lookup's SECOND + // belt. `resolveScope` is this layer's own predicate; the context lets + // the engine's middleware scope the same per-record read itself. + await resolveDimensionLabels(dataset.object, dims, result.rows, this.labelResolver, resolveScope, 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, resolveScope); + await resolveDimensionLabels(dataset.object, subset, total.rows, this.labelResolver, resolveScope, context); } } } catch (e) { diff --git a/packages/services/service-analytics/src/dimension-labels.ts b/packages/services/service-analytics/src/dimension-labels.ts index 684098973..bb807d260 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; @@ -48,11 +50,19 @@ export interface DimensionLabelDeps { * object is more restricted than the base object whose rows carry the id. * `undefined` means "no scope for this object" (global table / unrestricted * caller) — the same contract as the read-scope provider. + * + * `context` is the request's ExecutionContext — the SECOND belt on the same + * read (#3602). `scope` is the analytics layer's own predicate; forwarding the + * context lets the ENGINE's middleware chain scope this per-record read + * itself, so it stays scoped even if a caller ever reaches this hook without + * a resolved `scope`. Implementations bridging to an ObjectQL engine MUST + * forward it; a bridge with nowhere to put it may ignore it. */ fetchRecordLabels( targetObject: string, ids: unknown[], scope?: Record, + context?: ExecutionContext, ): Promise>; } @@ -129,6 +139,10 @@ export function formatDateBucket(value: unknown, granularity?: DateGranularity | * throws, that dimension's labels are SKIPPED (fail-closed — the raw id renders * instead) rather than fetched unscoped. Omit when no read-scope provider is * configured (labels then fetch unscoped, as before — no security in play). + * @param context - the request's ExecutionContext, forwarded to + * {@link DimensionLabelDeps.fetchRecordLabels} so the engine's own middleware + * scopes the per-record label read too — the second belt beside `resolveScope` + * (#3602) */ export async function resolveDimensionLabels( baseObject: string, @@ -136,6 +150,7 @@ export async function resolveDimensionLabels( rows: Record[], deps: DimensionLabelDeps, resolveScope?: LabelScopeResolver, + context?: ExecutionContext, ): Promise { if (!rows.length || !dims.length) return; const fields = deps.getObjectFields(baseObject); @@ -191,7 +206,7 @@ export async function resolveDimensionLabels( continue; } } - const labelById = await deps.fetchRecordLabels(meta.reference, ids, scope ?? undefined); + const labelById = await deps.fetchRecordLabels(meta.reference, ids, scope ?? undefined, 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 be2cdd08d..caca6d39f 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,7 +371,7 @@ export class AnalyticsServicePlugin implements Plugin { }; const labelResolver: DimensionLabelDeps = { getObjectFields: (objectName) => dataEngine()?.getObject?.(objectName)?.fields, - fetchRecordLabels: async (targetObject, ids, scope) => { + fetchRecordLabels: async (targetObject, ids, scope, context) => { const map = new Map(); const displayField = pickDisplayField(dataEngine()?.getObject?.(targetObject)?.fields); if (!displayField || !executeAggregate || ids.length === 0) return map; @@ -371,6 +389,10 @@ export class AnalyticsServicePlugin implements Plugin { groupBy: ['id', displayField], aggregations: [{ field: 'id', method: 'count', alias: '_c' }], filter, + // #3602 second belt — `scope` above is the analytics layer's own + // predicate on this per-record read; the context makes the engine's + // middleware scope it as well. + 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 e6e99b9f3..027a52f67 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 @@ -84,7 +90,7 @@ export class ObjectQLStrategy implements AnalyticsStrategy { // runs BEFORE scope injection and subsumes the #3597 joined-scope concern: // a rejected query never loads the joined object, so nothing is left // unscoped. Independent of read scope, so it fires with security off too. - this.assertNoCrossObjectReferences(cube, query, groupBy, filter); + this.assertNoCrossObjectReferences(cube, query); // ADR-0021 D-C — the base object's read scope (tenant + RLS) MUST be ANDed // in before the query leaves the strategy (#3597). @@ -102,6 +108,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 @@ -206,32 +218,57 @@ export class ObjectQLStrategy implements AnalyticsStrategy { } const tableName = this.extractObjectName(cube); - let sql = `SELECT ${selectParts.join(', ')} FROM "${tableName}"`; - const SQL_OPERATORS: Record = { - equals: '=', notEquals: '<>', gt: '>', gte: '>=', lt: '<', lte: '<=', - contains: 'LIKE', in: 'IN', notIn: 'NOT IN', set: 'IS NOT NULL', notSet: 'IS NULL', - }; + // ADR-0021 D-C (#3602) — render the READ SCOPE too, not just the caller's + // own filters (#3652 added those). Without it this string still reads as an + // unscoped table scan while the real aggregate is scoped (#3601), so anyone + // debugging a "why is this row missing" gets SQL that cannot reproduce the + // result. Nothing leaks — the string is never executed, and scope VALUES + // stay in `params`, which `execute()`'s echo discards — but a rendering + // that contradicts execution is worse than no rendering. + // + // The cross-object guard runs here for the same reason: this must not + // render SQL for a query `execute()` would reject outright (#3654). + // + // 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.assertNoCrossObjectReferences(cube, query); + const whereParts: string[] = []; for (const f of normalizeAnalyticsFilters(query)) { - const col = this.resolveFieldName(cube, f.member, 'any'); - const op = SQL_OPERATORS[f.operator] ?? '='; - if (f.operator === 'set' || f.operator === 'notSet') { - whereParts.push(`${col} ${op}`); - continue; - } - const values = f.values ?? []; - if (values.length === 0) continue; - if (f.operator === 'in' || f.operator === 'notIn') { - const placeholders = values.map((v) => { params.push(v); return `$${params.length}`; }); - whereParts.push(`${col} ${op} (${placeholders.join(', ')})`); - } else { - params.push(values[0]); - whereParts.push(`${col} ${op} $${params.length}`); + 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})`); } } - if (whereParts.length > 0) sql += ` WHERE ${whereParts.join(' AND ')}`; + 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(', ')}`; } @@ -305,17 +342,11 @@ export class ObjectQLStrategy implements AnalyticsStrategy { private assertNoCrossObjectReferences( cube: Cube, query: AnalyticsQuery, - groupBy: Array, - filter: Record, ): void { - // Field names as they will reach the engine. A dotted name whose first - // segment names a DIFFERENT object is a relationship traversal the engine - // cannot perform in an aggregate. - const referenced = [ - ...groupBy.map((g) => (typeof g === 'string' ? g : g.field)), - ...Object.keys(filter), - ...(query.measures ?? []).map((m) => this.resolveMeasureAggregation(cube, m).field), - ]; + // Derived from `(cube, query)` alone rather than from `execute()`'s built + // `groupBy`/`filter`, so `generateSql()` runs the IDENTICAL check (#3602): + // the rendered string must never describe a query `execute()` would reject. + const referenced = this.referencedFieldNames(cube, query); const baseObject = this.extractObjectName(cube); const offending = new Set(); @@ -341,6 +372,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 64c9880c1..2deb994ac 100644 --- a/packages/spec/src/contracts/analytics-service.ts +++ b/packages/spec/src/contracts/analytics-service.ts @@ -277,6 +277,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). @@ -287,6 +312,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 738b0ae73..f3d04cc15 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 7b501e83c..13d102528 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